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

Validation contexts #19

Merged
merged 14 commits into from Nov 17, 2015

Conversation

Projects
None yet
3 participants
@stallings
Copy link
Contributor

stallings commented Nov 13, 2015

Adds support for validation context that is backwards compatible with previous methods of validation.

For example a validation with context can be defined like this:

this.validate('name', {
  on: 'nameContext',
  validator: function() {
    if(this.name && this.name.length <= 5) {
      this.addError('name', 'must be greater than 5 characters');
    }
  }
});

And validation with context like this:

model.validateAttr('name', 'nameContext');
model.validate('nameContext');

@stallings stallings assigned peterwmwong and burrows and unassigned peterwmwong Nov 13, 2015

stallings added some commits Nov 14, 2015

Allows validation without context to run
attars without a context will run regardless of the context that is
supplied.
return !(name in this.ownErrors);
};

// Public: Runs all registered validators for all properties and also validates owned
// associations.
//
// ctx - The context in which to run validations

This comment has been minimized.

@burrows

burrows Nov 16, 2015

Member

Missing a period and need to indicate that its optional.

@burrows

This comment has been minimized.

Copy link
Member

burrows commented Nov 16, 2015

@stallings This is great, thanks for adding this much needed functionality. I'm not sure I like the new Model.validate API though. What if we just added an optional third argument?

this.validate('name', function() {
  if (this.name && this.name.length <= 5) {
    this.addError('name', 'must be greater than 5 characters');
  }
}, {on: 'nameContext'});

I think this is a more natural extension of the API.

if (typeof validator === 'function') { validator.call(this); }
else if (typeof validator === 'string' && validator in this) { this[validator](); }
else { throw new Error(`${this.constructor}#validateAttr: don't know how to execute validator: \`${validator}\``); }
let attrCtx = this.validators[name].find( (v)=> {return v.on && v.on === ctx} );

This comment has been minimized.

@peterwmwong

peterwmwong Nov 16, 2015

Contributor

Small Perf Issue: We are decreasing the performance of majority case (no context) by traversing the validators twice (here and in the else).
We could easily short circuit this first traversal by checking whether we have a ctx first, so...

let attrCtx = ctx && this.validators[name].find( (v)=> {return v.on && v.on === ctx} );
@stallings

This comment has been minimized.

Copy link
Contributor

stallings commented Nov 16, 2015

@burrows @peterwmwong Updated based on your comments: 54d8957

README.md Outdated
var invoice = new Invoice({name: 'foo'});
console.log(invoice.validate());
// true since there is no context provided

This comment has been minimized.

@burrows

burrows Nov 16, 2015

Member

Only true gets logged here so I'd only show that. The rest of the examples don't have explanations in the output.

else { throw new Error(`${this.constructor}#validateAttr: don't know how to execute validator: \`${validator}\``); }
let attrCtx = ctx && this.validators[name].find( (v)=> {return v.opts.on && v.opts.on === ctx} );
if(attrCtx) {
attrCtx.validator.call(this);

This comment has been minimized.

@burrows

burrows Nov 16, 2015

Member

This won't work if I specify a context and a method name instead of a function:

this.validate('foo', 'validateFoo', {on: 'something'});

this.prototype.validateFoo = function() {
};

This comment has been minimized.

@stallings

stallings Nov 16, 2015

Contributor

@burrows I've addressed this issue in this commit: 019b7b3

var m = new ValidatedFoo({name: 'FOO'});
m.validateAttr('name', 'nameContext');
expect(m.ownErrors.name).toEqual([ 'must be lower case', 'must be greater than 5 characters']);
});

This comment has been minimized.

@burrows

burrows Nov 16, 2015

Member

This spec is basically identical to the previous one. Change the first such that it only checks for the contextual validation error and the second to only check for the non-contextual one.

README.md Outdated
@@ -1098,6 +1098,31 @@ console.log(invoice.errors);
So even though the invoice object didn't have any validation errors, the `#validate` method returned
`false` because one of its owned line item object did have an error.

Transis also supports validation contexts. Multiple contextual validations can be registered for the same attribute.
Each contextual validation will executed depending on the context provided. It's important to note that all attributes

This comment has been minimized.

@burrows

burrows Nov 16, 2015

Member

Contextual validations will only be applied when a matching context value is provided to the Transis.Model.validate method.

@stallings

This comment has been minimized.

Copy link
Contributor

stallings commented Nov 16, 2015

@burrows Updated the specs and README: 594a03c

@stallings

This comment has been minimized.

Copy link
Contributor

stallings commented Nov 16, 2015

@burrows Updated: 4ebea0a

@burrows

This comment has been minimized.

Copy link
Member

burrows commented Nov 17, 2015

@stallings Looks good, thanks!

burrows added a commit that referenced this pull request Nov 17, 2015

@burrows burrows merged commit d3c1ed7 into master Nov 17, 2015

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