lukejagodzinski / meteor-astronomy-validators

https://atmospherejs.com/jagi/astronomy-validators
MIT License
11 stars 13 forks source link

Conditional Validations #23

Closed frabrunelle closed 9 years ago

frabrunelle commented 9 years ago

I am wondering if you have any thoughts or plans about having conditional validations in Astronomy. This is something that is possible in other frameworks such as Ruby on Rails.

For example, my use case is to have a field that is only required if the value of another field (in this case, level) is equal to a specific value (in this case, 2).

At the moment, my solution was to implement a custom validator:

Astro.createValidator({
  name: 'requiredIf',
  validate: function(fieldValue, fieldName, options) {
    var parentValue = this.get(options[0]);
    var compareValue = options[1];

    if (parentValue === compareValue) {
      return !_.isNull(fieldValue) && fieldValue !== '';
    } else {
      return true;
    }
  },
  events: {
    validationerror: function(e) {
      var fieldName = e.data.field;

      e.data.message = 'The "' + fieldName +
        '" field\'s value is required';
    }
  }
});

And to use it like this:

occupation: {
  type: 'string',
  validators: [
    Validators.requiredIf(['level', 2])
  ]
}

But maybe that's not the best way. I'm thinking that there could be "Conditional validators" in Astronomy; it would be very useful! We could use this issue for brainstorming :smile:

And thank you for the great package, I love it! I'm also very much looking forward to the new features you are developing, as well as an eventual forms package :smiley: I am starting a project on Monday and we decided to use Astronomy for the model layer.

Cheers.

ghost commented 9 years ago

I've asked the same question a few days ago with this issue https://github.com/jagi/meteor-astronomy-validators/issues/21

lukejagodzinski commented 9 years ago

@shadowRR it's a little bit different issue then yours. You wanted to validate nested object only if it's present. And if it goes about @frabrunelle issue, he wants to validates fields on the same level but only if some other field has certain value.

I see that's it's very common use case, so maybe it's a time to implement it. I would see it more like IF statement from the spreadsheets:

occupation: {
  type: 'string',
  validators: Validators.if({
    condition: function() {
      return this.get('level') === 2;
    },
    true: Validators.minLength(10)
    false: /* optional */
  })
}

Maybe it's not easily readable but it gives a lot of flexibility. What do you think guys?

ghost commented 9 years ago

@jagi yeah but the idea remain the same, validation depending on the state of another field by some sort of an if validation. And from your proposal, i could use it for my own issue.

As about your proposal, what if someone would want to have multiple of those ? Like in @frabrunelle example:

if(level === 3){
    // use those validators...
}
if(level === 4){
    // use those validators and so on...
}
lukejagodzinski commented 9 years ago

You would have to use multiple if validators.

occupation: {
  type: 'string',
  validators: [
    Validators.if({
      condition: function() {
        return this.get('level') === 2;
      },
      true: Validators.minLength(10)
    }),
    Validators.if({
      condition: function() {
        return this.get('level') === 4;
      },
      true: Validators.minLength(20)
    })
  ]
}

If the condition form the first if validator is not meet then the validator would return true and move forward to the next one, etc. etc.

However, it maybe also a good idea to implement the switch validator:

occupation: {
  type: 'string',
  validators: [
    Validators.switch({
      value: function() {
        return this.get('level');
      },
      case: {
        2: Validators.minLength(10),
        4: Validators.and([
          Validators.minLength(20),
          Validators.maxLength(40)
        ])
      }
    })
  ]
}

Possibilities are endless :)

ghost commented 9 years ago

The switch validator would be awesome :D ! Both of them would be really useful, and i can see quite a few case where to use them to ease up my schemas...

I've finished yesterday switching my app over astronomy, and those would definitely permit to finally finish gracefully some part of it ;)

lukejagodzinski commented 9 years ago

Ok, I will add both if and switch validators. They will probably appear with the next release, because they are not so hard to implement. We will see

ghost commented 9 years ago

Ok, thanks ;) Any idea when this new version will be ready ? (not pushing you, just wondering)

lukejagodzinski commented 9 years ago

Yesterday I've just finished the last big feature. Now I have to do some code fixes, cleaning and implement few minor features. It will take about 2 weeks and after that I will start testing 1.0 pre release version. I will invite everyone to test it. Of course there are so many changes that it will need good documentation. I hope that the 1.0 final release will be available in 4-6 weeks from now. Time will tell, how many bugs are there and if I missed something :)

ghost commented 9 years ago

Ok, nice to hear. For sure, the new documentation is gonna need quite a lot work to get up to speed. I can't really help for this one (since you're the only one who knows clearly what has changed), but as far as testing goes, i'll definitely be there :D. Thanks again for you're amazing work.

frabrunelle commented 9 years ago
occupation: {
  type: 'string',
  validators: Validators.if({
    condition: function() {
      return this.get('level') === 2;
    },
    true: Validators.minLength(10)
    false: /* optional */
  })
}

I like that a lot! And it's very readable I find :smile: Also, since I might have for example 10 fields that are only required if the value of level equals 2, I might take advantage of the ability to reuse validators.

:+1: for the switch validator. It seems like it could be useful in some situations.

And I'm super interested to test 1.0 pre release!

lukejagodzinski commented 9 years ago

Stay tuned :)

lukejagodzinski commented 9 years ago

if and switch validators implemented. They will appear in Astronomy 1.0

frabrunelle commented 9 years ago

@jagi: The if validator works well, but I am not able to use the switch validator.

After studying the code, I find out that this line is causing the issue: https://github.com/jagi/meteor-astronomy-validators/blob/1.0.0-rc.1/lib/validators/logical/switch.js#L25

if (_.contains(_.keys(options.cases), expression)) {

The problem is that if the expression is a number, it won't match with the keys since they are converted to strings.

So doing the following doesn't work:

validator: Validators.switch({
  expression: function() {
    return this.level;
  },
  cases: {
    1: Validators.minLength(8),
    2: Validators.required()
  }
})

My solution is to add toString():

return this.level.toString();

Maybe it could be done automatically?

if (_.contains(_.keys(options.cases), expression.toString())) {

Or let me know if you have a better solution :smile:

Thank you!

lukejagodzinski commented 9 years ago

Oh, thank you for letting me know. I will change it to use the _.has method. Thanks to that it won't be converted to the string.