Merge pull request openfoodfoundation#12726 from mkllnk/order-stock-spec

Track (negative) stock for on-demand products and overrides

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?
 

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
 

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?

config/locales/en.yml

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

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

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
 

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

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

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

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

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",

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