fix: Remove region_id from countries on region deletes (#1330)

* Remove region id from countries on delete

* Update snapshots

integration-tests/api/__tests__/admin/__snapshots__/region.js.snap

@@ -0,0 +1,49 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`/admin/regions Remove region from country on delete successfully creates a region with countries from a previously deleted region 1`] = `
+Object {
+  "deleted": true,
+  "id": "test-region",
+  "object": "region",
+}
+`;
+
+exports[`/admin/regions Remove region from country on delete successfully creates a region with countries from a previously deleted region 2`] = `
+Object {
+  "automatic_taxes": true,
+  "countries": Array [
+    Object {
+      "display_name": "United States",
+      "id": 236,
+      "iso_2": "us",
+      "iso_3": "usa",
+      "name": "UNITED STATES",
+      "num_code": 840,
+      "region_id": Any<String>,
+    },
+  ],
+  "created_at": Any<String>,
+  "currency_code": "usd",
+  "deleted_at": null,
+  "fulfillment_providers": Array [
+    Object {
+      "id": "test-ful",
+      "is_installed": true,
+    },
+  ],
+  "gift_cards_taxable": true,
+  "id": Any<String>,
+  "metadata": null,
+  "name": "World",
+  "payment_providers": Array [
+    Object {
+      "id": "test-pay",
+      "is_installed": true,
+    },
+  ],
+  "tax_code": null,
+  "tax_provider_id": null,
+  "tax_rate": 0,
+  "updated_at": Any<String>,
+}
+`;

integration-tests/api/__tests__/admin/region.js

