fix: shipping option updates (#426)

* fix to remove req

* tested fix

packages/medusa/src/migrations/1632828114899-delete_date_on_shipping_option_requirements.ts

@@ -0,0 +1,18 @@
+import { MigrationInterface, QueryRunner } from "typeorm"
+
+export class deleteDateOnShippingOptionRequirements1632828114899
+  implements MigrationInterface {
+  name = "deleteDateOnShippingOptionRequirements1632828114899"
+
+  public async up(queryRunner: QueryRunner): Promise<void> {
+    await queryRunner.query(
+      `ALTER TABLE "shipping_option_requirement" ADD "deleted_at" TIMESTAMP WITH TIME ZONE`
+    )
+  }
+
+  public async down(queryRunner: QueryRunner): Promise<void> {
+    await queryRunner.query(
+      `ALTER TABLE "shipping_option_requirement" DROP COLUMN "deleted_at"`
+    )
+  }
+}

packages/medusa/src/models/shipping-option-requirement.ts

@@ -16,7 +16,7 @@ import {
   JoinTable,
 } from "typeorm"
 import { ulid } from "ulid"
-import { DbAwareColumn } from "../utils/db-aware-column"
+import { DbAwareColumn, resolveDbType } from "../utils/db-aware-column"
 
 import { ShippingOption } from "./shipping-option"
 
@@ -44,6 +44,9 @@ export class ShippingOptionRequirement {
   @Column({ type: "int" })
   amount: number
 
+  @DeleteDateColumn({ type: resolveDbType("timestamptz") })
+  deleted_at: Date
+
   @BeforeInsert()
   private beforeInsert() {
     if (this.id) return

packages/medusa/src/services/__tests__/shipping-option.js

@@ -296,24 +296,19 @@ describe("ShippingOptionService", () => {
   })
 
   describe("removeRequirement", () => {
-    const shippingOptionRepository = MockRepository({
-      findOne: q => {
-        switch (q.where.id) {
-          default:
-            return Promise.resolve({
-              requirements: [
-                {
-                  id: IdMap.getId("requirement_id"),
-                },
-              ],
-            })
-        }
+    const shippingOptionRequirementRepository = MockRepository({
+      softRemove: q => {
+        return Promise.resolve()
       },
+      findOne: i =>
+        i.where.id === IdMap.getId("requirement_id")
+          ? { id: IdMap.getId("requirement_id") }
+          : null,
     })
 
     const optionService = new ShippingOptionService({
       manager: MockManager,
-      shippingOptionRepository,
+      shippingOptionRequirementRepository,
     })
 
     beforeEach(() => {
@@ -321,22 +316,19 @@ describe("ShippingOptionService", () => {
     })
 
     it("remove requirement successfully", async () => {
-      await optionService.removeRequirement(
-        IdMap.getId("validId"),
-        IdMap.getId("requirement_id")
-      )
+      await optionService.removeRequirement(IdMap.getId("requirement_id"))
 
-      expect(shippingOptionRepository.save).toBeCalledTimes(1)
-      expect(shippingOptionRepository.save).toBeCalledWith({ requirements: [] })
+      expect(shippingOptionRequirementRepository.findOne).toBeCalledTimes(1)
+      expect(shippingOptionRequirementRepository.findOne).toBeCalledWith({
+        where: { id: IdMap.getId("requirement_id") },
+      })
+      expect(shippingOptionRequirementRepository.softRemove).toBeCalledTimes(1)
     })
 
     it("is idempotent", async () => {
       await optionService.removeRequirement(IdMap.getId("validId"), "something")
 
-      expect(shippingOptionRepository.save).toBeCalledTimes(1)
-      expect(shippingOptionRepository.save).toBeCalledWith({
-        requirements: [{ id: IdMap.getId("requirement_id") }],
-      })
+      expect(shippingOptionRequirementRepository.softRemove).toBeCalledTimes(1)
     })
   })
 

packages/medusa/src/services/shipping-option.js

@@ -450,7 +450,9 @@ class ShippingOptionService extends BaseService {
    */
   async update(optionId, update) {
     return this.atomicPhase_(async manager => {
-      const option = await this.retrieve(optionId)
+      const option = await this.retrieve(optionId, {
+        relations: ["requirements"],
+      })
 
       if ("metadata" in update) {
         option.metadata = await this.setMetadata_(option, update.metadata)
@@ -498,6 +500,20 @@ class ShippingOptionService extends BaseService {
 
           acc.push(validated)
         }
+
+        if (option.requirements) {
+          const accReqs = acc.map(a => a.id)
+          const toRemove = option.requirements.filter(
+            r => !accReqs.includes(r.id)
+          )
+          await Promise.all(
+            toRemove.map(async req => {
+              await this.removeRequirement(req.id)
+            })
+          )
+        }
+
+        option.requirements = acc
       }
 
       if ("price_type" in update) {
@@ -585,28 +601,24 @@ class ShippingOptionService extends BaseService {
 
   /**
    * Removes a requirement from a shipping option
-   * @param {string} optionId - the shipping option to remove from
    * @param {string} requirementId - the id of the requirement to remove
    * @return {Promise} the result of update
    */
-  async removeRequirement(optionId, requirementId) {
+  async removeRequirement(requirementId) {
     return this.atomicPhase_(async manager => {
-      const option = await this.retrieve(optionId, {
-        relations: "requirements",
-      })
-      const newReqs = option.requirements.map(r => {
-        if (r.id === requirementId) {
-          return null
-        } else {
-          return r
-        }
-      })
+      try {
+        const reqRepo = manager.getCustomRepository(this.requirementRepository_)
+        const requirement = await reqRepo.findOne({
+          where: { id: requirementId },
+        })
 
-      option.requirements = newReqs.filter(Boolean)
+        const result = await reqRepo.softRemove(requirement)
 
-      const optionRepo = manager.getCustomRepository(this.optionRepository_)
-      const result = await optionRepo.save(option)
-      return result
+        return result
+      } catch (error) {
+        // Delete is idempotent, but we return a promise to allow then-chaining
+        return Promise.resolve()
+      }
     })
   }