mgr/dashboard: Improve error message handling

The current implementation of the HTTP interceptor does not use the error description in all cases.

Fixes: https://tracker.ceph.com/issues/36190
Signed-off-by: Volker Theile <vtheile@suse.com>

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/api-interceptor.service.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/api-interceptor.service.spec.ts
new file mode 100644 (file)
index 0000000..6d98709
--- /dev/null
+++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/api-interceptor.service.spec.ts
@@ -0,0 +1,168 @@
+import { HTTP_INTERCEPTORS, HttpClient, HttpErrorResponse } from '@angular/common/http';
+import { HttpClientTestingModule, HttpTestingController } from '@angular/common/http/testing';
+import { TestBed } from '@angular/core/testing';
+import { Router } from '@angular/router';
+
+import { configureTestBed } from '../../../testing/unit-test-helper';
+import { AppModule } from '../../app.module';
+import { ApiInterceptorService } from './api-interceptor.service';
+import { NotificationService } from './notification.service';
+
+describe('ApiInterceptorService', () => {
+  let notificationService: NotificationService;
+  let httpTesting: HttpTestingController;
+  let httpClient: HttpClient;
+  let router: Router;
+  const url = 'api/xyz';
+
+  const runRouterTest = (errorOpts, expectedCallParams, done) => {
+    httpClient.get(url).subscribe(
+      () => {},
+      (resp) => {
+        // Error must have been forwarded by the interceptor.
+        expect(resp instanceof HttpErrorResponse).toBeTruthy();
+        done();
+      }
+    );
+    httpTesting.expectOne(url).error(new ErrorEvent('abc'), errorOpts);
+    httpTesting.verify();
+    expect(router.navigate).toHaveBeenCalledWith(...expectedCallParams);
+  };
+
+  const runNotificationTest = (error, errorOpts, expectedCallParams, done) => {
+    httpClient.get(url).subscribe(
+      () => {},
+      (resp) => {
+        // Error must have been forwarded by the interceptor.
+        expect(resp instanceof HttpErrorResponse).toBeTruthy();
+        done();
+      }
+    );
+    httpTesting.expectOne(url).error(error, errorOpts);
+    httpTesting.verify();
+    expect(notificationService.show).toHaveBeenCalledWith(...expectedCallParams);
+  };
+
+  configureTestBed({
+    imports: [AppModule, HttpClientTestingModule],
+    providers: [NotificationService]
+  });
+
+  beforeEach(() => {
+    httpClient = TestBed.get(HttpClient);
+    httpTesting = TestBed.get(HttpTestingController);
+    notificationService = TestBed.get(NotificationService);
+    spyOn(notificationService, 'show');
+    router = TestBed.get(Router);
+    spyOn(router, 'navigate');
+  });
+
+  it('should be created', () => {
+    const service = TestBed.get(ApiInterceptorService);
+    expect(service).toBeTruthy();
+  });
+
+  it('should redirect 401', (done) => {
+    runRouterTest(
+      {
+        status: 401
+      },
+      [['/login']],
+      done
+    );
+  });
+
+  it('should redirect 403', (done) => {
+    runRouterTest(
+      {
+        status: 403
+      },
+      [['/403']],
+      done
+    );
+  });
+
+  it('should redirect 404', (done) => {
+    runRouterTest(
+      {
+        status: 404
+      },
+      [['/404']],
+      done
+    );
+  });
+
+  it('should show notification (error string)', function(done) {
+    runNotificationTest(
+      'foobar',
+      {
+        status: 500,
+        statusText: 'Foo Bar'
+      },
+      [0, '500 - Foo Bar', 'foobar'],
+      done
+    );
+  });
+
+  it('should show notification (error object, triggered from backend)', function(done) {
+    runNotificationTest(
+      { detail: 'abc' },
+      {
+        status: 504,
+        statusText: 'AAA bbb CCC'
+      },
+      [0, '504 - AAA bbb CCC', 'abc'],
+      done
+    );
+  });
+
+  it('should show notification (error object with unknown keys)', function(done) {
+    runNotificationTest(
+      { type: 'error' },
+      {
+        status: 0,
+        statusText: 'Unknown Error',
+        message: 'Http failure response for (unknown url): 0 Unknown Error',
+        name: 'HttpErrorResponse',
+        ok: false,
+        url: null
+      },
+      [0, '0 - Unknown Error', 'Http failure response for api/xyz: 0 Unknown Error'],
+      done
+    );
+  });
+
+  it('should show notification (undefined error)', function(done) {
+    runNotificationTest(
+      undefined,
+      {
+        status: 502
+      },
+      [0, '502 - Unknown Error', 'Http failure response for api/xyz: 502 '],
+      done
+    );
+  });
+
+  it('should show 400 notification', function(done) {
+    spyOn(notificationService, 'notifyTask');
+    httpClient.get(url).subscribe(
+      () => {},
+      (resp) => {
+        // Error must have been forwarded by the interceptor.
+        expect(resp instanceof HttpErrorResponse).toBeTruthy();
+        done();
+      }
+    );
+    httpTesting
+      .expectOne(url)
+      .error({ task: { name: 'mytask', metadata: { component: 'foobar' } } }, { status: 400 });
+    httpTesting.verify();
+    expect(notificationService.show).toHaveBeenCalledTimes(0);
+    expect(notificationService.notifyTask).toHaveBeenCalledWith({
+      exception: { task: { metadata: { component: 'foobar' }, name: 'mytask' } },
+      metadata: { component: 'foobar' },
+      name: 'mytask',
+      success: false
+    });
+  });
+});

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/api-interceptor.service.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/api-interceptor.service.ts
index 28ea70ff40c0a60d905c3f0368e38d157a083d66..60c41debc56084c06a9924e6ea86e036981d4b50 100644 (file)
--- a/src/pybind/mgr/dashboard/frontend/src/app/shared/services/api-interceptor.service.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/api-interceptor.service.ts
@@ -66,10 +66,18 @@ export class ApiInterceptorService implements HttpInterceptor {
 
           let timeoutId;
           if (showNotification) {
+            let message = '';
+            if (_.isPlainObject(resp.error) && _.isString(resp.error.detail)) {
+              message = resp.error.detail; // Error was triggered by the backend.
+            } else if (_.isString(resp.error)) {
+              message = resp.error;
+            } else if (_.isString(resp.message)) {
+              message = resp.message;
+            }
             timeoutId = this.notificationService.show(
               NotificationType.error,
-              resp.error.detail || '',
-              `${resp.status} - ${resp.statusText}`
+              `${resp.status} - ${resp.statusText}`,
+              message
             );
           }