feat(core): Recalculate active orders for current pricing, promotions & shipping eligibility#4928
Conversation
Relates to #OSS-274
…cart-does-not-update-discounts-shipping-pricing-or
…djustmentsIfStale
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds configurable active-order read-time recalculation with TTL and no-op strategies, tracks pricing timestamps, separates shipping promotion recalculation, and validates shipping eligibility and refreshed totals during checkout transitions. ChangesOrder recalculation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ActiveOrderService
participant OrderService
participant OrderCalculator
participant OrderProcess
Client->>ActiveOrderService: read active order
ActiveOrderService->>OrderService: check and recalculate if stale
OrderService->>OrderCalculator: update prices and promotions
OrderCalculator-->>OrderService: recalculated order
OrderService-->>ActiveOrderService: active order
Client->>OrderProcess: transition to ArrangingPayment
OrderProcess->>OrderService: recalculate before eligibility check
OrderService-->>OrderProcess: refreshed pricing
OrderProcess-->>Client: allow or reject transition
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Before/after proof (e2e)The same e2e test was run against the code with the fix reverted vs. with the fix, each on a real rebuild. BEFORE — read-time hook ( The active order still shows the stale total (155880) after an admin price change — the exact "cart does not update pricing" bug. AFTER — with the fix: PASS (5/5) Identical test, red without the fix / green with it → the read-time recalculation and the checkout eligibility gate are what close the bug. Not mocked — both states were real rebuilds. |
|
Thanks for this PR — I think this is the right place for the Saleor-style cart/checkout problems discussion. I like the direction here: recalculating stale active orders and refusing checkout when the selected shipping method is no longer eligible both make sense. The correctness fix is important because stale prices, stale promotions, and stale shipping eligibility should not silently pass into payment. One additional layer I think would be valuable is exposing a queryable The important part for me is not only that the backend recalculates the active order, but that the storefront can explain what changed to the customer before they continue. Examples:
|
|
Adding one more clarification: I do not mean forcing a full heavy recalculation on every browser refresh. I think the important part is defining when the order should be checked and how the result is exposed to the storefront. Suggested flow:
I also think the problem contract should be structured so storefronts can build reliable UX around it. For example, each problem/update could include:
So I see two separate concepts:
This would make the recalculation behavior much clearer for storefronts. The backend can safely recalculate, and the customer can understand exactly why the cart changed instead of seeing totals, discounts, or shipping options change silently. |
|
One broader follow-up thought: I think #4928 and #4930 together solve important parts of checkout correctness, but there may still be a few major pieces needed for a fully storefront-ready checkout reliability model. For me, the follow-up areas would be:
I understand these are probably separate enhancements, not blockers for this PR. But I think they are closely related to the same goal: no silent stale checkout state, no wrong payment amount, no oversell, and clear customer/merchant recovery when something changes. |
|
Thanks @mezzat11 — really thoughtful writeup, and I agree with the direction. For this PR I want to keep the scope tight: #4928 is about correctness — recalculating stale active orders (prices/promotions/taxes) on read via an opt-in strategy, and refusing checkout when the selected shipping method is no longer eligible. That part is right and self-contained, so I'd like to land it as-is. The layer you're describing — a queryable, Saleor-style On your broader points: the payment-session safety (order version / amount hash / idempotency, redirect handling) and the unpaid-order expiration / stock-reservation model are closely related to the stock/oversell work in #4930 (which fixes the oversell TOCTOU) rather than to this PR, so we'll track those under their own issues too. @michaelbromley — flagging this for you: could you weigh in on whether we take the Saleor-style Thanks again for pushing on the storefront-facing side of this — it's the right instinct. |
|
Two issues undercut the PR's stated guarantees. Both reproduced locally; failing tests in a follow-up comment. 1. Read-time recalc leaves stale shipping discounts
Repro: cart with a free-shipping promo → disable the promo → read The customer keeps free shipping from a disabled promotion, and Suggestion: skip 2. The checkout eligibility gate doesn't catch price-driven ineligibility The gate ( Repro on default config ( (The swap in The existing gate test passes because it raises the method's Suggestion: invert the order — recalculate first, then check eligibility, then refuse. The PR body defends the current order with "a refusal never persists a price change", but a recalculated price is the correct price; persisting it before refusing is harmless. Checking after the recalc is the only way the gate sees the totals the eligibility checkers are judged against. Two smaller scope questions, non-blocking:
|
|
Failing tests for the two issues above. Written as the correct expectation, so they go green once fixed rather than needing a rewrite. Verified against this branch — both fail for the diagnosed reason, and the 5 existing tests in the suite still pass: Worth noting the first failure isolates the bug cleanly: the intermediate assertions pass (the promo applies, and after disabling it Append to const getActiveOrderShippingDocument = gql`
query GetActiveOrderShipping {
activeOrder {
id
shippingWithTax
shippingLines {
shippingMethod {
code
}
}
discounts {
description
}
promotions {
name
}
}
}
`;
const flatRateCalculator = {
code: defaultShippingCalculator.code,
arguments: [
{ name: 'rate', value: '500' },
{ name: 'taxRate', value: '0' },
{ name: 'includesTax', value: 'auto' },
],
};
const shippingAddressInput = {
fullName: 'Test Customer',
streetLine1: '1 Test Street',
city: 'Test City',
province: 'Test',
postalCode: '12345',
countryCode: 'US',
phoneNumber: '555-0100',
};
// #3510 — read-time recalc skips applyShippingPromotions, so shipping adjustments are never cleared
describe('OrderRecalculationStrategy — shipping promotions on read', () => {
const { server, shopClient, adminClient } = createTestEnvironment(
mergeConfig(testConfig(), {
orderOptions: {
orderRecalculationStrategy: new TtlOrderRecalculationStrategy({ ttlMs: 0 }),
},
shippingOptions: {
shippingEligibilityCheckers: [defaultShippingEligibilityChecker],
shippingCalculators: [defaultShippingCalculator],
},
}),
);
beforeAll(async () => {
await server.init({
initialData,
productsCsvPath: path.join(__dirname, 'fixtures/e2e-products-full.csv'),
customerCount: 1,
});
await adminClient.asSuperAdmin();
}, TEST_SETUP_TIMEOUT_MS);
afterAll(async () => {
await server.destroy();
});
// #3510 — a disabled free-shipping promotion must not keep discounting shipping on read
it('clears shipping discounts on read after a shipping promotion is disabled', async () => {
await shopClient.asAnonymousUser();
const { createShippingMethod } = await adminClient.query(createShippingMethodDocument, {
input: {
code: 'flat-rate-shipping',
translations: [{ languageCode: LanguageCode.en, name: 'Flat rate', description: '' }],
fulfillmentHandler: manualFulfillmentHandler.code,
checker: {
code: defaultShippingEligibilityChecker.code,
arguments: [{ name: 'orderMinimum', value: '0' }],
},
calculator: flatRateCalculator,
},
});
const shippingMethodId = (createShippingMethod as any).id;
const { createPromotion } = await adminClient.query(createPromotionDocument, {
input: {
enabled: true,
translations: [{ languageCode: LanguageCode.en, name: 'Free shipping' }],
conditions: [
{
code: minimumOrderAmount.code,
arguments: [
{ name: 'amount', value: '0' },
{ name: 'taxInclusive', value: 'true' },
],
},
],
actions: [{ code: freeShipping.code, arguments: [] }],
},
});
const promotionId = (createPromotion as any).id;
await shopClient.query(addItemToOrderDocument, { productVariantId: 'T_1', quantity: 1 });
await shopClient.query(setShippingAddressDocument, { input: shippingAddressInput });
await shopClient.query(setShippingMethodDocument, { id: [shippingMethodId] });
const before = await shopClient.query(getActiveOrderShippingDocument);
expect(before.activeOrder.shippingWithTax).toBe(0);
expect(before.activeOrder.promotions.map((p: any) => p.name)).toEqual(['Free shipping']);
// Admin disables the free-shipping promotion.
await adminClient.query(updatePromotionDocument, {
input: { id: promotionId, enabled: false },
});
// Reading the active order with TTL(0) recalculates — the shipping discount must be gone.
const after = await shopClient.query(getActiveOrderShippingDocument);
expect(after.activeOrder.promotions).toEqual([]);
expect(after.activeOrder.discounts).toEqual([]);
expect(after.activeOrder.shippingWithTax).toBe(500);
await adminClient.query(deletePromotionDocument, { id: promotionId });
});
});
// #3510 — the checkout gate must catch a method made ineligible by the checkout recalculation itself
describe('OrderRecalculationStrategy — checkout gate with default strategy', () => {
const { server, shopClient, adminClient } = createTestEnvironment(
mergeConfig(testConfig(), {
orderOptions: {
orderRecalculationStrategy: new NoOrderRecalculationStrategy(),
},
shippingOptions: {
shippingEligibilityCheckers: [defaultShippingEligibilityChecker],
shippingCalculators: [defaultShippingCalculator],
},
}),
);
const transitionGuard: ErrorResultGuard<{ state: string }> = createErrorResultGuard(
(input: any) => !!input.state,
);
beforeAll(async () => {
await server.init({
initialData,
productsCsvPath: path.join(__dirname, 'fixtures/e2e-products-full.csv'),
customerCount: 1,
});
await adminClient.asSuperAdmin();
}, TEST_SETUP_TIMEOUT_MS);
afterAll(async () => {
await server.destroy();
});
// #3510 — a price drop that makes the chosen method ineligible must refuse, not silently swap
it('refuses ArrangingPayment when the checkout recalculation makes the method ineligible', async () => {
await shopClient.asAnonymousUser();
// Only eligible while the order total stays above 100000.
const { createShippingMethod } = await adminClient.query(createShippingMethodDocument, {
input: {
code: 'premium-shipping',
translations: [{ languageCode: LanguageCode.en, name: 'Premium', description: '' }],
fulfillmentHandler: manualFulfillmentHandler.code,
checker: {
code: defaultShippingEligibilityChecker.code,
arguments: [{ name: 'orderMinimum', value: '100000' }],
},
calculator: flatRateCalculator,
},
});
const shippingMethodId = (createShippingMethod as any).id;
await shopClient.query(addItemToOrderDocument, { productVariantId: 'T_1', quantity: 1 });
await shopClient.query(setCustomerDocument, {
input: { firstName: 'Test', lastName: 'User', emailAddress: 'swap@test.com' },
});
await shopClient.query(setShippingAddressDocument, { input: shippingAddressInput });
await shopClient.query(setShippingMethodDocument, { id: [shippingMethodId] });
// Admin slashes the price. With the default strategy there is no read-time recalc, so the
// persisted Order still holds the stale (high) totals when the checkout gate runs.
await adminClient.query(updateProductVariantsDocument, { input: [{ id: 'T_1', price: 100 }] });
const { transitionOrderToState } = await shopClient.query(transitionToStateDocument, {
state: 'ArrangingPayment',
});
transitionGuard.assertErrorResult(transitionOrderToState);
expect((transitionOrderToState as any).errorCode).toBe(ErrorCode.ORDER_STATE_TRANSITION_ERROR);
expect((transitionOrderToState as any).transitionError).toContain('no longer eligible');
// The customer's chosen method must never be changed behind their back.
const { activeOrder } = await shopClient.query(getActiveOrderShippingDocument);
expect(activeOrder.shippingLines[0].shippingMethod.code).toBe('premium-shipping');
});
}); |
…ibility after recalc
|
Great catch on both @biggamesmallworld — these are real, and your repros + failing tests made them trivial to confirm. Both are now fixed (pushed) and your two tests are in and green. 1. Stale shipping discounts on read You were right: 2. Checkout gate missing price-driven ineligibility Also right, and thanks for flagging it hits the default Verification: your 2 tests + the 5 existing = 7/7 green, and the On your two scope questions:
Thanks again — this materially improved the PR. |
|
Follow-up on your two scope questions — both handled now:
Both commits pushed. Full |
… fix MySQL staleness
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/core/e2e/order-recalculation.e2e-spec.ts (1)
1-629: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
Draft → ArrangingPaymentrecalculation.The PR objectives and code at Line 321 (
if (toState === 'ArrangingPayment')) explicitly support transitions from bothAddingItemsandDraft, but all transition tests only exerciseAddingItems → ArrangingPayment. A Draft order transitioning to payment is a distinct code path (e.g.,order.activeis set totrueinonTransitionEndat Line 475) and should have coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/e2e/order-recalculation.e2e-spec.ts` around lines 1 - 629, Missing coverage for Draft → ArrangingPayment recalculation. Add an e2e test in the checkout transition suites that creates or retrieves a Draft order, changes its variant price, transitions it to ArrangingPayment, and asserts the transition succeeds with recalculated totals; also verify the order becomes active if exposed by the test API. Reuse the existing transitionToStateDocument, updateProductVariantsDocument, and price assertions used by the AddingItems checkout tests.packages/core/src/service/helpers/order-calculator/order-calculator.ts (1)
32-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor doc gap:
recalculateShippinghas no JSDoc, unlike its sibling.
recalculateShippingPromotionsis thoroughly documented butrecalculateShippinghas none, despite being the property whose semantics the sibling's doc depends on.✏️ Suggested doc addition
export interface ApplyPriceAdjustmentsOptions { + /** + * `@description` + * Whether the shipping method/rate should be re-evaluated (and potentially swapped to a + * cheaper eligible alternative). Defaults to `true`. + */ recalculateShipping?: boolean;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/service/helpers/order-calculator/order-calculator.ts` around lines 32 - 44, Add JSDoc for the recalculateShipping property in ApplyPriceAdjustmentsOptions, describing what enabling or disabling shipping recalculation does and clarifying its default behavior, so recalculateShippingPromotions has the necessary context.packages/core/src/service/services/order.service.ts (2)
2429-2477: 🩺 Stability & Availability | 🔵 TrivialRead requests now can acquire a write lock and hold it across the full recalculation.
applyPriceAdjustmentsIfStaleis invoked from the active-order read path, and once staleness is confirmed it acquires apessimistic_writelock on the Order row and holds it for the entire recalculation (promotion re-tests, tax calc, shipping-promotion re-test, and the final save) before the transaction commits. This differs from the existing lock usages elsewhere in this file (revalidateCouponCodesForOrder,mergeOrders), which are only reachable from write/checkout mutations — this is the first time a plain read can escalate into a blocking write-lock hold. Under a configuredTtlOrderRecalculationStrategywith meaningful traffic, a concurrentaddItemToOrder/adjustOrderLineson the same order (which alsoUPDATEs the Order row without pre-acquiring a lock) could block behind this recalculation, or hit a lock-wait timeout under contention. This is opt-in (defaultNoOrderRecalculationStrategyis a no-op) and the double-checked staleness re-check does bound the number of orders that actually pay this cost, but it's worth calling out as an operational characteristic for anyone enabling TTL-based recalculation on high-traffic carts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/service/services/order.service.ts` around lines 2429 - 2477, Revise applyPriceAdjustmentsIfStale so the active-order read path does not acquire and hold a pessimistic write lock across recalculation and save. Remove the transaction-scoped write-locking flow, or otherwise move recalculation to an approach that avoids blocking concurrent addItemToOrder and adjustOrderLines mutations while preserving stale-order checks and safe persistence.
2443-2468: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffThree sequential queries where one combined fetch might suffice.
The method issues a lock-only query (
createQueryBuilder(...).setLock(...).getOne()), then a second full-relations query viagetOrderOrThrowonce staleness is confirmed. This is a minor inefficiency (extra round-trip) rather than a correctness issue, and combining them would require reworkinggetOrderOrThrow's two-step (order + separate lines query) loading strategy to support locking, which is nontrivial for the incremental benefit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/service/services/order.service.ts` around lines 2443 - 2468, Consider reducing the sequential database round trips in the recalculation flow around the lock acquisition and getOrderOrThrow calls. Where feasible, combine the lock and required relation loading into one fetch while preserving the double-checked staleness validation; otherwise retain the current behavior and avoid a broad refactor of getOrderOrThrow’s separate order and lines loading strategy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/e2e/order-recalculation.e2e-spec.ts`:
- Around line 1-629: Missing coverage for Draft → ArrangingPayment
recalculation. Add an e2e test in the checkout transition suites that creates or
retrieves a Draft order, changes its variant price, transitions it to
ArrangingPayment, and asserts the transition succeeds with recalculated totals;
also verify the order becomes active if exposed by the test API. Reuse the
existing transitionToStateDocument, updateProductVariantsDocument, and price
assertions used by the AddingItems checkout tests.
In `@packages/core/src/service/helpers/order-calculator/order-calculator.ts`:
- Around line 32-44: Add JSDoc for the recalculateShipping property in
ApplyPriceAdjustmentsOptions, describing what enabling or disabling shipping
recalculation does and clarifying its default behavior, so
recalculateShippingPromotions has the necessary context.
In `@packages/core/src/service/services/order.service.ts`:
- Around line 2429-2477: Revise applyPriceAdjustmentsIfStale so the active-order
read path does not acquire and hold a pessimistic write lock across
recalculation and save. Remove the transaction-scoped write-locking flow, or
otherwise move recalculation to an approach that avoids blocking concurrent
addItemToOrder and adjustOrderLines mutations while preserving stale-order
checks and safe persistence.
- Around line 2443-2468: Consider reducing the sequential database round trips
in the recalculation flow around the lock acquisition and getOrderOrThrow calls.
Where feasible, combine the lock and required relation loading into one fetch
while preserving the double-checked staleness validation; otherwise retain the
current behavior and avoid a broad refactor of getOrderOrThrow’s separate order
and lines loading strategy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33c7f995-8250-461c-bbd4-654939a8bc2b
📒 Files selected for processing (15)
packages/core/e2e/order-recalculation.e2e-spec.tspackages/core/src/config/config.module.tspackages/core/src/config/default-config.tspackages/core/src/config/index.tspackages/core/src/config/order/default-order-process.tspackages/core/src/config/order/no-order-recalculation-strategy.tspackages/core/src/config/order/order-recalculation-strategy.tspackages/core/src/config/order/ttl-order-recalculation-strategy.spec.tspackages/core/src/config/order/ttl-order-recalculation-strategy.tspackages/core/src/config/vendure-config.tspackages/core/src/entity/order/order.entity.tspackages/core/src/i18n/messages/en.jsonpackages/core/src/service/helpers/active-order/active-order.service.tspackages/core/src/service/helpers/order-calculator/order-calculator.tspackages/core/src/service/services/order.service.ts
biggamesmallworld
left a comment
biggamesmallworld
left a comment
There was a problem hiding this comment.
Both fixes look right — the recalculateShippingPromotions split and the inverted checkout gate do exactly what we discussed, and the two tests are in and green. Also confirmed the gate's recalc persists before getEligibleShippingMethods re-reads the order, so eligibility really is judged against the fresh totals.
One leftover: two JSDoc blocks predate the shipping-promotion fix and now state the opposite of what the code does. Both are published to the docs site, and one sits on the public strategy interface — a reader would conclude the stale-shipping-discount bug was never fixed. Committable suggestions below, plus the recalculateShipping doc gap CodeRabbit flagged.
| * This strategy determines whether an active Order's prices, promotions and taxes should be | ||
| * re-calculated when the Order is read (e.g. via the `activeOrder` query). Recalculation is only | ||
| * ever attempted for Orders in the `AddingItems` state. | ||
| * | ||
| * **Note:** read-time recalculation refreshes item/order prices, promotions and taxes only. | ||
| * Shipping method eligibility, rates and shipping promotions are NOT re-evaluated on read — they | ||
| * are re-evaluated when the order transitions to `ArrangingPayment` (checkout). |
There was a problem hiding this comment.
This is now inverted by the shipping-promotion fix in b08d211: read-time recalc does re-test shipping promotions (recalculateShippingPromotions: true), only the method & rate are frozen. As written, the public strategy docs tell integrators the disabled-free-shipping bug is still there.
| * This strategy determines whether an active Order's prices, promotions and taxes should be | |
| * re-calculated when the Order is read (e.g. via the `activeOrder` query). Recalculation is only | |
| * ever attempted for Orders in the `AddingItems` state. | |
| * | |
| * **Note:** read-time recalculation refreshes item/order prices, promotions and taxes only. | |
| * Shipping method eligibility, rates and shipping promotions are NOT re-evaluated on read — they | |
| * are re-evaluated when the order transitions to `ArrangingPayment` (checkout). | |
| * This strategy determines whether an active Order's prices, promotions, taxes and shipping | |
| * promotions should be re-calculated when the Order is read (e.g. via the `activeOrder` query). | |
| * Recalculation is only ever attempted for Orders in the `AddingItems` state. | |
| * | |
| * **Note:** the shipping *method* and *rate* are NOT re-evaluated on read, so a read never silently | |
| * swaps the customer's chosen method — those are re-evaluated when the Order transitions to | |
| * `ArrangingPayment` (checkout). Shipping *promotions* are re-tested, so a now-inactive shipping | |
| * promotion's discount is cleared rather than surviving on the Order. |
| * Recalculates the given active Order's prices, promotions and taxes if the configured | ||
| * {@link OrderRecalculationStrategy} reports it as stale. Shipping (method eligibility, rates and | ||
| * shipping promotions) is deliberately NOT re-evaluated on this read path — that happens when the | ||
| * Order transitions to `ArrangingPayment`. Only Orders in the `AddingItems` state are eligible. | ||
| * No-ops (returning the Order unchanged) otherwise. Invoked on the active-order read path. |
There was a problem hiding this comment.
Same stale claim, and it directly contradicts the inline comment eight lines below ("still re-test shipping promotions so a disabled promotion's discount is cleared").
| * Recalculates the given active Order's prices, promotions and taxes if the configured | |
| * {@link OrderRecalculationStrategy} reports it as stale. Shipping (method eligibility, rates and | |
| * shipping promotions) is deliberately NOT re-evaluated on this read path — that happens when the | |
| * Order transitions to `ArrangingPayment`. Only Orders in the `AddingItems` state are eligible. | |
| * No-ops (returning the Order unchanged) otherwise. Invoked on the active-order read path. | |
| * Recalculates the given active Order's prices, promotions, taxes and shipping promotions if the | |
| * configured {@link OrderRecalculationStrategy} reports it as stale. The shipping *method* and | |
| * *rate* are deliberately NOT re-evaluated on this read path — the customer's chosen method is | |
| * never silently swapped; those are re-evaluated when the Order transitions to `ArrangingPayment`. | |
| * Only Orders in the `AddingItems` state are eligible; no-ops (returning the Order unchanged) | |
| * otherwise. Invoked on the active-order read path. |
| * @since 3.8.0 | ||
| */ | ||
| export interface ApplyPriceAdjustmentsOptions { | ||
| recalculateShipping?: boolean; |
There was a problem hiding this comment.
Non-blocking (CodeRabbit flagged this too): recalculateShippingPromotions has a thorough doc block whose meaning depends on this flag, but this one has none.
| recalculateShipping?: boolean; | |
| /** | |
| * @description | |
| * Whether the shipping method & rate should be re-evaluated — which may swap the Order's | |
| * shipping method to a different eligible one. Defaults to `true`. | |
| */ | |
| recalculateShipping?: boolean; |
biggamesmallworld
left a comment
biggamesmallworld
left a comment
There was a problem hiding this comment.
Approved but still have JSDoc suggestions out for approval
|
Hope to be included in the next release @michaelbromley |
…t-does-not-update-discounts-shipping-pricing-or
3 participants
Fixes #3510
Problem
Order pricing in core is purely mutation-driven:
OrderService.applyPriceAdjustmentsre-tests promotions, shipping eligibility and prices, but only on write mutations. A cart left untouched since an underlying price / promotion / shipping change displays — and can check out with — stale totals, invalid discounts and ineligible shipping.Changes
Read-time recalculation (opt-in, backward compatible)
OrderRecalculationStrategyonOrderOptions.orderRecalculationStrategy, defaulting toNoOrderRecalculationStrategy— no behavior change unless opted in.TtlOrderRecalculationStrategy({ ttlMs })("price-freeze period" model).OrderService.applyPriceAdjustmentsIfStale, invoked fromActiveOrderService.getActiveOrder, recalculates an activeAddingItemsorder when the strategy reports it stale. Runs inside a transaction with a pessimistic row lock and double-checked staleness, so the read path never half-commits and concurrent reads don't stampede.Order.pricingUpdatedAtcolumn tracks the last recalculation (stamped insideapplyPriceAdjustments). Consumers should regenerate a migration for the new column.Checkout eligibility gate (always on)
ArrangingPayment(fromAddingItemsorDraft), the default order process first recalculates prices, promotions and shipping promotions against current data — without re-selecting the shipping method — then verifies the chosen method is still eligible against those fresh totals. If it is not, the transition is refused viaOrderStateTransitionError(message keycannot-transition-to-payment-with-ineligible-shipping-method); persisting the corrected price before refusing is harmless. This pre-payment recalculation runs for every checkout, including shipping-less orders, so payment always uses fresh prices. When the method is still eligible, a full recalculation (including the shipping rate) is applied before payment.Testing
order-recalculation.e2e-spec.ts: read-time recalc (variant price change, promotion removal), backward-compatibility (default no-op does not recalculate on read), and the checkout gate (refuses ineligible shipping withTtlOrderRecalculationStrategyactive; happy-path recalc + transition). Also: read-time clearing of a disabled shipping promotion's discount, the checkout gate catching a price-driven ineligibility on the defaultNoOrderRecalculationStrategy, and pre-payment recalc for a shipping-less order.TtlOrderRecalculationStrategy(null / within-TTL / past-TTL / exact boundary).order,shop-orderandorder-changed-price-handlinge2e suites pass.Relates to #1738 (out-of-stock oversell) — a separate stock TOCTOU concern, tracked independently and out of scope here.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Tests