From 138d14994d240764f7be71d25c3034e2eaadb0a7 Mon Sep 17 00:00:00 2001 From: Marcelo Sauerbrunn Portugal Date: Thu, 29 Dec 2016 14:30:17 -0500 Subject: [PATCH] fix(modifyRows): modifyRows uses the new entity when using enableRowHashing When using enableRowHashing (which is set right now to true as default), modifyRows match the old and the new row but not using the new entity at all. Credit to @idangozlan for the fix. This PR merely adds unit tests to his [work](https://github.com/angular-ui/ui-grid/pull/4818). --- src/js/core/factories/Grid.js | 15 ++-- test/unit/core/factories/Grid.spec.js | 99 +++++++++++++++++---------- 2 files changed, 72 insertions(+), 42 deletions(-) diff --git a/src/js/core/factories/Grid.js b/src/js/core/factories/Grid.js index ea3fd662f3..01f5bdaa90 100644 --- a/src/js/core/factories/Grid.js +++ b/src/js/core/factories/Grid.js @@ -1066,7 +1066,7 @@ angular.module('ui.grid') * @methodOf ui.grid.class:Grid * @description returns the GridRow that contains the rowEntity * @param {object} rowEntity the gridOptions.data array element instance - * @param {array} rows [optional] the rows to look in - if not provided then + * @param {array} lookInRows [optional] the rows to look in - if not provided then * looks in grid.rows */ Grid.prototype.getRow = function getRow(rowEntity, lookInRows) { @@ -1131,13 +1131,20 @@ angular.module('ui.grid') self.rows.length = 0; newRawData.forEach( function( newEntity, i ) { - var newRow; + var newRow, oldRow; + if ( self.options.enableRowHashing ){ // if hashing is enabled, then this row will be in the hash if we already know about it - newRow = oldRowHash.get( newEntity ); + oldRow = oldRowHash.get( newEntity ); } else { // otherwise, manually search the oldRows to see if we can find this row - newRow = self.getRow(newEntity, oldRows); + oldRow = self.getRow(newEntity, oldRows); + } + + // update newRow to have an entity + if ( oldRow ) { + newRow = oldRow; + newRow.entity = newEntity; } // if we didn't find the row, it must be new, so create it diff --git a/test/unit/core/factories/Grid.spec.js b/test/unit/core/factories/Grid.spec.js index 54db2c268e..09ecc66fb5 100644 --- a/test/unit/core/factories/Grid.spec.js +++ b/test/unit/core/factories/Grid.spec.js @@ -414,86 +414,109 @@ describe('Grid factory', function () { }); describe('follow source array', function() { - it('should insert it on position 0', function() { - var dataRows = [{str:'abc'}]; - var grid = new Grid({ id: 1 }); + var dataRows, grid; - grid.modifyRows(dataRows); + beforeEach(function() { + dataRows = [{str:'abc'},{str:'cba'},{str:'bac'}]; + grid = new Grid({ id: 1 }); + grid.options.enableRowHashing = false; + + spyOn(grid, 'getRow').and.callThrough(); + grid.modifyRows(dataRows); + }); - expect(grid.rows.length).toBe(1); + it('should update the grid rows', function() { + expect(grid.rows.length).toBe(3); expect(grid.rows[0].entity.str).toBe('abc'); + expect(grid.rows[1].entity.str).toBe('cba'); + expect(grid.rows[2].entity.str).toBe('bac'); + }); + it('should insert it on position 0', function() { dataRows.splice(0,0,{str:'cba'}); grid.modifyRows(dataRows); - expect(grid.rows.length).toBe(2); + expect(grid.getRow).toHaveBeenCalled(); + expect(grid.rows.length).toBe(4); expect(grid.rows[0].entity.str).toBe('cba'); }); it('should swap', function() { - var dataRows = [{str:'abc'},{str:'cba'}]; - var grid = new Grid({ id: 1 }); - - grid.modifyRows(dataRows); - - expect(grid.rows[0].entity.str).toBe('abc'); - expect(grid.rows[1].entity.str).toBe('cba'); - var tmpRow = dataRows[0]; + dataRows[0] = dataRows[1]; dataRows[1] = tmpRow; grid.modifyRows(dataRows); + expect(grid.getRow).toHaveBeenCalled(); expect(grid.rows[0].entity.str).toBe('cba'); expect(grid.rows[1].entity.str).toBe('abc'); }); it('should delete and insert new in the middle', function() { - var dataRows = [{str:'abc'},{str:'cba'},{str:'bac'}]; - var grid = new Grid({ id: 1 }); - - grid.modifyRows(dataRows); - - expect(grid.rows.length).toBe(3); - expect(grid.rows[0].entity.str).toBe('abc'); - expect(grid.rows[1].entity.str).toBe('cba'); - expect(grid.rows[2].entity.str).toBe('bac'); - dataRows[1] = {str:'xyz'}; grid.modifyRows(dataRows); + expect(grid.getRow).toHaveBeenCalled(); expect(grid.rows.length).toBe(3); expect(grid.rows[0].entity.str).toBe('abc'); expect(grid.rows[1].entity.str).toBe('xyz'); expect(grid.rows[2].entity.str).toBe('bac'); }); + }); + + describe('when row hashing is enabled', function() { + var dataRows, grid; + + beforeEach(function() { + dataRows = [{str:'abc'},{str:'cba'},{str:'bac'}]; + grid = new Grid({ id: 1 }); + grid.options.enableRowHashing = true; + + spyOn(grid, 'getRow').and.callThrough(); - /* - * No longer trying to keep order of sort - we run rowsProcessors - * immediately after anyway, which will resort. - * - it('should keep the order of the sort', function() { - var dataRows = [{str:'abc'},{str:'cba'},{str:'bac'}]; - var grid = new Grid({ id: 1 }); - grid.options.columnDefs = [{name:'1',type:'string'}]; - grid.buildColumns(); grid.modifyRows(dataRows); + }); + it('should update the grid rows', function() { expect(grid.rows.length).toBe(3); expect(grid.rows[0].entity.str).toBe('abc'); expect(grid.rows[1].entity.str).toBe('cba'); expect(grid.rows[2].entity.str).toBe('bac'); + }); - grid.sortColumn(grid.columns[0]); - - dataRows.splice(0,0,{str:'xyz'}); + it('should insert it on position 0', function() { + dataRows.splice(0,0,{str:'cba'}); grid.modifyRows(dataRows); + + expect(grid.getRow).not.toHaveBeenCalled(); expect(grid.rows.length).toBe(4); + expect(grid.rows[0].entity.str).toBe('cba'); + }); + + it('should swap', function() { + var tmpRow = dataRows[0]; + + dataRows[0] = dataRows[1]; + dataRows[1] = tmpRow; + grid.modifyRows(dataRows); + + expect(grid.getRow).not.toHaveBeenCalled(); + expect(grid.rows[0].entity.str).toBe('cba'); + expect(grid.rows[1].entity.str).toBe('abc'); + }); + + it('should delete and insert new in the middle', function() { + dataRows[1] = {str:'xyz'}; + grid.modifyRows(dataRows); + + expect(grid.getRow).not.toHaveBeenCalled(); + expect(grid.rows.length).toBe(3); expect(grid.rows[0].entity.str).toBe('abc'); - expect(grid.rows[3].entity.str).toBe('xyz'); + expect(grid.rows[1].entity.str).toBe('xyz'); + expect(grid.rows[2].entity.str).toBe('bac'); }); - */ }); describe('binding', function() {