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.

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

DAS-7055, DAS-7064 Improve feedback errors for missing fields in report forms #496

Merged
merged 11 commits into from
Oct 13, 2021

Conversation

amicavi
Copy link
Contributor

@amicavi amicavi commented Oct 4, 2021

@amicavi amicavi requested a review from JoshuaVulcan October 4, 2021 13:25
@@ -30,7 +32,7 @@ const filterOutEnumErrors = (errors, schema) => errors // filter out enum-based
}, schema);
}

return !!match && !match.enum;
return !!match || match?.enum?.length;
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 was causing for some required fields to not show an error

Copy link
Collaborator

@JoshuaVulcan JoshuaVulcan left a comment

Choose a reason for hiding this comment

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

Looks really good! Could we get a couple unit tests for ErrorMessages.js to document and ensure its functionality?

src/ReportForm/ErrorMessages.js Outdated Show resolved Hide resolved
src/ReportForm/ReportFormBody.js Show resolved Hide resolved
src/ReportForm/styles.module.scss Outdated Show resolved Hide resolved
@amicavi amicavi requested a review from JoshuaVulcan October 8, 2021 17:07
Copy link
Collaborator

@JoshuaVulcan JoshuaVulcan left a comment

Choose a reason for hiding this comment

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

Getting close! Let's give the tests another round first.

src/ReportForm/ErrorMessages.test.js Outdated Show resolved Hide resolved
src/ReportForm/ErrorMessages.test.js Outdated Show resolved Hide resolved
src/ReportForm/ErrorMessages.test.js Outdated Show resolved Hide resolved
src/ReportForm/ErrorMessages.test.js Outdated Show resolved Hide resolved
@JoshuaVulcan JoshuaVulcan requested a review from luixlive October 11, 2021 21:13
@amicavi amicavi requested a review from JoshuaVulcan October 12, 2021 14:09
src/ReportForm/ErrorMessages.test.js Show resolved Hide resolved
@@ -8,14 +8,16 @@ import styles from './styles.module.scss';

const additionalMetaSchemas = [draft4JsonSchema];

const getLinearErrorPropTree = (errorProperty) => {
return errorProperty.replace(/'|\.properties|\[|\]|\.enumNames|\.enum/g, '.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can create a constant for the regex, using an explicit name that explains the purpose of it. This way it would be easier to know what we are trying to replace here.

@amicavi amicavi requested a review from luixlive October 12, 2021 19:30
Copy link
Contributor

@luixlive luixlive left a comment

Choose a reason for hiding this comment

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

💯

@@ -70,7 +70,8 @@ describe('Error messages', () => {
expect(expandedAccordion).toBeTruthy();

userEvent.click(detailsButton);
expect(screen.getByRole('menuitem', { expanded: false })).toBeTruthy();
notExpandedAccordion = screen.getByRole('menuitem', { expanded: false });
expect(notExpandedAccordion).toBeTruthy();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Much better! 🎉

@JoshuaVulcan JoshuaVulcan merged commit bc6e3f5 into develop Oct 13, 2021
@JoshuaVulcan JoshuaVulcan deleted the DAS-7057 branch October 13, 2021 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants