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

Validation events... #318

Closed
jsobell opened this issue Sep 6, 2016 · 34 comments
Closed

Validation events... #318

jsobell opened this issue Sep 6, 2016 · 34 comments
Labels

Comments

@jsobell
Copy link
Contributor

jsobell commented Sep 6, 2016

There are times when it's helpful to know when the validation collection has been changed, for instance enabling/disabling buttons, and having custom error displays for specific rules/properties.
Is it worth adding an event to the controller class so VMs can register to it?
One use-case I was experimenting with was how to display a specific property's error code in the UI. Obviously the errors collection can contain the error, but I don't see an obvious way to validation message for property "A", as without a Signal the message display fields won't refresh.
I suppose we could tell people to add another validation renderer to generate a signal, but it's all a bit inelegant.

@jdanyow
Copy link
Contributor

jdanyow commented Sep 6, 2016

I'm open to adding events to the controller. We even have a decorator we could apply that would automatically add the publish, subscribe, etc events to the controller: https://github.com/aurelia/event-aggregator/blob/master/src/index.js#L133

One question about the use-case part of your comments:

One use-case I was experimenting with was how to display a specific property's error code in the UI. Obviously the errors collection can contain the error, but I don't see an obvious way to validation message for property "A", as without a Signal the message display fields won't refresh.
I suppose we could tell people to add another validation renderer to generate a signal, but it's all a bit inelegant.

Did you try the validation-errors attribute? Docs for it here: http://aurelia.io/hub.html#/doc/article/aurelia/validation/latest/validation-basics/6
It's job is to gather the errors related to a specific element or set of elements for binding purposes.

@jsobell
Copy link
Contributor Author

jsobell commented Sep 6, 2016

Yes, I played with that, but it requires the presentation information be duplicated from the renderer into the form for those fields.
I was trying to find a way of detecting validation changes from code, so any error can be processed when it changes, be it to enable a button or display error information.
Also, the field may not be on the UI at the time of validation.

Re the Event Aggregator, I don't think this is suitable for validation events, as they are not global events. If the same concept could be applied to the scope of the ValidationController, great, but there is no need for the entire application to know when one field has validated.
Perhaps we need the concept of either adding/removing function callbacks to the controller, or adding features to the renderer interface to support additional status changes?

@jdanyow
Copy link
Contributor

jdanyow commented Sep 7, 2016

I agree, they are not global events. The decorator I linked is a means of reusing the event aggregator logic on a class instance. Any class whose instances need to expose an event api (publish + subscribe) can use the decorator to "mix-in" the functionality.

@jsobell
Copy link
Contributor Author

jsobell commented Sep 7, 2016

Blog for you to check for accuracy. It tests the concept nicely, although (as I mention in the blog) it doesn't expose enough of the operation for all scenarios.
http://www.sobell.net/aurelia-validation-events/

@jdanyow
Copy link
Contributor

jdanyow commented Sep 26, 2016

@jsobell nice- a renderer that renders events. Should this issue remain open- have you been seeing a lot of questions related to validation events?

@jsobell
Copy link
Contributor Author

jsobell commented Sep 26, 2016

No, not many, so let's close it for now.
I'll have to update the blog post at some point for the new reason property.
The only thing not currently possible is any kind of event to indicate overall validation success or failure, which would probably be the most useful event. We'll see how we go, but it may be worth considering passing that as a status to the renderer at some point, as I can imagine that being one of the most commonly required events.
A simple more generic option is to pass in the errors array as a parameter to the renderer so it can look for length zero, etc. Do you want to add this as a new enhancement?

@jsobell jsobell closed this as completed Sep 26, 2016
@jsobell jsobell reopened this Sep 26, 2016
@jadrake75
Copy link

I actually ran into this today... I had three "components" which were small forms that represented child models of a parent model. Each component had validation setup to ensure the "value" was a numeric entry the "cat number" was required and less than 25 characters etc. However the editor that has these three components should only be enabled for save when the components are valid. Since I am using validateTrigger= validateTrigger.change, validation was correctly validating the fields when I was changing stuff, but unless I manually invoked validationController.validate() - which I do on initialization only (in a _.defer()) there was no place to hook into the promise (besides that would be a double validation). Instead I used the bindingEngine to listen to collectionObserver changes on the validationController.errors but this really only lets me know when there is a change in the errors of validation. So for example, on initial load, if the validator fires on binding, and there are no errors there is no way to "inform" that everything is "ok". The collectionObserver handler currently emits an event (which can be caught by the editor) that will check the validation state passed to see if all three are valid and if so trigger the buttons to not be disabled. My assumption of course was that forms were inherently invalid, so perhaps switching this to be assumed valid and wait for the validator to fail would be cleaner.

Having a "hook" to attach a callback (ala listener pattern) on validation would be helpful here. I suppose an alternative I may explore is rather than raising events out of my components is to have my components have a valid state flag and set that in the collectionObserver handler internally, and then have my editor listen for changes in the component valid states with the normal variableChanged( ) functions. Either way applying a callback would be a lot easier to handle more cleanly... something like

this.validationController.onValidate = function() { // do stuff }

@jsobell
Copy link
Contributor Author

jsobell commented Sep 28, 2016

@jadrake75 There is this that partially addresses what you mention:
http://www.sobell.net/aurelia-validation-events/

@dkent600
Copy link
Contributor

  1. using ValidationRenderer to publish events violates SRP. As such it constrains event publishing functionality to ValidationRenderer functionality.

  2. Using the nifty EventAggregator.includeEventsIn doesn't accomplish much unless the instance already publishes the desired events.

My current use case:

I am validating at the object level, thus must manually call ValidationController.validate(). At that point I can manually publish an event myself. But if one is doing property-only validation, then validation can happen entirely under the hood, and some kind of built-in event or callback mechanism can then be desired.

@jadrake75
Copy link

@jsobell I have to admit that approach would be cleaner in my case. I never considered it because in my head "renderers" are for rendering stuff (I think this is sort of @dkent600 comment (1) above) - for my code I am willing to use it and eliminate my bindingEngine watching on the errors collection (it would be cleaner)

But I think the core issue is, is a workaround like above sufficient (even though mentally non-intuitive at first glance) vs. a cleaner way to hook into the validation without manually invoking it (and thus having a promise you can respond to)

@jsobell
Copy link
Contributor Author

jsobell commented Sep 28, 2016

@jadrake75 No matter what you do you will always have to assign a validation controller to an object, whether through bindings auto-linking them or code doing it explicitly.
The ValidationRenderer name might be considered a bit misleading, as it doesn't actually have to render anything, bit is really a callback method for any controller.

I'm not sure what there might be as a 'cleaner' way of doing this. We could have events (callbacks) on the controller itself, but that's exactly what the ValidationRenderer is; a predefined set of callback hooks.

It's also not practical to have the objects themselves raise events, as an object shouldn't have any additional data related to validation status, and this is why things like change tracking are difficult.
In theory we could add .pristine, .valid, .changed, and a load of other attributes to every single property of every object, and we could add callbacks of modified and validationChanged to every objects field, but we'd be creating a monster. Take a look at many ORM systems with self-managed objects to see how horrendously complex it gets.

Mixing UI validation with object validation is always going to be a compromise, but underlying the whole thing is the fact that the object itself has priority over the UI. Sure, you can have an object with validation on field 'X', and if you forget to make 'X' editable on-screen you can never save the object.
But that's absolutely correct. If 'X' is invalid, you can either ignore that error in your back-end code (implying you're happy to allow an invalid field) or you fix this at design time.

