diff --git a/CHANGELOG.md b/CHANGELOG.md index b452e61238c..fe25ece1746 100644 --- a/CHANGELOG.md +++ b/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)) 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 9e8579f97d8..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 @@ -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) 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/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb index 32e6f9671a3..0aa0737561e 100644 --- a/core/app/models/spree/shipment.rb +++ b/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? diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index b7bc085fbb0..caebe5b8cca 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 diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb index ed064f66ef4..7ad68ae4929 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