File upload fixes

src/app/core/entity-components/entity-form/entity-form/entity-form.component.spec.ts

@@ -166,4 +166,31 @@ describe("EntityFormComponent", () => {
     expect(component.entity.name).toBe("Changed Name");
     expect(component.form.get("name")).toHaveValue("Changed Name");
   });
+
+  it("should align form with entity if canceled and notify about click", () => {
+    const child = new Child();
+    child.name = "test child";
+    component.entity = child;
+    component.form.enable();
+    component.form.get("name").setValue("other name");
+    let cancelEmitted = false;
+    component.cancel.subscribe(() => (cancelEmitted = true));
+
+    component.cancelClicked();
+
+    expect(component.form.disabled).toBeTrue();
+    expect(component.form.get("name")).toHaveValue("test child");
+    expect(cancelEmitted).toBeTrue();
+  });
+
+  it("should also reset form values which where not set before", () => {
+    component.entity = new Child();
+    component.ngOnInit();
+    component.form.enable();
+
+    component.form.get("name").setValue("my name");
+    component.cancelClicked();
+
+    expect(component.form.get("name")).toHaveValue(null);
+  });
 });

src/app/core/entity-components/entity-form/entity-form/entity-form.component.ts

@@ -76,6 +76,7 @@ export class EntityFormComponent<T extends Entity = Entity> implements OnInit {
   form: EntityForm<T>;
 
   private saveInProgress = false;
+  private initialFormValues: any;
 
   constructor(
     private entityFormService: EntityFormService,
@@ -86,8 +87,8 @@ export class EntityFormComponent<T extends Entity = Entity> implements OnInit {
 
   ngOnInit() {
     this.buildFormConfig();
-    if (this.editing) {
-      this.form.enable();
+    if (!this.editing) {
+      this.form.disable();
     }
     this.entityMapper
       .receiveUpdates(this.entity.getConstructor())
@@ -96,7 +97,7 @@ export class EntityFormComponent<T extends Entity = Entity> implements OnInit {
   }
 
   private async applyChanges(entity) {
-    if (this.saveInProgress) {
+    if (this.saveInProgress || this.formIsUpToDate(entity)) {
       // this is the component that currently saves the values -> no need to apply changes.
       return;
     }
@@ -107,7 +108,7 @@ export class EntityFormComponent<T extends Entity = Entity> implements OnInit {
         $localize`Local changes are in conflict with updated values synced from the server. Do you want the local changes to be overwritten with the latest values?`
       ))
     ) {
-      this.buildFormConfig(entity);
+      this.resetForm(entity);
     }
   }
 
@@ -129,21 +130,35 @@ export class EntityFormComponent<T extends Entity = Entity> implements OnInit {
 
   cancelClicked() {
     this.cancel.emit();
-    this.buildFormConfig();
+    this.resetForm();
+    this.form.disable();
   }
 
-  private buildFormConfig(entity = this.entity) {
+  private buildFormConfig() {
     const flattenedFormFields = new Array<FormFieldConfig>().concat(
       ...this._columns
     );
     this.entityFormService.extendFormFieldConfig(
       flattenedFormFields,
-      entity.getConstructor()
+      this.entity.getConstructor()
     );
     this.form = this.entityFormService.createFormGroup(
       flattenedFormFields,
-      entity
+      this.entity
+    );
+    this.initialFormValues = this.form.getRawValue();
+  }
+
+  private resetForm(entity = this.entity) {
+    // Patch form with values from the entity
+    this.form.patchValue(Object.assign(this.initialFormValues, entity));
+    this.form.markAsPristine();
+  }
+
+  private formIsUpToDate(entity: T): boolean {
+    return Object.entries(this.form.getRawValue()).every(
+      ([key, value]) =>
+        entity[key] === value || (entity[key] === undefined && value === null)
     );
-    this.form.disable();
   }
 }

src/app/features/file/couchdb-file.service.spec.ts

@@ -9,7 +9,14 @@ import {
   HttpStatusCode,
 } from "@angular/common/http";
 import { MatDialog } from "@angular/material/dialog";
-import { EMPTY, of, Subject, throwError } from "rxjs";
+import {
+  BehaviorSubject,
+  EMPTY,
+  firstValueFrom,
+  of,
+  Subject,
+  throwError,
+} from "rxjs";
 import { ShowFileComponent } from "./show-file/show-file.component";
 import { Entity } from "../../core/entity/model/entity";
 import { EntityMapperService } from "../../core/entity/entity-mapper.service";
@@ -26,7 +33,6 @@ describe("CouchdbFileService", () => {
   let service: CouchdbFileService;
   let mockHttp: jasmine.SpyObj<HttpClient>;
   let mockDialog: jasmine.SpyObj<MatDialog>;
-  let mockEntityMapper: jasmine.SpyObj<EntityMapperService>;
   let mockSnackbar: jasmine.SpyObj<MatSnackBar>;
   let dismiss: jasmine.Spy;
   let updates: Subject<UpdatedEntity<Entity>>;
@@ -36,8 +42,6 @@ describe("CouchdbFileService", () => {
     mockHttp = jasmine.createSpyObj(["get", "put", "delete"]);
     mockDialog = jasmine.createSpyObj(["open"]);
     updates = new Subject();
-    mockEntityMapper = jasmine.createSpyObj(["save", "receiveUpdates"]);
-    mockEntityMapper.receiveUpdates.and.returnValue(updates);
     mockSnackbar = jasmine.createSpyObj(["openFromComponent"]);
     dismiss = jasmine.createSpy();
     mockSnackbar.openFromComponent.and.returnValue({ dismiss } as any);
@@ -49,7 +53,10 @@ describe("CouchdbFileService", () => {
         { provide: HttpClient, useValue: mockHttp },
         { provide: MatSnackBar, useValue: mockSnackbar },
         { provide: MatDialog, useValue: mockDialog },
-        { provide: EntityMapperService, useValue: mockEntityMapper },
+        {
+          provide: EntityMapperService,
+          useValue: { receiveUpdates: () => updates },
+        },
         { provide: EntityRegistry, useValue: entityRegistry },
       ],
     });
@@ -60,7 +67,7 @@ describe("CouchdbFileService", () => {
     expect(service).toBeTruthy();
   });
 
-  it("should add a attachment to a existing document and update the entity", () => {
+  it("should add a attachment to a existing document", () => {
     mockHttp.get.and.returnValue(of({ _rev: "test_rev" }));
     mockHttp.put.and.returnValue(of({ ok: true }));
     const file = { type: "image/png", name: "file.name" } as File;
@@ -78,8 +85,6 @@ describe("CouchdbFileService", () => {
       jasmine.anything(),
       jasmine.anything()
     );
-    expect(entity["testProp"]).toBe("file.name");
-    expect(mockEntityMapper.save).toHaveBeenCalledWith(entity);
   });
 
   it("should create attachment document if it does not exist yet", (done) => {
@@ -102,8 +107,6 @@ describe("CouchdbFileService", () => {
         jasmine.anything(),
         jasmine.anything()
       );
-      expect(entity["testProp"]).toBe("file.name");
-      expect(mockEntityMapper.save).toHaveBeenCalledWith(entity);
       done();
     });
   });
@@ -124,12 +127,10 @@ describe("CouchdbFileService", () => {
     });
   });
 
-  it("should remove a file using the latest rev and update the entity", () => {
+  it("should remove a file using the latest rev", () => {
     mockHttp.get.and.returnValue(of({ _rev: "test_rev" }));
     mockHttp.delete.and.returnValue(of({ ok: true }));
     const entity = new Entity("testId");
-    const prop = "testProp";
-    entity[prop] = "test.file";
 
     service.removeFile(entity, "testProp").subscribe();
 
@@ -141,8 +142,6 @@ describe("CouchdbFileService", () => {
         `${attachmentUrlPrefix}/Entity:testId/testProp?rev=test_rev`
       )
     );
-    expect(entity[prop]).toBe(undefined);
-    expect(mockEntityMapper.save).toHaveBeenCalledWith(entity);
   });
 
   it("should show progress while downloading a file", () => {
@@ -196,19 +195,84 @@ describe("CouchdbFileService", () => {
   }));
 
   it("should not fail if to-be-removed file reference could not be found", () => {
-    const entity = new Entity();
-    entity["testProp"] = "some.file";
     mockHttp.get.and.returnValue(
       throwError(
         () => new HttpErrorResponse({ status: HttpStatusCode.NotFound })
       )
     );
 
-    service.removeFile(entity, "testProp").subscribe({
-      error: () => fail("Removing file should not fail"),
-    });
+    return expectAsync(
+      firstValueFrom(service.removeFile(new Entity(), "testProp"))
+    ).toBeResolved();
+  });
+
+  it("should wait for previous request to finish before starting a new one", () => {
+    const firstPut = new BehaviorSubject({ ok: true });
+    const secondPut = new BehaviorSubject({ ok: true });
+    const thirdPut = new BehaviorSubject({ ok: true });
+    const file1 = { type: "image/png", name: "file1.name" } as File;
+    const file2 = { type: "image/png", name: "file2.name" } as File;
+    const file3 = { type: "image/png", name: "file3.name" } as File;
+    const entity = new Entity("testId");
+    mockHttp.get.and.returnValues(
+      of({ _rev: "1-rev" }),
+      of({ _rev: "2-rev" }),
+      of({ _rev: "3-rev" })
+    );
+    mockHttp.put.and.returnValues(firstPut, secondPut, thirdPut);
+
+    let file1Done = false;
+    let file2Done = false;
+    let file3Done = false;
+    service
+      .uploadFile(file1, entity, "prop1")
+      .subscribe({ complete: () => (file1Done = true) });
+    service
+      .uploadFile(file2, entity, "prop2")
+      .subscribe({ complete: () => (file2Done = true) });
+    service
+      .uploadFile(file3, entity, "prop3")
+      .subscribe({ complete: () => (file3Done = true) });
+
+    expect(firstPut.observed).toBeTrue();
+    expect(secondPut.observed).toBeFalse();
+    expect(mockHttp.put).toHaveBeenCalledTimes(1);
+    expect(mockHttp.put).toHaveBeenCalledWith(
+      jasmine.stringContaining(
+        `${attachmentUrlPrefix}/Entity:testId/prop1?rev=1-rev`
+      ),
+      jasmine.anything(),
+      jasmine.anything()
+    );
 
-    expect(entity["testProp"]).toBeUndefined();
-    expect(mockEntityMapper.save).toHaveBeenCalledWith(entity);
+    firstPut.complete();
+
+    expect(file1Done).toBeTrue();
+    expect(file2Done).toBeFalse();
+    expect(file3Done).toBeFalse();
+    expect(secondPut.observed).toBeTrue();
+    expect(thirdPut.observed).toBeFalse();
+    expect(mockHttp.put).toHaveBeenCalledTimes(2);
+    expect(mockHttp.put).toHaveBeenCalledWith(
+      jasmine.stringContaining(
+        `${attachmentUrlPrefix}/Entity:testId/prop2?rev=2-rev`
+      ),
+      jasmine.anything(),
+      jasmine.anything()
+    );
+
+    secondPut.complete();
+
+    expect(file2Done).toBeTrue();
+    expect(file3Done).toBeFalse();
+    expect(thirdPut.observed).toBeTrue();
+    expect(mockHttp.put).toHaveBeenCalledTimes(3);
+    expect(mockHttp.put).toHaveBeenCalledWith(
+      jasmine.stringContaining(
+        `${attachmentUrlPrefix}/Entity:testId/prop3?rev=3-rev`
+      ),
+      jasmine.anything(),
+      jasmine.anything()
+    );
   });
 });

src/app/features/file/couchdb-file.service.ts

@@ -12,7 +12,6 @@ import {
   catchError,
   concatMap,
   filter,
-  finalize,
   map,
   shareReplay,
 } from "rxjs/operators";
@@ -26,6 +25,7 @@ import { MatSnackBar } from "@angular/material/snack-bar";
 import { ProgressComponent } from "./progress/progress.component";
 import { EntityRegistry } from "../../core/entity/database-entity.decorator";
 import { LoggingService } from "../../core/logging/logging.service";
+import { ObservableQueue } from "./observable-queue/observable-queue";
 
 /**
  * Stores the files in the CouchDB.
@@ -35,6 +35,7 @@ import { LoggingService } from "../../core/logging/logging.service";
 @Injectable()
 export class CouchdbFileService extends FileService {
   private attachmentsUrl = `${AppSettings.DB_PROXY_PREFIX}/${AppSettings.DB_NAME}-attachments`;
+  private requestQueue = new ObservableQueue();
 
   constructor(
     private http: HttpClient,
@@ -48,25 +49,27 @@ export class CouchdbFileService extends FileService {
   }
 
   uploadFile(file: File, entity: Entity, property: string): Observable<any> {
+    const obs = this.requestQueue.add(
+      this.runFileUpload(file, entity, property)
+    );
+    this.reportProgress($localize`Uploading "${file.name}"`, obs);
+    return obs;
+  }
+
+  private runFileUpload(file: File, entity: Entity, property: string) {
     const blob = new Blob([file]);
     const attachmentPath = `${this.attachmentsUrl}/${entity.getId(true)}`;
-    const obs = this.getAttachmentsDocument(attachmentPath).pipe(
+    return this.getAttachmentsDocument(attachmentPath).pipe(
       concatMap(({ _rev }) =>
         this.http.put(`${attachmentPath}/${property}?rev=${_rev}`, blob, {
           headers: { "Content-Type": file.type, "ngsw-bypass": "" },
           reportProgress: true,
           observe: "events",
         })
       ),
-      finalize(() => {
-        entity[property] = file.name;
-        this.entityMapper.save(entity);
-      }),
       // prevent http request to be executed multiple times (whenever .subscribe is called)
       shareReplay()
     );
-    this.reportProgress($localize`Uploading "${file.name}"`, obs);
-    return obs;
   }
 
   private getAttachmentsDocument(
@@ -85,6 +88,10 @@ export class CouchdbFileService extends FileService {
   }
 
   removeFile(entity: Entity, property: string) {
+    return this.requestQueue.add(this.runFileRemoval(entity, property));
+  }
+
+  private runFileRemoval(entity: Entity, property: string) {
     const attachmentPath = `${this.attachmentsUrl}/${entity.getId(true)}`;
     return this.http.get<{ _rev: string }>(attachmentPath).pipe(
       concatMap(({ _rev }) =>
@@ -96,15 +103,15 @@ export class CouchdbFileService extends FileService {
         } else {
           throw err;
         }
-      }),
-      finalize(() => {
-        entity[property] = undefined;
-        this.entityMapper.save(entity);
       })
     );
   }
 
   removeAllFiles(entity: Entity): Observable<any> {
+    return this.requestQueue.add(this.runAllFilesRemoval(entity));
+  }
+
+  private runAllFilesRemoval(entity: Entity) {
     const attachmentPath = `${this.attachmentsUrl}/${entity.getId(true)}`;
     return this.http
       .get<{ _rev: string }>(attachmentPath)