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

Checkout V2: Correct success criteria #3205

Closed

Conversation

curiousepic
Copy link
Contributor

@curiousepic curiousepic commented May 1, 2019

Also corrects address field value detection.

Unit:
22 tests, 99 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Unfortunately we don't currently have working test credentials for remote tests

@curiousepic curiousepic requested a review from a team May 1, 2019 15:42
@curiousepic curiousepic force-pushed the checkout_success_fix branch 2 times, most recently from eaf4b21 to 6241e8b Compare May 1, 2019 15:49
Also corrects address field value detection.

Unit:
22 tests, 99 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
@curiousepic curiousepic force-pushed the checkout_success_fix branch from 6241e8b to db755f7 Compare May 1, 2019 15:50
Copy link

@nfarve nfarve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

@@ -215,7 +215,7 @@ def parse(body)
end

def success_from(response)
response['response_summary'] == 'Approved' || response.key?('action_id')
response['response_summary'] == 'Approved' || !response.key?('response_summary') && response.key?('action_id')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use () here to make it clear the precedence of these conditions vs. having to rely solely on our knowledge of Ruby's rules around || and &&?

(obviously something to change once we've resolved any immediate issues)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's just ruby. I think the general order of precedence for logic operators is ~, &, ||. But () would make it more clear.

whitby3001 pushed a commit to whitby3001/active_merchant that referenced this pull request Sep 3, 2019
Also corrects address field value detection.

Closes activemerchant#3205

Unit:
22 tests, 99 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
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.

3 participants