You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ri...@apache.org on 2026/08/21 22:33:40 UTC
(superset) branch master updated: fix(charts): surface blocking alerts/reports when archiving a chart (#43402)
This is an automated email from the ASF dual-hosted git repository.
richardfogaca 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 5f3e34908cc fix(charts): surface blocking alerts/reports when archiving a chart (#43402)
5f3e34908cc is described below
commit 5f3e34908cc0590b5cbdae47e991fe42394e7a70
Author: Mike Bridge <mi...@preset.io>
AuthorDate: Fri Aug 21 16:33:32 2026 -0600
fix(charts): surface blocking alerts/reports when archiving a chart (#43402)
Co-authored-by: Claude Fable 5 <no...@anthropic.com>
---
docs/docs/using-superset/recently-archived.mdx | 5 +
.../src/pages/ChartList/ChartList.test.tsx | 220 +++++++++++++++++++++
superset-frontend/src/pages/ChartList/index.tsx | 218 +++++++++++++++-----
superset-frontend/src/views/CRUD/utils.test.tsx | 28 +++
superset-frontend/src/views/CRUD/utils.tsx | 6 +-
superset/commands/chart/delete.py | 60 +++++-
tests/integration_tests/charts/api_tests.py | 10 +-
.../integration_tests/charts/soft_delete_tests.py | 5 +-
.../commands/chart/delete_message_test.py | 63 ++++++
9 files changed, 554 insertions(+), 61 deletions(-)
diff --git a/docs/docs/using-superset/recently-archived.mdx b/docs/docs/using-superset/recently-archived.mdx
index 0aa76cb35b5..777e4efa56f 100644
--- a/docs/docs/using-superset/recently-archived.mdx
+++ b/docs/docs/using-superset/recently-archived.mdx
@@ -11,6 +11,11 @@ When soft-delete is enabled, deleting a chart, dashboard, or dataset archives it
instead of removing it permanently. The **Recently Archived** view lets owners
and admins find archived objects and restore them.
+A chart used by an alert or report cannot be archived while that dependency
+exists. In the chart list view, the archive confirmation lists the alerts and
+reports that use the chart; a blocked attempt names them and asks you to
+detach or delete them first.
+
:::note
This view is gated by the `SOFT_DELETE` feature flag. When the flag is off the
diff --git a/superset-frontend/src/pages/ChartList/ChartList.test.tsx b/superset-frontend/src/pages/ChartList/ChartList.test.tsx
index d577743e765..f5e557b9bba 100644
--- a/superset-frontend/src/pages/ChartList/ChartList.test.tsx
+++ b/superset-frontend/src/pages/ChartList/ChartList.test.tsx
@@ -383,3 +383,223 @@ describe('ChartList - Global Filter Interactions', () => {
expect((searchInput as HTMLInputElement).value).toBe('');
});
});
+
+// The blocking-alerts/reports pre-flight in the Archive modal (sc-117151).
+// Each test registers its report-API route BEFORE setupMocks so it takes
+// precedence over the catch-all route.
+const adminChartUser = { ...mockUser, username: 'admin', permissions: {} };
+
+const openFirstDeleteModal = async () => {
+ // ALERT_REPORTS must be on for the pre-flight to fire at all — with it off
+ // the modal opens synchronously with no dependency fetch (see the flag-off
+ // test below).
+ (
+ isFeatureEnabled as jest.MockedFunction<typeof isFeatureEnabled>
+ ).mockImplementation(
+ (feature: string) =>
+ feature === 'SOFT_DELETE' || feature === 'ALERT_REPORTS',
+ );
+ renderChartList(adminChartUser);
+ await screen.findByTestId('chart-list-view');
+ const deleteButtons = await screen.findAllByTestId('chart-row-delete');
+ fireEvent.click(deleteButtons[0]);
+ return screen.findByRole('dialog');
+};
+
+test('archive modal lists the blocking alerts and reports with their types', async () => {
+ fetchMock.removeRoutes();
+ fetchMock.get('glob:*/api/v1/report/*', {
+ count: 2,
+ result: [
+ { id: 1, name: 'TC-081 rerun report', type: 'Report' },
+ { id: 2, name: 'Threshold alert', type: 'Alert' },
+ ],
+ });
+ setupMocks();
+ try {
+ const dialog = await openFirstDeleteModal();
+ expect(
+ within(dialog).getByText('Associated alerts and reports'),
+ ).toBeInTheDocument();
+ expect(within(dialog).getByText('TC-081 rerun report')).toBeInTheDocument();
+ expect(within(dialog).getByText('Threshold alert')).toBeInTheDocument();
+ expect(within(dialog).getByText('Report')).toBeInTheDocument();
+ expect(within(dialog).getByText('Alert')).toBeInTheDocument();
+ // Advisory only: the Archive button stays enabled.
+ expect(
+ within(dialog).getByRole('button', { name: 'Archive' }),
+ ).toBeEnabled();
+ } finally {
+ fetchMock.clearHistory();
+ (
+ isFeatureEnabled as jest.MockedFunction<typeof isFeatureEnabled>
+ ).mockReset();
+ }
+});
+
+test('archive modal is unchanged when the chart has no alerts or reports', async () => {
+ fetchMock.removeRoutes();
+ fetchMock.get('glob:*/api/v1/report/*', { count: 0, result: [] });
+ setupMocks();
+ try {
+ const dialog = await openFirstDeleteModal();
+ expect(
+ within(dialog).getByText(/moved to Recently Archived/i),
+ ).toBeInTheDocument();
+ expect(
+ within(dialog).queryByText('Associated alerts and reports'),
+ ).not.toBeInTheDocument();
+ } finally {
+ fetchMock.clearHistory();
+ (
+ isFeatureEnabled as jest.MockedFunction<typeof isFeatureEnabled>
+ ).mockReset();
+ }
+});
+
+test('archive modal opens unchanged and confirm still deletes when the report API 404s', async () => {
+ fetchMock.removeRoutes();
+ fetchMock.get('glob:*/api/v1/report/*', 404);
+ fetchMock.delete(`glob:*/api/v1/chart/${mockCharts[0].id}`, {});
+ setupMocks();
+ try {
+ const dialog = await openFirstDeleteModal();
+ expect(
+ within(dialog).queryByText('Associated alerts and reports'),
+ ).not.toBeInTheDocument();
+ fireEvent.click(within(dialog).getByRole('button', { name: 'Archive' }));
+ await waitFor(() =>
+ expect(
+ fetchMock.callHistory.calls(`glob:*/api/v1/chart/${mockCharts[0].id}`),
+ ).toHaveLength(1),
+ );
+ } finally {
+ fetchMock.clearHistory();
+ (
+ isFeatureEnabled as jest.MockedFunction<typeof isFeatureEnabled>
+ ).mockReset();
+ }
+});
+
+test('archive modal caps the list at ten and reports the overflow count', async () => {
+ fetchMock.removeRoutes();
+ fetchMock.get('glob:*/api/v1/report/*', {
+ count: 12,
+ result: Array.from({ length: 10 }, (_, i) => ({
+ id: i + 1,
+ name: `Blocking report ${i + 1}`,
+ type: 'Report',
+ })),
+ });
+ setupMocks();
+ try {
+ const dialog = await openFirstDeleteModal();
+ expect(within(dialog).getByText('Blocking report 10')).toBeInTheDocument();
+ expect(within(dialog).getByText('... and 2 more')).toBeInTheDocument();
+ } finally {
+ fetchMock.clearHistory();
+ (
+ isFeatureEnabled as jest.MockedFunction<typeof isFeatureEnabled>
+ ).mockReset();
+ }
+});
+
+test('archive modal refetches on every open so the list stays fresh', async () => {
+ fetchMock.removeRoutes();
+ fetchMock.get(
+ 'glob:*/api/v1/report/*',
+ {
+ count: 1,
+ result: [{ id: 1, name: 'Detach me first', type: 'Report' }],
+ },
+ { name: 'blocking-reports' },
+ );
+ setupMocks();
+ try {
+ const dialog = await openFirstDeleteModal();
+ expect(within(dialog).getByText('Detach me first')).toBeInTheDocument();
+ fireEvent.click(within(dialog).getByRole('button', { name: 'Cancel' }));
+ await waitFor(() =>
+ expect(screen.queryByRole('dialog')).not.toBeInTheDocument(),
+ );
+
+ // The user detaches the report; the next open must show the new truth.
+ fetchMock.removeRoute('blocking-reports');
+ fetchMock.get(
+ 'glob:*/api/v1/report/*',
+ { count: 0, result: [] },
+ { name: 'blocking-reports-empty' },
+ );
+ const deleteButtons = await screen.findAllByTestId('chart-row-delete');
+ fireEvent.click(deleteButtons[0]);
+ const reopened = await screen.findByRole('dialog');
+ expect(
+ within(reopened).queryByText('Detach me first'),
+ ).not.toBeInTheDocument();
+ expect(
+ within(reopened).queryByText('Associated alerts and reports'),
+ ).not.toBeInTheDocument();
+ } finally {
+ fetchMock.clearHistory();
+ (
+ isFeatureEnabled as jest.MockedFunction<typeof isFeatureEnabled>
+ ).mockReset();
+ }
+});
+
+test('archive modal opens without any report fetch when ALERT_REPORTS is off', async () => {
+ fetchMock.removeRoutes();
+ fetchMock.get(
+ 'glob:*/api/v1/report/*',
+ {
+ count: 1,
+ result: [{ id: 1, name: 'Should not appear', type: 'Report' }],
+ },
+ { name: 'reports-should-not-be-called' },
+ );
+ setupMocks();
+ (
+ isFeatureEnabled as jest.MockedFunction<typeof isFeatureEnabled>
+ ).mockImplementation((feature: string) => feature === 'SOFT_DELETE');
+ try {
+ renderChartList(adminChartUser);
+ await screen.findByTestId('chart-list-view');
+ const deleteButtons = await screen.findAllByTestId('chart-row-delete');
+ fireEvent.click(deleteButtons[0]);
+ const dialog = await screen.findByRole('dialog');
+ expect(
+ within(dialog).queryByText('Associated alerts and reports'),
+ ).not.toBeInTheDocument();
+ expect(
+ fetchMock.callHistory.calls('reports-should-not-be-called'),
+ ).toHaveLength(0);
+ } finally {
+ fetchMock.clearHistory();
+ (
+ isFeatureEnabled as jest.MockedFunction<typeof isFeatureEnabled>
+ ).mockReset();
+ }
+});
+
+test('delete confirmation keeps the type-DELETE gate when SOFT_DELETE is off', async () => {
+ fetchMock.removeRoutes();
+ fetchMock.get('glob:*/api/v1/report/*', { count: 0, result: [] });
+ setupMocks();
+ (
+ isFeatureEnabled as jest.MockedFunction<typeof isFeatureEnabled>
+ ).mockImplementation((feature: string) => feature === 'ALERT_REPORTS');
+ try {
+ renderChartList(adminChartUser);
+ await screen.findByTestId('chart-list-view');
+ const deleteButtons = await screen.findAllByTestId('chart-row-delete');
+ fireEvent.click(deleteButtons[0]);
+ const dialog = await screen.findByRole('dialog');
+ expect(within(dialog).getByText('Please confirm')).toBeInTheDocument();
+ expect(screen.getByTestId('delete-modal-input')).toBeInTheDocument();
+ } finally {
+ fetchMock.clearHistory();
+ (
+ isFeatureEnabled as jest.MockedFunction<typeof isFeatureEnabled>
+ ).mockReset();
+ }
+});
diff --git a/superset-frontend/src/pages/ChartList/index.tsx b/superset-frontend/src/pages/ChartList/index.tsx
index 09666a74dc5..9af0161e6a9 100644
--- a/superset-frontend/src/pages/ChartList/index.tsx
+++ b/superset-frontend/src/pages/ChartList/index.tsx
@@ -17,7 +17,7 @@
* under the License.
*/
import { SupersetTheme, css, styled } from '@apache-superset/core/theme';
-import { t } from '@apache-superset/core/translation';
+import { t, tn } from '@apache-superset/core/translation';
import {
isFeatureEnabled,
FeatureFlag,
@@ -26,7 +26,7 @@ import {
SupersetClient,
isMatrixifyEnabled,
} from '@superset-ui/core';
-import { useState, useMemo, useCallback } from 'react';
+import { useState, useMemo, useCallback, useRef } from 'react';
import rison from 'rison';
import { uniqBy } from 'lodash-es';
import { useSelector } from 'react-redux';
@@ -53,6 +53,8 @@ import {
ActionButton,
ConfirmStatusChange,
CertifiedBadge,
+ DeleteModal,
+ List,
Tooltip,
FaveStar,
InfoTooltip,
@@ -111,6 +113,77 @@ const FlexRowContainer = styled.div`
`;
const PAGE_SIZE = 25;
+// How many blocking alerts/reports the archive modal previews before the
+// "... and N more" overflow line (dataset-modal parity).
+const BLOCKING_REPORTS_PREVIEW_SIZE = 10;
+
+interface BlockingReport {
+ id: number;
+ name: string;
+ type: 'Alert' | 'Report';
+}
+
+interface ChartDeleteState {
+ chart: Chart;
+ blockingReports: BlockingReport[];
+ blockingReportsCount: number;
+}
+
+function ChartArchiveDescription({
+ chart,
+ blockingReports,
+ blockingReportsCount,
+ softDelete,
+}: ChartDeleteState & { softDelete: boolean }) {
+ const overflowCount = blockingReportsCount - blockingReports.length;
+ return (
+ <>
+ {softDelete ? (
+ <p>{archiveConfirmDescription(t('chart'))}</p>
+ ) : (
+ <p>
+ {t('Are you sure you want to delete')} <b>{chart.slice_name}</b>?
+ </p>
+ )}
+ {blockingReports.length > 0 && (
+ <>
+ <h4>{t('Associated alerts and reports')}</h4>
+ <p>
+ {t(
+ 'Archiving or deleting this chart will be blocked while the following alerts or reports use it. Detach or delete them first.',
+ )}
+ </p>
+ <List
+ split={false}
+ size="small"
+ dataSource={blockingReports}
+ renderItem={(report: BlockingReport) => (
+ <List.Item key={report.id} compact>
+ <List.Item.Meta
+ avatar={<span aria-hidden="true">•</span>}
+ title={report.name}
+ description={
+ report.type === 'Alert' ? t('Alert') : t('Report')
+ }
+ />
+ </List.Item>
+ )}
+ />
+ {overflowCount > 0 && (
+ <p>
+ {tn(
+ '... and %s more',
+ '... and %s more',
+ overflowCount,
+ overflowCount,
+ )}
+ </p>
+ )}
+ </>
+ )}
+ </>
+ );
+}
const PASSWORDS_NEEDED_MESSAGE = t(
'The passwords for the databases below are needed in order to ' +
'import them together with the charts. Please note that the ' +
@@ -211,6 +284,11 @@ function ChartList(props: ChartListProps) {
} = useChartEditModal(setCharts, charts);
const [importingChart, showImportModal] = useState<boolean>(false);
+ const [chartCurrentlyDeleting, setChartCurrentlyDeleting] =
+ useState<ChartDeleteState | null>(null);
+ // Monotonic token: a late pre-flight response for an earlier click must not
+ // swap the modal to a different chart (last-response-wins race).
+ const deleteModalRequestRef = useRef(0);
const [passwordFields, setPasswordFields] = useState<string[]>([]);
const [preparingExport, setPreparingExport] = useState<boolean>(false);
const [sshTunnelPasswordFields, setSSHTunnelPasswordFields] = useState<
@@ -274,6 +352,51 @@ function ChartList(props: ChartListProps) {
[addDangerToast],
);
+ const openChartDeleteModal = useCallback((chart: Chart) => {
+ deleteModalRequestRef.current += 1;
+ const requestToken = deleteModalRequestRef.current;
+ if (!isFeatureEnabled(FeatureFlag.AlertReports)) {
+ // The whole report API 404s when ALERT_REPORTS is off, while the delete
+ // guard still fires server-side. Skip the doomed request and open the
+ // unchanged modal immediately.
+ setChartCurrentlyDeleting({
+ chart,
+ blockingReports: [],
+ blockingReportsCount: 0,
+ });
+ return;
+ }
+ const queryParams = rison.encode({
+ filters: [{ col: 'chart_id', opr: 'eq', value: chart.id }],
+ columns: ['id', 'name', 'type'],
+ order_column: 'name',
+ order_direction: 'asc',
+ page_size: BLOCKING_REPORTS_PREVIEW_SIZE,
+ });
+ SupersetClient.get({ endpoint: `/api/v1/report/?q=${queryParams}` })
+ .then(({ json = {} }) => {
+ if (requestToken !== deleteModalRequestRef.current) return;
+ const blockingReports: BlockingReport[] = json.result ?? [];
+ setChartCurrentlyDeleting({
+ chart,
+ blockingReports,
+ blockingReportsCount: json.count ?? blockingReports.length,
+ });
+ })
+ .catch(() => {
+ if (requestToken !== deleteModalRequestRef.current) return;
+ // The report API can be visibility-filtered below what the delete
+ // guard sees, or fail outright. The list is advisory only, so every
+ // failure opens the unchanged modal rather than blocking the action;
+ // the confirm-time guard stays authoritative.
+ setChartCurrentlyDeleting({
+ chart,
+ blockingReports: [],
+ blockingReportsCount: 0,
+ });
+ });
+ }, []);
+
function handleBulkChartDelete(chartsToDelete: Chart[]) {
SupersetClient.delete({
endpoint: `/api/v1/chart/?q=${rison.encode(
@@ -528,13 +651,6 @@ function ChartList(props: ChartListProps) {
{
Cell: ({ row: { original } }: CellProps<Chart>) => {
const allowEdit = isUserEditorOrAdmin(user, original.editors);
- const handleDelete = () =>
- handleChartDelete(
- original,
- addSuccessToast,
- addDangerToast,
- refreshData,
- );
const openEditModal = () => openChartEditModal(original);
const handleExport = () => handleBulkChartExport([original]);
if (!canEdit && !canDelete && !canExport) {
@@ -573,43 +689,21 @@ function ChartList(props: ChartListProps) {
/>
)}
{canDelete && (
- <ConfirmStatusChange
- recoverable={softDelete}
- title={
- softDelete
- ? t('Archive %(name)s?', { name: original.slice_name })
- : t('Please confirm')
- }
- description={
- softDelete ? (
- archiveConfirmDescription(t('chart'))
- ) : (
- <>
- {t('Are you sure you want to delete')}{' '}
- <b>{original.slice_name}</b>?
- </>
- )
+ <ActionButton
+ label={deleteActionLabel()}
+ tooltip={
+ allowEdit
+ ? deleteActionLabel()
+ : t(
+ 'You must be a chart editor in order to delete. Please reach out to a chart editor to request modifications or edit access.',
+ )
}
- onConfirm={handleDelete}
- >
- {confirmDelete => (
- <ActionButton
- label={deleteActionLabel()}
- tooltip={
- allowEdit
- ? deleteActionLabel()
- : t(
- 'You must be a chart editor in order to delete. Please reach out to a chart editor to request modifications or edit access.',
- )
- }
- placement="bottom"
- icon={<Icons.DeleteOutlined iconSize="l" />}
- dataTest="chart-row-delete"
- disabled={!allowEdit}
- onClick={confirmDelete}
- />
- )}
- </ConfirmStatusChange>
+ placement="bottom"
+ icon={<Icons.DeleteOutlined iconSize="l" />}
+ dataTest="chart-row-delete"
+ disabled={!allowEdit}
+ onClick={() => openChartDeleteModal(original)}
+ />
)}
</Actions>
);
@@ -634,11 +728,9 @@ function ChartList(props: ChartListProps) {
canExport,
saveFavoriteStatus,
favoriteStatus,
- refreshData,
- addSuccessToast,
- addDangerToast,
handleBulkChartExport,
openChartEditModal,
+ openChartDeleteModal,
],
);
@@ -933,6 +1025,36 @@ function ChartList(props: ChartListProps) {
slice={sliceCurrentlyEditing}
/>
)}
+ {chartCurrentlyDeleting && (
+ <DeleteModal
+ recoverable={softDelete}
+ title={
+ softDelete
+ ? t('Archive %(name)s?', {
+ name: chartCurrentlyDeleting.chart.slice_name,
+ })
+ : t('Please confirm')
+ }
+ name={chartCurrentlyDeleting.chart.slice_name}
+ open
+ description={
+ <ChartArchiveDescription
+ {...chartCurrentlyDeleting}
+ softDelete={softDelete}
+ />
+ }
+ onConfirm={() => {
+ handleChartDelete(
+ chartCurrentlyDeleting.chart,
+ addSuccessToast,
+ addDangerToast,
+ refreshData,
+ );
+ setChartCurrentlyDeleting(null);
+ }}
+ onHide={() => setChartCurrentlyDeleting(null)}
+ />
+ )}
<ConfirmStatusChange
recoverable={softDelete}
title={softDelete ? t('Archive selected charts?') : t('Please confirm')}
diff --git a/superset-frontend/src/views/CRUD/utils.test.tsx b/superset-frontend/src/views/CRUD/utils.test.tsx
index 4eee1c5b53c..5ba30f22911 100644
--- a/superset-frontend/src/views/CRUD/utils.test.tsx
+++ b/superset-frontend/src/views/CRUD/utils.test.tsx
@@ -17,8 +17,12 @@
* under the License.
*/
import rison from 'rison';
+import { waitFor } from '@testing-library/react';
+import { SupersetClient } from '@superset-ui/core';
+import Chart from 'src/types/Chart';
import {
checkUploadExtensions,
+ handleChartDelete,
getAlreadyExists,
getEncryptedExtraFieldsNeeded,
getFilterValues,
@@ -743,3 +747,27 @@ test('getFilterValues', () => {
);
});
});
+
+test('handleChartDelete surfaces the blocking alert/report names from a 422', async () => {
+ const guardMessage =
+ 'There are associated alerts or reports: TC-081 rerun report';
+ const deleteSpy = jest
+ .spyOn(SupersetClient, 'delete')
+ .mockRejectedValue(
+ new Response(JSON.stringify({ message: guardMessage }), { status: 422 }),
+ );
+ const addDangerToast = jest.fn();
+ try {
+ handleChartDelete(
+ { id: 1, slice_name: 'blocked chart' } as Chart,
+ jest.fn(),
+ addDangerToast,
+ jest.fn(),
+ );
+ await waitFor(() => expect(addDangerToast).toHaveBeenCalledTimes(1));
+ expect(addDangerToast.mock.calls[0][0]).toContain('TC-081 rerun report');
+ expect(addDangerToast.mock.calls[0][0]).toContain('blocked chart');
+ } finally {
+ deleteSpy.mockRestore();
+ }
+});
diff --git a/superset-frontend/src/views/CRUD/utils.tsx b/superset-frontend/src/views/CRUD/utils.tsx
index f429572e137..ac328ec1e7d 100644
--- a/superset-frontend/src/views/CRUD/utils.tsx
+++ b/superset-frontend/src/views/CRUD/utils.tsx
@@ -379,9 +379,9 @@ export function handleChartDelete(
else refreshData();
addSuccessToast(deletedToast(sliceName));
},
- () => {
- addDangerToast(deleteFailedToast(sliceName));
- },
+ createErrorHandler(errMsg =>
+ addDangerToast(deleteFailedToast(sliceName, errMsg)),
+ ),
);
}
diff --git a/superset/commands/chart/delete.py b/superset/commands/chart/delete.py
index fe628e1ac65..614b979d66a 100644
--- a/superset/commands/chart/delete.py
+++ b/superset/commands/chart/delete.py
@@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
import logging
+from collections import defaultdict
from functools import partial
from typing import Optional
@@ -32,11 +33,62 @@ from superset.daos.chart import ChartDAO
from superset.daos.report import ReportScheduleDAO
from superset.exceptions import SupersetSecurityException
from superset.models.slice import Slice
+from superset.reports.models import ReportSchedule
from superset.utils.decorators import on_error, transaction
logger = logging.getLogger(__name__)
+def build_blocked_by_reports_message(
+ charts: list[Slice],
+ reports: list[ReportSchedule],
+ single_target: bool,
+) -> str:
+ """Build the user-facing message naming the alerts/reports that block deletion.
+
+ Groups the blocking reports per chart, sorted by chart name (chart id as
+ the tie-breaker) and then report name so the message is deterministic
+ across database backends, and appends the remedy sentence. When the
+ command targets a single chart id the group prefix is dropped — in the
+ single-delete endpoint the surrounding toast already names the chart, and
+ in a one-chart bulk selection the user's own selection provides the
+ context. A multi-id command keeps the prefix on every group because the
+ bulk toast is generic, so the prefix is the only chart identification the
+ user gets.
+ """
+ sentences: list[str] = []
+ if single_target:
+ names = sorted(report.name for report in reports)
+ sentences.append(
+ str(
+ _(
+ "This chart is used by alerts or reports: %(names)s.",
+ names=", ".join(names),
+ )
+ )
+ )
+ else:
+ report_names_by_chart_id: dict[int, list[str]] = defaultdict(list)
+ for report in reports:
+ report_names_by_chart_id[report.chart_id].append(report.name)
+ charts_by_id = {chart.id: chart for chart in charts}
+ for chart_id, names in sorted(
+ report_names_by_chart_id.items(),
+ key=lambda item: (charts_by_id[item[0]].slice_name or "", item[0]),
+ ):
+ sentences.append(
+ str(
+ _(
+ 'Chart "%(chart)s" is used by alerts or reports: %(names)s.',
+ chart=charts_by_id[chart_id].slice_name or str(chart_id),
+ names=", ".join(sorted(names)),
+ )
+ )
+ )
+ sentences.append(str(_("Detach or delete them first.")))
+ return " ".join(sentences)
+
+
class DeleteChartCommand(BaseCommand):
def __init__(self, model_ids: list[int]):
self._model_ids = model_ids
@@ -55,11 +107,11 @@ class DeleteChartCommand(BaseCommand):
raise ChartNotFoundError()
# Check there are no associated ReportSchedules
if reports := ReportScheduleDAO.find_by_chart_ids(self._model_ids):
- report_names = [report.name for report in reports]
raise ChartDeleteFailedReportsExistError(
- _(
- "There are associated alerts or reports: %(report_names)s",
- report_names=",".join(report_names),
+ build_blocked_by_reports_message(
+ self._models,
+ reports,
+ single_target=len(self._model_ids) == 1,
)
)
# Check editorship
diff --git a/tests/integration_tests/charts/api_tests.py b/tests/integration_tests/charts/api_tests.py
index c32272b0e63..fd6be78e054 100644
--- a/tests/integration_tests/charts/api_tests.py
+++ b/tests/integration_tests/charts/api_tests.py
@@ -393,7 +393,10 @@ class TestChartApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCase):
response = json.loads(rv.data.decode("utf-8"))
assert rv.status_code == 422
expected_response = {
- "message": "There are associated alerts or reports: report_with_chart"
+ "message": (
+ "This chart is used by alerts or reports: report_with_chart. "
+ "Detach or delete them first."
+ )
}
assert response == expected_response
@@ -429,7 +432,10 @@ class TestChartApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCase):
response = json.loads(rv.data.decode("utf-8"))
assert rv.status_code == 422
expected_response = {
- "message": "There are associated alerts or reports: report_with_chart"
+ "message": (
+ 'Chart "chart_report" is used by alerts or reports: '
+ "report_with_chart. Detach or delete them first."
+ )
}
assert response == expected_response
diff --git a/tests/integration_tests/charts/soft_delete_tests.py b/tests/integration_tests/charts/soft_delete_tests.py
index fc01617eaa5..039fd6513ea 100644
--- a/tests/integration_tests/charts/soft_delete_tests.py
+++ b/tests/integration_tests/charts/soft_delete_tests.py
@@ -317,10 +317,7 @@ class TestChartSoftDelete(InsertChartMixin, SupersetTestCase):
rv = self.client.delete(f"/api/v1/chart/{chart_id}")
assert rv.status_code == 422
body = json.loads(rv.data)
- assert "associated alerts or reports" in body.get("message", "").lower() or (
- "associated" in body.get("message", "").lower()
- and "report" in body.get("message", "").lower()
- )
+ assert "is used by alerts or reports" in body.get("message", "")
assert "blocking_report_for_chart_delete" in body.get("message", "")
# Confirm the chart was NOT soft-deleted (deleted_at remains NULL).
diff --git a/tests/unit_tests/commands/chart/delete_message_test.py b/tests/unit_tests/commands/chart/delete_message_test.py
new file mode 100644
index 00000000000..1b7aa149849
--- /dev/null
+++ b/tests/unit_tests/commands/chart/delete_message_test.py
@@ -0,0 +1,63 @@
+# 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.
+from superset.commands.chart.delete import build_blocked_by_reports_message
+from superset.models.slice import Slice
+from superset.reports.models import ReportSchedule
+
+
+def _chart(chart_id: int, name: str) -> Slice:
+ return Slice(id=chart_id, slice_name=name)
+
+
+def _report(chart_id: int, name: str) -> ReportSchedule:
+ return ReportSchedule(chart_id=chart_id, name=name)
+
+
+def test_single_target_drops_chart_prefix_and_sorts_names() -> None:
+ charts = [_chart(1, "Sales")]
+ reports = [_report(1, "Weekly report"), _report(1, "Audit alert")]
+ assert build_blocked_by_reports_message(charts, reports, single_target=True) == (
+ "This chart is used by alerts or reports: Audit alert, Weekly report. "
+ "Detach or delete them first."
+ )
+
+
+def test_multi_target_groups_by_chart_name_deterministically() -> None:
+ # Charts and reports deliberately supplied out of order.
+ charts = [_chart(2, "Zebra"), _chart(1, "Alpha")]
+ reports = [
+ _report(2, "Z report"),
+ _report(1, "B alert"),
+ _report(1, "A report"),
+ ]
+ assert build_blocked_by_reports_message(charts, reports, single_target=False) == (
+ 'Chart "Alpha" is used by alerts or reports: A report, B alert. '
+ 'Chart "Zebra" is used by alerts or reports: Z report. '
+ "Detach or delete them first."
+ )
+
+
+def test_multi_target_with_one_blocked_chart_keeps_prefix() -> None:
+ # A bulk command with several ids keeps the chart prefix even when only
+ # one selected chart is blocked — the bulk toast is generic, so the
+ # prefix is the only chart identification the user gets.
+ charts = [_chart(1, "Costs"), _chart(2, "Unblocked")]
+ reports = [_report(1, "Audit report")]
+ assert build_blocked_by_reports_message(charts, reports, single_target=False) == (
+ 'Chart "Costs" is used by alerts or reports: Audit report. '
+ "Detach or delete them first."
+ )