Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

rimian
Copy link

@rimian rimian commented Feb 11, 2022

This adds a logger configuration option. This is helpful for debugging mysterious failures.

It requires the logger respond to :info.

    Recaptcha.configure do |config|
      config.site_key              = '0000000000000000000000000000000000000000'
      config.secret_key            = 'XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX'
      config.enterprise_api_key    = 'ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ'
      config.logger = Logger.new(STDOUT) # or Rails.logger
    end

Pre-Merge Checklist

sorry, I haven't written this... should I put it in "next"?

  • CHANGELOG.md updated with short summary

Rimian Perkins added 2 commits February 11, 2022 11:56
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)
Copy link
Collaborator

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"

Copy link
Collaborator

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

Copy link
Author

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.

@grosser
Copy link
Collaborator

grosser commented Feb 11, 2022

not sure if that is even needed, users can request with_reply: true and then do their own logging ?

@rimian
Copy link
Author

rimian commented Feb 11, 2022

not sure if that is even needed, users can request with_reply: true and then do their own logging ?

Oh, I might give that a go... thanks!

@rimian
Copy link
Author

rimian commented Feb 11, 2022

not sure if that is even needed, users can request with_reply: true and then do their own logging ?

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.

@grosser
Copy link
Collaborator

grosser commented Feb 17, 2022

does #402 work ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants