diff --git a/.changeset/two-walls-warn.md b/.changeset/two-walls-warn.md new file mode 100644 index 0000000000..06b12c8de9 --- /dev/null +++ b/.changeset/two-walls-warn.md @@ -0,0 +1,5 @@ +--- +"@medusajs/medusa": patch +--- + +fix(medusa): Throw on flat rate shipping options with no amount diff --git a/integration-tests/api/__tests__/admin/shipping-options.js b/integration-tests/api/__tests__/admin/shipping-options.js index dc077bf39a..396c0bd155 100644 --- a/integration-tests/api/__tests__/admin/shipping-options.js +++ b/integration-tests/api/__tests__/admin/shipping-options.js @@ -28,7 +28,7 @@ describe("/admin/shipping-options", () => { beforeAll(async () => { const cwd = path.resolve(path.join(__dirname, "..", "..")) dbConnection = await initDb({ cwd }) - medusaProcess = await setupServer({ cwd }) + medusaProcess = await setupServer({ cwd, verbose: false }) }) afterAll(async () => { diff --git a/integration-tests/api/__tests__/totals/orders.js b/integration-tests/api/__tests__/totals/orders.js index 97b9343ee8..72620d858f 100644 --- a/integration-tests/api/__tests__/totals/orders.js +++ b/integration-tests/api/__tests__/totals/orders.js @@ -14,15 +14,16 @@ const { jest.setTimeout(30000) +const adminReqConfig = { + headers: { + Authorization: "Bearer test_token", + }, +} + describe("Order Totals", () => { let medusaProcess let dbConnection - const doAfterEach = async () => { - const db = useDb() - return await db.teardown() - } - beforeAll(async () => { const cwd = path.resolve(path.join(__dirname, "..", "..")) dbConnection = await initDb({ cwd }) @@ -36,137 +37,154 @@ describe("Order Totals", () => { }) afterEach(async () => { - return await doAfterEach() + const db = useDb() + await db.teardown() }) - test("calculates totals correctly for order with non-taxable gift card", async () => { - await adminSeeder(dbConnection) - await simpleProductFactory(dbConnection, { - variants: [ - { id: "variant_1", prices: [{ currency: "usd", amount: 95600 }] }, - { id: "variant_2", prices: [{ currency: "usd", amount: 79600 }] }, - ], + describe("GET /admin/orders/:id/totals", () => { + beforeEach(async () => { + await adminSeeder(dbConnection) + + await simpleProductFactory(dbConnection, { + variants: [ + { id: "variant_1", prices: [{ currency: "usd", amount: 95600 }] }, + { id: "variant_2", prices: [{ currency: "usd", amount: 79600 }] }, + ], + }) }) - const region = await simpleRegionFactory(dbConnection, { - gift_cards_taxable: false, - tax_rate: 25, + it("calculates totals correctly for order with non-taxable gift card", async () => { + const api = useApi() + + // Seed data + const region = await simpleRegionFactory(dbConnection, { + gift_cards_taxable: false, + tax_rate: 25, + }) + + const cart = await simpleCartFactory(dbConnection, { + id: "test-cart", + email: "testnation@medusajs.com", + region: region.id, + line_items: [], + }) + + const giftCard = await simpleGiftCardFactory(dbConnection, { + region_id: region.id, + value: 160000, + balance: 160000, + }) + + // Add variant 1 to cart + await api.post("/store/carts/test-cart/line-items", { + quantity: 1, + variant_id: "variant_1", + }) + + // Add variant 2 to cart + await api.post("/store/carts/test-cart/line-items", { + quantity: 1, + variant_id: "variant_2", + }) + + // Add gift card to cart + await api.post("/store/carts/test-cart", { + gift_cards: [{ code: giftCard.code }], + }) + + // Init payment sessions + await api.post(`/store/carts/${cart.id}/payment-sessions`) + + // Complete cart + const response = await api.post(`/store/carts/test-cart/complete`) + + expect(response.status).toEqual(200) + expect(response.data.type).toEqual("order") + const orderId = response.data.data.id + + // Retrieve the completed order + const { data } = await api.get(`/admin/orders/${orderId}`, adminReqConfig) + + expect(data.order.gift_card_transactions).toHaveLength(1) + expect(data.order.gift_card_transactions).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + amount: 160000, + is_taxable: false, + tax_rate: null, + }), + ]) + ) + expect(data.order.gift_card_total).toEqual(160000) + expect(data.order.gift_card_tax_total).toEqual(0) + expect(data.order.total).toEqual(59000) }) - const cart = await simpleCartFactory(dbConnection, { - id: "test-cart", - email: "testnation@medusajs.com", - region: region.id, - line_items: [], - }) + it("calculates totals correctly for order with taxable gift card", async () => { + const api = useApi() + // Seed data + const region = await simpleRegionFactory(dbConnection, { + gift_cards_taxable: true, + tax_rate: 25, + }) - const giftCard = await simpleGiftCardFactory(dbConnection, { - region_id: region.id, - value: 160000, - balance: 160000, - }) + const cart = await simpleCartFactory(dbConnection, { + id: "test-cart", + email: "testnation@medusajs.com", + region: region.id, + line_items: [], + }) - const api = useApi() + const giftCard = await simpleGiftCardFactory(dbConnection, { + region_id: region.id, + value: 160000, + balance: 160000, + }) - await api.post("/store/carts/test-cart/line-items", { - quantity: 1, - variant_id: "variant_1", - }) - await api.post("/store/carts/test-cart/line-items", { - quantity: 1, - variant_id: "variant_2", - }) - await api.post("/store/carts/test-cart", { - gift_cards: [{ code: giftCard.code }], - }) - await api.post(`/store/carts/${cart.id}/payment-sessions`) - const response = await api.post(`/store/carts/test-cart/complete`) - expect(response.status).toEqual(200) - expect(response.data.type).toEqual("order") - const orderId = response.data.data.id + // Add variant 1 to cart + await api.post("/store/carts/test-cart/line-items", { + quantity: 1, + variant_id: "variant_1", + }) - const { data } = await api.get(`/admin/orders/${orderId}`, { - headers: { Authorization: `Bearer test_token` }, - }) + // Add variant 2 to cart + await api.post("/store/carts/test-cart/line-items", { + quantity: 1, + variant_id: "variant_2", + }) - expect(data.order.gift_card_transactions).toHaveLength(1) - expect(data.order.gift_card_transactions).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - amount: 160000, - is_taxable: false, - tax_rate: null, - }), - ]) - ) - expect(data.order.gift_card_total).toEqual(160000) - expect(data.order.gift_card_tax_total).toEqual(0) - expect(data.order.total).toEqual(59000) - }) + // Add gift card to cart + await api.post("/store/carts/test-cart", { + gift_cards: [{ code: giftCard.code }], + }) - test("calculates totals correctly for order with taxable gift card", async () => { - await adminSeeder(dbConnection) - await simpleProductFactory(dbConnection, { - variants: [ - { id: "variant_1", prices: [{ currency: "usd", amount: 95600 }] }, - { id: "variant_2", prices: [{ currency: "usd", amount: 79600 }] }, - ], - }) + // Init payment sessions + await api.post(`/store/carts/${cart.id}/payment-sessions`) - const region = await simpleRegionFactory(dbConnection, { - gift_cards_taxable: true, - tax_rate: 25, - }) + // Complete cart + const response = await api.post(`/store/carts/test-cart/complete`) - const cart = await simpleCartFactory(dbConnection, { - id: "test-cart", - email: "testnation@medusajs.com", - region: region.id, - line_items: [], - }) + expect(response.status).toEqual(200) + expect(response.data.type).toEqual("order") + const orderId = response.data.data.id - const giftCard = await simpleGiftCardFactory(dbConnection, { - region_id: region.id, - value: 160000, - balance: 160000, - }) + // Retrieve the completed order + const { data } = await api.get(`/admin/orders/${orderId}`, adminReqConfig) - const api = useApi() - - await api.post("/store/carts/test-cart/line-items", { - quantity: 1, - variant_id: "variant_1", + expect(data.order.gift_card_transactions).toHaveLength(1) + expect(data.order.gift_card_transactions).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + amount: 160000, + is_taxable: true, + tax_rate: 25, + }), + ]) + ) + expect(data.order.gift_card_total).toEqual(160000) + expect(data.order.gift_card_tax_total).toEqual(40000) + expect(data.order.tax_total).toEqual(3800) + expect(data.order.total).toEqual(19000) }) - await api.post("/store/carts/test-cart/line-items", { - quantity: 1, - variant_id: "variant_2", - }) - await api.post("/store/carts/test-cart", { - gift_cards: [{ code: giftCard.code }], - }) - await api.post(`/store/carts/${cart.id}/payment-sessions`) - const response = await api.post(`/store/carts/test-cart/complete`) - expect(response.status).toEqual(200) - expect(response.data.type).toEqual("order") - const orderId = response.data.data.id - - const { data } = await api.get(`/admin/orders/${orderId}`, { - headers: { Authorization: `Bearer test_token` }, - }) - - expect(data.order.gift_card_transactions).toHaveLength(1) - expect(data.order.gift_card_transactions).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - amount: 160000, - is_taxable: true, - tax_rate: 25, - }), - ]) - ) - expect(data.order.gift_card_total).toEqual(160000) - expect(data.order.gift_card_tax_total).toEqual(40000) - expect(data.order.tax_total).toEqual(3800) - expect(data.order.total).toEqual(19000) }) }) diff --git a/integration-tests/api/factories/simple-product-factory.ts b/integration-tests/api/factories/simple-product-factory.ts index d0b3ad4b43..b972a94f95 100644 --- a/integration-tests/api/factories/simple-product-factory.ts +++ b/integration-tests/api/factories/simple-product-factory.ts @@ -129,7 +129,7 @@ export const simpleProductFactory = async ( await simpleProductVariantFactory(connection, factoryData) } - return manager.findOne( + return await manager.findOne( Product, { id: prodId }, { relations: ["tags", "variants", "variants.prices"] } diff --git a/packages/medusa/src/services/__tests__/shipping-option.js b/packages/medusa/src/services/__tests__/shipping-option.js index f483e8c59b..12b1f56bdb 100644 --- a/packages/medusa/src/services/__tests__/shipping-option.js +++ b/packages/medusa/src/services/__tests__/shipping-option.js @@ -1,8 +1,7 @@ -import _ from "lodash" -import { IdMap, MockRepository, MockManager } from "medusa-test-utils" +import { IdMap, MockManager, MockRepository } from "medusa-test-utils" +import TaxInclusivePricingFeatureFlag from "../../loaders/feature-flags/tax-inclusive-pricing" +import { FlagRouter } from "../../utils/flag-router" import ShippingOptionService from "../shipping-option" -import { FlagRouter } from "../../utils/flag-router"; -import TaxInclusivePricingFeatureFlag from "../../loaders/feature-flags/tax-inclusive-pricing"; describe("ShippingOptionService", () => { describe("retrieve", () => { @@ -36,13 +35,20 @@ describe("ShippingOptionService", () => { provider_id: "no_calc", }) case IdMap.getId("validId"): + return Promise.resolve({ + provider_id: "provider", + amount: 100, + data: { + provider_data: "true", + }, + }) + case "flat-rate-no-amount": return Promise.resolve({ provider_id: "provider", data: { provider_data: "true", }, }) - default: return Promise.resolve({}) } @@ -137,7 +143,7 @@ describe("ShippingOptionService", () => { it("sets flat rate price", async () => { await optionService.update(IdMap.getId("validId"), { price_type: "flat_rate", - amount: 100, + amount: 200, }) expect(shippingOptionRepository.save).toHaveBeenCalledTimes(1) @@ -147,7 +153,37 @@ describe("ShippingOptionService", () => { provider_data: "true", }, price_type: "flat_rate", - amount: 100, + amount: 200, + }) + }) + + it("throws on flat rate but no amount", async () => { + expect.assertions(1) + try { + await optionService.update(IdMap.getId("flat-rate-no-amount"), { + price_type: "flat_rate", + }) + } catch (error) { + expect(error.message).toEqual( + "Shipping options of type `flat_rate` must have an `amount`" + ) + } + }) + + it("sets a price to", async () => { + await optionService.update(IdMap.getId("validId"), { + price_type: "flat_rate", + amount: 0, + }) + + expect(shippingOptionRepository.save).toHaveBeenCalledTimes(1) + expect(shippingOptionRepository.save).toHaveBeenCalledWith({ + provider_id: "provider", + data: { + provider_data: "true", + }, + price_type: "flat_rate", + amount: 0, }) }) @@ -158,10 +194,8 @@ describe("ShippingOptionService", () => { expect(fulfillmentProviderService.canCalculate).toHaveBeenCalledTimes(1) expect(fulfillmentProviderService.canCalculate).toHaveBeenCalledWith({ - amount: null, data: { provider_data: "true" }, provider_id: "provider", - price_type: "calculated", }) expect(shippingOptionRepository.save).toHaveBeenCalledTimes(1) @@ -645,7 +679,7 @@ describe("ShippingOptionService", () => { totalsService, fulfillmentProviderService: providerService, featureFlagRouter: new FlagRouter({ - [TaxInclusivePricingFeatureFlag.key]: true + [TaxInclusivePricingFeatureFlag.key]: true, }), }) @@ -653,11 +687,11 @@ describe("ShippingOptionService", () => { jest.clearAllMocks() }) - it("should create a shipping method that also includes the taxes", async () => { + it("should create a shipping method that also includes the taxes", async () => { await optionService.createShippingMethod("random_id", {}, { price: 10 }) expect(shippingMethodRepository.save).toHaveBeenCalledWith( expect.objectContaining({ - includes_tax: true + includes_tax: true, }) ) }) diff --git a/packages/medusa/src/services/fulfillment-provider.ts b/packages/medusa/src/services/fulfillment-provider.ts index b7609bc182..10d426aec6 100644 --- a/packages/medusa/src/services/fulfillment-provider.ts +++ b/packages/medusa/src/services/fulfillment-provider.ts @@ -28,6 +28,11 @@ type FulfillmentProviderContainer = MedusaContainer & { [key in `${FulfillmentProviderKey}`]: typeof BaseFulfillmentService } +type CalculateOptionPriceInput = { + provider_id: string + data: Record +} + /** * Helps retrive fulfillment providers */ @@ -118,7 +123,7 @@ class FulfillmentProviderService extends TransactionBaseService { ) as unknown as Record } - async canCalculate(option: ShippingOption): Promise { + async canCalculate(option: CalculateOptionPriceInput): Promise { const provider = this.retrieveProvider(option.provider_id) return provider.canCalculate(option.data) as unknown as boolean } diff --git a/packages/medusa/src/services/shipping-option.ts b/packages/medusa/src/services/shipping-option.ts index c7a60db717..4f5a04de54 100644 --- a/packages/medusa/src/services/shipping-option.ts +++ b/packages/medusa/src/services/shipping-option.ts @@ -1,6 +1,7 @@ import { MedusaError } from "medusa-core-utils" -import { DeepPartial, EntityManager } from "typeorm" +import { EntityManager } from "typeorm" import { TransactionBaseService } from "../interfaces" +import TaxInclusivePricingFeatureFlag from "../loaders/feature-flags/tax-inclusive-pricing" import { Cart, Order, @@ -18,12 +19,12 @@ import { CreateShippingOptionInput, ShippingMethodUpdate, UpdateShippingOptionInput, + ValidatePriceTypeAndAmountInput, } from "../types/shipping-options" import { buildQuery, isDefined, setMetadata } from "../utils" +import { FlagRouter } from "../utils/flag-router" import FulfillmentProviderService from "./fulfillment-provider" import RegionService from "./region" -import { FlagRouter } from "../utils/flag-router" -import TaxInclusivePricingFeatureFlag from "../loaders/feature-flags/tax-inclusive-pricing" type InjectedDependencies = { manager: EntityManager @@ -397,6 +398,42 @@ class ShippingOptionService extends TransactionBaseService { return option } + private async validateAndMutatePrice( + option: ShippingOption | CreateShippingOptionInput, + priceInput: ValidatePriceTypeAndAmountInput + ): Promise | CreateShippingOptionInput> { + const option_: + | Omit + | CreateShippingOptionInput = { ...option } + + if (isDefined(priceInput.amount)) { + option_.amount = priceInput.amount + } + + if (isDefined(priceInput.price_type)) { + option_.price_type = await this.validatePriceType_( + priceInput.price_type, + option_ as ShippingOption + ) + + if (priceInput.price_type === ShippingOptionPriceType.CALCULATED) { + option_.amount = null + } + } + + if ( + option_.price_type === ShippingOptionPriceType.FLAT_RATE && + (option_.amount == null || option_.amount < 0) + ) { + throw new MedusaError( + MedusaError.Types.INVALID_DATA, + "Shipping options of type `flat_rate` must have an `amount`" + ) + } + + return option_ + } + /** * Creates a new shipping option. Used both for outbound and inbound shipping * options. The difference is registered by the `is_return` field which @@ -406,8 +443,12 @@ class ShippingOptionService extends TransactionBaseService { */ async create(data: CreateShippingOptionInput): Promise { return this.atomicPhase_(async (manager) => { + const optionWithValidatedPrice = await this.validateAndMutatePrice(data, { + price_type: data.price_type, + }) + const optionRepo = manager.getCustomRepository(this.optionRepository_) - const option = optionRepo.create(data as DeepPartial) + const option = optionRepo.create(optionWithValidatedPrice) const region = await this.regionService_ .withTransaction(manager) @@ -426,10 +467,6 @@ class ShippingOptionService extends TransactionBaseService { ) } - option.price_type = await this.validatePriceType_(data.price_type, option) - option.amount = - data.price_type === "calculated" ? null : data.amount ?? null - if ( this.featureFlagRouter_.isFeatureEnabled( TaxInclusivePricingFeatureFlag.key @@ -440,7 +477,9 @@ class ShippingOptionService extends TransactionBaseService { } } - const isValid = await this.providerService_.validateOption(option) + const isValid = await this.providerService_.validateOption( + option as ShippingOption + ) if (!isValid) { throw new MedusaError( @@ -496,7 +535,8 @@ class ShippingOptionService extends TransactionBaseService { ): Promise { if ( !priceType || - (priceType !== "flat_rate" && priceType !== "calculated") + (priceType !== ShippingOptionPriceType.FLAT_RATE && + priceType !== ShippingOptionPriceType.CALCULATED) ) { throw new MedusaError( MedusaError.Types.INVALID_DATA, @@ -504,8 +544,11 @@ class ShippingOptionService extends TransactionBaseService { ) } - if (priceType === "calculated") { - const canCalculate = await this.providerService_.canCalculate(option) + if (priceType === ShippingOptionPriceType.CALCULATED) { + const canCalculate = await this.providerService_.canCalculate({ + provider_id: option.provider_id, + data: option.data, + }) if (!canCalculate) { throw new MedusaError( MedusaError.Types.INVALID_DATA, @@ -597,26 +640,20 @@ class ShippingOptionService extends TransactionBaseService { option.requirements = acc } - if (isDefined(update.price_type)) { - option.price_type = await this.validatePriceType_( - update.price_type, - option - ) - if (update.price_type === "calculated") { - option.amount = null + const optionWithValidatedPrice = await this.validateAndMutatePrice( + option, + { + price_type: update.price_type, + amount: update.amount, } - } - - if (isDefined(update.amount) && option.price_type !== "calculated") { - option.amount = update.amount - } + ) if (isDefined(update.name)) { - option.name = update.name + optionWithValidatedPrice.name = update.name } if (isDefined(update.admin_only)) { - option.admin_only = update.admin_only + optionWithValidatedPrice.admin_only = update.admin_only } if ( @@ -625,12 +662,12 @@ class ShippingOptionService extends TransactionBaseService { ) ) { if (typeof update.includes_tax !== "undefined") { - option.includes_tax = update.includes_tax + optionWithValidatedPrice.includes_tax = update.includes_tax } } const optionRepo = manager.getCustomRepository(this.optionRepository_) - return await optionRepo.save(option) + return await optionRepo.save(optionWithValidatedPrice) }) } diff --git a/packages/medusa/src/types/shipping-options.ts b/packages/medusa/src/types/shipping-options.ts index 44c29c3856..d025c86850 100644 --- a/packages/medusa/src/types/shipping-options.ts +++ b/packages/medusa/src/types/shipping-options.ts @@ -74,3 +74,8 @@ export type UpdateShippingOptionInput = { data?: string includes_tax?: boolean } + +export type ValidatePriceTypeAndAmountInput = { + amount?: number + price_type?: ShippingOptionPriceType +}