Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement/Recaptcha #648

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

brunoocasali
Copy link
Contributor

@brunoocasali brunoocasali commented Oct 14, 2020

Context

Helpers are hard do test, and mainly used by views, but this recaptcha helper was not used directly on views so, there was no direct reason to keep it in that way!

Summary of Changes

  • Created a command class based on RecaptchaHelper
    • Add test coverage to some useful cases (I'm open to suggestions about improving them)
    • Add "testability" by receiving params from public interface of class
    • Add read_timeout with a pretty arbitrary number to prevent handling a request for too much (blocks by default)
  • Replace usages with new code.

Checklist

  • Added Unit Tests
  • CI Passes
  • Deploys to Heroku on test Correctly (Maintainers will handle)

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 17, 2020

@mi-wood if you get a chance to review this PR, that would be appreciated.

It re-factors some Ruby code (having to do with ReCaptcha) so that testing it is easier... and adds some tests for the ReCaptcha code.

Seems like a good idea to make sure everything keeps working over time. It's a bit beyond my personal skill level with Ruby at the moment. Though I can try to review it myself at some point anyhow.

@DeeDeeG DeeDeeG added Hacktoberfest These are issues or pull requests related to Hacktoberfest (https://hacktoberfest.digitalocean.com/) Ready for Review labels Oct 17, 2020
@brunoocasali
Copy link
Contributor Author

@DeeDeeG there are something I could do to help get this merged?

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 29, 2020

Hi @brunoocasali,

We have some maintainers who are good with Ruby -- mainly @tkwidmer and @mi-wood. But I have trouble getting ahold of them at times.

I personally don't understand Ruby well enough to want to review such subtle changes. That said, if I've given them a significant amount of time to review this, I may go in and review it myself (largely by doing manual functional checks) out of a desire to have the test you added.

I will "approve" this, like one other pull request I did that for that I felt unqualified to review, as it seems clear to me this is a good-faith contribution to improve the app. That makes it count toward Hacktoberfest. Beyond that, I want to leave more time for either of @tkwidmer or @mi-wood to potentially review it.

Thanks for this and your other contributions.

@brunoocasali
Copy link
Contributor Author

Thanks for the message @DeeDeeG

Beyond the hacktoberfest thing (merge some PR's to get a gift) I search by nice projects I can improve and I think I found one!
And I really appreciate seeing my contributions in production! :D

Actually even after hacktoberfest I want to keep contributing to this project when I have time to it (I'm on a vacation this month 🎉).
And I could help review other pull requests too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
continuous integration developer-oriented Hacktoberfest These are issues or pull requests related to Hacktoberfest (https://hacktoberfest.digitalocean.com/) Ready for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants