From a6bf8d20420b156a232848be5f4baa1995b71257 Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Fri, 8 Aug 2025 11:11:08 -0300 Subject: [PATCH 1/3] fix: Use labels in Drill to Detail --- .../ChartContextMenu/ChartContextMenu.tsx | 65 +++---------- .../Chart/DrillDetail/DrillDetailPane.tsx | 11 ++- .../components/SliceHeaderControls/index.tsx | 15 +++ .../src/hooks/apiResources/datasets.ts | 93 ++++++++++++++++++- 4 files changed, 129 insertions(+), 55 deletions(-) diff --git a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx index b5362e578e0b..ab79f1ad43c5 100644 --- a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx +++ b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx @@ -22,7 +22,6 @@ import { ReactNode, RefObject, useCallback, - useEffect, useImperativeHandle, useMemo, useState, @@ -39,7 +38,6 @@ import { getChartMetadataRegistry, getExtensionsRegistry, isFeatureEnabled, - logging, QueryFormData, t, useTheme, @@ -50,12 +48,8 @@ import { usePermissions } from 'src/hooks/usePermissions'; import { Dropdown } from '@superset-ui/core/components'; import { updateDataMask } from 'src/dataMask/actions'; import DrillByModal from 'src/components/Chart/DrillBy/DrillByModal'; -import { useVerboseMap } from 'src/hooks/apiResources/datasets'; -import { Dataset } from 'src/components/Chart/types'; -import { - cachedSupersetGet, - supersetGetCache, -} from 'src/utils/cachedSupersetGet'; +import { useDatasetDrillInfo } from 'src/hooks/apiResources/datasets'; +import { ResourceStatus } from 'src/hooks/apiResources/apiResources'; import { DrillDetailMenuItems } from '../DrillDetail'; import { getMenuAdjustedY } from '../utils'; import { MenuItemTooltip } from '../DisabledMenuItemTooltip'; @@ -131,9 +125,6 @@ const ChartContextMenu = ( const [drillModalIsOpen, setDrillModalIsOpen] = useState(false); const [drillByColumn, setDrillByColumn] = useState(); const [showDrillByModal, setShowDrillByModal] = useState(false); - const [dataset, setDataset] = useState(); - const [isLoadingDataset, setIsLoadingDataset] = useState(false); - const verboseMap = useVerboseMap(dataset); const closeContextMenu = useCallback(() => { setVisible(false); @@ -166,43 +157,17 @@ const ChartContextMenu = ( canDrillBy && isDisplayed(ContextMenuItem.DrillBy); - useEffect(() => { - async function fetchDataset() { - if (!visible || dataset || (!showDrillBy && !showDrillToDetail)) return; - - const datasetId = Number(formData.datasource.split('__')[0]); - try { - setIsLoadingDataset(true); - let response; - - if (loadDrillByOptionsExtension) { - response = await loadDrillByOptionsExtension(datasetId, formData); - } else { - const endpoint = `/api/v1/dataset/${datasetId}/drill_info/?q=(dashboard_id:${dashboardId})`; - response = await cachedSupersetGet({ endpoint }); - } - - const { json } = response; - const { result } = json; - - setDataset(result); - } catch (error) { - logging.error('Failed to load dataset:', error); - supersetGetCache.delete(`/api/v1/dataset/${datasetId}/drill_info/`); - } finally { - setIsLoadingDataset(false); - } - } - - fetchDataset(); - }, [ - visible, - showDrillBy, - showDrillToDetail, + const datasetResource = useDatasetDrillInfo( formData.datasource, - loadDrillByOptionsExtension, dashboardId, - ]); + formData, + ); + + const dataset = + datasetResource.status === ResourceStatus.Complete + ? datasetResource.result + : undefined; + const isLoadingDataset = datasetResource.status === ResourceStatus.Loading; // Compute filteredDataset with all columns returned + a filtered list of valid drillable options const filteredDataset = useMemo(() => { @@ -338,7 +303,7 @@ const ChartContextMenu = ( onSelection={onSelection} submenuIndex={showCrossFilters ? 2 : 1} setShowModal={setDrillModalIsOpen} - dataset={filteredDataset} + dataset={filteredDataset!} isLoadingDataset={isLoadingDataset} {...(additionalConfig?.drillToDetail || {})} />, @@ -363,7 +328,7 @@ const ChartContextMenu = ( open={openKeys.includes('drill-by-submenu')} key="drill-by-submenu" onDrillBy={handleDrillBy} - dataset={filteredDataset} + dataset={filteredDataset!} isLoadingDataset={isLoadingDataset} {...(additionalConfig?.drillBy || {})} />, @@ -447,7 +412,7 @@ const ChartContextMenu = ( onHideModal={() => { setDrillModalIsOpen(false); }} - dataset={filteredDataset} + dataset={filteredDataset!} /> )} {showDrillByModal && drillByColumn && dataset && filters?.drillBy && ( @@ -456,7 +421,7 @@ const ChartContextMenu = ( drillByConfig={filters?.drillBy} formData={formData} onHideModal={handleCloseDrillByModal} - dataset={{ ...filteredDataset!, verbose_map: verboseMap }} + dataset={filteredDataset!} canDownload={canDownload} /> )} diff --git a/superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx b/superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx index 08508fe06c76..1f2b1188d6f0 100644 --- a/superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx +++ b/superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx @@ -137,7 +137,7 @@ export default function DrillDetailPane({ title: resultsPage?.colTypes[index] === GenericDataType.Temporal ? ( ) : ( - column + dataset?.verbose_map?.[column] || column ), render: value => { if (value === true || value === false) { @@ -179,7 +179,12 @@ export default function DrillDetailPane({ }, width: 150, })) || [], - [resultsPage?.colNames, resultsPage?.colTypes, timeFormatting], + [ + resultsPage?.colNames, + resultsPage?.colTypes, + timeFormatting, + dataset?.verbose_map, + ], ); const data: DataType[] = useMemo( diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx index b43941ad821f..2dcc08966d3b 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx @@ -59,6 +59,8 @@ import { LOG_ACTIONS_CHART_DOWNLOAD_AS_IMAGE } from 'src/logger/LogUtils'; import { MenuKeys, RootState } from 'src/dashboard/types'; import DrillDetailModal from 'src/components/Chart/DrillDetail/DrillDetailModal'; import { usePermissions } from 'src/hooks/usePermissions'; +import { useDatasetDrillInfo } from 'src/hooks/apiResources/datasets'; +import { ResourceStatus } from 'src/hooks/apiResources/apiResources'; import { useCrossFiltersScopingModal } from '../nativeFilters/FilterBar/CrossFilters/ScopingModal/useCrossFiltersScopingModal'; import { ViewResultsModalTrigger } from './ViewResultsModalTrigger'; @@ -177,6 +179,18 @@ const SliceHeaderControls = ( ?.behaviors?.includes(Behavior.InteractiveChart); const canExplore = props.supersetCanExplore; const { canDrillToDetail, canViewQuery, canViewTable } = usePermissions(); + + const datasetResource = useDatasetDrillInfo( + props.slice.datasource, + props.dashboardId, + props.formData, + ); + + const datasetWithVerboseMap = + datasetResource.status === ResourceStatus.Complete + ? datasetResource.result + : undefined; + const refreshChart = () => { if (props.updatedDttm) { props.forceRefresh(props.slice.slice_id, props.dashboardId); @@ -574,6 +588,7 @@ const SliceHeaderControls = ( }} chartId={slice.slice_id} showModal={drillModalIsOpen} + dataset={datasetWithVerboseMap} /> {canEditCrossFilters && scopingModal} diff --git a/superset-frontend/src/hooks/apiResources/datasets.ts b/superset-frontend/src/hooks/apiResources/datasets.ts index 4c8648085627..70e57574acce 100644 --- a/superset-frontend/src/hooks/apiResources/datasets.ts +++ b/superset-frontend/src/hooks/apiResources/datasets.ts @@ -17,10 +17,34 @@ * specific language governing permissions and limitations * under the License. */ -import { Column, Metric, ensureIsArray } from '@superset-ui/core'; +import { + Column, + logging, + Metric, + ensureIsArray, + getExtensionsRegistry, + QueryFormData, +} from '@superset-ui/core'; +import { useEffect, useState } from 'react'; import { Dataset } from 'src/components/Chart/types'; +import { + cachedSupersetGet, + supersetGetCache, +} from 'src/utils/cachedSupersetGet'; +import { Resource, ResourceStatus } from './apiResources'; -export const useVerboseMap = (dataset?: Dataset) => { +/** + * Utility function to extract numeric dataset ID from datasource string + */ +const getDatasetId = (datasetId: string | number): number => + typeof datasetId === 'string' + ? Number(datasetId.split('__')[0]) + : Number(datasetId); + +/** + * Helper function to create verbose_map from a dataset + */ +const createVerboseMap = (dataset?: Dataset): Record => { const verbose_map: Record = {}; ensureIsArray(dataset?.columns).forEach((column: Column) => { verbose_map[column.column_name] = column.verbose_name || column.column_name; @@ -30,3 +54,68 @@ export const useVerboseMap = (dataset?: Dataset) => { }); return verbose_map; }; + +/** + * Hook to fetch dataset drill info with extension support and verbose_map + * Handles both extension and standard API cases internally + */ +export const useDatasetDrillInfo = ( + datasetId: string | number, + dashboardId: number, + formData?: QueryFormData, +): Resource => { + const [resource, setResource] = useState>({ + status: ResourceStatus.Loading, + result: null, + error: null, + }); + + useEffect(() => { + const fetchDataset = async () => { + try { + const numericDatasetId = getDatasetId(datasetId); + const loadDrillByOptionsExtension = getExtensionsRegistry().get( + 'load.drillby.options', + ); + let result; + + if (loadDrillByOptionsExtension && formData) { + const response = await loadDrillByOptionsExtension( + numericDatasetId, + formData, + ); + result = response?.json?.result; + } else { + const endpoint = `/api/v1/dataset/${numericDatasetId}/drill_info/?q=(dashboard_id:${dashboardId})`; + try { + const { json } = await cachedSupersetGet({ endpoint }); + const { result: datasetResult } = json; + result = datasetResult; + } catch (error) { + logging.error('Failed to load dataset: ', error); + supersetGetCache.delete(endpoint); + throw error; + } + } + + const verbose_map = createVerboseMap(result); + + setResource({ + status: ResourceStatus.Complete, + result: { ...result, verbose_map }, + error: null, + }); + } catch (error) { + setResource({ + status: ResourceStatus.Error, + result: null, + error: error instanceof Error ? error : new Error(String(error)), + }); + } + }; + + fetchDataset(); + }, [datasetId, dashboardId, formData]); + + return resource; +}; From 38b206acbb1e688a6731bf11d168043c5845bf91 Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Fri, 8 Aug 2025 11:54:12 -0300 Subject: [PATCH 2/3] Adding test coverage --- .../DrillDetail/DrillDetailPane.test.tsx | 46 +++++++++++++++++++ .../src/hooks/apiResources/datasets.ts | 4 +- 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.test.tsx b/superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.test.tsx index 07a9cdf6e87b..3d5213f90e2d 100644 --- a/superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.test.tsx +++ b/superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.test.tsx @@ -188,3 +188,49 @@ test('should render the error', async () => { await waitForRender(); expect(screen.getByText('Error: Something went wrong')).toBeInTheDocument(); }); + +test('should use verbose_map for column headers when available', async () => { + jest.restoreAllMocks(); + + const datasetWithVerboseMap = { + ...MOCKED_DATASET, + verbose_map: { + year: 'Year of Release', + na_sales: 'North America Sales', + }, + }; + + fetchMock.post(SAMPLES_ENDPOINT, { + result: { + total_count: 1, + data: [ + { + year: 1996, + na_sales: 11.27, + eu_sales: 8.89, + }, + ], + colnames: ['year', 'na_sales', 'eu_sales'], + coltypes: [0, 0, 0], + }, + }); + + await waitForRender({ dataset: datasetWithVerboseMap }); + + expect( + screen.getByRole('columnheader', { name: 'Year of Release' }), + ).toBeInTheDocument(); + expect( + screen.getByRole('columnheader', { name: 'North America Sales' }), + ).toBeInTheDocument(); + expect( + screen.queryByRole('columnheader', { name: 'eu_sales' }), + ).toBeInTheDocument(); + + expect( + screen.queryByRole('columnheader', { name: 'year' }), + ).not.toBeInTheDocument(); + expect( + screen.queryByRole('columnheader', { name: 'na_sales' }), + ).not.toBeInTheDocument(); +}); diff --git a/superset-frontend/src/hooks/apiResources/datasets.ts b/superset-frontend/src/hooks/apiResources/datasets.ts index 70e57574acce..ddd091fbe252 100644 --- a/superset-frontend/src/hooks/apiResources/datasets.ts +++ b/superset-frontend/src/hooks/apiResources/datasets.ts @@ -36,7 +36,7 @@ import { Resource, ResourceStatus } from './apiResources'; /** * Utility function to extract numeric dataset ID from datasource string */ -const getDatasetId = (datasetId: string | number): number => +export const getDatasetId = (datasetId: string | number): number => typeof datasetId === 'string' ? Number(datasetId.split('__')[0]) : Number(datasetId); @@ -44,7 +44,7 @@ const getDatasetId = (datasetId: string | number): number => /** * Helper function to create verbose_map from a dataset */ -const createVerboseMap = (dataset?: Dataset): Record => { +export const createVerboseMap = (dataset?: Dataset): Record => { const verbose_map: Record = {}; ensureIsArray(dataset?.columns).forEach((column: Column) => { verbose_map[column.column_name] = column.verbose_name || column.column_name; From eb75d820c479d68214ad67c84ef2dfa2981dbc26 Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Fri, 8 Aug 2025 19:03:07 -0300 Subject: [PATCH 3/3] Addressing PR feedback --- .../ChartContextMenu/ChartContextMenu.tsx | 48 +++++++++++-------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx index ab79f1ad43c5..20c65effed3a 100644 --- a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx +++ b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx @@ -163,15 +163,21 @@ const ChartContextMenu = ( formData, ); - const dataset = - datasetResource.status === ResourceStatus.Complete - ? datasetResource.result - : undefined; const isLoadingDataset = datasetResource.status === ResourceStatus.Loading; // Compute filteredDataset with all columns returned + a filtered list of valid drillable options const filteredDataset = useMemo(() => { - if (!dataset || !showDrillBy) return dataset; + // Short circuit if still loading + if (datasetResource.status !== ResourceStatus.Complete) { + return undefined; + } + + // No need to filter the dataset if Drill By is not allowed + if (!showDrillBy) { + return datasetResource.result; + } + + const dataset = datasetResource.result; const filteredColumns = ensureIsArray(dataset.columns).filter( column => @@ -191,7 +197,8 @@ const ChartContextMenu = ( drillable_columns: filteredColumns, }; }, [ - dataset, + datasetResource.status, + datasetResource.result, showDrillBy, filters?.drillBy?.groupbyFieldName, formData.x_axis, @@ -303,7 +310,7 @@ const ChartContextMenu = ( onSelection={onSelection} submenuIndex={showCrossFilters ? 2 : 1} setShowModal={setDrillModalIsOpen} - dataset={filteredDataset!} + dataset={filteredDataset} isLoadingDataset={isLoadingDataset} {...(additionalConfig?.drillToDetail || {})} />, @@ -328,7 +335,7 @@ const ChartContextMenu = ( open={openKeys.includes('drill-by-submenu')} key="drill-by-submenu" onDrillBy={handleDrillBy} - dataset={filteredDataset!} + dataset={filteredDataset} isLoadingDataset={isLoadingDataset} {...(additionalConfig?.drillBy || {})} />, @@ -412,19 +419,22 @@ const ChartContextMenu = ( onHideModal={() => { setDrillModalIsOpen(false); }} - dataset={filteredDataset!} - /> - )} - {showDrillByModal && drillByColumn && dataset && filters?.drillBy && ( - )} + {showDrillByModal && + drillByColumn && + filteredDataset && + filters?.drillBy && ( + + )} , document.body, );