From 982b96375b0c5150d40ddb620ac81c10426a452d Mon Sep 17 00:00:00 2001
From: Jesse Yang <jesse.yang@airbnb.com>
Date: Sun, 8 Nov 2020 23:17:27 -0800
Subject: [PATCH 1/5] feat: keep modal open when saving database failed

---
 .../CRUD/data/database/DatabaseModal.tsx      | 26 +++++++++++--------
 superset-frontend/src/views/CRUD/hooks.ts     |  3 ++-
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
index e384bc977306..96b52fa1693c 100644
--- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
+++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
@@ -202,22 +202,24 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
       }
 
       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();
       });
     }
   };
@@ -356,7 +358,9 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
                 type="text"
                 name="sqlalchemy_uri"
                 value={db ? db.sqlalchemy_uri : ''}
-                placeholder={t('SQLAlchemy URI')}
+                placeholder={t(
+                  'dialect+driver://username:password@host:port/database',
+                )}
                 onChange={onInputChange}
               />
               <Button buttonStyle="primary" onClick={testConnection} cta>
diff --git a/superset-frontend/src/views/CRUD/hooks.ts b/superset-frontend/src/views/CRUD/hooks.ts
index b6963d40326a..e04786ec342e 100644
--- a/superset-frontend/src/views/CRUD/hooks.ts
+++ b/superset-frontend/src/views/CRUD/hooks.ts
@@ -211,6 +211,7 @@ export function useSingleViewResource<D extends object = any>(
           updateState({
             resource: json.result,
           });
+          return json.result;
         },
         createErrorHandler(errMsg =>
           handleErrorMsg(
@@ -243,7 +244,6 @@ export function useSingleViewResource<D extends object = any>(
           updateState({
             resource: json.result,
           });
-
           return json.id;
         },
         createErrorHandler(errMsg =>
@@ -277,6 +277,7 @@ export function useSingleViewResource<D extends object = any>(
           updateState({
             resource: json.result,
           });
+          return json.result;
         },
         createErrorHandler(errMsg =>
           handleErrorMsg(

From 9600bd215390b72f2c7ff284b5c9c1d291bfb168 Mon Sep 17 00:00:00 2001
From: Jesse Yang <jesse.yang@airbnb.com>
Date: Tue, 10 Nov 2020 17:17:02 -0800
Subject: [PATCH 2/5] Dont throw error on invalid url

---
 superset/models/core.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/superset/models/core.py b/superset/models/core.py
index 952ba83bba41..c9e87b056f42 100755
--- a/superset/models/core.py
+++ b/superset/models/core.py
@@ -46,6 +46,7 @@
 from sqlalchemy.engine import Dialect, Engine, url
 from sqlalchemy.engine.reflection import Inspector
 from sqlalchemy.engine.url import make_url, URL
+from sqlalchemy.exc import ArgumentError
 from sqlalchemy.ext.hybrid import hybrid_property
 from sqlalchemy.orm import relationship
 from sqlalchemy.pool import NullPool
@@ -646,7 +647,11 @@ def get_schema_access_for_csv_upload(  # pylint: disable=invalid-name
 
     @property
     def sqlalchemy_uri_decrypted(self) -> str:
-        conn = sqla.engine.url.make_url(self.sqlalchemy_uri)
+        try:
+            conn = sqla.engine.url.make_url(self.sqlalchemy_uri)
+        except ArgumentError:
+            # if the URI is invalid, ignore and return a placeholder url
+            return "dialect://host"
         if custom_password_store:
             conn.password = custom_password_store(conn)
         else:

From b7f7243a3e7204e80011c738e5963dade3770ba4 Mon Sep 17 00:00:00 2001
From: Jesse Yang <jesse.yang@airbnb.com>
Date: Wed, 11 Nov 2020 09:49:00 -0800
Subject: [PATCH 3/5] Friendlier error message

---
 superset-frontend/src/utils/getClientErrorObject.ts      | 4 +++-
 .../src/views/CRUD/data/database/DatabaseModal.tsx       | 9 ++++++++-
 superset-frontend/src/views/CRUD/hooks.ts                | 2 +-
 superset/databases/schemas.py                            | 7 ++-----
 4 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/superset-frontend/src/utils/getClientErrorObject.ts b/superset-frontend/src/utils/getClientErrorObject.ts
index bd58d0136abe..6d892fdc64f4 100644
--- a/superset-frontend/src/utils/getClientErrorObject.ts
+++ b/superset-frontend/src/utils/getClientErrorObject.ts
@@ -29,7 +29,9 @@ export type ClientErrorObject = {
   error: string;
   errors?: SupersetError[];
   link?: string;
-  message?: string;
+  // marshmallow field validation returns the error mssage in the format
+  // of { field: [msg1, msg2] }
+  message?: string | Record<string, string[]>;
   severity?: string;
   stacktrace?: string;
 } & Partial<SupersetClientResponse>;
diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
index 96b52fa1693c..2a74552f3265 100644
--- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
+++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
@@ -175,7 +175,13 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
       .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.sqlalchemy_uri
+                }`
+              : t('ERROR: Connection failed. '),
           );
         }),
       );
@@ -358,6 +364,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
                 type="text"
                 name="sqlalchemy_uri"
                 value={db ? db.sqlalchemy_uri : ''}
+                autoComplete="off"
                 placeholder={t(
                   'dialect+driver://username:password@host:port/database',
                 )}
diff --git a/superset-frontend/src/views/CRUD/hooks.ts b/superset-frontend/src/views/CRUD/hooks.ts
index e04786ec342e..0eb37b8c870e 100644
--- a/superset-frontend/src/views/CRUD/hooks.ts
+++ b/superset-frontend/src/views/CRUD/hooks.ts
@@ -249,7 +249,7 @@ export function useSingleViewResource<D extends object = any>(
         createErrorHandler(errMsg =>
           handleErrorMsg(
             t(
-              'An error occurred while fetching %ss: %s',
+              'An error occurred while creating %ss: %s',
               resourceLabel,
               JSON.stringify(errMsg),
             ),
diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py
index 4bcca21531b4..5c3cc7481c3f 100644
--- a/superset/databases/schemas.py
+++ b/superset/databases/schemas.py
@@ -135,11 +135,8 @@ def sqlalchemy_uri_validator(value: str) -> str:
         raise ValidationError(
             [
                 _(
-                    "Invalid connection string, a valid string usually follows:"
-                    "'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
-                    "<p>"
-                    "Example:'postgresql://user:password@your-postgres-db/database'"
-                    "</p>"
+                    "Invalid connection string, a valid string usually follows: "
+                    "dirver://user:password@database-host/database-name"
                 )
             ]
         )

From ba231332507f153d20000d322bceed74736433ef Mon Sep 17 00:00:00 2001
From: Jesse Yang <jesse.yang@airbnb.com>
Date: Wed, 11 Nov 2020 11:29:14 -0800
Subject: [PATCH 4/5] Add cypress tests

---
 .../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      |  3 +-
 superset/databases/schemas.py                 |  2 +-
 superset/models/core.py                       |  5 +-
 8 files changed, 104 insertions(+), 6 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 000000000000..a339865e1fe6
--- /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 000000000000..0e999793bcaa
--- /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 0f997e353a93..4431fd32c85d 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 = ({
         </span>
       }
       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 6d892fdc64f4..274b8b4d93cf 100644
--- a/superset-frontend/src/utils/getClientErrorObject.ts
+++ b/superset-frontend/src/utils/getClientErrorObject.ts
@@ -31,7 +31,7 @@ export type ClientErrorObject = {
   link?: string;
   // marshmallow field validation returns the error mssage in the format
   // of { field: [msg1, msg2] }
-  message?: string | Record<string, string[]>;
+  message?: string;
   severity?: string;
   stacktrace?: string;
 } & Partial<SupersetClientResponse>;
diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx
index 80407899ce20..db9252c68ca8 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: (
           <>
             <i className="fa fa-plus" /> {t('Database')}{' '}
@@ -295,6 +296,7 @@ function DatabaseList({ addDangerToast, addSuccessToast }: DatabaseListProps) {
                 >
                   <span
                     role="button"
+                    data-test="database-edit"
                     tabIndex={0}
                     className="action-button"
                     onClick={handleEdit}
diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
index 2a74552f3265..1fa7f7bd13a0 100644
--- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
+++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
@@ -179,7 +179,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
               ? `${t('ERROR: ')}${
                   typeof error.message === 'string'
                     ? error.message
-                    : error.message.sqlalchemy_uri
+                    : (error.message as Record<string, string[]>).sqlalchemy_uri
                 }`
               : t('ERROR: Connection failed. '),
           );
@@ -315,6 +315,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
 
   return (
     <Modal
+      name="database"
       className="database-modal"
       disablePrimaryButton={disableSave}
       onHandledPrimaryAction={onSave}
diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py
index 5c3cc7481c3f..e0f7242a9fda 100644
--- a/superset/databases/schemas.py
+++ b/superset/databases/schemas.py
@@ -131,7 +131,7 @@ def sqlalchemy_uri_validator(value: str) -> str:
     """
     try:
         make_url(value.strip())
-    except (ArgumentError, AttributeError):
+    except (ArgumentError, AttributeError, ValueError):
         raise ValidationError(
             [
                 _(
diff --git a/superset/models/core.py b/superset/models/core.py
index c9e87b056f42..382239209f9e 100755
--- a/superset/models/core.py
+++ b/superset/models/core.py
@@ -649,9 +649,10 @@ def get_schema_access_for_csv_upload(  # pylint: disable=invalid-name
     def sqlalchemy_uri_decrypted(self) -> str:
         try:
             conn = sqla.engine.url.make_url(self.sqlalchemy_uri)
-        except ArgumentError:
+        except (ArgumentError, ValueError):
             # if the URI is invalid, ignore and return a placeholder url
-            return "dialect://host"
+            # (so users see 500 less often)
+            return "dialect://invalid_uri"
         if custom_password_store:
             conn.password = custom_password_store(conn)
         else:

From 779d5580d00f2da23dc99f828bcb896162007aa7 Mon Sep 17 00:00:00 2001
From: Jesse Yang <jesse.yang@airbnb.com>
Date: Wed, 11 Nov 2020 12:03:05 -0800
Subject: [PATCH 5/5] Fix pytest

---
 tests/databases/api_tests.py | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/tests/databases/api_tests.py b/tests/databases/api_tests.py
index 62a14005932b..4cfe14aee1b9 100644
--- a/tests/databases/api_tests.py
+++ b/tests/databases/api_tests.py
@@ -297,17 +297,9 @@ def test_create_database_uri_validate(self):
         rv = self.client.post(uri, json=database_data)
         response = json.loads(rv.data.decode("utf-8"))
         self.assertEqual(rv.status_code, 400)
-        expected_response = {
-            "message": {
-                "sqlalchemy_uri": [
-                    "Invalid connection string, a valid string usually "
-                    "follows:'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
-                    "<p>Example:'postgresql://user:password@your-postgres-db/database'"
-                    "</p>"
-                ]
-            }
-        }
-        self.assertEqual(response, expected_response)
+        self.assertIn(
+            "Invalid connection string", response["message"]["sqlalchemy_uri"][0],
+        )
 
     def test_create_database_fail_sqllite(self):
         """
@@ -456,17 +448,9 @@ def test_update_database_uri_validate(self):
         rv = self.client.put(uri, json=database_data)
         response = json.loads(rv.data.decode("utf-8"))
         self.assertEqual(rv.status_code, 400)
-        expected_response = {
-            "message": {
-                "sqlalchemy_uri": [
-                    "Invalid connection string, a valid string usually "
-                    "follows:'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
-                    "<p>Example:'postgresql://user:password@your-postgres-db/database'"
-                    "</p>"
-                ]
-            }
-        }
-        self.assertEqual(response, expected_response)
+        self.assertIn(
+            "Invalid connection string", response["message"]["sqlalchemy_uri"][0],
+        )
 
     def test_delete_database(self):
         """