michael-s-molina commented on code in PR #34526: URL: https://github.com/apache/superset/pull/34526#discussion_r2283081984
########## superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridGenerator.test.ts: ########## @@ -0,0 +1,327 @@ +/** + * 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 { generateMatrixifyGrid } from './MatrixifyGridGenerator'; +import { AdhocMetric } from '../../../query/types/Metric'; + +// Use 'any' to bypass strict typing in tests +type TestFormData = any; + +describe('MatrixifyGridGenerator', () => { Review Comment: Replace `describe/it` with `test`. ########## superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.test.tsx: ########## @@ -0,0 +1,457 @@ +/** + * 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 { render, screen, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import StatefulChart from './StatefulChart'; + +// Mock SuperChart to avoid complex chart registry setup +jest.mock('./SuperChart', () => ({ + __esModule: true, + default: jest.fn(({ formData, queriesData }) => ( + <div data-testid="superchart"> + {formData?.viz_type && <span>Chart Type: {formData.viz_type}</span>} + {queriesData && <span>Has Data</span>} + </div> + )), +})); + +// Mock the ChartClient +jest.mock('../clients/ChartClient', () => ({ + __esModule: true, + default: jest.fn().mockImplementation(() => ({ + client: { + post: jest.fn(), + }, + loadFormData: jest.fn(), + })), +})); + +// Mock the registries +jest.mock('../registries/ChartMetadataRegistrySingleton', () => ({ + __esModule: true, + default: jest.fn(() => ({ + get: jest.fn(() => ({ useLegacyApi: false })), + })), +})); + +jest.mock('../registries/ChartBuildQueryRegistrySingleton', () => ({ + __esModule: true, + default: jest.fn(() => ({ + get: jest.fn(() => null), + })), +})); + +// Mock buildQueryContext +jest.mock('../..', () => ({ + ...jest.requireActual('../..'), + buildQueryContext: jest.fn(formData => ({ + queries: [{ formData }], + })), +})); + +// Mock Loading component +jest.mock('../../components/Loading', () => ({ + Loading: () => <div>Loading...</div>, +})); + +describe('StatefulChart', () => { Review Comment: Replace `describe/it` with `test`. ########## superset-frontend/packages/superset-ui-core/test/chart/types/matrixify.test.ts: ########## @@ -0,0 +1,601 @@ +/** + * 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 { + isMatrixifyEnabled, + getMatrixifyConfig, + getMatrixifyValidationErrors, + MatrixifyFormData, + MatrixifyMode, + MatrixifySelectionMode, +} from '../../../src/chart/types/matrixify'; +import { AdhocMetric } from '../../../src/query/types/Metric'; + +describe('Matrixify Types and Utilities', () => { Review Comment: Replace `describe/it` with `test`. ########## superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.tsx: ########## @@ -0,0 +1,463 @@ +/** + * 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 { Component } from 'react'; +import { ParentSize } from '@visx/responsive'; +import { + QueryFormData, + QueryData, + SupersetClientInterface, + buildQueryContext, + RequestConfig, +} from '../..'; +import { Loading } from '../../components/Loading'; +import ChartClient from '../clients/ChartClient'; +import getChartBuildQueryRegistry from '../registries/ChartBuildQueryRegistrySingleton'; +import getChartMetadataRegistry from '../registries/ChartMetadataRegistrySingleton'; +import getChartControlPanelRegistry from '../registries/ChartControlPanelRegistrySingleton'; +import SuperChart from './SuperChart'; + +// Using more specific states that align with chart loading process +type LoadingState = 'uninitialized' | 'loading' | 'loaded' | 'error'; + +/** + * Helper function to determine if data should be refetched based on formData changes + * @param prevFormData Previous formData + * @param nextFormData New formData + * @param vizType Chart visualization type + * @returns true if data should be refetched, false if only re-render is needed + */ +function shouldRefetchData( + prevFormData: QueryFormData | undefined, + nextFormData: QueryFormData | undefined, + vizType: string | undefined, +): boolean { + // If no previous formData or viz types don't match, always refetch + if (!prevFormData || !nextFormData || !vizType) { + return true; + } + + // If viz_type changed, always refetch + if (prevFormData.viz_type !== nextFormData.viz_type) { + return true; + } + + try { + // Try to get control panel configuration + const controlPanel = getChartControlPanelRegistry().get(vizType); + if (!controlPanel || !controlPanel.controlPanelSections) { + // If no control panel config available, be conservative and refetch + return true; + } + + // Build a map of control names to their renderTrigger status + const renderTriggerControls = new Set<string>(); + controlPanel.controlPanelSections.forEach((section: any) => { + if (section.controlSetRows) { + section.controlSetRows.forEach((row: any) => { + row.forEach((control: any) => { + if (control && typeof control === 'object') { + const controlName = control.name || control.config?.name; + if (controlName && control.config?.renderTrigger === true) { + renderTriggerControls.add(controlName); + } + } + }); + }); + } + }); + + // Check which fields changed + const changedFields = Object.keys(nextFormData).filter( + key => + JSON.stringify(prevFormData[key]) !== JSON.stringify(nextFormData[key]), + ); + + // If no fields changed, no need to refetch + if (changedFields.length === 0) { + return false; + } + + // Check if all changed fields are renderTrigger controls + const allChangesAreRenderTrigger = changedFields.every(field => + renderTriggerControls.has(field), + ); + + // Only skip refetch if ALL changes are renderTrigger-only + return !allChangesAreRenderTrigger; + } catch (error) { + // If there's any error accessing the registry, be conservative and refetch + return true; + } +} + +export interface StatefulChartProps { + // Option 1: Provide chartId to load saved chart + chartId?: number; + + // Option 2: Provide formData directly + formData?: QueryFormData; + + // Option 3: Override specific formData fields + formDataOverrides?: Partial<QueryFormData>; + + // Chart type (required if using formData without viz_type) + chartType?: string; + + // Chart dimensions + width?: number | string; + height?: number | string; + + // Event handlers + onLoad?: (data: QueryData[]) => void; + onError?: (error: Error) => void; + onRenderSuccess?: () => void; + onRenderFailure?: (error: Error) => void; + + // Data fetching options + force?: boolean; + timeout?: number; + + // UI options + showLoading?: boolean; + loadingComponent?: React.ComponentType; + errorComponent?: React.ComponentType<{ error: Error }>; + noDataComponent?: React.ComponentType; + + // Advanced options + client?: SupersetClientInterface; + disableErrorBoundary?: boolean; + enableNoResults?: boolean; + + // Additional SuperChart props + id?: string; + className?: string; + + // Hooks for chart interactions (drill, cross-filter, etc.) + hooks?: any; +} + +interface StatefulChartState { + status: LoadingState; + data?: QueryData[]; + error?: Error; + formData?: QueryFormData; +} + +export default class StatefulChart extends Component< Review Comment: Given that this is a new component, could you use a functional component instead of a class component? ########## superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.test.tsx: ########## @@ -0,0 +1,354 @@ +/** + * 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 { render, screen } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { ThemeProvider, supersetTheme } from '../../..'; +import MatrixifyGridRenderer from './MatrixifyGridRenderer'; +import { MatrixifyGrid } from '../../types/matrixify'; +import { AdhocMetric } from '../../../query/types/Metric'; + +import { generateMatrixifyGrid } from './MatrixifyGridGenerator'; + +// Use 'any' to bypass strict typing in tests +type TestFormData = any; + +// Mock MatrixifyGridCell component +jest.mock('./MatrixifyGridCell', () => ({ + __esModule: true, + default: ({ cell }: any) => ( + <div data-testid={`cell-${cell.id}`} className="matrixify-cell"> + {cell.title || `${cell.rowLabel}-${cell.colLabel}`} + </div> + ), +})); + +// Mock generateMatrixifyGrid function +jest.mock('./MatrixifyGridGenerator', () => ({ + generateMatrixifyGrid: jest.fn(), +})); + +const mockedGenerateMatrixifyGrid = + generateMatrixifyGrid as jest.MockedFunction<typeof generateMatrixifyGrid>; + +describe('MatrixifyGridRenderer', () => { Review Comment: Replace `describe/it` with `test`. ########## superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.test.tsx: ########## @@ -0,0 +1,762 @@ +/** + * 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 { render, screen, waitFor } from 'spec/helpers/testing-library'; +import userEvent from '@testing-library/user-event'; +import { SupersetClient } from '@superset-ui/core'; +import fetchMock from 'fetch-mock'; +import { getChartDataRequest } from 'src/components/Chart/chartAction'; +import MatrixifyDimensionControl, { + MatrixifyDimensionControlValue, +} from './MatrixifyDimensionControl'; + +// Mock the SupersetClient +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + SupersetClient: { + get: jest.fn(), + post: jest.fn(), + }, + t: (str: string, ...args: any[]) => { + // Simple format replacement for %s + if (args.length > 0 && str.includes('%s')) { + return str.replace('%s', args[0]); + } + return str; + }, + styled: jest.requireActual('@superset-ui/core').styled, + getColumnLabel: (col: string) => col, +})); + +// Mock ControlHeader +jest.mock('src/explore/components/ControlHeader', () => ({ + __esModule: true, + default: ({ label, description }: any) => ( + <div data-testid="control-header"> + {label && <span data-testid="control-label">{label}</span>} + {description && ( + <span data-testid="control-description">{description}</span> + )} + </div> + ), +})); + +// Mock getChartDataRequest +jest.mock('src/components/Chart/chartAction', () => ({ + getChartDataRequest: jest.fn(), +})); + +const mockDatasource = { + id: 1, + type: 'table', + columns: [ + { column_name: 'country', verbose_name: 'Country' }, + { column_name: 'region', verbose_name: 'Region' }, + { column_name: 'product', verbose_name: 'Product' }, + ], + filter_select: true, +}; + +const defaultProps = { + datasource: mockDatasource, + onChange: jest.fn(), +}; + +describe('MatrixifyDimensionControl', () => { Review Comment: Replace `describe/it` with `test`. ########## superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridCell.test.tsx: ########## @@ -0,0 +1,214 @@ +/** + * 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 { render, screen } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { ThemeProvider, supersetTheme } from '../../..'; +import MatrixifyGridCell from './MatrixifyGridCell'; +import { MatrixifyGridCell as MatrixifyGridCellType } from '../../types/matrixify'; + +// Mock StatefulChart component +jest.mock('../StatefulChart', () => { + /* eslint-disable no-restricted-syntax, global-require, @typescript-eslint/no-var-requires */ + const React = require('react'); + /* eslint-enable no-restricted-syntax, global-require, @typescript-eslint/no-var-requires */ + + return { + __esModule: true, + default: ({ formData, height, width }: any) => + React.createElement( + 'div', + { + 'data-testid': 'superchart', + 'data-viz-type': formData.viz_type, + style: { height, width }, + }, + 'SuperChart Mock', + ), + }; +}); + +describe('MatrixifyGridCell', () => { Review Comment: Replace `describe/it` with `test`. ########## superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx: ########## @@ -473,3 +472,236 @@ export default { sort_by_metric, order_by_cols, }; + +// dynamically add matrixify controls Review Comment: This file is getting out of control. Should we move this code to a specific file? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
