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

Add support for SetupIntents and skipping the confirm step for PaymentIntents #203

Merged
merged 8 commits into from
Mar 7, 2023

Conversation

kennyadsl
Copy link
Member

@kennyadsl kennyadsl commented Mar 3, 2023

Summary

  • Allows to use SetupIntents in the payment step and avoid the premature authorization on credit cards
  • Allows to skip the confirm step when using PaymentIntents and have the checkout end with the payment
  • Stores the payment intent id on a model attached to the order instead of using an "in progress" payment that gets created at the payment step

Checkout specs covering the happy path for each case have been added, this should avoid regression and demonstrate the expected flows.

Authors

@kennyadsl @elia @rainerdema @waiting-for-dev

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

@waiting-for-dev
Copy link
Contributor

waiting-for-dev commented Mar 6, 2023

That's nothing short of astounding, folks! Superb work! I still need to go through the code, but the flow looks very consistent. I hope the authorization prompts are also consistent (:crossed_fingers:, I think they will), and I look forward to hearing your ideas for the guest checkout flow.

The incoming webhooks are what you could expect: no surprises!

First successful checkout

On payment element creation On payment element submission On clicking "Confirm"
customer.created payment_method.attached payment_intent.created
setup_intent.created setup_intent.succeeded payment_intent.amount_capturable_updated
charge.succeeded

Subsequent successful checkout with the same source

On payment element creation On payment element submission On clicking "Confirm"
setup_intent.created setup_intent.succeeded payment_intent.created
payment_intent.amount_capturable_updated
charge.succeeded

Setup failure (using the same source, tested with card 4000000000000002)

On payment element creation On payment element submission On clicking "Confirm"
setup_intent.created
setup_intent.setup_failed

Payment failure (using the same source, tested with card 4100000000000019)

On payment element creation On payment element submission On clicking "Confirm"
setup_intent.created setup_intent.succeeded payment_intent.created
payment_intent.failed
charge.failed

I'm pretty sure you're aware of at least some of the following mostly minor issues that we need to address if we can settle it as the path forward:

  • A setup intent is created with only the payment element rendered. That means that if the user selects another Solidus payment method, that'll still have side effects on Stripe. If I'm not wrong, there's no workaround for that, as it's how Stripe is designed.
  • The payment that is created after the payment element submission is in the :checkout state. Wouldn't :processing be more accurate to Solidus standards (I'm not sure)?
  • We're always saving the payment source. Users have no way to opt-out.
  • A payment failure raises an error that we aren't handling (although the order stays on the :confirm sate 👍 ).

I also tried to go back from the :confirm state to add more products to the cart, and the payment was updated accordingly 👌

@waiting-for-dev
Copy link
Contributor

The payment that is created after the payment element submission is in the :checkout state. Wouldn't :processing be more accurate to Solidus standards (I'm not sure)?

As discussed offline, :checkout is the right state. Solidus will move them to :processing once the payment processing starts, which will happen just before the call to Stripe's API.

@kennyadsl
Copy link
Member Author

We're always saving the payment source. Users have no way to opt-out.

Yes, and that's what's happened with every other payment method (unless I'm missing something). One good thing is that at least the Stripe form states what's happening very clearly. BTW I imagine this to be an issue with guest checkouts only, and in that case we could explore deleting the setup intent after the checkout is placed (if we still have the ability to capture/void via admin using the payment intent generated at confirm)

kennyadsl and others added 8 commits March 6, 2023 13:58
Pave the way to using it for both payment and setup intents.

Co-Authored-By: Elia Schito <elia@schito.me>
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
Allow each payment method to pick which flow to use.

Co-Authored-By: Elia Schito <elia@schito.me>
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
Co-Authored-By: Marc Busqué <marc@lamarciana.com>
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
Co-Authored-By: Marc Busqué <marc@lamarciana.com>
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
…t intents

Pave the way for future refactorings.

Co-Authored-By: Marc Busqué <marc@lamarciana.com>
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
No longer use the payment as the connection point.

Co-Authored-By: Marc Busqué <marc@lamarciana.com>
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
Co-Authored-By: Marc Busqué <marc@lamarciana.com>
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
Co-Authored-By: Marc Busqué <marc@lamarciana.com>
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
@elia elia force-pushed the nebulab/setup-intent-prototype branch from 98eeda5 to 8064f1c Compare March 6, 2023 13:00
@elia elia marked this pull request as ready for review March 6, 2023 14:09
@elia elia changed the title Prototype: Convert PaymentIntent to SetupIntent Add support for SetupIntents and skipping the confirm step for PaymentIntents Mar 6, 2023
@waiting-for-dev
Copy link
Contributor

Recap after another pairing session:

Guest checkout through setup intent

We implemented the guest checkout flow through setup intents by creating a Stripe customer with the order email and the number as a reference in its metadata fields. It worked.

However, we didn't try to remove the created setup intent. That means that it's possible to charge the customer afterward from the Stripe dashboard:

That doesn't happen when charged through a payment intent (when you try to create a payment for the same customer, you're forced to enter a new card):

