-
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
Authnet: accept.js support #3224
base: master
Are you sure you want to change the base?
Conversation
629ba3d
to
495368b
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.
Thanks for the PR -- it looks pretty good to me :D Hoping to get at least one more set of eyes on it for feedback.
Hi @dingels35, still looking at this, but my first thought is whether the |
@curiousepic - Good question. I definitely wanted this to be generic enough to handle other (possibly future) payment types. The way I have things set now, this could be used for apple pay scenarios. It can also be used for Visa Checkout and Android Pay (documentation links below). So I think having just one OpaqueData type that can be used for all 4 scenarios (accept.js, apple, android, visapay) is a good way to build support for multiple current, and possible future, authorize.net payment options. https://developer.authorize.net/api/reference/index.html#visa-checkout |
495368b
to
66b0ac4
Compare
@dingels35 I was actually thinking more about applications beyond Authnet, rather than other Authnet token types. If it's tied to Authnet, let's maybe make that explicit in the name. |
@curiousepic - I like your thoughts. We could definitely add token classes for other payment types - AndroidPayPaymentToken, VisaCheckoutPaymentToken, and AcceptJsPaymentToken. Doing this would mostly require that each merchant consider each class separately. Another option would be to allow the existing PaymentToken act on its own. I think I could use the same code listed in my PR summary to use a PaymentToken instead of an OpaqueDataPaymentToken. I'd likely just need to modify the PaymentToken class to something like this: class PaymentToken
attr_reader :payment_data, :metadata
def initialize(payment_data, options = {})
@payment_data = payment_data
@metadata = options.with_indifferent_access
end
def type
'payment_token'
end
end Exposing Thoughts on this approach? If you're not keen on it, I can also just rename that class to |
@dingels35 Hi Cory, these are some interesting approaches. I think we should probably keep things slim here since we're approaching scope bloat, but I'd like to get some more perspectives for how to proceed from my colleagues and get back to you. @activemerchant/shopify-payments-devs may be interested as well. |
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.
Things look very nice with the code and tests. I especially like the remote tests and the special gateway class to simulate the necessary front-end flow. Well done!
With respect to modeling the specialized token needed to support the Accept.js
token type, I believe we'd be better off scoping these changes to only within the Authorize.net-relevant code, rather than expanding any top-level payment classes. We should be especially cautious with modifying the PaymentToken
class directly right now.
There is precedence for the use of a nested gateway-specific token class within the StripeGateway
class: StripeGateway::StripePaymentToken
. Could we do something similar for the opaque data required by AuthorizeNetGateway
? Perhaps AuthorizeNetGateway::OpaqueDataToken
? Then we can move forward with minimal risk to existing code. We can always lift the class up to the top-level if and when we discover that this specific format of opaque data might be applicable to other gateways.
If that would work, the modifications to update the tests should be minimal, and it will be easier for us to reason about how it will interact with our own existing code when we go to integrate this patch into our internal systems.
@bayprogrammer Nice, I like that path as well. |
what was wrong with #2422 ? |
@taf2 I'm not aware that anything was necessarily wrong with it, but I'm guessing it simply didn't get picked up on our radar at the time. We've been making a renewed push at becoming more proactive in responding to issues and PRs. @dingels35 How are things going on your side? Were my recommended changes something you can do? |
@bayprogrammer - Thanks for checking back in. Your comments are good - I like that approach. I had some time off earlier in June and am heading out for a long weekend again. But I'll get this updated for you to look at in the next 2 weeks. |
@dingels35 Awesome, thanks so much. Have a great long weekend! 😄 |
Hi all 👋 Would love to get this merged so we can all drop our Accept.js monkeypatches. Any updates on it? |
@dingels35 @dfltr I apologize I didn't see that this PR had been updated since I last looked at it. It's back on my radar and I will try to re-review and hopefully we can get this one merged in! |
Hey guys, checking back in on this one: I'd love to use Accept.js via ActiveMerchant! :) |
My patch still works... |
@bayprogrammer or @curiousepic 👋 I'm on @dingels35 's team, revisiting this issue. We'd love to drop our fork of ActiveMerchant if we can get this (or #4406) across the line. Last we saw I think we were waiting on comments from @bayprogrammer. Is it realistic to revive this PR at this point, or how can we proceed? |
@ChrisNelsonBHG apologies, I am afraid I wasn't able to get back to this while I was still at Spreedly. I no longer work for Spreedly and I no longer have commit access to this project (nor would I remember enough about this accept.js business to be of much help any longer 😅). |
Marking this "of interest" before a cleanup of stale PRs |
same: Marking this "of interest" before a cleanup of stale PRs |
Pretty sure we forked active_merchant long ago for this specific feature in authnet - seems pretty important feature... |
Authorize.net provides a way to capture credit cards without credit card data (number, expiration, etc) ever passing through the merchant's server. Using their accept.js client library, CC info is submitted directly to their server, which responds with a payment nonce. This nonce can then be sent safely though the merchant's server and used in authorize and purchase transactions. Full documentation here: https://developer.authorize.net/api/reference/features/acceptjs.html
This PR adds an OpaqueDataPaymentToken to handle authnet's payment nonce.
Given the following output from the API called made by the accept.js library:
Authorization can be submitted like this:
Resolves #3187