fix: make shipping_option_id on requirements optional (#340)

* changed validator so that reqiurement shipping_option_id is now optional + added integration test that confirms that when an update contains a requirement without an ID it is created

* fix: formatting

* fix: un-bump babel-preset-medusa-package

* chore: update yarn.lock

* fix: implemented suggested changes, need to validate behaviour on clean branch so NOT ready for merging just yet

* fix: implemented suggested changes, need to validate behaviour on clean branch so NOT ready for merging just yet

* afix: made it impossible to set a min. subtotal requirement that is greater than max. subtotal

* fix: added explanation to error

* fix: Error when removing requirement on update

Co-authored-by: olivermrbl <oliver@mrbltech.com>

integration-tests/api/__tests__/admin/shipping-options.js

@@ -0,0 +1,301 @@
+const path = require("path");
+const {
+  Region,
+  ShippingProfile,
+  ShippingOption,
+  ShippingOptionRequirement,
+} = require("@medusajs/medusa");
+
+const setupServer = require("../../../helpers/setup-server");
+const { useApi } = require("../../../helpers/use-api");
+const { initDb, useDb } = require("../../../helpers/use-db");
+const adminSeeder = require("../../helpers/admin-seeder");
+
+jest.setTimeout(30000);
+
+describe("/admin/shipping-options", () => {
+  let medusaProcess;
+  let dbConnection;
+
+  beforeAll(async () => {
+    const cwd = path.resolve(path.join(__dirname, "..", ".."));
+    dbConnection = await initDb({ cwd });
+    medusaProcess = await setupServer({ cwd });
+  });
+
+  afterAll(async () => {
+    const db = useDb();
+    await db.shutdown();
+    medusaProcess.kill();
+  });
+
+  describe("POST /admin/shipping-options", () => {
+    beforeEach(async () => {
+      const manager = dbConnection.manager;
+
+      try {
+        await adminSeeder(dbConnection);
+
+        await manager.insert(Region, {
+          id: "region",
+          name: "Test Region",
+          currency_code: "usd",
+          tax_rate: 0,
+        });
+
+        const defaultProfile = await manager.findOne(ShippingProfile, {
+          type: "default",
+        });
+
+        await manager.insert(ShippingOption, {
+          id: "test-out",
+          name: "Test out",
+          profile_id: defaultProfile.id,
+          region_id: "region",
+          provider_id: "test-ful",
+          data: {},
+          price_type: "flat_rate",
+          amount: 2000,
+          is_return: false,
+        });
+
+        await manager.insert(ShippingOption, {
+          id: "test-option-req",
+          name: "With req",
+          profile_id: defaultProfile.id,
+          region_id: "region",
+          provider_id: "test-ful",
+          data: {},
+          price_type: "flat_rate",
+          amount: 2000,
+          is_return: false,
+        });
+
+        await manager.insert(ShippingOptionRequirement, {
+          id: "option-req",
+          shipping_option_id: "test-option-req",
+          type: "min_subtotal",
+          amount: 5,
+        });
+
+        await manager.insert(ShippingOptionRequirement, {
+          id: "option-req-2",
+          shipping_option_id: "test-option-req",
+          type: "max_subtotal",
+          amount: 10,
+        });
+      } catch (err) {
+        console.error(err);
+        throw err;
+      }
+    });
+
+    afterEach(async () => {
+      const db = useDb();
+      await db.teardown();
+    });
+
+    it("updates a shipping option with no existing requirements", async () => {
+      const api = useApi();
+
+      const payload = {
+        name: "Test option",
+        amount: 100,
+        requirements: [
+          {
+            type: "min_subtotal",
+            amount: 1,
+          },
+          {
+            type: "max_subtotal",
+            amount: 2,
+          },
+        ],
+      };
+
+      const res = await api.post(`/admin/shipping-options/test-out`, payload, {
+        headers: {
+          Authorization: "Bearer test_token",
+        },
+      });
+
+      const requirements = res.data.shipping_option.requirements;
+
+      expect(res.status).toEqual(200);
+      expect(requirements.length).toEqual(2);
+      expect(requirements[0]).toEqual(
+        expect.objectContaining({
+          type: "min_subtotal",
+          shipping_option_id: "test-out",
+          amount: 1,
+        })
+      );
+      expect(requirements[1]).toEqual(
+        expect.objectContaining({
+          type: "max_subtotal",
+          shipping_option_id: "test-out",
+          amount: 2,
+        })
+      );
+    });
+
+    it("fails as it is not allowed to set id from client side", async () => {
+      const api = useApi();
+
+      const payload = {
+        name: "Test option",
+        amount: 100,
+        requirements: [
+          {
+            id: "not_allowed",
+            type: "min_subtotal",
+            amount: 1,
+          },
+          {
+            id: "really_not_allowed",
+            type: "max_subtotal",
+            amount: 2,
+          },
+        ],
+      };
+
+      const res = await api
+        .post(`/admin/shipping-options/test-out`, payload, {
+          headers: {
+            Authorization: "Bearer test_token",
+          },
+        })
+        .catch((err) => {
+          return err.response;
+        });
+
+      expect(res.status).toEqual(400);
+      expect(res.data.message).toEqual("ID does not exist");
+    });
+
+    it("it succesfully updates a set of existing requirements", async () => {
+      const api = useApi();
+
+      const payload = {
+        requirements: [
+          {
+            id: "option-req",
+            type: "min_subtotal",
+            amount: 15,
+          },
+          {
+            id: "option-req-2",
+            type: "max_subtotal",
+            amount: 20,
+          },
+        ],
+        amount: 200,
+      };
+
+      const res = await api
+        .post(`/admin/shipping-options/test-option-req`, payload, {
+          headers: {
+            Authorization: "Bearer test_token",
+          },
+        })
+        .catch((err) => {
+          console.log(err.response.data.message);
+        });
+
+      expect(res.status).toEqual(200);
+    });
+
+    it("it succesfully updates a set of existing requirements by updating one and deleting the other", async () => {
+      const api = useApi();
+
+      const payload = {
+        requirements: [
+          {
+            id: "option-req",
+            type: "min_subtotal",
+            amount: 15,
+          },
+        ],
+      };
+
+      const res = await api
+        .post(`/admin/shipping-options/test-option-req`, payload, {
+          headers: {
+            Authorization: "Bearer test_token",
+          },
+        })
+        .catch((err) => {
+          console.log(err.response.data.message);
+        });
+
+      expect(res.status).toEqual(200);
+    });
+
+    it("succesfully updates a set of requirements because max. subtotal >= min. subtotal", async () => {
+      const api = useApi();
+
+      const payload = {
+        requirements: [
+          {
+            id: "option-req",
+            type: "min_subtotal",
+            amount: 150,
+          },
+          {
+            id: "option-req-2",
+            type: "max_subtotal",
+            amount: 200,
+          },
+        ],
+      };
+
+      const res = await api
+        .post(`/admin/shipping-options/test-option-req`, payload, {
+          headers: {
+            Authorization: "Bearer test_token",
+          },
+        })
+        .catch((err) => {
+          console.log(err.response.data.message);
+        });
+
+      expect(res.status).toEqual(200);
+      expect(res.data.shipping_option.requirements[0].amount).toEqual(150);
+      expect(res.data.shipping_option.requirements[1].amount).toEqual(200);
+    });
+
+    it("fails to updates a set of requirements because max. subtotal <= min. subtotal", async () => {
+      const api = useApi();
+
+      const payload = {
+        requirements: [
+          {
+            id: "option-req",
+            type: "min_subtotal",
+            amount: 1500,
+          },
+          {
+            id: "option-req-2",
+            type: "max_subtotal",
+            amount: 200,
+          },
+        ],
+      };
+
+      const res = await api
+        .post(`/admin/shipping-options/test-option-req`, payload, {
+          headers: {
+            Authorization: "Bearer test_token",
+          },
+        })
+        .catch((err) => {
+          return err.response;
+        });
+
+      expect(res.status).toEqual(400);
+      expect(res.data.message).toEqual(
+        "Max. subtotal must be greater than Min. subtotal"
+      );
+    });
+  });
+});

integration-tests/api/package.json

@@ -8,15 +8,15 @@
     "build": "babel src -d dist --extensions \".ts,.js\""
   },
   "dependencies": {
-    "@medusajs/medusa": "1.1.38-dev-1630001256218",
-    "medusa-interfaces": "1.1.21-dev-1630001256218",
+    "@medusajs/medusa": "1.1.36",
+    "medusa-interfaces": "1.1.21",
     "typeorm": "^0.2.31"
   },
   "devDependencies": {
     "@babel/cli": "^7.12.10",
     "@babel/core": "^7.12.10",
     "@babel/node": "^7.12.10",
-    "babel-preset-medusa-package": "1.1.13-dev-1630001256218",
+    "babel-preset-medusa-package": "1.1.13",
     "jest": "^26.6.3"
   }
