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

Allow SCA cards to be setup and charged offline for subscriptions #6469

Merged
merged 25 commits into from
Dec 11, 2020

Conversation

andrewpbrett
Copy link
Contributor

@andrewpbrett andrewpbrett commented Dec 8, 2020

What? Why?

Closes #4182
Closes #4723

This change allows customers to perform the extra SCA auth step when they authorize shops to charge their card offline.
It also no longer creates a clone of the card on the shop's Stripe account each time we need to authorize or charge a card stored on the platform (instance) Stripe account.

What should we test?

Adding a card that requires SCA (see http://stripe.com/docs/testing ) to the /account page, setting it to be the default card, and then ticking the box to authorize a shop to charge it offline should prompt the extra SCA auth step.

Release notes

We now support SCA when charging cards offline for subscriptions.

Changelog Category: User facing changes

Dependencies

Documentation updates

@andrewpbrett
Copy link
Contributor Author

That rubocop failure is not showing up for me locally...

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

There are still a few detours in the commit history but it all looks reasonable. I would need to invest a lot more time to properly understand how it all works. Instead I'll trust you. 😜

And I'm glad we have thorough testers.

app/models/spree/order.rb Show resolved Hide resolved
app/models/spree/payment/processing.rb Outdated Show resolved Hide resolved
app/models/spree/payment/processing.rb Outdated Show resolved Hide resolved
app/models/spree/payment/processing.rb Show resolved Hide resolved
lib/stripe/credit_card_cloner.rb Show resolved Hide resolved
lib/stripe/credit_card_cloner.rb Outdated Show resolved Hide resolved
lib/stripe/credit_card_cloner.rb Outdated Show resolved Hide resolved
lib/stripe/credit_card_cloner.rb Outdated Show resolved Hide resolved
lib/stripe/credit_card_cloner.rb Outdated Show resolved Hide resolved
@andrewpbrett andrewpbrett removed the request for review from Matt-Yorkley December 10, 2020 21:50
Copy link
Contributor

@sauloperez sauloperez left a comment

Choose a reason for hiding this comment

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

This is really good! Congrats on this great effort @andrewpbrett 👏 !

@filipefurtad0 filipefurtad0 self-assigned this Dec 11, 2020
@filipefurtad0 filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-es labels Dec 11, 2020
@filipefurtad0
Copy link
Contributor

Hey @andrewpbrett ,

Awesome work, very interesting changes and quite a lot going on... Let's have a look at some test cases, starting with the acceptance criteria & tests from the issues this PR is closing:

  1. adding a new card under /account#cards
  1. enabling authorizations for an enterprise to charge:
  • a newly added, valid 3D-card - 4000 0027 6000 3184:
    All good. The authentication window kicks-in, and one can cancel, proceed or decline authentication, as usual (pic below):

image

  • a newly added, invalid 3D-card - 4000008260003178:

Same as above. Authentication works here as well 👍

  • a previously existing, valid 3D-card - 4000 0027 6000 3184
  • a previously existing, invalid 3D-card - 4000008260003178

All good!

  1. deleting cards

all good: 3D- or non-3D cards can be deleted.

I found a issue with some "really old" cards, from Spree Upgrade still. these can't be deleted, for some reason: generates error 500

for example, https://staging.openfoodnetwork.org.uk/credit_cards/2995

https://app.bugsnag.com/yaycode/openfoodnetwork-uk/errors/5fd355b56e76b60018be4983?event_id=5fd355b500661b9c4d590000&i=sk&m=nw

I don't think this PR introduced this.

  1. Regression test: I made a quick test on subscriptions, using the newly introduced testing-method on PR Add tasks to help manually test subscriptions #6291 (it works!)
    I found subs not to be working for 3D-Secure cards yet, but are working with the usual settings.

All good! Awesome work 🎉

@filipefurtad0 filipefurtad0 removed pr-staged-es pr-staged-uk staging.openfoodnetwork.org.uk labels Dec 11, 2020
@andrewpbrett andrewpbrett merged commit a745fce into openfoodfoundation:master Dec 11, 2020
@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Dec 14, 2020
@filipefurtad0
Copy link
Contributor

I've extended the regression test mentioned in the testing notes (point 4) to Stripe Connect - before, this was only tested for StripeSCA for 3D-Secure (not working) and non-3D-Secure cards (working).

This PR appears to break subscriptions for Stripe Connect and triggers the error:

https://app.bugsnag.com/yaycode/openfoodnetwork-uk/errors/5fd7a8e03ba8d20017bb3ae0?event_id=5fd7a8e000658c991a880000&i=sk&m=nw

@filipefurtad0 filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Dec 14, 2020
Matt-Yorkley added a commit that referenced this pull request Dec 17, 2020
Patch #6469: use `purchase` to charge offline for Stripe Connect
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants