-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Orbital: accept order_id option for capture [#2247] #2248
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
base: master
Are you sure you want to change the base?
Orbital: accept order_id option for capture [#2247] #2248
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.
In the linked issue you mention
"capture" test cases were rejected because I did not provide the OrderID.
Why is it that the order_id from the authorization did not work in your case? Line 584 (the top of this method) attempts to extract it from the authorization, which is what would be typically expected in an auth and capture flow. Is that behavior buggy? It seems weird to me that you'd have to pass this in separately when trying to capture an auth.
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.
Apologies for not being more detailed in the issue.
To my knowledge, that behavior is not buggy. However, it only works if you performed the auth using Active Merchant. I'm using Chase's Hosted Pay Form, which performs the auth in an iframe and provides the authorization token. I send the token to the server and capture there, using Active Merchant.
This can be resolved by combining the token and order ID into a string, like Active Merchant does. That's non obvious, though. Given:
- Chase requires both the authorization token and the order ID to be certified for production;
- There are two common ways to perform an auth outside of Active Merchant (Hosted Pay Form and Hosted Pay Page);
- Combining into a string breaks encapsulation by requiring my app to know about Active Merchant's internals (
authorization_string, which does the combining, is a private method); and - This change preserves existing behavior
It seems reasonable to make this a first-class parameter, rather than tacking it on to the authorization.
|
@jasonwebster Is this something you're still interested in landing? |
|
Hi! I'm interested in getting this PR merged, if that's still on the table, and I'd be happy to help if I can. I'm working with the iframe auth/server capture setup Chris described above, and it would be nice to be able to pass in the order id. |
|
Awesome, I'll look @albaer . I've been going through and cleaning up/landing a lot of PRs, but it's a slow burn. I'll try to get you feedback within the next week or so. |
|
@albaer What would you need to update this and help land it? |
|
I'm not sure. If adding this option is good with you, I can just rebase it, make sure everything is all set with contribution guidelines and documentation, and then let you know. How does that sound? |
843eb53 to
2c3e095
Compare
|
@bpollack I was just talking to some coworkers, and they said they had this open. It's been rebased and should be ready to go. Let me know if there are any changes you would need to it. |
This commit resolves the issue identified in #2247.