-}
+}

packages/medusa/src/api/routes/admin/shipping-options/update-shipping-option.js

@@ -56,7 +56,7 @@ export default async (req, res) => {
     requirements: Validator.array()
       .items(
         Validator.object({
-          id: Validator.string().required(),
+          id: Validator.string().optional(),
           type: Validator.string().required(),
           amount: Validator.number()
             .integer()

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

@@ -84,14 +84,11 @@ describe("ShippingOptionService", () => {
       await optionService.update(IdMap.getId("option"), { requirements })
 
       expect(shippingOptionRepository.save).toHaveBeenCalledTimes(1)
-      expect(shippingOptionRepository.save).toHaveBeenCalledWith({
-        requirements: [
-          {
-            shipping_option_id: IdMap.getId("option"),
-            type: "min_subtotal",
-            amount: 1,
-          },
-        ],
+      expect(shippingOptionRequirementRepository.save).toHaveBeenCalledTimes(1)
+      expect(shippingOptionRequirementRepository.save).toHaveBeenCalledWith({
+        shipping_option_id: IdMap.getId("option"),
+        type: "min_subtotal",
+        amount: 1,
       })
     })
 

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

@@ -92,6 +92,10 @@ class ShippingOptionService extends BaseService {
       where: { id: requirement.id },
     })
 
+    if (!existingReq && requirement.id) {
+      throw new MedusaError(MedusaError.Types.INVALID_DATA, "ID does not exist")
+    }
+
     let req
     if (existingReq) {
       req = await reqRepo.save({
@@ -99,10 +103,12 @@ class ShippingOptionService extends BaseService {
         ...requirement,
       })
     } else {
-      req = await reqRepo.create({
+      const created = reqRepo.create({
         shipping_option_id: optionId,
         ...requirement,
       })
+
+      req = await reqRepo.save(created)
     }
 
     return req
@@ -447,9 +453,22 @@ class ShippingOptionService extends BaseService {
             )
           }
 
+          if (
+            acc.find(
+              raw =>
+                (raw.type === "max_subtotal" &&
+                  validated.amount > raw.amount) ||
+                (raw.type === "min_subtotal" && validated.amount < raw.amount)
+            )
+          ) {
+            throw new MedusaError(
+              MedusaError.Types.INVALID_DATA,
+              "Max. subtotal must be greater than Min. subtotal"
+            )
+          }
+
           acc.push(validated)
         }
-        option.requirements = acc
       }
 
       if ("price_type" in update) {