Introduce new middleware stock coordinator by sofiabesenski4 · Pull Request #6484 · solidusio/solidus

sofiabesenski4 · 2026-05-14T18:54:31Z

WIP

This refactoring of the existing SimpleCoordinator leaves it in a state that retains the original functionality, and higher level shape, but encapsulates the lower level logic into middleware components.

Next steps will be to reconsider how the middleware components are encapsulated, setting up the middleware orchestration, and adding unit specs to prevent regressions.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I agree that my PR will be published under the same license as Solidus.
I have written a thorough PR description.
I have kept my commits small and atomic.
I have localized any and all user-facing strings that I added to the source code.
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.

We want to support additional keyword arguments to this class and a positional argument interferes with this. Co-authored-by: Jared Norman <jared@super.gd> Co-authored-by: Adam Mueller <adam@super.gd> Co-authored-by: Noah Silvera <noah@super.gd>

codecov · 2026-05-14T19:02:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.46%. Comparing base (a0746e6) to head (851a690).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6484      +/-   ##
==========================================
+ Coverage   89.68%   93.46%   +3.78%     
==========================================
  Files         991      534     -457     
  Lines       20832    10532   -10300     
==========================================
- Hits        18683     9844    -8839     
+ Misses       2149      688    -1461

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

Co-authored-by: Noah Silvera <noah@super.gd> Co-authored-by: Adam Mueller <adam@super.gd>

This starts the pattern of delegating low level operations to middleware objects.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

jarednorman · 2026-05-14T21:03:07Z

+          context[:availability] = Spree::Stock::Availability.new(
+            variants: context[:desired].variants,
+            stock_locations: context[:stock_locations]
+          )


This is definitely an example of how overusing this approach makes the code worse. This has seriously obfuscated the dependency chain of the availability and desired variants objects. If we have some dependent computed stuff like this, maybe we just make this methods on the context object or something? It's worth considering this further.

Agreed! I didn't put too much thought into how things shook out entirely at this point in the draft. Will definitely revisit before opening for review.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

These are pure derived values with tight dependency chains, not independent pipeline stages. Expressing them as memoized methods on the Context makes their relationships explicit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Will fix up later

sofiabesenski4 · 2026-06-02T17:48:18Z

@jarednorman ignore the tedious back + forth refactoring that is happening here. I will fixup a lot of these commits and make the history cohesive.

Is this shape in the final diff more aligned with the pattern you were thinking about?

Will fix this up

jarednorman

Yeah, this is much closer. I think we can do even better than this, but this is the right direction, I think.

I do think the "shape" of this problem fits less well than with the order recalculator, so I want to be careful about this change. We may find a different design that fits this part of the code better (and solves the original problem of parameterization) while refactoring this, and that's totally fine.

Noah-Silvera

Hey Sofia! I took a pass on this and had some ideas around how the middleware pattern might make more sense for this specific use case!

In summary, IMO the middleware pattern will only makes things easier in this case to understand and override if the following is true:

It reduces the obfuscated relationships and tracing rabbit holes introduced by configurable classes
It produces structured outputs at each step that are easy to locate and understand in the codebase without having to fully understand the previous middleware
The middleware are tiny, iterative pieces that can be individually replaced & injected between to allow for maximum flexibility in the building process.

I added some comments to this end, LMK what you think of them! Curious about your thoughts and if we can come to some solution in between!

Dropping the configurable classes entirely in favour of new code/a new pattern might be a dealbreaker, but I'm not sure! cc: @jarednorman @adammathys for thoughts too

Noah-Silvera · 2026-06-17T18:09:17Z

+        "Spree::Stock::Middleware::Package",
+        "Spree::Stock::Middleware::Shipment"
+      ]
      class_name_attribute :estimator_class, default: "::Spree::Stock::Estimator"


My first thought is we should let the middleware list be overridden, or use class name attributes to choose which classes exist in the middleware, but not both.

Using both just increases the possible matrix of overrides that users can configure, and makes the code path hard to trace

My gut is that this should just be a "opt in"feature, and you have to port your existing overrides to middlewares if you want to use. I think we should just get rid of configurable classes being used inside middleware or monkey patching middleware; it's better to have smaller middlewares that users can just override wholesale

Noah-Silvera · 2026-06-17T18:56:33Z

+          order = context[:order]
+
+          context[:inventory_units] = context[:inventory_units] ||
+            Spree::Config.stock.inventory_unit_builder_class.new(order).units


as I mentioned in a previous comment, WDYT of getting rid of this configuration altogether in favor of putting the work in this class? Reducing obfuscation, but at the cost of a potential harder migration?

Noah-Silvera · 2026-06-17T18:58:29Z

-          inventory_units || Spree::Config.stock.inventory_unit_builder_class.new(order).units
+
+        Middleware::InventoryUnit.new.call(context)
+        @inventory_units = context[:inventory_units]


If I understand @jarednorman's point correctly, I agree we should have getters/setters for the methods on context to reduce obfuscation/add some runtime guarantees.

Maybe make context an object that extends hash? then custom attributes can be set? I'm not sure, this might be overkill and a hash like it is rn might be fine

Noah-Silvera · 2026-06-17T18:59:50Z

  module Stock
    class Coordinator
      def initialize(order, inventory_units: nil)
+        context = {order:, inventory_units:}


Once of the goals of the initial work to make this more flexible was to allow arbitrary context to be passed into the coordinator that can be used to guide it's behavior.

Instead of turning inventory_units: into a keyword argument, what about making order and inventory_units optional, and adding a context keyword argument that can be passed instead that contains both attributes? That would allow arbitrary extension of the arguments passed into the stock coordinator

