ngRequired: suggest mark empty arrays as invalid

[gorkunov]

Current validator in the ngRequired directive looks like:

var validator = function(value) {
  if (attr.required && (isEmpty(value) || value === false)) {
      ctrl.$setValidity('required', false);
      return;
  } else {
      ctrl.$setValidity('required', true);
      return value;
  }
};

Value checked by isEmpty function which returns true for empty arrays.

if (attr.required && (isEmpty(value) || value === false)) {

I think that [] should be marked as invalid for this case.

[jrust]

👍 This took us quite a while to figure out when our models with multiple-select were not being required even though they had no options selected.

[jrust]

Here's a fiddle showing the problem. This is against the latest 1.2.0rc1, but 1.0.7 has the same issue.

[hbulhoes]

This would be super useful. We're expecting to employ ng-required for a custom directive bound to an array, so that if the array is empty, the model is invalid.

[petebacondarwin]

This should be easily fixed by this particular select directive updating NgModelController.isEmpty to believe that empty arrays are an empty value as far as things like "required" are concerned.

[petebacondarwin]

I have hacked angular.js in this plunker to fix the issue: http://plnkr.co/edit/iolpERje098xNXSWbnVR?p=preview
Look at line 19885.

Does anyone want to turn this into a PR?

[gorkunov]

@petebacondarwin could you explain your hack?

[romario333]

@petebacondarwin You have a typo in there - ngModeCtrl instead of ngModelCtrl.

Based on your fix I have created a simple workaround, which can be applied without the need to hack the original angular sources. Just add this directive to your app:

APP.directive('select', function() {
  return {
    restrict: 'E',
    require: '?ngModel',
    link: function(scope, element, attr, ngModelCtrl) {
      if (ngModelCtrl && attr.multiple) {
        ngModelCtrl.$isEmpty = function(value) {
          return !value || value.length === 0;
        }
      }
    }
  }

Demo of the workaround is here: http://plnkr.co/edit/0VnbPb?p=preview

[caitp]

I'm a bit confused...

Why is

it('should require', function() {
      compile(
        '<select name="select" ng-model="selection" multiple required>' +
          '<option>A</option>' +
          '<option>B</option>' +
        '</select>');

      scope.$apply(function() {
        scope.selection = [];
      });

      expect(scope.form.select.$error.required).toBeTruthy();
      expect(element).toBeInvalid();
      expect(element).toBePristine();

      scope.$apply(function() {
        scope.selection = ['A'];
      });

      expect(element).toBeValid();
      expect(element).toBePristine();

      element[0].value = 'B';
      browserTrigger(element, 'change');
      expect(element).toBeValid();
      expect(element).toBeDirty();
    });
  });

passing even before applying @petebacondarwin's changes?

[petebacondarwin]

Because it is not using ng-options, maybe?

On 7 December 2013 18:56, Caitlin Potter notifications@github.com wrote:

I'm a bit confused...

Why is

it('should require', function() {
compile(
'' + 'A' + 'B' + '');

  scope.$apply(function() {
    scope.selection = [];
  });

  expect(scope.form.select.$error.required).toBeTruthy();
  expect(element).toBeInvalid();
  expect(element).toBePristine();

  scope.$apply(function() {
    scope.selection = ['A'];
  });

  expect(element).toBeValid();
  expect(element).toBePristine();

  element[0].value = 'B';
  browserTrigger(element, 'change');
  expect(element).toBeValid();
  expect(element).toBeDirty();
});

});

passing even before applying @petebacondarwinhttps://github.com/petebacondarwin's
changes?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/2365#issuecomment-30061959
.

[caitp]

The ngRequired tests are using ng-options, though. I don't get it :( It seems like this case is thoroughly covered in the test suite... Even in @jrust's fiddle, the case with the empty array + required is giving ng-invalid-required class... So I must be missing something, this seems like a non-issue --- edit I'm wrong, it wasn't initiallized to an empty array.

Okay, so maybe the test suite is not testing the case where the value actually is an empty array, I see... that explains.

---

The proposed changeset does invalidate the empty array, but we don't really have the same level of binding behaviour, compared with the input directives (probably due to the lack of parsers/formatters).

So, adding a new test

createSelect({
  'ng-model': 'value',
  'ng-options': 'item.name for item in values',
  'ng-required': 'required',
  'multiple': ''
}, true);

scope.value = [];
scope.$apply(function() {
  scope.values = [{name: 'A', id: 1}, {name: 'B', id: 2}];
  scope.required = true;
});
expect(element).toBeInvalid(); /* PASSES */

scope.$apply(function() {
  scope.value.push(scope.values[0]);
});
expect(element).toBeValid(); /* FAILS, apparently ngRequired still thinks this is wrong? */

It seems like there is some work to do in select.js beyond this minor change

[petebacondarwin]

@caitp - is this test failing with on master or with my change?

[caitp]

For the "new test" I just posted, it fails completely without your change, but the second portion which should trigger $formatters fails even with the change

[petebacondarwin]

I see. As you say, It looks to me like there is too much work being done in $render: https://github.com/angular/angular.js/blob/master/src/ng/directive/select.js#L406
And that some of this needs to move into a $formatter, to allow the required directive to work properly.

[caitp]

Okay so, I guess the ngModelWatch doesn't use the objectEquality flag, which explains why that second assertion was failing...

I still think this thing is doing too much work, but I guess it does technically work so long as the model value always points to a new object when it changes.

I'll submit a PR with the upgraded tests and code, but yeah, the select directive really needs to be rebuilt I think. It's a bit crazy at the moment :(

[petebacondarwin]

Fixed by 5c97731