bito-code-review[bot] commented on code in PR #44560:
URL: https://github.com/apache/superset/pull/44560#discussion_r4117202075
##########
superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py:
##########
@@ -35,6 +35,9 @@
from superset.commands.exceptions import CommandException, ForbiddenError
from superset.extensions import event_logger
+from superset.mcp_service.dashboard.layout_placement import (
+ _remove_component_and_prune,
+)
Review Comment:
<!-- Bito Reply -->
The suggestion provided by the reviewer is appropriate and improves the code
by establishing a stable, public contract for the shared helper function. By
exposing the helper with a public name in the module, you avoid relying on
underscore-private symbols, which makes the code more maintainable and
resilient to future refactoring. Applying this change is recommended.
##########
superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:
##########
@@ -36,6 +35,12 @@
GRID_COLUMN_COUNT,
GRID_DEFAULT_CHART_WIDTH,
)
+from superset.mcp_service.dashboard.layout_placement import (
+ _collect_available_tab_names,
+ _ensure_layout_structure,
+ _find_next_row_position,
+ _find_tab_insert_target,
+)
Review Comment:
<!-- Bito Reply -->
The suggestion to rename the private-by-convention helpers to public names
is a valid improvement for maintaining a stable, intentional API surface in
`layout_placement`. As the user noted, this change has already been
implemented, and the helpers are now exposed with public names, which aligns
with the reviewer's recommendation to improve module encapsulation and follow
standard naming conventions.
##########
tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_markdown.py:
##########
@@ -0,0 +1,1099 @@
+# 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.
+
+"""
+Unit tests for the manage_dashboard_markdown MCP tool.
+
+Follows the pattern from test_manage_native_filters.py:
+- Tests run through the async MCP Client (not direct function calls)
+- auth/editorship are mocked via the directory's autouse mock_auth fixture
+ (tests/unit_tests/mcp_service/dashboard/tool/conftest.py)
+- Patches applied at source locations (superset.daos.dashboard.*, etc.)
+
+Covers:
+- Adding markdown/header/divider components to the default grid and to a tab
+- Updating an existing component (type-specific field rejection)
+- Removing a component (including pruning the wrapper ROW a markdown tile
+ leaves behind)
+- Validation errors: unknown removal ID, update+remove conflict, duplicate
+ update IDs, malformed position_json, missing target tab
+- Header text sanitization
+- Dashboard not found / permission denied
+- "at least one operation" request validation (ToolError at the call boundary)
+"""
+
+from collections.abc import Iterator
+from typing import Any
+from unittest.mock import Mock, patch, PropertyMock
+
+import pytest
+from fastmcp import Client, FastMCP
+from fastmcp.exceptions import ToolError
+from sqlalchemy.exc import SQLAlchemyError
+
+from superset.commands.dashboard.exceptions import DashboardNotFoundError
+from superset.exceptions import SupersetSecurityException
+from superset.utils import json
+
+DAO_GET = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug"
+
+
[email protected](autouse=True)
+def mock_event_logging() -> Iterator[None]:
+ """Isolate dashboard commits from the event logger's separate audit
commits."""
+ with patch("superset.extensions.event_logger.log_context"):
+ yield
+
+
+def _empty_grid_layout() -> dict[str, Any]:
+ """Build an empty frontend-compatible grid."""
+ return {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {"type": "ROOT", "id": "ROOT_ID", "children": ["GRID_ID"]},
+ "GRID_ID": {"type": "GRID", "id": "GRID_ID", "children": []},
+ }
+
+
+def _grid_layout_with_existing_components() -> dict[str, Any]:
+ """Build a grid with each supported component type."""
+ layout = _empty_grid_layout()
+ layout["GRID_ID"]["children"] = [
+ "ROW-existing1",
+ "HEADER-existing1",
+ "DIVIDER-existing1",
+ ]
+ layout["ROW-existing1"] = {
+ "type": "ROW",
+ "id": "ROW-existing1",
+ "children": ["MARKDOWN-existing1"],
+ "meta": {"background": "BACKGROUND_TRANSPARENT"},
+ "parents": ["ROOT_ID", "GRID_ID"],
+ }
+ layout["MARKDOWN-existing1"] = {
+ "type": "MARKDOWN",
+ "id": "MARKDOWN-existing1",
+ "children": [],
+ "meta": {"code": "Hello", "width": 4, "height": 50},
+ "parents": ["ROOT_ID", "GRID_ID", "ROW-existing1"],
+ }
+ layout["HEADER-existing1"] = {
+ "type": "HEADER",
+ "id": "HEADER-existing1",
+ "children": [],
+ "meta": {
+ "text": "Old header",
+ "headerSize": "MEDIUM_HEADER",
+ "background": "BACKGROUND_TRANSPARENT",
+ },
+ "parents": ["ROOT_ID", "GRID_ID"],
+ }
+ layout["DIVIDER-existing1"] = {
+ "type": "DIVIDER",
+ "id": "DIVIDER-existing1",
+ "children": [],
+ "meta": {},
+ "parents": ["ROOT_ID", "GRID_ID"],
+ }
+ return layout
+
+
+def _tabbed_layout() -> dict[str, Any]:
+ """Build a top-level tab layout."""
+ return {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {"type": "ROOT", "id": "ROOT_ID", "children": ["TABS-1"]},
+ "TABS-1": {
+ "type": "TABS",
+ "id": "TABS-1",
+ "children": ["TAB-a", "TAB-b"],
+ "meta": {},
+ },
+ "TAB-a": {
+ "type": "TAB",
+ "id": "TAB-a",
+ "children": [],
+ "meta": {"text": "Overview"},
+ "parents": ["ROOT_ID", "TABS-1"],
+ },
+ "TAB-b": {
+ "type": "TAB",
+ "id": "TAB-b",
+ "children": [],
+ "meta": {"text": "Details"},
+ "parents": ["ROOT_ID", "TABS-1"],
+ },
+ }
+
+
+def _mock_dashboard(
+ id: int = 1,
+ layout: dict[str, Any] | None = None,
+ chart_ids: list[int] | None = None,
+ slug: str | None = None,
+) -> Mock:
+ """Build a dashboard without touching the metadata database."""
+ dashboard = Mock()
+ dashboard.id = id
+ dashboard.dashboard_title = "Test Dashboard"
+ dashboard.slug = slug
+ dashboard.position_json = json.dumps(
+ layout if layout is not None else _empty_grid_layout()
+ )
+ slices = []
+ for chart_id in chart_ids or []:
+ slc = Mock()
+ slc.id = chart_id
+ slices.append(slc)
+ dashboard.slices = slices
+ return dashboard
+
+
+async def _call(mcp_server: FastMCP, request: dict[str, Any]) -> dict[str,
Any]:
+ """Exercise validation and serialization through the MCP boundary."""
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "manage_dashboard_markdown", {"request": request}
+ )
+ return json.loads(result.content[0].text)
+
+
+# ---------------------------------------------------------------------------
+# Add
+# ---------------------------------------------------------------------------
+
+
[email protected]
+async def test_add_markdown_creates_new_row(mcp_server: FastMCP) -> None:
+ dashboard = _mock_dashboard()
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "add": [{"component_type": "markdown", "code": "**Hello**"}],
+ },
+ )
+
+ assert data["error"] is None
+ assert len(data["added_component_ids"]) == 1
+ markdown_id = data["added_component_ids"][0]
+ assert markdown_id.startswith("MARKDOWN-")
+
+ saved_layout = json.loads(dashboard.position_json)
+ markdown_node = saved_layout[markdown_id]
+ assert markdown_node["type"] == "MARKDOWN"
+ assert markdown_node["meta"] == {"code": "**Hello**", "width": 4,
"height": 50}
+
+ # Markdown tiles are wrapped in their own new ROW, not placed directly
+ # under GRID_ID.
+ row_key = next(
+ key
+ for key, node in saved_layout.items()
+ if isinstance(node, dict)
+ and node.get("type") == "ROW"
+ and markdown_id in node.get("children", [])
+ )
+ assert row_key in saved_layout["GRID_ID"]["children"]
+
+ summary = next(c for c in data["components"] if c["id"] == markdown_id)
+ assert summary["component_type"] == "markdown"
+
+
[email protected]
+async def test_add_header_placed_directly_under_grid(mcp_server: FastMCP) ->
None:
+ dashboard = _mock_dashboard()
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "add": [
+ {
+ "component_type": "header",
+ "text": "Sales",
+ "header_size": "LARGE_HEADER",
+ }
+ ],
+ },
+ )
+
+ assert data["error"] is None
+ header_id = data["added_component_ids"][0]
+ assert header_id.startswith("HEADER-")
+
+ saved_layout = json.loads(dashboard.position_json)
+ # HEADER is a full-width band: a direct child of GRID_ID, not wrapped
+ # in a ROW (ROW does not accept HEADER children).
+ assert header_id in saved_layout["GRID_ID"]["children"]
+ assert saved_layout[header_id]["meta"] == {
+ "text": "Sales",
+ "headerSize": "LARGE_HEADER",
+ "background": "BACKGROUND_TRANSPARENT",
+ }
+
+
[email protected]
+async def test_add_divider_placed_directly_under_grid(mcp_server: FastMCP) ->
None:
+ dashboard = _mock_dashboard()
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {"dashboard_id": 1, "add": [{"component_type": "divider"}]},
+ )
+
+ assert data["error"] is None
+ divider_id = data["added_component_ids"][0]
+ assert divider_id.startswith("DIVIDER-")
+
+ saved_layout = json.loads(dashboard.position_json)
+ assert divider_id in saved_layout["GRID_ID"]["children"]
+ assert saved_layout[divider_id]["meta"] == {}
+
+
[email protected]
+async def test_add_multiple_components_in_request_order(mcp_server: FastMCP)
-> None:
+ dashboard = _mock_dashboard()
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "add": [
+ {"component_type": "header", "text": "Section 1"},
+ {"component_type": "markdown", "code": "text"},
+ {"component_type": "divider"},
+ ],
+ },
+ )
+
+ assert data["error"] is None
+ assert len(data["added_component_ids"]) == 3
+ types = [c["component_type"] for c in data["components"]]
+ assert set(types) == {"header", "markdown", "divider"}
+
+
[email protected]
+async def test_add_to_target_tab_by_name(mcp_server: FastMCP) -> None:
+ dashboard = _mock_dashboard(layout=_tabbed_layout())
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "add": [
+ {
+ "component_type": "header",
+ "text": "Details header",
+ "target_tab": "Details",
+ }
+ ],
+ },
+ )
+
+ assert data["error"] is None
+ header_id = data["added_component_ids"][0]
+ saved_layout = json.loads(dashboard.position_json)
+ assert header_id in saved_layout["TAB-b"]["children"]
+ assert header_id not in saved_layout["TAB-a"]["children"]
+
+
[email protected]
+async def test_add_target_tab_not_found_lists_available_tabs(
+ mcp_server: FastMCP,
+) -> None:
+ dashboard = _mock_dashboard(layout=_tabbed_layout())
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "add": [
+ {
+ "component_type": "divider",
+ "target_tab": "Nonexistent",
+ }
+ ],
+ },
+ )
+
+ assert "Nonexistent" in data["error"]
+ assert "Overview" in data["error"]
+ assert "Details" in data["error"]
+
+
[email protected]
+async def test_add_target_tab_on_dashboard_without_tabs(mcp_server: FastMCP)
-> None:
+ dashboard = _mock_dashboard()
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "add": [{"component_type": "divider", "target_tab":
"Anything"}],
+ },
+ )
+
+ assert "no tabs" in data["error"]
Review Comment:
<!-- Bito Reply -->
The reviewer provided two actionable suggestions in the review thread for
`tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_markdown.py`:
1. **New tests missing docstrings**: The reviewer noted that four new tests
lacked docstrings documenting the scenario and expected outcome, as required by
project rules.
2. **Locals missing type annotations**: The reviewer pointed out that
several local variables (`dashboard`, `data`, `types`, `header_id`,
`saved_layout`) introduced in the tests lacked explicit type annotations, which
are required in test files.
--
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]
