jurassix / react-validation-mixin

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

Password confirmation validation #18

Open skiwi2 opened 9 years ago

skiwi2 commented 9 years ago

Currently I'm using the following schema:

validatorTypes: function() {
    return {
        username: Joi.string().min(4).max(20).required().label(this.refs.username.props.label),
        email: Joi.string().email().required().label(this.refs.email.props.label),
        password: Joi.string().min(6).max(20).required().label(this.refs.password.props.label),
        confirmPassword: Joi.any().valid(Joi.ref('password')).required().label(this.refs.confirmPassword.props.label)
    };
},

However this is leading to situations which might be incorrect, but are definitely unwanted:

  1. When both password and confirmPassword are empty, then this.getValidationMessages('confirmPassword')[0] is empty.
  2. When both password and confirmPassword are matching but have an incorrect length, then this.getValidationMessages('confirmPassword')[0] is also empty.

How can I prevent these situations from occuring?

I have also opened an issue over at https://github.com/hapijs/joi/issues/652, but it seems to be that the issue is directly related to this library.

jurassix commented 9 years ago

@skiwi2 getValidationMessages will not validate the field, instead it pulls the messages from the last validation. You either need to add a change/blur handler to your input via onBlur={this.handleValidation('confirmPassword')} or call this.validate('confirmPassword', callback) directly to validate a field. You can also call this.validate(callback) to validate all forms. Once validation has been called on a field, you can check that field for errors.

skiwi2 commented 9 years ago

@jurassix I have called this.validate() already.

There is definitely something wrong with this.getValidationMessages().

jurassix commented 9 years ago

Can you paste a simple example so i can see your approach? On May 21, 2015 10:52 AM, "Frank van Heeswijk" notifications@github.com wrote:

@jurassix https://github.com/jurassix I have called this.validate() already.

There is definitely something wrong with this.getValidationMessages().

— Reply to this email directly or view it on GitHub https://github.com/jurassix/react-validation-mixin/issues/18#issuecomment-104307144 .

skiwi2 commented 9 years ago

I can't provide a runnable example as of now, but here's the snippets I use:

form = (
    <form onSubmit={this.handleSubmit}>
        <Input type="text" label="Username" bsStyle={this.validationStyle('username')} help={this.getValidationMessages('username')[0]} ref="username" onChange={this.customValidate.bind(null, 'username')} hasFeedback />
        <Input type="email" label="Email" bsStyle={this.validationStyle('email')} help={this.getValidationMessages('email')[0]} ref="email" onChange={this.customValidate.bind(null, 'email')} hasFeedback />
        <Input type="password" label="Password" bsStyle={this.validationStyle('password')}  help={this.getValidationMessages('password')[0]} ref="password" onChange={this.customValidate.bind(null, 'password')}  hasFeedback />
        <Input type="password" label="Confirm password" bsStyle={this.validationStyle('confirmPassword')}  help={this.getValidationMessages('confirmPassword')[0]} ref="confirmPassword" onChange={this.customValidate.bind(null, 'confirmPassword')} hasFeedback />
        <br/>
        <ButtonInput type="submit" bsStyle={this.validationStyle()} value="Sign Up" />
    </form>
);

Where the interesting function is customValidate:

customValidate: function (field, event) {
    return function (field, event) {
        var value = event.target.value;
        this.state[field] = value;
        var stateDictionary = {};
        stateDictionary[field] = value;
        this.setState(stateDictionary);
        this.validate();
    }.bind(this)(field, event);
},
jurassix commented 9 years ago

I think you are executing the customValidate method instead of returning a bound function. Thy this:

function customValidate(field, event) {
    return function (field, event) {
        var value = event.target.value;
        this.state[field] = value;
        var stateDictionary = {};
        stateDictionary[field] = value;
        this.setState(stateDictionary);
        this.validate();
    }.bind(this, field, event);
}
skiwi2 commented 9 years ago

Tried it, but doesn't seem to work.

jurassix commented 9 years ago

Ok I looked at your onChange handler, which you are binding, you should try this instead:

function customValidate(field, event) {
        var value = event.target.value;
        this.state[field] = value;
        var stateDictionary = {};
        stateDictionary[field] = value;
        this.setState(stateDictionary);
        this.validate();
}
skiwi2 commented 9 years ago

That function indeed seems to work and is clearer in intent, however it doesn't fix said issues unfortunately.

jurassix commented 9 years ago

