fix(core): Unassigning shipping lines from a channel breaks active orders#4494
fix(core): Unassigning shipping lines from a channel breaks active orders#4494kwerie wants to merge 24 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/service/services/shipping-method.service.ts (1)
317-336: Stale order totals after shipping line removal require explicit recalculation.The method removes
ShippingLinerecords directly from the database without recalculating the order's totals (shipping,shippingWithTax,total,totalWithTax). Affected orders will retain stale shipping cost values until the next cart modification triggersapplyPriceAdjustments().Contrast this with
OrderCalculator.applyShipping(), which recalculates shipping as part of the price adjustment workflow. Consider whether orders should have their totals recalculated immediately after shipping line removal, or if eventual recalculation on next modification is acceptable design. If immediate recalculation is needed, injectingOrderServiceand calling a recalculation method would be required, though this adds complexity and potential circular dependency concerns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/services/shipping-method.service.ts` around lines 317 - 336, The removal of ShippingLine rows in removeShippingMethodFromActiveOrders leaves orders with stale totals because no recalculation is triggered; after removing ShippingLine entities (ShippingLine) you should trigger an immediate price recalculation for each affected order by invoking the order-level recalculation routine instead of relying on eventual updates. Inject or obtain the OrderService (or a method that wraps OrderCalculator.applyShipping/applyPriceAdjustments) into the class, collect the distinct order ids from shippingLinesToRemove (or from the loaded relations), remove the ShippingLine entities, then call the OrderService's recalculation method (e.g. orderService.recalculateOrderTotals(order.id) or orderService.applyPriceAdjustments(order.id)) for each affected order; if adding OrderService would create a circular dependency, emit an event (e.g. "order.shippingRemoved") with the order ids and handle recalculation in the OrderService to avoid the cycle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/e2e/shipping-method.e2e-spec.ts`:
- Line 526: Replace the placeholder issue reference in the inline comment "//
https://github.com/vendure-ecommerce/vendure/issues/XXXX" with the real issue
number; update it to "//
https://github.com/vendure-ecommerce/vendure/issues/4492" so the comment points
to the correct issue (search for that exact comment string in
shipping-method.e2e-spec.ts and change XXXX to 4492).
---
Nitpick comments:
In `@packages/core/src/service/services/shipping-method.service.ts`:
- Around line 317-336: The removal of ShippingLine rows in
removeShippingMethodFromActiveOrders leaves orders with stale totals because no
recalculation is triggered; after removing ShippingLine entities (ShippingLine)
you should trigger an immediate price recalculation for each affected order by
invoking the order-level recalculation routine instead of relying on eventual
updates. Inject or obtain the OrderService (or a method that wraps
OrderCalculator.applyShipping/applyPriceAdjustments) into the class, collect the
distinct order ids from shippingLinesToRemove (or from the loaded relations),
remove the ShippingLine entities, then call the OrderService's recalculation
method (e.g. orderService.recalculateOrderTotals(order.id) or
orderService.applyPriceAdjustments(order.id)) for each affected order; if adding
OrderService would create a circular dependency, emit an event (e.g.
"order.shippingRemoved") with the order ids and handle recalculation in the
OrderService to avoid the cycle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32811309-1555-4a16-ba09-f1cceb81056f
📒 Files selected for processing (2)
packages/core/e2e/shipping-method.e2e-spec.tspackages/core/src/service/services/shipping-method.service.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/e2e/shipping-method.e2e-spec.ts`:
- Around line 559-582: The current test only verifies removal of shipping lines
in the same channel; add a regression case that ensures deletion cleans up
orders across channels by: creating/ensuring a second Channel, assigning the
same shipping method to that channel (use whatever mutation/utility you already
have to add the shipping method to another channel), creating an active order in
that second channel via shopClient configured to that channel (use
shopClient.asAnonymousUser() or channel-aware client), applying the shipping
method to that order with SET_SHIPPING_METHOD, then calling
DELETE_SHIPPING_METHOD via adminClient (which triggers
removeShippingMethodFromActiveOrders(ctx, ctx.channelId, id)) and finally
querying GET_ACTIVE_ORDER for the second channel to assert its shippingLines are
removed; reference the existing test symbols SET_SHIPPING_METHOD,
DELETE_SHIPPING_METHOD, GET_ACTIVE_ORDER, shopClient, adminClient and the helper
removeShippingMethodFromActiveOrders to locate where to add this new case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2dddd37-920b-4ec3-af42-414b97174dde
📒 Files selected for processing (1)
packages/core/e2e/shipping-method.e2e-spec.ts
…od is deleted preserves historical state
kwerie
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/service/services/shipping-method.service.ts`:
- Around line 321-334: After removing the ShippingLine entities, iterate the
unique parent orders from shippingLinesToRemove and trigger the order
totals/promotions recalculation for each so shipping, shippingWithTax, total and
promotion state are updated; specifically, after await
this.connection.getRepository(ctx, ShippingLine).remove(shippingLinesToRemove)
collect the distinct order objects from shippingLinesToRemove and call the
existing order recalculation method (e.g. OrderService.recalculateOrderTotals,
OrderCalculator.recalculateOrder or whichever project function handles order
aggregates) for each order, then persist any changes to the Order repository.
- Around line 251-252: The soft-delete path in ShippingMethodService
(softDelete) does not invoke removeShippingMethodFromActiveOrders, so active
orders can still reference the deleted shipping method; update the softDelete
implementation in shipping-method.service.ts to call
this.removeShippingMethodFromActiveOrders(ctx, null, shippingMethodId) (or
undefined) so the cleanup runs without a channel filter, and ensure
removeShippingMethodFromActiveOrders correctly treats a null/undefined channelId
as “all channels” and removes the shipping method from any active orders prior
to or immediately after marking the method as soft-deleted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51bbf822-88fc-4e3e-9ef6-23a7822658d9
📒 Files selected for processing (2)
packages/core/e2e/shipping-method.e2e-spec.tspackages/core/src/service/services/shipping-method.service.ts
michaelbromley
left a comment
michaelbromley
left a comment
There was a problem hiding this comment.
Thanks for tackling this — the bug is real and the fix direction makes sense. Cleaning up shipping lines proactively on unassign is the right approach over trying to handle nulls in the resolver.
A couple of things to address though, one of which is a blocker:
1. Order totals go stale after removing the shipping line (blocker)
When you remove the ShippingLine rows, the parent Order's shipping and shippingWithTax columns still hold the old values — they're persisted @Money() columns, not derived on-the-fly. So after this runs, the customer sees no shipping lines but the order total still includes the shipping cost. It won't self-correct until the next applyPriceAdjustments() (e.g. adding another item to the cart).
You'll need to either zero out shipping/shippingWithTax on affected orders and save them, or trigger a proper order recalculation after the removal. Given that pulling in OrderService here would create a circular dependency, the simplest option might be to reset those columns directly and save the order.
2. Test should assert on order totals
The test checks that shippingLines is empty, which is good — but it should also verify that shipping, total, and totalWithTax are correct after the operation. That would have caught the stale totals issue.
See inline comments for the specifics.
|
Had a closer look at this and there's a broader issue to consider beyond active orders. The non-active order problemThe This is arguably worse than the active order case, since historical orders are read-only and there's nothing the customer or admin can do to "fix" them. Two separate fixes needed1. Active orders — use
|
|
@michaelbromley Thanks for the detailed feedback! Good call on the historical orders angle, I hadn't considered that. I'll rework the approach along those lines. 😄 |
…ed from channel Instead of directly removing shipping lines, subscribe to ChangeChannelEvent and use applyPriceAdjustments() to properly recalculate totals. Also allow the ShippingLine resolver to look up shipping methods across channels so historical orders still resolve correctly. Fixes vendurehq#4494
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/e2e/shipping-method.e2e-spec.ts (1)
655-657: Use one source of truth for payment method code.Line 657 hardcodes
'test-payment-method'while Line 694 usestestSuccessfulPaymentMethod.code. Reuse the same constant in both places to prevent drift.♻️ Suggested change
- code: 'test-payment-method', + code: testSuccessfulPaymentMethod.code,Also applies to: 694-694
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/e2e/shipping-method.e2e-spec.ts` around lines 655 - 657, The payment method code is hardcoded in the CREATE_PAYMENT_METHOD call; replace the literal 'test-payment-method' with a single source of truth (either use the existing testSuccessfulPaymentMethod.code value or introduce a shared constant like testPaymentMethodCode) so both the CREATE_PAYMENT_METHOD invocation and the later reference (testSuccessfulPaymentMethod.code) use the same identifier; update the adminClient.query(CREATE_PAYMENT_METHOD, { input: { code: ... }}) call to reference that shared symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/e2e/shipping-method.e2e-spec.ts`:
- Line 39: You're importing CREATE_PAYMENT_METHOD from another spec which causes
that spec's top-level describe to run as a side effect; remove the import from
payment-method.e2e-spec and either inline the CREATE_PAYMENT_METHOD GraphQL
mutation in shipping-method.e2e-spec.ts (e.g., define the gql mutation constant
in this file) or move the mutation (and any fragments like PaymentMethod) into a
shared GraphQL definitions module and import from that module instead so no spec
files are imported; update references in shipping-method.e2e-spec.ts to use the
new local/shared CREATE_PAYMENT_METHOD constant.
---
Nitpick comments:
In `@packages/core/e2e/shipping-method.e2e-spec.ts`:
- Around line 655-657: The payment method code is hardcoded in the
CREATE_PAYMENT_METHOD call; replace the literal 'test-payment-method' with a
single source of truth (either use the existing testSuccessfulPaymentMethod.code
value or introduce a shared constant like testPaymentMethodCode) so both the
CREATE_PAYMENT_METHOD invocation and the later reference
(testSuccessfulPaymentMethod.code) use the same identifier; update the
adminClient.query(CREATE_PAYMENT_METHOD, { input: { code: ... }}) call to
reference that shared symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50a1c734-78e2-4f97-bf7e-7f7f30dcca55
📒 Files selected for processing (1)
packages/core/e2e/shipping-method.e2e-spec.ts
…eletion' into fix/shipping-line-deletion
|
I've added an optional channel param to |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Hi @michaelbromley, gentle ping on this one. Could you take a look when you have a moment? :) |
|
@kwerie Hi, thanks for making the changes. I’ve got this on the radar for the team to review at the next opportunity. Thanks for your patience. |
2 participants
Description
When unassigning a shipping method from a channel it breaks currently active orders because a reference on the shipping method still remains on the order, but the resolver cannot find the shipping method because it is removed from the channel and the schema has ShippingMethod declared as non-nullable.
Breaking changes
No
Screenshots
See #4492
Checklist
📌 Always:
👍 Most of the time:
Fixes #4492, relates to #4486