bito-code-review[bot] commented on code in PR #44096:
URL: https://github.com/apache/superset/pull/44096#discussion_r4003167861
##########
tests/unit_tests/mcp_service/dashboard/test_permalink.py:
##########
@@ -372,3 +375,119 @@ def
test_lookup_dashboard_reference_unresolvable_reference(mock_get_permalink) -
assert result.permalink_key == "nonexistent"
assert result.permalink_value is None
assert result.resolved_from_permalink is False
+
+
[email protected]("denied", [False, True])
+def test_stacking_resolves_with_calling_user(app: Flask, denied: bool) -> None:
+ """The real get command checks access using the refreshed calling user."""
+ user = Mock(username="viewer", is_anonymous=False)
+ refreshed_user = Mock(username="viewer")
+ value: DashboardPermalinkValue = {
+ "dashboardId": "42",
+ "state": {
+ "dataMask": {
+ "legacy-filter": {
+ "extraFormData": {"filters": [{"col": "region", "val":
["EMEA"]}]},
+ "ownState": {"arbitrary": "preserved"},
+ }
+ }
+ },
+ }
+
+ def check_access(reference: str) -> Mock:
+ """Observe the request principal at the command's authorization
gate."""
+ assert reference == "42"
+ assert g.user is refreshed_user
+ if denied:
+ raise DashboardAccessDeniedError()
+ return Mock()
+
+ with (
+ app.test_request_context("/mcp"),
+ patch(
+
"superset.mcp_service.dashboard.permalink.load_user_with_relationships",
+ return_value=refreshed_user,
+ ) as load_user,
+ patch(
+ "superset.commands.dashboard.permalink.get."
+ "GetDashboardPermalinkCommand.salt",
+ new_callable=PropertyMock,
+ return_value="test-salt",
+ ),
+ patch(
+ "superset.commands.dashboard.permalink.get.decode_permalink_id",
+ return_value=1,
+ ),
+ patch(
+ "superset.commands.dashboard.permalink.get.KeyValueDAO.get_value",
+ return_value=value,
+ ),
+ patch(
+ "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug",
+ side_effect=check_access,
+ ) as access,
+ ):
+ g.user = user
+ if denied:
+ with pytest.raises(DashboardAccessDeniedError):
+ get_dashboard_permalink_data_mask("base-key", 42)
+ else:
+ mask = get_dashboard_permalink_data_mask("base-key", 42)
+ assert mask == value["state"]["dataMask"]
+ assert mask is not value["state"]["dataMask"]
+ load_user.assert_called_once_with(username="viewer")
+ access.assert_called_once_with("42")
+
+
[email protected]("state", [{}, {"dataMask": None}, {"dataMask": {}}])
+def test_stacking_base_without_selections(state: dict[str, object]) -> None:
+ """A valid permalink without explicit filter selections supplies an empty
mask."""
+ with (
+ patch(
+ "superset.mcp_service.dashboard.permalink."
+ "refresh_request_user_for_permalink_access"
+ ),
+ patch(
+
"superset.mcp_service.dashboard.permalink.GetDashboardPermalinkCommand"
+ ) as command,
+ ):
+ command.return_value.run.return_value = {"dashboardId": "42", "state":
state}
+ assert get_dashboard_permalink_data_mask("base-key", 42) == {}
+
+
[email protected]("application_root", ["/", "/analytics"])
[email protected]("in_request", [False, True])
+def test_build_dashboard_permalink_url(
+ app: Flask, application_root: str, in_request: bool
+) -> None:
+ """Use the configured public origin and the Flask deployment prefix."""
+ with patch.dict(
+ app.config,
+ WEBDRIVER_BASEURL_USER_FRIENDLY="https://dashboards.example.test/";,
+ APPLICATION_ROOT=application_root,
+ ):
+ expected = (
+ f"https://dashboards.example.test{application_root.rstrip('/')}"
+ "/dashboard/p/shared-key/"
+ )
+ if in_request:
+ with app.test_request_context("/mcp"):
+ assert build_dashboard_permalink_url("shared-key") == expected
+ else:
+ with app.app_context():
+ assert build_dashboard_permalink_url("shared-key") == expected
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Wrong expected URL in test</b></div>
<div id="fix">
For `in_request=True` with `application_root="/analytics"`, `get_url_path`
(in `superset/utils/urls.py`) calls `url_for` inside the request context, which
uses `SCRIPT_NAME` and ignores `APPLICATION_ROOT`. The actual URL is
`https://dashboards.example.test/dashboard/p/shared-key/`, but `expected`
includes `/analytics`, so this parametrized case fails. Move the prefix into
the `else` branch only.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
if in_request:
expected =
"https://dashboards.example.test/dashboard/p/shared-key/";
with app.test_request_context("/mcp"):
assert build_dashboard_permalink_url("shared-key") ==
expected
else:
expected = (
f"https://dashboards.example.test{application_root.rstrip('/')}"
"/dashboard/p/shared-key/"
)
with app.app_context():
assert build_dashboard_permalink_url("shared-key") ==
expected
````
</div>
</details>
</div>
<small><i>Code Review Run #1caea3</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]