My belief is that everything validation-wise has to be object based, and the UI is a secondary assistance layer to make it pleasant for users. If you mess up your UI stuff, the underlying system will still prevent you trying to submit invalid data. As such, anything related to validation extension needs to apply at the model layer, and because we don't extend models for validation, that means it has to be applied to the validation controller.
This is also important given that validation also needs to function server-side, where no UI exists, and this system needs to work in both situations (even if Aurelia isn't fully server-side compliant yet)

Any ideas you have for ways to improve this stuff, please do post them all in here. I'm not arguing against any of your suggestions, simply trying to explain why some of the stuff has taken the direction it has, and if you have ideas that improve, extend, or find fault in anything as it stands we most definitely want to hear them!

@jsobell
Copy link
Contributor Author

jsobell commented Sep 28, 2016

@dkent600

  1. using ValidationRenderer to publish events violates SRP. As such it constrains event publishing functionality to ValidationRenderer functionality.

Possibly, although a second ValidationEventHandler object would probably have the same signature. DRY vs SRP... :)

  1. Using the nifty EventAggregator.includeEventsIn doesn't accomplish much unless the instance already publishes the desired events.

Obviously, includeEventsIn adds two methods dynamically to an object, which is why I don't like it, but it's also not a magical event generator, simply a pub-sub creation feature. In pub-sub you always have to add the pub calls.

My current use case:
I am validating at the object level, thus must manually call ValidationController.validate(). At that point I can manually publish an event myself. But if one is doing property-only validation, then validation can happen entirely under the hood, and some kind of built-in event or callback mechanism can then be desired.

So if you're doing property-only validation, what is the issue with the example I posted? Does this not get triggered when you validate a property?
Sorry, I'm probably being a bit slow here...

@dkent600
Copy link
Contributor

dkent600 commented Sep 29, 2016

@jsobell

So if you're doing property-only validation, what is the issue with the example I posted? Does this not get triggered when you validate a property?

To be clear: I am doing object-level validation. But if one is doing property-level validation, then I would prefer not to ask the user to overload the ValidationRender. I would prefer to see a feature change whereby the ValidationController or some-such entity publish the desired events as a system entirely independent from rendering.

I don't think that the validation modules needs to provide a ValidationEventHandler that you referred to. The user can write their own if that is how they would like to handle the published events.

But if the ValidationRenderer could be renamed to be more general and be made to truly handle all the types of events, then that seems OK with me.

@jsobell
Copy link
Contributor Author

jsobell commented Sep 29, 2016

That's where I'm a bit confused. The ValidationRenderer is not related to the UI, it's a service hook that can be used for the UI (typically for rendering).
If someone is doing property level validation exactly the same solution applies, and you simply add an instance of the event generator to the ValidationController instance. Even with property level validation you have to have a ValidationController instance, and you can only validate a property as part of an object, not in isolation.

How else might you see this being done? Are you saying that you think there should be some other type of events generated by something? Can you give an example?

@dkent600
Copy link
Contributor

dkent600 commented Sep 29, 2016

Given the name "ValidationRenderer" and all the documentation covering it, it does appear to have been intended to be used in relation to rendering the GUI. It makes sense to me that going forward that intent should be respected, considering that other GUI-related functionality may be desired in the future, and that if this subsystem were also responsible for presenting events to the programmer, then one might start to encounter contention between the two purposes. This is the whole SRP thing.

So yes, I was suggesting that a separate subsytem be used to present events to the developer. I'm not suggesting how that would be, just that it makes sense from a SRP point of view. In my mind, a pure event system (such as in what EventAggregator does) makes sense, but I'm not stuck on that by any means.

There is also the question of whether ValidationRenderer, in its current state, is actually representing
all of the events we might want to see, and all of the information we might like about each event. I haven't thoroughly analyzed that (except for the feature request I made today). Did you (@jsobell) allude in your blog post to something being missing?

@jadrake75
Copy link

I would agree with @dkent600. The intent might not have been to have the ValidationRenderer only be a UI construct but the name implies it. It doesn't help that the main method of it is "render". A cleaner convention would be to have a ValidationResultHandler with a "process" or "evaluate" method. ValidationRenderer could either be an extension of this. My 2cents

