From 1332ada6c579f1175590baffa16900f6b1071ce1 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Mon, 3 Aug 2026 16:46:05 +0200 Subject: [PATCH] Show the uploaded file name in submission upload notifications Dropping several files into a submission produced identical "Upload successful" toasts, so a single failure inside a batch was impossible to attribute. Each notification now names the file it refers to, falling back to the existing generic messages when no client-side name is available. The uploader gains an additive `onCompleteItemWithFile` output carrying the parsed response together with the file name. The existing `onCompleteItem` is retained and still emits the bare response first, so the other consumers of `ds-uploader` are unaffected. `onUploadError` is retyped from `any` to the existing `UploaderError`, and a single `getNotificationContent()` helper is now the only place an upload notification key appears. Locales that have not translated the two new keys render the generic message via the `default` interpolate param that MissingTranslationHelper already honours, rather than a raw dotted key. Co-Authored-By: Claude Opus 5 (1M context) --- .../uploader/uploader-complete-event.model.ts | 17 ++ .../uploader/uploader.component.spec.ts | 122 ++++++++++++ .../upload/uploader/uploader.component.ts | 12 +- .../submission-upload-files.component.html | 4 +- .../submission-upload-files.component.spec.ts | 174 +++++++++++++++++- .../submission-upload-files.component.ts | 45 ++++- src/assets/i18n/en.json5 | 4 + 7 files changed, 362 insertions(+), 16 deletions(-) create mode 100644 src/app/shared/upload/uploader/uploader-complete-event.model.ts diff --git a/src/app/shared/upload/uploader/uploader-complete-event.model.ts b/src/app/shared/upload/uploader/uploader-complete-event.model.ts new file mode 100644 index 00000000000..59cda3e11c7 --- /dev/null +++ b/src/app/shared/upload/uploader/uploader-complete-event.model.ts @@ -0,0 +1,17 @@ +/** + * An interface that represents a completed single-file upload, carrying both the + * parsed response body and the client-side file name of the file that completed. + */ +export interface UploaderCompleteEvent { + /** + * The parsed response body (e.g. the submission object returned by REST) + */ + response: any; + + /** + * The client-side name of the file that completed uploading. Present only when a + * non-empty file name is known — an empty file name is never emitted, so the presence + * of this key means a usable name is available. Whitespace-only names are not trimmed. + */ + fileName?: string; +} diff --git a/src/app/shared/upload/uploader/uploader.component.spec.ts b/src/app/shared/upload/uploader/uploader.component.spec.ts index 2bd66227702..59fa45ad2a9 100644 --- a/src/app/shared/upload/uploader/uploader.component.spec.ts +++ b/src/app/shared/upload/uploader/uploader.component.spec.ts @@ -29,6 +29,21 @@ describe('UploaderComponent', () => { let testFixture: ComponentFixture; let html; + /** + * Bring an injected UploaderComponent instance to the state in which its ng2-file-upload + * callbacks are installed, so that `app.uploader.onCompleteItem(...)` runs the component's code. + */ + const driveUploader = (app: UploaderComponent): void => { + app.uploadFilesOptions = Object.assign(new UploaderOptions(), { + url: 'http://test', + authToken: null, + disableMultipart: false, + itemAlias: null, + }); + app.ngOnInit(); + app.ngAfterViewInit(); + }; + // waitForAsync beforeEach beforeEach(waitForAsync(() => { @@ -68,6 +83,113 @@ describe('UploaderComponent', () => { expect(app).toBeDefined(); })); + it('should emit both the legacy completion output and the new completion event on a completed upload', inject([UploaderComponent], (app: UploaderComponent) => { + driveUploader(app); + spyOn(app.onCompleteItem, 'emit'); + spyOn(app.onCompleteItemWithFile, 'emit'); + + const parsed = { foo: 'bar' }; + app.uploader.onCompleteItem({ file: { name: 'test.pdf' } } as any, JSON.stringify(parsed), 200, {}); + + expect(app.onCompleteItem.emit).toHaveBeenCalledWith(parsed); + expect(app.onCompleteItem.emit).toHaveBeenCalledTimes(1); + expect(app.onCompleteItemWithFile.emit).toHaveBeenCalledWith({ response: parsed, fileName: 'test.pdf' }); + })); + + it('should emit a distinct file name for each of two sequential completed uploads', inject([UploaderComponent], (app: UploaderComponent) => { + driveUploader(app); + spyOn(app.onCompleteItemWithFile, 'emit'); + + app.uploader.onCompleteItem({ file: { name: 'first.pdf' } } as any, JSON.stringify({ n: 1 }), 200, {}); + app.uploader.onCompleteItem({ file: { name: 'second.pdf' } } as any, JSON.stringify({ n: 2 }), 200, {}); + + const calls = (app.onCompleteItemWithFile.emit as jasmine.Spy).calls; + expect(calls.count()).toBe(2); + expect(calls.argsFor(0)[0]).toEqual({ response: { n: 1 }, fileName: 'first.pdf' }); + expect(calls.argsFor(1)[0]).toEqual({ response: { n: 2 }, fileName: 'second.pdf' }); + })); + + it('should omit fileName from the completion event when the item is undefined', inject([UploaderComponent], (app: UploaderComponent) => { + driveUploader(app); + spyOn(app.onCompleteItemWithFile, 'emit'); + + app.uploader.onCompleteItem(undefined, JSON.stringify({ foo: 'bar' }), 200, {}); + + const arg = (app.onCompleteItemWithFile.emit as jasmine.Spy).calls.mostRecent().args[0]; + expect(app.onCompleteItemWithFile.emit).toHaveBeenCalledWith({ response: { foo: 'bar' } }); + expect(Object.keys(arg)).toEqual(['response']); + expect('fileName' in arg).toBeFalse(); + })); + + it('should omit fileName from the completion event when the item has no file', inject([UploaderComponent], (app: UploaderComponent) => { + driveUploader(app); + spyOn(app.onCompleteItemWithFile, 'emit'); + + app.uploader.onCompleteItem({} as any, JSON.stringify({ foo: 'bar' }), 200, {}); + + const arg = (app.onCompleteItemWithFile.emit as jasmine.Spy).calls.mostRecent().args[0]; + expect(app.onCompleteItemWithFile.emit).toHaveBeenCalledWith({ response: { foo: 'bar' } }); + expect(Object.keys(arg)).toEqual(['response']); + expect('fileName' in arg).toBeFalse(); + })); + + it('should omit fileName from the completion event when the file has no name', inject([UploaderComponent], (app: UploaderComponent) => { + driveUploader(app); + spyOn(app.onCompleteItemWithFile, 'emit'); + + app.uploader.onCompleteItem({ file: {} } as any, JSON.stringify({ foo: 'bar' }), 200, {}); + + const arg = (app.onCompleteItemWithFile.emit as jasmine.Spy).calls.mostRecent().args[0]; + expect(app.onCompleteItemWithFile.emit).toHaveBeenCalledWith({ response: { foo: 'bar' } }); + expect(Object.keys(arg)).toEqual(['response']); + expect('fileName' in arg).toBeFalse(); + })); + + it('should omit fileName from the completion event when the file name is an empty string', inject([UploaderComponent], (app: UploaderComponent) => { + driveUploader(app); + spyOn(app.onCompleteItemWithFile, 'emit'); + + app.uploader.onCompleteItem({ file: { name: '' } } as any, JSON.stringify({ foo: 'bar' }), 200, {}); + + const arg = (app.onCompleteItemWithFile.emit as jasmine.Spy).calls.mostRecent().args[0]; + expect(app.onCompleteItemWithFile.emit).toHaveBeenCalledWith({ response: { foo: 'bar' } }); + expect('fileName' in arg).toBeFalse(); + })); + + it('should keep a whitespace-only file name on the completion event', inject([UploaderComponent], (app: UploaderComponent) => { + driveUploader(app); + spyOn(app.onCompleteItemWithFile, 'emit'); + + app.uploader.onCompleteItem({ file: { name: ' ' } } as any, JSON.stringify({ foo: 'bar' }), 200, {}); + + expect(app.onCompleteItemWithFile.emit).toHaveBeenCalledWith({ response: { foo: 'bar' }, fileName: ' ' }); + })); + + it('should not emit either completion output when the response body is empty', inject([UploaderComponent], (app: UploaderComponent) => { + driveUploader(app); + spyOn(app.onCompleteItem, 'emit'); + spyOn(app.onCompleteItemWithFile, 'emit'); + + app.uploader.onCompleteItem({ file: { name: 'test.pdf' } } as any, '', 204, {}); + + expect(app.onCompleteItem.emit).not.toHaveBeenCalled(); + expect(app.onCompleteItemWithFile.emit).not.toHaveBeenCalled(); + })); + + it('should emit onUploadError with the item, response, status and headers of the failed upload', inject([UploaderComponent], (app: UploaderComponent) => { + driveUploader(app); + spyOn(app.onUploadError, 'emit'); + + app.uploader.onErrorItem({ file: { name: 'broken.zip' } } as any, 'boom', 500, {}); + + expect(app.onUploadError.emit).toHaveBeenCalledWith({ + item: { file: { name: 'broken.zip' } }, + response: 'boom', + status: 500, + headers: {}, + }); + })); + }); // declare a test component diff --git a/src/app/shared/upload/uploader/uploader.component.ts b/src/app/shared/upload/uploader/uploader.component.ts index a016514c791..fd908d88375 100644 --- a/src/app/shared/upload/uploader/uploader.component.ts +++ b/src/app/shared/upload/uploader/uploader.component.ts @@ -35,6 +35,8 @@ import { of } from 'rxjs'; import { BtnDisabledDirective } from '../../btn-disabled.directive'; import { LiveRegionService } from '../../live-region/live-region.service'; +import { UploaderCompleteEvent } from './uploader-complete-event.model'; +import { UploaderError } from './uploader-error.model'; import { UploaderOptions } from './uploader-options.model'; import { UploaderProperties } from './uploader-properties.model'; @@ -103,10 +105,16 @@ export class UploaderComponent implements OnInit, AfterViewInit { */ @Output() onCompleteItem: EventEmitter = new EventEmitter(); + /** + * The function to call when upload is completed, carrying the parsed response together with the + * client-side file name. Emitted alongside {@link onCompleteItem} so existing consumers are unaffected. + */ + @Output() onCompleteItemWithFile: EventEmitter = new EventEmitter(); + /** * The function to call on error occurred */ - @Output() onUploadError: EventEmitter = new EventEmitter(); + @Output() onUploadError: EventEmitter = new EventEmitter(); /** * The function to call when a file is selected @@ -227,6 +235,8 @@ export class UploaderComponent implements OnInit, AfterViewInit { if (isNotEmpty(response)) { const responsePath = JSON.parse(response); this.onCompleteItem.emit(responsePath); + const fileName = item?.file?.name; + this.onCompleteItemWithFile.emit(isNotEmpty(fileName) ? { response: responsePath, fileName } : { response: responsePath }); } }; this.uploader.onErrorItem = (item: any, response: any, status: any, headers: any) => { diff --git a/src/app/submission/form/submission-upload-files/submission-upload-files.component.html b/src/app/submission/form/submission-upload-files/submission-upload-files.component.html index a8c8b9ca489..fcddb2fa3df 100644 --- a/src/app/submission/form/submission-upload-files/submission-upload-files.component.html +++ b/src/app/submission/form/submission-upload-files/submission-upload-files.component.html @@ -5,6 +5,6 @@ [enableDragOverDocument]="enableDragOverDocument" [onBeforeUpload]="onBeforeUpload" [uploadFilesOptions]="uploadFilesOptions" - (onCompleteItem)="onCompleteItem($event)" - (onUploadError)="onUploadError()"> + (onCompleteItemWithFile)="onCompleteItem($event)" + (onUploadError)="onUploadError($event)"> } diff --git a/src/app/submission/form/submission-upload-files/submission-upload-files.component.spec.ts b/src/app/submission/form/submission-upload-files/submission-upload-files.component.spec.ts index cca72508de3..050491a7c14 100644 --- a/src/app/submission/form/submission-upload-files/submission-upload-files.component.spec.ts +++ b/src/app/submission/form/submission-upload-files/submission-upload-files.component.spec.ts @@ -118,6 +118,7 @@ describe('SubmissionUploadFilesComponent Component', () => { sectionsServiceStub.isSectionTypeAvailable.and.returnValue(of(true)); notificationsServiceStub = TestBed.inject(NotificationsService as any); translateService = TestBed.inject(TranslateService); + translateService.instant.and.callFake((key: string) => `translated:${key}`); comp.submissionId = submissionId; comp.collectionId = collectionId; comp.uploadFilesOptions = Object.assign(new UploaderOptions(),{ @@ -169,7 +170,10 @@ describe('SubmissionUploadFilesComponent Component', () => { upload: { files: [{ url: 'testUrl' }], } }; - comp.onCompleteItem(Object.assign({}, uploadRestResponse, { sections: data })); + comp.onCompleteItem({ + response: Object.assign({}, uploadRestResponse, { sections: data }), + fileName: 'test.pdf', + }); Object.keys(data).forEach((sectionId) => { expect(sectionsServiceStub.updateSectionData).toHaveBeenCalledWith( @@ -190,10 +194,13 @@ describe('SubmissionUploadFilesComponent Component', () => { const expectedErrors: any = mockUploadResponse2ParsedErrors; fixture.detectChanges(); - comp.onCompleteItem(Object.assign({}, uploadRestResponse, { - sections: mockSectionsData, - errors: responseErrors.errors, - })); + comp.onCompleteItem({ + response: Object.assign({}, uploadRestResponse, { + sections: mockSectionsData, + errors: responseErrors.errors, + }), + fileName: 'test.pdf', + }); Object.keys(mockSectionsData).forEach((sectionId) => { expect(sectionsServiceStub.updateSectionData).toHaveBeenCalledWith( @@ -208,6 +215,163 @@ describe('SubmissionUploadFilesComponent Component', () => { expect(notificationsServiceStub.success).not.toHaveBeenCalled(); }); + + it('should include the file name in the success notification content', () => { + fixture.detectChanges(); + + comp.onCompleteItem({ + response: Object.assign({}, uploadRestResponse, { sections: mockSectionsData }), + fileName: 'test.pdf', + }); + + expect(translateService.get).toHaveBeenCalledWith( + 'submission.sections.upload.upload-successful-file', + { + fileName: 'test.pdf', + default: 'translated:submission.sections.upload.upload-successful', + }, + ); + expect(translateService.get).not.toHaveBeenCalledWith('submission.sections.upload.upload-successful'); + expect(notificationsServiceStub.success).toHaveBeenCalledTimes(1); + }); + + it('should fall back to the generic success key when no file name is available', () => { + fixture.detectChanges(); + + comp.onCompleteItem({ + response: Object.assign({}, uploadRestResponse, { sections: mockSectionsData }), + }); + + expect(translateService.get).toHaveBeenCalledWith('submission.sections.upload.upload-successful'); + expect(translateService.get).not.toHaveBeenCalledWith('submission.sections.upload.upload-successful-file', jasmine.anything()); + expect(notificationsServiceStub.success).toHaveBeenCalledTimes(1); + }); + + it('should fall back to the generic success key when the file name is an empty string', () => { + fixture.detectChanges(); + + comp.onCompleteItem({ + response: Object.assign({}, uploadRestResponse, { sections: mockSectionsData }), + fileName: '', + }); + + expect(translateService.get).toHaveBeenCalledWith('submission.sections.upload.upload-successful'); + expect(translateService.get).not.toHaveBeenCalledWith('submission.sections.upload.upload-successful-file', jasmine.anything()); + }); + + it('should include the file name in the error notification content when the upload section has errors', () => { + fixture.detectChanges(); + + comp.onCompleteItem({ + response: Object.assign({}, uploadRestResponse, { + sections: mockSectionsData, + errors: mockUploadResponse2Errors.errors, + }), + fileName: 'test.pdf', + }); + + expect(translateService.get).toHaveBeenCalledWith( + 'submission.sections.upload.upload-failed-file', + { + fileName: 'test.pdf', + default: 'translated:submission.sections.upload.upload-failed', + }, + ); + expect(translateService.get).not.toHaveBeenCalledWith('submission.sections.upload.upload-failed'); + expect(notificationsServiceStub.error).toHaveBeenCalledTimes(1); + expect(notificationsServiceStub.success).not.toHaveBeenCalled(); + }); + + it('should fall back to the generic error key when the upload section has errors and no file name is available', () => { + fixture.detectChanges(); + + comp.onCompleteItem({ + response: Object.assign({}, uploadRestResponse, { + sections: mockSectionsData, + errors: mockUploadResponse2Errors.errors, + }), + }); + + expect(translateService.get).toHaveBeenCalledWith('submission.sections.upload.upload-failed'); + expect(translateService.get).not.toHaveBeenCalledWith('submission.sections.upload.upload-failed-file', jasmine.anything()); + expect(notificationsServiceStub.error).toHaveBeenCalledTimes(1); + }); + + it('should not notify when the completion response carries no sections', () => { + fixture.detectChanges(); + + comp.onCompleteItem({ response: { message: 'forced' }, fileName: 'x.pdf' }); + + expect(notificationsServiceStub.success).not.toHaveBeenCalled(); + expect(notificationsServiceStub.error).not.toHaveBeenCalled(); + }); + + it('should not throw when the completion event is malformed', () => { + fixture.detectChanges(); + + expect(() => comp.onCompleteItem(undefined as any)).not.toThrow(); + expect(() => comp.onCompleteItem({ response: undefined })).not.toThrow(); + + expect(notificationsServiceStub.success).not.toHaveBeenCalled(); + expect(notificationsServiceStub.error).not.toHaveBeenCalled(); + }); + + it('should raise file-name notifications on the escaped rendering path', () => { + const hostileName = '.pdf'; + fixture.detectChanges(); + + comp.onCompleteItem({ + response: Object.assign({}, uploadRestResponse, { sections: mockSectionsData }), + fileName: hostileName, + }); + comp.onUploadError({ item: { file: { name: hostileName } }, response: 'boom', status: 500, headers: {} }); + + // Two arguments only: NotificationsService.success/error(title, content, options?, html = false). + // A 4th positional `true` would move the content to the [innerHTML] branch of + // notification.component.html, where an attacker-controlled file name would be parsed as markup. + expect(notificationsServiceStub.success.calls.mostRecent().args.length).toBe(2); + expect(notificationsServiceStub.error.calls.mostRecent().args.length).toBe(2); + expect(translateService.get).toHaveBeenCalledWith( + 'submission.sections.upload.upload-successful-file', + jasmine.objectContaining({ fileName: hostileName }), + ); + expect(translateService.get).toHaveBeenCalledWith( + 'submission.sections.upload.upload-failed-file', + jasmine.objectContaining({ fileName: hostileName }), + ); + }); + }); + + describe('on upload error', () => { + it('should show an error notification including the file name when available', () => { + comp.onUploadError({ item: { file: { name: 'broken.zip' } }, response: 'boom', status: 500, headers: {} }); + + expect(notificationsServiceStub.error).toHaveBeenCalledTimes(1); + expect(translateService.get).toHaveBeenCalledWith( + 'submission.sections.upload.upload-failed-file', + { + fileName: 'broken.zip', + default: 'translated:submission.sections.upload.upload-failed', + }, + ); + expect(translateService.get).not.toHaveBeenCalledWith('submission.sections.upload.upload-failed'); + }); + + it('should fall back to the generic error key when no file name is available', () => { + comp.onUploadError(); + + expect(notificationsServiceStub.error).toHaveBeenCalledTimes(1); + expect(translateService.get).toHaveBeenCalledWith('submission.sections.upload.upload-failed'); + expect(translateService.get).not.toHaveBeenCalledWith('submission.sections.upload.upload-failed-file', jasmine.anything()); + }); + + it('should fall back to the generic error key when the error carries no item', () => { + comp.onUploadError({}); + + expect(notificationsServiceStub.error).toHaveBeenCalledTimes(1); + expect(translateService.get).toHaveBeenCalledWith('submission.sections.upload.upload-failed'); + expect(translateService.get).not.toHaveBeenCalledWith('submission.sections.upload.upload-failed-file', jasmine.anything()); + }); }); }); }); diff --git a/src/app/submission/form/submission-upload-files/submission-upload-files.component.ts b/src/app/submission/form/submission-upload-files/submission-upload-files.component.ts index 1ab77cf3b32..030bb4d17e7 100644 --- a/src/app/submission/form/submission-upload-files/submission-upload-files.component.ts +++ b/src/app/submission/form/submission-upload-files/submission-upload-files.component.ts @@ -27,6 +27,8 @@ import { } from 'rxjs/operators'; import { UploaderComponent } from '../../../shared/upload/uploader/uploader.component'; +import { UploaderCompleteEvent } from '../../../shared/upload/uploader/uploader-complete-event.model'; +import { UploaderError } from '../../../shared/upload/uploader/uploader-error.model'; import { UploaderOptions } from '../../../shared/upload/uploader/uploader-options.model'; import { SectionsService } from '../../sections/sections.service'; import { SubmissionService } from '../../submission.service'; @@ -131,16 +133,19 @@ export class SubmissionUploadFilesComponent implements OnChanges, OnDestroy { /** * Parse the submission object retrieved from REST after upload * - * @param workspaceitem - * The submission object retrieved from REST + * @param event + * The completed upload event, carrying the submission object retrieved from REST and the + * client-side name of the file that completed */ - public onCompleteItem(workspaceitem: WorkspaceItem) { + public onCompleteItem(event: UploaderCompleteEvent) { + const workspaceitem = event?.response as WorkspaceItem; + const fileName = event?.fileName; // Checks if upload section is enabled so do upload this.subs.push( this.uploadEnabled .pipe(first()) .subscribe((isUploadEnabled) => { - if (isUploadEnabled) { + if (isUploadEnabled && hasValue(workspaceitem)) { const { sections } = workspaceitem; const { errors } = workspaceitem; @@ -157,9 +162,9 @@ export class SubmissionUploadFilesComponent implements OnChanges, OnDestroy { if (isUpload) { // Look for errors on upload if ((isEmpty(sectionErrors))) { - this.notificationsService.success(null, this.translate.get('submission.sections.upload.upload-successful')); + this.notificationsService.success(null, this.getNotificationContent('upload-successful', fileName)); } else { - this.notificationsService.error(null, this.translate.get('submission.sections.upload.upload-failed')); + this.notificationsService.error(null, this.getNotificationContent('upload-failed', fileName)); } } }); @@ -174,9 +179,33 @@ export class SubmissionUploadFilesComponent implements OnChanges, OnDestroy { /** * Show error notification on upload fails + * + * @param error + * The upload error, carrying the file that failed to upload (when available) + */ + public onUploadError(error?: UploaderError) { + this.notificationsService.error(null, this.getNotificationContent('upload-failed', error?.item?.file?.name)); + } + + /** + * Build the translated notification content for an upload outcome, including the file name when + * available. Falls back to the generic (file-name-less) message when the file name is missing. + * The `default` interpolate param is honoured by MissingTranslationHelper, so a locale that has not + * yet translated the `-file` key renders the generic message rather than a raw dotted key. + * + * @param suffix + * The i18n key suffix within the upload section (e.g. `upload-successful`); the helper reads + * `-file` when a file name is known and plain `` otherwise + * @param fileName + * The name of the file the notification refers to, if known */ - public onUploadError() { - this.notificationsService.error(null, this.translate.get('submission.sections.upload.upload-failed')); + private getNotificationContent(suffix: string, fileName?: string): Observable { + return isNotEmpty(fileName) + ? this.translate.get(`submission.sections.upload.${suffix}-file`, { + fileName, + default: this.translate.instant(`submission.sections.upload.${suffix}`), + }) + : this.translate.get(`submission.sections.upload.${suffix}`); } /** diff --git a/src/assets/i18n/en.json5 b/src/assets/i18n/en.json5 index 6551143ff13..4ff3970dc50 100644 --- a/src/assets/i18n/en.json5 +++ b/src/assets/i18n/en.json5 @@ -6006,8 +6006,12 @@ "submission.sections.upload.upload-failed": "Upload failed", + "submission.sections.upload.upload-failed-file": "Upload failed for file \"{{fileName}}\"", + "submission.sections.upload.upload-successful": "Upload successful", + "submission.sections.upload.upload-successful-file": "File \"{{fileName}}\" uploaded successfully", + "submission.sections.custom-url.label.previous-urls": "Previous Urls", "submission.sections.custom-url.alert.info": "Define here a custom URL which will be used to reach the item instead of using an internal randomly generated UUID identifier. ",