feat(core): Add configurable OrderTaxSummaryCalculationStrategy#4376
Conversation
Introduces a strategy pattern for how order-level tax totals and tax summaries are calculated, allowing different rounding approaches. The default strategy (DefaultOrderTaxSummaryCalculationStrategy) rounds tax per line then sums, preserving existing Vendure behavior. The new OrderLevelTaxSummaryCalculationStrategy groups net subtotals by tax rate and rounds once per group, eliminating per-line rounding accumulation. This is required by certain jurisdictions and ERP systems that expect tax calculated on the subtotal per rate. Configurable via taxOptions.orderTaxSummaryCalculationStrategy. Relates to vendurehq#4375
|
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:
📝 WalkthroughWalkthroughThis pull request adds a configurable tax summary calculation strategy system. It defines the 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/core/src/config/vendure-config.ts (1)
1034-1034: Unrelated@sincetag changes for SchedulerOptions.The
@sincetags forSchedulerOptionsandVendureConfig.schedulerOptionswere updated (per the AI summary, from3.3.0to3.6.0). This appears to be an unrelated correction bundled into this PR. Consider splitting this into a separate commit or confirming this is intentional.Also applies to: 1338-1338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/config/vendure-config.ts` at line 1034, The change to the `@since` tag for SchedulerOptions and VendureConfig.schedulerOptions looks unrelated to this PR; revert the `@since` tags back to their original value (e.g., 3.3.0) or move that doc-only change into a separate commit. Locate the JSDoc for SchedulerOptions and the VendureConfig.schedulerOptions entry in vendure-config.ts (symbols: SchedulerOptions, VendureConfig.schedulerOptions) and either restore the previous `@since` value or stage a separate commit with the updated `@since` to keep the functional PR diffs focused.packages/core/src/config/tax/order-level-tax-summary-calculation-strategy.ts (1)
99-106: Verify:order.surchargescould beundefinedif the relation is not loaded.
order.linesandorder.surchargesare iterated without null guards (unlikeorder.shippingLines ?? []on line 107). The strategy interface contract states relations must be loaded, but defensiveness here would be consistent with howshippingLinesis handled.Add null guards for consistency
- for (const line of order.lines) { + for (const line of order.lines ?? []) { subTotal += line.proratedLinePrice; this.accumulateIntoGroups(subTotalGroups, line.taxLines, line.proratedLinePrice); } - for (const surcharge of order.surcharges) { + for (const surcharge of order.surcharges ?? []) { subTotal += surcharge.price; this.accumulateIntoGroups(subTotalGroups, surcharge.taxLines, surcharge.price); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/config/tax/order-level-tax-summary-calculation-strategy.ts` around lines 99 - 106, The loop over order.lines and order.surcharges should guard against undefined relations like shippingLines does; update the code around accumulateIntoGroups to iterate over order.lines ?? [] and order.surcharges ?? [] (or check for existence before looping) so you won't access properties when the relations aren't loaded—specifically modify the blocks that use order.lines and order.surcharges and keep using accumulateIntoGroups(subTotalGroups, ..., ...) and the same price fields (line.proratedLinePrice, surcharge.price) unchanged.packages/core/src/service/helpers/order-calculator/order-calculator.spec.ts (1)
1688-1691: Tax consistency assertion is overly indirect for a no-shipping test.Since this test has no shipping lines,
order.shippingWithTax - order.shippingis always 0, making the assertion equivalent tosubTotalWithTax - subTotal === taxTotal. Consider simplifying for clarity:✏️ Simplified assertion
- const taxTotal = order.taxSummary.reduce((sum, s) => sum + s.taxTotal, 0); - expect(order.subTotalWithTax - order.subTotal).toBe( - taxTotal - (order.shippingWithTax - order.shipping), - ); + const taxTotal = order.taxSummary.reduce((sum, s) => sum + s.taxTotal, 0); + expect(order.subTotalWithTax - order.subTotal).toBe(taxTotal);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/helpers/order-calculator/order-calculator.spec.ts` around lines 1688 - 1691, The test's tax consistency assertion uses an unnecessary shipping adjustment even though there are no shipping lines; replace the indirect check using order.shippingWithTax - order.shipping with a direct comparison so that order.subTotalWithTax - order.subTotal equals the computed taxTotal (or assert order.subTotalWithTax === order.subTotal + taxTotal). Update the expect call that currently references order.shippingWithTax and order.shipping to the simplified assertion using taxTotal and order.subTotalWithTax/order.subTotal, leaving order.taxSummary and taxTotal calculation unchanged.
🤖 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/config/tax/default-order-tax-summary-calculation-strategy.ts`:
- Around line 24-44: calculateOrderTotals is iterating order.lines and
order.surcharges directly while calculateTaxSummary and shippingLines use
defensive guards; make iteration null-safe and consistent by using the same
guards (e.g. iterate over order.lines ?? [] and order.surcharges ?? [] and keep
order.shippingLines ?? []) so unloaded relations won’t throw; apply the same
change to OrderLevelTaxSummaryCalculationStrategy.groupOrder (ensure any loop
over order.lines/order.surcharges uses the ?? [] guard).
In
`@packages/core/src/config/tax/order-level-tax-summary-calculation-strategy.ts`:
- Around line 46-91: calculateTaxSummary currently merges subTotalGroups and
shippingGroups then computes taxTotal by rounding taxPayableOn on the merged
netBase, which can differ from the separate rounding used in
calculateOrderTotals; update calculateTaxSummary so the merged map accumulates
both netBase and the already-rounded per-group tax (use
Math.round(taxPayableOn(group.netBase, group.rate)) when merging each [key,
group]) and when emitting OrderTaxSummary use the accumulated taxTotal instead
of recomputing/rounding from the merged netBase; reference the
calculateTaxSummary function and the merged Map plus TaxGroup entries to locate
where to change accumulation and output.
In `@packages/core/src/config/tax/order-tax-summary-calculation-strategy.spec.ts`:
- Around line 240-268: Add a new test to cover the case where items and shipping
share the same tax rate to surface the inconsistency between
calculateOrderTotals and calculateTaxSummary: create a request context
(createRequestContext), an order with a line using taxCategoryStandard and a
ShippingLine whose taxLines use the same rate/description (e.g. 21% "VAT"), then
call strategy.calculateOrderTotals(order) and
strategy.calculateTaxSummary(order) and assert that the summed taxTotal from the
taxSummary equals the actual tax computed from totals ((subTotalWithTax -
subTotal) + (shippingWithTax - shipping)); reference the existing spec helpers
(createOrder, ShippingLine) and the methods calculateOrderTotals and
calculateTaxSummary to locate where to add this test.
---
Nitpick comments:
In
`@packages/core/src/config/tax/order-level-tax-summary-calculation-strategy.ts`:
- Around line 99-106: The loop over order.lines and order.surcharges should
guard against undefined relations like shippingLines does; update the code
around accumulateIntoGroups to iterate over order.lines ?? [] and
order.surcharges ?? [] (or check for existence before looping) so you won't
access properties when the relations aren't loaded—specifically modify the
blocks that use order.lines and order.surcharges and keep using
accumulateIntoGroups(subTotalGroups, ..., ...) and the same price fields
(line.proratedLinePrice, surcharge.price) unchanged.
In `@packages/core/src/config/vendure-config.ts`:
- Line 1034: The change to the `@since` tag for SchedulerOptions and
VendureConfig.schedulerOptions looks unrelated to this PR; revert the `@since`
tags back to their original value (e.g., 3.3.0) or move that doc-only change
into a separate commit. Locate the JSDoc for SchedulerOptions and the
VendureConfig.schedulerOptions entry in vendure-config.ts (symbols:
SchedulerOptions, VendureConfig.schedulerOptions) and either restore the
previous `@since` value or stage a separate commit with the updated `@since` to keep
the functional PR diffs focused.
In `@packages/core/src/service/helpers/order-calculator/order-calculator.spec.ts`:
- Around line 1688-1691: The test's tax consistency assertion uses an
unnecessary shipping adjustment even though there are no shipping lines; replace
the indirect check using order.shippingWithTax - order.shipping with a direct
comparison so that order.subTotalWithTax - order.subTotal equals the computed
taxTotal (or assert order.subTotalWithTax === order.subTotal + taxTotal). Update
the expect call that currently references order.shippingWithTax and
order.shipping to the simplified assertion using taxTotal and
order.subTotalWithTax/order.subTotal, leaving order.taxSummary and taxTotal
calculation unchanged.
packages/core/src/config/tax/order-level-tax-summary-calculation-strategy.ts
Show resolved
Hide resolved
packages/core/src/config/tax/order-tax-summary-calculation-strategy.spec.ts
Show resolved
Hide resolved
…maryCalculationStrategy
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/core/src/config/tax/order-tax-summary-calculation-strategy.spec.ts (2)
38-67: MissingsubTotalWithTaxassertion.The test verifies
subTotalandtaxSummarybut omitssubTotalWithTax. For completeness, the expected value is600 + 30 + 400 + 200 + 80 = 1310.💡 Suggested addition
expect(totals.subTotal).toBe(600 + 400); +expect(totals.subTotalWithTax).toBe(1310); // 600+30 (5%) + 400+200 (50%) + 400*0.20 (20%) expect(taxSummary).toEqual([🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/config/tax/order-tax-summary-calculation-strategy.spec.ts` around lines 38 - 67, Add an assertion verifying totals.subTotalWithTax in the "includes surcharges with multiple tax lines in totals and summary" test: after computing totals via strategy.calculateOrderTotals(order), assert totals.subTotalWithTax === 1310 (600 + 30 + 400 + 200 + 80) so the test checks subtotal including taxes as well.
91-96:expect.arrayContainingwithout a length guard.Used in three places (Lines 91–96, 229–235, 262–267): an extra unexpected entry in the summary would silently pass. Consider adding a
.toHaveLength(n)assertion alongside eachexpect.arrayContainingcheck.💡 Example (Line 91 block)
+expect(taxSummary).toHaveLength(2); expect(taxSummary).toEqual( expect.arrayContaining([ { description: 'tax a', taxRate: 5, taxBase: 600, taxTotal: 30 }, { description: 'shipping tax', taxRate: 20, taxBase: 500, taxTotal: 100 }, ]), );Also applies to: 229-235, 262-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/config/tax/order-tax-summary-calculation-strategy.spec.ts` around lines 91 - 96, The tests use expect(taxSummary).toEqual(expect.arrayContaining([...])) in multiple places (the assertions that check taxSummary entries using arrayContaining) which can pass if extra entries are present; update each occurrence to also assert the exact length by adding a .toHaveLength(2) (or the correct expected count) on taxSummary immediately after the arrayContaining check so the summary must contain those entries and no extras—apply this change to the three assertions that reference taxSummary in this spec file.packages/core/src/service/helpers/order-calculator/order-calculator.spec.ts (1)
1671-1691: The test doesn't actually exercise the distinguishing rounding behavior.With
taxCategoryStandardat 20%, the inputs102and215happen to produce identical results under both strategies:
- Per-line:
round(102 × 0.20) + round(215 × 0.20) = 20 + 43 = 63- Order-level:
round(317 × 0.20) = round(63.4) = 63So this test (and likewise Tests 2 and 3 in this describe, each a single-group scenario) would pass even if
DefaultOrderTaxSummaryCalculationStrategywere still wired. The consistency assertion at Lines 1688–1689 is good but only checks internal coherence, not the rounding mode.Because
taxCategoryStandardis hardcoded to 20% in the mock, it's not straightforward to surface a difference here. A comment clarifying this limitation, or a note pointing to the dedicated spec file (order-tax-summary-calculation-strategy.spec.ts, Lines 119–152) where 21% does demonstrate the split, would prevent future confusion. The suite still has value as a regression/integration smoke-check, but the description is misleading.💡 Suggested clarification comment
it('calculates order-level totals using grouped rounding', async () => { + // Note: with the 20% mock tax rate, per-line and order-level rounding + // coincidentally produce the same result for these inputs. The rounding + // difference is verified in order-tax-summary-calculation-strategy.spec.ts. const ctx = createRequestContext({ pricesIncludeTax: false });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/helpers/order-calculator/order-calculator.spec.ts` around lines 1671 - 1691, The test "calculates order-level totals using grouped rounding" currently cannot distinguish grouped-vs-per-line rounding because taxCategoryStandard is 20% and the chosen prices yield identical totals; update the spec to add a clarifying comment above this test that notes the limitation (that with taxCategoryStandard at 20% these inputs produce the same result for both strategies) and point readers to the dedicated test that demonstrates the difference (order-tax-summary-calculation-strategy.spec.ts where a 21% rate is used). Mention the relevant symbols in the comment (taxCategoryStandard, DefaultOrderTaxSummaryCalculationStrategy, and the test name) so reviewers know why this test remains a regression/integration smoke-check rather than a rounding-mode verifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/config/tax/order-tax-summary-calculation-strategy.spec.ts`:
- Around line 38-67: Add an assertion verifying totals.subTotalWithTax in the
"includes surcharges with multiple tax lines in totals and summary" test: after
computing totals via strategy.calculateOrderTotals(order), assert
totals.subTotalWithTax === 1310 (600 + 30 + 400 + 200 + 80) so the test checks
subtotal including taxes as well.
- Around line 91-96: The tests use
expect(taxSummary).toEqual(expect.arrayContaining([...])) in multiple places
(the assertions that check taxSummary entries using arrayContaining) which can
pass if extra entries are present; update each occurrence to also assert the
exact length by adding a .toHaveLength(2) (or the correct expected count) on
taxSummary immediately after the arrayContaining check so the summary must
contain those entries and no extras—apply this change to the three assertions
that reference taxSummary in this spec file.
In `@packages/core/src/service/helpers/order-calculator/order-calculator.spec.ts`:
- Around line 1671-1691: The test "calculates order-level totals using grouped
rounding" currently cannot distinguish grouped-vs-per-line rounding because
taxCategoryStandard is 20% and the chosen prices yield identical totals; update
the spec to add a clarifying comment above this test that notes the limitation
(that with taxCategoryStandard at 20% these inputs produce the same result for
both strategies) and point readers to the dedicated test that demonstrates the
difference (order-tax-summary-calculation-strategy.spec.ts where a 21% rate is
used). Mention the relevant symbols in the comment (taxCategoryStandard,
DefaultOrderTaxSummaryCalculationStrategy, and the test name) so reviewers know
why this test remains a regression/integration smoke-check rather than a
rounding-mode verifier.
colinpieper
changed the title
michaelbromley
left a comment
michaelbromley
left a comment
There was a problem hiding this comment.
Great PR, thank you!
The failing e2e tests are something unrelated I need to investigate.
Let me know what you think about the naming or splitting into 2 strategies.
packages/core/src/config/tax/order-tax-summary-calculation-strategy.ts
Outdated
Show resolved
Hide resolved
2 participants
Description
Introduces a strategy pattern for how order-level tax totals and tax summaries are calculated, allowing different rounding approaches.
The default strategy (DefaultOrderTaxSummaryCalculationStrategy) rounds tax per line then sums, preserving existing Vendure behavior.
The new OrderLevelTaxSummaryCalculationStrategy groups net subtotals by tax rate and rounds once per group, eliminating per-line rounding accumulation. This is required by certain jurisdictions and ERP systems that expect tax calculated on the subtotal per rate.
Configurable via taxOptions.orderTaxSummaryCalculationStrategy.
Relates to #4375
Breaking changes
No breaking changes. Defaults to the existing behavior.
Checklist
📌 Always:
👍 Most of the time:
--
This is my first feature contribution to Vendure, so please let me know if I missed anything. Feedback is welcome.