bito-code-review[bot] commented on code in PR #40128:
URL: https://github.com/apache/superset/pull/40128#discussion_r3455376892
##########
superset/translations/mi/LC_MESSAGES/messages.po:
##########
@@ -4372,6 +4372,9 @@ msgstr "Kāore i taea te whakahou i te whirihora tae
papatohu."
msgid "Dashboard could not be deleted."
msgstr "Kāore i taea te muku i te papatohu."
+msgid "Dashboard could not be restored."
+msgstr ""
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete translation entry</b></div>
<div id="fix">
The new translation entry has an empty `msgstr ""`. Based on the consistent
pattern in this file for similar dashboard error messages (lines 4372-4373:
"Kāore i taea te muku i te papatohu." for delete; lines 4378-4379: "Kāore i
taea te whakahou i te papatohu." for update), the Māori translation should be
"Kāore i taea te whakahoki i te papatohu." for restore. This maintains
consistency for Māori-speaking users.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/translations/mi/LC_MESSAGES/messages.po
+++ superset/translations/mi/LC_MESSAGES/messages.po
@@ -4373,7 +4373,7 @@
msgstr "Kāore i taea te muku i te papatohu."
msgid "Dashboard could not be restored."
-msgstr ""
+msgstr "Kāore i taea te whakahoki i te papatohu."
msgid "Dashboard could not be updated."
msgstr "Kāore i taea te whakahou i te papatohu."
```
</div>
</details>
</div>
<small><i>Code Review Run #b0186f</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/unit_tests/dashboards/commands/importers/v1/import_test.py:
##########
@@ -294,6 +295,258 @@ def test_import_new_dashboard_adds_importer_as_owner(
assert user in result.owners
+def test_import_soft_deleted_dashboard_overwrite_restores_in_place(
+ mocker: MockerFixture,
+ session_with_data: Session,
+) -> None:
+ """
+ Overwrite-importing a soft-deleted dashboard must restore the row in
+ place rather than hard-delete-and-replace. A hard delete would
+ cascade through dashboard_slices junctions; in-place restore
+ preserves them.
+
+ Asserts not just that the PK survives, but that the
+ ``dashboard_slices`` junction rows do too — that's the whole point
+ of restore-in-place vs delete-and-replace, so a regression that
+ re-introduces the hard-delete shape must trip this test.
+ """
+ from superset.connectors.sqla.models import Database, SqlaTable
+ from superset.models.dashboard import dashboard_slices
+ from superset.models.slice import Slice
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "can_access_dashboard",
return_value=True)
+
+ existing = (
+ session_with_data.query(Dashboard)
+ .filter(Dashboard.uuid == dashboard_config["uuid"])
+ .one_or_none()
+ )
+ assert existing is not None
+ original_id = existing.id
+
+ # Attach a chart via the dashboard_slices M2M before soft-delete so
+ # we can assert the junction row survives the restore-in-place.
+ dataset = SqlaTable(
+ table_name="junction_test_table",
+ database=Database(database_name="junction_test_db",
sqlalchemy_uri="sqlite://"),
+ )
+ session_with_data.add(dataset)
+ session_with_data.flush()
+ chart = Slice(
+ slice_name="junction_test_chart",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ )
+ session_with_data.add(chart)
+ session_with_data.flush()
+ existing.slices.append(chart)
+ session_with_data.flush()
+ chart_id = chart.id
+
+ junction_before = (
+ session_with_data.query(dashboard_slices)
+ .filter(
+ dashboard_slices.c.dashboard_id == original_id,
+ dashboard_slices.c.slice_id == chart_id,
+ )
+ .count()
+ )
+ assert junction_before == 1, "junction row precondition not established"
+
+ existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0, tzinfo=timezone.utc)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate test data setup in importer tests</b></div>
<div id="fix">
Code duplication detected: Test data setup logic for User creation and
override_user context is duplicated (13 lines) within
tests/unit_tests/dashboards/commands/importers/v1/import_test.py (lines
357-369, 412-424). Consider using a pytest fixture.
</div>
</div>
<small><i>Code Review Run #b0186f</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/unit_tests/commands/dashboard/restore_test.py:
##########
@@ -0,0 +1,201 @@
+# 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.
+"""Unit tests for RestoreDashboardCommand."""
+
+from __future__ import annotations
+
+from datetime import datetime, timezone
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+
+def test_restore_dashboard_not_found_raises(app_context: None) -> None:
+ """RestoreDashboardCommand raises DashboardNotFoundError for missing
dashboard."""
+ from superset.commands.dashboard.exceptions import DashboardNotFoundError
+ from superset.commands.dashboard.restore import RestoreDashboardCommand
+
+ with patch("superset.daos.dashboard.DashboardDAO.find_by_id",
return_value=None):
+ cmd = RestoreDashboardCommand("999")
+ with pytest.raises(DashboardNotFoundError):
+ cmd.run()
+
+
+def test_restore_active_dashboard_raises_not_found(app_context: None) -> None:
+ """RestoreDashboardCommand raises DashboardNotFoundError for non-deleted
dashboard.""" # noqa: E501
+ from superset.commands.dashboard.exceptions import DashboardNotFoundError
+ from superset.commands.dashboard.restore import RestoreDashboardCommand
+
+ dashboard = MagicMock()
+ dashboard.deleted_at = None # not soft-deleted
+
+ with patch(
+ "superset.daos.dashboard.DashboardDAO.find_by_id",
return_value=dashboard
+ ):
+ cmd = RestoreDashboardCommand("1")
+ with pytest.raises(DashboardNotFoundError):
+ cmd.run()
+
+
+def test_restore_dashboard_forbidden_raises(app_context: None) -> None:
+ """RestoreDashboardCommand raises DashboardForbiddenError on permission
check."""
+ from superset.commands.dashboard.exceptions import DashboardForbiddenError
+ from superset.commands.dashboard.restore import RestoreDashboardCommand
+ from superset.exceptions import SupersetSecurityException
+
+ dashboard = MagicMock()
+ dashboard.deleted_at = datetime(2026, 1, 1, tzinfo=timezone.utc)
+
+ def raise_security(*args: object, **kwargs: object) -> None:
+ raise SupersetSecurityException(MagicMock())
+
+ with (
+ patch(
+ "superset.daos.dashboard.DashboardDAO.find_by_id",
return_value=dashboard
+ ),
+ patch("superset.commands.restore.security_manager") as mock_sec,
+ ):
+ mock_sec.raise_for_ownership = raise_security
+
+ cmd = RestoreDashboardCommand("1")
+ with pytest.raises(DashboardForbiddenError):
+ cmd.run()
+
+
+def test_restore_dashboard_slug_conflict_raises(app_context: None) -> None:
+ """Restore raises DashboardSlugConflictError when slug is now claimed by
an active dashboard.
+
+ The partial unique index ``WHERE deleted_at IS NULL`` allows another
+ dashboard to claim the slug while this one was soft-deleted. The
+ command catches that case before flushing so the operator sees a
+ domain-specific error instead of an opaque unique-index violation.
+ """ # noqa: E501
+ from superset.commands.dashboard.exceptions import
DashboardSlugConflictError
+ from superset.commands.dashboard.restore import RestoreDashboardCommand
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate mock setup for restore tests</b></div>
<div id="fix">
Code duplication detected: Mock dashboard setup is duplicated (14 lines) in
tests/unit_tests/commands/dashboard/restore_test.py (lines 88-101, 117-130).
Consider using a pytest fixture to reduce duplication.
</div>
</div>
<small><i>Code Review Run #b0186f</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/it/LC_MESSAGES/messages.po:
##########
@@ -4331,6 +4331,9 @@ msgstr "La tua query non può essere salvata"
msgid "Dashboard could not be deleted."
msgstr "La tua query non può essere salvata"
+msgid "Dashboard could not be restored."
+msgstr ""
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Empty Italian translation</b></div>
<div id="fix">
The translation for "Dashboard could not be restored." is left empty
(`msgstr ""`), causing Italian users to see the untranslated English error
message. This error originates from `DashboardRestoreFailedError` at
`superset/commands/dashboard/exceptions.py:81` and surfaces when a dashboard
restore operation fails.
</div>
</div>
<small><i>Code Review Run #cb1ccb</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Missing Italian translation</b></div>
<div id="fix">
New message entry at line 4344-4345 has empty `msgstr ""`. This new
translation key should have a proper Italian translation, not an empty string.
</div>
</div>
<small><i>Code Review Run #b0186f</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]
