From d862d54d0688f3526d1eec5f84c871b360e33623 Mon Sep 17 00:00:00 2001 From: Phillip Kelley-Dotson Date: Fri, 29 Apr 2022 08:48:22 -0700 Subject: [PATCH 1/7] fix: always create parameter json field --- .../src/views/CRUD/data/database/DatabaseModal/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 a6e93f865327..b32b04b63c94 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx @@ -246,7 +246,7 @@ function dbReducer( }; case ActionType.parametersChange: if ( - trimmedState.catalog !== undefined && + trimmedState.catalog && action.payload.type?.startsWith('catalog') ) { // Formatting wrapping google sheets table catalog From 5af1f651d54451a9d40e57ffc06ee1630f274e8a Mon Sep 17 00:00:00 2001 From: Phillip Kelley-Dotson Date: Fri, 29 Apr 2022 14:47:12 -0700 Subject: [PATCH 2/7] ensure validation for empty catalog --- .../views/CRUD/data/database/DatabaseModal/index.tsx | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) 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 b32b04b63c94..3043f1e54872 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx @@ -246,7 +246,7 @@ function dbReducer( }; case ActionType.parametersChange: if ( - trimmedState.catalog && + trimmedState.catalog !== undefined && action.payload.type?.startsWith('catalog') ) { // Formatting wrapping google sheets table catalog @@ -532,6 +532,15 @@ const DatabaseModal: FunctionComponent = ({ const dbToUpdate = JSON.parse(JSON.stringify(update)); if (dbToUpdate.configuration_method === CONFIGURATION_METHOD.DYNAMIC_FORM) { + if ( + !dbToUpdate?.parameters?.catalog && + dbToUpdate.database_name === 'Google Sheets' + ) { + // send empty catalog if there is empty fields for name or url + // since the database could parse it as valid + dbToUpdate.parameters = { catalog: { name: '', url: '' } }; + } + // Validate DB before saving const errors = await getValidation(dbToUpdate, true); if ((validationErrors && !isEmpty(validationErrors)) || errors) { From e1cc040c8db267225695f4368109d54269fe6bcd Mon Sep 17 00:00:00 2001 From: Phillip Kelley-Dotson Date: Wed, 11 May 2022 14:34:23 -0700 Subject: [PATCH 3/7] check engine instead of name --- .../src/views/CRUD/data/database/DatabaseModal/index.tsx | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) 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 3043f1e54872..259b86c45771 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx @@ -532,10 +532,7 @@ const DatabaseModal: FunctionComponent = ({ const dbToUpdate = JSON.parse(JSON.stringify(update)); if (dbToUpdate.configuration_method === CONFIGURATION_METHOD.DYNAMIC_FORM) { - if ( - !dbToUpdate?.parameters?.catalog && - dbToUpdate.database_name === 'Google Sheets' - ) { + if (!dbToUpdate?.parameters?.catalog && dbToUpdate.engine === 'gsheets') { // send empty catalog if there is empty fields for name or url // since the database could parse it as valid dbToUpdate.parameters = { catalog: { name: '', url: '' } }; From 9f9ae7a3edc4cb8855b14e183ddf5eb97a097110 Mon Sep 17 00:00:00 2001 From: Phillip Kelley-Dotson Date: Mon, 23 May 2022 17:14:03 -0700 Subject: [PATCH 4/7] put validation in be --- .../src/views/CRUD/data/database/DatabaseModal/index.tsx | 6 ------ superset/db_engine_specs/gsheets.py | 8 ++++++++ 2 files changed, 8 insertions(+), 6 deletions(-) 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 259b86c45771..a6e93f865327 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx @@ -532,12 +532,6 @@ const DatabaseModal: FunctionComponent = ({ const dbToUpdate = JSON.parse(JSON.stringify(update)); if (dbToUpdate.configuration_method === CONFIGURATION_METHOD.DYNAMIC_FORM) { - if (!dbToUpdate?.parameters?.catalog && dbToUpdate.engine === 'gsheets') { - // send empty catalog if there is empty fields for name or url - // since the database could parse it as valid - dbToUpdate.parameters = { catalog: { name: '', url: '' } }; - } - // Validate DB before saving const errors = await getValidation(dbToUpdate, true); if ((validationErrors && !isEmpty(validationErrors)) || errors) { diff --git a/superset/db_engine_specs/gsheets.py b/superset/db_engine_specs/gsheets.py index 94c4cf424b0b..4a7264c103e7 100644 --- a/superset/db_engine_specs/gsheets.py +++ b/superset/db_engine_specs/gsheets.py @@ -171,6 +171,14 @@ def validate_parameters( if not table_catalog: # Allowing users to submit empty catalogs + errors.append( + SupersetError( + message="Sheet name is required", + error_type=SupersetErrorType.CONNECTION_MISSING_PARAMETERS_ERROR, + level=ErrorLevel.WARNING, + extra={"catalog": {"idx": 0, "name": True}}, + ), + ) return errors # We need a subject in case domain wide delegation is set, otherwise the From 6b0319f8f5bd8f3deca6e0a48802f07a19eb7612 Mon Sep 17 00:00:00 2001 From: Phillip Kelley-Dotson Date: Mon, 23 May 2022 18:36:04 -0700 Subject: [PATCH 5/7] fix test --- tests/unit_tests/db_engine_specs/test_gsheets.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/unit_tests/db_engine_specs/test_gsheets.py b/tests/unit_tests/db_engine_specs/test_gsheets.py index b050c6fdbf2a..c2e8346c3c7a 100644 --- a/tests/unit_tests/db_engine_specs/test_gsheets.py +++ b/tests/unit_tests/db_engine_specs/test_gsheets.py @@ -40,7 +40,14 @@ def test_validate_parameters_simple( "catalog": {}, } errors = GSheetsEngineSpec.validate_parameters(parameters) - assert errors == [] + assert errors == [ + SupersetError( + message="Sheet name is required", + error_type=SupersetErrorType.CONNECTION_MISSING_PARAMETERS_ERROR, + level=ErrorLevel.WARNING, + extra={"catalog": {"idx": 0, "name": True}}, + ), + ] def test_validate_parameters_catalog( From 123f07c135d3ed2f4273a5bdc2effb7f7e6d061f Mon Sep 17 00:00:00 2001 From: Phillip Kelley-Dotson Date: Tue, 24 May 2022 13:56:48 -0700 Subject: [PATCH 6/7] fix test --- .../cypress/integration/alerts_and_reports/alerts.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/superset-frontend/cypress-base/cypress/integration/alerts_and_reports/alerts.test.ts b/superset-frontend/cypress-base/cypress/integration/alerts_and_reports/alerts.test.ts index bc27e831b5af..6f29b616827f 100644 --- a/superset-frontend/cypress-base/cypress/integration/alerts_and_reports/alerts.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/alerts_and_reports/alerts.test.ts @@ -38,7 +38,8 @@ describe('alert list view', () => { cy.get('[data-test="sort-header"]').eq(4).contains('Notification method'); cy.get('[data-test="sort-header"]').eq(5).contains('Created by'); cy.get('[data-test="sort-header"]').eq(6).contains('Owners'); - cy.get('[data-test="sort-header"]').eq(7).contains('Active'); - cy.get('[data-test="sort-header"]').eq(8).contains('Actions'); + cy.get('[data-test="sort-header"]').eq(7).contains('Modified'); + cy.get('[data-test="sort-header"]').eq(8).contains('Active'); + cy.get('[data-test="sort-header"]').eq(9).contains('Actions'); }); }); From 541337535538d17ab4e83aedf2d66fd26bcb1274 Mon Sep 17 00:00:00 2001 From: Phillip Kelley-Dotson Date: Tue, 24 May 2022 13:57:58 -0700 Subject: [PATCH 7/7] remove test --- .../cypress/integration/alerts_and_reports/alerts.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/superset-frontend/cypress-base/cypress/integration/alerts_and_reports/alerts.test.ts b/superset-frontend/cypress-base/cypress/integration/alerts_and_reports/alerts.test.ts index 6f29b616827f..bc27e831b5af 100644 --- a/superset-frontend/cypress-base/cypress/integration/alerts_and_reports/alerts.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/alerts_and_reports/alerts.test.ts @@ -38,8 +38,7 @@ describe('alert list view', () => { cy.get('[data-test="sort-header"]').eq(4).contains('Notification method'); cy.get('[data-test="sort-header"]').eq(5).contains('Created by'); cy.get('[data-test="sort-header"]').eq(6).contains('Owners'); - cy.get('[data-test="sort-header"]').eq(7).contains('Modified'); - cy.get('[data-test="sort-header"]').eq(8).contains('Active'); - cy.get('[data-test="sort-header"]').eq(9).contains('Actions'); + cy.get('[data-test="sort-header"]').eq(7).contains('Active'); + cy.get('[data-test="sort-header"]').eq(8).contains('Actions'); }); });