From ad715429f92323057110b39d36653018d05c2505 Mon Sep 17 00:00:00 2001 From: Phillip Kelley-Dotson Date: Thu, 21 Apr 2022 12:44:21 -0700 Subject: [PATCH] chore: simplify error messaging in database modal (#19165) * testing for dbconn modal error message * remove consoles and error alert mapping * lint fix * update url message * add modal fix * update comments * fix err msg bugs * fix pylint * fix line * fix tests * fix assertions --- .../DatabaseConnectionForm/TableCatalog.tsx | 1 - .../data/database/DatabaseModal/index.tsx | 87 +++---------------- superset-frontend/src/views/CRUD/hooks.ts | 1 + superset/db_engine_specs/gsheets.py | 6 +- .../db_engine_specs/test_gsheets.py | 18 +++- 5 files changed, 31 insertions(+), 82 deletions(-) diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm/TableCatalog.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm/TableCatalog.tsx index bc8cb40c161c7..fb70b9c3652a1 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm/TableCatalog.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm/TableCatalog.tsx @@ -34,7 +34,6 @@ export const TableCatalog = ({ }: FieldPropTypes) => { const tableCatalog = db?.catalog || []; const catalogError = validationErrors || {}; - return (

diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx index c4faa8a483ebe..a6e93f8653271 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx @@ -96,49 +96,6 @@ const engineSpecificAlertMapping = { }, }; -const errorAlertMapping = { - GENERIC_DB_ENGINE_ERROR: { - message: t('Generic database engine error'), - }, - CONNECTION_MISSING_PARAMETERS_ERROR: { - message: t('Missing Required Fields'), - description: t('Please complete all required fields.'), - }, - CONNECTION_INVALID_HOSTNAME_ERROR: { - message: t('Could not verify the host'), - description: t( - 'The host is invalid. Please verify that this field is entered correctly.', - ), - }, - CONNECTION_PORT_CLOSED_ERROR: { - message: t('Port is closed'), - description: t('Please verify that port is open to connect.'), - }, - CONNECTION_INVALID_PORT_ERROR: { - message: t('Invalid Port Number'), - description: t( - 'The port must be a whole number less than or equal to 65535.', - ), - }, - CONNECTION_ACCESS_DENIED_ERROR: { - message: t('Invalid account information'), - description: t('Either the username or password is incorrect.'), - }, - CONNECTION_INVALID_PASSWORD_ERROR: { - message: t('Invalid account information'), - description: t('Either the username or password is incorrect.'), - }, - INVALID_PAYLOAD_SCHEMA_ERROR: { - message: t('Incorrect Fields'), - description: t('Please make sure all fields are filled out correctly'), - }, - TABLE_DOES_NOT_EXIST_ERROR: { - message: t('URL could not be identified'), - description: t( - 'The URL could not be identified. Please check for typos and make sure that "Type of google sheet allowed" selection matches the input', - ), - }, -}; const googleSheetConnectionEngine = 'gsheets'; interface DatabaseModalProps { @@ -227,7 +184,7 @@ function dbReducer( }; let query = {}; let query_input = ''; - let deserializeExtraJSON = { allows_virtual_table_explore: true }; + let deserializeExtraJSON = {}; let extra_json: DatabaseObject['extra_json']; switch (action.type) { @@ -576,8 +533,8 @@ const DatabaseModal: FunctionComponent = ({ if (dbToUpdate.configuration_method === CONFIGURATION_METHOD.DYNAMIC_FORM) { // Validate DB before saving - await getValidation(dbToUpdate, true); - if (validationErrors && !isEmpty(validationErrors)) { + const errors = await getValidation(dbToUpdate, true); + if ((validationErrors && !isEmpty(validationErrors)) || errors) { return; } const parameters_schema = isEditMode @@ -679,7 +636,6 @@ const DatabaseModal: FunctionComponent = ({ passwords, confirmedOverwrite, ); - if (dbId) { onClose(); addSuccessToast(t('Database connected')); @@ -1112,44 +1068,21 @@ const DatabaseModal: FunctionComponent = ({ ); }; + // eslint-disable-next-line consistent-return const errorAlert = () => { - if ( - isEmpty(dbErrors) || - (isEmpty(validationErrors) && - !(validationErrors?.error_type in errorAlertMapping)) - ) { - return <>; - } - - if (validationErrors) { + if (isEmpty(dbErrors) === false) { + const message: Array = + typeof dbErrors === 'object' ? Object.values(dbErrors) : []; return ( antDErrorAlertStyles(theme)} - message={ - errorAlertMapping[validationErrors?.error_type]?.message || - validationErrors?.error_type - } - description={ - errorAlertMapping[validationErrors?.error_type]?.description || - validationErrors?.description || - JSON.stringify(validationErrors) - } - showIcon - closable={false} + message={t('Database Creation Error')} + description={message?.[0] || dbErrors} /> ); } - const message: Array = - typeof dbErrors === 'object' ? Object.values(dbErrors) : []; - return ( - antDErrorAlertStyles(theme)} - message={t('Database Creation Error')} - description={message?.[0] || dbErrors} - /> - ); + return <>; }; const renderFinishState = () => { diff --git a/superset-frontend/src/views/CRUD/hooks.ts b/superset-frontend/src/views/CRUD/hooks.ts index 5a0e26131efc0..ba544909cbead 100644 --- a/superset-frontend/src/views/CRUD/hooks.ts +++ b/superset-frontend/src/views/CRUD/hooks.ts @@ -777,6 +777,7 @@ export function useDatabaseValidation() { {}, ); setValidationErrors(parsedErrors); + return parsedErrors; }); } // eslint-disable-next-line no-console diff --git a/superset/db_engine_specs/gsheets.py b/superset/db_engine_specs/gsheets.py index 888513f518482..94c4cf424b0b3 100644 --- a/superset/db_engine_specs/gsheets.py +++ b/superset/db_engine_specs/gsheets.py @@ -216,7 +216,11 @@ def validate_parameters( except Exception: # pylint: disable=broad-except errors.append( SupersetError( - message="URL could not be identified", + message=( + "The URL could not be identified. Please check for typos " + "and make sure that ‘Type of Google Sheets allowed’ " + "selection matches the input." + ), error_type=SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR, level=ErrorLevel.WARNING, extra={"catalog": {"idx": idx, "url": True}}, diff --git a/tests/unit_tests/db_engine_specs/test_gsheets.py b/tests/unit_tests/db_engine_specs/test_gsheets.py index a13895e75e1d5..b050c6fdbf2ab 100644 --- a/tests/unit_tests/db_engine_specs/test_gsheets.py +++ b/tests/unit_tests/db_engine_specs/test_gsheets.py @@ -76,7 +76,11 @@ def test_validate_parameters_catalog( assert errors == [ SupersetError( - message="URL could not be identified", + message=( + "The URL could not be identified. Please check for typos " + "and make sure that ‘Type of Google Sheets allowed’ " + "selection matches the input." + ), error_type=SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR, level=ErrorLevel.WARNING, extra={ @@ -97,7 +101,11 @@ def test_validate_parameters_catalog( }, ), SupersetError( - message="URL could not be identified", + message=( + "The URL could not be identified. Please check for typos " + "and make sure that ‘Type of Google Sheets allowed’ " + "selection matches the input." + ), error_type=SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR, level=ErrorLevel.WARNING, extra={ @@ -158,7 +166,11 @@ def test_validate_parameters_catalog_and_credentials( errors = GSheetsEngineSpec.validate_parameters(parameters) # ignore: type assert errors == [ SupersetError( - message="URL could not be identified", + message=( + "The URL could not be identified. Please check for typos " + "and make sure that ‘Type of Google Sheets allowed’ " + "selection matches the input." + ), error_type=SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR, level=ErrorLevel.WARNING, extra={