Copilot commented on code in PR #38591:
URL: https://github.com/apache/superset/pull/38591#discussion_r3012608891
##########
superset/commands/report/execute.py:
##########
@@ -287,14 +287,19 @@ def get_dashboard_urls(
except json.JSONDecodeError:
logger.debug("Anchor value is not a list, Fall back to
single tab")
+ # Merge native_filters into existing urlParams instead of
+ # overwriting — dashboard_state may already have urlParams
+ # (e.g. standalone=true) that must be preserved.
+ state: DashboardPermalinkState = {**dashboard_state}
+ existing_params: list[tuple[str, str]] = state.get("urlParams") or
[]
+ merged_params: list[list[str]] = [
+ list(p) for p in existing_params if p[0] != "native_filters"
+ ]
+ merged_params.append(["native_filters", native_filter_params or
""])
+ state["urlParams"] = merged_params # type: ignore[typeddict-item]
Review Comment:
`DashboardPermalinkState.urlParams` is typed (and marshmallow-validated) as
`list[tuple[str, str]]`, but this code builds `merged_params` as
`list[list[str]]` and then assigns it back with a `type: ignore`. Since this
module is under strict mypy, it would be better to normalize `existing_params`
entries to tuples (e.g. `tuple(p)`), build `merged_params` as `list[tuple[str,
str]]`, and then assign without the TypedDict ignore. This also keeps permalink
payloads consistent and avoids changing tuple→list representation unnecessarily
for deterministic permalink key generation.
```suggestion
merged_params: list[tuple[str, str]] = [
p for p in existing_params if p[0] != "native_filters"
]
merged_params.append(("native_filters", native_filter_params or
""))
state["urlParams"] = merged_params
```
##########
tests/unit_tests/reports/model_test.py:
##########
@@ -378,3 +524,136 @@ def test_get_native_filters_params_unknown_filter_type():
assert len(warnings) == 1
assert "unrecognized filter type" in warnings[0]
assert "filter_unknown_type" in warnings[0]
+
+
+def test_get_native_filters_params_missing_filter_id_key():
+ report_schedule = ReportSchedule()
+ report_schedule.extra = {
+ "dashboard": {
+ "nativeFilters": [
+ {
+ "filterType": "filter_select",
+ "columnName": "col",
+ "filterValues": ["v"],
+ # Missing "nativeFilterId" key — skipped by defensive guard
+ }
+ ]
+ }
+ }
+ result, warnings = report_schedule.get_native_filters_params()
+ assert result == "()"
+ assert len(warnings) == 1
+ assert "Skipping malformed native filter" in warnings[0]
+
+
+def test_generate_native_filter_empty_filter_id():
+ """Empty native_filter_id triggers the ``or ""`` fallback branches."""
+ report_schedule = ReportSchedule()
+ result, warning = report_schedule._generate_native_filter(
+ "", "filter_select", "col", ["x"]
+ )
+ assert "" in result
+ assert result[""]["id"] == ""
+ assert warning is None
+
+
+def test_generate_native_filter_range_zero_min():
+ """Zero min_val should produce a two-sided label, not a max-only label."""
+ report_schedule = ReportSchedule()
+ result, warning = report_schedule._generate_native_filter(
+ "F5", "filter_range", "price", [0, 100]
+ )
+ # Filters use `is not None` so they are correct
+ assert result["F5"]["extraFormData"]["filters"] == [
+ {"col": "price", "op": ">=", "val": 0},
+ {"col": "price", "op": "<=", "val": 100},
+ ]
+ # Label uses truthiness so this assertion documents the bug
Review Comment:
The comment in this test is now inaccurate: it says the range-label logic
uses truthiness and that the assertion is documenting a bug, but
`ReportSchedule._generate_native_filter()` was updated to use explicit `is not
None` checks (so 0 should be handled correctly). Please update/remove the
comment so it doesn’t mislead future readers about the current behavior.
```suggestion
# Label generation correctly treats zero as a valid bound
```
--
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]
