-
Notifications
You must be signed in to change notification settings - Fork 440
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
Feature: Adds api response logging. #401
base: master
Are you sure you want to change the base?
Conversation
Pass in a logger instance to the configuration and the verification response gets logged.
lib/recaptcha.rb
Outdated
verify_via_api_call_enterprise(response, options) | ||
else | ||
verify_via_api_call_free(response, options) | ||
end | ||
|
||
logger(verification.last) |
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.
not a fan of the indirection and duplicated logic
prefer something like:
Recaptcha.configuration.logger&.info success: success, reply: reply, event: "recaptcha-response"
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.
also would help to pull the with_reply
logic into this method so we always have the reply and can log it
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.
I moved the method to use reply
. It does some tagging too.
not sure if that is even needed, users can request |
Oh, I might give that a go... thanks! |
I was finding it hard to debug our implementation of this. We are using a logger but when exceptions are raised, it gets a bit fiddly. I just wanted the gem to log responses from the API and I could just forget about it. |
does #402 work ? |
This adds a logger configuration option. This is helpful for debugging mysterious failures.
It requires the logger respond to :info.
Pre-Merge Checklist
sorry, I haven't written this... should I put it in "next"?