Add new Stripe payment method compatible with the new Stripe Payment Intents API #4672

luisramos0 · 2020-01-13T19:09:36Z

What? Why?

Relates to #4180
Closes #4686 and #4173

The decision to duplicate the existing Stripe payment method has the objective of making the migration as smooth as possible and also of making it possible for us, after we migrate all users, to delete the original stripe payment method (and its code), and keep only the new one (that uses the new SCA compatible APIs).
Although this new API was made by stripe for European SCA it does work worldwide, we don't need to keep both integrations 👍

This PR is not introducing any new auth processes yet. It should just work like the existing stripe payment method.

I have added entries to rubocop_manual_todo for the remaining rubocop issues. Code climate is not picking the exceptions list from rubocop_manual_todo...

In terms of tests we are not adding any feature tests here as we plan to get something new with #4639

What should we test?

We need to verify that the existing Stripe payment method works.
We need to verify the new payment method, it should just work like the existing stripe payment method.
Processes we need to test:

make a payment without saving card
make a payment saving card
make a payment using previously saved card
add a card on account section and use it to checkout
delete saved card
capture the payment in the admin payments section of an order
as a user authorize a hub to use your card and verify the subscription payments work with that card

The following details are valid before #4719, if we are testing 4719, all cases above should work.

As discussed in comments below: case 4 above is not working yet for this payment method. But case 7, subscriptions, is working. Because case 4 doesnt work (if you add a card in the account section and use it in a subscription with this new payment methods, the payments will fail). To add a card that works for a subscription with this new payment method you need to checkout an order with this new payment method with the save card option enabled. After that go to account to authorize the card you have used to checkout.

Release notes

Changelog Category: Added
Added new payment method that integrates with the new Stripe Payment Intents API that is compatible with SCA. This is a initial preparation for us to implement the more complex payment authentication flows that banks will start to enforce in Europe soon.

Matt-Yorkley · 2020-01-15T09:44:33Z

Nice work! Looks like some of the stubbed requests in the new specs need adjusting a bit...

luisramos0 · 2020-01-15T10:57:03Z

yes, thanks @Matt-Yorkley I am finishing that spec today. I wanted to get some early feedback 👍

mkllnk

Fabulous. 🚀

lib/stripe/profile_storer.rb

luisramos0 · 2020-01-16T14:30:03Z

ok, I rebased and added the two last commits: make one method better looking and add some missing translations.

There are two test scenarios that are not working yet. 4 and 7.

Adding a card in the account section

the problem here is that this customer form does not have any association with a payment method, so we dont know if this will be used for a stripe payment method with SCA or the legacy stripe api. Currently the form is adding the card to the legacy stripe api and these cards will only work with payment methods that use the legacy stripe api (non sca). I am not sure how we should proceed here because while the new sca payment method is not completed and all user data migrated, we will still want the old stripe payment methods to work.

Subscriptions

Without point 4 above, cards that work with the new sca method can only be added through checking out with the save card option enabled.
(EDITED) otherwise I think subscriptions are working with this new payment method 🎉

I think we should ship this PR as is if no one disagress. the new stripe sca payment method will be available but only works in limited list of use cases. We can then improve it until we can replace the old one.

lin-d-hop · 2020-01-16T14:41:25Z

Thanks @luisramos0 I agree with your approach above.

Do you foresee any issues when we come to merge the old API user data with the new API user data. As the above approach means we'll have split user data including potentially the same user across both APIs. I guess this is totally handled by Stripe?

RachL · 2020-01-16T14:48:44Z

Something I'm not sure to understand: if we upgrade the API, doesn't it means that we should disable the current form that saves card in the legacy API?
I mean, migration is always a mess, so the less card to migrate the better?

luisramos0 · 2020-01-16T16:42:29Z

@RachL this is not related to upgrading to the latest api version.
this is related to using a different api: charges api (non sca compliant) vs payment intents api (sca compliant).
All stripe api versions include the charges api but we want to upgrade so that we get the recent versions of the payment intents api with all the sca features.

We can have both payment methods work at the same time, we just need to see what to do with this customer form.

if you agree we can just go forward with this PR as is. I can start investigate the easiest approach afterwards:

maybe I can make the add card form add to both apis
maybe I can make the cards added through the old api (charges api) work with the new api (payment intents api) - this would be great because we wouldnt need to migrate the user cards
other approach

luisramos0 · 2020-01-16T16:45:04Z

@lin-d-hop answering your question: I still have to see if I can use the existing data in the new api. I havent managed so far but I think it will be possible.

