Fix/polish discount (#569)

* formatting and optional "is_dynamic" on updates

* formatting

* uppercasing discount code on update

* multiple regions fail with fixed discounts

* discount testing

* testing

* spelling

* removed comment

Co-authored-by: Sebastian Rindom <skrindom@gmail.com>

integration-tests/api/__tests__/admin/discount.js

@@ -5,6 +5,8 @@ const setupServer = require("../../../helpers/setup-server")
 const { useApi } = require("../../../helpers/use-api")
 const { initDb, useDb } = require("../../../helpers/use-db")
 const adminSeeder = require("../../helpers/admin-seeder")
+const discountSeeder = require("../../helpers/discount-seeder")
+const { exportAllDeclaration } = require("@babel/types")
 
 jest.setTimeout(30000)
 
@@ -89,6 +91,7 @@ describe("/admin/discounts", () => {
     beforeEach(async () => {
       try {
         await adminSeeder(dbConnection)
+        await discountSeeder(dbConnection)
       } catch (err) {
         console.log(err)
         throw err
@@ -159,6 +162,256 @@ describe("/admin/discounts", () => {
       )
     })
 
+    it("automatically sets the code to an uppercase string on update", async () => {
+      const api = useApi()
+
+      const response = await api
+        .post(
+          "/admin/discounts",
+          {
+            code: "HELLOworld",
+            rule: {
+              description: "test",
+              type: "percentage",
+              value: 10,
+              allocation: "total",
+            },
+            usage_limit: 10,
+          },
+          {
+            headers: {
+              Authorization: "Bearer test_token",
+            },
+          }
+        )
+        .catch((err) => {
+          console.log(err)
+        })
+
+      expect(response.status).toEqual(200)
+      expect(response.data.discount).toEqual(
+        expect.objectContaining({
+          code: "HELLOWORLD",
+          usage_limit: 10,
+        })
+      )
+
+      const updated = await api
+        .post(
+          `/admin/discounts/${response.data.discount.id}`,
+          {
+            code: "HELLOWORLD_test",
+            usage_limit: 20,
+          },
+          {
+            headers: {
+              Authorization: "Bearer test_token",
+            },
+          }
+        )
+        .catch((err) => {
+          console.log(err)
+        })
+
+      expect(updated.status).toEqual(200)
+      expect(updated.data.discount).toEqual(
+        expect.objectContaining({
+          code: "HELLOWORLD_TEST",
+          usage_limit: 20,
+        })
+      )
+    })
+
+    it("creates a dynamic discount and updates it", async () => {
+      const api = useApi()
+
+      const response = await api
+        .post(
+          "/admin/discounts",
+          {
+            code: "HELLOWORLD_DYNAMIC",
+            is_dynamic: true,
+            rule: {
+              description: "test",
+              type: "percentage",
+              value: 10,
+              allocation: "total",
+            },
+            usage_limit: 10,
+          },
+          {
+            headers: {
+              Authorization: "Bearer test_token",
+            },
+          }
+        )
+        .catch((err) => {
+          console.log(err)
+        })
+
+      expect(response.status).toEqual(200)
+      expect(response.data.discount).toEqual(
+        expect.objectContaining({
+          code: "HELLOWORLD_DYNAMIC",
+          usage_limit: 10,
+          is_dynamic: true,
+        })
+      )
+
+      const updated = await api
+        .post(
+          `/admin/discounts/${response.data.discount.id}`,
+          {
+            usage_limit: 20,
+          },
+          {
+            headers: {
+              Authorization: "Bearer test_token",
+            },
+          }
+        )
+        .catch((err) => {
+          console.log(err)
+        })
+
+      expect(updated.status).toEqual(200)
+      expect(updated.data.discount).toEqual(
+        expect.objectContaining({
+          code: "HELLOWORLD_DYNAMIC",
+          usage_limit: 20,
+          is_dynamic: true,
+        })
+      )
+    })
+
+    it("fails to create a fixed discount with multiple regions", async () => {
+      expect.assertions(2)
+      const api = useApi()
+
+      await api
+        .post(
+          "/admin/discounts",
+          {
+            code: "HELLOWORLD",
+            is_dynamic: true,
+            rule: {
+              description: "test",
+              type: "fixed",
+              value: 10,
+              allocation: "total",
+            },
+            usage_limit: 10,
+            regions: ["test-region", "test-region-2"],
+          },
+          {
+            headers: {
+              Authorization: "Bearer test_token",
+            },
+          }
+        )
+        .catch((err) => {
+          expect(err.response.status).toEqual(400)
+          expect(err.response.data.message).toEqual(
+            `Fixed discounts can have one region`
+          )
+        })
+    })
+
+    it("fails to update a fixed discount with multiple regions", async () => {
+      expect.assertions(2)
+      const api = useApi()
+
+      const response = await api
+        .post(
+          "/admin/discounts",
+          {
+            code: "HELLOWORLD",
+            rule: {
+              description: "test",
+              type: "fixed",
+              value: 10,
+              allocation: "total",
+            },
+            usage_limit: 10,
+          },
+          {
+            headers: {
+              Authorization: "Bearer test_token",
+            },
+          }
+        )
+        .catch((err) => {
+          console.log(err)
+        })
+
+      await api
+        .post(
+          `/admin/discounts/${response.data.discount.id}`,
+          {
+            regions: ["test-region", "test-region-2"],
+          },
+          {
+            headers: {
+              Authorization: "Bearer test_token",
+            },
+          }
+        )
+
+        .catch((err) => {
+          expect(err.response.status).toEqual(400)
+          expect(err.response.data.message).toEqual(
+            `Fixed discounts can have one region`
+          )
+        })
+    })
+
+    it("fails to add a region to a fixed discount with an existing region", async () => {
+      expect.assertions(2)
+      const api = useApi()
+
+      const response = await api
+        .post(
+          "/admin/discounts",
+          {
+            code: "HELLOWORLD",
+            rule: {
+              description: "test",
+              type: "fixed",
+              value: 10,
+              allocation: "total",
+            },
+            usage_limit: 10,
+            regions: ["test-region"],
+          },
+          {
+            headers: {
+              Authorization: "Bearer test_token",
+            },
+          }
+        )
+        .catch((err) => {
+          console.log(err)
+        })
+
+      await api
+        .post(
+          `/admin/discounts/${response.data.discount.id}/regions/test-region-2`,
+          {},
+          {
+            headers: {
+              Authorization: "Bearer test_token",
+            },
+          }
+        )
+
+        .catch((err) => {
+          expect(err.response.status).toEqual(400)
+          expect(err.response.data.message).toEqual(
+            `Fixed discounts can have one region`
+          )
+        })
+    })
+
     it("creates a discount with start and end dates", async () => {
       const api = useApi()
 

integration-tests/api/helpers/discount-seeder.js

@@ -0,0 +1,35 @@
+const {
+  ShippingProfile,
+  Region,
+  Discount,
+  DiscountRule,
+} = require("@medusajs/medusa")
+module.exports = async (connection, data = {}) => {
+  const manager = connection.manager
+
+  await manager.insert(Region, {
+    id: "test-region",
+    name: "Test Region",
+    currency_code: "usd",
+    tax_rate: 0,
+    payment_providers: [
+      {
+        id: "test-pay",
+        is_installed: true,
+      },
+    ],
+  })
+
+  await manager.insert(Region, {
+    id: "test-region-2",
+    name: "Test Region 2",
+    currency_code: "eur",
+    tax_rate: 0,
+    payment_providers: [
+      {
+        id: "test-pay",
+        is_installed: true,
+      },
+    ],
+  })
+}

packages/medusa/src/api/routes/admin/discounts/__tests__/update-discount.js

@@ -46,7 +46,6 @@ describe("POST /admin/discounts", () => {
             value: 10,
             allocation: "total",
           },
-          is_dynamic: false,
         }
       )
     })

packages/medusa/src/api/routes/admin/discounts/update-discount.js

@@ -55,7 +55,7 @@ export default async (req, res) => {
   const { discount_id } = req.params
   const schema = Validator.object().keys({
     code: Validator.string().optional(),
-    is_dynamic: Validator.boolean().default(false),
+    is_dynamic: Validator.boolean().optional(),
     rule: Validator.object()
       .keys({
         id: Validator.string().required(),
@@ -86,7 +86,6 @@ export default async (req, res) => {
 
   const discountService = req.scope.resolve("discountService")
   await discountService.update(discount_id, value)
-
   const discount = await discountService.retrieve(discount_id, {
     select: defaultFields,
     relations: defaultRelations,

packages/medusa/src/services/__tests__/discount.js

@@ -1,5 +1,7 @@
 import DiscountService from "../discount"
 import { IdMap, MockManager, MockRepository } from "medusa-test-utils"
+import { MedusaError } from "medusa-core-utils"
+import { exportAllDeclaration } from "@babel/types"
 
 describe("DiscountService", () => {
   describe("create", () => {
@@ -13,7 +15,7 @@ describe("DiscountService", () => {
           id: IdMap.getId("france"),
         }
       },
-      withTransaction: function() {
+      withTransaction: function () {
         return this
       },
     }
@@ -29,6 +31,25 @@ describe("DiscountService", () => {
       jest.clearAllMocks()
     })
 
+    it("fails to create a fixed discount with multiple regions", async () => {
+      expect.assertions(3)
+      try {
+        await discountService.create({
+          code: "test",
+          rule: {
+            type: "fixed",
+            allocation: "total",
+            value: 20,
+          },
+          regions: [IdMap.getId("france"), IdMap.getId("Italy")],
+        })
+      } catch (err) {
+        expect(err.type).toEqual("invalid_data")
+        expect(err.message).toEqual("Fixed discounts can have one region")
+        expect(discountRepository.create).toHaveBeenCalledTimes(0)
+      }
+    })
+
     it("successfully creates discount", async () => {
       await discountService.create({
         code: "test",
@@ -130,7 +151,7 @@ describe("DiscountService", () => {
 
   describe("retrieve", () => {
     const discountRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id) {
           return Promise.resolve({ id: IdMap.getId("total10") })
         }
@@ -170,7 +191,7 @@ describe("DiscountService", () => {
 
   describe("retrieveByCode", () => {
     const discountRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.code === "10%OFF") {
           return Promise.resolve({ id: IdMap.getId("total10"), code: "10%OFF" })
         }
@@ -206,7 +227,11 @@ describe("DiscountService", () => {
   describe("update", () => {
     const discountRepository = MockRepository({
       findOne: () =>
-        Promise.resolve({ id: IdMap.getId("total10"), code: "10%OFF" }),
+        Promise.resolve({
+          id: IdMap.getId("total10"),
+          code: "10%OFF",
+          rule: { type: "fixed" },
+        }),
     })
 
     const discountRuleRepository = MockRepository({})
@@ -230,6 +255,20 @@ describe("DiscountService", () => {
       jest.clearAllMocks()
     })
 
+    it("fails to update a fixed discount with multiple regions", async () => {
+      expect.assertions(3)
+      try {
+        await discountService.update(IdMap.getId("total10"), {
+          code: "test",
+          regions: [IdMap.getId("france"), IdMap.getId("Italy")],
+        })
+      } catch (err) {
+        expect(err.type).toEqual("invalid_data")
+        expect(err.message).toEqual("Fixed discounts can have one region")
+        expect(discountRepository.create).toHaveBeenCalledTimes(0)
+      }
+    })
+
     it("successfully updates discount", async () => {
       await discountService.update(IdMap.getId("total10"), {
         code: "test",
@@ -238,7 +277,8 @@ describe("DiscountService", () => {
       expect(discountRepository.save).toHaveBeenCalledTimes(1)
       expect(discountRepository.save).toHaveBeenCalledWith({
         id: IdMap.getId("total10"),
-        code: "test",
+        code: "TEST",
+        rule: { type: "fixed" },
         regions: [{ id: IdMap.getId("france") }],
       })
     })
@@ -262,6 +302,7 @@ describe("DiscountService", () => {
       expect(discountRepository.save).toHaveBeenCalledTimes(1)
       expect(discountRepository.save).toHaveBeenCalledWith({
         id: IdMap.getId("total10"),
+        rule: { type: "fixed" },
         code: "10%OFF",
         metadata: { testKey: "testValue" },
       })
@@ -385,11 +426,24 @@ describe("DiscountService", () => {
 
   describe("addRegion", () => {
     const discountRepository = MockRepository({
-      findOne: () =>
+      findOne: (q) => {
-        Promise.resolve({
+        if (q.where.id === "fixed") {
+          return Promise.resolve({
+            id: IdMap.getId("total10"),
+            regions: [{ id: IdMap.getId("test-region") }],
+            rule: {
+              type: "fixed",
+            },
+          })
+        }
+        return Promise.resolve({
           id: IdMap.getId("total10"),
           regions: [{ id: IdMap.getId("test-region") }],
-        }),
+          rule: {
+            type: "percentage",
+          },
+        })
+      },
     })
 
     const discountRuleRepository = MockRepository({})
@@ -413,6 +467,17 @@ describe("DiscountService", () => {
       jest.clearAllMocks()
     })
 
+    it("fails to add a region to a fixed discount with an existing region", async () => {
+      expect.assertions(3)
+      try {
+        await discountService.addRegion("fixed", IdMap.getId("test-region-2"))
+      } catch (err) {
+        expect(err.type).toEqual("invalid_data")
+        expect(err.message).toEqual("Fixed discounts can have one region")
+        expect(discountRepository.save).toHaveBeenCalledTimes(0)
+      }
+    })
+
     it("successfully adds a region", async () => {
       await discountService.addRegion(
         IdMap.getId("total10"),
@@ -426,6 +491,9 @@ describe("DiscountService", () => {
           { id: IdMap.getId("test-region") },
           { id: IdMap.getId("test-region-2") },
         ],
+        rule: {
+          type: "percentage",
+        },
       })
     })
 
@@ -441,7 +509,7 @@ describe("DiscountService", () => {
 
   describe("createDynamicDiscount", () => {
     const discountRepository = MockRepository({
-      create: d => d,
+      create: (d) => d,
       findOne: () =>
         Promise.resolve({
           id: "parent",

packages/medusa/src/services/__tests__/product-variant.js

@@ -4,7 +4,7 @@ import ProductVariantService from "../product-variant"
 
 const eventBusService = {
   emit: jest.fn(),
-  withTransaction: function() {
+  withTransaction: function () {
     return this
   },
 }
@@ -12,7 +12,7 @@ const eventBusService = {
 describe("ProductVariantService", () => {
   describe("retrieve", () => {
     const productVariantRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id === IdMap.getId("batman")) {
           return Promise.resolve(undefined)
         }
@@ -54,13 +54,13 @@ describe("ProductVariantService", () => {
 
   describe("create", () => {
     const productVariantRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         return Promise.resolve()
       },
     })
 
     const productRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id === IdMap.getId("ironmans")) {
           return Promise.resolve({
             id: IdMap.getId("ironman"),
@@ -229,7 +229,7 @@ describe("ProductVariantService", () => {
 
   describe("publishVariant", () => {
     const productVariantRepository = MockRepository({
-      findOne: query => Promise.resolve({ id: IdMap.getId("ironman") }),
+      findOne: (query) => Promise.resolve({ id: IdMap.getId("ironman") }),
     })
 
     const productVariantService = new ProductVariantService({
@@ -268,7 +268,7 @@ describe("ProductVariantService", () => {
 
   describe("update", () => {
     const productVariantRepository = MockRepository({
-      findOne: query => Promise.resolve({ id: IdMap.getId("ironman") }),
+      findOne: (query) => Promise.resolve({ id: IdMap.getId("ironman") }),
     })
 
     const moneyAmountRepository = MockRepository({
@@ -457,11 +457,11 @@ describe("ProductVariantService", () => {
 
   describe("setCurrencyPrice", () => {
     const productVariantRepository = MockRepository({
-      findOne: query => Promise.resolve({ id: IdMap.getId("ironman") }),
+      findOne: (query) => Promise.resolve({ id: IdMap.getId("ironman") }),
     })
 
     const moneyAmountRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.currency_code === "usd") {
           return Promise.resolve(undefined)
         }
@@ -521,18 +521,18 @@ describe("ProductVariantService", () => {
 
   describe("getRegionPrice", () => {
     const regionService = {
-      retrieve: function() {
+      retrieve: function () {
         return Promise.resolve({
           id: IdMap.getId("california"),
           name: "California",
         })
       },
-      withTransaction: function() {
+      withTransaction: function () {
         return this
       },
     }
     const moneyAmountRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.variant_id === IdMap.getId("ironmanv2")) {
           return Promise.resolve(undefined)
         }
@@ -601,7 +601,7 @@ describe("ProductVariantService", () => {
 
   describe("setRegionPrice", () => {
     const moneyAmountRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.region_id === IdMap.getId("cali")) {
           return Promise.resolve(undefined)
         }
@@ -665,7 +665,7 @@ describe("ProductVariantService", () => {
 
   describe("updateOptionValue", () => {
     const productOptionValueRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.variant_id === IdMap.getId("jibberish")) {
           return Promise.resolve(undefined)
         }
@@ -748,7 +748,7 @@ describe("ProductVariantService", () => {
 
   describe("deleteOptionValue", () => {
     const productOptionValueRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.option_id === IdMap.getId("size")) {
           return Promise.resolve(undefined)
         }
@@ -796,7 +796,7 @@ describe("ProductVariantService", () => {
 
   describe("delete", () => {
     const productVariantRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id === IdMap.getId("ironmanv2")) {
           return Promise.resolve(undefined)
         }

packages/medusa/src/services/discount.js

@@ -176,6 +176,13 @@ class DiscountService extends BaseService {
 
       const validatedRule = this.validateDiscountRule_(discount.rule)
 
+      if (discount.regions?.length > 1 && discount.rule.type === "fixed") {
+        throw new MedusaError(
+          MedusaError.Types.INVALID_DATA,
+          "Fixed discounts can have one region"
+        )
+      }
+
       if (discount.regions) {
         discount.regions = await Promise.all(
           discount.regions.map((regionId) =>
@@ -264,7 +271,9 @@ class DiscountService extends BaseService {
     return this.atomicPhase_(async (manager) => {
       const discountRepo = manager.getCustomRepository(this.discountRepository_)
 
-      const discount = await this.retrieve(discountId)
+      const discount = await this.retrieve(discountId, {
+        relations: ["rule"],
+      })
 
       const { rule, metadata, regions, ...rest } = update
 
@@ -277,6 +286,13 @@ class DiscountService extends BaseService {
         }
       }
 
+      if (regions?.length > 1 && discount.rule.type === "fixed") {
+        throw new MedusaError(
+          MedusaError.Types.INVALID_DATA,
+          "Fixed discounts can have one region"
+        )
+      }
+
       if (regions) {
         discount.regions = await Promise.all(
           regions.map((regionId) => this.regionService_.retrieve(regionId))
@@ -300,6 +316,8 @@ class DiscountService extends BaseService {
         discount[key] = value
       }
 
+      discount.code = discount.code.toUpperCase()
+
       const updated = await discountRepo.save(discount)
       return updated
     })
@@ -452,7 +470,7 @@ class DiscountService extends BaseService {
       const discountRepo = manager.getCustomRepository(this.discountRepository_)
 
       const discount = await this.retrieve(discountId, {
-        relations: ["regions"],
+        relations: ["regions", "rule"],
       })
 
       const exists = discount.regions.find((r) => r.id === regionId)
@@ -461,6 +479,13 @@ class DiscountService extends BaseService {
         return discount
       }
 
+      if (discount.regions?.length === 1 && discount.rule.type === "fixed") {
+        throw new MedusaError(
+          MedusaError.Types.INVALID_DATA,
+          "Fixed discounts can have one region"
+        )
+      }
+
       const region = await this.regionService_.retrieve(regionId)
 
       discount.regions = [...discount.regions, region]