Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cancelling a partially refunded order triggers fatal exception #418

Open
robgwin opened this issue Aug 6, 2021 · 4 comments
Open

Cancelling a partially refunded order triggers fatal exception #418

robgwin opened this issue Aug 6, 2021 · 4 comments

Comments

@robgwin
Copy link

robgwin commented Aug 6, 2021

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
Copy link

stloyd commented Aug 7, 2021

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
Copy link
Author

robgwin commented Aug 7, 2021

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
Copy link
Author

robgwin commented Aug 7, 2021

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
Copy link
Author

robgwin commented Aug 9, 2021

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.

@TheMilek TheMilek transferred this issue from Sylius/Sylius Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants