From 72f8e94e8165e740d97b1c6e9047ddb3267e29ac Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 18 Jul 2023 15:05:06 +0200 Subject: [PATCH] implemented requested review changes --- .../import-column-mapping.component.html | 2 +- .../import-column-mapping.component.spec.ts | 45 ++++++------------ .../import-column-mapping.component.ts | 46 ++++--------------- .../import-history.component.html | 4 +- .../import-history.component.spec.ts | 3 +- .../import-history.component.ts | 14 ++---- .../features/import/import.service.spec.ts | 6 +-- src/app/features/import/import.service.ts | 10 ++-- .../import/import/import.component.spec.ts | 10 +--- .../import/import/import.component.ts | 25 ++++------ 10 files changed, 50 insertions(+), 115 deletions(-) diff --git a/src/app/features/import/import-column-mapping/import-column-mapping.component.html b/src/app/features/import/import-column-mapping/import-column-mapping.component.html index 13366fcdca..ec9376a99c 100644 --- a/src/app/features/import/import-column-mapping/import-column-mapping.component.html +++ b/src/app/features/import/import-column-mapping/import-column-mapping.component.html @@ -19,7 +19,7 @@ diff --git a/src/app/features/import/import-column-mapping/import-column-mapping.component.spec.ts b/src/app/features/import/import-column-mapping/import-column-mapping.component.spec.ts index d7c6a3964c..198b7702dd 100644 --- a/src/app/features/import/import-column-mapping/import-column-mapping.component.spec.ts +++ b/src/app/features/import/import-column-mapping/import-column-mapping.component.spec.ts @@ -5,8 +5,8 @@ import { MockedTestingModule } from "../../../utils/mocked-testing.module"; import { MatDialog } from "@angular/material/dialog"; import { EnumValueMappingComponent } from "./enum-value-mapping/enum-value-mapping.component"; import { Child } from "../../../child-dev-project/children/model/child"; -import { SimpleChange } from "@angular/core"; import { ColumnMapping } from "../column-mapping"; +import { of } from "rxjs"; describe("ImportMapColumnsComponent", () => { let component: ImportColumnMappingComponent; @@ -24,31 +24,6 @@ describe("ImportMapColumnsComponent", () => { spyOn(component.columnMappingChange, "emit"); }); - it("should reset invalid mappings when new entityType selected", () => { - component.columnMapping = [ - { column: "x", propertyName: "name" }, // property also exists on new entityType - { column: "y", propertyName: "projectNumber" }, // property does not exist on new entityType - ]; - - component.entityType = "School"; - component.ngOnChanges({ - entityType: new SimpleChange("Child", "School", false), - }); - - const expectedNewMapping: ColumnMapping[] = [ - { column: "x", propertyName: "name" }, - { - column: "y", - propertyName: undefined, - }, - ]; - - expect(component.columnMapping).toEqual(expectedNewMapping); - expect(component.columnMappingChange.emit).toHaveBeenCalledWith( - expectedNewMapping - ); - }); - it("should open mapping component with required data", () => { component.rawData = [ { name: "first", gender: "male" }, @@ -58,6 +33,7 @@ describe("ImportMapColumnsComponent", () => { component.entityType = "Child"; component.columnMapping = [{ column: "name" }, { column: "gender" }]; const openSpy = spyOn(TestBed.inject(MatDialog), "open"); + openSpy.and.returnValue({ afterClosed: () => of(undefined) } as any); const genderColumn = component.columnMapping[1]; genderColumn.propertyName = "gender"; @@ -73,13 +49,18 @@ describe("ImportMapColumnsComponent", () => { }); }); - it("should emit on updateMapping from UI", () => { - component.updateMapping(); + it("should emit changes after popup is closed", () => { + spyOn(TestBed.inject(MatDialog), "open").and.returnValue({ + afterClosed: () => of(undefined), + } as any); + component.entityType = "Child"; + const columnMapping: ColumnMapping = { + column: "test", + propertyName: "gender", + }; + + component.openMappingComponent(columnMapping); expect(component.columnMappingChange.emit).toHaveBeenCalled(); }); - - it("should emit on changes from popup form with additional details", () => { - // TODO: test additional mapping details from popup form to be applied and emitted - }); }); diff --git a/src/app/features/import/import-column-mapping/import-column-mapping.component.ts b/src/app/features/import/import-column-mapping/import-column-mapping.component.ts index ae85c0f31f..7fd1ebf8fa 100644 --- a/src/app/features/import/import-column-mapping/import-column-mapping.component.ts +++ b/src/app/features/import/import-column-mapping/import-column-mapping.component.ts @@ -1,11 +1,4 @@ -import { - Component, - EventEmitter, - Input, - OnChanges, - Output, - SimpleChanges, -} from "@angular/core"; +import { Component, EventEmitter, Input, Output } from "@angular/core"; import { ColumnMapping } from "../column-mapping"; import { ComponentType } from "@angular/cdk/overlay"; import { EntityRegistry } from "../../../core/entity/database-entity.decorator"; @@ -38,7 +31,7 @@ import { MatButtonModule } from "@angular/material/button"; NgIf, ], }) -export class ImportColumnMappingComponent implements OnChanges { +export class ImportColumnMappingComponent { @Input() rawData: any[] = []; @Input() columnMapping: ColumnMapping[] = []; @Output() columnMappingChange = new EventEmitter(); @@ -73,43 +66,24 @@ export class ImportColumnMappingComponent implements OnChanges { private dialog: MatDialog ) {} - ngOnChanges(changes: SimpleChanges): void { - if (changes.hasOwnProperty("entityType")) { - this.resetMappingToEntityType(); - } - } - - /** - * un-map properties not matching the entityType - * @private - */ - private resetMappingToEntityType() { - this.columnMapping - .filter((c) => !this.allProps.includes(c.propertyName)) - .forEach((c) => (c.propertyName = undefined)); - this.updateMapping(); - } - openMappingComponent(col: ColumnMapping) { - const uniqueValues = new Set(); + const uniqueValues = new Set(); this.rawData.forEach((obj) => uniqueValues.add(obj[col.column])); - this.dialog.open( - this.mappingCmp[col.propertyName], - { + this.dialog + .open(this.mappingCmp[col.propertyName], { data: { col: col, values: [...uniqueValues], entityType: this.entityCtor, }, disableClose: true, - } - ); + }) + .afterClosed() + .subscribe(() => this.emitUpdate()); } - /** - * Emit an updated columnMapping array and emit change event, to ensure smooth change detection. - */ - updateMapping() { + emitUpdate() { + // Emitting copy of array to trigger change detection this.columnMappingChange.emit([...this.columnMapping]); } diff --git a/src/app/features/import/import-history/import-history.component.html b/src/app/features/import/import-history/import-history.component.html index 59acb86bfe..e5fa654ec9 100644 --- a/src/app/features/import/import-history/import-history.component.html +++ b/src/app/features/import/import-history/import-history.component.html @@ -2,8 +2,8 @@

Previous Imports

diff --git a/src/app/features/import/import-history/import-history.component.spec.ts b/src/app/features/import/import-history/import-history.component.spec.ts index 5f31342169..82f9ebf044 100644 --- a/src/app/features/import/import-history/import-history.component.spec.ts +++ b/src/app/features/import/import-history/import-history.component.spec.ts @@ -55,14 +55,13 @@ describe("ImportHistoryComponent", () => { fixture.detectChanges(); }); - it("should load all previous imports, sorted by date and highlight most recent", fakeAsync(() => { + it("should load all previous imports, sorted by date", fakeAsync(() => { mockEntityMapper.loadType.and.resolveTo([testImport1, testImport2]); component.ngOnInit(); tick(); expect(component.previousImports).toEqual([testImport2, testImport1]); - expect(component.highlightedPreviousImport).toEqual(testImport2); })); it("should ask for confirmation before undo action", fakeAsync(() => { diff --git a/src/app/features/import/import-history/import-history.component.ts b/src/app/features/import/import-history/import-history.component.ts index 6138843361..8729ba751b 100644 --- a/src/app/features/import/import-history/import-history.component.ts +++ b/src/app/features/import/import-history/import-history.component.ts @@ -1,4 +1,4 @@ -import { Component, EventEmitter, Input, OnInit, Output } from "@angular/core"; +import { Component, EventEmitter, OnInit, Output } from "@angular/core"; import { EntityMapperService } from "../../../core/entity/entity-mapper.service"; import { ImportMetadata } from "../import-metadata"; import { ImportService } from "../import.service"; @@ -28,8 +28,6 @@ import { MatButtonModule } from "@angular/material/button"; ], }) export class ImportHistoryComponent implements OnInit { - @Input() highlightedPreviousImport: ImportMetadata; - @Output() itemSelected = new EventEmitter(); previousImports: ImportMetadata[]; @@ -44,7 +42,7 @@ export class ImportHistoryComponent implements OnInit { .pipe(untilDestroyed(this)) .subscribe((update) => { this.previousImports = applyUpdate(this.previousImports, update); - this.sortAndHighlightImports(); + this.sortImports(); }); } @@ -56,17 +54,13 @@ export class ImportHistoryComponent implements OnInit { this.previousImports = await this.entityMapper.loadType( ImportMetadata ); - this.sortAndHighlightImports(); + this.sortImports(); } - private sortAndHighlightImports() { + private sortImports() { this.previousImports = this.previousImports.sort( (a, b) => b.date.getTime() - a.date.getTime() ); - - if (!this.highlightedPreviousImport) { - this.highlightedPreviousImport = this.previousImports?.[0]; - } } async undoImport(item: ImportMetadata) { diff --git a/src/app/features/import/import.service.spec.ts b/src/app/features/import/import.service.spec.ts index 6180601a5d..708ef6df84 100644 --- a/src/app/features/import/import.service.spec.ts +++ b/src/app/features/import/import.service.spec.ts @@ -136,8 +136,8 @@ describe("ImportService", () => { const relations = [ { childId: "1", schoolId: "4" }, { childId: "2", schoolId: "4" }, - { childId: "3", schoolId: "4" }, - { childId: "2", schoolId: "3" }, + { childId: "3", schoolId: "4" }, // Other child same school -> keep + { childId: "2", schoolId: "3" }, // Imported child different school -> remove ].map((e) => Object.assign(new ChildSchoolRelation(), e)); const activity = new RecurringActivity("3"); activity.participants = ["3", "2", "1"]; @@ -153,7 +153,7 @@ describe("ImportService", () => { await service.undoImport(importMeta); await expectEntitiesToBeInDatabase([children[2]], false, true); - await expectEntitiesToBeInDatabase(relations.slice(2), false, true); + await expectEntitiesToBeInDatabase([relations[2]], false, true); expect(activity.participants).toEqual(["3"]); await expectAsync( entityMapper.load(ImportMetadata, importMeta.getId()) diff --git a/src/app/features/import/import.service.ts b/src/app/features/import/import.service.ts index 6477159890..b02854b50a 100644 --- a/src/app/features/import/import.service.ts +++ b/src/app/features/import/import.service.ts @@ -82,7 +82,7 @@ export class ImportService { ) { return { mappingCmp: EnumValueMappingComponent, - mappingFn: (val, additional) => additional[val], + mappingFn: (val, additional) => additional?.[val], }; } if (this.dateDataTypes.includes(schema.dataType)) { @@ -138,12 +138,10 @@ export class ImportService { return this.entityMapper.saveAll(relations); } - private async undoSchoolLink(importMeta: ImportMetadata, id: string) { + private async undoSchoolLink(importMeta: ImportMetadata) { const relations = await this.entityMapper.loadType(ChildSchoolRelation); - const imported = relations.filter( - (rel) => - rel.schoolId === id && - importMeta.ids.includes(Entity.createPrefixedId("Child", rel.childId)) + const imported = relations.filter((rel) => + importMeta.ids.includes(Entity.createPrefixedId("Child", rel.childId)) ); return Promise.all(imported.map((rel) => this.entityMapper.remove(rel))); } diff --git a/src/app/features/import/import/import.component.spec.ts b/src/app/features/import/import/import.component.spec.ts index ff6ceab5f6..241f19d668 100644 --- a/src/app/features/import/import/import.component.spec.ts +++ b/src/app/features/import/import/import.component.spec.ts @@ -122,10 +122,7 @@ describe("ImportComponent", () => { testApplyColumnMapping( loadedMapping, - [ - { column: "x", propertyName: "name" }, - { column: "y", propertyName: undefined }, - ], + [{ column: "x", propertyName: "name" }, { column: "y" }], 1 ); }); @@ -142,10 +139,7 @@ describe("ImportComponent", () => { testApplyColumnMapping( loadedMapping, - [ - { column: "x", propertyName: "name" }, - { column: "y", propertyName: undefined }, - ], + [{ column: "x", propertyName: "name" }, { column: "y" }], 1 ); }); diff --git a/src/app/features/import/import/import.component.ts b/src/app/features/import/import/import.component.ts index 4c1a4188e5..c993228f05 100644 --- a/src/app/features/import/import/import.component.ts +++ b/src/app/features/import/import/import.component.ts @@ -20,11 +20,12 @@ import { ImportAdditionalActionsComponent } from "../import-additional-actions/i import { MatButtonModule } from "@angular/material/button"; import { ImportColumnMappingComponent } from "../import-column-mapping/import-column-mapping.component"; import { ImportReviewDataComponent } from "../import-review-data/import-review-data.component"; +import { RouteTarget } from "../../../app.routing"; /** * View providing a full UI workflow to import data from an uploaded file. */ -//TODO: @RouteTarget("Import") +@RouteTarget("Import") @Component({ selector: "app-import", templateUrl: "./import.component.html", @@ -113,25 +114,19 @@ export class ImportComponent { applyPreviousMapping(importMetadata: ImportMetadata) { this.entityType = importMetadata.config.entityType; - // apply columns individually to ensure only valid mappings are merging on top of existing mapping - const applicableMapping: ColumnMapping[] = []; - for (const existingCol of this.columnMapping) { - let applicableCol = importMetadata.config.columnMapping.find( - (c) => existingCol.column === c.column - ); - if (!applicableCol) { - // reset any existing mapping - the loading should also apply which columns are ignored - applicableCol = { column: existingCol.column, propertyName: undefined }; - } - applicableMapping.push(Object.assign({}, existingCol, applicableCol)); - } + const adjustedMappings = this.columnMapping.map( + ({ column }) => + importMetadata.config.columnMapping.find( + (c) => column === c.column + ) ?? { column } + ); - this.onColumnMappingUpdate(applicableMapping); + this.onColumnMappingUpdate(adjustedMappings); } onImportCompleted(completedImport: ImportMetadata) { // TODO EntitySubrecord shows saved entities for a moment (because it listens to the entity updates) // TODO some components can't handle the reset and throw errors (maybe reload page instead to destroy the state completely) - this.reset(true); + return this.reset(true); } }