Reuse the order's line items when building a shipping manifest - #6524
Open
mamhoff wants to merge 3 commits into
Open
Reuse the order's line items when building a shipping manifest#6524mamhoff wants to merge 3 commits into
mamhoff wants to merge 3 commits into
Conversation
With the new in-memory order updater and especially SolidusPromotions, the shipment manifest needs to be aware of any in-memory changes to line items. This adds code that allows the manifest to load line items from a specific order.
When creating a manifest, this uses the Spree::ShippingManifest's new ability to re-use already loaded line items for the shipping manifest items.
This uses the same optimization from before to use loaded line items in the shipment manifest (for unpacked items).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6524 +/- ##
=======================================
Coverage 92.02% 92.03%
=======================================
Files 1035 1035
Lines 21141 21152 +11
=======================================
+ Hits 19456 19467 +11
Misses 1685 1685 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Spree::ShippingManifestbuilds its items from inventory units, and reaches the lineitem through
unit.line_item. Since Rails has no identity map, that returns a secondinstance of a row the caller almost always already holds in memory as part of
order.line_items.Two consequences:
persisted are invisible to the manifest, which reports the values as they were on
disk. This is the bug that motivated the change.
This adds an optional
order:keyword toShippingManifest#initialize. When given, andwhen that order's line items are already loaded, the manifest resolves each item's line
item from the order instead of from the inventory unit. It falls back to
unit.line_itemotherwise, so behaviour is unchanged for every existing caller.
Implementation notes
order.association(:line_items).loaded?. It is an optimization for callers that alreadyhave the line items, never a reason to issue a query.
Shipment#manifestpassesorder: (order if association(:order).loaded?)rather thanorder: order— it must not trigger a load ofthe order. In the common path (
order.shipments) the association is already populatedvia
inverse_of, so this costs nothing.association targets are reassigned and no inventory unit is touched.
for_orderpropagates the order, which is howCarton#manifest_for_orderpicks thisup.
Carton#manifestis deliberately left alone — a carton can span multiple orders,so there is no single order to pass.
nilline_item_id(the association isoptional: true) and units belonging to adifferent order both fall through to the existing behaviour via
Hash#fetchwith ablock.
group_by(&:variant_id)/group_by(&:line_item_id)optimization initemsispreserved. This arguably extends it: the one remaining association call per group now
resolves from memory too.
Call sites updated
Spree::Shipment#manifestspree/admin/orders/_shipment— this one builds the manifest fromshipment.inventory_units.where(carton_id: nil), and already hasorderin scope.Behaviour change to be aware of
A manifest built with
order:now surfaces in-memory, unsaved changes to line items.That is the intent, but it is observable: anything relying on the manifest's line item
being a clean read of the row will see the order's working copy instead.
Why not
inverse_of?InventoryUnitandLineItemalready declare it on both sides, butinverse_ofonlylinks objects loaded through one another. The shipment → inventory units → line item
path never passes through
order.line_items.Not addressed
first_unit.varianthas the same shape and gets no equivalent treatment. Variants arenot mutated in memory during order recalculation the way adjustments are, so there is no
correctness bug there — only a preloading question, which callers can handle with
includes.Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: