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

UIREC-308 Add validation for the claimingInterval field #474

Merged
merged 2 commits into from
Jan 3, 2024
Merged

UIREC-308 Add validation for the claimingInterval field #474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e3c51b
UIREC-308 Add validation for the 'claimingInterval' field
usavkov-epam Dec 27, 2023
f56f81d
UIREC-308 Add unit test for validation
usavkov-epam Dec 27, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* Display claim actions for Piece. Refs UIREC-291.
* Set claiming workflow statuses for single Piece. Refs UIREC-292.
* Add unreceivable accordion to receiving title view. Refs UIREC-302.
* Add validation for the `claimingInterval` field. Refs UIREC-308.

## [4.0.0](https://github.com/folio-org/ui-receiving/tree/v4.0.0) (2023-10-12)
[Full Changelog](https://github.com/folio-org/ui-receiving/compare/v3.0.0...v4.0.0)
Expand Down
15 changes: 13 additions & 2 deletions src/TitleForm/TitleForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
FormFooter,
handleKeyCommand,
validateRequired,
validateRequiredPositiveNumber,
} from '@folio/stripes-acq-components';

import {
Expand All @@ -42,6 +43,12 @@ import ContributorsForm from './ContributorsForm';

const ALLOWED_YEAR_LENGTH = 4;

const validateClaimingInterval = (value, { claimingActive }) => {
mariia-aloshyna marked this conversation as resolved.
Show resolved Hide resolved
if (!claimingActive) return undefined;

return validateRequired(value) || validateRequiredPositiveNumber(value);
};

const TitleForm = ({
handleSubmit,
hasValidationErrors,
Expand Down Expand Up @@ -72,6 +79,7 @@ const TitleForm = ({
const paneTitle = isEditMode
? title
: <FormattedMessage id="ui-receiving.title.paneTitle.create" />;
const isClaimingActive = Boolean(values.claimingActive);

const addInstance = form.mutators.setTitleValue;
const addLines = form.mutators.setPOLine;
Expand Down Expand Up @@ -246,7 +254,7 @@ const TitleForm = ({
name="claimingActive"
type="checkbox"
vertical
validateFields={[]}
validateFields={['claimingInterval']}
/>
</Col>

Expand All @@ -259,8 +267,11 @@ const TitleForm = ({
name="claimingInterval"
component={TextField}
type="number"
min={1}
fullWidth
disabled={!values.claimingActive}
disabled={!isClaimingActive}
required={isClaimingActive}
validate={validateClaimingInterval}
validateFields={[]}
/>
</Col>
Expand Down
29 changes: 29 additions & 0 deletions src/TitleForm/TitleForm.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,35 @@ describe('TitleForm', () => {
expect(claimingIntervalField).toHaveValue(null);
});

it('should validate \'Claiming interval\' field', async () => {
Copy link

Choose a reason for hiding this comment

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

I suggest separating all these checks into separate test cases. You have a lot of checks here, if one of them fails, you won't be able to detect which one exactly does. Try the next structure:

describe('when "Claiming active" unchecked', () => {
  it('field shouldn't be required')
});

describe('when "Claiming active" checked', () => {
  it('field should be required')
});
and so on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! However, this test is also designed to track the state of the field over time, so it is reduced to one test, this allows us to ensure that validation works when dependent fields change.

renderTitleForm();

const claimingActiveField = screen.getByRole('checkbox', { name: 'ui-receiving.title.claimingActive' });
const claimingIntervalField = screen.getByLabelText('ui-receiving.title.claimingInterval');

// Shouldn't be required if "Claiming active" unchecked
expect(claimingIntervalField).not.toBeRequired();
expect(claimingIntervalField).toBeValid();

await user.click(claimingActiveField);
expect(claimingIntervalField).toBeRequired();
expect(claimingIntervalField).not.toBeValid();

await user.type(claimingIntervalField, '-2');
expect(claimingIntervalField).toHaveValue(-2);
expect(claimingIntervalField).not.toBeValid();

await user.clear(claimingIntervalField);
await user.type(claimingIntervalField, '0');
expect(claimingIntervalField).toHaveValue(0);
expect(claimingIntervalField).not.toBeValid();

await user.clear(claimingIntervalField);
await user.type(claimingIntervalField, '69');
expect(claimingIntervalField).toHaveValue(69);
expect(claimingIntervalField).toBeValid();
});

describe('Close form', () => {
it('should close Title form', async () => {
const { getByText } = renderTitleForm();
Expand Down
Loading