fix(ngRequired): set valid to false if select value is required (value is not in options)

[m-amr]

This pull request solve Issue 13172
#13172

i validate that the view value should be one of the select option
so i overrideng ModelCtrl.$validators.required function in ngOptionsDirective

ngModelCtrl.$validators.required = function(modelValue, viewValue) {

      if (!attr.required) {
        return true;
      } else if (ngModelCtrl.$isEmpty(viewValue)) {
        return false;
      }

      var isValidOption = true;
      var viewOptions = [];
      // Get all option and add them to viewOptions array
      angular.forEach(options.items, function(item) {
        viewOptions.push(options.getViewValueFromOption(item));
      });

      // In case of multiple view model is an array so validate all view values
      // if one of them doesn't match set isValidOption to false
      if (multiple) {
        for (var i = 0, length = viewValue.length; i < length; i++) {
          if (viewOptions.indexOf(viewValue[i]) === -1) {
            isValidOption = false;
             break;
          }
        }
      } else {
        if (viewOptions.indexOf(viewValue) === -1) {
          isValidOption = false;
        }
      }

      return isValidOption;
    };

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

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

i think this will solve the problem,

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[m-amr]

I signed it!

[Narretz]

Thanks for the PR. It's going in the right direction, but there are a few things that should be done differently.

1. It's adding the required validator to every select with ngoptions, which also means it gets called every time the options change. The (ng)Required directive should decide if the validator gets added not the ngoptions directive. We are duplicating code here and would have to change it in two places. => The logic should go into the $isEmpty function. I assume you didn't put it into the isEmpty function because it gets also called in the updateOptions function?
2. It's doing too much work. When validation happens, we should have done all the operations we need to determine if we have a real option or not. Maybe we need to set a flag during updateOptions.

Do you want to take another shot at this?

[Narretz]

I don't think we want this behavior with multiple the required error should only be set if no option is selected. If it's multiple and a at least option is selected, then the required error doesn't make much sense. Maybe we should actually introduce a new kind of error called optionMismatch or something like that.

[m-amr]

@Narretz
Thanks for your feedback
I understand the problem and i will try to improve mu solution.
but i would like to ask about something
in ngOptionsDirective
$isEmpty function is overwritten to be
in case of multiple

ngModelCtrl.$isEmpty = function(value) {
      return !value || value.length === 0;
    };

and it is already overwritten in selectDirective with same code in case of multiple

 ngModelCtrl.$isEmpty = function(value) {
      return !value || value.length === 0;
    };

why this duplication ? i think that the overwritten of the $isEmpty function in ngOptionsDirective is
useless since it is already overwritten by the selectDirective with the code same ?

[googlebot]

CLAs look good, thanks!

[m-amr]

I have improved by solution
1 - i added a isOptionValid flag and i found that i could set it in the selectCtrl.writeValue function
so i will not do any extra iteration to check validation and will not cause any performance Issue

2 - I have override $isEmpty function to check if option is valid or not

  // Copy the implementation of $isEmpty function to be used in overwritten one.
  var $$isEmpty = ngModelCtrl.$isEmpty;

  ngModelCtrl.$isEmpty = function(value) {
    return $$isEmpty(value) || !isOptionValid;
  };

3 - if select is multiple and value is an array we check that at least one of the values of the array must be in values of the select options to be valid

4 - I have removed since it is already overwritten in selectDirective with same code in case of multiple

ngModelCtrl.$isEmpty = function(value) {
  return !value || value.length === 0;
};

@Narretz Please check it tell me your feedback
I think i am now handling all the cases you have mentioned in your previous comment.

and i just have a question about the data in the model
if value is invalid should the data is model set to null ??

I have a problem with this test cases
we are now should NOT allow false value incase of required is true

it('should allow falsy values as values', function() {
  createSelect({
    'ng-model': 'value',
    'ng-options': 'item.value as item.name for item in values',
    'ng-required': 'required'
  }, true);

    scope.$apply(function() {
    scope.values = [{name: 'True', value: true}, {name: 'False', value: false}];
    scope.required = false;
  });

  setSelectValue(element, 2);
  expect(element).toBeValid();
  expect(scope.value).toBe(false);

  scope.$apply('required = true');
  expect(element).toBeValid();
  expect(scope.value).toBe(false);
});

so i change it to be

  expect(element).toBeInvalid();
  expect(scope.value).toBe(undefined);    

Thanks

[Narretz]

Imo, this is not right. required means that at least one option must be selected, it doesn't matter if the value is false. I'll look into why this test is failing.

[Narretz]

Thanks for updating this! The test fails because you are not setting isOptionValid to true whenever an option is selected from the ui, i.e. in the readValue fn. I'll take this PR from here, thanks for your work!

[m-amr]

@Narretz Thank for replying
I have tried another solution
i added this function to options object,
i found it is better to make the options object responsible to tell us it's valid state so we can call this function any time options.isSelectedOptionValid()

 function isSelectedOptionValid() {

      var selectedValue = selectElement.val();

      if (!selectedValue) {
        return false;
      }

      // in case  select is multiple then check
      // if one of the selected values matched with the options  return true
      // or return false if no value match
      if (isArray(selectedValue)) {
        for (var i = 0, length = selectedValue.length; i < length; i++) {
          if (selectValueMap[selectedValue[i]]) {
            return true;
          }
        }
        return false;
      }

      return !!selectValueMap[selectedValue];

    }

I check if the value found in selectValueMap to know if option is valid or not.
and validation is need to be done every time the model change since that it can be changed
programatically with a value that doesn't match the options.
Thanks a lot for replying and your feedback .

[changus]

Hi @Narretz, you mentioned you were taking the PR from here, did you ever commit this? This seems to still be an issue for me and I'd love to see what the fix was for this. Thanks in advance!

[Narretz]

@changus You are right. This is still on my radar. I had some other ideas about the implementation, that's why it was never merged. I'll try to get it in until the end of the month.

[changus]

@Narretz Awesome, thank you! I did find another thread:
#13172

with this pull request:
#13962

but I copied those changes into my select.js and did not seem to fix the issue.

Thanks in advance for looking into it!

[Narretz]

We've decided that this should be handled by the application instead of imposing it by default. To make this easier, 1.6.5 will expose info about the unknown option via the selectCtrl: 6fc0f02