Merge pull request #2070 from mamhoff/introduce-fulfilment-changer

[RFC] Add FulfillmentChanger

CHANGELOG.md

@@ -1,7 +1,11 @@
 ## Solidus 2.4.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))
+
 - Customized responders have been removed. They are available in the `solidus_responders` extension
 
+
 ## Solidus 2.3.0 (unreleased)
 
 - Rails 5.1 [\#1895](https://github.com/solidusio/solidus/pull/1895) ([jhawthorn](https://github.com/jhawthorn))

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

@@ -81,15 +81,26 @@ def remove
       end
 
       def transfer_to_location
-        @stock_location = Spree::StockLocation.find(params[:stock_location_id])
-        @original_shipment.transfer_to_location(@variant, @quantity, @stock_location)
-        render json: { success: true, message: Spree.t(:shipment_transfer_success) }, status: 201
+        @desired_stock_location = Spree::StockLocation.find(params[:stock_location_id])
+        @desired_shipment = @original_shipment.order.shipments.build(stock_location: @desired_stock_location)
+        transfer_to_shipment
       end
 
       def transfer_to_shipment
-        @target_shipment = Spree::Shipment.find_by!(number: params[:target_shipment_number])
-        @original_shipment.transfer_to_shipment(@variant, @quantity, @target_shipment)
-        render json: { success: true, message: Spree.t(:shipment_transfer_success) }, status: 201
+        @desired_shipment ||= Spree::Shipment.find_by!(number: params[:target_shipment_number])
+
+        fulfilment_changer = Spree::FulfilmentChanger.new(
+          current_shipment: @original_shipment,
+          desired_shipment: @desired_shipment,
+          variant: @variant,
+          quantity: @quantity
+        )
+
+        if fulfilment_changer.run!
+          render json: { success: true, message: Spree.t(:shipment_transfer_success) }, status: :accepted
+        else
+          render json: { success: false, message: fulfilment_changer.errors.full_messages.to_sentence }, status: :accepted
+        end
       end
 
       private

api/spec/requests/spree/api/shipments_controller_spec.rb

@@ -354,6 +354,21 @@
         end
       end
 
+      context "for an unsuccessful transfer" do
+        before do
+          source_shipment
+          variant
+          stock_location.stock_items.update_all(backorderable: false)
+        end
+
+        it "returns the correct message" do
+          subject
+          expect(response).to be_accepted
+          expect(parsed_response["success"]).to be false
+          expect(parsed_response["message"]).to eq("Desired shipment not enough stock in desired stock location")
+        end
+      end
+
       context "if the source shipment can not be found" do
         let(:stock_location_id) { 9999 }
 
@@ -422,7 +437,7 @@
 
         it "returns the correct message" do
           subject
-          expect(response).to be_success
+          expect(response).to be_accepted
           expect(parsed_response["success"]).to be true
           expect(parsed_response["message"]).to eq("Variants successfully transferred")
         end

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

@@ -189,8 +189,12 @@ var ShipmentSplitItemView = Backbone.View.extend({
     }
     jqXHR.error(function(msg) {
       alert(Spree.t("split_failed"));
-    }).done(function() {
-      window.Spree.advanceOrder();
+    }).done(function(response) {
+      if (response.success) {
+        window.Spree.advanceOrder();
+      } else {
+        alert(response.message);
+      };
     });
   },
 

backend/spec/features/admin/orders/order_details_spec.rb

@@ -203,7 +203,7 @@
             expect(order.shipments.first.stock_location.id).to eq(stock_location2.id)
           end
 
-          it 'should allow me to split more than I have if available there' do
+          it 'should not allow me to split more than I had in the original shipment' do
             expect(order.shipments.first.stock_location.id).to eq(stock_location.id)
 
             within_row(1) { click_icon 'arrows-h' }
@@ -213,7 +213,7 @@
 
             expect(order.shipments.count).to eq(1)
             expect(order.shipments.last.backordered?).to eq(false)
-            expect(order.shipments.first.inventory_units_for(product.master).count).to eq(5)
+            expect(order.shipments.first.inventory_units_for(product.master).count).to eq(2)
             expect(order.shipments.first.stock_location.id).to eq(stock_location2.id)
           end
 
@@ -222,7 +222,7 @@
 
             within_row(1) { click_icon 'arrows-h' }
 
-            accept_alert "Unable to complete split" do
+            accept_alert "Quantity must be greater than 0" do
               complete_split_to(stock_location2, quantity: 0)
             end
 
@@ -232,7 +232,7 @@
 
             fill_in 'item_quantity', with: -1
 
-            accept_alert "Unable to complete split" do
+            accept_alert "Quantity must be greater than 0" do
               click_icon :ok
             end
 
@@ -265,7 +265,7 @@
               within_row(1) { click_icon 'arrows-h' }
               complete_split_to(stock_location2, quantity: 2)
 
-              accept_alert "Unable to complete split"
+              accept_alert "Desired shipment not enough stock in desired stock location"
 
               expect(order.shipments.count).to eq(1)
               expect(order.shipments.first.inventory_units_for(product.master).count).to eq(2)
@@ -364,7 +364,7 @@
             within(all('.stock-contents', count: 2).first) do
               within_row(1) { click_icon 'arrows-h' }
 
-              accept_alert("Unable to complete split") do
+              accept_alert("Desired shipment not enough stock in desired stock location") do
                 complete_split_to(@shipment2, quantity: 200)
               end
             end
@@ -404,6 +404,7 @@
 
         context 'receiving shipment can backorder' do
           it 'should add more to the backorder' do
+            @shipment1.inventory_units.update_all(state: :on_hand)
             product.master.stock_items.last.update_column(:backorderable, true)
             product.master.stock_items.last.update_column(:count_on_hand, 0)
             expect(@shipment2.reload.backordered?).to eq(false)

core/app/models/spree/fulfilment_changer.rb

@@ -0,0 +1,125 @@
+module Spree
+  # Service class to change fulfilment of inventory units of a particular variant
+  # to another shipment. The other shipment would typically have a different
+  # shipping method, stock location or delivery date, such that we actually change
+  # the planned fulfilment for the items in question.
+  #
+  # Can be used to merge shipments by moving all items to another shipment, because
+  # this class will delete any empty original shipment.
+  #
+  # @attr [Spree::Shipment] current_shipment The shipment we transfer units from
+  # @attr [Spree::Shipment] desired_shipment The shipment we want to move units onto
+  # @attr [Spree::StockLocation] current_stock_location The stock location of the current shipment
+  # @attr [Spree::StockLocation] desired_stock_location The stock location of the desired shipment
+  # @attr [Spree::Variant] variant We only move units that represent this variant
+  # @attr [Integer] quantity How many units we want to move
+  #
+  class FulfilmentChanger
+    include ActiveModel::Validations
+
+    attr_accessor :current_shipment, :desired_shipment
+    attr_reader :variant, :quantity, :current_stock_location, :desired_stock_location
+
+    def initialize(current_shipment:, desired_shipment:, variant:, quantity:)
+      @current_shipment = current_shipment
+      @desired_shipment = desired_shipment
+      @current_stock_location = current_shipment.stock_location
+      @desired_stock_location = desired_shipment.stock_location
+      @variant = variant
+      @quantity = quantity
+    end
+
+    validates :quantity, numericality: { greater_than: 0 }
+    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_counts?
+
+    # Performs the change of fulfilment
+    # @return [true, false] Whether the requested fulfilment change was successful
+    def run!
+      # Validations here are intended to catch all necessary prerequisites.
+      # We return early so all checks have happened already.
+      return false if invalid?
+      desired_shipment.save! if desired_shipment.new_record?
+
+      new_on_hand_quantity = [desired_shipment.stock_location.count_on_hand(variant), quantity].min
+
+      ActiveRecord::Base.transaction do
+        if handle_stock_counts?
+          # We only run this query if we need it.
+          current_on_hand_quantity = [current_shipment.inventory_units.on_hand.size, quantity].min
+
+          # Restock things we will not fulfil from the current shipment anymore
+          current_stock_location.restock(variant, current_on_hand_quantity, current_shipment)
+          # Unstock what we will fulfil with the new shipment
+          desired_stock_location.unstock(variant, new_on_hand_quantity, desired_shipment)
+        end
+
+        # These two statements are the heart of this class. We change the number
+        # of inventory units requested from one shipment to the other.
+        # We order by state, because `'backordered' < 'on_hand'`.
+        current_shipment.
+          inventory_units.
+          where(variant: variant).
+          order(state: :asc).
+          limit(new_on_hand_quantity).
+          update_all(shipment_id: desired_shipment.id, state: :on_hand)
+
+        current_shipment.
+          inventory_units.
+          where(variant: variant).
+          order(state: :asc).
+          limit(quantity - new_on_hand_quantity).
+          update_all(shipment_id: desired_shipment.id, state: :backordered)
+      end
+
+      # We modified the inventory units at the database level for speed reasons.
+      # The downside of that is that we need to reload the associations.
+      current_shipment.inventory_units.reload
+      desired_shipment.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.length.zero?
+        current_shipment.destroy!
+      else
+        # The current shipment has changed, so we need to make sure that shipping rates
+        # have the correct amount.
+        current_shipment.refresh_rates
+      end
+
+      # The desired shipment has also change, so we need to make sure shipping rates
+      # are up-to-date, too.
+      desired_shipment.refresh_rates
+
+      true
+    end
+
+    private
+
+    # We don't need to handle stock counts for incomplete orders. Also, if
+    # the new shipment and the desired shipment will ship from the same stock location,
+    # unstocking and restocking will not be necessary.
+    def handle_stock_counts?
+      current_shipment.order.completed? && current_stock_location != desired_stock_location
+    end
+
+    def current_shipment_not_already_shipped
+      if current_shipment.shipped?
+        errors.add(:current_shipment, :has_already_been_shipped)
+      end
+    end
+
+    def enough_stock_at_desired_location
+      unless Spree::Stock::Quantifier.new(variant, desired_stock_location).can_supply?(quantity)
+        errors.add(:desired_shipment, :not_enough_stock_at_desired_location)
+      end
+    end
+
+    def desired_shipment_different_from_current
+      if desired_shipment.id == current_shipment.id
+        errors.add(:desired_shipment, :can_not_transfer_within_same_shipment)
+      end
+    end
+  end
+end

core/app/models/spree/shipment.rb

@@ -334,37 +334,19 @@ def update!(order_or_attrs)
     end
 
     def transfer_to_location(variant, quantity, stock_location)
-      if quantity <= 0
-        raise ArgumentError
-      end
-
-      transaction do
-        new_shipment = order.shipments.create!(stock_location: stock_location)
-
-        order.contents.remove(variant, quantity, { shipment: self })
-        order.contents.add(variant, quantity, { shipment: new_shipment })
-
-        refresh_rates
-        new_shipment.refresh_rates
-        save!
-        new_shipment.save!
-      end
+      Spree::Deprecation.warn("Please use the Spree::FulfilmentChanger class instead of Spree::Shipment#transfer_to_location", caller)
+      new_shipment = order.shipments.create!(stock_location: stock_location)
+      transfer_to_shipment(variant, quantity, new_shipment)
     end
 
     def transfer_to_shipment(variant, quantity, shipment_to_transfer_to)
-      if quantity <= 0 || self == shipment_to_transfer_to
-        raise ArgumentError
-      end
-
-      transaction do
-        order.contents.remove(variant, quantity, { shipment: self })
-        order.contents.add(variant, quantity, { shipment: shipment_to_transfer_to })
-
-        refresh_rates
-        save!
-        shipment_to_transfer_to.refresh_rates
-        shipment_to_transfer_to.save!
-      end
+      Spree::Deprecation.warn("Please use the Spree::FulfilmentChanger class instead of Spree::Shipment#transfer_to_location", caller)
+      Spree::FulfilmentChanger.new(
+        current_shipment: self,
+        desired_shipment: shipment_to_transfer_to,
+        variant: variant,
+        quantity: quantity
+      ).run!
     end
 
     def requires_shipment?

core/config/locales/en.yml

@@ -6,6 +6,16 @@ en:
         state: State
         shipment: Shipment
         cancel: Cancel
+    errors:
+      models:
+        spree/fulfilment_changer:
+          attributes:
+            desired_shipment:
+              can_not_transfer_within_same_shipment: can not be same as current shipment
+              not_enough_stock_at_desired_location: not enough stock in desired stock location
+            current_shipment:
+              has_already_been_shipped: has already been shipped
+              can_not_have_backordered_inventory_units: has backordered inventory units
   activerecord:
     attributes:
       spree/address: