Validation groups

[ghost]

I was looking at the next version of your has() validators and like to throw in some idea about it.

I was mislead at first because of the name, which doesn't point out that it's only working on nested objects...I think it should like you did test if the fieldValue is an object, but if not, it should take the entire Class properties, and test it against the 'propertyName'. Also, the _.has() function only test if a key exist in an object, but if that property as a null value, it doesn't care. So maybe it should also verified that the property actually has some values with it ?

What do you think ?

ps: I'd be happy to fork and work on that if you think my idea is any good...

[lukejagodzinski]

Hmm I don't know if I understand you correctly, but if the field's value is not a nested object you want the validator to look for the given property in entire "parent" object?

User = Astro.Class({
  /* ... */
  fields: {
    address: 'object',
    validators: Validators.has('city')
  }
});

var user = new User();
user.validate('address');

And now, if the address field is let's say null, then the validator should look for the city property in the entire user document? It's what you meant?

---

If it goes about second question. It's no point of checking if a value is null, because in the new version there is new required attribute in the field's definition. And validation will not take place if required is set to false and the value is null.

[ghost]

I'm gonna try to explain myself with examples. Let's say, like in your documentation, that yout have an object in your Class, called 'adress' which has two property inside: 'city' and 'zipcode'. And on this object, you have you're Validators.has('city') and Validators.has('zipcode'). On that case, the actual validators should work has intended. But now, let's say that my adress isn't in an object. The fields zipcode and city are directly there like this :

User = Astro.Class({
  /* ... */
  fields: {
       city: {
         type: 'string'
       }
  }
});

And also, that those two (city and zipcode) aren't required, except if one or the other has been filled. To be clear, if i don't fill either city or zipcode, it's fine. But if i fill for example city, then i have to also fill zipcode. Which would means to have a Validators.or() like this.

User = Astro.Class({
  /* ... */
  fields: {
       city: {
         type: 'string',
         validators: Validators.or({
              Validators.and({
                  Validators.string(),
                  Validators.has('zipcode')
              }),
              Validators.null()
         })
       }
  }
});

The idea here would be to check with the Validators.has() that if city is there, then it must also have the zipcode filled. Which it doesn't do right now, since it only check in an object. Now, maybe it's not what this validators is intended for...

I've even actually already built my own validators for what i need (thanks to your awesome createValidator), and it looks like this :

Astro.createValidator({
  name: 'hasProperty',
  validate: function(fieldValue, fieldName, propertyName) {
      var obj = _.isObject(fieldValue) ? fieldValue : this;
      if(_.has(obj, propertyName)) {
          return !_.isEmpty(obj[propertyName]);
      }
      return false;
  }
});

He's basically checking if the fieldValue is an object (like adress on your example), and if not, take the entire class to get all property inside it.Then it check if the property exist and if yes, if the property has is value filled. And i'm using it like this inside the fields{} where if street property is filled, then city and street has to be filled to:

postalcode: {
            type: 'string',
            validators: Validators.or([
                Validators.and([
                    Validators.string(),
                    Validators.regexp(/^[0-9]{5}$/),
                    Validators.hasProperty('street', 'Incomplete adress'),
                    Validators.hasProperty('city', 'Incomplete adress')
                ]),
                Validators.null()
            ])
    }

I hope it's clear enough...

[lukejagodzinski]

Ok, now I understand what you mean. Your case is very specific and I don't think that such validator would fit other developers' needs. But that's exactly what for are custom validators :) as you said.

Your validation rule is rather like if something then do something otherwise do something else. Or it could be called group, because we want to validate multiple fields only if any of them has non null value.

Maybe it's good idea to group validators. I will add this feature for consideration. I would have to think how it could be implemented to not mess with the current code.

For now, I advice you to use your code. It does its job. I don't want to introduce such validators because I decided that validators should work on single field, eventually it should be based on the value of an another field but not existence of this field or its validation.

[ghost]

I've actually came to the same conclusion while reading my own post ;). On a side note, would be nice with a "group validator" to be able to actually trigger the error message on a different field than the one in validation. For example, still with the adress example, easily being able to trigger an "Incomplete adress" on the city field while validating street field. I don't think there's an easy way to do this right now ?

[lukejagodzinski]

In my list of features to implement I have the feature that would allow settting custom errors. Right now indeed it would be very hard to implement. You would need to mess with Astronomy internals. I'm planning to create some utils functions that would make creating custom validators even easier.

The question is how group validators should work. Should they be treated as a new field? So, it would required to name a group. So you would be able to get validation error for a group. Should there be only groups of type "All fields in a group have to be valid" or maybe also "If any of the fields in a group is valid". Those groups would use every and some functions accordingly. Maybe they would also have some other feature? It's better to discuss it right now.

[ghost]

Ok, nice to hear for the custom errors system ;).

About the group validators. I don't think it should be seen as a new field. Seeing how powerful with just a few lines a Validators can already get, i'm not sure anything else than adding a new Validators similar to has() but for that purpose would be useful...

Something like hasAllProp() and hasSomeProp maybe ? Then you'd just provide an array of fields that have to be checked in the validators options. Then, for example, inside the hasAllProp(), you'd have something like a special trigger for custom errors on every fields that hasn't been validated ? This way, in the adress example, if city would be filled, but not zipcode, the validator would trigger an error message on zipcode saying that you have to filled the entire adress or none at all.

[ghost]

Or, if i follow you're idea correctly, you'd have something like this ?

groups: {
    fields: ['city', 'zipcode']
    validators: Validators.or([
        Validators,.and([
            Validators.string(),
            Validators.minLength(2)
        ]),
        Validators.null()
    ])
}

[lukejagodzinski]

By saying "treated as a field" I didn't mean that it should be added as a field. And I didn't want it to be another validator but rather new feature. I will give you an example.

Post = Astro.Class({
  name: 'Post',
  fields: {
    first: {
      type: 'string',
      validators: Validators.length(5)
    },
    second: {
      type: 'string',
      validators: Validators.length(5)
    },
    third: {
      type: 'string',
      validators: Validators.length(5)
    }
  },
  validationGroup: {
    address: {
      type: 'every',
      fields: ['first', 'second', 'third']
    }
  }
});

Validation groups would be taken into account when validating entire document or on multiple fields validation where all the fields from the group are being validated.

post.getValidationError('address');

What do you think? Maybe it's not the best approach but it was my first thought.

[ghost]

Ok, now i get it. And basically, with your type: 'every', the fields then should be either all there or none at all, right ? If that's the case, it's a better solution than mine, and you'd get a direct access to a global error on a group, which would be awesome. I can also see a use case, with a type: 'some' which could force the user to select at least one of multiple checkbox on a form.

We should also have a property inside a validationGroup to directly setup the error message if it fails.

[lukejagodzinski]

Yes right. We will come back to details when I start implementing it :-)