diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.test.ts b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.test.ts index 2b007fda917..4efadc18f91 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.test.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.test.ts @@ -18,6 +18,7 @@ */ import { isMatrixifyVisible } from './matrixifyControls'; +import type { ControlStateMapping } from '../types'; /** * Helper to build a controls object matching the shape used by @@ -25,7 +26,7 @@ import { isMatrixifyVisible } from './matrixifyControls'; */ function makeControls( overrides: Record = {}, -): Record { +): ControlStateMapping { const defaults: Record = { matrixify_enable: false, matrixify_mode_rows: 'disabled', @@ -36,7 +37,7 @@ function makeControls( const merged = { ...defaults, ...overrides }; return Object.fromEntries( Object.entries(merged).map(([k, v]) => [k, { value: v }]), - ); + ) as ControlStateMapping; } // ── matrixify_enable guard ────────────────────────────────────────── diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx index 3e1b331d167..1ec8a340297 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx @@ -20,7 +20,7 @@ import { t } from '@apache-superset/core/translation'; import { validateNonEmpty } from '@superset-ui/core'; -import { SharedControlConfig } from '../types'; +import { ControlStateMapping, SharedControlConfig } from '../types'; import { dndAdhocMetricControl } from './dndControls'; import { defineSavedMetrics } from '../utils'; @@ -29,9 +29,12 @@ import { defineSavedMetrics } from '../utils'; * Controls for transforming charts into matrix/grid layouts */ -// Utility function to check if matrixify controls should be visible +// Utility function to check if matrixify controls should be visible. +// Controls both visibility callbacks and validator injection via mapStateToProps. +// The matrixify_enable guard prevents hidden validators from firing on +// pre-revamp charts with stale matrixify_mode defaults (fix for #38519). const isMatrixifyVisible = ( - controls: any, + controls: ControlStateMapping | undefined, axis: 'rows' | 'columns', mode?: 'metrics' | 'dimensions', selectionMode?: 'members' | 'topn' | 'all', diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/matrixifyControls.test.tsx b/superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/matrixifyControls.test.tsx new file mode 100644 index 00000000000..f78e6162f47 --- /dev/null +++ b/superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/matrixifyControls.test.tsx @@ -0,0 +1,238 @@ +/** + * 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. + */ + +/** + * Tests for the matrixify_enable guard in isMatrixifyVisible() and + * validator injection via mapStateToProps on real matrixify control definitions. + * + * These are TDD tests for the fix to apache/superset#38519 regression: + * isMatrixifyVisible() must check matrixify_enable before evaluating mode, + * otherwise pre-revamp charts with stale matrixify_mode defaults trigger + * hidden validators that block save. + */ + +import { + matrixifyControls, + isMatrixifyVisible, +} from '../../src/shared-controls/matrixifyControls'; +import type { ControlPanelState, ControlStateMapping } from '../../src/types'; + +// Helper: build a minimal controls object for ControlPanelState +const buildControls = ( + overrides: Record = {}, +): ControlStateMapping => { + const controls: Record = {}; + Object.entries(overrides).forEach(([key, value]) => { + controls[key] = { value }; + }); + return controls as ControlStateMapping; +}; + +// Helper: build a minimal ControlPanelState for mapStateToProps. +// Only provides fields that isMatrixifyVisible and mapStateToProps actually read. +const buildState = ( + controlValues: Record = {}, + formData: Record = {}, +) => + ({ + controls: buildControls(controlValues), + datasource: { columns: [], type: 'table' }, + form_data: formData, + common: {}, + metadata: {}, + slice: { slice_id: 0 }, + }) as unknown as ControlPanelState; + +// ============================================================ +// Validator injection tests via real mapStateToProps (rows) +// ============================================================ + +// --- matrixify_dimension_rows --- + +test('matrixify_dimension_rows: validators empty when matrixify_enable is falsy', () => { + const control = matrixifyControls.matrixify_dimension_rows; + const state = buildState( + { + matrixify_enable: undefined, + matrixify_mode_rows: 'dimensions', + matrixify_dimension_selection_mode_rows: 'members', + }, + { matrixify_mode_rows: 'dimensions' }, + ); + + const result = control.mapStateToProps!(state, {} as any); + expect(result.validators).toEqual([]); +}); + +test('matrixify_dimension_rows: validators present when matrixify_enable is true', () => { + const control = matrixifyControls.matrixify_dimension_rows; + const state = buildState( + { + matrixify_enable: true, + matrixify_mode_rows: 'dimensions', + matrixify_dimension_selection_mode_rows: 'members', + }, + { matrixify_mode_rows: 'dimensions' }, + ); + + const result = control.mapStateToProps!(state, {} as any); + expect(result.validators.length).toBeGreaterThan(0); +}); + +// --- matrixify_topn_value_rows --- + +test('matrixify_topn_value_rows: validators empty when matrixify_enable is falsy', () => { + const control = matrixifyControls.matrixify_topn_value_rows; + const state = buildState( + { + matrixify_enable: undefined, + matrixify_mode_rows: 'dimensions', + matrixify_dimension_selection_mode_rows: 'topn', + }, + { matrixify_mode_rows: 'dimensions' }, + ); + + const result = control.mapStateToProps!(state, {} as any); + expect(result.validators).toEqual([]); +}); + +test('matrixify_topn_value_rows: validators present when matrixify_enable is true', () => { + const control = matrixifyControls.matrixify_topn_value_rows; + const state = buildState( + { + matrixify_enable: true, + matrixify_mode_rows: 'dimensions', + matrixify_dimension_selection_mode_rows: 'topn', + }, + { matrixify_mode_rows: 'dimensions' }, + ); + + const result = control.mapStateToProps!(state, {} as any); + expect(result.validators.length).toBeGreaterThan(0); +}); + +// --- matrixify_topn_metric_rows --- + +test('matrixify_topn_metric_rows: validators empty when matrixify_enable is falsy', () => { + const control = matrixifyControls.matrixify_topn_metric_rows; + const state = buildState( + { + matrixify_enable: undefined, + matrixify_mode_rows: 'dimensions', + matrixify_dimension_selection_mode_rows: 'topn', + }, + { matrixify_mode_rows: 'dimensions' }, + ); + + const result = control.mapStateToProps!(state, {} as any); + expect(result.validators).toEqual([]); +}); + +test('matrixify_topn_metric_rows: validators present when matrixify_enable is true', () => { + const control = matrixifyControls.matrixify_topn_metric_rows; + const state = buildState( + { + matrixify_enable: true, + matrixify_mode_rows: 'dimensions', + matrixify_dimension_selection_mode_rows: 'topn', + }, + { matrixify_mode_rows: 'dimensions' }, + ); + + const result = control.mapStateToProps!(state, {} as any); + expect(result.validators.length).toBeGreaterThan(0); +}); + +// ============================================================ +// Validator injection tests via real mapStateToProps (columns) +// ============================================================ + +test('matrixify_dimension_columns: validators empty when matrixify_enable is falsy', () => { + const control = matrixifyControls.matrixify_dimension_columns; + const state = buildState( + { + matrixify_enable: undefined, + matrixify_mode_columns: 'dimensions', + matrixify_dimension_selection_mode_columns: 'members', + }, + { matrixify_mode_columns: 'dimensions' }, + ); + + const result = control.mapStateToProps!(state, {} as any); + expect(result.validators).toEqual([]); +}); + +test('matrixify_dimension_columns: validators present when matrixify_enable is true', () => { + const control = matrixifyControls.matrixify_dimension_columns; + const state = buildState( + { + matrixify_enable: true, + matrixify_mode_columns: 'dimensions', + matrixify_dimension_selection_mode_columns: 'members', + }, + { matrixify_mode_columns: 'dimensions' }, + ); + + const result = control.mapStateToProps!(state, {} as any); + expect(result.validators.length).toBeGreaterThan(0); +}); + +// ============================================================ +// Direct isMatrixifyVisible guard tests +// ============================================================ + +test.each([ + ['undefined', undefined], + ['null', null], + ['false', false], + ['0', 0], +])( + 'isMatrixifyVisible returns false when matrixify_enable is %s', + (_, value) => { + const controls = buildControls({ + matrixify_enable: value, + matrixify_mode_rows: 'dimensions', + }); + expect(isMatrixifyVisible(controls, 'rows')).toBe(false); + }, +); + +test('isMatrixifyVisible returns true when matrixify_enable is true and mode matches', () => { + const controls = buildControls({ + matrixify_enable: true, + matrixify_mode_rows: 'dimensions', + }); + expect(isMatrixifyVisible(controls, 'rows', 'dimensions')).toBe(true); +}); + +test('isMatrixifyVisible returns false when matrixify_enable is true but mode is disabled', () => { + const controls = buildControls({ + matrixify_enable: true, + matrixify_mode_rows: 'disabled', + }); + expect(isMatrixifyVisible(controls, 'rows')).toBe(false); +}); + +test('isMatrixifyVisible returns true when matrixify_enable is true and any non-disabled mode (no mode filter)', () => { + const controls = buildControls({ + matrixify_enable: true, + matrixify_mode_columns: 'metrics', + }); + expect(isMatrixifyVisible(controls, 'columns')).toBe(true); +}); diff --git a/superset-frontend/src/explore/actions/hydrateExplore.ts b/superset-frontend/src/explore/actions/hydrateExplore.ts index d488dfb4d29..574707975c2 100644 --- a/superset-frontend/src/explore/actions/hydrateExplore.ts +++ b/superset-frontend/src/explore/actions/hydrateExplore.ts @@ -24,7 +24,7 @@ import { ExplorePageState, } from 'src/explore/types'; import { getChartKey } from 'src/explore/exploreUtils'; -import { getControlsState } from 'src/explore/store'; +import { getControlsState, handleDeprecatedControls } from 'src/explore/store'; import { Dispatch } from 'redux'; import { Currency, @@ -116,6 +116,12 @@ export const hydrateExplore = ]), ); + // Normalize deprecated controls (e.g., migrate old per-axis matrixify + // flags to matrixify_enable) before form_data is stored in Redux state. + // getControlsState also calls this on its own copy, but state.form_data + // must reflect the same migration so the two stay consistent. + handleDeprecatedControls(initialFormData); + const initialExploreState = { form_data: initialFormData, slice: initialSlice, diff --git a/superset-frontend/src/explore/store.test.tsx b/superset-frontend/src/explore/store.test.tsx index 602390a4d4f..d74b5bf0c18 100644 --- a/superset-frontend/src/explore/store.test.tsx +++ b/superset-frontend/src/explore/store.test.tsx @@ -17,55 +17,359 @@ * under the License. */ import { getChartControlPanelRegistry } from '@superset-ui/core'; -import { applyDefaultFormData } from 'src/explore/store'; +import { + applyDefaultFormData, + getControlsState, + handleDeprecatedControls, +} from 'src/explore/store'; -// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks -describe('store', () => { - beforeAll(() => { - getChartControlPanelRegistry().registerValue('test-chart', { - controlPanelSections: [ - { - label: 'Test section', - expanded: true, - controlSetRows: [['row_limit']], - }, - ], - }); - }); +// eslint-disable-next-line @typescript-eslint/no-explicit-any +(window as any).featureFlags = {}; - afterAll(() => { - getChartControlPanelRegistry().remove('test-chart'); - }); - - // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks - describe('applyDefaultFormData', () => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (window as any).featureFlags = {}; - - test('applies default to formData if the key is missing', () => { - const inputFormData = { - datasource: '11_table', - viz_type: 'test-chart', - }; - let outputFormData = applyDefaultFormData(inputFormData); - expect(outputFormData.row_limit).toEqual(10000); - - const inputWithRowLimit = { - ...inputFormData, - row_limit: 888, - }; - outputFormData = applyDefaultFormData(inputWithRowLimit); - expect(outputFormData.row_limit).toEqual(888); - }); - - test('keeps null if key is defined with null', () => { - const inputFormData = { - datasource: '11_table', - viz_type: 'test-chart', - row_limit: null, - }; - const outputFormData = applyDefaultFormData(inputFormData); - expect(outputFormData.row_limit).toBe(null); - }); +beforeAll(() => { + getChartControlPanelRegistry().registerValue('test-chart', { + controlPanelSections: [ + { + label: 'Test section', + expanded: true, + controlSetRows: [['row_limit']], + }, + ], }); }); + +afterAll(() => { + getChartControlPanelRegistry().remove('test-chart'); +}); + +// Helper: build ExploreState for getControlsState +const buildExploreState = (controlOverrides: Record = {}) => ({ + datasource: { type: 'table' }, + controls: Object.fromEntries( + Object.entries(controlOverrides).map(([k, v]) => [k, { value: v }]), + ), +}); + +// ============================================================ +// Existing applyDefaultFormData tests +// ============================================================ + +test('applyDefaultFormData applies default to formData if the key is missing', () => { + const inputFormData = { + datasource: '11_table', + viz_type: 'test-chart', + }; + let outputFormData = applyDefaultFormData(inputFormData); + expect(outputFormData.row_limit).toEqual(10000); + + const inputWithRowLimit = { + ...inputFormData, + row_limit: 888, + }; + outputFormData = applyDefaultFormData(inputWithRowLimit); + expect(outputFormData.row_limit).toEqual(888); +}); + +test('applyDefaultFormData keeps null if key is defined with null', () => { + const inputFormData = { + datasource: '11_table', + viz_type: 'test-chart', + row_limit: null, + }; + const outputFormData = applyDefaultFormData(inputFormData); + expect(outputFormData.row_limit).toBe(null); +}); + +// ============================================================ +// Migration tests: handleDeprecatedControls normalizes stale matrixify modes +// (fix for apache/superset#38519 regression — guards validators AND +// downstream UI consumers that infer matrixify state from mode values) +// ============================================================ + +test('getControlsState resets stale matrixify_mode_rows to disabled when matrixify_enable key absent', () => { + const state = buildExploreState(); + const formData = { + datasource: '1__table', + viz_type: 'test-chart', + matrixify_mode_rows: 'dimensions', // stale pre-revamp default + }; + + const result = getControlsState(state as any, formData as any); + const modeControl = result.matrixify_mode_rows as any; + expect(modeControl?.value).toBe('disabled'); +}); + +test('getControlsState resets stale matrixify_mode_columns to disabled when matrixify_enable key absent', () => { + const state = buildExploreState(); + const formData = { + datasource: '1__table', + viz_type: 'test-chart', + matrixify_mode_columns: 'metrics', // stale pre-revamp default + }; + + const result = getControlsState(state as any, formData as any); + const modeControl = result.matrixify_mode_columns as any; + expect(modeControl?.value).toBe('disabled'); +}); + +test('getControlsState preserves matrixify mode values when matrixify_enable is true', () => { + const state = buildExploreState(); + const formData = { + datasource: '1__table', + viz_type: 'test-chart', + matrixify_enable: true, + matrixify_mode_rows: 'dimensions', + }; + + const result = getControlsState(state as any, formData as any); + const modeControl = result.matrixify_mode_rows as any; + expect(modeControl?.value).toBe('dimensions'); +}); + +test('getControlsState preserves matrixify mode values when matrixify_enable is explicitly false', () => { + const state = buildExploreState(); + const formData = { + datasource: '1__table', + viz_type: 'test-chart', + matrixify_enable: false, + matrixify_mode_rows: 'dimensions', + }; + + const result = getControlsState(state as any, formData as any); + const modeControl = result.matrixify_mode_rows as any; + // matrixify_enable key IS present (just false) — migration does NOT fire + expect(modeControl?.value).toBe('dimensions'); +}); + +test('getControlsState is idempotent when matrixify modes already disabled', () => { + const state = buildExploreState(); + const formData = { + datasource: '1__table', + viz_type: 'test-chart', + matrixify_mode_rows: 'disabled', + matrixify_mode_columns: 'disabled', + }; + + const result = getControlsState(state as any, formData as any); + expect((result.matrixify_mode_rows as any)?.value).toBe('disabled'); + expect((result.matrixify_mode_columns as any)?.value).toBe('disabled'); +}); + +test('getControlsState handles form_data with no matrixify keys', () => { + const state = buildExploreState(); + const formData = { + datasource: '1__table', + viz_type: 'test-chart', + }; + + const result = getControlsState(state as any, formData as any); + // Controls should get their defaults — matrixify_mode defaults to 'disabled' + expect((result.matrixify_mode_rows as any)?.value).toBe('disabled'); + expect((result.matrixify_mode_columns as any)?.value).toBe('disabled'); +}); + +test('getControlsState round-trip: pre-revamp form_data produces no matrixify validation errors', () => { + // Simulate a chart saved before #38519 with stale matrixify defaults + // Empty controls: on real first-load hydration, no pre-existing controls exist + const state = buildExploreState(); + const preRevampFormData = { + datasource: '1__table', + viz_type: 'test-chart', + // Stale old defaults — no matrixify_enable key (legacy chart) + matrixify_mode_rows: 'dimensions', + matrixify_mode_columns: 'metrics', + }; + + const result = getControlsState(state as any, preRevampFormData as any); + + // Every matrixify control should have zero validation errors + const matrixifyControlEntries = Object.entries(result).filter(([name]) => + name.startsWith('matrixify_'), + ); + const controlsWithErrors = matrixifyControlEntries.filter( + ([, control]) => (control as any)?.validationErrors?.length > 0, + ); + + expect(controlsWithErrors).toEqual([]); +}); + +// ============================================================ +// Dashboard hydration: applyDefaultFormData with stale form_data +// ============================================================ + +test('applyDefaultFormData normalizes stale matrixify modes for legacy charts', () => { + // Dashboard hydration now runs handleDeprecatedControls too, so stale + // matrixify modes from pre-revamp charts are normalized to 'disabled'. + // This protects downstream consumers (ChartContextMenu, DrillBySubmenu, + // ChartRenderer) that infer "matrixify is active" from mode values alone. + const preRevampFormData = { + datasource: '1__table', + viz_type: 'test-chart', + matrixify_mode_rows: 'dimensions', + matrixify_mode_columns: 'metrics', + // No matrixify_enable key — legacy chart that never used matrixify + }; + + const outputFormData = applyDefaultFormData(preRevampFormData as any); + + // Stale values are now normalized to 'disabled' + expect(outputFormData.matrixify_mode_rows).toBe('disabled'); + expect(outputFormData.matrixify_mode_columns).toBe('disabled'); + expect(outputFormData.matrixify_enable).toBe(false); +}); + +// ============================================================ +// P1: Pre-revamp charts that actually used matrixify via old per-axis flags +// (matrixify_enable_vertical_layout / matrixify_enable_horizontal_layout) +// ============================================================ + +test('getControlsState preserves modes and sets matrixify_enable when old vertical flag is true', () => { + const state = buildExploreState(); + const formData = { + datasource: '1__table', + viz_type: 'test-chart', + matrixify_enable_vertical_layout: true, + matrixify_mode_rows: 'dimensions', + matrixify_mode_columns: 'metrics', + }; + + const result = getControlsState(state as any, formData as any); + // Vertical layout was enabled — rows mode preserved, matrixify_enable migrated + expect((result.matrixify_mode_rows as any)?.value).toBe('dimensions'); + expect((result.matrixify_enable as any)?.value).toBe(true); + // Horizontal layout was NOT enabled — columns mode reset + expect((result.matrixify_mode_columns as any)?.value).toBe('disabled'); +}); + +test('getControlsState preserves modes and sets matrixify_enable when old horizontal flag is true', () => { + const state = buildExploreState(); + const formData = { + datasource: '1__table', + viz_type: 'test-chart', + matrixify_enable_horizontal_layout: true, + matrixify_mode_rows: 'dimensions', + matrixify_mode_columns: 'metrics', + }; + + const result = getControlsState(state as any, formData as any); + // Horizontal layout was enabled — columns mode preserved, matrixify_enable migrated + expect((result.matrixify_mode_columns as any)?.value).toBe('metrics'); + expect((result.matrixify_enable as any)?.value).toBe(true); + // Vertical layout was NOT enabled — rows mode reset + expect((result.matrixify_mode_rows as any)?.value).toBe('disabled'); +}); + +test('getControlsState preserves both modes when both old per-axis flags are true', () => { + const state = buildExploreState(); + const formData = { + datasource: '1__table', + viz_type: 'test-chart', + matrixify_enable_vertical_layout: true, + matrixify_enable_horizontal_layout: true, + matrixify_mode_rows: 'dimensions', + matrixify_mode_columns: 'metrics', + }; + + const result = getControlsState(state as any, formData as any); + expect((result.matrixify_mode_rows as any)?.value).toBe('dimensions'); + expect((result.matrixify_mode_columns as any)?.value).toBe('metrics'); + expect((result.matrixify_enable as any)?.value).toBe(true); +}); + +test('getControlsState resets modes when old per-axis flags are explicitly false', () => { + const state = buildExploreState(); + const formData = { + datasource: '1__table', + viz_type: 'test-chart', + matrixify_enable_vertical_layout: false, + matrixify_enable_horizontal_layout: false, + matrixify_mode_rows: 'dimensions', + matrixify_mode_columns: 'metrics', + }; + + const result = getControlsState(state as any, formData as any); + // Old flags present but false — chart never used matrixify, reset stale modes + expect((result.matrixify_mode_rows as any)?.value).toBe('disabled'); + expect((result.matrixify_mode_columns as any)?.value).toBe('disabled'); +}); + +// ============================================================ +// P2: Dashboard hydration (applyDefaultFormData) with old per-axis flags +// ============================================================ + +test('applyDefaultFormData preserves modes when old vertical flag is true', () => { + const formData = { + datasource: '1__table', + viz_type: 'test-chart', + matrixify_enable_vertical_layout: true, + matrixify_mode_rows: 'dimensions', + matrixify_mode_columns: 'metrics', + }; + + const outputFormData = applyDefaultFormData(formData as any); + expect(outputFormData.matrixify_mode_rows).toBe('dimensions'); + expect(outputFormData.matrixify_enable).toBe(true); + // Horizontal not enabled — columns reset + expect(outputFormData.matrixify_mode_columns).toBe('disabled'); +}); + +test('applyDefaultFormData preserves modes when both old flags are true', () => { + const formData = { + datasource: '1__table', + viz_type: 'test-chart', + matrixify_enable_vertical_layout: true, + matrixify_enable_horizontal_layout: true, + matrixify_mode_rows: 'dimensions', + matrixify_mode_columns: 'metrics', + }; + + const outputFormData = applyDefaultFormData(formData as any); + expect(outputFormData.matrixify_mode_rows).toBe('dimensions'); + expect(outputFormData.matrixify_mode_columns).toBe('metrics'); + expect(outputFormData.matrixify_enable).toBe(true); +}); + +// ============================================================ +// Direct handleDeprecatedControls tests: verify form_data mutation +// so callers (hydrateExplore) can propagate migrated fields into state +// ============================================================ + +test('handleDeprecatedControls sets matrixify_enable on form_data when old vertical flag is true', () => { + const formData: any = { + matrixify_enable_vertical_layout: true, + matrixify_mode_rows: 'dimensions', + matrixify_mode_columns: 'metrics', + }; + handleDeprecatedControls(formData); + + expect(formData.matrixify_enable).toBe(true); + expect(formData.matrixify_mode_rows).toBe('dimensions'); + // Horizontal not enabled — columns reset + expect(formData.matrixify_mode_columns).toBe('disabled'); +}); + +test('handleDeprecatedControls resets modes when no matrixify_enable and no old flags', () => { + const formData: any = { + matrixify_mode_rows: 'dimensions', + matrixify_mode_columns: 'metrics', + }; + handleDeprecatedControls(formData); + + expect(formData.matrixify_enable).toBeUndefined(); + expect(formData.matrixify_mode_rows).toBe('disabled'); + expect(formData.matrixify_mode_columns).toBe('disabled'); +}); + +test('handleDeprecatedControls is idempotent — no-op when matrixify_enable already present', () => { + const formData: any = { + matrixify_enable: true, + matrixify_mode_rows: 'dimensions', + matrixify_mode_columns: 'metrics', + }; + handleDeprecatedControls(formData); + + // No mutation — matrixify_enable key is present + expect(formData.matrixify_enable).toBe(true); + expect(formData.matrixify_mode_rows).toBe('dimensions'); + expect(formData.matrixify_mode_columns).toBe('metrics'); +}); diff --git a/superset-frontend/src/explore/store.ts b/superset-frontend/src/explore/store.ts index b1189efff44..79b5826df03 100644 --- a/superset-frontend/src/explore/store.ts +++ b/superset-frontend/src/explore/store.ts @@ -41,9 +41,16 @@ type FormData = QueryFormData & { y_axis_zero?: boolean; y_axis_bounds?: [number | null, number | null]; datasource?: string; + matrixify_enable?: boolean; + matrixify_mode_rows?: string; + matrixify_mode_columns?: string; + // Pre-revamp per-axis enable flags (removed in #38519, may still exist in + // persisted form_data for charts that actually used matrixify) + matrixify_enable_vertical_layout?: boolean; + matrixify_enable_horizontal_layout?: boolean; }; -function handleDeprecatedControls(formData: FormData): void { +export function handleDeprecatedControls(formData: FormData): void { // Reaffectation / handling of deprecated controls /* eslint-disable no-param-reassign */ @@ -51,6 +58,37 @@ function handleDeprecatedControls(formData: FormData): void { if (formData.y_axis_zero) { formData.y_axis_bounds = [0, null]; } + + // #38519: migrate pre-revamp matrixify controls to the new single-toggle + // system. Before the revamp, per-axis enable flags + // (matrixify_enable_vertical_layout / matrixify_enable_horizontal_layout) + // gated visibility, and matrixify_mode_rows/columns defaulted to + // non-disabled values ('dimensions'/'metrics'). The revamp replaced those + // with a single matrixify_enable toggle and mode default 'disabled'. + // + // Charts that actually used matrixify pre-revamp have the old per-axis + // flags set to true — we must preserve their modes and set + // matrixify_enable: true. Charts that never used matrixify (or predate it) + // need stale mode defaults reset to 'disabled' because 4 downstream UI + // consumers (ExploreChartPanel, ChartContextMenu, DrillBySubmenu, + // ChartRenderer) infer "matrixify is active" from mode values alone. + if (!('matrixify_enable' in formData)) { + const hadVerticalLayout = + formData.matrixify_enable_vertical_layout === true; + const hadHorizontalLayout = + formData.matrixify_enable_horizontal_layout === true; + + if (hadVerticalLayout || hadHorizontalLayout) { + // Pre-revamp chart that genuinely used matrixify — migrate to new flag + formData.matrixify_enable = true; + if (!hadVerticalLayout) formData.matrixify_mode_rows = 'disabled'; + if (!hadHorizontalLayout) formData.matrixify_mode_columns = 'disabled'; + } else { + // Never used matrixify — reset stale defaults + formData.matrixify_mode_rows = 'disabled'; + formData.matrixify_mode_columns = 'disabled'; + } + } } export function getControlsState( @@ -89,25 +127,31 @@ export function getControlsState( export function applyDefaultFormData( inputFormData: FormData, ): Record { - const datasourceType = inputFormData.datasource?.split('__')[1] ?? ''; - const vizType = inputFormData.viz_type; + // Normalize deprecated controls before building control state — ensures + // stale matrixify modes are cleaned on the dashboard hydration path too, + // not just the explore path (getControlsState). + const cleanedFormData = { ...inputFormData }; + handleDeprecatedControls(cleanedFormData); + + const datasourceType = cleanedFormData.datasource?.split('__')[1] ?? ''; + const vizType = cleanedFormData.viz_type; const controlsState = getAllControlsState( vizType, datasourceType as DatasourceType, null, - inputFormData, + cleanedFormData, ); // eslint-disable-next-line @typescript-eslint/no-explicit-any const controlFormData = getFormDataFromControls(controlsState as any); const formData: Record = {}; Object.keys(controlsState) - .concat(Object.keys(inputFormData)) + .concat(Object.keys(cleanedFormData)) .forEach(controlName => { - if (inputFormData[controlName as keyof FormData] === undefined) { + if (cleanedFormData[controlName as keyof FormData] === undefined) { formData[controlName] = controlFormData[controlName]; } else { - formData[controlName] = inputFormData[controlName as keyof FormData]; + formData[controlName] = cleanedFormData[controlName as keyof FormData]; } });