From 0b162e6eec33e6837849f581702f74a68dbc312a Mon Sep 17 00:00:00 2001 From: Jared Norman Date: Mon, 30 May 2022 15:43:03 -0700 Subject: [PATCH 1/4] Make the inventory unit builder configurable This adds the inventory unit builder (Spree::Stock::InventoryUnitBuilder by default) to the Spree::Config.stock configuration, so that it can be overridden. This is desirable for any store that doesn't have a one-to-one mapping between purchased variants and the fulfilled inventory units. The extension solidus_product_assembly heavily overrides this class to accomplish this, so with this change it will be able to stop decorating Spree::Stock::InventoryUnitBuilder and instead provide its own. I also reworked the stock configuration specs a bit. I fixed the repeated typo, simplified the structure slightly, and ensured they don't leak constants anymore. --- core/app/models/spree/order.rb | 3 +- .../models/spree/stock/simple_coordinator.rb | 3 +- core/lib/spree/core/stock_configuration.rb | 6 ++ .../spree/core/stock_configuration_spec.rb | 92 +++++++++++-------- core/spec/models/spree/order_spec.rb | 40 +++++++- .../spree/stock/simple_coordinator_spec.rb | 8 ++ 6 files changed, 109 insertions(+), 43 deletions(-) diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index 29bb53f0b73..2e1200cd8be 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -502,7 +502,8 @@ def create_proposed_shipments end def create_shipments_for_line_item(line_item) - units = Spree::Stock::InventoryUnitBuilder.new(self).missing_units_for_line_item(line_item) + units = Spree::Config.stock.inventory_unit_builder_class.new(self).missing_units_for_line_item(line_item) + Spree::Config.stock.coordinator_class.new(self, units).shipments.each do |shipment| shipments << shipment end diff --git a/core/app/models/spree/stock/simple_coordinator.rb b/core/app/models/spree/stock/simple_coordinator.rb index de523ef804d..60913cf372d 100644 --- a/core/app/models/spree/stock/simple_coordinator.rb +++ b/core/app/models/spree/stock/simple_coordinator.rb @@ -24,7 +24,8 @@ class SimpleCoordinator def initialize(order, inventory_units = nil) @order = order - @inventory_units = inventory_units || InventoryUnitBuilder.new(order).units + @inventory_units = + inventory_units || Spree::Config.stock.inventory_unit_builder_class.new(order).units @splitters = Spree::Config.environment.stock_splitters filtered_stock_locations = Spree::Config.stock.location_filter_class.new(Spree::StockLocation.all, @order).filter diff --git a/core/lib/spree/core/stock_configuration.rb b/core/lib/spree/core/stock_configuration.rb index b4da441742a..97bea22ade4 100644 --- a/core/lib/spree/core/stock_configuration.rb +++ b/core/lib/spree/core/stock_configuration.rb @@ -8,6 +8,7 @@ class StockConfiguration attr_writer :location_filter_class attr_writer :location_sorter_class attr_writer :allocator_class + attr_writer :inventory_unit_builder_class def coordinator_class @coordinator_class ||= '::Spree::Stock::SimpleCoordinator' @@ -33,6 +34,11 @@ def allocator_class @allocator_class ||= '::Spree::Stock::Allocator::OnHandFirst' @allocator_class.constantize end + + def inventory_unit_builder_class + @inventory_unit_builder_class ||= '::Spree::Stock::InventoryUnitBuilder' + @inventory_unit_builder_class.constantize + end end end end diff --git a/core/spec/lib/spree/core/stock_configuration_spec.rb b/core/spec/lib/spree/core/stock_configuration_spec.rb index 0c03644880e..36577d66ef1 100644 --- a/core/spec/lib/spree/core/stock_configuration_spec.rb +++ b/core/spec/lib/spree/core/stock_configuration_spec.rb @@ -3,93 +3,107 @@ require 'rails_helper' RSpec.describe Spree::Core::StockConfiguration do + let(:stock_configuration) { described_class.new } + describe '#coordinator_class' do - let(:stock_configuration) { described_class.new } subject { stock_configuration.coordinator_class } - it "returns Spree::Stock::Coordinator" do - is_expected.to be ::Spree::Stock::SimpleCoordinator + it "returns Spree::Stock::Coordinator by default" do + expect(subject).to be ::Spree::Stock::SimpleCoordinator end - context "with another constant name assiged" do + it "can be reassigned" do MyCoordinator = Class.new - before { stock_configuration.coordinator_class = MyCoordinator.to_s } + stock_configuration.coordinator_class = MyCoordinator.to_s + + expect(subject).to be MyCoordinator - it "returns the constant" do - is_expected.to be MyCoordinator - end + Object.send(:remove_const, :MyCoordinator) end end describe '#estimator_class' do - let(:stock_configuration) { described_class.new } subject { stock_configuration.estimator_class } it "returns Spree::Stock::Estimator" do - is_expected.to be ::Spree::Stock::Estimator + expect(subject).to be ::Spree::Stock::Estimator end - context "with another constant name assiged" do + it "can be reassigned" do MyEstimator = Class.new - before { stock_configuration.estimator_class = MyEstimator.to_s } + stock_configuration.estimator_class = MyEstimator.to_s - it "returns the constant" do - is_expected.to be MyEstimator - end + expect(subject).to be MyEstimator + + Object.send(:remove_const, :MyEstimator) end end describe '#location_filter_class' do - let(:stock_configuration) { described_class.new } subject { stock_configuration.location_filter_class } - it "returns Spree::Stock::LocationFilter::Active" do - is_expected.to be ::Spree::Stock::LocationFilter::Active + it "returns Spree::Stock::LocationFilter::Active by default" do + expect(subject).to be ::Spree::Stock::LocationFilter::Active end - context "with another constant name assiged" do + it "can be reassigned" do MyFilter = Class.new - before { stock_configuration.location_filter_class = MyFilter.to_s } + stock_configuration.location_filter_class = MyFilter.to_s + + expect(subject).to be MyFilter - it "returns the constant" do - is_expected.to be MyFilter - end + Object.send(:remove_const, :MyFilter) end end describe '#location_sorter_class' do - let(:stock_configuration) { described_class.new } subject { stock_configuration.location_sorter_class } - it "returns Spree::Stock::LocationSorter::Unsorted" do - is_expected.to be ::Spree::Stock::LocationSorter::Unsorted + it "returns Spree::Stock::LocationSorter::Unsorted by default" do + expect(subject).to be ::Spree::Stock::LocationSorter::Unsorted end - context "with another constant name assiged" do + it "can be reassigned" do MySorter = Class.new - before { stock_configuration.location_sorter_class = MySorter.to_s } + stock_configuration.location_sorter_class = MySorter.to_s - it "returns the constant" do - is_expected.to be MySorter - end + expect(subject).to be MySorter + + Object.send(:remove_const, :MySorter) end end describe '#allocator_class' do - let(:stock_configuration) { described_class.new } subject { stock_configuration.allocator_class } - it "returns Spree::Stock::Allocator::OnHandFirst" do - is_expected.to be ::Spree::Stock::Allocator::OnHandFirst + it "returns Spree::Stock::Allocator::OnHandFirst by default" do + expect(subject).to be ::Spree::Stock::Allocator::OnHandFirst end - context "with another constant name assiged" do + it "can be reassigned" do MyAllocator = Class.new - before { stock_configuration.allocator_class = MyAllocator.to_s } + stock_configuration.allocator_class = MyAllocator.to_s + + expect(subject).to be MyAllocator + + Object.send(:remove_const, :MyAllocator) + end + end + + describe '#inventory_unit_builder_class' do + subject { stock_configuration.inventory_unit_builder_class } + + it "returns Spree::Stock::InventoryUnitBuilder by default" do + expect(subject).to be ::Spree::Stock::InventoryUnitBuilder + end + + it "can be reassigned" do + MyInventoryUnitBuilder = Class.new + stock_configuration.inventory_unit_builder_class = MyInventoryUnitBuilder.to_s + + expect(subject).to be MyInventoryUnitBuilder - it "returns the constant" do - is_expected.to be MyAllocator - end + Object.send(:remove_const, :MyInventoryUnitBuilder) end end end diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb index 6a068a2eceb..118a3f1bbd2 100644 --- a/core/spec/models/spree/order_spec.rb +++ b/core/spec/models/spree/order_spec.rb @@ -1643,11 +1643,47 @@ def generate describe '#create_shipments_for_line_item' do subject { order.create_shipments_for_line_item(line_item) } - let(:order) { create :order_with_line_items } + let(:order) { create :order, shipments: [] } let(:line_item) { build(:line_item, order: order) } it 'creates at least one new shipment for the order' do - expect { subject }.to change { order.shipments.count }.by 1 + expect { subject }.to change { order.shipments.count }.from(0).to(1) + end + + context "with a custom inventory unit builder" do + before do + # Because the defined method runs in the context of the instance of our + # test inventory unit builder, not the RSpec example context, we need + # to make this value available as a local variable. We're using + # Class.new and define_method to avoid creating scope gates that would + # take this local variable out of scope. + inventory_unit = arbitrary_inventory_unit + TestInventoryUnitBuilder = Class.new do + def initialize(order) + end + + define_method(:missing_units_for_line_item) { |line_item| + [inventory_unit] + } + end + + test_stock_config = Spree::Core::StockConfiguration.new + test_stock_config.inventory_unit_builder_class = TestInventoryUnitBuilder.to_s + stub_spree_preferences(stock: test_stock_config) + end + + after do + Object.send(:remove_const, :TestInventoryUnitBuilder) + end + + let(:arbitrary_inventory_unit) { build :inventory_unit, order: order, line_item: line_item, variant: line_item.variant } + + it "relies on the custom builder" do + expect { subject }.to change { order.shipments.count }.from(0).to(1) + + expect(order.shipments.order(:created_at).first.inventory_units) + .to contain_exactly arbitrary_inventory_unit + end end end diff --git a/core/spec/models/spree/stock/simple_coordinator_spec.rb b/core/spec/models/spree/stock/simple_coordinator_spec.rb index 34554740302..02a245b1288 100644 --- a/core/spec/models/spree/stock/simple_coordinator_spec.rb +++ b/core/spec/models/spree/stock/simple_coordinator_spec.rb @@ -38,6 +38,14 @@ module Stock subject.shipments.count == StockLocation.count end + it 'uses the pluggable inventory unit builder class' do + expect(Spree::Config.stock) + .to receive(:inventory_unit_builder_class) + .and_call_original + + subject.shipments + end + context "missing stock items in active stock location" do let!(:another_location) { create(:stock_location, propagate_all_variants: false) } From ea5c132deac98e1570f82c6a65bc5c57b3f5a50b Mon Sep 17 00:00:00 2001 From: Jared Norman Date: Tue, 31 May 2022 16:59:03 -0700 Subject: [PATCH 2/4] Make the availability validator configurable This makes the Spree::Stock::AvailabilityValidator swappable. I need to justify using `send` in that test: this method is called by the order state machine, itself a swappable component. I could imagine making it a public method, but I don't want to increase the surface area of Spree::Order's API. Instead, I think it's useful to test this method in isolation (i.e. separately from the state machine) with the assumption that it is part of the API that the configured state machine will call. I've not added tests for the change to the checkout controller in the frontend... because it's too hard. The configured class already gets called a bunch just in setting up these tests, so trying to write a test that cares that it gets called with the right arguments at that particular moment did not seem worth the effort given we're deprecated frontend. Hopefully we can do something better in solidus_starter_frontend, where we also need to care about whether we're on a Solidus version that supports this configuration option. --- core/app/models/spree/order.rb | 11 +++++- core/lib/spree/core/stock_configuration.rb | 6 +++ .../spree/core/stock_configuration_spec.rb | 19 +++++++++ core/spec/models/spree/order_spec.rb | 39 +++++++++++++++++++ .../controllers/spree/checkout_controller.rb | 2 +- 5 files changed, 74 insertions(+), 3 deletions(-) diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index 2e1200cd8be..7f0cfcc6da3 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -810,8 +810,15 @@ def ensure_promotions_eligible end def validate_line_item_availability - availability_validator = Spree::Stock::AvailabilityValidator.new - raise InsufficientStock unless line_items.all? { |line_item| availability_validator.validate(line_item) } + availability_validator = Spree::Config.stock.availability_validator_class.new + + # NOTE: This code assumes that the availability validator will return + # true for success and false for failure. This is not normally the + # behaviour of validators, as the framework only cares about the + # population of the errors, not the return value of the validate method. + raise InsufficientStock unless line_items.all? { |line_item| + availability_validator.validate(line_item) + } end def ensure_line_items_present diff --git a/core/lib/spree/core/stock_configuration.rb b/core/lib/spree/core/stock_configuration.rb index 97bea22ade4..2f3a4374f60 100644 --- a/core/lib/spree/core/stock_configuration.rb +++ b/core/lib/spree/core/stock_configuration.rb @@ -9,6 +9,7 @@ class StockConfiguration attr_writer :location_sorter_class attr_writer :allocator_class attr_writer :inventory_unit_builder_class + attr_writer :availability_validator_class def coordinator_class @coordinator_class ||= '::Spree::Stock::SimpleCoordinator' @@ -39,6 +40,11 @@ def inventory_unit_builder_class @inventory_unit_builder_class ||= '::Spree::Stock::InventoryUnitBuilder' @inventory_unit_builder_class.constantize end + + def availability_validator_class + @availability_validator_class ||= '::Spree::Stock::AvailabilityValidator' + @availability_validator_class.constantize + end end end end diff --git a/core/spec/lib/spree/core/stock_configuration_spec.rb b/core/spec/lib/spree/core/stock_configuration_spec.rb index 36577d66ef1..df98de87d50 100644 --- a/core/spec/lib/spree/core/stock_configuration_spec.rb +++ b/core/spec/lib/spree/core/stock_configuration_spec.rb @@ -106,4 +106,23 @@ Object.send(:remove_const, :MyInventoryUnitBuilder) end end + + describe '#availability_validator_class' do + subject { stock_configuration.availability_validator_class } + + let(:stock_configuration) { described_class.new } + + it "returns Spree::Stock::AvailabilityValidator" do + is_expected.to be ::Spree::Stock::AvailabilityValidator + end + + it "can be reassigned" do + MyAvailabilityValidator = Class.new + stock_configuration.availability_validator_class = MyAvailabilityValidator.to_s + + expect(subject).to be MyAvailabilityValidator + + Object.send(:remove_const, :MyAvailabilityValidator) + end + end end diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb index 118a3f1bbd2..facffe288dd 100644 --- a/core/spec/models/spree/order_spec.rb +++ b/core/spec/models/spree/order_spec.rb @@ -1701,4 +1701,43 @@ def initialize(order) expect(subject).to eq 40 end end + + describe "#validate_line_item_availability" do + subject { order.send(:validate_line_item_availability) } + + before do + class TestValidator + def validate(line_item) + if line_item.variant.sku == "UNAVAILABLE" + line_item.errors.add(:quantity, ":(") + false + else + true + end + end + end + + test_stock_config = Spree::Core::StockConfiguration.new + test_stock_config.availability_validator_class = TestValidator.to_s + stub_spree_preferences(stock: test_stock_config) + end + + let(:order) { create :order_with_line_items, line_items_count: 2 } + + context "when the line items are valid" do + it "doesn't raise an exception" do + expect { subject }.not_to raise_error + end + end + + context "when the line items are not valid" do + before do + order.line_items.last.variant.sku = "UNAVAILABLE" + end + + it "raises an exception" do + expect { subject }.to raise_error(Spree::Order::InsufficientStock) + end + end + end end diff --git a/frontend/app/controllers/spree/checkout_controller.rb b/frontend/app/controllers/spree/checkout_controller.rb index afd71eecdf7..7dc1d71629f 100644 --- a/frontend/app/controllers/spree/checkout_controller.rb +++ b/frontend/app/controllers/spree/checkout_controller.rb @@ -230,7 +230,7 @@ def insufficient_stock_error flash[:error] = I18n.t('spree.insufficient_stock_for_order') redirect_to cart_path else - availability_validator = Spree::Stock::AvailabilityValidator.new + availability_validator = Spree::Config.stock.availability_validator_class.new unavailable_items = @order.line_items.reject { |line_item| availability_validator.validate(line_item) } if unavailable_items.any? item_names = unavailable_items.map(&:name).to_sentence From ade11abc9b41d91ff5fa40a09b22c9d8a4137eae Mon Sep 17 00:00:00 2001 From: Jared Norman Date: Thu, 2 Jun 2022 14:32:07 -0700 Subject: [PATCH 3/4] Make the inventory validator configurable Similar to the two parent commits, I'm making more of the stock-related classes in Solidus configurable to better support functionality like solidus_product_assembly provides. Also like the previous commit, I've chosen to unit test a private method on Spree::Order because I think it warrants being tested independently of the order state machine (the only place that calls this method). --- core/app/models/spree/order.rb | 7 +++- core/lib/spree/core/stock_configuration.rb | 6 +++ .../spree/core/stock_configuration_spec.rb | 19 +++++++++ core/spec/models/spree/order_spec.rb | 42 +++++++++++++++++++ 4 files changed, 72 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index 7f0cfcc6da3..9a62af0973f 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -790,9 +790,12 @@ def require_email def ensure_inventory_units if has_checkout_step?("delivery") - inventory_validator = Spree::Stock::InventoryValidator.new + inventory_validator = Spree::Config.stock.inventory_validator_class.new + + errors = line_items.map { |line_item| + inventory_validator.validate(line_item) + }.compact - errors = line_items.map { |line_item| inventory_validator.validate(line_item) }.compact raise InsufficientStock if errors.any? end end diff --git a/core/lib/spree/core/stock_configuration.rb b/core/lib/spree/core/stock_configuration.rb index 2f3a4374f60..e3a3c9d07c2 100644 --- a/core/lib/spree/core/stock_configuration.rb +++ b/core/lib/spree/core/stock_configuration.rb @@ -10,6 +10,7 @@ class StockConfiguration attr_writer :allocator_class attr_writer :inventory_unit_builder_class attr_writer :availability_validator_class + attr_writer :inventory_validator_class def coordinator_class @coordinator_class ||= '::Spree::Stock::SimpleCoordinator' @@ -45,6 +46,11 @@ def availability_validator_class @availability_validator_class ||= '::Spree::Stock::AvailabilityValidator' @availability_validator_class.constantize end + + def inventory_validator_class + @inventory_validator_class ||= '::Spree::Stock::InventoryValidator' + @inventory_validator_class.constantize + end end end end diff --git a/core/spec/lib/spree/core/stock_configuration_spec.rb b/core/spec/lib/spree/core/stock_configuration_spec.rb index df98de87d50..8d2437752d0 100644 --- a/core/spec/lib/spree/core/stock_configuration_spec.rb +++ b/core/spec/lib/spree/core/stock_configuration_spec.rb @@ -125,4 +125,23 @@ Object.send(:remove_const, :MyAvailabilityValidator) end end + + describe '#inventory_validator_class' do + subject { stock_configuration.inventory_validator_class } + + let(:stock_configuration) { described_class.new } + + it "returns Spree::Stock::InventoryValidator" do + is_expected.to be ::Spree::Stock::InventoryValidator + end + + it "can be reassigned" do + MyInventoryValidator = Class.new + stock_configuration.inventory_validator_class = MyInventoryValidator.to_s + + expect(subject).to be MyInventoryValidator + + Object.send(:remove_const, :MyInventoryValidator) + end + end end diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb index facffe288dd..ac00e2f4d03 100644 --- a/core/spec/models/spree/order_spec.rb +++ b/core/spec/models/spree/order_spec.rb @@ -1702,6 +1702,48 @@ def initialize(order) end end + describe "#ensure_inventory_units" do + subject { order.send(:ensure_inventory_units) } + + before do + class TestValidator + def validate(line_item) + if line_item.quantity != 1 + line_item.errors.add(:quantity, ":(") + end + end + end + + test_stock_config = Spree::Core::StockConfiguration.new + test_stock_config.inventory_validator_class = TestValidator.to_s + stub_spree_preferences(stock: test_stock_config) + end + + let(:order) { create :order_with_line_items, line_items_count: 2 } + + it "uses the configured validator" do + expect_any_instance_of(TestValidator).to receive(:validate).twice.and_call_original + + subject + end + + context "when the line items are valid" do + it "doesn't raise an exception" do + expect { subject }.not_to raise_error + end + end + + context "when the line items are not valid" do + before do + order.line_items.last.quantity = 2 + end + + it "raises an exception" do + expect { subject }.to raise_error(Spree::Order::InsufficientStock) + end + end + end + describe "#validate_line_item_availability" do subject { order.send(:validate_line_item_availability) } From 244c5c056bfd6f5d0af6cc4abc3a12e3542bc809 Mon Sep 17 00:00:00 2001 From: Jared Norman Date: Mon, 6 Jun 2022 10:05:09 -0700 Subject: [PATCH 4/4] Ensure removal of constants in specs This ensures that even if the specs fail, they don't leak constants. --- core/spec/lib/spree/core/stock_configuration_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/spec/lib/spree/core/stock_configuration_spec.rb b/core/spec/lib/spree/core/stock_configuration_spec.rb index 8d2437752d0..8b082840c1b 100644 --- a/core/spec/lib/spree/core/stock_configuration_spec.rb +++ b/core/spec/lib/spree/core/stock_configuration_spec.rb @@ -18,6 +18,7 @@ expect(subject).to be MyCoordinator + ensure Object.send(:remove_const, :MyCoordinator) end end @@ -35,6 +36,7 @@ expect(subject).to be MyEstimator + ensure Object.send(:remove_const, :MyEstimator) end end @@ -52,6 +54,7 @@ expect(subject).to be MyFilter + ensure Object.send(:remove_const, :MyFilter) end end @@ -69,6 +72,7 @@ expect(subject).to be MySorter + ensure Object.send(:remove_const, :MySorter) end end @@ -86,6 +90,7 @@ expect(subject).to be MyAllocator + ensure Object.send(:remove_const, :MyAllocator) end end @@ -103,6 +108,7 @@ expect(subject).to be MyInventoryUnitBuilder + ensure Object.send(:remove_const, :MyInventoryUnitBuilder) end end @@ -122,6 +128,7 @@ expect(subject).to be MyAvailabilityValidator + ensure Object.send(:remove_const, :MyAvailabilityValidator) end end @@ -141,6 +148,7 @@ expect(subject).to be MyInventoryValidator + ensure Object.send(:remove_const, :MyInventoryValidator) end end