diff --git a/.changeset/violet-weeks-repair.md b/.changeset/violet-weeks-repair.md new file mode 100644 index 0000000000..8b8feaf9c0 --- /dev/null +++ b/.changeset/violet-weeks-repair.md @@ -0,0 +1,5 @@ +--- +"@medusajs/medusa": patch +--- + +fix(medusa): findVariantPricesNotIn now finds obsolete prices to delete diff --git a/integration-tests/api/__tests__/admin/product.js b/integration-tests/api/__tests__/admin/product.js index 564e07f6a1..b0e3505002 100644 --- a/integration-tests/api/__tests__/admin/product.js +++ b/integration-tests/api/__tests__/admin/product.js @@ -6,7 +6,7 @@ const { initDb, useDb } = require("../../../helpers/use-db") const adminSeeder = require("../../helpers/admin-seeder") const productSeeder = require("../../helpers/product-seeder") -const { Product, ProductCategory } = require("@medusajs/medusa") +const { ProductCategory } = require("@medusajs/medusa") const { ProductVariant, @@ -20,6 +20,7 @@ const { simpleProductFactory, simpleDiscountFactory, simpleProductCategoryFactory, + simpleRegionFactory, } = require("../../factories") const { DiscountRuleType, AllocationType } = require("@medusajs/medusa/dist") const { IdMap } = require("medusa-test-utils") @@ -447,7 +448,10 @@ describe("/admin/products", () => { }) describe("Product Category filtering", () => { - let categoryWithProduct, categoryWithoutProduct, nestedCategoryWithProduct, nested2CategoryWithProduct + let categoryWithProduct + let categoryWithoutProduct + let nestedCategoryWithProduct + let nested2CategoryWithProduct const nestedCategoryWithProductId = "nested-category-with-product-id" const nested2CategoryWithProductId = "nested2-category-with-product-id" const categoryWithProductId = "category-with-product-id" @@ -455,14 +459,11 @@ describe("/admin/products", () => { beforeEach(async () => { const manager = dbConnection.manager - categoryWithProduct = await simpleProductCategoryFactory( - dbConnection, - { - id: categoryWithProductId, - name: "category with Product", - products: [{ id: testProductId }], - } - ) + categoryWithProduct = await simpleProductCategoryFactory(dbConnection, { + id: categoryWithProductId, + name: "category with Product", + products: [{ id: testProductId }], + }) nestedCategoryWithProduct = await simpleProductCategoryFactory( dbConnection, @@ -496,52 +497,45 @@ describe("/admin/products", () => { it("returns a list of products in product category without category children", async () => { const api = useApi() const params = `category_id[]=${categoryWithProductId}` - const response = await api - .get( - `/admin/products?${params}`, - adminHeaders - ) + const response = await api.get( + `/admin/products?${params}`, + adminHeaders + ) expect(response.status).toEqual(200) expect(response.data.products).toHaveLength(1) - expect(response.data.products).toEqual( - [ - expect.objectContaining({ - id: testProductId, - }), - ] - ) + expect(response.data.products).toEqual([ + expect.objectContaining({ + id: testProductId, + }), + ]) }) it("returns a list of products in product category without category children explicitly set to false", async () => { const api = useApi() const params = `category_id[]=${categoryWithProductId}&include_category_children=false` - const response = await api - .get( - `/admin/products?${params}`, - adminHeaders - ) + const response = await api.get( + `/admin/products?${params}`, + adminHeaders + ) expect(response.status).toEqual(200) expect(response.data.products).toHaveLength(1) - expect(response.data.products).toEqual( - [ - expect.objectContaining({ - id: testProductId, - }), - ] - ) + expect(response.data.products).toEqual([ + expect.objectContaining({ + id: testProductId, + }), + ]) }) it("returns a list of products in product category with category children", async () => { const api = useApi() const params = `category_id[]=${categoryWithProductId}&include_category_children=true` - const response = await api - .get( - `/admin/products?${params}`, - adminHeaders - ) + const response = await api.get( + `/admin/products?${params}`, + adminHeaders + ) expect(response.status).toEqual(200) expect(response.data.products).toHaveLength(3) @@ -555,7 +549,7 @@ describe("/admin/products", () => { }), expect.objectContaining({ id: testProductFilteringId1, - }) + }), ]) ) }) @@ -564,11 +558,10 @@ describe("/admin/products", () => { const api = useApi() const params = `category_id[]=${categoryWithoutProductId}&include_category_children=true` - const response = await api - .get( - `/admin/products?${params}`, - adminHeaders - ) + const response = await api.get( + `/admin/products?${params}`, + adminHeaders + ) expect(response.status).toEqual(200) expect(response.data.products).toHaveLength(0) @@ -1594,7 +1587,8 @@ describe("/admin/products", () => { }) describe("Categories", () => { - let categoryWithProduct, categoryWithoutProduct + let categoryWithProduct + let categoryWithoutProduct const categoryWithProductId = "category-with-product-id" const categoryWithoutProductId = "category-without-product-id" @@ -1620,12 +1614,15 @@ describe("/admin/products", () => { const payload = { title: "Test", description: "test-product-description", - categories: [{ id: categoryWithProductId }, { id: categoryWithoutProductId }] + categories: [ + { id: categoryWithProductId }, + { id: categoryWithoutProductId }, + ], } const response = await api .post("/admin/products", payload, adminHeaders) - .catch(e => e) + .catch((e) => e) expect(response.status).toEqual(200) expect(response.data.product).toEqual( @@ -1649,16 +1646,18 @@ describe("/admin/products", () => { const payload = { title: "Test", description: "test-product-description", - categories: [{ id: categoryNotFoundId }] + categories: [{ id: categoryNotFoundId }], } const error = await api .post("/admin/products", payload, adminHeaders) - .catch(e => e) + .catch((e) => e) expect(error.response.status).toEqual(404) expect(error.response.data.type).toEqual("not_found") - expect(error.response.data.message).toEqual(`Product_category with product_category_id ${categoryNotFoundId} does not exist.`) + expect(error.response.data.message).toEqual( + `Product_category with product_category_id ${categoryNotFoundId} does not exist.` + ) }) it("updates a product's categories", async () => { @@ -1668,8 +1667,11 @@ describe("/admin/products", () => { categories: [{ id: categoryWithoutProductId }], } - const response = await api - .post(`/admin/products/${testProductId}`, payload, adminHeaders) + const response = await api.post( + `/admin/products/${testProductId}`, + payload, + adminHeaders + ) expect(response.status).toEqual(200) expect(response.data.product).toEqual( @@ -1687,21 +1689,21 @@ describe("/admin/products", () => { it("remove all categories of a product", async () => { const api = useApi() - const category = await simpleProductCategoryFactory( - dbConnection, - { - id: "existing-category", - name: "existing category", - products: [{ id: "test-product" }] - } - ) + const category = await simpleProductCategoryFactory(dbConnection, { + id: "existing-category", + name: "existing category", + products: [{ id: "test-product" }], + }) const payload = { categories: [], } - const response = await api - .post("/admin/products/test-product", payload, adminHeaders) + const response = await api.post( + "/admin/products/test-product", + payload, + adminHeaders + ) expect(response.status).toEqual(200) expect(response.data.product).toEqual( @@ -1720,11 +1722,13 @@ describe("/admin/products", () => { const error = await api .post("/admin/products/test-product", payload, adminHeaders) - .catch(e => e) + .catch((e) => e) expect(error.response.status).toEqual(400) expect(error.response.data.type).toEqual("invalid_data") - expect(error.response.data.message).toEqual("property incorrect should not exist, id must be a string") + expect(error.response.data.message).toEqual( + "property incorrect should not exist, id must be a string" + ) }) }) }) @@ -2126,6 +2130,193 @@ describe("/admin/products", () => { ]) ) }) + + it("successfully deletes a region price", async () => { + const api = useApi() + + const createRegionPricePayload = { + prices: [ + { + currency_code: "usd", + amount: 1000, + }, + { + region_id: "test-region", + amount: 8000, + }, + { + currency_code: "eur", + amount: 900, + }, + ], + } + + const variantId = "test-variant_3" + + const createRegionPriceResponse = await api.post( + "/admin/products/test-product1/variants/test-variant_3", + createRegionPricePayload, + adminHeaders + ) + + const initialPriceArray = + createRegionPriceResponse.data.product.variants.find( + (v) => v.id === variantId + ).prices + + expect(createRegionPriceResponse.status).toEqual(200) + expect(initialPriceArray).toHaveLength(3) + expect(initialPriceArray).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + amount: 1000, + currency_code: "usd", + }), + expect.objectContaining({ + amount: 8000, + currency_code: "usd", + region_id: "test-region", + }), + expect.objectContaining({ + amount: 900, + currency_code: "eur", + }), + ]) + ) + + const deleteRegionPricePayload = { + prices: [ + { + currency_code: "usd", + amount: 1000, + }, + { + currency_code: "eur", + amount: 900, + }, + ], + } + + const deleteRegionPriceResponse = await api.post( + "/admin/products/test-product1/variants/test-variant_3", + deleteRegionPricePayload, + adminHeaders + ) + + const finalPriceArray = + deleteRegionPriceResponse.data.product.variants.find( + (v) => v.id === variantId + ).prices + + expect(deleteRegionPriceResponse.status).toEqual(200) + expect(finalPriceArray).toHaveLength(2) + expect(finalPriceArray).toEqual([ + expect.objectContaining({ + amount: 1000, + currency_code: "usd", + }), + expect.objectContaining({ + amount: 900, + currency_code: "eur", + }), + ]) + }) + + it("successfully updates a variants prices by deleting both a currency and region price", async () => { + const api = useApi() + + await Promise.all( + ["reg_1", "reg_2", "reg_3"].map(async (regionId) => { + return await simpleRegionFactory(dbConnection, { + id: regionId, + currency_code: regionId === "reg_1" ? "eur" : "usd", + }) + }) + ) + + const createPrices = { + prices: [ + { + region_id: "reg_1", + amount: 1, + }, + { + region_id: "reg_2", + amount: 2, + }, + { + currency_code: "usd", + amount: 3, + }, + { + region_id: "reg_3", + amount: 4, + }, + { + currency_code: "eur", + amount: 5, + }, + ], + } + + const variantId = "test-variant_3" + + await api + .post( + `/admin/products/test-product1/variants/${variantId}`, + createPrices, + adminHeaders + ) + .catch((err) => { + console.log(err) + }) + + const updatePrices = { + prices: [ + { + region_id: "reg_1", + amount: 100, + }, + { + region_id: "reg_2", + amount: 200, + }, + { + currency_code: "usd", + amount: 300, + }, + ], + } + + const response = await api.post( + `/admin/products/test-product1/variants/${variantId}`, + updatePrices, + adminHeaders + ) + + const finalPriceArray = response.data.product.variants.find( + (v) => v.id === variantId + ).prices + + expect(response.status).toEqual(200) + expect(finalPriceArray).toHaveLength(3) + expect(finalPriceArray).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + amount: 100, + region_id: "reg_1", + }), + expect.objectContaining({ + amount: 200, + region_id: "reg_2", + }), + expect.objectContaining({ + amount: 300, + currency_code: "usd", + }), + ]) + ) + }) }) describe("variant creation", () => { diff --git a/packages/medusa/src/repositories/money-amount.ts b/packages/medusa/src/repositories/money-amount.ts index 72f803b6ee..2906b8cc48 100644 --- a/packages/medusa/src/repositories/money-amount.ts +++ b/packages/medusa/src/repositories/money-amount.ts @@ -5,15 +5,16 @@ import { In, IsNull, Not, + ObjectLiteral, Repository, WhereExpressionBuilder, } from "typeorm" +import { QueryDeepPartialEntity } from "typeorm/query-builder/QueryPartialEntity" import { MoneyAmount } from "../models/money-amount" import { PriceListPriceCreateInput, PriceListPriceUpdateInput, } from "../types/price-list" -import { QueryDeepPartialEntity } from "typeorm/query-builder/QueryPartialEntity" type Price = Partial< Omit @@ -23,6 +24,11 @@ type Price = Partial< @EntityRepository(MoneyAmount) export class MoneyAmountRepository extends Repository { + /** + * Will be removed in a future release. + * Use `deleteVariantPricesNotIn` instead. + * @deprecated + */ public async findVariantPricesNotIn( variantId: string, prices: Price[] @@ -43,6 +49,44 @@ export class MoneyAmountRepository extends Repository { return pricesNotInPricesPayload } + public async deleteVariantPricesNotIn( + variantId: string, + prices: Price[] + ): Promise { + const where = { + variant_id: variantId, + price_list_id: IsNull(), + } + + const orWhere: ObjectLiteral[] = [] + + for (const price of prices) { + if (price.currency_code) { + orWhere.push( + { + currency_code: Not(price.currency_code), + }, + { + region_id: price.region_id ? Not(price.region_id) : Not(IsNull()), + currency_code: price.currency_code, + } + ) + } + + if (price.region_id) { + orWhere.push({ + region_id: Not(price.region_id), + }) + } + } + + await this.createQueryBuilder() + .delete() + .where(where) + .andWhere(orWhere) + .execute() + } + public async upsertVariantCurrencyPrice( variantId: string, price: Price diff --git a/packages/medusa/src/services/__tests__/product-variant.js b/packages/medusa/src/services/__tests__/product-variant.js index 0b834f2e40..389f19bbe8 100644 --- a/packages/medusa/src/services/__tests__/product-variant.js +++ b/packages/medusa/src/services/__tests__/product-variant.js @@ -477,7 +477,7 @@ describe("ProductVariantService", () => { }, ] - moneyAmountRepository.findVariantPricesNotIn = jest + moneyAmountRepository.deleteVariantPricesNotIn = jest .fn() .mockImplementation(() => Promise.resolve(oldPrices)) @@ -641,7 +641,7 @@ describe("ProductVariantService", () => { }, ] - moneyAmountRepository.findVariantPricesNotIn = jest + moneyAmountRepository.deleteVariantPricesNotIn = jest .fn() .mockImplementation(() => Promise.resolve(oldPrices)) @@ -692,7 +692,7 @@ describe("ProductVariantService", () => { ]) expect( - moneyAmountRepository.findVariantPricesNotIn + moneyAmountRepository.deleteVariantPricesNotIn ).toHaveBeenCalledTimes(1) expect( @@ -704,9 +704,6 @@ describe("ProductVariantService", () => { currency_code: "usd", amount: 4000, }) - - expect(moneyAmountRepository.remove).toHaveBeenCalledTimes(1) - expect(moneyAmountRepository.remove).toHaveBeenCalledWith(oldPrices) }) it("successfully creates new a region price", async () => { diff --git a/packages/medusa/src/services/product-variant.ts b/packages/medusa/src/services/product-variant.ts index 1bf5b4392a..d6232d65ee 100644 --- a/packages/medusa/src/services/product-variant.ts +++ b/packages/medusa/src/services/product-variant.ts @@ -19,8 +19,6 @@ import { FindWithRelationsOptions, ProductVariantRepository, } from "../repositories/product-variant" -import EventBusService from "./event-bus" -import RegionService from "./region" import { FindConfig } from "../types/common" import { CreateProductVariantInput, @@ -30,6 +28,8 @@ import { UpdateProductVariantInput, } from "../types/product-variant" import { buildQuery, setMetadata } from "../utils" +import EventBusService from "./event-bus" +import RegionService from "./region" class ProductVariantService extends TransactionBaseService { static Events = { @@ -341,11 +341,8 @@ class ProductVariantService extends TransactionBaseService { this.moneyAmountRepository_ ) - // get prices to be deleted - const obsoletePrices = await moneyAmountRepo.findVariantPricesNotIn( - variantId, - prices - ) + // Delete obsolete prices + await moneyAmountRepo.deleteVariantPricesNotIn(variantId, prices) const regionsServiceTx = this.regionService_.withTransaction(manager) @@ -362,8 +359,6 @@ class ProductVariantService extends TransactionBaseService { await this.setCurrencyPrice(variantId, price) } } - - await moneyAmountRepo.remove(obsoletePrices) }) }