FrancescoCastaldi commented on code in PR #45036:
URL: https://github.com/apache/superset/pull/45036#discussion_r4243558482
##########
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:
`_rebind` uses `Sequence[Subject | None]` to satisfy mypy invariance
requirements.
##########
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:
Agreed, will track the `editors` lookup timing as an independent item since
it predates this change and does not affect the rebind logic.
##########
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:
Cleaned up the unused `# type: ignore` in `test_creator_group_call_sites.py`
now that `copy_dashboard` accepts `target: Any`.
##########
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:
Thank you for validating the `_rebind` test and testing harness. Appreciate
the feedback and guidance.
--
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]