@jadrake75
Copy link

jadrake75 commented Sep 29, 2016

I just wanted to comment about the use of the EventAggregator extension pattern (I implemented it and testing around with it). So now I understand the comment better of @dkent600 about object validation. I am in the same boat. I want to know when the validators on my object are clear (or faulty) as a set not as they are validated. So hooking into the renderer lets me know when "a" validation has changed, but it does not allow me to evaluate the overall validation status of the controller. For example, using a case mentioned earlier, if you had a controller with 5 validation rules on 4 fields assigned after evaluation of the fields (ie. completion of validate()) it would be good to do something about it. However unless you are manually invoking validate( ) there is no way to hook into the promise. Maybe that "is" the solution. Provide a way to provide the callback of the promise success and failure and invoke it with the caller of the validate within the code in the validation module.

To further this, in order to do what I needed, I had to pass the controller into the Renderer, which is kind of odd (I am sure this breaks a host of encapsulation rules) but it works....

`
export class ValidationEventHandler extends EventAggregator {

constructor(controller) {
    super();
    this.controller = controller;
}
render(instruction) {
    let msg = {
        valid: this.controller ? this.controller.errors.length === 0 : true
    };
    if( !msg.valid ) {
        msg.error = _.first(this.controller.errors)  // in case we want it (footer messaging etc)
    };
    this.publish('validation-status',msg);
}

}
`

@jsobell
Copy link
Contributor Author

jsobell commented Sep 29, 2016

@jadrake75 You mean as mentioned here: #318 (comment) ?
Yes, I see this as a useful feature too, and yes @dkent600, it's these events I was alluding too in my blog.

Re the nomenclature of the class, if we are to use it for non-UI related actions it would probably be better renamed, although most people will undoubtedly use it purely as a render management service, even to the extent of changing UI feedback on the basis of events.

But... you have to bear in mind that the validation list is inherently unsafe and inaccurate, and should not be used as an indication of model validity.

It's dangerous to assume that all because you have no errors in the collection that you object is somehow valid. Firstly, object level rules are not evaluated without a full .validate(), and secondly there may well be properties with rules dependent on other properties that will not auto-revalidate if a dependency changes. Password/confirmation equality is an example of this; if you update the password property, the confirm field will not be re-validated automatically, so from a model point of view you must perform a full validation before processing.
Oh, and of course there's always the async validation rules, requiring a Promise be resolved before you make any subsequent decisions, and no fields are flagged as invalid until a property level validation has been performed (which is why required fields don't show errors initially).

So I suppose the core question here is that if the intention is to use it for model validation, when are you likely to ever want to know the status without validating the whole model?

@jadrake75
Copy link

Excuse my misunderstanding... so if the validateTrigger is set to "change" you are not calling validate() internally which is analogous to a object model validation? You are only evaluating the rules for the defined model property? (I suppose that makes sense) That makes it a bit trickier since I figured there was a validation cycle on any change, but it sounds like it is only on the properties' change. So it sounds like the only way to evaluate and object model validation state would be via the promise of validate()?

@jsobell
Copy link
Contributor Author

jsobell commented Sep 29, 2016

validateTrigger refers to the UI trigger, hence why it has 'blur' as an option. Also, while it does call validate internally, it uses the single-property method because we don't validate an entire object when one field changes, as this would force all of your required fields to error on the first change, and would trigger all of the async server-side rules to fire too.

So even if there was a solution detecting changes to individual fields on an object, you still have all of the issues outlined above. It's impossible to predict every validation requirement, so the best we can do is to support object level fully, and make it as easy as possible to implement the UI validation.
At the moment you can ensure every single rule is valid on an object by calling validate(), and this should always be done before you use the data (and the UI should not allow updates to the model before you've used that data).

So yes, calling validate().then is indeed the only way to ensure an object is valid :)

@dkent600
Copy link
Contributor

@jsobell

So I suppose the core question here is that if the intention is to use it for model validation, when are you likely to ever want to know the status without validating the whole model?

