Cancelling a partially refunded order triggers fatal exception

Sylius / RefundPlugin

Basic refunds functionality for Sylius

MIT License

64 stars 67 forks source link

Cancelling a partially refunded order triggers fatal exception #418

Open robgwin opened 3 years ago

robgwin commented 3 years ago

Sylius version affected: 1.7 and appears unfixed in master

Description
If you partially refund an order and then attempt to cancel it (for example you refunded the items but not the shipping) you get InvalidArgumentException "Not enough units to decrease on hold quantity from the inventory of a variant". But it should not be attempting to release the hold, because the hold was already released when the order was paid.

Steps to reproduce

Place an order with a tracked product
Complete the payment
View the order in admin, refund part of the payment
Cancel the order, get exception

Possible Solution
Account for OrderPaymentStates::STATE_PARTIALLY_REFUNDED in OrderInventoryOperator::cancel()

stloyd commented 3 years ago

AFAIK this is correct behaviour that you should not be able to cancel refunded order.

Cancelling IMHO should be doable only before someone pays for the order, after payment you can only refund the order partially (and deliver part of it) or refund full order.

robgwin commented 3 years ago

Sure, that makes sense too. But somebody at one time must have thought refunded orders should be cancel-able, because the UI currently allows you to cancel an order in any state, and OrderInventoryOperator::cancel() explicitly handles a refunded order (but crashes on a partially refunded order).

If refunded orders should not be cancel-able, then the UI should prevent it, and OrderInventoryOperator::cancel() should not have a use case for refunded (or paid?) orders.

robgwin commented 3 years ago

I just realized the refund UI is actually provided by sylius/refund-plugin, and updated my OP to note this. Ideally perhaps this issue should be addressed in the plugin somehow, but I'm leaving it here for now because (a) the plugin is the officially recommended way to handle refunds, and (b) core sylius already makes some attempt to negotiate the refunded state.

robgwin commented 3 years ago

Regardless of the UI and what an admin "should" be able to do, it seems safe to say that when an order is cancelled:

Sylius should release holds only if there are active holds (basing that on payment status is not ideal but I think it's all we have?)
Otherwise Sylius should probably not do any inventory adjustment, because there is no way to know the real status of the physical item. (Even shipping state isn't a sure indicator, cause you might have an in-store pickup that isn't shipped). If you cancel an order at a late stage such that it may or may not have left your inventory, it's reasonable to expect you to fix the inventory manually.