You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/10/21 19:42:06 UTC
(superset) branch master updated: fix(dashboards): update last modified timestamp when adding chart to dashboard (#44305) (#44358)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 3de22376c5c fix(dashboards): update last modified timestamp when adding chart to dashboard (#44305) (#44358)
3de22376c5c is described below
commit 3de22376c5ca3cac394d7515158823b5e554ff4d
Author: Francesco Castaldi <in...@francescocastaldi.it>
AuthorDate: Wed Oct 21 21:41:58 2026 +0200
fix(dashboards): update last modified timestamp when adding chart to dashboard (#44305) (#44358)
Co-authored-by: Evan Rusackas <ev...@preset.io>
Co-authored-by: Claude Fable 5.1 <no...@anthropic.com>
Co-authored-by: Joe Li <jo...@preset.io>
---
superset/commands/chart/create.py | 4 +
superset/commands/chart/update.py | 19 ++
superset/commands/chart/utils.py | 39 ++++
tests/unit_tests/commands/chart/create_test.py | 252 ++++++++++++++++++++++-
tests/unit_tests/commands/chart/update_test.py | 266 ++++++++++++++++++++++++-
5 files changed, 578 insertions(+), 2 deletions(-)
diff --git a/superset/commands/chart/create.py b/superset/commands/chart/create.py
index 467c380b265..ecdab6fb44d 100644
--- a/superset/commands/chart/create.py
+++ b/superset/commands/chart/create.py
@@ -33,6 +33,7 @@ from superset.commands.chart.exceptions import (
DashboardsNotFoundValidationError,
)
from superset.commands.chart.utils import (
+ touch_dashboards,
validate_chart_datasource_type,
validate_query_context_datasource,
)
@@ -63,6 +64,9 @@ class CreateChartCommand(CreateMixin, BaseCommand):
self._properties["last_saved_at"] = datetime.now()
self._properties["last_saved_by"] = g.user
chart = ChartDAO.create(attributes=self._properties)
+ # Touch attached dashboards to bump changed_on/changed_by (resolves #44305).
+ # Ensures adding a chart updates the dashboard's last modified state.
+ touch_dashboards(self._properties.get("dashboards"))
if after_create := current_app.config.get("AFTER_ASSET_CREATE"):
after_create(chart, "chart")
return chart
diff --git a/superset/commands/chart/update.py b/superset/commands/chart/update.py
index 4af4abd0a47..5b67fdd58aa 100644
--- a/superset/commands/chart/update.py
+++ b/superset/commands/chart/update.py
@@ -35,6 +35,7 @@ from superset.commands.chart.exceptions import (
DatasourceTypeUpdateRequiredValidationError,
)
from superset.commands.chart.utils import (
+ touch_dashboards,
validate_chart_datasource_type,
validate_query_context_datasource,
)
@@ -105,6 +106,24 @@ class UpdateChartCommand(UpdateMixin, BaseCommand):
self._properties["params"],
)
+ # Touch dashboards whose links to this chart changed to bump their audit
+ # metadata (resolves #44305). Dashboards the chart was added to and
+ # removed from are both affected, so their last modified state stays
+ # accurate in either direction.
+ if "dashboards" in self._properties:
+ existing_dashboard_ids = {d.id for d in self._model.dashboards}
+ requested_dashboard_ids = {d.id for d in self._properties["dashboards"]}
+ affected_dashboards = [
+ dashboard
+ for dashboard in self._model.dashboards
+ if dashboard.id not in requested_dashboard_ids
+ ] + [
+ dashboard
+ for dashboard in self._properties["dashboards"]
+ if dashboard.id not in existing_dashboard_ids
+ ]
+ touch_dashboards(affected_dashboards)
+
return ChartDAO.update(self._model, self._properties)
def _validate_new_dashboard_access(
diff --git a/superset/commands/chart/utils.py b/superset/commands/chart/utils.py
index 5035327f045..e1a79501718 100644
--- a/superset/commands/chart/utils.py
+++ b/superset/commands/chart/utils.py
@@ -14,8 +14,11 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from contextlib import suppress
+from datetime import datetime
from typing import Any
+from flask import g
from marshmallow import ValidationError
from superset.commands.chart.exceptions import (
@@ -90,3 +93,39 @@ def validate_query_context_datasource(
if not ids_match or not types_match:
exceptions.append(ChartQueryContextDatasourceMismatchValidationError())
+
+
+def touch_dashboards(dashboards: list[Any] | None) -> None:
+ """
+ Touch the audit metadata (changed_on and changed_by) for the given dashboards.
+
+ When a chart's links to dashboards change during chart creation or update
+ (dashboards added to or removed from the chart), this ensures the dashboards'
+ last modified timestamps reflect the change (issue #44305).
+
+ In no-user contexts (such as Celery tasks, CLI commands, or programmatic
+ updates), the pre-existing changed_by / changed_by_fk attribution is explicitly
+ preserved so that SQLAlchemy's column onupdate generator (onupdate=get_user_id)
+ does not overwrite it with None.
+
+ :param dashboards: list of Dashboard models to touch.
+ """
+ if not dashboards:
+ return
+ now = datetime.now()
+ user = getattr(g, "user", None)
+ for dashboard in dashboards:
+ prior_changed_by_fk = getattr(dashboard, "changed_by_fk", None)
+ prior_changed_by = getattr(dashboard, "changed_by", None)
+ dashboard.changed_on = now
+ if user is not None:
+ dashboard.changed_by = user
+ else:
+ if prior_changed_by_fk is not None:
+ dashboard.changed_by_fk = prior_changed_by_fk
+ with suppress(Exception):
+ from sqlalchemy.orm.attributes import flag_modified
+
+ flag_modified(dashboard, "changed_by_fk")
+ elif prior_changed_by is not None:
+ dashboard.changed_by = prior_changed_by
diff --git a/tests/unit_tests/commands/chart/create_test.py b/tests/unit_tests/commands/chart/create_test.py
index 64732c76312..b0c059b2210 100644
--- a/tests/unit_tests/commands/chart/create_test.py
+++ b/tests/unit_tests/commands/chart/create_test.py
@@ -21,9 +21,11 @@ objects must be rejected before datasource lookup, not fail with an opaque
500. Table and semantic-view datasources are supported chart sources.
"""
-from unittest.mock import Mock
+from datetime import datetime
+from unittest.mock import MagicMock, Mock
import pytest
+from flask import g
from pytest_mock import MockerFixture
from superset.commands.chart.create import CreateChartCommand
@@ -302,3 +304,251 @@ def test_create_chart_query_context_without_datasource_is_allowed(
_mock_table_datasource(mocker)
CreateChartCommand(_create_payload(query_context)).validate()
+
+
+def test_create_chart_updates_dashboard_changed_on(mocker: MockerFixture) -> None:
+ """Issue #44305: Creating a chart linked to dashboards must touch audit metadata."""
+ _mock_table_datasource(mocker)
+ user = MagicMock()
+ g.user = user
+
+ seeded_changed_on: datetime = datetime(2020, 1, 1)
+ dashboard = MagicMock(
+ is_managed_externally=False, changed_on=seeded_changed_on, changed_by=None
+ )
+ mocker.patch(
+ "superset.commands.chart.create.DashboardDAO.find_by_ids",
+ return_value=[dashboard],
+ )
+ mocker.patch(
+ "superset.commands.chart.create.security_manager.is_editor",
+ return_value=True,
+ )
+ mocker.patch(
+ "superset.commands.chart.create.ChartDAO.create",
+ return_value=MagicMock(),
+ )
+
+ cmd = CreateChartCommand(
+ {
+ "datasource_id": 42,
+ "datasource_type": "table",
+ "slice_name": "New Chart",
+ "viz_type": "table",
+ "dashboards": [101],
+ }
+ )
+ cmd.run()
+
+ assert dashboard.changed_on > seeded_changed_on
+ assert dashboard.changed_by == user
+
+
+def test_create_chart_updates_multiple_dashboards_changed_on(
+ mocker: MockerFixture,
+) -> None:
+ """Ensure all dashboards linked to the newly created chart get touched."""
+ _mock_table_datasource(mocker)
+ user = MagicMock()
+ g.user = user
+
+ seeded_changed_on: datetime = datetime(2020, 1, 1)
+ d1 = MagicMock(
+ is_managed_externally=False, changed_on=seeded_changed_on, changed_by=None
+ )
+ d2 = MagicMock(
+ is_managed_externally=False, changed_on=seeded_changed_on, changed_by=None
+ )
+ mocker.patch(
+ "superset.commands.chart.create.DashboardDAO.find_by_ids",
+ return_value=[d1, d2],
+ )
+ mocker.patch(
+ "superset.commands.chart.create.security_manager.is_editor",
+ return_value=True,
+ )
+ mocker.patch(
+ "superset.commands.chart.create.ChartDAO.create",
+ return_value=MagicMock(),
+ )
+
+ cmd = CreateChartCommand(
+ {
+ "datasource_id": 42,
+ "datasource_type": "table",
+ "slice_name": "Multi-Dash Chart",
+ "viz_type": "table",
+ "dashboards": [101, 102],
+ }
+ )
+ cmd.run()
+
+ assert d1.changed_on > seeded_changed_on
+ assert d1.changed_by == user
+ assert d2.changed_on > seeded_changed_on
+ assert d2.changed_by == user
+
+
+def test_create_chart_updates_dashboard_changed_on_without_user_preserves_attribution(
+ mocker: MockerFixture,
+) -> None:
+ """Issue #44305: In no-user contexts, audit metadata attribution is preserved."""
+ _mock_table_datasource(mocker)
+ g.user = None
+
+ seeded_changed_on: datetime = datetime(2020, 1, 1)
+ prior_user = MagicMock()
+ dashboard = MagicMock(
+ is_managed_externally=False,
+ changed_on=seeded_changed_on,
+ changed_by=prior_user,
+ changed_by_fk=42,
+ )
+ mocker.patch(
+ "superset.commands.chart.create.DashboardDAO.find_by_ids",
+ return_value=[dashboard],
+ )
+ mocker.patch(
+ "superset.commands.chart.create.security_manager.is_editor",
+ return_value=True,
+ )
+ mocker.patch(
+ "superset.commands.chart.create.ChartDAO.create",
+ return_value=MagicMock(),
+ )
+
+ cmd = CreateChartCommand(
+ {
+ "datasource_id": 42,
+ "datasource_type": "table",
+ "slice_name": "New Chart Without User",
+ "viz_type": "table",
+ "dashboards": [101],
+ }
+ )
+ cmd.run()
+
+ assert dashboard.changed_on > seeded_changed_on
+ assert dashboard.changed_by == prior_user
+ assert dashboard.changed_by_fk == 42
+
+
+def test_create_chart_without_dashboards_runs_cleanly(
+ mocker: MockerFixture,
+) -> None:
+ """Creating a chart with no attached dashboards runs smoothly without error."""
+ _mock_table_datasource(mocker)
+ g.user = MagicMock()
+ create_mock = mocker.patch(
+ "superset.commands.chart.create.ChartDAO.create",
+ return_value=MagicMock(),
+ )
+
+ cmd = CreateChartCommand(
+ {
+ "datasource_id": 42,
+ "datasource_type": "table",
+ "slice_name": "Standalone Chart",
+ "viz_type": "table",
+ }
+ )
+ chart = cmd.run()
+ create_mock.assert_called_once()
+ assert chart == create_mock.return_value
+
+
+def test_touch_dashboards_with_no_user() -> None:
+ """When g.user is None, changed_on is updated while changed_by remains untouched."""
+ from superset.commands.chart.utils import touch_dashboards
+
+ seeded_changed_on: datetime = datetime(2020, 1, 1)
+ dashboard = MagicMock(changed_on=seeded_changed_on, changed_by=None)
+ g.user = None
+
+ touch_dashboards([dashboard])
+
+ assert dashboard.changed_on > seeded_changed_on
+ assert dashboard.changed_by is None
+
+
+def test_touch_dashboards_with_no_user_preserves_existing_attribution() -> None:
+ """When g.user is None, changed_on is updated while pre-existing
+ attribution is preserved.
+ """
+ from superset.commands.chart.utils import touch_dashboards
+
+ seeded_changed_on: datetime = datetime(2020, 1, 1)
+ prior_user = MagicMock()
+ dashboard = MagicMock(
+ changed_on=seeded_changed_on,
+ changed_by=prior_user,
+ changed_by_fk=42,
+ )
+ g.user = None
+
+ touch_dashboards([dashboard])
+
+ assert dashboard.changed_on > seeded_changed_on
+ assert dashboard.changed_by == prior_user
+ assert dashboard.changed_by_fk == 42
+
+
+def test_touch_dashboards_persisted_model_preserves_audit() -> None:
+ """Issue #44305: Ensure SQLAlchemy onupdate=get_user_id does not clear
+ changed_by_fk on a real persisted model in no-user contexts.
+ """
+ from typing import Any
+
+ import sqlalchemy as sa
+ from sqlalchemy.orm import declarative_base, relationship, sessionmaker
+
+ from superset.commands.chart.utils import touch_dashboards
+
+ base: Any = declarative_base()
+
+ def mock_get_user_id() -> int | None:
+ return None
+
+ class User(base):
+ __tablename__ = "test_user"
+ id = sa.Column(sa.Integer, primary_key=True)
+ username = sa.Column(sa.String)
+
+ class PersistedDashboard(base):
+ __tablename__ = "test_dashboard"
+ id = sa.Column(sa.Integer, primary_key=True)
+ changed_on = sa.Column(sa.DateTime, default=datetime.now, onupdate=datetime.now)
+ changed_by_fk = sa.Column(
+ sa.Integer,
+ sa.ForeignKey("test_user.id"),
+ default=mock_get_user_id,
+ onupdate=mock_get_user_id,
+ nullable=True,
+ )
+ changed_by = relationship("User", foreign_keys=[changed_by_fk])
+
+ engine = sa.create_engine("sqlite:///:memory:")
+ base.metadata.create_all(engine)
+ session_factory = sessionmaker(bind=engine)
+ session = session_factory()
+
+ user = User(id=1, username="admin")
+ session.add(user)
+ session.commit()
+
+ initial_time: datetime = datetime(2020, 1, 1)
+ d = PersistedDashboard(id=10, changed_on=initial_time, changed_by=user)
+ session.add(d)
+ session.commit()
+
+ assert d.changed_by_fk == 1
+ assert d.changed_by.username == "admin"
+
+ g.user = None
+ touch_dashboards([d])
+ session.commit()
+
+ assert d.changed_on > initial_time
+ assert d.changed_by_fk == 1
+ assert d.changed_by is not None
+ assert d.changed_by.username == "admin"
diff --git a/tests/unit_tests/commands/chart/update_test.py b/tests/unit_tests/commands/chart/update_test.py
index d21a900b3ba..7658a952498 100644
--- a/tests/unit_tests/commands/chart/update_test.py
+++ b/tests/unit_tests/commands/chart/update_test.py
@@ -14,7 +14,9 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from unittest.mock import Mock
+from datetime import datetime
+from typing import Any
+from unittest.mock import MagicMock, Mock
import pytest
from flask import g
@@ -546,3 +548,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",
+ )
+ 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,
+ )
+
+ g.user = None
+
+ # 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 == prior_user
+ assert d1.changed_by_fk == 1
+ # The dashboard the chart was removed from is touched and attribution is preserved
+ assert d2.changed_on > seeded_changed_on
+ assert d2.changed_by == prior_user
+ assert d2.changed_by_fk == 2