Validation rules per field do not appear to be independent resulting in misleading UI feedback

[waywardz]

I'm submitting a bug report

Please tell us about your environment:

Operating System:
Windows 10

Node Version:
v6.9.1

NPM Version:
3.10.9

JSPM OR Webpack AND Version
webpack 2.3.3

Browser:
all

Language:
ESNext

Current behavior:
Validation when using a ValidationRules object doesn't behave as I'd expect. Given this example from the doc hub

ValidationRules
  .ensure('email')
    .email()
    .required()
    .then()
    .satisfiesRule('emailNotAlreadyRegistered')
  .ensure('username')
    .required()
    .minLength(3)
    .maxLength(50)
    .then()
    .satisfiesRule('usernameNotInUse');

validation will run in two sequences when validate() is called. First to evaluate both email and username up to the then() statements, and then email and username for the two satisfiesRules. If username isn't present, and email() and required() are valid on email, emailNotAlreadyRegisterd is not evaluated.

Expected/desired behavior:
I would expect each "ensure" to be handled independently, with two sets of sequence arrays per ensured attribute. As an example, email would have a sequence[0] of email, required, and a sequence[1] of satisfilesRule('emailNotAlreadyRegistered')
username would have a separate sequence set, sequence[0] of required, minlength, maxlength, and sequence[1] of satisfiesRule('usernameNotInUse');

Even if username is not completely evaluated, email should be validated as completely as possible.

What is the motivation / use case for changing the behavior?
If the top level validation (sequence[0]) failed for field1 and succeeds for field2, but the second level of validation (sequence[1]) would fail for field2, the user isn't alerted to the failure until they fix the failure with field1, even though they don't have any reason to be linked. This is a poor user experience.

[jsobell]

When I came up with the requirements for validation I intended there be two concepts of then(). One is as described in the documentation (which is what you expected to work), and the other was the fact that there may be 'groups' of rules that should not be executed unless other rules have passed. What you're describing suggests this might also be happening on the current then function.
In your usage scenario the current process makes sense from a performance point of view, as you won't be getting any server calls unless both email and username are valid, but perhaps there may not currently be a means of doing what you're asking.
Should we have then() apply only to the current property, and a function .sync() to make all the validation rules sync at that point?

So this would call the server once if email is valid and username is invalid:

ValidationRules
  .ensure('email')
    .email()
    .required()
    .then()
    .satisfiesRule('emailNotAlreadyRegistered')
  .ensure('username')
    .required()
    .minLength(3)
    .maxLength(50)
    .then()
    .satisfiesRule('usernameNotInUse');

and this would call the server twice, but only if both email and username are valid

ValidationRules
  .ensure('email')
    .email()
    .required()
    .sync()
    .satisfiesRule('emailNotAlreadyRegistered')
  .ensure('username')
    .required()
    .minLength(3)
    .maxLength(50)
    .sync()
    .satisfiesRule('usernameNotInUse');

I'd also like to see an optional parameter sync(n) to group subsequent rules, so they are always chained based on their depth. That covers all aspects of partial validations preventing expensive subsequent validations being called.
Might this introduce any issues with nested rules? Does it matter?

[waywardz]

To extend on your examples, lets say we have this:

ValidationRules
  .ensure('email')
    .email()
    .required()
    .then()
      .satisfiesRule('myEmailRule')
    .sync()
    .satisfiesRule('emailNotAlreadyRegistered')
  .ensure('username')
    .required()
    .minLength(3)
    .maxLength(50)    
    .then()
      .satisfiesRule('myUsernameRule')
    .sync()
    .satisfiesRule('usernameNotInUse');

If email is valid and present, then myEmailRule will run regardless of the status of the username field. Likewise if username is present and satisfies the length requirements, then myUsernameRule will run regardless of the status of the email field.

Only when they have both satisfied all rules (including myEmailRule and myUsernameRule) will emailNotAlreadyRegistered and usernameNotInUse be called.

Am I understanding what you're saying correctly?

[jsobell]

