diff --git a/docs/docs/using-superset/recently-archived.mdx b/docs/docs/using-superset/recently-archived.mdx index ede931d9343b..170b6187aaee 100644 --- a/docs/docs/using-superset/recently-archived.mdx +++ b/docs/docs/using-superset/recently-archived.mdx @@ -103,12 +103,15 @@ filter cleanup; the response warns that the dashboard's filters need review. Archived charts and dashboards still block deletion of semantic views and layers they depend on. The refusal includes the number of archived dependents. Use **Recently Archived** to restore and repoint them, or permanently delete them, -before deleting the semantic source. If you cannot access those assets, ask an -admin for help. +before deleting the semantic layer or view. If you cannot access those assets, +ask an admin for help. When deleting a semantic layer or view, a dependency refusal stays in the -confirmation dialog with the total and archived counts and a link to Recently -Archived. The typed confirmation is preserved so you can retry after removing +confirmation dialog with the total and archived counts, the names of up to 20 +blocking assets you can see, and a link to Recently Archived. Archived blockers +are named only to users who could restore them from Recently Archived (admins +and the asset's editors with write permission on its resource type); the dialog counts the others and suggests asking an +admin. The typed confirmation is preserved so you can retry after removing or repointing the dependents. Dependencies are checked when you confirm deletion, not before you type the confirmation. diff --git a/docs/static/resources/openapi.json b/docs/static/resources/openapi.json index 028af7aa04bb..07af26d4a7c7 100644 --- a/docs/static/resources/openapi.json +++ b/docs/static/resources/openapi.json @@ -35269,12 +35269,15 @@ "schema": { "properties": { "archived_count": { - "description": "Archived dependents are included in inaccessible_count; these counts overlap and must not be added together.", + "description": "Archived dependents are named only to users who could restore them from Recently Archived; the others are included in inaccessible_count. The counts overlap and must not be added together.", "type": "integer" }, "dependents": { "items": { "properties": { + "archived": { + "type": "boolean" + }, "id": { "type": "integer" }, @@ -35294,7 +35297,8 @@ "required": [ "type", "id", - "name" + "name", + "archived" ], "type": "object" }, @@ -35659,12 +35663,15 @@ "schema": { "properties": { "archived_count": { - "description": "Archived dependents are included in inaccessible_count; these counts overlap and must not be added together.", + "description": "Archived dependents are named only to users who could restore them from Recently Archived; the others are included in inaccessible_count. The counts overlap and must not be added together.", "type": "integer" }, "dependents": { "items": { "properties": { + "archived": { + "type": "boolean" + }, "id": { "type": "integer" }, @@ -35684,7 +35691,8 @@ "required": [ "type", "id", - "name" + "name", + "archived" ], "type": "object" }, @@ -35827,12 +35835,15 @@ "schema": { "properties": { "archived_count": { - "description": "Archived dependents are included in inaccessible_count; these counts overlap and must not be added together.", + "description": "Archived dependents are named only to users who could restore them from Recently Archived; the others are included in inaccessible_count. The counts overlap and must not be added together.", "type": "integer" }, "dependents": { "items": { "properties": { + "archived": { + "type": "boolean" + }, "id": { "type": "integer" }, @@ -35852,7 +35863,8 @@ "required": [ "type", "id", - "name" + "name", + "archived" ], "type": "object" }, diff --git a/superset-frontend/src/features/semanticLayers/SemanticDeleteBlockers.test.tsx b/superset-frontend/src/features/semanticLayers/SemanticDeleteBlockers.test.tsx new file mode 100644 index 000000000000..c16b4e8d89b1 --- /dev/null +++ b/superset-frontend/src/features/semanticLayers/SemanticDeleteBlockers.test.tsx @@ -0,0 +1,51 @@ +/** + * 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. + */ +import { render, screen } from 'spec/helpers/testing-library'; +import SemanticDeleteBlockers, { + SemanticDeleteError, +} from './SemanticDeleteBlockers'; + +test('markup in the refusal is shown as text, not rendered as HTML', () => { + const error = { + error: 'Blocked', + dependents: [ + { type: 'chart', id: 7, name: 'Revenue', archived: false }, + { + type: 'dashboard', + id: '/evil.example' as unknown as number, + name: 'Odd board', + archived: false, + }, + ], + } satisfies SemanticDeleteError; + const { container } = render(); + expect( + screen.getByText('Blocked'), + ).toBeInTheDocument(); + expect(screen.getByRole('link', { name: 'Revenue' })).toHaveAttribute( + 'href', + '/explore/?slice_id=7', + ); + expect(container.querySelector('img, b')).toBeNull(); + // An id that is not a whole number never becomes a link. + expect(screen.getByText(/Odd board/)).toBeInTheDocument(); + expect( + screen.queryByRole('link', { name: 'Odd board' }), + ).not.toBeInTheDocument(); +}); diff --git a/superset-frontend/src/features/semanticLayers/SemanticDeleteBlockers.tsx b/superset-frontend/src/features/semanticLayers/SemanticDeleteBlockers.tsx new file mode 100644 index 000000000000..d51960e6b5ea --- /dev/null +++ b/superset-frontend/src/features/semanticLayers/SemanticDeleteBlockers.tsx @@ -0,0 +1,119 @@ +/** + * 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. + */ +import { t, tn } from '@apache-superset/core/translation'; +import { Alert } from '@apache-superset/core/components'; +import { ensureAppRoot } from 'src/utils/navigationUtils'; + +interface Dependent { + type: string; + id: number; + name: string; + archived?: boolean; +} + +export interface SemanticDeleteError { + error: string; + total?: number; + archived_count?: number; + inaccessible_count?: number; + dependents?: Dependent[]; +} + +const typeLabel = (type: string): string => { + const labels: Record = { + chart: t('chart'), + dashboard: t('dashboard'), + alert: t('alert'), + report: t('report'), + }; + return labels[type] ?? type; +}; + +const LIVE_LINKS: Record string> = { + chart: id => `/explore/?slice_id=${id}`, + dashboard: id => `/dashboard/${id}/`, +}; + +/** The refusal shown inside a semantic layer or view delete dialog. */ +export default function SemanticDeleteBlockers({ + error, +}: { + error: SemanticDeleteError; +}) { + const dependents = error.dependents ?? []; + const hidden = error.inaccessible_count ?? 0; + const unlisted = (error.total ?? 0) - hidden - dependents.length; + const counted = + typeof error.total === 'number' && typeof error.archived_count === 'number'; + const summary = counted + ? t( + 'Deletion is blocked by %s dependent assets (%s archived). Remove or repoint them before deleting.', + error.total, + error.archived_count, + ) + : error.error; + // Archived assets have no page to open; Recently Archived lists them. + // The id comes from the error body, so only a whole number becomes a link. + const href = (item: Dependent): string | undefined => { + const id = Number(item.id); + return item.archived || !Number.isSafeInteger(id) + ? undefined + : LIVE_LINKS[item.type]?.(id); + }; + return ( + + {dependents.length > 0 && ( +
    + {dependents.map(item => { + const link = href(item); + const name = String(item.name); + return ( +
  • + {link ? {name} : name} ( + {item.archived + ? t('archived %s', typeLabel(item.type)) + : typeLabel(item.type)} + ) +
    • + ); + })} +
+ )} + {unlisted > 0 &&

{t('and %s more', unlisted)}

} + {hidden > 0 && ( +

+ {tn( + "%s dependent asset is one you can't see. Ask an admin for help.", + "%s dependent assets are ones you can't see. Ask an admin for help.", + hidden, + hidden, + )} +

+ )} + {t('Recently Archived')} + + } + /> + ); +} diff --git a/superset-frontend/src/pages/DatabaseList/index.tsx b/superset-frontend/src/pages/DatabaseList/index.tsx index 921ed517161b..9f8ba2ca70ee 100644 --- a/superset-frontend/src/pages/DatabaseList/index.tsx +++ b/superset-frontend/src/pages/DatabaseList/index.tsx @@ -16,7 +16,6 @@ * specific language governing permissions and limitations * under the License. */ -import { Alert } from '@apache-superset/core/components'; import { t, tn } from '@apache-superset/core/translation'; import { getExtensionsRegistry, @@ -75,6 +74,9 @@ import { DatabaseObject } from 'src/features/databases/types'; import { QueryObjectColumns } from 'src/views/CRUD/types'; import { WIDER_DROPDOWN_WIDTH } from 'src/components/ListView/utils'; import { ModalTitleWithIcon } from 'src/components/ModalTitleWithIcon'; +import SemanticDeleteBlockers, { + SemanticDeleteError, +} from 'src/features/semanticLayers/SemanticDeleteBlockers'; import type User from 'src/types/User'; import { databaseLabel, @@ -433,11 +435,8 @@ function DatabaseList({ const [slCurrentlyEditing, setSlCurrentlyEditing] = useState( null, ); - const [slDeleteError, setSLDeleteError] = useState<{ - error: string; - total?: number; - archived_count?: number; - } | null>(null); + const [slDeleteError, setSLDeleteError] = + useState(null); const [slDeleting, setSLDeleting] = useState(false); const slDeleteRequest = useRef(0); const [slDeletePreview, setSlDeletePreview] = @@ -1250,25 +1249,7 @@ function DatabaseList({

{slDeleteError && ( - - {typeof slDeleteError.total === 'number' && - typeof slDeleteError.archived_count === 'number' - ? t( - 'Deletion is blocked by %s dependent assets (%s archived). Remove or repoint them before deleting.', - slDeleteError.total, - slDeleteError.archived_count, - ) - : slDeleteError.error}{' '} - - {t('Recently Archived')} - - - } - /> + )} } @@ -1308,7 +1289,7 @@ function DatabaseList({ databaseCurrentlyDeleting.datasets.count, )}{' '} {t( - 'Delete or move active datasets. To remove archived datasets, open Recently archived and select Delete permanently.', + 'Delete or move active datasets. To remove archived datasets, open Recently Archived and select Delete permanently.', )}

) : ( diff --git a/superset-frontend/src/pages/DatasetList/DatasetList.behavior.test.tsx b/superset-frontend/src/pages/DatasetList/DatasetList.behavior.test.tsx index 9634be0b6e76..dbce49bee833 100644 --- a/superset-frontend/src/pages/DatasetList/DatasetList.behavior.test.tsx +++ b/superset-frontend/src/pages/DatasetList/DatasetList.behavior.test.tsx @@ -121,6 +121,57 @@ test('single semantic view delete displays the counted dependency conflict', asy ).toHaveValue(''); }); +test('single semantic view delete names its blockers, including archived ones', async () => { + const view = { + ...mockDatasets[0], + id: 99, + table_name: 'orders_semantic', + kind: 'semantic_view', + }; + mockDatasetListEndpoints({ result: [view], count: 1 }); + fetchMock.delete('glob:*/api/v1/semantic_view/99', { + status: 409, + body: { + message: 'blocked', + total: 6, + archived_count: 1, + inaccessible_count: 2, + dependents: [ + { type: 'chart', id: 7, name: 'Revenue chart', archived: false }, + { type: 'dashboard', id: 8, name: 'Old board', archived: true }, + { type: 'dashboard', id: 9, name: 'Live board', archived: false }, + ], + }, + }); + renderDatasetList(mockAdminUser, { + addDangerToast: jest.fn(), + addSuccessToast: jest.fn(), + }); + await screen.findByText(view.table_name); + await userEvent.click(screen.getByTestId('dataset-row-delete')); + const modal = await screen.findByRole('dialog'); + await userEvent.type( + within(modal).getByTestId('delete-modal-input'), + 'DELETE', + ); + await userEvent.click(within(modal).getByRole('button', { name: 'Delete' })); + const alert = await within(modal).findByRole('alert'); + expect( + within(alert).getByRole('link', { name: 'Revenue chart' }), + ).toHaveAttribute('href', '/explore/?slice_id=7'); + expect(alert).toHaveTextContent('Old board (archived dashboard)'); + expect( + within(alert).getByRole('link', { name: 'Live board' }), + ).toHaveAttribute('href', '/dashboard/9/'); + expect( + within(alert).queryByRole('link', { name: 'Old board' }), + ).not.toBeInTheDocument(); + expect(alert).toHaveTextContent('and 1 more'); + expect(alert).toHaveTextContent( + "2 dependent assets are ones you can't see. Ask an admin for help.", + ); +}); + test('mixed bulk delete names refused semantic views and their conflict reason', async () => { const dataset = mockDatasets[0]; const semanticView = { diff --git a/superset-frontend/src/pages/DatasetList/index.tsx b/superset-frontend/src/pages/DatasetList/index.tsx index ffc15ca1b43d..ed1c1ef766e7 100644 --- a/superset-frontend/src/pages/DatasetList/index.tsx +++ b/superset-frontend/src/pages/DatasetList/index.tsx @@ -16,7 +16,6 @@ * specific language governing permissions and limitations * under the License. */ -import { Alert } from '@apache-superset/core/components'; import { t, tn } from '@apache-superset/core/translation'; import { getExtensionsRegistry, @@ -115,6 +114,10 @@ import { import { useSelector } from 'react-redux'; import { QueryObjectColumns } from 'src/views/CRUD/types'; import { WIDER_DROPDOWN_WIDTH } from 'src/components/ListView/utils'; +import { ModalTitleWithIcon } from 'src/components/ModalTitleWithIcon'; +import SemanticDeleteBlockers, { + SemanticDeleteError, +} from 'src/features/semanticLayers/SemanticDeleteBlockers'; import type { BootstrapData, UserWithPermissionsAndRoles, @@ -635,11 +638,8 @@ const DatasetList: FunctionComponent = ({ null, ); - const [svDeleteError, setSVDeleteError] = useState<{ - error: string; - total?: number; - archived_count?: number; - } | null>(null); + const [svDeleteError, setSVDeleteError] = + useState(null); const [svDeleting, setSVDeleting] = useState(false); const svDeleteRequest = useRef(0); const [svCurrentlyDeleting, setSvCurrentlyDeleting] = @@ -1649,31 +1649,12 @@ const DatasetList: FunctionComponent = ({ description={ <>

- {t( - 'Are you sure you want to delete %s? This cannot be undone.', - svCurrentlyDeleting.table_name, - )} + {t('Are you sure you want to delete')}{' '} + {svCurrentlyDeleting.table_name}?{' '} + {t('This cannot be undone.')}

{svDeleteError && ( - - {typeof svDeleteError.total === 'number' && - typeof svDeleteError.archived_count === 'number' - ? t( - 'Deletion is blocked by %s dependent assets (%s archived). Remove or repoint them before deleting.', - svDeleteError.total, - svDeleteError.archived_count, - ) - : svDeleteError.error}{' '} - - {t('Recently Archived')} - - - } - /> + )} } @@ -1683,7 +1664,12 @@ const DatasetList: FunctionComponent = ({ setSvCurrentlyDeleting(null); }} open - title={t('Delete Semantic View?')} + title={ + } + title={t('Delete Semantic View?')} + /> + } /> )} {datasetCurrentlyEditing && ( diff --git a/superset/commands/semantic_layer/delete.py b/superset/commands/semantic_layer/delete.py index 17f150d52365..74a866ff3bf3 100644 --- a/superset/commands/semantic_layer/delete.py +++ b/superset/commands/semantic_layer/delete.py @@ -46,7 +46,10 @@ from superset.reports.filters import ReportScheduleFilter from superset.reports.models import ReportSchedule from superset.semantic_layers.models import SemanticLayer, SemanticView +from superset.subjects.models import chart_editors, dashboard_editors +from superset.subjects.utils import get_user_subject_ids_subquery from superset.utils import json +from superset.utils.core import get_user_id from superset.utils.decorators import on_error, transaction logger = logging.getLogger(__name__) @@ -117,6 +120,25 @@ def _dashboard_target_ids(view_ids: sa.Select) -> set[int]: return matched_ids +def _archived_audience_ids(table: sa.Table, editors: sa.Table, fk: str) -> sa.Select: + """Archived ids the user could restore from Recently Archived. + + Mirrors ``BaseDeletedStateFilter``: admins see every archived row, others + only rows they edit. Core tables keep the soft-delete listener out of it. + """ + # A guest token never restores archived assets, whatever its role. + if security_manager.is_guest_user(): + return sa.select(table.c.id).where(sa.false()) + if security_manager.is_admin(): + return sa.select(table.c.id) + user_id: int | None = get_user_id() + if not user_id: + return sa.select(table.c.id).where(sa.false()) + return sa.select(editors.c[fk]).where( + editors.c.subject_id.in_(get_user_subject_ids_subquery(user_id)) + ) + + def _dependent_assets( view_ids: sa.Select, ) -> tuple[int, list[dict[str, str | int]], int, int]: @@ -209,14 +231,30 @@ def _dependent_assets( if security_manager.can_access("can_read", "ReportSchedule") else sa.select(ReportSchedule.id).where(sa.false()) ) + archived_chart_ids: sa.Select = ( + _archived_audience_ids(Slice.__table__, chart_editors, "chart_id") + if security_manager.can_access("can_write", "Chart") + else sa.select(Slice.__table__.c.id).where(sa.false()) + ) + archived_dashboard_ids: sa.Select = ( + _archived_audience_ids(Dashboard.__table__, dashboard_editors, "dashboard_id") + if security_manager.can_access("can_write", "Dashboard") + else sa.select(Dashboard.__table__.c.id).where(sa.false()) + ) visibility: sa.ColumnElement[bool] = sa.or_( sa.and_( dependents.c.type == "chart", - dependents.c.id.in_(visible_chart_ids), + sa.or_( + dependents.c.id.in_(visible_chart_ids), + dependents.c.archived & dependents.c.id.in_(archived_chart_ids), + ), ), sa.and_( dependents.c.type == "dashboard", - dependents.c.id.in_(visible_dashboard_ids), + sa.or_( + dependents.c.id.in_(visible_dashboard_ids), + dependents.c.archived & dependents.c.id.in_(archived_dashboard_ids), + ), ), sa.and_( dependents.c.type.notin_(("chart", "dashboard")), @@ -239,10 +277,10 @@ def _dependent_assets( return 0, [], 0, 0 visible_total: int = int(counts[1] or 0) visible: sa.Subquery = sa.select(dependents).where(visibility).subquery() - rows: list[tuple[str, int, str]] = cast( - list[tuple[str, int, str]], + rows: list[tuple[str, int, str, bool]] = cast( + list[tuple[str, int, str, bool]], db.session.execute( - sa.select(visible.c.type, visible.c.id, visible.c.name) + sa.select(visible.c.type, visible.c.id, visible.c.name, visible.c.archived) .order_by( sa.case( (visible.c.type == "chart", 0), @@ -258,8 +296,8 @@ def _dependent_assets( return ( total, [ - {"type": asset_type, "id": asset_id, "name": name} - for asset_type, asset_id, name in rows + {"type": asset_type, "id": asset_id, "name": name, "archived": archived} + for asset_type, asset_id, name, archived in rows ], total - visible_total, int(counts[2] or 0), diff --git a/superset/semantic_layers/api.py b/superset/semantic_layers/api.py index d37b3c891dbf..1134e18763b5 100644 --- a/superset/semantic_layers/api.py +++ b/superset/semantic_layers/api.py @@ -592,7 +592,7 @@ def delete(self, pk: int) -> Response: maxItems: 20 items: type: object - required: [type, id, name] + required: [type, id, name, archived] properties: type: type: string @@ -601,13 +601,17 @@ def delete(self, pk: int) -> Response: type: integer name: type: string + archived: + type: boolean inaccessible_count: type: integer archived_count: type: integer description: >- - Archived dependents are included in inaccessible_count; - these counts overlap and must not be added together. + Archived dependents are named only to users who could + restore them from Recently Archived; the others are + included in inaccessible_count. The counts overlap and + must not be added together. 422: $ref: '#/components/responses/422' """ @@ -696,7 +700,7 @@ def bulk_delete(self, **kwargs: Any) -> Response: maxItems: 20 items: type: object - required: [type, id, name] + required: [type, id, name, archived] properties: type: type: string @@ -705,13 +709,17 @@ def bulk_delete(self, **kwargs: Any) -> Response: type: integer name: type: string + archived: + type: boolean inaccessible_count: type: integer archived_count: type: integer description: >- - Archived dependents are included in inaccessible_count; - these counts overlap and must not be added together. + Archived dependents are named only to users who could + restore them from Recently Archived; the others are + included in inaccessible_count. The counts overlap and + must not be added together. 422: $ref: '#/components/responses/422' """ @@ -1235,7 +1243,7 @@ def delete(self, uuid: str) -> FlaskResponse: maxItems: 20 items: type: object - required: [type, id, name] + required: [type, id, name, archived] properties: type: type: string @@ -1244,13 +1252,17 @@ def delete(self, uuid: str) -> FlaskResponse: type: integer name: type: string + archived: + type: boolean inaccessible_count: type: integer archived_count: type: integer description: >- - Archived dependents are included in inaccessible_count; - these counts overlap and must not be added together. + Archived dependents are named only to users who could + restore them from Recently Archived; the others are + included in inaccessible_count. The counts overlap and + must not be added together. 422: $ref: '#/components/responses/422' """ diff --git a/superset/translations/messages.pot b/superset/translations/messages.pot index f4555d86d762..d3ec6e96e4c5 100644 --- a/superset/translations/messages.pot +++ b/superset/translations/messages.pot @@ -427,6 +427,12 @@ msgid_plural "%s days ago" msgstr[0] "" msgstr[1] "" +#, python-format +msgid "%s dependent asset is one you can't see. Ask an admin for help." +msgid_plural "%s dependent assets are ones you can't see. Ask an admin for help." +msgstr[0] "" +msgstr[1] "" + #, python-format msgid "%s field" msgid_plural "%s fields" @@ -2364,10 +2370,6 @@ msgstr "" msgid "Are you sure you want to delete %s?" msgstr "" -#, python-format -msgid "Are you sure you want to delete %s? This cannot be undone." -msgstr "" - #, python-format msgid "Are you sure you want to delete the selected %s?" msgstr "" @@ -5244,7 +5246,7 @@ msgstr "" msgid "Delete item" msgstr "" -msgid "Delete or move active datasets. To remove archived datasets, open Recently archived and select Delete permanently." +msgid "Delete or move active datasets. To remove archived datasets, open Recently Archived and select Delete permanently." msgstr "" msgid "Delete permanently" @@ -15302,6 +15304,9 @@ msgstr "" msgid "This can be either an IP address (e.g. 127.0.0.1) or a domain name (e.g. mydatabase.com)." msgstr "" +msgid "This cannot be undone." +msgstr "" + msgid "This chart applies cross-filters to charts whose datasets contain columns with the same name." msgstr "" @@ -17842,6 +17847,9 @@ msgstr "" msgid "aggregate" msgstr "" +msgid "alert" +msgstr "" + msgid "alert condition" msgstr "" @@ -17857,6 +17865,10 @@ msgstr "" msgid "ancestor" msgstr "" +#, python-format +msgid "and %s more" +msgstr "" + msgid "annotation" msgstr "" @@ -17866,6 +17878,10 @@ msgstr "" msgid "apple" msgstr "" +#, python-format +msgid "archived %s" +msgstr "" + msgid "asfreq" msgstr "" diff --git a/tests/unit_tests/commands/semantic_layer/delete_test.py b/tests/unit_tests/commands/semantic_layer/delete_test.py index 8c094a62f98d..f9071ca602e4 100644 --- a/tests/unit_tests/commands/semantic_layer/delete_test.py +++ b/tests/unit_tests/commands/semantic_layer/delete_test.py @@ -264,7 +264,7 @@ def test_bulk_delete_semantic_views_refuses_all_on_one_dependency( @pytest.mark.parametrize("orm_execution", [False, True]) -def test_semantic_delete_counts_archived_dependents_without_listing_them( +def test_semantic_delete_counts_and_names_archived_dependents( session: Session, mocker: MockerFixture, orm_execution: bool ) -> None: """Charts, dashboard membership and active schedules form the dependency set.""" @@ -363,9 +363,13 @@ def test_semantic_delete_counts_archived_dependents_without_listing_them( assert total == 6 assert archived_count == 1 - assert inaccessible_count == 1 + assert inaccessible_count == 0 + assert {item["name"] for item in dependents if item["archived"]} == { + "Deleted chart" + } assert {item["name"] for item in dependents} == { "Live chart", + "Deleted chart", "Live dashboard", "Live alert", "Live report", @@ -387,11 +391,21 @@ def test_semantic_delete_counts_archived_dependents_without_listing_them( assert capped_total == 6 assert len(capped_dependents) == 2 assert [item["type"] for item in capped_dependents] == ["chart", "chart"] - assert capped_inaccessible_count == 1 + assert capped_inaccessible_count == 0 mocker.patch( "superset.commands.semantic_layer.delete.ChartFilter.apply", return_value=session.query(Slice.id).filter(sa.false()), ) + # A user who cannot see the charts is not an admin or their editor either. + mocker.patch("superset.security_manager.is_admin", return_value=False) + for access_filter in ("DashboardAccessFilter", "ReportScheduleFilter"): + mocker.patch( + f"superset.commands.semantic_layer.delete.{access_filter}.apply", + side_effect=lambda query, value: query, + ) + mocker.patch( + "superset.commands.semantic_layer.delete.get_user_id", return_value=None + ) hidden_total: int hidden_dependents: list[dict[str, str | int]] hidden_count: int @@ -547,13 +561,14 @@ def metadata(source_type: str | None, dataset_id: int | str) -> str: ) assert total == 6 assert archived_count == 1 - assert inaccessible_count == 1 + assert inaccessible_count == 0 assert {item["name"] for item in dependents} == { "Typed integer", "Typed string", "Filter and chart", "Member chart", "Native report", + "Deleted", } dao: MagicMock = mocker.patch( "superset.commands.semantic_layer.delete.SemanticViewDAO" @@ -571,6 +586,10 @@ def metadata(source_type: str | None, dataset_id: int | str) -> str: "superset.commands.semantic_layer.delete.DashboardAccessFilter.apply", return_value=session.query(Dashboard.id).filter(sa.false()), ) + mocker.patch("superset.security_manager.is_admin", return_value=False) + mocker.patch( + "superset.commands.semantic_layer.delete.get_user_id", return_value=None + ) hidden_total: int hidden_dependents: list[dict[str, str | int]] hidden_count: int @@ -654,7 +673,7 @@ def test_semantic_layer_delete_finds_dashboard_target( DeleteSemanticLayerCommand(str(layer_uuid)).run() assert exc_info.value.total == 1 assert exc_info.value.dependents == [ - {"type": "dashboard", "id": 741, "name": "Filter-only"} + {"type": "dashboard", "id": 741, "name": "Filter-only", "archived": False} ] dao.delete.assert_not_called() finally: @@ -1129,3 +1148,100 @@ def test_archived_dashboard_blocks_delete_with_orm_listener( assert exc_info.value.total == (2 if path == "membership" else 1) assert exc_info.value.archived_count == 1 assert exc_info.value.dependents == [] + + +@pytest.mark.parametrize( + ("viewer", "named"), + [ + ("admin", {"Live chart": False, "Archived chart": True}), + ("editor", {"Live chart": False, "Archived chart": True}), + ("reader", {"Live chart": False}), + ("read-only-editor", {"Live chart": False}), + ("guest", {"Live chart": False}), + ("hidden-editor", {"Archived chart": True}), + ], +) +def test_semantic_delete_names_archived_dependents_to_restore_audience( + session: Session, mocker: MockerFixture, viewer: str, named: dict[str, bool] +) -> None: + """Archived blockers are named only to callers with restore capability.""" + from datetime import datetime + + from superset.commands.semantic_layer.delete import _dependent_assets + from superset.models.slice import Slice + from superset.subjects.models import chart_editors, Subject + + Slice.metadata.create_all(session.get_bind()) + session.execute(Subject.__table__.insert().values(id=1, label="editor", type=1)) + session.execute( + Slice.__table__.insert(), + [ + { + "id": 961, + "slice_name": "Live chart", + "datasource_type": "semantic_view", + "datasource_id": 42, + "deleted_at": None, + }, + { + "id": 962, + "slice_name": "Archived chart", + "datasource_type": "semantic_view", + "datasource_id": 42, + "deleted_at": datetime(2026, 1, 1), + }, + ], + ) + session.execute(chart_editors.insert().values(chart_id=962, subject_id=1)) + # Prove the session listener is active; do not substitute Connection.execute. + assert session.scalar(sa.select(Slice.id).where(Slice.id == 962)) is None + mocker.patch( + "superset.security_manager.can_access", + side_effect=lambda permission, resource: viewer != "read-only-editor" + or permission != "can_write", + ) + for access_filter in ( + "ChartFilter", + "DashboardAccessFilter", + "ReportScheduleFilter", + ): + mocker.patch( + f"superset.commands.semantic_layer.delete.{access_filter}.apply", + side_effect=lambda query, value: query, + ) + if viewer == "hidden-editor": + # Editorship names archived rows only: a live row the base filter hides + # stays hidden even from its editor. + session.execute(chart_editors.insert().values(chart_id=961, subject_id=1)) + mocker.patch( + "superset.commands.semantic_layer.delete.ChartFilter.apply", + side_effect=lambda query, value: query.filter(sa.false()), + ) + mocker.patch( + "superset.security_manager.is_admin", + return_value=viewer in ("admin", "guest"), + ) + mocker.patch( + "superset.security_manager.is_guest_user", return_value=viewer == "guest" + ) + mocker.patch("superset.commands.semantic_layer.delete.get_user_id", return_value=7) + mocker.patch( + "superset.commands.semantic_layer.delete.get_user_subject_ids_subquery", + return_value=sa.select( + sa.literal( + 1 if viewer in ("editor", "hidden-editor", "read-only-editor") else 2 + ) + ), + ) + + total: int + dependents: list[dict[str, str | int | bool]] + inaccessible_count: int + archived_count: int + total, dependents, inaccessible_count, archived_count = _dependent_assets( + sa.select(sa.literal(42)) + ) + + assert (total, archived_count) == (2, 1) + assert {str(item["name"]): item["archived"] for item in dependents} == named + assert inaccessible_count == 2 - len(named)