Pass options to coordinator dependencies

[Noah-Silvera]

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.

Summary

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

• [ ] I have written a thorough PR description.
• [ ] I have kept my commits small and atomic.
• [ ] I have used clear, explanatory commit messages.

The following are not always needed:

• 📖 I have updated the README to account for my changes.
• 📑 I have documented new code with YARD.
• 🛣️ I have opened a PR to update the guides.
• ✅ I have added automated tests to cover my changes.
• 📸 I have attached screenshots to demo visual changes.

[jarednorman]

I think this makes a lot of sense. Did you consider any other approaches?

[Noah-Silvera]

I think this makes a lot of sense. Did you consider any other approaches?

@jarednorman yes! I can summarize them briefly

Using dependency injection to inject (optionally) the configurable classes

This was a solution we explored that would change the method signature of the simple_coordinator to allow dependency injection for all of it's configurable classes

e.g.

class SimpleCoordinator
	...
	def initialize(order, inventory_units = nil, inventory_unit_builder: nil, stock_location_filter: nil, stock_location_sorter: nil, allocator: nil, ...)
		...
		(inventory_unit_builder || Spree::Config.stock.inventory_unit_builder_class.new(order)).units
		...
	end
	...
end

This accomplishes the same goal of allowing users to change the behavior of the simple_coordinator on a case by case basis, in this case, by passing instance classes they build for each configurable dependency.

This would be a non-breaking change to the simple_coordinator interface, but we felt it went against the existing paradigm of using classes that you can configure and was mixing two patterns. Definitely a viable option though!

@benjaminwil do you remember the downsides to this pattern?

Fully non-breaking change - replacing the simple coordinator and dependency classes

For a fully-non-breaking change for existing users, we could create new base classes for each of the configurable base classes we touch in this change, and replace them in the default config. User's would have the option of switching over to them on upgrade or not. This felt like significant overhead to be worth it for preventing a breaking change that we think will only affect a minority of users.

One problem with this solution is that we would have to use reflection at some point in the code (where we are calling the simple coordinator I think) to determine if we should pass options to the coordinator or not

e.g.

# new BaseClass
module Spree
  module Stock
    module LocationFilter
      class BaseWithOptions
        def initialize(stock_locations, order, options: {})
          @stock_locations = stock_locations
          @order = order
          @options = options
        end
      end
    end
  end
end

# old BaseClass
module Spree
  module Stock
    module LocationFilter
      class BaseWithOptions
        def initialize(stock_locations, order, options: {})
          @stock_locations = stock_locations
          @order = order
          @options = options
        end
      end
    end
  end
end

# new Default Class
module Spree
  module Stock
    module LocationFilter
      class ActiveWithOptions
        def initialize(stock_locations, order, options: {})
          ...
        end
      end
    end
  end
end

# Simple Coordinator

class SimpleCoordinatorWithOptions
	...
	def initialize(order, inventory_units = nil, coordinator_options: {})
		...
		(inventory_unit_builder || Spree::Config.stock.inventory_unit_builder_class.new(order, coordinator_options:)).units
		...
	end
	...
end

# New default in core/stock_configuration
def coordinator_class
  @coordinator_class ||= '::Spree::Stock::SimpleCoordinatorWithOptions'
  @coordinator_class.constantize
end

...

def location_filter_class
  @location_filter_class ||= '::Spree::Stock::LocationFilter::Active'
  @location_filter_class.constantize
end

def location_filter_class
  @location_filter_class ||= '::Spree::Stock::LocationFilter::Active'
  @location_filter_class.constantize
end

# This line would be added to an initializer when running 'install' after the solidus upgrade, with a comment to delete it
# If you want to use the new base filter. We could then remove the old base classes in a major version release (or not!)

config.stock.location_filter_class = 'Spree::Stock::LocationFilter::Active'
config.stock.coordinator_class = 'Spree::Stock::SimpleCoordinatorWithOptions'

# Some place where we call the coordinator
if Spree::Config.stock.coordinator_class.instance_method(:initialize).parameters.map(&:last).include?(:coordinator_options)
  ...
else
 ..
end

@benjaminwil do you remember any other solutions we considered?

[benjaminwil]

No, I don't think we considered anything else. Great summary!

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.82%. Comparing base (fc44726) to head (3b25d6e).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5854   +/-   ##
=======================================
  Coverage   88.82%   88.82%           
=======================================
  Files         837      837           
  Lines       18169    18178    +9     
=======================================
+ Hits        16138    16147    +9     
  Misses       2031     2031           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.