From f94de107b4ef19ee2b77173350d1d11d11a00244 Mon Sep 17 00:00:00 2001 From: amaannawab923 Date: Tue, 7 Jul 2026 01:21:26 +0530 Subject: [PATCH] fix(ag-grid): persist "None" value aggregation selection (#41386) Co-authored-by: Enzo Martellucci <52219496+EnxDev@users.noreply.github.com> Co-authored-by: Enzo Martellucci Co-authored-by: Claude Fable 5 (cherry picked from commit fb496e158a2a9ab84387239286293b58b1af7f6b) --- .../src/AgGridTable/index.tsx | 11 +-- .../src/utils/getColumnStateSignature.ts | 51 ++++++++++++ .../src/utils/reconcileColumnState.test.ts | 17 ++++ .../utils/getColumnStateSignature.test.ts | 82 +++++++++++++++++++ 4 files changed, 156 insertions(+), 5 deletions(-) create mode 100644 superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getColumnStateSignature.ts create mode 100644 superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getColumnStateSignature.test.ts diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx index 956199b18c4..bfc7ad9c018 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx @@ -57,6 +57,7 @@ import { SearchOption, SortByItem } from '../types'; import getInitialSortState, { shouldSort } from '../utils/getInitialSortState'; import getInitialFilterModel from '../utils/getInitialFilterModel'; import reconcileColumnState from '../utils/reconcileColumnState'; +import getColumnStateSignature from '../utils/getColumnStateSignature'; import { PAGE_SIZE_OPTIONS } from '../consts'; import { getCompleteFilterState } from '../utils/filterStateManager'; @@ -335,11 +336,11 @@ const AgGridDataTable: FunctionComponent = memo( timestamp: Date.now(), }; - const stateHash = JSON.stringify({ - columnOrder: columnState.map(c => c.colId), - sorts: sortModel, - filters: filterModel, - }); + const stateHash = getColumnStateSignature( + columnState, + sortModel, + filterModel, + ); if (stateHash !== lastCapturedStateRef.current) { lastCapturedStateRef.current = stateHash; diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getColumnStateSignature.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getColumnStateSignature.ts new file mode 100644 index 00000000000..239c7eba190 --- /dev/null +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getColumnStateSignature.ts @@ -0,0 +1,51 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import type { ColumnState } from '@superset-ui/core/components/ThemedAgGridReact'; + +type SortModelEntry = { + colId: string; + sort: 'asc' | 'desc'; + sortIndex: number; +}; + +/** + * Stable signature of the persisted parts of AG Grid column state (order, + * value aggregation, sorts, filters), used to detect changes worth saving. + * `aggFunc` is normalized so the "None" option (`null`/`undefined`) produces + * a distinct signature instead of reverting to the default on reload. + */ +export default function getColumnStateSignature( + columnState: ColumnState[], + sortModel: SortModelEntry[], + filterModel: Record, +): string { + return JSON.stringify({ + columnOrder: columnState.map(col => col.colId), + aggregations: columnState.map(col => ({ + colId: col.colId, + // "None" (null/undefined) maps to an explicit sentinel; functions map + // to a fixed marker so the signature never depends on JSON.stringify + // dropping them (defensive: persisted aggFuncs are always strings). + aggFunc: + typeof col.aggFunc === 'function' ? 'custom' : (col.aggFunc ?? null), + })), + sorts: sortModel, + filters: filterModel, + }); +} diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/reconcileColumnState.test.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/reconcileColumnState.test.ts index 78e0e79b2da..814b85b2a4c 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/reconcileColumnState.test.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/reconcileColumnState.test.ts @@ -84,3 +84,20 @@ test('drops stale order when a dynamic group by swaps the dimension column', () ], }); }); + +test('preserves per-column aggFunc, including explicit "None" (null), through reconciliation', () => { + // Regression #107166: saved aggFunc must reach applyColumnState untouched. + const colDefs: ColDef[] = [{ field: 'name' }, { field: 'SUM(Sales_Amount)' }]; + const savedColumnState: ColumnState[] = [ + { colId: 'name' }, + { colId: 'SUM(Sales_Amount)', aggFunc: null }, + ]; + + expect(reconcileColumnState(savedColumnState, colDefs)).toEqual({ + applyOrder: true, + columnState: [ + { colId: 'name' }, + { colId: 'SUM(Sales_Amount)', aggFunc: null }, + ], + }); +}); diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getColumnStateSignature.test.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getColumnStateSignature.test.ts new file mode 100644 index 00000000000..3c9e894463f --- /dev/null +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getColumnStateSignature.test.ts @@ -0,0 +1,82 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import type { ColumnState } from '@superset-ui/core/components/ThemedAgGridReact'; +import getColumnStateSignature from '../../src/utils/getColumnStateSignature'; + +const filterModel = {}; + +test('signature changes when a column value aggregation changes', () => { + const before = getColumnStateSignature( + [{ colId: 'sales', aggFunc: 'sum' }], + [], + filterModel, + ); + const after = getColumnStateSignature( + [{ colId: 'sales', aggFunc: 'avg' }], + [], + filterModel, + ); + expect(after).not.toEqual(before); +}); + +test('switching value aggregation to "None" (null/undefined) changes the signature', () => { + // Regression #107166: "None" must be captured so it persists on reload. + const sumState = getColumnStateSignature( + [{ colId: 'sales', aggFunc: 'sum' }], + [], + filterModel, + ); + const noneNull = getColumnStateSignature( + [{ colId: 'sales', aggFunc: null }], + [], + filterModel, + ); + const noneUndefined = getColumnStateSignature( + [{ colId: 'sales' }], + [], + filterModel, + ); + + expect(noneNull).not.toEqual(sumState); + expect(noneUndefined).not.toEqual(sumState); + // null and undefined both represent "None" and must be treated identically. + expect(noneNull).toEqual(noneUndefined); +}); + +test('signature is stable when nothing changes', () => { + const columnState: ColumnState[] = [{ colId: 'sales', aggFunc: 'sum' }]; + const a = getColumnStateSignature(columnState, [], filterModel); + const b = getColumnStateSignature([...columnState], [], filterModel); + expect(a).toEqual(b); +}); + +test('a function aggFunc is distinguishable from "None"', () => { + // Pins the defensive marker: function aggFuncs stay distinct from "None". + const customAgg = getColumnStateSignature( + [{ colId: 'sales', aggFunc: () => 42 }], + [], + filterModel, + ); + const none = getColumnStateSignature( + [{ colId: 'sales', aggFunc: null }], + [], + filterModel, + ); + expect(customAgg).not.toEqual(none); +});