Skip to content

Commit

Permalink
Error messages aren't displaying on the remote container create form (a…
Browse files Browse the repository at this point in the history 
…nsible#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
  • Loading branch information
@MilanPospisil
MilanPospisil committed Dec 14, 2022
1 parent 4afaca7 commit 77ecd51
Show file tree
Hide file tree
Showing 6 changed files with 216 additions and 130 deletions.
1 change: 1 addition & 0 deletions CHANGES/1845.bug
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Repair error mesages in EE form.
148 changes: 97 additions & 51 deletions src/components/execution-environment/repository-form.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -42,23 +55,20 @@ 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[];
includeTags?: string[];
registries?: { id: string; name: string }[];
registrySelection?: { id: string; name: string }[];
upstreamName: string;
formErrors?: {
registries?: AlertType;
};
formErrors: ErrorMessagesType;
}

export class RepositoryForm extends React.Component<IProps, IState> {
Expand All @@ -75,9 +85,8 @@ export class RepositoryForm extends React.Component<IProps, IState> {
registries: null,
registrySelection: [],
upstreamName: this.props.upstreamName || '',
formErrors: {
registries: null,
},
formErrors: {},
alerts: [],
};
}

Expand All @@ -96,15 +105,14 @@ export class RepositoryForm extends React.Component<IProps, IState> {
})
.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 },
});
});
}
Expand Down Expand Up @@ -147,6 +155,10 @@ export class RepositoryForm extends React.Component<IProps, IState> {
</Button>,
]}
>
<AlertList
alerts={this.state.alerts}
closeAlert={(i) => this.closeAlert(i)}
/>
<Form>
{!isRemote ? (
<>
Expand Down Expand Up @@ -179,15 +191,18 @@ export class RepositoryForm extends React.Component<IProps, IState> {
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')}
>
<TextInput
id='name'
value={name}
isDisabled={!isNew}
onChange={(value) => this.setState({ name: value })}
validated={this.validateName(name)}
onChange={(value) => {
this.setState({ name: value });
this.validateName(value);
}}
validated={isFieldValid(this.state.formErrors, 'name')}
/>
</FormGroup>

Expand Down Expand Up @@ -215,16 +230,16 @@ export class RepositoryForm extends React.Component<IProps, IState> {
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 ? (
<Alert
title={formErrors.registries.title}
variant='danger'
isInline
>
{formErrors.registries.description}
</Alert>
) : (
{!formErrors?.registries && (
<>
{registries ? (
<APISearchTypeAhead
Expand All @@ -235,6 +250,7 @@ export class RepositoryForm extends React.Component<IProps, IState> {
registrySelection: registries.filter(
({ name }) => name === value,
),
formErrors: { ...formErrors, registry: null },
})
}
placeholderText={t`Select a registry`}
Expand Down Expand Up @@ -390,9 +406,11 @@ export class RepositoryForm extends React.Component<IProps, IState> {
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 } });
}
}

Expand All @@ -402,8 +420,13 @@ export class RepositoryForm extends React.Component<IProps, IState> {
// 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?) {
Expand Down Expand Up @@ -455,30 +478,53 @@ export class RepositoryForm extends React.Component<IProps, IState> {
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');
}
}
63 changes: 24 additions & 39 deletions src/containers/execution-environment-detail/base.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ interface IState {
editing: boolean;
alerts: AlertType[];
showDeleteModal: boolean;
formError: { title: string; detail: string }[];
}

export interface IDetailSharedProps extends RouteComponentProps {
Expand All @@ -64,7 +63,6 @@ export function withContainerRepo(WrappedComponent) {
editing: false,
alerts: [],
showDeleteModal: false,
formError: [],
};
}

Expand Down Expand Up @@ -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: (
<Trans>
Saved changes to execution environment &quot;
{this.state.repo.name}&quot;.
</Trans>
),
}),
});
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: (
<Trans>
Saved changes to execution environment &quot;
{this.state.repo.name}&quot;.
</Trans>
),
}),
);
});
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}
Expand Down

0 comments on commit 77ecd51

Please sign in to comment.