-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Introduce new middleware stock coordinator #6484
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
base: main
Are you sure you want to change the base?
Changes from 11 commits
b67737d
8b98fa7
dbb8066
b762d99
f861dcd
7b98c26
a4a3a6d
c6f14a4
2feb441
26e9893
ca9ae0d
cec1968
a5893c3
898446b
c4a3a33
d449631
bb9e0ed
3028892
8547546
558d149
27c1073
851a690
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Spree | ||
| module Stock | ||
| class Coordinator | ||
| def initialize(order, inventory_units: nil) | ||
| @context = {order:, inventory_units:} | ||
| @order = order | ||
|
|
||
| Middleware::InventoryUnit.new.call(@context) | ||
| Middleware::InventoryUnitGroup.new.call(@context) | ||
| Middleware::StockLocation.new.call(@context) | ||
| Middleware::Desired.new.call(@context) | ||
| Middleware::Availability.new.call(@context) | ||
| end | ||
|
|
||
| def shipments | ||
| @shipments ||= begin | ||
| Middleware::Allocate.new.call(@context) | ||
| Middleware::Package.new.call(@context) | ||
| Middleware::Shipment.new.call(@context) | ||
|
|
||
| shipments = @context[:shipments] | ||
|
|
||
| # Make sure we don't add the proposed shipments to the order | ||
| @order.shipments = @order.shipments - shipments | ||
|
|
||
| shipments | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| module Spree | ||
| module Stock | ||
| module Middleware | ||
| class Allocate | ||
| def call(context) | ||
| allocator = Spree::Config.stock.allocator_class.new(context[:availability]) | ||
| on_hand_packages, backordered_packages, leftover = allocator.allocate_inventory(context[:desired]) | ||
|
|
||
| raise Spree::Order::InsufficientStock.new(items: leftover.quantities) unless leftover.empty? | ||
|
|
||
| context[:on_hand_packages] = on_hand_packages | ||
| context[:backordered_packages] = backordered_packages | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| module Spree | ||
| module Stock | ||
| module Middleware | ||
| class Availability | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a similar manner to thinking about just ignoring the configured classes/monkey patches and developing a new pattern users can opt into, I think the code would be easier to understand, trace and override if we just copied the stock availability logic into this class |
||
| def call(context) | ||
| context[:availability] = Spree::Stock::Availability.new( | ||
| variants: context[:desired].variants, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Piggybacking off jared's point here - I would use a |
||
| stock_locations: context[:stock_locations] | ||
| ) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| module Spree | ||
| module Stock | ||
| module Middleware | ||
| class Desired | ||
| def call(context) | ||
| context[:desired] = Spree::StockQuantities.new(context[:inventory_unit_groups].transform_values(&:count)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think stock quantities is a great object to have defined on the context and be created by middleware - it's a well defined custom object, build and used by items farther down in the chain! If anything, it's value can be read, overridden and replaced by inserted middleware if a user desires |
||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| module Spree | ||
| module Stock | ||
| module Middleware | ||
| class InventoryUnit | ||
| def call(context) | ||
| order = context[:order] | ||
|
|
||
| context[:inventory_units] = context[:inventory_units] || | ||
| Spree::Config.stock.inventory_unit_builder_class.new(order).units | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's worth entertaining, at least. |
||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| module Spree | ||
| module Stock | ||
| module Middleware | ||
| class InventoryUnitGroup | ||
| def call(context) | ||
| context[:inventory_unit_groups] = context[:inventory_units].group_by(&:variant) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| module Spree | ||
| module Stock | ||
| module Middleware | ||
| class Package | ||
| def call(context) | ||
| packages = context[:stock_locations].map do |stock_location| | ||
| on_hand = context[:on_hand_packages][stock_location.id] || Spree::StockQuantities.new | ||
| backordered = context[:backordered_packages][stock_location.id] || Spree::StockQuantities.new | ||
|
|
||
| next if on_hand.empty? && backordered.empty? | ||
|
|
||
| package = Spree::Stock::Package.new(stock_location) | ||
| package.add_multiple(get_units(context, on_hand), :on_hand) | ||
| package.add_multiple(get_units(context, backordered), :backordered) | ||
|
|
||
| package | ||
| end.compact | ||
|
|
||
| context[:packages] = split_packages(packages) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def get_units(context, quantities) | ||
| quantities.flat_map do |variant, quantity| | ||
| context[:inventory_unit_groups][variant].shift(quantity) | ||
| end | ||
| end | ||
|
|
||
| def split_packages(initial_packages) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO splitter chain should be a separate middleware that splits packages after they are created |
||
| splitters = Spree::Config.environment.stock_splitters | ||
|
|
||
| initial_packages.flat_map do |initial_package| | ||
| stock_location = initial_package.stock_location | ||
| Spree::Stock::SplitterChain.new(stock_location, splitters).split([initial_package]) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| module Spree | ||
| module Stock | ||
| module Middleware | ||
| class Shipment | ||
| 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still leaning towards inlining these classes and having users replace |
||
| shipment | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| module Spree | ||
| module Stock | ||
| module Middleware | ||
| class StockLocation | ||
| def call(context) | ||
| filtered_stock_locations = Spree::Config.stock.location_filter_class.new( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think one of the valuable ways to use middleware is to allow these transformations to occur inline, chains! I was imagining the middleware chain would look like [
...
"Spree::Stock::Middleware::LoadStockLocations",
"Spree::Stock::Middleware::SortStockLocations",
...
] |
||
| load_stock_locations, context[:order] | ||
| ).filter | ||
| sorted_stock_locations = Spree::Config.stock.location_sorter_class.new( | ||
| filtered_stock_locations | ||
| ).sort | ||
|
|
||
| context[:stock_locations] = sorted_stock_locations | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def load_stock_locations | ||
| Spree::StockLocation.all | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the goal of middleware is to make it really easy for users to change behavior with minor config changes or overridding tiny classes.
This seems like a great candidate to be split into multiple middleware.