From 083ab09361271e48fd39a7d60f2ca3e172c20b23 Mon Sep 17 00:00:00 2001 From: Kasper Fabricius Kristensen <45367945+kasperkristensen@users.noreply.github.com> Date: Mon, 30 May 2022 09:44:53 +0200 Subject: [PATCH] fix(medusa): Prevent discount type updates (#1584) --- .../api/__tests__/admin/discount.js | 157 +++++++++++++++--- .../discounts/__tests__/create-discount.js | 2 +- .../discounts/__tests__/update-discount.js | 60 ------- .../routes/admin/discounts/create-discount.ts | 16 +- .../admin/discounts/get-discount-by-code.ts | 33 +++- .../routes/admin/discounts/update-discount.ts | 24 +-- .../medusa/src/services/__tests__/discount.js | 5 +- packages/medusa/src/services/cart.ts | 2 +- packages/medusa/src/services/discount.ts | 98 ++++++----- packages/medusa/src/types/discount.ts | 10 +- 10 files changed, 245 insertions(+), 162 deletions(-) diff --git a/integration-tests/api/__tests__/admin/discount.js b/integration-tests/api/__tests__/admin/discount.js index bb194bf895..5a56698f7d 100644 --- a/integration-tests/api/__tests__/admin/discount.js +++ b/integration-tests/api/__tests__/admin/discount.js @@ -634,7 +634,6 @@ describe("/admin/discounts", () => { { rule: { id: createdRule.id, - type: createdRule.type, value: createdRule.value, allocation: createdRule.allocation, conditions: [ @@ -657,24 +656,26 @@ describe("/admin/discounts", () => { }) expect(updated.status).toEqual(200) - expect(updated.data.discount.rule.conditions).toEqual(expect.arrayContaining([ - expect.objectContaining({ - type: "products", - operator: "not_in", - products: expect.arrayContaining([ - expect.objectContaining({ - id: product.id, - }), - expect.objectContaining({ - id: anotherProduct.id, - }), - ]), - }), - expect.objectContaining({ - type: "product_types", - operator: "not_in", - }), - ])) + expect(updated.data.discount.rule.conditions).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + type: "products", + operator: "not_in", + products: expect.arrayContaining([ + expect.objectContaining({ + id: product.id, + }), + expect.objectContaining({ + id: anotherProduct.id, + }), + ]), + }), + expect.objectContaining({ + type: "product_types", + operator: "not_in", + }), + ]) + ) }) it("fails to add condition on rule with existing comb. of type and operator", async () => { @@ -728,9 +729,7 @@ describe("/admin/discounts", () => { { rule: { id: createdRule.id, - type: createdRule.type, value: createdRule.value, - allocation: createdRule.allocation, conditions: [ { products: [anotherProduct.id], @@ -841,7 +840,6 @@ describe("/admin/discounts", () => { { rule: { id: createdRule.id, - type: createdRule.type, value: createdRule.value, allocation: createdRule.allocation, conditions: [ @@ -922,6 +920,121 @@ describe("/admin/discounts", () => { ) }) + it("creates a discount and fails to update it because attempting type 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, + }) + ) + + await api + .post( + `/admin/discounts/${response.data.discount.id}`, + { + usage_limit: 20, + rule: { + id: response.data.discount.rule.id, + type: "free_shipping", + }, + }, + { + headers: { + Authorization: "Bearer test_token", + }, + } + ) + .catch((err) => { + expect(err.response.status).toEqual(400) + expect(err.response.data.message).toEqual( + "property type should not exist" + ) + }) + }) + + it("creates a discount and fails to update it because attempting is_dynamic update", async () => { + const api = useApi() + + const response = await api + .post( + "/admin/discounts", + { + code: "HELLOWORLD", + 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", + usage_limit: 10, + is_dynamic: true, + }) + ) + + await api + .post( + `/admin/discounts/${response.data.discount.id}`, + { + usage_limit: 20, + is_dynamic: false, + }, + { + headers: { + Authorization: "Bearer test_token", + }, + } + ) + .catch((err) => { + 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 () => { const api = useApi() 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 b9ed3fa9f8..c858562dd9 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 @@ -161,7 +161,7 @@ describe("POST /admin/discounts", () => { it("returns error", () => { expect(subject.body.message).toEqual( - `type should not be empty, type must be a string` + `Invalid rule type, must be one of "fixed", "percentage" or "free_shipping"` ) }) }) diff --git a/packages/medusa/src/api/routes/admin/discounts/__tests__/update-discount.js b/packages/medusa/src/api/routes/admin/discounts/__tests__/update-discount.js index cb6be66a97..9a04ecd9ae 100644 --- a/packages/medusa/src/api/routes/admin/discounts/__tests__/update-discount.js +++ b/packages/medusa/src/api/routes/admin/discounts/__tests__/update-discount.js @@ -16,7 +16,6 @@ describe("POST /admin/discounts", () => { code: "10TOTALOFF", rule: { id: "1234", - type: "fixed", value: 10, allocation: "total", }, @@ -42,7 +41,6 @@ describe("POST /admin/discounts", () => { code: "10TOTALOFF", rule: { id: "1234", - type: "fixed", value: 10, allocation: "total", }, @@ -64,12 +62,9 @@ describe("POST /admin/discounts", () => { code: "10TOTALOFF", rule: { id: "1234", - type: "fixed", value: 10, - allocation: "total", }, starts_at: "02/02/2021 13:45", - is_dynamic: true, valid_duration: "PaMT2D", }, adminSession: { @@ -92,60 +87,6 @@ describe("POST /admin/discounts", () => { }) }) - describe("successful update with dynamic discount", () => { - let subject - - beforeAll(async () => { - jest.clearAllMocks() - subject = await request( - "POST", - `/admin/discounts/${IdMap.getId("total10")}`, - { - payload: { - code: "10TOTALOFF", - rule: { - id: "1234", - type: "fixed", - value: 10, - allocation: "total", - }, - starts_at: "02/02/2021 13:45", - is_dynamic: true, - valid_duration: "P1Y2M03DT04H05M", - }, - adminSession: { - jwt: { - userId: IdMap.getId("admin_user"), - }, - }, - } - ) - }) - - it("returns 200", () => { - expect(subject.status).toEqual(200) - }) - - it("calls service update", () => { - expect(DiscountServiceMock.update).toHaveBeenCalledTimes(1) - expect(DiscountServiceMock.update).toHaveBeenCalledWith( - IdMap.getId("total10"), - { - code: "10TOTALOFF", - rule: { - id: "1234", - type: "fixed", - value: 10, - allocation: "total", - }, - starts_at: new Date("02/02/2021 13:45"), - is_dynamic: true, - valid_duration: "P1Y2M03DT04H05M", - } - ) - }) - }) - describe("fails on invalid date intervals", () => { let subject @@ -159,7 +100,6 @@ describe("POST /admin/discounts", () => { code: "10TOTALOFF", rule: { id: "1234", - type: "fixed", value: 10, allocation: "total", }, 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 9adaf530c0..c8ae3d1f34 100644 --- a/packages/medusa/src/api/routes/admin/discounts/create-discount.ts +++ b/packages/medusa/src/api/routes/admin/discounts/create-discount.ts @@ -3,6 +3,7 @@ import { IsArray, IsBoolean, IsDate, + IsEnum, IsNotEmpty, IsNumber, IsObject, @@ -12,6 +13,7 @@ import { ValidateNested, } from "class-validator" import { defaultAdminDiscountsFields, defaultAdminDiscountsRelations } from "." +import { AllocationType, DiscountRuleType } from "../../../../models" import { Discount } from "../../../../models/discount" import { DiscountConditionOperator } from "../../../../models/discount-condition" import DiscountService from "../../../../services/discount" @@ -157,16 +159,18 @@ export class AdminPostDiscountsDiscountRule { @IsOptional() description?: string - @IsString() - @IsNotEmpty() - type: string + @IsEnum(DiscountRuleType, { + message: `Invalid rule type, must be one of "fixed", "percentage" or "free_shipping"`, + }) + type: DiscountRuleType @IsNumber() value: number - @IsString() - @IsNotEmpty() - allocation: string + @IsEnum(AllocationType, { + message: `Invalid allocation type, must be one of "total" or "item"`, + }) + allocation: AllocationType @IsOptional() @IsArray() diff --git a/packages/medusa/src/api/routes/admin/discounts/get-discount-by-code.ts b/packages/medusa/src/api/routes/admin/discounts/get-discount-by-code.ts index 9675834aae..98a9894077 100644 --- a/packages/medusa/src/api/routes/admin/discounts/get-discount-by-code.ts +++ b/packages/medusa/src/api/routes/admin/discounts/get-discount-by-code.ts @@ -1,5 +1,9 @@ -import { defaultAdminDiscountsRelations } from "." +import { IsOptional, IsString } from "class-validator" +import { defaultAdminDiscountsFields, defaultAdminDiscountsRelations } from "." +import { Discount } from "../../../../models" import DiscountService from "../../../../services/discount" +import { getRetrieveConfig } from "../../../../utils/get-query-config" +import { validator } from "../../../../utils/validator" /** * @oas [get] /discounts/code/{code} * operationId: "GetDiscountsDiscountCode" @@ -23,11 +27,30 @@ import DiscountService from "../../../../services/discount" export default async (req, res) => { const { code } = req.params - const discountService: DiscountService = req.scope.resolve("discountService") - const discount = await discountService.retrieveByCode( - code, - defaultAdminDiscountsRelations + const validated = await validator( + AdminGetDiscountsDiscountCodeParams, + req.query ) + const config = getRetrieveConfig( + defaultAdminDiscountsFields, + defaultAdminDiscountsRelations, + validated?.fields?.split(",") as (keyof Discount)[], + validated?.expand?.split(",") + ) + + const discountService: DiscountService = req.scope.resolve("discountService") + const discount = await discountService.retrieveByCode(code, config) + res.status(200).json({ discount }) } + +export class AdminGetDiscountsDiscountCodeParams { + @IsOptional() + @IsString() + expand?: string + + @IsOptional() + @IsString() + fields?: string +} diff --git a/packages/medusa/src/api/routes/admin/discounts/update-discount.ts b/packages/medusa/src/api/routes/admin/discounts/update-discount.ts index 8cdf89282c..5d1e6677e3 100644 --- a/packages/medusa/src/api/routes/admin/discounts/update-discount.ts +++ b/packages/medusa/src/api/routes/admin/discounts/update-discount.ts @@ -3,6 +3,7 @@ import { IsArray, IsBoolean, IsDate, + IsEnum, IsNotEmpty, IsNumber, IsObject, @@ -12,6 +13,7 @@ import { ValidateNested, } from "class-validator" import { defaultAdminDiscountsFields, defaultAdminDiscountsRelations } from "." +import { AllocationType } from "../../../../models" import { Discount } from "../../../../models/discount" import { DiscountConditionOperator } from "../../../../models/discount-condition" import DiscountService from "../../../../services/discount" @@ -37,9 +39,6 @@ import { IsISO8601Duration } from "../../../../utils/validators/iso8601-duration * code: * type: string * description: A unique code that will be used to redeem the Discount - * is_dynamic: - * type: string - * description: Whether the Discount should have multiple instances of itself, each with a different code. This can be useful for automatically generated codes that all have to follow a common set of rules. * rule: * description: The Discount Rule that defines how Discounts are calculated * oneOf: @@ -109,10 +108,6 @@ export class AdminPostDiscountsDiscountReq { @Type(() => AdminUpdateDiscountRule) rule?: AdminUpdateDiscountRule - @IsBoolean() - @IsOptional() - is_dynamic?: boolean - @IsBoolean() @IsOptional() is_disabled?: boolean @@ -156,16 +151,15 @@ export class AdminUpdateDiscountRule { @IsOptional() description?: string - @IsString() - @IsNotEmpty() - type: string - @IsNumber() - value: number + @IsOptional() + value?: number - @IsString() - @IsNotEmpty() - allocation: string + @IsOptional() + @IsEnum(AllocationType, { + message: `Invalid allocation type, must be one of "total" or "item"`, + }) + allocation?: AllocationType @IsOptional() @IsArray() diff --git a/packages/medusa/src/services/__tests__/discount.js b/packages/medusa/src/services/__tests__/discount.js index 0e87887819..733439d430 100644 --- a/packages/medusa/src/services/__tests__/discount.js +++ b/packages/medusa/src/services/__tests__/discount.js @@ -217,7 +217,6 @@ describe("DiscountService", () => { code: "10%OFF", is_dynamic: false, }, - relations: [], }) }) }) @@ -232,7 +231,9 @@ describe("DiscountService", () => { }), }) - const discountRuleRepository = MockRepository({}) + const discountRuleRepository = MockRepository({ + create: (values) => values, + }) const regionService = { retrieve: () => { diff --git a/packages/medusa/src/services/cart.ts b/packages/medusa/src/services/cart.ts index 780e7e5e43..e05e2b6310 100644 --- a/packages/medusa/src/services/cart.ts +++ b/packages/medusa/src/services/cart.ts @@ -1046,7 +1046,7 @@ class CartService extends TransactionBaseService { async (transactionManager: EntityManager) => { const discount = await this.discountService_ .withTransaction(transactionManager) - .retrieveByCode(discountCode, ["rule", "regions"]) + .retrieveByCode(discountCode, { relations: ["rule", "regions"] }) await this.discountService_ .withTransaction(transactionManager) diff --git a/packages/medusa/src/services/discount.ts b/packages/medusa/src/services/discount.ts index 04af4a5c80..61906f66ee 100644 --- a/packages/medusa/src/services/discount.ts +++ b/packages/medusa/src/services/discount.ts @@ -1,8 +1,14 @@ import { parse, toSeconds } from "iso8601-duration" import { isEmpty, omit } from "lodash" -import { MedusaError, Validator } from "medusa-core-utils" +import { MedusaError } from "medusa-core-utils" import { BaseService } from "medusa-interfaces" -import { Brackets, EntityManager, ILike, SelectQueryBuilder } from "typeorm" +import { + Brackets, + DeepPartial, + EntityManager, + ILike, + SelectQueryBuilder, +} from "typeorm" import { EventBusService, ProductService, @@ -24,9 +30,11 @@ import { GiftCardRepository } from "../repositories/gift-card" import { FindConfig } from "../types/common" import { CreateDiscountInput, + CreateDiscountRuleInput, CreateDynamicDiscountInput, FilterableDiscountProps, UpdateDiscountInput, + UpdateDiscountRuleInput, } from "../types/discount" import { isFuture, isPast } from "../utils/date-helpers" import { formatException } from "../utils/exception-formatter" @@ -127,35 +135,17 @@ class DiscountService extends BaseService { * @param {DiscountRule} discountRule - the discount rule to create * @return {Promise} the result of the create operation */ - validateDiscountRule_(discountRule): DiscountRule { - const schema = Validator.object().keys({ - id: Validator.string().optional(), - description: Validator.string().optional(), - type: Validator.string().required(), - value: Validator.number().min(0).required(), - allocation: Validator.string().required(), - created_at: Validator.date().optional(), - updated_at: Validator.date().allow(null).optional(), - deleted_at: Validator.date().allow(null).optional(), - metadata: Validator.object().allow(null).optional(), - }) - - const { value, error } = schema.validate(discountRule) - if (error) { - throw new MedusaError( - MedusaError.Types.INVALID_DATA, - error.details[0].message - ) - } - - if (value.type === "percentage" && value.value > 100) { + validateDiscountRule_( + discountRule: T + ): T { + if (discountRule.type === "percentage" && discountRule.value > 100) { throw new MedusaError( MedusaError.Types.INVALID_DATA, "Discount value above 100 is not allowed when type is percentage" ) } - return value + return discountRule } /** @@ -228,16 +218,15 @@ class DiscountService extends BaseService { * @return {Promise} the result of the create operation */ async create(discount: CreateDiscountInput): Promise { - return this.atomicPhase_(async (manager) => { + return this.atomicPhase_(async (manager: EntityManager) => { const discountRepo = manager.getCustomRepository(this.discountRepository_) const ruleRepo = manager.getCustomRepository(this.discountRuleRepository_) const conditions = discount.rule?.conditions const ruleToCreate = omit(discount.rule, ["conditions"]) - discount.rule = ruleToCreate - - const validatedRule = this.validateDiscountRule_(discount.rule) + const validatedRule: Omit = + this.validateDiscountRule_(ruleToCreate) if ( discount?.regions && @@ -258,13 +247,16 @@ class DiscountService extends BaseService { ) } - const discountRule = await ruleRepo.create(validatedRule) + const discountRule = ruleRepo.create(validatedRule) const createdDiscountRule = await ruleRepo.save(discountRule) discount.code = discount.code!.toUpperCase() - discount.rule = createdDiscountRule - const created = await discountRepo.create(discount) + const created: Discount = discountRepo.create( + discount as DeepPartial + ) + created.rule = createdDiscountRule + const result = await discountRepo.save(created) if (conditions?.length) { @@ -315,27 +307,26 @@ class DiscountService extends BaseService { /** * Gets a discount by discount code. * @param {string} discountCode - discount code of discount to retrieve - * @param {array} relations - list of relations + * @param {Object} config - the config object containing query settings * @return {Promise} the discount document */ async retrieveByCode( discountCode: string, - relations: string[] = [] + config: FindConfig = {} ): Promise { const discountRepo = this.manager_.getCustomRepository( this.discountRepository_ ) - let discount = await discountRepo.findOne({ - where: { code: discountCode.toUpperCase(), is_dynamic: false }, - relations, - }) + let query = this.buildQuery_( + { code: discountCode, is_dynamic: false }, + config + ) + let discount = await discountRepo.findOne(query) if (!discount) { - discount = await discountRepo.findOne({ - where: { code: discountCode.toUpperCase(), is_dynamic: true }, - relations, - }) + query = this.buildQuery_({ code: discountCode, is_dynamic: true }, config) + discount = await discountRepo.findOne(query) if (!discount) { throw new MedusaError( @@ -359,7 +350,12 @@ class DiscountService extends BaseService { update: UpdateDiscountInput ): Promise { return this.atomicPhase_(async (manager) => { - const discountRepo = manager.getCustomRepository(this.discountRepository_) + const discountRepo: DiscountRepository = manager.getCustomRepository( + this.discountRepository_ + ) + const ruleRepo: DiscountRuleRepository = manager.getCustomRepository( + this.discountRuleRepository_ + ) const discount = await this.retrieve(discountId, { relations: ["rule"], @@ -411,7 +407,21 @@ class DiscountService extends BaseService { } if (rule) { - discount.rule = this.validateDiscountRule_(ruleToUpdate) + const ruleUpdate: Omit = rule + + if (rule.value) { + this.validateDiscountRule_({ + value: rule.value, + type: discount.rule.type, + }) + } + + const updatedRule = ruleRepo.create({ + ...discount.rule, + ...ruleUpdate, + }) + + discount.rule = updatedRule } for (const key of Object.keys(rest).filter( diff --git a/packages/medusa/src/types/discount.ts b/packages/medusa/src/types/discount.ts index 388721eb96..f835e0ff1d 100644 --- a/packages/medusa/src/types/discount.ts +++ b/packages/medusa/src/types/discount.ts @@ -117,9 +117,9 @@ export type UpsertDiscountConditionInput = { export type CreateDiscountRuleInput = { description?: string - type: string + type: DiscountRuleType value: number - allocation: string + allocation: AllocationType conditions?: UpsertDiscountConditionInput[] } @@ -139,16 +139,14 @@ export type CreateDiscountInput = { export type UpdateDiscountRuleInput = { id: string description?: string - type: string - value: number - allocation: string + value?: number + allocation?: AllocationType conditions?: UpsertDiscountConditionInput[] } export type UpdateDiscountInput = { code?: string rule?: UpdateDiscountRuleInput - is_dynamic?: boolean is_disabled?: boolean starts_at?: Date ends_at?: Date | null