Merge pull request #566 from jhawthorn/packer_dont_raise_none_on_hand

Raise InsufficientStock if and only if there is insufficient stock

CHANGELOG.md

@@ -42,6 +42,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)
 

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

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

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

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,119 @@ 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 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 }
+            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

core/spec/models/spree/stock/packer_spec.rb

@@ -41,20 +41,20 @@ 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
             stock_location.stock_items.update_all(backorderable: false)
             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