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

WIP - Fix Nested Validation #4306

Closed
wants to merge 1 commit into from
Closed

Conversation

djabarovgeorge
Copy link
Contributor

What change does this PR introduce?

The PR introduces WIP of what we need to do in order to fix the nested validation that currently is broken. In order to add nested validation to the variants we will need to do the following:

  • Create a custom validation decorator.
  • Fix the nested validation that does not work at the moment. In order to fix it we need to do the following add ValidateNested decorator, if it is an array we should add each as well (@ValidateNested({ each: true })), then add @Type(() => NotificationStepVariant) decorator from class-transformer in order to map the type implicitly.
  • The hardest part IMO will be to return the nested errors that we will get, at the moment we throw an exception on the first layer, but it an error is not thrown we do flat on the first layer on BaseComman which won't return us the nested constrains.

Why was this change needed?

So we could validate nested objects and return a valid error if validations had errors.

Other information

In this PR I am adding nested validation only for variants as POC, once we merge this PR we most definitely need to validate all of the rest ValidateNested decorators.

DOD

  • Add unit tests for the extractConstraints function.
  • Add tests of command in order to check that the new logic works with it as expected.
  • Add max validation depth in order to not over-compute extractConstraints if the input is unexpectedly long or infinite.

Base automatically changed from vatiants-polishing to variants-condition October 2, 2023 08:51
Copy link
Contributor

@ainouzgali ainouzgali left a comment

Choose a reason for hiding this comment

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

@djabarovgeorge thanks for doing this!! Left a couple of comments

uuid?: string;

@IsOptional()
@IsBoolean()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@IsBoolean()
@IsString()

Comment on lines +51 to 53
@ValidateNested({ each: true })
@Type(() => NotificationStep)
steps: NotificationStep[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be added in update-notification-template.command as well

@github-actions github-actions bot added the stale Pull Request that needs to be reviewed label Oct 17, 2023
Base automatically changed from variants-condition to next November 27, 2023 14:54
@github-actions github-actions bot removed the stale Pull Request that needs to be reviewed label Nov 27, 2023
@github-actions github-actions bot added the stale Pull Request that needs to be reviewed label Dec 11, 2023
Copy link

github-actions bot commented Apr 7, 2024

This PR is being closed due to inactivity. Please reopen if work is intended to be continued.

@github-actions github-actions bot closed this Apr 7, 2024
@rifont rifont deleted the fix-nessted-varitants branch November 7, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@novu/api stale Pull Request that needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants