[28209] Better error handling on attachment upload

When uploading on a resource that cannot be saved, there will be errors.
We can actually output the validation error to help the user know what
to do.

https://community.openproject.com/wp/28209

frontend/src/app/components/modals/save-modal/save-query.modal.ts

@@ -103,7 +103,7 @@ export class SaveQueryModal extends OpModalComponent {
         this.closeMe($event);
         return Promise.resolve(true);
       })
-      .catch((error:any) => this.wpNotificationsService.handleErrorResponse(error))
+      .catch((error:any) => this.wpNotificationsService.handleRawError(error))
       .then(() => this.isBusy = false); // Same as .finally()
   }
 }

frontend/src/app/components/work-packages/work-package-cache.service.ts

@@ -99,7 +99,7 @@ export class WorkPackageCacheService extends StateCacheService<WorkPackageResour
           resolve(workPackage);
         })
         .catch((error:any) => {
-          this.wpNotificationsService.handleErrorResponse(error, workPackage);
+          this.wpNotificationsService.handleRawError(error, workPackage);
           reject(workPackage);
         });
     });
@@ -150,7 +150,7 @@ export class WorkPackageCacheService extends StateCacheService<WorkPackageResour
     return new Promise<WorkPackageResource>((resolve, reject) => {
 
       const errorAndReject = (error:any) => {
-        this.wpNotificationsService.handleErrorResponse(error);
+        this.wpNotificationsService.handleRawError(error);
         reject(error);
       };
 

frontend/src/app/components/wp-activity/comment-service.ts

@@ -77,7 +77,7 @@ export class CommentService {
   }
 
   private errorAndReject(error:HalResource, workPackage?:WorkPackageResource) {
-    this.wpNotificationsService.handleErrorResponse(error, workPackage);
+    this.wpNotificationsService.handleRawError(error, workPackage);
 
     // returning a reject will enable to correctly work with subsequent then/catch handlers.
     return Promise.reject(error);

frontend/src/app/components/wp-custom-actions/wp-custom-actions/wp-custom-action.component.ts

@@ -75,7 +75,7 @@ export class WpCustomActionComponent {
         this.wpActivity.clear(this.workPackage.id);
         this.wpCacheService.updateWorkPackage(savedWp);
       }).catch((errorResource:any) => {
-        this.wpNotificationsService.handleErrorResponse(errorResource, this.workPackage);
+        this.wpNotificationsService.handleRawError(errorResource, this.workPackage);
       });
   }
 

frontend/src/app/components/wp-edit-form/work-package-changeset.ts

@@ -161,7 +161,7 @@ export class WorkPackageChangeset {
         })
         .catch((error:any) => {
           this.wpForm.clear();
-          this.wpNotificationsService.handleErrorResponse(error, this.workPackage);
+          this.wpNotificationsService.handleRawError(error, this.workPackage);
           reject(error);
         });
     });

frontend/src/app/components/wp-edit-form/work-package-edit-form.ts

@@ -190,7 +190,7 @@ export class WorkPackageEditForm {
           this.wpTableRefresh.request(`Saved work package ${savedWorkPackage.id}`);
         })
         .catch((error:ErrorResource|Object) => {
-          this.wpNotificationsService.handleErrorResponse(error, this.workPackage);
+          this.wpNotificationsService.handleRawError(error, this.workPackage);
 
           if (error instanceof ErrorResource) {
             this.handleSubmissionErrors(error, reject);

frontend/src/app/components/wp-edit/wp-notification.service.ts

@@ -34,6 +34,7 @@ import {Injectable} from '@angular/core';
 import {LoadingIndicatorService} from 'core-app/modules/common/loading-indicator/loading-indicator.service';
 import {NotificationsService} from 'core-app/modules/common/notifications/notifications.service';
 import {I18nService} from "core-app/modules/common/i18n/i18n.service";
+import {HttpErrorResponse} from "@angular/common/http";
 
 @Injectable()
 export class WorkPackageNotificationService {
@@ -57,16 +58,46 @@ export class WorkPackageNotificationService {
     this.NotificationsService.addSuccess(message);
   }
 
+  /**
+   * Handle any kind of error response:
+   * - HAL ErrorResources
+   * - Angular HttpErrorResponses
+   * - Older .data error responses
+   * - String error messages
+   *
+   * @param response
+   * @param workPackage
+   */
   public handleRawError(response:any, workPackage?:WorkPackageResource) {
+    // Some transformation may already have returned the error as a HAL resource,
+    // which we will forward to handleErrorResponse
+    if (response instanceof ErrorResource) {
+      return this.handleErrorResponse(response, workPackage);
+    }
+
+    // Otherwise, we try to detect what we got, this may either be an HttpErrorResponse,
+    // some older XHR response object or a string
+    let errorBody:any|string|null;
+
+    // Angular http response have an error body attribute
+    if (response instanceof HttpErrorResponse) {
+      errorBody = response.error || response.message;
+    }
+
+    // Some older response may have a data attribute
     if (response && response.data && response.data._type === 'Error') {
-      const resource = this.halResourceService.createHalResource(response.data);
+      errorBody = response.data;
+    }
+
+    if (errorBody && errorBody._type === 'Error') {
+      const resource = this.halResourceService.createHalResource(errorBody);
       return this.handleErrorResponse(resource, workPackage);
     }
 
     this.showGeneralError(response);
   }
 
-  public handleErrorResponse(errorResource:any, workPackage?:WorkPackageResource) {
+  protected handleErrorResponse(errorResource:any, workPackage?:WorkPackageResource) {
     if (!(errorResource instanceof ErrorResource)) {
       return this.showGeneralError(errorResource);
     }

frontend/src/app/components/wp-fast-table/state/wp-table-additional-elements.service.ts

@@ -66,7 +66,7 @@ export class WorkPackageTableAdditionalElementsService {
       })
       .catch((e) => {
         this.tableState.additionalRequiredWorkPackages.putValue(null, 'Failure loading required work packages');
-        this.wpNotificationsService.handleErrorResponse(e);
+        this.wpNotificationsService.handleRawError(e);
       });
   }
 

frontend/src/app/components/wp-new/wp-create.controller.ts

@@ -118,7 +118,7 @@ export class WorkPackageCreateController implements OnInit, OnDestroy {
               window.location.href = url.toString();
             }
           });
-          this.wpNotificationsService.handleErrorResponse(error);
+          this.wpNotificationsService.handleRawError(error);
         }
       });
   }

frontend/src/app/components/wp-relations/wp-relation-add-child/wp-relation-add-child.ts

@@ -65,7 +65,7 @@ export class WpRelationAddChildComponent implements OnInit {
         this.toggleRelationsCreateForm();
       })
       .catch(err => {
-        this.wpNotificationsService.handleErrorResponse(err, this.workPackage);
+        this.wpNotificationsService.handleRawError(err, this.workPackage);
         this.isDisabled = false;
         this.toggleRelationsCreateForm();
       });

frontend/src/app/components/wp-relations/wp-relation-row/wp-relation-row.component.ts

@@ -145,7 +145,7 @@ export class WorkPackageRelationRowComponent implements OnInit {
 
         this.userInputs.showRelationTypesForm = false;
       })
-      .catch((error:any) => this.wpNotificationsService.handleErrorResponse(error, this.workPackage));
+      .catch((error:any) => this.wpNotificationsService.handleRawError(error, this.workPackage));
   }
 
   public toggleUserDescriptionForm() {
@@ -158,7 +158,7 @@ export class WorkPackageRelationRowComponent implements OnInit {
         this.wpCacheService.updateWorkPackage(this.relatedWorkPackage);
         this.wpNotificationsService.showSave(this.relatedWorkPackage);
       })
-      .catch((err:any) => this.wpNotificationsService.handleErrorResponse(err,
+      .catch((err:any) => this.wpNotificationsService.handleRawError(err,
         this.relatedWorkPackage));
   }
 }

frontend/src/app/components/wp-relations/wp-relations-create/wp-relations-create.component.ts

@@ -60,7 +60,7 @@ export class WorkPackageRelationsCreateComponent {
         this.toggleRelationsCreateForm();
       })
       .catch(err => {
-        this.wpNotificationsService.handleErrorResponse(err, this.workPackage);
+        this.wpNotificationsService.handleRawError(err, this.workPackage);
         this.toggleRelationsCreateForm();
       });
   }

frontend/src/app/components/wp-relations/wp-relations-hierarchy/wp-relations-hierarchy.service.ts

@@ -74,7 +74,7 @@ export class WorkPackageRelationsHierarchyService {
         return wp;
       })
       .catch((error) => {
-        this.wpNotificationsService.handleErrorResponse(error, workPackage);
+        this.wpNotificationsService.handleRawError(error, workPackage);
         return Promise.reject(error);
       });
   }
@@ -128,7 +128,7 @@ export class WorkPackageRelationsHierarchyService {
         this.wpCacheService.updateWorkPackage(wp);
       })
       .catch((error) => {
-        this.wpNotificationsService.handleErrorResponse(error, childWorkPackage);
+        this.wpNotificationsService.handleRawError(error, childWorkPackage);
         return Promise.reject(error);
       });
     });

