bito-code-review[bot] commented on code in PR #40128:
URL: https://github.com/apache/superset/pull/40128#discussion_r3384012920
##########
superset/translations/sl/LC_MESSAGES/messages.po:
##########
@@ -4437,6 +4437,9 @@ msgstr "Nadzorne plošče ni mogoče posodobiti."
msgid "Dashboard could not be deleted."
msgstr "Nadzorne plošče ni mogoče izbrisati."
+msgid "Dashboard could not be restored."
+msgstr ""
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing Slovenian translation</b></div>
<div id="fix">
The `msgstr` is empty, causing the English message to display to
Slovenian-speaking users when a dashboard restore operation fails. Compare with
the pattern used for similar messages: 'Nadzorne plošče ni mogoče izbrisati.'
(deleted) and 'Nadzorne plošče ni mogoče posodobiti.' (updated). The
appropriate translation following this pattern would be 'Nadzorne plošče ni
mogoče obnoviti.'
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/translations/sl/LC_MESSAGES/messages.po
+++ superset/translations/sl/LC_MESSAGES/messages.po
@@ -4438,7 +4438,7 @@
msgstr "Nadzorne plo\u0161\u010de ni mogo\u010de izbrisati."
msgid "Dashboard could not be restored."
-msgstr ""
+msgstr "Nadzorne plo\u0161\u010de ni mogo\u010de obnoviti."
msgid "Dashboard could not be updated."
msgstr "Nadzorne plo\u0161\u010de ni mogo\u010de posodobiti."
```
</div>
</details>
</div>
<small><i>Code Review Run #92713c</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
##########
superset/dashboards/api.py:
##########
@@ -1213,6 +1235,64 @@ def bulk_delete(self, **kwargs: Any) -> Response:
except DashboardDeleteFailedError as ex:
return self.response_422(message=str(ex))
+ @expose("/<uuid>/restore", methods=("POST",))
+ @protect()
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.restore",
+ log_to_statsd=False,
+ )
+ def restore(self, uuid: str) -> Response:
+ """Restore a soft-deleted dashboard.
+ ---
+ post:
+ summary: Restore a soft-deleted dashboard
+ parameters:
+ - in: path
+ schema:
+ type: string
+ format: uuid
+ name: uuid
+ responses:
+ 200:
+ description: Dashboard restored
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ try:
+ RestoreDashboardCommand(uuid).run()
+ return self.response(200, message="OK")
+ except DashboardNotFoundError:
+ return self.response_404()
+ except DashboardForbiddenError:
+ return self.response_403()
+ except DashboardSlugConflictError as ex:
+ return self.response_422(message=str(ex))
+ except DashboardRestoreFailedError as ex:
+ logger.error(
+ "Error restoring model %s: %s",
+ self.__class__.__name__,
+ str(ex),
+ exc_info=True,
+ )
+ return self.response_422(message=str(ex))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test coverage for error path</b></div>
<div id="fix">
The `DashboardRestoreFailedError` exception handler (lines 1287-1294) is not
covered by any test. Per BITO.md rule [11730], new features should include
tests for error scenarios including resource-not-found and failure cases. Add a
test that simulates a restore failure to verify the 422 response is returned
correctly.
</div>
</div>
<small><i>Code Review Run #92713c</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
##########
superset/translations/fi/LC_MESSAGES/messages.po:
##########
@@ -7748,6 +7748,9 @@ msgstr "Kojelautaa ei voitu poistaa."
# de, es, fa, fr, it, ja, ko, lv, mi, nl, pl, pt, pt_BR, ru, sk, sl, tr, uk,
# zh, zh_TW]
#, fuzzy
+msgid "Dashboard could not be restored."
+msgstr ""
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Empty translation string</b></div>
<div id="fix">
The added `msgstr ""` is empty, which will display the English message to
Finnish users. Per adaptive rule [6516], user-facing strings must have proper
translations. Consider providing a Finnish translation or removing this entry
until a proper translation is supplied.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/translations/fi/LC_MESSAGES/messages.po
+++ superset/translations/fi/LC_MESSAGES/messages.po
@@ -7749,7 +7749,7 @@
# de, es, fa, fr, it, ja, ko, lv, mi, nl, pl, pt, pt_BR, ru, sk, sl, tr,
uk,
# zh, zh_TW]
#, fuzzy
msgid "Dashboard could not be restored."
-msgstr ""
+msgstr "Kojelautaa ei voitu palauttaa."
```
</div>
</details>
</div>
<small><i>Code Review Run #92713c</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
##########
superset/migrations/versions/2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.py:
##########
@@ -0,0 +1,184 @@
+# 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.
+"""Add deleted_at + partial unique slug index for soft-delete.
+
+Adds to the ``dashboards`` table:
+- a nullable ``deleted_at`` column for soft-delete state
+- an index ``ix_dashboards_deleted_at`` for the visibility-filter listener
+- a partial unique index ``ix_dashboards_active_slug`` enforcing slug
+ uniqueness only among active (non-soft-deleted) rows
+
+Drops:
+- the existing full unique constraint on ``slug`` (named
+ ``idx_unique_slug``, created in migration 1a48a5411020)
+
+The constraint change makes the ``slug`` field reusable after soft-delete:
+soft-deleted rows no longer reserve their slug for the lifetime of the
+row. ``RestoreDashboardCommand`` handles the reverse case (restoring a
+dashboard whose slug has since been claimed by another active row) with
+an explicit conflict error. See UPDATING.md for the user-facing change.
+
+Dialect support for the partial index:
+- PostgreSQL: native ``WHERE deleted_at IS NULL`` partial index
+- MySQL 8.0+: functional index over
+ ``(CASE WHEN deleted_at IS NULL THEN slug END)``
+- MySQL <8.0: keeps the original full unique constraint (documented
+ limitation; functional indexes are not supported on these versions)
+- SQLite: keeps the original full unique constraint (column-level
+ ``UNIQUE`` cannot be dropped without recreating the table, which is
+ not worth the migration complexity for a test-only dialect). Tests
+ that need to verify the partial-index behaviour run only on
+ PostgreSQL and MySQL 8+.
+
+Revision ID: 9e1f3b8c4d2a
+Revises: 31dae2559c05
+Create Date: 2026-05-08 12:05:00.000000
+"""
+
+from alembic import op
+from sqlalchemy import Column, DateTime
+from sqlalchemy.engine import Connection
+
+from superset.migrations.shared.utils import (
+ add_columns,
+ create_index,
+ drop_columns,
+ drop_index,
+)
+
+# revision identifiers, used by Alembic.
+revision = "9e1f3b8c4d2a"
+down_revision = "31dae2559c05"
+
+TABLE_NAME = "dashboards"
+DELETED_AT_INDEX_NAME = f"ix_{TABLE_NAME}_deleted_at"
+PARTIAL_SLUG_INDEX_NAME = f"ix_{TABLE_NAME}_active_slug"
+# The original full unique constraint on ``slug`` was created with an
+# explicit name in migration 1a48a5411020 (2015-12-04). Same name on
+# PostgreSQL (constraint) and MySQL (index).
+LEGACY_SLUG_INDEX_NAME = "idx_unique_slug"
+
+
+def _mysql_supports_functional_index(bind: Connection) -> bool:
+ """Return True iff the connected MySQL is 8.0.13+ (supports functional
indexes).
+
+ MySQL added functional key parts in 8.0.13; 8.0.0–8.0.12 reject the
+ ``(CASE WHEN deleted_at IS NULL THEN slug END)`` expression at index
+ creation time, so deployments on those patch releases must keep the
+ original full slug constraint. See
+ https://dev.mysql.com/doc/mysql/8.0/en/create-index.html for the
+ 8.0.13 minimum.
+
+ Excludes MariaDB even at server version ``>= (10, x)`` because MariaDB
+ reports through the same ``server_version_info`` attribute but uses
+ different functional-index semantics around ``CASE`` expressions.
+ Uses SQLAlchemy's parsed ``server_version_info`` rather than ``SELECT
+ VERSION()`` to avoid an extra round-trip and brittle string parsing.
+ """
+ if getattr(bind.dialect, "is_mariadb", False):
+ return False
+ return (bind.dialect.server_version_info or ()) >= (8, 0, 13)
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ _add_deleted_at_column()
+ _replace_slug_constraint_with_partial_index(bind)
+
+
+def downgrade() -> None:
+ bind = op.get_bind()
+ _restore_slug_constraint(bind)
+ _drop_deleted_at_column()
+
+
+def _add_deleted_at_column() -> None:
+ add_columns(TABLE_NAME, Column("deleted_at", DateTime(), nullable=True))
+ create_index(TABLE_NAME, DELETED_AT_INDEX_NAME, ["deleted_at"])
+
+
+def _drop_deleted_at_column() -> None:
+ drop_index(TABLE_NAME, DELETED_AT_INDEX_NAME)
+ drop_columns(TABLE_NAME, "deleted_at")
+
+
+def _replace_slug_constraint_with_partial_index(bind: Connection) -> None:
+ """Swap the full UNIQUE on ``slug`` for a partial index where supported.
+
+ The original constraint is named ``idx_unique_slug`` from migration
+ 1a48a5411020 — same name on PostgreSQL (constraint) and MySQL (index).
+
+ SQLite and MySQL <8.0 are no-ops here: they keep the original full
+ unique constraint. See the module docstring for the rationale.
+ """
+ dialect = bind.dialect.name
+ if dialect == "postgresql":
+ op.execute(
+ f"ALTER TABLE {TABLE_NAME} "
+ f"DROP CONSTRAINT IF EXISTS {LEGACY_SLUG_INDEX_NAME}"
+ )
+ # Some installations may have the unique enforced as a plain
+ # index rather than a constraint. Both DROPs are IF EXISTS, so
+ # whichever path applies cleans up.
+ op.execute(f"DROP INDEX IF EXISTS {LEGACY_SLUG_INDEX_NAME}")
+ op.execute(
+ f"CREATE UNIQUE INDEX {PARTIAL_SLUG_INDEX_NAME} "
+ f"ON {TABLE_NAME} (slug) WHERE deleted_at IS NULL"
+ )
+ elif dialect == "mysql" and _mysql_supports_functional_index(bind):
+ # Create the functional replacement BEFORE dropping the legacy unique
+ # index. MySQL autocommits each DDL statement (unlike PostgreSQL's
+ # transactional DDL above, where a failed CREATE rolls back the DROP),
+ # so a drop-then-create ordering would leave the table with no slug
+ # uniqueness if the CREATE failed. Creating first keeps the stricter
+ # existing uniqueness in place until the replacement is confirmed.
+ op.execute(
+ f"CREATE UNIQUE INDEX {PARTIAL_SLUG_INDEX_NAME} "
+ f"ON {TABLE_NAME} ((CASE WHEN deleted_at IS NULL THEN slug END))"
+ )
+ op.execute(f"ALTER TABLE {TABLE_NAME} DROP INDEX
{LEGACY_SLUG_INDEX_NAME}")
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>MySQL DDL lacks idempotency guard</b></div>
<div id="fix">
Wrap the MySQL `CREATE UNIQUE INDEX` in an existence check (e.g., query
`SHOW INDEXES` or use SQLAlchemy inspection) before executing it, so rerunning
`upgrade()` does not error if the index already exists.
</div>
</div>
<small><i>Code Review Run #92713c</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
##########
superset/migrations/versions/2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.py:
##########
@@ -0,0 +1,184 @@
+# 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.
+"""Add deleted_at + partial unique slug index for soft-delete.
+
+Adds to the ``dashboards`` table:
+- a nullable ``deleted_at`` column for soft-delete state
+- an index ``ix_dashboards_deleted_at`` for the visibility-filter listener
+- a partial unique index ``ix_dashboards_active_slug`` enforcing slug
+ uniqueness only among active (non-soft-deleted) rows
+
+Drops:
+- the existing full unique constraint on ``slug`` (named
+ ``idx_unique_slug``, created in migration 1a48a5411020)
+
+The constraint change makes the ``slug`` field reusable after soft-delete:
+soft-deleted rows no longer reserve their slug for the lifetime of the
+row. ``RestoreDashboardCommand`` handles the reverse case (restoring a
+dashboard whose slug has since been claimed by another active row) with
+an explicit conflict error. See UPDATING.md for the user-facing change.
+
+Dialect support for the partial index:
+- PostgreSQL: native ``WHERE deleted_at IS NULL`` partial index
+- MySQL 8.0+: functional index over
+ ``(CASE WHEN deleted_at IS NULL THEN slug END)``
+- MySQL <8.0: keeps the original full unique constraint (documented
+ limitation; functional indexes are not supported on these versions)
+- SQLite: keeps the original full unique constraint (column-level
+ ``UNIQUE`` cannot be dropped without recreating the table, which is
+ not worth the migration complexity for a test-only dialect). Tests
+ that need to verify the partial-index behaviour run only on
+ PostgreSQL and MySQL 8+.
+
+Revision ID: 9e1f3b8c4d2a
+Revises: 31dae2559c05
+Create Date: 2026-05-08 12:05:00.000000
+"""
+
+from alembic import op
+from sqlalchemy import Column, DateTime
+from sqlalchemy.engine import Connection
+
+from superset.migrations.shared.utils import (
+ add_columns,
+ create_index,
+ drop_columns,
+ drop_index,
+)
+
+# revision identifiers, used by Alembic.
+revision = "9e1f3b8c4d2a"
+down_revision = "31dae2559c05"
+
+TABLE_NAME = "dashboards"
+DELETED_AT_INDEX_NAME = f"ix_{TABLE_NAME}_deleted_at"
+PARTIAL_SLUG_INDEX_NAME = f"ix_{TABLE_NAME}_active_slug"
+# The original full unique constraint on ``slug`` was created with an
+# explicit name in migration 1a48a5411020 (2015-12-04). Same name on
+# PostgreSQL (constraint) and MySQL (index).
+LEGACY_SLUG_INDEX_NAME = "idx_unique_slug"
+
+
+def _mysql_supports_functional_index(bind: Connection) -> bool:
+ """Return True iff the connected MySQL is 8.0.13+ (supports functional
indexes).
+
+ MySQL added functional key parts in 8.0.13; 8.0.0–8.0.12 reject the
+ ``(CASE WHEN deleted_at IS NULL THEN slug END)`` expression at index
+ creation time, so deployments on those patch releases must keep the
+ original full slug constraint. See
+ https://dev.mysql.com/doc/mysql/8.0/en/create-index.html for the
+ 8.0.13 minimum.
+
+ Excludes MariaDB even at server version ``>= (10, x)`` because MariaDB
+ reports through the same ``server_version_info`` attribute but uses
+ different functional-index semantics around ``CASE`` expressions.
+ Uses SQLAlchemy's parsed ``server_version_info`` rather than ``SELECT
+ VERSION()`` to avoid an extra round-trip and brittle string parsing.
+ """
+ if getattr(bind.dialect, "is_mariadb", False):
+ return False
+ return (bind.dialect.server_version_info or ()) >= (8, 0, 13)
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ _add_deleted_at_column()
+ _replace_slug_constraint_with_partial_index(bind)
+
+
+def downgrade() -> None:
+ bind = op.get_bind()
+ _restore_slug_constraint(bind)
+ _drop_deleted_at_column()
+
+
+def _add_deleted_at_column() -> None:
+ add_columns(TABLE_NAME, Column("deleted_at", DateTime(), nullable=True))
+ create_index(TABLE_NAME, DELETED_AT_INDEX_NAME, ["deleted_at"])
+
+
+def _drop_deleted_at_column() -> None:
+ drop_index(TABLE_NAME, DELETED_AT_INDEX_NAME)
+ drop_columns(TABLE_NAME, "deleted_at")
+
+
+def _replace_slug_constraint_with_partial_index(bind: Connection) -> None:
+ """Swap the full UNIQUE on ``slug`` for a partial index where supported.
+
+ The original constraint is named ``idx_unique_slug`` from migration
+ 1a48a5411020 — same name on PostgreSQL (constraint) and MySQL (index).
+
+ SQLite and MySQL <8.0 are no-ops here: they keep the original full
+ unique constraint. See the module docstring for the rationale.
+ """
+ dialect = bind.dialect.name
+ if dialect == "postgresql":
+ op.execute(
+ f"ALTER TABLE {TABLE_NAME} "
+ f"DROP CONSTRAINT IF EXISTS {LEGACY_SLUG_INDEX_NAME}"
+ )
+ # Some installations may have the unique enforced as a plain
+ # index rather than a constraint. Both DROPs are IF EXISTS, so
+ # whichever path applies cleans up.
+ op.execute(f"DROP INDEX IF EXISTS {LEGACY_SLUG_INDEX_NAME}")
+ op.execute(
+ f"CREATE UNIQUE INDEX {PARTIAL_SLUG_INDEX_NAME} "
+ f"ON {TABLE_NAME} (slug) WHERE deleted_at IS NULL"
+ )
+ elif dialect == "mysql" and _mysql_supports_functional_index(bind):
+ # Create the functional replacement BEFORE dropping the legacy unique
+ # index. MySQL autocommits each DDL statement (unlike PostgreSQL's
+ # transactional DDL above, where a failed CREATE rolls back the DROP),
+ # so a drop-then-create ordering would leave the table with no slug
+ # uniqueness if the CREATE failed. Creating first keeps the stricter
+ # existing uniqueness in place until the replacement is confirmed.
+ op.execute(
+ f"CREATE UNIQUE INDEX {PARTIAL_SLUG_INDEX_NAME} "
+ f"ON {TABLE_NAME} ((CASE WHEN deleted_at IS NULL THEN slug END))"
+ )
+ op.execute(f"ALTER TABLE {TABLE_NAME} DROP INDEX
{LEGACY_SLUG_INDEX_NAME}")
+
+
+def _restore_slug_constraint(bind: Connection) -> None:
+ """Restore the full UNIQUE on ``slug`` from the partial index.
+
+ Symmetric counterpart to ``_replace_slug_constraint_with_partial_index``.
+ No-op on dialects that never received the partial index.
+
+ Pre-condition: each value of ``slug`` (other than NULL) must appear at
+ most once across the entire ``dashboards`` table. The partial-index
+ window allowed an active row and a soft-deleted row to share a slug;
+ rebuilding the full unique constraint will abort with a
+ ``UniqueViolation`` if any such pair still exists. Before downgrading,
+ hard-delete the soft-deleted duplicates (or rename one side of each
+ pair) so the constraint can be added cleanly.
+ """
+ dialect = bind.dialect.name
+ if dialect == "postgresql":
+ op.execute(f"DROP INDEX IF EXISTS {PARTIAL_SLUG_INDEX_NAME}")
+ op.execute(
+ f"ALTER TABLE {TABLE_NAME} "
+ f"ADD CONSTRAINT {LEGACY_SLUG_INDEX_NAME} UNIQUE (slug)"
+ )
+ elif dialect == "mysql" and _mysql_supports_functional_index(bind):
+ # Symmetric to the upgrade: add the full unique index before dropping
+ # the partial one, so a failed ADD leaves the partial uniqueness intact
+ # rather than no uniqueness (MySQL autocommits each DDL statement).
+ op.execute(
+ f"ALTER TABLE {TABLE_NAME} ADD UNIQUE INDEX
{LEGACY_SLUG_INDEX_NAME} (slug)"
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>MySQL downgrade lacks idempotency guard</b></div>
<div id="fix">
Before calling `ALTER TABLE ADD UNIQUE INDEX` and `ALTER TABLE DROP INDEX`,
query `SHOW INDEXES` (or use SQLAlchemy inspection) to verify the target index
exists or not, ensuring the downgrade path is idempotent.
</div>
</div>
<small><i>Code Review Run #92713c</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
##########
superset/translations/ru/LC_MESSAGES/messages.po:
##########
@@ -4345,6 +4345,9 @@ msgstr "Не удалось обновить цветовую конфигура
msgid "Dashboard could not be deleted."
msgstr "Не удалось удалить дашборд"
+msgid "Dashboard could not be restored."
+msgstr ""
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing Russian translation</b></div>
<div id="fix">
The new translation entry for "Dashboard could not be restored." has an
empty `msgstr`. Following the established pattern for similar dashboard error
messages (e.g., "Не удалось удалить дашборд", "Не удалось обновить дашборд"),
the Russian translation should be: "Не удалось восстановить дашборд".
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset/translations/ru/LC_MESSAGES/messages.po
+++ b/superset/translations/ru/LC_MESSAGES/messages.po
@@ -4346,7 +4346,7 @@ msgstr "Не удалось удалить дашборд"
msgid "Dashboard could not be restored."
-msgstr ""
+msgstr "Не удалось восстановить дашборд"
msgid "Dashboard could not be updated."
msgstr "Не удалось обновить дашборд"
```
</div>
</details>
</div>
<small><i>Code Review Run #92713c</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
##########
tests/integration_tests/dashboards/soft_delete_tests.py:
##########
@@ -0,0 +1,687 @@
+# 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
+
+ admin = self.get_user("admin")
+ database = Database(database_name="sd_acl_db",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+ table = SqlaTable(table_name="sd_acl_tbl", database=database)
+ db.session.add(table)
+ db.session.flush()
+ chart = Slice(
+ slice_name="sd_acl_slice",
+ datasource_id=table.id,
+ datasource_type="table",
+ viz_type="table",
+ )
+ db.session.add(chart)
+ dashboard = Dashboard(
+ dashboard_title="sd_acl_dash",
+ slug="sd_acl_dash",
+ owners=[admin],
+ slices=[chart],
+ published=True,
+ )
+ db.session.add(dashboard)
+ db.session.commit()
+ dashboard_id = dashboard.id
+
+ gamma_role = security_manager.find_role("Gamma")
+ pvm = security_manager.add_permission_view_menu("datasource_access",
table.perm)
+ gamma_role.permissions.append(pvm)
+ db.session.commit()
+
+ title_filter =
"(col:dashboard_title,opr:title_or_slug,value:sd_acl_dash)"
+ try:
+ # Precondition: gamma can see the dashboard while it is live.
+ self.login(GAMMA_USERNAME)
+ rv =
self.client.get(f"/api/v1/dashboard/?q=(filters:!({title_filter}))")
+ live_ids = [row["id"] for row in json.loads(rv.data)["result"]]
+ assert dashboard_id in live_ids, (
+ "precondition failed: gamma should see the live dashboard via "
+ "datasource access"
+ )
+
+ # Soft-delete directly (no admin re-login needed; the DELETE
+ # endpoint's auth is exercised elsewhere). This isolates the
+ # deleted-state visibility behaviour under test.
+ dashboard.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
+ db.session.commit()
+
+ # Gamma (still logged in) must not see the soft-deleted dashboard.
+ for value in ("include", "only"):
+ rison_query = (
+ f"(filters:!({title_filter},"
+ f"(col:id,opr:dashboard_deleted_state,value:{value})))"
+ )
+ 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 not in ids, (
+ "read-access non-owner must not enumerate a soft-deleted "
+ f"dashboard via deleted_state={value}"
+ )
+ finally:
+ pvm = security_manager.find_permission_view_menu(
+ "datasource_access", table.perm
+ )
+ if pvm:
+ security_manager.del_permission_role(gamma_role, pvm)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Non-existent method call in cleanup</b></div>
<div id="fix">
The method `security_manager.del_permission_role` does not exist in
`SupersetSecurityManager`, causing an `AttributeError` at runtime when the test
cleanup block executes. Other Superset tests (e.g.,
`datasets/api_tests.py:352`) use `gamma_role.permissions.remove(pvm)` directly
instead.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/tests/integration_tests/dashboards/soft_delete_tests.py
+++ b/tests/integration_tests/dashboards/soft_delete_tests.py
@@ -252,7 +252,7 @@ class TestDashboardSoftDelete(SupersetTestCase):
if pvm:
- security_manager.del_permission_role(gamma_role, pvm)
+ gamma_role.permissions.remove(pvm)
_hard_delete_dashboard(dashboard_id)
db.session.delete(chart)
db.session.delete(table)
```
</div>
</details>
</div>
<small><i>Code Review Run #92713c</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]
