Copilot commented on code in PR #38591:
URL: https://github.com/apache/superset/pull/38591#discussion_r2922527167
##########
tests/unit_tests/commands/report/execute_test.py:
##########
@@ -343,6 +360,123 @@ def test_get_dashboard_urls_with_exporting_dashboard_only(
assert expected_url == result[0]
+@patch("superset.commands.report.execute.CreateDashboardPermalinkCommand")
+@with_feature_flags(ALERT_REPORT_TABS=True)
+def test_get_dashboard_urls_with_filters_and_tabs(
+ mock_permalink_cls,
+ mocker: MockerFixture,
+ app,
+) -> None:
+ mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule)
+ mock_report_schedule.chart = False
+ mock_report_schedule.chart_id = None
+ mock_report_schedule.dashboard_id = 123
+ mock_report_schedule.type = "report_type"
+ mock_report_schedule.report_format = "report_format"
+ mock_report_schedule.owners = [1, 2]
+ mock_report_schedule.recipients = []
+ native_filter_rison = "(NATIVE_FILTER-1:(filterType:filter_select))"
+ mock_report_schedule.extra = {
+ "dashboard": {
+ "anchor": json.dumps(["TAB-1", "TAB-2"]),
+ "dataMask": {"NATIVE_FILTER-1": {"filterState": {"value":
["Sales"]}}},
+ "activeTabs": ["TAB-1", "TAB-2"],
+ "urlParams": None,
+ "nativeFilters": [ # type: ignore[typeddict-unknown-key]
+ {
+ "nativeFilterId": "NATIVE_FILTER-1",
+ "filterType": "filter_select",
+ "columnName": "department",
+ "filterValues": ["Sales"],
+ }
+ ],
+ }
+ }
+ mock_report_schedule.get_native_filters_params.return_value =
native_filter_rison # type: ignore[attr-defined]
+ mock_permalink_cls.return_value.run.side_effect = ["key1", "key2"]
+
+ class_instance: BaseReportState = BaseReportState(
+ mock_report_schedule, "January 1, 2021", "execution_id_example"
+ )
+ class_instance._report_schedule = mock_report_schedule
+
+ result: list[str] = class_instance.get_dashboard_urls()
+
+ import urllib.parse
+
+ base_url = app.config.get("WEBDRIVER_BASEURL", "http://0.0.0.0:8080/";)
+ assert result == [
+ urllib.parse.urljoin(base_url, "superset/dashboard/p/key1/"),
+ urllib.parse.urljoin(base_url, "superset/dashboard/p/key2/"),
+ ]
+ mock_report_schedule.get_native_filters_params.assert_called_once() #
type: ignore[attr-defined]
+ assert mock_permalink_cls.call_count == 2
+ for call in mock_permalink_cls.call_args_list:
+ state = call.kwargs["state"]
+ assert state["urlParams"] == [["native_filters", native_filter_rison]]
+ assert mock_permalink_cls.call_args_list[0].kwargs["state"]["anchor"] ==
"TAB-1"
+ assert mock_permalink_cls.call_args_list[1].kwargs["state"]["anchor"] ==
"TAB-2"
+
+
+@patch("superset.commands.report.execute.CreateDashboardPermalinkCommand")
+@with_feature_flags(ALERT_REPORT_TABS=True)
+def test_get_dashboard_urls_with_filters_no_tabs(
+ mock_permalink_cls,
+ mocker: MockerFixture,
+ app,
+) -> None:
+ mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule)
+ mock_report_schedule.chart = False
+ mock_report_schedule.chart_id = None
+ mock_report_schedule.dashboard_id = 123
+ mock_report_schedule.type = "report_type"
+ mock_report_schedule.report_format = "report_format"
+ mock_report_schedule.owners = [1, 2]
+ mock_report_schedule.recipients = []
+ native_filter_rison = "(NATIVE_FILTER-1:(filterType:filter_select))"
+ mock_report_schedule.extra = {
+ "dashboard": {
+ "anchor": "",
+ "dataMask": {"NATIVE_FILTER-1": {"filterState": {"value":
["Sales"]}}},
+ "activeTabs": None,
+ "urlParams": None,
+ "nativeFilters": [ # type: ignore[typeddict-unknown-key]
+ {
+ "nativeFilterId": "NATIVE_FILTER-1",
+ "filterType": "filter_select",
+ "columnName": "department",
+ "filterValues": ["Sales"],
+ }
+ ],
+ }
+ }
+ mock_report_schedule.get_native_filters_params.return_value =
native_filter_rison # type: ignore[attr-defined]
+ mock_permalink_cls.return_value.run.return_value = "key1"
+
+ class_instance: BaseReportState = BaseReportState(
+ mock_report_schedule, "January 1, 2021", "execution_id_example"
+ )
+ class_instance._report_schedule = mock_report_schedule
+
+ result: list[str] = class_instance.get_dashboard_urls()
+
+ import urllib.parse
+
+ base_url = app.config.get("WEBDRIVER_BASEURL", "http://0.0.0.0:8080/";)
+ assert result == [
+ urllib.parse.urljoin(base_url, "superset/dashboard/p/key1/"),
+ ]
+ mock_report_schedule.get_native_filters_params.assert_called_once() #
type: ignore[attr-defined]
+ assert mock_permalink_cls.call_count == 1
+ state = mock_permalink_cls.call_args_list[0].kwargs["state"]
+ # BUG: {urlParams: [...], **dashboard_state} lets
dashboard_state["urlParams"]
+ # overwrite the native_filters param. The tabs path (_get_tabs_urls) does
not
+ # have this issue because it builds the dict without **dashboard_state.
+ assert (
+ state["urlParams"] is None
+ ) # should be [["native_filters", native_filter_rison]]
Review Comment:
This test is asserting the current buggy behavior where
`dashboard_state["urlParams"]` overwrites the injected `native_filters` URL
param (because the state dict is built as `{ "urlParams": ...,
**dashboard_state }`). That means dashboard native filters are silently dropped
whenever `dashboard_state.urlParams` is present (even `None`). Prefer asserting
the correct behavior (native_filters preserved) and fix
`BaseReportState.get_dashboard_urls()` to merge state without letting
`dashboard_state.urlParams` override the computed value (eg merge in the
opposite order or explicitly set/append `urlParams`).
```suggestion
# Ensure that dashboard_state["urlParams"] does not overwrite the
injected
# native_filters param. The tabs path (_get_tabs_urls) already builds
the dict
# correctly, so get_dashboard_urls() should behave the same way.
assert state["urlParams"] == [["native_filters", native_filter_rison]]
```
##########
superset-frontend/src/pages/AlertReportList/index.tsx:
##########
@@ -242,9 +242,13 @@ function AlertList({
}),
);
- updateResource(update_id, { active: checked }, false, false)
- .then()
- .catch(() => setResourceCollection(original));
+ updateResource(update_id, { active: checked }, false, false).then(
+ response => {
+ if (!response) {
+ setResourceCollection(original);
+ }
+ },
+ );
Review Comment:
PR description states this is a test-only change, but this hunk modifies
production UI behavior (the rollback logic after `updateResource`). Please
update the PR description to reflect the presence of production changes, or
split the production change into a separate PR so reviewers can assess it
independently.
--
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]
