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

feat(framework,novui): controls validation #6428

Closed
wants to merge 22 commits into from
Closed

Conversation

ainouzgali
Copy link
Contributor

What changed? Why was the change needed?

Opened instead of this closed PR

Currently, validation is only with the default JSON schema validation

In this PR we ensure that the errors object provided by the validate action maps the errors back to the correct locations in the form (both errors defined in Zod and from Json Schema)

Screenshots

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

Copy link

linear bot commented Sep 2, 2024

@ainouzgali ainouzgali changed the title feat: controls validation feat(framework,novui): controls validation Sep 2, 2024
Comment on lines 64 to 70
const nestedPath = err.instancePath.split('/').join('.');
const requiredFieldName = err.params?.missingProperty as string;

return {
path: err.instancePath,
message: err.message as string,
property: `${nestedPath}${requiredFieldName ? `.${requiredFieldName}` : ''}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For missing required fields, we have to manually had the field name. It is not part of the instancePath. Otherwise we won't show those "required" missing errors on the input

@@ -6,6 +6,7 @@ export type ValidateFunction<T = unknown> = AjvValidateFunction<T> | ((data: T)

export type ValidationError = {
path: string;
property: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mapping of the validation error to the schema object uses the property attribute with . and not /

I was wondering why we need the path @rifont ? should we maybe have only one of those (path/property) and where needed change one if those strings?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're absolutely right, we should use just one of these. My preference would be to modify the existing path property to use the .-separated syntax as I feel the name is better suited to describing a full path, where property is more singular.

Let's modify the validators to return .-separated syntax on the path property and remove the property entirely.

Copy link
Contributor

@antonjoel82 antonjoel82 left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines +57 to +64
// adding a default error for the case of errors in an array not updating correctly
return { __errors: ['enableShowExtraErrors'], ...toErrorSchema(errors || []) };
};

// Always return an empty array to prevent the default error messages from showing
function transformErrors(_: RJSFValidationError[]) {
return [];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 praise: Helpful comments, thank you!‏

libs/novui/src/json-schema-components/JsonSchemaForm.tsx Outdated Show resolved Hide resolved
Copy link

pkg-pr-new bot commented Sep 19, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/novuhq/novu/@novu/stateless@6428

commit: edd146f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants