Skip to content
This repository has been archived by the owner on Dec 21, 2021. It is now read-only.

Commit

Permalink
AUI-1958 Revert AUI-533. Custom rules should not be required.
Browse files Browse the repository at this point in the history
  • Loading branch information
@blzaugg
blzaugg committed Aug 27, 2015
1 parent 6b50503 commit 3315c7a
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/aui-form-validator/js/aui-form-validator.js
View file
Expand Up @@ -933,7 +933,7 @@ var FormValidator = A.Component.create({
var required = instance.normalizeRuleValue(fieldRules.required);

validatable = (required || (!required && defaults.RULES.required.apply(instance, [field.val(),
field])) || fieldRules.custom);
field])));
}

return !!validatable;
Expand Down

9 comments on commit 3315c7a

@cloutierjo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes a custom rule need to be validated even if the input content is empty.

So the original if was saying validate this field when:

  • it's required
  • when it has content
  • when it is a custom validator

in our specific case, we have many input that have are required only in some case eg: if checkbox a is checked, input b is required. that doesn't work anymore because of this specific change.

@blzaugg
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the original change in AUI-533, all custom rules required validation, regardless of the required property being set.

The reasoning for reverting AUI-533 was:

Custom Rule: custom_AOL: Value must end in "@aol.com"

  • Form (A)
    • Email field with rules:
      • required
      • email
      • custom_AOL
  • Form (B)
    • Email field with rules:
      • email
      • custom_AOL

So on Form (B), we want to still validate custom_AOL but we don't want to make it required.

But, with the original change, it will be "required". Even if we explicitly handle blank values...

E.g. updated Custom Rule: custom_AOL: Value must end in "@aol.com" or blank "".

...it will still try to enforce the email rule as required ever though we don't want it to be.

My suggestion for you is; add required to your input (B) and have it's custom validator check checkbox (A). This will work as long as you only have one rule (your custom validator) on the field.

@cloutierjo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

I quite well understand the original request for this fix as I was the requester from liferay EE. If I stay with your sample:
In the case where the field is empty, I expect that in the form (B) the email validator won't trigger an error as the email validator in itself should not make the field required. But I also expect the custom_AOL validator to be trigered, because it might have to trigger an error because the field is empty.

The suggested solution wouldn't work as making the field required would actually make it required, but that is not what we want. Or maybe I didn't understand the suggestion.

Furthermore, I believe that this drastically change a behaviour that has been there for 4 year and that many application rely on. Worst, I'm not even certain how it got in our application as I couldn't find it in the Liferay patch applied.

Thank you for the fast reply.
Regards,

@blzaugg
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case where the field is empty, I expect that in the form (B) the email validator won't trigger an error as the email validator in itself should not make the field required

That’s correct

But I also expect the custom_AOL validator to be trigered, because it might have to trigger an error because the field is empty.

This is incorrect.

Each rule, itself, does not determine if the field gets validated or not, and does not determine if the rule will be executed.

Whether a field gets validated is determined in the validatable() method above. When this is true, all rules on that field will be executed. It’s all or nothing … and you just got me thinking about the required rule. More on that in a sec.

Furthermore, I believe that this drastically change a behaviour that has been there for 4 year and that many application rely on.

We considered this, but felt it better to have the correct (and less restrictive) behavior, rather than continuing to rely on buggy behavior.

The suggested solution wouldn't work as making the field required would actually make it required, but that is not what we want. Or maybe I didn't understand the suggestion.

You’re right, the required rule would execute. This is an oversight on my part. I’m going to create a ticket to allow a “better” dynamic required rule. This is partially implemented but isn’t very useful in it’s current state.

This will allow something like:

magicFn(node) {
    return node.ancestor('*').val() == ‘magic’;
}
  • Form (C)
    • Email field with rules:
      • required: magicFn
      • email
      • custom_AOL

Would this give you the flexibility to validate your two conditional fields together?

@cloutierjo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might work but there is two consequence with this:

  • First that's still a major behavior change considering that it come in a release where we doesn't expect major change, and it actually break application in prod without people knowing it. We found it by accident after a few week in test and a bit more than a week in prod. Now if this solution become final, we have to check back and maybe fix about an hundred validators, the same apply for every other developer using this feature when they find out that the application doesn't work anymore as expected
  • I believed that it increase complexity where it isn't required, but that might be personal taste.

On my side, I doesn't believe that it's a buggy behavior to run a validator on an empty field, as long as it is clear that this is the behavior. Even more when we only talk about custom validator, the user of that feature expected there validator to run at all time. The first time I found out that validators weren't run on empty field, I took a few days to dive inside the liferay code then the aui code to find the existence of the validatable() method. It took that time because it is counterintuitive. What was my intuition is that a validator is run every time the field is focused out and the form is submitted. This is the old behavior.

Each rule, itself, does not determine if the field gets validated or not, and does not determine if the rule will be executed.

I think that's taking the problem from the wrong side. Each rules shouldn't determine if the field gets validated. I think every rules should be executed at all time but each rules should be written to work in every case as expected (that being the email validator shouldn't make the field required and other similar case)

On last question about how this updated code got in Liferay as it clearly isn't in the applied patch. Does it come from CDN?

Regards,

@blzaugg
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked to our team about this issue.

In Portal 6.2/AUI 2.0, we are going to revert back to the AUI-533 behavior, by default, and provide an opt-in property for the correct behavior.

So in Portal 6.2 your custom validators should work, without any changes necessary.

Going forward, we'd like Portal7.0/AUI 3 to default to the correct behavior. You'd need to dynamically determine if the field is required using required: function (node) { ... },.

Please, let us know your thoughts on this.

To follow up on this fix. Please create a LESA ticket so you can receive the proper patch.

Let them know:

@cloutierjo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not answering sooner. The followup on LESA has been done.

I believe that it's a good solution and strategy for the aui3 change.

Thank you for your collaboration.

@blzaugg
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, as will.

@blzaugg
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I documented a solution to your use case here:
https://issues.liferay.com/browse/LPS-60587?focusedCommentId=712453&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-712453

Please sign in to comment.