From 21976566069034f3c3df799fc99cf5575ef1c0c3 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Fri, 9 May 2025 15:27:35 +1000
Subject: [PATCH 1/4] Stop creating stock movements

Spree added stock movements to track the movements between stock
locations. But we got rid of stock locations and the only stock
movements we have now are just records of stock level changes.

These records were not created in all cases though and there were also
not created for variant overrides (inventory items). And since these
records aren't visible anywhere, I think it's best we remove them
altogether.

I do think that some kind of log would be useful but I don't think that
AR records like this are the best solution for that. And the
StockMovement model just added complexity to our already complex stock
level storage. The actual adjustment of the count_on_hand attribute of
the StockItem was performed in an after_create hook of the
StockMovement. Now we call it explicitely.
---
 app/models/concerns/variant_stock.rb      |  9 ++-------
 app/models/spree/return_authorization.rb  |  2 +-
 spec/models/spree/order_inventory_spec.rb | 14 --------------
 3 files changed, 3 insertions(+), 22 deletions(-)

diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb
index 7e40bd2afa..d8d7f1419a 100644
--- a/app/models/concerns/variant_stock.rb
+++ b/app/models/concerns/variant_stock.rb
@@ -108,13 +108,12 @@ module VariantStock
   #   only one stock item per variant
   #
   # This enables us to override this behaviour for variant overrides
-  def move(quantity, originator = nil)
+  def move(quantity, _originator = nil)
     return if deleted_at
 
     raise_error_if_no_stock_item_available
 
-    # Creates a stock movement: it updates stock_item.count_on_hand and fills backorders
-    stock_item.stock_movements.create!(quantity:, originator:)
+    stock_item.adjust_count_on_hand(quantity)
   end
 
   # There shouldn't be any other stock items, because we should
@@ -141,10 +140,6 @@ module VariantStock
   end
 
   # Overwrites stock_item.count_on_hand
-  #
-  # Calling stock_item.adjust_count_on_hand will bypass filling backorders
-  #   and creating stock movements
-  # If that was required we could call self.move
   def overwrite_stock_levels(new_level)
     stock_item.adjust_count_on_hand(new_level.to_i - stock_item.count_on_hand)
   end
diff --git a/app/models/spree/return_authorization.rb b/app/models/spree/return_authorization.rb
index 9410899763..b7f7849c61 100644
--- a/app/models/spree/return_authorization.rb
+++ b/app/models/spree/return_authorization.rb
@@ -92,7 +92,7 @@ module Spree
     def process_return
       inventory_units.each do |iu|
         iu.return!
