aminghadersohi commented on code in PR #45021:
URL: https://github.com/apache/superset/pull/45021#discussion_r4200922203
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -488,16 +519,23 @@ def _merge_filter_update(
existing: dict[str, Any],
dashboard_chart_ids: list[int],
) -> dict[str, Any]:
- """Merge a partial update into an existing filter config.
+ """Merge a partial update into an existing filter (or divider) config.
Returns a FULL filter config (the backend command substitutes whole
entries, it does not merge deltas).
"""
merged = copy.deepcopy(existing)
- _validate_update_type_compat(spec, merged.get("filterType"))
+ is_divider = merged.get("type") == "DIVIDER"
Review Comment:
Fixed in a79c12de227123b18cfc4aaac8325d29ec7751d0. Reused the existing
_is_divider helper in _merge_filter_update, _filter_summary, and
_extract_native_filters so prefix-only entries match the frontend and scope
handling. Regression coverage verifies updates retain title/description without
adding filter-only fields, and summaries recognize prefix-only and type-only
dividers. Dashboard unit tests: 993 passed; pre-commit passed.
##########
superset/mcp_service/app.py:
##########
@@ -134,7 +134,7 @@ def get_default_instructions(
- duplicate_dashboard: Duplicate an existing dashboard, optionally
deep-copying its charts (requires write access)
- add_chart_to_existing_dashboard: Add a chart to an existing dashboard
(requires write access)
- delete_dashboard: Delete a dashboard by ID/UUID/slug (requires editor rights
— owner or Admin; destructive; does not delete its charts; soft-deletes to
trash when the SOFT_DELETE feature flag is on, permanent otherwise)
-- manage_native_filters: Add, update, remove, or reorder native filters on a
dashboard (requires write access; supports filter_select, filter_time,
filter_range, and filter_timegrain)
+- manage_native_filters: Add, update, remove, or reorder native filters on a
dashboard (requires write access; supports filter_select, filter_time,
filter_range, filter_timegrain, and divider)
Review Comment:
Fixed in a79c12de227123b18cfc4aaac8325d29ec7751d0. The manage_native_filters
instructions explicitly say “filter-bar divider,” distinguishing it from
manage_dashboard_markdown layout dividers. Dashboard unit tests: 993 passed;
assertion/tool-inventory tests: 162 passed; pre-commit passed.
##########
tests/unit_tests/mcp_service/dashboard/tool/test_manage_native_filters.py:
##########
@@ -528,6 +536,87 @@ async def
test_add_filter_timegrain_with_invalid_dataset(mcp_server):
command.assert_not_called()
[email protected]
+async def test_add_divider(mcp_server):
+ """A divider has no dataset/column; it stores title+description and is
+ keyed by a NATIVE_FILTER_DIVIDER- id, matching the frontend's shape."""
+ captured: dict = {"current_config": []}
+ dashboard = _mock_dashboard(filters=[])
+
+ with (
+ patch(DAO_FIND_BY_ID, return_value=dashboard),
+ patch(COMMAND_PATH, side_effect=_mock_command(captured)),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "add": [
+ {
+ "filter_type": "divider",
+ "name": "Geography",
+ "description": "Location filters",
+ }
+ ],
+ },
+ )
+
+ assert data["error"] is None
+ new_id = data["added_filter_ids"][0]
+ assert new_id.startswith("NATIVE_FILTER_DIVIDER-")
+ config = captured["payload"]["modified"][0]
+ assert config == {
+ "id": new_id,
+ "type": "DIVIDER",
+ "title": "Geography",
+ "description": "Location filters",
+ "scope": {"rootPath": ["ROOT_ID"], "excluded": []},
+ }
+
+ summary = data["filters"][0]
+ assert summary["id"] == new_id
+ assert summary["name"] == "Geography"
+ assert summary["filter_type"] == "divider"
+ assert summary["targets"] == []
+
+
[email protected]
+async def test_add_divider_default_description(mcp_server):
+ captured: dict = {"current_config": []}
+ dashboard = _mock_dashboard(filters=[])
+
+ with (
+ patch(DAO_FIND_BY_ID, return_value=dashboard),
+ patch(COMMAND_PATH, side_effect=_mock_command(captured)),
+ ):
+ data = await _call(
+ mcp_server,
+ {"dashboard_id": 1, "add": [{"filter_type": "divider", "name":
"Time"}]},
+ )
+
+ assert data["error"] is None
+ config = captured["payload"]["modified"][0]
+ assert config["description"] == ""
+
+
[email protected]
+async def test_add_divider_requires_non_empty_name(mcp_server):
+ dashboard = _mock_dashboard(filters=[])
+
+ with patch(DAO_FIND_BY_ID, return_value=dashboard):
+ async with Client(mcp_server) as client:
+ with pytest.raises(Exception, match="name"):
Review Comment:
Fixed in a79c12de227123b18cfc4aaac8325d29ec7751d0. Tightened the assertion
to match “at least 1 character.” Verified it passes on this branch and fails
against fetched master (5a2f4c8e80), where the union-tag error does not match.
Dashboard unit tests: 993 passed; assertion/tool-inventory tests: 162 passed;
pre-commit passed.
--
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]
