Add Reply class to handle Recaptcha responses#466
Conversation
|
yeah cleans up the logic nicely |
| return raw_reply['hostname'] unless enterprise? | ||
|
|
||
| token_properties&.dig('hostname') |
There was a problem hiding this comment.
might be cleaner as if enterprise? x else y end
| success = reply.success? && | ||
| hostname_valid?(reply.hostname, options[:hostname]) && | ||
| action_valid?(reply.action, options[:action]) && | ||
| score_above_threshold?(reply.score, options[:minimum_score]) && | ||
| score_below_threshold?(reply.score, options[:maximum_score]) |
There was a problem hiding this comment.
can be dried up since they are the same now ?
There was a problem hiding this comment.
yes, I can put that validations in the class
There was a problem hiding this comment.
was more thinking as a reused method in recaptcha.rb but in the class would also work
There was a problem hiding this comment.
i moved the validation to the success? method, then I added the success method to allow access to the raw value of that key. What do you think?
| def free? | ||
| !enterprise? | ||
| end |
There was a problem hiding this comment.
would actually avoid having 2 different methods, so the code is easier to reason about since it has less concepts to keep in mind
There was a problem hiding this comment.
so remove the free? method?
There was a problem hiding this comment.
I would remove it to simplify
| @raw_reply['tokenProperties'] if enterprise? | ||
| end | ||
|
|
||
| def success?(options = {}) |
There was a problem hiding this comment.
having a ? return a non-bool is a bit sus
| def success?(options = {}) | |
| def success(options = {}) |
There was a problem hiding this comment.
right, in that case we can keep success? like a boolean for test success and add validate_success method. What do you think?
There was a problem hiding this comment.
maybe keep success? here and make the caller do the if options[:with_reply] == true dance
| def free? | ||
| !enterprise? | ||
| end |
There was a problem hiding this comment.
I would remove it to simplify
|
just some rubocop, rest seems fine 🎉 |
Co-authored-by: Michael Grosser <michael@grosser.it>
it seems it's because I forgot to add |
|
I'll do the polish :) |
|
👁️ #467 |
|
A bit of both, initially I was just going to report the error message
problem with the recaptcha enterprise and in the process came up with a
solution
El sáb, 19 jul 2025 a la(s) 11:19 p.m., Michael Grosser (
***@***.***) escribió:
… *grosser* left a comment (ambethia/recaptcha#466)
<#466 (comment)>
I'll do the polish :)
... do you want this released or this was just for fun refactor ?
—
Reply to this email directly, view it on GitHub
<#466 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACH4TUNKGPYHGPS35UM6RUT3JMKD7AVCNFSM6AAAAACB3GNTRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAOJTGE4DSMRRHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
5.20.0 |
2 participants
Add
Recaptcha::Replyclass to standardize and simplify response handling for both free and enterprise versions. It also defines the[]=method for backward compatibility and prevents theRecaptcha failure after API call. API reply: #{@_recaptcha_reply}.error message when using recaptcha enterprise.