Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display errors in SUSE Manager settings form #2455

Merged
merged 4 commits into from
Mar 25, 2024
Merged

Display errors in SUSE Manager settings form #2455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5c2a4bf
Display errors in suse manager settings form
dottorblaster Mar 21, 2024
d4a9f5c
Add a Jest test for the modal
dottorblaster Mar 22, 2024
3b1d290
Remove a useless jest.fn() from a test
dottorblaster Mar 22, 2024
e816e64
Clear errors on canceling
dottorblaster Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 67 additions & 43 deletions assets/js/common/SuseManagerSettingsDialog/SuseManagerSettingsModal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, { useState } from 'react';
import { noop } from 'lodash';
import { capitalize, noop } from 'lodash';
import { format } from 'date-fns';
import { EOS_LOCK_OUTLINED } from 'eos-icons-react';

Expand All @@ -8,7 +8,7 @@ import Modal from '@common/Modal';
import Input, { Password, Textarea } from '@common/Input';
import Label from '@common/Label';

import { hasError } from './errors';
import { hasError, getError } from './errors';

const defaultErrors = [];

Expand Down Expand Up @@ -54,37 +54,49 @@ function SuseManagerSettingsModal({

return (
<Modal title="Enter SUSE Manager Settings" open={open} onClose={onCancel}>
<div className="grid grid-cols-6 items-center my-5 gap-6">
<div className="grid grid-cols-6 my-5 gap-6">
<Label className="col-span-2" required>
SUSE Manager URL
</Label>
<Input
className="col-span-4"
value={url}
placeholder="Enter a URL"
error={hasError('url', errors)}
onChange={({ target: { value } }) => {
setUrl(value);
onClearErrors();
}}
/>
<div className="col-span-4">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about embedding the error in the input component?

<Input
  value={url}
  placeholder="Enter a URL"
  error={getError('url', errors)}
  onChange={({ target: { value } }) => {
    setUrl(value);
    onClearErrors();
  }}
/>

Really up to you 😄 There are just many hasError and related getError.
I am fine either way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something I'd be up to but it would tie us to one single implementation of error visualization and I want to wait instead for more form implementations to see what we settle upon.

<Input
value={url}
placeholder="Enter a URL"
error={hasError('url', errors)}
onChange={({ target: { value } }) => {
setUrl(value);
onClearErrors();
}}
/>
{hasError('url', errors) && (
<p className="text-red-500 mt-1">
{capitalize(getError('url', errors))}
</p>
)}
</div>
<Label
className="col-span-2 self-start"
info="Only required for self-signed certificates"
>
CA Certificate
</Label>
{editingCertificate ? (
<Textarea
className="col-span-4"
value={certificate}
placeholder="Starts with -----BEGIN CERTIFICATE-----"
error={hasError('ca_cert', errors)}
onChange={({ target: { value } }) => {
setCertificate(value);
onClearErrors();
}}
/>
<div className="col-span-4">
<Textarea
value={certificate}
placeholder="Starts with -----BEGIN CERTIFICATE-----"
error={hasError('ca_cert', errors)}
onChange={({ target: { value } }) => {
setCertificate(value);
onClearErrors();
}}
/>
{hasError('ca_cert', errors) && (
<p className="text-red-500 mt-1">
{capitalize(getError('ca_cert', errors))}
</p>
)}
</div>
) : (
<div className="col-span-4 flex flex-row items-center justify-start p-5 border border-gray-200 rounded-md">
<EOS_LOCK_OUTLINED className="mr-3" size="25" />
Expand All @@ -104,30 +116,42 @@ function SuseManagerSettingsModal({
<Label className="col-span-2" required>
Username
</Label>
<Input
className="col-span-4"
value={username}
placeholder="Enter a SUSE Manager username"
error={hasError('username', errors)}
onChange={({ target: { value } }) => {
setUsername(value);
onClearErrors();
}}
/>
<Label className="col-span-2" required>
Password
</Label>
{editingPassword ? (
<Password
className="col-span-4"
value={password}
placeholder="Enter a SUSE Manager password"
error={hasError('password', errors)}
<div className="col-span-4">
<Input
value={username}
placeholder="Enter a SUSE Manager username"
error={hasError('username', errors)}
onChange={({ target: { value } }) => {
setPassword(value);
setUsername(value);
onClearErrors();
}}
/>
{hasError('username', errors) && (
<p className="text-red-500 mt-1">
{capitalize(getError('username', errors))}
</p>
)}
</div>
<Label className="col-span-2" required>
Password
</Label>
{editingPassword ? (
<div className="col-span-4">
<Password
value={password}
placeholder="Enter a SUSE Manager password"
error={hasError('password', errors)}
onChange={({ target: { value } }) => {
setPassword(value);
onClearErrors();
}}
/>
{hasError('password', errors) && (
<p className="text-red-500 mt-1">
{capitalize(getError('password', errors))}
</p>
)}
</div>
) : (
<div className="col-span-4 border border-gray-200 p-5 rounded-md">
<p className="inline align-sub leading-10">•••••</p>
Expand Down
28 changes: 28 additions & 0 deletions assets/js/common/SuseManagerSettingsDialog/SuseManagerSettingsModal.stories.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,34 @@ export const WithErrors = {
},
};

export const WithAllErrors = {
args: {
open: false,
errors: [
{
detail: "can't be blank",
source: { pointer: '/url' },
title: 'Invalid value',
},
{
detail: "can't be blank",
source: { pointer: '/ca_cert' },
title: 'Invalid value',
},
{
detail: "can't be blank",
source: { pointer: '/password' },
title: 'Invalid value',
},
{
detail: "can't be blank",
source: { pointer: '/username' },
title: 'Invalid value',
},
],
},
};

export const Loading = {
args: {
open: false,
Expand Down
34 changes: 34 additions & 0 deletions assets/js/common/SuseManagerSettingsDialog/SuseManagerSettingsModal.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { render, screen, act } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import '@testing-library/jest-dom';

import { capitalize } from 'lodash';

import SuseManagerSettingsModal from '.';

describe('SuseManagerSettingsModal component', () => {
Expand Down Expand Up @@ -126,4 +128,36 @@ describe('SuseManagerSettingsModal component', () => {
url,
});
});

it('should display errors', async () => {
const detail = capitalize(faker.lorem.words(5));

const errors = [
{
detail,
source: { pointer: '/url' },
title: 'Invalid value',
},
{
detail,
source: { pointer: '/ca_cert' },
title: 'Invalid value',
},
];

await act(async () => {
render(
<SuseManagerSettingsModal
initialUsername={faker.word.noun()}
initialUrl={faker.internet.url()}
errors={errors}
open
onSave={() => {}}
onCancel={() => {}}
/>
);
});

expect(screen.getAllByText(detail)).toHaveLength(2);
});
});
20 changes: 14 additions & 6 deletions assets/js/common/SuseManagerSettingsDialog/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
import { get } from 'lodash';
import { flow, first } from 'lodash';
import { get, filter } from 'lodash/fp';

export const hasError = (keyword, errors) =>
errors.some((error) => {
const pointer = get(error, ['source', 'pointer']);

if (!pointer) {
return false;
}
const pointer = get(['source', 'pointer'], error);

return pointer === `/${keyword}`;
});

export const getError = (keyword, errors) =>
flow([
filter((error) => {
const pointer = get(['source', 'pointer'], error);

return pointer === `/${keyword}`;
}),
first,
get('detail'),
])(errors);
42 changes: 41 additions & 1 deletion assets/js/common/SuseManagerSettingsDialog/errors.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { hasError } from './errors';
import { hasError, getError } from './errors';

describe('hasError', () => {
it('should tell that a list contains an error about a specific field', () => {
Expand Down Expand Up @@ -39,3 +39,43 @@ describe('hasError', () => {
expect(hasError('url', errors)).toBe(false);
});
});

describe('getError', () => {
it('should spot nothing in an empty list', () => {
expect(getError('url', [])).toBe(undefined);
});

it('should get nothing when the keyword is not in the list', () => {
const errors = [
{
detail: "can't be blank",
source: { pointer: '/username' },
title: 'Invalid value',
},
{
detail: "can't be blank",
source: { pointer: '/ca_cert' },
title: 'Invalid value',
},
];

expect(getError('username', errors)).toBe("can't be blank");
});

it('should get nothing when the keyword is not in the list', () => {
const errors = [
{
detail: "can't be blank",
source: { pointer: '/username' },
title: 'Invalid value',
},
{
detail: "can't be blank",
source: { pointer: '/ca_cert' },
title: 'Invalid value',
},
];

expect(getError('url', errors)).toBe(undefined);
});
});
6 changes: 5 additions & 1 deletion assets/js/pages/SettingsPage/SettingsPage.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
setEditingSoftwareUpdatesSettings,
clearSoftwareUpdatesSettings,
testSoftwareUpdatesConnection,
setSoftwareUpdatesSettingsErrors,
} from '@state/softwareUpdatesSettings';
import {
getSoftwareUpdatesSettings,
Expand Down Expand Up @@ -299,7 +300,10 @@ function SettingsPage() {
dispatch(saveSoftwareUpdatesSettings(payload));
}
}}
onCancel={() => dispatch(setEditingSoftwareUpdatesSettings(false))}
onCancel={() => {
dispatch(setSoftwareUpdatesSettingsErrors([]));
dispatch(setEditingSoftwareUpdatesSettings(false));
}}
/>
</div>
)}
Expand Down
Loading