From 658a0488b2caee7b8ba0f32eebf434ef3534446c Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Wed, 25 Jan 2017 15:50:06 -0800 Subject: [PATCH 1/8] Simplify Stock::Quantifier query building We can use ActiveRecord to build queries instead of a hash, which allows us to reuse StockLocation.active and makes it a little more readable. --- core/app/models/spree/stock/quantifier.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/core/app/models/spree/stock/quantifier.rb b/core/app/models/spree/stock/quantifier.rb index 52c6d446dfb..ed1f8e6aa03 100644 --- a/core/app/models/spree/stock/quantifier.rb +++ b/core/app/models/spree/stock/quantifier.rb @@ -5,13 +5,12 @@ class Quantifier def initialize(variant, stock_location = nil) @variant = variant - where_args = { variant_id: @variant } + @stock_items = Spree::StockItem.joins(:stock_location).where(variant_id: variant) if stock_location - where_args[:stock_location] = stock_location + @stock_items.where!(stock_location: stock_location) else - where_args[Spree::StockLocation.table_name] = { active: true } + @stock_items.merge!(Spree::StockLocation.active) end - @stock_items = Spree::StockItem.joins(:stock_location).where(where_args) end # Returns the total number of inventory units on hand for the variant. From 3953098ad1cfe8c2e106b5ca7b0ed977c6948260 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 26 Jan 2017 11:26:42 -0800 Subject: [PATCH 2/8] Add failing specs on AvailabilityValidator These specs exibit two issues when a line item is split across multiple shipments. If the line item is split across shipments, any availabilty errors will be reported twice. If a shipment is split across shipments with the same stock location, it won't detect insufficient stock if both shipments are able to separately fulfill the inventory. --- .../stock/availability_validator_spec.rb | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/core/spec/models/spree/stock/availability_validator_spec.rb b/core/spec/models/spree/stock/availability_validator_spec.rb index d32fa39a717..29b8c0dd15c 100644 --- a/core/spec/models/spree/stock/availability_validator_spec.rb +++ b/core/spec/models/spree/stock/availability_validator_spec.rb @@ -87,6 +87,34 @@ module Stock end end end + + context "line_item is split across two shipments" do + let!(:order) { create(:order_with_line_items) } + let(:line_item) { order.line_items.first } + let(:variant) { line_item.variant } + let(:stock_location) { order.shipments.first.stock_location } + + before do + shipment2 = order.shipments.create!(stock_location: order.shipments.first.stock_location) + order.contents.add(variant, 1, shipment: shipment2) + variant.stock_items.first.update_columns(count_on_hand: count_on_hand, backorderable: false) + end + + context "and there is just enough stock" do + let(:count_on_hand) { 2 } + include_examples "passes validation" + end + + context "and there is not enough stock", pending: true do + let(:count_on_hand) { 1 } + include_examples "fails validation" + end + + context "and there is no available stock", skip: true do + let(:count_on_hand) { 0 } + include_examples "fails validation" + end + end end end end From 8f35cd151e335a817b6c0690cb5ea4ed2075aa87 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 26 Jan 2017 11:57:12 -0800 Subject: [PATCH 3/8] Avoid unnecessary join in Stock::Quantifier --- core/app/models/spree/stock/quantifier.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/stock/quantifier.rb b/core/app/models/spree/stock/quantifier.rb index ed1f8e6aa03..4c8f841e553 100644 --- a/core/app/models/spree/stock/quantifier.rb +++ b/core/app/models/spree/stock/quantifier.rb @@ -5,11 +5,11 @@ class Quantifier def initialize(variant, stock_location = nil) @variant = variant - @stock_items = Spree::StockItem.joins(:stock_location).where(variant_id: variant) + @stock_items = Spree::StockItem.where(variant_id: variant) if stock_location @stock_items.where!(stock_location: stock_location) else - @stock_items.merge!(Spree::StockLocation.active) + @stock_items.joins!(:stock_location).merge!(Spree::StockLocation.active) end end From 6fc4dad5f356e0e76e6137942332719b7d7b2965 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 26 Jan 2017 12:50:09 -0800 Subject: [PATCH 4/8] Validate inventory by stock_location, not shipment Previously stock was checked per-shipment which could lead to incorrect results as the same inventory from one stock location could be double-counted across multiple shipments. This also greatly reduces the number of queries, by replacing one Shipment load per inventory_unit with a single query to get the quantity of inventory units per stock location. --- core/app/models/spree/inventory_unit.rb | 1 + core/app/models/spree/stock/availability_validator.rb | 10 +++++----- .../models/spree/stock/availability_validator_spec.rb | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/core/app/models/spree/inventory_unit.rb b/core/app/models/spree/inventory_unit.rb index 7dbd577da6e..c9440cd87ea 100644 --- a/core/app/models/spree/inventory_unit.rb +++ b/core/app/models/spree/inventory_unit.rb @@ -21,6 +21,7 @@ class InventoryUnit < Spree::Base before_destroy :ensure_can_destroy + scope :pending, -> { where pending: true } scope :backordered, -> { where state: 'backordered' } scope :on_hand, -> { where state: 'on_hand' } scope :pre_shipment, -> { where(state: PRE_SHIPMENT_STATES) } diff --git a/core/app/models/spree/stock/availability_validator.rb b/core/app/models/spree/stock/availability_validator.rb index 399ed823cf1..b6f47e51b2b 100644 --- a/core/app/models/spree/stock/availability_validator.rb +++ b/core/app/models/spree/stock/availability_validator.rb @@ -2,14 +2,14 @@ module Spree module Stock class AvailabilityValidator < ActiveModel::Validator def validate(line_item) - units_by_shipment = line_item.inventory_units.group_by(&:shipment) + quantity_by_stock_location_id = line_item.inventory_units.pending.joins(:shipment).group(:stock_location_id).count - if units_by_shipment.blank? + if quantity_by_stock_location_id.blank? ensure_in_stock(line_item, line_item.quantity) else - units_by_shipment.each do |shipment, inventory_units| - inventory_units.select!(&:pending?) - ensure_in_stock(line_item, inventory_units.size, shipment.stock_location) + stock_locations = Spree::StockLocation.where(id: quantity_by_stock_location_id.keys).to_a + stock_locations.each do |stock_location| + ensure_in_stock(line_item, quantity_by_stock_location_id[stock_location.id], stock_location) end end diff --git a/core/spec/models/spree/stock/availability_validator_spec.rb b/core/spec/models/spree/stock/availability_validator_spec.rb index 29b8c0dd15c..3209f6c8762 100644 --- a/core/spec/models/spree/stock/availability_validator_spec.rb +++ b/core/spec/models/spree/stock/availability_validator_spec.rb @@ -105,12 +105,12 @@ module Stock include_examples "passes validation" end - context "and there is not enough stock", pending: true do + context "and there is not enough stock" do let(:count_on_hand) { 1 } include_examples "fails validation" end - context "and there is no available stock", skip: true do + context "and there is no available stock" do let(:count_on_hand) { 0 } include_examples "fails validation" end From e6163f25f8be329ad112214ad937dd49feaffe3b Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 26 Jan 2017 12:05:17 -0800 Subject: [PATCH 5/8] Reduce queries in AvailabilityValidator using ids We can avoid selecting StockLocation records by just passing their id to Stock::Quantifier --- core/app/models/spree/stock/availability_validator.rb | 5 ++--- core/app/models/spree/stock/quantifier.rb | 2 ++ 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/core/app/models/spree/stock/availability_validator.rb b/core/app/models/spree/stock/availability_validator.rb index b6f47e51b2b..f93ba0bf32f 100644 --- a/core/app/models/spree/stock/availability_validator.rb +++ b/core/app/models/spree/stock/availability_validator.rb @@ -7,9 +7,8 @@ def validate(line_item) if quantity_by_stock_location_id.blank? ensure_in_stock(line_item, line_item.quantity) else - stock_locations = Spree::StockLocation.where(id: quantity_by_stock_location_id.keys).to_a - stock_locations.each do |stock_location| - ensure_in_stock(line_item, quantity_by_stock_location_id[stock_location.id], stock_location) + quantity_by_stock_location_id.each do |stock_location_id, quantity| + ensure_in_stock(line_item, quantity, stock_location_id) end end diff --git a/core/app/models/spree/stock/quantifier.rb b/core/app/models/spree/stock/quantifier.rb index 4c8f841e553..8a5b5a1f854 100644 --- a/core/app/models/spree/stock/quantifier.rb +++ b/core/app/models/spree/stock/quantifier.rb @@ -3,6 +3,8 @@ module Stock class Quantifier attr_reader :stock_items + # @param [Variant] variant The variant to check inventory for. + # @param [StockLocation, Integer] stock_location The stock_location to check inventory in. If unspecified it will check inventory in all available StockLocations def initialize(variant, stock_location = nil) @variant = variant @stock_items = Spree::StockItem.where(variant_id: variant) From 4df66e41818bc77edc91b15b48b02e09feb55208 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 26 Jan 2017 13:41:50 -0800 Subject: [PATCH 6/8] Improve AvailabilityValidator spec Instead of relying on the behaviour of stock_location_quantities and the entire order state machine, we should just create shipments and inventory units in the state we expect. This adds tests for the line item needing some stock from each stock location. This adds one skipped test which fails because the error on the line item is added twice. --- .../stock/availability_validator_spec.rb | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/core/spec/models/spree/stock/availability_validator_spec.rb b/core/spec/models/spree/stock/availability_validator_spec.rb index 3209f6c8762..611fb5d4430 100644 --- a/core/spec/models/spree/stock/availability_validator_spec.rb +++ b/core/spec/models/spree/stock/availability_validator_spec.rb @@ -54,33 +54,51 @@ module Stock context "line_item is part of a shipment" do let!(:order) { create(:order_with_line_items) } - context "has stock in all stock locations" do + context "has stock in one stock location" do let(:line_item) { order.line_items.first } before do - variant_ids = order.line_items.map(&:variant_id) - Spree::StockItem.where(variant_id: variant_ids).update_all(count_on_hand: 10, backorderable: false) + line_item.variant.stock_items.update_all(count_on_hand: 10, backorderable: false) end include_examples "passes validation" end - context "doesn't have stock in a particular stock location" do - let(:variant) { create(:variant) } - let(:line_item) { order.line_items.find_by(variant_id: variant.id) } + context "with stock in multiple locations" do + let(:line_item) { order.line_items.first } + let(:variant) { line_item.variant } let!(:stock_location_1) { create(:stock_location, name: "Test Warehouse", active: false) } before do - order.contents.add(variant, 1, stock_location_quantities: { stock_location_1.id => 1 }) - order.contents.advance - stock_location_1.stock_items.update_all(count_on_hand: 0, backorderable: false) + shipment = order.shipments.create(stock_location: stock_location_1) + order.contents.add(variant, 1, shipment: shipment) end - include_examples "fails validation" + context "but no stock in either location", skip: true do + before do + variant.stock_items.update_all(count_on_hand: 0, backorderable: false) + end + include_examples "fails validation" + end + + context "but no stock in one location" do + before do + stock_location_1.stock_items.update_all(count_on_hand: 0, backorderable: false) + end + + include_examples "fails validation" + end + + context "with enough stock only across locations" do + before do + variant.stock_items.update_all(count_on_hand: 1, backorderable: false) + end + include_examples "passes validation" + end context "but inventory units are finalized" do before do - Spree::InventoryUnit.update_all(pending: false) + order.inventory_units.update_all(pending: false) end include_examples "passes validation" From a2e4ad68838b6037a0f2011733c1879ec7417da9 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 26 Jan 2017 13:55:38 -0800 Subject: [PATCH 7/8] Only add errors once in AvailabilityValidator We shouldn't report the same error twice on the same item. --- .../spree/stock/availability_validator.rb | 33 +++++++++---------- .../stock/availability_validator_spec.rb | 2 +- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/core/app/models/spree/stock/availability_validator.rb b/core/app/models/spree/stock/availability_validator.rb index f93ba0bf32f..de9ddaed2d6 100644 --- a/core/app/models/spree/stock/availability_validator.rb +++ b/core/app/models/spree/stock/availability_validator.rb @@ -2,24 +2,9 @@ module Spree module Stock class AvailabilityValidator < ActiveModel::Validator def validate(line_item) - quantity_by_stock_location_id = line_item.inventory_units.pending.joins(:shipment).group(:stock_location_id).count - - if quantity_by_stock_location_id.blank? - ensure_in_stock(line_item, line_item.quantity) + if is_valid?(line_item) + true else - quantity_by_stock_location_id.each do |stock_location_id, quantity| - ensure_in_stock(line_item, quantity, stock_location_id) - end - end - - line_item.errors[:quantity].empty? - end - - private - - def ensure_in_stock(line_item, quantity, stock_location = nil) - quantifier = Stock::Quantifier.new(line_item.variant, stock_location) - unless quantifier.can_supply?(quantity) variant = line_item.variant display_name = variant.name.to_s display_name += %{ (#{variant.options_text})} unless variant.options_text.blank? @@ -28,6 +13,20 @@ def ensure_in_stock(line_item, quantity, stock_location = nil) :selected_quantity_not_available, item: display_name.inspect ) + false + end + end + + private + + def is_valid?(line_item) + if line_item.inventory_units.empty? + Stock::Quantifier.new(line_item.variant).can_supply?(line_item.quantity) + else + quantity_by_stock_location_id = line_item.inventory_units.pending.joins(:shipment).group(:stock_location_id).count + quantity_by_stock_location_id.all? do |stock_location_id, quantity| + Stock::Quantifier.new(line_item.variant, stock_location_id).can_supply?(quantity) + end end end end diff --git a/core/spec/models/spree/stock/availability_validator_spec.rb b/core/spec/models/spree/stock/availability_validator_spec.rb index 611fb5d4430..14e93c90589 100644 --- a/core/spec/models/spree/stock/availability_validator_spec.rb +++ b/core/spec/models/spree/stock/availability_validator_spec.rb @@ -74,7 +74,7 @@ module Stock order.contents.add(variant, 1, shipment: shipment) end - context "but no stock in either location", skip: true do + context "but no stock in either location" do before do variant.stock_items.update_all(count_on_hand: 0, backorderable: false) end From fff20eb4e33364ff1f64e4e01d210f040b1007f5 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 27 Feb 2017 15:30:43 -0800 Subject: [PATCH 8/8] Add AvailabilityValidator change to CHANGELOG --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec75373c159..148552dd166 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,9 @@ * The "Stores" section of Settings in the admin now supports creating and editing multiple stores. +* The `AvailabilityValidator` now correctly detects out of stock when there + are multiple shipments from the same stock location. + * Deprecations * `cache_key_for_taxons` helper has been deprecated in favour of `cache [I18n.locale, @taxons]`