aminghadersohi commented on code in PR #44022:
URL: https://github.com/apache/superset/pull/44022#discussion_r3972949709
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -3506,6 +3524,27 @@ class ChartFiltersInfo(BaseModel):
"chart."
),
)
+ active_filters: List[Dict[str, Any]] = Field(
+ default_factory=list,
+ description=(
+ "Dashboard native filters the user currently has ACTIVE on this
chart "
+ "(live selections forwarded from the dashboard via
extra_form_data), "
+ "distinct from the chart's own saved filters and from
dashboard_filters "
+ "(the dashboard's default/configured state). A non-empty list
means the "
+ "chart is being viewed filtered; report these as the active
filters. "
+ "Column-based filters only; an active time-range filter is
reported "
+ "separately under active_time_range."
Review Comment:
`_attach_active_filters` also appends `extra_form_data["adhoc_filters"]`,
and this PR's own test lands an `expressionType: "SQL"` clause here, so
"Column-based filters only" misdescribes the field to the model reading it.
```suggestion
"Column-based and adhoc filters exactly as forwarded, in their "
"original shapes; an active time-range filter is reported "
"separately under active_time_range."
```
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -265,6 +265,7 @@ def generate(self) -> ASCIIPreview | ChartError:
query_context = build_query_context_from_form_data(
form_data,
chart=self.chart,
+ extra_form_data=self.request.extra_form_data,
Review Comment:
Only `format="table"` is covered — I deleted this line and the `vega_lite`
one and the whole preview suite still passed. Worth extending
`test_extra_form_data_filters_reach_preview_query` over `ascii` and `vega_lite`.
##########
superset/mcp_service/chart/tool/get_chart_info.py:
##########
@@ -334,6 +361,9 @@ async def get_chart_info(
if error is not None:
return error
+ if request.extra_form_data:
Review Comment:
With `form_data_key` only (no `identifier`) the unsaved-chart branch returns
above, so this never runs: I measured `active_filters: []` and
`active_time_range: null` for a call forwarding a gender filter plus "Last
year", which per the schema reads as "not viewed filtered".
--
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]
