From 88b1bb61d69536740cbe4e3f7a08ed29853b875c Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 13 Dec 2018 16:19:19 +0000 Subject: [PATCH 1/5] Fix availability validator to include inventory_units in it's validation --- .../stock/availability_validator_decorator.rb | 37 +++++++++++++++++++ .../admin/bulk_order_management_spec.rb | 10 ++--- 2 files changed, 42 insertions(+), 5 deletions(-) create mode 100644 app/models/spree/stock/availability_validator_decorator.rb diff --git a/app/models/spree/stock/availability_validator_decorator.rb b/app/models/spree/stock/availability_validator_decorator.rb new file mode 100644 index 0000000000..f0411881e7 --- /dev/null +++ b/app/models/spree/stock/availability_validator_decorator.rb @@ -0,0 +1,37 @@ +Spree::Stock::AvailabilityValidator.class_eval do + def validate(line_item) + quantity = adapt_line_item_quantity_to_inventory_units(line_item) + + validate_quantity(line_item, quantity) + end + + private + + # This is an adapted version of a fix to the inventory_units not being considered here. + # See #3090 for details. + # This can be removed after upgrading to Spree 2.4. + def adapt_line_item_quantity_to_inventory_units(line_item) + shipment = line_item_shipment(line_item) + return line_item.quantity unless shipment + + units = shipment.inventory_units_for(line_item.variant) + line_item.quantity - units.count + end + + def line_item_shipment(line_item) + return line_item.target_shipment if line_item.target_shipment + return line_item.order.shipments.first if line_item.order.present? && line_item.order.shipments.any? + end + + # This is the spree v2.0.4 implementation of validate + # But using the calculated quantity instead of the line_item.quantity. + def validate_quantity(line_item, quantity) + quantifier = Spree::Stock::Quantifier.new(line_item.variant_id) + return if quantifier.can_supply? quantity + + variant = line_item.variant + display_name = %Q{#{variant.name}} + display_name += %Q{ (#{variant.options_text})} unless variant.options_text.blank? + line_item.errors[:quantity] << Spree.t(:out_of_stock, :scope => :order_populator, :item => display_name.inspect) + end +end diff --git a/spec/features/admin/bulk_order_management_spec.rb b/spec/features/admin/bulk_order_management_spec.rb index e6a3423868..6cbdd615b2 100644 --- a/spec/features/admin/bulk_order_management_spec.rb +++ b/spec/features/admin/bulk_order_management_spec.rb @@ -139,11 +139,10 @@ feature %q{ end context "submitting data to the server" do - let!(:o1) { create(:order_with_distributor, state: 'complete', completed_at: Time.zone.now ) } - let!(:li1) { create(:line_item_with_shipment, order: o1, :quantity => 5 ) } + let!(:order) { create(:completed_order_with_fees) } before :each do - li1.variant.update_attributes(on_hand: 1, on_demand: false) + order.line_items.second.destroy # we keep only one line item for this test visit '/admin/orders/bulk_management' end @@ -162,13 +161,14 @@ feature %q{ context "when unacceptable data is sent to the server" do it "displays an update button which submits pending changes" do expect(page).to have_no_selector "#save-bar" - fill_in "quantity", :with => li1.variant.on_hand + li1.quantity + 10 + line_item = order.line_items.first + fill_in "quantity", :with => line_item.variant.on_hand + line_item.quantity + 10 expect(page).to have_selector "input[name='quantity'].ng-dirty" expect(page).to have_selector "#save-bar", text: "You have unsaved changes" click_button "Save Changes" expect(page).to have_selector "#save-bar", text: "Fields with red borders contain errors." expect(page).to have_selector "input[name='quantity'].ng-dirty.update-error" - expect(page).to have_content "exceeds available stock. Please ensure line items have a valid quantity." + expect(page).to have_content "is out of stock" end end end From 3085e159616a5f35583a6d5846274b75627b20a4 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 13 Dec 2018 20:47:53 +0000 Subject: [PATCH 2/5] Fix order factory by making line_item.skip_stock_check work in spree 2 --- app/models/spree/line_item_decorator.rb | 9 +-------- .../stock/availability_validator_decorator.rb | 3 +++ spec/models/spree/line_item_spec.rb | 19 ------------------- 3 files changed, 4 insertions(+), 27 deletions(-) diff --git a/app/models/spree/line_item_decorator.rb b/app/models/spree/line_item_decorator.rb index 7bc718e857..58ca4c2b39 100644 --- a/app/models/spree/line_item_decorator.rb +++ b/app/models/spree/line_item_decorator.rb @@ -8,7 +8,7 @@ Spree::LineItem.class_eval do # Redefining here to add the inverse_of option belongs_to :order, :class_name => "Spree::Order", inverse_of: :line_items - # Allows manual skipping of stock_availability check + # Allows manual skipping of Stock::AvailabilityValidator attr_accessor :skip_stock_check attr_accessible :max_quantity, :final_weight_volume, :price @@ -133,13 +133,6 @@ Spree::LineItem.class_eval do end alias_method_chain :update_inventory, :scoping - # Override of Spree validation method - # Added check for in-memory :skip_stock_check attribute - def stock_availability - return if skip_stock_check || sufficient_stock? - errors.add(:quantity, I18n.t('validation.exceeds_available_stock')) - end - def scoper @scoper ||= OpenFoodNetwork::ScopeVariantToHub.new(order.distributor) end diff --git a/app/models/spree/stock/availability_validator_decorator.rb b/app/models/spree/stock/availability_validator_decorator.rb index f0411881e7..ef6c77cec8 100644 --- a/app/models/spree/stock/availability_validator_decorator.rb +++ b/app/models/spree/stock/availability_validator_decorator.rb @@ -1,5 +1,8 @@ Spree::Stock::AvailabilityValidator.class_eval do def validate(line_item) + # OFN specific check for in-memory :skip_stock_check attribute + return if line_item.skip_stock_check + quantity = adapt_line_item_quantity_to_inventory_units(line_item) validate_quantity(line_item, quantity) diff --git a/spec/models/spree/line_item_spec.rb b/spec/models/spree/line_item_spec.rb index 4690285be3..9f85774bf9 100644 --- a/spec/models/spree/line_item_spec.rb +++ b/spec/models/spree/line_item_spec.rb @@ -585,24 +585,5 @@ module Spree }.to change(Spree::OptionValue, :count).by(0) end end - - describe "checking stock availability" do - let(:line_item) { LineItem.new } - - context "when skip_stock_check is not set" do - it "checks stock" do - expect(line_item).to receive(:sufficient_stock?) { true } - line_item.send(:stock_availability) - end - end - - context "when skip_stock_check is set to true" do - before { line_item.skip_stock_check = true } - it "does not check stock" do - expect(line_item).to_not receive(:sufficient_stock?) - line_item.send(:stock_availability) - end - end - end end end From bc22b0a58e3ffbd59d40ae4dfb45467170ba1667 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 13 Dec 2018 20:49:58 +0000 Subject: [PATCH 3/5] Fix capping quantity to stock levels by always validating line items with quantity zero Stock::Quantifier.can_supply? returns false for an input of zero when stock level is negative --- app/models/spree/stock/availability_validator_decorator.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/spree/stock/availability_validator_decorator.rb b/app/models/spree/stock/availability_validator_decorator.rb index ef6c77cec8..790778f181 100644 --- a/app/models/spree/stock/availability_validator_decorator.rb +++ b/app/models/spree/stock/availability_validator_decorator.rb @@ -4,6 +4,7 @@ Spree::Stock::AvailabilityValidator.class_eval do return if line_item.skip_stock_check quantity = adapt_line_item_quantity_to_inventory_units(line_item) + return if quantity == 0 validate_quantity(line_item, quantity) end From c1aeb2e9a37cc829b79d362351b9107033af0643 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Fri, 14 Dec 2018 14:02:32 +0000 Subject: [PATCH 4/5] Improve naming in availability_validator_decorator and cover it with tests --- .../stock/availability_validator_decorator.rb | 12 +-- .../stock/availability_validator_spec.rb | 75 +++++++++++++++++++ 2 files changed, 81 insertions(+), 6 deletions(-) create mode 100644 spec/models/spree/stock/availability_validator_spec.rb diff --git a/app/models/spree/stock/availability_validator_decorator.rb b/app/models/spree/stock/availability_validator_decorator.rb index 790778f181..29f9d0413c 100644 --- a/app/models/spree/stock/availability_validator_decorator.rb +++ b/app/models/spree/stock/availability_validator_decorator.rb @@ -3,10 +3,10 @@ Spree::Stock::AvailabilityValidator.class_eval do # OFN specific check for in-memory :skip_stock_check attribute return if line_item.skip_stock_check - quantity = adapt_line_item_quantity_to_inventory_units(line_item) - return if quantity == 0 + quantity_to_validate = line_item.quantity - quantity_in_shipment(line_item) + return if quantity_to_validate < 1 - validate_quantity(line_item, quantity) + validate_quantity(line_item, quantity_to_validate) end private @@ -14,12 +14,12 @@ Spree::Stock::AvailabilityValidator.class_eval do # This is an adapted version of a fix to the inventory_units not being considered here. # See #3090 for details. # This can be removed after upgrading to Spree 2.4. - def adapt_line_item_quantity_to_inventory_units(line_item) + def quantity_in_shipment(line_item) shipment = line_item_shipment(line_item) - return line_item.quantity unless shipment + return 0 unless shipment units = shipment.inventory_units_for(line_item.variant) - line_item.quantity - units.count + units.count end def line_item_shipment(line_item) diff --git a/spec/models/spree/stock/availability_validator_spec.rb b/spec/models/spree/stock/availability_validator_spec.rb new file mode 100644 index 0000000000..8eb29fbe0f --- /dev/null +++ b/spec/models/spree/stock/availability_validator_spec.rb @@ -0,0 +1,75 @@ +require 'spec_helper' + +module Spree + module Stock + describe AvailabilityValidator do + let(:validator) { AvailabilityValidator.new({}) } + + context "line item without existing inventory units" do + let(:order) { create(:order_with_line_items) } + let(:line_item) { order.line_items.first } + + before do + expect(order.shipment.inventory_units).to be_empty + expect(line_item.target_shipment).to be_nil + end + + context "available quantity when variant.on_hand > line_item.quantity" do + it "suceeds" do + line_item.quantity = line_item.variant.on_hand - 1 + validator.validate(line_item) + expect(line_item.errors[:quantity].size).to eq(0) + end + end + + context "unavailable quantity when variant.on_hand < line_item.quantity" do + it "fails" do + line_item.quantity = line_item.variant.on_hand + 1 + validator.validate(line_item) + expect_product_out_of_stock_error + end + + it "succeeds with line_item skip_stock_check" do + line_item.skip_stock_check = true + line_item.quantity = line_item.variant.on_hand + 1 + validator.validate(line_item) + expect(line_item.errors[:quantity].size).to eq(0) + end + end + end + + describe "line item with existing inventory units" do + let(:order) { create(:completed_order_with_totals) } + let(:line_item) { order.line_items.first } + + before do + expect(line_item.quantity).to eq(1) + expect(line_item.variant.on_hand).to eq(4) + + expect(order.shipment.inventory_units).to_not be_empty + expect(order.shipment.inventory_units_for(line_item.variant).size).to eq(1) + end + + context "when adding all variant.on_hand quantity to existing line_item quantity" do + it "succeeds because it excludes existing inventory units from the validation" do + line_item.quantity += line_item.variant.on_hand + validator.validate(line_item) + expect(line_item.errors[:quantity].size).to eq(0) + end + + it "fails if one more item is added" do + line_item.quantity += line_item.variant.on_hand + 1 + validator.validate(line_item) + expect_product_out_of_stock_error + end + end + end + + def expect_product_out_of_stock_error + quantity_error = line_item.errors[:quantity].first + expect(quantity_error).to include(line_item.variant.product.name) + expect(quantity_error).to include("out of stock") + end + end + end +end From 4f79fd5be088132637591d871e5d013dceb99cff Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 20 Dec 2018 11:12:04 +0000 Subject: [PATCH 5/5] Improve availability validator spec readability and remove unnecessary expect statements --- .../stock/availability_validator_spec.rb | 29 ++++--------------- 1 file changed, 5 insertions(+), 24 deletions(-) diff --git a/spec/models/spree/stock/availability_validator_spec.rb b/spec/models/spree/stock/availability_validator_spec.rb index 8eb29fbe0f..de14a5a1c0 100644 --- a/spec/models/spree/stock/availability_validator_spec.rb +++ b/spec/models/spree/stock/availability_validator_spec.rb @@ -9,16 +9,11 @@ module Spree let(:order) { create(:order_with_line_items) } let(:line_item) { order.line_items.first } - before do - expect(order.shipment.inventory_units).to be_empty - expect(line_item.target_shipment).to be_nil - end - context "available quantity when variant.on_hand > line_item.quantity" do it "suceeds" do line_item.quantity = line_item.variant.on_hand - 1 validator.validate(line_item) - expect(line_item.errors[:quantity].size).to eq(0) + expect(line_item).to be_valid end end @@ -26,14 +21,14 @@ module Spree it "fails" do line_item.quantity = line_item.variant.on_hand + 1 validator.validate(line_item) - expect_product_out_of_stock_error + expect(line_item).not_to be_valid end it "succeeds with line_item skip_stock_check" do line_item.skip_stock_check = true line_item.quantity = line_item.variant.on_hand + 1 validator.validate(line_item) - expect(line_item.errors[:quantity].size).to eq(0) + expect(line_item).to be_valid end end end @@ -42,34 +37,20 @@ module Spree let(:order) { create(:completed_order_with_totals) } let(:line_item) { order.line_items.first } - before do - expect(line_item.quantity).to eq(1) - expect(line_item.variant.on_hand).to eq(4) - - expect(order.shipment.inventory_units).to_not be_empty - expect(order.shipment.inventory_units_for(line_item.variant).size).to eq(1) - end - context "when adding all variant.on_hand quantity to existing line_item quantity" do it "succeeds because it excludes existing inventory units from the validation" do line_item.quantity += line_item.variant.on_hand validator.validate(line_item) - expect(line_item.errors[:quantity].size).to eq(0) + expect(line_item).to be_valid end it "fails if one more item is added" do line_item.quantity += line_item.variant.on_hand + 1 validator.validate(line_item) - expect_product_out_of_stock_error + expect(line_item).not_to be_valid end end end - - def expect_product_out_of_stock_error - quantity_error = line_item.errors[:quantity].first - expect(quantity_error).to include(line_item.variant.product.name) - expect(quantity_error).to include("out of stock") - end end end end