From 81c85ecf0fee17f830148808338b9a673c38799f Mon Sep 17 00:00:00 2001 From: Endi Monan Date: Thu, 20 Aug 2026 14:00:56 +0200 Subject: [PATCH 1/5] fix(reports): look up fetched report schedules by the list API id fields The reducer matched fetched reports by dashboard/chart, but the list API returns dashboard_id/chart_id, so the schedule was dropped and the menu stayed on "Set up an email report" after a reload. Fixes #43344 --- .../features/reports/ReportModal/actions.ts | 2 + .../reports/ReportModal/reducer.test.ts | 82 ++++++++++++++++++- .../features/reports/ReportModal/reducer.ts | 12 +-- .../src/features/reports/types.ts | 2 + 4 files changed, 89 insertions(+), 9 deletions(-) diff --git a/superset-frontend/src/features/reports/ReportModal/actions.ts b/superset-frontend/src/features/reports/ReportModal/actions.ts index 25119e1a7742..607df3a1aab2 100644 --- a/superset-frontend/src/features/reports/ReportModal/actions.ts +++ b/superset-frontend/src/features/reports/ReportModal/actions.ts @@ -252,6 +252,8 @@ export interface DeletableReport { creation_method?: string; dashboard?: number; chart?: number; + dashboard_id?: number | null; + chart_id?: number | null; } export interface DeleteReportAction { diff --git a/superset-frontend/src/features/reports/ReportModal/reducer.test.ts b/superset-frontend/src/features/reports/ReportModal/reducer.test.ts index 7fa70ef23cd4..9f34327619f5 100644 --- a/superset-frontend/src/features/reports/ReportModal/reducer.test.ts +++ b/superset-frontend/src/features/reports/ReportModal/reducer.test.ts @@ -53,7 +53,7 @@ const makeReport = (overrides: Partial = {}): ReportObject => ({ }); test('SET_REPORT stores report keyed by resourceId under creationMethod', () => { - const report = makeReport({ id: 5, dashboard: 10 }); + const report = makeReport({ id: 5, dashboard_id: 10, chart_id: null }); const action: SetReportAction = { type: SET_REPORT, report: { result: [report] }, @@ -84,10 +84,11 @@ test('SET_REPORT removes entry when API returns empty result', () => { expect(result.dashboards?.[10]).toBeUndefined(); }); -test('SET_REPORT uses chart property when filterField is chart_id', () => { +test('SET_REPORT matches on chart_id when filterField is chart_id', () => { const report = makeReport({ id: 7, - chart: 42, + chart_id: 42, + dashboard_id: null, creation_method: 'charts', }); const action: SetReportAction = { @@ -103,6 +104,23 @@ test('SET_REPORT uses chart property when filterField is chart_id', () => { expect(result.charts?.[42]).toEqual(report); }); +test('SET_REPORT keeps an existing report when the fetched payload matches it', () => { + const existing = makeReport({ id: 5, dashboard_id: 10 }); + const initial: ReportsState = { dashboards: { 10: existing } }; + const refetched = makeReport({ id: 5, dashboard_id: 10, active: false }); + const action: SetReportAction = { + type: SET_REPORT, + report: { result: [refetched] }, + resourceId: 10, + creationMethod: 'dashboards', + filterField: 'dashboard_id', + }; + + const result = reportsReducer(initial, action); + + expect(result.dashboards?.[10]).toEqual(refetched); +}); + test('ADD_REPORT keys dashboard report by dashboard id', () => { const action: AddReportAction = { type: ADD_REPORT, @@ -191,6 +209,64 @@ test('DELETE_REPORT removes report from state', () => { expect(result.dashboards?.[10]).toBeUndefined(); }); +test('DELETE_REPORT removes a report fetched from the list API', () => { + const report = makeReport({ + id: 5, + dashboard_id: 10, + creation_method: 'dashboards', + }); + const initial: ReportsState = { dashboards: { 10: report } }; + const action: DeleteReportAction = { + type: DELETE_REPORT, + report: { + id: 5, + dashboard_id: 10, + creation_method: 'dashboards', + }, + }; + + const result = reportsReducer(initial, action); + + expect(result.dashboards?.[10]).toBeUndefined(); +}); + +test('DELETE_REPORT keys a list API chart report by chart_id', () => { + const report = makeReport({ + id: 7, + dashboard_id: null, + chart_id: 42, + creation_method: 'charts', + }); + const initial: ReportsState = { charts: { 42: report } }; + const action: DeleteReportAction = { + type: DELETE_REPORT, + report: { + id: 7, + dashboard_id: null, + chart_id: 42, + creation_method: 'charts', + }, + }; + + const result = reportsReducer(initial, action); + + expect(result.charts?.[42]).toBeUndefined(); +}); + +test('DELETE_REPORT returns unchanged state when no resource id is present', () => { + const initial: ReportsState = { + dashboards: { 10: makeReport({ id: 5, dashboard_id: 10 }) }, + }; + const action: DeleteReportAction = { + type: DELETE_REPORT, + report: { id: 5, creation_method: 'dashboards' }, + }; + + const result = reportsReducer(initial, action); + + expect(result).toBe(initial); +}); + test('DELETE_REPORT for alerts_reports keys by report id', () => { const report = makeReport({ id: 99, creation_method: 'alerts_reports' }); const initial: ReportsState = { alerts_reports: { 99: report } }; diff --git a/superset-frontend/src/features/reports/ReportModal/reducer.ts b/superset-frontend/src/features/reports/ReportModal/reducer.ts index 08a3001c0067..83e9fba9cde3 100644 --- a/superset-frontend/src/features/reports/ReportModal/reducer.ts +++ b/superset-frontend/src/features/reports/ReportModal/reducer.ts @@ -52,14 +52,10 @@ export default function reportsReducer( [SET_REPORT]() { const { report, resourceId, creationMethod, filterField } = action as SetReportAction; - // Map filterField ('dashboard_id' or 'chart_id') to the corresponding - // ReportObject property ('dashboard' or 'chart') - const propertyName = - filterField === 'dashboard_id' ? 'dashboard' : 'chart'; // For now report count should only be one, but we are checking in case // functionality changes. const reportObject = report.result?.find( - (r: ReportObject) => r[propertyName] === resourceId, + (r: ReportObject) => r[filterField] === resourceId, ); if (reportObject) { @@ -159,7 +155,11 @@ export default function reportsReducer( const key = creationMethod === 'alerts_reports' ? report.id - : (report.dashboard ?? report.chart); + : (report.dashboard ?? + report.chart ?? + report.dashboard_id ?? + report.chart_id ?? + undefined); if (key === undefined) { return state; diff --git a/superset-frontend/src/features/reports/types.ts b/superset-frontend/src/features/reports/types.ts index 133a3d03047a..841fd76283a1 100644 --- a/superset-frontend/src/features/reports/types.ts +++ b/superset-frontend/src/features/reports/types.ts @@ -42,6 +42,8 @@ export interface ReportObject { crontab: string; dashboard?: number; chart?: number; + dashboard_id?: number | null; + chart_id?: number | null; description?: string; log_retention: number; name: string; From 32a58aafb0501f29234462db4a2418b3031bff83 Mon Sep 17 00:00:00 2001 From: Endi Monan Date: Thu, 20 Aug 2026 14:01:04 +0200 Subject: [PATCH 2/5] test(reports): cover the report fetch round-trip against the real list payload --- .../HeaderReportDropdown/index.test.tsx | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/superset-frontend/src/features/reports/ReportModal/HeaderReportDropdown/index.test.tsx b/superset-frontend/src/features/reports/ReportModal/HeaderReportDropdown/index.test.tsx index 94c94354d444..863665df0793 100644 --- a/superset-frontend/src/features/reports/ReportModal/HeaderReportDropdown/index.test.tsx +++ b/superset-frontend/src/features/reports/ReportModal/HeaderReportDropdown/index.test.tsx @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ +import fetchMock from 'fetch-mock'; import { act, render, screen, userEvent } from 'spec/helpers/testing-library'; import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core'; import { Menu, MenuItem } from '@superset-ui/core/components/Menu'; @@ -143,6 +144,35 @@ describe('Header Report Dropdown', () => { mockedIsFeatureEnabled.mockRestore(); }); + afterEach(() => { + fetchMock.clearHistory().removeRoutes(); + }); + + test('switches to the manage menu once the existing report is fetched', async () => { + const mockedProps = createProps(); + fetchMock.get('glob:*/api/v1/report/?q=*', { + count: 1, + result: [ + { + active: true, + creation_method: 'dashboards', + crontab: '0 12 * * 1', + dashboard_id: 1, + chart_id: null, + id: 5, + name: 'Weekly Report', + type: 'Report', + }, + ], + }); + act(() => { + setup(mockedProps, stateWithOnlyUser); + }); + expect(await screen.findByText('Email reports active')).toBeInTheDocument(); + expect(screen.getByText('Edit email report')).toBeInTheDocument(); + expect(screen.getByText('Delete email report')).toBeInTheDocument(); + }); + test('renders correctly', () => { const mockedProps = createProps(); act(() => { From 73e50abbc93bdd7b4e28e0a001ae515fa57c7a74 Mon Sep 17 00:00:00 2001 From: Endi Monan Date: Wed, 26 Aug 2026 12:38:12 +0200 Subject: [PATCH 3/5] fix(reports): include report format in schedule list response --- superset/reports/api.py | 1 + tests/integration_tests/reports/api_tests.py | 1 + 2 files changed, 2 insertions(+) diff --git a/superset/reports/api.py b/superset/reports/api.py index 05337f9c0651..c19332c6282e 100644 --- a/superset/reports/api.py +++ b/superset/reports/api.py @@ -183,6 +183,7 @@ def ensure_alert_reports_enabled(self) -> Optional[Response]: "name", "recipients.id", "recipients.type", + "report_format", "timezone", "type", "retry_on_failure", diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index 9b6c629e58c8..8a89eec34714 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -377,6 +377,7 @@ def test_get_list_report_schedule(self): "last_state", "name", "recipients", + "report_format", "retry_max_attempts", "retry_notify_owners", "retry_notify_recipients", From 267917a89dc8ee418c3bdb06425167659fe9f44c Mon Sep 17 00:00:00 2001 From: Endi Monan Date: Wed, 26 Aug 2026 12:38:43 +0200 Subject: [PATCH 4/5] test(reports): cover report format edit round trip --- .../HeaderReportDropdown/index.test.tsx | 1 + .../reports/ReportModal/ReportModal.test.tsx | 61 +++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/superset-frontend/src/features/reports/ReportModal/HeaderReportDropdown/index.test.tsx b/superset-frontend/src/features/reports/ReportModal/HeaderReportDropdown/index.test.tsx index 863665df0793..cdf09ff0bd94 100644 --- a/superset-frontend/src/features/reports/ReportModal/HeaderReportDropdown/index.test.tsx +++ b/superset-frontend/src/features/reports/ReportModal/HeaderReportDropdown/index.test.tsx @@ -161,6 +161,7 @@ describe('Header Report Dropdown', () => { chart_id: null, id: 5, name: 'Weekly Report', + report_format: 'PNG', type: 'Report', }, ], diff --git a/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx b/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx index 9e0ec1e2b497..5a935e21f115 100644 --- a/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx +++ b/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx @@ -352,6 +352,67 @@ test('edit mode dispatches editReport via PUT on save', async () => { fetchMock.removeRoute('put-report-42'); }); +test('edit mode preserves the fetched report_format on save', async () => { + // XLSX is never the visualization default, so this fails if the stored + // format is dropped and the save falls back to defaultNotificationFormat. + const fetchedReport = { + active: true, + chart_id: 96, + creation_method: 'charts', + crontab: '0 12 * * 1', + dashboard_id: null, + description: '', + id: 44, + name: 'Existing XLSX Report', + report_format: 'XLSX', + timezone: 'America/New_York', + type: 'Report', + }; + const store = createStore( + { + reports: { + charts: { 96: fetchedReport }, + }, + }, + reducerIndex, + ); + + fetchMock.put( + 'glob:*/api/v1/report/44', + { id: 44, result: {} }, + { + name: 'put-report-44', + }, + ); + + const chartProps = { + ...defaultProps, + dashboardId: undefined, + chart: { id: 96, sliceFormData: { viz_type: VizType.Line } }, + creationMethod: 'charts' as const, + }; + render(, { useRedux: true, store }); + + expect(screen.getByText('Edit email report')).toBeInTheDocument(); + expect( + screen.getByRole('radio', { name: 'Formatted Excel attached in email' }), + ).toBeChecked(); + + const saveButton = screen.getByRole('button', { name: /save/i }); + await waitFor(() => userEvent.click(saveButton)); + + await waitFor(() => { + const calls = fetchMock.callHistory.calls('put-report-44'); + expect(calls.length).toBeGreaterThan(0); + }); + + const calls = fetchMock.callHistory.calls('put-report-44'); + const body = JSON.parse(calls[calls.length - 1].options.body as string); + expect(body.report_format).toBe('XLSX'); + + fetchMock.removeRoute('put-report-44'); +}); + test('edit mode does not fall back to user id when subject id is unavailable', async () => { mockedGetBootstrapData.mockReturnValue( bootstrapData({ From 756eb49354a207d2badd95ebd9d81cf84ffa2fd3 Mon Sep 17 00:00:00 2001 From: Endi Monan Date: Mon, 31 Aug 2026 19:08:53 +0200 Subject: [PATCH 5/5] fix(reports): scope report modal by creation method --- .../reports/ReportModal/ReportModal.test.tsx | 107 +++++++++++++++++- .../features/reports/ReportModal/index.tsx | 16 +-- 2 files changed, 115 insertions(+), 8 deletions(-) diff --git a/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx b/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx index 08f000167858..4ae28ac7f40d 100644 --- a/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx +++ b/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx @@ -170,10 +170,45 @@ test('creates a new email report via modal Add button', async () => { // creation_method, editors, and recipients are set server-side; not in the client payload expect(body.creation_method).toBeUndefined(); expect(body.recipients).toBeUndefined(); + expect(body.dashboard).toBe(1); + expect(body.chart).toBeUndefined(); fetchMock.removeRoute('post-subscribe'); }); +test('creating a chart report in a dashboard context sends only the chart id', async () => { + // Explore opened from a dashboard passes a dashboardId alongside the chart; + // a payload carrying both ids is rejected by the API with a 422. + fetchMock.post( + 'glob:*/api/v1/report/subscribe', + { id: 1, result: {} }, + { name: 'post-subscribe-chart' }, + ); + + const chartFromDashboardProps = { + ...defaultProps, + dashboardId: 7, + chart: { id: 119, sliceFormData: { viz_type: VizType.Line } }, + creationMethod: 'charts' as const, + }; + render(, { useRedux: true }); + + const addButton = screen.getByRole('button', { name: /add/i }); + await userEvent.click(addButton); + + await waitFor(() => { + const postCalls = fetchMock.callHistory.calls('post-subscribe-chart'); + expect(postCalls).toHaveLength(1); + }); + + const postCalls = fetchMock.callHistory.calls('post-subscribe-chart'); + const body = JSON.parse(postCalls[0].options.body as string); + expect(body.chart).toBe(119); + expect(body.dashboard).toBeUndefined(); + + fetchMock.removeRoute('post-subscribe-chart'); +}); + test('text-based chart hides screenshot width and shows message content', () => { // Table is text-based: should show message content but hide custom width const textChartProps = { @@ -288,6 +323,76 @@ test('renders edit mode when report exists in store', () => { expect(screen.getByRole('button', { name: /save/i })).toBeInTheDocument(); }); +test('edit mode in a dashboard context resolves and saves the chart report', async () => { + // With both schedules in the store, the modal must load the chart's own + // report, not the dashboard's, and PUT to its id with only the chart set. + const dashboardReport = { + id: 42, + name: 'Existing Dashboard Report', + creation_method: 'dashboards', + crontab: '0 12 * * 1', + dashboard_id: 7, + chart_id: null, + report_format: 'PNG', + active: true, + type: 'Report', + timezone: 'America/New_York', + }; + const chartReport = { + ...dashboardReport, + id: 77, + name: 'Existing Chart Report', + creation_method: 'charts', + dashboard_id: null, + chart_id: 119, + }; + const store = createStore( + { + reports: { + dashboards: { 7: dashboardReport }, + charts: { 119: chartReport }, + }, + }, + reducerIndex, + ); + + fetchMock.put( + 'glob:*/api/v1/report/77', + { id: 77, result: {} }, + { name: 'put-report-77' }, + ); + + const chartFromDashboardProps = { + ...defaultProps, + dashboardId: 7, + chart: { id: 119, sliceFormData: { viz_type: VizType.Line } }, + creationMethod: 'charts' as const, + }; + render(, { + useRedux: true, + store, + }); + + expect(screen.getByTestId('report-name-test')).toHaveDisplayValue( + 'Existing Chart Report', + ); + + const saveButton = screen.getByRole('button', { name: /save/i }); + await userEvent.click(saveButton); + + await waitFor(() => { + const calls = fetchMock.callHistory.calls('put-report-77'); + expect(calls.length).toBeGreaterThan(0); + }); + + const calls = fetchMock.callHistory.calls('put-report-77'); + const body = JSON.parse(calls[calls.length - 1].options.body as string); + expect(body.chart).toBe(119); + expect(body.dashboard).toBeUndefined(); + + fetchMock.removeRoute('put-report-77'); +}); + test('edit mode dispatches editReport via PUT on save', async () => { const existingReport = { id: 42, @@ -399,7 +504,7 @@ test('edit mode preserves the fetched report_format on save', async () => { ).toBeChecked(); const saveButton = screen.getByRole('button', { name: /save/i }); - await waitFor(() => userEvent.click(saveButton)); + await userEvent.click(saveButton); await waitFor(() => { const calls = fetchMock.callHistory.calls('put-report-44'); diff --git a/superset-frontend/src/features/reports/ReportModal/index.tsx b/superset-frontend/src/features/reports/ReportModal/index.tsx index c90e9393b239..13108b92e483 100644 --- a/superset-frontend/src/features/reports/ReportModal/index.tsx +++ b/superset-frontend/src/features/reports/ReportModal/index.tsx @@ -170,12 +170,13 @@ function ReportModal({ const dispatch = useDispatch(); // Report fetch logic const report = useSelector(state => { - const resourceType = dashboardId - ? CreationMethod.Dashboards - : CreationMethod.Charts; + const isChartReport = creationMethod === CreationMethod.Charts; return ( - reportSelector(state, resourceType, dashboardId || chart?.id) || - EMPTY_OBJECT + reportSelector( + state, + isChartReport ? CreationMethod.Charts : CreationMethod.Dashboards, + isChartReport ? chart?.id : dashboardId, + ) || EMPTY_OBJECT ); }); const isEditMode = report && Object.keys(report).length; @@ -194,8 +195,9 @@ function ReportModal({ active: true, force_screenshot: false, custom_width: currentReport.custom_width, - dashboard: dashboardId, - chart: chart?.id, + ...(creationMethod === CreationMethod.Charts + ? { chart: chart?.id } + : { dashboard: dashboardId }), name: currentReport.name, description: currentReport.description, crontab: currentReport.crontab,