From 248110cfb3885afb0e1fcd4ceb4bbc78ceebc7d9 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 9 Jan 2025 11:26:15 +1100 Subject: [PATCH 01/20] 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 From 68a0f8df1f77d1347a62274861e4b553721e9f30 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 9 Jan 2025 14:32:08 +1100 Subject: [PATCH 02/20] Remove detour via StockLocation updating stock --- app/models/spree/order_inventory.rb | 8 ++++---- app/models/spree/shipment.rb | 6 +++--- app/models/spree/stock_location.rb | 12 ------------ spec/models/spree/order_inventory_spec.rb | 6 +++--- spec/models/spree/shipment_spec.rb | 4 ++-- spec/models/spree/stock_location_spec.rb | 19 ------------------- .../admin/bulk_order_management_spec.rb | 6 +++--- spec/system/admin/order_spec.rb | 10 ++++++---- 8 files changed, 21 insertions(+), 50 deletions(-) diff --git a/app/models/spree/order_inventory.rb b/app/models/spree/order_inventory.rb index 6ce171ac9e..c2f28a6549 100644 --- a/app/models/spree/order_inventory.rb +++ b/app/models/spree/order_inventory.rb @@ -12,8 +12,8 @@ module Spree # have inventory assigned via +order.create_proposed_shipment+) or when # shipment is explicitly passed # - # In case shipment is passed the stock location should only unstock or - # restock items if the order is completed. That is so because stock items + # In case shipment is passed stock should only be adjusted + # if the order is completed. That is so because stock items # are always unstocked when the order is completed through +shipment.finalize+ def verify(line_item, shipment = nil) if order.completed? || shipment.present? @@ -79,7 +79,7 @@ module Spree # adding to this shipment, and removing from stock_location if order.completed? - shipment.stock_location.unstock(variant, quantity, shipment) + variant.move(-quantity, shipment) end quantity @@ -104,7 +104,7 @@ module Spree # removing this from shipment, and adding to stock_location if order.completed? && restock_item - shipment.stock_location.restock variant, removed_quantity, shipment + variant.move(removed_quantity, shipment) end removed_quantity diff --git a/app/models/spree/shipment.rb b/app/models/spree/shipment.rb index 9988095f0c..711eedcb48 100644 --- a/app/models/spree/shipment.rb +++ b/app/models/spree/shipment.rb @@ -303,7 +303,7 @@ module Spree !shipped? && !order.canceled? end - # We still use the `unstock` and `restock` methods of the stock location. + # Other code still calls this for convenience. def stock_location @stock_location ||= DefaultStockLocation.find_or_create end @@ -320,11 +320,11 @@ module Spree end def manifest_unstock(item) - stock_location.unstock item.variant, item.quantity, self + item.variant.move(-1 * item.quantity, self) end def manifest_restock(item) - stock_location.restock item.variant, item.quantity, self + item.variant.move(item.quantity, self) end def generate_shipment_number diff --git a/app/models/spree/stock_location.rb b/app/models/spree/stock_location.rb index fd3e6475c8..1d378da4b2 100644 --- a/app/models/spree/stock_location.rb +++ b/app/models/spree/stock_location.rb @@ -40,18 +40,6 @@ module Spree stock_item(variant).try(:backorderable?) end - def restock(variant, quantity, originator = nil) - move(variant, quantity, originator) - end - - def unstock(variant, quantity, originator = nil) - move(variant, -quantity, originator) - end - - def move(variant, quantity, originator = nil) - variant.move(quantity, originator) - end - def fill_status(variant, quantity) variant.fill_status(quantity) end diff --git a/spec/models/spree/order_inventory_spec.rb b/spec/models/spree/order_inventory_spec.rb index 814cca78d6..dade323c93 100644 --- a/spec/models/spree/order_inventory_spec.rb +++ b/spec/models/spree/order_inventory_spec.rb @@ -36,7 +36,7 @@ RSpec.describe Spree::OrderInventory do before { allow(order).to receive_messages completed?: false } it "doesn't unstock items" do - expect(shipment.stock_location).not_to receive(:unstock) + expect(line_item.variant).not_to receive(:move) expect(subject.__send__(:add_to_shipment, shipment, variant, 5)).to eq 5 end end @@ -88,7 +88,7 @@ RSpec.describe Spree::OrderInventory do before { allow(order).to receive_messages completed?: false } it "doesn't restock items" do - expect(shipment.stock_location).not_to receive(:restock) + expect(variant).not_to receive(:move) expect(subject.__send__(:remove_from_shipment, shipment, variant, 1, true)).to eq 1 end end @@ -97,7 +97,7 @@ RSpec.describe Spree::OrderInventory do before { allow(order).to receive_messages completed?: true } it "doesn't restock items" do - expect(shipment.stock_location).not_to receive(:restock) + expect(variant).not_to receive(:move) expect(subject.__send__(:remove_from_shipment, shipment, variant, 1, false)).to eq 1 end end diff --git a/spec/models/spree/shipment_spec.rb b/spec/models/spree/shipment_spec.rb index ecaba70205..840b293282 100644 --- a/spec/models/spree/shipment_spec.rb +++ b/spec/models/spree/shipment_spec.rb @@ -325,7 +325,7 @@ RSpec.describe Spree::Shipment do :group_by, map: [unit]) shipment.stock_location = build(:stock_location) - expect(shipment.stock_location).to receive(:restock).with(variant, 1, shipment) + expect(variant).to receive(:move).with(1, shipment) shipment.after_cancel end end @@ -349,7 +349,7 @@ RSpec.describe Spree::Shipment do :group_by, map: [unit]) shipment.stock_location = create(:stock_location) - expect(shipment.stock_location).to receive(:unstock).with(variant, 1, shipment) + expect(variant).to receive(:move).with(-1, shipment) shipment.after_resume end diff --git a/spec/models/spree/stock_location_spec.rb b/spec/models/spree/stock_location_spec.rb index c197adce18..1666bf02ae 100644 --- a/spec/models/spree/stock_location_spec.rb +++ b/spec/models/spree/stock_location_spec.rb @@ -35,25 +35,6 @@ module Spree expect(subject.backorderable?(variant)).to eq false end - it 'restocks a variant with a positive stock movement' do - originator = double - expect(subject).to receive(:move).with(variant, 5, originator) - subject.restock(variant, 5, originator) - end - - it 'unstocks a variant with a negative stock movement' do - originator = double - expect(subject).to receive(:move).with(variant, -5, originator) - subject.unstock(variant, 5, originator) - end - - it 'it creates a stock_movement' do - variant.on_demand = false - expect { - subject.move variant, 5 - }.to change { subject.stock_movements.where(stock_item_id: stock_item).count }.by(1) - end - context 'fill_status' do before { variant.on_demand = false } diff --git a/spec/system/admin/bulk_order_management_spec.rb b/spec/system/admin/bulk_order_management_spec.rb index e8af8337c9..95af4bac5b 100644 --- a/spec/system/admin/bulk_order_management_spec.rb +++ b/spec/system/admin/bulk_order_management_spec.rb @@ -1100,7 +1100,7 @@ RSpec.describe ' end it "the user can confirm : line item is then deleted and order is canceled" do - expect_any_instance_of(Spree::StockLocation).to receive(:restock).at_least(1).times + expect_any_instance_of(Spree::Variant).to receive(:move).at_least(1).times expect do within(".modal") do uncheck("send_cancellation_email") @@ -1113,7 +1113,7 @@ RSpec.describe ' it "the user can confirm + wants to send email confirmation : line item is " \ "then deleted, order is canceled and email is sent" do - expect_any_instance_of(Spree::StockLocation).to receive(:restock).at_least(1).times + expect_any_instance_of(Spree::Variant).to receive(:move).at_least(1).times expect do within(".modal") do check("send_cancellation_email") @@ -1126,7 +1126,7 @@ RSpec.describe ' it "the user can confirm + uncheck the restock option: line item is then deleted and " \ "order is canceled without retocking" do - expect_any_instance_of(Spree::StockLocation).not_to receive(:restock) + expect_any_instance_of(Spree::Variant).not_to receive(:move) expect do within(".modal") do uncheck("Restock Items: return all items to stock") diff --git a/spec/system/admin/order_spec.rb b/spec/system/admin/order_spec.rb index 643c73d585..3ffde208a9 100644 --- a/spec/system/admin/order_spec.rb +++ b/spec/system/admin/order_spec.rb @@ -218,7 +218,6 @@ RSpec.describe ' let(:shipment) { order.shipments.first } it "and by default an Email is sent and the items are restocked" do - expect_any_instance_of(Spree::StockLocation).to receive(:restock).at_least(1).times expect do within(".modal") do click_on("OK") @@ -226,10 +225,10 @@ RSpec.describe ' expect(page).to have_content "Cannot add item to canceled order" expect(order.reload.state).to eq("canceled") end.to have_enqueued_mail(Spree::OrderMailer, :cancel_email) + .and change { Spree::StockItem.pluck(:count_on_hand) } end it "and then the order is cancelled and email is not sent when unchecked" do - expect_any_instance_of(Spree::StockLocation).to receive(:restock).at_least(1).times expect do within(".modal") do uncheck("send_cancellation_email") @@ -237,11 +236,12 @@ RSpec.describe ' end expect(page).to have_content "Cannot add item to canceled order" expect(order.reload.state).to eq("canceled") - end.not_to have_enqueued_mail(Spree::OrderMailer, :cancel_email) + end.to have_enqueued_mail(Spree::OrderMailer, :cancel_email).at_most(0).times + .and change { Spree::StockItem.pluck(:count_on_hand) } end it "and the items are not restocked when the user uncheck the checkbox to restock items" do - expect_any_instance_of(Spree::StockLocation).not_to receive(:restock) + expect_any_instance_of(Spree::Variant).not_to receive(:move) expect do within(".modal") do uncheck("restock_items") @@ -250,6 +250,8 @@ RSpec.describe ' expect(page).to have_content "Cannot add item to canceled order" expect(order.reload.state).to eq("canceled") end.to have_enqueued_mail(Spree::OrderMailer, :cancel_email) + # Not change stock. Rspec can't combine `to` and `not_to` though. + .and change { Spree::StockItem.pluck(:count_on_hand) }.by([]) end end end From a4b92f289cbaff54360d8e319a601492a164537b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 17 Jan 2025 12:54:05 +1100 Subject: [PATCH 03/20] Move fill_status spec to prepare removal from StockLocation --- spec/models/spree/stock_location_spec.rb | 32 ---------------------- spec/models/spree/variant_stock_spec.rb | 34 +++++++++++++++++++++++- 2 files changed, 33 insertions(+), 33 deletions(-) diff --git a/spec/models/spree/stock_location_spec.rb b/spec/models/spree/stock_location_spec.rb index 1666bf02ae..77a555ed0a 100644 --- a/spec/models/spree/stock_location_spec.rb +++ b/spec/models/spree/stock_location_spec.rb @@ -34,37 +34,5 @@ module Spree it 'finds determines if you a variant is backorderable' do expect(subject.backorderable?(variant)).to eq false end - - context 'fill_status' do - before { variant.on_demand = false } - - it 'is all on_hand if variant is on_demand' do - variant.on_demand = true - - on_hand, backordered = subject.fill_status(variant, 25) - expect(on_hand).to eq 25 - expect(backordered).to eq 0 - end - - it 'is all on_hand if on_hand is enough' do - on_hand, backordered = subject.fill_status(variant, 5) - expect(on_hand).to eq 5 - expect(backordered).to eq 0 - end - - it 'is some on_hand if not all available' do - on_hand, backordered = subject.fill_status(variant, 20) - expect(on_hand).to eq 15 - expect(backordered).to eq 0 - end - - it 'is zero on_hand if none available' do - variant.on_hand = 0 - - on_hand, backordered = subject.fill_status(variant, 20) - expect(on_hand).to eq 0 - expect(backordered).to eq 0 - end - end end end diff --git a/spec/models/spree/variant_stock_spec.rb b/spec/models/spree/variant_stock_spec.rb index 224fa3e855..4597fd1114 100644 --- a/spec/models/spree/variant_stock_spec.rb +++ b/spec/models/spree/variant_stock_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Spree::Variant do - # This method is defined in app/models/concerns/variant_stock.rb. + # These methods are 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. # @@ -11,6 +11,38 @@ RSpec.describe Spree::Variant do # like this one get overridden. Future calls to `variant.move` are then # handled by the ScopeVariantToHub module which may call the # VariantOverride. + describe "#fill_status" do + subject(:variant) { create(:variant, on_hand: 15) } + + it 'is all on_hand if variant is on_demand' do + variant.on_demand = true + + on_hand, backordered = subject.fill_status(25) + expect(on_hand).to eq 25 + expect(backordered).to eq 0 + end + + it 'is all on_hand if on_hand is enough' do + on_hand, backordered = subject.fill_status(5) + expect(on_hand).to eq 5 + expect(backordered).to eq 0 + end + + it 'is some on_hand if not all available' do + on_hand, backordered = subject.fill_status(20) + expect(on_hand).to eq 15 + expect(backordered).to eq 0 + end + + it 'is zero on_hand if none available' do + variant.on_hand = 0 + + on_hand, backordered = subject.fill_status(20) + expect(on_hand).to eq 0 + expect(backordered).to eq 0 + end + end + describe "#move" do subject(:variant) { create(:variant, on_hand: 5) } From 531c068347c952760d4686b4adc1384aecac2b94 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 17 Jan 2025 13:00:17 +1100 Subject: [PATCH 04/20] Remove StockLocation#fill_status, now on Variant --- app/models/spree/order_inventory.rb | 2 +- app/models/spree/stock_location.rb | 4 ---- .../app/services/order_management/stock/packer.rb | 2 +- .../spec/services/order_management/stock/packer_spec.rb | 4 +++- spec/models/spree/order_inventory_spec.rb | 2 +- 5 files changed, 6 insertions(+), 8 deletions(-) diff --git a/app/models/spree/order_inventory.rb b/app/models/spree/order_inventory.rb index c2f28a6549..02a1dc8286 100644 --- a/app/models/spree/order_inventory.rb +++ b/app/models/spree/order_inventory.rb @@ -72,7 +72,7 @@ module Spree end def add_to_shipment(shipment, variant, quantity) - on_hand, back_order = shipment.stock_location.fill_status(variant, quantity) + on_hand, back_order = variant.fill_status(quantity) on_hand.times { shipment.set_up_inventory('on_hand', variant, order) } back_order.times { shipment.set_up_inventory('backordered', variant, order) } diff --git a/app/models/spree/stock_location.rb b/app/models/spree/stock_location.rb index 1d378da4b2..b52288ae4b 100644 --- a/app/models/spree/stock_location.rb +++ b/app/models/spree/stock_location.rb @@ -39,9 +39,5 @@ module Spree def backorderable?(variant) stock_item(variant).try(:backorderable?) end - - def fill_status(variant, quantity) - variant.fill_status(quantity) - end end end 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 153debf8e9..b0c2fa7299 100644 --- a/engines/order_management/app/services/order_management/stock/packer.rb +++ b/engines/order_management/app/services/order_management/stock/packer.rb @@ -18,7 +18,7 @@ module OrderManagement variant = line_item.variant OpenFoodNetwork::ScopeVariantToHub.new(order.distributor).scope(variant) - on_hand, backordered = stock_location.fill_status(variant, line_item.quantity) + on_hand, backordered = variant.fill_status(line_item.quantity) package.add variant, on_hand, :on_hand if on_hand.positive? package.add variant, backordered, :backordered if backordered.positive? end diff --git a/engines/order_management/spec/services/order_management/stock/packer_spec.rb b/engines/order_management/spec/services/order_management/stock/packer_spec.rb index 1b0895bc28..7cb6ec6260 100644 --- a/engines/order_management/spec/services/order_management/stock/packer_spec.rb +++ b/engines/order_management/spec/services/order_management/stock/packer_spec.rb @@ -21,7 +21,9 @@ module OrderManagement end it 'variants are added as backordered without enough on_hand' do - expect(stock_location).to receive(:fill_status).exactly(5).times.and_return([2, 3]) + order.line_items.each do |item| + expect(item.variant).to receive(:fill_status).and_return([2, 3]) + end package = subject.package expect(package.on_hand.size).to eq 5 diff --git a/spec/models/spree/order_inventory_spec.rb b/spec/models/spree/order_inventory_spec.rb index dade323c93..efb7670232 100644 --- a/spec/models/spree/order_inventory_spec.rb +++ b/spec/models/spree/order_inventory_spec.rb @@ -42,7 +42,7 @@ RSpec.describe Spree::OrderInventory do end it 'should create inventory_units in the necessary states' do - expect(shipment.stock_location).to receive(:fill_status).with(variant, 5).and_return([3, 2]) + expect(variant).to receive(:fill_status).with(5).and_return([3, 2]) expect(subject.__send__(:add_to_shipment, shipment, variant, 5)).to eq 5 From 1650ccd55a2ad5152446fb12173e984d0f4e3ed0 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 17 Jan 2025 13:52:00 +1100 Subject: [PATCH 05/20] Remove unused StockLocation#backorderable? --- app/models/spree/stock_location.rb | 4 ---- spec/models/spree/stock_location_spec.rb | 4 ---- 2 files changed, 8 deletions(-) diff --git a/app/models/spree/stock_location.rb b/app/models/spree/stock_location.rb index b52288ae4b..fe3044c654 100644 --- a/app/models/spree/stock_location.rb +++ b/app/models/spree/stock_location.rb @@ -35,9 +35,5 @@ module Spree def count_on_hand(variant) stock_item(variant).try(:count_on_hand) end - - def backorderable?(variant) - stock_item(variant).try(:backorderable?) - end end end diff --git a/spec/models/spree/stock_location_spec.rb b/spec/models/spree/stock_location_spec.rb index 77a555ed0a..c885bc8543 100644 --- a/spec/models/spree/stock_location_spec.rb +++ b/spec/models/spree/stock_location_spec.rb @@ -30,9 +30,5 @@ module Spree it 'finds a count_on_hand for a variant' do expect(subject.count_on_hand(variant)).to eq 15 end - - it 'finds determines if you a variant is backorderable' do - expect(subject.backorderable?(variant)).to eq false - end end end From 450576a93818836a8bbfc9f90856a765288aa403 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 17 Jan 2025 14:48:59 +1100 Subject: [PATCH 06/20] Remove unused StockLocation#count_on_hand --- app/models/spree/stock_location.rb | 4 ---- spec/models/spree/stock_location_spec.rb | 4 ---- 2 files changed, 8 deletions(-) diff --git a/app/models/spree/stock_location.rb b/app/models/spree/stock_location.rb index fe3044c654..5a477e98aa 100644 --- a/app/models/spree/stock_location.rb +++ b/app/models/spree/stock_location.rb @@ -31,9 +31,5 @@ module Spree def stock_item_or_create(variant) stock_item(variant) || stock_items.create(variant:) end - - def count_on_hand(variant) - stock_item(variant).try(:count_on_hand) - end end end diff --git a/spec/models/spree/stock_location_spec.rb b/spec/models/spree/stock_location_spec.rb index c885bc8543..1e76d1f334 100644 --- a/spec/models/spree/stock_location_spec.rb +++ b/spec/models/spree/stock_location_spec.rb @@ -26,9 +26,5 @@ module Spree stock_item = subject.stock_item(100) expect(stock_item).to be_nil end - - it 'finds a count_on_hand for a variant' do - expect(subject.count_on_hand(variant)).to eq 15 - end end end From 34fdcface264bd31a6d3af33d4f0c98513c3bb42 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 17 Jan 2025 14:55:19 +1100 Subject: [PATCH 07/20] Remove unused StockLocation#stock_item_or_create --- app/models/spree/stock_location.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/models/spree/stock_location.rb b/app/models/spree/stock_location.rb index 5a477e98aa..6543939bc9 100644 --- a/app/models/spree/stock_location.rb +++ b/app/models/spree/stock_location.rb @@ -27,9 +27,5 @@ module Spree def stock_movements StockMovement.all end - - def stock_item_or_create(variant) - stock_item(variant) || stock_items.create(variant:) - end end end From 6dfa184a150d6326141e7c6546378d9654e13981 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 17 Jan 2025 14:57:14 +1100 Subject: [PATCH 08/20] Remove unused StockLocation#stock_movements --- app/models/spree/stock_location.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/models/spree/stock_location.rb b/app/models/spree/stock_location.rb index 6543939bc9..ad1bd359a3 100644 --- a/app/models/spree/stock_location.rb +++ b/app/models/spree/stock_location.rb @@ -6,7 +6,6 @@ module Spree self.ignored_columns += [:backorderable_default, :active] has_many :stock_items, dependent: :delete_all, inverse_of: :stock_location - has_many :stock_movements, through: :stock_items belongs_to :state, class_name: 'Spree::State' belongs_to :country, class_name: 'Spree::Country' @@ -23,9 +22,5 @@ module Spree def stock_items StockItem.all end - - def stock_movements - StockMovement.all - end end end From aa9daed66e1d77831b09a629a8d6c24f8d87045a Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 17 Jan 2025 15:03:39 +1100 Subject: [PATCH 09/20] Remove unused StockLocation#stock_items And the reverse association. --- app/models/spree/stock_item.rb | 5 ++--- app/models/spree/stock_location.rb | 8 -------- app/models/spree/variant.rb | 1 - app/queries/product_scope_query.rb | 2 +- app/services/products_renderer.rb | 2 +- spec/controllers/api/v0/products_controller_spec.rb | 6 ++---- spec/models/concerns/variant_stock_spec.rb | 6 ------ spec/models/spree/inventory_unit_spec.rb | 4 ++-- spec/models/spree/stock_item_spec.rb | 6 ++---- spec/models/spree/stock_location_spec.rb | 6 +----- spec/models/spree/stock_movement_spec.rb | 4 ++-- spec/models/spree/variant_spec.rb | 1 - 12 files changed, 13 insertions(+), 38 deletions(-) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index fc13d27b5c..fa586a0741 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -4,9 +4,6 @@ module Spree class StockItem < ApplicationRecord acts_as_paranoid - # 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 @@ -48,6 +45,8 @@ module Spree @stock_location ||= DefaultStockLocation.find_or_create end + attr_writer :stock_location + private def process_backorders diff --git a/app/models/spree/stock_location.rb b/app/models/spree/stock_location.rb index ad1bd359a3..b7b0547328 100644 --- a/app/models/spree/stock_location.rb +++ b/app/models/spree/stock_location.rb @@ -5,8 +5,6 @@ module Spree self.belongs_to_required_by_default = false self.ignored_columns += [:backorderable_default, :active] - has_many :stock_items, dependent: :delete_all, inverse_of: :stock_location - belongs_to :state, class_name: 'Spree::State' belongs_to :country, class_name: 'Spree::Country' @@ -16,11 +14,5 @@ module Spree def stock_item(variant) 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 end end diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index eda45daa17..acf0e78408 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -39,7 +39,6 @@ module Spree has_many :line_items, inverse_of: :variant, dependent: nil has_many :stock_items, dependent: :destroy, inverse_of: :variant - has_many :stock_locations, through: :stock_items has_many :images, -> { order(:position) }, as: :viewable, dependent: :destroy, class_name: "Spree::Image" diff --git a/app/queries/product_scope_query.rb b/app/queries/product_scope_query.rb index 678104695c..bdadbc3401 100644 --- a/app/queries/product_scope_query.rb +++ b/app/queries/product_scope_query.rb @@ -61,7 +61,7 @@ class ProductScopeQuery def product_query_includes [ image: { attachment_attachment: :blob }, - variants: [:default_price, :stock_locations, :stock_items, :variant_overrides] + variants: [:default_price, :stock_items, :variant_overrides] ] end diff --git a/app/services/products_renderer.rb b/app/services/products_renderer.rb index 339d16c925..90b9e8e029 100644 --- a/app/services/products_renderer.rb +++ b/app/services/products_renderer.rb @@ -117,7 +117,7 @@ class ProductsRenderer # rubocop:disable Rails/FindEach # .each returns an array, .find_each returns nil distributed_products.variants_relation. - includes(:default_price, :stock_locations, :product). + includes(:default_price, :product). where(product_id: products). each { |v| scoper.scope(v) } # Scope results with variant_overrides # rubocop:enable Rails/FindEach diff --git a/spec/controllers/api/v0/products_controller_spec.rb b/spec/controllers/api/v0/products_controller_spec.rb index 4c45845b8d..9e961e9ae0 100644 --- a/spec/controllers/api/v0/products_controller_spec.rb +++ b/spec/controllers/api/v0/products_controller_spec.rb @@ -42,10 +42,8 @@ RSpec.describe Api::V0::ProductsController, type: :controller do api_get :show, id: product.to_param - expect(all_attributes.all?{ |attr| json_response.keys.include? attr }).to eq(true) - expect(variants_attributes.all?{ |attr| - json_response['variants'].first.keys.include? attr - } ).to eq(true) + expect(json_response.keys).to include(*all_attributes) + expect(json_response["variants"].first.keys).to include(*variants_attributes) end it "returns a 404 error when it cannot find a product" do diff --git a/spec/models/concerns/variant_stock_spec.rb b/spec/models/concerns/variant_stock_spec.rb index eeae48fcab..26f650d86c 100644 --- a/spec/models/concerns/variant_stock_spec.rb +++ b/spec/models/concerns/variant_stock_spec.rb @@ -83,7 +83,6 @@ RSpec.describe VariantStock do it 'returns false' do variant = build_stubbed( :variant, - stock_locations: [build_stubbed(:stock_location)] ) expect(variant.on_demand).to be_falsy end @@ -94,9 +93,6 @@ RSpec.describe VariantStock do let(:variant) do build_stubbed( :variant, - stock_locations: [ - build_stubbed(:stock_location) - ] ) end @@ -148,7 +144,6 @@ RSpec.describe VariantStock do build_stubbed( :variant, on_demand: true, - stock_locations: [build_stubbed(:stock_location)] ) end let(:stock_item) { Spree::StockItem.new(backorderable: true) } @@ -172,7 +167,6 @@ RSpec.describe VariantStock do build_stubbed( :variant, on_demand: false, - stock_locations: [build_stubbed(:stock_location)] ) end diff --git a/spec/models/spree/inventory_unit_spec.rb b/spec/models/spree/inventory_unit_spec.rb index e4743b652e..d4ea6b3622 100644 --- a/spec/models/spree/inventory_unit_spec.rb +++ b/spec/models/spree/inventory_unit_spec.rb @@ -3,8 +3,8 @@ require 'spec_helper' RSpec.describe Spree::InventoryUnit do - let(:stock_location) { create(:stock_location_with_items) } - let(:stock_item) { stock_location.stock_items.order(:id).first } + let!(:stock_location) { create(:stock_location_with_items) } + let(:stock_item) { Spree::StockItem.order(:id).first } context "#backordered_for_stock_item" do let(:order) { create(:order) } diff --git a/spec/models/spree/stock_item_spec.rb b/spec/models/spree/stock_item_spec.rb index c3f94ef71a..fd3f74965f 100644 --- a/spec/models/spree/stock_item_spec.rb +++ b/spec/models/spree/stock_item_spec.rb @@ -3,13 +3,11 @@ require 'spec_helper' RSpec.describe Spree::StockItem do - let(:stock_location) { create(:stock_location_with_items) } + let!(:stock_location) { create(:stock_location_with_items) } - subject { stock_location.stock_items.order(:id).first } + subject(:stock_item) { Spree::StockItem.order(:id).first } describe "validation" do - let(:stock_item) { stock_location.stock_items.first } - it "requires count_on_hand to be positive if not backorderable" do stock_item.backorderable = false diff --git a/spec/models/spree/stock_location_spec.rb b/spec/models/spree/stock_location_spec.rb index 1e76d1f334..a9189045cc 100644 --- a/spec/models/spree/stock_location_spec.rb +++ b/spec/models/spree/stock_location_spec.rb @@ -5,13 +5,9 @@ require 'spec_helper' module Spree RSpec.describe StockLocation do subject { create(:stock_location_with_items) } - let(:stock_item) { subject.stock_items.order(:id).first } + let(:stock_item) { StockItem.order(:id).first } let(:variant) { stock_item.variant } - it 'creates stock_items for all variants' do - expect(subject.stock_items.count).to eq Variant.count - 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 diff --git a/spec/models/spree/stock_movement_spec.rb b/spec/models/spree/stock_movement_spec.rb index 19ff09341c..235f542db2 100644 --- a/spec/models/spree/stock_movement_spec.rb +++ b/spec/models/spree/stock_movement_spec.rb @@ -3,8 +3,8 @@ require 'spec_helper' RSpec.describe Spree::StockMovement do - let(:stock_location) { create(:stock_location_with_items) } - let(:stock_item) { stock_location.stock_items.order(:id).first } + let!(:stock_location) { create(:stock_location_with_items) } + let(:stock_item) { Spree::StockItem.order(:id).first } subject { build(:stock_movement, stock_item:) } it 'should belong to a stock item' do diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 70068b4164..97a3d4695e 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -12,7 +12,6 @@ RSpec.describe Spree::Variant do it { is_expected.to have_many(:inventory_units) } it { is_expected.to have_many(:line_items) } it { is_expected.to have_many(:stock_items) } - it { is_expected.to have_many(:stock_locations).through(:stock_items) } it { is_expected.to have_many(:images) } it { is_expected.to have_one(:default_price) } it { is_expected.to have_many(:prices) } From 33c6f3b94f77233385e440401e5700ada7c457e3 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 17 Jan 2025 15:12:18 +1100 Subject: [PATCH 10/20] Remove StockLocation#stock_item --- app/models/concerns/variant_stock.rb | 12 ++++----- app/models/spree/stock_location.rb | 5 ---- .../services/order_management/stock/packer.rb | 4 +-- spec/models/spree/order_inventory_spec.rb | 6 ++--- spec/models/spree/stock_location_spec.rb | 26 ------------------- 5 files changed, 10 insertions(+), 43 deletions(-) delete mode 100644 spec/models/spree/stock_location_spec.rb diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb index 710095502f..d3837a1ce5 100644 --- a/app/models/concerns/variant_stock.rb +++ b/app/models/concerns/variant_stock.rb @@ -114,6 +114,12 @@ module VariantStock stock_item.stock_movements.create!(quantity:, originator:) end + # There shouldn't be any other stock items, because we should + # have only one stock location. + def stock_item + stock_items.first + end + private # Persists the single stock item associated to this variant. As defined in @@ -139,10 +145,4 @@ module VariantStock def overwrite_stock_levels(new_level) stock_item.adjust_count_on_hand(new_level.to_i - stock_item.count_on_hand) end - - # There shouldn't be any other stock items, because we should - # have only one stock location. - def stock_item - stock_items.first - end end diff --git a/app/models/spree/stock_location.rb b/app/models/spree/stock_location.rb index b7b0547328..9640ef474f 100644 --- a/app/models/spree/stock_location.rb +++ b/app/models/spree/stock_location.rb @@ -9,10 +9,5 @@ module Spree belongs_to :country, class_name: 'Spree::Country' validates :name, presence: true - - # Wrapper for creating a new stock item respecting the backorderable config - def stock_item(variant) - StockItem.where(variant_id: variant).order(:id).first - end end end 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 b0c2fa7299..6f168fa183 100644 --- a/engines/order_management/app/services/order_management/stock/packer.rb +++ b/engines/order_management/app/services/order_management/stock/packer.rb @@ -13,9 +13,9 @@ module OrderManagement def package package = OrderManagement::Stock::Package.new(order) order.line_items.each do |line_item| - next unless stock_location.stock_item(line_item.variant) - variant = line_item.variant + next unless variant.stock_item + OpenFoodNetwork::ScopeVariantToHub.new(order.distributor).scope(variant) on_hand, backordered = variant.fill_status(line_item.quantity) diff --git a/spec/models/spree/order_inventory_spec.rb b/spec/models/spree/order_inventory_spec.rb index efb7670232..cc7d520bca 100644 --- a/spec/models/spree/order_inventory_spec.rb +++ b/spec/models/spree/order_inventory_spec.rb @@ -55,8 +55,7 @@ RSpec.describe Spree::OrderInventory do it 'should create stock_movement' do expect(subject.__send__(:add_to_shipment, shipment, variant, 5)).to eq 5 - stock_item = shipment.stock_location.stock_item(variant) - movement = stock_item.stock_movements.last + movement = variant.stock_item.stock_movements.last expect(movement.quantity).to eq(-5) end end @@ -105,8 +104,7 @@ RSpec.describe Spree::OrderInventory do it 'should create stock_movement' do expect(subject.__send__(:remove_from_shipment, shipment, variant, 1, true)).to eq 1 - stock_item = shipment.stock_location.stock_item(variant) - movement = stock_item.stock_movements.last + movement = variant.stock_item.stock_movements.last expect(movement.quantity).to eq 1 end diff --git a/spec/models/spree/stock_location_spec.rb b/spec/models/spree/stock_location_spec.rb deleted file mode 100644 index a9189045cc..0000000000 --- a/spec/models/spree/stock_location_spec.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -module Spree - RSpec.describe StockLocation do - subject { create(:stock_location_with_items) } - let(:stock_item) { StockItem.order(:id).first } - let(:variant) { stock_item.variant } - - it 'finds a stock_item for a variant' do - stock_item = subject.stock_item(variant) - expect(stock_item.count_on_hand).to eq 15 - end - - it 'finds a stock_item for a variant by id' do - stock_item = subject.stock_item(variant.id) - expect(stock_item.variant).to eq variant - end - - it 'returns nil when stock_item is not found for variant' do - stock_item = subject.stock_item(100) - expect(stock_item).to be_nil - end - end -end From 87d20877ade68ee8a598a19fad6d079d501845f9 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 21 Jan 2025 15:03:06 +1100 Subject: [PATCH 11/20] Remove Shipment#stock_location --- app/controllers/api/v0/shipments_controller.rb | 6 +----- app/models/spree/inventory_unit.rb | 4 +--- app/models/spree/shipment.rb | 9 +-------- app/serializers/api/shipment_serializer.rb | 6 +----- app/views/spree/admin/orders/_form.html.haml | 2 +- .../api/v0/shipments_controller_spec.rb | 14 +++++--------- spec/factories/shipment_factory.rb | 2 -- spec/factories/stock_factory.rb | 1 - spec/models/spree/inventory_unit_spec.rb | 1 - spec/models/spree/shipment_spec.rb | 2 -- 10 files changed, 10 insertions(+), 37 deletions(-) diff --git a/app/controllers/api/v0/shipments_controller.rb b/app/controllers/api/v0/shipments_controller.rb index b041eacd97..1580e1f89f 100644 --- a/app/controllers/api/v0/shipments_controller.rb +++ b/app/controllers/api/v0/shipments_controller.rb @@ -14,7 +14,7 @@ module Api def create variant = scoped_variant(params[:variant_id]) quantity = params[:quantity].to_i - @shipment = get_or_create_shipment(params[:stock_location_id]) + @shipment = @order.shipment || @order.shipments.create @order.contents.add(variant, quantity, @shipment) @@ -116,10 +116,6 @@ module Api variant end - def get_or_create_shipment(stock_location_id) - @order.shipment || @order.shipments.create(stock_location_id:) - end - def shipment_params return {} unless params.has_key? :shipment diff --git a/app/models/spree/inventory_unit.rb b/app/models/spree/inventory_unit.rb index 7fb69152e6..0f66dbb8dd 100644 --- a/app/models/spree/inventory_unit.rb +++ b/app/models/spree/inventory_unit.rb @@ -42,9 +42,7 @@ module Spree # # Returns an array of backordered inventory units as per a given stock item def self.backordered_for_stock_item(stock_item) - backordered_per_variant(stock_item).select do |unit| - unit.shipment.stock_location == stock_item.stock_location - end + backordered_per_variant(stock_item) end def self.finalize_units!(inventory_units) diff --git a/app/models/spree/shipment.rb b/app/models/spree/shipment.rb index 711eedcb48..5b9692912c 100644 --- a/app/models/spree/shipment.rb +++ b/app/models/spree/shipment.rb @@ -5,13 +5,11 @@ require 'ostruct' module Spree class Shipment < ApplicationRecord self.belongs_to_required_by_default = false + self.ignored_columns += [:stock_location_id] belongs_to :order, class_name: 'Spree::Order' belongs_to :address, class_name: 'Spree::Address' - # 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 has_many :state_changes, as: :stateful, dependent: :destroy @@ -303,11 +301,6 @@ module Spree !shipped? && !order.canceled? end - # Other code still calls this for convenience. - def stock_location - @stock_location ||= DefaultStockLocation.find_or_create - end - private def line_items diff --git a/app/serializers/api/shipment_serializer.rb b/app/serializers/api/shipment_serializer.rb index 1c03d9eb5e..278112bbf8 100644 --- a/app/serializers/api/shipment_serializer.rb +++ b/app/serializers/api/shipment_serializer.rb @@ -2,14 +2,10 @@ module Api class ShipmentSerializer < ActiveModel::Serializer - attributes :id, :tracking, :number, :order_id, :cost, :shipped_at, :stock_location_name, :state + attributes :id, :tracking, :number, :order_id, :cost, :shipped_at, :state def order_id object.order.number end - - def stock_location_name - object.stock_location.name - end end end diff --git a/app/views/spree/admin/orders/_form.html.haml b/app/views/spree/admin/orders/_form.html.haml index 4d43acf8dd..02b1acf841 100644 --- a/app/views/spree/admin/orders/_form.html.haml +++ b/app/views/spree/admin/orders/_form.html.haml @@ -30,5 +30,5 @@ var order_number = '#{@order.number}'; var shipments = []; - @order.shipments.each do |shipment| - shipments.push(#{shipment.to_json(:root => false, :include => [:inventory_units, :stock_location]).html_safe}); + shipments.push(#{shipment.to_json(:root => false, :include => [:inventory_units]).html_safe}); = render :partial => 'spree/admin/shared/update_order_state', :handlers => [:erb], :formats => [:js] diff --git a/spec/controllers/api/v0/shipments_controller_spec.rb b/spec/controllers/api/v0/shipments_controller_spec.rb index ad70950001..1c2196b36b 100644 --- a/spec/controllers/api/v0/shipments_controller_spec.rb +++ b/spec/controllers/api/v0/shipments_controller_spec.rb @@ -6,9 +6,7 @@ RSpec.describe Api::V0::ShipmentsController, type: :controller do render_views let!(:shipment) { create(:shipment) } - let!(:attributes) do - [:id, :tracking, :number, :cost, :shipped_at, :stock_location_name, :order_id] - end + let(:attributes) { %w[id tracking number cost shipped_at order_id] } let(:current_api_user) { build(:user) } before do @@ -31,13 +29,11 @@ RSpec.describe Api::V0::ShipmentsController, type: :controller do let(:current_api_user) { build(:admin_user) } let!(:order) { shipment.order } let(:order_ship_address) { create(:address) } - let!(:stock_location) { DefaultStockLocation.find_or_create } let!(:variant) { create(:variant) } let(:params) do { quantity: 2, variant_id: variant.to_param, order_id: order.number, - stock_location_id: stock_location.to_param, format: :json } end let(:error_message) { "broken shipments creation" } @@ -109,7 +105,7 @@ RSpec.describe Api::V0::ShipmentsController, type: :controller do allow_any_instance_of(Spree::Order).to receive_messages(paid?: true, complete?: true) api_put :ready, order_id: shipment.order.to_param, id: shipment.to_param - expect(attributes.all?{ |attr| json_response.key? attr.to_s }).to be_truthy + expect(json_response.keys).to include(*attributes) expect(json_response["state"]).to eq("ready") expect(shipment.reload.state).to eq("ready") end @@ -120,7 +116,7 @@ RSpec.describe Api::V0::ShipmentsController, type: :controller do api_put :ready, order_id: shipment.order.to_param, id: shipment.to_param - expect(attributes.all?{ |attr| json_response.key? attr.to_s }).to be_truthy + expect(json_response.keys).to include(*attributes) expect(json_response["state"]).to eq("ready") expect(shipment.reload.state).to eq("ready") end @@ -324,7 +320,7 @@ RSpec.describe Api::V0::ShipmentsController, type: :controller do id: shipment.to_param, shipment: { tracking: "123123" } - expect(attributes.all?{ |attr| json_response.key? attr.to_s }).to be_truthy + expect(json_response.keys).to include(*attributes) expect(json_response["state"]).to eq("shipped") end end @@ -424,7 +420,7 @@ RSpec.describe Api::V0::ShipmentsController, type: :controller do def expect_valid_response expect(response.status).to eq 200 - attributes.all?{ |attr| json_response.key? attr.to_s } + expect(json_response.keys).to include(*attributes) end def make_order_contents_fail diff --git a/spec/factories/shipment_factory.rb b/spec/factories/shipment_factory.rb index f168880861..26a665dd9d 100644 --- a/spec/factories/shipment_factory.rb +++ b/spec/factories/shipment_factory.rb @@ -11,7 +11,6 @@ FactoryBot.define do state { 'pending' } order address - stock_location { DefaultStockLocation.find_or_create } after(:create) do |shipment, _evalulator| shipment.add_shipping_method(create(:shipping_method), true) @@ -31,7 +30,6 @@ FactoryBot.define do state { 'pending' } order address - stock_location { DefaultStockLocation.find_or_create } trait :shipping_method do transient do diff --git a/spec/factories/stock_factory.rb b/spec/factories/stock_factory.rb index 254eff8ee1..84e9d35405 100644 --- a/spec/factories/stock_factory.rb +++ b/spec/factories/stock_factory.rb @@ -3,7 +3,6 @@ FactoryBot.define do factory :stock_package, class: OrderManagement::Stock::Package do transient do - stock_location { build(:stock_location) } order { create(:order_with_line_items, line_items_count: 2) } contents { [] } end diff --git a/spec/models/spree/inventory_unit_spec.rb b/spec/models/spree/inventory_unit_spec.rb index d4ea6b3622..3c1f620191 100644 --- a/spec/models/spree/inventory_unit_spec.rb +++ b/spec/models/spree/inventory_unit_spec.rb @@ -11,7 +11,6 @@ RSpec.describe Spree::InventoryUnit do let(:shipment) do shipment = Spree::Shipment.new - shipment.stock_location = stock_location shipment.shipping_methods << create(:shipping_method) shipment.order = order # We don't care about this in this test diff --git a/spec/models/spree/shipment_spec.rb b/spec/models/spree/shipment_spec.rb index 840b293282..6f420aabff 100644 --- a/spec/models/spree/shipment_spec.rb +++ b/spec/models/spree/shipment_spec.rb @@ -324,7 +324,6 @@ RSpec.describe Spree::Shipment do allow(shipment).to receive_message_chain(:inventory_units, :group_by, map: [unit]) - shipment.stock_location = build(:stock_location) expect(variant).to receive(:move).with(1, shipment) shipment.after_cancel end @@ -348,7 +347,6 @@ RSpec.describe Spree::Shipment do allow(shipment).to receive_message_chain(:inventory_units, :group_by, map: [unit]) - shipment.stock_location = create(:stock_location) expect(variant).to receive(:move).with(-1, shipment) shipment.after_resume end From d5ff1f5c71fd8779baec8fa493471478e47b63da Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 21 Jan 2025 16:08:45 +1100 Subject: [PATCH 12/20] Remove StockItem#stock_location --- app/models/spree/stock_item.rb | 12 +++--------- app/serializers/api/admin/variant_serializer.rb | 14 +------------- .../app/services/catalog_item_builder.rb | 1 - .../api/admin/variant_serializer_spec.rb | 6 ------ 4 files changed, 4 insertions(+), 29 deletions(-) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index fa586a0741..4879d13354 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -2,12 +2,14 @@ module Spree class StockItem < ApplicationRecord + self.ignored_columns += [:stock_location_id] + acts_as_paranoid belongs_to :variant, -> { with_deleted }, class_name: 'Spree::Variant' has_many :stock_movements, dependent: :destroy - validates :variant_id, uniqueness: { scope: [:stock_location_id, :deleted_at] } + validates :variant_id, uniqueness: { scope: [:deleted_at] } validates :count_on_hand, numericality: { greater_than_or_equal_to: 0, unless: :backorderable? } delegate :weight, to: :variant @@ -39,14 +41,6 @@ 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 - - attr_writer :stock_location - private def process_backorders diff --git a/app/serializers/api/admin/variant_serializer.rb b/app/serializers/api/admin/variant_serializer.rb index 516351a3e4..7099207433 100644 --- a/app/serializers/api/admin/variant_serializer.rb +++ b/app/serializers/api/admin/variant_serializer.rb @@ -6,7 +6,7 @@ module Api attributes :id, :name, :producer_name, :image, :sku, :import_date, :tax_category_id, :options_text, :unit_value, :unit_description, :unit_to_display, :display_as, :display_name, :name_to_display, :variant_overrides_count, - :price, :on_demand, :on_hand, :in_stock, :stock_location_id, :stock_location_name, + :price, :on_demand, :on_hand, :in_stock, :variant_unit, :variant_unit_scale, :variant_unit_name, :variant_unit_with_scale has_one :primary_taxon, key: :category_id, embed: :id @@ -44,18 +44,6 @@ module Api object.in_stock? end - def stock_location_id - return if object.stock_items.empty? - - options[:stock_location]&.id || object.stock_items.first.stock_location.id - end - - def stock_location_name - return if object.stock_items.empty? - - options[:stock_location]&.name || object.stock_items.first.stock_location.name - end - def variant_overrides_count object.variant_overrides.count end diff --git a/engines/dfc_provider/app/services/catalog_item_builder.rb b/engines/dfc_provider/app/services/catalog_item_builder.rb index 11aff59e40..3af8c0e4f4 100644 --- a/engines/dfc_provider/app/services/catalog_item_builder.rb +++ b/engines/dfc_provider/app/services/catalog_item_builder.rb @@ -8,7 +8,6 @@ class CatalogItemBuilder < DfcBuilder if variant.stock_items.empty? variant.stock_items << Spree::StockItem.new( - stock_location: DefaultStockLocation.find_or_create, variant:, ) end diff --git a/spec/serializers/api/admin/variant_serializer_spec.rb b/spec/serializers/api/admin/variant_serializer_spec.rb index 7c496d721d..60f8ad45d4 100644 --- a/spec/serializers/api/admin/variant_serializer_spec.rb +++ b/spec/serializers/api/admin/variant_serializer_spec.rb @@ -22,10 +22,4 @@ RSpec.describe Api::Admin::VariantSerializer do expect(serializer.to_json).to match variant.full_name end - - it "serializes the variant stock location id" do - serializer = Api::Admin::VariantSerializer.new variant - - expect(serializer.to_json).to match variant.stock_items.first.stock_location.id.to_s - end end From 8e15d571e7466f08f0d874baa13891abea11fced Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 22 Jan 2025 11:19:57 +1100 Subject: [PATCH 13/20] Remove StockLocation from Packer --- .../app/services/order_management/stock/coordinator.rb | 3 +-- .../app/services/order_management/stock/packer.rb | 5 ++--- .../spec/services/order_management/stock/packer_spec.rb | 3 +-- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/engines/order_management/app/services/order_management/stock/coordinator.rb b/engines/order_management/app/services/order_management/stock/coordinator.rb index 64bbbc8cbc..4434bbe13b 100644 --- a/engines/order_management/app/services/order_management/stock/coordinator.rb +++ b/engines/order_management/app/services/order_management/stock/coordinator.rb @@ -41,8 +41,7 @@ module OrderManagement end def build_packer(order) - stock_location = DefaultStockLocation.find_or_create - OrderManagement::Stock::Packer.new(stock_location, order) + OrderManagement::Stock::Packer.new(order) end end end 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 6f168fa183..dc8c462783 100644 --- a/engines/order_management/app/services/order_management/stock/packer.rb +++ b/engines/order_management/app/services/order_management/stock/packer.rb @@ -3,10 +3,9 @@ module OrderManagement module Stock class Packer - attr_reader :stock_location, :order + attr_reader :order - def initialize(stock_location, order) - @stock_location = stock_location + def initialize(order) @order = order end diff --git a/engines/order_management/spec/services/order_management/stock/packer_spec.rb b/engines/order_management/spec/services/order_management/stock/packer_spec.rb index 7cb6ec6260..a2669160af 100644 --- a/engines/order_management/spec/services/order_management/stock/packer_spec.rb +++ b/engines/order_management/spec/services/order_management/stock/packer_spec.rb @@ -7,9 +7,8 @@ module OrderManagement RSpec.describe Packer do let(:distributor) { create(:distributor_enterprise) } let(:order) { create(:order_with_line_items, line_items_count: 5, distributor:) } - let(:stock_location) { create(:stock_location) } - subject { Packer.new(stock_location, order) } + subject { Packer.new(order) } before { order.line_items.first.variant.update(unit_value: 100) } From c27aa00bed57a6a1ce28698578e04a9f77a0372c Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 22 Jan 2025 11:25:58 +1100 Subject: [PATCH 14/20] Make old migration independent of app code --- ...20190313142051_set_default_stock_location_on_shipments.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/db/migrate/20190313142051_set_default_stock_location_on_shipments.rb b/db/migrate/20190313142051_set_default_stock_location_on_shipments.rb index 66d5f6cbde..1724efea01 100644 --- a/db/migrate/20190313142051_set_default_stock_location_on_shipments.rb +++ b/db/migrate/20190313142051_set_default_stock_location_on_shipments.rb @@ -1,7 +1,10 @@ class SetDefaultStockLocationOnShipments < ActiveRecord::Migration[4.2] + class SpreeStockLocation < ActiveRecord::Base + end + def up if Spree::Shipment.where('stock_location_id IS NULL').count > 0 - location = DefaultStockLocation.find_or_create + location = SpreeStockLocation.find_or_create_by(name: "default") Spree::Shipment.where('stock_location_id IS NULL').update_all(stock_location_id: location.id) end end From f0983278080aaccbc8bfb49d2f0c0582ba06ac7d Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 22 Jan 2025 11:31:27 +1100 Subject: [PATCH 15/20] Remove now useless wrapper method --- app/models/spree/inventory_unit.rb | 10 ------ app/models/spree/stock_item.rb | 2 +- spec/models/spree/inventory_unit_spec.rb | 41 ------------------------ 3 files changed, 1 insertion(+), 52 deletions(-) diff --git a/app/models/spree/inventory_unit.rb b/app/models/spree/inventory_unit.rb index 0f66dbb8dd..6240883659 100644 --- a/app/models/spree/inventory_unit.rb +++ b/app/models/spree/inventory_unit.rb @@ -35,16 +35,6 @@ module Spree end end - # This was refactored from a simpler query because the previous implementation - # lead to issues once users tried to modify the objects returned. That's due - # to ActiveRecord `joins(shipment: :stock_location)` only return readonly - # objects - # - # Returns an array of backordered inventory units as per a given stock item - def self.backordered_for_stock_item(stock_item) - backordered_per_variant(stock_item) - end - def self.finalize_units!(inventory_units) inventory_units.map do |iu| iu.update_columns( diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index 4879d13354..db236f7ecf 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -16,7 +16,7 @@ module Spree delegate :name, to: :variant, prefix: true def backordered_inventory_units - Spree::InventoryUnit.backordered_for_stock_item(self) + Spree::InventoryUnit.backordered_per_variant(self) end def adjust_count_on_hand(value) diff --git a/spec/models/spree/inventory_unit_spec.rb b/spec/models/spree/inventory_unit_spec.rb index 3c1f620191..9d4053ad18 100644 --- a/spec/models/spree/inventory_unit_spec.rb +++ b/spec/models/spree/inventory_unit_spec.rb @@ -6,47 +6,6 @@ RSpec.describe Spree::InventoryUnit do let!(:stock_location) { create(:stock_location_with_items) } let(:stock_item) { Spree::StockItem.order(:id).first } - context "#backordered_for_stock_item" do - let(:order) { create(:order) } - - let(:shipment) do - shipment = Spree::Shipment.new - shipment.shipping_methods << create(:shipping_method) - shipment.order = order - # We don't care about this in this test - allow(shipment).to receive(:ensure_correct_adjustment) - shipment.tap(&:save!) - end - - let!(:unit) do - unit = shipment.inventory_units.build - unit.state = 'backordered' - unit.variant_id = stock_item.variant.id - unit.tap(&:save!) - end - - # Regression for Spree #3066 - it "returns modifiable objects" do - units = Spree::InventoryUnit.backordered_for_stock_item(stock_item) - expect { units.first.save! }.not_to raise_error - end - - it "finds inventory units from its stock location " \ - "when the unit's variant matches the stock item's variant" do - expect(Spree::InventoryUnit.backordered_for_stock_item(stock_item)).to eq [unit] - end - - it "does not find inventory units that don't match the stock item's variant" do - other_variant_unit = shipment.inventory_units.build - other_variant_unit.state = 'backordered' - other_variant_unit.variant = create(:variant) - other_variant_unit.save! - - expect(Spree::InventoryUnit.backordered_for_stock_item(stock_item)) - .not_to include(other_variant_unit) - end - end - context "variants deleted" do let!(:unit) do Spree::InventoryUnit.create(variant: stock_item.variant) From 33ed0998a9540d435983c1dc303a463cc3425b20 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 22 Jan 2025 11:34:54 +1100 Subject: [PATCH 16/20] Removing old references to stock location --- app/models/spree/order_inventory.rb | 3 --- app/serializers/api/admin/product_serializer.rb | 1 - 2 files changed, 4 deletions(-) diff --git a/app/models/spree/order_inventory.rb b/app/models/spree/order_inventory.rb index 02a1dc8286..fdf0d9ed9b 100644 --- a/app/models/spree/order_inventory.rb +++ b/app/models/spree/order_inventory.rb @@ -60,7 +60,6 @@ module Spree # Returns either one of the shipment: # # first unshipped that already includes this variant - # first unshipped that's leaving from a stock_location that stocks this variant def determine_target_shipment(variant) target_shipment = order.shipments.detect do |shipment| (shipment.ready? || shipment.pending?) && shipment.contains?(variant) @@ -77,7 +76,6 @@ module Spree on_hand.times { shipment.set_up_inventory('on_hand', variant, order) } back_order.times { shipment.set_up_inventory('backordered', variant, order) } - # adding to this shipment, and removing from stock_location if order.completed? variant.move(-quantity, shipment) end @@ -102,7 +100,6 @@ module Spree end shipment.destroy if shipment.inventory_units.reload.count == 0 - # removing this from shipment, and adding to stock_location if order.completed? && restock_item variant.move(removed_quantity, shipment) end diff --git a/app/serializers/api/admin/product_serializer.rb b/app/serializers/api/admin/product_serializer.rb index 7056c5745c..991cb8e8b9 100644 --- a/app/serializers/api/admin/product_serializer.rb +++ b/app/serializers/api/admin/product_serializer.rb @@ -11,7 +11,6 @@ module Api object.variants, each_serializer: Api::Admin::VariantSerializer, image: thumb_url, - stock_location: Spree::StockLocation.first ) end From 70ebe7b964f273c6070991e2c4ffdf1a319ce0e3 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 22 Jan 2025 11:51:42 +1100 Subject: [PATCH 17/20] Remove stock location from specs --- .rubocop_todo.yml | 1 - .../spree/admin/products_controller_spec.rb | 1 - spec/factories/stock_location_factory.rb | 33 ------------------- spec/models/spree/inventory_unit_spec.rb | 6 ++-- spec/models/spree/product_spec.rb | 4 --- spec/models/spree/stock_item_spec.rb | 4 +-- spec/models/spree/stock_movement_spec.rb | 3 +- spec/services/sets/product_set_spec.rb | 1 - spec/system/admin/products_spec.rb | 1 - spec/system/admin/products_v3/create_spec.rb | 1 - spec/system/admin/unit_price_spec.rb | 2 -- 11 files changed, 4 insertions(+), 53 deletions(-) delete mode 100644 spec/factories/stock_location_factory.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 92432aae32..cacc26fc1c 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -339,7 +339,6 @@ Naming/VariableNumber: - 'app/models/preference_sections/main_links_section.rb' - 'lib/spree/core/controller_helpers/common.rb' - 'spec/controllers/spree/admin/search_controller_spec.rb' - - 'spec/factories/stock_location_factory.rb' - 'spec/models/spree/stock_item_spec.rb' - 'spec/models/spree/tax_rate_spec.rb' - 'spec/requests/api/orders_spec.rb' diff --git a/spec/controllers/spree/admin/products_controller_spec.rb b/spec/controllers/spree/admin/products_controller_spec.rb index deae56bd80..f6f14206ab 100644 --- a/spec/controllers/spree/admin/products_controller_spec.rb +++ b/spec/controllers/spree/admin/products_controller_spec.rb @@ -142,7 +142,6 @@ RSpec.describe Spree::Admin::ProductsController, type: :controller do before do controller_login_as_admin - create(:stock_location) end it "redirects to products when the user hits 'create'" do diff --git a/spec/factories/stock_location_factory.rb b/spec/factories/stock_location_factory.rb deleted file mode 100644 index 37d43d7237..0000000000 --- a/spec/factories/stock_location_factory.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -FactoryBot.define do - factory :stock_location, class: Spree::StockLocation do - # keeps the test stock_location unique - initialize_with { Spree::StockLocation.first || DefaultStockLocation.find_or_create } - - address1 { '1600 Pennsylvania Ave NW' } - city { 'Washington' } - zipcode { '20500' } - phone { '(202) 456-1111' } - - country { |stock_location| Spree::Country.first || stock_location.association(:country) } - state do |stock_location| - stock_location.country.states.first || - stock_location.association(:state, country: stock_location.country) - end - - factory :stock_location_with_items do - 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) - - Spree::StockItem.where(variant_id: product_1.variants.first.id) - .first.adjust_count_on_hand(10) - Spree::StockItem.where(variant_id: product_2.variants.first.id) - .first.adjust_count_on_hand(20) - end - end - end -end diff --git a/spec/models/spree/inventory_unit_spec.rb b/spec/models/spree/inventory_unit_spec.rb index 9d4053ad18..8a755fa6d6 100644 --- a/spec/models/spree/inventory_unit_spec.rb +++ b/spec/models/spree/inventory_unit_spec.rb @@ -3,8 +3,8 @@ require 'spec_helper' RSpec.describe Spree::InventoryUnit do - let!(:stock_location) { create(:stock_location_with_items) } - let(:stock_item) { Spree::StockItem.order(:id).first } + let(:variant) { create(:variant) } + let(:stock_item) { variant.stock_item } context "variants deleted" do let!(:unit) do @@ -18,8 +18,6 @@ RSpec.describe Spree::InventoryUnit do end context "#finalize_units!" do - let!(:stock_location) { create(:stock_location) } - let(:variant) { create(:variant) } let(:inventory_units) { [ create(:inventory_unit, variant:), diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 26fef88569..9f9937f897 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -130,10 +130,6 @@ module Spree let!(:taxon){ create(:taxon) } let(:supplier){ create(:enterprise) } - before do - create(:stock_location) - end - it "copies properties to the first standard variant" do product.primary_taxon_id = taxon.id product.name = "Product1" diff --git a/spec/models/spree/stock_item_spec.rb b/spec/models/spree/stock_item_spec.rb index fd3f74965f..9268c04b18 100644 --- a/spec/models/spree/stock_item_spec.rb +++ b/spec/models/spree/stock_item_spec.rb @@ -3,9 +3,7 @@ require 'spec_helper' RSpec.describe Spree::StockItem do - let!(:stock_location) { create(:stock_location_with_items) } - - subject(:stock_item) { Spree::StockItem.order(:id).first } + subject(:stock_item) { create(:variant, on_hand: 15).stock_item } describe "validation" do it "requires count_on_hand to be positive if not backorderable" do diff --git a/spec/models/spree/stock_movement_spec.rb b/spec/models/spree/stock_movement_spec.rb index 235f542db2..02f4f4c543 100644 --- a/spec/models/spree/stock_movement_spec.rb +++ b/spec/models/spree/stock_movement_spec.rb @@ -3,8 +3,7 @@ require 'spec_helper' RSpec.describe Spree::StockMovement do - let!(:stock_location) { create(:stock_location_with_items) } - let(:stock_item) { Spree::StockItem.order(:id).first } + let(:stock_item) { create(:variant, on_hand: 15).stock_item } subject { build(:stock_movement, stock_item:) } it 'should belong to a stock item' do diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb index d415f7096d..f6b28a5fc1 100644 --- a/spec/services/sets/product_set_spec.rb +++ b/spec/services/sets/product_set_spec.rb @@ -10,7 +10,6 @@ RSpec.describe Sets::ProductSet do subject{ product_set.save } context 'when the product does not exist yet' do - let!(:stock_location) { create(:stock_location) } let(:collection_hash) do { 0 => { diff --git a/spec/system/admin/products_spec.rb b/spec/system/admin/products_spec.rb index f2a1c322f3..8f10416261 100644 --- a/spec/system/admin/products_spec.rb +++ b/spec/system/admin/products_spec.rb @@ -11,7 +11,6 @@ RSpec.describe ' include FileHelper let!(:taxon) { create(:taxon) } - let!(:stock_location) { create(:stock_location) } let!(:shipping_category) { DefaultShippingCategory.find_or_create } let!(:supplier) { create(:supplier_enterprise, name: 'New supplier') } diff --git a/spec/system/admin/products_v3/create_spec.rb b/spec/system/admin/products_v3/create_spec.rb index e767ab3891..d2af1c8899 100644 --- a/spec/system/admin/products_v3/create_spec.rb +++ b/spec/system/admin/products_v3/create_spec.rb @@ -13,7 +13,6 @@ RSpec.describe 'As an enterprise user, I can manage my products' do let!(:taxon) { create(:taxon) } describe "creating a new product" do - let!(:stock_location) { create(:stock_location) } let!(:distributor) { create(:distributor_enterprise) } let!(:shipping_category) { create(:shipping_category) } diff --git a/spec/system/admin/unit_price_spec.rb b/spec/system/admin/unit_price_spec.rb index eb64b11172..5ad5b4fc7c 100644 --- a/spec/system/admin/unit_price_spec.rb +++ b/spec/system/admin/unit_price_spec.rb @@ -9,8 +9,6 @@ RSpec.describe ' include AuthenticationHelper include WebHelper - let!(:stock_location) { create(:stock_location) } - describe "product" do it "creating a new product" do login_as_admin From 187a78b78d3d8537c6051d3433c0351e49256901 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 22 Jan 2025 11:58:15 +1100 Subject: [PATCH 18/20] Remove stock location from add-variant JS --- .../admin/spree/orders/variant_autocomplete.js.erb | 7 +++---- app/views/spree/admin/variants/_autocomplete.js.erb | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/admin/spree/orders/variant_autocomplete.js.erb b/app/assets/javascripts/admin/spree/orders/variant_autocomplete.js.erb index 001055417e..547a5a1b17 100644 --- a/app/assets/javascripts/admin/spree/orders/variant_autocomplete.js.erb +++ b/app/assets/javascripts/admin/spree/orders/variant_autocomplete.js.erb @@ -187,18 +187,17 @@ addVariantFromStockLocation = function() { $('#stock_details').hide(); var variant_id = $('input.variant_autocomplete').val(); - var stock_location_id = $(this).data('stock-location-id'); - var quantity = $("input.quantity[data-stock-location-id='" + stock_location_id + "']").val(); + var quantity = $("input.quantity").val(); var shipment = _.find(shipments, function(shipment){ - return shipment.stock_location_id == stock_location_id && (shipment.state == 'ready' || shipment.state == 'pending'); + return shipment.state == 'ready' || shipment.state == 'pending'; }); if(shipment==undefined){ $.ajax({ type: "POST", url: Spree.url(Spree.routes.orders_api + "/" + order_number + "/shipments.json"), - data: { variant_id: variant_id, quantity: quantity, stock_location_id: stock_location_id } + data: { variant_id: variant_id, quantity: quantity } }).done(function( msg ) { window.location.reload(); }).error(function( msg ) { diff --git a/app/views/spree/admin/variants/_autocomplete.js.erb b/app/views/spree/admin/variants/_autocomplete.js.erb index c4f3bae954..b42de3f6b7 100644 --- a/app/views/spree/admin/variants/_autocomplete.js.erb +++ b/app/views/spree/admin/variants/_autocomplete.js.erb @@ -51,10 +51,10 @@ {{/if}} - + - + {{else}} <%= t('.out_of_stock') %> From 64608beaa8b093e005418bde3f85f695354d046f Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 22 Jan 2025 12:01:57 +1100 Subject: [PATCH 19/20] Remove DefaultStockLocation created in setup --- app/models/concerns/variant_stock.rb | 9 ----- app/services/default_stock_location.rb | 11 ------ db/seeds.rb | 1 - .../services/supplied_product_builder_spec.rb | 3 -- spec/factories/product_factory.rb | 3 -- spec/factories/variant_factory.rb | 3 -- spec/lib/tasks/sample_data_rake_spec.rb | 1 - spec/services/default_stock_location_spec.rb | 39 ------------------- 8 files changed, 70 deletions(-) delete mode 100644 app/services/default_stock_location.rb delete mode 100644 spec/services/default_stock_location_spec.rb diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb index d3837a1ce5..7489aa5893 100644 --- a/app/models/concerns/variant_stock.rb +++ b/app/models/concerns/variant_stock.rb @@ -78,11 +78,6 @@ module VariantStock on_demand || total_on_hand >= quantity end - # Moving Spree::StockLocation.fill_status to the variant enables us - # to override this behaviour for variant overrides - # We can have this responsibility here in the variant because there is - # only one stock item per variant - # # 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) @@ -107,10 +102,6 @@ module VariantStock raise_error_if_no_stock_item_available # Creates a stock movement: it updates stock_item.count_on_hand and fills backorders - # - # This is the original Spree::StockLocation#move, - # except that we raise an error if the stock item is missing, - # because, unlike Spree, we should always have exactly one stock item per variant. stock_item.stock_movements.create!(quantity:, originator:) end diff --git a/app/services/default_stock_location.rb b/app/services/default_stock_location.rb deleted file mode 100644 index fb58f537d1..0000000000 --- a/app/services/default_stock_location.rb +++ /dev/null @@ -1,11 +0,0 @@ -# frozen_string_literal: true - -# Encapsulates the concept of default stock location that OFN has, as explained -# in https://github.com/openfoodfoundation/openfoodnetwork/wiki/Spree-Upgrade%3A-Stock-locations -class DefaultStockLocation - NAME = 'default' - - def self.find_or_create - Spree::StockLocation.find_or_create_by(name: NAME) - end -end diff --git a/db/seeds.rb b/db/seeds.rb index 9192a35e61..70512b1463 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -30,5 +30,4 @@ require File.join(File.dirname(__FILE__), 'default', 'zones') Rails.logger.info "[db:seed] Seeding Users" require File.join(File.dirname(__FILE__), 'default', 'users') -DefaultStockLocation.find_or_create DefaultShippingCategory.find_or_create diff --git a/engines/dfc_provider/spec/services/supplied_product_builder_spec.rb b/engines/dfc_provider/spec/services/supplied_product_builder_spec.rb index 8c307fe5a5..bd222af048 100644 --- a/engines/dfc_provider/spec/services/supplied_product_builder_spec.rb +++ b/engines/dfc_provider/spec/services/supplied_product_builder_spec.rb @@ -222,9 +222,6 @@ RSpec.describe SuppliedProductBuilder do } it "creates a new Spree::Product and variant" do - # We need this to save stock: - DefaultStockLocation.find_or_create - create(:taxon) expect(imported_variant).to be_a(Spree::Variant) diff --git a/spec/factories/product_factory.rb b/spec/factories/product_factory.rb index 453a442be4..3d3dafeb53 100644 --- a/spec/factories/product_factory.rb +++ b/spec/factories/product_factory.rb @@ -23,9 +23,6 @@ FactoryBot.define do variant_unit { 'weight' } variant_unit_scale { 1 } - # ensure stock item will be created for this products master - before(:create) { DefaultStockLocation.find_or_create } - factory :product do transient do on_hand { 5 } diff --git a/spec/factories/variant_factory.rb b/spec/factories/variant_factory.rb index 1aed03f183..38560acd81 100644 --- a/spec/factories/variant_factory.rb +++ b/spec/factories/variant_factory.rb @@ -28,9 +28,6 @@ FactoryBot.define do # create a "standard variant" product { association :base_product } - # ensure stock item will be created for this variant - before(:create) { DefaultStockLocation.find_or_create } - factory :variant do transient do on_demand { false } diff --git a/spec/lib/tasks/sample_data_rake_spec.rb b/spec/lib/tasks/sample_data_rake_spec.rb index 4fa798e53a..f16a1e3be0 100644 --- a/spec/lib/tasks/sample_data_rake_spec.rb +++ b/spec/lib/tasks/sample_data_rake_spec.rb @@ -12,7 +12,6 @@ RSpec.describe 'sample_data.rake' do before do # Create seed data required by the sample data. create(:user) - DefaultStockLocation.find_or_create DefaultShippingCategory.find_or_create end diff --git a/spec/services/default_stock_location_spec.rb b/spec/services/default_stock_location_spec.rb deleted file mode 100644 index 93b0a053f7..0000000000 --- a/spec/services/default_stock_location_spec.rb +++ /dev/null @@ -1,39 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe DefaultStockLocation do - describe '.find_or_create' do - context 'when a location named default already exists' do - let!(:location) do - country = create(:country) - state = Spree::State.create(name: 'Alabama', country:) - Spree::StockLocation.create!( - name: 'default', - country_id: country.id, - state_id: state.id - ) - end - - it 'returns the location' do - expect(described_class.find_or_create).to eq(location) - end - - it 'does not create any other location' do - expect { described_class.find_or_create }.not_to change { Spree::StockLocation.count } - end - end - - context 'when a location named default does not exist' do - it 'returns the location' do - location = described_class.find_or_create - expect(location.name).to eq('default') - end - - it 'does not create any other location' do - expect { described_class.find_or_create } - .to change { Spree::StockLocation.count }.from(0).to(1) - end - end - end -end From a2459a374287e45c6be287e133d29324f3258f42 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 22 Jan 2025 12:09:17 +1100 Subject: [PATCH 20/20] Remove class Spree::StockLocation The database table is still there and can stay for a while until we are confident that nothing is broken in production. --- app/models/spree/stock_location.rb | 13 ------------- 1 file changed, 13 deletions(-) delete mode 100644 app/models/spree/stock_location.rb diff --git a/app/models/spree/stock_location.rb b/app/models/spree/stock_location.rb deleted file mode 100644 index 9640ef474f..0000000000 --- a/app/models/spree/stock_location.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -module Spree - class StockLocation < ApplicationRecord - self.belongs_to_required_by_default = false - self.ignored_columns += [:backorderable_default, :active] - - belongs_to :state, class_name: 'Spree::State' - belongs_to :country, class_name: 'Spree::Country' - - validates :name, presence: true - end -end