I think it is valid to think of validation in terms of two major use cases, either property-level or object-level. To answer your question above, "when are you likely to ever want to know the status without validating the whole model", I think it would be when we have no rules that require whole-object validation.

This is precisely the use case that is most commonly presented in examples of the use of aurelia validation, especially when talking about the bindings. It is the use case that is most fleshed-out aurelia validation.

I have discussed this further here and here.

@dkent600
Copy link
Contributor

dkent600 commented Sep 29, 2016

@jsobell Further to your question "when are you likely to ever want to know the status without validating the whole model".

I can only think of one use case where one would not need to call validate, where 1) one has no multi-property dependencies, and 2) one is assured to always be starting from a valid object, and 3) one is persisting the object each time a single property is altered with a valid value. (I actually have this case in some places in my application.)

I can't think of any other case where one would not have to call validate() before persisting the object.

I think this supports my case for making sure that the ValidationRenderer (or whatever we call it) works well in the context of whole-object validation and multi-property rules, which it currently doesn't.

Of course, it also needs to work well when evaluating one property at a time, as it is currently primarily optimized to do.

@dkent600
Copy link
Contributor

BTW, the fact that the ValidationRenderer does not work well in the context of whole-object validation and multi-property rules was my original motivation for requesting an alternative event mechanism.

@jsobell
Copy link
Contributor Author

jsobell commented Sep 29, 2016

I think it would be when we have no rules that require whole-object validation

OK, but that's not what the whole-object validation refers to.
ensureObject is a means of saying "We don't know or care which properties are used by this rule, but run this rule anyway when the object is validated, and it can read whatever it likes out of it"
ensure(a => a.propertyname) is used to say "key this rule to this property name, so we can do useful and nice things like link it automatically in bindings and change events"
Both cases will be called when we perform a complete object validation, regardless of whether the rules are against individual properties or a condition involving multiple properties, as (for the many reasons mentioned earlier) we can't guarantee all rules match if we cache earlier results.

Remember that we sometimes define a custom rule on a property, for example

.ensure(a => a.lastName).satisfies((v,obj) => obj.firstName=='Fred')

Now if the user changes firstName from 'Fred' to 'Bob', this rule fails, but we don't know until we call validate() to re-validate every property, so the object/property validation difference you mention probably doesn't really exist.

We did discuss this in great detail when we were designing the solution, and the ideal would be to track dependencies between rules, but that is extremely expensive. An alternative is to allow the computedFrom style approach so the user can specify dependent properties, but this is also risky and requires dependency trees be tracked, making things potentially messy with predicates etc.
We also talked about hierarchies of rules, so not only do we have chaining of rules per-property, but we could also say that rule (or possibly property) X should not be called unless property Z has been validated. Again, it brings in lots of potential issues, and we know we can work around it for now by having multiple rules collections and chaining the .validate() Promises.

The use case you just added "one is assured to always be starting from a valid object" is probably not a typical one, as most people are simply asking for a valid email and a few required fields, but unless they are editing an existing (previously validated) record, the rules for required() will all fail.
The big risk here is that while you know that there are no multi-property rules, you don't know that a developer is not going to add a new important rule without realising you have this unusual assumption of correctness, and that's why we default to an assumption of invalid :)

Can you explain what you mean by

ValidationRenderer does not work well in the context of whole-object validation

Can you give an example of exactly what you would like to see happen? What events would you expect to see, and where? The current ValidationRender provides all failed rules, even if there's no element associated, so is that not what you wanted?
I know we don't have arrays automatically supported, although I seem to remember @jdanyow did a demo Gist with a validated Grid control

@dkent600
Copy link
Contributor

dkent600 commented Sep 29, 2016

@jsobell

unless they are editing an existing (previously validated) record, the rules for required() will all fail.

yes, that is # 2 of the use case. I think this use case is common when editing simple entities.

I'm not suggesting any changes based on that use case. I was simply answering your question to identify a use case where calling validate is not necessary, for what it was worth.

