fix: Creating shipping options with requirements (#428)

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

@@ -1,102 +1,53 @@
-const path = require("path");
+const path = require("path")
 const {
   Region,
   ShippingProfile,
   ShippingOption,
   ShippingOptionRequirement,
-} = require("@medusajs/medusa");
+} = require("@medusajs/medusa")
 
-const setupServer = require("../../../helpers/setup-server");
+const setupServer = require("../../../helpers/setup-server")
-const { useApi } = require("../../../helpers/use-api");
+const { useApi } = require("../../../helpers/use-api")
-const { initDb, useDb } = require("../../../helpers/use-db");
+const { initDb, useDb } = require("../../../helpers/use-db")
-const adminSeeder = require("../../helpers/admin-seeder");
+const adminSeeder = require("../../helpers/admin-seeder")
+const shippingOptionSeeder = require("../../helpers/shipping-option-seeder")
 
-jest.setTimeout(30000);
+jest.setTimeout(30000)
 
 describe("/admin/shipping-options", () => {
-  let medusaProcess;
+  let medusaProcess
-  let dbConnection;
+  let dbConnection
 
   beforeAll(async () => {
-    const cwd = path.resolve(path.join(__dirname, "..", ".."));
+    const cwd = path.resolve(path.join(__dirname, "..", ".."))
-    dbConnection = await initDb({ cwd });
+    dbConnection = await initDb({ cwd })
-    medusaProcess = await setupServer({ cwd });
+    medusaProcess = await setupServer({ cwd })
-  });
+  })
 
   afterAll(async () => {
-    const db = useDb();
+    const db = useDb()
-    await db.shutdown();
+    await db.shutdown()
-    medusaProcess.kill();
+    medusaProcess.kill()
-  });
+  })
 
-  describe("POST /admin/shipping-options", () => {
+  describe("POST /admin/shipping-options/:id", () => {
     beforeEach(async () => {
-      const manager = dbConnection.manager;
-
       try {
-        await adminSeeder(dbConnection);
+        await adminSeeder(dbConnection)
-
+        await shippingOptionSeeder(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);
+        console.error(err)
-        throw err;
+        throw err
       }
-    });
+    })
 
     afterEach(async () => {
-      const db = useDb();
+      const db = useDb()
-      await db.teardown();
+      await db.teardown()
-    });
+    })
 
     it("updates a shipping option with no existing requirements", async () => {
-      const api = useApi();
+      const api = useApi()
 
       const payload = {
         name: "Test option",
@@ -111,36 +62,36 @@ describe("/admin/shipping-options", () => {
             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;
+      const requirements = res.data.shipping_option.requirements
 
-      expect(res.status).toEqual(200);
+      expect(res.status).toEqual(200)
-      expect(requirements.length).toEqual(2);
+      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 api = useApi()
 
       const payload = {
         name: "Test option",
@@ -157,7 +108,7 @@ describe("/admin/shipping-options", () => {
             amount: 2,
           },
         ],
-      };
+      }
 
       const res = await api
         .post(`/admin/shipping-options/test-out`, payload, {
@@ -166,15 +117,15 @@ describe("/admin/shipping-options", () => {
           },
         })
         .catch((err) => {
-          return err.response;
+          return err.response
-        });
+        })
 
-      expect(res.status).toEqual(400);
+      expect(res.status).toEqual(400)
-      expect(res.data.message).toEqual("ID does not exist");
+      expect(res.data.message).toEqual("ID does not exist")
-    });
+    })
 
     it("it succesfully updates a set of existing requirements", async () => {
-      const api = useApi();
+      const api = useApi()
 
       const payload = {
         requirements: [
@@ -190,7 +141,7 @@ describe("/admin/shipping-options", () => {
           },
         ],
         amount: 200,
-      };
+      }
 
       const res = await api
         .post(`/admin/shipping-options/test-option-req`, payload, {
@@ -199,14 +150,14 @@ describe("/admin/shipping-options", () => {
           },
         })
         .catch((err) => {
-          console.log(err.response.data.message);
+          console.log(err.response.data.message)
-        });
+        })
 
-      expect(res.status).toEqual(200);
+      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 api = useApi()
 
       const payload = {
         requirements: [
@@ -216,7 +167,7 @@ describe("/admin/shipping-options", () => {
             amount: 15,
           },
         ],
-      };
+      }
 
       const res = await api
         .post(`/admin/shipping-options/test-option-req`, payload, {
@@ -225,14 +176,14 @@ describe("/admin/shipping-options", () => {
           },
         })
         .catch((err) => {
-          console.log(err.response.data.message);
+          console.log(err.response.data.message)
-        });
+        })
 