Yes, that's what I'm suggesting.
The number of combinations of validation rules is huge, and my original idea was to have the concept of 'levels' of validation, so expensive operations could be delayed until cheap ones were passed.
However, sometimes we want an expensive one anyway, and sometimes we don't know if an operation will be expensive or not, so we have to leave all of these conditional decisions to the developer and not make assumptions.
The situation is complicated further by the fact that validations can be nested, where an 'Address' component embedded in a 'Registration' form. The Registration form has lots of validation rules on fields, but the embedded address input component provides its own rules. This is why the current solution uses that strange method of instantiating the ValidationController; there may already be one in play at the parent level.
One issue with the sync() suggestion is that if this is a child form, does it sync with the parent rules, or only within it's own local set of rules? Also, do enough people need the concept of synchronising the validation rules to justify coding it in?
In your case you don't want the syncing to occur, but if we fix any then() syncing issue have we upset things for other people who do want then synced?

[RomkeVdMeulen]

See also #279 and #424. Fine-grained control over rule execution sequence is hard to get with the fluent API. Some other solutions that will work right now:

• Tag rules that require extra work like backend requests and execute those separately
• Put such rules in a different ruleset, keep a reference to them and execute them separately

A more comprehensive solution would be ideal, but I'm not sure what that should look like.

[jdanyow]

Does this work for you:

ValidationRules
  .ensure('email')
    .email()
    .required()
  .ensure('username')
    .required()
    .minLength(3)
    .maxLength(50)
  .then()
  .ensure('email')
    .satisfiesRule('emailNotAlreadyRegistered')
  .ensure('username')
    .satisfiesRule('usernameNotInUse');

[jsobell]

I'm not keen on this syntax for the concept of then. It's ambiguous from a readability point of view, and there's no way to dynamically add rules because you can't specify at which level each was added.
The fluent syntax looks awesome, but doesn't work well for anything with sub-grouping, as indentation is required to fathom out the developers intention.
Validation rules in their simplest form are a parallel set of local checks. In their complex form they are a combination of blocks of parallel, sequential, and cross-dependent rules, resulting in more of a prioritised graph than a simple tree.

We don't need to implement everything nicely in the syntax, but we do need to try and avoid a situation where something is almost impossible.

The above example (and current syntax) doesn't allow nesting of rules. Simple nesting support might allow 'groups' of validation, each implementing the same 'rule' interface, so any combination of rules could be applied. Since each 'group' was a 'rule' it would also provide messages such as "The address is invalid" if any of the child properties was in error.
I brought this up last year, but at the time the focus was on base features only, so we agreed to leave it until later.

What if we have a 'fullname' rule that requires firstname and lastname be valid?

As a thought-provoker:

ValidationRules
  .ensure('email')
    .email()
    .required()
  .ensure('username')
    .required()
    .minLength(3)
    .maxLength(50)
  .ensure('firstname')
    .required()
  .ensure('lastname')
    .required()
  .then(v =>
    v.ensureObject()
      .satisfies(obj => check_server_first_last_valid(obj.firstname, obj.lastname))
      .withMessage("Server says First and Lastname are invalid")
  )
  .then(v =>
    v.ensure('email')
      .satisfiesRule('emailNotAlreadyRegistered')
    .ensure('username')
      .satisfiesRule('usernameNotInUse')
  ).withMessage("Invalid username/email combination")

I think this would address the sequential handling issues, and I suppose the default or 'implied' handling is parallel, although I'd prefer to see an explicit grouping of the parallel to remove visual ambiguity, perhaps with a group(v=>... construct?
You can also nest .then() conditions, making it far more flexible.

[jdanyow]

I meant does #438 (comment) solve the issue using the current release?

[waywardz]

I thought it did at first, but we switched over to using bindings vs objects at some point so I think that's why it seems like it works. I swapped back to what I had put in the original question (without object validation) and that also behaved as I expected.

I had to swap over to bindings vs objects because it seemed like the objects just randomly wouldn't be there, no idea why.

I still have a screen that is doing objects, I will put something together and try it out. I also owe you a gist for the components and validation.