fix: onBeforeSort should be cancellable from Header Menu (#2610)

Author: ghiscoding

frameworks/angular-slickgrid/src/library/components/angular-slickgrid-outputs.interface.ts

@@ -1,6 +1,5 @@
 import type {
   Column,
-  ColumnSort,
   CurrentFilter,
   CurrentSorter,
   DragRowMove,
@@ -50,6 +49,7 @@ import type {
   OnHeaderMouseEventArgs,
   OnHeaderRowCellRenderedEventArgs,
   OnKeyDownEventArgs,
+  OnLocalSortChangedArgs,
   OnRenderedEventArgs,
   OnRowCountChangedEventArgs,
   OnRowsChangedEventArgs,
@@ -190,7 +190,7 @@ export interface AngularSlickgridOutputs {
   onBeforeFilterChange: (e: CurrentFilter[]) => void;
   onBeforeFilterClear: (e: { columnId: string } | boolean) => void;
   onBeforeSearchChange: (e: OnSearchChangeEventArgs) => boolean | void;
-  onBeforeSortChange: (e: Array<ColumnSort & { clearSortTriggered?: boolean }>) => void;
+  onBeforeSortChange: (e: OnLocalSortChangedArgs) => void;
   onContextMenuClearGrouping: () => void;
   onContextMenuCollapseAllGroups: () => void;
   onContextMenuExpandAllGroups: () => void;

frameworks/slickgrid-react/src/components/slickgridReactProps.ts

@@ -1,6 +1,5 @@
 import type {
   Column,
-  ColumnSort,
   ContainerService,
   CurrentFilter,
   CurrentSorter,
@@ -53,6 +52,7 @@ import type {
   OnHeaderMouseEventArgs,
   OnHeaderRowCellRenderedEventArgs,
   OnKeyDownEventArgs,
+  OnLocalSortChangedArgs,
   OnRenderedEventArgs,
   OnRowCountChangedEventArgs,
   OnRowsChangedEventArgs,
@@ -186,7 +186,7 @@ export interface SlickgridReactProps {
   onBeforeFilterChange?: ReactRegularEventHandler<CurrentFilter[]>;
   onBeforeFilterClear?: ReactRegularEventHandler<{ columnId: string } | boolean>;
   onBeforeSearchChange?: ReactRegularEventHandler<OnSearchChangeEventArgs, boolean | void>;
-  onBeforeSortChange?: ReactRegularEventHandler<Array<ColumnSort & { clearSortTriggered?: boolean }>>;
+  onBeforeSortChange?: ReactRegularEventHandler<OnLocalSortChangedArgs>;
   onContextMenuClearGrouping?: ReactRegularEventHandler<void>;
   onContextMenuCollapseAllGroups?: ReactRegularEventHandler<void>;
   onContextMenuExpandAllGroups?: ReactRegularEventHandler<void>;

frameworks/slickgrid-vue/src/components/slickgridVueProps.interface.ts

@@ -1,6 +1,5 @@
 import type {
   Column,
-  ColumnSort,
   CurrentFilter,
   CurrentSorter,
   DragRowMove,
@@ -51,6 +50,7 @@ import type {
   OnHeaderMouseEventArgs,
   OnHeaderRowCellRenderedEventArgs,
   OnKeyDownEventArgs,
+  OnLocalSortChangedArgs,
   OnRenderedEventArgs,
   OnRowCountChangedEventArgs,
   OnRowsChangedEventArgs,
@@ -176,7 +176,7 @@ export interface SlickgridVueProps {
   onOnBeforeFilterChange?: VueRegularEventHandler<CurrentFilter[]>;
   onOnBeforeFilterClear?: VueRegularEventHandler<{ columnId: string } | boolean>;
   onOnBeforeSearchChange?: VueRegularEventHandler<OnSearchChangeEventArgs, boolean | void>;
-  onOnBeforeSortChange?: VueRegularEventHandler<Array<ColumnSort & { clearSortTriggered?: boolean }>>;
+  onOnBeforeSortChange?: VueRegularEventHandler<OnLocalSortChangedArgs>;
   onOnContextMenuClearGrouping?: VueRegularEventHandler<void>;
   onOnContextMenuCollapseAllGroups?: VueRegularEventHandler<void>;
   onOnContextMenuExpandAllGroups?: VueRegularEventHandler<void>;

packages/common/src/extensions/__tests__/slickHeaderMenu.spec.ts

@@ -64,6 +64,7 @@ const gridStub = {
   updateColumns: vi.fn(),
   validateColumnFreezeWidth: vi.fn(),
   validateColumnFreeze: vi.fn(),
+  onBeforeSort: new SlickEvent(),
   onBeforeSetColumns: new SlickEvent(),
   onBeforeHeaderCellDestroy: new SlickEvent(),
   onClick: new SlickEvent(),
@@ -1231,7 +1232,7 @@ describe('HeaderMenu Plugin', () => {
 
         // Verify slotRenderer was called with the click event as the third argument
         expect(mockSlotRenderer).toHaveBeenCalledTimes(2); // once for render, once for click
-        const clickCallArgs = mockSlotRenderer.mock.calls[1]; // second call is from click
+        const clickCallArgs: any[] = mockSlotRenderer.mock.calls[1]; // second call is from click
         expect(clickCallArgs[2]).toBeDefined();
         expect(clickCallArgs[2]!.type).toBe('click');
       });
@@ -2104,6 +2105,42 @@ describe('HeaderMenu Plugin', () => {
         expect(sharedService.slickGrid.setSortColumns).toHaveBeenCalled();
       });
 
+      it('should NOT trigger the command "sort-desc" and expect Sort Service to call "onBackendSortChanged" being called without the sorted column', () => {
+        const mockSortedCols: ColumnSort[] = [
+          { columnId: 'field1', sortAsc: true, sortCol: { id: 'field1', field: 'field1' } },
+          { columnId: 'field2', sortAsc: true, sortCol: { id: 'field2', field: 'field2' } },
+        ];
+        const mockSortedOuput: ColumnSort[] = [
+          { columnId: 'field1', sortAsc: true, sortCol: { id: 'field1', field: 'field1' } },
+          { columnId: 'field2', sortAsc: false, sortCol: { id: 'field2', field: 'field2' } },
+        ];
+        const previousSortSpy = vi.spyOn(sortServiceStub, 'getCurrentColumnSorts').mockReturnValue([mockSortedCols[0]]);
+        const backendSortSpy = vi.spyOn(sortServiceStub, 'onBackendSortChanged');
+        vi.spyOn(SharedService.prototype, 'gridOptions', 'get').mockReturnValue({
+          ...gridOptionsMock,
+          enableSorting: true,
+          headerMenu: { hideFreezeColumnsCommand: true, hideColumnHideCommand: true, hideColumnResizeByContentCommand: true },
+        });
+
+        // cancel onBeforeSort event
+        const sed = new SlickEventData();
+        vi.spyOn(sed, 'getReturnValue').mockReturnValueOnce(false);
+        vi.spyOn(gridStub.onBeforeSort, 'notify').mockReturnValue(sed);
+
+        gridStub.onBeforeSetColumns.notify({ previousColumns: [], newColumns: columnsMock, grid: gridStub }, eventData as any, gridStub);
+        gridStub.onHeaderCellRendered.notify({ column: columnsMock[1], node: headerDiv, grid: gridStub }, eventData as any, gridStub);
+        const headerButtonElm = headerDiv.querySelector('.slick-header-menu-button') as HTMLDivElement;
+        headerButtonElm.dispatchEvent(new Event('click', { bubbles: true, cancelable: true, composed: false }));
+        const commandDivElm = gridContainerDiv.querySelector('[data-command="sort-desc"]') as HTMLDivElement;
+
+        const clickEvent = new Event('click');
+        commandDivElm.dispatchEvent(clickEvent);
+        expect(previousSortSpy).toHaveBeenCalled();
+        mockSortedOuput[1].sortCol = { ...columnsMock[1], ...mockSortedOuput[1].sortCol }; // merge with column header menu
+        expect(backendSortSpy).not.toHaveBeenCalledWith(expect.anything(), { multiColumnSort: true, sortCols: mockSortedOuput, grid: gridStub });
+        expect(sharedService.slickGrid.setSortColumns).not.toHaveBeenCalled();
+      });
+
       it('should trigger the command "sort-desc" and expect Sort Service to call "onLocalSortChanged" being called without the sorted column', () => {
         sharedService.dataView = dataViewStub;
         const mockSortedCols: ColumnSort[] = [
@@ -2165,6 +2202,42 @@ describe('HeaderMenu Plugin', () => {
         expect(gridSortSpy).toHaveBeenCalledWith(mockSortedOuput);
         expect(gridStub.setSortColumns).toHaveBeenCalled();
       });
+
+      it('should NOT trigger the command "sort-desc" and expect "onSort" event triggered when no DataView is provided', () => {
+        sharedService.dataView = undefined as any;
+        const mockSortedCols: ColumnSort[] = [
+          { columnId: 'field1', sortAsc: true, sortCol: { id: 'field1', field: 'field1' } },
+          { columnId: 'field2', sortAsc: true, sortCol: { id: 'field2', field: 'field2' } },
+        ];
+        const mockSortedOuput: ColumnSort[] = [
+          { columnId: 'field1', sortAsc: true, sortCol: { id: 'field1', field: 'field1' } },
+          { columnId: 'field2', sortAsc: false, sortCol: { id: 'field2', field: 'field2' } },
+        ];
+        const previousSortSpy = vi.spyOn(sortServiceStub, 'getCurrentColumnSorts').mockReturnValue([mockSortedCols[0]]);
+        const gridSortSpy = vi.spyOn(gridStub.onSort, 'notify');
+        vi.spyOn(SharedService.prototype, 'gridOptions', 'get').mockReturnValue({
+          ...gridOptionsMock,
+          enableSorting: true,
+          backendServiceApi: undefined,
+          headerMenu: { hideFreezeColumnsCommand: true, hideColumnHideCommand: true, hideColumnResizeByContentCommand: true },
+        });
+
+        // cancel onBeforeSort event
+        const sed = new SlickEventData();
+        vi.spyOn(sed, 'getReturnValue').mockReturnValueOnce(false);
+        vi.spyOn(gridStub.onBeforeSort, 'notify').mockReturnValue(sed);
+
+        gridStub.onBeforeSetColumns.notify({ previousColumns: [], newColumns: columnsMock, grid: gridStub }, eventData as any, gridStub);
+        gridStub.onHeaderCellRendered.notify({ column: columnsMock[1], node: headerDiv, grid: gridStub }, eventData as any, gridStub);
+        const headerButtonElm = headerDiv.querySelector('.slick-header-menu-button') as HTMLDivElement;
+        headerButtonElm.dispatchEvent(new Event('click', { bubbles: true, cancelable: true, composed: false }));
+        gridContainerDiv.querySelector('[data-command="sort-desc"]')!.dispatchEvent(new Event('click'));
+        expect(previousSortSpy).toHaveBeenCalled();
+        mockSortedOuput[1].sortCol = { ...columnsMock[1], ...mockSortedOuput[1].sortCol }; // merge with column header menu
+        expect(previousSortSpy).toHaveBeenCalled();
+        expect(gridSortSpy).not.toHaveBeenCalledWith(mockSortedOuput);
+        expect(gridStub.setSortColumns).not.toHaveBeenCalled();
+      });
     });
   });
 });

packages/common/src/extensions/slickHeaderMenu.ts

@@ -15,6 +15,7 @@ import type {
   MenuCommandItemCallbackArgs,
   MultiColumnSort,
   OnHeaderCellRenderedEventArgs,
+  SingleColumnSort,
 } from '../interfaces/index.js';
 import type { FilterService } from '../services/filter.service.js';
 import { getTranslationPrefix } from '../services/index.js';
@@ -725,54 +726,63 @@ export class SlickHeaderMenu extends MenuBaseClass<HeaderMenu> {
     if (args?.column) {
       // get previously sorted columns
       const columnDef = args.column;
+      const hasBackendServiceApi = !!this.sharedService.gridOptions.backendServiceApi;
+      const isMultiColumnSort = !!this.sharedService.gridOptions.multiColumnSort;
 
       // 1- get the sort columns without the current column, in the case of a single sort that would equal to an empty array
-      // prettier-ignore
-      const tmpSortedColumns = !this.sharedService.gridOptions.multiColumnSort ? [] : this.sortService.getCurrentColumnSorts(columnDef.id + '');
+      const tmpSortedColumns = !isMultiColumnSort ? [] : this.sortService.getCurrentColumnSorts(columnDef.id + '');
 
-      let emitterType: EmitterType = 'local';
+      const sortArgs = {
+        multiColumnSort: hasBackendServiceApi || isMultiColumnSort,
+        grid: this.grid,
+      } as SingleColumnSort | MultiColumnSort;
 
-      // 2- add to the column array, the new sorted column by the header menu
-      tmpSortedColumns.push({ columnId: columnDef.id, sortCol: columnDef, sortAsc: isSortingAsc });
-
-      if (this.sharedService.gridOptions.backendServiceApi) {
-        this.sortService.onBackendSortChanged(event, {
-          multiColumnSort: true,
-          sortCols: tmpSortedColumns,
-          grid: this.grid,
-        });
-        emitterType = 'remote';
+      if (hasBackendServiceApi) {
+        (sortArgs as MultiColumnSort).sortCols = tmpSortedColumns;
       } else if (this.sharedService.dataView) {
-        this.sortService.onLocalSortChanged(this.grid, tmpSortedColumns);
-        emitterType = 'local';
-      } else {
-        // when using customDataView, we will simply send it as a onSort event with notify
-        args.grid.onSort.notify(tmpSortedColumns as unknown as MultiColumnSort);
+        (sortArgs as SingleColumnSort).sortCol = columnDef;
       }
 
-      // update the sharedService.slickGrid sortColumns array which will at the same add the visual sort icon(s) on the UI
-      const newSortColumns = tmpSortedColumns.map((col) => {
-        return {
-          columnId: col?.sortCol?.id ?? '',
-          sortAsc: col?.sortAsc ?? true,
-        };
-      });
+      if (this.grid.onBeforeSort.notify(sortArgs).getReturnValue() !== false) {
+        let emitterType: EmitterType = 'local';
+        // 2- add to the column array, the new sorted column by the header menu
+        tmpSortedColumns.push({ columnId: columnDef.id, sortCol: columnDef, sortAsc: isSortingAsc });
+
+        if (hasBackendServiceApi) {
+          this.sortService.onBackendSortChanged(event, sortArgs);
+          emitterType = 'remote';
+        } else if (this.sharedService.dataView) {
+          this.sortService.onLocalSortChanged(this.grid, tmpSortedColumns);
+          emitterType = 'local';
+        } else {
+          // when using customDataView, we will simply send it as a onSort event with notify
+          args.grid.onSort.notify(tmpSortedColumns as unknown as MultiColumnSort);
+        }
 
-      // add sort icon in UI
-      this.grid.setSortColumns(newSortColumns);
-
-      // if we have an emitter type set, we will emit a sort changed
-      // for the Grid State Service to see the change.
-      // We also need to pass current sorters changed to the emitSortChanged method
-      if (emitterType) {
-        const currentLocalSorters: CurrentSorter[] = [];
-        newSortColumns.forEach((sortCol) => {
-          currentLocalSorters.push({
-            columnId: `${sortCol.columnId}`,
-            direction: sortCol.sortAsc ? 'ASC' : 'DESC',
-          });
+        // update the sharedService.slickGrid sortColumns array which will at the same add the visual sort icon(s) on the UI
+        const newSortColumns = tmpSortedColumns.map((col) => {
+          return {
+            columnId: col?.sortCol?.id ?? '',
+            sortAsc: col?.sortAsc ?? true,
+          };
         });
-        this.sortService.emitSortChanged(emitterType, currentLocalSorters);
+
+        // add sort icon in UI
+        this.grid.setSortColumns(newSortColumns);
+
+        // if we have an emitter type set, we will emit a sort changed
+        // for the Grid State Service to see the change.
+        // We also need to pass current sorters changed to the emitSortChanged method
+        if (emitterType) {
+          const currentLocalSorters: CurrentSorter[] = [];
+          newSortColumns.forEach((sortCol) => {
+            currentLocalSorters.push({
+              columnId: `${sortCol.columnId}`,
+              direction: sortCol.sortAsc ? 'ASC' : 'DESC',
+            });
+          });
+          this.sortService.emitSortChanged(emitterType, currentLocalSorters);
+        }
       }
     }
   }

packages/common/src/interfaces/index.ts

@@ -23,7 +23,6 @@ export type * from './columnExcelExportOption.interface.js';
 export type * from './columnFilter.interface.js';
 export type * from './columnFilters.interface.js';
 export type * from './columnPicker.interface.js';
-export type * from './columnSort.interface.js';
 export type * from './componentEvents.interface.js';
 export type * from './compositeEditorError.interface.js';
 export type * from './compositeEditorLabel.interface.js';
@@ -115,7 +114,6 @@ export type * from './menuOptionItemCallbackArgs.interface.js';
 export type * from './metrics.interface.js';
 export type * from './metricTexts.interface.js';
 export type * from './mouseOffsetViewport.interface.js';
-export type * from './multiColumnSort.interface.js';
 export type * from './onEventArgs.interface.js';
 export type * from './onValidationErrorResult.interface.js';
 export type * from './operatorDetail.interface.js';
@@ -137,12 +135,12 @@ export type * from './searchColumnFilter.interface.js';
 export type * from './selectableOverrideCallback.interface.js';
 export type * from './selectionModelOption.interface.js';
 export type * from './selectOption.interface.js';
-export type * from './singleColumnSort.interface.js';
 export type * from './slickPlugin.interface.js';
 export type * from './slickRemoteModel.interface.js';
 export type * from './slickResizer.interface.js';
 export type * from './slickRowDetailView.interface.js';
 export type * from './sliderOption.interface.js';
+export type * from './sort.interfaces.js';
 export type * from './sorter.interface.js';
 export type * from './textExportOption.interface.js';
 export type * from './treeDataOption.interface.js';

packages/common/src/interfaces/multiColumnSort.interface.ts

@@ -0,0 +1,41 @@
+import type { SlickGrid } from '../core/index.js';
+import type { Column } from './column.interface.js';
+
+export interface ColumnSort {
+  /** Column Id to be sorted */
+  columnId: string | number;
+
+  /** Are we sorting in Ascending order? It will default to ascending when this property is undefined */
+  sortAsc?: boolean;
+
+  /** Column to be sorted */
+  sortCol?: Column;
+}
+
+export interface MultiColumnSort {
+  /** SlickGrid grid object */
+  grid: SlickGrid;
+
+  /** is it a multi-column sort? */
+  multiColumnSort: true;
+
+  /** Array of Columns to be sorted */
+  sortCols: ColumnSort[];
+
+  /** previous sort columns before calling onSort */
+  previousSortColumns?: ColumnSort[];
+}
+
+export interface SingleColumnSort extends ColumnSort {
+  /** SlickGrid grid object */
+  grid?: SlickGrid;
+
+  /** is it a multi-column sort? */
+  multiColumnSort?: false;
+
+  /** previous sort columns before calling onSort */
+  previousSortColumns?: ColumnSort[];
+}
+
+export type OnBackendSortChangedArgs = (SingleColumnSort | MultiColumnSort) & { clearSortTriggered?: boolean };
+export type OnLocalSortChangedArgs = Array<ColumnSort & { clearSortTriggered?: boolean }>;