-
-
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 4 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,98 @@ | ||
| # 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) | ||
| @inventory_units = context[:inventory_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. 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
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. Context object should definitely be a PORO, not something that extends hash. We want something that exposes an API that reflects how it's meant to be used. |
||
|
|
||
| @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 | ||
| @stock_locations = sorted_stock_locations | ||
|
|
||
| @desired = Spree::StockQuantities.new(@inventory_units_by_variant.transform_values(&:count)) | ||
| @availability = Spree::Stock::Availability.new( | ||
| variants: @desired.variants, | ||
| stock_locations: @stock_locations | ||
| ) | ||
|
|
||
| @allocator = Spree::Config.stock.allocator_class.new(@availability) | ||
| end | ||
|
|
||
| def shipments | ||
| @shipments ||= begin | ||
| @packages = build_packages | ||
| shipments = build_shipments | ||
|
|
||
| # Make sure we don't add the proposed shipments to the order | ||
| @order.shipments = @order.shipments - shipments | ||
|
|
||
| shipments | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def load_stock_locations | ||
| Spree::StockLocation.all | ||
| end | ||
|
|
||
| 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 | ||
| end | ||
| end | ||
|
|
||
| def build_packages | ||
| # Allocate any available on hand inventory and remaining desired inventory from backorders | ||
| on_hand_packages, backordered_packages, leftover = @allocator.allocate_inventory(@desired) | ||
|
|
||
| raise Spree::Order::InsufficientStock.new(items: leftover.quantities) unless leftover.empty? | ||
|
|
||
| packages = @stock_locations.map do |stock_location| | ||
| # Combine on_hand and backorders into a single package per-location | ||
| on_hand = on_hand_packages[stock_location.id] || Spree::StockQuantities.new | ||
| backordered = backordered_packages[stock_location.id] || Spree::StockQuantities.new | ||
|
|
||
| # Skip this location it has no inventory | ||
| next if on_hand.empty? && backordered.empty? | ||
|
|
||
| # Turn our raw quantities into a Stock::Package | ||
| package = Spree::Stock::Package.new(stock_location) | ||
| package.add_multiple(get_units(on_hand), :on_hand) | ||
| package.add_multiple(get_units(backordered), :backordered) | ||
|
|
||
| package | ||
| end.compact | ||
|
|
||
| # Split the packages | ||
| split_packages(packages) | ||
| end | ||
|
|
||
| def split_packages(initial_packages) | ||
| 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 | ||
|
|
||
| def get_units(quantities) | ||
| # Change our raw quantities back into inventory units | ||
| quantities.flat_map do |variant, quantity| | ||
| @inventory_units_by_variant[variant].shift(quantity) | ||
| 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 | ||
|
|
||
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.
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 makingorderandinventory_unitsoptional, and adding acontextkeyword argument that can be passed instead that contains both attributes? That would allow arbitrary extension of the arguments passed into the stock coordinatorThere 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.
I like this idea, but we are dealing with the existing API, so that would be a trickier change to make.