Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Backend] Pass options to Simple Coordinator dependencies #5855

Open
Noah-Silvera opened this issue Sep 17, 2024 · 0 comments
Open

[Backend] Pass options to Simple Coordinator dependencies #5855

Noah-Silvera opened this issue Sep 17, 2024 · 0 comments

Comments

@Noah-Silvera
Copy link
Contributor

Noah-Silvera commented Sep 17, 2024

Quick Link to example implementation

Desired Behavior

The problem

It's very useful that the simple_coordinator can have so many of its 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 its 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 proposed 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

The Plan

We propose addressing this 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 configuring the extensible classes that the simple_coordinator uses, such as the location_filter_class.

We have an example of what this implementation could look like in a fork here

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

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.

Concerns

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.) overrod e 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant