Skip to content
This repository has been archived by the owner on Nov 27, 2018. It is now read-only.

Option to validate the error always on non-required fields from custom validation #19

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

daxsorbito
Copy link

Added a new property to the fields object so that you could always validate the error thrown from the custom validation by non-required fields.

Added a test also to validate it:

  "field : validateErrorsAlways : custom not required with error" : function() {
    var request = {
      body: {
        field0: "",
      }
    };
    var helpers = {
      customHelperWithError: function(data){
        throw new Error('fail');
      }
    };
    form(
      field("field0").custom(helpers.customHelperWithError).validateErrorsAlways()
    )(request, {});
    assert.equal(request.form.isValid, false);
    assert.equal(request.form.errors.length, 1);
  }

This is based on @dr-nick's fork implementation ....

@freewil
Copy link
Owner

freewil commented Sep 25, 2014

So what is the use case for this?

@daxsorbito
Copy link
Author

The use case might be if you are validating a field that at certain condition might be either required or not. For example, in our case we have a separate start_month and start_year field. We had a custom validation on the start_year field that if both fields are supplied, we have to validate it that the fields should be valid (either 1-12 for months and 4 digits for years) and some other business validations. This field should be validated if the value is supplied (only if supplied) - this is not a required field.
For the current implementation, if we throw an error inside our custom validation for the start_year field, it would always say that the form is valid.
For the proposal, this is same as @dr-nick's implementation, we could specify at the field level that we should propagate the error even the field is not required.

Just noticed, there was an issue raised regarding this.

@freewil
Copy link
Owner

freewil commented Oct 18, 2014

For the current implementation, if we throw an error inside our custom validation for the start_year field, it would always say that the form is valid.

It would only say the form was valid if it the field doesn't have a value.

I'm trying to wrap my head around whether this is an actual issue that can't be solved with using the second param source that is passed to a custom validator.

Maybe your test is just a bad example? I could change your test case by removing validateErrorsAlways() and adding .required() and it would pass your test.

@freewil
Copy link
Owner

freewil commented Oct 18, 2014

I think we need a more real-world example that shows why the new feature is required and why you can't just impelement this with required() and using some conditionals based on the 2nd param source

@daxsorbito
Copy link
Author

Hi, apologies for not so clear in my previous example.

We have start_month and start_year fields that would be required if and only if the card_type has a value of 5. We have a custom validation on the start_year field to validate these scenarios.

  1. start_month and start_year should be required if card_type has a value of 5, would throw an Error if start_month and/or start_year is/are empty.
  2. Should validate that start_month and start_year is a valid date if either start_month and start_year has value, would throw an Error if not a valid date.

So if the user passes a value of 5 on the card_type but didn't put any value on the start_year (where our custom validation resides), the custom validation catches this scenario and throws an Error. For the current implementation, the form is still valid since the Error was not propagated because start_year is empty and was not flagged as required.

Another one is that if the user passes a value on start_month but didn't pass a value on start_year, the custom validation catches this scenario and throws an Error (invalid start date). But since start_year is empty and was not flagged as required, the form is still valid.

In the test, it should pass if you make the field as required – this is the current behavior, but in this scenario, the field is only required if certain fields has certain values. I've updated the test to make it a little bit clearer.

Please let me know if this is not clear enough or if you have some suggestions/comments about this. Thanks a lot. :)

@whynotmatt
Copy link

+1 for adding this feature

@duynguyenhoang
Copy link

+1 for adding this feature

Useful when you want to use custom validation with dependency fields.

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

Successfully merging this pull request may close these issues.

4 participants