New payment state - Pending

[ashitikov]

Is your feature request related to a problem? Please describe.
Looking at vendure's payment plugins I see that current payment workflow is:

1. Client call createStripePaymentIntent through shop-ai or some method, which creates an entity in payment gateway.
2. Client make a payment through payment form.
3. An incoming webhook informs vendure about successfull payment.
4. Vendure calls addPaymentToOrder, which calls createPayment method on appropriate payment handler.

Basically, this scheme is okay, but problem arises when client want to specify more than one payments in the order.
Say an order has total - 100$.

1. Client adds a gift card or bonus payment to an order - 20$. It does not cover order's total.
2. Client creates a stripe payment intent through createStripePaymentIntent. But stripe payment plugin force order.totalWithTax field as amount, so amount result is 100$.
3. Client see 100$ amount in payment form, which is wrong.

What I would like to see:

1. Client adds a gift card or bonus payment to an order - 20$. It does not cover order's total.
2. Client adds a stripe payment intent through classical addPaymentToOrder, with amount 80$, because it calculates by formula: order.totalWithTax - all authorized/settled payments.
3. createPayment handler returns an object, like { state: 'pending or created', metadata: { url: 'stripe payment url' } }
4. Client uses an url from metadata to go to payment page, where amount to pay is 80$, which is right.
5. An incoming webhook informs vendure about successfull payment.
6. Vendure calls transitionPaymentToState which automatically transit a payment as well as an order to appropriate state.

I understand that I am completely able to implement this, thanks to vendure's extensible mechanisms, but it's cool to have a solid pattern here.

Describe the solution you'd like

1. Do not add new mutations, like createStripePaymentIntent. It generates an additional logic on storefront side. Imagine you have 3-4 connected payment plugins - your storefront or multiple storefronts should support every mutation for every provider.
2. Use traditional addPaymentToOrder to add new payments. It worth to place payment url into predefined field for that, not to metadata directly. Any of your storefront will be able to pay with any payment provider without an additional logic in unified way.
3. Consider adding new payment state 'Pending'. Right after payment creation, payment handler may transit to state 'Pending' (Created -> Pending), so it means payment plugin waits user to be paid with payment url/form.

Describe alternatives you've considered
@vrosa
doesn't that depend on the implementation of this additional payment, e.g. gift card? I would expect the order.totalWithTax to be $80 by the time createStripePaymentIntent is called because that's what the customer is going to be charged. Other platforms, e.g. Shopify, also consider subTotal or total to the be amount after the gift card has been applied.
Not sure how Vendure's paid gift card plugin will behave but I would expect it to subtract the amount from the order's total.

Additional context
@michaelbromley
The gift card plugin I'm working on actually treats gift cards/store credit as a payment method, in the way described by Alexander. One reason for this is to do with the issue of how to deal with something like a return - if I buy 2 t-shirts @ $20 each, and then use a Promotion-style discount to make the order total $0, then Vendure is actually treating each OrderItem as having a value of $0 - i.e. the value of OrderItem.proratedUnitPrice will be 0. So for the purposes of returns & refunds, this indicates that the amount to refund would be $0, which is not correct.
Treating the gift card as a payment method allows us to model the actual transaction values more accurately - i.e. if I use a gift card to "pay" for the above order, then a return of 1 t-shirt implies a refund/re-credit of $20.
So I think it does make sense to take into account any existing payments on this line when creating the Stripe payment intent.

Slack thread: https://vendure-ecommerce.slack.com/archives/CKYMF0ZTJ/p1656064741229719

[ashitikov]

@vrosa you could think gift card in another way: as amount which you can spend from your bonus account. Say you have an order with total - $100. You also have a loyalty system, integrated with vendure, where your account balance is $300 of bonuses. Loyalty system has a constraint, where you can spend 20% of the order's total. You add bonus payment in amount of $20 to this order, and then add a stripe/mollie/braintree/... payment in amount of $80 to fully cover order's total.
To me it's perfectly laid out to vendure's built-in mechanisms, except implementation of current vendure's payment plugins.

[michaelbromley]

I've been working on the StripePlugin over the past couple of days, improving on some edge cases and security holes that were descovered. I now tend to agree that it would be best to unify the payment flows with calls to addPaymentToOrder.

At this point, we can create a Payment in the Created state and attach the payment intent client_secret to the Payment's public metadata, so that it can be used in the frontend part. I'd prefer not to change the return type of addPaymentToOrder (which on success returns Order), but in any case the Payment metadata is easily retrievable from the Order type.

Doing it this way also has the following advantage: when we create the Payment, we can add the Stripe payment intent ID to the Payment's metadata. Then we can implement a CustomOrderProcess onTransitionEnd hook and catch transitions from ArrangingPayment -> AddingItems. When this occurs, we can look up the Payment and use the payment intent ID to cancel the payment intent with Stripe.

This would give use a more robust and auditable sequence of events for each order.

[ashitikov]

To me, Created state already used in order state machine, and in fact, normally you will not be able to get an order somehow in this state, because on creation order transit to AddingItems state immediately. I guess this is done to get proper event OrderStateChangedEvent after order creation. So, getting Created state means that something went wrong, and I think the same rule should be applied to payment state machine also.
Consider adding new state Pending instead of using existing Created to have similar behavior with other state machines.

[martijnvdbrug]

I've been working on the StripePlugin over the past couple of days, improving on some edge cases and security holes that were descovered. I now tend to agree that it would be best to unify the payment flows with calls to addPaymentToOrder.

@michaelbromley Any reference to the fixes you made? Mollie payment plugin is similar to the Stripe one, so I want to check the Mollie plugin for the same security fixes.

[kyunal]

I like the proposal, but I have some concerns:

