chore: improve modal error handling (apache#13342)

* improve modal error handling * update hook to handle string error message
zkan · Mar 16, 2021 · 21cc495 · 21cc495
1 parent 10d8872
commit 21cc495
Show file tree

Hide file tree

Showing 5 changed files with 79 additions and 63 deletions.
diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
@@ -17,11 +17,13 @@
  * under the License.
  */
 import React, { FunctionComponent, useState, useEffect } from 'react';
-import { styled, t, SupersetClient } from '@superset-ui/core';
+import { styled, t } from '@superset-ui/core';
 import InfoTooltip from 'src/common/components/InfoTooltip';
-import { useSingleViewResource } from 'src/views/CRUD/hooks';
+import {
+  useSingleViewResource,
+  testDatabaseConnection,
+} from 'src/views/CRUD/hooks';
 import withToasts from 'src/messageToasts/enhancers/withToasts';
-import { getClientErrorObject } from 'src/utils/getClientErrorObject';
 import Icon from 'src/components/Icon';
 import Modal from 'src/common/components/Modal';
 import Tabs from 'src/common/components/Tabs';
@@ -168,29 +170,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
       server_cert: db ? db.server_cert || undefined : undefined,
     };
 
-    SupersetClient.post({
-      endpoint: 'api/v1/database/test_connection',
-      body: JSON.stringify(connection),
-      headers: { 'Content-Type': 'application/json' },
-    })
-      .then(() => {
-        addSuccessToast(t('Connection looks good!'));
-      })
-      .catch(response =>
-        getClientErrorObject(response).then(error => {
-          addDangerToast(
-            error?.message
-              ? `${t('ERROR: ')}${
-                  typeof error.message === 'string'
-                    ? error.message
-                    : Object.entries(error.message as Record<string, string[]>)
-                        .map(([key, value]) => `(${key}) ${value.join(', ')}`)
-                        .join('\n')
-                }`
-              : t('ERROR: Connection failed. '),
-          );
-        }),
-      );
+    testDatabaseConnection(connection, addDangerToast, addSuccessToast);
   };
 
   // Functions

diff --git a/superset-frontend/src/views/CRUD/data/dataset/AddDatasetModal.tsx b/superset-frontend/src/views/CRUD/data/dataset/AddDatasetModal.tsx
@@ -17,13 +17,13 @@
  * under the License.
  */
 import React, { FunctionComponent, useState } from 'react';
-import { styled, SupersetClient, t } from '@superset-ui/core';
+import { styled, t } from '@superset-ui/core';
+import { useSingleViewResource } from 'src/views/CRUD/hooks';
 import { isEmpty, isNil } from 'lodash';
 import Icon from 'src/components/Icon';
 import Modal from 'src/common/components/Modal';
 import TableSelector from 'src/components/TableSelector';
 import withToasts from 'src/messageToasts/enhancers/withToasts';
