bito-code-review[bot] commented on code in PR #44358:
URL: https://github.com/apache/superset/pull/44358#discussion_r4167737326
##########
tests/unit_tests/commands/chart/update_test.py:
##########
@@ -437,3 +440,265 @@ def
test_update_chart_rejects_type_only_non_table_datasource(
isinstance(ex, DatasourceTypeInvalidError) for ex in
exc_info.value._exceptions
)
get_datasource_by_id.assert_not_called()
+
+
+def test_update_chart_touches_newly_linked_dashboards(
+ mocker: MockerFixture,
+) -> None:
+ """Issue #44305: Adding new dashboards to a chart touches audit
metadata."""
+ seeded_changed_on: datetime = datetime(2020, 1, 1)
+ existing_dashboard = MagicMock(id=1, changed_on=seeded_changed_on,
changed_by=None)
+ new_dashboard = MagicMock(
+ id=2,
+ is_managed_externally=False,
+ changed_on=seeded_changed_on,
+ changed_by=None,
+ )
+
+ chart = MagicMock(
+ is_managed_externally=False,
+ id=10,
+ tags=[],
+ dashboards=[existing_dashboard],
+ datasource_id=42,
+ datasource_type="table",
+ )
+ mocker.patch(
+ "superset.commands.chart.update.ChartDAO.find_by_id",
+ return_value=chart,
+ )
+
+ def _find_dashboards_by_ids(ids: list[int], **kwargs: Any) -> list[Any]:
+ lookup = {1: existing_dashboard, 2: new_dashboard}
+ return [lookup[i] for i in ids if i in lookup]
+
+ mocker.patch(
+ "superset.commands.chart.update.DashboardDAO.find_by_ids",
+ side_effect=_find_dashboards_by_ids,
+ )
+ mocker.patch(
+ "superset.commands.chart.update.security_manager.raise_for_editorship",
+ )
+ mocker.patch(
+ "superset.commands.chart.update.security_manager.is_editor",
+ return_value=True,
+ )
+ mocker.patch(
+ "superset.commands.chart.update.compute_subjects",
+ side_effect=lambda model, properties, exceptions: None,
+ )
+ mocker.patch(
+ "superset.commands.chart.update.ChartDAO.update",
+ return_value=chart,
+ )
+
+ user = MagicMock()
+ g.user = user
+
+ cmd = UpdateChartCommand(10, {"dashboards": [1, 2]})
+ cmd.run()
+
+ # The existing dashboard should NOT be touched
+ assert existing_dashboard.changed_on == seeded_changed_on
+ assert existing_dashboard.changed_by is None
+ # The newly linked dashboard MUST be touched
+ assert new_dashboard.changed_on > seeded_changed_on
+ assert new_dashboard.changed_by == user
+
+
+def test_update_chart_does_not_touch_unchanged_dashboards(
+ mocker: MockerFixture,
+) -> None:
+ """Updating a chart without changing dashboard attachments does not touch
them."""
+ seeded_changed_on: datetime = datetime(2020, 1, 1)
+ existing_dashboard = MagicMock(id=1, changed_on=seeded_changed_on,
changed_by=None)
+
+ chart = MagicMock(
+ is_managed_externally=False,
+ id=10,
+ tags=[],
+ dashboards=[existing_dashboard],
+ datasource_id=42,
+ datasource_type="table",
+ )
+ mocker.patch(
+ "superset.commands.chart.update.ChartDAO.find_by_id",
+ return_value=chart,
+ )
+ mocker.patch(
+ "superset.commands.chart.update.DashboardDAO.find_by_ids",
+ return_value=[existing_dashboard],
+ )
+ mocker.patch(
+ "superset.commands.chart.update.security_manager.raise_for_editorship",
+ )
+ mocker.patch(
+ "superset.commands.chart.update.compute_subjects",
+ side_effect=lambda model, properties, exceptions: None,
+ )
+ mocker.patch(
+ "superset.commands.chart.update.ChartDAO.update",
+ return_value=chart,
+ )
+
+ user = MagicMock()
+ g.user = user
+
+ cmd = UpdateChartCommand(10, {"dashboards": [1]})
+ cmd.run()
+
+ # The unchanged dashboard should remain untouched
+ assert existing_dashboard.changed_on == seeded_changed_on
+ assert existing_dashboard.changed_by is None
+
+
+def test_update_chart_touches_dashboards_when_removed(
+ mocker: MockerFixture,
+) -> None:
+ """Issue #44305: Removing a chart from a dashboard touches that
dashboard."""
+ seeded_changed_on: datetime = datetime(2020, 1, 1)
+ d1 = MagicMock(id=1, changed_on=seeded_changed_on, changed_by=None)
+ d2 = MagicMock(id=2, changed_on=seeded_changed_on, changed_by=None)
+
+ chart = MagicMock(
+ is_managed_externally=False,
+ id=10,
+ tags=[],
+ dashboards=[d1, d2],
+ datasource_id=42,
+ datasource_type="table",
+ )
+ mocker.patch(
+ "superset.commands.chart.update.ChartDAO.find_by_id",
+ return_value=chart,
+ )
+ mocker.patch(
+ "superset.commands.chart.update.DashboardDAO.find_by_ids",
+ return_value=[d1],
+ )
+ mocker.patch(
+ "superset.commands.chart.update.security_manager.raise_for_editorship",
+ )
+ mocker.patch(
+ "superset.commands.chart.update.compute_subjects",
+ side_effect=lambda model, properties, exceptions: None,
+ )
+ mocker.patch(
+ "superset.commands.chart.update.ChartDAO.update",
+ return_value=chart,
+ )
+
+ user = MagicMock()
+ g.user = user
+
+ # Keep only d1, removing d2
+ cmd = UpdateChartCommand(10, {"dashboards": [1]})
+ cmd.run()
+
+ # The dashboard that keeps the chart is untouched
+ assert d1.changed_on == seeded_changed_on
+ assert d1.changed_by is None
+ # The dashboard the chart was removed from is touched
+ assert d2.changed_on > seeded_changed_on
+ assert d2.changed_by == user
+
+
+def
test_update_chart_without_dashboards_in_payload_leaves_dashboards_untouched(
+ mocker: MockerFixture,
+) -> None:
+ """Updates that do not alter dashboard links do not evaluate dashboard
touches."""
+ seeded_changed_on: datetime = datetime(2020, 1, 1)
+ dashboard = MagicMock(id=1, changed_on=seeded_changed_on, changed_by=None)
+ chart = MagicMock(
+ is_managed_externally=False,
+ id=10,
+ tags=[],
+ dashboards=[dashboard],
+ datasource_id=42,
+ datasource_type="table",
+ )
+ mocker.patch(
+ "superset.commands.chart.update.ChartDAO.find_by_id",
+ return_value=chart,
+ )
+ mocker.patch(
+ "superset.commands.chart.update.security_manager.raise_for_editorship",
+ )
+ mocker.patch(
+ "superset.commands.chart.update.compute_subjects",
+ side_effect=lambda model, properties, exceptions: None,
+ )
+ mocker.patch(
+ "superset.commands.chart.update.ChartDAO.update",
+ return_value=chart,
+ )
+
+ g.user = MagicMock()
+ cmd = UpdateChartCommand(10, {"slice_name": "Renamed only"})
+ cmd.run()
+
+ assert dashboard.changed_on == seeded_changed_on
+ assert dashboard.changed_by is None
+
+
+def test_update_chart_touches_dashboards_when_removed_in_no_user_context(
+ mocker: MockerFixture,
+) -> None:
+ """Issue #44305: Removing a chart without active user preserves audit
metadata."""
+ seeded_changed_on: datetime = datetime(2020, 1, 1)
+ prior_user = MagicMock()
+ d1 = MagicMock(
+ id=1,
+ changed_on=seeded_changed_on,
+ changed_by=prior_user,
+ changed_by_fk=1,
+ )
+ d2 = MagicMock(
+ id=2,
+ changed_on=seeded_changed_on,
+ changed_by=prior_user,
+ changed_by_fk=2,
+ )
+
+ chart = MagicMock(
+ is_managed_externally=False,
+ id=10,
+ tags=[],
+ dashboards=[d1, d2],
+ datasource_id=42,
+ datasource_type="table",
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unannotated mock variables</b></div>
<div id="fix">
BITO.md adaptive rule 12787 requires explicit type annotations on all mock
variables in test files. The new test annotates `seeded_changed_on` but leaves
`prior_user`, `d1`, `d2`, and `chart` unannotated, unlike the annotated mocks
already in this file (e.g. `chart: Mock` at line 48). Add `: MagicMock` to each
declaration.
</div>
</div>
<small><i>Code Review Run #0caab4</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]