Use traditional addPaymentToOrder to add new payments. It worth to place payment url into predefined field for that, not to metadata directly. Any of your storefront will be able to pay with any payment provider without an additional logic in unified way.

Keep in mind that not every payment method works or is desired to work with URL redirects. Stripe in particularly can easily be included on a site using Stripe Elements and the hosted checkout possibilities work quite different from that. As far as I understand the current Stripe implementation isn't even compatible with hosted checkouts. Unification to payment URLs could eliminate the possibility to include your own checkout, so metadata may still be a valid choice. I don't think it's too much to expect from developers to include specific handlers for the payment methods they intend to use.

---

Consider adding new payment state 'Pending'. Right after payment creation, payment handler may transit to state 'Pending' (Created -> Pending), so it means payment plugin waits user to be paid with payment url/form.

If I understand this correctly, as an extension of the payment (not order) state, then this may be a bigger change than what it sounds like.

First of all, this diverges a lot from what at least Mollie and Stripe handlers are doing right now. Right now, Payment objects for these methods are only stored in Vendure when some kind of confirmation was received through the respective webhook handlers. Before that, no reference to any created intents/tokens/.. is held. At most, the order is transitioned to the ArrangingPayment state.

Considering you now want to add a payment to the order even if you don't know it's ever being completed, how many pending payments would you want to allow? Infinite or one at the time? I believe only one at the time is feasible, else everything could become a huge mess. This then means that you need some kind of (user) opportunity to cancel a pending payment if another payment method is asked for.

This can be done automatically when addPaymentToOrder is called again, but in return may require a new callback function for payment handlers to deal with this case. For example, one might want (or need) to cleanly cancel the payment intent when this occurs.

This also begs the question for what to do if a user returns to the same payment later - should you try to reuse the old pending payment or create a new one?

In addition, some payment methods such as Mollie have expiring intents/tokens. This means a pending payment needs to be invalidated here.

[ashitikov]

Considering you now want to add a payment to the order even if you don't know it's ever being completed, how many pending payments would you want to allow? Infinite or one at the time? I believe only one at the time is feasible, else everything could become a huge mess.

This is reasonable question. I think one at the time is the most appropriate solution. Multiple pending payments can be an option, but requires some time to adopt order's total covering mechanism. Also introduces another questions, like what if customer will pay two or more pending payments. Theoretically it can be accomplished by cancelling (invalidating) other pending payments once one of the payment successfully settle.

This then means that you need some kind of (user) opportunity to cancel a pending payment if another payment method is asked for.

Exactly. Not so far away Michael has been done a cancellation handler: vendurehq/vendure#1637. I guess this is available since 1.7 version. So the only thing to be done is to have an exposed version of cancelPayment mutation on the shop-api.

This also begs the question for what to do if a user returns to the same payment later - should you try to reuse the old pending payment or create a new one?

This is up to the storefront logic. I guess both options are usable.

In addition, some payment methods such as Mollie have expiring intents/tokens. This means a pending payment needs to be invalidated here.

Yes it has. I think payment provider can share expiration date to metadata. Storefront then can read it and decide what to do: either cancel payment and recreate, or make a payment. Additionally cron may be introduced by payment plugin, which will cancel pending payments automatically by checking expiration date periodically. Instead of cron it can be done with a job queue, but it lacks possibility of delayed jobs currently ;(

[kyunal]

[Cancellation of old/unused pending payments] Exactly. Not so far away Michael has been done a cancellation handler: vendurehq/vendure#1637. I guess this is available since 1.7 version. So the only thing to be done is to have an exposed version of cancelPayment mutation on the shop-api.

I don't think this is a good idea as it may introduce a lot of headroom for bugs and potentially raises security concerns. I would prefer if the whole cancel/invalidation mechanism happens automatically on the backend side and is not influenced by GraphQL mutations. But maybe I'm too worried here.

[Reusing payments] This is up to the storefront logic. I guess both options are usable.

I tend to disagree, as it is highly dependant on the payment method and could give headroom to bad implementations. See the next response for a more in-depth example.

[Expiring open payments] Yes it has. I think payment provider can share expiration date to metadata. Storefront then can read it and decide what to do: either cancel payment and recreate, or make a payment. Additionally cron may be introduced by payment plugin, which will cancel pending payments automatically by checking expiration date periodically. Instead of cron it can be done with a job queue, but it lacks possibility of delayed jobs currently ;(

I don't think this should be accomplished with jobs. In the case of Mollie, the expiration time is dependant on the chosen payment method (if any was chosen) and is inprecise. Mollie recommends listening for the cancellation webhook event.

---

In summary, if a pending state is introduced which is supposed to display that there's an open payment which the user has not yet confirmed, in my mind this is the best way to go about this:

1. Pending Payments are always newly created on addPaymentToOrder calls.
2. Only one pending payment should be allowed at a time.
3. Pending payments are always cancelled whenever a new addPaymentToOrder call occurs.
4. Stripe/Mollie integrations should be refactored to only rely on addPaymentToOrder .
5. The storefront should always make an addPaymentToOrder call when a user is proceeding to payment. This ensures a newly created payment is used and that there is no expiration whatsoever.

[michaelbromley]

Thanks @kyunal & @ashitikov for the input on this issue so far.

Just as an update from my side - I have limited bandwidth right now to dive deep into this, but I'm keeping it on the "under consideration" list because I think there are good ideas here that will improve Vendure.

If anyone wants to explore these ideas in a custom version of one of the existing plugins then please do advise on the results & learnings so we can converge on a great design for a future version.

[martijnvdbrug]

@kyunal @ashitikov Any updates on this issue? It seems to be a little old now. Happy to advise/compare to what the Mollie plugin does, if needed!

[mezzat11]

@michaelbromley @dlhck any update