From 19c1804b50a927f0547185aa20d1abbe8f5f37f5 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <martin@stembolt.com>
Date: Sat, 8 Jul 2017 20:49:52 +0200
Subject: [PATCH 1/5] Fix Order Details spec

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.
---
 backend/spec/features/admin/orders/order_details_spec.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/backend/spec/features/admin/orders/order_details_spec.rb b/backend/spec/features/admin/orders/order_details_spec.rb
index 9e8579f97d8..3a06f7b624d 100644
--- a/backend/spec/features/admin/orders/order_details_spec.rb
+++ b/backend/spec/features/admin/orders/order_details_spec.rb
@@ -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)

From 7de80e4b9e7fcbeb5d718c618c8210a9c9e79bbf Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <martin@stembolt.com>
Date: Fri, 7 Jul 2017 16:34:00 +0200
Subject: [PATCH 2/5] Add FulfillmentChanger

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.
---
 core/app/models/spree/fulfilment_changer.rb   | 125 ++++++++
 core/config/locales/en.yml                    |  10 +
 .../models/spree/fulfilment_changer_spec.rb   | 273 ++++++++++++++++++
 3 files changed, 408 insertions(+)
 create mode 100644 core/app/models/spree/fulfilment_changer.rb
 create mode 100644 core/spec/models/spree/fulfilment_changer_spec.rb

