From cd5b4be61c61b98df881e4e91b61a738112e99f4 Mon Sep 17 00:00:00 2001 From: Noah Silvera Date: Tue, 3 Sep 2024 13:13:32 -0400 Subject: [PATCH] Pass options to coordinator dependencies It's very useful that the simple_coordinator can have so many of it's internal classes configured. However, they can only be configured for one set of hardcoded behavior, based on the two input values, an order and inventory_units If you want the coordinator to behave differently in different scenarios e.g, the admin and the frontend, then you have to start getting creative. The simple_coordinator (and all it's configured classes) in their current state can only react to the state of the order and inventory_units argument, or they can react to globally set state (which is not a great pattern). Currently, the simple_coordinator is only called in two places in Solidus: during exchanges, and during creating proprosed shipments. However, it is reasonable for a complicated store to want to build shipments in other scenarios. One workaround to getting the coordinator to behave differently in these different locations is to include any arguments that you want to pass to the coordinator on the order as an attribute or database column, and then read the order attribute in your configured custom class. However, this isn't even a perfect workaround, because not every configurable class is passed the order (e.g. the location_sorter_class). To truly have the coordinator behave differently in different locations you need to do minor to extensive monkey patching This solution addresses the problem by allowing generic options to be passed to the simple coordinator, which are then passed down to each configurable class. This means that any caller of the simple_coordinator can customize the behavior it wants through these options and overriding the configurable inner classes. This for example, allows for customizations like shipment building behavior that is specific to the admin, where a admin user could be allowed to rebuild shipments with a stock location that is not normally available to users. This is however a **breaking change** for certain consumers of Solidus. Since this change adds an argument to the constructor of the following classes, if a consumer of solidus has a.) configured their own custom class and b.) overrode the constructor of their own custom class then their custom class could break. However, this error will cause shipment building to fail, so it should be very obvious to spot and correct. Additionally, there were few reasons to override the constructor of these configurable classes unless you had also overridden the simple_coordinator, as you did not have control of the arguments passed to these configurable classes. `inventory_unit_builder_class` `location_filter_class` `location_sorter_class` `allocator_class` `estimator_class` e.g. a custom configured stock location filter like this would be broken class StockLocationFilter < Spree::Stock::LocationFilter::Base def initialize(stock_locations, order) super(stock_locations, order) @some_variable = 'Some initializer' end ... end However, initializers like this will be fine, as they implicitly pass the original arguments: class StockLocationFilter < Spree::Stock::LocationFilter::Base def initialize(_stock_locations, order) super @my_variable = 'Some Value' end ... end Co-authored-by: Benjamin Willems --- core/app/models/spree/stock/allocator/base.rb | 5 +++-- core/app/models/spree/stock/estimator.rb | 4 ++++ .../models/spree/stock/inventory_unit_builder.rb | 3 ++- .../models/spree/stock/location_filter/base.rb | 7 +++++-- .../models/spree/stock/location_sorter/base.rb | 7 +++++-- .../app/models/spree/stock/simple_coordinator.rb | 15 ++++++++------- .../spree/stock/simple_coordinator_spec.rb | 16 +++++++++------- 7 files changed, 36 insertions(+), 21 deletions(-) diff --git a/core/app/models/spree/stock/allocator/base.rb b/core/app/models/spree/stock/allocator/base.rb index b724d6d8bdf..4aef1122bee 100644 --- a/core/app/models/spree/stock/allocator/base.rb +++ b/core/app/models/spree/stock/allocator/base.rb @@ -4,10 +4,11 @@ module Spree module Stock module Allocator class Base - attr_reader :availability + attr_reader :availability, :coordinator_options - def initialize(availability) + def initialize(availability, coordinator_options: {}) @availability = availability + @coordinator_options = coordinator_options end def allocate_inventory(_desired) diff --git a/core/app/models/spree/stock/estimator.rb b/core/app/models/spree/stock/estimator.rb index bb992f0116b..e1f19112c10 100644 --- a/core/app/models/spree/stock/estimator.rb +++ b/core/app/models/spree/stock/estimator.rb @@ -6,6 +6,10 @@ class Estimator class ShipmentRequired < StandardError; end class OrderRequired < StandardError; end + def initialize(coordinator_options: {}) + @coordinator_options = coordinator_options + end + # Estimate the shipping rates for a package. # # @param package [Spree::Stock::Package] the package to be shipped diff --git a/core/app/models/spree/stock/inventory_unit_builder.rb b/core/app/models/spree/stock/inventory_unit_builder.rb index 342adc0d258..ce813cefd0e 100644 --- a/core/app/models/spree/stock/inventory_unit_builder.rb +++ b/core/app/models/spree/stock/inventory_unit_builder.rb @@ -3,8 +3,9 @@ module Spree module Stock class InventoryUnitBuilder - def initialize(order) + def initialize(order, coordinator_options: {}) @order = order + @coordinator_options = coordinator_options end def units diff --git a/core/app/models/spree/stock/location_filter/base.rb b/core/app/models/spree/stock/location_filter/base.rb index e85246030ef..e1490e4847a 100644 --- a/core/app/models/spree/stock/location_filter/base.rb +++ b/core/app/models/spree/stock/location_filter/base.rb @@ -12,7 +12,7 @@ class Base # @!attribute [r] stock_locations # @return [Enumerable] # a collection of locations to sort - attr_reader :stock_locations + attr_reader :stock_locations, :coordinator_options # @!attribute [r] order # @return @@ -25,9 +25,12 @@ class Base # a collection of locations to sort # @param order # the order we are creating the shipment for - def initialize(stock_locations, order) + # @param coordinator_options [Hash] + # a set of options passed from the stock_coordinator + def initialize(stock_locations, order, coordinator_options: {}) @stock_locations = stock_locations @order = order + @coordinator_options = coordinator_options end # Filter the stock locations. diff --git a/core/app/models/spree/stock/location_sorter/base.rb b/core/app/models/spree/stock/location_sorter/base.rb index 821f34b1930..bab46ba3734 100644 --- a/core/app/models/spree/stock/location_sorter/base.rb +++ b/core/app/models/spree/stock/location_sorter/base.rb @@ -15,14 +15,17 @@ class Base # @!attribute [r] stock_locations # @return [Enumerable] # a collection of locations to sort - attr_reader :stock_locations + attr_reader :stock_locations, :coordinator_options # Initializes the stock location sorter. # # @param stock_locations [Enumerable] # a collection of locations to sort - def initialize(stock_locations) + # @param coordinator_options [Hash] + # a set of options passed from the stock_coordinator + def initialize(stock_locations, coordinator_options: {}) @stock_locations = stock_locations + @coordinator_options = coordinator_options end # Sorts the stock locations. diff --git a/core/app/models/spree/stock/simple_coordinator.rb b/core/app/models/spree/stock/simple_coordinator.rb index fa5a11308aa..b2735b34df4 100644 --- a/core/app/models/spree/stock/simple_coordinator.rb +++ b/core/app/models/spree/stock/simple_coordinator.rb @@ -25,16 +25,17 @@ class SimpleCoordinator # @api private attr_reader :inventory_units, :splitters, :stock_locations, :filtered_stock_locations, :inventory_units_by_variant, :desired, - :availability, :allocator, :packages + :availability, :allocator, :packages, :coordinator_options - def initialize(order, inventory_units = nil) + def initialize(order, inventory_units = nil, coordinator_options: {}) @order = order + @coordinator_options = coordinator_options @inventory_units = - inventory_units || Spree::Config.stock.inventory_unit_builder_class.new(order).units + inventory_units || Spree::Config.stock.inventory_unit_builder_class.new(order, coordinator_options:).units @splitters = Spree::Config.environment.stock_splitters - @filtered_stock_locations = Spree::Config.stock.location_filter_class.new(load_stock_locations, order).filter - sorted_stock_locations = Spree::Config.stock.location_sorter_class.new(filtered_stock_locations).sort + @filtered_stock_locations = Spree::Config.stock.location_filter_class.new(load_stock_locations, order, coordinator_options:).filter + sorted_stock_locations = Spree::Config.stock.location_sorter_class.new(filtered_stock_locations, coordinator_options:).sort @stock_locations = sorted_stock_locations @inventory_units_by_variant = @inventory_units.group_by(&:variant) @@ -44,7 +45,7 @@ def initialize(order, inventory_units = nil) stock_locations: stock_locations ) - @allocator = Spree::Config.stock.allocator_class.new(availability) + @allocator = Spree::Config.stock.allocator_class.new(availability, coordinator_options:) end def shipments @@ -69,7 +70,7 @@ def build_shipments # Turn the Stock::Packages into a Shipment with rates packages.map do |package| shipment = package.shipment = package.to_shipment - shipment.shipping_rates = Spree::Config.stock.estimator_class.new.shipping_rates(package) + shipment.shipping_rates = Spree::Config.stock.estimator_class.new(coordinator_options:).shipping_rates(package) shipment 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 02a245b1288..f3ba604f347 100644 --- a/core/spec/models/spree/stock/simple_coordinator_spec.rb +++ b/core/spec/models/spree/stock/simple_coordinator_spec.rb @@ -11,22 +11,26 @@ module Stock describe "#shipments" do it 'uses the pluggable estimator class' do - expect(Spree::Config.stock).to receive(:estimator_class).and_call_original + expect(Spree::Config.stock.estimator_class).to receive(:new).with(coordinator_options: {}).and_call_original + subject.shipments end it 'uses the configured stock location filter' do - expect(Spree::Config.stock).to receive(:location_filter_class).and_call_original + expect(Spree::Config.stock.location_filter_class).to receive(:new).with(anything, anything, coordinator_options: {}).and_call_original + subject.shipments end it 'uses the configured stock location sorter' do - expect(Spree::Config.stock).to receive(:location_sorter_class).and_call_original + expect(Spree::Config.stock.location_sorter_class).to receive(:new).with(anything, coordinator_options: {}).and_call_original + subject.shipments end it 'uses the pluggable allocator class' do - expect(Spree::Config.stock).to receive(:allocator_class).and_call_original + expect(Spree::Config.stock.allocator_class).to receive(:new).with(anything, coordinator_options: {}).and_call_original + subject.shipments end @@ -39,9 +43,7 @@ module Stock end it 'uses the pluggable inventory unit builder class' do - expect(Spree::Config.stock) - .to receive(:inventory_unit_builder_class) - .and_call_original + expect(Spree::Config.stock.inventory_unit_builder_class).to receive(:new).with(anything, coordinator_options: {}).and_call_original subject.shipments end