mikebridge commented on code in PR #39973:
URL: https://github.com/apache/superset/pull/39973#discussion_r3573457720
##########
superset/commands/report/execute.py:
##########
@@ -244,10 +265,7 @@ def _get_url(
) and feature_flag_manager.is_feature_enabled("ALERT_REPORT_TABS"):
return self._get_tab_url(dashboard_state,
user_friendly=user_friendly)
- dashboard = self._report_schedule.dashboard
- dashboard_id_or_slug = (
- dashboard.uuid if dashboard and dashboard.uuid else dashboard.id
- )
+ dashboard_id_or_slug = dashboard.uuid or dashboard.id
Review Comment:
Addressed by the rebase: `master` now has a dedicated missing-dashboard
guard in `get_dashboard_urls`, plus
`test_get_dashboard_urls_raises_when_target_dashboard_soft_deleted`. The
rebased PR preserves both.
##########
superset/commands/report/execute.py:
##########
@@ -244,10 +265,7 @@ def _get_url(
) and feature_flag_manager.is_feature_enabled("ALERT_REPORT_TABS"):
return self._get_tab_url(dashboard_state,
user_friendly=user_friendly)
- dashboard = self._report_schedule.dashboard
- dashboard_id_or_slug = (
- dashboard.uuid if dashboard and dashboard.uuid else dashboard.id
- )
+ dashboard_id_or_slug = dashboard.uuid or dashboard.id
Review Comment:
Good point. Updated the fallback to `dashboard.uuid if dashboard.uuid is not
None else dashboard.id`, preserving the precise prior semantics.
##########
superset/commands/report/execute.py:
##########
@@ -263,6 +263,9 @@ def _get_url(
"""
Get the url for this report schedule: chart or dashboard
"""
+ chart = self._report_schedule.chart
+ dashboard = self._report_schedule.dashboard
Review Comment:
The locals are inferred from the typed `ReportSchedule` relationships, and
the PR-file MyPy hook passes. Superset requires typed Python code, but does not
require redundant annotations for every inferred local; adding model imports
here would not improve type safety.
##########
superset/commands/report/execute.py:
##########
@@ -315,10 +325,7 @@ def _get_url(
) and feature_flag_manager.is_feature_enabled("ALERT_REPORT_TABS"):
return self._get_tab_url(dashboard_state,
user_friendly=user_friendly)
- dashboard = self._report_schedule.dashboard
- dashboard_id_or_slug = (
- dashboard.uuid if dashboard and dashboard.uuid else dashboard.id
- )
+ dashboard_id_or_slug = dashboard.uuid or dashboard.id
Review Comment:
This union is inferred from the typed dashboard fields, and the PR-file MyPy
hook passes. An explicit local annotation would duplicate the model types
without adding safety, so I am keeping inference here.
##########
tests/unit_tests/commands/report/execute_test.py:
##########
@@ -2359,3 +2359,32 @@ def
test_get_dashboard_urls_raises_when_target_dashboard_soft_deleted(
state = BaseReportState(report_schedule, datetime.utcnow(), uuid4())
with pytest.raises(ReportScheduleTargetDashboardDeletedError):
state.get_dashboard_urls()
+
+
+def test_get_url_raises_unexpected_error_when_target_is_missing(
+ mocker: MockerFixture,
+ app,
+) -> None:
Review Comment:
Fixed: the test fixture is now annotated as `app: SupersetApp`.
--
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]
