jurassix / react-validation-mixin

Simple validation mixin (HoC) for React.
MIT License
283 stars 38 forks source link

Fix for #38 #41

Closed boldt closed 8 years ago

boldt commented 8 years ago

This is a fix for the Issue #38 (error messages doesn't clear when validating array of items)

Observation: The result looks as follows:

data: Array[0]
data.0: Array[1]
data.1: Array[1]
data.2: Array[1]

If I make the array field valid, then the data.* are still set in the errors object. Because of this, this fix does not extends the stored errors object anymore. Instead, it rewrites them and Reacts setState() should still just update the diff.

Like this, errors for nested objects should be removedas well.

jurassix commented 8 years ago

Thanks for the pull request. Before I merge into master, I'll need to verify and update the existing tests to ensure this change is consistent with previous expected functionality.

jurassix commented 8 years ago

ok I've added test case to validate this PR. The test case will not pass with the current PR because we loose previous validations.

https://github.com/jurassix/react-validation-mixin/commit/38da4911ac197235cd68248f4b4e422f89a56677

jurassix commented 8 years ago

I think work done for #42 will also address this issue. Since this is really an issue with the strategy.

boldt commented 8 years ago

What exactly do you mean with "loosing previous validations"? I never used TestUtils, but what is the test you just added doing exactly?

jurassix commented 8 years ago

When a user adds onBlur={this.props.handleValidation('email')} to their input field we validate only that field and merge any errors back into the validation components state. If we replace the state on each validation then we loose the previous fields validation.

Example: Signup form

Render the Signup component and get DOM references to both the email and username fields.

const signup = TestUtils.renderIntoDocument(<Signup/>);
const email = findDOMNode(signup.refs.component.refs.email);
const username = findDOMNode(signup.refs.component.refs.username);

Simulate an onBlur event for email field; this will validate only the email field and return any new errors. If there were other errors in other fields we need to keep them. e.g. username is valid because it hasn't actually been validated yet.

TestUtils.Simulate.blur(email);
expect(signup.isValid('username')).to.equal(true);
expect(signup.isValid('email')).to.equal(false);

Now we simulate an onBlur event for username field; this will validate only the username field and return new errors. email is still invalid but the response coming from the validator only has new errors for the single field username. If we simply replace our state then we loose the previous field validation of email.

TestUtils.Simulate.blur(username);
expect(signup.isValid('username')).to.equal(false);
expect(signup.isValid('email')).to.equal(false);

So what this test establishes is that field level validation calls (handleValidations('field')) only validate single fields and not the entire form. You cannot simply replace state, you have to merge the this.state.errors object with the new errors, leaving the old ones in place.

boldt commented 8 years ago

I just do the validation of entire form with validation(). Your test uses handleValidationvalidation() to validate single fields. I see the dilemma.

I am looking forward to your fix. It seems, that all my reported issues are related.

jurassix commented 8 years ago

Exactly right. I believe all your issues are coming from my naive formatting of errors on the strategy side. I've made this priority to resolve and should have something out soon.

jurassix commented 8 years ago

This has been fixed with 5.2.0: https://github.com/jurassix/react-validation-mixin/releases/tag/v5.2.0 Ensure you upgrade joi-validation-strategy to v0.2.1