diff --git a/app/models/concerns/stock_settings_override_validation.rb b/app/models/concerns/stock_settings_override_validation.rb index 7040cb50e06..ecc3f2ec3bd 100644 --- a/app/models/concerns/stock_settings_override_validation.rb +++ b/app/models/concerns/stock_settings_override_validation.rb @@ -6,14 +6,14 @@ # `count_on_hand` can either be: nil or a number # # This means that a variant override can be in six different stock states -# but only three of them are valid. +# but only four of them are valid. # # | on_demand | count_on_hand | stock_overridden? | use_producer_stock_settings? | valid? | # |-----------|---------------|-------------------|------------------------------|--------| -# | 1 | nil | false | false | true | +# | 1 | nil | true | false | true | # | 0 | x | true | false | true | # | nil | nil | false | true | true | -# | 1 | x | ? | ? | false | +# | 1 | x | true | false | true | # | 0 | nil | ? | ? | false | # | nil | x | ? | ? | false | # @@ -27,7 +27,6 @@ module StockSettingsOverrideValidation def require_compatible_on_demand_and_count_on_hand disallow_count_on_hand_if_using_producer_stock_settings - disallow_count_on_hand_if_on_demand require_count_on_hand_if_limited_stock end @@ -39,14 +38,6 @@ def disallow_count_on_hand_if_using_producer_stock_settings errors.add(:count_on_hand, error_message) end - def disallow_count_on_hand_if_on_demand - return unless on_demand? && count_on_hand.present? - - error_message = I18n.t("count_on_hand.on_demand_but_count_on_hand_set", - scope: i18n_scope_for_stock_settings_override_validation_error) - errors.add(:count_on_hand, error_message) - end - def require_count_on_hand_if_limited_stock return unless on_demand == false && count_on_hand.blank? diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb index f39ab38dc9c..2e5023d46cc 100644 --- a/app/models/concerns/variant_stock.rb +++ b/app/models/concerns/variant_stock.rb @@ -96,7 +96,7 @@ def can_supply?(quantity) # Here we depend only on variant.total_on_hand and variant.on_demand. # This way, variant_overrides only need to override variant.total_on_hand and variant.on_demand. def fill_status(quantity) - on_hand = if total_on_hand >= quantity || on_demand + on_hand = if total_on_hand.to_i >= quantity || on_demand quantity else [0, total_on_hand].max @@ -112,8 +112,7 @@ def fill_status(quantity) # # This enables us to override this behaviour for variant overrides def move(quantity, originator = nil) - # Don't change variant stock if variant is on_demand or has been deleted - return if on_demand || deleted_at + return if deleted_at raise_error_if_no_stock_item_available diff --git a/app/models/variant_override.rb b/app/models/variant_override.rb index cabc309a938..79a28508f2b 100644 --- a/app/models/variant_override.rb +++ b/app/models/variant_override.rb @@ -15,7 +15,9 @@ class VariantOverride < ApplicationRecord # Need to ensure this can be set by the user. validates :default_stock, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true validates :price, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true - validates :count_on_hand, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true + validates :count_on_hand, numericality: { + greater_than_or_equal_to: 0, unless: :on_demand? + }, allow_nil: true default_scope { where(permission_revoked_at: nil) } @@ -36,9 +38,8 @@ def self.indexed(hub) end def stock_overridden? - # If count_on_hand is present, it means on_demand is false - # See StockSettingsOverrideValidation for details - count_on_hand.present? + # Testing for not nil because for a boolean `false.present?` is false. + !on_demand.nil? || !count_on_hand.nil? end def use_producer_stock_settings? diff --git a/config/locales/en.yml b/config/locales/en.yml index f2f0ed92ab9..d5743302fa9 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -127,7 +127,6 @@ en: variant_override: count_on_hand: using_producer_stock_settings_but_count_on_hand_set: "must be blank because using producer stock settings" - on_demand_but_count_on_hand_set: "must be blank if on demand" limited_stock_but_no_count_on_hand: "must be specified because forcing limited stock" messages: confirmation: "doesn't match %{attribute}" diff --git a/lib/open_food_network/scope_variant_to_hub.rb b/lib/open_food_network/scope_variant_to_hub.rb index d7a29cbb9c2..90bfef39d40 100644 --- a/lib/open_food_network/scope_variant_to_hub.rb +++ b/lib/open_food_network/scope_variant_to_hub.rb @@ -43,11 +43,7 @@ def on_demand # - updates variant_override.count_on_hand # - does not create stock_movement # - does not update stock_item.count_on_hand - # If it is a variant override with on_demand: - # - don't change stock or call super (super would change the variant's stock) def move(quantity, originator = nil) - return if @variant_override&.on_demand - if @variant_override&.stock_overridden? @variant_override.move_stock! quantity else diff --git a/spec/lib/open_food_network/scope_variant_to_hub_spec.rb b/spec/lib/open_food_network/scope_variant_to_hub_spec.rb index cbff22dc359..faa5d0583b6 100644 --- a/spec/lib/open_food_network/scope_variant_to_hub_spec.rb +++ b/spec/lib/open_food_network/scope_variant_to_hub_spec.rb @@ -181,9 +181,10 @@ module OpenFoodNetwork scoper.scope v2 end - it "doesn't reduce variant's stock" do + it "reduces override stock, not variant's stock" do v2.move(-2) expect(Spree::Variant.find(v2.id).on_hand).to eq 5 + expect(v2.on_hand).to eq(-2) end end diff --git a/spec/models/spree/line_item_spec.rb b/spec/models/spree/line_item_spec.rb index d715fec503f..b6fecf1f33a 100644 --- a/spec/models/spree/line_item_spec.rb +++ b/spec/models/spree/line_item_spec.rb @@ -313,8 +313,8 @@ module Spree expect(order.shipment.manifest.first.variant).to eq line_item.variant end - it "does not reduce the variant's stock level" do - expect(variant_on_demand.reload.on_hand).to eq 1 + it "reduces the variant's stock level" do + expect(variant_on_demand.reload.on_hand).to eq(-9) end it "does not mark inventory units as backorderd" do diff --git a/spec/models/spree/shipment_spec.rb b/spec/models/spree/shipment_spec.rb index a559d8b855a..ecaba702058 100644 --- a/spec/models/spree/shipment_spec.rb +++ b/spec/models/spree/shipment_spec.rb @@ -264,6 +264,37 @@ end end + describe "#finalize!" do + subject(:shipment) { order.shipments.first } + let(:variant) { order.variants.first } + let(:order) { create(:order_ready_for_confirmation) } + + it "reduces stock" do + variant.on_hand = 5 + + expect { shipment.finalize! } + .to change { variant.on_hand }.from(5).to(4) + end + + it "reduces stock of a variant override" do + variant.on_hand = 5 + variant_override = VariantOverride.create!( + variant:, + hub: order.distributor, + count_on_hand: 7, + on_demand: false, + ) + + expect { + shipment.finalize! + variant.reload + variant_override.reload + } + .to change { variant_override.count_on_hand }.from(7).to(6) + .and change { variant.on_hand }.by(0) + end + end + context "when order is completed" do before do allow(order).to receive_messages completed?: true diff --git a/spec/models/spree/variant_stock_spec.rb b/spec/models/spree/variant_stock_spec.rb new file mode 100644 index 00000000000..224fa3e855a --- /dev/null +++ b/spec/models/spree/variant_stock_spec.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: false + +require 'spec_helper' + +RSpec.describe Spree::Variant do + # This method is defined in app/models/concerns/variant_stock.rb. + # There is a separate spec for that concern but here I want to test + # the interplay of Spree::Variant and VariantOverride. + # + # A variant can be scoped to a hub which means that all stock methods + # like this one get overridden. Future calls to `variant.move` are then + # handled by the ScopeVariantToHub module which may call the + # VariantOverride. + describe "#move" do + subject(:variant) { create(:variant, on_hand: 5) } + + it "changes stock" do + expect { variant.move(-2) }.to change { variant.on_hand }.from(5).to(3) + end + + it "reduces stock even when on demand" do + variant.on_demand = true + + expect { variant.move(-2) }.to change { variant.on_hand }.from(5).to(3) + end + + it "rejects negative stock" do + expect { variant.move(-7) }.to raise_error( + ActiveRecord::RecordInvalid, + "Validation failed: Count on hand must be greater than or equal to 0" + ) + end + + describe "with VariantOverride" do + subject(:hub_variant) { + Spree::Variant.find(variant.id).tap { |v| scoper.scope(v) } + } + let(:override) { + VariantOverride.create!( + variant:, + hub: create(:distributor_enterprise), + count_on_hand: 7, + on_demand: false, + ) + } + let(:scoper) { OpenFoodNetwork::ScopeVariantToHub.new(override.hub) } + + it "changes stock only on the variant override" do + expect { + hub_variant.move(-3) + override.reload + } + .to change { override.count_on_hand }.from(7).to(4) + .and change { hub_variant.on_hand }.from(7).to(4) + .and change { variant.on_hand }.by(0) + end + + it "reduces stock when on demand" do + override.update!(on_demand: true, count_on_hand: 7) + + expect { + hub_variant.move(-3) + override.reload + } + .to change { override.count_on_hand }.from(7).to(4) + .and change { hub_variant.on_hand }.from(7).to(4) + .and change { variant.on_hand }.by(0) + end + + it "doesn't prevent negative stock" do + # VariantOverride relies on other stock checks during checkout. :-( + expect { + hub_variant.move(-8) + override.reload + } + .to change { override.count_on_hand }.from(7).to(-1) + .and change { hub_variant.on_hand }.from(7).to(-1) + .and change { variant.on_hand }.by(0) + + # The update didn't run validations and now it's invalid: + expect(override).not_to be_valid + end + + it "doesn't fail on negative stock when on demand" do + override.update!(on_demand: true, count_on_hand: nil) + + expect { + hub_variant.move(-8) + override.reload + } + .to change { override.count_on_hand }.from(nil).to(-8) + .and change { hub_variant.on_hand }.from(nil).to(-8) + .and change { variant.on_hand }.by(0) + end + end + end +end diff --git a/spec/models/variant_override_spec.rb b/spec/models/variant_override_spec.rb index 15c9a1f4bf6..41122e6e71e 100644 --- a/spec/models/variant_override_spec.rb +++ b/spec/models/variant_override_spec.rb @@ -97,11 +97,8 @@ context "when count_on_hand is set" do let(:count_on_hand) { 1 } - it "is invalid" do - expect(variant_override).not_to be_valid - error_message = I18n.t("on_demand_but_count_on_hand_set", - scope: [i18n_scope_for_error, "count_on_hand"]) - expect(variant_override.errors[:count_on_hand]).to eq([error_message]) + it "is valid" do + expect(variant_override).to be_valid end end end @@ -168,7 +165,7 @@ describe "with nil count on hand" do let(:variant_override) do - build_stubbed( + build( :variant_override, variant: build_stubbed(:variant), hub: build_stubbed(:distributor_enterprise), @@ -178,15 +175,18 @@ end describe "stock_overridden?" do - it "returns false" do - expect(variant_override.stock_overridden?).to be false + it "returns true" do + expect(variant_override.stock_overridden?).to be true end end describe "move_stock!" do it "silently logs an error" do - expect(Bugsnag).to receive(:notify) - variant_override.move_stock!(5) + expect { + variant_override.move_stock!(5) + }.to change { + variant_override.count_on_hand + }.from(nil).to(5) end end end diff --git a/spec/system/admin/product_import_spec.rb b/spec/system/admin/product_import_spec.rb index e916a56a74f..49c90699180 100644 --- a/spec/system/admin/product_import_spec.rb +++ b/spec/system/admin/product_import_spec.rb @@ -624,33 +624,6 @@ expect(page).not_to have_content "line 3: Sprouts" end - it "handles on_demand and on_hand validations with inventory - With both values set" do - csv_data = <<~CSV - name, distributor, producer, category, on_hand, price, units, on_demand - Beans, Another Enterprise, User Enterprise, Vegetables, 6, 3.20, 500, 1 - Sprouts, Another Enterprise, User Enterprise, Vegetables, 6, 6.50, 500, 1 - Cabbage, Another Enterprise, User Enterprise, Vegetables, 0, 1.50, 500, 1 - CSV - File.write('/tmp/test.csv', csv_data) - - visit main_app.admin_product_import_path - select 'Inventories', from: "settings_import_into" - attach_file 'file', '/tmp/test.csv' - click_button 'Upload' - - proceed_to_validation - - expect(page).to have_selector '.item-count', text: "3" - expect(page).to have_selector '.invalid-count', text: "3" - - find('div.header-description', text: 'Items contain errors').click - expect(page).to have_content "line 2: Beans - Count_on_hand must be blank if on demand" - expect(page).to have_content "line 3: Sprouts - Count_on_hand must be blank if on demand" - expect(page).to have_content "line 4: Cabbage - Count_on_hand must be blank if on demand" - expect(page).to have_content "Imported file contains invalid entries" - expect(page).not_to have_selector 'input[type=submit][value="Save"]' - end - it "imports lines with all allowed units" do csv_data = CSV.generate do |csv| csv << ["name", "producer", "category", "on_hand", "price", "units", "unit_type", diff --git a/spec/system/consumer/shopping/variant_overrides_spec.rb b/spec/system/consumer/shopping/variant_overrides_spec.rb index 01dd56c3493..4cc8d11eb02 100644 --- a/spec/system/consumer/shopping/variant_overrides_spec.rb +++ b/spec/system/consumer/shopping/variant_overrides_spec.rb @@ -222,13 +222,13 @@ expect(product1_variant1_override.reload.count_on_hand).to be_nil end - it "does not subtract stock from variants where the override has on_demand: true" do + it "subtracts stock from override but not variants where the override has on_demand: true" do click_add_to_cart product4_variant1, 2 click_checkout expect do complete_checkout end.to change { product4_variant1.reload.on_hand }.by(0) - expect(product4_variant1_override.reload.count_on_hand).to be_nil + expect(product4_variant1_override.reload.count_on_hand).to eq(-2) end it "does not show out of stock flags on order confirmation page" do