Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngModel): test & update correct model when running $validate #7837

Closed
wants to merge 1 commit into from
Closed

fix(ngModel): test & update correct model when running $validate #7837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 24 additions & 15 deletions src/ng/directive/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1770,13 +1770,24 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
* Runs each of the registered validations set on the $validators object.
*/
this.$validate = function() {
this.$$runValidators(ctrl.$modelValue, ctrl.$viewValue);
// ignore $validate before model initialized
if (ctrl.$modelValue !== ctrl.$modelValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to use !equals() here rather than !== in case the models are objects with changing properties, see #8047

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is just a trick to test if $modelValue is NaN (it compares it against itself)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, sorry I was really focussing on the following !==

return;
}

var prev = ctrl.$modelValue;
ctrl.$$runValidators(ctrl.$$invalidModelValue || ctrl.$modelValue, ctrl.$viewValue);
if (prev !== ctrl.$modelValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

ctrl.$$writeModelToScope();
}
};

this.$$runValidators = function(modelValue, viewValue) {
forEach(ctrl.$validators, function(fn, name) {
ctrl.$setValidity(name, fn(modelValue, viewValue));
});
ctrl.$modelValue = ctrl.$valid ? modelValue : undefined;
ctrl.$$invalidModelValue = ctrl.$valid ? undefined : modelValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Then we may need to make copies of the values for comparison purposes...

Copy link
Contributor

Choose a reason for hiding this comment

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

But this is more problematic if we then want to reassign $$invalidModelValue back to $modelValue later, since really we should be flattening the copy, which loses its prototype hierarchy.

Copy link
Contributor

Choose a reason for hiding this comment

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

We decided that this is not an issue as we are going to expect people using ngModel to deal with making copies if necessary.

};

