aminghadersohi commented on code in PR #40960:
URL: https://github.com/apache/superset/pull/40960#discussion_r3501652256
##########
superset/daos/dashboard.py:
##########
@@ -431,36 +431,25 @@ def update_native_filters_config(
raise DashboardUpdateFailedError("Dashboard not found")
if attributes:
- metadata = json.loads(dashboard.json_metadata or "{}")
+ try:
+ _parsed = json.loads(dashboard.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ _parsed = {}
+ metadata = _parsed if isinstance(_parsed, dict) else {}
native_filter_configuration = metadata.get(
"native_filter_configuration", []
)
reordered_filter_ids: list[int] = attributes.get("reordered", [])
+ deleted_ids = set(attributes.get("deleted", []))
+ modified_map = {f.get("id"): f for f in attributes.get("modified",
[])}
Review Comment:
Declining: same as `deleted_ids` above — `modified_map` is a
clearly-inferred dict comprehension result and annotating it is not the
codebase pattern. CodeAnt's saved custom instruction from earlier threads
covers this.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1717,3 +1717,217 @@ def dashboard_layout_serializer(dashboard: "Dashboard")
-> DashboardLayout:
has_layout=bool(position_json_str),
)
)
+
+
+# ---------------------------------------------------------------------------
+# manage_native_filters schemas
+# ---------------------------------------------------------------------------
+
+
+class BaseNewFilterSpec(BaseModel):
+ """Common fields shared by all new native filter specs."""
+
+ name: str = Field(..., min_length=1, description="Filter display name")
+ description: str = Field("", description="Optional filter description")
+ scope_chart_ids: List[int] | None = Field(
+ None,
+ description=(
+ "Chart IDs this filter should apply to. When omitted the filter "
+ "applies to all charts on the dashboard. All IDs must belong to "
+ "charts that are on the dashboard."
+ ),
+ )
+
+
+class FilterSelectSpec(BaseNewFilterSpec):
+ """Spec for a new dropdown (filter_select) native filter."""
+
+ filter_type: Literal["filter_select"] = Field(
+ ..., description="Discriminator - must be 'filter_select'"
+ )
+ dataset_id: int = Field(..., description="ID of the dataset to filter on")
+ column: str = Field(
+ ..., min_length=1, description="Name of the dataset column to filter
on"
+ )
+ multi_select: bool = Field(
+ True, description="Allow selecting multiple values (default True)"
+ )
+ default_to_first_item: bool = Field(
+ False, description="Default the filter to the first item in the list"
+ )
+ enable_empty_filter: bool = Field(
+ False, description="Require a value before the filter is applied"
+ )
Review Comment:
Declining: the description is correct. The UI label for this control is
`t('User must select a value before applying the filter')` (see
`superset-frontend/src/filters/components/Select/controlPanel.ts:177` and the
Range/Time/TimeGrain/TimeColumn control panels). When `enableEmptyFilter` is
`true`, a value is required before the filter fires — confirmed by
RangeFilterPlugin.tsx line 203: `// If enableEmptyFilter is true, at least one
value is required`. Our description 'Require a value before the filter is
applied' matches this exactly. CodeAnt's severity block itself notes '⚠️
Suggestion addresses non-issue; description matches frontend semantics.'
--
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]
