vendure-ecommerce / vendure

The commerce platform with customization in its DNA.
https://www.vendure.io
Other
5.77k stars 1.02k forks source link

Customers are able to pay twice for an order in Mollie #2620

Closed nicoes closed 8 months ago

nicoes commented 10 months ago

Describe the bug We get reports from customer service that every now and then a customer pays twice for an order in Mollie. In Mollie I do indeed see two payments created for that order. Whereas we would expect that this is prevented somehow. As in prevent the customers from starting a 2nd check-out.

Screenshot 2024-01-11 at 10 47 53

To Reproduce Steps to reproduce the behavior:

  1. Add items to an order
  2. Use createPaymentIntent mutation
  3. Go to the returned check-out url
  4. Use createPaymentIntent for the order again
  5. A new url is returned
  6. Go to the returned check-out url
  7. Finish payments in both urls
  8. You have done two payments which should be revealed in Mollie's dashboard as well. (See schreenshot)

Expected behavior At step 4 an error is returned / user is warned that another payment has already been started.

Environment (please complete the following information):

martijnvdbrug commented 10 months ago

:eyes: Joining this issue :-)

seminarian commented 10 months ago

I remember encountering this.

There are multiple scenarios possibly:

  1. Customer goes back and starts a new payment for same order content.
  2. Customer goes back and starts a new payment for a changed order content.

So depending on the scenario, different limitions / checks should probably be applied.

Perhaps a solution could be the Mollie Plugin:

This will probably tackle the issue reported. It won't be perfect but I will be an improvement.

I think I remember the "Payment API" of Mollie to be a limitation in removing payment intents/updating them/.. Would a more refined payment handling be possible with the "Order API" of Mollie? This last paragraph should probably be checked for correctness, as mentioned it's been a while.

martijnvdbrug commented 10 months ago

Thanks for yout input @seminarian. The Mollie Plugin now uses the order API, so indeed with a reference to the Mollie order, we are able to cancel the order and create a new one.

How would you cancel the Mollie order when transitioning to AddingItems? I guess we can listen for OrderTransitionEvent, but that will make it async. I guess this is fine, but if you are REALLY quick you could still pay for your order twice

seminarian commented 10 months ago

@martijnvdbrug

The Mollie Plugin now uses the order API, so indeed with a reference to the Mollie order, we are able to cancel the order and create a new one.

Just make sure this statement correct. I remember something about payments not always being cancelable: https://docs.mollie.com/reference/v1/payments-api/cancel-payment#:~:text=Some%20payment%20methods%20are%20cancellable,the%20payment%20can%20be%20canceled.

How would you cancel the Mollie order when transitioning to AddingItems? I guess we can listen for OrderTransitionEvent, but that will make it async. I guess this is fine, but if you are REALLY quick you could still pay for your order twice

Not sure about that, the backend could probably track if it got succesfully cancelled or if it's a different amount to be paid. Perhaps this decision can happen in Vendure Backend when a new paymentIntent is requested?

martijnvdbrug commented 10 months ago

Perhaps this decision can happen in Vendure Backend when a new paymentIntent is requested? Even better indeed! I guess to keep things simple, we always cancel the current mollie-order with payments when a new pament intent is created.

I guess when the order or one of the payments couldn't be cancelled, we just log an error and create a new order with payments, just as we do now.

nicoes commented 10 months ago

Thanks for all the input. Great that this has been moved to the backlog

michaelbromley commented 10 months ago

How would you cancel the Mollie order when transitioning to AddingItems? I guess we can listen for OrderTransitionEvent, but that will make it async. I guess this is fine, but if you are REALLY quick you could still pay for your order twice

If we use a custom OrderProcess we could put this check in the inTransitionEnd hook which would execute non-async (i.e. serially with the state transition) which should prevent the edge case you mention.

@martijnvdbrug do you have any capacity to work on this in the near future?

martijnvdbrug commented 10 months ago

How would you cancel the Mollie order when transitioning to AddingItems? I guess we can listen for OrderTransitionEvent, but that will make it async. I guess this is fine, but if you are REALLY quick you could still pay for your order twice

If we use a custom OrderProcess we could put this check in the inTransitionEnd hook which would execute non-async (i.e. serially with the state transition) which should prevent the edge case you mention.

Thats an option, but it feels pretty intrusive if a payment plugin installs a custom order process. Plus a consumer could define their own order process, breaking the payment plugin. @seminarian s suggestion for doing it on new payment creation also works ai think.

@martijnvdbrug do you have any capacity to work on this in the near future?

Yep, still working together with Nico, and its causing issues, so I will be picking this up.

michaelbromley commented 10 months ago

it feels pretty intrusive if a payment plugin installs a custom order process

Remember that the orderOptions.process is an array, so all the configured processes compose together as a pipeline. So you would just create a bare-bones OrderProcess which only adds the specific logic for this check. All other processes that are already configured will still run as usual.

And great to hear you can work on this 👍

martijnvdbrug commented 10 months ago

Remember that the orderOptions.process is an array, so all the configured processes compose together as a pipeline. So you would just create a bare-bones OrderProcess which only adds the specific logic for this check. All other processes that are already configured will still run as usual.

And great to hear you can work on this 👍

Ahh I see it now. Thanks. mergeStrategy: 'replace' is only for actual state transitions, so an onTransitionEnd would always be called and can not be overridden?

michaelbromley commented 10 months ago

Yes correct. The only way to prevent it from executing would be to actively remove that OrderProcess from the array. Source reference: https://github.com/vendure-ecommerce/vendure/blob/cf301eb788fca9ea9682e957369880ec6cf64804/packages/core/src/service/helpers/order-state-machine/order-state-machine.ts#L79

martijnvdbrug commented 10 months ago

Seeking input

Ok, so the proposed solution is:

  1. We will transition to ArrangingPayment before redirecting the customer to Mollie
  2. When an order is transitioned from ArrangingPayment back into AddingItems we cancel the Mollie order (if possible)

The question still remains, who will transition the order back into AddingItems? Is it the storefront? The customer could still have another tab open with the storefront, so on what event would you transition an order back to AddingItems?

michaelbromley commented 10 months ago

So do you mean that the createMolliePaymentIntent mutation will do the state transition automatically to ArrangingPayment (if not already in that state)?

The question still remains, who will transition the order back into AddingItems?

I would leave this as a concern of the storefront implementation.

martijnvdbrug commented 9 months ago

So, we are struggling a bit on when the storefront should transition an order back to AddingItems. When does the storefront know that the order is in ArrangingPayment. There are some possibilities, but none seem very clean:

Any suggestions are welcome :-)

martijnvdbrug commented 9 months ago

After discusssing with @nicoes we opted for the following solution:

Transition to ArrangingPayment after payment

What does it not solve

A customer can still open a new tab with the storefront after a Mollie Checkout has been started and add items to the active order. If the customer then finalizes payment with the old Mollie order, the Vendure order will go to ArrangingAdditionalPayment. This is something the storefront could handle, but doesn't have to.

Why not transition to ArrangingPayment before going to Mollie checkout?

It's quite a big change for the storefront compared to the current way of working. All mutations need to check if the order is in AddingItems or catch the OrderModificationError.

The proposed solution doesn't require any changes on frontend. The issue where an order goes to ArrangingAdditionalPayment instead of PaymentSettled can optionally be handled on the storefront's confirmation page, but is not required.