-        Spree::StockMovement.create!(stock_item_id: iu.find_stock_item.id, quantity: 1)
+        iu.find_stock_item.adjust_count_on_hand(1)
       end
 
       Adjustment.create(
diff --git a/spec/models/spree/order_inventory_spec.rb b/spec/models/spree/order_inventory_spec.rb
index cc7d520bca..234e9d5207 100644
--- a/spec/models/spree/order_inventory_spec.rb
+++ b/spec/models/spree/order_inventory_spec.rb
@@ -51,13 +51,6 @@ RSpec.describe Spree::OrderInventory do
       expect(units['backordered'].size).to eq 2
       expect(units['on_hand'].size).to eq 3
     end
-
-    it 'should create stock_movement' do
-      expect(subject.__send__(:add_to_shipment, shipment, variant, 5)).to eq 5
-
-      movement = variant.stock_item.stock_movements.last
-      expect(movement.quantity).to eq(-5)
-    end
   end
 
   context 'when order has too many inventory units' do
@@ -101,13 +94,6 @@ RSpec.describe Spree::OrderInventory do
         end
       end
 
-      it 'should create stock_movement' do
-        expect(subject.__send__(:remove_from_shipment, shipment, variant, 1, true)).to eq 1
-
-        movement = variant.stock_item.stock_movements.last
-        expect(movement.quantity).to eq 1
-      end
-
       it 'should destroy backordered units first' do
         allow(shipment).to receive_messages(inventory_units_for: [
                                               build(:inventory_unit,

From 729e62d7dba8b26c3e057996b101c2a14056e56a Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 13 May 2025 15:01:16 +1000
Subject: [PATCH 2/4] Remove unused stock move originator parameter

---
 app/models/concerns/variant_stock.rb          | 2 +-
 app/models/spree/order_inventory.rb           | 4 ++--
 app/models/spree/shipment.rb                  | 4 ++--
 lib/open_food_network/scope_variant_to_hub.rb | 3 +--
 spec/models/spree/shipment_spec.rb            | 4 ++--
 5 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb
index d8d7f1419a..aced0efc8d 100644
--- a/app/models/concerns/variant_stock.rb
+++ b/app/models/concerns/variant_stock.rb
@@ -108,7 +108,7 @@ module VariantStock
   #   only one stock item per variant
   #
   # This enables us to override this behaviour for variant overrides
-  def move(quantity, _originator = nil)
+  def move(quantity)
     return if deleted_at
 
     raise_error_if_no_stock_item_available
diff --git a/app/models/spree/order_inventory.rb b/app/models/spree/order_inventory.rb
index fdf0d9ed9b..504c62f7e7 100644
--- a/app/models/spree/order_inventory.rb
+++ b/app/models/spree/order_inventory.rb
@@ -77,7 +77,7 @@ module Spree
       back_order.times { shipment.set_up_inventory('backordered', variant, order) }
 
       if order.completed?
-        variant.move(-quantity, shipment)
+        variant.move(-quantity)
       end
 
       quantity
@@ -101,7 +101,7 @@ module Spree
       shipment.destroy if shipment.inventory_units.reload.count == 0
 
       if order.completed? && restock_item
-        variant.move(removed_quantity, shipment)
+        variant.move(removed_quantity)
       end
 
       removed_quantity
diff --git a/app/models/spree/shipment.rb b/app/models/spree/shipment.rb
index 5b9692912c..8288d564f5 100644
--- a/app/models/spree/shipment.rb
+++ b/app/models/spree/shipment.rb
@@ -313,11 +313,11 @@ module Spree
     end
 
     def manifest_unstock(item)
-      item.variant.move(-1 * item.quantity, self)
+      item.variant.move(-1 * item.quantity)
     end
 
     def manifest_restock(item)
-      item.variant.move(item.quantity, self)
+      item.variant.move(item.quantity)
     end
 
     def generate_shipment_number
diff --git a/lib/open_food_network/scope_variant_to_hub.rb b/lib/open_food_network/scope_variant_to_hub.rb
index 90bfef39d4..03c9ec3408 100644
--- a/lib/open_food_network/scope_variant_to_hub.rb
+++ b/lib/open_food_network/scope_variant_to_hub.rb
@@ -41,9 +41,8 @@ module OpenFoodNetwork
 
       # If it is an variant override with a count_on_hand value:
       #   - updates variant_override.count_on_hand
-      #   - does not create stock_movement
       #   - does not update stock_item.count_on_hand
-      def move(quantity, originator = nil)
+      def move(quantity)
         if @variant_override&.stock_overridden?
           @variant_override.move_stock! quantity
         else
diff --git a/spec/models/spree/shipment_spec.rb b/spec/models/spree/shipment_spec.rb
index 6f420aabff..2da2999d50 100644
--- a/spec/models/spree/shipment_spec.rb
+++ b/spec/models/spree/shipment_spec.rb
@@ -324,7 +324,7 @@ RSpec.describe Spree::Shipment do
       allow(shipment).to receive_message_chain(:inventory_units,
                                                :group_by,
                                                map: [unit])
-      expect(variant).to receive(:move).with(1, shipment)
+      expect(variant).to receive(:move).with(1)
       shipment.after_cancel
     end
   end
@@ -347,7 +347,7 @@ RSpec.describe Spree::Shipment do
       allow(shipment).to receive_message_chain(:inventory_units,
                                                :group_by,
                                                map: [unit])
-      expect(variant).to receive(:move).with(-1, shipment)
+      expect(variant).to receive(:move).with(-1)
       shipment.after_resume
     end
 

From be312246ec3a8e1cd13b2e25be80b5461beaa232 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 13 May 2025 15:07:22 +1000
Subject: [PATCH 3/4] Stop referring to stock movements

---
 app/models/spree/ability.rb              |  1 -
 app/models/spree/stock_item.rb           |  1 -
 spec/models/spree/ability_spec.rb        |  7 -------
 spec/models/spree/inventory_unit_spec.rb |  2 +-
 spec/models/spree/product_spec.rb        | 11 -----------
 spec/models/spree/stock_item_spec.rb     |  8 --------
 6 files changed, 1 insertion(+), 29 deletions(-)

diff --git a/app/models/spree/ability.rb b/app/models/spree/ability.rb
index 08e27d60d7..de867108e5 100644
--- a/app/models/spree/ability.rb
+++ b/app/models/spree/ability.rb
@@ -35,7 +35,6 @@ module Spree
         can [:read, :update, :destroy], Spree::User, id: user.id
         can [:index, :read], State
         can [:index, :read], StockItem
-        can [:index, :read], StockMovement
         can [:index, :read], Taxon
         can [:index, :read], Variant
         can [:index, :read], Zone
diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb
index 82fb461342..67575f4e66 100644
--- a/app/models/spree/stock_item.rb
+++ b/app/models/spree/stock_item.rb
@@ -7,7 +7,6 @@ module Spree
     acts_as_paranoid
 
     belongs_to :variant, -> { with_deleted }, class_name: 'Spree::Variant', inverse_of: :stock_items
-    has_many :stock_movements, dependent: :destroy
 
     validates :variant_id, uniqueness: { scope: [:deleted_at] }
     validates :count_on_hand, numericality: { greater_than_or_equal_to: 0, unless: :backorderable? }
diff --git a/spec/models/spree/ability_spec.rb b/spec/models/spree/ability_spec.rb
index 6060d7bfc5..5756b9795f 100644
--- a/spec/models/spree/ability_spec.rb
+++ b/spec/models/spree/ability_spec.rb
@@ -119,13 +119,6 @@ RSpec.describe Spree::Ability do
       end
     end
 
-    context 'for StockMovement' do
-      let(:resource) { Spree::StockMovement.new }
-      context 'requested by any user' do
-        it_should_behave_like 'read only'
-      end
-    end
-
     context 'for Taxons' do
       let(:resource) { Spree::Taxon.new }
       context 'requested by any user' do
diff --git a/spec/models/spree/inventory_unit_spec.rb b/spec/models/spree/inventory_unit_spec.rb
index 8a755fa6d6..8af09dc651 100644
--- a/spec/models/spree/inventory_unit_spec.rb
+++ b/spec/models/spree/inventory_unit_spec.rb
@@ -25,7 +25,7 @@ RSpec.describe Spree::InventoryUnit do
       ]
     }
 
-    it "should create a stock movement" do
+    it "finalizes pending units" do
       Spree::InventoryUnit.finalize_units!(inventory_units)
       expect(inventory_units.any?(&:pending)).to be_falsy
     end
diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb
index 59884d05e8..7967fd37c2 100644
--- a/spec/models/spree/product_spec.rb
+++ b/spec/models/spree/product_spec.rb
@@ -84,17 +84,6 @@ module Spree
       end
     end
 
-    context "has stock movements" do
-      let(:product) { create(:product) }
-      let(:variant) { product.variants.first }
-      let(:stock_item) { variant.stock_items.first }
-
-      it "doesnt raise ReadOnlyRecord error" do
-        Spree::StockMovement.create!(stock_item:, quantity: 1)
-        expect { product.destroy }.not_to raise_error
-      end
-    end
-
     describe "associations" do
       it { is_expected.to have_one(:image) }
 
diff --git a/spec/models/spree/stock_item_spec.rb b/spec/models/spree/stock_item_spec.rb
index 9268c04b18..e20291fb21 100644
--- a/spec/models/spree/stock_item_spec.rb
+++ b/spec/models/spree/stock_item_spec.rb
@@ -101,13 +101,5 @@ RSpec.describe Spree::StockItem do
         end
       end
     end
-
-    context "with stock movements" do
-      before { Spree::StockMovement.create(stock_item: subject, quantity: 1) }
-
-      it "does not destroy stock_movements when destroyed" do
-        expect { subject.destroy }.to change { Spree::StockMovement.count }.by(-1)
-      end
-    end
   end
 end

From d650d0604939cad8c15a6917135734a87079ac6c Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 13 May 2025 15:09:33 +1000
Subject: [PATCH 4/4] Delete StockMovement model

I'm not dropping the database table just yet. Let's keep this change
reversible without data loss. And dropping the table would break the
currently running app code during deploy. We can do that another time.
---
 app/models/spree/stock_movement.rb       | 20 --------------
 spec/factories/stock_movement_factory.rb |  8 ------
 spec/models/spree/stock_movement_spec.rb | 34 ------------------------
 3 files changed, 62 deletions(-)
 delete mode 100644 app/models/spree/stock_movement.rb
 delete mode 100644 spec/factories/stock_movement_factory.rb
 delete mode 100644 spec/models/spree/stock_movement_spec.rb

diff --git a/app/models/spree/stock_movement.rb b/app/models/spree/stock_movement.rb
deleted file mode 100644
index 4de0dea3a0..0000000000
--- a/app/models/spree/stock_movement.rb
+++ /dev/null
@@ -1,20 +0,0 @@
-# frozen_string_literal: true
-
-module Spree
-  class StockMovement < ApplicationRecord
-    belongs_to :stock_item, class_name: 'Spree::StockItem'
-    belongs_to :originator, polymorphic: true, optional: true
-
-    after_create :update_stock_item_quantity
-
-    validates :quantity, presence: true
-
-    scope :recent, -> { order('created_at DESC') }
-
-    private
-
-    def update_stock_item_quantity
-      stock_item.adjust_count_on_hand quantity
-    end
-  end
-end
diff --git a/spec/factories/stock_movement_factory.rb b/spec/factories/stock_movement_factory.rb
deleted file mode 100644
index 2ca6e3e714..0000000000
--- a/spec/factories/stock_movement_factory.rb
+++ /dev/null
@@ -1,8 +0,0 @@
-# frozen_string_literal: true
-
-FactoryBot.define do
-  factory :stock_movement, class: Spree::StockMovement do
-    quantity { 1 }
-    action { 'sold' }
-  end
-end
diff --git a/spec/models/spree/stock_movement_spec.rb b/spec/models/spree/stock_movement_spec.rb
deleted file mode 100644
index 02f4f4c543..0000000000
--- a/spec/models/spree/stock_movement_spec.rb
+++ /dev/null
@@ -1,34 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Spree::StockMovement do
-  let(:stock_item) { create(:variant, on_hand: 15).stock_item }
-  subject { build(:stock_movement, stock_item:) }
-
-  it 'should belong to a stock item' do
-    expect(subject).to respond_to(:stock_item)
-  end
-
-  context "when quantity is negative" do
-    context "after save" do
-      it "should decrement the stock item count on hand" do
-        subject.quantity = -1
-        subject.save
-        stock_item.reload
-        expect(stock_item.count_on_hand).to eq 14
-      end
-    end
-  end
-
-  context "when quantity is positive" do
-    context "after save" do
-      it "should increment the stock item count on hand" do
-        subject.quantity = 1
-        subject.save
-        stock_item.reload
-        expect(stock_item.count_on_hand).to eq 16
-      end
-    end
-  end
-end