From 5cdb17cbebf49147ebd754efe331fddc38327752 Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 2 Dec 2022 18:02:26 +0100 Subject: [PATCH] fix(file-upload): resolve issues saving entities with files (#1562) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The file upload has been restructured to be more in-line with other form components and to support the automatic form-update functionality. * Uploading/removing files only happens when 'save' is clicked * No more confirmation popup when replacing/deleting files, as this can be undone by clicking 'cancel' * PUT-Requests to CouchDB are executed in sequence in order to support uploading of multiple files at the same time * EntityFormComponent has been cleaned up to less often create new forms. Now it only creates the form one and then updates the values when needed. This functionality has been developed for the project “QualitätsMENTOR”. QualitätsMENTOR is developed under the projects “Landungsbrücken – Patenschaften in Hamburg stärken” and “openTransfer Patenschaften”. It is funded through the program “Menschen stärken Menschen” by the German Federal Ministry of Family Affairs, Senior Citizens, Women and Youth. More information at https://github.com/qualitaetsmentor “Landungsbrücken – Patenschaften in Hamburg stärken” is a project of BürgerStiftung Hamburg in cooperation with the Mentor.Ring Hamburg. With a mix of networking opportunities, capacity building and financial support the project strengthens Hamburg’s scene of mentoring projects since its founding in 2016. The “Stiftung Bürgermut” foundation since 2007 supports the digital and real exchange of experiences and connections of active citizens. Within the federal program “Menschen stärken Menschen” the foundation as part of its program “openTransfer Patenschaften” offers support services for connecting, spreading and upskilling mentoring organisations across Germany. Diese Funktion wurde entwickelt für das Projekt QualitätsMENTOR. Der QualitätsMENTOR wird entwickelt im Rahmen der Projekte Landungsbrücken – Patenschaften in Hamburg stärken und openTransfer Patenschaften. Er ist gefördert durch das Bundesprogramm Menschen stärken Menschen des Bundesministeriums für Familie, Senioren, Frauen und Jugend. Mehr Informationen unter https://github.com/qualitaetsmentor “Landungsbrücken – Patenschaften in Hamburg stärken” ist ein Projekt der BürgerStiftung Hamburg in Kooperation mit dem Mentor.Ring Hamburg. Mit einer Mischung aus Vernetzungsangeboten, Qualifizierungsmaßnahmen und finanzieller Förderung stärkt das Projekt die Hamburger Szene der Patenschaftsprojekte seit der Gründung im Jahr 2016. Die Stiftung Bürgermut fördert seit 2007 den digitalen und realen Erfahrungsaustausch und die Vernetzung von engagierten Bürger:innen. Innerhalb des Bundesprogramms „Menschen stärken Menschen” bietet die Stiftung im Rahmen ihres Programms openTransfer Patenschaften Unterstützungsleistungen zur Vernetzung, Verbreitung und Qualifizierung von Patenschafts- und Mentoringorganisationen bundesweit. Co-authored-by: QualitaetsMENTOR <117934638+QualitaetsMENTOR@users.noreply.github.com> --- .../entity-form/entity-form.component.spec.ts | 27 ++ .../entity-form/entity-form.component.ts | 33 ++- .../file/couchdb-file.service.spec.ts | 108 ++++++-- src/app/features/file/couchdb-file.service.ts | 31 ++- .../edit-file/edit-file.component.spec.ts | 246 ++++++++++++++---- .../file/edit-file/edit-file.component.ts | 79 ++++-- src/app/features/file/file.service.ts | 10 +- .../features/file/mock-file.service.spec.ts | 16 +- src/app/features/file/mock-file.service.ts | 12 +- .../observable-queue/observable-queue.spec.ts | 166 ++++++++++++ .../file/observable-queue/observable-queue.ts | 18 ++ 11 files changed, 603 insertions(+), 143 deletions(-) create mode 100644 src/app/features/file/observable-queue/observable-queue.spec.ts create mode 100644 src/app/features/file/observable-queue/observable-queue.ts diff --git a/src/app/core/entity-components/entity-form/entity-form/entity-form.component.spec.ts b/src/app/core/entity-components/entity-form/entity-form/entity-form.component.spec.ts index f057bdb18c..5bdd579aa2 100644 --- a/src/app/core/entity-components/entity-form/entity-form/entity-form.component.spec.ts +++ b/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); + }); }); diff --git a/src/app/core/entity-components/entity-form/entity-form/entity-form.component.ts b/src/app/core/entity-components/entity-form/entity-form/entity-form.component.ts index 1e2a0cc354..3a5fe39330 100644 --- a/src/app/core/entity-components/entity-form/entity-form/entity-form.component.ts +++ b/src/app/core/entity-components/entity-form/entity-form/entity-form.component.ts @@ -76,6 +76,7 @@ export class EntityFormComponent implements OnInit { form: EntityForm; private saveInProgress = false; + private initialFormValues: any; constructor( private entityFormService: EntityFormService, @@ -86,8 +87,8 @@ export class EntityFormComponent 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 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 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 implements OnInit { cancelClicked() { this.cancel.emit(); - this.buildFormConfig(); + this.resetForm(); + this.form.disable(); } - private buildFormConfig(entity = this.entity) { + private buildFormConfig() { const flattenedFormFields = new Array().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(); } } diff --git a/src/app/features/file/couchdb-file.service.spec.ts b/src/app/features/file/couchdb-file.service.spec.ts index c59275d7d6..9bd55eaed6 100644 --- a/src/app/features/file/couchdb-file.service.spec.ts +++ b/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; let mockDialog: jasmine.SpyObj; - let mockEntityMapper: jasmine.SpyObj; let mockSnackbar: jasmine.SpyObj; let dismiss: jasmine.Spy; let updates: Subject>; @@ -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() + ); }); }); diff --git a/src/app/features/file/couchdb-file.service.ts b/src/app/features/file/couchdb-file.service.ts index 98afe4babc..73a408c14e 100644 --- a/src/app/features/file/couchdb-file.service.ts +++ b/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,9 +49,17 @@ export class CouchdbFileService extends FileService { } uploadFile(file: File, entity: Entity, property: string): Observable { + 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": "" }, @@ -58,15 +67,9 @@ export class CouchdbFileService extends FileService { 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 { + 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) diff --git a/src/app/features/file/edit-file/edit-file.component.spec.ts b/src/app/features/file/edit-file/edit-file.component.spec.ts index d96da7ec4e..d21cb12bfa 100644 --- a/src/app/features/file/edit-file/edit-file.component.spec.ts +++ b/src/app/features/file/edit-file/edit-file.component.spec.ts @@ -1,28 +1,24 @@ -import { - ComponentFixture, - fakeAsync, - TestBed, - tick, -} from "@angular/core/testing"; +import { ComponentFixture, TestBed } from "@angular/core/testing"; import { EditFileComponent } from "./edit-file.component"; import { FileModule } from "../file.module"; import { AlertService } from "../../../core/alerts/alert.service"; -import { ConfirmationDialogService } from "../../../core/confirmation-dialog/confirmation-dialog.service"; import { FormControl } from "@angular/forms"; import { FontAwesomeTestingModule } from "@fortawesome/angular-fontawesome/testing"; -import { of, throwError } from "rxjs"; +import { of, Subject } from "rxjs"; import { Entity } from "app/core/entity/model/entity"; import { NoopAnimationsModule } from "@angular/platform-browser/animations"; import { FileService } from "../file.service"; import { EntitySchemaService } from "../../../core/entity/schema/entity-schema.service"; +import { EntityMapperService } from "../../../core/entity/entity-mapper.service"; describe("EditFileComponent", () => { let component: EditFileComponent; let fixture: ComponentFixture; let mockFileService: jasmine.SpyObj; - let mockConfirmationDialog: jasmine.SpyObj; let mockAlertService: jasmine.SpyObj; + let mockEntityMapper: jasmine.SpyObj; + const file = { name: "test.file" } as File; const fileEvent = { target: { files: [file] } }; @@ -32,110 +28,252 @@ describe("EditFileComponent", () => { "showFile", "removeFile", ]); - mockConfirmationDialog = jasmine.createSpyObj(["getConfirmation"]); mockAlertService = jasmine.createSpyObj(["addDanger", "addInfo"]); + mockEntityMapper = jasmine.createSpyObj(["save"]); await TestBed.configureTestingModule({ imports: [FileModule, FontAwesomeTestingModule, NoopAnimationsModule], providers: [ EntitySchemaService, { provide: AlertService, useValue: mockAlertService }, { provide: FileService, useValue: mockFileService }, - { - provide: ConfirmationDialogService, - useValue: mockConfirmationDialog, - }, + { provide: EntityMapperService, useValue: mockEntityMapper }, ], }).compileComponents(); fixture = TestBed.createComponent(EditFileComponent); component = fixture.componentInstance; - component.formControl = new FormControl(""); - component.entity = new Entity(); - component.formControlName = "testProp"; - fixture.detectChanges(); }); it("should create", () => { expect(component).toBeTruthy(); }); - it("update the form after successfully uploading the file", () => { + it("should not upload a file that was selected but the process was canceled", () => { + setupComponent(); + component.formControl.enable(); + + component.onFileSelected(fileEvent); + expect(component.formControl).toHaveValue(file.name); + cancelForm(); + + expect(component.formControl).toHaveValue(null); + expect(mockFileService.uploadFile).not.toHaveBeenCalled(); + }); + + it("should not remove or upload a file if a file was selected and then removed and then canceled", () => { + setupComponent(); + component.formControl.enable(); + + component.onFileSelected(fileEvent); + expect(component.formControl).toHaveValue(file.name); + + component.delete(); + + cancelForm(); + + expect(component.formControl).toHaveValue(null); + expect(mockFileService.uploadFile).not.toHaveBeenCalled(); + expect(mockFileService.removeFile).not.toHaveBeenCalled(); + }); + + it("should not remove or upload a file if a file was selected, replaced and canceled", () => { + setupComponent(); + component.formControl.enable(); + + component.onFileSelected(fileEvent); + expect(component.formControl).toHaveValue(file.name); + + const otherFileEvent = { target: { files: [{ name: "other.file" }] } }; + component.onFileSelected(otherFileEvent); + expect(component.formControl).toHaveValue("other.file"); + + cancelForm(); + + expect(component.formControl).toHaveValue(null); + expect(mockFileService.uploadFile).not.toHaveBeenCalled(); + expect(mockFileService.removeFile).not.toHaveBeenCalled(); + }); + + it("should upload a file if a new file was selected and the form saved", () => { + setupComponent(); mockFileService.uploadFile.and.returnValue(of({ ok: true })); + component.formControl.enable(); component.onFileSelected(fileEvent); + expect(component.formControl).toHaveValue(file.name); + + component.formControl.disable(); + expect(component.formControl).toHaveValue(file.name); expect(mockFileService.uploadFile).toHaveBeenCalledWith( file, component.entity, component.formControlName ); - expect(component.formControl).toHaveValue(file.name); }); - it("should not replace existing file if user declines", fakeAsync(() => { - mockFileService.uploadFile.and.returnValue(of({ ok: true })); - mockConfirmationDialog.getConfirmation.and.resolveTo(false); - component.formControl.setValue("existing.file"); + it("should not remove or upload a file if a file was selected, then removed and then saved", () => { + setupComponent(); + component.formControl.enable(); component.onFileSelected(fileEvent); - tick(); + expect(component.formControl).toHaveValue(file.name); + + component.delete(); - expect(mockConfirmationDialog.getConfirmation).toHaveBeenCalled(); + component.formControl.disable(); + + expect(component.formControl).toHaveValue(null); expect(mockFileService.uploadFile).not.toHaveBeenCalled(); - })); + expect(mockFileService.removeFile).not.toHaveBeenCalled(); + }); - it("should show upload errors as an alert", () => { - mockFileService.uploadFile.and.returnValue(throwError(() => new Error())); + it("should only upload the last file if a file was selected and then replaced", () => { + setupComponent(); + mockFileService.uploadFile.and.returnValue(of({ ok: true })); + component.formControl.enable(); component.onFileSelected(fileEvent); + expect(component.formControl).toHaveValue(file.name); - expect(mockAlertService.addDanger).toHaveBeenCalled(); - expect(component.formControl).not.toHaveValue(file.name); - }); + const otherFile = { name: "other.file" } as File; + const otherFileEvent = { target: { files: [otherFile] } }; + component.onFileSelected(otherFileEvent); + expect(component.formControl).toHaveValue(otherFile.name); - it("should show a file when clicking on the form element", () => { component.formControl.disable(); - component.formControl.setValue("existing.file"); - - component.formClicked(); - expect(mockFileService.showFile).toHaveBeenCalledWith( + expect(component.formControl).toHaveValue(otherFile.name); + expect(mockFileService.removeFile).not.toHaveBeenCalled(); + expect(mockFileService.uploadFile).toHaveBeenCalledWith( + otherFile, component.entity, component.formControlName ); }); - it("should not open a file if no value is set", () => { - component.fileClicked(); + it("should not remove a file if the file input was cleared but then canceled", () => { + setupComponent(file.name); + component.formControl.enable(); - expect(mockFileService.showFile).not.toHaveBeenCalled(); + component.delete(); + expect(component.formControl).toHaveValue(null); + + cancelForm(); + + expect(component.formControl).toHaveValue(file.name); + expect(mockFileService.removeFile).not.toHaveBeenCalled(); }); - it("should reset the form control after a file has been deleted", fakeAsync(() => { - mockConfirmationDialog.getConfirmation.and.resolveTo(true); + it("should not remove and upload a file if the file was replaced but then canceled", () => { + setupComponent(file.name); + component.formControl.enable(); + + const otherFileEvent = { target: { files: [{ name: "other.file" }] } }; + component.onFileSelected(otherFileEvent); + expect(component.formControl).toHaveValue("other.file"); + + cancelForm(); + + expect(component.formControl).toHaveValue(file.name); + expect(mockFileService.removeFile).not.toHaveBeenCalled(); + expect(mockFileService.uploadFile).not.toHaveBeenCalled(); + }); + + it("should remove a file if the file input was cleared and saved", () => { mockFileService.removeFile.and.returnValue(of({ ok: true })); - component.formControl.setValue("existing.file"); + setupComponent(file.name); + component.formControl.enable(); component.delete(); - tick(); + expect(component.formControl).toHaveValue(null); + component.formControl.disable(); + + expect(component.formControl).toHaveValue(null); expect(mockFileService.removeFile).toHaveBeenCalledWith( component.entity, component.formControlName ); - expect(component.formControl).toHaveValue(undefined); expect(mockAlertService.addInfo).toHaveBeenCalled(); - })); + }); - it("should not remove a file if the user declines", fakeAsync(() => { - mockConfirmationDialog.getConfirmation.and.resolveTo(false); - component.formControl.setValue("existing.file"); + it("should upload the new file if the file was replaced and then saved", () => { + mockFileService.uploadFile.and.returnValue(of({ ok: true })); + setupComponent(file.name); + component.formControl.enable(); - component.delete(); - tick(); + const otherFile = { name: "other.file" } as File; + const otherFileEvent = { target: { files: [otherFile] } }; + component.onFileSelected(otherFileEvent); + + component.formControl.disable(); + expect(component.formControl).toHaveValue(otherFile.name); expect(mockFileService.removeFile).not.toHaveBeenCalled(); - expect(component.formControl).toHaveValue("existing.file"); - expect(mockAlertService.addInfo).not.toHaveBeenCalled(); - })); + expect(mockFileService.uploadFile).toHaveBeenCalledWith( + otherFile, + component.entity, + component.formControlName + ); + }); + + it("should show upload errors as an alert and reset entity", () => { + setupComponent("old.file"); + const subject = new Subject(); + mockFileService.uploadFile.and.returnValue(subject); + component.formControl.enable(); + + component.onFileSelected(fileEvent); + + component.entity[component.formControlName] = file.name; + component.formControl.disable(); + + expect(component.formControl).toHaveValue(file.name); + expect(component.entity[component.formControlName]).toBe(file.name); + + subject.error(new Error()); + + expect(mockAlertService.addDanger).toHaveBeenCalled(); + expect(component.formControl).toHaveValue("old.file"); + expect(component.entity[component.formControlName]).toBe("old.file"); + expect(mockEntityMapper.save).toHaveBeenCalledWith(component.entity); + }); + + it("should show a file when clicking on the form element", () => { + setupComponent("existing.file"); + + component.formClicked(); + + expect(mockFileService.showFile).toHaveBeenCalledWith( + component.entity, + component.formControlName + ); + }); + + it("should not open a file if no value is set", () => { + setupComponent(); + component.fileClicked(); + + expect(mockFileService.showFile).not.toHaveBeenCalled(); + }); + + let initialValue: string; + + function setupComponent(value = null) { + initialValue = value; + component.onInitFromDynamicConfig({ + formControl: new FormControl(initialValue), + entity: Object.assign(new Entity(), { testProp: initialValue }), + formFieldConfig: { id: "testProp" }, + propertySchema: undefined, + }); + component.formControl.disable(); + fixture.detectChanges(); + } + + function cancelForm() { + component.formControl.setValue(initialValue); + component.formControl.disable(); + } }); diff --git a/src/app/features/file/edit-file/edit-file.component.ts b/src/app/features/file/edit-file/edit-file.component.ts index f427f8883c..9e0edb189d 100644 --- a/src/app/features/file/edit-file/edit-file.component.ts +++ b/src/app/features/file/edit-file/edit-file.component.ts @@ -1,10 +1,14 @@ import { Component, ElementRef, ViewChild } from "@angular/core"; -import { EditComponent } from "../../../core/entity-components/entity-utils/dynamic-form-components/edit-component"; +import { + EditComponent, + EditPropertyConfig, +} from "../../../core/entity-components/entity-utils/dynamic-form-components/edit-component"; import { DynamicComponent } from "../../../core/view/dynamic-components/dynamic-component.decorator"; import { AlertService } from "../../../core/alerts/alert.service"; import { LoggingService } from "../../../core/logging/logging.service"; -import { ConfirmationDialogService } from "../../../core/confirmation-dialog/confirmation-dialog.service"; import { FileService } from "../file.service"; +import { distinctUntilChanged, filter } from "rxjs/operators"; +import { EntityMapperService } from "../../../core/entity/entity-mapper.service"; /** * This component should be used as a `editComponent` when a property should store files. @@ -18,43 +22,67 @@ import { FileService } from "../file.service"; }) export class EditFileComponent extends EditComponent { @ViewChild("fileUpload") fileInput: ElementRef; + private selectedFile: File; + private removeClicked = false; + private initialValue: string; constructor( private fileService: FileService, private alertService: AlertService, private logger: LoggingService, - private confirmationDialog: ConfirmationDialogService + private entityMapper: EntityMapperService ) { super(); } + onInitFromDynamicConfig(config: EditPropertyConfig) { + super.onInitFromDynamicConfig(config); + this.initialValue = this.formControl.value; + this.formControl.statusChanges + .pipe( + distinctUntilChanged(), + filter((change) => change === "DISABLED") + ) + .subscribe(() => { + if ( + this.selectedFile && + this.selectedFile.name === this.formControl.value + ) { + this.uploadFile(this.selectedFile); + } else if (this.removeClicked && !this.formControl.value) { + this.removeFile(); + } + }); + } + async onFileSelected(event) { const file: File = event.target.files[0]; // directly reset input so subsequent selections with the same name also trigger the change event this.fileInput.nativeElement.value = ""; + this.selectedFile = file; + this.formControl.setValue(file.name); + } - if (this.formControl.value) { - const shouldReplace = await this.confirmationDialog.getConfirmation( - $localize`Replacing file`, - $localize`Do you want to replace the file "${this.formControl.value}" with "${file.name}"?` - ); - if (!shouldReplace) { - return; - } - } - + private uploadFile(file: File) { // The maximum file size which can be processed by CouchDB before a timeout is around 200mb this.fileService .uploadFile(file, this.entity, this.formControlName) .subscribe({ error: (err) => this.handleError(err), - complete: () => this.formControl.setValue(file.name), + complete: () => { + this.initialValue = this.formControl.value; + this.selectedFile = undefined; + }, }); } private handleError(err) { this.logger.error("Failed uploading file: " + JSON.stringify(err)); this.alertService.addDanger("Could not upload file, please try again."); + // Reset entity to how it was before + this.entity[this.formControlName] = this.initialValue; + this.formControl.setValue(this.initialValue); + return this.entityMapper.save(this.entity); } formClicked() { @@ -69,19 +97,22 @@ export class EditFileComponent extends EditComponent { } } - async delete() { - const shouldDelete = await this.confirmationDialog.getConfirmation( - $localize`Deleting file`, - $localize`Do you want to delete the file "${this.formControl.value}"?` - ); - if (!shouldDelete) { - return; - } + delete() { + this.formControl.setValue(null); + this.selectedFile = undefined; + // remove is only necessary if an initial value was set + this.removeClicked = !!this.initialValue; + } + + private removeFile() { this.fileService .removeFile(this.entity, this.formControlName) .subscribe(() => { - this.formControl.setValue(undefined); - this.alertService.addInfo($localize`:Message for user:File deleted`); + this.alertService.addInfo( + $localize`:Message for user:File "${this.initialValue}" deleted` + ); + this.initialValue = undefined; + this.removeClicked = false; }); } } diff --git a/src/app/features/file/file.service.ts b/src/app/features/file/file.service.ts index 6cbfefe455..f75f28c1eb 100644 --- a/src/app/features/file/file.service.ts +++ b/src/app/features/file/file.service.ts @@ -12,9 +12,9 @@ import { LoggingService } from "../../core/logging/logging.service"; */ export abstract class FileService { protected constructor( - protected entityMapper: EntityMapperService, - protected entities: EntityRegistry, - protected logger: LoggingService + private entityMapper: EntityMapperService, + private entities: EntityRegistry, + private logger: LoggingService ) { // TODO maybe registration is to late (only when component is rendered) this.deleteFilesOfDeletedEntities(); @@ -59,7 +59,7 @@ export abstract class FileService { } /** - * Removes a file and updates the entity to reflect this change + * Removes the file * @param entity * @param property of the entity which points to a file */ @@ -79,7 +79,7 @@ export abstract class FileService { abstract showFile(entity: Entity, property: string): void; /** - * Uploads the file and stores the information on `entity[property]` + * Uploads the file * @param file to be uploaded * @param entity * @param property where the information about the file should be stored diff --git a/src/app/features/file/mock-file.service.spec.ts b/src/app/features/file/mock-file.service.spec.ts index c5ba2b6066..e7a864f771 100644 --- a/src/app/features/file/mock-file.service.spec.ts +++ b/src/app/features/file/mock-file.service.spec.ts @@ -11,15 +11,15 @@ import { describe("MockFileService", () => { let service: MockFileService; - let mockEntityMapper: jasmine.SpyObj; beforeEach(() => { - mockEntityMapper = jasmine.createSpyObj(["save", "receiveUpdates"]); - mockEntityMapper.receiveUpdates.and.returnValue(NEVER); TestBed.configureTestingModule({ providers: [ MockFileService, - { provide: EntityMapperService, useValue: mockEntityMapper }, + { + provide: EntityMapperService, + useValue: { receiveUpdates: () => NEVER }, + }, { provide: EntityRegistry, useValue: entityRegistry }, ], }); @@ -39,9 +39,7 @@ describe("MockFileService", () => { await firstValueFrom(service.uploadFile(file, entity, prop)); - expect(entity[prop]).toBe(file.name); expect(URL.createObjectURL).toHaveBeenCalledWith(file); - expect(mockEntityMapper.save).toHaveBeenCalledWith(entity); service.showFile(entity, prop); @@ -51,11 +49,9 @@ describe("MockFileService", () => { it("should remove a file from a entity", async () => { const entity = new Entity(); const prop = "testProp"; - entity[prop] = "test.file"; - await firstValueFrom(service.removeFile(entity, prop)); + const res = await firstValueFrom(service.removeFile(entity, prop)); - expect(entity[prop]).toBe(undefined); - expect(mockEntityMapper.save).toHaveBeenCalledWith(entity); + expect(res).toEqual({ ok: true }); }); }); diff --git a/src/app/features/file/mock-file.service.ts b/src/app/features/file/mock-file.service.ts index 370b78ab30..b6f8716258 100644 --- a/src/app/features/file/mock-file.service.ts +++ b/src/app/features/file/mock-file.service.ts @@ -24,9 +24,8 @@ export class MockFileService extends FileService { } removeFile(entity: Entity, property: string): Observable { - this.fileMap.delete(entity[property]); - entity[property] = undefined; - return of(this.entityMapper.save(entity)); + this.fileMap.delete(entity + property); + return of({ ok: true }); } removeAllFiles(entity: Entity): Observable { @@ -34,13 +33,12 @@ export class MockFileService extends FileService { } showFile(entity: Entity, property: string): void { - window.open(this.fileMap.get(entity[property]), "_blank"); + window.open(this.fileMap.get(entity + property), "_blank"); } uploadFile(file: File, entity: Entity, property: string): Observable { const fileURL = URL.createObjectURL(file); - this.fileMap.set(file.name, fileURL); - entity[property] = file.name; - return of(this.entityMapper.save(entity)); + this.fileMap.set(entity + property, fileURL); + return of({ ok: true }); } } diff --git a/src/app/features/file/observable-queue/observable-queue.spec.ts b/src/app/features/file/observable-queue/observable-queue.spec.ts new file mode 100644 index 0000000000..1547685b45 --- /dev/null +++ b/src/app/features/file/observable-queue/observable-queue.spec.ts @@ -0,0 +1,166 @@ +import { ObservableQueue } from "./observable-queue"; +import { BehaviorSubject, of, Subject } from "rxjs"; +import { tap } from "rxjs/operators"; + +describe("ObservableQueue", () => { + let queue: ObservableQueue; + + beforeEach(() => { + queue = new ObservableQueue(); + }); + + it("should only start a new job after the previous one is done", () => { + const job1 = new Subject(); + const job2 = new Subject(); + const job3 = new BehaviorSubject(1); + let job1Done = false; + let job2Done = false; + let job3Done = false; + queue.add(job1).subscribe(() => (job1Done = true)); + queue.add(job2).subscribe(() => (job2Done = true)); + queue.add(job3).subscribe(() => (job3Done = true)); + + expect(job1.observed).toBeTrue(); + expect(job1Done).toBeFalse(); + expect(job2.observed).toBeFalse(); + expect(job2Done).toBeFalse(); + expect(job3.observed).toBeFalse(); + expect(job3Done).toBeFalse(); + + job1.next(undefined); + job1.complete(); + + expect(job1Done).toBeTrue(); + expect(job2.observed).toBeTrue(); + expect(job2Done).toBeFalse(); + expect(job3.observed).toBeFalse(); + expect(job3Done).toBeFalse(); + + job2.next(undefined); + job2.complete(); + + expect(job1Done).toBeTrue(); + expect(job2Done).toBeTrue(); + // Job 3 is executed once 2 is done + expect(job3Done).toBeTrue(); + }); + + it("should directly run a job if the previous one is already done", () => { + let job1done = false; + queue.add(of(1)).subscribe(() => (job1done = true)); + + expect(job1done).toBeTrue(); + + let job2done = true; + queue.add(of(1)).subscribe(() => (job2done = true)); + expect(job2done).toBeTrue(); + }); + + it("should not run the same observable multiple times", () => { + let job1Calls = 0; + const job1 = of(1).pipe(tap(() => job1Calls++)); + + queue.add(job1).subscribe(); + expect(job1Calls).toBe(1); + + queue.add(of(2)).subscribe(); + queue.add(of(3)).subscribe(); + expect(job1Calls).toBe(1); + }); + + it("should continue running observables if one throws an error", () => { + const errorJob = new Subject(); + const normalJob = new Subject(); + let errorJobDone = false; + let errorJobFailed = false; + let normalJobDone = false; + let normalJobFailed = false; + + queue.add(errorJob).subscribe({ + next: () => (errorJobDone = true), + error: () => (errorJobFailed = true), + }); + queue.add(normalJob).subscribe({ + next: () => (normalJobDone = true), + error: () => (normalJobFailed = true), + }); + + expect(errorJob.observed).toBeTrue(); + expect(normalJob.observed).toBeFalse(); + expect(errorJobFailed).toBeFalse(); + expect(errorJobDone).toBeFalse(); + expect(normalJobFailed).toBeFalse(); + expect(normalJobDone).toBeFalse(); + + errorJob.error(new Error()); + + expect(normalJob.observed).toBeTrue(); + expect(errorJobFailed).toBeTrue(); + expect(errorJobDone).toBeFalse(); + expect(normalJobFailed).toBeFalse(); + expect(normalJobDone).toBeFalse(); + + normalJob.next(undefined); + normalJob.complete(); + + expect(errorJobFailed).toBeTrue(); + expect(errorJobDone).toBeFalse(); + expect(normalJobFailed).toBeFalse(); + expect(normalJobDone).toBeTrue(); + }); + + it("should return the result of the correct observable", () => { + const job1 = new Subject(); + const job2 = of("job2"); + let res1; + let res2; + queue.add(job1).subscribe((res) => (res1 = res)); + queue.add(job2).subscribe((res) => (res2 = res)); + + expect(res1).toBeUndefined(); + expect(res1).toBeUndefined(); + + job1.next({ value: "job1 res" }); + job1.complete(); + + expect(res1).toEqual({ value: "job1 res" }); + expect(res2).toBe("job2"); + }); + + it("should only run next job if previous completes", () => { + const job1 = new Subject(); + const job2 = new Subject(); + let job1res: string; + let job1done = false; + let job2done = false; + queue.add(job1).subscribe({ + next: (res) => (job1res = res), + complete: () => (job1done = true), + }); + queue.add(job2).subscribe({ complete: () => (job2done = true) }); + + expect(job1.observed).toBeTrue(); + expect(job2.observed).toBeFalse(); + expect(job1done).toBeFalse(); + expect(job2done).toBeFalse(); + + job1.next("some"); + expect(job1res).toBe("some"); + job1.next("values"); + expect(job1res).toBe("values"); + job1.next("emitted"); + expect(job1res).toBe("emitted"); + + expect(job1.observed).toBeTrue(); + expect(job2.observed).toBeFalse(); + expect(job1done).toBeFalse(); + expect(job2done).toBeFalse(); + + job1.complete(); + + expect(job1res).toBe("emitted"); + expect(job2.observed).toBeTrue(); + expect(job1done).toBeTrue(); + expect(job2done).toBeFalse(); + }); +}); diff --git a/src/app/features/file/observable-queue/observable-queue.ts b/src/app/features/file/observable-queue/observable-queue.ts new file mode 100644 index 0000000000..3a9906b9d0 --- /dev/null +++ b/src/app/features/file/observable-queue/observable-queue.ts @@ -0,0 +1,18 @@ +import { last, Observable, of } from "rxjs"; +import { catchError, concatMap, shareReplay } from "rxjs/operators"; + +export class ObservableQueue { + private jobQueue: Observable = of(undefined); + + add(obs: Observable): Observable { + const newJob = this.jobQueue.pipe( + concatMap(() => obs), + shareReplay() + ); + this.jobQueue = newJob.pipe( + last(), + catchError(() => of(undefined)) + ); + return newJob; + } +}