From ec8ccd4cf14f20b86b9a088d6ffce00aad6f4be0 Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Thu, 12 Nov 2020 16:10:14 -0800 Subject: [PATCH] feat: keep modal open when saving database failed (#11618) --- .../cypress/integration/database/helper.ts | 19 +++++ .../integration/database/modal.test.ts | 73 +++++++++++++++++++ .../src/common/components/Modal/Modal.tsx | 4 +- .../src/utils/getClientErrorObject.ts | 2 + .../views/CRUD/data/database/DatabaseList.tsx | 2 + .../CRUD/data/database/DatabaseModal.tsx | 36 ++++++--- superset-frontend/src/views/CRUD/hooks.ts | 5 +- superset/databases/schemas.py | 9 +-- superset/models/core.py | 8 +- tests/databases/api_tests.py | 28 ++----- 10 files changed, 142 insertions(+), 44 deletions(-) create mode 100644 superset-frontend/cypress-base/cypress/integration/database/helper.ts create mode 100644 superset-frontend/cypress-base/cypress/integration/database/modal.test.ts diff --git a/superset-frontend/cypress-base/cypress/integration/database/helper.ts b/superset-frontend/cypress-base/cypress/integration/database/helper.ts new file mode 100644 index 0000000000..a339865e1f --- /dev/null +++ b/superset-frontend/cypress-base/cypress/integration/database/helper.ts @@ -0,0 +1,19 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +export const DATABASE_LIST = '/databaseview/list'; diff --git a/superset-frontend/cypress-base/cypress/integration/database/modal.test.ts b/superset-frontend/cypress-base/cypress/integration/database/modal.test.ts new file mode 100644 index 0000000000..0e999793bc --- /dev/null +++ b/superset-frontend/cypress-base/cypress/integration/database/modal.test.ts @@ -0,0 +1,73 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { DATABASE_LIST } from './helper'; + +describe('Add database', () => { + beforeEach(() => { + cy.server(); + cy.login(); + }); + + it('should keep create modal open when error', () => { + cy.visit(DATABASE_LIST); + + // open modal + cy.get('[data-test="btn-create-database"]').click(); + + // type values + cy.get('[data-test="database-modal"] input[name="database_name"]') + .focus() + .type('cypress'); + cy.get('[data-test="database-modal"] input[name="sqlalchemy_uri"]') + .focus() + .type('bad_db_uri'); + + // click save + cy.get('[data-test="modal-confirm-button"]:not(:disabled)').click(); + + // should show error alerts and keep modal open + cy.get('.toast').contains('error'); + cy.wait(1000); // wait for potential (incorrect) closing annimation + cy.get('[data-test="database-modal"]').should('be.visible'); + + // should be able to close modal + cy.get('[data-test="modal-cancel-button"]').click(); + cy.get('[data-test="database-modal"]').should('not.be.visible'); + }); + + it('should keep update modal open when error', () => { + // open modal + cy.get('[data-test="database-edit"]:last').click(); + + cy.get('[data-test="database-modal"]:last input[name="sqlalchemy_uri"]') + .focus() + .dblclick() + .type('{selectall}{backspace}bad_uri'); + + // click save + cy.get('[data-test="modal-confirm-button"]:not(:disabled)').click(); + + // should show error alerts + cy.get('.toast').contains('error').should('be.visible'); + + // modal should still be open + cy.wait(1000); // wait for potential (incorrect) closing annimation + cy.get('[data-test="database-modal"]').should('be.visible'); + }); +}); diff --git a/superset-frontend/src/common/components/Modal/Modal.tsx b/superset-frontend/src/common/components/Modal/Modal.tsx index 0f997e353a..4431fd32c8 100644 --- a/superset-frontend/src/common/components/Modal/Modal.tsx +++ b/superset-frontend/src/common/components/Modal/Modal.tsx @@ -32,6 +32,7 @@ interface ModalProps { primaryButtonName?: string; primaryButtonType?: 'primary' | 'danger'; show: boolean; + name?: string; title: React.ReactNode; width?: string; maxWidth?: string; @@ -114,6 +115,7 @@ const CustomModal = ({ primaryButtonName = t('OK'), primaryButtonType = 'primary', show, + name, title, width, maxWidth, @@ -159,7 +161,7 @@ const CustomModal = ({ } footer={!hideFooter ? modalFooter : null} - wrapProps={{ 'data-test': `${title}-modal`, ...wrapProps }} + wrapProps={{ 'data-test': `${name || title}-modal`, ...wrapProps }} {...rest} > {children} diff --git a/superset-frontend/src/utils/getClientErrorObject.ts b/superset-frontend/src/utils/getClientErrorObject.ts index bd58d0136a..274b8b4d93 100644 --- a/superset-frontend/src/utils/getClientErrorObject.ts +++ b/superset-frontend/src/utils/getClientErrorObject.ts @@ -29,6 +29,8 @@ export type ClientErrorObject = { error: string; errors?: SupersetError[]; link?: string; + // marshmallow field validation returns the error mssage in the format + // of { field: [msg1, msg2] } message?: string; severity?: string; stacktrace?: string; diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx index 80407899ce..db9252c68c 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx @@ -132,6 +132,7 @@ function DatabaseList({ addDangerToast, addSuccessToast }: DatabaseListProps) { if (canCreate) { menuData.buttons = [ { + 'data-test': 'btn-create-database', name: ( <> {t('Database')}{' '} @@ -295,6 +296,7 @@ function DatabaseList({ addDangerToast, addSuccessToast }: DatabaseListProps) { > = ({ .catch(response => getClientErrorObject(response).then(error => { addDangerToast( - t('ERROR: Connection failed. ') + error?.message || '', + error?.message + ? `${t('ERROR: ')}${ + typeof error.message === 'string' + ? error.message + : (error.message as Record).sqlalchemy_uri + }` + : t('ERROR: Connection failed. '), ); }), ); @@ -202,22 +208,24 @@ const DatabaseModal: FunctionComponent = ({ } if (db && db.id) { - updateResource(db.id, update).then(() => { - if (onDatabaseAdd) { - onDatabaseAdd(); + updateResource(db.id, update).then(result => { + if (result) { + if (onDatabaseAdd) { + onDatabaseAdd(); + } + hide(); } - - hide(); }); } } else if (db) { // Create - createResource(db).then(() => { - if (onDatabaseAdd) { - onDatabaseAdd(); + createResource(db).then(dbId => { + if (dbId) { + if (onDatabaseAdd) { + onDatabaseAdd(); + } + hide(); } - - hide(); }); } }; @@ -307,6 +315,7 @@ const DatabaseModal: FunctionComponent = ({ return ( = ({ type="text" name="sqlalchemy_uri" value={db ? db.sqlalchemy_uri : ''} - placeholder={t('SQLAlchemy URI')} + autoComplete="off" + placeholder={t( + 'dialect+driver://username:password@host:port/database', + )} onChange={onInputChange} />