[Rollups] Improve Rollup Job Wizard error handling (elastic#25092)

* Consistently format errors with showApiWarning and showApiError helpers. * Use fatalError for unexpected errors. * Localize errors. * Surface error in Job List when the jobs can't be loaded.
cjcenizal · Nov 5, 2018 · ff6a80b · ff6a80b
1 parent 3a5e748
commit ff6a80b
Show file tree

Hide file tree

Showing 10 changed files with 168 additions and 33 deletions.
diff --git a/x-pack/plugins/rollup/public/crud_app/app.js b/x-pack/plugins/rollup/public/crud_app/app.js
@@ -9,7 +9,7 @@ import PropTypes from 'prop-types';
 import { Switch, Route, Redirect } from 'react-router-dom';
 
 import { CRUD_APP_BASE_PATH } from './constants';
-import { registerRouter } from './services';
+import { registerRouter, setUserHasLeftApp } from './services';
 import { JobList, JobCreate } from './sections';
 
 export class App extends Component {
@@ -33,6 +33,11 @@ export class App extends Component {
     registerRouter(router);
   }
 
+  componentWillUnmount() {
+    // Set internal flag so we can prevent reacting to route changes internally.
+    setUserHasLeftApp(true);
+  }
+
   render() {
     return (
       <div>

diff --git a/x-pack/plugins/rollup/public/crud_app/sections/job_create/job_create.js b/x-pack/plugins/rollup/public/crud_app/sections/job_create/job_create.js
@@ -267,7 +267,9 @@ export class JobCreateUi extends Component {
 
       // This error isn't an HTTP error, so let the fatal error screen tell the user something
       // unexpected happened.
-      fatalError(error, 'Rollup Job Wizard index pattern validation');
+      fatalError(error, i18n.translate('xpack.rollupJobs.create.errors.indexPatternValidationFatalErrorTitle', {
+        defaultMessage: 'Rollup Job Wizard index pattern validation',
+      }));
     });
   }, 300);
 

diff --git a/x-pack/plugins/rollup/public/crud_app/sections/job_list/job_list.js b/x-pack/plugins/rollup/public/crud_app/sections/job_list/job_list.js
@@ -127,6 +127,34 @@ export class JobListUi extends Component {
     );
   }
 
+  renderError(error) {
+    // We can safely depend upon the shape of this error coming from Angular $http, because we
+    // handle unexpected error shapes in the API action.
+    const {
+      statusCode,
+      error: errorString,
+    } = error.data;
+
+    const { intl } = this.props;
+    const title = intl.formatMessage({
+      id: 'xpack.rollupJobs.jobList.loadingErrorTitle',
+      defaultMessage: 'Error loading rollup jobs',
+    });
+    return (
+      <Fragment>
+        {this.getHeaderSection()}
+        <EuiSpacer size="m" />
+        <EuiCallOut
+          title={title}
+          color="danger"
+          iconType="alert"
+        >
+          {statusCode} {errorString}
+        </EuiCallOut>
+      </Fragment>
+    );
+  }
+
   renderEmpty() {
     return (
       <EuiEmptyPrompt
@@ -224,8 +252,13 @@ export class JobListUi extends Component {
     const { isLoading, jobs, jobLoadError } = this.props;
 
     let content;
-    if (jobLoadError && jobLoadError.status === 403) {
-      content = this.renderNoPermission();
+
+    if (jobLoadError) {
+      if (jobLoadError.status === 403) {
+        content = this.renderNoPermission();
+      } else {
+        content = this.renderError(jobLoadError);
+      }
     } else if (!isLoading && !jobs.length) {
       content = this.renderEmpty();
     } else {

diff --git a/x-pack/plugins/rollup/public/crud_app/services/api_errors.js b/x-pack/plugins/rollup/public/crud_app/services/api_errors.js
@@ -0,0 +1,42 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { fatalError, toastNotifications } from 'ui/notify';
+
+function createToastConfig(error, errorTitle) {
+  // Expect an error in the shape provided by Angular's $http service.
+  if (error && error.data) {
+    const { error: errorString, statusCode, message } = error.data;
+    return {
+      title: errorTitle,
+      text: `${statusCode}: ${errorString}. ${message}`,
+    };
+  }
+}
+
+export function showApiWarning(error, errorTitle) {
+  const toastConfig = createToastConfig(error, errorTitle);
+
+  if (toastConfig) {
+    return toastNotifications.addWarning(toastConfig);
+  }
+
+  // This error isn't an HTTP error, so let the fatal error screen tell the user something
+  // unexpected happened.
+  return fatalError(error, errorTitle);
+}
+
+export function showApiError(error, errorTitle) {
+  const toastConfig = createToastConfig(error, errorTitle);
+
+  if (toastConfig) {
+    return toastNotifications.addDanger(toastConfig);
+  }
+
+  // This error isn't an HTTP error, so let the fatal error screen tell the user something
+  // unexpected happened.
+  fatalError(error, errorTitle);
+}
diff --git a/x-pack/plugins/rollup/public/crud_app/services/index.js b/x-pack/plugins/rollup/public/crud_app/services/index.js
@@ -13,6 +13,11 @@ export {
   validateIndexPattern,
 } from './api';
 
+export {
+  showApiError,
+  showApiWarning,
+} from './api_errors';
+
 export {
   cronExpressionToParts,
   cronPartsToExpression,

diff --git a/x-pack/plugins/rollup/public/crud_app/store/actions/change_job_status.js b/x-pack/plugins/rollup/public/crud_app/store/actions/change_job_status.js
@@ -4,11 +4,13 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { toastNotifications } from 'ui/notify';
+import { i18n } from '@kbn/i18n';
+
 import {
   startJobs as sendStartJobsRequest,
   stopJobs as sendStopJobsRequest,
   createNoticeableDelay,
+  showApiError,
 } from '../../services';
 
 import {
@@ -31,7 +33,9 @@ export const startJobs = (jobIds) => async (dispatch) => {
       type: UPDATE_JOB_FAILURE,
     });
 
-    return toastNotifications.addDanger(error.data.message);
+    return showApiError(error, i18n.translate('xpack.rollupJobs.startJobsAction.errorTitle', {
+      defaultMessage: 'Error starting rollup jobs',
+    }));
   }
 
   dispatch({
@@ -53,7 +57,9 @@ export const stopJobs = (jobIds) => async (dispatch) => {
       type: UPDATE_JOB_FAILURE,
     });
 
-    return toastNotifications.addDanger(error.data.message);
+    return showApiError(error, i18n.translate('xpack.rollupJobs.stopJobsAction.errorTitle', {
+      defaultMessage: 'Error stopping rollup jobs',
+    }));
   }
 
   dispatch({

diff --git a/x-pack/plugins/rollup/public/crud_app/store/actions/create_job.js b/x-pack/plugins/rollup/public/crud_app/store/actions/create_job.js
@@ -4,6 +4,9 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
+import { i18n } from '@kbn/i18n';
+import { fatalError } from 'ui/notify';
+
 import { CRUD_APP_BASE_PATH } from '../../constants';
 import {
   createJob as sendCreateJobRequest,
@@ -33,34 +36,46 @@ export const createJob = (jobConfig) => async (dispatch) => {
       new Promise(resolve => setTimeout(resolve, 500)),
     ]);
   } catch (error) {
-    const { statusCode, data } = error;
+    if (error) {
+      const { statusCode, data } = error;
 
-    // Some errors have statusCode directly available but some are under a data property.
-    switch (statusCode || data.statusCode) {
-      case 409:
-        dispatch({
-          type: CREATE_JOB_FAILURE,
-          payload: {
-            error: {
-              message: `A job with ID '${jobConfig.id}' already exists.`,
+      // Expect an error in the shape provided by Angular's $http service.
+      if (data) {
+        // Some errors have statusCode directly available but some are under a data property.
+        if ((statusCode || (data && data.statusCode)) === 409) {
+          return dispatch({
+            type: CREATE_JOB_FAILURE,
+            payload: {
+              error: {
+                message: i18n.translate('xpack.rollupJobs.createAction.jobIdAlreadyExistsErrorMessage', {
+                  defaultMessage: `A job with ID '{jobConfigId}' already exists.`,
+                  values: { jobConfigId: jobConfig.id },
+                }),
+              },
             },
-          },
-        });
-        break;
+          });
+        }
 
-      default:
-        dispatch({
+        return dispatch({
           type: CREATE_JOB_FAILURE,
           payload: {
             error: {
-              message: `Request failed with a ${statusCode} error. ${data.message}`,
+              message: i18n.translate('xpack.rollupJobs.createAction.failedDefaultErrorMessage', {
+                defaultMessage: 'Request failed with a {statusCode} error. {message}',
+                values: { statusCode, message: data.message },
+              }),
               cause: data.cause,
             },
           },
         });
+      }
     }
 
-    return;
+    // This error isn't an HTTP error, so let the fatal error screen tell the user something
+    // unexpected happened.
+    return fatalError(error, i18n.translate('xpack.rollupJobs.createAction.errorTitle', {
+      defaultMessage: 'Error creating rollup job',
+    }));
   }
 
   dispatch({

diff --git a/x-pack/plugins/rollup/public/crud_app/store/actions/delete_jobs.js b/x-pack/plugins/rollup/public/crud_app/store/actions/delete_jobs.js
@@ -4,9 +4,14 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
+import { i18n } from '@kbn/i18n';
 import { toastNotifications } from 'ui/notify';
 
-import { deleteJobs as sendDeleteJobsRequest, createNoticeableDelay } from '../../services';
+import {
+  deleteJobs as sendDeleteJobsRequest,
+  createNoticeableDelay,
+  showApiError,
+} from '../../services';
 import { getDetailPanelJob } from '../selectors';
 
 import {
@@ -30,13 +35,21 @@ export const deleteJobs = (jobIds) => async (dispatch, getState) => {
       type: UPDATE_JOB_FAILURE,
     });
 
-    return toastNotifications.addDanger(error.data.message);
+    return showApiError(error, i18n.translate('xpack.rollupJobs.deleteAction.errorTitle', {
+      defaultMessage: 'Error deleting rollup jobs',
+    }));
   }
 
   if (jobIds.length === 1) {
-    toastNotifications.addSuccess(`Rollup job '${jobIds[0]}' was deleted`);
+    toastNotifications.addSuccess(i18n.translate('xpack.rollupJobs.deleteAction.successSingleNotificationTitle', {
+      defaultMessage: `Rollup job '{jobId}' was deleted`,
+      values: { jobId: jobIds[0] },
+    }));
   } else {
-    toastNotifications.addSuccess(`${jobIds.length} rollup jobs were deleted`);
+    toastNotifications.addSuccess(i18n.translate('xpack.rollupJobs.deleteAction.successMultipleNotificationTitle', {
+      defaultMessage: '{count} rollup jobs were deleted',
+      values: { count: jobIds.length },
+    }));
   }
 
   // If we've just deleted a job we were looking at, we need to close the panel.

diff --git a/x-pack/plugins/rollup/public/crud_app/store/actions/load_jobs.js b/x-pack/plugins/rollup/public/crud_app/store/actions/load_jobs.js
@@ -4,8 +4,13 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { toastNotifications } from 'ui/notify';
-import { loadJobs as sendLoadJobsRequest, deserializeJobs } from '../../services';
+import { i18n } from '@kbn/i18n';
+
+import {
+  loadJobs as sendLoadJobsRequest,
+  deserializeJobs,
+  showApiError,
+} from '../../services';
 import {
   LOAD_JOBS_START,
   LOAD_JOBS_SUCCESS,
@@ -26,7 +31,9 @@ export const loadJobs = () => async (dispatch) => {
       payload: { error }
     });
 
-    return toastNotifications.addDanger(error.data.message);
+    return showApiError(error, i18n.translate('xpack.rollupJobs.loadAction.errorTitle', {
+      defaultMessage: 'Error loading rollup jobs',
+    }));
   }
 
   dispatch({

diff --git a/x-pack/plugins/rollup/public/crud_app/store/actions/refresh_jobs.js b/x-pack/plugins/rollup/public/crud_app/store/actions/refresh_jobs.js
@@ -4,8 +4,13 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { toastNotifications } from 'ui/notify';
-import { loadJobs as sendLoadJobsRequest, deserializeJobs } from '../../services';
+import { i18n } from '@kbn/i18n';
+
+import {
+  loadJobs as sendLoadJobsRequest,
+  deserializeJobs,
+  showApiWarning,
+} from '../../services';
 import {
   REFRESH_JOBS_SUCCESS,
 } from '../action_types';
@@ -15,7 +20,9 @@ export const refreshJobs = () => async (dispatch) => {
   try {
     jobs = await sendLoadJobsRequest();
   } catch (error) {
-    return toastNotifications.addWarning(error.data.message);
+    return showApiWarning(error, i18n.translate('xpack.rollupJobs.refreshAction.errorTitle', {
+      defaultMessage: 'Error refreshing rollup jobs',
+    }));
   }
 
   dispatch({