From 32003a72fdb785722942c9bc958802e1954ea3dc Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Fri, 18 Jun 2021 11:57:36 +0200 Subject: [PATCH 1/6] Fix hardcoded submission section IDs --- .../submission-form-collection.component.ts | 3 ++- .../form/submission-form.component.html | 1 - .../submission-upload-files.component.ts | 25 ++++++++----------- .../submission/sections/sections.service.ts | 16 ++++++++++++ 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/src/app/submission/form/collection/submission-form-collection.component.ts b/src/app/submission/form/collection/submission-form-collection.component.ts index ba7eb88c6f..f90814f185 100644 --- a/src/app/submission/form/collection/submission-form-collection.component.ts +++ b/src/app/submission/form/collection/submission-form-collection.component.ts @@ -28,6 +28,7 @@ import { CollectionDataService } from '../../../core/data/collection-data.servic import { CollectionDropdownComponent } from '../../../shared/collection-dropdown/collection-dropdown.component'; import { SectionsService } from '../../sections/sections.service'; import { getFirstSucceededRemoteDataPayload } from '../../../core/shared/operators'; +import { SectionsType } from '../../sections/sections-type'; /** * This component allows to show the current collection the submission belonging to and to change it. @@ -142,7 +143,7 @@ export class SubmissionFormCollectionComponent implements OnChanges, OnInit { */ ngOnInit() { this.pathCombiner = new JsonPatchOperationPathCombiner('sections', 'collection'); - this.available$ = this.sectionsService.isSectionAvailable(this.submissionId, 'collection'); + this.available$ = this.sectionsService.isSectionTypeAvailable(this.submissionId, SectionsType.collection); } /** diff --git a/src/app/submission/form/submission-form.component.html b/src/app/submission/form/submission-form.component.html index 33b5d4be12..c86d4e0195 100644 --- a/src/app/submission/form/submission-form.component.html +++ b/src/app/submission/form/submission-form.component.html @@ -3,7 +3,6 @@
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 2c348d2c87..e49127e80f 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 @@ -13,6 +13,7 @@ import { UploaderOptions } from '../../../shared/uploader/uploader-options.model import parseSectionErrors from '../../utils/parseSectionErrors'; import { SubmissionJsonPatchOperationsService } from '../../../core/submission/submission-json-patch-operations.service'; import { WorkspaceItem } from '../../../core/submission/models/workspaceitem.model'; +import { SectionsType } from '../../sections/sections-type'; /** * This component represents the drop zone that provides to add files to the submission. @@ -35,12 +36,6 @@ export class SubmissionUploadFilesComponent implements OnChanges { */ @Input() submissionId: string; - /** - * The upload section id - * @type {string} - */ - @Input() sectionId: string; - /** * The uploader configuration options * @type {UploaderOptions} @@ -110,7 +105,7 @@ export class SubmissionUploadFilesComponent implements OnChanges { * Check if upload functionality is enabled */ ngOnChanges() { - this.uploadEnabled = this.sectionService.isSectionAvailable(this.submissionId, this.sectionId); + this.uploadEnabled = this.sectionService.isSectionTypeAvailable(this.submissionId, SectionsType.Upload); } /** @@ -136,14 +131,16 @@ export class SubmissionUploadFilesComponent implements OnChanges { .forEach((sectionId) => { const sectionData = normalizeSectionData(sections[sectionId]); const sectionErrors = errorsList[sectionId]; - if (sectionId === 'upload') { - // Look for errors on upload - if ((isEmpty(sectionErrors))) { - this.notificationsService.success(null, this.translate.get('submission.sections.upload.upload-successful')); - } else { - this.notificationsService.error(null, this.translate.get('submission.sections.upload.upload-failed')); + this.sectionService.isSectionType(this.submissionId, sectionId, SectionsType.Upload).subscribe((isUpload) => { + if (isUpload) { + // Look for errors on upload + if ((isEmpty(sectionErrors))) { + this.notificationsService.success(null, this.translate.get('submission.sections.upload.upload-successful')); + } else { + this.notificationsService.error(null, this.translate.get('submission.sections.upload.upload-failed')); + } } - } + }); this.sectionService.updateSectionData(this.submissionId, sectionId, sectionData, sectionErrors); }); } diff --git a/src/app/submission/sections/sections.service.ts b/src/app/submission/sections/sections.service.ts index d8d1491cb7..afe5dde570 100644 --- a/src/app/submission/sections/sections.service.ts +++ b/src/app/submission/sections/sections.service.ts @@ -328,6 +328,22 @@ export class SectionsService { distinctUntilChanged()); } + /** + * Check if given section id is of a given section type + * @param submissionId + * @param sectionId + * @param sectionType + */ + public isSectionType(submissionId: string, sectionId: string, sectionType: SectionsType): Observable { + return this.store.select(submissionObjectFromIdSelector(submissionId)).pipe( + filter((submissionState: SubmissionObjectEntry) => isNotUndefined(submissionState)), + map((submissionState: SubmissionObjectEntry) => { + return isNotUndefined(submissionState.sections) && isNotUndefined(submissionState.sections[sectionId] + && submissionState.sections[sectionId].sectionType === sectionType ); + }), + distinctUntilChanged()); + } + /** * Dispatch a new [EnableSectionAction] to add a new section and move page target to it * From 72e97ca6b44ec19dc49e9b422169999f0d1d06d5 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Fri, 18 Jun 2021 12:33:01 +0200 Subject: [PATCH 2/6] Update unit tests --- .../shared/testing/sections-service.stub.ts | 2 + ...bmission-form-collection.component.spec.ts | 2 +- .../submission-upload-files.component.spec.ts | 83 ++++++++++--------- 3 files changed, 45 insertions(+), 42 deletions(-) diff --git a/src/app/shared/testing/sections-service.stub.ts b/src/app/shared/testing/sections-service.stub.ts index 3b311c5e19..c372cceadd 100644 --- a/src/app/shared/testing/sections-service.stub.ts +++ b/src/app/shared/testing/sections-service.stub.ts @@ -10,6 +10,8 @@ export class SectionsServiceStub { isSectionEnabled = jasmine.createSpy('isSectionEnabled'); isSectionReadOnly = jasmine.createSpy('isSectionReadOnly'); isSectionAvailable = jasmine.createSpy('isSectionAvailable'); + isSectionTypeAvailable = jasmine.createSpy('isSectionTypeAvailable'); + isSectionType = jasmine.createSpy('isSectionType'); addSection = jasmine.createSpy('addSection'); removeSection = jasmine.createSpy('removeSection'); updateSectionData = jasmine.createSpy('updateSectionData'); diff --git a/src/app/submission/form/collection/submission-form-collection.component.spec.ts b/src/app/submission/form/collection/submission-form-collection.component.spec.ts index 14dfcef864..5b9946e1a4 100644 --- a/src/app/submission/form/collection/submission-form-collection.component.spec.ts +++ b/src/app/submission/form/collection/submission-form-collection.component.spec.ts @@ -120,7 +120,7 @@ describe('SubmissionFormCollectionComponent Component', () => { }); const sectionsService: any = jasmine.createSpyObj('sectionsService', { - isSectionAvailable: of(true) + isSectionTypeAvailable: of(true) }); beforeEach(waitForAsync(() => { 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 4a943276e5..cc7d0e8484 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 @@ -83,7 +83,6 @@ describe('SubmissionUploadFilesComponent Component', () => { const html = ` `; testFixture = createTestComponent(html, TestComponent) as ComponentFixture; @@ -108,11 +107,11 @@ describe('SubmissionUploadFilesComponent Component', () => { compAsAny = comp; submissionServiceStub = TestBed.inject(SubmissionService as any); sectionsServiceStub = TestBed.inject(SectionsService as any); + sectionsServiceStub.isSectionTypeAvailable.and.returnValue(observableOf(true)); notificationsServiceStub = TestBed.inject(NotificationsService as any); translateService = TestBed.inject(TranslateService); comp.submissionId = submissionId; comp.collectionId = collectionId; - comp.sectionId = 'upload'; comp.uploadFilesOptions = Object.assign(new UploaderOptions(),{ url: '', authToken: null, @@ -133,7 +132,7 @@ describe('SubmissionUploadFilesComponent Component', () => { }); it('should init uploadEnabled properly', () => { - sectionsServiceStub.isSectionAvailable.and.returnValue(hot('-a-b', { + sectionsServiceStub.isSectionTypeAvailable.and.returnValue(hot('-a-b', { a: false, b: true })); @@ -149,53 +148,56 @@ describe('SubmissionUploadFilesComponent Component', () => { expect(compAsAny.uploadEnabled).toBeObservable(expected); }); - it('should show a success notification and call updateSectionData on upload complete', () => { - - const expectedErrors: any = mockUploadResponse1ParsedErrors; - compAsAny.uploadEnabled = observableOf(true); - fixture.detectChanges(); - - comp.onCompleteItem(Object.assign({}, uploadRestResponse, { sections: mockSectionsData })); - - Object.keys(mockSectionsData).forEach((sectionId) => { - expect(sectionsServiceStub.updateSectionData).toHaveBeenCalledWith( - submissionId, - sectionId, - mockSectionsData[sectionId], - expectedErrors[sectionId] - ); + describe('on upload complete', () => { + beforeEach(() => { + sectionsServiceStub.isSectionType.and.callFake((submissionId, sectionId, sectionType) => { + return observableOf(sectionId === 'upload') + }); + compAsAny.uploadEnabled = observableOf(true); }); - expect(notificationsServiceStub.success).toHaveBeenCalled(); + it('should show a success notification and call updateSectionData if successful', () => { + const expectedErrors: any = mockUploadResponse1ParsedErrors; + fixture.detectChanges(); - }); + comp.onCompleteItem(Object.assign({}, uploadRestResponse, { sections: mockSectionsData })); - it('should show an error notification and call updateSectionData on upload complete', () => { + Object.keys(mockSectionsData).forEach((sectionId) => { + expect(sectionsServiceStub.updateSectionData).toHaveBeenCalledWith( + submissionId, + sectionId, + mockSectionsData[sectionId], + expectedErrors[sectionId] + ); + }); - const responseErrors = mockUploadResponse2Errors; + expect(notificationsServiceStub.success).toHaveBeenCalled(); - const expectedErrors: any = mockUploadResponse2ParsedErrors; - compAsAny.uploadEnabled = observableOf(true); - fixture.detectChanges(); - - comp.onCompleteItem(Object.assign({}, uploadRestResponse, { - sections: mockSectionsData, - errors: responseErrors.errors - })); - - Object.keys(mockSectionsData).forEach((sectionId) => { - expect(sectionsServiceStub.updateSectionData).toHaveBeenCalledWith( - submissionId, - sectionId, - mockSectionsData[sectionId], - expectedErrors[sectionId] - ); }); - expect(notificationsServiceStub.success).not.toHaveBeenCalled(); + it('should show an error notification and call updateSectionData if unsuccessful', () => { + const responseErrors = mockUploadResponse2Errors; + const expectedErrors: any = mockUploadResponse2ParsedErrors; + fixture.detectChanges(); + comp.onCompleteItem(Object.assign({}, uploadRestResponse, { + sections: mockSectionsData, + errors: responseErrors.errors + })); + + Object.keys(mockSectionsData).forEach((sectionId) => { + expect(sectionsServiceStub.updateSectionData).toHaveBeenCalledWith( + submissionId, + sectionId, + mockSectionsData[sectionId], + expectedErrors[sectionId] + ); + }); + + expect(notificationsServiceStub.success).not.toHaveBeenCalled(); + + }); }); - }); }); @@ -208,7 +210,6 @@ class TestComponent { submissionId = mockSubmissionId; collectionId = mockSubmissionCollectionId; - sectionId = 'upload'; uploadFilesOptions = Object.assign(new UploaderOptions(), { url: '', authToken: null, From a3ba4e59b3954393e1ac77c3453a5a1d602480c6 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Fri, 18 Jun 2021 13:01:07 +0200 Subject: [PATCH 3/6] Fix lint issues --- .../submission-upload-files.component.spec.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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 cc7d0e8484..823cbb5d82 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 @@ -150,9 +150,7 @@ describe('SubmissionUploadFilesComponent Component', () => { describe('on upload complete', () => { beforeEach(() => { - sectionsServiceStub.isSectionType.and.callFake((submissionId, sectionId, sectionType) => { - return observableOf(sectionId === 'upload') - }); + sectionsServiceStub.isSectionType.and.callFake((_, sectionId, __) => observableOf(sectionId === 'upload')); compAsAny.uploadEnabled = observableOf(true); }); From 7670ba8a43abf474aeb1b531b32336a4c87b0f04 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Fri, 18 Jun 2021 13:59:39 +0200 Subject: [PATCH 4/6] Fix duplicate notifications --- .../submission-upload-files.component.ts | 24 ++++++++++--------- .../submission/sections/sections.service.ts | 4 ++-- 2 files changed, 15 insertions(+), 13 deletions(-) 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 e49127e80f..b1b5051458 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 @@ -2,7 +2,7 @@ import { Component, Input, OnChanges } from '@angular/core'; import { TranslateService } from '@ngx-translate/core'; import { Observable, of as observableOf, Subscription } from 'rxjs'; -import { first } from 'rxjs/operators'; +import { first, take } from 'rxjs/operators'; import { SectionsService } from '../../sections/sections.service'; import { hasValue, isEmpty, isNotEmpty } from '../../../shared/empty.util'; @@ -131,16 +131,18 @@ export class SubmissionUploadFilesComponent implements OnChanges { .forEach((sectionId) => { const sectionData = normalizeSectionData(sections[sectionId]); const sectionErrors = errorsList[sectionId]; - this.sectionService.isSectionType(this.submissionId, sectionId, SectionsType.Upload).subscribe((isUpload) => { - if (isUpload) { - // Look for errors on upload - if ((isEmpty(sectionErrors))) { - this.notificationsService.success(null, this.translate.get('submission.sections.upload.upload-successful')); - } else { - this.notificationsService.error(null, this.translate.get('submission.sections.upload.upload-failed')); - } - } - }); + this.sectionService.isSectionType(this.submissionId, sectionId, SectionsType.Upload) + .pipe(take(1)) + .subscribe((isUpload) => { + if (isUpload) { + // Look for errors on upload + if ((isEmpty(sectionErrors))) { + this.notificationsService.success(null, this.translate.get('submission.sections.upload.upload-successful')); + } else { + this.notificationsService.error(null, this.translate.get('submission.sections.upload.upload-failed')); + } + } + }); this.sectionService.updateSectionData(this.submissionId, sectionId, sectionData, sectionErrors); }); } diff --git a/src/app/submission/sections/sections.service.ts b/src/app/submission/sections/sections.service.ts index afe5dde570..05e9a96267 100644 --- a/src/app/submission/sections/sections.service.ts +++ b/src/app/submission/sections/sections.service.ts @@ -338,8 +338,8 @@ export class SectionsService { return this.store.select(submissionObjectFromIdSelector(submissionId)).pipe( filter((submissionState: SubmissionObjectEntry) => isNotUndefined(submissionState)), map((submissionState: SubmissionObjectEntry) => { - return isNotUndefined(submissionState.sections) && isNotUndefined(submissionState.sections[sectionId] - && submissionState.sections[sectionId].sectionType === sectionType ); + return isNotUndefined(submissionState.sections) && isNotUndefined(submissionState.sections[sectionId]) + && submissionState.sections[sectionId].sectionType === sectionType; }), distinctUntilChanged()); } From 8736c0b572064f6b19c1229b672506d9d2d2cbab Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Fri, 2 Jul 2021 18:12:18 +0200 Subject: [PATCH 5/6] Add unit tests for SectionsService#isSectionType --- .../sections/sections.service.spec.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/app/submission/sections/sections.service.spec.ts b/src/app/submission/sections/sections.service.spec.ts index 0d63beeaea..d199f1beae 100644 --- a/src/app/submission/sections/sections.service.spec.ts +++ b/src/app/submission/sections/sections.service.spec.ts @@ -334,6 +334,38 @@ describe('SectionsService test suite', () => { }); }); + describe('isSectionType', () => { + it('should return true if the section matches the provided type', () => { + store.select.and.returnValue(observableOf(submissionState)); + + const expected = cold('(b|)', { + b: true + }); + + expect(service.isSectionType(submissionId, 'upload', SectionsType.Upload)).toBeObservable(expected); + }); + + it('should return false if the section doesn\'t match the provided type', () => { + store.select.and.returnValue(observableOf(submissionState)); + + const expected = cold('(b|)', { + b: false + }); + + expect(service.isSectionType(submissionId, sectionId, SectionsType.Upload)).toBeObservable(expected); + }); + + it('should return false if the provided sectionId doesn\'t exist', () => { + store.select.and.returnValue(observableOf(submissionState)); + + const expected = cold('(b|)', { + b: false + }); + + expect(service.isSectionType(submissionId, 'no-such-id', SectionsType.Upload)).toBeObservable(expected); + }); + }); + describe('addSection', () => { it('should dispatch a new EnableSectionAction a move target to new section', () => { From bfcff12499b687417e58a6dc81c7d6c3842cbc83 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Wed, 7 Jul 2021 11:35:12 +0200 Subject: [PATCH 6/6] Fix hardcoded reference to 'upload' section --- .../upload/file/section-upload-file.component.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/app/submission/sections/upload/file/section-upload-file.component.ts b/src/app/submission/sections/upload/file/section-upload-file.component.ts index d4c901b290..c18796d7f3 100644 --- a/src/app/submission/sections/upload/file/section-upload-file.component.ts +++ b/src/app/submission/sections/upload/file/section-upload-file.component.ts @@ -291,14 +291,13 @@ export class SubmissionSectionUploadFileComponent implements OnChanges, OnInit { this.pathCombiner.subRootElement); }) ).subscribe((result: SubmissionObject[]) => { - if (result[0].sections.upload) { - Object.keys((result[0].sections.upload as WorkspaceitemSectionUploadObject).files) - .filter((key) => (result[0].sections.upload as WorkspaceitemSectionUploadObject).files[key].uuid === this.fileId) + if (result[0].sections[this.sectionId]) { + const uploadSection = (result[0].sections[this.sectionId] as WorkspaceitemSectionUploadObject); + Object.keys(uploadSection.files) + .filter((key) => uploadSection.files[key].uuid === this.fileId) .forEach((key) => this.uploadService.updateFileData( - this.submissionId, - this.sectionId, - this.fileId, - (result[0].sections.upload as WorkspaceitemSectionUploadObject).files[key])); + this.submissionId, this.sectionId, this.fileId, uploadSection.files[key]) + ); } this.switchMode(); }));