diff --git a/.changeset/tender-coins-kiss.md b/.changeset/tender-coins-kiss.md new file mode 100644 index 0000000000..63ba03f0ab --- /dev/null +++ b/.changeset/tender-coins-kiss.md @@ -0,0 +1,5 @@ +--- +"@medusajs/medusa": patch +--- + +Enforces a validation for discounts that makes regions a required parameter diff --git a/integration-tests/api/__tests__/admin/discount.js b/integration-tests/api/__tests__/admin/discount.js index ae695865b7..bcd2a4ecf4 100644 --- a/integration-tests/api/__tests__/admin/discount.js +++ b/integration-tests/api/__tests__/admin/discount.js @@ -24,6 +24,9 @@ const adminReqConfig = { }, } +const validRegionId = "test-region" +const invalidRegionId = "not-a-valid-region" + describe("/admin/discounts", () => { let medusaProcess let dbConnection @@ -408,6 +411,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -471,6 +475,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -538,6 +543,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -576,6 +582,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: [validRegionId], }, adminReqConfig ) @@ -630,6 +637,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -654,6 +662,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: [validRegionId], }, { headers: { @@ -690,6 +699,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -729,6 +739,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -755,6 +766,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: [validRegionId], }, { headers: { @@ -779,6 +791,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -822,6 +835,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -844,6 +858,7 @@ describe("/admin/discounts", () => { id: response.data.discount.rule.id, type: "free_shipping", }, + regions: [validRegionId], }, adminReqConfig ) @@ -869,6 +884,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -883,7 +899,7 @@ describe("/admin/discounts", () => { }) ) - await api + const err = await api .post( `/admin/discounts/${response.data.discount.id}`, { @@ -891,13 +907,12 @@ describe("/admin/discounts", () => { is_dynamic: false, }, adminReqConfig - ) - .catch((err) => { - expect(err.response.status).toEqual(400) - expect(err.response.data.message).toEqual( - "property is_dynamic should not exist" - ) - }) + ).catch(e => e) + + expect(err.response.status).toEqual(400) + expect(err.response.data.message).toEqual( + "property is_dynamic should not exist" + ) }) it("automatically sets the code to an uppercase string on update", async () => { @@ -913,6 +928,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -958,6 +974,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -991,10 +1008,9 @@ describe("/admin/discounts", () => { }) it("fails to create a fixed discount with multiple regions", async () => { - expect.assertions(2) const api = useApi() - await api + const err = await api .post( "/admin/discounts", { @@ -1007,20 +1023,18 @@ describe("/admin/discounts", () => { allocation: "total", }, usage_limit: 10, - regions: ["test-region", "test-region-2"], + regions: [validRegionId, "test-region-2"], }, adminReqConfig - ) - .catch((err) => { - expect(err.response.status).toEqual(400) - expect(err.response.data.message).toEqual( - `Fixed discounts can have one region` - ) - }) + ).catch(e => e) + + 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( @@ -1033,30 +1047,28 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig ) - await api + const err = await api .post( `/admin/discounts/${response.data.discount.id}`, { - regions: ["test-region", "test-region-2"], + regions: [validRegionId, "test-region-2"], }, adminReqConfig - ) + ).catch(e => e) - .catch((err) => { - expect(err.response.status).toEqual(400) - expect(err.response.data.message).toEqual( - `Fixed discounts can have one region` - ) - }) + 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( @@ -1070,24 +1082,22 @@ describe("/admin/discounts", () => { allocation: "total", }, usage_limit: 10, - regions: ["test-region"], + regions: [validRegionId], }, adminReqConfig ) - await api + const err = await api .post( `/admin/discounts/${response.data.discount.id}/regions/test-region-2`, {}, adminReqConfig - ) + ).catch(e => e) - .catch((err) => { - expect(err.response.status).toEqual(400) - expect(err.response.data.message).toEqual( - `Fixed discounts can have one region` - ) - }) + 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 () => { @@ -1103,6 +1113,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], usage_limit: 10, starts_at: new Date("09/15/2021 11:50"), ends_at: new Date("09/15/2021 17:50"), @@ -1172,6 +1183,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], usage_limit: 10, starts_at: new Date("09/15/2021 11:50"), ends_at: new Date("09/15/2021 17:50"), @@ -1215,10 +1227,9 @@ describe("/admin/discounts", () => { }) it("fails to create discount with end date before start date", async () => { - expect.assertions(2) const api = useApi() - await api + const err = await api .post( "/admin/discounts", { @@ -1229,20 +1240,91 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], usage_limit: 10, starts_at: new Date("09/15/2021 11:50"), ends_at: new Date("09/14/2021 17:50"), }, adminReqConfig - ) - .catch((err) => { - expect(err.response.status).toEqual(400) - expect(err.response.data).toEqual( - expect.objectContaining({ - message: `"ends_at" must be greater than "starts_at"`, - }) - ) + ).catch(e => e) + + expect(err.response.status).toEqual(400) + expect(err.response.data).toEqual( + expect.objectContaining({ + message: `"ends_at" must be greater than "starts_at"`, }) + ) + }) + + it("fails to create a discount if the regions contains an invalid regionId ", async () => { + const api = useApi() + + const err = await api.post( + "/admin/discounts", + { + code: "HELLOWORLD", + rule: { + description: "test", + type: "percentage", + value: 10, + allocation: "total", + }, + regions: [validRegionId, invalidRegionId], + }, + adminReqConfig + ).catch(e => e) + + expect(err.response.status).toEqual(404) + expect(err.response.data.message).toEqual( + `Region with not-a-valid-region was not found` + ) + }) + + it("fails to create a discount if the regions contains only invalid regionIds ", async () => { + const api = useApi() + + const err = await api.post( + "/admin/discounts", + { + code: "HELLOWORLD", + rule: { + description: "test", + type: "percentage", + value: 10, + allocation: "total", + }, + regions: [invalidRegionId], + }, + adminReqConfig + ).catch(e => e) + + expect(err.response.status).toEqual(404) + expect(err.response.data.message).toEqual( + `Region with not-a-valid-region was not found` + ) + }) + + it("fails to create a discount if regions are not present ", async () => { + const api = useApi() + + const err = await api.post( + "/admin/discounts", + { + code: "HELLOWORLD", + rule: { + description: "test", + type: "percentage", + value: 10, + allocation: "total", + }, + }, + adminReqConfig + ).catch((e) => e) + + expect(err.response.status).toEqual(400) + expect(err.response.data.message).toEqual( + `each value in regions must be a string, regions must be an array` + ) }) }) @@ -1306,7 +1388,10 @@ describe("/admin/discounts", () => { let manager beforeEach(async () => { manager = dbConnection.manager + await adminSeeder(dbConnection) + await discountSeeder(dbConnection) + await manager.insert(DiscountRule, { id: "test-discount-rule", description: "Test discount rule", @@ -1345,6 +1430,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -1374,6 +1460,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -2033,7 +2120,9 @@ describe("/admin/discounts", () => { describe("GET /admin/discounts/code/:code", () => { beforeEach(async () => { const manager = dbConnection.manager + await adminSeeder(dbConnection) + await discountSeeder(dbConnection) await manager.insert(DiscountRule, { id: "test-discount-rule-fixed", @@ -2115,15 +2204,12 @@ describe("/admin/discounts", () => { it("should respond with 404 on non-existing code", async () => { const api = useApi() + const err = await api.get("/admin/discounts/code/non-existing", adminReqConfig).catch(e => e) - try { - await api.get("/admin/discounts/code/non-existing", adminReqConfig) - } catch (error) { - expect(error.response.status).toEqual(404) - expect(error.response.data.message).toBe( - "Discount with code non-existing was not found" - ) - } + expect(err.response.status).toEqual(404) + expect(err.response.data.message).toBe( + "Discount with code non-existing was not found" + ) }) it("should trim and uppercase code on insert", async () => { @@ -2139,6 +2225,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], usage_limit: 10, }, adminReqConfig diff --git a/packages/medusa/src/api/routes/admin/discounts/__tests__/create-discount.js b/packages/medusa/src/api/routes/admin/discounts/__tests__/create-discount.js index c858562dd9..9d0cbb7964 100644 --- a/packages/medusa/src/api/routes/admin/discounts/__tests__/create-discount.js +++ b/packages/medusa/src/api/routes/admin/discounts/__tests__/create-discount.js @@ -2,7 +2,15 @@ import { IdMap } from "medusa-test-utils" import { request } from "../../../../../helpers/test-request" import { DiscountServiceMock } from "../../../../../services/__mocks__/discount" +const validRegionId = IdMap.getId("region-france") + describe("POST /admin/discounts", () => { + const adminSession = { + jwt: { + userId: IdMap.getId("admin_user") + } + } + describe("successful creation", () => { let subject @@ -16,14 +24,11 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], starts_at: "02/02/2021 13:45", ends_at: "03/14/2021 04:30", }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession, }) }) @@ -41,6 +46,7 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], starts_at: new Date("02/02/2021 13:45"), ends_at: new Date("03/14/2021 04:30"), is_disabled: false, @@ -54,6 +60,7 @@ describe("POST /admin/discounts", () => { beforeAll(async () => { jest.clearAllMocks() + subject = await request("POST", "/admin/discounts", { payload: { code: "TEST", @@ -63,15 +70,12 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], starts_at: "02/02/2021 13:45", is_dynamic: true, valid_duration: "PaMT2D", }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession, }) }) @@ -100,15 +104,12 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], starts_at: "02/02/2021 13:45", is_dynamic: true, valid_duration: "P1Y2M03DT04H05M", }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession, }) }) @@ -126,6 +127,7 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], starts_at: new Date("02/02/2021 13:45"), is_disabled: false, is_dynamic: true, @@ -146,12 +148,9 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession, }) }) @@ -186,15 +185,12 @@ describe("POST /admin/discounts", () => { }, ], }, + regions: [validRegionId], starts_at: "02/02/2021 13:45", is_dynamic: true, valid_duration: "P1Y2M03DT04H05M", }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession, }) }) @@ -222,14 +218,11 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], ends_at: "02/02/2021", starts_at: "03/14/2021", }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession, }) }) @@ -259,13 +252,10 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], starts_at: "03/14/2021 14:30", }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession, }) }) @@ -284,8 +274,44 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [validRegionId], starts_at: new Date("03/14/2021 14:30"), }) }) }) + + describe("unsuccessful creation with invalid regions", () => { + let subject + + beforeAll(async () => { + jest.clearAllMocks() + + subject = await request("POST", "/admin/discounts", { + payload: { + code: "TEST", + rule: { + description: "Test", + type: "fixed", + value: 10, + allocation: "total", + }, + }, + adminSession, + }) + }) + + it("returns 400", () => { + expect(subject.status).toEqual(400) + }) + + it("returns error", () => { + expect(subject.body.message).toEqual( + `each value in regions must be a string, regions must be an array` + ) + }) + + it("does not call service create", () => { + expect(DiscountServiceMock.create).toHaveBeenCalledTimes(0) + }) + }) }) diff --git a/packages/medusa/src/api/routes/admin/discounts/create-discount.ts b/packages/medusa/src/api/routes/admin/discounts/create-discount.ts index 30ede6452b..37da3037a2 100644 --- a/packages/medusa/src/api/routes/admin/discounts/create-discount.ts +++ b/packages/medusa/src/api/routes/admin/discounts/create-discount.ts @@ -42,6 +42,7 @@ import { FindParams } from "../../../../types/common" * required: * - code * - rule + * - regions * properties: * code: * type: string @@ -151,6 +152,7 @@ import { FindParams } from "../../../../types/common" * value: 10, * allocation: AllocationType.ITEM * }, + * regions: ['reg_XXXXXXXX'], * is_dynamic: false, * is_disabled: false * }) @@ -169,7 +171,8 @@ import { FindParams } from "../../../../types/common" * "type": "fixed", * "value": 10, * "allocation": "item" - * } + * }, + * "regions": ['reg_XXXXXXXX'] * }' * security: * - api_token: [] @@ -257,9 +260,8 @@ export class AdminPostDiscountsReq { usage_limit?: number @IsArray() - @IsOptional() @IsString({ each: true }) - regions?: string[] + regions: string[] @IsObject() @IsOptional() diff --git a/packages/medusa/src/helpers/test-request.js b/packages/medusa/src/helpers/test-request.js index 8654410abb..cb0ee38003 100644 --- a/packages/medusa/src/helpers/test-request.js +++ b/packages/medusa/src/helpers/test-request.js @@ -87,16 +87,18 @@ export async function request(method, url, opts = {}) { ) headers.Cookie = headers.Cookie || "" if (opts.adminSession) { - if (opts.adminSession.jwt) { - opts.adminSession.jwt = jwt.sign( - opts.adminSession.jwt, + const adminSession = { ...opts.adminSession } + + if (adminSession.jwt) { + adminSession.jwt = jwt.sign( + adminSession.jwt, config.projectConfig.jwt_secret, { expiresIn: "30m", } ) } - headers.Cookie = JSON.stringify(opts.adminSession) || "" + headers.Cookie = JSON.stringify(adminSession) || "" } if (opts.clientSession) { if (opts.clientSession.jwt) { diff --git a/packages/medusa/src/services/__tests__/discount.js b/packages/medusa/src/services/__tests__/discount.js index b820ca7590..48ec11131d 100644 --- a/packages/medusa/src/services/__tests__/discount.js +++ b/packages/medusa/src/services/__tests__/discount.js @@ -9,7 +9,6 @@ const featureFlagRouter = new FlagRouter({}) describe("DiscountService", () => { describe("create", () => { const discountRepository = MockRepository({}) - const discountRuleRepository = MockRepository({}) const regionService = { @@ -54,6 +53,21 @@ describe("DiscountService", () => { } }) + it("fails to create a discount without regions", async () => { + const err = await discountService.create({ + code: "test", + rule: { + type: "fixed", + allocation: "total", + value: 20, + }, + }).catch(e => e) + + expect(err.type).toEqual("invalid_data") + expect(err.message).toEqual("Discount must have atleast 1 region") + expect(discountRepository.create).toHaveBeenCalledTimes(0) + }) + it("successfully creates discount", async () => { await discountService.create({ code: "test", diff --git a/packages/medusa/src/services/discount.ts b/packages/medusa/src/services/discount.ts index 0253c4f70a..f609d24940 100644 --- a/packages/medusa/src/services/discount.ts +++ b/packages/medusa/src/services/discount.ts @@ -212,6 +212,13 @@ class DiscountService extends TransactionBaseService { )) as Region[] } + if (!discount.regions?.length) { + throw new MedusaError( + MedusaError.Types.INVALID_DATA, + "Discount must have atleast 1 region" + ) + } + const discountRule = ruleRepo.create(validatedRule) const createdDiscountRule = await ruleRepo.save(discountRule)