Select with ngRequired not invalid with unmatched Model set from scope

[mailinger-mate]

I might have found some inconsistency in select validity. When setting the model to a value not listed in options, the form controller remains valid. I think the right behavior would be an invalid state, as the model is not matching the range of available options.

http://plnkr.co/edit/2qm7P1ag2wMCYOwDGRsF

[Narretz]

Oh, yes when the select has ngRequired, it should add the required error. Looks like a bug.

Actually, it works with just required. ngRequired is broken.

[Narretz]

The problem is that the select element has its value property set to whatever is set on the scope, even if there's an error somewhere. That makes sense for inputs, because they would otherwise be empty. required uses this value to check if a control is "empty", and if it is, sets the error. Select is a special case, though ... I could see how it would make sense to set the error if the model finds not match in the options. Wdyt @petebacondarwin?

[petebacondarwin]

I didn't think we did validation on programmatic changes for any input control?

[gkalpak]

@Narretz, just to be clear, are you suggesting that the required error is set when the unknown option is selected ?

[Narretz]

Yes, that would be the change.

For the other question, we do validate after the model is programmatically
changed, but we do not set the model to undefined if there's an error in
that case.
Am 27.10.2015 19:48 schrieb "Georgios Kalpakas" [email protected]:

@Narretz https://github.com/Narretz, just to be clear, are you
suggesting that the required error is set when the unknown option is
selected ?

—
Reply to this email directly or view it on GitHub
#13172 (comment)
.

[petebacondarwin]

OK, so setting the value of a select so that the unknown option is displayed should cause required validation to fail.

[Spidy88]

I think rather than labeling this as a bug with ngRequired, it should be a feature request for an additional validation directive like selected-from or value-within that both take an array of valid options.

The challenge I'm facing when thinking about this issue is that ngOptions is not the only way to create a select element with options. It's definitely the preferred way but what about custom directives for creating your own options, or the common beginner method of using ngRepeat. Both valid ways to create a select statement, and you are still able to use ngModel with validators.

The solution this issue is requesting would only support using ngOptions. Maybe that's the intent, but I wanted to broach this subject before spending anytime submitting a PR

[Narretz]

If we can agree that the select should be invalid when the model is not matched by any option, we should fix this in the pure select, too. Required uses a function in the ngModelController called isEmpty which is called with the current viewValue and when it returns true it will set the required error. Select and ngOptions overwrite this function with their own implementation. To fix this bug we should account for the unmatched case in this logic.
I'm not sure if the problem exists in a pure select though.

[petebacondarwin]

I agree with @Narretz - this should be fixed by providing a better $isEmpty for select and ngOptions directives.

[Spidy88]

Ah I see. Thanks for clearing that up!

[m-amr]

@Narretz
@petebacondarwin

ngOptions overwrite isEmpty function is case of multiple select only,

I have solved this problem
by overrideng ModelCtrl.$validators.required function in ngOptionsDirective
to handle all the cases
please check this pull request

#13354

we have the following cases

1 - case attr.required is false i return true

2 - case $isEmpty(viewValue) return false

3 - case is required and not empty now we need to validate that value is one of the options
and in this case we have two subcases

i think this are the cases that solve the problem.

a - select in multiple and value is an array
we check that all the values of the array must be in of the select options to return true

b - select is a single value so this value must be one of the select options to return true

[danielflippance]

Any news on when we should expect this fix?

[petebacondarwin]

Any chance you can take a look at this @Narretz ?

[Narretz]

I already did iirc correctly, just didn't open a PR for some reason