Merged in task/dspace-cris-2023_02_x/DSC-1677 (pull request DSpace#1855)

[DSC-1677] implement error visualization for upload section, fix section state update, add tests Approved-by: Giuseppe Digilio
uniba-ub · Jul 23, 2024 · d8da69f · d8da69f
2 parents 2cbd7fe + fece8f3
commit d8da69f
Show file tree

Hide file tree

Showing 14 changed files with 195 additions and 42 deletions.
diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/onebox/dynamic-onebox.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/onebox/dynamic-onebox.component.ts
@@ -323,7 +323,7 @@ export class DsDynamicOneboxComponent extends DsDynamicVocabularyComponent imple
       this.previousValue = result;
       this.cdr.detectChanges();
     }
-    if (hasValue(this.currentValue.otherInformation)) {
+    if (hasValue(this.currentValue?.otherInformation)) {
       const infoKeys = Object.keys(this.currentValue.otherInformation);
       this.setMultipleValuesForOtherInfo(infoKeys, this.currentValue);
     }

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
@@ -11,7 +11,6 @@ import {
   mockSubmissionCollectionId,
   mockSubmissionId,
   mockSubmissionObject,
-  mockUploadResponse1ParsedErrors,
   mockUploadResponse2Errors,
   mockUploadResponse2ParsedErrors
 } from '../../../shared/mocks/submission.mock';
@@ -154,17 +153,21 @@ describe('SubmissionUploadFilesComponent Component', () => {
         compAsAny.uploadEnabled = observableOf(true);
       });
 
-      it('should show a success notification and call updateSectionData if successful', () => {
-        const expectedErrors: any = mockUploadResponse1ParsedErrors;
+      it('should show a success notification and call updateSectionData if successful and no errors are present', () => {
+        const expectedErrors: any = [];
         fixture.detectChanges();
+        const data = {
+          upload: {
+            files: [{url: 'testUrl'}]
+          }};
 
-        comp.onCompleteItem(Object.assign({}, uploadRestResponse, { sections: mockSectionsData }));
+        comp.onCompleteItem(Object.assign({}, uploadRestResponse, { sections: data }));
 
-        Object.keys(mockSectionsData).forEach((sectionId) => {
+        Object.keys(data).forEach((sectionId) => {
           expect(sectionsServiceStub.updateSectionData).toHaveBeenCalledWith(
             submissionId,
             sectionId,
-            mockSectionsData[sectionId],
+            data[sectionId],
           expectedErrors[sectionId],
             expectedErrors[sectionId]
           );

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
@@ -14,6 +14,12 @@ 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';
+import { SubmissionSectionError } from '../../objects/submission-section-error.model';
+import { isNumeric } from '../../../shared/numeric.util';
+import { difference } from '../../../shared/object.util';
+import {
+  WorkspaceitemSectionUploadFileObject
+} from '../../../core/submission/models/workspaceitem-section-upload-file.model';
 
 /**
  * This component represents the drop zone that provides to add files to the submission.
@@ -130,17 +136,24 @@ export class SubmissionUploadFilesComponent implements OnChanges {
               Object.keys(sections)
                 .forEach((sectionId) => {
                   const sectionData = normalizeSectionData(sections[sectionId]);
+                  const sectionWarning = hasValue(sectionData.files) ? this.parseErrorsForWarning(sectionData.files, errorsList[sectionId]) : [];
+                  const errorsForErrorNotification = difference(errorsList[sectionId], sectionWarning) as SubmissionSectionError[];
                   const sectionErrors = errorsList[sectionId];
+
                   this.sectionService.isSectionType(this.submissionId, sectionId, SectionsType.Upload)
                       .pipe(take(1))
                       .subscribe((isUpload) => {
                         if (isUpload) {
                           // Look for errors on upload
-                          if ((isEmpty(sectionErrors))) {
+                          if ((isEmpty(sectionErrors)) || !isEmpty(sectionWarning)) {
                             this.notificationsService.success(null, this.translate.get('submission.sections.upload.upload-successful'));
-                          } else {
+                          } else if (!isEmpty(errorsForErrorNotification)) {
                             this.notificationsService.error(null, this.translate.get('submission.sections.upload.upload-failed'));
                           }
+
+                          if (!(isEmpty(sectionWarning))) {
+                            this.notificationsService.warning(null, this.translate.get('submission.sections.upload.upload-warning'));
+                          }
                         }
                       });
                   this.sectionService.updateSectionData(this.submissionId, sectionId, sectionData, sectionErrors, sectionErrors);
@@ -167,4 +180,22 @@ export class SubmissionUploadFilesComponent implements OnChanges {
       .filter((subscription) => hasValue(subscription))
       .forEach((subscription) => subscription.unsubscribe());
   }
+
+  /**
+   * Check if there are errors related to metadata connected to files successfully uploaded
+   *
+   * @param files
+   * @param sectionErrors
+   * @private
+   */
+  private parseErrorsForWarning(files: WorkspaceitemSectionUploadFileObject[], sectionErrors: SubmissionSectionError[]): SubmissionSectionError[] {
+    return !isEmpty(sectionErrors) ? sectionErrors.filter((error) => {
+      const errorPaths = error.path.split('/');
+      const errorIndexPart = errorPaths[errorPaths.length - 1];
+      // if index is not a number means that only one item is present
+      const errorIndex =  isNumeric(errorIndexPart) ? parseInt(errorIndexPart, 10) : 0;
+      //we check if the files as an url with value, meaning the upload has been successfull
+      return hasValue(files[errorIndex]) && hasValue(files[errorIndex].url);
+    }) : [];
+  }
 }
diff --git a/src/app/submission/objects/submission-objects.reducer.ts b/src/app/submission/objects/submission-objects.reducer.ts
@@ -927,12 +927,20 @@ function editFileData(state: SubmissionObjectState, action: EditFileDataAction):
  */
 function deleteFile(state: SubmissionObjectState, action: DeleteUploadedFileAction): SubmissionObjectState {
   const filesData = state[ action.payload.submissionId ].sections[ action.payload.sectionId ].data as WorkspaceitemSectionUploadObject;
+  const filesErrorsToShow = state[ action.payload.submissionId ].sections[ action.payload.sectionId ].errorsToShow ?? [];
+  const filesSeverValidationErrors = state[ action.payload.submissionId ].sections[ action.payload.sectionId ].serverValidationErrors ?? [];
+
   if (hasValue(filesData.files)) {
     const fileIndex: any = findKey(
       filesData.files,
       {uuid: action.payload.fileId});
     if (isNotNull(fileIndex)) {
       const newData = Array.from(filesData.files);
+      const newErrorsToShow = filesData.files.length > 1  ? filesErrorsToShow
+        .filter(errorToShow => !errorToShow.path.includes(fileIndex)) : [];
+      const newServerErrorsToShow = filesData.files.length > 1  ? filesSeverValidationErrors
+        .filter(serverError => !serverError.path.includes(fileIndex)) : [];
+
       newData.splice(fileIndex, 1);
       return Object.assign({}, state, {
         [ action.payload.submissionId ]: Object.assign({}, state[action.payload.submissionId], {
@@ -941,7 +949,9 @@ function deleteFile(state: SubmissionObjectState, action: DeleteUploadedFileActi
               [ action.payload.sectionId ]: Object.assign({}, state[ action.payload.submissionId ].sections[ action.payload.sectionId ], {
                 data: Object.assign({}, state[ action.payload.submissionId ].sections[ action.payload.sectionId ].data, {
                   files: newData
-                })
+                }),
+                errorsToShow: newErrorsToShow,
+                serverValidationErrors: newServerErrorsToShow,
               })
             })
           )

diff --git a/src/app/submission/sections/sections.directive.ts b/src/app/submission/sections/sections.directive.ts
@@ -92,6 +92,12 @@ export class SectionsDirective implements OnDestroy, OnInit {
    */
   private subs: Subscription[] = [];
 
+  /**
+   * Dedicated properties for error checking
+   * @private
+   */
+  private errorsSub: Subscription;
+
   /**
    * A boolean representing if section is valid
    * @type {boolean}
@@ -119,29 +125,16 @@ export class SectionsDirective implements OnDestroy, OnInit {
       map((valid: boolean) => {
         if (valid) {
           this.resetErrors();
+        } else if (hasValue(this.errorsSub)) {
+          //create new subscription to set any possible remaining error, so to avoid timing issue in status update
+          this.errorsSub.unsubscribe();
+          this.checkForNewErrors();
         }
         return valid;
       }));
 
+    this.errorsSub = this.checkForNewErrors();
     this.subs.push(
-      this.sectionService.getShownSectionErrors(this.submissionId, this.sectionId, this.sectionType)
-        .subscribe((errors: SubmissionSectionError[]) => {
-          if (isNotEmpty(errors)) {
-            errors.forEach((errorItem: SubmissionSectionError) => {
-              const parsedErrors: SectionErrorPath[] = parseSectionErrorPaths(errorItem.path);
-
-              parsedErrors.forEach((error: SectionErrorPath) => {
-                if (!error.fieldId) {
-                  this.genericSectionErrors = uniq(this.genericSectionErrors.concat(errorItem.message));
-                } else {
-                  this.allSectionErrors.push(errorItem.message);
-                }
-              });
-            });
-          } else {
-            this.resetErrors();
-          }
-        }),
       this.submissionService.getActiveSectionId(this.submissionId)
         .subscribe((activeSectionId) => {
           const previousActive = this.active;
@@ -344,4 +337,25 @@ export class SectionsDirective implements OnDestroy, OnInit {
     this.allSectionErrors = [];
 
   }
+
+  private checkForNewErrors() {
+    return this.sectionService.getShownSectionErrors(this.submissionId, this.sectionId, this.sectionType)
+      .subscribe((errors: SubmissionSectionError[]) => {
+        if (isNotEmpty(errors)) {
+          errors.forEach((errorItem: SubmissionSectionError) => {
+            const parsedErrors: SectionErrorPath[] = parseSectionErrorPaths(errorItem.path);
+
+            parsedErrors.forEach((error: SectionErrorPath) => {
+              if (!error.fieldId) {
+                this.genericSectionErrors = uniq(this.genericSectionErrors.concat(errorItem.message));
+              } else {
+                this.allSectionErrors.push(errorItem.message);
+              }
+            });
+          });
+        } else {
+          this.resetErrors();
+        }
+      });
+  }
 }
diff --git a/src/app/submission/sections/upload/file/edit/section-upload-file-edit.component.spec.ts b/src/app/submission/sections/upload/file/edit/section-upload-file-edit.component.spec.ts
@@ -47,6 +47,8 @@ import {
 } from '../../../../../core/json-patch/builder/json-patch-operation-path-combiner';
 import { dateToISOFormat } from '../../../../../shared/date.util';
 import { of } from 'rxjs';
+import { SectionsService } from '../../../sections.service';
+import { SectionsServiceStub } from '../../../../../shared/testing/sections-service.stub';
 
 const jsonPatchOpBuilder: any = jasmine.createSpyObj('jsonPatchOpBuilder', {
   add: jasmine.createSpy('add'),
@@ -62,6 +64,7 @@ describe('SubmissionSectionUploadFileEditComponent test suite', () => {
   let compAsAny: any;
   let fixture: ComponentFixture<SubmissionSectionUploadFileEditComponent>;
   let submissionServiceStub: SubmissionServiceStub;
+  let sectionServiceStub: SectionsServiceStub;
   let formbuilderService: any;
   let operationsBuilder: any;
   let operationsService: any;
@@ -100,6 +103,7 @@ describe('SubmissionSectionUploadFileEditComponent test suite', () => {
         { provide: SubmissionJsonPatchOperationsService, useValue: submissionJsonPatchOperationsServiceStub },
         { provide: JsonPatchOperationsBuilder, useValue: jsonPatchOpBuilder },
         { provide: SectionUploadService, useValue: getMockSectionUploadService() },
+        { provide: SectionsService, useClass: SectionsServiceStub },
         FormBuilderService,
         ChangeDetectorRef,
         SubmissionSectionUploadFileEditComponent,
@@ -151,6 +155,7 @@ describe('SubmissionSectionUploadFileEditComponent test suite', () => {
       comp = fixture.componentInstance;
       compAsAny = comp;
       submissionServiceStub = TestBed.inject(SubmissionService as any);
+      sectionServiceStub = TestBed.inject(SectionsService as any);
       formbuilderService = TestBed.inject(FormBuilderService);
       operationsBuilder = TestBed.inject(JsonPatchOperationsBuilder);
       operationsService = TestBed.inject(SubmissionJsonPatchOperationsService);
@@ -325,7 +330,6 @@ describe('SubmissionSectionUploadFileEditComponent test suite', () => {
       expect(uploadService.updateFileData).not.toHaveBeenCalled();
 
     }));
-
   });
 });
 

diff --git a/src/app/submission/sections/upload/file/edit/section-upload-file-edit.component.ts b/src/app/submission/sections/upload/file/edit/section-upload-file-edit.component.ts
@@ -61,6 +61,9 @@ import { DynamicFormControlCondition } from '@ng-dynamic-forms/core/lib/model/mi
 import { DynamicDateControlValue } from '@ng-dynamic-forms/core/lib/model/dynamic-date-control.model';
 import { DynamicSelectModelConfig } from '@ng-dynamic-forms/core/lib/model/select/dynamic-select.model';
 import { TranslateService } from '@ngx-translate/core';
+import parseSectionErrors from '../../../../utils/parseSectionErrors';
+import { SectionsService } from '../../../sections.service';
+import { normalizeSectionData } from '../../../../../core/submission/submission-response-parsing.service';
 
 /**
  * This component represents the edit form for bitstream
@@ -181,6 +184,8 @@ export class SubmissionSectionUploadFileEditComponent
    * @param {JsonPatchOperationsBuilder} operationsBuilder
    * @param {SubmissionJsonPatchOperationsService} operationsService
    * @param {SectionUploadService} uploadService
+   * @param translate
+   * @param sectionService
    */
   constructor(
     protected activeModal: NgbActiveModal,
@@ -192,6 +197,7 @@ export class SubmissionSectionUploadFileEditComponent
     private operationsService: SubmissionJsonPatchOperationsService,
     private uploadService: SectionUploadService,
     private translate: TranslateService,
+    private sectionService: SectionsService,
   ) {
   }
 
@@ -478,12 +484,20 @@ export class SubmissionSectionUploadFileEditComponent
       })
     ).subscribe((result: SubmissionObject[]) => {
       if (result[0].sections[this.sectionId]) {
-        const uploadSection = (result[0].sections[this.sectionId] as WorkspaceitemSectionUploadObject);
+        const resultSection = result[0].sections[this.sectionId];
+        const uploadSection = (resultSection as WorkspaceitemSectionUploadObject);
+        const { errors } = result[0];
+        const errorsList = parseSectionErrors(errors);
+        const sectionData = normalizeSectionData(resultSection);
+        const sectionErrors = errorsList[this.sectionId];
+
         Object.keys(uploadSection.files)
           .filter((key) => uploadSection.files[key].uuid === this.fileId)
           .forEach((key) => this.uploadService.updateFileData(
             this.submissionId, this.sectionId, this.fileId, uploadSection.files[key])
           );
+
+        this.sectionService.updateSectionData(this.submissionId, this.sectionId, sectionData, sectionErrors, sectionErrors);
       }
       this.isSaving = false;
       this.activeModal.close();

diff --git a/src/app/submission/sections/upload/file/section-upload-file.component.html b/src/app/submission/sections/upload/file/section-upload-file.component.html
@@ -1,5 +1,7 @@
 <ng-container *ngIf="fileData">
-  <div class="row">
+  <div
+    class="row py-1"
+  >
     <div class="col-md-12">
       <div class="float-left ml-3 mb-2 badge badge-pill bg-primary text-white" *ngIf="(vocabularyFileType$ | async)">
         {{(vocabularyFileType$ | async)}}
@@ -35,6 +37,12 @@ <h4>{{fileName}} <span class="text-secondary">({{fileData?.sizeBytes | dsFileSiz
       <ds-submission-section-upload-file-view [fileData]="fileData"></ds-submission-section-upload-file-view>
     </div>
   </div>
+
+  <div class="pt-4" *ngIf="(getFileHasErrors() | async)">
+    <ds-alert [type]="AlertTypeEnum.Error" [dismissible]="false">
+      <span [innerHTML]="'submission.sections.upload.genericError' | translate"></span>
+    </ds-alert>
+  </div>
 </ng-container>
 
 <ng-template #content let-c="close" let-d="dismiss">