refugerestrooms icon indicating copy to clipboard operation
refugerestrooms copied to clipboard

Enhancement/Recaptcha

Open brunoocasali opened this issue 4 years ago • 4 comments

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

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

brunoocasali avatar Oct 14 '20 02:10 brunoocasali

@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 avatar Oct 17 '20 02:10 DeeDeeG

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

brunoocasali avatar Oct 29 '20 13:10 brunoocasali

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.

DeeDeeG avatar Oct 29 '20 15:10 DeeDeeG

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 :tada:). And I could help review other pull requests too.

brunoocasali avatar Oct 29 '20 20:10 brunoocasali