aurelia / validation

A validation plugin for Aurelia.
MIT License
132 stars 128 forks source link

Support 'foreach' #10

Closed janvanderhaegen closed 8 years ago

janvanderhaegen commented 9 years ago

Desired:

this.validation = validation.on(this)
  .ensure('orderItems')
    .minLength(1)
  .ensureForEach('orderItems')
    .ensure('qty')
       .minimumValue(1);
```  .
janvanderhaegen commented 9 years ago

Additional scenario and thoughts by @charlespockert:

In the application I'm working on, the users need to be able to create "alerts" (it's a front end for some monitoring software we have). Each alert has a "type" and each type has a different set of metadata associated with it (the metadata is simply a list of expected properties that require values on a particular type of alert).

I started with creating a view for each type of alert in which I created an input for each property and bound it to the metadata property on my alert model. I realised that the users would likely request a new alert type to be added (this may require some work in the back end to handle it or may not - but I'd rather cut down on the amount of work I have to do so I don't want to touch the front end if possible) and that since I was already holding a list of expected metadata in the database I could be more flexible by generating the inputs in a single view.

I now repeat over the expected property names and bind each input to the model using indexer syntax e.g:

<!-- repeat.for="property of requiredMetadata" wraps these inputs -->
    <input value.bind="$parent.alert.metadata[property.Name]" validate="${ property.Name }" validate.bind="property.Name">
<!-- end of repeat -->

This is where the problem occurs - the view strategy only receives the string value from either of these attributes (validate or validate.bind) since it's looking at the DOM and not the VM and it can't evaluate the variables in the VM scope. I tried adding validate.bind to the bindingPathAttributes array at first but realised this would just result in the same problem - I'd just get the binding string and not the bound value

The easiest solution was to create a custom attribute to write the actual evaluated value to the DOM:

import {inject, customAttribute} from 'aurelia-framework';

@customAttribute("validate-hint")
@inject(Element)
export class ValidateHint {
    constructor(element) {
        this.element = element;
    }

    valueChanged(newValue) {
        if (newValue) {
            this.element.setAttribute("validate", newValue);
        }
    }
}

Which I applied to the input.

The view strategy is a view concern and I like the clean separation between the validatior and the visuals, so I don't think it makes sense to involve the viewmodel in any way - is this the simplest solution do you think?

bfil commented 9 years ago

@janvanderhaegen how did you set up the validation in the view model to make that work with validate-hint?

charlespockert commented 9 years ago

Hi @bfil - it was something I was working on. Validation in viewmodel was just normal validation setup but I wanted it over an array of properties that might change (when the user selected a different option and I generated a different set of inputs) e.g.

   addDynamicValidation() {

        // Also add "required" validation to all alert type properties
        if(this.alert.Metadata != null) {
            console.log("Adding alert properties for alert type " + this.alert.AlertType);

            this.alertPropertiesValidation = this.validationPlugin.on(this.alert.Metadata);

            for (var i = this.requiredMetadata.length - 1; i >= 0; i--) {
                var md = this.requiredMetadata[i];

                this.alertPropertiesValidation.ensure(md.Name).isNotEmpty().withMessage("please supply a value");
            };
        }
    }

I then used:

<form validate.bind="alertPropertiesValidation" id="alertProps">
                <div class="form-group" repeat.for="metadata of requiredMetadata">
                    <label>${metadata.FriendlyName}: </label>
                    <div>
                        <input type="text" placeholder="Please supply a value" class="form-control" value.bind="$parent.alert.Metadata[metadata.Name]" validate-hint.bind="metadata.Name"></input>
                    </div>
                </div>
            </form>

As you can see, due to it being bound to Metadata[metadata.Name] I had to use the validate-hint to write the property name into the DOM so that the validation strategy could pick it up from the view

janvanderhaegen commented 9 years ago

Github needs a 'like' button :)

EisenbergEffect commented 9 years ago

If you install the ZenHub chrome extension, you can +1 anything you want.

On Thu, May 21, 2015 at 4:06 PM, Jan Van der Haegen < notifications@github.com> wrote:

Github needs a 'like' button :)

— Reply to this email directly or view it on GitHub https://github.com/aurelia/validation/issues/10#issuecomment-104405895.

Rob Eisenberg, President - Blue Spire www.durandaljs.com

bfil commented 9 years ago

I cannot seem to be able to get it to work for me, what is this.alert.Metadata in your example? When does addDynamicValidation() get called?

How do you make it work with something like the following?

class Profile {
    constructor(firstName, lastName, links) {
        this.firstName = firstName;
        this.lastName = lastName;
        this.links = links;
    }
}
this.profile = new Profile('First name', 'Last name', [ { label: 'Some label', url: 'http://example.com' } ]);
this.formValidation =
    validation.on(this)
        .ensure('profile.firstName')
            .isNotEmpty().withMessage('Please enter your first name')
        .ensure('profile.lastName')
            .isNotEmpty().withMessage('Please enter your last name');

Then the following would add add dynamic link validation based on the index later on, for example (with index 0, first element of the array):

this.formValidation
    .ensure('link-label-0')
        .isNotEmpty().withMessage('Please enter a label for the url')
    .ensure('link-url-0')
        .isNotEmpty().withMessage('Please enter a valid url');

Then I would expect the following field to trigger the validation on my form:

<div repeat.for="link of profile.links">
    <input type="text" class="form-control" value.bind="link.label" validate="link-label-0">
    <input type="text" class="form-control" value.bind="link.url" validate="link-url-0">
</div>

Of course the index is hardcoded to make the example easier to follow but in theory it would use the validate-hint custom attribute to generate the right one and then add the dynamic validation rules.

I tried using validate.on(this.profile) and various other combinations but it does not seem to work for me, I would expect that the value provided to the validate attribute in the HTML would match the one defined on the formValidation object, but it doesn't on an array, any clue?

bfil commented 9 years ago

It looks like that using the validate attribute in that way does not set up the right observers, if I use the following:

this.formValidation
    .ensure('profile.links.0.label')
        .isNotEmpty().withMessage('Please enter a label for the url')
    .ensure('profile.links.0.url')
        .isNotEmpty().withMessage('Please enter a valid url');

And I use the same format in the validate attribute: profile.links.0.label and profile.links.0.url than the observers are set up correctly and the validation is triggered correctly.

Making it work properly it still quite tricky though, if I remove an element from the array I need to set up the validation again since the elements at the indexes of the array are changing, doable, but not ideal.

Thanks for the help anyway! I'll probably wait for the ensureForEach to be implemented.

bfil commented 9 years ago

I'm providing the complete example of how I have a dynamic form working in my app (a simplified version of it for clarity purposes).

I have a set of links attached to a Profile class that can be added/removed and need to be validated.

Basically profile.links is an array of objects with 2 properties, label and url, the template I'm using looks like the following:

<form validate.bind="formValidation">
    <div repeat.for="link of profile.links">
        <input type="text" value.bind="link.label" validate-hint="${$index >= 0 ? 'profile.links.' + $index + '.label' : ''}">
        <input type="text" value.bind="link.url" validate-hint="${$index >= 0 ? 'profile.links.' + $index + '.url' : ''}">
        <button click.delegate="$parent.removeLink($index)">Remove link</button>
    </div>
</form>

Note that I'm using the validate-hint custom attribute suggested above by @CharlesPockert.

In addition, the check $index >= 0 is there because sometimes $index turned out to be undefined, which caused me some headaches, not sure if that's an issue though.

In the view model I have the following code that sets up the validation:

setUpFormValidation() {
    this.formValidation = this.validation.on(this);
    this.profile.links.forEach( (link, index) =>
        this.formValidation
            .ensure(`profile.links.${index}.label`)
                .isNotEmpty().withMessage('Please enter a label')
            .ensure(`profile.links.${index}.url`)
                .isNotEmpty().withMessage('Please enter a valid url')
    );
}

And two methods to add/remove links:

addLink() {
    this.profile.links.push({ label: '', url: '' });
    this.updateFormValidation();
}

removeLink(linkIndex) {
    _.pullAt(this.profile.links, linkIndex); // using lodash here to remove the link at the index specified
    this.updateFormValidation();
}

Basically every time a link is added/removed this is what updateFormValidation does:

updateFormValidation() {
    this.formValidation.destroy();
    setTimeout( () => this.setUpFormValidation() );
}

Which is partially working with version 0.2.5, but causing validations to be triggered on the wrong elements when adding/removing elements. These issues have been resolved by https://github.com/aurelia/validation/pull/65.

Maybe this approach can be used as a base, the ugliest bit I guess is the need of the validate-hint custom attribute to create the correct validate attribute needed by Aurelia validation. Everything else could be easily refactored into an ensureForEach method.

Hope this helps.

alvarezmario commented 9 years ago

Any news to share about the current state of the ensureForEach constraint? It'll be awesome to have it. :)

janvanderhaegen commented 9 years ago

@nomack84 It'll actually be this weekend. We've come to a point in one of our own apps where we can't move forward without this either ;)

alvarezmario commented 9 years ago

Then God Bless that app :smiley: :laughing:

janvanderhaegen commented 9 years ago

Hah!

janvanderhaegen commented 9 years ago

Arrrr, ETA todayish, sorry I didn't get to it this weekend :(

JeroenVinke commented 9 years ago

+1, my app would benefit from this

grofit commented 9 years ago

+1 this would help greatly with array based interactoins.

alvarezmario commented 9 years ago

@janvanderhaegen When you plan to make us happy? ;)

rpawlaszek commented 9 years ago

I just stumbled upon the same issue yesterday, and am too interested in this feature:) But I think it may be interesting to think of such a syntax:

validation.on(this)
          .forAll('items', itemValidation => {
              itemValidation.ensure('name')
                            .isNotEmpty()
                            .isUnique()
                            ...
          })

It then would require few things:

  1. Being able to communicate between ValidationGroups (as the itemValidation one would need to transfer the results upwards)
  2. Using ArrayObserver/MapObserver to get the information
  3. (probably) hooking up to repeat.for and then (what I think) to 'repeat.for' attribute with (if that's possible) dynamically getting $parent and $index elements on changes from the observers. This would help with getting rid of validation-hint (but, once again, it's only if that's possible).

I'd opt for

.forAll(...)

as it is about the collection rather than single elements. Another thing is that this way we could create a validation group for an element in the array and it would require some performance analysis.

And there this

.isUnique()

that I think would be useful and with arrayObservers it (I think) can be achieved.

It may be not far from what @bfil did and right now I'm trying to get through the source to see if all I wrote above is actually... well... implementable, but wouldn't want to reinvent the wheel:)

rpawlaszek commented 9 years ago

Okay, so option 3. is not possible as there's not repeat.for after a template gets processed by aurelia. Is there any possibility to know which views are governed by repeat.for?

EisenbergEffect commented 9 years ago

As of the latest release, the View instance is available through the created callback of any custom attribute or behavior. This may assist. @janvanderhaegen There are a number of advanced capabilities that are available in the latest release which you might find useful. Not only can you get the View instance cut also the original compiler TargetInstruction for any html element with behaviors or bindings. You cna get it from DI. Take a look in the templating resources repo at the view-spy and compile-spy attributes to get an idea. Maybe it can help here?

plwalters commented 9 years ago

Question on this - if items are a collection of other models why not apply the validation to those models?

Question on this - if items are a collection of other models why not apply the validation to those models?

What's the benefit over using a forEach to something like this -

import {Container} from 'aurelia-dependency-injection';
import {Validation} from 'aurelia-validation';
let validation = new Container().get(Validation);
class Person {
  firstName = '';
  friends = [];
  constructor() {
    this.validation = validation.on(this)
          .ensure('firstName')
            .isNotEmpty()
            .hasMinLength(3)
            .hasMaxLength(10);
    this.friends.push(new Friend())
  }
  validate() {
    return this.validation.validate();
  }
}

class Friend {
  fullName = '';
  constructor() {
    this.validation = validation.on(this)
          .ensure('fullName')
            .isNotEmpty()
            .hasMinLength(3)
            .hasMaxLength(10);
  }
}

It seems to me this prevents crazy scenarios where you are trying to ensureForEach over classes (models) that already have validation and seems more natural. Still reading through whole thread was just curious.

alvarezmario commented 9 years ago

How can you ensure that the friends array contains at least n items and that those items are valid?

plwalters commented 9 years ago

Good point I think I was looking at the most recent comments where I thought it was to apply validation to each of an array of objects, need to read more thoroughly.

alvarezmario commented 9 years ago

I think @janvanderhaegen had an idea of the implementation, he simply have not time to do it. Perhaps he can provide some thoughts of a possible implementation...

alvarezmario commented 9 years ago

@PWKad @janvanderhaegen any news?

MicahZoltu commented 9 years ago

The example from @PWKad is exactly what I am looking for. I have a form that is auto-generated with an array of things. Each item in the array must be valid for the form to be valid. Currently, I am having to validate each individually and then aggregate their results to determine overall validity. At the moment I am gaining very little from using the Validation plugin since I effectively have to manually validate everything anyway.

<form validate.bind="validation" submit.delegate="takeAction()">
    <div repeat.for="item of items" class="form-group">
        <input value.bind="value"/>
    </div>
    <div class="form-group">
        <button type="submit">Submit</button>
    </div>
</form>
export class Foo {
    public items: Item[];
    private validationGroup: ValidationGroup;

    constructor(public validation: Validation) {
        this.validationGroup = validation.on(this);
        this.items = getItems(this.validationGroup);
    }

    public takeAction() {
        this.validationGroup.validate().then(() => {
            // TODO: action
        });
    }
}

export class Item {
    public value: string;

    constructor(validationGroup: ValidationGroup) {
        // this is wrong, there is no 'value' on the validationGroup, value is on a sub-model.
        validationGroup.ensure('value').isNotEmpty();
    }
}
MicahZoltu commented 9 years ago

After much trial and failure, I have been unable to figure out how to get the twbootstrap-view-strategy working correctly with arrays of items. I have validation correctly working with the array by doing the following at some point after the array has been populated.

onItemsPopulated() {
    for (var i: number; i < items.length; ++i) {
        this.validationGroup
            .ensure(`items.${i}.value`)
            .isNotEmpty();
    }
}

onAction() {
    this.validationGroup.validate().then(() => {
        // do action
    });
}

I am unable to get the form to show the validation tips however:

<form validate.bind="validationGroup">
    <div repeat.for="item of items" class="form-group">
        <input value.bind="item.value" validate="items.0.value"/>
    </div>
</form>

Note: I hard-coded the 0 index in validate to ensure that the problem wasn't with resolving the template string items.${index}.value.

MicahZoltu commented 9 years ago

Upon further investigation, it appears that the problem is that the repeat.for adds DOM elements after the validation change handlers have already been setup. Because of this, the newly added DOM elements don't get change handlers applied to them, despite being children of a DOM element that should be looked at.

I believe the number one thing that is needed here is to have the form re-run its validation binding when the form changes. Currently, having a form with dynamic elements that need validation is pretty painful and you have to do weird things like manually destroying and reconnecting validation every time the form changes.

Second up I would say is supporting validate.bind. This would remove the need for the awkward custom attribute that is necessary to have validate point at dynamic things.

Third would be native support for repeat.for and in general validation against arrays.

valichek commented 8 years ago

I have tried to implement @bfil 's solution in beta, but it doesn't work for me, so I have solved that in more simple way. I have model and have array model.contacts of {firstname: "Mike"}, I have added another array model.contactForms of {data: contact, validation: validation, sub: subscription}, and use that array in repeat.for="...", I define <form validate="item.validation"> for every item and bind inputs to item.data.firstname. So I create validation object for ever contact like validation = this.Validation.on(contact).ensure('firstname').isNotEmpy();, and when I have created it, I call validation.validate().catch(() => {});, because validation.result.isValid is always true when validation created (bug here?), even if data in contact are not valid like firstname is empty in my case. So at that point I have main validation object for my model and many validation objects for every contact in model.contacts stored like model.contactForms and used in repeat.for. Next thing I did, was subscribing to validation.result.isValid for every contact validation using ObserverLocator,

form.sub = (newValue, oldValue) => {
   this.validateContactForms();
};
this.observerLocator.getObserver(form.validation.result, 'isValid').subscribe(form.sub);

Also I have validateContactForms method, where I check if all of the contactForms are valid and set flag model.areContactsValid.

And then I have

<button disabled="!validation.result.isValid || !model.areContactsValid" click.trigger="...">submit<button>

And of course I need to dispose my validations, when contact is removed from array.

grofit commented 8 years ago

I have been doing some work in validation (agnostic to aurelia) and one of the other problems here is how to manage array changes. i.e lets assume you have an inventory which contains n items, each item requires a name and a value for example. Now you could walk the model and take a snapshot at validator creation, however then if I add an item to the inventory after the validator has been created you ideally want it to automatically apply the same ruleset for the other items into the new item, however I am sure there would be some fringe cases where arrays may contain different types or only some should be validated etc.

This again kinda ties in to my previous notion of "rulesets" which would simplify these sort of situations as you could more easily re-use validation rules if you do need to apply the same rule to other entities of same type.

https://github.com/aurelia/validation/issues/100

KamBha commented 8 years ago

Not sure if this is the right place for this, but I have a related but slightly different problem. I have a scenario where I have an object like this:-

{
  firstTab:  { description: "blah" }, 
  secondTab: {
   description: "Blah",
   someArray: [{
      dateRange: { start: "2015-12-12", end: "2016-12-12"},
      description: "Blah"
   },
   {
      dateRange: { start: "2017-12-12", end: "2018-12-12"},
      description: "Blah"
   },
   ]
 }
}

In this example, I would like firstTab and secondTab to be separate tabs and I want the values in someArray to be displayed in a side tab within secondTab. When there is an error, I would like to highlight the tabs in error (top tabs and side tabs), as well as highlighting the fields in errors (and block some buttons). Additionally, I have some validation that no one date can cross another (eg, if the second date range started before 2018-12-12, that is an error, but it isn't an error with a specific date, but with the whole date range.

It seems to me that this is easy enough to support. I create a wrapper around the ValidatorGroup that allow me to construct new validators and have a hierarchy of validators. Something is considered invalid if either the fields on the current validator are invalid or any sub validators are invalid. Thinking about this, I think you could bake this into the validation framework. For example, you could pass into a custom element, you parent validator:-

<my-repeating-value data.bind="data" parent-validator.bind="validator"></my-repeating-value>

The component could do this then:-

this.validation = this.parentValidator.newValidator(this.data)
          .ensure('description')
            .isNotEmpty()
            .hasMinLength(3)
            .hasMaxLength(10);

This way, each template can have its own validator and react to validation errors.

I can also properly validate the date ranges and can put the validation error on the whole date range. As I said, I can create a nasty wrapper around this, but surely the behaviour I am describing is not unusual.

tomtomau commented 8 years ago

Any update on this? Pretty important but I guess will try and use the validate-hint example provided.

valichek commented 8 years ago

@tomtomau Looks like there will be no updates on current implementation of aurelia validation, they are going to rewrite this library completely

grofit commented 8 years ago

sounds good, I would like to know more about the goals of the re-write though, any docs or conv snippets anywhere on the subject?

valichek commented 8 years ago

You can read more about re-write here http://blog.durandal.io/2016/02/01/big-february-status-update/

plwalters commented 8 years ago

Thanks for submitting this / commenting on this. At this time we are closing this because we have completely re-written and are deprecating the previous feature set. If you feel this should be re-opened please feel free to review this blog post and submit either again on this repository, or on the new validatejs bridge repository

Thanks again!