gabotorresruiz commented on code in PR #45036:
URL: https://github.com/apache/superset/pull/45036#discussion_r4221017607
##########
superset/models/dashboard.py:
##########
@@ -70,26 +69,41 @@
logger = logging.getLogger(__name__)
-def copy_dashboard(_mapper: Mapper, _connection: Connection, target:
Dashboard) -> None:
- dashboard_id = app.config["DASHBOARD_TEMPLATE_ID"]
- if dashboard_id is None:
- return
+def _copy_dashboard_for_user(
+ session: Session,
+ target: Any,
+ dashboard_id: int | str,
+) -> None:
+ """Copy the configured template dashboard to a newly created user.
+ Clones the template dashboard metadata and slices, associates the new user
+ as an editor and their groups as viewers, and configures the cloned
dashboard
+ as the user's welcome dashboard.
+
+ :param session: The active SQLAlchemy session to use for queries and
persistence.
+ :param target: The newly created user instance.
+ :param dashboard_id: The ID of the template dashboard to copy.
+ """
+ from superset.subjects.models import Subject
from superset.subjects.utils import (
get_default_viewers_for_groups,
get_user_subject,
)
- session = sqla.inspect(target).session # pylint: disable=disallowed-name
- new_user = session.query(User).filter_by(id=target.id).first()
+ def _rebind(subjects: list[Subject | None]) -> list[Subject]:
Review Comment:
`pre-commit` fails here: `list` is invariant, so the `viewers` call below
cannot pass its `list[Subject]` into a `list[Subject | None]` parameter. mypy's
own note suggests `Sequence`:
```suggestion
def _rebind(subjects: Sequence[Subject | None]) -> list[Subject]:
```
with `from collections.abc import Sequence` added at the top of the module.
##########
tests/unit_tests/subjects/test_creator_group_call_sites.py:
##########
@@ -202,7 +202,7 @@ def _capture(**kwargs):
dashboard_module.copy_dashboard(
MagicMock(),
MagicMock(),
- SimpleNamespace(id=5), # type: ignore[arg-type]
+ user, # type: ignore[arg-type]
Review Comment:
The other mypy error: this ignore is unused now that `copy_dashboard` takes
`target: Any`.
```suggestion
user,
```
##########
superset/models/dashboard.py:
##########
@@ -70,26 +69,41 @@
logger = logging.getLogger(__name__)
-def copy_dashboard(_mapper: Mapper, _connection: Connection, target:
Dashboard) -> None:
- dashboard_id = app.config["DASHBOARD_TEMPLATE_ID"]
- if dashboard_id is None:
- return
+def _copy_dashboard_for_user(
+ session: Session,
+ target: Any,
+ dashboard_id: int | str,
+) -> None:
+ """Copy the configured template dashboard to a newly created user.
+ Clones the template dashboard metadata and slices, associates the new user
+ as an editor and their groups as viewers, and configures the cloned
dashboard
+ as the user's welcome dashboard.
+
+ :param session: The active SQLAlchemy session to use for queries and
persistence.
+ :param target: The newly created user instance.
+ :param dashboard_id: The ID of the template dashboard to copy.
+ """
+ from superset.subjects.models import Subject
from superset.subjects.utils import (
get_default_viewers_for_groups,
get_user_subject,
)
- session = sqla.inspect(target).session # pylint: disable=disallowed-name
- new_user = session.query(User).filter_by(id=target.id).first()
+ def _rebind(subjects: list[Subject | None]) -> list[Subject]:
+ """The helpers resolve subjects on ``db.session``; a persistent object
+ cannot be cascaded into a second session."""
+ ids = [s.id for s in subjects if s is not None]
+ return session.query(Subject).filter(Subject.id.in_(ids)).all() if ids
else []
- # copy template dashboard to user
template = session.query(Dashboard).filter_by(id=int(dashboard_id)).first()
- editors = []
- if new_user:
- subj = get_user_subject(new_user.id)
- if subj:
- editors.append(subj)
+ if not template:
+ return
+
+ editors = _rebind([get_user_subject(target.id)])
Review Comment:
Not a blocker, and it predates this PR: on a real insert this comes out
`[]`, because the user's `Subject` row is written after the user itself, so
`get_user_subject(target.id)` finds nothing at `after_insert` time. Did you
ever get a non empty `editors` here? If not, maybe worth a follow up issue
rather than keeping the lookup.
##########
tests/unit_tests/models/dashboard_test.py:
##########
@@ -737,3 +740,191 @@ def
test_datasets_trimmed_for_slices_keeps_colliding_ids_separate() -> None:
# with the exact slice list (no semantic-view slice leaking into it).
assert result == [(table_datasource, {"cols": ["column"]})]
table_datasource.data_for_slices.assert_called_once_with([sesh_table_slice])
+
+
+def test_custom_user_model_dashboard_copy_listener(app_context: None) -> None:
+ """Ensure dashboard copy events can be registered dynamically
+ for custom user models.
+ """
+ from superset.models.dashboard import register_dashboard_copy_events
+
+ assert callable(register_dashboard_copy_events)
+
+
+class CustomUserModel(User):
+ """Custom user model subclass for testing dynamic security manager
models."""
+
+ __tablename__ = "ab_user"
+ custom_field = "custom_value"
+
+
[email protected]
+def custom_user_model() -> type[CustomUserModel]:
+ """Fixture providing a mock CustomUserModel class."""
+ return CustomUserModel
+
+
[email protected]
+def mock_dashboard_template() -> Dashboard:
+ """Fixture providing a mock template Dashboard instance."""
+ dash = Dashboard()
+ dash.id = 100
+ dash.dashboard_title = "Template Dashboard"
+ dash.position_json = "{}"
+ dash.description = "Template description"
+ dash.css = ""
+ dash.json_metadata = "{}"
+ dash.slices = []
+ return dash
+
+
+def test_register_dashboard_copy_events_dynamic_and_idempotent(
+ custom_user_model: type[CustomUserModel],
+) -> None:
+ """Ensure dynamic listener registration attaches to custom user model
+ and is strictly idempotent.
+ """
+ from superset.models.dashboard import (
+ copy_dashboard,
+ register_dashboard_copy_events,
+ )
+
+ register_dashboard_copy_events(custom_user_model)
+ assert sqla.event.contains(custom_user_model, "after_insert",
copy_dashboard)
+
+ # Calling a second time should not register duplicate listeners
+ register_dashboard_copy_events(custom_user_model)
+ assert sqla.event.contains(custom_user_model, "after_insert",
copy_dashboard)
+
+
+def test_copy_dashboard_early_return_when_no_template_configured(
+ app_context: None,
+) -> None:
+ """Ensure copy_dashboard returns early without query when no template
+ is configured.
+ """
+ from superset.models.dashboard import copy_dashboard
+
+ mock_connection = Mock()
+ mock_target = Mock()
+ with patch.dict(current_app.config, {"DASHBOARD_TEMPLATE_ID": None}):
+ copy_dashboard(Mock(), mock_connection, mock_target)
+
+ mock_connection.execute.assert_not_called()
+
+
+def test_copy_dashboard_missing_template_returns_early(
+ app_context: None,
+) -> None:
+ """Ensure copy_dashboard exits gracefully if template dashboard is not
found."""
+ from superset.models.dashboard import copy_dashboard
+
+ mock_session = Mock()
+ mock_session.query.return_value.filter_by.return_value.first.return_value
= None
+
+ target = Mock(id=42, groups=[])
+ with (
+ patch.dict(current_app.config, {"DASHBOARD_TEMPLATE_ID": 999}),
+ patch(
+ "superset.models.dashboard.Session",
+ return_value=mock_session,
+ ),
+ ):
+ mock_session.__enter__ = Mock(return_value=mock_session)
+ mock_session.__exit__ = Mock(return_value=None)
+ copy_dashboard(Mock(), Mock(), target)
+
+ mock_session.add.assert_not_called()
+ mock_session.commit.assert_not_called()
+
+
+def test_copy_dashboard_with_custom_user_model_in_isolated_session(
+ app_context: None,
+ mock_dashboard_template: Dashboard,
+) -> None:
+ """Ensure copy_dashboard copies the template dashboard for custom user
model
+ using an isolated session without SQLAlchemy FlushError.
+ """
+ from superset.models.dashboard import copy_dashboard
+
+ target = Mock(id=55, groups=[])
+
+ mock_session = Mock()
+ mock_session.query.return_value.filter_by.return_value.first.return_value
= (
+ mock_dashboard_template
+ )
+ mock_session.__enter__ = Mock(return_value=mock_session)
+ mock_session.__exit__ = Mock(return_value=None)
+
+ added_objects: list[Any] = []
+ mock_session.add.side_effect = lambda obj: added_objects.append(obj)
+
+ with (
+ patch.dict(current_app.config, {"DASHBOARD_TEMPLATE_ID": 100}),
+ patch(
+ "superset.models.dashboard.Session",
+ return_value=mock_session,
+ ),
+ patch("superset.subjects.utils.get_user_subject", return_value=None),
+ patch(
+ "superset.subjects.utils.get_default_viewers_for_groups",
+ return_value=[],
+ ),
+ ):
+ copy_dashboard(Mock(), Mock(), target)
+
+ mock_session.flush.assert_called_once()
+ mock_session.commit.assert_called_once()
+ assert len(added_objects) == 2
+ cloned_dashboard = added_objects[0]
+ extra_attributes = added_objects[1]
+ assert cloned_dashboard.dashboard_title == "Template Dashboard"
+ assert extra_attributes.user_id == 55
+
+
+def test_copy_dashboard_rebinds_subjects_in_isolated_session(
Review Comment:
This one does pin `_rebind`: drop it from `_copy_dashboard_for_user` and
this is the test that fails, so thank you for adding it.
Non blocking, but all six new tests fail on master only because `Session`
and `register_dashboard_copy_events` do not exist there yet, so none of them
would catch the cross-session bug coming back: a `Mock` session cannot raise
`is already attached to session`. The shape that can is a real one, a `Group`
with a `Subject` row, a user inserted carrying that group,
`DASHBOARD_TEMPLATE_ID` pointing at a real dashboard, then assert the copy's
`viewers` is that group's subject. Happy to share the harness I used if it
helps.
--
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]