frontend/src/app/components/wp-relations/wp-relations-parent/wp-relations-parent.component.ts

@@ -71,7 +71,7 @@ export class WpRelationParentComponent implements OnInit, OnDestroy {
         setTimeout(() => jQuery('#hierarchy--parent').focus());
       })
       .catch((err:any) => {
-        this.wpNotificationsService.handleErrorResponse(err, this.workPackage);
+        this.wpNotificationsService.handleRawError(err, this.workPackage);
       })
       .then(() => this.isSaving = false); // Behaves as .finally()
   }

frontend/src/app/components/wp-table/timeline/cells/wp-timeline-cell-mouse-handler.ts

@@ -258,7 +258,7 @@ export function registerWorkPackageMouseHandler(this:void,
           });
       })
       .catch((error) => {
-        wpNotificationsService.handleErrorResponse(error, renderInfo.workPackage);
+        wpNotificationsService.handleRawError(error, renderInfo.workPackage);
       });
   }
 }

frontend/src/app/components/wp-table/timeline/container/wp-timeline-container.directive.ts

@@ -281,7 +281,7 @@ export class WorkPackageTimelineTableController implements AfterViewInit, OnDest
     this.activateSelectionMode(start.id, end => {
       this.wpRelations
         .addCommonRelation(start as any, 'follows', end.id)
-        .catch((error:any) => this.wpNotificationsService.handleErrorResponse(error, end));
+        .catch((error:any) => this.wpNotificationsService.handleRawError(error, end));
     });
   }
 
@@ -289,7 +289,7 @@ export class WorkPackageTimelineTableController implements AfterViewInit, OnDest
     this.activateSelectionMode(start.id, end => {
       this.wpRelations
         .addCommonRelation(start as any, 'precedes', end.id)
-        .catch((error:any) => this.wpNotificationsService.handleErrorResponse(error, end));
+        .catch((error:any) => this.wpNotificationsService.handleRawError(error, end));
     });
   }
 

frontend/src/app/modules/common/notifications/upload-progress.component.ts

@@ -116,7 +116,7 @@ export class UploadProgressComponent implements OnInit, OnDestroy {
   private handleError(error:HttpErrorResponse, file:UploadFile) {
     let message:string;
 
-    debugLog(error);
+    console.error("Error while uploading: %O", error);
     if (error.error instanceof ErrorEvent) {
       // A client-side or network error occurred.
       message = this.I18n.t('js.error_attachment_upload', {name: file.name, error: error});

frontend/src/app/modules/hal/resources/mixins/attachable-mixin.ts

@@ -38,6 +38,7 @@ import {
 import {WorkPackageNotificationService} from 'core-components/wp-edit/wp-notification.service';
 import {PathHelperService} from 'core-app/modules/common/path-helper/path-helper.service';
 import {NotificationsService} from 'core-app/modules/common/notifications/notifications.service';
+import {HttpErrorResponse} from "@angular/common/http";
 
 type Constructor<T = {}> = new (...args:any[]) => T;
 
@@ -86,7 +87,7 @@ export function Attachable<TBase extends Constructor<HalResource>>(Base:TBase) {
             }
           })
           .catch((error:any) => {
-            this.wpNotificationsService.handleErrorResponse(error, this as any);
+            this.wpNotificationsService.handleRawError(error, this as any);
             this.attachments.elements.push(attachment);
           });
       }
@@ -118,9 +119,9 @@ export function Attachable<TBase extends Constructor<HalResource>>(Base:TBase) {
 
           return result;
         })
-        .catch((error:any) => {
+        .catch((error:HttpErrorResponse) => {
           this.wpNotificationsService.handleRawError(error);
-          return;
+          return _.get(error, 'message', I18n.t('js.error.internal'));
         });
     }
 

frontend/src/app/modules/hal/resources/work-package-resource.spec.ts

@@ -203,10 +203,10 @@ describe('WorkPackage', () => {
           attachment.delete = jasmine.createSpy('delete')
             .and.returnValue(Promise.reject({ foo: 'bar'}));
 
-          errorStub = spyOn(wpNotificationsService, 'handleErrorResponse');
+          errorStub = spyOn(wpNotificationsService, 'handleRawError');
         });
 
-        it('should call the handleErrorResponse notification', (done) => {
+        it('should call the handleRawError notification', (done) => {
           workPackage.removeAttachment(attachment).then(() => {
             expect(errorStub).toHaveBeenCalled();
             done();