-import { createErrorHandler } from 'src/views/CRUD/utils';
 
 type DatasetAddObject = {
   id: number;
@@ -59,6 +59,11 @@ const DatasetModal: FunctionComponent<DatasetModalProps> = ({
   const [currentTableName, setTableName] = useState('');
   const [datasourceId, setDatasourceId] = useState<number>(0);
   const [disableSave, setDisableSave] = useState(true);
+  const { createResource } = useSingleViewResource<Partial<DatasetAddObject>>(
+    'dataset',
+    t('dataset'),
+    addDangerToast,
+  );
 
   const onChange = ({
     dbId,
@@ -76,32 +81,21 @@ const DatasetModal: FunctionComponent<DatasetModalProps> = ({
   };
 
   const onSave = () => {
-    SupersetClient.post({
-      endpoint: '/api/v1/dataset/',
-      body: JSON.stringify({
-        database: datasourceId,
-        ...(currentSchema ? { schema: currentSchema } : {}),
-        table_name: currentTableName,
-      }),
-      headers: { 'Content-Type': 'application/json' },
-    })
-      .then(({ json = {} }) => {
-        if (onDatasetAdd) {
-          onDatasetAdd({ id: json.id, ...json.result });
-        }
-        addSuccessToast(t('The dataset has been saved'));
-        onHide();
-      })
-      .catch(
-        createErrorHandler((errMsg: unknown) =>
-          addDangerToast(
-            t(
-              'Error while saving dataset: %s',
-              (errMsg as { table_name?: string }).table_name,
-            ),
-          ),
-        ),
-      );
+    const data = {
+      database: datasourceId,
+      ...(currentSchema ? { schema: currentSchema } : {}),
+      table_name: currentTableName,
+    };
+    createResource(data).then(response => {
+      if (!response) {
+        return;
+      }
+      if (onDatasetAdd) {
+        onDatasetAdd({ id: response.id, ...response });
+      }
+      addSuccessToast(t('The dataset has been saved'));
+      onHide();
+    });
   };
 
   return (

diff --git a/superset-frontend/src/views/CRUD/hooks.ts b/superset-frontend/src/views/CRUD/hooks.ts
@@ -26,7 +26,7 @@ import { FilterValue } from 'src/components/ListView/types';
 import Chart, { Slice } from 'src/types/Chart';
 import copyTextToClipboard from 'src/utils/copy';
 import { getClientErrorObject } from 'src/utils/getClientErrorObject';
-import { FavoriteStatus, ImportResourceName } from './types';
+import { FavoriteStatus, ImportResourceName, DatabaseObject } from './types';
 
 interface ListViewResourceState<D extends object = any> {
   loading: boolean;
@@ -38,6 +38,17 @@ interface ListViewResourceState<D extends object = any> {
   lastFetched?: string;
 }
 
+const parsedErrorMessage = (
+  errorMessage: Record<string, string[]> | string,
+) => {
+  if (typeof errorMessage === 'string') {
+    return errorMessage;
+  }
+  return Object.entries(errorMessage)
+    .map(([key, value]) => `(${key}) ${value.join(', ')}`)
+    .join('\n');
+};
+
 export function useListViewResource<D extends object = any>(
   resource: string,
   resourceLabel: string, // resourceLabel for translations
@@ -188,7 +199,7 @@ export function useListViewResource<D extends object = any>(
 interface SingleViewResourceState<D extends object = any> {
   loading: boolean;
   resource: D | null;
-  error: string | null;
+  error: string | Record<string, string[]> | null;
 }
 
 export function useSingleViewResource<D extends object = any>(
@@ -224,12 +235,12 @@ export function useSingleViewResource<D extends object = any>(
             });
             return json.result;
           },
-          createErrorHandler(errMsg => {
+          createErrorHandler((errMsg: Record<string, string[]>) => {
             handleErrorMsg(
               t(
                 'An error occurred while fetching %ss: %s',
                 resourceLabel,
-                JSON.stringify(errMsg),
+                parsedErrorMessage(errMsg),
               ),
             );
 
@@ -265,12 +276,12 @@ export function useSingleViewResource<D extends object = any>(
             });
             return json.id;
           },
-          createErrorHandler(errMsg => {
+          createErrorHandler((errMsg: Record<string, string[]>) => {
             handleErrorMsg(
               t(
                 'An error occurred while creating %ss: %s',
                 resourceLabel,
-                JSON.stringify(errMsg),
+                parsedErrorMessage(errMsg),
               ),
             );
 
@@ -439,7 +450,7 @@ export function useImportResource(
                 t(
                   'An error occurred while importing %s: %s',
                   resourceLabel,
-                  errMsg,
+                  parsedErrorMessage(errMsg),
                 ),
               );
               return false;
@@ -449,7 +460,7 @@ export function useImportResource(
                 t(
                   'An error occurred while importing %s: %s',
                   resourceLabel,
-                  JSON.stringify(errMsg),
+                  parsedErrorMessage(errMsg),
                 ),
               );
             } else {
@@ -605,3 +616,22 @@ export const copyQueryLink = (
       addDangerToast(t('Sorry, your browser does not support copying.'));
     });
 };
+
+export const testDatabaseConnection = (
+  connection: DatabaseObject,
+  handleErrorMsg: (errorMsg: string) => void,
+  addSuccessToast: (arg0: string) => void,
+) => {
+  SupersetClient.post({
+    endpoint: 'api/v1/database/test_connection',
+    body: JSON.stringify(connection),
+    headers: { 'Content-Type': 'application/json' },
+  }).then(
+    () => {
+      addSuccessToast(t('Connection looks good!'));
+    },
+    createErrorHandler((errMsg: Record<string, string[]> | string) => {
+      handleErrorMsg(t(`${t('ERROR: ')}${parsedErrorMessage(errMsg)}`));
+    }),
+  );
+};
diff --git a/superset-frontend/src/views/CRUD/types.ts b/superset-frontend/src/views/CRUD/types.ts
@@ -116,3 +116,13 @@ export enum QueryObjectColumns {
 }
 
 export type ImportResourceName = 'chart' | 'dashboard' | 'database' | 'dataset';
+
+export type DatabaseObject = {
+  allow_run_async?: boolean;
+  database_name?: string;
+  encrypted_extra?: string;
+  extra?: string;
+  impersonate_user?: boolean;
+  server_cert?: string;
+  sqlalchemy_uri: string;
+};
diff --git a/superset-frontend/src/views/CRUD/utils.tsx b/superset-frontend/src/views/CRUD/utils.tsx
@@ -160,7 +160,9 @@ export const getRecentAcitivtyObjs = (
 export const createFetchRelated = createFetchResourceMethod('related');
 export const createFetchDistinct = createFetchResourceMethod('distinct');
 
-export function createErrorHandler(handleErrorFunc: (errMsg?: string) => void) {
+export function createErrorHandler(
+  handleErrorFunc: (errMsg?: string | Record<string, string[]>) => void,
+) {
   return async (e: SupersetClientResponse | string) => {
     const parsedError = await getClientErrorObject(e);
     logging.error(e);