sadpandajoe commented on code in PR #38591:
URL: https://github.com/apache/superset/pull/38591#discussion_r3012702252
##########
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:
Fixed in `9b9768e` — removed stale comments that referenced the old
truthiness bug.
##########
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:
Valid point, but `_get_tabs_urls` (line 349) uses the same list pattern with
`type: ignore`. Changing only this path would create inconsistency — both
should change together if at all. Keeping as-is for now.
--
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]
