fix(addons): do not add special columns twice (like Row Selection)

- some addons are adding special columns dynamically (Row Move, Row Detail & Row Selection) and in rare occasion it could end up adding the same special column multiple time
ghiscoding · Nov 29, 2022 · 8572dfb · 8572dfb
1 parent 6575805
commit 8572dfb
Show file tree

Hide file tree

Showing 5 changed files with 84 additions and 51 deletions.
diff --git a/packages/common/src/extensions/__tests__/slickCheckboxSelectColumn.spec.ts b/packages/common/src/extensions/__tests__/slickCheckboxSelectColumn.spec.ts
@@ -4,18 +4,22 @@ import { BasePubSubService } from '@slickgrid-universal/event-pub-sub';
 import { SlickCheckboxSelectColumn } from '../slickCheckboxSelectColumn';
 import { Column, OnSelectedRowsChangedEventArgs, SlickGrid, SlickNamespace, } from '../../interfaces/index';
 import { SlickRowSelectionModel } from '../../extensions/slickRowSelectionModel';
+import { KeyCode } from '../../enums';
 
 declare const Slick: SlickNamespace;
 
-const addJQueryEventPropagation = function (event, commandKey = '', keyName = '', target?: HTMLElement) {
+const addJQueryEventPropagation = function (event, commandKey = '', keyName = '', target?: HTMLElement, which: string | number = '') {
   Object.defineProperty(event, 'isPropagationStopped', { writable: true, configurable: true, value: jest.fn() });
   Object.defineProperty(event, 'isImmediatePropagationStopped', { writable: true, configurable: true, value: jest.fn() });
   if (commandKey) {
     Object.defineProperty(event, commandKey, { writable: true, configurable: true, value: true });
   }
-  if (keyName) {
+  if (keyName !== '') {
     Object.defineProperty(event, 'key', { writable: true, configurable: true, value: keyName });
   }
+  if (which !== '') {
+    Object.defineProperty(event, 'which', { writable: true, configurable: true, value: which });
+  }
   if (target) {
     Object.defineProperty(event, 'target', { writable: true, configurable: true, value: target });
   }
@@ -82,14 +86,20 @@ jest.mock('../../extensions/slickRowSelectionModel', () => ({
 describe('SlickCheckboxSelectColumn Plugin', () => {
   Slick.RowMoveManager = mockAddon;
 
-  const mockColumns = [
-    { id: 'firstName', field: 'firstName', name: 'First Name', },
-    { id: 'lastName', field: 'lastName', name: 'Last Name', },
-    { id: 'age', field: 'age', name: 'Age', },
-  ] as Column[];
+  let mockColumns: Column[];
+  // let mockColumns: Column[] = [
+  //   { id: 'firstName', field: 'firstName', name: 'First Name', },
+  //   { id: 'lastName', field: 'lastName', name: 'Last Name', },
+  //   { id: 'age', field: 'age', name: 'Age', },
+  // ];
   let plugin: SlickCheckboxSelectColumn;
 
   beforeEach(() => {
+    mockColumns = [
+      { id: 'firstName', field: 'firstName', name: 'First Name', },
+      { id: 'lastName', field: 'lastName', name: 'Last Name', },
+      { id: 'age', field: 'age', name: 'Age', },
+    ];
     plugin = new SlickCheckboxSelectColumn(pubSubServiceStub);
     jest.spyOn(gridStub.getEditorLock(), 'isActive').mockReturnValue(false);
     jest.spyOn(gridStub.getEditorLock(), 'commitCurrentEdit').mockReturnValue(true);
@@ -423,7 +433,13 @@ describe('SlickCheckboxSelectColumn Plugin', () => {
   });
 
   it('should trigger "onClick" event and expect toggleRowSelection to be called', () => {
+    const newCols = [
+      { id: '_checkbox_selector', toolTip: 'Select/Deselect All', field: 'sel', },
+      { id: 'firstName', field: 'firstName', name: 'First Name', },
+    ];
     const toggleRowSpy = jest.spyOn(plugin, 'toggleRowSelectionWithEvent');
+    jest.spyOn(gridStub, 'getColumns').mockReturnValue(newCols);
+    plugin.create(newCols, { checkboxSelector: { columnIndexPosition: 0 } });
 
     plugin.init(gridStub);
     const checkboxElm = document.createElement('input');
@@ -517,13 +533,18 @@ describe('SlickCheckboxSelectColumn Plugin', () => {
   });
 
   it('should trigger "onKeyDown" event and expect toggleRowSelection to be called when editor "commitCurrentEdit" returns True', () => {
+    const newCols = [
+      { id: '_checkbox_selector', toolTip: 'Select/Deselect All', field: 'sel', },
+      { id: 'firstName', field: 'firstName', name: 'First Name', },
+    ];
     const toggleRowSpy = jest.spyOn(plugin, 'toggleRowSelectionWithEvent');
     jest.spyOn(gridStub.getEditorLock(), 'commitCurrentEdit').mockReturnValue(true);
+    jest.spyOn(gridStub, 'getColumns').mockReturnValue(newCols);
 
     plugin.init(gridStub);
     const checkboxElm = document.createElement('input');
     checkboxElm.type = 'checkbox';
-    const keyboardEvent = addJQueryEventPropagation(new Event('keyDown'), '', ' ', checkboxElm);
+    const keyboardEvent = addJQueryEventPropagation(new Event('keyDown'), '', ' ', checkboxElm, KeyCode.SPACE);
     const preventDefaultSpy = jest.spyOn(keyboardEvent, 'preventDefault');
     const stopImmediatePropagationSpy = jest.spyOn(keyboardEvent, 'stopImmediatePropagation');
     gridStub.onKeyDown.notify({ cell: 0, row: 2, grid: gridStub }, keyboardEvent);

diff --git a/packages/common/src/extensions/slickCheckboxSelectColumn.ts b/packages/common/src/extensions/slickCheckboxSelectColumn.ts
@@ -106,18 +106,21 @@ export class SlickCheckboxSelectColumn<T = any> {
     this._addonOptions = { ...this._defaults, ...gridOptions.checkboxSelector } as CheckboxSelectorOption;
     if (Array.isArray(columnDefinitions) && gridOptions) {
       const selectionColumn: Column = this.getColumnDefinition();
-      // column index position in the grid
-      const columnPosition = gridOptions?.checkboxSelector?.columnIndexPosition ?? 0;
-      if (columnPosition > 0) {
-        columnDefinitions.splice(columnPosition, 0, selectionColumn);
-      } else {
-        columnDefinitions.unshift(selectionColumn);
-      }
 
-      this.pubSubService.publish(`onPluginColumnsChanged`, {
-        columns: columnDefinitions,
-        pluginName: this.pluginName
-      });
+      // add new checkbox column unless it was already added
+      if (!columnDefinitions.some(col => col.id === selectionColumn.id)) {
+        // column index position in the grid
+        const columnPosition = gridOptions?.checkboxSelector?.columnIndexPosition ?? 0;
+        if (columnPosition > 0) {
+          columnDefinitions.splice(columnPosition, 0, selectionColumn);
+        } else {
+          columnDefinitions.unshift(selectionColumn);
+        }
+        this.pubSubService.publish(`onPluginColumnsChanged`, {
+          columns: columnDefinitions,
+          pluginName: this.pluginName
+        });
+      }
     }
     return this;
   }

diff --git a/packages/common/src/extensions/slickRowMoveManager.ts b/packages/common/src/extensions/slickRowMoveManager.ts
@@ -103,21 +103,25 @@ export class SlickRowMoveManager {
     this._addonOptions = { ...this._defaults, ...gridOptions.rowMoveManager } as RowMoveManagerOption;
     if (Array.isArray(columnDefinitions) && gridOptions) {
       const newRowMoveColumn: Column = this.getColumnDefinition();
-      const rowMoveColDef = Array.isArray(columnDefinitions) && columnDefinitions.find((col: Column) => col?.behavior === 'selectAndMove');
-      const finalRowMoveColumn = rowMoveColDef ? rowMoveColDef : newRowMoveColumn;
-
-      // column index position in the grid
-      const columnPosition = gridOptions?.rowMoveManager?.columnIndexPosition ?? 0;
-      if (columnPosition > 0) {
-        columnDefinitions.splice(columnPosition, 0, finalRowMoveColumn);
-      } else {
-        columnDefinitions.unshift(finalRowMoveColumn);
-      }
 
-      this.pubSubService.publish(`onPluginColumnsChanged`, {
-        columns: columnDefinitions,
-        pluginName: this.pluginName
-      });
+      // add new row move column unless it was already added
+      if (!columnDefinitions.some(col => col.id === newRowMoveColumn.id)) {
+        const rowMoveColDef = Array.isArray(columnDefinitions) && columnDefinitions.find((col: Column) => col?.behavior === 'selectAndMove');
+        const finalRowMoveColumn = rowMoveColDef ? rowMoveColDef : newRowMoveColumn;
+
+        // column index position in the grid
+        const columnPosition = gridOptions?.rowMoveManager?.columnIndexPosition ?? 0;
+        if (columnPosition > 0) {
+          columnDefinitions.splice(columnPosition, 0, finalRowMoveColumn);
+        } else {
+          columnDefinitions.unshift(finalRowMoveColumn);
+        }
+
+        this.pubSubService.publish(`onPluginColumnsChanged`, {
+          columns: columnDefinitions,
+          pluginName: this.pluginName
+        });
+      }
     }
     return this;
   }

diff --git a/packages/row-detail-view-plugin/src/slickRowDetailView.spec.ts b/packages/row-detail-view-plugin/src/slickRowDetailView.spec.ts
@@ -1,4 +1,4 @@
-import { GridOption, PubSubService, SlickDataView, SlickGrid, SlickNamespace, } from '@slickgrid-universal/common';
+import { Column, GridOption, PubSubService, SlickDataView, SlickGrid, SlickNamespace, } from '@slickgrid-universal/common';
 
 import { SlickRowDetailView } from './slickRowDetailView';
 
@@ -55,10 +55,7 @@ const pubSubServiceStub = {
   unsubscribeAll: jest.fn(),
 } as PubSubService;
 
-const mockColumns = [
-  { id: 'firstName', name: 'First Name', field: 'firstName', width: 100 },
-  { id: 'lasstName', name: 'Last Name', field: 'lasstName', width: 100 },
-];
+let mockColumns: Column[];
 
 describe('SlickRowDetailView plugin', () => {
   const divContainer = document.createElement('div');
@@ -67,6 +64,10 @@ describe('SlickRowDetailView plugin', () => {
   gridContainerElm.className = GRID_UID;
 
   beforeEach(() => {
+    mockColumns = [
+      { id: 'firstName', name: 'First Name', field: 'firstName', width: 100 },
+      { id: 'lasstName', name: 'Last Name', field: 'lasstName', width: 100 },
+    ];
     plugin = new SlickRowDetailView(pubSubServiceStub);
     divContainer.className = `slickgrid-container ${GRID_UID}`;
     document.body.appendChild(divContainer);

diff --git a/packages/row-detail-view-plugin/src/slickRowDetailView.ts b/packages/row-detail-view-plugin/src/slickRowDetailView.ts
@@ -197,21 +197,25 @@ export class SlickRowDetailView implements ExternalResource, UniversalRowDetailV
 
     if (Array.isArray(columnDefinitions) && gridOptions) {
       const newRowDetailViewColumn: Column = this.getColumnDefinition();
-      const rowDetailColDef = Array.isArray(columnDefinitions) && columnDefinitions.find(col => col?.behavior === 'selectAndMove');
-      const finalRowDetailViewColumn = rowDetailColDef ? rowDetailColDef : newRowDetailViewColumn;
 
-      // column index position in the grid
-      const columnPosition = gridOptions?.rowDetailView?.columnIndexPosition ?? 0;
-      if (columnPosition > 0) {
-        columnDefinitions.splice(columnPosition, 0, finalRowDetailViewColumn);
-      } else {
-        columnDefinitions.unshift(finalRowDetailViewColumn);
-      }
+      // add new row detail column unless it was already added
+      if (!columnDefinitions.some(col => col.id === newRowDetailViewColumn.id)) {
+        const rowDetailColDef = Array.isArray(columnDefinitions) && columnDefinitions.find(col => col?.behavior === 'selectAndMove');
+        const finalRowDetailViewColumn = rowDetailColDef ? rowDetailColDef : newRowDetailViewColumn;
+
+        // column index position in the grid
+        const columnPosition = gridOptions?.rowDetailView?.columnIndexPosition ?? 0;
+        if (columnPosition > 0) {
+          columnDefinitions.splice(columnPosition, 0, finalRowDetailViewColumn);
+        } else {
+          columnDefinitions.unshift(finalRowDetailViewColumn);
+        }
 
-      this.pubSubService.publish(`onPluginColumnsChanged`, {
-        columns: columnDefinitions,
-        pluginName: this.pluginName
-      });
+        this.pubSubService.publish(`onPluginColumnsChanged`, {
+          columns: columnDefinitions,
+          pluginName: this.pluginName
+        });
+      }
     }
     return this as unknown as UniversalRowDetailView;
   }