diff --git a/core/app/models/spree/fulfilment_changer.rb b/core/app/models/spree/fulfilment_changer.rb
new file mode 100644
index 00000000000..747cb86f5a4
--- /dev/null
+++ b/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
diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml
index b2fd2fbfd80..8d0cc49b1e8 100644
--- a/core/config/locales/en.yml
+++ b/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:
diff --git a/core/spec/models/spree/fulfilment_changer_spec.rb b/core/spec/models/spree/fulfilment_changer_spec.rb
new file mode 100644
index 00000000000..fd614a8ebb8
--- /dev/null
+++ b/core/spec/models/spree/fulfilment_changer_spec.rb
@@ -0,0 +1,273 @@
+require 'spec_helper'
+
+RSpec.describe Spree::FulfilmentChanger do
+  let(:variant) { create(:variant) }
+
+  let(:order) do
+    create(
+      :completed_order_with_totals,
+      line_items_attributes: [
+        {
+          quantity: current_shipment_inventory_unit_count,
+          variant: variant
+        }
+      ]
+    )
+  end
+
+  let(:current_shipment) { order.shipments.first }
+  let(:desired_shipment) { order.shipments.create!(stock_location: desired_stock_location) }
+  let(:desired_stock_location) { current_shipment.stock_location }
+
+  let(:shipment_splitter) do
+    described_class.new(
+      current_shipment: current_shipment,
+      desired_shipment: desired_shipment,
+      variant: variant,
+      quantity: quantity
+    )
+  end
+
+  subject { shipment_splitter.run! }
+
+  before do
+    order && desired_shipment
+    variant.stock_items.first.update_column(:count_on_hand, 100)
+  end
+
+  context "when the current shipment has enough inventory units" do
+    let(:current_shipment_inventory_unit_count) { 2 }
+    let(:quantity) { 1 }
+
+    it "adds the desired inventory units to the desired shipment" do
+      expect { subject }.to change { desired_shipment.inventory_units.length }.by(quantity)
+    end
+
+    it "removes the desired inventory units from the current shipment" do
+      expect { subject }.to change { current_shipment.inventory_units.length }.by(-quantity)
+    end
+
+    it "recalculates shipping costs for the current shipment" do
+      expect(current_shipment).to receive(:refresh_rates)
+      subject
+    end
+
+    it "recalculates shipping costs for the new shipment" do
+      expect(desired_shipment).to receive(:refresh_rates)
+      subject
+    end
+
+    context "when transferring to another stock location" do
+      let(:desired_stock_location) { create(:stock_location) }
+      let!(:stock_item) do
+        variant.stock_items.find_or_create_by!(
+          stock_location: desired_stock_location,
+          variant: variant,
+        )
+      end
+
+      before do
+        stock_item.set_count_on_hand(desired_count_on_hand)
+        stock_item.update(backorderable: false)
+      end
+
+      context "when the other stock location has enough stock" do
+        let(:desired_count_on_hand) { 2 }
+
+        it "is marked as a successful transfer" do
+          expect(subject).to be true
+        end
+
+        it "stocks the current stock location back up" do
+          expect { subject }.to change { current_shipment.stock_location.count_on_hand(variant) }.by(quantity)
+        end
+
+        it "unstocks the desired stock location" do
+          expect { subject }.to change { desired_shipment.stock_location.count_on_hand(variant) }.by(-quantity)
+        end
+
+        context "when the order is not completed" do
+          before do
+            allow(order).to receive(:completed?).and_return(false)
+          end
+
+          it "does not stock the current stock location back up" do
+            expect { subject }.not_to change { current_shipment.stock_location.count_on_hand(variant) }
+          end
+
+          it "does not unstock the desired location" do
+            expect { subject }.not_to change { stock_item.count_on_hand }
+          end
+        end
+      end
+
+      context "when the desired stock location can only partially fulfil the quantity" do
+        let(:current_shipment_inventory_unit_count) { 10 }
+        let(:quantity) { 7 }
+        let(:desired_count_on_hand) { 5 }
+
+        before do
+          stock_item.update(backorderable: true)
+        end
+
+        it "restocks five at the original stock location" do
+          expect { subject }.to change { current_shipment.stock_location.count_on_hand(variant) }.by(7)
+        end
+
+        it "unstocks five at the desired stock location" do
+          expect { subject }.to change { desired_shipment.stock_location.count_on_hand(variant) }.by(-5)
+        end
+
+        it "creates a shipment with the correct number of on hand and backordered units" do
+          subject
+          expect(desired_shipment.inventory_units.on_hand.count).to eq(5)
+          expect(desired_shipment.inventory_units.backordered.count).to eq(2)
+        end
+
+        context "when the original shipment has on hand and backordered units" do
+          before do
+            current_shipment.inventory_units.limit(6).update_all(state: :backordered)
+          end
+
+          it "removes the backordered items first" do
+            subject
+            expect(current_shipment.inventory_units.backordered.count).to eq(0)
+            expect(current_shipment.inventory_units.on_hand.count).to eq(3)
+          end
+        end
+
+        context "when the original shipment had some backordered units" do
+          before do
+            current_shipment.inventory_units.limit(6).update_all(state: :backordered)
+          end
+
+          it "restocks four at the original stock location" do
+            expect { subject }.to change { current_shipment.stock_location.count_on_hand(variant) }.by(4)
+          end
+
+          it "unstocks five at the desired stock location" do
+            expect { subject }.to change { desired_shipment.stock_location.count_on_hand(variant) }.by(-5)
+          end
+
+          it "creates a shipment with the correct number of on hand and backordered units" do
+            subject
+            expect(desired_shipment.inventory_units.on_hand.count).to eq(5)
+            expect(desired_shipment.inventory_units.backordered.count).to eq(2)
+          end
+        end
+      end
+
+      context "when the other stock location does not have enough stock" do
+        let(:desired_count_on_hand) { 0 }
+
+        it "is not successful" do
+          expect(subject).to be false
+        end
+
+        it "has an activemodel error hash" do
+          subject
+          expect(shipment_splitter.errors.messages).to eq(desired_shipment: ["not enough stock in desired stock location"])
+        end
+      end
+    end
+
+    context "when the quantity to transfer is not positive" do
+      let(:quantity) { 0 }
+
+      it "is not successful" do
+        expect(subject).to be false
+      end
+
+      it "has an activemodel error hash" do
+        subject
+        expect(shipment_splitter.errors.messages).to eq(quantity: ["must be greater than 0"])
+      end
+    end
+
+    context "when the desired shipment is identical to the current shipment" do
+      let(:desired_shipment) { current_shipment }
+
+      it "is not successful" do
+        expect(subject).to be false
+      end
+
+      it "has an activemodel error hash" do
+        subject
+        expect(shipment_splitter.errors.messages).to eq(desired_shipment: ["can not be same as current shipment"])
+      end
+    end
+
+    context "when the desired shipment has no stock location" do
+      let(:desired_stock_location) { nil }
+
+      it "is not successful" do
+        expect(subject).to be false
+      end
+
+      it "has an activemodel error hash" do
+        subject
+        expect(shipment_splitter.errors.messages).to eq(desired_stock_location: ["can't be blank"])
+      end
+    end
+
+    context "when the current shipment has been shipped already" do
+      let(:order) do
+        create(
+          :shipped_order,
+          line_items_attributes: [
+            {
+              quantity: current_shipment_inventory_unit_count,
+              variant: variant
+            }
+          ]
+        )
+      end
+
+      it "is not successful" do
+        expect(subject).to be false
+      end
+
+      it "has an activemodel error hash" do
+        subject
+        expect(shipment_splitter.errors.messages).to eq(current_shipment: ["has already been shipped"])
+      end
+    end
+  end
+
+  context "when the current shipment is emptied out by the transfer" do
+    let(:current_shipment_inventory_unit_count) { 30 }
+    let(:quantity) { 30 }
+
+    it "adds the desired inventory units to the desired shipment" do
+      expect { subject }.to change { desired_shipment.inventory_units.length }.by(quantity)
+    end
+
+    it "removes the current shipment" do
+      expect { subject }.to change { Spree::Shipment.count }.by(-1)
+    end
+  end
+
+  context "when the desired shipment is not yet persisted" do
+    let(:current_shipment_inventory_unit_count) { 2 }
+    let(:quantity) { 1 }
+
+    let(:desired_shipment) { order.shipments.build(stock_location: current_shipment.stock_location) }
+
+    it "adds the desired inventory units to the desired shipment" do
+      expect { subject }.to change { Spree::Shipment.count }.by(1)
+    end
+
+    context "if the desired shipment is invalid" do
+      let(:desired_shipment) { order.shipments.build(stock_location_id: 99_999_999) }
+
+      it "is not successful" do
+        expect(subject).to be false
+      end
+
+      it "has an activemodel error hash" do
+        subject
+        expect(shipment_splitter.errors.messages).to eq(desired_stock_location: ["can't be blank"])
+      end
+    end
+  end
+end