Noah-Silvera · 2026-06-17T19:01:18Z

        sorted_stock_locations = Spree::Config.stock.location_sorter_class.new(@filtered_stock_locations).sort
        @stock_locations = sorted_stock_locations

-        @inventory_units_by_variant = @inventory_units.group_by(&:variant)


absolute nit of a nit - this should be in the next commit right?

Noah-Silvera · 2026-06-17T19:15:26Z

+          end
+        end
+
+        def split_packages(initial_packages)


IMO splitter chain should be a separate middleware that splits packages after they are created

Noah-Silvera · 2026-06-17T19:16:35Z

+        def call(context)
+          context[:shipments] = context[:packages].map do |package|
+            shipment = package.shipment = package.to_shipment
+            shipment.shipping_rates = Spree::Config.stock.estimator_class.new.shipping_rates(package)


I'm still leaning towards inlining these classes and having users replace Spree::Stock::Middleware::BuildShipment in the middleware list if they want to change behavior

Noah-Silvera · 2026-06-17T19:18:04Z

            shipment
          end
+
+          yield context


Love the yield pattern!

Noah-Silvera · 2026-06-17T19:22:20Z

@@ -11,8 +11,6 @@ class StockConfiguration < Spree::Preferences::Configuration
        "Spree::Stock::Middleware::InventoryUnit",


One of the things that this way of defining middleware obfuscates is the outputs of each class. Since the outputs could theoretically be anything, it be useful to define them to make the relationships easier to understand. It would also allow us to define getter and setter methods on the defined outputs

WDYT? I'm not sold on this, but it was just an idea
e.g.

middleware = [ { class: "Spree::Stock::Middleware::InventoryUnit", outputs: [:inventory_units] }, { class: "Spree::Stock::Middleware::InventoryUnitGroup", outputs: [:inventory_units_by_variant] }, { class: "Spree::Stock::Middleware::LoadStockLocations", outputs: [:stock_locations] }, { class: "Spree::Stock::Middleware::SortStockLocations", outputs: [:stock_locations] }, ... ]

Noah-Silvera · 2026-06-17T19:24:25Z

@@ -6,13 +6,24 @@ class Coordinator
      class Context


TBH I prefer them as independent middleware pipeline stages

These are pure derived values with tight dependency chains, not
independent pipeline stages.

You are correct! But also, this is true for the whole pipeline! Almost every step is essential, and produces output required by the next. I'd rather implicitly assume that in the chain, or use a more strict configuration format (even defining inputs in addition to outputs), and have smaller middleware, easier to override, replace, or insert other middleware in between

github-actions Bot added the changelog:solidus_core Changes to the solidus_core gem label May 14, 2026

sofiabesenski4 mentioned this pull request May 14, 2026

Make SimpleCoordinator inventory_units keyword arg #6483

Merged

5 tasks

Port SimpleCoordinator code into new Coordinator

8b98fa7

Co-authored-by: Noah Silvera <noah@super.gd> Co-authored-by: Adam Mueller <adam@super.gd>

sofiabesenski4 force-pushed the introduce-new-middleware-stock-coordinator branch from 6b44d32 to 4a86af5 Compare May 14, 2026 20:34

sofiabesenski4 and others added 9 commits May 14, 2026 13:37

Update to use the @ instance variable syntax

dbb8066

Co-authored-by: Noah Silvera <noah@super.gd> Co-authored-by: Adam Mueller <adam@super.gd>

Introduce InventoryUnit Stock middleware

b762d99

This starts the pattern of delegating low level operations to middleware objects.

Delegate InventoryUnit grouping to middleware

f861dcd

Delegate Stock StockLocation logic to middleware

7b98c26

Delegate Desired logic to middleware

a4a3a6d

Delegate Availability logic to middleware

c6f14a4

Delegate Allocate logic to middleware

2feb441

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Delegate Package logic to middleware

26e9893

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Delegate Shipment logic to middleware

ca9ae0d

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

sofiabesenski4 force-pushed the introduce-new-middleware-stock-coordinator branch from 4a86af5 to ca9ae0d Compare May 14, 2026 20:38

jarednorman requested changes May 14, 2026

View reviewed changes

sofiabesenski4 and others added 10 commits May 21, 2026 15:00

Add Runner and Context for stock coordination middleware

cec1968

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Add Spree::Core::ClassConstantizer::List for ordered class registries

a5893c3

Move Spree::Core::ClassConstantizer::Set to its own file

898446b

Add add_class_list helper to Spree::Core::EnvironmentExtension

c4a3a33

Register stock coordination middleware list in config

d449631

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Delegate middleware chaining to Runner via config

bb9e0ed

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Extract generic Spree::MiddlewareRunner

3028892

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

WIP: Maybe remove redundant assignment

558d149

WIP: Fix linting

27c1073

Will fix up later

sofiabesenski4 requested a review from jarednorman June 2, 2026 17:47

FIXUP Simplify coordinator logic

851a690

Will fix this up

jarednorman reviewed Jun 2, 2026

View reviewed changes

jarednorman changed the title ~~WIP: Introduce new middleware stock coordinator~~ Introduce new middleware stock coordinator Jun 4, 2026

Noah-Silvera reviewed Jun 17, 2026

View reviewed changes

		@@ -11,8 +11,6 @@ class StockConfiguration < Spree::Preferences::Configuration
		"Spree::Stock::Middleware::InventoryUnit",

Uh oh!

Conversation

sofiabesenski4 commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WIP

Checklist

Uh oh!

codecov Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sofiabesenski4 commented Jun 2, 2026

Uh oh!

jarednorman left a comment

Choose a reason for hiding this comment

Uh oh!

Noah-Silvera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sofiabesenski4 commented May 14, 2026 •

edited

Loading

codecov Bot commented May 14, 2026 •

edited

Loading