From 4cb44a3a2ec5bcf3d90e3b6a0e1f6bb9ff45e2b6 Mon Sep 17 00:00:00 2001 From: Adrien de Peretti Date: Mon, 13 Feb 2023 17:22:18 +0100 Subject: [PATCH] fix(medusa): Discount allocation precision issues (#3244) --- .changeset/fast-onions-tie.md | 5 + .../__tests__/admin/order-edit/order-edit.js | 6 +- .../api/__tests__/admin/product.js | 22 +-- .../api/__tests__/returns/index.js | 157 ++++++++++++++++++ .../api/__tests__/store/cart/cart.js | 93 ++++++++++- .../api/factories/simple-order-factory.ts | 40 ++++- integration-tests/api/helpers/admin-seeder.js | 1 - packages/medusa/src/services/new-totals.ts | 9 +- packages/medusa/src/services/totals.ts | 9 +- .../medusa/src/strategies/tax-calculation.ts | 4 +- 10 files changed, 306 insertions(+), 40 deletions(-) create mode 100644 .changeset/fast-onions-tie.md diff --git a/.changeset/fast-onions-tie.md b/.changeset/fast-onions-tie.md new file mode 100644 index 0000000000..4d0d09a85a --- /dev/null +++ b/.changeset/fast-onions-tie.md @@ -0,0 +1,5 @@ +--- +"@medusajs/medusa": patch +--- + +fix(medusa): Discount allocation precision issues diff --git a/integration-tests/api/__tests__/admin/order-edit/order-edit.js b/integration-tests/api/__tests__/admin/order-edit/order-edit.js index b66d7c4dd8..c8c3a2c1d1 100644 --- a/integration-tests/api/__tests__/admin/order-edit/order-edit.js +++ b/integration-tests/api/__tests__/admin/order-edit/order-edit.js @@ -2569,15 +2569,13 @@ describe("/admin/order-edits", () => { ]), }), ]), - // rounding issue since we are allocating 1/3 of the discount to one item and 2/3 to the other item where both cost 10 - // resulting in adjustment amounts like: 1333... - discount_total: 2001, - total: 1099, + discount_total: 2000, gift_card_total: 0, gift_card_tax_total: 0, shipping_total: 0, subtotal: 3000, tax_total: 100, + total: 1100, }) ) diff --git a/integration-tests/api/__tests__/admin/product.js b/integration-tests/api/__tests__/admin/product.js index b0e3505002..dd08dc74ec 100644 --- a/integration-tests/api/__tests__/admin/product.js +++ b/integration-tests/api/__tests__/admin/product.js @@ -2210,16 +2210,18 @@ describe("/admin/products", () => { expect(deleteRegionPriceResponse.status).toEqual(200) expect(finalPriceArray).toHaveLength(2) - expect(finalPriceArray).toEqual([ - expect.objectContaining({ - amount: 1000, - currency_code: "usd", - }), - expect.objectContaining({ - amount: 900, - currency_code: "eur", - }), - ]) + expect(finalPriceArray).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + amount: 1000, + currency_code: "usd", + }), + expect.objectContaining({ + amount: 900, + currency_code: "eur", + }), + ]) + ) }) it("successfully updates a variants prices by deleting both a currency and region price", async () => { diff --git a/integration-tests/api/__tests__/returns/index.js b/integration-tests/api/__tests__/returns/index.js index a8d80feac8..02432bc060 100644 --- a/integration-tests/api/__tests__/returns/index.js +++ b/integration-tests/api/__tests__/returns/index.js @@ -10,7 +10,13 @@ const { simpleOrderFactory, simpleProductFactory, simpleShippingOptionFactory, + simpleRegionFactory, } = require("../../factories") +const { + simpleDiscountFactory, +} = require("../../factories/simple-discount-factory") + +jest.setTimeout(30000) describe("/admin/orders", () => { let medusaProcess @@ -236,6 +242,157 @@ describe("/admin/orders", () => { ) }) + test("creates a return w. fixed discount on the total and return the total with the right precision", async () => { + await adminSeeder(dbConnection) + + const variant1Price = 4452600 + const product1 = await simpleProductFactory(dbConnection, { + variants: [ + { + id: "test-variant", + prices: [ + { + amount: variant1Price, + currency: "usd", + variant_id: "test-variant", + }, + ], + }, + ], + }) + + const variant2Price = 482200 + const product2 = await simpleProductFactory(dbConnection, { + variants: [ + { + id: "test-variant-2", + prices: [ + { + amount: variant2Price, + currency: "usd", + variant_id: "test-variant-2", + }, + ], + }, + ], + }) + + const region = await simpleRegionFactory(dbConnection, { + id: "test-region", + tax_rate: 12.5, + }) + + const discountAmount = 10000 + const discount = await simpleDiscountFactory(dbConnection, { + id: "test-discount", + code: "TEST-2", + regions: [region.id], + rule: { + type: "fixed", + value: discountAmount, + allocation: "total", + }, + }) + + const item1Id = "test-item" + const item2Id = "test-item-2" + + const order = await simpleOrderFactory(dbConnection, { + email: "test@testson.com", + region: region.id, + currency_code: "usd", + line_items: [ + { + id: item1Id, + variant_id: product1.variants[0].id, + quantity: 2, + fulfilled_quantity: 2, + shipped_quantity: 2, + unit_price: variant1Price, + adjustments: [ + { + amount: 9023, + discount_code: discount.code, + description: "discount", + item_id: "test-item", + }, + ], + tax_lines: [ + { + name: "default", + code: "default", + rate: 20, + }, + ], + }, + { + id: item2Id, + variant_id: product2.variants[0].id, + quantity: 2, + fulfilled_quantity: 2, + shipped_quantity: 2, + unit_price: variant2Price, + adjustments: [ + { + amount: 977, + discount_code: discount.code, + description: "discount", + item_id: "test-item", + }, + ], + tax_lines: [ + { + name: "default", + code: "default", + rate: 20, + }, + ], + }, + ], + }) + + const api = useApi() + + const response = await api.post( + `/admin/orders/${order.id}/return`, + { + items: [ + { + item_id: item1Id, + quantity: 1, + note: "TOO SMALL", + }, + ], + }, + { + headers: { + authorization: "Bearer test_token", + }, + } + ) + + expect(response.status).toEqual(200) + + /* + * Region has default tax rate 12.5 but line item has tax rate 20 + * total item 1 amount (4452600 * 2 - 9023) * 1.2 = 10675412 + * therefore refund amount should be (4452600 - 9023 / 2) * 1.2 = 5337706 + * therefore if the second item gets refunded 5337706.2 * 2 = 10675412 which is the expected total amount + */ + expect(response.data.order.returns[0].refund_amount).toEqual(5337706) + + expect(response.data.order.returns[0].items).toHaveLength(1) + expect(response.data.order.returns[0].items).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + item_id: "test-item", + quantity: 1, + note: "TOO SMALL", + }), + ]) + ) + }) + test("receives a return with a claimed line item", async () => { await adminSeeder(dbConnection) diff --git a/integration-tests/api/__tests__/store/cart/cart.js b/integration-tests/api/__tests__/store/cart/cart.js index 5cdaa3bbe9..233abf535b 100644 --- a/integration-tests/api/__tests__/store/cart/cart.js +++ b/integration-tests/api/__tests__/store/cart/cart.js @@ -1466,18 +1466,97 @@ describe("/store/carts", () => { it("Applies dynamic discount to cart correctly", async () => { const api = useApi() - const cart = await api.post( - "/store/carts/test-cart", - { - discounts: [{ code: "DYN_DISC" }], - }, - { withCredentials: true } - ) + const cart = await api.post("/store/carts/test-cart", { + discounts: [{ code: "DYN_DISC" }], + }) expect(cart.data.cart.shipping_total).toBe(1000) expect(cart.status).toEqual(200) }) + it("successfully apply a fixed discount with total allocation and return the total with the right precision", async () => { + const api = useApi() + + const cartId = "test-cart" + const quantity = 2 + + const variant1Price = 4452600 + const product1 = await simpleProductFactory(dbConnection, { + variants: [ + { + id: "test-variant-1-fixed-discount-total", + prices: [ + { + amount: variant1Price, + currency: "usd", + variant_id: "test-variant-1-fixed-discount-total", + }, + ], + }, + ], + }) + + await api.post(`/store/carts/${cartId}/line-items`, { + variant_id: product1.variants[0].id, + quantity, + }) + + const variant2Price = 482200 + const product2 = await simpleProductFactory(dbConnection, { + variants: [ + { + id: "test-variant-2-fixed-discount-total", + prices: [ + { + amount: variant2Price, + currency: "usd", + variant_id: "test-variant-2-fixed-discount-total", + }, + ], + }, + ], + }) + + await api.post(`/store/carts/${cartId}/line-items`, { + variant_id: product2.variants[0].id, + quantity, + }) + + const discountAmount = 10000 + const discount = await simpleDiscountFactory(dbConnection, { + id: "test-discount", + code: "TEST", + regions: ["test-region"], + rule: { + type: "fixed", + value: discountAmount, + allocation: "total", + }, + }) + + const response = await api.post(`/store/carts/${cartId}`, { + discounts: [{ code: discount.code }], + }) + + expect(response.status).toBe(200) + + const cart = response.data.cart + + const shippingAmount = 1000 + const expectedTotal = + quantity * variant1Price + + quantity * variant2Price + + shippingAmount - + discountAmount + + const expectedSubtotal = expectedTotal + discountAmount - shippingAmount + + expect(cart.total).toBe(expectedTotal) + expect(cart.subtotal).toBe(expectedSubtotal) + expect(cart.discount_total).toBe(discountAmount) + expect(cart.shipping_total).toBe(1000) + }) + it("updates cart customer id", async () => { const api = useApi() diff --git a/integration-tests/api/factories/simple-order-factory.ts b/integration-tests/api/factories/simple-order-factory.ts index 7d3a58a7c9..8450e09484 100644 --- a/integration-tests/api/factories/simple-order-factory.ts +++ b/integration-tests/api/factories/simple-order-factory.ts @@ -1,13 +1,37 @@ import { Connection } from "typeorm" import faker from "faker" -import { Discount, FulfillmentStatus, Order, PaymentStatus, Refund, } from "@medusajs/medusa" -import { DiscountFactoryData, simpleDiscountFactory, } from "./simple-discount-factory" +import { + Discount, + FulfillmentStatus, + Order, + PaymentStatus, + Refund, +} from "@medusajs/medusa" +import { + DiscountFactoryData, + simpleDiscountFactory, +} from "./simple-discount-factory" import { RegionFactoryData, simpleRegionFactory } from "./simple-region-factory" -import { LineItemFactoryData, simpleLineItemFactory, } from "./simple-line-item-factory" -import { AddressFactoryData, simpleAddressFactory, } from "./simple-address-factory" -import { ShippingMethodFactoryData, simpleShippingMethodFactory, } from "./simple-shipping-method-factory" -import { SalesChannelFactoryData, simpleSalesChannelFactory, } from "./simple-sales-channel-factory" -import { CustomerFactoryData, simpleCustomerFactory, } from "./simple-customer-factory" +import { + LineItemFactoryData, + simpleLineItemFactory, +} from "./simple-line-item-factory" +import { + AddressFactoryData, + simpleAddressFactory, +} from "./simple-address-factory" +import { + ShippingMethodFactoryData, + simpleShippingMethodFactory, +} from "./simple-shipping-method-factory" +import { + SalesChannelFactoryData, + simpleSalesChannelFactory, +} from "./simple-sales-channel-factory" +import { + CustomerFactoryData, + simpleCustomerFactory, +} from "./simple-customer-factory" export type OrderFactoryData = { id?: string @@ -40,6 +64,7 @@ export const simpleOrderFactory = async ( let currencyCode: string let regionId: string let taxRate: number + if (typeof data.region === "string") { currencyCode = data.currency_code as string regionId = data.region @@ -51,6 +76,7 @@ export const simpleOrderFactory = async ( currencyCode = region.currency_code regionId = region.id } + const address = await simpleAddressFactory(connection, data.shipping_address) const customer = await simpleCustomerFactory(connection, { diff --git a/integration-tests/api/helpers/admin-seeder.js b/integration-tests/api/helpers/admin-seeder.js index 1397533a88..660bad0470 100644 --- a/integration-tests/api/helpers/admin-seeder.js +++ b/integration-tests/api/helpers/admin-seeder.js @@ -1,6 +1,5 @@ const Scrypt = require("scrypt-kdf") const { User } = require("@medusajs/medusa") -const { simpleSalesChannelFactory } = require("../factories") module.exports = async (connection, data = {}) => { const manager = connection.manager diff --git a/packages/medusa/src/services/new-totals.ts b/packages/medusa/src/services/new-totals.ts index 5f22fbd957..ccc7b3d782 100644 --- a/packages/medusa/src/services/new-totals.ts +++ b/packages/medusa/src/services/new-totals.ts @@ -172,8 +172,7 @@ export default class NewTotalsService extends TransactionBaseService { subtotal = 0 // in that case we need to know the tax rate to compute it later } - const discount_total = - (lineItemAllocation.discount?.unit_amount || 0) * item.quantity + const discount_total = lineItemAllocation.discount?.amount ?? 0 const totals: LineItemTotals = { unit_price: item.unit_price, @@ -275,8 +274,7 @@ export default class NewTotalsService extends TransactionBaseService { subtotal = 0 // in that case we need to know the tax rate to compute it later } - const discount_total = - (lineItemAllocation.discount?.unit_amount || 0) * item.quantity + const discount_total = lineItemAllocation.discount?.amount ?? 0 const totals: LineItemTotals = { unit_price: item.unit_price, @@ -428,8 +426,7 @@ export default class NewTotalsService extends TransactionBaseService { ) const discountAmount = - (calculationContext.allocation_map[lineItem.id]?.discount?.unit_amount || - 0) * lineItem.quantity + calculationContext.allocation_map[lineItem.id]?.discount?.amount ?? 0 const lineSubtotal = (lineItem.unit_price - taxAmountIncludedInPrice) * lineItem.quantity - diff --git a/packages/medusa/src/services/totals.ts b/packages/medusa/src/services/totals.ts index a0ad963f29..dfc3d1606c 100644 --- a/packages/medusa/src/services/totals.ts +++ b/packages/medusa/src/services/totals.ts @@ -462,12 +462,18 @@ class TotalsService extends TransactionBaseService { if (allocationMap[ld.item.id]) { allocationMap[ld.item.id].discount = { amount: adjustmentAmount, + /** + * Used for the refund computation + */ unit_amount: Math.round(adjustmentAmount / ld.item.quantity), } } else { allocationMap[ld.item.id] = { discount: { amount: adjustmentAmount, + /** + * Used for the refund computation + */ unit_amount: Math.round(adjustmentAmount / ld.item.quantity), }, } @@ -794,8 +800,7 @@ class TotalsService extends TransactionBaseService { subtotal = 0 // in that case we need to know the tax rate to compute it later } - const discount_total = - (lineItemAllocation.discount?.unit_amount || 0) * lineItem.quantity + const discount_total = lineItemAllocation.discount?.amount ?? 0 const lineItemTotals: LineItemTotals = { unit_price: lineItem.unit_price, diff --git a/packages/medusa/src/strategies/tax-calculation.ts b/packages/medusa/src/strategies/tax-calculation.ts index 5780a43620..eec2900fc2 100644 --- a/packages/medusa/src/strategies/tax-calculation.ts +++ b/packages/medusa/src/strategies/tax-calculation.ts @@ -73,9 +73,7 @@ class TaxCalculationStrategy implements ITaxCalculationStrategy { taxableAmount = item.unit_price * item.quantity } - taxableAmount -= - ((allocations.discount && allocations.discount.unit_amount) || 0) * - item.quantity + taxableAmount -= allocations.discount?.amount ?? 0 for (const filteredTaxLine of filteredTaxLines) { taxTotal += Math.round(