Fix how orders payment total is calculated #1536
Conversation
| @@ -73,7 +73,7 @@ def update_shipments | |||
| end | |||
|
|
|||
| def update_payment_total | |||
| order.payment_total = payments.completed.includes(:refunds).map { |payment| payment.amount - payment.refunds.sum(:amount) }.sum | |||
| order.payment_total = payments.completed.sum(&:amount) | |||
There was a problem hiding this comment.
I'm concerned that we're changing the meaning of this column. Both because existing applications may expect the amount to have the refunds subtracted, and because existing records in the database will already exist with the existing values.
|
cc @jordan-brough who has written a good summary of the state of Refunds, payment_total and outstanding balanace in #1254 |
This commit addresses an issue in `Spree::Order#outstanding_balance`, in circumstances where an Order has had refunds applied. The existing behaviour cancels out the amount of refunded payments, resulting in a negative outstanding balance and an order state of 'credit_due', when in fact the order balance is zero. This is caused by the calculation of `payment_total` already factoring in refunds. Some more discussion about this behaviour can be found in the following issues: solidusio#1107 solidusio#1536 With these changes, `outstanding_balance` is now defined as `total - payment_total`. This corrects the behaviour around refunds. Tests have been updated accordingly, with some help from DanielePalombo's commit: solidusio@c851404
In Spree::OrderUpdater the `update_payment_total` was incorrectly removing the refunds total from each order payment. This seems incorrect because they are separate things. Refund total has to be taken away into the outstanding_balance calculation instead. Replace payment_total column with method that subtract refunds from incoming_payment. To keep backward compatibility update_payment_total call update_incoming_payment, and put in incoming_payment the sum of completed payments. rel solidusio#1475
DanielePalombo
force-pushed
the
update-credits-owed
branch
from
c851404 to
777c8da
Compare
|
After some insights i refactor the code to keep backward compatibility |
|
@DanielePalombo this PR didn't work as expected for me. Steps I took:
Expected to see balance due as $0.00 with a payment state of Thank you very much for your effort on this. I'll be happy to continue to QA, but I believe we now need some feature tests to automate some of this QA if wish to avoid future regressions. |
|
@bbuchalter,
I Agree with you! I can confirm the result after the same steps, but maybe i found a workaround. After the refund i create an adjustment ,or delete the item from
I don't know if this can resolve your issues or if we can create an adjustment after the reimbursment... i would like to deepen the discussion. |
|
I'm going to defer further conversation on this topic to the core team at the moment. |
|
Any updates on this? |
|
Thanks Daniele. I think this is fixed #2002. If not please file a new issue or PR |
In Spree::OrderUpdater the
update_payment_totalwas incorrectlyremoving the refunds total from each order payment. This seems
incorrect because they are separate things. Refund total has to be
taken away into the outstanding_balance calculation instead.
rel #1475