[RFC] Add FulfillmentChanger #2070

mamhoff · 2017-07-07T14:44:27Z

(built on top of #1906, without which this does not work either)

This class takes two shipments and moves a certain amount of stuff
between them. This used to be called "splitting" a shipment, but really we
are changing fulfillment for those variants, hence the name.

This will replace the transfer_to_* methods on Spree::Shipment.

It is designed to be quite a bit faster, since it does not move inventory units one by one, but in bulk.

There's a slight change in behaviour: When splitting a shipment such that the new shipment would have more inventory than the original one, that's not possible anymore, because I can't imagine a situation where that would be beneficial.

I had to adjust one spec for the aforementioned change in behaviour. I also had to amend one spec so that the order starts out with only on_hand inventory units.

houndci-bot · 2017-07-07T14:44:35Z

core/spec/models/spree/fulfilment_changer_spec.rb

+    end
+
+    context "if the desired shipment is invalid" do
+      let(:desired_shipment) { order.shipments.build(stock_location_id: 99999999) }


Use underscores(_) as decimal mark and separate every 3 digits with them.

houndci-bot · 2017-07-07T14:44:35Z

core/app/models/spree/fulfilment_changer.rb

+      [current_shipment, desired_shipment].map { |s| s.inventory_units.reload }
+
+      # If the current shipment now has no inventory units left, we won't need it any longer.
+      if current_shipment.inventory_units.count == 0


Use current_shipment.inventory_units.count.zero? instead of current_shipment.inventory_units.count == 0.

tvdeyen

Great stuff. The amount of specs alone is worth it. I like the usage of active model validations to render meaningful error responses to the user 👍

I have some small minor nits.

tvdeyen · 2017-07-10T10:49:20Z

core/app/models/spree/fulfilment_changer.rb

+    validate :current_shipment_not_already_shipped
+    validate :desired_shipment_different_from_current
+    validates :desired_stock_location, presence: true
+    validate :enough_stock_at_desired_location, if: :handle_stock_location_counts?


This name confuses me. Maybe just handle_stock? or handle_stock_count?

tvdeyen · 2017-07-10T10:53:23Z

core/app/models/spree/fulfilment_changer.rb

+      return false if invalid?
+      desired_shipment.save! if desired_shipment.new_record?
+
+      current_on_hand_quantity = [current_shipment.inventory_units.on_hand.size, quantity].min


Could we move this query into the if block on line 26?

tvdeyen · 2017-07-10T10:58:27Z

core/app/models/spree/fulfilment_changer.rb

+        limit(new_on_hand_quantity).
+        update_all(shipment_id: desired_shipment.id, state: :on_hand)
+
+      current_shipment.


Could we spare this query if no backordered units are left?

tvdeyen · 2017-07-10T11:22:37Z

api/app/controllers/spree/api/shipments_controller.rb

-        render json: { success: true, message: Spree.t(:shipment_transfer_success) }, status: 201
+        @desired_shipment ||= Spree::Shipment.find_by!(number: params[:target_shipment_number])
+
+        splitter = Spree::FulfilmentChanger.new(


Maybe don't call this var splitter

tvdeyen · 2017-07-10T11:28:29Z

api/app/controllers/spree/api/shipments_controller.rb

+        )
+
+        if splitter.run!
+          render json: { success: true, message: Spree.t(:shipment_transfer_success) }, status: :accepted


This changed status code is maybe worth noting in the change log.

tvdeyen · 2017-07-10T11:38:27Z

backend/app/assets/javascripts/spree/backend/shipments.js

+      if (response.success) {
+        window.Spree.advanceOrder();
+      } else {
+        alert(response.message);


Could we use show_flash instead of an alert here?

Using show_flash resulted in quite a lot of testing trouble for some reason: Locked Sqlite databases, hung phantomjs, weird things. Can we move that to a separate PR?

tvdeyen · 2017-07-10T12:07:46Z

core/app/models/spree/fulfilment_changer.rb

+      current_on_hand_quantity = [current_shipment.inventory_units.on_hand.size, quantity].min
+      new_on_hand_quantity = [desired_shipment.stock_location.count_on_hand(variant), quantity].min
+
+      if handle_stock_location_counts?


Maybe it makes sense to put all the stock and shipment changing code into a transaction to avoid potential stock count mismatches?

tvdeyen · 2017-07-10T12:09:35Z

core/app/models/spree/fulfilment_changer.rb

@@ -0,0 +1,92 @@
+module Spree
+  class FulfilmentChanger


I would like to see some documentation on this class.

jhawthorn · 2017-07-13T18:19:13Z

CHANGELOG.md

@@ -1,5 +1,7 @@
 ## Solidus 2.3.0 (master, unreleased)

+- Change HTTP Status code for Api::ShipmentsController#transfer_to_* to be always 202 Accepted rather than 201 Created or 500.
+  Speed up changing fulfilment for parts of a shipment [\#2070](https://github.com/solidusio/solidus/pull/2070) ([mamhoff](https://github.com/mamhoff))


These will be in 2.4.0

This spec had an implicit expectation of all inventory units of the first shipment to be setup as on hand, but somehow managed to get into that state by what I think is whacky magic. Updating the two items to be on hand at the beginning of the spec makes sure that the remaining item after the first split is also on hand.

This class takes two shipments and moves a certain amount of stuff between them. This used to be called "splitting" a shipment, but really we are changing fulfillment for those variants, hence the name. The class has the following features: 1. Speed It only does four or five requests to the database, instead on one for every inventory unit moved. 2. Refresh rates after changing fulfilment If we change how things are fulfilled, the costs for those fulfilments will change. This change reflects that. 3. Make sure that backordered items are moved first When changing the fulfilment of an item, we want backordered items to be changed to a new shipment first. There, we have a chance of them becoming on_hand (shipped faster). Because "backordered" is earlier in the alphabet than "on_hand", we can use an `order` clause in the update SQL statement to achieve this.

With the new FulfilmentChanger, we can stop using Shipment#tranfer_to_*. This comes with a few minor API changes: The transfer_to_* endpoints now respond with a 202 Accepted instead of 201 Created. They only return 4xx responses if the shipment can not be found or the user is unauthorized. When changing fulfilment for a shipment, the backend now generates readable errors and will not return a 4xx JSON response unless something goes badly wrong. This commit changes the response handling so our JS code knows whether a split has been successful, and displays a meaningful error message to the user.

Let's remove this interface from the codebase.

mamhoff · 2017-07-28T20:40:10Z

Thank you John for updating the changelog. I've had too much on my plate.

houndci-bot reviewed Jul 7, 2017

View reviewed changes

mamhoff force-pushed the introduce-fulfilment-changer branch 3 times, most recently from 2ca373b to 80132ab Compare July 8, 2017 12:37

mamhoff changed the title ~~Work in Progress: Add FulfillmentChanger~~ [RfC] Add FulfillmentChanger Jul 8, 2017

tvdeyen reviewed Jul 10, 2017

View reviewed changes

mamhoff force-pushed the introduce-fulfilment-changer branch 4 times, most recently from f0b432a to 7adba79 Compare July 11, 2017 12:35

tvdeyen approved these changes Jul 11, 2017

View reviewed changes

tvdeyen changed the title ~~[RfC] Add FulfillmentChanger~~ [RFC] Add FulfillmentChanger Jul 12, 2017

jhawthorn added this to the 2.4.0 milestone Jul 12, 2017

jhawthorn reviewed Jul 13, 2017

View reviewed changes

mamhoff added 5 commits July 14, 2017 19:16

Deprecate Spree::Shipment#transfer_to_*

40b6767

Let's remove this interface from the codebase.

Add Changelog entry for changes to shipment fulfilment

aabfaf5

mamhoff force-pushed the introduce-fulfilment-changer branch from 7adba79 to aabfaf5 Compare July 14, 2017 17:17

jhawthorn approved these changes Jul 26, 2017

View reviewed changes

Merge branch 'master' into introduce-fulfilment-changer

b7e73a4

jhawthorn merged commit 22cf072 into solidusio:master Jul 28, 2017

mamhoff deleted the introduce-fulfilment-changer branch July 28, 2017 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Add FulfillmentChanger #2070

[RFC] Add FulfillmentChanger #2070

mamhoff commented Jul 7, 2017 •

edited

Loading

houndci-bot Jul 7, 2017

houndci-bot Jul 7, 2017

tvdeyen left a comment •

edited

Loading

tvdeyen Jul 10, 2017

tvdeyen Jul 10, 2017

tvdeyen Jul 10, 2017

tvdeyen Jul 10, 2017

tvdeyen Jul 10, 2017

tvdeyen Jul 10, 2017

mamhoff Jul 11, 2017

tvdeyen Jul 11, 2017

tvdeyen Jul 10, 2017

tvdeyen Jul 10, 2017

jhawthorn Jul 13, 2017

mamhoff Jul 14, 2017

mamhoff commented Jul 28, 2017

[RFC] Add FulfillmentChanger #2070

[RFC] Add FulfillmentChanger #2070

Conversation

mamhoff commented Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen left a comment • edited Loading

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

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

Choose a reason for hiding this comment

mamhoff commented Jul 28, 2017

mamhoff commented Jul 7, 2017 •

edited

Loading

tvdeyen left a comment •

edited

Loading