Besides, Stripe renders a notice message that will be confusing even if we manage to remove the setup intent:

Another important issue is that some bank-associated payment methods are designed as one-off payments (Sofort, Ideal...), but Stripe translates them into SEPA Debit authorizations when used through a setup intent. There're two major issues with that:

  • We'd asking for authorization of more than one payment not only at the Stripe layer but beyond: the flow to authorize could be very different.
  • We'd be, in fact, charging via a different payment method than the one chosen by the user.

Configurable flow: setup intent vs. payment intent

Because of the problems abovementioned, we're now supporting both flows: setup intent and payment intent. Users can choose which one they use through the stripe_intents_flow preference on the Stripe payment method ('payment' vs. 'setup).

Squashing 'confirm' & 'complete' step in the payment intent's callback controller

Because of the problem with authorized payments before completion, we modified the payment intents callback controller to call order.complete! when a payment is successful. That's still a configurable option through the skip_confirmation_for_payment_intent boolean preference.

Getting rid of the double payment for the payment intents flow

We're making use of a Solidus' payment intent model so that we can defer the payment creation after the payment element has been submitted. That way, we no longer have the double payment problem when reusing payment sources or using a payment method different from Stripe.


Where are we now?

Let's summarize the main blockers:

Setup intent flow

  • Guest checkout:
    • it's possible to charge a user after the checkout from the Stripe dashboard (TODO: we didn't try to cancel the setup intent on completion).
    • "you allow your card to be charged in future payments" message.
  • One-off bank payments
    • Authorization flow is the one for SEPA Debit.
    • The selected payment method is not actually the one to be used.

Payment intent flow

  • Squashing the complete and confirm steps is kind of hacky. For instance, store credits won't be probably usable.

Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

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

Sounds like a self approval… because it is! 😅

Copy link
Contributor

@rainerdema rainerdema left a comment

Choose a reason for hiding this comment

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

💯

@waiting-for-dev
Copy link
Contributor

Setup intent flow
Guest checkout:
it's possible to charge a user after the checkout from the Stripe dashboard (TODO: we didn't try to cancel the setup intent on completion).

@elia, @rainerdema, @kennyadsl, I added another commit to prove that we can implement guest checkout through setup intents without opening gates to future charges. It's more of a workaround for Stripe's semantics for a setup intent, but it can be done. Some considerations:

  • Detaching a payment method from Stripe is the API call we need. I think we could also cancel the setup intent after that (I didn't try), but it can't be canceled before because of having an associated payment method.
  • We can't use the callback controller, as that's leaving the order on the confirm state. Later, on order.complete!, we still need access to the payment method to authorize the payment.
  • Because of the above, I created a subscriber that runs when the order is finalized.
  • The previously created payment intent can still be manually captured after the payment method is detached.
  • Running on the :order_finalized event has the known issue of blocking the database transaction.

@elia
Copy link
Member

elia commented Mar 7, 2023

@waiting-for-dev that's great, super quick and simple, given the specs are red, would you prefer to fix them here or would be ok to merge and have a dedicated PR for this feature?

@waiting-for-dev waiting-for-dev force-pushed the nebulab/setup-intent-prototype branch 2 times, most recently from e8e4d85 to 8064f1c Compare March 7, 2023 10:53
@waiting-for-dev
Copy link
Contributor

@waiting-for-dev that's great, super quick and simple, given the specs are red, would you prefer to fix them here or would be ok to merge and have a dedicated PR for this feature?

As discussed offline, we'll defer that for later and, for now, we'll be clear in the README

@elia elia merged commit a44bdc5 into master Mar 7, 2023
@elia elia deleted the nebulab/setup-intent-prototype branch March 7, 2023 13:07
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.

4 participants