From 9bdf6c827cca5c7ba5ca6ee24a0d38b4412c3bcc Mon Sep 17 00:00:00 2001 From: Derek Meyer Date: Tue, 15 Dec 2015 15:44:47 -0600 Subject: [PATCH 1/2] Bug Fix: A tagged item was getting removed accidentally in the following scenario: ui-select configuration: -tagging -async with refresh delay problem code flow: 1) user enters tag text and immediately accepts the new tag(s) (with tab or the like), such that the tag is created before the refresh delay expires 2) a validator determines that the tag is invalid, and sets the model value of the ui-select instance to undefined. At this point the view value still shows the invalid value. 3) refresh is triggered after the delay, and updates the items list on the uiSelectCtrl 4) the item watcher sees the change and causes the view value to be updated from the model value, in order to reflect the data in the fresh items 5) since the model value is undefined until the invalid condition is resolved, the formatters run and create an empty view value, erasing the tagged item(s) - the user does not have an opportunity to resolve the invalid condition, their data is just erased. solution: do not update the view value on items refresh, when the model value is undefined --- src/uiSelectController.js | 6 +++++- src/uiSelectMultipleDirective.js | 5 ++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/uiSelectController.js b/src/uiSelectController.js index 1afccb72e..7ae3671ef 100644 --- a/src/uiSelectController.js +++ b/src/uiSelectController.js @@ -200,7 +200,11 @@ uis.controller('uiSelectCtrl', //Remove already selected items (ex: while searching) //TODO Should add a test ctrl.refreshItems(items); - ctrl.ngModel.$modelValue = null; //Force scope model value and ngModel value to be out of sync to re-run formatters + + //update the view value with fresh data from items, if there is a valid model value + if(angular.isDefined(ctrl.ngModel.$modelValue)) { + ctrl.ngModel.$modelValue = null; //Force scope model value and ngModel value to be out of sync to re-run formatters + } } } }); diff --git a/src/uiSelectMultipleDirective.js b/src/uiSelectMultipleDirective.js index 261640105..19e42a423 100644 --- a/src/uiSelectMultipleDirective.js +++ b/src/uiSelectMultipleDirective.js @@ -135,7 +135,10 @@ uis.directive('uiSelectMultiple', ['uiSelectMinErr','$timeout', function(uiSelec //Watch for external model changes scope.$watchCollection(function(){ return ngModel.$modelValue; }, function(newValue, oldValue) { if (oldValue != newValue){ - ngModel.$modelValue = null; //Force scope model value and ngModel value to be out of sync to re-run formatters + //update the view value with fresh data from items, if there is a valid model value + if(angular.isDefined(ngModel.$modelValue)) { + ngModel.$modelValue = null; //Force scope model value and ngModel value to be out of sync to re-run formatters + } $selectMultiple.refreshComponent(); } }); From 953e4b8aaf857981daaf26bf186854ac3124e510 Mon Sep 17 00:00:00 2001 From: Derek Meyer Date: Wed, 16 Dec 2015 15:20:39 -0600 Subject: [PATCH 2/2] Add Tests: Tagged items should not be removed when the control is invalid --- test/select.spec.js | 110 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-) diff --git a/test/select.spec.js b/test/select.spec.js index 3663ab839..f65dfefae 100644 --- a/test/select.spec.js +++ b/test/select.spec.js @@ -36,7 +36,37 @@ describe('ui-select tests', function() { }); - beforeEach(module('ngSanitize', 'ui.select', 'wrapperDirective')); + /* Create a directive that can be applied to the ui-select instance to test + * the effects of Angular's validation process on the control. + * + * Does not currently work with single property binding. Looks at the + * selected object or objects for a "valid" property. If all selected objects + * have a "valid" property that is truthy, the validator passes. + */ + angular.module('testValidator', []); + angular.module('testValidator').directive('testValidator', function() { + return { + restrict: 'A', + require: 'ngModel', + link: function(scope, element, attrs, ngModel) { + ngModel.$validators.testValidator = function(modelValue, viewValue) { + if(angular.isUndefined(modelValue) || modelValue === null) { + return true; + } else if(angular.isArray(modelValue)) { + var allValid = true, idx = modelValue.length; + while(idx-- > 0 && allValid) { + allValid = allValid && modelValue[idx].valid; + } + return allValid; + } else { + return !!modelValue.valid; + } + }; + } + } + }); + + beforeEach(module('ngSanitize', 'ui.select', 'wrapperDirective', 'testValidator')); beforeEach(function() { module(function($provide) { @@ -1433,6 +1463,45 @@ describe('ui-select tests', function() { }); + it('should retain an invalid view value after refreshing items', function() { + scope.taggingFunc = function (name) { + return { + name: name, + email: name + '@email.com', + valid: name === "iamvalid" + }; + }; + + var el = compileTemplate( + ' \ + {{$select.selected.email}} \ + \ +
\ +
\ + \ + ' + ); + + clickMatch(el); + var searchInput = el.find('.ui-select-search'); + + setSearchText(el, 'iamvalid'); + triggerKeydown(searchInput, Key.Tab); + + //model value defined because it's valid, view value defined as expected + var validTag = scope.taggingFunc("iamvalid"); + expect(scope.selection.selected).toEqual(validTag); + expect($(el).scope().$select.selected).toEqual(validTag); + + clickMatch(el); + setSearchText(el, 'notvalid'); + triggerKeydown(searchInput, Key.Tab); + + //model value undefined because it's invalid, view value STILL defined as expected + expect(scope.selection.selected).toEqual(undefined); + expect($(el).scope().$select.selected).toEqual(scope.taggingFunc("notvalid")); + }); + describe('search-enabled option', function() { var el; @@ -2018,6 +2087,45 @@ describe('ui-select tests', function() { }); + it('should retain an invalid view value after refreshing items', function() { + scope.taggingFunc = function (name) { + return { + name: name, + email: name + '@email.com', + valid: name === "iamvalid" + }; + }; + + var el = compileTemplate( + ' \ + {{$select.selected.email}} \ + \ +
\ +
\ + \ + ' + ); + + clickMatch(el); + var searchInput = el.find('.ui-select-search'); + + setSearchText(el, 'iamvalid'); + triggerKeydown(searchInput, Key.Tab); + + //model value defined because it's valid, view value defined as expected + var validTag = scope.taggingFunc("iamvalid"); + expect(scope.selection.selectedMultiple).toEqual([jasmine.objectContaining(validTag)]); + expect($(el).scope().$select.selected).toEqual([jasmine.objectContaining(validTag)]); + + clickMatch(el); + setSearchText(el, 'notvalid'); + triggerKeydown(searchInput, Key.Tab); + + //model value undefined because it's invalid, view value STILL defined as expected + var invalidTag = scope.taggingFunc("notvalid"); + expect(scope.selection.selected).toEqual(undefined); + expect($(el).scope().$select.selected).toEqual([jasmine.objectContaining(validTag), jasmine.objectContaining(invalidTag)]); + }); it('should run $formatters when changing model directly', function () {