-      expect(res.status).toEqual(200);
+      expect(res.status).toEqual(200)
-    });
+    })
 
     it("succesfully updates a set of requirements because max. subtotal >= min. subtotal", async () => {
-      const api = useApi();
+      const api = useApi()
 
       const payload = {
         requirements: [
@@ -247,7 +198,7 @@ describe("/admin/shipping-options", () => {
             amount: 200,
           },
         ],
-      };
+      }
 
       const res = await api
         .post(`/admin/shipping-options/test-option-req`, payload, {
@@ -256,16 +207,16 @@ describe("/admin/shipping-options", () => {
           },
         })
         .catch((err) => {
-          console.log(err.response.data.message);
+          console.log(err.response.data.message)
-        });
+        })
 
-      expect(res.status).toEqual(200);
+      expect(res.status).toEqual(200)
-      expect(res.data.shipping_option.requirements[0].amount).toEqual(150);
+      expect(res.data.shipping_option.requirements[0].amount).toEqual(150)
-      expect(res.data.shipping_option.requirements[1].amount).toEqual(200);
+      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 api = useApi()
 
       const payload = {
         requirements: [
@@ -280,7 +231,7 @@ describe("/admin/shipping-options", () => {
             amount: 200,
           },
         ],
-      };
+      }
 
       const res = await api
         .post(`/admin/shipping-options/test-option-req`, payload, {
@@ -289,13 +240,147 @@ describe("/admin/shipping-options", () => {
           },
         })
         .catch((err) => {
-          return err.response;
+          return err.response
-        });
+        })
 
-      expect(res.status).toEqual(400);
+      expect(res.status).toEqual(400)
       expect(res.data.message).toEqual(
         "Max. subtotal must be greater than Min. subtotal"
-      );
+      )
-    });
+    })
-  });
+  })
-});
+
+  describe("POST /admin/shipping-options", () => {
+    let payload
+
+    beforeEach(async () => {
+      try {
+        await adminSeeder(dbConnection)
+        await shippingOptionSeeder(dbConnection)
+
+        const api = useApi()
+        await api.post(
+          `/admin/regions/region`,
+          {
+            fulfillment_providers: ["test-ful"],
+          },
+          {
+            headers: {
+              Authorization: "Bearer test_token",
+            },
+          }
+        )
+
+        const manager = dbConnection.manager
+        const defaultProfile = await manager.findOne(ShippingProfile, {
+          type: "default",
+        })
+
+        payload = {
+          name: "Test option",
+          amount: 100,
+          price_type: "flat_rate",
+          region_id: "region",
+          provider_id: "test-ful",
+          data: {},
+          profile_id: defaultProfile.id,
+        }
+      } catch (err) {
+        console.error(err)
+        throw err
+      }
+    })
+
+    afterEach(async () => {
+      const db = useDb()
+      await db.teardown()
+    })
+
+    it("creates a shipping option with requirements", async () => {
+      const api = useApi()
+      payload.requirements = [
+        {
+          type: "max_subtotal",
+          amount: 2,
+        },
+        {
+          type: "min_subtotal",
+          amount: 1,
+        },
+      ]
+
+      const res = await api.post(`/admin/shipping-options`, payload, {
+        headers: {
+          Authorization: "Bearer test_token",
+        },
+      })
+
+      expect(res.status).toEqual(200)
+      expect(res.data.shipping_option.requirements.length).toEqual(2)
+    })
+
+    it("creates a shipping option with no requirements", async () => {
+      const api = useApi()
+      const res = await api.post(`/admin/shipping-options`, payload, {
+        headers: {
+          Authorization: "Bearer test_token",
+        },
+      })
+
+      expect(res.status).toEqual(200)
+      expect(res.data.shipping_option.requirements.length).toEqual(0)
+    })
+
+    it("fails on same requirement types", async () => {
+      const api = useApi()
+      payload.requirements = [
+        {
+          type: "max_subtotal",
+          amount: 2,
+        },
+        {
+          type: "max_subtotal",
+          amount: 1,
+        },
+      ]
+
+      try {
+        await api.post(`/admin/shipping-options`, payload, {
+          headers: {
+            Authorization: "Bearer test_token",
+          },
+        })
+      } catch (error) {
+        expect(error.response.data.message).toEqual(
+          "Only one requirement of each type is allowed"
+        )
+      }
+    })
+
+    it("fails when min_subtotal > max_subtotal", async () => {
+      const api = useApi()
+      payload.requirements = [
+        {
+          type: "max_subtotal",
+          amount: 2,
+        },
+        {
+          type: "min_subtotal",
+          amount: 4,
+        },
+      ]
+
+      try {
+        await api.post(`/admin/shipping-options`, payload, {
+          headers: {
+            Authorization: "Bearer test_token",
+          },
+        })
+      } catch (error) {
+        expect(error.response.data.message).toEqual(
+          "Max. subtotal must be greater than Min. subtotal"
+        )
+      }
+    })
+  })
+})

integration-tests/api/helpers/shipping-option-seeder.js

@@ -0,0 +1,59 @@
+const {
+  Region,
+  ShippingProfile,
+  ShippingOption,
+  ShippingOptionRequirement,
+} = require("@medusajs/medusa")
+
+module.exports = async (connection, data = {}) => {
+  const manager = connection.manager
+
+  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,
+  })
+}

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

@@ -272,11 +272,7 @@ describe("ShippingOptionService", () => {
         amount: 10,
       })
 
-      expect(shippingOptionRequirementRepository.create).toBeCalledTimes(1)
+      expect(shippingOptionRequirementRepository.create).toBeCalledTimes(0)
-      expect(shippingOptionRequirementRepository.create).toBeCalledWith({
-        type: "max_subtotal",
-        amount: 10,
-      })
 
       expect(shippingOptionRepository.save).toBeCalledTimes(1)
       expect(shippingOptionRepository.save).toBeCalledWith({

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

@@ -96,6 +96,13 @@ class ShippingOptionService extends BaseService {
       throw new MedusaError(MedusaError.Types.INVALID_DATA, "ID does not exist")
     }
 
+    // If no option id is provided, we are currently in the process of creating
+    // a new shipping option. Therefore, simply return the requirement, such
+    // that the cascading will take care of the creation of the requirement.
+    if (!optionId) {
+      return requirement
+    }
+
     let req
     if (existingReq) {
       req = await reqRepo.save({
@@ -359,11 +366,33 @@ class ShippingOptionService extends BaseService {
       }
 
       if ("requirements" in data) {
-        option.requirements = await Promise.all(
+        const acc = []
-          data.requirements.map(r => {
+        for (const r of data.requirements) {
-            return this.validateRequirement_(r, option.id)
+          const validated = await this.validateRequirement_(r)
-          })
+
-        )
+          if (acc.find(raw => raw.type === validated.type)) {
+            throw new MedusaError(
+              MedusaError.Types.INVALID_DATA,
+              "Only one requirement of each type is allowed"
+            )
+          }
+
+          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)
+        }
       }
 
       const result = await optionRepo.save(option)