Conclusion: lets go forward with this PR that adds the new payment method (Stripe SCA) and works with the new payment intents api. This way we can already validate it in staging and live. And meanwhile we can work on making this payment method ready for the full process.

luisramos0 · 2020-01-16T18:53:15Z

ok, it looks like subscriptions are working :-)

This is ready to be reviewed and tested 👍
@mkllnk I have added the 4 last commits since your review.
@Matt-Yorkley can you please review?
Thanks!!!!

mkllnk

This is really exciting. Two questions though:

Do we need to place a warning there or mark SCA as experimental so that people don't start using SCA without it working 100%? I'm imagining a new enterprise setting up, not talking to us and then using SCA and adding cards as admin. What would happen then?
I guess, with the new legislation, if an admin adds a card from the backend, it could happen that the system decides that it needs further authorisation and emails or texts the owner of the card. But it's still legal to add a card to the system without being the owner? I could imagine that that's why a card added with the old API is not usable in the intents API (not sufficiently authorised).

luisramos0 · 2020-01-16T23:28:45Z

I dont think this will be a problem because the new payment method is basically working. The only case not working is for users to add a card in the account section. Frontoffice. And they will see it doesnt work because the payments will fail. Also I think we will be ready very soon and we will probably rename the existing stripe one to something like "Stripe (legacy)". Having said that: what do other people think? Shall we name this new payment method "Stripe SCA (beta)"?
"a card added with the old API is not usable in the intents API " this is not a fact and probably it is incorrect, I need to investigate further.
It is legal to add a card as an admin, yes, for example, buying by phone. what we will have is a process to send the customer an email to authorize the transaction if that is what the bank requires. These cases are covered in the stripe epic issues if you want to see more details: Support for Strong Customer Authentication (SCA) in Stripe integration - Part 2 #4170

app/assets/javascripts/admin/payments/services/payment.js.coffee

app/controllers/spree/admin/payment_methods_controller.rb

lib/open_food_network/subscription_payment_updater.rb

spec/requests/checkout/stripe_sca_spec.rb

app/models/spree/gateway/stripe_connect.rb

Matt-Yorkley

Awesome 👍

I left a couple of comments, and I'm also not sure we should make the payment method public until it's all finished. Maybe superadmin only would be good, so we can test it whilst we iterate?

luisramos0 · 2020-01-19T21:49:21Z

thanks for the review Matt.

Yeah, we can change its name or make it super admin only.
I am pretty confident we will make this migration really quickly so I'd just go for it as is.

… Intents API This commit can be reverted once we upgrade to v1.98.0

…ents API instead of the Stripe Charges API that the current StripeConnect gatreway uses

Update Source is dead since a74c502

…nt id before making the call to the stripe api This account id cannot be sent when dealing with the old StripeConnect gateway

…ition to it and the other stripe template We need to set the stripe object with the stripe account id to work with the payment intents api but we cannot set it to work with the stripe charges api This makes the two payment methods incompatible: a given enterprise cannot use both the old stripe integration and this new one at the same time.

…aking profile storer work with the slightly different api responses from stripe

…ake it work

…ipe_account_id must be included in the stripe api call

stripe_connect_spec will be deleted at some point when all users are migrated to the sca api

…ribute cases: existin stripe integration and new stripe sca

…etting up of process of a payment method and subscriptions Here we are copy pasting and adding stripe SCA because we are planning to delete the StripeConnect that will be replaced by the stripe sca implementation

luisramos0 · 2020-02-21T10:54:00Z

rebased to resolve conflicts and pushed to upstream so that this is also seen in #4719

luisramos0 · 2020-03-02T17:18:57Z

tested as part of 4719, merging.

luisramos0 self-assigned this Jan 13, 2020

luisramos0 changed the title ~~Stripe sca method~~ Add new Stripe payment method compatible with the new Stripe Payment Intents API Jan 13, 2020

luisramos0 force-pushed the stripe_sca_method branch 2 times, most recently from 40a0061 to 71f910b Compare January 13, 2020 19:38

mkllnk approved these changes Jan 16, 2020

View reviewed changes

lib/stripe/profile_storer.rb Outdated Show resolved Hide resolved

lib/stripe/profile_storer.rb Outdated Show resolved Hide resolved

luisramos0 force-pushed the stripe_sca_method branch from 556f7f3 to ca33d8a Compare January 16, 2020 13:19

luisramos0 force-pushed the stripe_sca_method branch 4 times, most recently from 7c67667 to 8ede82b Compare January 16, 2020 18:41

mkllnk approved these changes Jan 16, 2020

View reviewed changes