Update order cancellation, sets inventory units to canceled #3781

stefano-sarioli · 2020-09-28T12:59:51Z

This PR fix #3719.

Before this commit when an order was canceled the inventory units
of the order will remain untouched. So there were canceled orders with
inventory units with the state on_hand or backordered.

This situation was not only inconsistent but can lead to subtle bugs
with the stock management system.
How to reproduce the bug:

Create an order for a single variant, but select a number of items greater than the available amount.
The order generates 2 shipments, one with the inventory units on_hand and one with the inventory units back-ordered.
Cancel the order.
The inventory units on_hand remains on_hand.
The inventory units back-ordered go to the on_hand state.
If you resume the order all the inventory units will be on_hand, and the shipment will be ready to be shipped.

This problem is caused by the fact that when the order is canceled, a movement is created to restock the stock_item correlated with the inventory_units. As soon as the stock_items get back to a positive value it will try to convert as much as possible back-ordered inventory units to on_hand.

With this commit the inventory units related to an order are canceled when the order is canceled.

When an order is resumed we recalculate the state of the connected
inventory units, based on the actual stocks available.

What do you think about this issue, and my solution?
I'm not sure that the testing is enough, probably I can add an E2E test, what do you think? any idea on what can be the best place to put this test?

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
I have attached screenshots to this PR for visual changes (if needed)

core/spec/models/spree/shipment_spec.rb

kennyadsl

Left a preliminary concern, also some specs seem to be failing.

kennyadsl · 2020-09-28T13:17:42Z

core/app/models/spree/shipment.rb

+          units_available, units_to_back_order = stock_location.fill_status(item.variant, item.quantity)
+
+          inventory_units.canceled.limit(units_available).each(&:on_hand!)
+          inventory_units.canceled.limit(units_to_back_order).each(&:backorder!)


Is canceling an order the only way to cancel an IU at the moment? Just wondering if other IUs can have the state canceled and they would be resumed, even if not part of the order cancelation.

No, you are right, there are other places.
I have to pick only the canceled units for the variant reported on the manifest items.

I've used the line_item to be sure.

BTW I'm double-checking this to be sure. Sorry for the message spam 🙇 .

core/spec/models/spree/shipment_spec.rb

Before this commit when an order was canceled the inventory units of the order will remain untouched. So there were canceled orders with inventory units with the state on_hand or backordered. This situation was not only inconsistent but can lead to subtle bugs with the stock management system. With this commit the inventory units related to an order are canceled when the order is canceled. When an order is resume we recalculate the state of the connected inventory units, based on the actual stocks available.

kennyadsl

Just left a question, overall I can't see any issues with this approach.

kennyadsl · 2020-09-29T14:37:29Z

core/app/models/spree/shipment.rb

+        number_of_items_to_restore = item.states["on_hand"].to_i + item.states["backordered"].to_i
+
+        if item.variant.should_track_inventory?
+          units_available, units_to_back_order = stock_location.fill_status(item.variant, number_of_items_to_restore)


Are these variables filled with the current availability in the stock location, right?

Exactly, this will update the inventory units state based on the current stock level.

This is why inside #after_resume I have to call #resume_inventory_units before unstocking the items. If you call #resume_inventory_units after the unstocking the states of the inventory units will be wrong.

There is a test covering this use-case to avoid regression.

kennyadsl · 2020-09-29T14:37:57Z

core/spec/models/spree/shipment_spec.rb

        shipment.resume!
        expect(shipment.state).to eq 'pending'
      end
    end

+    context "when any inventory was already canceled" do


Thanks for adding this spec!

Thanks to you for pointing out the edge case 🙇, I totally missed it!

kennyadsl

Thanks @stefano-sarioli !

spaghetticode · 2020-09-30T15:28:08Z

core/lib/spree/core/state_machines/inventory_unit.rb

@@ -34,6 +34,14 @@ module InventoryUnit
            event :cancel do
              transition to: :canceled, from: ::Spree::InventoryUnit::CANCELABLE_STATES.map(&:to_sym)
            end
