diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx index 886563472ce3..cd144fb9799f 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx @@ -21,10 +21,11 @@ import fetchMock from 'fetch-mock'; import { storeWithState } from 'spec/fixtures/mockStore'; import mockState from 'spec/fixtures/mockState'; import { sliceId } from 'spec/fixtures/mockChartQueries'; -import { NativeFilterType } from '@superset-ui/core'; +import { ChartCustomizationType, NativeFilterType } from '@superset-ui/core'; import { CHART_TYPE } from '../../util/componentTypes'; import DashboardContainer from './DashboardContainer'; import * as nativeFiltersActions from '../../actions/nativeFilters'; +import * as chartCustomizationActions from '../../actions/chartCustomizationActions'; fetchMock.get('glob:*/csstemplateasyncmodelview/api/read', {}); fetchMock.put('glob:*/api/v1/dashboard/*/colors*', {}); @@ -43,6 +44,16 @@ jest.mock('src/dashboard/containers/DashboardGrid', () => ({ default: () =>
, })); +// DashboardContainer dispatches these on mount, so unit tests stub them. +jest.mock('src/dashboard/actions/dashboardState', () => ({ + ...jest.requireActual('src/dashboard/actions/dashboardState'), + applyDashboardLabelsColorOnLoad: jest.fn(() => () => undefined), + updateDashboardLabelsColor: jest.fn(() => () => undefined), + persistDashboardLabelsColor: jest.fn(() => () => undefined), + ensureSyncedSharedLabelsColors: jest.fn(() => () => undefined), + ensureSyncedLabelsColorMap: jest.fn(() => () => undefined), +})); + const defaultTestFilter = { id: 'FILTER-1', name: 'Test Filter', @@ -438,3 +449,281 @@ test('calculates tabsInScope for filters with tab-scoped charts', async () => { ]), ); }); + +// Chart customization scope tests. + +test('calculates chartsInScope correctly for new-format chart customizations', async () => { + const customizationId = 'CHART_CUSTOMIZATION-1'; + const originalFn = chartCustomizationActions.setInScopeStatusOfCustomizations; + const spy = jest.spyOn( + chartCustomizationActions, + 'setInScopeStatusOfCustomizations', + ); + spy.mockImplementation(args => originalFn(args)); + + try { + const state = { + dashboardInfo: { + ...mockState.dashboardInfo, + metadata: { + ...mockState.dashboardInfo.metadata, + native_filter_configuration: [], + chart_customization_config: [ + { + id: customizationId, + type: 'CHART_CUSTOMIZATION', + name: 'Dynamic Group By', + filterType: 'chart_customization_dynamic_groupby', + targets: [{ datasetId: 1, column: { name: 'status' } }], + scope: { rootPath: ['ROOT_ID'], excluded: [] }, + chartsInScope: [], + defaultDataMask: {}, + controlValues: {}, + }, + ], + }, + }, + nativeFilters: { + filters: { + [customizationId]: { + id: customizationId, + type: 'CHART_CUSTOMIZATION', + chartsInScope: [], + }, + }, + }, + }; + setup(state); + + await waitFor(() => { + expect(spy).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + customizationId, + chartsInScope: [sliceId], + }), + ]), + ); + }); + } finally { + spy.mockRestore(); + } +}); + +test('migrates legacy-format customizations before scope calculation for scope-less items', async () => { + const legacyCustomizationId = 'CHART_CUSTOMIZATION-legacy-1'; + const originalFn = chartCustomizationActions.setInScopeStatusOfCustomizations; + const spy = jest.spyOn( + chartCustomizationActions, + 'setInScopeStatusOfCustomizations', + ); + spy.mockImplementation(args => originalFn(args)); + + try { + const state = { + dashboardInfo: { + ...mockState.dashboardInfo, + metadata: { + ...mockState.dashboardInfo.metadata, + native_filter_configuration: [], + chart_customization_config: [ + { + id: legacyCustomizationId, + customization: { + dataset: 1, + column: 'status', + filterType: 'chart_customization_dynamic_groupby', + name: 'Legacy Group By', + }, + }, + ], + }, + }, + nativeFilters: { + filters: { + [legacyCustomizationId]: { + id: legacyCustomizationId, + chartsInScope: [], + }, + }, + }, + }; + setup(state); + + await waitFor(() => { + expect(spy).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + customizationId: legacyCustomizationId, + chartsInScope: [sliceId], + }), + ]), + ); + }); + } finally { + spy.mockRestore(); + } +}); + +test('preserves legacy chart-specific customizations during scope calculation', async () => { + const legacyCustomizationId = 'CHART_CUSTOMIZATION-legacy-chart-1'; + const originalFn = chartCustomizationActions.setInScopeStatusOfCustomizations; + const spy = jest.spyOn( + chartCustomizationActions, + 'setInScopeStatusOfCustomizations', + ); + spy.mockImplementation(args => originalFn(args)); + + const baseDashboardLayout = mockState.dashboardLayout.present; + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { CHART_ID: _removed, ...cleanLayout } = baseDashboardLayout; + + try { + const state = { + dashboardState: { + ...mockState.dashboardState, + sliceIds: [18, 19], + }, + dashboardLayout: { + ...mockState.dashboardLayout, + present: { + ...cleanLayout, + CHART_ID_1: { + id: 'CHART_ID_1', + type: CHART_TYPE, + meta: { chartId: 18, width: 4, height: 10 }, + parents: ['ROOT_ID', 'GRID_ID', 'ROW_ID'], + }, + CHART_ID_2: { + id: 'CHART_ID_2', + type: CHART_TYPE, + meta: { chartId: 19, width: 4, height: 10 }, + parents: ['ROOT_ID', 'GRID_ID', 'ROW_ID'], + }, + }, + }, + dashboardInfo: { + ...mockState.dashboardInfo, + metadata: { + ...mockState.dashboardInfo.metadata, + native_filter_configuration: [], + chart_customization_config: [ + { + id: legacyCustomizationId, + chartId: 18, + customization: { + dataset: 1, + column: 'status', + filterType: 'chart_customization_dynamic_groupby', + name: 'Legacy Chart Scoped Group By', + }, + }, + ], + }, + }, + nativeFilters: { + filters: { + [legacyCustomizationId]: { + id: legacyCustomizationId, + chartsInScope: [], + }, + }, + }, + }; + setup(state); + + await waitFor(() => { + expect(spy).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + customizationId: legacyCustomizationId, + chartsInScope: [18], + }), + ]), + ); + }); + } finally { + spy.mockRestore(); + } +}); + +test('returns empty scope data for chart customization dividers', async () => { + const dividerId = 'CHART_CUSTOMIZATION_DIVIDER-1'; + const originalFn = chartCustomizationActions.setInScopeStatusOfCustomizations; + const spy = jest.spyOn( + chartCustomizationActions, + 'setInScopeStatusOfCustomizations', + ); + spy.mockImplementation(args => originalFn(args)); + + try { + const state = { + dashboardInfo: { + ...mockState.dashboardInfo, + metadata: { + ...mockState.dashboardInfo.metadata, + native_filter_configuration: [], + chart_customization_config: [ + { + id: dividerId, + type: ChartCustomizationType.Divider, + title: 'Divider', + description: 'Section divider', + }, + ], + }, + }, + nativeFilters: { + filters: { + [dividerId]: { + id: dividerId, + type: ChartCustomizationType.Divider, + }, + }, + }, + }; + setup(state); + + await waitFor(() => { + expect(spy).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + customizationId: dividerId, + chartsInScope: [], + tabsInScope: [], + }), + ]), + ); + }); + } finally { + spy.mockRestore(); + } +}); + +test('does not dispatch setInScopeStatusOfCustomizations when chart_customization_config is empty', async () => { + const spy = jest.spyOn( + chartCustomizationActions, + 'setInScopeStatusOfCustomizations', + ); + + try { + const state = { + dashboardInfo: { + ...mockState.dashboardInfo, + metadata: { + ...mockState.dashboardInfo.metadata, + native_filter_configuration: [], + chart_customization_config: [], + }, + }, + nativeFilters: { filters: {} }, + }; + setup(state); + + await waitFor(() => { + expect(spy).not.toHaveBeenCalled(); + }); + } finally { + spy.mockRestore(); + } +}); diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx index d7f0be5274e8..a9c954aecbf5 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx @@ -63,6 +63,10 @@ import { } from 'src/dashboard/actions/dashboardState'; import { getColorNamespace, resetColors } from 'src/utils/colorScheme'; import { calculateScopes } from 'src/dashboard/util/calculateScopes'; +import { + isLegacyChartCustomizationFormat, + migrateChartCustomization, +} from 'src/dashboard/util/migrateChartCustomization'; import { CHART_TYPE } from 'src/dashboard/util/componentTypes'; import { NATIVE_FILTER_DIVIDER_PREFIX } from '../nativeFilters/FiltersConfigModal/utils'; import { selectFilterConfiguration } from '../nativeFilters/state'; @@ -85,6 +89,37 @@ interface CustomizationScopeData extends ScopeData { customizationId: string; } +function normalizeChartCustomizationsForScopeCalculation( + chartCustomizations: ChartCustomizationConfiguration, + chartIds: number[], +): ChartCustomizationConfiguration { + if (!chartCustomizations.some(isLegacyChartCustomizationFormat)) { + return chartCustomizations; + } + + return chartCustomizations.map(item => { + if (!isLegacyChartCustomizationFormat(item)) { + return item; + } + + const migratedCustomization = migrateChartCustomization(item); + + if (!item.chartId) { + return migratedCustomization; + } + + return { + ...migratedCustomization, + // Legacy items could target a single chart without an explicit scope. + // Preserve that targeting before calculateScopes recomputes chartsInScope. + scope: { + ...migratedCustomization.scope, + excluded: chartIds.filter(chartId => chartId !== item.chartId), + }, + }; + }); +} + export const renderedChartIdsSelector: (state: RootState) => number[] = createSelector([(state: RootState) => state.charts], charts => Object.values(charts) @@ -192,8 +227,14 @@ const DashboardContainer: FC = ({ topLevelTabs }) => { return; } + const normalizedCustomizations = + normalizeChartCustomizationsForScopeCalculation( + chartCustomizations, + chartIds, + ); + const scopes = calculateScopes( - chartCustomizations, + normalizedCustomizations, chartIds, chartLayoutItems, item => item.type === ChartCustomizationType.Divider, diff --git a/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts b/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts index ee987bbeb238..45756b77481f 100644 --- a/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts +++ b/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts @@ -24,7 +24,6 @@ import { ensureIsArray, JsonObject, PartialFilters, - QueryFormExtraFilter, ChartCustomization, } from '@superset-ui/core'; import { @@ -135,9 +134,6 @@ function buildExistingColumnsSet(chart: ChartQueryPayload): Set { const existingColumns = new Set(); const chartType = chart.form_data?.viz_type; - const existingGroupBy = ensureIsArray(chart.form_data?.groupby); - existingGroupBy.forEach((col: string) => existingColumns.add(col)); - const xAxisColumn = chart.form_data?.x_axis; if (xAxisColumn && chartType !== 'heatmap' && chartType !== 'heatmap_v2') { existingColumns.add(xAxisColumn); @@ -251,7 +247,8 @@ function applyChartSpecificGroupBy( groupByFormData.target = limitedColumns[1]; } } else if (['chord'].includes(chartType)) { - groupByFormData.groupby = [...existingGroupBy, ...groupByColumns]; + groupByFormData.groupby = + groupByColumns.length > 0 ? [groupByColumns[0]] : existingGroupBy; } else if (chartType === 'bubble_v2') { const { limitedColumns } = limitColumnsForChartType( chartType, @@ -283,7 +280,6 @@ function processGroupByCustomizations( ): { groupby?: string[]; order_by_cols?: string[]; - filters?: QueryFormExtraFilter[]; x_axis?: string; series?: string; columns?: string[]; @@ -319,7 +315,7 @@ function processGroupByCustomizations( }); const chartType = chart.form_data?.viz_type; - if (isChartWithoutGroupBy(chartType)) { + if (isChartWithoutGroupBy(chartType) || chartType === 'chord') { return {}; } @@ -328,7 +324,6 @@ function processGroupByCustomizations( const xAxisColumn = chart.form_data?.x_axis; const groupByColumns: string[] = []; - const allFilters: QueryFormExtraFilter[] = []; let orderByConfig: string[] | undefined; let heatmapColumnAdded = false; @@ -342,7 +337,9 @@ function processGroupByCustomizations( return; } - const selectedValues = groupByInfo.selectedValues || []; + const selectedValues = (groupByInfo.selectedValues || []).filter( + (value): value is string => typeof value === 'string' && value.length > 0, + ); const columnNames = selectedValues; if (columnNames.length === 0) { @@ -376,16 +373,6 @@ function processGroupByCustomizations( }); } - columnNames.forEach(columnName => { - if (selectedValues.length > 0) { - allFilters.push({ - col: columnName, - op: 'IN', - val: selectedValues, - }); - } - }); - const sortMetric = item.controlValues?.sortMetric; const sortAscending = item.controlValues?.sortAscending; if (sortMetric) { @@ -400,10 +387,6 @@ function processGroupByCustomizations( xAxisColumn, ); - if (allFilters.length > 0) { - groupByFormData.filters = allFilters; - } - if (orderByConfig) { groupByFormData.order_by_cols = orderByConfig; } @@ -548,7 +531,11 @@ export default function getFormDataWithExtraFilters({ const selectedValues = mask.filterState?.value; groupByState[key] = { - selectedValues: Array.isArray(selectedValues) ? selectedValues : [], + selectedValues: Array.isArray(selectedValues) + ? selectedValues + : typeof selectedValues === 'string' + ? [selectedValues] + : [], hasInteracted: mask.filterState?.value !== undefined, }; } diff --git a/superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts b/superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts index d871a877ccc4..3e090134bee3 100644 --- a/superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts +++ b/superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts @@ -20,191 +20,449 @@ import getFormDataWithExtraFilters, { CachedFormDataWithExtraControls, GetFormDataWithExtraFiltersArguments, } from 'src/dashboard/util/charts/getFormDataWithExtraFilters'; +import { ChartCustomizationType } from '@superset-ui/core'; import { sliceId as chartId } from 'spec/fixtures/mockChartQueries'; -// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks -describe('getFormDataWithExtraFilters', () => { - const filterId = 'native-filter-1'; - const mockChart = { - id: chartId, - chartAlert: null, - chartStatus: null, - chartUpdateEndTime: null, - chartUpdateStartTime: 1, - lastRendered: 1, - latestQueryFormData: {}, - sliceFormData: null, - queryController: null, - queriesResponse: null, - triggerQuery: false, - form_data: { - viz_type: 'filter_select', - filters: [ - { - col: 'country_name', - op: 'IN', - val: ['United States'], - }, - ], - datasource: '123', - url_params: {}, - }, +type ChartCustomizationItem = NonNullable< + GetFormDataWithExtraFiltersArguments['chartCustomizationItems'] +>[number]; + +function createChartCustomization( + overrides: Partial = {}, +): ChartCustomizationItem { + return { + id: 'CHART_CUSTOMIZATION-1', + type: ChartCustomizationType.ChartCustomization, + name: 'Dynamic Group By', + filterType: 'chart_customization_dynamic_groupby', + targets: [{ datasetId: 3, column: { name: 'status' } }], + scope: { rootPath: [], excluded: [] }, + chartsInScope: [chartId], + defaultDataMask: {}, + controlValues: {}, + ...overrides, }; - const mockArgs: GetFormDataWithExtraFiltersArguments = { - chartConfiguration: {}, - chart: mockChart, - filters: { - region: ['Spain'], - color: ['pink', 'purple'], - }, - sliceId: chartId, - nativeFilters: {}, - dataMask: { - [filterId]: { - id: filterId, - extraFormData: {}, - filterState: {}, - ownState: {}, +} + +const expectGroupBy = ( + result: CachedFormDataWithExtraControls, + expected: unknown, +) => { + expect('groupby' in result).toBe(true); + if (!('groupby' in result)) { + throw new Error('Expected groupby to be present in form data'); + } + expect(result.groupby).toEqual(expected); +}; + +const expectGroupByLength = ( + result: CachedFormDataWithExtraControls, + length: number, +) => { + expect('groupby' in result).toBe(true); + if (!('groupby' in result)) { + throw new Error('Expected groupby to be present in form data'); + } + expect(result.groupby).toHaveLength(length); +}; + +const getResultFilters = (result: CachedFormDataWithExtraControls) => { + if (!('filters' in result) || !Array.isArray(result.filters)) { + return []; + } + + return result.filters.filter( + ( + filter, + ): filter is { + col: string; + val: unknown[]; + } => + typeof filter === 'object' && + filter !== null && + 'col' in filter && + 'val' in filter && + typeof filter.col === 'string' && + Array.isArray(filter.val), + ); +}; + +const filterId = 'native-filter-1'; +const mockChart = { + id: chartId, + chartAlert: null, + chartStatus: null, + chartUpdateEndTime: null, + chartUpdateStartTime: 1, + lastRendered: 1, + latestQueryFormData: {}, + sliceFormData: null, + queryController: null, + queriesResponse: null, + triggerQuery: false, + form_data: { + viz_type: 'filter_select', + filters: [ + { + col: 'country_name', + op: 'IN', + val: ['United States'], }, + ], + datasource: '123', + url_params: {}, + }, +}; +const mockArgs: GetFormDataWithExtraFiltersArguments = { + chartConfiguration: {}, + chart: mockChart, + filters: { + region: ['Spain'], + color: ['pink', 'purple'], + }, + sliceId: chartId, + nativeFilters: {}, + dataMask: { + [filterId]: { + id: filterId, + extraFormData: {}, + filterState: {}, + ownState: {}, }, - extraControls: { - stack: 'Stacked', - }, - allSliceIds: [chartId], - }; + }, + extraControls: { + stack: 'Stacked', + }, + allSliceIds: [chartId], +}; - test('should include filters from the passed filters', () => { - const result = getFormDataWithExtraFilters(mockArgs); - expect(result.extra_filters).toHaveLength(2); - expect(result.extra_filters[0]).toEqual({ - col: 'region', - op: 'IN', - val: ['Spain'], - }); - expect(result.extra_filters[1]).toEqual({ - col: 'color', - op: 'IN', - val: ['pink', 'purple'], - }); +test('should include filters from the passed filters', () => { + const result = getFormDataWithExtraFilters(mockArgs); + expect(result.extra_filters).toHaveLength(2); + expect(result.extra_filters[0]).toEqual({ + col: 'region', + op: 'IN', + val: ['Spain'], }); - - test('should compose extra control', () => { - const result: CachedFormDataWithExtraControls = - getFormDataWithExtraFilters(mockArgs); - expect(result.stack).toEqual('Stacked'); + expect(result.extra_filters[1]).toEqual({ + col: 'color', + op: 'IN', + val: ['pink', 'purple'], }); +}); - test('should merge extraFormData from chart customizations', () => { - const customizationId = 'CHART_CUSTOMIZATION-1'; - const argsWithCustomization: GetFormDataWithExtraFiltersArguments = { - ...mockArgs, - dataMask: { - [customizationId]: { - id: customizationId, - extraFormData: { - time_grain_sqla: 'PT1H', - }, - filterState: { - value: ['category1', 'category2'], - }, - ownState: {}, +test('should compose extra control', () => { + const result: CachedFormDataWithExtraControls = + getFormDataWithExtraFilters(mockArgs); + expect(result.stack).toEqual('Stacked'); +}); + +test('should merge extraFormData from chart customizations', () => { + const customizationId = 'CHART_CUSTOMIZATION-1'; + const argsWithCustomization: GetFormDataWithExtraFiltersArguments = { + ...mockArgs, + dataMask: { + [customizationId]: { + id: customizationId, + extraFormData: { + time_grain_sqla: 'PT1H', }, + filterState: { + value: ['category1', 'category2'], + }, + ownState: {}, }, - chartCustomizationItems: [ - { - id: customizationId, - type: 'CHART_CUSTOMIZATION' as any, - name: 'Time Grain Customization', - filterType: 'chart_customization_time_grain', - targets: [ - { - datasetId: 123, - column: { name: 'time_column' }, - }, - ], - scope: { - rootPath: [], - excluded: [], + }, + chartCustomizationItems: [ + createChartCustomization({ + id: customizationId, + name: 'Time Grain Customization', + filterType: 'chart_customization_time_grain', + targets: [ + { + datasetId: 123, + column: { name: 'time_column' }, }, - chartsInScope: [chartId], - defaultDataMask: {}, - controlValues: {}, + ], + scope: { + rootPath: [], + excluded: [], }, - ], - }; + chartsInScope: [chartId], + defaultDataMask: {}, + controlValues: {}, + }), + ], + }; - const result = getFormDataWithExtraFilters(argsWithCustomization); - expect((result as any).time_grain_sqla).toEqual('PT1H'); - }); + const result = getFormDataWithExtraFilters(argsWithCustomization); + expect(result).toEqual(expect.objectContaining({ time_grain_sqla: 'PT1H' })); +}); - test('should merge both filters and customization extraFormData', () => { - const customizationId = 'CHART_CUSTOMIZATION-1'; - const argsWithBoth: GetFormDataWithExtraFiltersArguments = { - ...mockArgs, - chartConfiguration: { - [filterId]: { - id: filterId, - targets: [ +test('should merge both filters and customization extraFormData', () => { + const customizationId = 'CHART_CUSTOMIZATION-1'; + const argsWithBoth: GetFormDataWithExtraFiltersArguments = { + ...mockArgs, + activeFilters: { + [filterId]: { + targets: [ + { + datasetId: 123, + column: { name: 'country_name' }, + }, + ], + scope: [chartId], + values: { + filters: [ { - datasetId: 123, - column: { name: 'country_name' }, + col: 'country_name', + op: 'IN', + val: ['United States'], }, ], - scope: { rootPath: ['ROOT_ID'], excluded: [] }, - cascadeParentIds: [], - }, - } as any, - dataMask: { - [filterId]: { - id: filterId, - extraFormData: { - filters: [ - { - col: 'country_name', - op: 'IN', - val: ['United States'], - }, - ], - }, - filterState: {}, - ownState: {}, - }, - [customizationId]: { - id: customizationId, - extraFormData: { - time_grain_sqla: 'PT1H', - }, - filterState: { - value: ['category1'], - }, - ownState: {}, }, }, - chartCustomizationItems: [ - { - id: customizationId, - type: 'CHART_CUSTOMIZATION' as any, - name: 'Time Grain Customization', - filterType: 'chart_customization_time_grain', - targets: [ + }, + dataMask: { + [filterId]: { + id: filterId, + extraFormData: { + filters: [ { - datasetId: 123, - column: { name: 'time_column' }, + col: 'country_name', + op: 'IN', + val: ['United States'], }, ], - scope: { - rootPath: [], - excluded: [], + }, + filterState: {}, + ownState: {}, + }, + [customizationId]: { + id: customizationId, + extraFormData: { + time_grain_sqla: 'PT1H', + }, + filterState: { + value: ['category1'], + }, + ownState: {}, + }, + }, + chartCustomizationItems: [ + createChartCustomization({ + id: customizationId, + name: 'Time Grain Customization', + filterType: 'chart_customization_time_grain', + targets: [ + { + datasetId: 123, + column: { name: 'time_column' }, }, - chartsInScope: [chartId], - defaultDataMask: {}, - controlValues: {}, + ], + scope: { + rootPath: [], + excluded: [], }, - ], - }; + chartsInScope: [chartId], + defaultDataMask: {}, + controlValues: {}, + }), + ], + }; - const result = getFormDataWithExtraFilters(argsWithBoth); - expect((result as any).time_grain_sqla).toEqual('PT1H'); - expect(result.extra_form_data).toBeDefined(); + const result = getFormDataWithExtraFilters(argsWithBoth); + expect(result).toEqual(expect.objectContaining({ time_grain_sqla: 'PT1H' })); + expect(result.extra_form_data).toBeDefined(); +}); + +const makeGroupByArgs = ( + selectedValue: string | string[], + baseGroupby: string[] = [], +): GetFormDataWithExtraFiltersArguments => { + const customizationId = 'CHART_CUSTOMIZATION-groupby-1'; + return { + ...mockArgs, + chart: { + ...mockChart, + form_data: { + ...mockChart.form_data, + viz_type: 'table', + datasource: '3__table', + groupby: baseGroupby, + }, + }, + dataMask: { + [customizationId]: { + id: customizationId, + extraFormData: {}, + filterState: { value: selectedValue }, + ownState: {}, + }, + }, + chartCustomizationItems: [ + createChartCustomization({ + id: customizationId, + }), + ], + }; +}; + +test('dynamic group by does not inject a filter using the selected column name as a value', () => { + const result = getFormDataWithExtraFilters(makeGroupByArgs(['status'])); + const spuriousFilter = getResultFilters(result).find( + filter => filter.col === 'status' && filter.val.includes('status'), + ); + expect(spuriousFilter).toBeUndefined(); + expectGroupBy(result, ['status']); +}); + +test('dynamic group by still applies when the selected column is already in the base groupby', () => { + const result = getFormDataWithExtraFilters( + makeGroupByArgs(['status'], ['status']), + ); + expectGroupBy(result, ['status']); +}); + +test('dynamic group by with no selection leaves the base groupby unchanged', () => { + const result = getFormDataWithExtraFilters(makeGroupByArgs([], ['status'])); + expectGroupBy(result, ['status']); +}); + +test('dynamic group by ignores empty-string selections and keeps the base groupby', () => { + const result = getFormDataWithExtraFilters( + makeGroupByArgs(['', 'status'], ['original_column']), + ); + expectGroupBy(result, ['status']); +}); + +test('chord chart ignores dynamic group by selections and keeps the existing source unchanged', () => { + const result = getFormDataWithExtraFilters({ + ...makeGroupByArgs(['payment_method'], ['status']), + chart: { + ...mockChart, + form_data: { + ...mockChart.form_data, + viz_type: 'chord', + datasource: '3__table', + groupby: ['status'], + }, + }, }); + + expectGroupBy(result, ['status']); +}); + +test('dynamic group by normalizes a single-select string value into a one-item groupby array', () => { + const result = getFormDataWithExtraFilters(makeGroupByArgs('status')); + expectGroupBy(result, ['status']); +}); + +test('structural conflict: metric column blocks groupby override (nonConflictingColumns guard)', () => { + const customizationId = 'CHART_CUSTOMIZATION-groupby-conflict'; + const argsWithMetricConflict: GetFormDataWithExtraFiltersArguments = { + ...mockArgs, + chart: { + ...mockChart, + form_data: { + ...mockChart.form_data, + viz_type: 'table', + datasource: '3__table', + groupby: ['original_column'], + metrics: ['revenue'], + }, + }, + dataMask: { + [customizationId]: { + id: customizationId, + extraFormData: {}, + filterState: { value: ['revenue'] }, + ownState: {}, + }, + }, + chartCustomizationItems: [ + createChartCustomization({ + id: customizationId, + targets: [{ datasetId: 3, column: { name: 'revenue' } }], + }), + ], + }; + + const result = getFormDataWithExtraFilters(argsWithMetricConflict); + expectGroupBy(result, ['original_column']); +}); + +test('multi-column selection: all selected columns appear in result groupby', () => { + const result = getFormDataWithExtraFilters( + makeGroupByArgs(['status', 'product_line']), + ); + expectGroupBy(result, expect.arrayContaining(['status', 'product_line'])); + expectGroupByLength(result, 2); +}); + +test('dataset mismatch: display control for a different dataset does not affect the chart', () => { + const customizationId = 'CHART_CUSTOMIZATION-wrong-dataset'; + const argsWrongDataset: GetFormDataWithExtraFiltersArguments = { + ...mockArgs, + chart: { + ...mockChart, + form_data: { + ...mockChart.form_data, + viz_type: 'table', + datasource: '3__table', + groupby: ['original_column'], + }, + }, + dataMask: { + [customizationId]: { + id: customizationId, + extraFormData: {}, + filterState: { value: ['status'] }, + ownState: {}, + }, + }, + chartCustomizationItems: [ + createChartCustomization({ + id: customizationId, + targets: [{ datasetId: 999, column: { name: 'status' } }], + }), + ], + }; + + const result = getFormDataWithExtraFilters(argsWrongDataset); + expectGroupBy(result, ['original_column']); +}); + +test('Scope boundary: display control with chartsInScope:[] does not affect the chart', () => { + const customizationId = 'CHART_CUSTOMIZATION-groupby-out-of-scope'; + const argsOutOfScope: GetFormDataWithExtraFiltersArguments = { + ...mockArgs, + chart: { + ...mockChart, + form_data: { + ...mockChart.form_data, + viz_type: 'table', + datasource: '3__table', + groupby: ['original_column'], + }, + }, + dataMask: { + [customizationId]: { + id: customizationId, + extraFormData: {}, + filterState: { value: ['replacement_column'] }, + ownState: {}, + }, + }, + chartCustomizationItems: [ + createChartCustomization({ + id: customizationId, + name: 'Out Of Scope Group By', + chartsInScope: [], + }), + ], + }; + + const result = getFormDataWithExtraFilters(argsOutOfScope); + expectGroupBy(result, ['original_column']); });