From 248110cfb3885afb0e1fcd4ceb4bbc78ceebc7d9 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 9 Jan 2025 11:26:15 +1100 Subject: [PATCH] Make stock location association optional Prepare for removal --- app/models/spree/inventory_unit.rb | 3 +-- app/models/spree/order_inventory.rb | 3 +-- app/models/spree/shipment.rb | 11 ++++++++-- app/models/spree/stock_item.rb | 10 +++++++++- app/models/spree/stock_location.rb | 20 ++++++++++--------- app/models/spree/variant.rb | 4 +--- ..._stock_location_id_on_spree_stock_items.rb | 7 +++++++ db/schema.rb | 2 +- .../order_management/stock/package.rb | 6 ++---- .../services/order_management/stock/packer.rb | 2 +- .../order_management/stock/package_spec.rb | 10 +++------- .../stock/prioritizer_spec.rb | 3 +-- spec/factories/stock_factory.rb | 2 +- spec/factories/stock_location_factory.rb | 6 +++--- spec/models/spree/stock_location_spec.rb | 15 -------------- 15 files changed, 51 insertions(+), 53 deletions(-) create mode 100644 db/migrate/20250108234658_allow_null_stock_location_id_on_spree_stock_items.rb diff --git a/app/models/spree/inventory_unit.rb b/app/models/spree/inventory_unit.rb index f5f0219919..7fb69152e6 100644 --- a/app/models/spree/inventory_unit.rb +++ b/app/models/spree/inventory_unit.rb @@ -57,8 +57,7 @@ module Spree end def find_stock_item - Spree::StockItem.find_by(stock_location_id: shipment.stock_location_id, - variant_id:) + Spree::StockItem.find_by(variant_id:) end private diff --git a/app/models/spree/order_inventory.rb b/app/models/spree/order_inventory.rb index e1e3e06182..6ce171ac9e 100644 --- a/app/models/spree/order_inventory.rb +++ b/app/models/spree/order_inventory.rb @@ -67,8 +67,7 @@ module Spree end target_shipment || order.shipments.detect do |shipment| - (shipment.ready? || shipment.pending?) && - variant.stock_location_ids.include?(shipment.stock_location_id) + shipment.ready? || shipment.pending? end end diff --git a/app/models/spree/shipment.rb b/app/models/spree/shipment.rb index 8e59029abc..9988095f0c 100644 --- a/app/models/spree/shipment.rb +++ b/app/models/spree/shipment.rb @@ -8,7 +8,9 @@ module Spree belongs_to :order, class_name: 'Spree::Order' belongs_to :address, class_name: 'Spree::Address' - belongs_to :stock_location, class_name: 'Spree::StockLocation' + + # WIP: phasing out stock location, it's not used. + belongs_to :stock_location, class_name: 'Spree::StockLocation', optional: true has_many :shipping_rates, dependent: :delete_all has_many :shipping_methods, through: :shipping_rates @@ -257,7 +259,7 @@ module Spree end def to_package - package = OrderManagement::Stock::Package.new(stock_location, order) + package = OrderManagement::Stock::Package.new(order) grouped_inventory_units = inventory_units.includes(:variant).group_by do |iu| [iu.variant, iu.state_name] end @@ -301,6 +303,11 @@ module Spree !shipped? && !order.canceled? end + # We still use the `unstock` and `restock` methods of the stock location. + def stock_location + @stock_location ||= DefaultStockLocation.find_or_create + end + private def line_items diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index a89a44975e..fc13d27b5c 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -4,7 +4,9 @@ module Spree class StockItem < ApplicationRecord acts_as_paranoid - belongs_to :stock_location, class_name: 'Spree::StockLocation', inverse_of: :stock_items + # WIP: phasing out stock location, it's not used. + belongs_to :stock_location, class_name: 'Spree::StockLocation', inverse_of: :stock_items, + optional: true belongs_to :variant, -> { with_deleted }, class_name: 'Spree::Variant' has_many :stock_movements, dependent: :destroy @@ -40,6 +42,12 @@ module Spree self[:count_on_hand] = value end + # Other code still calls this. + # TODO: remove usage + def stock_location + @stock_location ||= DefaultStockLocation.find_or_create + end + private def process_backorders diff --git a/app/models/spree/stock_location.rb b/app/models/spree/stock_location.rb index 268e3b084c..fd3e6475c8 100644 --- a/app/models/spree/stock_location.rb +++ b/app/models/spree/stock_location.rb @@ -13,11 +13,19 @@ module Spree validates :name, presence: true - after_create :create_stock_items - # Wrapper for creating a new stock item respecting the backorderable config def stock_item(variant) - stock_items.where(variant_id: variant).order(:id).first + StockItem.where(variant_id: variant).order(:id).first + end + + # We have only one default stock location and it may be unpersisted. + # So all stock items belong to any unpersisted stock location. + def stock_items + StockItem.all + end + + def stock_movements + StockMovement.all end def stock_item_or_create(variant) @@ -47,11 +55,5 @@ module Spree def fill_status(variant, quantity) variant.fill_status(quantity) end - - private - - def create_stock_items - Variant.find_each { |variant| stock_items.create!(variant:) } - end end end diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index acb430ecb6..eda45daa17 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -268,9 +268,7 @@ module Spree def create_stock_items return unless stock_items.empty? - StockLocation.find_each do |stock_location| - stock_items.create!(stock_location:) - end + stock_items.create! end def update_weight_from_unit_value diff --git a/db/migrate/20250108234658_allow_null_stock_location_id_on_spree_stock_items.rb b/db/migrate/20250108234658_allow_null_stock_location_id_on_spree_stock_items.rb new file mode 100644 index 0000000000..34900af1ec --- /dev/null +++ b/db/migrate/20250108234658_allow_null_stock_location_id_on_spree_stock_items.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AllowNullStockLocationIdOnSpreeStockItems < ActiveRecord::Migration[7.0] + def change + change_column_null :spree_stock_items, :stock_location_id, true + end +end diff --git a/db/schema.rb b/db/schema.rb index ea7efe830f..d97d554f99 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -822,7 +822,7 @@ ActiveRecord::Schema[7.0].define(version: 2025_01_13_055412) do end create_table "spree_stock_items", id: :serial, force: :cascade do |t| - t.integer "stock_location_id", null: false + t.integer "stock_location_id" t.integer "variant_id", null: false t.integer "count_on_hand", default: 0, null: false t.datetime "created_at", precision: nil, null: false diff --git a/engines/order_management/app/services/order_management/stock/package.rb b/engines/order_management/app/services/order_management/stock/package.rb index 2a937fa0bb..6db9db186b 100644 --- a/engines/order_management/app/services/order_management/stock/package.rb +++ b/engines/order_management/app/services/order_management/stock/package.rb @@ -5,11 +5,10 @@ module OrderManagement class Package ContentItem = Struct.new(:variant, :quantity, :state) - attr_reader :stock_location, :order, :contents + attr_reader :order, :contents attr_accessor :shipping_rates - def initialize(stock_location, order, contents = []) - @stock_location = stock_location + def initialize(order, contents = []) @order = order @contents = contents @shipping_rates = [] @@ -94,7 +93,6 @@ module OrderManagement def to_shipment shipment = Spree::Shipment.new shipment.order = order - shipment.stock_location = stock_location shipment.shipping_rates = shipping_rates contents.each do |item| diff --git a/engines/order_management/app/services/order_management/stock/packer.rb b/engines/order_management/app/services/order_management/stock/packer.rb index 87fe891c72..153debf8e9 100644 --- a/engines/order_management/app/services/order_management/stock/packer.rb +++ b/engines/order_management/app/services/order_management/stock/packer.rb @@ -11,7 +11,7 @@ module OrderManagement end def package - package = OrderManagement::Stock::Package.new(stock_location, order) + package = OrderManagement::Stock::Package.new(order) order.line_items.each do |line_item| next unless stock_location.stock_item(line_item.variant) diff --git a/engines/order_management/spec/services/order_management/stock/package_spec.rb b/engines/order_management/spec/services/order_management/stock/package_spec.rb index 22b97eb1ff..0aea97d177 100644 --- a/engines/order_management/spec/services/order_management/stock/package_spec.rb +++ b/engines/order_management/spec/services/order_management/stock/package_spec.rb @@ -7,11 +7,10 @@ module OrderManagement RSpec.describe Package do context "base tests" do let(:variant) { build(:variant, weight: 25.0) } - let(:stock_location) { build(:stock_location) } let(:distributor) { create(:enterprise) } let(:order) { build(:order, distributor:) } - subject { Package.new(stock_location, order) } + subject { Package.new(order) } it 'calculates the weight of all the contents' do subject.add variant, 4 @@ -80,7 +79,7 @@ module OrderManagement Package::ContentItem.new(variant2, 1), Package::ContentItem.new(variant3, 1)] - package = Package.new(stock_location, order, contents) + package = Package.new(order, contents) expect(package.shipping_methods.size).to eq 2 end @@ -96,7 +95,6 @@ module OrderManagement shipment = subject.to_shipment expect(shipment.order).to eq subject.order - expect(shipment.stock_location).to eq subject.stock_location expect(shipment.inventory_units.size).to eq 3 first_unit = shipment.inventory_units.first @@ -122,9 +120,7 @@ module OrderManagement end context "#shipping_methods and #shipping_categories" do - let(:stock_location) { double(:stock_location) } - - subject(:package) { Package.new(stock_location, order, contents) } + subject(:package) { Package.new(order, contents) } let(:enterprise) { create(:enterprise) } let(:other_enterprise) { create(:enterprise) } diff --git a/engines/order_management/spec/services/order_management/stock/prioritizer_spec.rb b/engines/order_management/spec/services/order_management/stock/prioritizer_spec.rb index c521eaad84..85883e8be9 100644 --- a/engines/order_management/spec/services/order_management/stock/prioritizer_spec.rb +++ b/engines/order_management/spec/services/order_management/stock/prioritizer_spec.rb @@ -6,12 +6,11 @@ module OrderManagement module Stock RSpec.describe Prioritizer do let(:order) { create(:order_with_line_items, line_items_count: 2) } - let(:stock_location) { build(:stock_location) } let(:variant1) { order.line_items[0].variant } let(:variant2) { order.line_items[1].variant } def pack - package = Package.new(order, stock_location) + package = Package.new(order) yield(package) if block_given? package end diff --git a/spec/factories/stock_factory.rb b/spec/factories/stock_factory.rb index 8497ec02f4..254eff8ee1 100644 --- a/spec/factories/stock_factory.rb +++ b/spec/factories/stock_factory.rb @@ -8,7 +8,7 @@ FactoryBot.define do contents { [] } end - initialize_with { new(stock_location, order, contents) } + initialize_with { new(order, contents) } factory :stock_package_fulfilled do after(:build) do |package, evaluator| diff --git a/spec/factories/stock_location_factory.rb b/spec/factories/stock_location_factory.rb index 466c4549a2..37d43d7237 100644 --- a/spec/factories/stock_location_factory.rb +++ b/spec/factories/stock_location_factory.rb @@ -17,15 +17,15 @@ FactoryBot.define do end factory :stock_location_with_items do - after(:create) do |stock_location, _evaluator| + after(:create) do |_stock_location, _evaluator| # variant will add itself to all stock_locations in an after_create # creating a product will automatically create a master variant product_1 = create(:product) product_2 = create(:product) - stock_location.stock_items.where(variant_id: product_1.variants.first.id) + Spree::StockItem.where(variant_id: product_1.variants.first.id) .first.adjust_count_on_hand(10) - stock_location.stock_items.where(variant_id: product_2.variants.first.id) + Spree::StockItem.where(variant_id: product_2.variants.first.id) .first.adjust_count_on_hand(20) end end diff --git a/spec/models/spree/stock_location_spec.rb b/spec/models/spree/stock_location_spec.rb index fd01212caa..c197adce18 100644 --- a/spec/models/spree/stock_location_spec.rb +++ b/spec/models/spree/stock_location_spec.rb @@ -12,21 +12,6 @@ module Spree expect(subject.stock_items.count).to eq Variant.count end - context "handling stock items" do - let!(:variant) { create(:variant) } - - context "given a variant" do - context "propagate all variants" do - subject { StockLocation.new(name: "testing") } - - specify do - expect(subject.stock_items).to receive(:create!).at_least(:once) - subject.save! - end - end - end - end - it 'finds a stock_item for a variant' do stock_item = subject.stock_item(variant) expect(stock_item.count_on_hand).to eq 15