/**
Expand Down Expand Up @@ -1815,22 +1826,22 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$

if (ctrl.$modelValue !== modelValue &&
(isUndefined(ctrl.$$invalidModelValue) || ctrl.$$invalidModelValue != modelValue)) {

ctrl.$$runValidators(modelValue, viewValue);
ctrl.$modelValue = ctrl.$valid ? modelValue : undefined;
ctrl.$$invalidModelValue = ctrl.$valid ? undefined : modelValue;

ngModelSet($scope, ctrl.$modelValue);
forEach(ctrl.$viewChangeListeners, function(listener) {
try {
listener();
} catch(e) {
$exceptionHandler(e);
}
});
ctrl.$$writeModelToScope();
}
};

this.$$writeModelToScope = function() {
ngModelSet($scope, ctrl.$modelValue);
forEach(ctrl.$viewChangeListeners, function(listener) {
try {
listener();
} catch(e) {
$exceptionHandler(e);
}
});
};

/**
* @ngdoc method
* @name ngModel.NgModelController#$setViewValue
Expand Down Expand Up @@ -1909,8 +1920,6 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
}

ctrl.$$runValidators(modelValue, viewValue);
ctrl.$modelValue = ctrl.$valid ? modelValue : undefined;
ctrl.$$invalidModelValue = ctrl.$valid ? undefined : modelValue;

if (ctrl.$viewValue !== viewValue) {
ctrl.$viewValue = ctrl.$$lastCommittedViewValue = viewValue;
Expand Down
94 changes: 78 additions & 16 deletions test/ng/directive/inputSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,27 +294,13 @@ describe('NgModelController', function() {
};

ctrl.$modelValue = 'test';
ctrl.$$invalidModelValue = undefined;
ctrl.$validate();

expect(ctrl.$valid).toBe(false);

ctrl.$modelValue = 'TEST';
ctrl.$validate();

expect(ctrl.$valid).toBe(true);
});

it('should perform validations when $validate() is called', function() {
ctrl.$validators.uppercase = function(value) {
return (/^[A-Z]+$/).test(value);
};

ctrl.$modelValue = 'test';
ctrl.$validate();

expect(ctrl.$valid).toBe(false);

ctrl.$modelValue = 'TEST';
ctrl.$$invalidModelValue = undefined;
ctrl.$validate();

expect(ctrl.$valid).toBe(true);
Expand Down Expand Up @@ -403,6 +389,7 @@ describe('NgModelController', function() {
};
};

ctrl.$modelValue = undefined;
ctrl.$validators.a = curry(true);
ctrl.$validators.b = curry(true);
ctrl.$validators.c = curry(false);
Expand All @@ -423,6 +410,7 @@ describe('NgModelController', function() {
};
};

ctrl.$modelValue = undefined;
ctrl.$validators.unique = curry(false);
ctrl.$validators.tooLong = curry(false);
ctrl.$validators.notNumeric = curry(true);
Expand Down Expand Up @@ -1489,6 +1477,80 @@ describe('input', function() {
expect(inputElm).toBeValid();
expect(scope.form.input.$error.maxlength).not.toBe(true);
});

it('should assign the correct model after an observed validator became valid', function() {
compileInput('<input type="text" name="input" ng-model="value" maxlength="{{ max }}" />');

scope.$apply(function() {
scope.max = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

just as a quick note, this is less code if you say scope.$apply("max = 1"), and easier to read. I wouldn't worry about assignment expressions being removed from the grammar, that's not going to happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

});
changeInputValueTo('12345');
expect(scope.value).toBeUndefined();

scope.$apply(function() {
scope.max = 6;
});
expect(scope.value).toBe('12345');
});

it('should assign the correct model after an observed validator became invalid', function() {
compileInput('<input type="text" name="input" ng-model="value" maxlength="{{ max }}" />');

scope.$apply(function() {
scope.max = 6;
});
changeInputValueTo('12345');
expect(scope.value).toBe('12345');

scope.$apply(function() {
scope.max = 1;
});
expect(scope.value).toBeUndefined();
});

it('should leave the value as invalid if observed maxlength changed, but is still invalid', function() {
compileInput('<input type="text" name="input" ng-model="value" maxlength="{{ max }}" />');
scope.$apply(function() {
scope.max = 1;
});

changeInputValueTo('12345');
expect(inputElm).toBeInvalid();
expect(scope.form.input.$error.maxlength).toBe(true);
expect(scope.value).toBeUndefined();

scope.$apply(function() {
scope.max = 3;
});

expect(inputElm).toBeInvalid();
expect(scope.form.input.$error.maxlength).toBe(true);
expect(scope.value).toBeUndefined();
});

it('should not notify if observed maxlength changed, but is still invalid', function() {
compileInput('<input type="text" name="input" ng-model="value" ng-change="ngChangeSpy()" ' +
'maxlength="{{ max }}" />');

scope.$apply(function() {
scope.max = 1;
});
changeInputValueTo('12345');
Copy link
Contributor

Choose a reason for hiding this comment

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

So what you are saying is change the input value to the same value it had before, which should trigger a call to $setViewValue, and so run the validation but not to run the parsers or update the model. Right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this really is changing the $viewValue. I think we need some tests that exercise the case where the validation criteria are changed without the $viewValue changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually setting the max to 1 and setting the input to 12345 is just the preparation so we have an invalid input. What I'm actually testing is that when I'm changing max to be 3 in the next line, ngChangeSpy is not invoked (since the value is still invalid)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, thought this is a different test. I mean that after the preparation I'm changing the max to be 6 and seeing that the previously entered value of 12345 is now on the scope (since now it is valid)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK cool.


scope.ngChangeSpy = jasmine.createSpy();
scope.$apply(function() {
scope.max = 3;
});

expect(scope.ngChangeSpy).not.toHaveBeenCalled();
});

it('should leave the model untouched when validating before model initialization', function() {
scope.value = '12345';
compileInput('<input type="text" name="input" ng-model="value" minlength="3" />');
expect(scope.value).toBe('12345');
});

});


Expand Down