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

feat(Validation): Add validation-object-errors binding (with new propertyNames parameter for ensureObject().satisfies()) #351

Closed
dkent600 opened this issue Sep 28, 2016 · 18 comments

Comments

@dkent600
Copy link
Contributor

dkent600 commented Sep 28, 2016

Proposed Change

I have previously proposed adding a propertyNames parameter to ensureObject().satisfies() (see here) and here.

Further to that, I here propose the following:

Create a new binding like validation-errors called validation-object-errors for instance-level (rather than property-level) validation.

This new binding would have the following effects:

  1. it would change behavior of the inner validate binding by causing whole object to be validated on each property change (rather than a single modified property).

  2. in the ValidationRenderer, RenderInstruction would be filled-in with elements and property names for all the rules that have failed. (Again, see my earlier proposal and here.

(#279 (comment)) that would come in quite handy here).

Use Case

My form contains a table of rows, each of which displays an object instance that has been added to the controller for validation.

I am validating at the object level, thus must manually call ValidationController.validate(). I want to display validation errors separately for each row and thus want an easy way to identify errors associated with each object.

Each object contains object rules (defined by ensureObject().satisfies()). These object rules do not depend on other rules, but they do relate to a set of properties on the object. However, unlike with ensure().satisfies() it is not possible to associate ensureObject().satisfies() with a property name, much less with multiple property names.

Further, I am using neither the validate binding nor a ValidationRenderer. Again, I am manually calling ValidationController.validate(), and manually rendering the error information when validate completes.

A ValidationRenderer could be useful for me in this scenario, but it is not, for the following reasons:

  1. without using the validate binding, it does not provide the element(s) with which a property is associated.
  2. it provides neither property names nor elements for rules define by ensureObject().satisfies()
@jdanyow
Copy link
Contributor

jdanyow commented Sep 29, 2016

@dkent600 sounds like there's two parts to this.

  1. Creating something similar to validation-errors custom attribute to make it easier to bind to a specific object's errors. Maybe something like this:

    <div object-validation-errors="object: myObj, errors: myObjErrors">
  2. Associating specific dom elements with errors resulting from ensureObject(). Few questions for you on this part:

    1. How do you propose this would work?
    2. Why are you using ensureObject() instead of ensureProperty(...)?
    3. In your view is rule dependencies #279 the same issue?

@dkent600
Copy link
Contributor Author

dkent600 commented Sep 30, 2016

@jdanyow

1. Creating something similar to validation-errors custom attribute to make it easier to bind to a specific object's errors. Maybe something like this:

<div object-validation-errors="object: myObj, errors: myObjErrors">

Minor correction (see .bind):

<div object-validation-errors.bind="object: myObj, errors: myObjErrors">

Otherwise that looks good. To reiterate: this would change the behavior of the validate binding behaviors contained within the element decorated with object-validation-errors such that the validate binding behaviors would cause the whole object to be validated on each property change (rather than the single modified property)

2. Associating specific dom elements with errors resulting from ensureObject(). Few questions for you on this part:
i. How do you propose this would work?

I propose this would work by adding a parameter to satisfies that allows us to specify multiple property names upon which the rule or object will depend. This additional parameter would only be available when following ensureObject (not following ensure).

The validate binding behavior would also serve to link the property names with the associated DOM elements.

(Side note: I note that the satisfies callback is the same under ensure as it is under ensureObject. Thus in the ensureObject case both the value and instance parameters appear to be set to the value of the object instance. To reduce confusion, perhaps this could be reduced to a single parameter, that being value of the object instance?)

ii. Why are you using ensureObject() instead of ensureProperty(...)?

By "ensureProperty" do you mean "ensure"? I'm not familiar with "ensureProperty". I assume you mean "ensure" (and perhaps are thinking of renaming it?).

In any case, I'm not really clear about this question in general. My entire use case is based on validating the entire object. It seems like property-based validation is working pretty well in the context of this feature request. Are you thinking there is an improvement here that I haven't thought of?

iii. In your view is #279 the same issue?

Yes, it is exactly this (ie, part 2. of your question). Good idea to put this all together in one place.

@dkent600
Copy link
Contributor Author

A corrolary of this could be that when one calls:

ValidationController.validate({ object: instance, propertyName: name })

then all associated rules that reference the property would be evaluated. This would include object-level rules where the names of the implicated properties will have been given in the new version of satisfies requested above.

@jdanyow
Copy link
Contributor

jdanyow commented Nov 30, 2016

There might be a cleaner way of triggering validation of a whole object when any input is changed, blurred or both, depending on the behavior you're going for.

<style>
  .has-error > input {
    border-color: red;
  }
</style>
<form blur.capture="controller.validate({ object: person })"
      change.delegate="controller.validate({ object: person })"
      input.delegate="controller.validate({ object: person })" 
      style.bind="controller.errors.length ? 'has-error' : ''">

  <input value.bind="person.firstName">
  <input value.bind="person.lastName">

</form>

@dkent600
Copy link
Contributor Author

@jdanyow Hmmm, honestly, that doesn't look cleaner to me than simply adding <div object-validation-errors="object: myObj, errors: myObjErrors">

Further, it adds nothing without the enhancement to ensureObject that I proposed because field names and elements associated with rules defined after ensureObject would still not be identified to the renderer.

I think that with the changes I've proposed here, it would move object-level validation closer to, if not on par with, the existing property-level support.

@jdanyow
Copy link
Contributor

jdanyow commented Dec 1, 2016

I was thinking you'd only be using one of the blur.capture / change.delegate / input.delegate options. I included all three because I wasn't sure which flavor of validation trigger you wanted.

It's true this doesn't do anything with the property names stuff you mentioned. I need to get on the same page with you on this part. It would be extremely helpful if you could share some more user-story / concrete use-case kind of information so I can visualize how this is supposed to work. Even better if you could put together a gist with real class names, property names, rules, actual UI layout, etc, that I would be able to use when testing out implementation changes that attempt to meet the need.

@dkent600
Copy link
Contributor Author

dkent600 commented Dec 1, 2016

I'll see what I can do!

BTW, I can't for the life of me find a reference to "blur.capture". What is that? (I know what blur is, but a capture binding command?)

@jdanyow
Copy link
Contributor

jdanyow commented Dec 1, 2016

whoops- capture is a new binding command- I'm just realizing it's not even released yet (or documented yet: aurelia/binding#538).

What it does is enable you to use event delegation for events that do not bubble upwards (like blur) by subscribing to their "capture" phase. Check out "useCapture" here for more info.

Capture example you can experiment with: https://gist.run/?id=196c99652a28d571527a8fff63297308
Capture issue in binding: aurelia/binding#530

@dkent600
Copy link
Contributor Author

dkent600 commented Dec 1, 2016

@jdanyow

In reference to the Gist found here: https://gist.run/?id=7103145949f2b899337a635ff6b8ca74

First, I would like to enumerate the problems I see with using something like blur.capture="controller.validate({ object: person })" instead of <div object-validation-errors="object: myObj, errors: myObjErrors">:

  1. ValidationRenderers won't be informed about elements associated with properties that rules defined by ensureObject.satisfies might depend on. This is demonstrated in the Gist.

  2. it works outside the aurelia-validation framework, which for property-based validation supports the ValidationController.validateTrigger property to trigger validation. I feel that object-based validation should be able to work through the same mechanism. The proposed object-validation-errors attribute would invoke whole-object validation instead of property-specific validation, and that it be coordinated with ValidationController.validateTrigger.

  3. it doesn't support automatically putting errors into an array property like validation-errors does. You will see in the Gist that this lack is more significant when working with an array of objects, each being validated separately and each needing to have a distinct set of ValidationErrors.

In the Gist, you can see the following

  1. the array generated by validation-errors does not include the object-level validation errors, whereas the proposed object-validation-errors would, by definition.

  2. when the ensureObject validation rule fails, the renderer is not notified of the element(s) that caused the error, whereas with the changes I have proposed to ensureObject.satisfies, it would

  3. you can see I had to jump through some hoops to manually generate a list of object-specific errors from controller.errors. (Note that the dynamically-created computed observable controllerErrors will be polled by the binding engine. One of them for each instance in the array. I haven't quite figured out how to fluently define dependencies for a computed observable, particularly when the dependency is something outside of the object containing the computed observable property.)

I hope this helps!

@dkent600
Copy link
Contributor Author

dkent600 commented Dec 1, 2016

I think I now understand that there is an asymmetry in my proposal between validation-errors and object-validation-errors. I have proposed object-validation-errors in such as way as to affect the behavior of the contained validate binding behaviors, causing them to trigger object-level validation rather than propery validation. I seriously doubt that validation-errors plays a corresponding role in forcing property-level rather than object-level validation.

My main concern in this comment, however, is whether forcing object-level validation would actually be feasible for object-validation-errors to do, and whether you would be comfortable with such an entanglement of enclosing attribute with enclosed binding-behavior.

Maybe it would make more sense to add a new option on the validate binding behavior like objectValidation:true.

@dkent600
Copy link
Contributor Author

dkent600 commented Dec 1, 2016

Criteria for acceptance:

  1. ValidationRenderers are apprised of elements that are associated with validated properties whose validation rules are defined using ensureObject.satisfies

  2. Either validation-errors, or a new object-validation-errors, generates a list of messages that includes those from validation rules defined using ensureObject.satisfies

  3. A new object-validation-errors attribute, or a new option on the existing validation-errors attribute, or a new option on the validate binding behavior, causes object-level, or rather than property-level, validation to be triggered when the specified ValidationController.validateTrigger is tripped on elements having the validate binding behavior.

@jdanyow
Copy link
Contributor

jdanyow commented Dec 3, 2016

@dkent600 thanks for all your work here fleshing this out- the one thing I was really hoping to get was an actual use case. Something we could put in the docs for this. I don't think the first name "abc" thing fits the bill. I really want a get something that people can relate to because it will make the docs understandable. Not to mention it will allow me to wrap my head around what the need is and hopefully come up with a good implementation with the least amount of boilerplate.

@dkent600
Copy link
Contributor Author

dkent600 commented Dec 4, 2016

@jdanyow Thanks for your patience on this.

Ok, how does this do:

Case 1

You are using ensureObject.satisfies to define one or more rules. Thus you will be using object-level validation.

So when displaying validation errors you want to give feedback as to which GUI elements are complicit in breaking rules defined by ensureObject.satisfies.

Case 2

In addition to case 1, you are displaying a set of objects, each as a row in a table in your GUI. You are using a single validation controller to validate all of them, but validating one object at-a-time (it would be quite inefficient and awkward to use a separate validation controller for each object).

So when displaying validation errors pertaining to a specific row/object, you want to include messages from failed rules defined by ensureObject.satisfies that are uniquely associated with the specific object/row.

Case 3

In addition to either case 1 or case 2, you are saving changes to the entire object whenever the user makes a valid modification to a property, without requiring that the GUI include a "Save Changes" button.

So you want the validateTrigger to cause object-level rather property-level validation

@dkent600
Copy link
Contributor Author

@jdanyow

Man, I've really made a mess of this. Thanks for your patience! With some more work I've narrowed this down much more cleanly.

First, please refer to a new gist, here.

The fundamental issue:

Rules defined by ensureObject().satisfies() cannot currently be associated with any properties that the rule depends on. Thus:

  1. Validation messages from rules defined in ensureObject().satisfies() are never associated with a GUI element and thus cannot be rendered to the user via the ValidationRenderer mechanism.
  2. The validate value converter, when a validation is triggered on an associated property, never triggers the evaluation of rules defined by ensureObject().satisfies().

In both of the above cases a property can appear to be valid when in fact it is not.

The gist demonstrates these two issues in two separate use cases.

To address this I propose adding a propertyNames parameter to ensureObject().satisfies() .

(This was the original request. I think we can scrap the suggestion about a new object-validation-errors custom attribute).

@jdanyow
Copy link
Contributor

jdanyow commented Dec 30, 2016

@dkent600 no worries! I'll close this one out and we can continue tracking/discussing a mechanism for expressing a rule's property dependencies in #279.

Thanks for sticking with this one. I'll add your gist to #279.

@dkent600
Copy link
Contributor Author

@jdanyow

Just a post mortem comment: This resolution does leave hanging one feature I had requested here, that is, the ability to force automatic, full object-level validation, either by configuration in the HTML or the JavaScript API. As it stands now the user is always required to manually write code to execute an object-level validation.

I abandonned that idea when I realized that much (all?) of the need for such a feature would be addressed with the proposed change to ensureObject().satisfies() (see my last comment, above).

But I would not want to foreclose the possibility of this coming up again. If nothing else, just in terms of conceptual symmetry and elegance, it seems like a hole in the functionality. We'll see how it goes....

@dkent600
Copy link
Contributor Author

dkent600 commented Jan 21, 2017

Similar to my last comment just above (the ability to force automatic, full object-level validation), I think is this one #349

@dkent600
Copy link
Contributor Author

In a recent comment above, I described the following two cases, both of which I've solved using the existing Aurelia validation api:

  1. Validation messages from rules defined in ensureObject().satisfies() are never associated with a GUI element and thus cannot be rendered to the user via the ValidationRenderer mechanism.

  2. The validate value converter, when a validation is triggered on an associated property, never triggers the evaluation of rules defined by ensureObject().satisfies().

Each of these use cases is solved by defining the rule as a named custom rule, and applying the rule to the properties that depend on it.

This is demonstrated in the following gist: https://gist.run/?id=37835cb9165093a1c2deb2ea8113e287

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

No branches or pull requests

3 participants