-
Notifications
You must be signed in to change notification settings - Fork 111
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor reCAPTCHA validator as form model #10540
Conversation
changelog: Internal, reCAPTCHA, Refactor reCAPTCHA validator as form model
@@ -1,14 +1,21 @@ | |||
# frozen_string_literal: true | |||
|
|||
class RecaptchaValidator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we rename to Form
and move to app/forms
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought about that. We probably should, wasn't sure if the amount of noise it might add to this pull request might be worth doing it incrementally (i.e. separate pull request), especially since there's subclasses that might also need to be moved.
I did find that we have a couple other examples of form validators in services
(e.g. SamlRequestValidator
), though I think those probably ought to be moved as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the app/validator
objects don't have the same interface as forms, but yes SamlRequestValidator
seems to implement the Form
interface
I will plan to move/rename this to |
recaptcha_validator.submit(recaptcha_token) | ||
errors.merge!(recaptcha_validator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does errors.merge!
work with an entire validator? I'd expect more like
recaptcha_validator.submit(recaptcha_token) | |
errors.merge!(recaptcha_validator) | |
recaptcha_validator.submit(recaptcha_token) | |
errors.merge!(recaptcha_validator.errors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I initially expected as well, but it error'd because ActiveModel::Errors
doesn't have an errors
method; it's expecting the model, not the errors.
* Refactor reCAPTCHA validator as form model changelog: Internal, reCAPTCHA, Refactor reCAPTCHA validator as form model * Update spec stubs * Update mock validator specs
馃帿 Ticket
Supports LG-12873
馃洜 Summary of changes
Refactors reCAPTCHA validator classes to behave and validate as a form class.
See previous discussion: #10522 (comment)
馃摐 Testing Plan
Validate that there are no regressions in the expected behavior of reCAPTCHA phone validation.
Configure reCAPTCHA score threshold in
config/application.yml
+610491570006