fix(medusa): Deleting product prices (#3152)

.changeset/violet-weeks-repair.md

@@ -0,0 +1,5 @@
+---
+"@medusajs/medusa": patch
+---
+
+fix(medusa): findVariantPricesNotIn now finds obsolete prices to delete

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(
+        categoryWithProduct = await simpleProductCategoryFactory(dbConnection, {
-          dbConnection,
-          {
           id: categoryWithProductId,
           name: "category with Product",
           products: [{ id: testProductId }],
-          }
+        })
-        )
 
         nestedCategoryWithProduct = await simpleProductCategoryFactory(
           dbConnection,
@@ -496,49 +497,42 @@ 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
+        const response = await api.get(
-          .get(
           `/admin/products?${params}`,
           adminHeaders
         )
 
         expect(response.status).toEqual(200)
         expect(response.data.products).toHaveLength(1)
-        expect(response.data.products).toEqual(
+        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
+        const response = await api.get(
-          .get(
           `/admin/products?${params}`,
           adminHeaders
         )
 
         expect(response.status).toEqual(200)
         expect(response.data.products).toHaveLength(1)
-        expect(response.data.products).toEqual(
+        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
+        const response = await api.get(
-          .get(
           `/admin/products?${params}`,
           adminHeaders
         )
@@ -555,7 +549,7 @@ describe("/admin/products", () => {
             }),
             expect.objectContaining({
               id: testProductFilteringId1,
-            })
+            }),
           ])
         )
       })
@@ -564,8 +558,7 @@ describe("/admin/products", () => {
         const api = useApi()
 
         const params = `category_id[]=${categoryWithoutProductId}&include_category_children=true`
-        const response = await api
+        const response = await api.get(
-          .get(
           `/admin/products?${params}`,
           adminHeaders
         )
@@ -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
+        const response = await api.post(
-          .post(`/admin/products/${testProductId}`, payload, adminHeaders)
+          `/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(
+        const category = await simpleProductCategoryFactory(dbConnection, {
-          dbConnection,
-          {
           id: "existing-category",
           name: "existing category",
-            products: [{ id: "test-product" }]
+          products: [{ id: "test-product" }],
-          }
+        })
-        )
 
         const payload = {
           categories: [],
         }
 
-        const response = await api
+        const response = await api.post(
-          .post("/admin/products/test-product", payload, adminHeaders)
+          "/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", () => {

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<MoneyAmount, "created_at" | "updated_at" | "deleted_at">
@@ -23,6 +24,11 @@ type Price = Partial<
 
 @EntityRepository(MoneyAmount)
 export class MoneyAmountRepository extends Repository<MoneyAmount> {
+  /**
+   * 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<MoneyAmount> {
     return pricesNotInPricesPayload
   }
 
+  public async deleteVariantPricesNotIn(
+    variantId: string,
+    prices: Price[]
+  ): Promise<void> {
+    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

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 () => {

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
+      // Delete obsolete prices
-      const obsoletePrices = await moneyAmountRepo.findVariantPricesNotIn(
+      await moneyAmountRepo.deleteVariantPricesNotIn(variantId, prices)
-        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)
     })
   }