From b33e456ee0e2f40097db43e2fa5ddafb5847fb3d Mon Sep 17 00:00:00 2001 From: milanmajchrak <90026355+milanmajchrak@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:29:38 +0200 Subject: [PATCH] UoE/Bitstream name in upload notifications Upload success and failure toasts in the submission form now name the file they refer to ("File "report.pdf" uploaded successfully") instead of the anonymous "Upload successful" / "Upload failed", so a submitter dropping several files can tell which one each toast is about. The uploader gains an additive `onCompleteItemWithFile` output carrying the parsed response plus the client-side file name; the legacy `onCompleteItem` is retained and still emits the bare parsed body first, byte-identically, so the five other uploader consumers are untouched. `onUploadError` is retyped from `any` to the existing `UploaderError`. A single `getNotificationContent` helper is the only place an upload notification key literal appears. The CLARIN client-side size-limit rejection is preserved exactly: its discriminator still compares against a one-argument `translate.instant(key)` and still runs first, and its message is emitted un-interpolated and without a file name. The error handler resolves the name from both emitted item shapes (FileItem from onErrorItem, FileLikeObject from onWhenAddingFileFailed). 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. cs.json5 carries human translations. Ported from dataquest-dev/uoe-dspace-datashare-angular#24. Co-Authored-By: Claude Opus 5 (1M context) --- .../uploader/uploader-complete-event.model.ts | 17 ++ .../uploader/uploader.component.spec.ts | 229 +++++++++++++++++- .../upload/uploader/uploader.component.ts | 12 +- .../submission-upload-files.component.html | 2 +- .../submission-upload-files.component.spec.ts | 223 ++++++++++++++++- .../submission-upload-files.component.ts | 62 ++++- src/assets/i18n/cs.json5 | 6 + src/assets/i18n/en.json5 | 4 + 8 files changed, 536 insertions(+), 19 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 90762875ca1..a6b0f2fb62a 100644 --- a/src/app/shared/upload/uploader/uploader.component.spec.ts +++ b/src/app/shared/upload/uploader/uploader.component.spec.ts @@ -8,7 +8,7 @@ import { DragService } from '../../../core/drag.service'; import { UploaderOptions } from './uploader-options.model'; import { UploaderComponent } from './uploader.component'; import { FileUploadModule } from 'ng2-file-upload'; -import { TranslateModule } from '@ngx-translate/core'; +import { TranslateModule, TranslateService } from '@ngx-translate/core'; import { createTestComponent } from '../../testing/utils.test'; import { HttpXsrfTokenExtractor } from '@angular/common/http'; import { CookieService } from '../../../core/services/cookie.service'; @@ -69,6 +69,233 @@ describe('Chips component', () => { expect(app).toBeDefined(); })); + it('should emit both onCompleteItem and onCompleteItemWithFile on a completed upload', inject([UploaderComponent], (app: UploaderComponent) => { + app.uploadFilesOptions = Object.assign(new UploaderOptions(), { + url: 'http://test', + authToken: null, + disableMultipart: false, + itemAlias: null, + }); + app.ngOnInit(); + app.ngAfterViewInit(); + + 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 not emit either completion output when the response body is empty', inject([UploaderComponent], (app: UploaderComponent) => { + app.uploadFilesOptions = Object.assign(new UploaderOptions(), { + url: 'http://test', + authToken: null, + disableMultipart: false, + itemAlias: null, + }); + app.ngOnInit(); + app.ngAfterViewInit(); + + 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 omit fileName from the completion event when the item is undefined', inject([UploaderComponent], (app: UploaderComponent) => { + app.uploadFilesOptions = Object.assign(new UploaderOptions(), { + url: 'http://test', + authToken: null, + disableMultipart: false, + itemAlias: null, + }); + app.ngOnInit(); + app.ngAfterViewInit(); + + spyOn(app.onCompleteItemWithFile, 'emit'); + + const parsed = { foo: 'bar' }; + app.uploader.onCompleteItem(undefined, JSON.stringify(parsed), 200, {}); + + const arg = (app.onCompleteItemWithFile.emit as jasmine.Spy).calls.mostRecent().args[0]; + expect(app.onCompleteItemWithFile.emit).toHaveBeenCalledWith({ response: parsed }); + 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) => { + app.uploadFilesOptions = Object.assign(new UploaderOptions(), { + url: 'http://test', + authToken: null, + disableMultipart: false, + itemAlias: null, + }); + app.ngOnInit(); + app.ngAfterViewInit(); + + spyOn(app.onCompleteItemWithFile, 'emit'); + + const parsed = { foo: 'bar' }; + app.uploader.onCompleteItem({} as any, JSON.stringify(parsed), 200, {}); + + const arg = (app.onCompleteItemWithFile.emit as jasmine.Spy).calls.mostRecent().args[0]; + expect(app.onCompleteItemWithFile.emit).toHaveBeenCalledWith({ response: parsed }); + 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) => { + app.uploadFilesOptions = Object.assign(new UploaderOptions(), { + url: 'http://test', + authToken: null, + disableMultipart: false, + itemAlias: null, + }); + app.ngOnInit(); + app.ngAfterViewInit(); + + spyOn(app.onCompleteItemWithFile, 'emit'); + + const parsed = { foo: 'bar' }; + app.uploader.onCompleteItem({ file: {} } as any, JSON.stringify(parsed), 200, {}); + + const arg = (app.onCompleteItemWithFile.emit as jasmine.Spy).calls.mostRecent().args[0]; + expect(app.onCompleteItemWithFile.emit).toHaveBeenCalledWith({ response: parsed }); + 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) => { + app.uploadFilesOptions = Object.assign(new UploaderOptions(), { + url: 'http://test', + authToken: null, + disableMultipart: false, + itemAlias: null, + }); + app.ngOnInit(); + app.ngAfterViewInit(); + + spyOn(app.onCompleteItemWithFile, 'emit'); + + const parsed = { foo: 'bar' }; + app.uploader.onCompleteItem({ file: { name: '' } } as any, JSON.stringify(parsed), 200, {}); + + const arg = (app.onCompleteItemWithFile.emit as jasmine.Spy).calls.mostRecent().args[0]; + expect(app.onCompleteItemWithFile.emit).toHaveBeenCalledWith({ response: parsed }); + expect(Object.keys(arg)).toEqual(['response']); + expect('fileName' in arg).toBeFalse(); + })); + + it('should keep a whitespace-only file name on the completion event', inject([UploaderComponent], (app: UploaderComponent) => { + app.uploadFilesOptions = Object.assign(new UploaderOptions(), { + url: 'http://test', + authToken: null, + disableMultipart: false, + itemAlias: null, + }); + app.ngOnInit(); + app.ngAfterViewInit(); + + spyOn(app.onCompleteItemWithFile, 'emit'); + + const parsed = { foo: 'bar' }; + app.uploader.onCompleteItem({ file: { name: ' ' } } as any, JSON.stringify(parsed), 200, {}); + + expect(app.onCompleteItemWithFile.emit).toHaveBeenCalledWith({ response: parsed, fileName: ' ' }); + })); + + it('should emit a distinct file name for each of two sequential completed uploads', inject([UploaderComponent], (app: UploaderComponent) => { + app.uploadFilesOptions = Object.assign(new UploaderOptions(), { + url: 'http://test', + authToken: null, + disableMultipart: false, + itemAlias: null, + }); + app.ngOnInit(); + app.ngAfterViewInit(); + + 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 emitSpy = app.onCompleteItemWithFile.emit as jasmine.Spy; + expect(emitSpy).toHaveBeenCalledTimes(2); + expect(emitSpy.calls.argsFor(0)[0]).toEqual({ response: { n: 1 }, fileName: 'first.pdf' }); + expect(emitSpy.calls.argsFor(1)[0]).toEqual({ response: { n: 2 }, fileName: 'second.pdf' }); + })); + + it('should emit onUploadError with the item, response, status and headers of the failed upload', inject([UploaderComponent], (app: UploaderComponent) => { + app.uploadFilesOptions = Object.assign(new UploaderOptions(), { + url: 'http://test', + authToken: null, + disableMultipart: false, + itemAlias: null, + }); + app.ngOnInit(); + app.ngAfterViewInit(); + + 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: {}, + }); + })); + + it('should emit the un-interpolated size-limit message when a file exceeds the maximum upload size', inject([UploaderComponent], (app: UploaderComponent) => { + app.uploadFilesOptions = Object.assign(new UploaderOptions(), { + url: 'http://test', + authToken: null, + disableMultipart: false, + itemAlias: null, + }); + app.ngOnInit(); + app.ngAfterViewInit(); + + const instantSpy = spyOn(TestBed.inject(TranslateService), 'instant').and.returnValue('SIZE-LIMIT-MSG'); + spyOn(app.onUploadError, 'emit'); + + app.uploader.options.maxFileSize = 1024; + app.uploader.onWhenAddingFileFailed({ name: 'big.zip', size: 2048 } as any, null, app.uploader.options); + + expect(instantSpy).toHaveBeenCalledWith('submission.sections.upload.upload-failed.size-limit-exceeded'); + expect(app.onUploadError.emit).toHaveBeenCalledWith(jasmine.objectContaining({ + status: 400, + response: 'SIZE-LIMIT-MSG', + })); + })); + + it('should not pass interpolation params to the size-limit instant() call', inject([UploaderComponent], (app: UploaderComponent) => { + app.uploadFilesOptions = Object.assign(new UploaderOptions(), { + url: 'http://test', + authToken: null, + disableMultipart: false, + itemAlias: null, + }); + app.ngOnInit(); + app.ngAfterViewInit(); + + const instantSpy = spyOn(TestBed.inject(TranslateService), 'instant').and.returnValue('SIZE-LIMIT-MSG'); + + app.uploader.options.maxFileSize = 1024; + app.uploader.onWhenAddingFileFailed({ name: 'big.zip', size: 2048 } as any, null, app.uploader.options); + + expect(instantSpy.calls.count()).toBe(1); + expect(instantSpy.calls.mostRecent().args.length).toBe(1); + })); + }); // 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 413f10cd411..c25b388748b 100644 --- a/src/app/shared/upload/uploader/uploader.component.ts +++ b/src/app/shared/upload/uploader/uploader.component.ts @@ -15,6 +15,8 @@ import { FileUploader } from 'ng2-file-upload'; import uniqueId from 'lodash/uniqueId'; import { ScrollToService } from '@nicky-lenaers/ngx-scroll-to'; +import { UploaderCompleteEvent } from './uploader-complete-event.model'; +import { UploaderError } from './uploader-error.model'; import { UploaderOptions } from './uploader-options.model'; import { hasValue, isNotEmpty, isUndefined } from '../../empty.util'; import { UploaderProperties } from './uploader-properties.model'; @@ -89,10 +91,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 @@ -201,6 +209,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 cf916fb413b..5e7be8fdf7d 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 @@ -4,5 +4,5 @@ [enableDragOverDocument]="enableDragOverDocument" [onBeforeUpload]="onBeforeUpload" [uploadFilesOptions]="uploadFilesOptions" - (onCompleteItem)="onCompleteItem($event)" + (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 fa7ecebbff6..5c958f14b15 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 @@ -158,7 +158,10 @@ describe('SubmissionUploadFilesComponent Component', () => { const expectedErrors: any = mockUploadResponse1ParsedErrors; fixture.detectChanges(); - comp.onCompleteItem(Object.assign({}, uploadRestResponse, { sections: mockSectionsData })); + comp.onCompleteItem({ + response: Object.assign({}, uploadRestResponse, { sections: mockSectionsData }), + fileName: 'test.pdf', + }); Object.keys(mockSectionsData).forEach((sectionId) => { expect(sectionsServiceStub.updateSectionData).toHaveBeenCalledWith( @@ -179,10 +182,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( @@ -197,6 +203,213 @@ describe('SubmissionUploadFilesComponent Component', () => { expect(notificationsServiceStub.success).not.toHaveBeenCalled(); }); + + it('should include the file name in the success notification content', () => { + translateService.instant.and.callFake((key: string) => 'T:' + key); + 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: 'T: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', () => { + translateService.instant.and.callFake((key: string) => 'T:' + key); + 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', () => { + translateService.instant.and.callFake((key: string) => 'T:' + key); + 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', () => { + const responseErrors = mockUploadResponse2Errors; + translateService.instant.and.callFake((key: string) => 'T:' + key); + fixture.detectChanges(); + + comp.onCompleteItem({ + response: Object.assign({}, uploadRestResponse, { + sections: mockSectionsData, + errors: responseErrors.errors + }), + fileName: 'test.pdf', + }); + + expect(translateService.get).toHaveBeenCalledWith( + 'submission.sections.upload.upload-failed-file', + { fileName: 'test.pdf', default: 'T: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', () => { + const responseErrors = mockUploadResponse2Errors; + translateService.instant.and.callFake((key: string) => 'T:' + key); + fixture.detectChanges(); + + comp.onCompleteItem({ + response: Object.assign({}, uploadRestResponse, { + sections: mockSectionsData, + errors: responseErrors.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', () => { + translateService.instant.and.callFake((key: string) => 'T:' + key); + 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', () => { + translateService.instant.and.callFake((key: string) => 'T:' + key); + 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'; + translateService.instant.and.callFake((key: string) => 'T:' + key); + 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', () => { + beforeEach(() => { + // The bare getMockTranslateService() spy returns undefined for every key, which would make the + // size-limit discriminator compare undefined === undefined and take the wrong branch. A real + // TranslateService never returns undefined from instant() - it returns the key when a + // translation is missing - so the callFake below is what makes this spec faithful. + translateService.instant.and.callFake((key: string) => 'T:' + key); + translateService.get.and.callFake((key: string, params?: any) => + observableOf(params ? key + ':' + params.fileName : key)); + }); + + 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(translateService.get).toHaveBeenCalledWith( + 'submission.sections.upload.upload-failed-file', + { fileName: 'broken.zip', default: 'T:submission.sections.upload.upload-failed' }, + ); + expect(translateService.get).not.toHaveBeenCalledWith('submission.sections.upload.upload-failed'); + expect(notificationsServiceStub.error).toHaveBeenCalledTimes(1); + }); + + it('should read the file name from item.name when the failed item has no file wrapper', () => { + comp.onUploadError({ item: { name: 'big.zip', size: 9e9 }, response: 'boom', status: 400, headers: {} }); + + expect(translateService.get).toHaveBeenCalledWith( + 'submission.sections.upload.upload-failed-file', + { fileName: 'big.zip', default: 'T:submission.sections.upload.upload-failed' }, + ); + expect(notificationsServiceStub.error).toHaveBeenCalledTimes(1); + }); + + it('should fall back to the generic error key when no file name is available', () => { + comp.onUploadError(); + comp.onUploadError({}); + + 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(2); + }); + + it('should show the un-interpolated size-limit message when the upload failed because the file is too large', () => { + comp.onUploadError({ + item: { name: 'big.zip', size: 9e9 }, + response: 'T:submission.sections.upload.upload-failed.size-limit-exceeded', + status: 400, + headers: {}, + }); + + expect(notificationsServiceStub.error).toHaveBeenCalledTimes(1); + expect(translateService.get).not.toHaveBeenCalled(); + expect(translateService.instant.calls.mostRecent().args.length).toBe(1); + expect(translateService.instant.calls.mostRecent().args[0]) + .toBe('submission.sections.upload.upload-failed.size-limit-exceeded'); + }); + + it('should not add the file name to the size-limit message', () => { + comp.onUploadError({ + item: { name: 'big.zip', size: 9e9 }, + response: 'T:submission.sections.upload.upload-failed.size-limit-exceeded', + status: 400, + headers: {}, + }); + + expect(translateService.get).not.toHaveBeenCalledWith( + 'submission.sections.upload.upload-failed-file', jasmine.anything()); + // getNotificationContent is the only other producer of notification content and it ALWAYS + // calls translate.get. `get` never being called therefore proves the ternary took the + // size-limit branch and that the raw, un-interpolated size-limit string is what reached + // NotificationsService - without asserting message identity through the notifications stub, + // which AC-T-05 forbids because the shared-spy mock makes such assertions unfalsifiable. + expect(translateService.get).not.toHaveBeenCalled(); + expect(notificationsServiceStub.error.calls.mostRecent().args.length).toBe(2); + }); }); }); }); 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 d16500a8640..1c103323d09 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 @@ -9,6 +9,8 @@ import { hasValue, isEmpty, isNotEmpty } from '../../../shared/empty.util'; import { normalizeSectionData } from '../../../core/submission/submission-response-parsing.service'; import { SubmissionService } from '../../submission.service'; import { NotificationsService } from '../../../shared/notifications/notifications.service'; +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 parseSectionErrors from '../../utils/parseSectionErrors'; import { SubmissionJsonPatchOperationsService } from '../../../core/submission/submission-json-patch-operations.service'; @@ -111,16 +113,19 @@ export class SubmissionUploadFilesComponent implements OnChanges { /** * 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; @@ -137,9 +142,9 @@ export class SubmissionUploadFilesComponent implements OnChanges { 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)); } } }); @@ -153,14 +158,49 @@ export class SubmissionUploadFilesComponent implements OnChanges { } /** - * Show error notification on upload fails + * Show error notification on upload fails. + * + * The client-side size-limit rejection is discriminated FIRST, by comparing the emitted `response` + * against the size-limit message the uploader produced with the very same ONE-ARGUMENT + * `translate.instant(key)` call. Adding interpolation params to either side would make this `===` + * silently false, with no compile error, so the comparison and both `instant()` arities must stay + * exactly as they are. Only when that comparison fails is the default message built, and the + * default message is the only one that carries the file name. + * + * @param error + * The upload error, carrying the file that failed to upload (when available) */ - public onUploadError(event: any) { + public onUploadError(error?: UploaderError) { const errorMessageUploadLimit = this.translate.instant('submission.sections.upload.upload-failed.size-limit-exceeded'); - const defaultErrorMessage = this.translate.instant('submission.sections.upload.upload-failed'); - const errorMessage = event?.response === errorMessageUploadLimit ? errorMessageUploadLimit : defaultErrorMessage; + const isFileSizeLimitError = error?.response === errorMessageUploadLimit; + // `onErrorItem` emits a ng2-file-upload FileItem (name under `file`), `onWhenAddingFileFailed` + // emits a bare FileLikeObject (name at the top level), so both shapes have to be covered. + const fileName = error?.item?.file?.name ?? error?.item?.name; + + this.notificationsService.error(null, isFileSizeLimitError + ? errorMessageUploadLimit + : this.getNotificationContent('upload-failed', fileName)); + } - this.notificationsService.error(null, errorMessage); + /** + * 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 + */ + 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/cs.json5 b/src/assets/i18n/cs.json5 index 8d8c4a6035a..9c1a82c6506 100644 --- a/src/assets/i18n/cs.json5 +++ b/src/assets/i18n/cs.json5 @@ -7215,9 +7215,15 @@ // "submission.sections.upload.upload-failed": "Upload failed", "submission.sections.upload.upload-failed": "Nahrání se nezdařilo", + // "submission.sections.upload.upload-failed-file": "Upload failed for file \"{{fileName}}\"", + "submission.sections.upload.upload-failed-file": "Nahrání souboru \"{{fileName}}\" se nezdařilo", + // "submission.sections.upload.upload-successful": "Upload successful", "submission.sections.upload.upload-successful": "Úspěšně nahráno", + // "submission.sections.upload.upload-successful-file": "File \"{{fileName}}\" uploaded successfully", + "submission.sections.upload.upload-successful-file": "Soubor \"{{fileName}}\" byl úspěšně nahrán", + // "submission.sections.accesses.form.discoverable-description": "When checked, this item will be discoverable in search/browse. When unchecked, the item will only be available via a direct link and will never appear in search/browse.", "submission.sections.accesses.form.discoverable-description": "Když je zaškrtnuto, bude tento záznam zjistitelný ve vyhledávání/prohlížení. Když není zaškrtnuto, záznam bude dostupný pouze prostřednictvím přímého odkazu a nikdy se nezobrazí ve vyhledávání/přehledu.", diff --git a/src/assets/i18n/en.json5 b/src/assets/i18n/en.json5 index 8def4121098..6f1ec59821f 100644 --- a/src/assets/i18n/en.json5 +++ b/src/assets/i18n/en.json5 @@ -4811,8 +4811,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.accesses.form.discoverable-description": "When checked, this item will be discoverable in search/browse. When unchecked, the item will only be available via a direct link and will never appear in search/browse.", "submission.sections.accesses.form.discoverable-label": "Discoverable",