solidusio / solidusio/solidus

Pass caller options through `Spree::Stock::SimpleCoordinator`

Open
#5,855 5 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

changelog:solidus_core
Dominant language
Ruby
Stars
5.3k
Forks
1.4k
Avg merge
1d 22h
Merged PRs (30d)
62

Description

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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with core/app/models/spree/stock/simple_coordinator.rb and the example implementation, then review the listed inventory_unit_builder_class, location_filter_class, location_sorter_class, allocator_class, and estimator_class integrations. Done means caller options reach each configurable class while the documented existing coordinator behavior remains covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby
Domain
backend
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.