RefugeRestrooms / refugerestrooms

REFUGE restrooms indexes and maps safe restroom locations for trans, intersex, and gender nonconforming individuals.

http://www.refugerestrooms.org

GNU Affero General Public License v3.0

888 stars 262 forks source link

Enhancement/Recaptcha #648

Open brunoocasali opened 3 years ago

brunoocasali commented 3 years ago

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)

DeeDeeG commented 3 years ago

@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.

brunoocasali commented 3 years ago

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

DeeDeeG commented 3 years ago

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 commented 3 years ago

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.