Regarding your other questions, can you please consult these: here and here.

Thanks

@jsobell
Copy link
Contributor Author

jsobell commented Sep 29, 2016

Yeah, I looked at those earlier.
#351 suggests propertyNames, which is similar to what I mentioned above re computedFrom(), but unless it's hooked into a dependency tracker we don't really gain anything from it other than using it as a metadata source to feed identifying information you might want associated with that rule.
There is also the tag() function to assign values to individual rules, so that can be used to hold your metadata. It's currently defined as a Typescript string type, but you can obviously put anything you like in there, including an array of associated property names.

I did mention adding a metadata property in here #328, but the tag property essentially already does that.

Your second post #279 (comment) seems to say the same thing, and again, I think tag() will solve that too. Try it and let us know.

@jdanyow
Copy link
Contributor

jdanyow commented Nov 30, 2016

@jsobell I think you had written a blog post I think on how to make an "event renderer". Does that cover the need or is there more we need to do here?

@dkent600
Copy link
Contributor

People are constantly asking: "I have a Save button that I want disabled until all fields have passed validation. Is there a way to check that the entire form is valid"? followed by: "How can I know when the validation state may have changed so that I can refresh the button enabled state accordingly?".

Clearly the answer to the first question can be something like "instantiate a Validator (via DI) and use it to validate the entire form without affecting the GUI."

The answer to the second question appears to have several potential answers, but the first choice appears to be: "check out ValidationRenderer.render because it is called whenever the validation state changes". This is a somewhat unintuitive answer to what seems like a simple question.

Ideally, I think, there would be a cleaner way of doing this. But at minimum it would be nice to document this answer clearly in the Aurelia docs, with some example code. This is a very common use case.

@jadrake75
Copy link

jadrake75 commented Jan 28, 2017 via email

@jsobell
Copy link
Contributor Author

jsobell commented Jan 30, 2017

The complexity is that because the objects are validated, not the form, we don't know at the form or validator level if a property has been validated or not.
As such, something somewhere has to say "is everything valid?", and we can't have the form doing that as it doesn't know what objects to validate, plus we may be firing off requests to the server. We may also have errors with no visual indication, because we have a validation rule on customerInCredit that requires it be non-zero, yet nothing on the form is linked to it, so the validation is never checked.

Essentially, enabling and disabling 'Save' buttons on a form is a risky business. If there is a server-side check to make before save, you can't make that check unless it's enabled. So if you leave it disabled until the rest of the fields are valid, the user will get an error when they click the enabled 'save', and will think "WTF? If it's invalid, why did it enable the button?"

Sure, there will be times when all you care about is whether all mandatory fields on the form are filled, but we don't provide a feature to enable it in those cases, as it will be misleading and inappropriate in other cases.
So remember that it's not a case of "Is the status exposed?", but of "When should I perform walk the validation chain?".
If you know you want simple synchronous field validation for a form and all your properties are visible, just implement your own call on the change or blur event to the validator and reflect the change yourself.
But don't cache the results of validation, as that may be more performant but is very dangerous from a data consistency viewpoint.

@jdanyow
Copy link
Contributor

jdanyow commented Mar 25, 2017

Closing- not a lot of requests for events at this time. We definitely can revisit in the future, should the need arise.

@jdanyow jdanyow closed this as completed Mar 25, 2017
@dkent600
Copy link
Contributor

I would just like to refer to my last comment above, where I noted that this use case is very commonly asked-about, and concluded "Ideally, I think, there would be a cleaner way of doing this. But at minimum it would be nice to document this answer clearly in the Aurelia docs, with some example code. This is a very common use case."

@jdanyow
Copy link
Contributor

jdanyow commented Mar 25, 2017

ok- re-opening 👍

@jdanyow jdanyow reopened this Mar 25, 2017
@dkent600
Copy link
Contributor

Whoa! That was some fast coding!

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

No branches or pull requests

4 participants