aminghadersohi commented on code in PR #44985:
URL: https://github.com/apache/superset/pull/44985#discussion_r4186742072
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -240,6 +328,15 @@ def _merge_filter_update(
control_values[control_key] = value
merged["controlValues"] = control_values
+ if spec.default_value is not None:
Review Comment:
P2: an existing default goes stale when other fields change and
`default_value` is omitted. Changing `column`/`dataset_id` keeps the old
`defaultDataMask`. On load, `SelectFilterPlugin` sees `filterState.value !==
undefined` and calls `updateDataMask(filterState.value)` with the *new* column,
so the old column's values get applied to the new column (the result is often
empty). The same gap applies to `multi_select=False` while a multi-value
default is stored (it bypasses the single-select check in `_select_data_mask`),
and to `default_to_first_item=True` while an explicit default exists (the
stored value wins, so first-item never takes effect). Suggestion: when the
target changes or first-item gets enabled and `default_value` is not given,
reset to `_empty_data_mask()`. Otherwise re-run `_select_data_mask(merged,
existing_values)` so an invalid combination is rejected rather than saved.
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -64,6 +72,83 @@ def _empty_data_mask() -> dict[str, Any]:
return {"filterState": {"value": None}, "extraFormData": {}}
+def _value_label(value: FilterSelectValue) -> str:
+ """Format one selected value the way the dashboard UI labels it."""
+ if value is None:
+ return _NULL_LABEL
+ if isinstance(value, bool):
+ return _TRUE_LABEL if value else _FALSE_LABEL
+ return str(value)
+
+
+def _select_data_mask(
+ conf: dict[str, Any], values: list[FilterSelectValue]
+) -> dict[str, Any]:
+ """Build the data mask a filter_select filter produces for ``values``.
+
+ Mirrors the frontend's ``getSelectExtraFormData``: a non-empty selection
+ becomes an ``IN`` predicate on the filter's target column, and an empty
+ selection on a filter marked ``enableEmptyFilter`` becomes an impossible
+ predicate (the "required filter, nothing chosen" state) rather than no
+ filtering at all. Shared by ``apply_dashboard_filters`` (applied values)
+ and this module (default values on create/update) so both paths agree.
+ """
+ targets = [target for target in (conf.get("targets") or []) if target]
+ column = (targets[0].get("column") or {}).get("name") if targets else None
+ if not column:
+ raise _FilterValidationError(
+ f"Filter '{conf.get('name') or conf.get('id')}' has no target "
+ "column, so a value cannot be applied to it."
+ )
+
+ control_values = conf.get("controlValues") or {}
+ if control_values.get("inverseSelection"):
Review Comment:
P3: the shared helper also runs for `default_value=[]` (clear). So a
UI-created filter that uses `inverseSelection` or a non-`exact` `operatorType`
cannot have its default cleared through this tool, even though clearing applies
no predicate. Consider skipping these guards when `values` is empty, or
clearing with `_empty_data_mask()` directly.
--
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]
