-
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 #4851
Conversation
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.
Would you be able to make some unit tests for this change?
7f341bc
to
9e04616
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.
Great test coverage! I think this looks good just one question about the options[:access_token]
. Was that done to make testing easier?
@@ -1,3 +1,4 @@ | |||
require 'pry' |
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.
Pesky pry statement!
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.
Good catch!
Yes, to add unit testing. Without it I can't skip the access token method for all the other units tests and it causes them to fail. It couldn't find a better way to do it, the way it was done before was calling the method within the unit test file, https://github.com/activemerchant/active_merchant/pull/4851/files#diff-f3fb17a2b3e2cdf9d21bc45cc072a7f6f740a76e2b362b2168892dc94a1904b0L3 |
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.
LGTM! I'm fine with the options[:access_token]
to facilitate testing. Just remember to remove the pry statement!
9e04616
to
20237f8
Compare
Add new OAuth Response error handling to PayTrace, Quickbooks, Simetrik, Alelo, CheckoutV2 and Airwallex.
20237f8
to
00fd53f
Compare
Add new OAuth Response error handling to PayTrace, Quickbooks, Simetrik, Alelo, CheckoutV2 and Airwallex.