From 77ecd51802d1c13977cc28fe00f6901c4378afd0 Mon Sep 17 00:00:00 2001 From: MilanPospisil Date: Wed, 14 Dec 2022 15:33:01 +0100 Subject: [PATCH] Error messages aren't displaying on the remote container create form (#2601) * WIP * Repair forms validation in EE form. Issue: 1845 Repair setState passing to function without lambda PR checks Issue: AAH-1845 Delete unecessary set functions in validation utility PR review Review - registry to registries Use of form valid Handle errors for registries and registry. Log the rest of the errors. Catch promises on save in one place. Alert the rest errors instead of displaying them. Translate error string, show error only for unknown field, clear the error after showing alert Add translation Rename rest remoteId PR review Prettier WIP Prettier Remove messages without field Prettier PR Checks Review - rename remotePulpId and add type to isFieldValid function Prettier Change order of prop WIP WIP --- CHANGES/1845.bug | 1 + .../execution-environment/repository-form.tsx | 148 ++++++++++++------ .../execution-environment-detail/base.tsx | 63 +++----- .../execution_environment_list.tsx | 61 +++----- src/utilities/index.ts | 8 +- src/utilities/map-error-messages.ts | 65 ++++++++ 6 files changed, 216 insertions(+), 130 deletions(-) create mode 100644 CHANGES/1845.bug diff --git a/CHANGES/1845.bug b/CHANGES/1845.bug new file mode 100644 index 0000000000..027c4c816b --- /dev/null +++ b/CHANGES/1845.bug @@ -0,0 +1 @@ +Repair error mesages in EE form. diff --git a/src/components/execution-environment/repository-form.tsx b/src/components/execution-environment/repository-form.tsx index ceee48a2ce..44e3549afc 100644 --- a/src/components/execution-environment/repository-form.tsx +++ b/src/components/execution-environment/repository-form.tsx @@ -15,14 +15,27 @@ import { } from '@patternfly/react-core'; import { ExternalLinkAltIcon, TagIcon } from '@patternfly/react-icons'; import { Link } from 'react-router-dom'; -import { AlertType, APISearchTypeAhead, HelperText } from 'src/components'; +import { + AlertType, + APISearchTypeAhead, + HelperText, + AlertList, + closeAlertMixin, +} from 'src/components'; import { ContainerDistributionAPI, ExecutionEnvironmentRegistryAPI, ExecutionEnvironmentRemoteAPI, } from 'src/api'; import { Paths, formatPath } from 'src/paths'; -import { errorMessage } from 'src/utilities'; +import { + errorMessage, + ErrorMessagesType, + isFieldValid, + isFormValid, + mapErrorMessages, + alertErrorsWithoutFields, +} from 'src/utilities'; interface IProps { name: string; @@ -42,13 +55,12 @@ interface IProps { upstreamName?: string; remoteId?: string; addAlert?: (variant, title, description?) => void; - formError: { title: string; detail: string }[]; } interface IState { name: string; description: string; - + alerts: AlertType[]; addTagsInclude: string; addTagsExclude: string; excludeTags?: string[]; @@ -56,9 +68,7 @@ interface IState { registries?: { id: string; name: string }[]; registrySelection?: { id: string; name: string }[]; upstreamName: string; - formErrors?: { - registries?: AlertType; - }; + formErrors: ErrorMessagesType; } export class RepositoryForm extends React.Component { @@ -75,9 +85,8 @@ export class RepositoryForm extends React.Component { registries: null, registrySelection: [], upstreamName: this.props.upstreamName || '', - formErrors: { - registries: null, - }, + formErrors: {}, + alerts: [], }; } @@ -96,15 +105,14 @@ export class RepositoryForm extends React.Component { }) .catch((e) => { const { status, statusText } = e.response; + const errorTitle = t`Registries list could not be displayed.`; + this.addAlert({ + variant: 'danger', + title: errorTitle, + description: errorMessage(status, statusText), + }); this.setState({ - formErrors: { - ...this.state.formErrors, - registries: { - title: t`Registries list could not be displayed.`, - description: errorMessage(status, statusText), - variant: 'danger', - }, - }, + formErrors: { ...this.state.formErrors, registries: errorTitle }, }); }); } @@ -147,6 +155,10 @@ export class RepositoryForm extends React.Component { , ]} > + this.closeAlert(i)} + />
{!isRemote ? ( <> @@ -179,15 +191,18 @@ export class RepositoryForm extends React.Component { key='name' fieldId='name' label={t`Name`} - helperTextInvalid={t`Container names can only contain alphanumeric characters, ".", "_", "-" and a up to one "/".`} - validated={this.validateName(name)} + helperTextInvalid={this.state.formErrors['name']} + validated={isFieldValid(this.state.formErrors, 'name')} > this.setState({ name: value })} - validated={this.validateName(name)} + onChange={(value) => { + this.setState({ name: value }); + this.validateName(value); + }} + validated={isFieldValid(this.state.formErrors, 'name')} /> @@ -215,16 +230,16 @@ export class RepositoryForm extends React.Component { label={t`Registry`} className='hub-formgroup-registry' isRequired={true} + helperTextInvalid={ + this.state.formErrors['registries'] || + this.state.formErrors['registry'] + } + validated={isFieldValid(this.state.formErrors, [ + 'registries', + 'registry', + ])} > - {formErrors?.registries ? ( - - {formErrors.registries.description} - - ) : ( + {!formErrors?.registries && ( <> {registries ? ( { registrySelection: registries.filter( ({ name }) => name === value, ), + formErrors: { ...formErrors, registry: null }, }) } placeholderText={t`Select a registry`} @@ -390,9 +406,11 @@ export class RepositoryForm extends React.Component { private validateName(name) { const regex = /^([0-9A-Za-z._-]+\/)?[0-9A-Za-z._-]+$/; if (name === '' || regex.test(name)) { - return 'default'; + this.setState({ formErrors: { ...this.state.formErrors, name: null } }); + return; } else { - return 'error'; + const error = t`Container names can only contain alphanumeric characters, ".", "_", "-" and a up to one "/".`; + this.setState({ formErrors: { ...this.state.formErrors, name: error } }); } } @@ -402,8 +420,13 @@ export class RepositoryForm extends React.Component { // no validation for local return true; } - const nameValid = name && this.validateName(name) === 'default'; - return nameValid && upstreamName && registrySelection.length; + + if (!isFormValid(this.state.formErrors)) { + return false; + } + + // validation for non empty fields + return name && upstreamName && registrySelection.length; } private loadRegistries(name?) { @@ -455,30 +478,53 @@ export class RepositoryForm extends React.Component { upstreamName: upstream_name, } = this.state; + let promise = null; if (isRemote && isNew) { - return ExecutionEnvironmentRemoteAPI.create({ + promise = ExecutionEnvironmentRemoteAPI.create({ name, upstream_name, registry, include_tags, exclude_tags, }); + } else { + promise = Promise.all([ + // remote edit - upstream, tags, registry + isRemote && + !isNew && + ExecutionEnvironmentRemoteAPI.update(remoteId, { + name: originalName, // readonly but required + upstream_name, + registry, + include_tags, + exclude_tags, + }), + // remote edit or local edit - description, if changed + description !== originalDescription && + ContainerDistributionAPI.patch(distributionPulpId, { description }), + ]); } - return Promise.all([ - // remote edit - upstream, tags, registry - isRemote && - !isNew && - ExecutionEnvironmentRemoteAPI.update(remoteId, { - name: originalName, // readonly but required - upstream_name, - registry, - include_tags, - exclude_tags, - }), - // remote edit or local edit - description, if changed - description !== originalDescription && - ContainerDistributionAPI.patch(distributionPulpId, { description }), - ]); + return promise.catch((e) => { + this.setState({ formErrors: mapErrorMessages(e) }); + alertErrorsWithoutFields( + this.state.formErrors, + ['name', 'registry', 'registries'], + (alert) => this.addAlert(alert), + t`Error when saving registry.`, + (state) => this.setState({ formErrors: state }), + ); + return Promise.reject(new Error(e)); + }); + } + + private addAlert(alert) { + this.setState({ + alerts: [...this.state.alerts, alert], + }); + } + + private get closeAlert() { + return closeAlertMixin('alerts'); } } diff --git a/src/containers/execution-environment-detail/base.tsx b/src/containers/execution-environment-detail/base.tsx index 7206c74cc5..ae409b4bfa 100644 --- a/src/containers/execution-environment-detail/base.tsx +++ b/src/containers/execution-environment-detail/base.tsx @@ -39,7 +39,6 @@ interface IState { editing: boolean; alerts: AlertType[]; showDeleteModal: boolean; - formError: { title: string; detail: string }[]; } export interface IDetailSharedProps extends RouteComponentProps { @@ -64,7 +63,6 @@ export function withContainerRepo(WrappedComponent) { editing: false, alerts: [], showDeleteModal: false, - formError: [], }; } @@ -223,45 +221,32 @@ export function withContainerRepo(WrappedComponent) { namespace={this.state.repo.namespace.name} description={this.state.repo.description} permissions={permissions} - formError={this.state.formError} onSave={(promise) => { - promise - .then((results) => { - const task = results.find((x) => x.data && x.data.task); - this.setState({ - editing: false, - loading: true, - alerts: alerts.concat({ - variant: 'success', - title: ( - - Saved changes to execution environment " - {this.state.repo.name}". - - ), - }), - }); - if (task) { - waitForTask( - task.data.task.split('tasks/')[1].replace('/', ''), - ).then(() => { - this.loadRepo(); - }); - } else { - this.loadRepo(); - } - }) - .catch((err) => - this.setState({ - formError: err.response.data.errors.map((error) => { - return { - title: error.title, - detail: - error.source.parameter + ': ' + error.detail, - }; - }), + promise.then((results) => { + const task = results.find((x) => x.data && x.data.task); + this.setState({ + editing: false, + loading: true, + alerts: alerts.concat({ + variant: 'success', + title: ( + + Saved changes to execution environment " + {this.state.repo.name}". + + ), }), - ); + }); + if (task) { + waitForTask( + task.data.task.split('tasks/')[1].replace('/', ''), + ).then(() => { + this.loadRepo(); + }); + } else { + this.loadRepo(); + } + }); }} onCancel={() => this.setState({ editing: false })} distributionPulpId={this.state.repo.pulp.distribution.id} diff --git a/src/containers/execution-environment-list/execution_environment_list.tsx b/src/containers/execution-environment-list/execution_environment_list.tsx index 8fc2c2eb51..94f418226b 100644 --- a/src/containers/execution-environment-list/execution_environment_list.tsx +++ b/src/containers/execution-environment-list/execution_environment_list.tsx @@ -59,7 +59,6 @@ interface IState { showDeleteModal: boolean; selectedItem: ExecutionEnvironmentType; inputText: string; - formError: { title: string; detail: string }[]; } class ExecutionEnvironmentList extends React.Component< @@ -95,7 +94,6 @@ class ExecutionEnvironmentList extends React.Component< showDeleteModal: false, selectedItem: null, inputText: '', - formError: [], }; } @@ -450,42 +448,30 @@ class ExecutionEnvironmentList extends React.Component< permissions={namespace?.my_permissions || []} remoteId={remoteId} distributionPulpId={distributionPulpId} - formError={this.state.formError} onSave={(promise, form) => { - promise - .then(() => { - this.setState( - { - showRemoteModal: false, - itemToEdit: null, - alerts: alerts.concat({ - variant: 'success', - title: isNew ? ( - - Execution environment "{form.name}" has been - added successfully. - - ) : ( - - Saved changes to execution environment "{form.name} - ". - - ), - }), - }, - () => this.queryEnvironments(), - ); - }) - .catch((err) => { - this.setState({ - formError: err.response.data.errors.map((error) => { - return { - title: error.title, - detail: error.source.parameter + ': ' + error.detail, - }; + promise.then(() => { + this.setState( + { + showRemoteModal: false, + itemToEdit: null, + alerts: alerts.concat({ + variant: 'success', + title: isNew ? ( + + Execution environment "{form.name}" has been + added successfully. + + ) : ( + + Saved changes to execution environment "{form.name} + ". + + ), }), - }); - }); + }, + () => this.queryEnvironments(), + ); + }); }} onCancel={() => this.setState({ @@ -493,9 +479,6 @@ class ExecutionEnvironmentList extends React.Component< itemToEdit: null, }) } - addAlert={(variant, title, description) => - this.addAlert(title, variant, description) - } /> ); } diff --git a/src/utilities/index.ts b/src/utilities/index.ts index b811f42048..98b7c3b403 100644 --- a/src/utilities/index.ts +++ b/src/utilities/index.ts @@ -1,7 +1,13 @@ export { convertContentSummaryCounts } from './content-summary'; export { ParamHelper } from './param-helper'; export { sanitizeDocsUrls } from './sanitize-docs-urls'; -export { mapErrorMessages, ErrorMessagesType } from './map-error-messages'; +export { + mapErrorMessages, + ErrorMessagesType, + isFieldValid, + isFormValid, + alertErrorsWithoutFields, +} from './map-error-messages'; export { getRepoUrl, getContainersURL } from './get-repo-url'; export { twoWayMapper } from './two-way-mapper'; export { diff --git a/src/utilities/map-error-messages.ts b/src/utilities/map-error-messages.ts index 9120f0bd3d..f3d15a2330 100644 --- a/src/utilities/map-error-messages.ts +++ b/src/utilities/map-error-messages.ts @@ -26,3 +26,68 @@ export function mapErrorMessages(err): ErrorMessagesType { return messages; } + +export function isFieldValid( + errorMessagesType: ErrorMessagesType, + name, +): 'default' | 'error' { + let names = []; + if (Array.isArray(name)) { + names = name; + } else { + names.push(name); + } + + if (!errorMessagesType) { + return 'default'; + } + + return names.find((n) => errorMessagesType[n]) ? 'error' : 'default'; +} + +export function isFormValid(errorMessages: ErrorMessagesType) { + if (!errorMessages) { + return true; + } + + return !Object.values(errorMessages).find(Boolean); +} + +export function alertErrorsWithoutFields( + errorMessages: ErrorMessagesType, + fields, + addAlert, + title, + setErrorMessages, +) { + if (!errorMessages) { + return; + } + + // select only errors without associated field + const errors = Object.keys(errorMessages) + .filter((field) => !fields.includes(field)) + .map((field) => errorMessages[field]); + + if (errors.length) { + // alert them + addAlert({ + variant: 'danger', + title: title, + description: errors.join('\n'), + }); + + // filter only errors with field, rest will be removed from the state, because they were already alerted + const formErrors = {}; + + Object.keys(errorMessages).forEach((field) => { + if (fields.includes(field)) { + formErrors[field] = errorMessages[field]; + } + }); + + setErrorMessages(formErrors); + } + + return; +}