-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Adding Oauth Response for access tokens #4869
Conversation
00612dc
to
0fbf623
Compare
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.
Can you share a Test Summary? I'm unable to get passing remote tests for these gateways on this branch so I wanted to see if it's just something wrong in my environment.
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.
Did some scattershot testing with other gateways that raise ActiveMerchant::OAuthResponseError
and that looks good. Just had a general question.
lib/active_merchant/errors.rb
Outdated
@@ -23,6 +23,12 @@ def initialize(response, message = nil) | |||
end | |||
|
|||
def to_s | |||
if response.kind_of?(String) | |||
return response if response.start_with?('Failed with') |
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.
As-is, an arbitrary String
response that doesn't start with Failed with
would say ActiveMerchant::OAuthResponseError: Failed with
. Is that desirable?
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.
Yes exactly. I add this check because some gateways like Alelo check access_token method in commit
so it would return ActiveMerchant::OAuthResponseError: Failed with Failed with
.
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.
But wouldn't returning the the response
string in that case be more informative?
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.
In the case I described the OAuthResponseError or maybe I'm not understanding what you trying to say
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 mean that error strings like An error happened
would return ActiveMerchant::OAuthResponseError: Failed with
and not fall to the general case ActiveMerchant::OAuthResponseError: Failed with An error happened
. So I'm wondering if the if response.start_with?('Failed with')
check is necessary, when it could read as ActiveMerchant::OAuthResponseError: An error happened
without that check.
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.
Oh, if response is a string then it wouldn't be a ActiveMerchant::OAuthResponseError
. If response was ActiveMerchant::OAuthResponseError
the it would fall into the second statement, elsif response.respond_to?(:message)
.
7be8808
to
2f2b2be
Compare
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.
Looks good 👍🏾
2f2b2be
to
69d4ffc
Compare
Add new OAuth Response error handling to PayTrace, Quickbooks, Simetrik, Alelo, CheckoutV2 and Airwallex.
69d4ffc
to
3931adf
Compare
Add new OAuth Response error handling to PayTrace, Quickbooks, Simetrik, Alelo, CheckoutV2 and Airwallex.