From e8cd4f4cab29ab7dfc9706c0dba6920f5115a513 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Wed, 21 Apr 2021 12:18:55 -0700 Subject: [PATCH 1/3] split db modal file --- .../src/views/CRUD/data/database/DatabaseList.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx index d1105e706222..fda65baa2703 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx @@ -29,8 +29,9 @@ import { Tooltip } from 'src/components/Tooltip'; import Icons from 'src/components/Icons'; import ListView, { FilterOperator, Filters } from 'src/components/ListView'; import { commonMenuData } from 'src/views/CRUD/data/common'; -import DatabaseModal from 'src/views/CRUD/data/database/DatabaseModal'; import ImportModelsModal from 'src/components/ImportModal/index'; +import DatabaseModal from './DatabaseModal'; + import { DatabaseObject } from './types'; const PAGE_SIZE = 25; From 95917e40eb6d833ef783a59f80734b539cf4e5da Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Tue, 27 Apr 2021 17:08:49 -0700 Subject: [PATCH 2/3] hook up available databases --- .../database/DatabaseModal/index.test.jsx | 67 +++++++++++++++++++ .../data/database/DatabaseModal/index.tsx | 16 +++-- .../data/database/DatabaseModal/styles.ts | 4 -- 3 files changed, 76 insertions(+), 11 deletions(-) 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 1fc51e45a584..86967257dd26 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 @@ -319,5 +319,72 @@ describe('DatabaseModal', () => { const todoText = screen.getAllByText(/todo/i); expect(todoText[0]).toBeVisible(); }); + + describe('create database', () => { + it('should show a form when dynamic_form is selected', async () => { + const props = { + ...dbProps, + databaseId: null, + database_name: null, + sqlalchemy_uri: null, + }; + render(, { useRedux: true }); + // it should have the correct header text + const headerText = screen.getByText(/connect a database/i); + expect(headerText).toBeVisible(); + + await screen.findByText(/display name/i); + + // it does not fetch any databases if no id is passed in + expect(fetchMock.calls().length).toEqual(0); + + // todo we haven't hooked this up to load dynamically yet so + // we can't currently test it + }); + }); + + describe('edit database', () => { + it('renders the sqlalchemy form when the sqlalchemy_form configuration method is set', async () => { + render(, { useRedux: true }); + + // it should have tabs + const tabs = screen.getAllByRole('tab'); + expect(tabs.length).toEqual(2); + expect(tabs[0]).toHaveTextContent('Basic'); + expect(tabs[1]).toHaveTextContent('Advanced'); + + // it should have the correct header text + const headerText = screen.getByText(/edit database/i); + expect(headerText).toBeVisible(); + + // todo add more when this form is built out + }); + it('renders the dynamic form when the dynamic_form configuration method is set', async () => { + fetchMock.get(DATABASE_FETCH_ENDPOINT, { + result: { + id: 10, + database_name: 'my database', + expose_in_sqllab: false, + allow_ctas: false, + allow_cvas: false, + configuration_method: 'dynamic_form', + parameters: { + database: 'mydatabase', + }, + }, + }); + render(, { useRedux: true }); + + await screen.findByText(/todo/i); + + // // it should have tabs + const tabs = screen.getAllByRole('tab'); + expect(tabs.length).toEqual(2); + + // it should show a TODO for now + const todoText = screen.getAllByText(/todo/i); + expect(todoText[0]).toBeVisible(); + }); + }); }); }); 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 545cba00a7b1..57104b816dfb 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx @@ -248,14 +248,16 @@ const DatabaseModal: FunctionComponent = ({ // don't pass parameters if using the sqlalchemy uri delete update.parameters; } - updateResource(db.id as number, update as DatabaseObject).then(result => { - if (result) { - if (onDatabaseAdd) { - onDatabaseAdd(); - } - onClose(); + const result = await updateResource( + db.id as number, + update as DatabaseObject, + ); + if (result) { + if (onDatabaseAdd) { + onDatabaseAdd(); } - }); + onClose(); + } } else if (db) { // Create const dbId = await createResource(update as DatabaseObject); diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts index 8c33756962b0..4ae562996080 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts @@ -181,10 +181,6 @@ export const formStyles = (theme: SupersetTheme) => css` font-size: ${theme.typography.sizes.s - 1}px; margin-top: ${theme.gridUnit * 1.5}px; } - .ant-modal-body { - padding-top: 0; - margin-bottom: 0; - } .ant-tabs-content-holder { overflow: auto; max-height: 475px; From 7cb409886ec51aa8441dbe7d57c9273c1b971947 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 21 May 2021 10:57:08 -0700 Subject: [PATCH 3/3] use new validation component --- .../Form/LabeledErrorBoundInput.tsx | 18 +- .../DatabaseModal/DatabaseConnectionForm.tsx | 240 +++++++++++------- .../database/DatabaseModal/index.test.jsx | 67 ----- .../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 | 57 +++++ superset/databases/commands/validate.py | 6 +- superset/databases/schemas.py | 6 + superset/db_engine_specs/base.py | 6 +- 10 files changed, 263 insertions(+), 206 deletions(-) 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..1fc51e45a584 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 @@ -319,72 +319,5 @@ describe('DatabaseModal', () => { const todoText = screen.getAllByText(/todo/i); expect(todoText[0]).toBeVisible(); }); - - describe('create database', () => { - it('should show a form when dynamic_form is selected', async () => { - const props = { - ...dbProps, - databaseId: null, - database_name: null, - sqlalchemy_uri: null, - }; - render(, { useRedux: true }); - // it should have the correct header text - const headerText = screen.getByText(/connect a database/i); - expect(headerText).toBeVisible(); - - await screen.findByText(/display name/i); - - // it does not fetch any databases if no id is passed in - expect(fetchMock.calls().length).toEqual(0); - - // todo we haven't hooked this up to load dynamically yet so - // we can't currently test it - }); - }); - - describe('edit database', () => { - it('renders the sqlalchemy form when the sqlalchemy_form configuration method is set', async () => { - render(, { useRedux: true }); - - // it should have tabs - const tabs = screen.getAllByRole('tab'); - expect(tabs.length).toEqual(2); - expect(tabs[0]).toHaveTextContent('Basic'); - expect(tabs[1]).toHaveTextContent('Advanced'); - - // it should have the correct header text - const headerText = screen.getByText(/edit database/i); - expect(headerText).toBeVisible(); - - // todo add more when this form is built out - }); - it('renders the dynamic form when the dynamic_form configuration method is set', async () => { - fetchMock.get(DATABASE_FETCH_ENDPOINT, { - result: { - id: 10, - database_name: 'my database', - expose_in_sqllab: false, - allow_ctas: false, - allow_cvas: false, - configuration_method: 'dynamic_form', - parameters: { - database: 'mydatabase', - }, - }, - }); - render(, { useRedux: true }); - - await screen.findByText(/todo/i); - - // // it should have tabs - const tabs = screen.getAllByRole('tab'); - expect(tabs.length).toEqual(2); - - // it should show a TODO for now - const todoText = screen.getAllByText(/todo/i); - expect(todoText[0]).toBeVisible(); - }); - }); }); }); 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} />