gabotorresruiz commented on code in PR #45021:
URL: https://github.com/apache/superset/pull/45021#discussion_r4200886244
##########
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:
Question, not a blocker. This keys divider detection off `type` alone, while
the repo's own helper at `superset/dashboards/filter_scope.py:130` treats an
entry as a divider when *either* the `NATIVE_FILTER_DIVIDER-` id prefix or the
type says so, and the frontend goes entirely by the prefix
(`FiltersConfigModal/utils.ts:148`, `FilterContentRenderer.tsx:74`,
`DashboardContainer.tsx:213`). If an entry ever reaches here with the prefix
but no `type`, this path writes `name` instead of `title` and re-adds
`controlValues`, which is the exact shape you are fixing.
I could not actually produce such an entry (`transformDivider` always writes
`type`), so this may well be dead weight. Are you comfortable keying on `type`
only, or is it worth also accepting the id prefix here and in `_filter_summary`
/ `_extract_native_filters`? I might be missing a reason the stricter check is
better.
##########
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:
Just a small NIT: now that `manage_dashboard_markdown` has landed on master,
the very next line advertises a `divider` too, and that one is a full-width
band in the grid rather than a filter-bar separator (`dashboard/schemas.py` now
carries both `filter_type: Literal["divider"]` and `component_type:
Literal["divider"]`). Your tool docstring disambiguates it nicely, but this
blurb is what a model reads when it is choosing between tools. Worth saying
`filter-bar divider` here?
##########
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:
NIT: `match="name"` passes on master too, because the union error there is
`Input tag 'divider' ... does not match any of the expected tags` and its
`input_value` echo happens to contain `'name': ''`. Tightening it to something
like `match="at least 1 character"` would pin the `min_length=1` you are
actually testing. It was the only one of your new cases that passed against the
merge base for the wrong reason.
--
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]
