Now that #2804227: Add getTotalPaid() and getBalance() methods to orders and #2856586: Add an "order paid" event are in, we should start using them in PaymentProcess.
1) If the order was paid, go to the next step.
2) When creating a payment, the amount should be taken from $order->getBalance().
This provides two benefits:
- Guards against duplicate payments in case the customer somehow goes through PaymentProcess twice (buggy checkout).
- Allows an admin to create a partial payment, then send the customer through checkout to pay the rest.
Plus other partial payment use cases.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 3003832-7.patch | 7.57 KB | bojanz |
| #2 | commerce-payment-prevent-duplicate-3003832-2.patch | 1.37 KB | zerolab |
Comments
Comment #2
zerolab commentedHere is a patch on top of the ones from #2804227: Add getTotalPaid() and getBalance() methods to orders
If this is going in the right direction, will continue with adding some tests.
Comment #3
zerolab commentedTo note: waiting on confirmation as how exactly the duplicate payments happen. i.e. is the second payment created while the previous one is not yet complete, or is it a case of cached redirect which results in another full payment after the first one is complete.
The patch currently works for the second scenario as an extension of #2804227: Add getTotalPaid() and getBalance() methods to orders
Comment #4
bojanz commentedThe parent issue has landed.
Comment #5
zerolab commentedThe patch doesn't break any of the existing tests, which is great.
What is the best place for a test for it?
Comment #6
bojanz commentedWe need to do two things:
1) If the order was paid, go to the next step.
2) When creating a payment, the amount should be taken from $order->getBalance().
#1 should use $order->isPaid() that we added in #2856586: Add an "order paid" event.
Comment #7
bojanz commentedLet's try this.
For simplicity made paid orders behave the same as free orders (if we don't need any money, there's no point in asking for a payment method).
Fun fact:
isPaid() is always true for free orders cause the balance is zero. So the second comparison is never reached. However, I've kept it to keep things explicit.
Comment #9
bojanz commentedLet's proceed.
Comment #10
zerolab commentedI'm afraid this doesn't fix it.
Scenario (the closest I have to reproducing the issues we were seeing on the live site):
- add something to cart
- get to the payment details step
- manually add a payment for the order
- click continue to payment
- now with 2 payments.
Comment #11
bojanz commented@zerolab
You can extend PaymentCheckoutTest to demonstrate additional bugs.
In this case it's possible that you didn't refresh the checkout page after creating the payment in the background, overwriting the total_paid field on submit.
This is something that is still possible, until we add locking.
Comment #12
zerolab commentedWe had another duplicate payment with the patch from #2 in place. Logs show 2 concurrent requests to the payment step, then Braintree duplicate payment kicking in, so the user see the expected (but generic) error message. They seem to change card details and go to pay again.
At this point there is a completed payment, so with your patch, the button they'd see on the the payment details page is "complete" rather than "proceed to payment".
We'll apply both patches for peace of mind.