This is an automated email from the ASF dual-hosted git repository. hugh pushed a commit to branch hugh/bg-validation-db-modal in repository https://gitbox.apache.org/repos/asf/superset.git
commit e7640724877caf356c114659f1d09e5e2691cf9f Author: Elizabeth Thompson <[email protected]> AuthorDate: Fri May 21 10:57:08 2021 -0700 use new validation component --- .../cypress/integration/database/modal.test.ts | 24 +++ superset-frontend/src/components/Form/FormItem.tsx | 4 + .../src/components/Form/LabeledErrorBoundInput.tsx | 18 +- .../DatabaseModal/DatabaseConnectionForm.tsx | 240 +++++++++++++-------- .../data/database/DatabaseModal/index.test.jsx | 2 +- .../CRUD/data/database/DatabaseModal/index.tsx | 33 +-- .../CRUD/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 e29ec48..cf27d21 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 ab301a8..2731a21 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 8569b55..75df2bb 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) => ( - <> + <StyledFormGroup className={className}> <FormLabel htmlFor={id} required={required}> {label} </FormLabel> @@ -83,7 +91,7 @@ const LabeledErrorBoundInput = ({ > <StyledInput {...props} {...validationMethods} /> </FormItem> - </> + </StyledFormGroup> ); 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 f0542e4..5c7c729 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) => ( + <ValidatedInput + id="host" + name="host" + required={required} + validationMethods={{ onBlur: getValidation }} + errorMessage={validationErrors?.host} + placeholder="e.g. 127.0.0.1" + className="form-group-w-50" + label="Host" + onChange={changeMethods.onParametersChange} + /> +); +const portField = ({ + required, + changeMethods, + getValidation, + validationErrors, +}: FieldPropTypes) => ( + <ValidatedInput + id="port" + name="port" + required={required} + validationMethods={{ onBlur: getValidation }} + errorMessage={validationErrors?.port} + placeholder="e.g. 5432" + className="form-group-w-50" + label="Port" + onChange={changeMethods.onParametersChange} + /> +); +const databaseField = ({ + required, + changeMethods, + getValidation, + validationErrors, +}: FieldPropTypes) => ( + <ValidatedInput + id="database" + name="database" + required={required} + validationMethods={{ onBlur: getValidation }} + errorMessage={validationErrors?.database} + placeholder="e.g. world_population" + label="Database name" + onChange={changeMethods.onParametersChange} + helpText="Copy the name of the PostgreSQL database you are trying to connect to." + /> +); +const usernameField = ({ + required, + changeMethods, + getValidation, + validationErrors, +}: FieldPropTypes) => ( + <ValidatedInput + id="username" + name="username" + required={required} + validationMethods={{ onBlur: getValidation }} + errorMessage={validationErrors?.username} + placeholder="e.g. Analytics" + label="Username" + onChange={changeMethods.onParametersChange} + /> +); +const passwordField = ({ + required, + changeMethods, + getValidation, + validationErrors, +}: FieldPropTypes) => ( + <ValidatedInput + id="password" + name="password" + required={required} + validationMethods={{ onBlur: getValidation }} + errorMessage={validationErrors?.password} + placeholder="e.g. ********" + label="Password" + onChange={changeMethods.onParametersChange} + /> +); +const displayField = ({ + required, + changeMethods, + getValidation, + validationErrors, +}: FieldPropTypes) => ( + <ValidatedInput + id="database_name" + name="database_name" + required={required} + validationMethods={{ onBlur: getValidation }} + errorMessage={validationErrors?.database_name} + placeholder="" + label="Display Name" + onChange={changeMethods.onChange} + helpText="Pick a nickname for this database to display as in Superset." + /> +); 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<InputProps> | { target: HTMLInputElement }, ) => void; + validationErrors: JsonObject | null; + getValidation: () => void; }) => ( <> <StyledFormHeader> @@ -107,52 +185,30 @@ const DatabaseConnectionForm = ({ Need help? Learn more about connecting to {name}. </p> </StyledFormHeader> - <div css={formScrollableStyles}> + <div + // @ts-ignore + css={(theme: SupersetTheme) => [ + 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 ( - <FormItem - className={cx(className, `form-group-${className}`)} - key={field} - > - <FormLabel - htmlFor={field} - required={parameters.required.includes(field)} - > - {description} - </FormLabel> - <Input - name={field} - type={type} - id={field} - autoComplete="off" - placeholder={placeholder} - onChange={onEdit} - /> - <p className="helper">{label}</p> - </FormItem> - ); - })} + ).map(field => + FORM_FIELD_MAP[field]({ + required: parameters.required.includes(field), + changeMethods: { onParametersChange, onChange }, + validationErrors, + getValidation, + key: field, + }), + )} </div> </> ); - 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 8696725..3f10914 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 57104b8..265a856 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<DatabaseObject> | 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<DatabaseModalProps> = ({ >(dbReducer, null); const [tabKey, setTabKey] = useState<string>(DEFAULT_TAB_KEY); const [availableDbs, getAvailableDbs] = useAvailableDatabases(); + const [validationErrors, getValidation] = useDatabaseValidation(); const [hasConnectedDb, setHasConnectedDb] = useState<boolean>(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<DatabaseModalProps> = ({ 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<DatabaseModalProps> = ({ 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<DatabaseModalProps> = ({ 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<DatabaseModalProps> = ({ } > {isEditMode ? ( - <EditHeader> + <TabHeader> <EditHeaderTitle>{db?.backend}</EditHeaderTitle> - <EditHeaderSubtitle>{db?.database_name}</EditHeaderSubtitle> - </EditHeader> + <EditHeaderSubtitle>{dbName}</EditHeaderSubtitle> + </TabHeader> ) : ( - <CreateHeader> + <TabHeader> <CreateHeaderTitle>Enter Primary Credentials</CreateHeaderTitle> <CreateHeaderSubtitle> Need help? Learn how to connect your database{' '} @@ -382,7 +383,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({ </a> . </CreateHeaderSubtitle> - </CreateHeader> + </TabHeader> )} <hr /> <Tabs @@ -514,6 +515,8 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({ value: target.value, }) } + getValidation={() => getValidation(db)} + validationErrors={validationErrors} /> <Button buttonStyle="link" 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 4ae5629..a02a228 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts @@ -164,13 +164,6 @@ export const formStyles = (theme: SupersetTheme) => css` margin-left: ${theme.gridUnit * 8}px; } } - .text-danger { - color: ${theme.colors.error.base}; - font-size: ${theme.typography.sizes.s - 1}px; - strong { - font-weight: normal; - } - } } .control-label { color: ${theme.colors.grayscale.dark1}; @@ -187,6 +180,14 @@ export const formStyles = (theme: SupersetTheme) => css` } `; +export const validatedFormStyles = (theme: SupersetTheme) => css` + label { + color: ${theme.colors.grayscale.dark1}; + font-size: ${theme.typography.sizes.s - 1}px; + margin-bottom: 0; + } +`; + export const StyledInputContainer = styled.div` margin-bottom: ${({ theme }) => theme.gridUnit * 6}px; &.mb-0 { @@ -306,23 +307,13 @@ export const buttonLinkStyles = css` text-transform: initial; `; -export const EditHeader = styled.div` - display: flex; - flex-direction: column; - justify-content: center; - padding: 0px; - margin: ${({ theme }) => theme.gridUnit * 4}px - ${({ theme }) => theme.gridUnit * 4}px - ${({ theme }) => theme.gridUnit * 9}px; -`; - -export const CreateHeader = styled.div` +export const TabHeader = styled.div` display: flex; flex-direction: column; justify-content: center; padding: 0px; margin: 0 ${({ theme }) => theme.gridUnit * 4}px - ${({ theme }) => theme.gridUnit * 6}px; + ${({ theme }) => theme.gridUnit * 8}px; `; export const CreateHeaderTitle = styled.div` @@ -339,12 +330,12 @@ export const CreateHeaderSubtitle = styled.div` export const EditHeaderTitle = styled.div` color: ${({ theme }) => theme.colors.grayscale.light1}; - font-size: ${({ theme }) => theme.typography.sizes.s}px; + font-size: ${({ theme }) => theme.typography.sizes.s - 1}px; text-transform: uppercase; `; export const EditHeaderSubtitle = styled.div` color: ${({ theme }) => theme.colors.grayscale.dark1}; - font-size: ${({ theme }) => theme.typography.sizes.xl}px; + font-size: ${({ theme }) => theme.typography.sizes.l}px; font-weight: bold; `; diff --git a/superset-frontend/src/views/CRUD/data/database/types.ts b/superset-frontend/src/views/CRUD/data/database/types.ts index 2c386b5..1baac06 100644 --- a/superset-frontend/src/views/CRUD/data/database/types.ts +++ b/superset-frontend/src/views/CRUD/data/database/types.ts @@ -30,8 +30,9 @@ export type DatabaseObject = { created_by?: null | DatabaseUser; changed_on_delta_humanized?: string; changed_on?: string; - parameters?: { database_name?: string; engine?: string }; + parameters?: { database_name?: string }; configuration_method: CONFIGURATION_METHOD; + engine?: string; // Performance cache_timeout?: string; diff --git a/superset-frontend/src/views/CRUD/hooks.ts b/superset-frontend/src/views/CRUD/hooks.ts index 4275499..0c08030 100644 --- a/superset-frontend/src/views/CRUD/hooks.ts +++ b/superset-frontend/src/views/CRUD/hooks.ts @@ -657,3 +657,52 @@ export function useAvailableDatabases() { return [availableDbs, getAvailable] as const; } + +export function useDatabaseValidation() { + const [validationErrors, setValidationErrors] = useState<JsonObject | null>( + null, + ); + const getValidation = useCallback( + (database: Partial<DatabaseObject> | null) => { + SupersetClient.post({ + endpoint: '/api/v1/database/validate_parameters', + body: JSON.stringify(database), + headers: { 'Content-Type': 'application/json' }, + }) + .then(() => { + setValidationErrors(null); + }) + .catch(e => { + if (typeof e.json === 'function') { + e.json().then(({ errors = [] }: JsonObject) => { + const parsedErrors = errors + .filter( + (error: { error_type: string }) => + error.error_type !== 'CONNECTION_MISSING_PARAMETERS_ERROR', + ) + .reduce( + ( + obj: {}, + { + extra, + message, + }: { + extra: { invalid: string[] }; + message: string; + }, + ) => ({ ...obj, [extra.invalid[0]]: message }), + {}, + ); + setValidationErrors(parsedErrors); + }); + } else { + // eslint-disable-next-line no-console + console.error(e); + } + }); + }, + [setValidationErrors], + ); + + return [validationErrors, getValidation] as const; +} diff --git a/superset/databases/commands/validate.py b/superset/databases/commands/validate.py index 93e32ef..2f084f6 100644 --- a/superset/databases/commands/validate.py +++ b/superset/databases/commands/validate.py @@ -78,7 +78,9 @@ class ValidateDatabaseParametersCommand(BaseCommand): ) # perform initial validation - errors = engine_spec.validate_parameters(self._properties["parameters"]) + errors = engine_spec.validate_parameters( + self._properties.get("parameters", None) + ) if errors: raise InvalidParametersError(errors) @@ -90,7 +92,7 @@ class ValidateDatabaseParametersCommand(BaseCommand): # try to connect sqlalchemy_uri = engine_spec.build_sqlalchemy_uri( - self._properties["parameters"], # type: ignore + self._properties.get("parameters", None), # type: ignore encrypted_extra, ) if self._model and sqlalchemy_uri == self._model.safe_sqlalchemy_uri(): diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py index e952967..a9ea868 100644 --- a/superset/databases/schemas.py +++ b/superset/databases/schemas.py @@ -307,6 +307,12 @@ class DatabaseValidateParametersSchema(Schema): allow_none=True, validate=server_cert_validator, ) + configuration_method = EnumField( + ConfigurationMethod, + by_value=True, + allow_none=True, + description=configuration_method_description, + ) class DatabasePostSchema(Schema, DatabaseParametersSchemaMixin): diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py index 97321b8..593b17a 100644 --- a/superset/db_engine_specs/base.py +++ b/superset/db_engine_specs/base.py @@ -1400,7 +1400,7 @@ class BasicParametersMixin: errors: List[SupersetError] = [] required = {"host", "port", "username", "database"} - present = {key for key in parameters if parameters[key]} # type: ignore + present = {key for key in parameters if parameters.get(key, ())} # type: ignore missing = sorted(required - present) if missing: @@ -1413,7 +1413,7 @@ class BasicParametersMixin: ), ) - host = parameters["host"] + host = parameters.get("host", None) if not host: return errors if not is_hostname_valid(host): @@ -1427,7 +1427,7 @@ class BasicParametersMixin: ) return errors - port = parameters["port"] + port = parameters.get("port", None) if not port: return errors if not is_port_open(host, port):
