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

Fix for clearing errors after updating and submitting form #2536

Merged
merged 5 commits into from
Sep 10, 2021

Conversation

newt10
Copy link
Collaborator

@newt10 newt10 commented Sep 2, 2021

Reasons for making this change

There is a bug in the core form (and other variations) where by the form continues to render errors even after the user has fixed them and submitted the form.

The reason this happens is as follows, when the user submits the form, onSubmit handler is called. If validation is enabled then, the handler runs validation functions to check for errors,

      let schemaValidation = this.validate(newFormData);
      let errors = schemaValidation.errors;
      let errorSchema = schemaValidation.errorSchema;
      const schemaValidationErrors = errors;
      const schemaValidationErrorSchema = errorSchema;

If errors are generated then the form State is updated with the new errors

        this.setState(
          {
            errors,
            errorSchema,
            schemaValidationErrors,
            schemaValidationErrorSchema,
          },

If errors are not generated then the state is updated as follows

    this.setState(
      { formData: newFormData, errors: errors, errorSchema: errorSchema },

As we see in the later state update schemaValidationErrors and schemaValidationErrorSchema are not being updated.

This creates a problem because the state for the form is derived using the above values as seen here

    const getCurrentErrors = () => {
      if (props.noValidate) {
        return { errors: [], errorSchema: {} };
      } else if (!props.liveValidate) {
        return {
          errors: state.schemaValidationErrors || [],
          errorSchema: state.schemaValidationErrorSchema || {},
        };
      }
      return {
        errors: state.errors || [],
        errorSchema: state.errorSchema || {},
      };
    };

What this does is that when the last error is fixed on re-submission of the form, the state is calculated from the stale values of schemaValidationErrors & schemaValidationErrorSchema

We fix this by updating the schemaValidationErrors & schemaValidationErrorSchema in the state when form is submitted with errors fixed.

If this is related to existing tickets, include links to them as well. Use the syntax fixes #[issue number] (ex: fixes #123).

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

Tests

Failing test due to this bug

Test Fail

Passing test after fix

Tests Passing

@newt10
Copy link
Collaborator Author

newt10 commented Sep 2, 2021

This also addresses the PR #2123 with addition of tests.

@newt10 newt10 linked an issue Sep 2, 2021 that may be closed by this pull request
3 tasks
@newt10 newt10 added this to Ready for Review in Prioritized PRs Sep 3, 2021
@newt10 newt10 removed the request for review from jimmycallin September 3, 2021 21:24
@newt10 newt10 added the core label Sep 3, 2021
dependabot bot and others added 3 commits September 6, 2021 00:25
Bumps [tar](https://github.com/npm/node-tar) from 4.4.16 to 4.4.19.
- [Release notes](https://github.com/npm/node-tar/releases)
- [Changelog](https://github.com/npm/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v4.4.16...v4.4.19)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
errorSchema: {},
schemaValidationErrors: [],
schemaValidationErrorSchema: {},
});
});
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a UI test like

expect(node.querySelector("#root_children_0_name")).to.not.exist;
Simulate.click(node.querySelector(".array-item-add button"));
expect(node.querySelector("#root_children_0_name")).to.exist;
that tests the exact scenario for the user that this PR fixes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added new UI tests as requested checking for generated HTML

packages/core/src/components/Form.js Show resolved Hide resolved
Prioritized PRs automation moved this from Ready for Review to Waiting For Merge Sep 10, 2021
Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

Address these changes and then feel free to merge

@newt10 newt10 merged commit 8bc7106 into rjsf-team:master Sep 10, 2021
Prioritized PRs automation moved this from Waiting For Merge to Resolved Sep 10, 2021
@newt10 newt10 deleted the fix/formErrors branch September 10, 2021 23:01
@newt10 newt10 restored the fix/formErrors branch September 10, 2021 23:01
@newt10 newt10 deleted the fix/formErrors branch September 10, 2021 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Bug: validation errors stay on screen onSubmit even after fixing the errors.
2 participants