bito-code-review[bot] commented on code in PR #40129:
URL: https://github.com/apache/superset/pull/40129#discussion_r3358120790
##########
superset/translations/sl/LC_MESSAGES/messages.po:
##########
@@ -3119,6 +3119,9 @@ msgstr "Spremembe grafikona"
msgid "Chart could not be created."
msgstr "Grafikona ni mogoče ustvariti."
+msgid "Chart 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` field is empty, so Slovenian users will see the untranslated
English error message "Chart could not be restored." Compare with the adjacent
translations: "Grafikona ni mogoče ustvariti." (create) and "Grafikona ni
mogoče posodobiti." (update) — the same construction applies.
</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 (lines 3122-3123) ---
3122: msgid "Chart could not be restored."
3123:-msgstr ""
3123:+msgstr "Grafikona ni mogoče obnoviti."
```
</div>
</details>
</div>
<small><i>Code Review Run #7d71bd</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/charts/commands/importers/v1/import_test.py:
##########
@@ -287,6 +288,184 @@ def test_import_existing_chart_with_permission(
mock_can_access_chart.assert_called_once_with(slice)
+def test_import_soft_deleted_chart_overwrite_restores_in_place(
+ mocker: MockerFixture,
+ session_with_data: Session,
+) -> None:
+ """
+ Overwrite-importing a soft-deleted chart must restore the row in place,
+ not hard-delete-and-replace. Otherwise out-of-archive references
+ (dashboard_slices junctions, report.chart_id) would cascade away.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Extract duplicated test setup</b></div>
<div id="fix">
This PR contains syntactic duplication in the test file at lines 300-326 and
372-398 where 27 lines of identical test setup code are repeated. Consider
extracting this into a shared fixture to reduce maintenance burden and
potential inconsistencies. No functional changes to production code detected.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
@@ -298,6 +298,28 @@ class TestChartImport:
+ def _setup_chart_overwrite_context(
+ self,
+ session_with_data,
+ security_manager,
+ chart_config,
+ mocker,
+ ):
+ """Shared helper to setup chart overwrite test context."""
+ mocker.patch.object(security_manager, "can_access",
return_value=True)
+ mocker.patch.object(security_manager, "can_access_chart",
return_value=True)
+
+ existing = (
+ session_with_data.query(Slice)
+ .filter(Slice.uuid == chart_config["uuid"])
+ .one_or_none()
+ )
+ assert existing is not None
+ original_id = existing.id
+ existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
+ session_with_data.flush()
+
+ admin = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="[email protected]",
+ username="admin",
+ roles=[Role(name="Admin")],
+ )
+
+ config = copy.deepcopy(chart_config)
+ config["datasource_id"] = 1
+ config["datasource_type"] = "table"
+
+ return admin, config
+
+
class TestChartImportOverwrite:
def test_import_chart_overwrite(self, session_with_data,
security_manager, mocker):
- mocker.patch.object(security_manager, "can_access",
return_value=True)
- mocker.patch.object(security_manager, "can_access_chart",
return_value=True)
-
- existing = (
- session_with_data.query(Slice)
- .filter(Slice.uuid == chart_config["uuid"])
- .one_or_none()
- )
- assert existing is not None
- original_id = existing.id
- existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
- session_with_data.flush()
-
- admin = User(
- first_name="Alice",
- last_name="Doe",
- email="[email protected]",
- username="admin",
- roles=[Role(name="Admin")],
- )
-
- config = copy.deepcopy(chart_config)
- config["datasource_id"] = 1
- config["datasource_type"] = "table"
-
- with override_user(admin):
- chart = import_chart(config, overwrite=True)
+ admin, config = self._setup_chart_overwrite_context(
+ session_with_data, security_manager, chart_config, mocker
+ )
+ with override_user(admin):
+ chart = import_chart(config, overwrite=True)
@@ -369,6 +391,9 @@ class TestChartImportOverwrite:
with override_user(admin):
chart = import_chart(config, overwrite=True)
+ def test_import_chart_overwrite_with_slice_in_datasource(self,
session_with_data, security_manager, mocker):
+ admin, config = self._setup_chart_overwrite_context(
+ session_with_data, security_manager, chart_config, mocker
+ )
with override_user(admin):
chart = import_chart(config, overwrite=True)
```
</div>
</details>
</div>
<small><i>Code Review Run #7d71bd</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/es/LC_MESSAGES/messages.po:
##########
@@ -3211,6 +3211,9 @@ msgstr "Cambios en el gráfico"
msgid "Chart could not be created."
msgstr "No se ha podido crear el gráfico."
+msgid "Chart could not be restored."
+msgstr ""
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing Spanish translation</b></div>
<div id="fix">
The new translation entry at line 3214-3215 has an empty `msgstr` value,
while adjacent entries follow the same pattern with proper translations. For
consistency with `Chart could not be created.` ("No se ha podido crear el
gráfico.") and `Chart could not be updated.` ("No se ha podido actualizar el
gráfico."), this should be translated as "No se ha podido restaurar el
gráfico." — otherwise Spanish-speaking users will see the untranslated English
error message for the restore action.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/translations/es/LC_MESSAGES/messages.po
+++ superset/translations/es/LC_MESSAGES/messages.po
@@ -3212,7 +3212,7 @@
msgstr "No se ha podido crear el gráfico."
msgid "Chart could not be restored."
-msgstr ""
+msgstr "No se ha podido restaurar el gráfico."
msgid "Chart could not be updated."
msgstr "No se ha podido actualizar el gráfico."
```
</div>
</details>
</div>
<small><i>Code Review Run #7d71bd</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/zh/LC_MESSAGES/messages.po:
##########
@@ -3024,6 +3024,9 @@ msgstr "图表变化"
msgid "Chart could not be created."
msgstr "您的图表无法创建。"
+msgid "Chart could not be restored."
+msgstr ""
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing Chinese translation</b></div>
<div id="fix">
The Chinese translation for 'Chart could not be restored.' is empty. Chinese
users will see untranslated English text. Following the established pattern
(e.g., '您的图表无法创建。' for 'Chart could not be created.'), the translation should
be '您的图表无法恢复。'
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/translations/zh/LC_MESSAGES/messages.po (lines 3027-3028)
+++ superset/translations/zh/LC_MESSAGES/messages.po (lines 3027-3028)
@@ -3025,6 +3025,6 @@ msgstr "您的图表无法创建。"
msgid "Chart could not be restored."
-msgstr ""
+msgstr "您的图表无法恢复。"
msgid "Chart could not be updated."
```
</div>
</details>
</div>
<small><i>Code Review Run #7d71bd</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]
