bito-code-review[bot] commented on code in PR #40128: URL: https://github.com/apache/superset/pull/40128#discussion_r3377257907
########## tests/integration_tests/dashboards/soft_delete_tests.py: ########## @@ -0,0 +1,689 @@ +# 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. +"""Integration tests for dashboard soft-delete and restore.""" + +from datetime import datetime +from typing import Any + +from superset import security_manager +from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES +from superset.extensions import db +from superset.models.dashboard import Dashboard +from superset.utils import json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ( + ADMIN_USERNAME, + ALPHA_USERNAME, + GAMMA_USERNAME, +) + + +def _hard_delete_dashboard(dashboard_id: int) -> None: + """Hard-delete a dashboard row regardless of soft-delete state.""" + row = ( + db.session.query(Dashboard) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Dashboard}}) + .filter(Dashboard.id == dashboard_id) + .one_or_none() + ) + if row: + db.session.delete(row) + db.session.commit() + + +class TestDashboardSoftDelete(SupersetTestCase): + """Tests for dashboard soft-delete behaviour (T014, T017).""" + + def _create_dashboard(self, title: str = "soft_delete_test") -> Dashboard: + admin = self.get_user("admin") + dashboard = Dashboard( + dashboard_title=title, + slug=f"slug_{title}", + owners=[admin], + published=True, + ) + db.session.add(dashboard) + db.session.commit() + return dashboard + + def test_delete_dashboard_soft_deletes(self) -> None: + """DELETE should set deleted_at instead of removing the row.""" + dashboard = self._create_dashboard("sd_test_1") + dashboard_id = dashboard.id + self.login(ADMIN_USERNAME) + + rv = self.client.delete(f"/api/v1/dashboard/{dashboard_id}") + assert rv.status_code == 200 + + row = ( + db.session.query(Dashboard) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Dashboard}}) + .filter(Dashboard.id == dashboard_id) + .one_or_none() + ) + assert row is not None + assert row.deleted_at is not None + + # Cleanup + _hard_delete_dashboard(dashboard_id) + + def test_soft_deleted_dashboard_excluded_from_list(self) -> None: + """GET /api/v1/dashboard/ should not include soft-deleted.""" + dashboard = self._create_dashboard("sd_list_test") + dashboard_id = dashboard.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/dashboard/{dashboard_id}") + + rv = self.client.get("/api/v1/dashboard/") + data = json.loads(rv.data) + ids = [d["id"] for d in data["result"]] + assert dashboard_id not in ids + + # Cleanup + _hard_delete_dashboard(dashboard_id) + + def test_soft_deleted_dashboard_included_in_list_when_requested(self) -> None: + """GET /api/v1/dashboard/ with dashboard_deleted_state=include returns deleted dashboards.""" # noqa: E501 + dashboard = self._create_dashboard("sd_list_with_deleted") + dashboard_id = dashboard.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/dashboard/{dashboard_id}") + + rison_query = "(filters:!((col:id,opr:dashboard_deleted_state,value:include)))" + rv = self.client.get(f"/api/v1/dashboard/?q={rison_query}") + assert rv.status_code == 200 + + data = json.loads(rv.data) + deleted_row = next( + (row for row in data["result"] if row["id"] == dashboard_id), + None, + ) + assert deleted_row is not None + assert deleted_row["deleted_at"] is not None + + # Cleanup + _hard_delete_dashboard(dashboard_id) + + def test_only_filter_returns_only_soft_deleted_dashboards(self) -> None: + """dashboard_deleted_state=only excludes live rows and returns only deleted ones.""" # noqa: E501 + live_dashboard = self._create_dashboard("only_live_dash") + deleted_dashboard = self._create_dashboard("only_deleted_dash") + live_id = live_dashboard.id + deleted_id = deleted_dashboard.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/dashboard/{deleted_id}") + + rison_query = "(filters:!((col:id,opr:dashboard_deleted_state,value:only)))" + rv = self.client.get(f"/api/v1/dashboard/?q={rison_query}") + assert rv.status_code == 200 + + data = json.loads(rv.data) + returned_ids = {row["id"] for row in data["result"]} + assert deleted_id in returned_ids + assert live_id not in returned_ids + + # Cleanup + _hard_delete_dashboard(live_id) + _hard_delete_dashboard(deleted_id) + + def test_deleted_state_list_shows_owner_their_own_deleted(self) -> None: + """A non-admin owner can still enumerate their own soft-deleted + dashboards. Deleted-state scoping mirrors the restore audience, so it + must not lock owners out of their own trash.""" + alpha = self.get_user("alpha") + dashboard = Dashboard( + dashboard_title="sd_owner_dash", + slug="sd_owner_dash", + owners=[alpha], + published=True, + ) + db.session.add(dashboard) + db.session.commit() + dashboard_id = dashboard.id + + self.login(ALPHA_USERNAME) + self.client.delete(f"/api/v1/dashboard/{dashboard_id}") + + rison_query = ( + "(filters:!((col:dashboard_title,opr:title_or_slug,value:sd_owner_dash)," + "(col:id,opr:dashboard_deleted_state,value:only)))" + ) + rv = self.client.get(f"/api/v1/dashboard/?q={rison_query}") + assert rv.status_code == 200 + ids = [row["id"] for row in json.loads(rv.data)["result"]] + assert dashboard_id in ids + + # Cleanup + _hard_delete_dashboard(dashboard_id) + + def test_deleted_state_list_hides_non_owned_from_read_access_user(self) -> None: + """A read-access non-owner must not be able to enumerate a dashboard + once it is soft-deleted. + + Gamma is granted ``datasource_access`` to a published dashboard's + dataset, so ``DashboardAccessFilter`` makes the dashboard visible to + gamma while it is live. After soft-delete, the deleted-state list is + scoped to the restore audience (owners/admins), so gamma — who could + never restore it — must not see it via ``include`` or ``only``. + """ + from superset.connectors.sqla.models import SqlaTable + from superset.models.core import Database + from superset.models.slice import Slice Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Redundant function-level imports</b></div> <div id="fix"> These imports execute on every test invocation rather than once at module load. Move them to the module-level imports alongside `security_manager` and `GAMMA_USERNAME` at lines 19-32. </div> </div> <small><i>Code Review Run #675b2b</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
