From e7640724877caf356c114659f1d09e5e2691cf9f Mon Sep 17 00:00:00 2001
From: Elizabeth Thompson
Date: Fri, 21 May 2021 10:57:08 -0700
Subject: [PATCH] use new validation component
---
.../integration/database/modal.test.ts | 24 ++
.../src/components/Form/FormItem.tsx | 4 +
.../Form/LabeledErrorBoundInput.tsx | 18 +-
.../DatabaseModal/DatabaseConnectionForm.tsx | 240 +++++++++++-------
.../database/DatabaseModal/index.test.jsx | 2 +-
.../data/database/DatabaseModal/index.tsx | 33 +--
.../data/database/DatabaseModal/styles.ts | 33 +--
.../src/views/CRUD/data/database/types.ts | 3 +-
superset-frontend/src/views/CRUD/hooks.ts | 49 ++++
superset/databases/commands/validate.py | 6 +-
superset/databases/schemas.py | 6 +
superset/db_engine_specs/base.py | 6 +-
12 files changed, 284 insertions(+), 140 deletions(-)
diff --git a/superset-frontend/cypress-base/cypress/integration/database/modal.test.ts b/superset-frontend/cypress-base/cypress/integration/database/modal.test.ts
index e29ec48a9479..cf27d219324e 100644
--- a/superset-frontend/cypress-base/cypress/integration/database/modal.test.ts
+++ b/superset-frontend/cypress-base/cypress/integration/database/modal.test.ts
@@ -88,4 +88,28 @@ describe('Add database', () => {
// cy.wait(1000); // wait for potential (incorrect) closing annimation
// cy.get('[data-test="database-modal"]').should('be.visible');
});
+
+ it('should close modal after save', () => {
+ 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('gsheets://');
+
+ // 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');
+ });
});
diff --git a/superset-frontend/src/components/Form/FormItem.tsx b/superset-frontend/src/components/Form/FormItem.tsx
index ab301a883e54..2731a215faf1 100644
--- a/superset-frontend/src/components/Form/FormItem.tsx
+++ b/superset-frontend/src/components/Form/FormItem.tsx
@@ -41,6 +41,10 @@ const StyledItem = styled(Form.Item)`
}
}
}
+ .ant-form-item-explain {
+ color: ${theme.colors.grayscale.light1};
+ font-size: ${theme.typography.sizes.s - 1}px;
+ }
`}
`;
diff --git a/superset-frontend/src/components/Form/LabeledErrorBoundInput.tsx b/superset-frontend/src/components/Form/LabeledErrorBoundInput.tsx
index 8569b554a020..75df2bb088cb 100644
--- a/superset-frontend/src/components/Form/LabeledErrorBoundInput.tsx
+++ b/superset-frontend/src/components/Form/LabeledErrorBoundInput.tsx
@@ -25,17 +25,18 @@ import FormLabel from './FormLabel';
export interface LabeledErrorBoundInputProps {
label?: string;
validationMethods:
- | { onBlur: (value: any) => string }
- | { onChange: (value: any) => string };
+ | { onBlur: (value: any) => void }
+ | { onChange: (value: any) => void };
errorMessage: string | null;
helpText?: string;
required?: boolean;
id?: string;
+ classname?: string;
[x: string]: any;
}
const StyledInput = styled(Input)`
- margin: 8px 0;
+ margin: ${({ theme }) => `${theme.gridUnit}px 0 ${theme.gridUnit * 2}px`};
`;
const alertIconStyles = (theme: SupersetTheme, hasError: boolean) => css`
@@ -60,6 +61,12 @@ const alertIconStyles = (theme: SupersetTheme, hasError: boolean) => css`
}
}`}
`;
+const StyledFormGroup = styled('div')`
+ margin-bottom: ${({ theme }) => theme.gridUnit * 5}px;
+ .ant-form-item {
+ margin-bottom: 0;
+ }
+`;
const LabeledErrorBoundInput = ({
label,
@@ -68,9 +75,10 @@ const LabeledErrorBoundInput = ({
helpText,
required = false,
id,
+ className,
...props
}: LabeledErrorBoundInputProps) => (
- <>
+
{label}
@@ -83,7 +91,7 @@ const LabeledErrorBoundInput = ({
>
- >
+
);
export default LabeledErrorBoundInput;
diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
index f0542e481c12..5c7c729da589 100644
--- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
+++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
@@ -17,11 +17,14 @@
* under the License.
*/
import React, { FormEvent } from 'react';
-import cx from 'classnames';
+import { SupersetTheme, JsonObject } from '@superset-ui/core';
import { InputProps } from 'antd/lib/input';
-import { FormLabel, FormItem } from 'src/components/Form';
-import { Input } from 'src/common/components';
-import { StyledFormHeader, formScrollableStyles } from './styles';
+import ValidatedInput from 'src/components/Form/LabeledErrorBoundInput';
+import {
+ StyledFormHeader,
+ formScrollableStyles,
+ validatedFormStyles,
+} from './styles';
import { DatabaseForm } from '../types';
export const FormFieldOrder = [
@@ -33,64 +36,137 @@ export const FormFieldOrder = [
'database_name',
];
-const CHANGE_METHOD = {
- onChange: 'onChange',
- onPropertiesChange: 'onPropertiesChange',
-};
+interface FieldPropTypes {
+ required: boolean;
+ changeMethods: { onParametersChange: (value: any) => string } & {
+ onChange: (value: any) => string;
+ };
+ validationErrors: JsonObject | null;
+ getValidation: () => void;
+}
+
+const hostField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => (
+
+);
+const portField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => (
+
+);
+const databaseField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => (
+
+);
+const usernameField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => (
+
+);
+const passwordField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => (
+
+);
+const displayField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => (
+
+);
const FORM_FIELD_MAP = {
- host: {
- description: 'Host',
- type: 'text',
- className: 'w-50',
- placeholder: 'e.g. 127.0.0.1',
- changeMethod: CHANGE_METHOD.onPropertiesChange,
- },
- port: {
- description: 'Port',
- type: 'text',
- className: 'w-50',
- placeholder: 'e.g. 5432',
- changeMethod: CHANGE_METHOD.onPropertiesChange,
- },
- database: {
- description: 'Database name',
- type: 'text',
- label:
- 'Copy the name of the PostgreSQL database you are trying to connect to.',
- placeholder: 'e.g. world_population',
- changeMethod: CHANGE_METHOD.onPropertiesChange,
- },
- username: {
- description: 'Username',
- type: 'text',
- placeholder: 'e.g. Analytics',
- changeMethod: CHANGE_METHOD.onPropertiesChange,
- },
- password: {
- description: 'Password',
- type: 'text',
- placeholder: 'e.g. ********',
- changeMethod: CHANGE_METHOD.onPropertiesChange,
- },
- database_name: {
- description: 'Display Name',
- type: 'text',
- label: 'Pick a nickname for this database to display as in Superset.',
- changeMethod: CHANGE_METHOD.onChange,
- },
- query: {
- additionalProperties: {},
- description: 'Additional parameters',
- type: 'object',
- changeMethod: CHANGE_METHOD.onPropertiesChange,
- },
+ host: hostField,
+ port: portField,
+ database: databaseField,
+ username: usernameField,
+ password: passwordField,
+ database_name: displayField,
};
const DatabaseConnectionForm = ({
dbModel: { name, parameters },
onParametersChange,
onChange,
+ validationErrors,
+ getValidation,
}: {
dbModel: DatabaseForm;
onParametersChange: (
@@ -99,6 +175,8 @@ const DatabaseConnectionForm = ({
onChange: (
event: FormEvent | { target: HTMLInputElement },
) => void;
+ validationErrors: JsonObject | null;
+ getValidation: () => void;
}) => (
<>
@@ -107,52 +185,30 @@ const DatabaseConnectionForm = ({
Need help? Learn more about connecting to {name}.
-
+
[
+ formScrollableStyles,
+ validatedFormStyles(theme),
+ ]}
+ >
{parameters &&
FormFieldOrder.filter(
(key: string) =>
Object.keys(parameters.properties).includes(key) ||
key === 'database_name',
- ).map(field => {
- const {
- className,
- description,
- type,
- placeholder,
- label,
- changeMethod,
- } = FORM_FIELD_MAP[field];
- const onEdit =
- changeMethod === CHANGE_METHOD.onChange
- ? onChange
- : onParametersChange;
- return (
-
-
- {description}
-
-
- {label}
-
- );
- })}
+ ).map(field =>
+ FORM_FIELD_MAP[field]({
+ required: parameters.required.includes(field),
+ changeMethods: { onParametersChange, onChange },
+ validationErrors,
+ getValidation,
+ key: field,
+ }),
+ )}
>
);
-
export const FormFieldMap = FORM_FIELD_MAP;
export default DatabaseConnectionForm;
diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.jsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.jsx
index 86967257dd26..3f10914cda0a 100644
--- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.jsx
+++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.jsx
@@ -336,7 +336,7 @@ describe('DatabaseModal', () => {
await screen.findByText(/display name/i);
// it does not fetch any databases if no id is passed in
- expect(fetchMock.calls().length).toEqual(0);
+ expect(fetchMock.calls(DATABASE_FETCH_ENDPOINT).length).toEqual(0);
// todo we haven't hooked this up to load dynamically yet so
// we can't currently test it
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 57104b816dfb..265a8561923c 100644
--- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
+++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
@@ -33,6 +33,7 @@ import {
testDatabaseConnection,
useSingleViewResource,
useAvailableDatabases,
+ useDatabaseValidation,
} from 'src/views/CRUD/hooks';
import { useCommonConf } from 'src/views/CRUD/data/database/state';
import {
@@ -50,10 +51,9 @@ import {
antDModalStyles,
antDTabsStyles,
buttonLinkStyles,
- CreateHeader,
+ TabHeader,
CreateHeaderSubtitle,
CreateHeaderTitle,
- EditHeader,
EditHeaderSubtitle,
EditHeaderTitle,
formHelperStyles,
@@ -109,7 +109,7 @@ type DBReducerActionType =
| {
type: ActionType.dbSelected;
payload: {
- parameters: { engine?: string };
+ engine?: string;
configuration_method: CONFIGURATION_METHOD;
};
}
@@ -127,8 +127,6 @@ function dbReducer(
): Partial
| null {
const trimmedState = {
...(state || {}),
- database_name: state?.database_name?.trim() || '',
- sqlalchemy_uri: state?.sqlalchemy_uri || '',
};
switch (action.type) {
@@ -163,9 +161,7 @@ function dbReducer(
};
case ActionType.fetched:
return {
- parameters: {
- engine: trimmedState.parameters?.engine,
- },
+ engine: trimmedState.engine,
configuration_method: trimmedState.configuration_method,
...action.payload,
};
@@ -196,13 +192,16 @@ const DatabaseModal: FunctionComponent = ({
>(dbReducer, null);
const [tabKey, setTabKey] = useState(DEFAULT_TAB_KEY);
const [availableDbs, getAvailableDbs] = useAvailableDatabases();
+ const [validationErrors, getValidation] = useDatabaseValidation();
const [hasConnectedDb, setHasConnectedDb] = useState(false);
+ const [dbName, setDbName] = useState('');
const conf = useCommonConf();
const isEditMode = !!databaseId;
const useSqlAlchemyForm =
db?.configuration_method === CONFIGURATION_METHOD.SQLALCHEMY_URI;
const useTabLayout = isEditMode || useSqlAlchemyForm;
+
// Database fetch logic
const {
state: { loading: dbLoading, resource: dbFetched },
@@ -302,7 +301,6 @@ const DatabaseModal: FunctionComponent = ({
setDB({
type: ActionType.dbSelected,
payload: {
- parameters: {},
configuration_method: CONFIGURATION_METHOD.SQLALCHEMY_URI,
}, // todo hook this up to step 1
});
@@ -318,6 +316,9 @@ const DatabaseModal: FunctionComponent = ({
type: ActionType.fetched,
payload: dbFetched,
});
+ // keep a copy of the name separate for display purposes
+ // because it shouldn't change when the form is updated
+ setDbName(dbFetched.database_name);
}
}, [dbFetched]);
@@ -328,7 +329,7 @@ const DatabaseModal: FunctionComponent = ({
const dbModel: DatabaseForm =
availableDbs?.databases?.find(
(available: { engine: string | undefined }) =>
- available.engine === db?.parameters?.engine,
+ available.engine === db?.engine,
) || {};
const disableSave =
@@ -364,12 +365,12 @@ const DatabaseModal: FunctionComponent = ({
}
>
{isEditMode ? (
-
+
{db?.backend}
- {db?.database_name}
-
+ {dbName}
+
) : (
-
+
Enter Primary Credentials
Need help? Learn how to connect your database{' '}
@@ -382,7 +383,7 @@ const DatabaseModal: FunctionComponent = ({
.
-
+
)}
= ({
value: target.value,
})
}
+ getValidation={() => getValidation(db)}
+ validationErrors={validationErrors}
/>