From 5d62a959f9baa1e3c5c260ff9fb1053b35045762 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Tue, 20 Jan 2015 21:08:32 +0100 Subject: [PATCH 1/2] 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 076b39b0a2685408231c3f733a743f10ab8e07f5 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Sun, 8 Feb 2015 13:55:49 +0100 Subject: [PATCH 2/2] 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}); }); });