You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/09/04 21:49:12 UTC
(superset) branch master updated: fix(reports): look up fetched report schedules by the list API id fields (#43365)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 67a417812d3 fix(reports): look up fetched report schedules by the list API id fields (#43365)
67a417812d3 is described below
commit 67a417812d37922fc4e4c467f719a09a882fa5af
Author: Endi Monan <65...@users.noreply.github.com>
AuthorDate: Fri Sep 4 23:49:06 2026 +0200
fix(reports): look up fetched report schedules by the list API id fields (#43365)
---
.../HeaderReportDropdown/index.test.tsx | 31 ++++
.../reports/ReportModal/ReportModal.test.tsx | 166 +++++++++++++++++++++
.../src/features/reports/ReportModal/actions.ts | 2 +
.../src/features/reports/ReportModal/index.tsx | 16 +-
.../features/reports/ReportModal/reducer.test.ts | 82 +++++++++-
.../src/features/reports/ReportModal/reducer.ts | 12 +-
superset-frontend/src/features/reports/types.ts | 2 +
superset/reports/api.py | 1 +
tests/integration_tests/reports/api_tests.py | 1 +
9 files changed, 297 insertions(+), 16 deletions(-)
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 94c94354d44..cdf09ff0bd9 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,36 @@ 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',
+ report_format: 'PNG',
+ 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(() => {
diff --git a/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx b/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx
index 3008c44c190..4ae28ac7f40 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(<ReportModal {...chartFromDashboardProps} />, { 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(<ReportModal {...chartFromDashboardProps} />, {
+ 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,
@@ -352,6 +457,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(<ReportModal {...chartProps} />, { 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 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({
diff --git a/superset-frontend/src/features/reports/ReportModal/actions.ts b/superset-frontend/src/features/reports/ReportModal/actions.ts
index 25119e1a774..607df3a1aab 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/index.tsx b/superset-frontend/src/features/reports/ReportModal/index.tsx
index c90e9393b23..13108b92e48 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<any, ReportObject>(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,
diff --git a/superset-frontend/src/features/reports/ReportModal/reducer.test.ts b/superset-frontend/src/features/reports/ReportModal/reducer.test.ts
index 7fa70ef23cd..9f34327619f 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> = {}): 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 08a3001c006..83e9fba9cde 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 133a3d03047..841fd76283a 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;
diff --git a/superset/reports/api.py b/superset/reports/api.py
index 19f303518fd..9a6e424c601 100644
--- a/superset/reports/api.py
+++ b/superset/reports/api.py
@@ -186,6 +186,7 @@ class ReportScheduleRestApi(BaseSupersetModelRestApi):
"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 9b6c629e58c..8a89eec3471 100644
--- a/tests/integration_tests/reports/api_tests.py
+++ b/tests/integration_tests/reports/api_tests.py
@@ -377,6 +377,7 @@ class TestReportSchedulesApi(SupersetTestCase):
"last_state",
"name",
"recipients",
+ "report_format",
"retry_max_attempts",
"retry_notify_owners",
"retry_notify_recipients",