fix(core): Update DefaultMoneyStrategy.round() Logic (#3023)

BREAKING CHANGE: A technically breaking change in this release is that we have corrected the default rounding logic: ```ts // v3.0 return Math.round(value) * quantity; // v3.1 return Math.round(value * quantity); ``` This makes order totals calculations much more "correct" as per most people's expectations, but it pointed out as a technically breaking change in the unlikely event that you rely on the old, less correct method of rounding.
vendure-ecommerce · Aug 21, 2024 · f43c204 · f43c204
1 parent 60cdae3
commit f43c204
Show file tree

Hide file tree

Showing 6 changed files with 21 additions and 24 deletions.
diff --git a/packages/core/e2e/order-taxes.e2e-spec.ts b/packages/core/e2e/order-taxes.e2e-spec.ts
@@ -15,7 +15,7 @@ import path from 'path';
 import { afterAll, beforeAll, describe, expect, it } from 'vitest';
 
 import { initialData } from '../../../e2e-common/e2e-initial-data';
-import { testConfig, TEST_SETUP_TIMEOUT_MS } from '../../../e2e-common/test-config';
+import { TEST_SETUP_TIMEOUT_MS, testConfig } from '../../../e2e-common/test-config';
 
 import { testSuccessfulPaymentMethod } from './fixtures/test-payment-methods';
 import * as Codegen from './graphql/generated-e2e-admin-types';
@@ -188,10 +188,10 @@ describe('Order taxes', () => {
                 GET_ACTIVE_ORDER_WITH_PRICE_DATA,
             );
             expect(activeOrder?.totalWithTax).toBe(200);
-            expect(activeOrder?.total).toBe(166);
+            expect(activeOrder?.total).toBe(167);
             expect(activeOrder?.lines[0].taxRate).toBe(20);
-            expect(activeOrder?.lines[0].linePrice).toBe(166);
-            expect(activeOrder?.lines[0].lineTax).toBe(34);
+            expect(activeOrder?.lines[0].linePrice).toBe(167);
+            expect(activeOrder?.lines[0].lineTax).toBe(33);
             expect(activeOrder?.lines[0].linePriceWithTax).toBe(200);
             expect(activeOrder?.lines[0].unitPrice).toBe(83);
             expect(activeOrder?.lines[0].unitPriceWithTax).toBe(100);
@@ -270,12 +270,12 @@ describe('Order taxes', () => {
             const { activeOrder } = await shopClient.query<CodegenShop.GetActiveOrderWithPriceDataQuery>(
                 GET_ACTIVE_ORDER_WITH_PRICE_DATA,
             );
-            expect(activeOrder?.totalWithTax).toBe(200);
+            expect(activeOrder?.totalWithTax).toBe(199);
             expect(activeOrder?.total).toBe(166);
             expect(activeOrder?.lines[0].taxRate).toBe(20);
             expect(activeOrder?.lines[0].linePrice).toBe(166);
-            expect(activeOrder?.lines[0].lineTax).toBe(34);
-            expect(activeOrder?.lines[0].linePriceWithTax).toBe(200);
+            expect(activeOrder?.lines[0].lineTax).toBe(33);
+            expect(activeOrder?.lines[0].linePriceWithTax).toBe(199);
             expect(activeOrder?.lines[0].unitPrice).toBe(83);
             expect(activeOrder?.lines[0].unitPriceWithTax).toBe(100);
             expect(activeOrder?.lines[0].taxLines).toEqual([

diff --git a/packages/core/src/config/entity/default-money-strategy.ts b/packages/core/src/config/entity/default-money-strategy.ts
@@ -1,6 +1,5 @@
 import { ColumnOptions } from 'typeorm';
 
-import { Logger } from '../logger/vendure-logger';
 
 import { MoneyStrategy } from './money-strategy';
 
@@ -19,6 +18,6 @@ export class DefaultMoneyStrategy implements MoneyStrategy {
     readonly precision: number = 2;
 
     round(value: number, quantity = 1): number {
-        return Math.round(value) * quantity;
+        return Math.round(value * quantity);
     }
 }
diff --git a/packages/core/src/config/entity/money-strategy.ts b/packages/core/src/config/entity/money-strategy.ts
@@ -88,7 +88,7 @@ export interface MoneyStrategy extends InjectableStrategy {
      * in the {@link DefaultMoneyStrategy} is to round the value, then multiply.
      *
      * ```ts
-     * return Math.round(value) * quantity;
+     * return Math.round(value * quantity);
      * ```
      *
      * However, it may be desirable to instead round only _after_ the unit amount has been

diff --git a/packages/core/src/entity/order/order.entity.spec.ts b/packages/core/src/entity/order/order.entity.spec.ts
@@ -119,7 +119,7 @@ describe('Order entity methods', () => {
                     description: 'tax b',
                     taxRate: 7.5,
                     taxBase: 1600,
-                    taxTotal: 121,
+                    taxTotal: 120,
                 },
             ]);
             assertOrderTaxesAddsUp(order);

diff --git a/packages/core/src/service/helpers/order-calculator/order-calculator.spec.ts b/packages/core/src/service/helpers/order-calculator/order-calculator.spec.ts
@@ -87,7 +87,7 @@ describe('OrderCalculator', () => {
             await orderCalculator.applyPriceAdjustments(ctx, order, []);
 
             expect(order.subTotal).toBe(369);
-            expect(order.subTotalWithTax).toBe(444);
+            expect(order.subTotalWithTax).toBe(443);
         });
 
         it('resets totals when lines array is empty', async () => {
@@ -364,7 +364,7 @@ describe('OrderCalculator', () => {
                     });
                     await orderCalculator.applyPriceAdjustments(ctx, order, [promotion], [order.lines[0]]);
 
-                    expect(order.subTotal).toBe(1173);
+                    expect(order.subTotal).toBe(1174);
                     expect(order.subTotalWithTax).toBe(1350);
                     expect(order.lines[0].adjustments.length).toBe(1);
                     expect(order.lines[0].adjustments[0].description).toBe('50% off each item');
@@ -1107,7 +1107,7 @@ describe('OrderCalculator', () => {
                     ]);
 
                     expect(order.subTotal).toBe(5719);
-                    expect(order.subTotalWithTax).toBe(6448);
+                    expect(order.subTotalWithTax).toBe(6446);
                     assertOrderTotalsAddUp(order);
                 });
 
@@ -1178,8 +1178,8 @@ describe('OrderCalculator', () => {
                     // ```
                     // However, there is always a tradeoff when using integer precision with compounding
                     // fractional multiplication.
-                    expect(order.subTotal).toBe(5079);
-                    expect(order.subTotalWithTax).toBe(5722);
+                    expect(order.subTotal).toBe(5081);
+                    expect(order.subTotalWithTax).toBe(5719);
                 });
             });
         });
@@ -1250,7 +1250,7 @@ describe('OrderCalculator', () => {
 
                 await orderCalculator.applyPriceAdjustments(ctx, order, []);
 
-                expect(order.subTotal).toBe(5087);
+                expect(order.subTotal).toBe(5088);
                 expect(order.subTotalWithTax).toBe(5739);
                 assertOrderTotalsAddUp(order);
             });

diff --git a/packages/payments-plugin/e2e/mollie-payment.e2e-spec.ts b/packages/payments-plugin/e2e/mollie-payment.e2e-spec.ts
@@ -4,10 +4,9 @@ import {
     EventBus,
     LanguageCode,
     mergeConfig,
-    Order,
     OrderPlacedEvent,
     OrderService,
-    RequestContext,
+    RequestContext
 } from '@vendure/core';
 import {
     SettlePaymentMutation,
@@ -26,7 +25,7 @@ import path from 'path';
 import { afterAll, afterEach, beforeAll, describe, expect, it } from 'vitest';
 
 import { initialData } from '../../../e2e-common/e2e-initial-data';
-import { testConfig, TEST_SETUP_TIMEOUT_MS } from '../../../e2e-common/test-config';
+import { TEST_SETUP_TIMEOUT_MS, testConfig } from '../../../e2e-common/test-config';
 import { UPDATE_PRODUCT_VARIANTS } from '../../core/e2e/graphql/shared-definitions';
 import { MolliePlugin } from '../src/mollie';
 import { molliePaymentHandler } from '../src/mollie/mollie.handler';
@@ -48,8 +47,7 @@ import {
 import {
     ADD_ITEM_TO_ORDER,
     APPLY_COUPON_CODE,
-    GET_ACTIVE_ORDER,
-    GET_ORDER_BY_CODE,
+    GET_ORDER_BY_CODE
 } from './graphql/shop-queries';
 import {
     addManualPayment,
@@ -347,7 +345,7 @@ describe('Mollie payments', () => {
             expect(mollieRequest?.webhookUrl).toEqual(
                 `${mockData.host}/payments/mollie/${E2E_DEFAULT_CHANNEL_TOKEN}/1`,
             );
-            expect(mollieRequest?.amount?.value).toBe('1009.90');
+            expect(mollieRequest?.amount?.value).toBe('1009.88');
             expect(mollieRequest?.amount?.currency).toBe('USD');
             expect(mollieRequest.lines[0].vatAmount.value).toEqual('199.98');
             let totalLineAmount = 0;
@@ -442,7 +440,7 @@ describe('Mollie payments', () => {
                     paymentMethodCode: mockData.methodCode,
                 },
             });
-            expect(mollieRequest.amount?.value).toBe('909.90'); // minus 100,00 from manual payment
+            expect(mollieRequest.amount?.value).toBe('909.88'); // minus 100,00 from manual payment
             let totalLineAmount = 0;
             for (const line of mollieRequest?.lines) {
                 totalLineAmount += Number(line.totalAmount.value);