implemented requested review changes

src/app/features/import/import-column-mapping/import-column-mapping.component.html

@@ -19,7 +19,7 @@
       <app-basic-autocomplete
         [options]="allProps"
         [(ngModel)]="col.propertyName"
-        (ngModelChange)="updateMapping()"
+        (ngModelChange)="emitUpdate()"
         [optionToString]="labelMapper"
         [hideOption]="isUsed"
       />

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
-  });
 });

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<ColumnMapping[]>();
@@ -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<any>();
     this.rawData.forEach((obj) => uniqueValues.add(obj[col.column]));
-    this.dialog.open<any, MappingDialogData>(
-      this.mappingCmp[col.propertyName],
-      {
+    this.dialog
+      .open<any, MappingDialogData>(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]);
   }
 

src/app/features/import/import-history/import-history.component.html

@@ -2,8 +2,8 @@ <h3 i18n>Previous Imports</h3>
 
 <mat-accordion>
   <mat-expansion-panel
-    *ngFor="let item of previousImports"
-    [expanded]="item === highlightedPreviousImport"
+    *ngFor="let item of previousImports; index as i"
+    [expanded]="i === 0"
   >
     <mat-expansion-panel-header>
       <mat-panel-title i18n>

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(() => {

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<ImportMetadata>();
 
   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>(
       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) {

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())

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)));
   }

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
     );
   });

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);
   }
 }