From d4e565b0b92b85818f9e49abc23e80129b215328 Mon Sep 17 00:00:00 2001 From: Riqwan Thamir Date: Tue, 6 Dec 2022 14:41:53 +0100 Subject: [PATCH 1/9] fix: regions are changed from an optional to required field A discount should always be associated to atleast 1 region - we validate it against the region in the cart service. Adding a request parameter validation to pass in atleast one regionID in an array of regions will ensure that regionID will be validated as a part of the discount service and fail if its not a valid region ID. --- .../discounts/__tests__/create-discount.js | 49 +++++++++++++++++++ .../routes/admin/discounts/create-discount.ts | 8 +-- 2 files changed, 54 insertions(+), 3 deletions(-) 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 c858562dd9d2b..fa315a1e442b9 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 @@ -16,6 +16,7 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [IdMap.getId("region-france")], starts_at: "02/02/2021 13:45", ends_at: "03/14/2021 04:30", }, @@ -41,6 +42,7 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [IdMap.getId("region-france")], starts_at: new Date("02/02/2021 13:45"), ends_at: new Date("03/14/2021 04:30"), is_disabled: false, @@ -63,6 +65,7 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [IdMap.getId("region-france")], starts_at: "02/02/2021 13:45", is_dynamic: true, valid_duration: "PaMT2D", @@ -100,6 +103,7 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [IdMap.getId("region-france")], starts_at: "02/02/2021 13:45", is_dynamic: true, valid_duration: "P1Y2M03DT04H05M", @@ -126,6 +130,7 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [IdMap.getId("region-france")], starts_at: new Date("02/02/2021 13:45"), is_disabled: false, is_dynamic: true, @@ -146,6 +151,7 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [IdMap.getId("region-france")], }, adminSession: { jwt: { @@ -186,6 +192,7 @@ describe("POST /admin/discounts", () => { }, ], }, + regions: [IdMap.getId("region-france")], starts_at: "02/02/2021 13:45", is_dynamic: true, valid_duration: "P1Y2M03DT04H05M", @@ -222,6 +229,7 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [IdMap.getId("region-france")], ends_at: "02/02/2021", starts_at: "03/14/2021", }, @@ -259,6 +267,7 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [IdMap.getId("region-france")], starts_at: "03/14/2021 14:30", }, adminSession: { @@ -284,8 +293,48 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, + regions: [IdMap.getId("region-france")], 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: { + jwt: { + userId: IdMap.getId("admin_user"), + }, + }, + }) + }) + + 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 30ede6452b13d..37da3037a2f70 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() From f22e063ed8f47df40b5c59c8fbc413e98eddec25 Mon Sep 17 00:00:00 2001 From: Riqwan Thamir Date: Tue, 6 Dec 2022 14:59:16 +0100 Subject: [PATCH 2/9] test: fix existing integration tests and add a new test that validates regions inclusion and validity --- .../api/__tests__/admin/discount.js | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/integration-tests/api/__tests__/admin/discount.js b/integration-tests/api/__tests__/admin/discount.js index ae695865b7d5f..86732554bdf5f 100644 --- a/integration-tests/api/__tests__/admin/discount.js +++ b/integration-tests/api/__tests__/admin/discount.js @@ -408,6 +408,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -471,6 +472,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -538,6 +540,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -576,6 +579,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: ["test-region"], }, adminReqConfig ) @@ -630,6 +634,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -654,6 +659,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: ["test-region"], }, { headers: { @@ -690,6 +696,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -729,6 +736,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -755,6 +763,7 @@ describe("/admin/discounts", () => { }, ], }, + regions: ["test-region"], }, { headers: { @@ -779,6 +788,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -822,6 +832,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -844,6 +855,7 @@ describe("/admin/discounts", () => { id: response.data.discount.rule.id, type: "free_shipping", }, + regions: ["test-region"], }, adminReqConfig ) @@ -869,6 +881,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -913,6 +926,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -958,6 +972,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -1033,6 +1048,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -1103,6 +1119,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: ["test-region"], usage_limit: 10, starts_at: new Date("09/15/2021 11:50"), ends_at: new Date("09/15/2021 17:50"), @@ -1172,6 +1189,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: ["test-region"], usage_limit: 10, starts_at: new Date("09/15/2021 11:50"), ends_at: new Date("09/15/2021 17:50"), @@ -1229,6 +1247,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: ["test-region"], usage_limit: 10, starts_at: new Date("09/15/2021 11:50"), ends_at: new Date("09/14/2021 17:50"), @@ -1244,6 +1263,55 @@ describe("/admin/discounts", () => { ) }) }) + + it("fails to create a discount if regions array contains an invalid regionID ", async () => { + expect.assertions(2) + const api = useApi() + + const response = await api.post( + "/admin/discounts", + { + code: "HELLOWORLD", + rule: { + description: "test", + type: "percentage", + value: 10, + allocation: "total", + }, + regions: ["test-region", "not-a-valid-region"], + }, + adminReqConfig + ).catch((err) => { + 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 array is not present ", async () => { + expect.assertions(2) + const api = useApi() + + const response = await api.post( + "/admin/discounts", + { + code: "HELLOWORLD", + rule: { + description: "test", + type: "percentage", + value: 10, + allocation: "total", + }, + }, + adminReqConfig + ).catch((err) => { + 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` + ) + }) + }) }) describe("POST /admin/discounts/:id", () => { @@ -1306,7 +1374,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 +1416,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -1374,6 +1446,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig @@ -2033,7 +2106,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", @@ -2139,6 +2214,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, + regions: ["test-region"], usage_limit: 10, }, adminReqConfig From cb2a32a82b2a4d89e57a120262a9eee002ee29a9 Mon Sep 17 00:00:00 2001 From: Riqwan Thamir Date: Tue, 6 Dec 2022 15:03:36 +0100 Subject: [PATCH 3/9] chore: add changeset --- .changeset/tender-coins-kiss.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tender-coins-kiss.md diff --git a/.changeset/tender-coins-kiss.md b/.changeset/tender-coins-kiss.md new file mode 100644 index 0000000000000..63ba03f0aba81 --- /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 From bcc1edbf2bbcd5294823d18a1d1e750ea90c047f Mon Sep 17 00:00:00 2001 From: Riqwan Thamir Date: Tue, 6 Dec 2022 16:19:07 +0100 Subject: [PATCH 4/9] chore: addressing review - move repeated strings and objects to constants --- .../api/__tests__/admin/discount.js | 87 ++++++++++--------- .../discounts/__tests__/create-discount.js | 77 ++++++---------- 2 files changed, 72 insertions(+), 92 deletions(-) diff --git a/integration-tests/api/__tests__/admin/discount.js b/integration-tests/api/__tests__/admin/discount.js index 86732554bdf5f..aa7236715eb95 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,7 +411,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -472,7 +475,7 @@ describe("/admin/discounts", () => { }, ], }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -540,7 +543,7 @@ describe("/admin/discounts", () => { }, ], }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -579,7 +582,7 @@ describe("/admin/discounts", () => { }, ], }, - regions: ["test-region"], + regions: [validRegionId], }, adminReqConfig ) @@ -634,7 +637,7 @@ describe("/admin/discounts", () => { }, ], }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -659,7 +662,7 @@ describe("/admin/discounts", () => { }, ], }, - regions: ["test-region"], + regions: [validRegionId], }, { headers: { @@ -696,7 +699,7 @@ describe("/admin/discounts", () => { }, ], }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -736,7 +739,7 @@ describe("/admin/discounts", () => { }, ], }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -763,7 +766,7 @@ describe("/admin/discounts", () => { }, ], }, - regions: ["test-region"], + regions: [validRegionId], }, { headers: { @@ -788,7 +791,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -832,7 +835,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -855,7 +858,7 @@ describe("/admin/discounts", () => { id: response.data.discount.rule.id, type: "free_shipping", }, - regions: ["test-region"], + regions: [validRegionId], }, adminReqConfig ) @@ -881,7 +884,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -926,7 +929,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -972,7 +975,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -1022,7 +1025,7 @@ describe("/admin/discounts", () => { allocation: "total", }, usage_limit: 10, - regions: ["test-region", "test-region-2"], + regions: [validRegionId, "test-region-2"], }, adminReqConfig ) @@ -1048,7 +1051,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -1058,7 +1061,7 @@ describe("/admin/discounts", () => { .post( `/admin/discounts/${response.data.discount.id}`, { - regions: ["test-region", "test-region-2"], + regions: [validRegionId, "test-region-2"], }, adminReqConfig ) @@ -1086,7 +1089,7 @@ describe("/admin/discounts", () => { allocation: "total", }, usage_limit: 10, - regions: ["test-region"], + regions: [validRegionId], }, adminReqConfig ) @@ -1119,7 +1122,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, starts_at: new Date("09/15/2021 11:50"), ends_at: new Date("09/15/2021 17:50"), @@ -1189,7 +1192,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, starts_at: new Date("09/15/2021 11:50"), ends_at: new Date("09/15/2021 17:50"), @@ -1247,7 +1250,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, starts_at: new Date("09/15/2021 11:50"), ends_at: new Date("09/14/2021 17:50"), @@ -1264,11 +1267,11 @@ describe("/admin/discounts", () => { }) }) - it("fails to create a discount if regions array contains an invalid regionID ", async () => { + it("fails to create a discount if the regions contains an invalid regionId ", async () => { expect.assertions(2) const api = useApi() - const response = await api.post( + const err = await api.post( "/admin/discounts", { code: "HELLOWORLD", @@ -1278,22 +1281,22 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region", "not-a-valid-region"], + regions: [validRegionId, invalidRegionId], }, adminReqConfig - ).catch((err) => { - expect(err.response.status).toEqual(404) - expect(err.response.data.message).toEqual( - `Region with not-a-valid-region was not found` - ) - }) + ).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 array is not present ", async () => { + it("fails to create a discount if regions are not present ", async () => { expect.assertions(2) const api = useApi() - const response = await api.post( + const err = await api.post( "/admin/discounts", { code: "HELLOWORLD", @@ -1305,12 +1308,12 @@ describe("/admin/discounts", () => { }, }, adminReqConfig - ).catch((err) => { - 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` - ) - }) + ).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` + ) }) }) @@ -1416,7 +1419,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -1446,7 +1449,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region"], + regions: [validRegionId], usage_limit: 10, }, adminReqConfig @@ -2214,7 +2217,7 @@ describe("/admin/discounts", () => { value: 10, allocation: "total", }, - regions: ["test-region"], + 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 fa315a1e442b9..ea83bc239e941 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 generateAdminJwtSession = () => ({ + jwt: { + userId: IdMap.getId("admin_user") + } + }) + describe("successful creation", () => { let subject @@ -16,15 +24,11 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, - regions: [IdMap.getId("region-france")], + regions: [validRegionId], starts_at: "02/02/2021 13:45", ends_at: "03/14/2021 04:30", }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession: generateAdminJwtSession(), }) }) @@ -42,7 +46,7 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, - regions: [IdMap.getId("region-france")], + regions: [validRegionId], starts_at: new Date("02/02/2021 13:45"), ends_at: new Date("03/14/2021 04:30"), is_disabled: false, @@ -56,6 +60,7 @@ describe("POST /admin/discounts", () => { beforeAll(async () => { jest.clearAllMocks() + subject = await request("POST", "/admin/discounts", { payload: { code: "TEST", @@ -65,16 +70,12 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, - regions: [IdMap.getId("region-france")], + regions: [validRegionId], starts_at: "02/02/2021 13:45", is_dynamic: true, valid_duration: "PaMT2D", }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession: generateAdminJwtSession(), }) }) @@ -103,16 +104,12 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, - regions: [IdMap.getId("region-france")], + regions: [validRegionId], starts_at: "02/02/2021 13:45", is_dynamic: true, valid_duration: "P1Y2M03DT04H05M", }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession: generateAdminJwtSession(), }) }) @@ -130,7 +127,7 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, - regions: [IdMap.getId("region-france")], + regions: [validRegionId], starts_at: new Date("02/02/2021 13:45"), is_disabled: false, is_dynamic: true, @@ -151,13 +148,9 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, - regions: [IdMap.getId("region-france")], - }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, + regions: [validRegionId], }, + adminSession: generateAdminJwtSession(), }) }) @@ -192,16 +185,12 @@ describe("POST /admin/discounts", () => { }, ], }, - regions: [IdMap.getId("region-france")], + regions: [validRegionId], starts_at: "02/02/2021 13:45", is_dynamic: true, valid_duration: "P1Y2M03DT04H05M", }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession: generateAdminJwtSession(), }) }) @@ -229,15 +218,11 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, - regions: [IdMap.getId("region-france")], + regions: [validRegionId], ends_at: "02/02/2021", starts_at: "03/14/2021", }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession: generateAdminJwtSession(), }) }) @@ -267,14 +252,10 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, - regions: [IdMap.getId("region-france")], + regions: [validRegionId], starts_at: "03/14/2021 14:30", }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession: generateAdminJwtSession(), }) }) @@ -293,7 +274,7 @@ describe("POST /admin/discounts", () => { value: 10, allocation: "total", }, - regions: [IdMap.getId("region-france")], + regions: [validRegionId], starts_at: new Date("03/14/2021 14:30"), }) }) @@ -315,11 +296,7 @@ describe("POST /admin/discounts", () => { allocation: "total", }, }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, + adminSession: generateAdminJwtSession(), }) }) From 66bec2f54db63a857f7b723b3f6a78b463fe7098 Mon Sep 17 00:00:00 2001 From: Riqwan Thamir Date: Tue, 6 Dec 2022 22:14:12 +0100 Subject: [PATCH 5/9] test: add a case when regions only contain invalid region Ids --- .../api/__tests__/admin/discount.js | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/integration-tests/api/__tests__/admin/discount.js b/integration-tests/api/__tests__/admin/discount.js index aa7236715eb95..89aca04aee554 100644 --- a/integration-tests/api/__tests__/admin/discount.js +++ b/integration-tests/api/__tests__/admin/discount.js @@ -1292,6 +1292,31 @@ describe("/admin/discounts", () => { ) }) + it("fails to create a discount if the regions contains only invalid regionIds ", async () => { + expect.assertions(2) + 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 () => { expect.assertions(2) const api = useApi() From 7fcf2b86a9ed44895d4098f6af6985f5c64e4212 Mon Sep 17 00:00:00 2001 From: Riqwan Thamir Date: Wed, 7 Dec 2022 10:23:20 +0100 Subject: [PATCH 6/9] chore: prevent adminSession variable from being mutated in the helpers --- .../discounts/__tests__/create-discount.js | 20 +++++++++---------- packages/medusa/src/helpers/test-request.js | 11 ++++++---- 2 files changed, 17 insertions(+), 14 deletions(-) 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 ea83bc239e941..9d0cbb7964f6f 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 @@ -5,11 +5,11 @@ import { DiscountServiceMock } from "../../../../../services/__mocks__/discount" const validRegionId = IdMap.getId("region-france") describe("POST /admin/discounts", () => { - const generateAdminJwtSession = () => ({ + const adminSession = { jwt: { userId: IdMap.getId("admin_user") } - }) + } describe("successful creation", () => { let subject @@ -28,7 +28,7 @@ describe("POST /admin/discounts", () => { starts_at: "02/02/2021 13:45", ends_at: "03/14/2021 04:30", }, - adminSession: generateAdminJwtSession(), + adminSession, }) }) @@ -75,7 +75,7 @@ describe("POST /admin/discounts", () => { is_dynamic: true, valid_duration: "PaMT2D", }, - adminSession: generateAdminJwtSession(), + adminSession, }) }) @@ -109,7 +109,7 @@ describe("POST /admin/discounts", () => { is_dynamic: true, valid_duration: "P1Y2M03DT04H05M", }, - adminSession: generateAdminJwtSession(), + adminSession, }) }) @@ -150,7 +150,7 @@ describe("POST /admin/discounts", () => { }, regions: [validRegionId], }, - adminSession: generateAdminJwtSession(), + adminSession, }) }) @@ -190,7 +190,7 @@ describe("POST /admin/discounts", () => { is_dynamic: true, valid_duration: "P1Y2M03DT04H05M", }, - adminSession: generateAdminJwtSession(), + adminSession, }) }) @@ -222,7 +222,7 @@ describe("POST /admin/discounts", () => { ends_at: "02/02/2021", starts_at: "03/14/2021", }, - adminSession: generateAdminJwtSession(), + adminSession, }) }) @@ -255,7 +255,7 @@ describe("POST /admin/discounts", () => { regions: [validRegionId], starts_at: "03/14/2021 14:30", }, - adminSession: generateAdminJwtSession(), + adminSession, }) }) @@ -296,7 +296,7 @@ describe("POST /admin/discounts", () => { allocation: "total", }, }, - adminSession: generateAdminJwtSession(), + adminSession, }) }) diff --git a/packages/medusa/src/helpers/test-request.js b/packages/medusa/src/helpers/test-request.js index 8654410abb0b3..c9951f80ad596 100644 --- a/packages/medusa/src/helpers/test-request.js +++ b/packages/medusa/src/helpers/test-request.js @@ -11,6 +11,7 @@ import { moduleHelper } from "../loaders/module" import passportLoader from "../loaders/passport" import servicesLoader from "../loaders/services" import strategiesLoader from "../loaders/strategies" +import { clone } from "lodash" const adminSessionOpts = { cookieName: "session", @@ -87,16 +88,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 = clone(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) { From 40694bd5d13ba9a78815f07c60ed2b4a3439da58 Mon Sep 17 00:00:00 2001 From: Riqwan Thamir Date: Wed, 7 Dec 2022 10:49:40 +0100 Subject: [PATCH 7/9] chore: add a validation in the service layer to conform to 1 region rule --- .../medusa/src/services/__tests__/discount.js | 19 ++++++++++++++++++- packages/medusa/src/services/discount.ts | 7 +++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/medusa/src/services/__tests__/discount.js b/packages/medusa/src/services/__tests__/discount.js index b820ca7590ac1..ce78505e6eb32 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,24 @@ describe("DiscountService", () => { } }) + it("fails to create a discount without regions", async () => { + expect.assertions(3) + try { + await discountService.create({ + code: "test", + rule: { + type: "fixed", + allocation: "total", + value: 20, + }, + }) + } catch (err) { + 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 0253c4f70a459..e37bbb3a50c6f 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 (isEmpty(discount.regions)) { + throw new MedusaError( + MedusaError.Types.INVALID_DATA, + "Discount must have atleast 1 region" + ) + } + const discountRule = ruleRepo.create(validatedRule) const createdDiscountRule = await ruleRepo.save(discountRule) From c1cdb8043607cd420fca9b366babf511f142d534 Mon Sep 17 00:00:00 2001 From: Riqwan Thamir Date: Wed, 7 Dec 2022 16:03:02 +0100 Subject: [PATCH 8/9] chore: use splat instead of lodash to clone, remove unnecessary expectations --- .../api/__tests__/admin/discount.js | 7 ----- packages/medusa/src/helpers/test-request.js | 3 +-- .../medusa/src/services/__tests__/discount.js | 27 +++++++++---------- packages/medusa/src/services/discount.ts | 2 +- 4 files changed, 14 insertions(+), 25 deletions(-) diff --git a/integration-tests/api/__tests__/admin/discount.js b/integration-tests/api/__tests__/admin/discount.js index 89aca04aee554..89d72acca8d38 100644 --- a/integration-tests/api/__tests__/admin/discount.js +++ b/integration-tests/api/__tests__/admin/discount.js @@ -1009,7 +1009,6 @@ describe("/admin/discounts", () => { }) it("fails to create a fixed discount with multiple regions", async () => { - expect.assertions(2) const api = useApi() await api @@ -1038,7 +1037,6 @@ describe("/admin/discounts", () => { }) it("fails to update a fixed discount with multiple regions", async () => { - expect.assertions(2) const api = useApi() const response = await api.post( @@ -1075,7 +1073,6 @@ describe("/admin/discounts", () => { }) 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( @@ -1236,7 +1233,6 @@ describe("/admin/discounts", () => { }) it("fails to create discount with end date before start date", async () => { - expect.assertions(2) const api = useApi() await api @@ -1268,7 +1264,6 @@ describe("/admin/discounts", () => { }) it("fails to create a discount if the regions contains an invalid regionId ", async () => { - expect.assertions(2) const api = useApi() const err = await api.post( @@ -1293,7 +1288,6 @@ describe("/admin/discounts", () => { }) it("fails to create a discount if the regions contains only invalid regionIds ", async () => { - expect.assertions(2) const api = useApi() const err = await api.post( @@ -1318,7 +1312,6 @@ describe("/admin/discounts", () => { }) it("fails to create a discount if regions are not present ", async () => { - expect.assertions(2) const api = useApi() const err = await api.post( diff --git a/packages/medusa/src/helpers/test-request.js b/packages/medusa/src/helpers/test-request.js index c9951f80ad596..cb0ee380031d2 100644 --- a/packages/medusa/src/helpers/test-request.js +++ b/packages/medusa/src/helpers/test-request.js @@ -11,7 +11,6 @@ import { moduleHelper } from "../loaders/module" import passportLoader from "../loaders/passport" import servicesLoader from "../loaders/services" import strategiesLoader from "../loaders/strategies" -import { clone } from "lodash" const adminSessionOpts = { cookieName: "session", @@ -88,7 +87,7 @@ export async function request(method, url, opts = {}) { ) headers.Cookie = headers.Cookie || "" if (opts.adminSession) { - const adminSession = clone(opts.adminSession) + const adminSession = { ...opts.adminSession } if (adminSession.jwt) { adminSession.jwt = jwt.sign( diff --git a/packages/medusa/src/services/__tests__/discount.js b/packages/medusa/src/services/__tests__/discount.js index ce78505e6eb32..48ec11131deaf 100644 --- a/packages/medusa/src/services/__tests__/discount.js +++ b/packages/medusa/src/services/__tests__/discount.js @@ -54,21 +54,18 @@ describe("DiscountService", () => { }) it("fails to create a discount without regions", async () => { - expect.assertions(3) - try { - await discountService.create({ - code: "test", - rule: { - type: "fixed", - allocation: "total", - value: 20, - }, - }) - } catch (err) { - expect(err.type).toEqual("invalid_data") - expect(err.message).toEqual("Discount must have atleast 1 region") - expect(discountRepository.create).toHaveBeenCalledTimes(0) - } + 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 () => { diff --git a/packages/medusa/src/services/discount.ts b/packages/medusa/src/services/discount.ts index e37bbb3a50c6f..f609d24940831 100644 --- a/packages/medusa/src/services/discount.ts +++ b/packages/medusa/src/services/discount.ts @@ -212,7 +212,7 @@ class DiscountService extends TransactionBaseService { )) as Region[] } - if (isEmpty(discount.regions)) { + if (!discount.regions?.length) { throw new MedusaError( MedusaError.Types.INVALID_DATA, "Discount must have atleast 1 region" From 5240972ceffbce9f00f33d61d98ea7de40ef2eff Mon Sep 17 00:00:00 2001 From: Riqwan Thamir Date: Wed, 7 Dec 2022 16:56:49 +0100 Subject: [PATCH 9/9] chore: remove expectations in callbacks --- .../api/__tests__/admin/discount.js | 88 ++++++++----------- 1 file changed, 39 insertions(+), 49 deletions(-) diff --git a/integration-tests/api/__tests__/admin/discount.js b/integration-tests/api/__tests__/admin/discount.js index 89d72acca8d38..bcd2a4ecf48e9 100644 --- a/integration-tests/api/__tests__/admin/discount.js +++ b/integration-tests/api/__tests__/admin/discount.js @@ -899,7 +899,7 @@ describe("/admin/discounts", () => { }) ) - await api + const err = await api .post( `/admin/discounts/${response.data.discount.id}`, { @@ -907,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 () => { @@ -1011,7 +1010,7 @@ describe("/admin/discounts", () => { it("fails to create a fixed discount with multiple regions", async () => { const api = useApi() - await api + const err = await api .post( "/admin/discounts", { @@ -1027,13 +1026,12 @@ describe("/admin/discounts", () => { 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 () => { @@ -1055,21 +1053,19 @@ describe("/admin/discounts", () => { adminReqConfig ) - await api + const err = await api .post( `/admin/discounts/${response.data.discount.id}`, { 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 () => { @@ -1091,19 +1087,17 @@ describe("/admin/discounts", () => { 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 () => { @@ -1235,7 +1229,7 @@ describe("/admin/discounts", () => { it("fails to create discount with end date before start date", async () => { const api = useApi() - await api + const err = await api .post( "/admin/discounts", { @@ -1252,15 +1246,14 @@ describe("/admin/discounts", () => { 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 () => { @@ -2211,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 () => {