Skip to content
This repository has been archived by the owner on Jul 1, 2020. It is now read-only.

Incorrect validation on "required" with a value of 0 #177

Closed
mgm09a opened this issue Oct 30, 2018 · 4 comments
Closed

Incorrect validation on "required" with a value of 0 #177

mgm09a opened this issue Oct 30, 2018 · 4 comments

Comments

@mgm09a
Copy link
Contributor

mgm09a commented Oct 30, 2018

I understand this project is just on life support, and I know that you've mostly moved on to working with other technologies, but I wanted to ask if this odd behavior was by design before I attempted to investigate much further.

http://plnkr.co/edit/t2zzENCdQp163F4IsZH3?p=preview

In this Plunker, you'll find an example of some odd behavior. On my ng-model, the value is initialized to 0. The select element to which it is bound is decorated with validation="required", but the classes that initially get assigned to that element indicate that it is invalid.

I would have expected that a value of 0 would have satisfied the validation="required" rule, but perhaps that was a mistaken assumption.

More interestingly, if I change the selection to another value, then back to 0, the required validation passes. This makes me think that something is not right, as the behavior changes after the element becomes dirty.

Should this behavior remain the same, or should I look into creating a pull request (my first ever) to try and fix it?

@ghiscoding
Copy link
Owner

ghiscoding commented Oct 30, 2018

Hmm that could be a bug, but yeah I probably won't spend much time on troubleshooting it because as you said it is in "Life support mode" but I certainly accept PR if you find the bug for sure.

If I look at the code, most of every validation rule are driven by RegEx (which BTW is awesome) and you can see the required validator here which basically check for non-empty spaces.

Quickly looking at the code and searching for the word "required", I can see that I do check for the value to be not empty string, neither undefined or null here but on this other line I checked the value as !!value and having 0 won't make it go in the if, so on your side you can try to change the code to check the empty string/undefined/null like I mentioned in previous line. That's a good troubleshooting start, if it's more than that then I'll let you dig more into it

EDIT
Thanks for pointing out the Life Support, I added a comment in the readme to mention exactly what you had in mind which is it's in life support but I certainly still accept PR for bug fixes. But I still accept votes ⭐️ 🤣

@mgm09a
Copy link
Contributor Author

mgm09a commented Nov 9, 2018

I believe I've fixed the issue. The cause is line 406 in validation-directive.js.

In the revalidateAndAttachOnBlur function, you're finding the value to validate...

var value = ctrl.$modelValue || '';

...but this causes 0 to be replaced with ''.

I've updated this line to be:

var value = ctrl.$modelValue !== null && typeof ctrl.$modelValue !== 'undefined'
                    ? ctrl.$modelValue
                    : '';

and the issue seems to be resolved.


Now, as I've mentioned, I've never done a pull request. I'm currently reading about how to do that, so it might be a little while... hahha.

@ghiscoding
Copy link
Owner

@mgm09a
There's always a first time for everything 😉

I quickly looked at the PR, it seems fine, I'll review it more in depth tonight and will probably push a new release by then.

Thanks again 👍

@ghiscoding
Copy link
Owner

ghiscoding commented Nov 14, 2018

closed by merge of PR #178, NPM is now bumped to version 1.5.26

@mgm09a
Thanks a lot for your first ever PR on GitHub, hopefully this will make you contribute to this wonderful Open Source community 😉 🥇

Cheers 🍺
You can also up vote ⭐️ if you haven't already 😃

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

No branches or pull requests

2 participants