bito-code-review[bot] commented on code in PR #41606: URL: https://github.com/apache/superset/pull/41606#discussion_r3589215410
########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_owners.py: ########## @@ -0,0 +1,408 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for the manage_dashboard_owners MCP tool. + +"Owners" are USER-type Subjects in the dashboard's ``editors`` list (the +Subject-based model apache/superset#38831 introduced, replacing the legacy +``owners`` relationship). +""" + +from collections.abc import Iterator +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp +from superset.subjects.types import SubjectType +from superset.utils import json + +DAO_GET = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" +GET_OR_CREATE_USER_SUBJECT = "superset.subjects.utils.get_or_create_user_subject" +POPULATE_SUBJECT_LIST = "superset.commands.utils.populate_subject_list" + + [email protected] +def mcp_server() -> object: + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + """Mock authentication for all tests in this module.""" + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + with patch("superset.security_manager.raise_for_editorship"): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_subject(id: int, user_id: int, label: str = "user") -> Mock: + subject = Mock() + subject.id = id + subject.type = SubjectType.USER + subject.user_id = user_id + subject.role_id = None + subject.label = label + subject.active = True + return subject + + +def _mock_dashboard( + id: int = 42, + title: str = "Test Dashboard", + slug: str | None = "test-slug", + editors: list[Mock] | None = None, +) -> Mock: + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = title + dashboard.slug = slug + dashboard.editors = ( + editors if editors is not None else [_mock_subject(100, 1, "admin")] + ) + return dashboard + + +class TestManageDashboardOwners: + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_add_owner( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + existing = _mock_subject(100, 1, "admin") Review Comment: <!-- Bito Reply --> Understood. Maintaining test isolation and readability by avoiding premature abstraction for small setup blocks is a reasonable approach. No further action is required for this comment. ########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py: ########## @@ -0,0 +1,283 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for the manage_dashboard_certification MCP tool.""" + +from collections.abc import Iterator +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp +from superset.utils import json + +DAO_GET = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" + + [email protected] +def mcp_server() -> object: + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + """Mock authentication for all tests in this module.""" + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + with patch("superset.security_manager.raise_for_editorship"): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_dashboard( + id: int = 42, + title: str = "Test Dashboard", + slug: str | None = "test-slug", + certified_by: str | None = None, + certification_details: str | None = None, +) -> Mock: + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = title + dashboard.slug = slug + dashboard.certified_by = certified_by + dashboard.certification_details = certification_details + return dashboard + + +class TestManageDashboardCertification: + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_set_certification( + self, mock_session: Mock, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard() + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + { + "request": { + "identifier": 42, + "certified_by": "Data Platform Team", + "certification_details": "Verified against source-of-truth.", + } + }, + ) + + assert dash.certified_by == "Data Platform Team" + assert dash.certification_details == "Verified against source-of-truth." + assert mock_session.commit.call_count >= 1 + payload = json.loads(result.content[0].text) + # Response text is wrapped for LLM context (mirrors the read-path + # sanitization on DashboardInfo.certified_by), so check substring. + assert "Data Platform Team" in payload["certified_by"] + assert set(payload["changed_fields"]) == { + "certified_by", + "certification_details", + } + + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_set_certified_by_only( + self, mock_session: Mock, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard(certification_details="Existing details") + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + {"request": {"identifier": 42, "certified_by": "Analytics Guild"}}, + ) + + assert dash.certified_by == "Analytics Guild" + assert dash.certification_details == "Existing details" + payload = json.loads(result.content[0].text) + assert payload["changed_fields"] == ["certified_by"] + + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_clear_with_empty_string( + self, mock_session: Mock, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard( + certified_by="Old Team", certification_details="Old details" + ) + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + { + "request": { + "identifier": 42, + "certified_by": "", + "certification_details": "", + } + }, + ) + + assert dash.certified_by is None + assert dash.certification_details is None + payload = json.loads(result.content[0].text) + assert payload["certified_by"] is None + assert payload["certification_details"] is None + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_no_fields_is_noop(self, mock_get: Mock, mcp_server: object) -> None: + dash = _mock_dashboard(certified_by="Existing") + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + {"request": {"identifier": 42}}, + ) + + assert dash.certified_by == "Existing" + payload = json.loads(result.content[0].text) + assert payload["changed_fields"] == [] + assert any("No fields provided" in w for w in payload["warnings"]) + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_dashboard_not_found( + self, mock_get: Mock, mcp_server: object + ) -> None: + from superset.commands.dashboard.exceptions import DashboardNotFoundError + + mock_get.side_effect = DashboardNotFoundError() + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + {"request": {"identifier": 999999, "certified_by": "Team"}}, Review Comment: <!-- Bito Reply --> The user's decision to maintain self-contained test files is reasonable given the small scope of the duplication (~20 lines per test). While extracting to a base class or utility is a common pattern for reducing boilerplate, the current approach keeps each test file independent and easy to understand, which is a valid trade-off for test maintainability. -- 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]
