improve error handling in the ui

scm-ui-components/packages/ui-components/package.json

@@ -18,6 +18,7 @@
     "create-index": "^2.3.0",
     "enzyme": "^3.5.0",
     "enzyme-adapter-react-16": "^1.3.1",
+    "fetch-mock": "^7.2.5",
     "flow-bin": "^0.79.1",
     "flow-typed": "^2.5.1",
     "jest": "^23.5.0",
@@ -55,4 +56,4 @@
       ]
     ]
   }
-}
+}

scm-ui-components/packages/ui-components/src/ErrorNotification.js

@@ -2,6 +2,7 @@
 import React from "react";
 import { translate } from "react-i18next";
 import Notification from "./Notification";
+import { BackendError } from "./errors";
 
 type Props = {
   t: string => string,
@@ -9,12 +10,71 @@ type Props = {
 };
 
 class ErrorNotification extends React.Component<Props> {
+
+  renderMoreInformationsLink(error: BackendError) {
+    if (error.url) {
+      // TODO i18n
+      return (
+        <p>
+          For more informations, see <a href={error.url} target="_blank">{error.errorCode}</a>
+        </p>
+      );
+    }
+  }
+
+  renderBackendError(error: BackendError) {
+    // TODO i18n
+    // how should we handle i18n for the message?
+    // we could add translation for known error codes to i18n and pass the context objects as parameters,
+    // but this will not work for errors from plugins, because the ErrorNotification will search for the translation
+    // in the wrong namespace (plugins could only add translations to the plugins namespace.
+    // should we add a special namespace for errors? which could be extend by plugins?
+
+    // TODO error page
+    // we have currently the ErrorNotification, which is often wrapped by the ErrorPage
+    // the ErrorPage has often a SubTitle like "Unkwown xzy error", which is no longer always the case
+    // if the error is a BackendError its not fully unknown
+    return (
+      <div className="content">
+        <p>{error.message}</p>
+        <p><strong>Context:</strong></p>
+        <ul>
+          {error.context.map((context, index) => {
+            return (
+              <li key={index}>
+                <strong>{context.type}:</strong> {context.id}
+              </li>
+            );
+          })}
+        </ul>
+        { this.renderMoreInformationsLink(error) }
+        <div className="level is-size-7">
+          <div className="left">
+            ErrorCode: {error.errorCode}
+          </div>
+          <div className="right">
+            TransactionId: {error.transactionId}
+          </div>
+        </div>
+      </div>
+    );
+  }
+
+  renderError(error: Error) {
+    if (error instanceof BackendError) {
+      return this.renderBackendError(error);
+    } else {
+      return error.message;
+    }
+  }
+
   render() {
-    const { t, error } = this.props;
+    const { error } = this.props;
+
     if (error) {
       return (
         <Notification type="danger">
-          <strong>{t("error-notification.prefix")}:</strong> {error.message}
+          {this.renderError(error)}
         </Notification>
       );
     }

scm-ui-components/packages/ui-components/src/apiclient.js

@@ -1,8 +1,7 @@
 // @flow
 import { contextPath } from "./urls";
-
+import { createBackendError } from "./errors";
-export const NOT_FOUND_ERROR = Error("not found");
+import type { BackendErrorContent } from "./errors";
-export const UNAUTHORIZED_ERROR = Error("unauthorized");
 
 const fetchOptions: RequestOptions = {
   credentials: "same-origin",
@@ -11,15 +10,21 @@ const fetchOptions: RequestOptions = {
   }
 };
 
-function handleStatusCode(response: Response) {
+
+function isBackendError(response) {
+  return response.headers.get("Content-Type") === "application/vnd.scmm-error+json;v=2";
+}
+
+function handleFailure(response: Response) {
   if (!response.ok) {
-    if (response.status === 401) {
+    if (isBackendError(response)) {
-      throw UNAUTHORIZED_ERROR;
+      return response.json()
+        .then((content: BackendErrorContent) => {
+          throw createBackendError(content, response.status);
+        });
+    } else {
+      throw new Error("server returned status code " + response.status);
     }
-    if (response.status === 404) {
-      throw NOT_FOUND_ERROR;
-    }
-    throw new Error("server returned status code " + response.status);
   }
   return response;
 }
@@ -37,7 +42,7 @@ export function createUrl(url: string) {
 
 class ApiClient {
   get(url: string): Promise<Response> {
-    return fetch(createUrl(url), fetchOptions).then(handleStatusCode);
+    return fetch(createUrl(url), fetchOptions).then(handleFailure);
   }
 
   post(url: string, payload: any, contentType: string = "application/json") {
@@ -53,7 +58,7 @@ class ApiClient {
       method: "HEAD"
     };
     options = Object.assign(options, fetchOptions);
-    return fetch(createUrl(url), options).then(handleStatusCode);
+    return fetch(createUrl(url), options).then(handleFailure);
   }
 
   delete(url: string): Promise<Response> {
@@ -61,7 +66,7 @@ class ApiClient {
       method: "DELETE"
     };
     options = Object.assign(options, fetchOptions);
-    return fetch(createUrl(url), options).then(handleStatusCode);
+    return fetch(createUrl(url), options).then(handleFailure);
   }
 
   httpRequestWithJSONBody(
@@ -78,7 +83,7 @@ class ApiClient {
     // $FlowFixMe
     options.headers["Content-Type"] = contentType;
 
-    return fetch(createUrl(url), options).then(handleStatusCode);
+    return fetch(createUrl(url), options).then(handleFailure);
   }
 }
 

scm-ui-components/packages/ui-components/src/apiclient.test.js

@@ -1,5 +1,7 @@
 // @flow
-import { createUrl } from "./apiclient";
+import { apiClient, createUrl } from "./apiclient";
+import {fetchMock} from "fetch-mock";
+import { BackendError } from "./errors";
 
 describe("create url", () => {
   it("should not change absolute urls", () => {
@@ -13,3 +15,64 @@ describe("create url", () => {
     expect(createUrl("users")).toBe("/api/v2/users");
   });
 });
+
+
+describe("error handling tests", () => {
+
+  const earthNotFoundError = {
+    transactionId: "42t",
+    errorCode: "42e",
+    message: "earth not found",
+    context: [{
+      type: "planet",
+      id: "earth"
+    }]
+  };
+
+  afterEach(() => {
+    fetchMock.reset();
+    fetchMock.restore();
+  });
+
+  it("should create a normal error, if the content type is not scmm-error", (done) => {
+
+    fetchMock.getOnce("/api/v2/error", {
+      status: 404
+    });
+
+    apiClient.get("/error")
+      .catch((err: Error) => {
+        expect(err.name).toEqual("Error");
+        expect(err.message).toContain("404");
+        done();
+      });
+  });
+
+  it("should create an backend error, if the content type is scmm-error", (done) => {
+    fetchMock.getOnce("/api/v2/error", {
+      status: 404,
+      headers: {
+        "Content-Type": "application/vnd.scmm-error+json;v=2"
+      },
+      body: earthNotFoundError
+    });
+
+    apiClient.get("/error")
+      .catch((err: BackendError) => {
+
+        expect(err).toBeInstanceOf(BackendError);
+
+        expect(err.message).toEqual("earth not found");
+        expect(err.statusCode).toBe(404);
+
+        expect(err.transactionId).toEqual("42t");
+        expect(err.errorCode).toEqual("42e");
+        expect(err.context).toEqual([{
+          type: "planet",
+          id: "earth"
+        }]);
+        done();
+      });
+  });
+
+});

scm-ui-components/packages/ui-components/src/errors.js

@@ -0,0 +1,53 @@
+// @flow
+type Context = {type: string, id: string}[];
+
+export type BackendErrorContent = {
+  transactionId: string,
+  errorCode: string,
+  message: string,
+  url?: string,
+  context: Context
+};
+
+export class BackendError extends Error {
+
+  transactionId: string;
+  errorCode: string;
+  url: ?string;
+  context: Context = [];
+  statusCode: number;
+
+  constructor(content: BackendErrorContent, name: string, statusCode: number) {
+    super(content.message);
+    this.name = name;
+    this.transactionId = content.transactionId;
+    this.errorCode = content.errorCode;
+    this.url = content.url;
+    this.context = content.context;
+    this.statusCode = statusCode;
+  }
+
+}
+
+export class UnauthorizedError extends BackendError {
+  constructor(content: BackendErrorContent, statusCode: number) {
+    super(content, "UnauthorizedError", statusCode);
+  }
+}
+
+export class NotFoundError extends BackendError {
+  constructor(content: BackendErrorContent, statusCode: number) {
+    super(content, "NotFoundError", statusCode);
+  }
+}
+
+export function createBackendError(content: BackendErrorContent, statusCode: number) {
+  switch (statusCode) {
+    case 403:
+      return new UnauthorizedError(content, statusCode);
+    case 404:
+      return new NotFoundError(content, statusCode);
+    default:
+      return new BackendError(content, "BackendError", statusCode);
+  }
+}

scm-ui-components/packages/ui-components/src/errors.test.js

@@ -0,0 +1,35 @@
+// @flow
+
+import { BackendError, UnauthorizedError, createBackendError, NotFoundError } from "./errors";
+
+describe("test createBackendError", () => {
+
+  const earthNotFoundError = {
+    transactionId: "42t",
+    errorCode: "42e",
+    message: "earth not found",
+    context: [{
+      type: "planet",
+      id: "earth"
+    }]
+  };
+
+  it("should return a default backend error", () => {
+    const err = createBackendError(earthNotFoundError, 500);
+    expect(err).toBeInstanceOf(BackendError);
+    expect(err.name).toBe("BackendError");
+  });
+
+  it("should return an unauthorized error for status code 403", () => {
+    const err = createBackendError(earthNotFoundError, 403);
+    expect(err).toBeInstanceOf(UnauthorizedError);
+    expect(err.name).toBe("UnauthorizedError");
+  });
+
+  it("should return an not found error for status code 404", () => {
+    const err = createBackendError(earthNotFoundError, 404);
+    expect(err).toBeInstanceOf(NotFoundError);
+    expect(err.name).toBe("NotFoundError");
+  });
+
+});

scm-ui-components/packages/ui-components/src/index.js

@@ -22,7 +22,8 @@ export { default as HelpIcon } from "./HelpIcon";
 export { default as Tooltip } from "./Tooltip";
 export { getPageFromMatch } from "./urls";
 
-export { apiClient, NOT_FOUND_ERROR, UNAUTHORIZED_ERROR } from "./apiclient.js";
+export { apiClient } from "./apiclient.js";
+export * from "./errors";
 
 export * from "./buttons";
 export * from "./config";

scm-ui-components/packages/ui-components/yarn.lock

@@ -688,6 +688,10 @@
     react "^16.4.2"
     react-dom "^16.4.2"
 
+"@scm-manager/ui-types@2.0.0-SNAPSHOT":
+  version "2.0.0-20181010-130547"
+  resolved "https://registry.yarnpkg.com/@scm-manager/ui-types/-/ui-types-2.0.0-20181010-130547.tgz#9987b519e43d5c4b895327d012d3fd72429a7953"
+
 "@types/node@*":
   version "10.12.0"
   resolved "https://registry.yarnpkg.com/@types/node/-/node-10.12.0.tgz#ea6dcbddbc5b584c83f06c60e82736d8fbb0c235"
@@ -2995,6 +2999,15 @@ fb-watchman@^2.0.0:
   dependencies:
     bser "^2.0.0"
 
+fetch-mock@^7.2.5:
+  version "7.2.5"
+  resolved "https://registry.yarnpkg.com/fetch-mock/-/fetch-mock-7.2.5.tgz#4682f51b9fa74d790e10a471066cb22f3ff84d48"
+  dependencies:
+    babel-polyfill "^6.26.0"
+    glob-to-regexp "^0.4.0"
+    path-to-regexp "^2.2.1"
+    whatwg-url "^6.5.0"
+
 figures@^2.0.0:
   version "2.0.0"
   resolved "https://registry.yarnpkg.com/figures/-/figures-2.0.0.tgz#3ab1a2d2a62c8bfb431a0c94cb797a2fce27c962"
@@ -3341,6 +3354,10 @@ glob-stream@^3.1.5:
     through2 "^0.6.1"
     unique-stream "^1.0.0"
 
+glob-to-regexp@^0.4.0:
+  version "0.4.0"
+  resolved "https://registry.yarnpkg.com/glob-to-regexp/-/glob-to-regexp-0.4.0.tgz#49bd677b1671022bd10921c3788f23cdebf9c7e6"
+
 glob-watcher@^0.0.6:
   version "0.0.6"
   resolved "https://registry.yarnpkg.com/glob-watcher/-/glob-watcher-0.0.6.tgz#b95b4a8df74b39c83298b0c05c978b4d9a3b710b"
@@ -5982,6 +5999,10 @@ path-to-regexp@^1.7.0:
   dependencies:
     isarray "0.0.1"
 
+path-to-regexp@^2.2.1:
+  version "2.4.0"
+  resolved "https://registry.yarnpkg.com/path-to-regexp/-/path-to-regexp-2.4.0.tgz#35ce7f333d5616f1c1e1bfe266c3aba2e5b2e704"
+
 path-type@^1.0.0:
   version "1.1.0"
   resolved "https://registry.yarnpkg.com/path-type/-/path-type-1.1.0.tgz#59c44f7ee491da704da415da5a4070ba4f8fe441"
@@ -7814,7 +7835,7 @@ whatwg-mimetype@^2.1.0:
   version "2.2.0"
   resolved "https://registry.yarnpkg.com/whatwg-mimetype/-/whatwg-mimetype-2.2.0.tgz#a3d58ef10b76009b042d03e25591ece89b88d171"
 
-whatwg-url@^6.4.1:
+whatwg-url@^6.4.1, whatwg-url@^6.5.0:
   version "6.5.0"
   resolved "https://registry.yarnpkg.com/whatwg-url/-/whatwg-url-6.5.0.tgz#f2df02bff176fd65070df74ad5ccbb5a199965a8"
   dependencies:

scm-ui/src/groups/modules/groups.js

@@ -54,8 +54,7 @@ export function fetchGroupsByLink(link: string) {
       .then(data => {
         dispatch(fetchGroupsSuccess(data));
       })
-      .catch(cause => {
+      .catch(error => {
-        const error = new Error(`could not fetch groups: ${cause.message}`);
         dispatch(fetchGroupsFailure(link, error));
       });
   };
@@ -105,8 +104,7 @@ function fetchGroup(link: string, name: string) {
       .then(data => {
         dispatch(fetchGroupSuccess(data));
       })
-      .catch(cause => {
+      .catch(error => {
-        const error = new Error(`could not fetch group: ${cause.message}`);
         dispatch(fetchGroupFailure(name, error));
       });
   };
@@ -152,11 +150,7 @@ export function createGroup(link: string, group: Group, callback?: () => void) {
         }
       })
       .catch(error => {
-        dispatch(
+        dispatch(createGroupFailure(error));
-          createGroupFailure(
-            new Error(`Failed to create group ${group.name}: ${error.message}`)
-          )
-        );
       });
   };
 }
@@ -201,13 +195,8 @@ export function modifyGroup(group: Group, callback?: () => void) {
       .then(() => {
         dispatch(fetchGroupByLink(group));
       })
-      .catch(cause => {
+      .catch(error => {
-        dispatch(
+        dispatch(modifyGroupFailure(group, error));
-          modifyGroupFailure(
-            group,
-            new Error(`could not modify group ${group.name}: ${cause.message}`)
-          )
-        );
       });
   };
 }
@@ -259,10 +248,7 @@ export function deleteGroup(group: Group, callback?: () => void) {
           callback();
         }
       })
-      .catch(cause => {
+      .catch(error => {
-        const error = new Error(
-          `could not delete group ${group.name}: ${cause.message}`
-        );
         dispatch(deleteGroupFailure(group, error));
       });
   };

scm-ui/src/modules/auth.js

@@ -2,7 +2,7 @@
 import type { Me } from "@scm-manager/ui-types";
 import * as types from "./types";
 
-import { apiClient, UNAUTHORIZED_ERROR } from "@scm-manager/ui-components";
+import { apiClient, UnauthorizedError } from "@scm-manager/ui-components";
 import { isPending } from "./pending";
 import { getFailure } from "./failure";
 import {
@@ -159,7 +159,7 @@ export const login = (
     dispatch(loginPending());
     return apiClient
       .post(loginLink, login_data)
-      .then(response => {
+      .then(() => {
         dispatch(fetchIndexResourcesPending());
         return callFetchIndexResources();
       })
@@ -185,7 +185,7 @@ export const fetchMe = (link: string) => {
         dispatch(fetchMeSuccess(me));
       })
       .catch((error: Error) => {
-        if (error === UNAUTHORIZED_ERROR) {
+        if (error instanceof UnauthorizedError) {
           dispatch(fetchMeUnauthenticated());
         } else {
           dispatch(fetchMeFailure(error));

scm-ui/src/repos/modules/repos.js

@@ -224,8 +224,7 @@ export function modifyRepo(repository: Repository, callback?: () => void) {
       .then(() => {
         dispatch(fetchRepoByLink(repository));
       })
-      .catch(cause => {
+      .catch(error => {
-        const error = new Error(`failed to modify repo: ${cause.message}`);
         dispatch(modifyRepoFailure(repository, error));
       });
   };

scm-ui/src/repos/modules/repositoryTypes.js

@@ -37,10 +37,7 @@ function fetchRepositoryTypes(dispatch: any) {
     .then(repositoryTypes => {
       dispatch(fetchRepositoryTypesSuccess(repositoryTypes));
     })
-    .catch(err => {
+    .catch(error => {
-      const error = new Error(
-        `failed to fetch repository types: ${err.message}`
-      );
       dispatch(fetchRepositoryTypesFailure(error));
     });
 }

scm-ui/src/users/modules/users.js

@@ -57,8 +57,7 @@ export function fetchUsersByLink(link: string) {
       .then(data => {
         dispatch(fetchUsersSuccess(data));
       })
-      .catch(cause => {
+      .catch(error => {
-        const error = new Error(`could not fetch users: ${cause.message}`);
         dispatch(fetchUsersFailure(link, error));
       });
   };
@@ -108,8 +107,7 @@ function fetchUser(link: string, name: string) {
       .then(data => {
         dispatch(fetchUserSuccess(data));
       })
-      .catch(cause => {
+      .catch(error => {
-        const error = new Error(`could not fetch user: ${cause.message}`);
         dispatch(fetchUserFailure(name, error));
       });
   };
@@ -155,12 +153,8 @@ export function createUser(link: string, user: User, callback?: () => void) {
           callback();
         }
       })
-      .catch(err =>
+      .catch(error =>
-        dispatch(
+        dispatch(createUserFailure(error))
-          createUserFailure(
-            new Error(`failed to add user ${user.name}: ${err.message}`)
-          )
-        )
       );
   };
 }
@@ -260,10 +254,7 @@ export function deleteUser(user: User, callback?: () => void) {
           callback();
         }
       })
-      .catch(cause => {
+      .catch(error => {
-        const error = new Error(
-          `could not delete user ${user.name}: ${cause.message}`
-        );
         dispatch(deleteUserFailure(user, error));
       });
   };