Skip to content

Fix: Better validation messages using schema/uiSchema title if it exists #3337

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

Merged
merged 28 commits into from
Jan 15, 2023
Merged

Fix: Better validation messages using schema/uiSchema title if it exists #3337

merged 28 commits into from
Jan 15, 2023

Conversation

dagda1
Copy link
Contributor

@dagda1 dagda1 commented Jan 3, 2023

Reasons for making this change

Fixes #3246 by reimplementing #2634

  • The ajv8 ErrorObject message is enhanced by replacing the error message field with either the uiSchema's ui:title field if one exists or the parentSchema title if one exists.

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npm run test:update to update snapshots, if needed.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

Copy link
Member

@heath-freenome heath-freenome left a comment

Choose a reason for hiding this comment

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

Whoops, these have been sitting pending for days as I forgot to click submit, sorry

@heath-freenome
Copy link
Member

@dagda1 Would you please revert the package-lock.json changes

@heath-freenome
Copy link
Member

Finally, once this is merged, we will release 5.0.0

@dagda1
Copy link
Contributor Author

dagda1 commented Jan 13, 2023

@dagda1 Would you please revert the package-lock.json changes

oops, I think I might have a different version of npm than you. What version are you using.

I have reverted the files.

@dagda1 dagda1 requested a review from heath-freenome January 13, 2023 16:29
@heath-freenome
Copy link
Member

@dagda1 Would you please revert the package-lock.json changes

oops, I think I might have a different version of npm than you. What version are you using.

I have reverted the files.

I'm using node 18 although node 16 also uses the same formats. I'm pushing a PR #3372 to bump the package-lock.json files using clean node_modules directories which is how new developers like you start out

@dagda1 dagda1 requested a review from heath-freenome January 14, 2023 10:25
@dagda1
Copy link
Contributor Author

dagda1 commented Jan 14, 2023

@heath-freenome I'm getting yellow lines in the coverage report because of the optional chaining around both message and parentSchema.

e.g.

message = message?.replace(currentProperty, uiSchemaTitle);

and

parentSchema?.[PROPERTIES_KEY]?.[currentProperty]?.title;

@dagda1 dagda1 closed this Jan 14, 2023
@dagda1
Copy link
Contributor Author

dagda1 commented Jan 14, 2023

@heath-freenome I am getting yellow lines in the coverage report around the optional chaining on message and parentSchema.

I notice the existing code does this with message

stack = message!;

This tells the typescript compiler that message will not be null or undefined. If this is the case, would it not make sense to make message in ErrorObject not optional?

And for parentSchema, do you know how I could be able to test it is undefined? The only way I can think is if transformRJSFValidationErrors was not private.

@dagda1 dagda1 reopened this Jan 14, 2023
@heath-freenome
Copy link
Member

heath-freenome commented Jan 14, 2023

@heath-freenome I am getting yellow lines in the coverage report around the optional chaining on message and parentSchema.

I notice the existing code does this with message

stack = message!;

This tells the typescript compiler that message will not be null or undefined. If this is the case, would it not make sense to make message in ErrorObject not optional?

ErrorObject comes from ajv and from all the testing it normally has a value so that's the reason for the !

And for parentSchema, do you know how I could be able to test it is undefined? The only way I can think is if transformRJSFValidationErrors was not private.

If you look up the top of validator.test.ts around line 16 you'll see how you can "expose" that function in a manner similar to how we exposed the private withIdRefPrefix so it could be tested for similar boundary conditions

@heath-freenome
Copy link
Member

One last thing, don't forget to update the CHANGELOG.md similar to how it is done here

@heath-freenome
Copy link
Member

One required and one optional change and then we can merge it!

@dagda1 dagda1 requested a review from heath-freenome January 14, 2023 22:30
Copy link
Member

@heath-freenome heath-freenome left a comment

Choose a reason for hiding this comment

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

Ah... Sorry for being so picky, but a few more little things. I'd make them myself except I can't easily update the location of the import

@heath-freenome
Copy link
Member

