Merge pull request #573 from jordan-brough/improve-packer-queries

Add StockItem#fill_status to enable perf improvements

core/app/models/spree/stock/packer.rb

@@ -22,9 +22,10 @@ def default_package
         inventory_units.group_by(&:variant).each do |variant, variant_inventory_units|
           units = variant_inventory_units.clone
           if variant.should_track_inventory?
-            next unless stock_location.stock_item(variant)
+            stock_item = stock_item_for(variant.id)
+            next unless stock_item
 
-            on_hand, backordered = stock_location.fill_status(variant, units.count)
+            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
@@ -37,6 +38,21 @@ def default_package
       end
 
       private
+
+      def stock_item_for(variant_id)
+        stock_item_lookup[variant_id]
+      end
+
+      # Returns a lookup table in the form of:
+      #   {<variant_id> => <stock_item>, ...}
+      def stock_item_lookup
+        @stock_item_lookup ||= Spree::StockItem.
+          where(variant_id: inventory_units.map(&:variant_id).uniq).
+          where(stock_location_id: stock_location.id).
+          map { |stock_item| [stock_item.variant_id, stock_item] }.
+          to_h # there is only one stock item per variant in a stock location
+      end
+
       def build_splitter
         splitter = nil
         splitters.reverse.each do |klass|

core/app/models/spree/stock_item.rb

@@ -70,6 +70,19 @@ def reduce_count_on_hand_to_zero
       self.set_count_on_hand(0) if count_on_hand > 0
     end
 
+    def fill_status(quantity)
+      if count_on_hand >= quantity
+        on_hand = quantity
+        backordered = 0
+      else
+        on_hand = count_on_hand
+        on_hand = 0 if on_hand < 0
+        backordered = backorderable? ? (quantity - on_hand) : 0
+      end
+
+      [on_hand, backordered]
+    end
+
     private
       def verify_count_on_hand?
         count_on_hand_changed? && !backorderable? && (count_on_hand < count_on_hand_was) && (count_on_hand < 0)

core/app/models/spree/stock_location.rb

@@ -95,17 +95,7 @@ def move(variant, quantity, originator = nil)
 
     def fill_status(variant, quantity)
       if item = stock_item(variant)
-
-        if item.count_on_hand >= quantity
-          on_hand = quantity
-          backordered = 0
-        else
-          on_hand = item.count_on_hand
-          on_hand = 0 if on_hand < 0
-          backordered = item.backorderable? ? (quantity - on_hand) : 0
-        end
-
-        [on_hand, backordered]
+        item.fill_status(quantity)
       else
         [0, 0]
       end

core/db/migrate/20151219020209_add_stock_item_unique_index.rb

@@ -0,0 +1,9 @@
+class AddStockItemUniqueIndex < ActiveRecord::Migration
+  def change
+    # Add a database-level uniqueness constraint for databases that support it
+    # (postgres & sqlite)
+    if connection.adapter_name =~ /postgres|sqlite/i
+      add_index 'spree_stock_items', ['variant_id', 'stock_location_id'], where: 'deleted_at is null', unique: true
+    end
+  end
+end

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

@@ -28,9 +28,8 @@ module Stock
         end
 
         it 'variants are added as backordered without enough on_hand' do
-          expect(stock_location).to receive(:fill_status).exactly(5).times.and_return(
-            *(Array.new(3, [1,0]) + Array.new(2, [0,1]))
-          )
+          inventory_units[0..2].each { |iu| stock_location.stock_item(iu.variant_id).set_count_on_hand(1) }
+          inventory_units[3..4].each { |iu| stock_location.stock_item(iu.variant_id).set_count_on_hand(0) }
 
           package = subject.default_package
           expect(package.on_hand.size).to eq 3

core/spec/models/spree/stock_location_spec.rb

@@ -170,10 +170,7 @@ module Spree
       end
 
       it 'zero on_hand with all backordered' do
-        zero_stock_item = mock_model(StockItem,
-                                     count_on_hand: 0,
-                                     backorderable?: true)
-        expect(subject).to receive(:stock_item).with(variant).and_return(zero_stock_item)
+        stock_item.set_count_on_hand(0)
 
         on_hand, backordered = subject.fill_status(variant, 20)
         expect(on_hand).to eq 0
@@ -182,28 +179,27 @@ module Spree
 
       context 'when backordering is not allowed' do
         before do
-          @stock_item = mock_model(StockItem, backorderable?: false)
-          expect(subject).to receive(:stock_item).with(variant).and_return(@stock_item)
+          stock_item.update!(backorderable: false)
         end
 
         it 'all on_hand' do
-          allow(@stock_item).to receive_messages(count_on_hand: 10)
+          stock_item.set_count_on_hand(10)
 
           on_hand, backordered = subject.fill_status(variant, 5)
           expect(on_hand).to eq 5
           expect(backordered).to eq 0
         end
 
         it 'some on_hand' do
-          allow(@stock_item).to receive_messages(count_on_hand: 10)
+          stock_item.set_count_on_hand(10)
 
           on_hand, backordered = subject.fill_status(variant, 20)
           expect(on_hand).to eq 10
           expect(backordered).to eq 0
         end
 
         it 'zero on_hand' do
-          allow(@stock_item).to receive_messages(count_on_hand: 0)
+          stock_item.set_count_on_hand(0)
 
           on_hand, backordered = subject.fill_status(variant, 20)
           expect(on_hand).to eq 0