Merge pull request #1239 from atmire/Fix-hardcoded-submission-section-IDs

Fix hardcoded submission section IDs

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');

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(() => {

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);
   }
 
   /**

src/app/submission/form/submission-form.component.html

@@ -3,7 +3,6 @@
         <div *ngIf="(uploadEnabled$ | async)" class="w-100">
             <ds-submission-upload-files [submissionId]="submissionId"
                                         [collectionId]="collectionId"
-                                        [sectionId]="'upload'"
                                         [uploadFilesOptions]="uploadFilesOptions"></ds-submission-upload-files>
             <div class="clearfix"></div>
         </div>

src/app/submission/form/submission-upload-files/submission-upload-files.component.spec.ts

@@ -83,7 +83,6 @@ describe('SubmissionUploadFilesComponent Component', () => {
       const html = `
         <ds-submission-upload-files [submissionId]="submissionId"
                                     [collectionId]="collectionId"
-                                    [sectionId]="'upload'"
                                     [uploadFilesOptions]="uploadFilesOptions"></ds-submission-upload-files>`;
 
       testFixture = createTestComponent(html, TestComponent) as ComponentFixture<TestComponent>;
@@ -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,54 @@ describe('SubmissionUploadFilesComponent Component', () => {
       expect(compAsAny.uploadEnabled).toBeObservable(expected);
     });
 
-    it('should show a success notification and call updateSectionData on upload complete', () => {
+    describe('on upload complete', () => {
-
+      beforeEach(() => {
-      const expectedErrors: any = mockUploadResponse1ParsedErrors;
+        sectionsServiceStub.isSectionType.and.callFake((_, sectionId, __) => observableOf(sectionId === 'upload'));
-      compAsAny.uploadEnabled = observableOf(true);
+        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]
-        );
       });
 
-      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 +208,6 @@ class TestComponent {
 
   submissionId = mockSubmissionId;
   collectionId = mockSubmissionCollectionId;
-  sectionId = 'upload';
   uploadFilesOptions = Object.assign(new UploaderOptions(), {
     url: '',
     authToken: null,

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';
@@ -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,18 @@ export class SubmissionUploadFilesComponent implements OnChanges {
                 .forEach((sectionId) => {
                   const sectionData = normalizeSectionData(sections[sectionId]);
                   const sectionErrors = errorsList[sectionId];
-                  if (sectionId === 'upload') {
+                  this.sectionService.isSectionType(this.submissionId, sectionId, SectionsType.Upload)
-                    // Look for errors on upload
+                      .pipe(take(1))
-                    if ((isEmpty(sectionErrors))) {
+                      .subscribe((isUpload) => {
-                      this.notificationsService.success(null, this.translate.get('submission.sections.upload.upload-successful'));
+                        if (isUpload) {
-                    } else {
+                          // Look for errors on upload
-                      this.notificationsService.error(null, this.translate.get('submission.sections.upload.upload-failed'));
+                          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);
                 });
             }

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', () => {
 

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<boolean> {
+    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
    *

src/app/submission/sections/upload/file/section-upload-file.component.ts

@@ -288,14 +288,13 @@ export class SubmissionSectionUploadFileComponent implements OnChanges, OnInit {
           this.pathCombiner.subRootElement);
       })
     ).subscribe((result: SubmissionObject[]) => {
-      if (result[0].sections.upload) {
+      if (result[0].sections[this.sectionId]) {
-        Object.keys((result[0].sections.upload as WorkspaceitemSectionUploadObject).files)
+        const uploadSection = (result[0].sections[this.sectionId] as WorkspaceitemSectionUploadObject);
-          .filter((key) => (result[0].sections.upload as WorkspaceitemSectionUploadObject).files[key].uuid === this.fileId)
+        Object.keys(uploadSection.files)
+          .filter((key) => uploadSection.files[key].uuid === this.fileId)
           .forEach((key) => this.uploadService.updateFileData(
-            this.submissionId,
+            this.submissionId, this.sectionId, this.fileId, uploadSection.files[key])
-            this.sectionId,
+          );
-            this.fileId,
-            (result[0].sections.upload as WorkspaceitemSectionUploadObject).files[key]));
       }
       this.switchMode();
     }));