From e044c784bbb0699e75bcc2e86a28d3df29a7bee4 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <martin@stembolt.com>
Date: Sat, 8 Jul 2017 12:09:10 +0200
Subject: [PATCH 3/5] Use FulfilmentChanger for transfer_to_* endpoints

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.
---
 .../spree/api/shipments_controller.rb         | 23 ++++++++++++++-----
 .../spree/api/shipments_controller_spec.rb    | 17 +++++++++++++-
 .../javascripts/spree/backend/shipments.js    |  8 +++++--
 .../admin/orders/order_details_spec.rb        | 12 +++++-----
 4 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/api/app/controllers/spree/api/shipments_controller.rb b/api/app/controllers/spree/api/shipments_controller.rb
index c715e3d0fc1..db848a9f4a8 100644
--- a/api/app/controllers/spree/api/shipments_controller.rb
+++ b/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
diff --git a/api/spec/requests/spree/api/shipments_controller_spec.rb b/api/spec/requests/spree/api/shipments_controller_spec.rb
index e34051190d8..cca3f196f69 100644
--- a/api/spec/requests/spree/api/shipments_controller_spec.rb
+++ b/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
diff --git a/backend/app/assets/javascripts/spree/backend/shipments.js b/backend/app/assets/javascripts/spree/backend/shipments.js
index 79511974b80..7bb340941ab 100644
--- a/backend/app/assets/javascripts/spree/backend/shipments.js
+++ b/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);
+      };
     });
   },
 
diff --git a/backend/spec/features/admin/orders/order_details_spec.rb b/backend/spec/features/admin/orders/order_details_spec.rb
index 3a06f7b624d..04493ad1c76 100644
--- a/backend/spec/features/admin/orders/order_details_spec.rb
+++ b/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

From 40b6767f2f9e87847b29ca0cb5ce616ebda63c12 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <martin@stembolt.com>
Date: Sat, 8 Jul 2017 12:18:32 +0200
Subject: [PATCH 4/5] Deprecate Spree::Shipment#transfer_to_*

Let's remove this interface from the codebase.
---
 core/app/models/spree/shipment.rb       | 38 +++++++------------------
 core/spec/models/spree/shipment_spec.rb |  4 ++-
 2 files changed, 13 insertions(+), 29 deletions(-)

diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb
index 7a9aea67731..72811152228 100644
--- a/core/app/models/spree/shipment.rb
+++ b/core/app/models/spree/shipment.rb
@@ -321,37 +321,19 @@ def update!(order)
     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?
diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb
index 6eb4f62148e..dcc8c0b5d3a 100644
--- a/core/spec/models/spree/shipment_spec.rb
+++ b/core/spec/models/spree/shipment_spec.rb
@@ -32,7 +32,9 @@
 
       aggregate_failures("verifying new shipment attributes") do
         expect do
-          shipment.transfer_to_location(variant, 1, stock_location)
+          Spree::Deprecation.silence do
+            shipment.transfer_to_location(variant, 1, stock_location)
+          end
         end.to change { Spree::Shipment.count }.by(1)
 
         new_shipment = order.shipments.last

From aabfaf5747425b95825633abf880183e57e2e97c Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <martin@stembolt.com>
Date: Tue, 11 Jul 2017 12:18:43 +0200
Subject: [PATCH 5/5] Add Changelog entry for changes to shipment fulfilment

---
 CHANGELOG.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ccd550d1a5a..2617535779e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,7 @@
 ## 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))
 
 ## Solidus 2.3.0 (unreleased)