Your inputs also need a `value={this.state.password}' for each onChange handler. (use the correct field for each input)

jurassix commented 9 years ago

It's also not very clear what your doing here? It seems you are updating state directly and then creating a new object, updating it, and wiping out your previous state?

this.state[field] = value;
var stateDictionary = {};
stateDictionary[field] = value;
this.setState(stateDictionary);

Try this for now:

this.state[field] = value;
this.setState(this.state);
skiwi2 commented 9 years ago

Sorry, but I do not see anything wrong with my methods here, they work as provided right now.

Have you been able to duplicate this bug on your end?

Also, from what I have heard from the Joi team, their intention is for Joi to be an API-validation library and therefore they do generate double error messages when the password does not match the confirm password and vice versa, because the overall form validation fails at that point.

I'm under the impression that with this library the goal is that this.getValidationMessages(key) always gives all messages concerning that key.

So if, like in my case, password and passwordConfirmation don't match, then I expect to see a valid error message for both calls.

Are these assumptions correct?

jurassix commented 9 years ago

The way you have written your code, calling `this.validate()' will validate all fields and provide error messages available for your fields correctly to your assumptions..

jurassix commented 9 years ago

I do believe the issue to be in your component, I have this library working on many forms. I'm seeing some inconstancies in your code that I'm pointing out above. This library works by by validating your state against your Joi schema, and adding and errors to you state. getValidationMessages simply pulls messages out of this.state.errors object. Can you console log the state object during render and see if it is correct.

skiwi2 commented 9 years ago

Giving all blank input on all 4 fields, I get the following output:

Object {username: Array[2], email: Array[2], password: Array[2], confirmPassword: Array[0]}confirmPassword: Array[0]length: 0proto: Array[0]email: Array[2]0: ""Email" is not allowed to be empty"1: ""Email" must be a valid email"length: 2proto: Array[0]password: Array[2]0: ""Password" is not allowed to be empty"1: ""Password" length must be at least 6 characters long"length: 2proto: Array[0]username: Array[2]0: ""Username" is not allowed to be empty"1: ""Username" length must be at least 4 characters long"length: 2proto: Array[0]proto: Object

(Sorry for the bad formatting, but it should show the result)

Here it's visible that the confirmPassword field has no errors, while I would expect there to be errors.

jurassix commented 9 years ago

Ok I understand. Once you start typing in the password field a error message should appear for confirm. Basically joi ignores the ref and focuses on the required validation on password field. On May 21, 2015 2:13 PM, "Frank van Heeswijk" notifications@github.com wrote:

Giving all blank input on all 4 fields, I get the following output:

Object {username: Array[2], email: Array[2], password: Array[2], confirmPassword: Array[0]}confirmPassword: Array[0]length: 0proto: Array[0]email: Array[2]0: ""Email" is not allowed to be empty"1: ""Email" must be a valid email"length: 2proto: Array[0]password: Array[2]0: ""Password" is not allowed to be empty"1: ""Password" length must be at least 6 characters long"length: 2proto: Array[0]username: Array[2]0: ""Username" is not allowed to be empty"1: ""Username" length must be at least 4 characters long"length: 2proto: Array[0]proto: Object

(Sorry for the bad formatting, but it should show the result)

Here it's visible that the confirmPassword field has no errors, while I would expect there to be errors.

— Reply to this email directly or view it on GitHub https://github.com/jurassix/react-validation-mixin/issues/18#issuecomment-104375790 .

skiwi2 commented 9 years ago

Correct, once I start typing that happens. But when both are equal, then confirmPassword is always correct.

Apparently this behavior is completely acceptable for Joi as they focus on full-form validation, but for a client-side validator which can be included on a per-key basis, I would expect to also see an error message on confirmPassword.

jurassix commented 9 years ago

I'm not sure I'll be able to support this, but I'll leave the issue open and do some discovery to see if it's possible. On May 21, 2015 2:20 PM, "Frank van Heeswijk" notifications@github.com wrote:

Correct, once I start typing that happens. But when both are equal, then confirmPassword is always correct.

Apparently this behavior is completely acceptable for Joi as they focus on full-form validation, but for a client-side validator which can be included on a per-key basis, I would expect to also see an error message on confirmPassword.

— Reply to this email directly or view it on GitHub https://github.com/jurassix/react-validation-mixin/issues/18#issuecomment-104377299 .

idealistic commented 8 years ago

Any updates on this one? I have the same issue where the validation for confirmPassword is not working properly. confirmPassword only errors out when it doesn't match password but it is passing validation when it is empty when it shouldn't since it has a required() validation.

validatorTypes: function() {
    return {
        password: Joi.string().min(6).max(20).required().label(this.refs.password.props.label),
        confirmPassword: Joi.any().valid(Joi.ref('password')).required().label(this.refs.confirmPassword.props.label)
    };
},
jurassix commented 8 years ago

@idealistic I can pick this back up. Wasn't sure the priority of this. Thx for the update.