@@ -11,9 +11,9 @@ jest.setTimeout(30000)
 describe("/admin/regions", () => {
   let medusaProcess
   let dbConnection
+  const cwd = path.resolve(path.join(__dirname, "..", ".."))
 
   beforeAll(async () => {
-    const cwd = path.resolve(path.join(__dirname, "..", ".."))
     dbConnection = await initDb({ cwd })
     medusaProcess = await setupServer({ cwd })
   })
@@ -21,11 +21,98 @@ describe("/admin/regions", () => {
   afterAll(async () => {
     const db = useDb()
     await db.shutdown()
+
     medusaProcess.kill()
   })
 
+  describe("Remove region from country on delete", () => {
+    beforeEach(async () => {
+      try {
+        await adminSeeder(dbConnection)
+        const manager = dbConnection.manager
+        await manager.insert(Region, {
+          id: "test-region",
+          name: "Test Region",
+          currency_code: "usd",
+          tax_rate: 0,
+        })
+
+        await manager.query(
+          `UPDATE "country" SET region_id='test-region' WHERE iso_2 = 'us'`
+        )
+      } catch (err) {
+        console.log(err)
+        throw err
+      }
+    })
+
+    afterEach(async () => {
+      const db = useDb()
+      await db.teardown()
+    })
+
+    it("successfully creates a region with countries from a previously deleted region", async () => {
+      const api = useApi()
+
+      const response = await api
+        .delete(`/admin/regions/test-region`, {
+          headers: {
+            Authorization: "Bearer test_token",
+          },
+        })
+        .catch((err) => {
+          console.log(err)
+        })
+
+      expect(response.status).toEqual(200)
+      expect(response.data).toMatchSnapshot({
+        id: "test-region",
+        object: "region",
+        deleted: true,
+      })
+
+      const newReg = await api.post(
+        `/admin/regions`,
+        {
+          name: "World",
+          currency_code: "usd",
+          tax_rate: 0,
+          payment_providers: ["test-pay"],
+          fulfillment_providers: ["test-ful"],
+          countries: ["us"],
+        },
+        {
+          headers: {
+            Authorization: "Bearer test_token",
+          },
+        }
+      )
+
+      expect(newReg.status).toEqual(200)
+      expect(newReg.data.region).toMatchSnapshot({
+        id: expect.any(String),
+        name: "World",
+        currency_code: "usd",
+        countries: [
+          {
+            region_id: expect.any(String),
+          },
+        ],
+        tax_rate: 0,
+        fulfillment_providers: [
+          {
+            id: "test-ful",
+            is_installed: true,
+          },
+        ],
+        created_at: expect.any(String),
+        updated_at: expect.any(String),
+      })
+    })
+  })
+
   describe("GET /admin/regions", () => {
-    beforeAll(async () => {
+    beforeEach(async () => {
       const manager = dbConnection.manager
       await adminSeeder(dbConnection)
       await manager.insert(Region, {
@@ -57,7 +144,7 @@ describe("/admin/regions", () => {
       })
     })
 
-    afterAll(async () => {
+    afterEach(async () => {
       const db = useDb()
       await db.teardown()
     })
@@ -116,14 +203,23 @@ describe("/admin/regions", () => {
 
   describe("DELETE /admin/regions/:id", () => {
     beforeEach(async () => {
-      const manager = dbConnection.manager
+      try {
-      await adminSeeder(dbConnection)
+        await adminSeeder(dbConnection)
-      await manager.insert(Region, {
+        const manager = dbConnection.manager
-        id: "test-region",
+        await manager.insert(Region, {
-        name: "Test Region",
+          id: "test-region",
-        currency_code: "usd",
+          name: "Test Region",
-        tax_rate: 0,
+          currency_code: "usd",
-      })
+          tax_rate: 0,
+        })
+
+        await manager.query(
+          `UPDATE "country" SET region_id='test-region' WHERE iso_2 = 'us'`
+        )
+      } catch (err) {
+        console.log(err)
+        throw err
+      }
     })
 
     afterEach(async () => {
@@ -164,5 +260,33 @@ describe("/admin/regions", () => {
       })
       expect(response.status).toEqual(200)
     })
+
+    it("fails to create when countries exists in different region", async () => {
+      const api = useApi()
+
+      try {
+        await api.post(
+          `/admin/regions`,
+          {
+            name: "World",
+            currency_code: "usd",
+            tax_rate: 0,
+            payment_providers: ["test-pay"],
+            fulfillment_providers: ["test-ful"],
+            countries: ["us"],
+          },
+          {
+            headers: {
+              Authorization: "Bearer test_token",
+            },
+          }
+        )
+      } catch (error) {
+        expect(error.response.status).toEqual(422)
+        expect(error.response.data.message).toEqual(
+          "United States already exists in region test-region"
+        )
+      }
+    })
   })
 })

packages/medusa/src/models/country.ts

@@ -1,12 +1,11 @@
 import {
-  Entity,
   Column,
-  ManyToOne,
+  Entity,
-  JoinColumn,
   Index,
+  JoinColumn,
+  ManyToOne,
   PrimaryGeneratedColumn,
 } from "typeorm"
-
 import { Region } from "./region"
 
 @Entity()
@@ -32,12 +31,9 @@ export class Country {
 
   @Index()
   @Column({ nullable: true })
-  region_id: string
+  region_id: string | null
 
-  @ManyToOne(
+  @ManyToOne(() => Region, (r) => r.countries)
-    () => Region,
-    r => r.countries
-  )
   @JoinColumn({ name: "region_id" })
   region: Region
 }

packages/medusa/src/services/__tests__/region.js

@@ -8,12 +8,11 @@ const eventBusService = {
   },
 }
 
-
 describe("RegionService", () => {
   describe("create", () => {
     const regionRepository = MockRepository({})
     const ppRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id === "should_fail") {
           return Promise.resolve(undefined)
         }
@@ -23,7 +22,7 @@ describe("RegionService", () => {
       },
     })
     const fpRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id === "should_fail") {
           return Promise.resolve(undefined)
         }
@@ -33,11 +32,12 @@ describe("RegionService", () => {
       },
     })
     const countryRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.iso_2 === "dk") {
           return Promise.resolve({
             id: IdMap.getId("dk"),
             name: "Denmark",
+            display_name: "Denmark",
             region_id: IdMap.getId("dk-reg"),
           })
         }
@@ -109,7 +109,7 @@ describe("RegionService", () => {
         })
       } catch (error) {
         expect(error.message).toBe(
-          "Denmark already exists in Denmark, delete it in that region before adding it"
+          `Denmark already exists in region ${IdMap.getId("dk-reg")}`
         )
       }
     })
@@ -198,7 +198,7 @@ describe("RegionService", () => {
   describe("validateFields_", () => {
     const regionRepository = MockRepository({})
     const ppRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id === "should_fail") {
           return Promise.resolve(undefined)
         }
@@ -208,7 +208,7 @@ describe("RegionService", () => {
       },
     })
     const fpRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id === "should_fail") {
           return Promise.resolve(undefined)
         }
@@ -218,11 +218,12 @@ describe("RegionService", () => {
       },
     })
     const countryRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.iso_2 === "dk") {
           return Promise.resolve({
             id: IdMap.getId("dk"),
             name: "Denmark",
+            display_name: "Denmark",
             region_id: IdMap.getId("dk-reg"),
           })
         }
@@ -269,7 +270,7 @@ describe("RegionService", () => {
       await expect(
         regionService.validateFields_({ countries: ["DK"] })
       ).rejects.toThrow(
-        "Denmark already exists in Denmark, delete it in that region before adding it"
+        `Denmark already exists in region ${IdMap.getId("dk-reg")}`
       )
     })
 
@@ -293,7 +294,7 @@ describe("RegionService", () => {
       findOne: () => Promise.resolve({ id: IdMap.getId("test-region") }),
     })
     const ppRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id === "should_fail") {
           return Promise.resolve(undefined)
         }
@@ -303,7 +304,7 @@ describe("RegionService", () => {
       },
     })
     const fpRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id === "should_fail") {
           return Promise.resolve(undefined)
         }
@@ -313,7 +314,7 @@ describe("RegionService", () => {
       },
     })
     const countryRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.iso_2 === "dk") {
           return Promise.resolve({
             id: IdMap.getId("dk"),
@@ -387,13 +388,21 @@ describe("RegionService", () => {
 
   describe("delete", () => {
     const regionRepository = MockRepository({
-      findOne: () => Promise.resolve({ id: IdMap.getId("region") }),
+      findOne: () =>
+        Promise.resolve({
+          id: IdMap.getId("region"),
+          countries: [{ id: "us" }],
+        }),
+    })
+    const countryRepository = MockRepository({
+      findOne: (query) => Promise.resolve(),
     })
 
     const regionService = new RegionService({
       manager: MockManager,
       eventBusService,
       regionRepository,
+      countryRepository,
     })
 
     beforeEach(async () => {
@@ -406,13 +415,14 @@ describe("RegionService", () => {
       expect(regionRepository.softRemove).toHaveBeenCalledTimes(1)
       expect(regionRepository.softRemove).toHaveBeenCalledWith({
         id: IdMap.getId("region"),
+        countries: [{ id: "us" }],
       })
     })
   })
 
   describe("addCountry", () => {
     const regionRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id === IdMap.getId("region-with-country")) {
           return Promise.resolve({
             id: IdMap.getId("region-with-country"),
@@ -425,7 +435,7 @@ describe("RegionService", () => {
       },
     })
     const countryRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.iso_2 === "dk") {
           return Promise.resolve({
             id: IdMap.getId("dk"),
@@ -477,7 +487,7 @@ describe("RegionService", () => {
 
   describe("removeCountry", () => {
     const regionRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         return Promise.resolve({
           id: IdMap.getId("region"),
           countries: [{ id: IdMap.getId("dk"), name: "Denmark", iso_2: "dk" }],
@@ -515,7 +525,7 @@ describe("RegionService", () => {
         }),
     })
     const ppRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id === "should_fail") {
           return Promise.resolve(undefined)
         }
@@ -525,7 +535,7 @@ describe("RegionService", () => {
       },
     })
     const fpRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id === "should_fail") {
           return Promise.resolve(undefined)
         }
@@ -586,7 +596,7 @@ describe("RegionService", () => {
         }),
     })
     const fpRepository = MockRepository({
-      findOne: query => {
+      findOne: (query) => {
         if (query.where.id === "should_fail") {
           return Promise.resolve(undefined)
         }

packages/medusa/src/services/region.js

@@ -350,7 +350,7 @@ class RegionService extends BaseService {
     if (country.region_id && country.region_id !== regionId) {
       throw new MedusaError(
         MedusaError.Types.DUPLICATE_ERROR,
-        `${country.name} already exists in ${country.name}, delete it in that region before adding it`
+        `${country.display_name} already exists in region ${country.region_id}`
       )
     }
 
@@ -430,14 +430,16 @@ class RegionService extends BaseService {
   async delete(regionId) {
     return this.atomicPhase_(async (manager) => {
       const regionRepo = manager.getCustomRepository(this.regionRepository_)
+      const countryRepo = manager.getCustomRepository(this.countryRepository_)
 
-      const region = await regionRepo.findOne({ where: { id: regionId } })
+      const region = await this.retrieve(regionId, { relations: ["countries"] })
 
       if (!region) {
         return Promise.resolve()
       }
 
       await regionRepo.softRemove(region)
+      await countryRepo.update({ region_id: region.id }, { region_id: null })
 
       await this.eventBus_
         .withTransaction(manager)