fix(medusa): Throw on flat rate shipping options with no amount (#2628)

2022-12-12 18:34:30 +01:00
parent e63777e3c5
commit b2ea8b7d45
8 changed files with 267 additions and 163 deletions
--- a/.changeset/two-walls-warn.md
+++ b/.changeset/two-walls-warn.md
@@ -0,0 +1,5 @@
+---
+"@medusajs/medusa": patch
+---
+
+fix(medusa): Throw on flat rate shipping options with no amount
--- 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 () => {
--- 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)
  })
 })
--- 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"] }
--- 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,
        })
      )
    })
--- 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<string, unknown>
+}
+
 /**
 * Helps retrive fulfillment providers
 */
@@ -118,7 +123,7 @@ class FulfillmentProviderService extends TransactionBaseService {
    ) as unknown as Record<string, unknown>
  }

-  async canCalculate(option: ShippingOption): Promise<boolean> {
+  async canCalculate(option: CalculateOptionPriceInput): Promise<boolean> {
    const provider = this.retrieveProvider(option.provider_id)
    return provider.canCalculate(option.data) as unknown as boolean
  }
--- 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<Omit<ShippingOption, "beforeInsert"> | CreateShippingOptionInput> {
+    const option_:
+      | Omit<ShippingOption, "beforeInsert">
+      | 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<ShippingOption> {
    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<ShippingOption>)
+      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<ShippingOptionPriceType> {
    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)
    })
  }

--- 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
+}