+
+            event :on_hand do


I'm not 100% sure about this event name.

Semantically speaking, event names should be verbs, as they actively change the state of an object. In this case, the action name is the same as the target state, and this may lead to some confusion too.
Also, there's the query method #can_on_hand? added automatically, which again does not make 100% sense to me.

What about changing the name to something like set_on_hand or make_on_hand? And if you find a better naming, just go for it.

I agree with you, I was not convinced too by the name of that event but couldn't think of something better.

I like set_on_hand proposed by you ❤️.

Sadly, as pointed out in the last comment of Kennyadsl, I'm probably going to close this PR to move the whole resume functionality out of Solidus.

Thanks a lot for your feedback, I'll note it down to use it when the work on the extracted functionality will start 🙏.

kennyadsl · 2020-09-30T15:55:52Z

core/app/models/spree/shipment.rb

+      inventory_units.where.not(state: 'canceled').each(&:cancel!)
+    end
+
+    def resume_inventory_units


We discussed this within the core team. We think that resuming concerns should be moved into a separate PR. There are a lot of opinions on how resuming an order should work and probably none of them will work as the majority of stores expect (everyone wants to restock items in different ways, depending on if they are using Solidus to manage stocks, which itself has a lot of variables, or they are relying on an external system).

We are thinking to actually deprecate the Resume order functionality. It's not used a lot and gives us more headaches than the benefits it provides to stores (none of us can think of a single store that ever used this functionality). The way to go here is moving the current behavior into a legacy extension, and deprecate the functionality in core (removing the Resume button from the admin UI). WDYT?

I agree with you, especially if it's an underused functionality.
My solution is far too general and makes a lot of assumptions about the stock management system.

I think that extracting the resume functionality into an external gem, maybe with this fix on it, would be the best solution.

Probably we can close this PR, I'll wait for the final decision about the resume functionality and will come back to contribute to that one as soon as it's ready.

Thank a lot for your feedback and consideration 🙏.

The cancellation part of this PR is well done and something that we definitely need in core. Please don't close the PR but just remove the part that resumes the inventory units. 🙏

houndci-bot reviewed Sep 28, 2020

View reviewed changes

core/spec/models/spree/shipment_spec.rb Outdated Show resolved Hide resolved

core/spec/models/spree/shipment_spec.rb Outdated Show resolved Hide resolved

kennyadsl reviewed Sep 28, 2020

View reviewed changes

houndci-bot reviewed Sep 29, 2020

View reviewed changes

core/spec/models/spree/shipment_spec.rb Outdated Show resolved Hide resolved

core/spec/models/spree/shipment_spec.rb Outdated Show resolved Hide resolved

kennyadsl reviewed Sep 29, 2020

View reviewed changes

kennyadsl approved these changes Sep 30, 2020

View reviewed changes

spaghetticode reviewed Sep 30, 2020

View reviewed changes

kennyadsl reviewed Sep 30, 2020

View reviewed changes

stefano-sarioli closed this Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update order cancellation, sets inventory units to canceled #3781

Update order cancellation, sets inventory units to canceled #3781

stefano-sarioli commented Sep 28, 2020 •

edited

Loading

kennyadsl left a comment

kennyadsl Sep 28, 2020

stefano-sarioli Sep 28, 2020

stefano-sarioli Sep 28, 2020

stefano-sarioli Sep 28, 2020

kennyadsl left a comment

kennyadsl Sep 29, 2020

stefano-sarioli Sep 30, 2020

kennyadsl Sep 29, 2020

stefano-sarioli Sep 30, 2020

kennyadsl left a comment

spaghetticode Sep 30, 2020

stefano-sarioli Oct 1, 2020

kennyadsl Sep 30, 2020

stefano-sarioli Oct 1, 2020

kennyadsl Oct 1, 2020

Update order cancellation, sets inventory units to canceled #3781

Update order cancellation, sets inventory units to canceled #3781

Conversation

stefano-sarioli commented Sep 28, 2020 • edited Loading

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefano-sarioli commented Sep 28, 2020 •

edited

Loading