From edc6d9d29c61924a476a1e83742508a2b7dcda08 Mon Sep 17 00:00:00 2001 From: Oliver Windall Juhl <59018053+olivermrbl@users.noreply.github.com> Date: Wed, 13 Apr 2022 10:52:25 +0200 Subject: [PATCH] fix: Remove `region_id` from countries on region deletes (#1330) * Remove region id from countries on delete * Update snapshots --- .../admin/__snapshots__/region.js.snap | 49 ++++++ .../api/__tests__/admin/region.js | 146 ++++++++++++++++-- packages/medusa/src/models/country.ts | 14 +- .../medusa/src/services/__tests__/region.js | 48 +++--- packages/medusa/src/services/region.js | 6 +- 5 files changed, 222 insertions(+), 41 deletions(-) create mode 100644 integration-tests/api/__tests__/admin/__snapshots__/region.js.snap diff --git a/integration-tests/api/__tests__/admin/__snapshots__/region.js.snap b/integration-tests/api/__tests__/admin/__snapshots__/region.js.snap new file mode 100644 index 0000000000..844c0de52b --- /dev/null +++ b/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, + }, + ], + "created_at": Any, + "currency_code": "usd", + "deleted_at": null, + "fulfillment_providers": Array [ + Object { + "id": "test-ful", + "is_installed": true, + }, + ], + "gift_cards_taxable": true, + "id": Any, + "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, +} +`; diff --git a/integration-tests/api/__tests__/admin/region.js b/integration-tests/api/__tests__/admin/region.js index bc7970baa5..fca514edb2 100644 --- a/integration-tests/api/__tests__/admin/region.js +++ b/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 - await adminSeeder(dbConnection) - await manager.insert(Region, { - id: "test-region", - name: "Test Region", - currency_code: "usd", - tax_rate: 0, - }) + 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 () => { @@ -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" + ) + } + }) }) }) diff --git a/packages/medusa/src/models/country.ts b/packages/medusa/src/models/country.ts index 2928591cf6..8f71104bac 100644 --- a/packages/medusa/src/models/country.ts +++ b/packages/medusa/src/models/country.ts @@ -1,12 +1,11 @@ import { - Entity, Column, - ManyToOne, - JoinColumn, + Entity, 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( - () => Region, - r => r.countries - ) + @ManyToOne(() => Region, (r) => r.countries) @JoinColumn({ name: "region_id" }) region: Region } diff --git a/packages/medusa/src/services/__tests__/region.js b/packages/medusa/src/services/__tests__/region.js index 4ea0efa171..7b1b573c24 100644 --- a/packages/medusa/src/services/__tests__/region.js +++ b/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) } diff --git a/packages/medusa/src/services/region.js b/packages/medusa/src/services/region.js index 7fb1c2010e..34c54e5d23 100644 --- a/packages/medusa/src/services/region.js +++ b/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)