Skip to content

Commit

Permalink
fix(sorting): multi-column sort shouldn't work when option is disabled
Browse files Browse the repository at this point in the history
- with Tree Data only supporting single sort, I found out that we could bypass the single sort by using the header menu
- also found that sort presets with multiple columns were also loading as multiple sort even when the multiColumnSort is disabled and now it's fixed
  • Loading branch information
ghiscoding committed May 21, 2021
1 parent 574d5c9 commit 9841155
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -945,17 +945,29 @@ describe('Angular-Slickgrid Custom Component instantiated via Constructor', () =
expect(spy).toHaveBeenCalledWith(mockFilters, true);
});

it('should call the "updateSorters" method when filters are defined in the "presets" property', () => {
it('should call the "updateSorters" method when sorters are defined in the "presets" property with multi-column sort enabled', () => {
jest.spyOn(mockGrid, 'getSelectionModel').mockReturnValue(true);
const spy = jest.spyOn(mockGraphqlService, 'updateSorters');
const mockSorters = [{ columnId: 'name', direction: 'asc' }] as CurrentSorter[];
const mockSorters = [{ columnId: 'firstName', direction: 'asc' }, { columnId: 'lastName', direction: 'desc' }] as CurrentSorter[];

component.gridOptions.presets = { sorters: mockSorters };
component.ngAfterViewInit();

expect(spy).toHaveBeenCalledWith(undefined, mockSorters);
});

it('should call the "updateSorters" method with ONLY 1 column sort when multi-column sort is disabled and user provided multiple sorters in the "presets" property', () => {
jest.spyOn(mockGrid, 'getSelectionModel').mockReturnValue(true as any);
const spy = jest.spyOn(mockGraphqlService, 'updateSorters');
const mockSorters = [{ columnId: 'firstName', direction: 'asc' }, { columnId: 'lastName', direction: 'desc' }] as CurrentSorter[];

component.gridOptions.multiColumnSort = false;
component.gridOptions.presets = { sorters: mockSorters };
component.ngAfterViewInit();

expect(spy).toHaveBeenCalledWith(undefined, [mockSorters[0]]);
});

it('should call the "updatePagination" method when filters are defined in the "presets" property', () => {
const spy = jest.spyOn(mockGraphqlService, 'updatePagination');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -719,7 +719,9 @@ export class AngularSlickgridComponent implements AfterViewInit, OnDestroy, OnIn
}
// Sorters "presets"
if (backendApiService.updateSorters && Array.isArray(gridOptions.presets.sorters) && gridOptions.presets.sorters.length > 0) {
backendApiService.updateSorters(undefined, gridOptions.presets.sorters);
// when using multi-column sort, we can have multiple but on single sort then only grab the first sort provided
const sortColumns = this.gridOptions.multiColumnSort ? gridOptions.presets.sorters : gridOptions.presets.sorters.slice(0, 1);
backendApiService.updateSorters(undefined, sortColumns);
}
// Pagination "presets"
if (backendApiService.updatePagination && gridOptions.presets.pagination) {
Expand Down Expand Up @@ -794,7 +796,9 @@ export class AngularSlickgridComponent implements AfterViewInit, OnDestroy, OnIn
// if user entered some Sort "presets", we need to reflect them all in the DOM
if (gridOptions.enableSorting) {
if (gridOptions.presets && Array.isArray(gridOptions.presets.sorters)) {
this.sortService.loadGridSorters(gridOptions.presets.sorters);
// when using multi-column sort, we can have multiple but on single sort then only grab the first sort provided
const sortColumns = this.gridOptions.multiColumnSort ? gridOptions.presets.sorters : gridOptions.presets.sorters.slice(0, 1);
this.sortService.loadGridSorters(sortColumns);
}
}
}
Expand Down
26 changes: 13 additions & 13 deletions src/app/modules/angular-slickgrid/extensions/headerMenuExtension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -400,31 +400,31 @@ export class HeaderMenuExtension implements Extension {
if (args && args.column) {
// get previously sorted columns
const columnDef = args.column;
const sortedColsWithoutCurrent: ColumnSort[] = this.sortService.getCurrentColumnSorts(args.column.id + '');

// 1- get the sort columns without the current column, in the case of a single sort that would equal to an empty array
const tmpSortedColumns = !this.sharedService.gridOptions.multiColumnSort ? [] : this.sortService.getCurrentColumnSorts(columnDef.id + '');

let emitterType: EmitterType = EmitterType.local;

// add to the column array, the column sorted by the header menu
sortedColsWithoutCurrent.push({ columnId: columnDef.id, sortCol: columnDef, sortAsc: isSortingAsc });
// 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: sortedColsWithoutCurrent, grid: this.sharedService.grid });
this.sortService.onBackendSortChanged(event, { multiColumnSort: true, sortCols: tmpSortedColumns, grid: this.sharedService.grid });
emitterType = EmitterType.remote;
} else if (this.sharedService.dataView) {
this.sortService.onLocalSortChanged(this.sharedService.grid, sortedColsWithoutCurrent);
this.sortService.onLocalSortChanged(this.sharedService.grid, tmpSortedColumns);
emitterType = EmitterType.local;
} else {
// when using customDataView, we will simply send it as a onSort event with notify
const isMultiSort = this.sharedService && this.sharedService.gridOptions && this.sharedService.gridOptions.multiColumnSort || false;
const sortOutput = isMultiSort ? sortedColsWithoutCurrent : sortedColsWithoutCurrent[0];
args.grid.onSort.notify(sortOutput);
args.grid.onSort.notify(tmpSortedColumns);
}

// update the this.sharedService.gridObj sortColumns array which will at the same add the visual sort icon(s) on the UI
const newSortColumns: ColumnSort[] = sortedColsWithoutCurrent.map((col) => {
// update the sharedService.grid sortColumns array which will at the same add the visual sort icon(s) on the UI
const newSortColumns = tmpSortedColumns.map(col => {
return {
columnId: col && col.sortCol && col.sortCol.id,
sortAsc: col && col.sortAsc,
sortCol: col && col.sortCol,
columnId: col?.sortCol?.id ?? '',
sortAsc: col?.sortAsc ?? true,
};
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ describe('SortService', () => {
service.bindLocalOnSort(gridStub);
const columnSorts = service.getCurrentColumnSorts();

expect(columnSorts).toEqual([{ sortCol: { id: 'firstName', field: 'firstName' }, sortAsc: true }]);
expect(columnSorts).toEqual([{ columnId: 'firstName', sortCol: { id: 'firstName', field: 'firstName' }, sortAsc: true }]);
});

it('should return the second sorted column without the first column since it was an exclusion', () => {
Expand All @@ -551,7 +551,7 @@ describe('SortService', () => {
service.bindLocalOnSort(gridStub);
const columnSorts = service.getCurrentColumnSorts('firstName');

expect(columnSorts).toEqual([{ sortCol: { id: 'lastName', field: 'lastName' }, sortAsc: false }]);
expect(columnSorts).toEqual([{ columnId: 'lastName', sortCol: { id: 'lastName', field: 'lastName' }, sortAsc: false }]);
});
});

Expand Down Expand Up @@ -634,6 +634,7 @@ describe('SortService', () => {
describe('toggleSortFunctionality method', () => {
beforeEach(() => {
gridOptionMock.enableSorting = true;
gridOptionMock.multiColumnSort = true;
});

it('should toggle the Sorting', () => {
Expand Down Expand Up @@ -673,7 +674,7 @@ describe('SortService', () => {
jest.spyOn(gridStub, 'getColumns').mockReturnValue(mockColumns);
});

it('should load local grid presets', () => {
it('should load local grid multiple presets sorting when multiColumnSort is enabled', () => {
const spySetCols = jest.spyOn(gridStub, 'setSortColumns');
const spySortChanged = jest.spyOn(service, 'onLocalSortChanged');
const expectation = [
Expand All @@ -690,6 +691,22 @@ describe('SortService', () => {
]);
expect(spySortChanged).toHaveBeenCalledWith(gridStub, expectation);
});

it('should load local grid with only a single sort when multiColumnSort is disabled even when passing multiple column sorters', () => {
const spySetCols = jest.spyOn(gridStub, 'setSortColumns');
const spySortChanged = jest.spyOn(service, 'onLocalSortChanged');
const expectation = [
{ columnId: 'firstName', sortAsc: true, sortCol: { id: 'firstName', field: 'firstName' } },
{ columnId: 'lastName', sortAsc: false, sortCol: { id: 'lastName', field: 'lastName' } },
];

gridOptionMock.multiColumnSort = false;
service.bindLocalOnSort(gridStub);
service.loadGridSorters(gridOptionMock.presets!.sorters as CurrentSorter[]);

expect(spySetCols).toHaveBeenCalledWith([{ columnId: 'firstName', sortAsc: true }]);
expect(spySortChanged).toHaveBeenCalledWith(gridStub, [expectation[0]]);
});
});

describe('undefined getColumns & getOptions', () => {
Expand Down Expand Up @@ -877,6 +894,7 @@ describe('SortService', () => {
gridStub.getOptions = () => gridOptionMock;
gridOptionMock.enableSorting = true;
gridOptionMock.backendServiceApi = undefined;
gridOptionMock.multiColumnSort = true;

mockNewSorters = [
{ columnId: 'firstName', direction: 'ASC' },
Expand Down
8 changes: 5 additions & 3 deletions src/app/modules/angular-slickgrid/services/sort.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,15 +260,15 @@ export class SortService {
* If a column is passed as an argument, that will be exclusion so we won't add this column to our output array since it is already in the array.
* The usage of this method is that we want to know the sort prior to calling the next sorting command
*/
getCurrentColumnSorts(excludedColumnId?: string): { sortCol: Column; sortAsc: boolean; }[] {
getCurrentColumnSorts(excludedColumnId?: string): ColumnSort[] {
// getSortColumns() only returns sortAsc & columnId, we want the entire column definition
const oldSortColumns = this._grid && this._grid.getSortColumns();

// get the column definition but only keep column which are not equal to our current column
if (Array.isArray(oldSortColumns)) {
const sortedCols = oldSortColumns.reduce((cols, col) => {
if (!excludedColumnId || col.columnId !== excludedColumnId) {
cols.push({ sortCol: this._columnDefinitions[this._grid.getColumnIndex(col.columnId)], sortAsc: col.sortAsc });
cols.push({ columnId: col.columnId || '', sortCol: this._columnDefinitions[this._grid.getColumnIndex(col.columnId)], sortAsc: col.sortAsc });
}
return cols;
}, []);
Expand All @@ -284,7 +284,9 @@ export class SortService {
const sortCols: ColumnSort[] = [];

if (Array.isArray(sorters)) {
sorters.forEach((sorter: CurrentSorter) => {
const tmpSorters = this._gridOptions.multiColumnSort ? sorters : sorters.slice(0, 1);

tmpSorters.forEach((sorter: CurrentSorter) => {
const gridColumn = this._columnDefinitions.find((col: Column) => col.id === sorter.columnId);
if (gridColumn) {
sortCols.push({
Expand Down

0 comments on commit 9841155

Please sign in to comment.