From 9603497d50f21b7c4917f532b4440170cac109ca Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 21 Dec 2015 14:00:45 -0800 Subject: [PATCH 1/4] Stock::Packer should not raise InsufficientStock Stock::Packer currently raises InsufficientStock under specific circumstances: * There is a stock item for a variant in the stock location. * There is exactly 0 inventory remaining for that variant in the stock location. * The variant does not allow backordering. This is odd, because it doesn't raise InsufficientStock when there is some but insufficient inventory (it adds the amount available to the package), or if the stock item doesn't exist at all in that stock location. These three cases should probably be handled in the same way. The way the Packer is used is to build the fullest possible package for this stock location. As it's possible to fulfill an variant half from one location and half from another, this method should not be raising InsufficientStock because it has insufficient information to do so. --- core/app/models/spree/stock/packer.rb | 1 - core/spec/models/spree/stock/packer_spec.rb | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/core/app/models/spree/stock/packer.rb b/core/app/models/spree/stock/packer.rb index 1a24b4fd9ca..13b021d499a 100644 --- a/core/app/models/spree/stock/packer.rb +++ b/core/app/models/spree/stock/packer.rb @@ -26,7 +26,6 @@ def default_package next unless stock_item on_hand, backordered = stock_item.fill_status(units.count) - raise Spree::Order::InsufficientStock unless on_hand > 0 || backordered > 0 package.add_multiple units.slice!(0, on_hand), :on_hand if on_hand > 0 package.add_multiple units.slice!(0, backordered), :backordered if backordered > 0 else diff --git a/core/spec/models/spree/stock/packer_spec.rb b/core/spec/models/spree/stock/packer_spec.rb index e8c78ae18ab..d328bfa6043 100644 --- a/core/spec/models/spree/stock/packer_spec.rb +++ b/core/spec/models/spree/stock/packer_spec.rb @@ -41,11 +41,11 @@ module Stock let(:packer) { Packer.new(stock_location, inventory_units) } it "builds an empty package" do - expect(packer.default_package.contents).to be_empty + expect(packer.default_package).to be_empty end end - context "not enough on hand and not backorderable" do + context "none on hand and not backorderable" do let(:packer) { Packer.new(stock_location, inventory_units) } before do @@ -53,8 +53,8 @@ module Stock stock_location.stock_items.each { |si| si.set_count_on_hand(0) } end - it "raises an error" do - expect { packer.default_package }.to raise_error Spree::Order::InsufficientStock + it "builds an empty package" do + expect(packer.default_package).to be_empty end end From d3e5cedaf2b3ce2c0d28b68f6d55b23019985479 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Fri, 4 Dec 2015 15:43:36 -0800 Subject: [PATCH 2/4] Coordinator to raise InsufficientStock on error Instead of raising this inside of the Packer (and only in some cases), raise it any time our final package didn't contain the full requsted quantity. --- core/app/models/spree/exchange.rb | 5 +- core/app/models/spree/stock/coordinator.rb | 11 ++ .../models/spree/stock/coordinator_spec.rb | 103 +++++++++++++++++- 3 files changed, 115 insertions(+), 4 deletions(-) diff --git a/core/app/models/spree/exchange.rb b/core/app/models/spree/exchange.rb index b6e87cbf2bc..0bc602c8000 100644 --- a/core/app/models/spree/exchange.rb +++ b/core/app/models/spree/exchange.rb @@ -18,8 +18,9 @@ def display_amount end def perform! - shipments = Spree::Stock::Coordinator.new(@order, @reimbursement_objects.map(&:build_exchange_inventory_unit)).shipments - if shipments.flat_map(&:inventory_units).size != @reimbursement_objects.size + begin + shipments = Spree::Stock::Coordinator.new(@order, @reimbursement_objects.map(&:build_exchange_inventory_unit)).shipments + rescue Spree::Order::InsufficientStock raise UnableToCreateShipments.new("Could not generate shipments for all items. Out of stock?") end @order.shipments += shipments diff --git a/core/app/models/spree/stock/coordinator.rb b/core/app/models/spree/stock/coordinator.rb index af92d033f8f..70d6a541d00 100644 --- a/core/app/models/spree/stock/coordinator.rb +++ b/core/app/models/spree/stock/coordinator.rb @@ -20,6 +20,8 @@ def packages packages = build_packages(packages) packages = prioritize_packages(packages) packages = estimate_packages(packages) + validate_packages(packages) + packages end # Build packages for the inventory units that have preferred stock locations first @@ -127,6 +129,15 @@ def estimate_packages(packages) packages end + def validate_packages(packages) + desired_quantity = inventory_units.size + packaged_quantity = packages.sum(&:quantity) + if packaged_quantity != desired_quantity + raise Spree::Order::InsufficientStock, + "Was only able to package #{packaged_quantity} inventory units of #{desired_quantity} requested" + end + end + def build_packer(stock_location, inventory_units) Packer.new(stock_location, inventory_units, splitters(stock_location)) end diff --git a/core/spec/models/spree/stock/coordinator_spec.rb b/core/spec/models/spree/stock/coordinator_spec.rb index ab107f2bcb9..fc80001ad9e 100644 --- a/core/spec/models/spree/stock/coordinator_spec.rb +++ b/core/spec/models/spree/stock/coordinator_spec.rb @@ -3,7 +3,7 @@ module Spree module Stock describe Coordinator, :type => :model do - let!(:order) { create(:order_with_line_items, line_items_count: 2) } + let(:order) { create(:order_with_line_items, line_items_count: 2) } subject { Coordinator.new(order) } @@ -13,6 +13,7 @@ module Stock expect(subject).to receive(:build_packages).ordered expect(subject).to receive(:prioritize_packages).ordered expect(subject).to receive(:estimate_packages).ordered + expect(subject).to receive(:validate_packages).ordered subject.packages end @@ -91,9 +92,107 @@ module Stock end end + context "with no backordering" do + let!(:stock_location_1) { create(:stock_location, propagate_all_variants: false, active: true) } + + let!(:variant) { create(:variant, track_inventory: true) } + + before do + stock_item1 = variant.stock_items.create!(stock_location: stock_location_1, backorderable: false) + stock_item1.set_count_on_hand(location_1_inventory) + end + + let!(:order) { create(:order) } + let!(:line_item) { create(:line_item, order: order, variant: variant, quantity: 5) } + before { order.reload } + let(:packages) { subject.packages } + + shared_examples "a fulfillable package" do + it "packages correctly" do + expect(packages).not_to be_empty + expect(packages.map(&:quantity).sum).to eq(5) + expect(packages.flat_map(&:contents).map(&:inventory_unit).uniq.size).to eq(5) + end + end + + shared_examples "an unfulfillable package" do + it "raises exception" do + expect{ packages }.to raise_error(Spree::Order::InsufficientStock) + end + end + + context 'with no stock locations' do + let(:location_1_inventory) { 0 } + before { variant.stock_items.destroy_all } + it_behaves_like "an unfulfillable package" + end + + context 'with a single stock location' do + context "with no inventory" do + let(:location_1_inventory) { 0 } + it_behaves_like "an unfulfillable package" + end + + context "with insufficient inventory" do + let(:location_1_inventory) { 1 } + it_behaves_like "an unfulfillable package" + end + + context "with sufficient inventory" do + let(:location_1_inventory) { 5 } + it_behaves_like "a fulfillable package" + end + end + + context 'with two stock locations' do + let!(:stock_location_2) { create(:stock_location, propagate_all_variants: false, active: true) } + before do + stock_item2 = variant.stock_items.create!(stock_location: stock_location_2, backorderable: false) + stock_item2.set_count_on_hand(location_2_inventory) + end + + context "with no inventory" do + let(:location_1_inventory) { 0 } + let(:location_2_inventory) { 0 } + it_behaves_like "an unfulfillable package" + end + + context "with some but insufficient inventory in each location" do + let(:location_1_inventory) { 1 } + let(:location_2_inventory) { 1 } + it_behaves_like "an unfulfillable package" + end + + context "has sufficient inventory in the first location" do + let(:location_1_inventory) { 5 } + let(:location_2_inventory) { 0 } + it_behaves_like "a fulfillable package" + end + + context "has sufficient inventory in the second location" do + let(:location_1_inventory) { 0 } + let(:location_2_inventory) { 5 } + it_behaves_like "a fulfillable package" + end + + context "with sufficient inventory across both locations" do + let(:location_1_inventory) { 2 } + let(:location_2_inventory) { 3 } + before { pending "This is broken. The coordinator packages this incorrectly" } + it_behaves_like "a fulfillable package" + end + + context "with sufficient inventory in both locations" do + let(:location_1_inventory) { 5 } + let(:location_2_inventory) { 5 } + it_behaves_like "a fulfillable package" + end + end + end + context "build location configured packages" do context "there are configured stock locations" do - let(:stock_location) { order.variants.first.stock_locations.first } + let!(:stock_location) { order.variants.first.stock_locations.first } let!(:stock_location_2) { create(:stock_location) } before do From 2ad32e37db9ef956f12739568438cb17b320baf1 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Tue, 15 Dec 2015 13:42:55 -0800 Subject: [PATCH 3/4] Add additional coordinator tests Since the previous spec having stock locations with insufficient stock failed (and is marked pending) I've added two more examples which do pass. --- core/spec/models/spree/stock/coordinator_spec.rb | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/core/spec/models/spree/stock/coordinator_spec.rb b/core/spec/models/spree/stock/coordinator_spec.rb index fc80001ad9e..f2cbfa861b7 100644 --- a/core/spec/models/spree/stock/coordinator_spec.rb +++ b/core/spec/models/spree/stock/coordinator_spec.rb @@ -175,13 +175,25 @@ module Stock it_behaves_like "a fulfillable package" end - context "with sufficient inventory across both locations" do + context "with sufficient inventory only across both locations" do let(:location_1_inventory) { 2 } let(:location_2_inventory) { 3 } before { pending "This is broken. The coordinator packages this incorrectly" } it_behaves_like "a fulfillable package" end + context "has sufficient inventory in the second location and some in the first" do + let(:location_1_inventory) { 2 } + let(:location_2_inventory) { 5 } + it_behaves_like "a fulfillable package" + end + + context "has sufficient inventory in the first location and some in the second" do + let(:location_1_inventory) { 5 } + let(:location_2_inventory) { 2 } + it_behaves_like "a fulfillable package" + end + context "with sufficient inventory in both locations" do let(:location_1_inventory) { 5 } let(:location_2_inventory) { 5 } From c1bac984384d024b6a2ed4c0c07e1f04a06673ab Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Tue, 12 Jan 2016 17:05:43 -0800 Subject: [PATCH 4/4] Add changelog entry for InsufficientStock change --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5712bfbd35..8ac55a59bf9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,15 @@ * Replaced admin taxon management interface [#569](https://github.com/solidusio/solidus/pull/569) +* Fix logic around raising InsufficientStock when creating shipments. [#566](https://github.com/solidusio/solidus/pull/566) + + Previously, `InsufficientStock` was raised if any StockLocations were fully + out of inventory. This was incorrect because it was possible other stock + locations could have fulfilled the inventory. This was also incorrect because + the stock location could have some, but insufficient inventory, and not raise + the exception (an incomplete package would be returned). Now the coordinator + checks that the package is complete and raises `InsufficientStock` if it is + incomplete for any reason. ## Solidus 1.1.0 (2015-11-25)