From bcfb038ec44e16fdb3290b81630f5c597aa7bb16 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Tue, 20 Jan 2015 21:08:32 +0100 Subject: [PATCH 1/3] fix(ngModel): fix issues when parserName is same as validator key For $validate(), it is necessary to store the parseError state in the controller. Otherwise, if the parser name equals a validator key, $validate() will assume a parse error occured if the validator is invalid. Also, setting the validity for the parser now happens after setting validity for the validator key. Otherwise, the parse key is set, and then immediately afterwards the validator key is unset (because parse errors remove all other validations). Fixes #10698 Closes #10850 --- src/ng/directive/ngModel.js | 12 ++++-- test/ng/directive/ngModelSpec.js | 74 ++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/src/ng/directive/ngModel.js b/src/ng/directive/ngModel.js index 1db2283586c4..7257b5a08dd6 100644 --- a/src/ng/directive/ngModel.js +++ b/src/ng/directive/ngModel.js @@ -233,6 +233,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ this.$dirty = false; this.$valid = true; this.$invalid = false; + this.$$parseError = false; this.$error = {}; // keep invalid keys here this.$$success = {}; // keep valid keys here this.$pending = undefined; // keep pending keys here @@ -518,7 +519,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ // Check if the there's a parse error, so we don't unset it accidentially var parserName = ctrl.$$parserName || 'parse'; - var parserValid = ctrl.$error[parserName] ? false : undefined; + var parserValid = ctrl.$$parseError ? false : undefined; var prevValid = ctrl.$valid; var prevModelValue = ctrl.$modelValue; @@ -563,7 +564,6 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ if (parseValid === undefined) { setValidity(errorKey, null); } else { - setValidity(errorKey, parseValid); if (!parseValid) { forEach(ctrl.$validators, function(v, name) { setValidity(name, null); @@ -571,9 +571,14 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ forEach(ctrl.$asyncValidators, function(v, name) { setValidity(name, null); }); - return false; + ctrl.$$parseError = true; } + // Set the parse error last, to prevent unsetting it, should a $validators key == parserName + setValidity(errorKey, parseValid); + return parseValid; } + + ctrl.$$parseError = false; return true; } @@ -814,6 +819,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ // TODO(perf): why not move this to the action fn? if (modelValue !== ctrl.$modelValue) { ctrl.$modelValue = ctrl.$$rawModelValue = modelValue; + ctrl.$$parseError = false; var formatters = ctrl.$formatters, idx = formatters.length; diff --git a/test/ng/directive/ngModelSpec.js b/test/ng/directive/ngModelSpec.js index 583414deaa9c..97015674aa76 100644 --- a/test/ng/directive/ngModelSpec.js +++ b/test/ng/directive/ngModelSpec.js @@ -1221,6 +1221,80 @@ describe('ngModel', function() { expect(ctrl.$validators.mock).toHaveBeenCalledWith('a', 'ab'); expect(ctrl.$validators.mock.calls.length).toEqual(2); }); + + it('should validate correctly when $parser name equals $validator key', function() { + + var testValid, otherValid, parseValid; + + ctrl.$validators.test = function(value) { + return testValid; + }; + + ctrl.$validators.other = function(value) { + return otherValid; + }; + + ctrl.$$parserName = 'test'; + ctrl.$parsers.push(function(value) { + return parseValid ? true : undefined; + }); + + + testValid = otherValid = parseValid = false; + scope.$apply('value = "allInvalid"'); + expect(scope.value).toBe('allInvalid'); + expect(ctrl.$error).toEqual({test: true, other: true}); + + ctrl.$validate(); + expect(scope.value).toBe('allInvalid'); + expect(ctrl.$error).toEqual({test: true, other: true}); + + ctrl.$setViewValue('stillAllInvalid'); + expect(scope.value).toBeUndefined(); + expect(ctrl.$error).toEqual({test: true}); + + ctrl.$validate(); + expect(scope.value).toBeUndefined(); + expect(ctrl.$error).toEqual({test: true}); + + + parseValid = true; + scope.$apply('value = "parseValidAndValidatorInvalid"'); + expect(scope.value).toBe('parseValidAndValidatorInvalid'); + expect(ctrl.$error).toEqual({test: true, other: true}); + + ctrl.$validate(); + expect(scope.value).toBe('parseValidAndValidatorInvalid'); + expect(ctrl.$error).toEqual({test: true, other: true}); + + ctrl.$setViewValue('stillParseValidAndValidatorInvalid'); + expect(scope.value).toBeUndefined(); + expect(ctrl.$error).toEqual({test: true, other: true}); + + ctrl.$validate(); + expect(scope.value).toBeUndefined(); + expect(ctrl.$error).toEqual({test: true, other: true}); + + + parseValid = false; + testValid = otherValid = true; + scope.$apply('value = "parseInvalidAndValidatorValid"'); + expect(scope.value).toBe('parseInvalidAndValidatorValid'); + expect(ctrl.$error).toEqual({}); + + ctrl.$validate(); + expect(scope.value).toBe('parseInvalidAndValidatorValid'); + expect(ctrl.$error).toEqual({}); + + ctrl.$setViewValue('stillParseInvalidAndValidatorValid'); + expect(scope.value).toBeUndefined(); + expect(ctrl.$error).toEqual({test: true}); + + ctrl.$validate(); + expect(scope.value).toBeUndefined(); + expect(ctrl.$error).toEqual({test: true}); + }); + }); }); From 0b3cc6d5e72548d5781a0dacb1005fa259a045d0 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Sun, 8 Feb 2015 13:55:49 +0100 Subject: [PATCH 2/3] test(ngModel): refactor the test --- test/ng/directive/ngModelSpec.js | 88 +++++++++++++++++++------------- 1 file changed, 52 insertions(+), 36 deletions(-) diff --git a/test/ng/directive/ngModelSpec.js b/test/ng/directive/ngModelSpec.js index 97015674aa76..b7857ad3dc0a 100644 --- a/test/ng/directive/ngModelSpec.js +++ b/test/ng/directive/ngModelSpec.js @@ -1222,77 +1222,93 @@ describe('ngModel', function() { expect(ctrl.$validators.mock.calls.length).toEqual(2); }); - it('should validate correctly when $parser name equals $validator key', function() { - - var testValid, otherValid, parseValid; - - ctrl.$validators.test = function(value) { - return testValid; + iit('should validate correctly when $parser name equals $validator key', function() { + + ctrl.$validators.parserOrValidator = function(value) { + switch (value) { + case 'allInvalid': + case 'parseValid-validatorsInvalid': + case 'stillParseValid-validatorsInvalid': + return false; + default: + return true; + } }; - ctrl.$validators.other = function(value) { - return otherValid; + ctrl.$validators.validator = function(value) { + switch (value) { + case 'allInvalid': + case 'parseValid-validatorsInvalid': + case 'stillParseValid-validatorsInvalid': + return false; + default: + return true; + } }; - ctrl.$$parserName = 'test'; + ctrl.$$parserName = 'parserOrValidator'; ctrl.$parsers.push(function(value) { - return parseValid ? true : undefined; + switch (value) { + case 'allInvalid': + case 'stillAllInvalid': + case 'parseInvalid-validatorsValid': + case 'stillParseInvalid-validatorsValid': + return undefined; + default: + return value; + } }); - - testValid = otherValid = parseValid = false; + //Parser and validators are invalid scope.$apply('value = "allInvalid"'); expect(scope.value).toBe('allInvalid'); - expect(ctrl.$error).toEqual({test: true, other: true}); + expect(ctrl.$error).toEqual({parserOrValidator: true, validator: true}); ctrl.$validate(); - expect(scope.value).toBe('allInvalid'); - expect(ctrl.$error).toEqual({test: true, other: true}); + expect(scope.value).toEqual('allInvalid'); + expect(ctrl.$error).toEqual({parserOrValidator: true, validator: true}); ctrl.$setViewValue('stillAllInvalid'); expect(scope.value).toBeUndefined(); - expect(ctrl.$error).toEqual({test: true}); + expect(ctrl.$error).toEqual({parserOrValidator: true}); ctrl.$validate(); expect(scope.value).toBeUndefined(); - expect(ctrl.$error).toEqual({test: true}); + expect(ctrl.$error).toEqual({parserOrValidator: true}); - - parseValid = true; - scope.$apply('value = "parseValidAndValidatorInvalid"'); - expect(scope.value).toBe('parseValidAndValidatorInvalid'); - expect(ctrl.$error).toEqual({test: true, other: true}); + //Parser is valid, validators are invalid + scope.$apply('value = "parseValid-validatorsInvalid"'); + expect(scope.value).toBe('parseValid-validatorsInvalid'); + expect(ctrl.$error).toEqual({parserOrValidator: true, validator: true}); ctrl.$validate(); - expect(scope.value).toBe('parseValidAndValidatorInvalid'); - expect(ctrl.$error).toEqual({test: true, other: true}); + expect(scope.value).toBe('parseValid-validatorsInvalid'); + expect(ctrl.$error).toEqual({parserOrValidator: true, validator: true}); - ctrl.$setViewValue('stillParseValidAndValidatorInvalid'); + ctrl.$setViewValue('stillParseValid-validatorsInvalid'); expect(scope.value).toBeUndefined(); - expect(ctrl.$error).toEqual({test: true, other: true}); + expect(ctrl.$error).toEqual({parserOrValidator: true, validator: true}); ctrl.$validate(); expect(scope.value).toBeUndefined(); - expect(ctrl.$error).toEqual({test: true, other: true}); - + expect(ctrl.$error).toEqual({parserOrValidator: true, validator: true}); - parseValid = false; - testValid = otherValid = true; - scope.$apply('value = "parseInvalidAndValidatorValid"'); - expect(scope.value).toBe('parseInvalidAndValidatorValid'); + //Parser is invalid, validators are valid + scope.$apply('value = "parseInvalid-validatorsValid"'); + expect(scope.value).toBe('parseInvalid-validatorsValid'); expect(ctrl.$error).toEqual({}); ctrl.$validate(); - expect(scope.value).toBe('parseInvalidAndValidatorValid'); + expect(scope.value).toBe('parseInvalid-validatorsValid'); expect(ctrl.$error).toEqual({}); - ctrl.$setViewValue('stillParseInvalidAndValidatorValid'); + ctrl.$setViewValue('stillParseInvalid-validatorsValid'); expect(scope.value).toBeUndefined(); - expect(ctrl.$error).toEqual({test: true}); + expect(ctrl.$error).toEqual({parserOrValidator: true}); ctrl.$validate(); expect(scope.value).toBeUndefined(); - expect(ctrl.$error).toEqual({test: true}); + expect(ctrl.$error).toEqual({parserOrValidator: true}); }); }); From f5cdbcd80ad6d1f9598c6ea7f90f8a93a605c1be Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 12 Feb 2015 08:28:13 +0000 Subject: [PATCH 3/3] squash: refact(ngModel): share the parserValid variable in the closure --- src/ng/directive/ngModel.js | 33 +++++++++++++------------------- test/ng/directive/ngModelSpec.js | 2 +- 2 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/ng/directive/ngModel.js b/src/ng/directive/ngModel.js index 7257b5a08dd6..8d9c6dc19ba5 100644 --- a/src/ng/directive/ngModel.js +++ b/src/ng/directive/ngModel.js @@ -233,7 +233,6 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ this.$dirty = false; this.$valid = true; this.$invalid = false; - this.$$parseError = false; this.$error = {}; // keep invalid keys here this.$$success = {}; // keep valid keys here this.$pending = undefined; // keep pending keys here @@ -245,6 +244,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ ngModelGet = parsedNgModel, ngModelSet = parsedNgModelAssign, pendingDebounce = null, + parserValid, ctrl = this; this.$$setOptions = function(options) { @@ -517,16 +517,12 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ // the model although neither viewValue nor the model on the scope changed var modelValue = ctrl.$$rawModelValue; - // Check if the there's a parse error, so we don't unset it accidentially - var parserName = ctrl.$$parserName || 'parse'; - var parserValid = ctrl.$$parseError ? false : undefined; - var prevValid = ctrl.$valid; var prevModelValue = ctrl.$modelValue; var allowInvalid = ctrl.$options && ctrl.$options.allowInvalid; - ctrl.$$runValidators(parserValid, modelValue, viewValue, function(allValid) { + ctrl.$$runValidators(modelValue, viewValue, function(allValid) { // If there was no change in validity, don't update the model // This prevents changing an invalid modelValue to undefined if (!allowInvalid && prevValid !== allValid) { @@ -544,12 +540,12 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ }; - this.$$runValidators = function(parseValid, modelValue, viewValue, doneCallback) { + this.$$runValidators = function(modelValue, viewValue, doneCallback) { currentValidationRunId++; var localValidationRunId = currentValidationRunId; // check parser error - if (!processParseErrors(parseValid)) { + if (!processParseErrors()) { validationDone(false); return; } @@ -559,26 +555,23 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ } processAsyncValidators(); - function processParseErrors(parseValid) { + function processParseErrors() { var errorKey = ctrl.$$parserName || 'parse'; - if (parseValid === undefined) { + if (parserValid === undefined) { setValidity(errorKey, null); } else { - if (!parseValid) { + if (!parserValid) { forEach(ctrl.$validators, function(v, name) { setValidity(name, null); }); forEach(ctrl.$asyncValidators, function(v, name) { setValidity(name, null); }); - ctrl.$$parseError = true; } // Set the parse error last, to prevent unsetting it, should a $validators key == parserName - setValidity(errorKey, parseValid); - return parseValid; + setValidity(errorKey, parserValid); + return parserValid; } - - ctrl.$$parseError = false; return true; } @@ -672,7 +665,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ this.$$parseAndValidate = function() { var viewValue = ctrl.$$lastCommittedViewValue; var modelValue = viewValue; - var parserValid = isUndefined(modelValue) ? undefined : true; + parserValid = isUndefined(modelValue) ? undefined : true; if (parserValid) { for (var i = 0; i < ctrl.$parsers.length; i++) { @@ -698,7 +691,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ // Pass the $$lastCommittedViewValue here, because the cached viewValue might be out of date. // This can happen if e.g. $setViewValue is called from inside a parser - ctrl.$$runValidators(parserValid, modelValue, ctrl.$$lastCommittedViewValue, function(allValid) { + ctrl.$$runValidators(modelValue, ctrl.$$lastCommittedViewValue, function(allValid) { if (!allowInvalid) { // Note: Don't check ctrl.$valid here, as we could have // external validators (e.g. calculated on the server), @@ -819,7 +812,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ // TODO(perf): why not move this to the action fn? if (modelValue !== ctrl.$modelValue) { ctrl.$modelValue = ctrl.$$rawModelValue = modelValue; - ctrl.$$parseError = false; + parserValid = undefined; var formatters = ctrl.$formatters, idx = formatters.length; @@ -832,7 +825,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ ctrl.$viewValue = ctrl.$$lastCommittedViewValue = viewValue; ctrl.$render(); - ctrl.$$runValidators(undefined, modelValue, viewValue, noop); + ctrl.$$runValidators(modelValue, viewValue, noop); } } diff --git a/test/ng/directive/ngModelSpec.js b/test/ng/directive/ngModelSpec.js index b7857ad3dc0a..019f078e6fad 100644 --- a/test/ng/directive/ngModelSpec.js +++ b/test/ng/directive/ngModelSpec.js @@ -1222,7 +1222,7 @@ describe('ngModel', function() { expect(ctrl.$validators.mock.calls.length).toEqual(2); }); - iit('should validate correctly when $parser name equals $validator key', function() { + it('should validate correctly when $parser name equals $validator key', function() { ctrl.$validators.parserOrValidator = function(value) { switch (value) {