@dagda1 Thanks for all your hard work on this PR and dealing with the feedback. I'm just awaiting the final move of the import in the test then I will merge

@heath-freenome heath-freenome mentioned this pull request Jan 14, 2023
8 tasks
@heath-freenome heath-freenome changed the title Better validation messages Fix: Better validation messages using schema/uiSchema title if it exists Jan 14, 2023
@heath-freenome heath-freenome merged commit 885969b into rjsf-team:main Jan 15, 2023
@dagda1 dagda1 deleted the better-validation-messages branch January 16, 2023 09:44
@dagda1
Copy link
Contributor Author

dagda1 commented Jan 16, 2023

@dagda1 Thanks for all your hard work on this PR and dealing with the feedback. I'm just awaiting the final move of the import in the test then I will merge

the feedback was constructive so thank you for the good review

shijistar pushed a commit to shijistar/react-jsonschema-form that referenced this pull request Jun 8, 2023
…sts (rjsf-team#3337)

* use title if available for validation errors

* add title tests to validator-ajv8 validator tests

* use scoped lodash/get

* set verbose in createAjvInstance

* use parentSchema for getting title

* remove . in validator tests

* rename local vars

* remove duplicate local var

* Update packages/validator-ajv8/test/validator.test.ts

Co-authored-by: Heath C <[email protected]>

* use params.missingProperty for currentProperty

* check uiSchema for ui:title override in transformRJSFValidationErrors

* revert package-lock.json files

* Update packages/validator-ajv8/src/validator.ts

Co-authored-by: Heath C <[email protected]>

* Update packages/validator-ajv8/src/validator.ts

Co-authored-by: Heath C <[email protected]>

* add getUiOptions import

* remove type arguments in call to getUiOptions

* revert to leading period in default stack message

* revert lock files

* test for ajv8 ErrorObject optional fields

* use array notation get

* update CHANGELOG

* Update CHANGELOG.md

Co-authored-by: Heath C <[email protected]>

* Update packages/validator-ajv8/package.json

Co-authored-by: Heath C <[email protected]>

* Update packages/validator-ajv8/src/validator.ts

Co-authored-by: Heath C <[email protected]>

* Update packages/validator-ajv8/src/validator.ts

Co-authored-by: Heath C <[email protected]>

* move import

* Update packages/validator-ajv8/test/validator.test.ts

* Update CHANGELOG.md

Co-authored-by: Heath C <[email protected]>
shijistar pushed a commit to shijistar/react-jsonschema-form that referenced this pull request Jun 8, 2023
…sts (rjsf-team#3337)

* use title if available for validation errors

* add title tests to validator-ajv8 validator tests

* use scoped lodash/get

* set verbose in createAjvInstance

* use parentSchema for getting title

* remove . in validator tests

* rename local vars

* remove duplicate local var

* Update packages/validator-ajv8/test/validator.test.ts

Co-authored-by: Heath C <[email protected]>

* use params.missingProperty for currentProperty

* check uiSchema for ui:title override in transformRJSFValidationErrors

* revert package-lock.json files

* Update packages/validator-ajv8/src/validator.ts

Co-authored-by: Heath C <[email protected]>

* Update packages/validator-ajv8/src/validator.ts

Co-authored-by: Heath C <[email protected]>

* add getUiOptions import

* remove type arguments in call to getUiOptions

* revert to leading period in default stack message

* revert lock files

* test for ajv8 ErrorObject optional fields

* use array notation get

* update CHANGELOG

* Update CHANGELOG.md

Co-authored-by: Heath C <[email protected]>

* Update packages/validator-ajv8/package.json

Co-authored-by: Heath C <[email protected]>

* Update packages/validator-ajv8/src/validator.ts

Co-authored-by: Heath C <[email protected]>

* Update packages/validator-ajv8/src/validator.ts

Co-authored-by: Heath C <[email protected]>

* move import

* Update packages/validator-ajv8/test/validator.test.ts

* Update CHANGELOG.md

Co-authored-by: Heath C <[email protected]>
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.

Is it possible to display field titles in validation error messages instead of real field names?
2 participants