Skip to content

Commit e8f275d

Browse files
mistercrunchclaude
andcommitted
feat: enhance drill-through modal Edit chart button visibility and navigation
- Only show Edit chart button when drill-through chart is configured - Fix navigation to properly route to explore page instead of dashboard - Use Button href for same-tab navigation (no target="_blank") - Add Dataset.id type definition for URL generation - Update tests to verify link behavior and required dataset properties 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent f0bb7b6 commit e8f275d

3 files changed

Lines changed: 76 additions & 30 deletions

File tree

superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.test.tsx

Lines changed: 43 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,10 @@ const drillToDetailModalState = {
5151
},
5252
};
5353

54-
const renderModal = async (overrideState: Record<string, any> = {}) => {
54+
const renderModal = async (
55+
overrideState: Record<string, any> = {},
56+
dataset?: any,
57+
) => {
5558
const DrillDetailModalWrapper = () => {
5659
const [showModal, setShowModal] = useState(false);
5760
return (
@@ -65,6 +68,7 @@ const renderModal = async (overrideState: Record<string, any> = {}) => {
6568
initialFilters={[]}
6669
showModal={showModal}
6770
onHideModal={() => setShowModal(false)}
71+
dataset={dataset}
6872
/>
6973
</>
7074
);
@@ -88,11 +92,21 @@ test('should render the title', async () => {
8892
expect(screen.getByText(`Drill to detail: ${chartName}`)).toBeInTheDocument();
8993
});
9094

91-
test('should render the button', async () => {
95+
test('should not render Edit chart button when no drill-through chart is configured', async () => {
9296
await renderModal();
9397
expect(
94-
screen.getByRole('button', { name: 'Edit chart' }),
95-
).toBeInTheDocument();
98+
screen.queryByRole('button', { name: 'Edit chart' }),
99+
).not.toBeInTheDocument();
100+
expect(screen.getAllByRole('button', { name: 'Close' })).toHaveLength(2);
101+
});
102+
103+
test('should render Edit chart link when drill-through chart is configured', async () => {
104+
const datasetWithDrillThrough = {
105+
drill_through_chart_id: 123,
106+
id: 456, // Required for URL generation
107+
};
108+
await renderModal({}, datasetWithDrillThrough);
109+
expect(screen.getByRole('link', { name: 'Edit chart' })).toBeInTheDocument();
96110
expect(screen.getAllByRole('button', { name: 'Close' })).toHaveLength(2);
97111
});
98112

@@ -103,20 +117,35 @@ test('should close the modal', async () => {
103117
expect(screen.queryByRole('dialog')).not.toBeInTheDocument();
104118
});
105119

106-
test('should forward to Explore', async () => {
107-
await renderModal();
108-
userEvent.click(screen.getByRole('button', { name: 'Edit chart' }));
109-
expect(mockHistoryPush).toHaveBeenCalledWith(
110-
`/explore/?dashboard_page_id=&slice_id=${sliceId}`,
120+
test('should have correct href for drill-through chart', async () => {
121+
const drillThroughChartId = 123;
122+
const datasetId = 456;
123+
const datasetWithDrillThrough = {
124+
drill_through_chart_id: drillThroughChartId,
125+
id: datasetId,
126+
};
127+
await renderModal({}, datasetWithDrillThrough);
128+
const editLink = screen.getByRole('link', { name: 'Edit chart' });
129+
expect(editLink).toHaveAttribute(
130+
'href',
131+
`/explore/?dashboard_page_id=&slice_id=${drillThroughChartId}`,
111132
);
133+
expect(editLink).not.toHaveAttribute('target'); // We removed target="_blank"
112134
});
113135

114136
test('should render "Edit chart" as disabled without can_explore permission', async () => {
115-
await renderModal({
116-
user: {
117-
...drillToDetailModalState.user,
118-
roles: { Admin: [['invalid_permission', 'Superset']] },
137+
const datasetWithDrillThrough = {
138+
drill_through_chart_id: 123,
139+
id: 456, // Required for URL generation
140+
};
141+
await renderModal(
142+
{
143+
user: {
144+
...drillToDetailModalState.user,
145+
roles: { Admin: [['invalid_permission', 'Superset']] },
146+
},
119147
},
120-
});
148+
datasetWithDrillThrough,
149+
);
121150
expect(screen.getByRole('button', { name: 'Edit chart' })).toBeDisabled();
122151
});

superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,7 @@
1717
* under the License.
1818
*/
1919

20-
import { useCallback, useContext, useMemo } from 'react';
21-
import { useHistory } from 'react-router-dom';
20+
import { useContext, useMemo } from 'react';
2221
import {
2322
BinaryQueryObjectFilterClause,
2423
css,
@@ -40,23 +39,25 @@ import DrillDetailPane from './DrillDetailPane';
4039
interface ModalFooterProps {
4140
canExplore: boolean;
4241
closeModal?: () => void;
43-
exploreChart: () => void;
42+
showEditButton: boolean;
43+
exploreUrl: string;
4444
}
4545

4646
const ModalFooter = ({
4747
canExplore,
4848
closeModal,
49-
exploreChart,
49+
showEditButton,
50+
exploreUrl,
5051
}: ModalFooterProps) => {
5152
const theme = useTheme();
5253

5354
return (
5455
<>
55-
{!isEmbedded() && (
56+
{!isEmbedded() && showEditButton && (
5657
<Button
5758
buttonStyle="secondary"
5859
buttonSize="small"
59-
onClick={exploreChart}
60+
href={canExplore ? exploreUrl : undefined}
6061
disabled={!canExplore}
6162
tooltip={
6263
!canExplore
@@ -100,7 +101,6 @@ export default function DrillDetailModal({
100101
dataset,
101102
}: DrillDetailModalProps) {
102103
const theme = useTheme();
103-
const history = useHistory();
104104
const dashboardPageId = useContext(DashboardPageIdContext);
105105
const { slice_name: chartName } = useSelector(
106106
(state: { sliceEntities: { slices: Record<number, Slice> } }) =>
@@ -110,23 +110,35 @@ export default function DrillDetailModal({
110110
findPermission('can_explore', 'Superset', state.user?.roles),
111111
);
112112

113+
// Determine if we should show the Edit Chart button
114+
const showEditButton = Boolean(dataset?.drill_through_chart_id);
115+
113116
const exploreUrl = useMemo(() => {
114-
// Use drill-through chart ID if configured, otherwise use original chart
115-
const targetChartId = dataset?.drill_through_chart_id || chartId;
117+
// Only compute URL when modal is open and drill-through chart exists
118+
if (!showModal || !dataset?.drill_through_chart_id || !dataset?.id) {
119+
return '';
120+
}
121+
122+
// Construct datasource string in the format expected by getExploreUrl
123+
const datasource = `${dataset.id}__table`;
124+
116125
return getExploreUrl({
117126
formData: {
118-
slice_id: targetChartId,
127+
slice_id: dataset.drill_through_chart_id,
128+
datasource,
119129
},
120130
endpointType: 'base',
131+
curUrl: null, // Explicitly prevent inheriting current dashboard URL
121132
requestParams: dashboardPageId
122133
? { dashboard_page_id: dashboardPageId }
123134
: {},
124135
});
125-
}, [chartId, dashboardPageId, dataset?.drill_through_chart_id]);
126-
127-
const exploreChart = useCallback(() => {
128-
history.push(exploreUrl);
129-
}, [exploreUrl, history]);
136+
}, [
137+
showModal,
138+
dashboardPageId,
139+
dataset?.drill_through_chart_id,
140+
dataset?.id,
141+
]);
130142

131143
return (
132144
<Modal
@@ -141,7 +153,11 @@ export default function DrillDetailModal({
141153
name={t('Drill to detail: %s', chartName)}
142154
title={t('Drill to detail: %s', chartName)}
143155
footer={
144-
<ModalFooter exploreChart={exploreChart} canExplore={canExplore} />
156+
<ModalFooter
157+
canExplore={canExplore}
158+
showEditButton={showEditButton}
159+
exploreUrl={exploreUrl || ''}
160+
/>
145161
}
146162
responsive
147163
resizable

superset-frontend/src/components/Chart/types.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ export enum DrillByType {
2424
}
2525

2626
export type Dataset = {
27+
id?: number;
2728
changed_by?: {
2829
first_name: string;
2930
last_name: string;

0 commit comments

Comments
 (0)