feat(alerts): enable tab selection for dashboard alerts/reports #29096

fisjac · 2024-06-05T16:43:39Z

SUMMARY

Implements a new feature enabling a tab to be selected as a view for an alert/report. When a dashboard is selected as the content for a report, an antd TreeSelect component is rendered to load and select tab options. When selected, the tab selection is added to the report payload as an anchor.

On execution, the feature takes advantage of existing logic for the report.extra.dashboard.anchor_tab attribute of the ReportSchedule SQLA model. When extra metadata are attached to the report schedule, a permalink is generated with the metadata included, and this permalink is then used to generate screenshots of the selected dashboard.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

https://www.loom.com/share/e075b22bd5574e30bcf3bbec5984884f?sid=aeefd476-bf0a-4289-b13f-6b5fe2854156

TESTING INSTRUCTIONS

Create a tab or tabs within an existing dashboard
Navigate to the alerts and reports listview from the settings menu
Create a new report
Under the content section select the dashboard
A selection field for tabs should appear with the tabs available within the select options
select a tab and save the report with the other required fields
at the selected interval, a report should be sent out with the selected tab in focus

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags: ALERT_REPORTS, ALERT_REPORT_TABS
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2024-06-05T16:58:48Z

Codecov Report

Attention: Patch coverage is 63.04348% with 17 lines in your changes missing coverage. Please review.

Project coverage is 70.39%. Comparing base (76d897e) to head (0001f9f).
Report is 477 commits behind head on master.

Files	Patch %	Lines
...-frontend/src/features/alerts/AlertReportModal.tsx	61.36%	14 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #29096      +/-   ##
==========================================
+ Coverage   60.48%   70.39%   +9.91%     
==========================================
  Files        1931     1969      +38     
  Lines       76236    78935    +2699     
  Branches     8568     9014     +446     
==========================================
+ Hits        46114    55570    +9456     
+ Misses      28017    21150    -6867     
- Partials     2105     2215     +110

Flag	Coverage Δ
hive	`49.10% <50.00%> (-0.07%)`	⬇️
javascript	`58.10% <61.36%> (+0.39%)`	⬆️
mysql	`76.91% <100.00%> (?)`
postgres	`77.00% <100.00%> (?)`
presto	`53.66% <50.00%> (-0.15%)`	⬇️
python	`83.69% <100.00%> (+20.20%)`	⬆️
sqlite	`76.48% <100.00%> (?)`
unit	`59.89% <50.00%> (+2.26%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

superset-frontend/src/features/alerts/AlertReportModal.tsx

superset-frontend/src/dashboard/containers/DashboardPage.tsx

superset-frontend/src/features/alerts/AlertReportModal.tsx

geido · 2024-07-17T15:06:30Z

superset/commands/report/execute.py

-        if dashboard_state := self._report_schedule.extra.get("dashboard"):
+        if (
+            dashboard_state := self._report_schedule.extra.get("dashboard")
+        ) and feature_flag_manager.is_feature_enabled("ALERT_REPORT_TABS"):


Is this supposed to be an and?

yes, the logic here is saying that if there is dashboard metadata and the feature flag is enabled, open in a stateful permalink, else open in the normal url without permalinks.

What I am saying is that now this is only applied if the ALERT_REPORT_TABS is enabled but shouldn't it be applied anyway if there is an extra parameter? Is there a reason to tie this to the ALERT_REPORT_TABS FF?

My rationale for hiding that functionality behind the ALERT_REPORT_TABS FF was that if a report was saved with an extra parameter, then the FF was turned off, there would no longer be an interface for viewing/updating the currently selected tab. It seemed more intuitive to have the behavior default to ignoring the extra parameter altogether while the FF was disabled.

If we are confident that extra is only used for this use case, then it's good.

superset-frontend/src/features/alerts/AlertReportModal.test.tsx

yousoph · 2024-07-17T18:55:30Z

/testenv up FLAG_ALERT_REPORTS = True

github-actions · 2024-07-17T19:06:41Z

@yousoph Ephemeral environment spinning up at http://35.94.63.93:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

yousoph · 2024-07-18T22:18:45Z

/testenv up FEATURE_ALERT_REPORTS = True

github-actions · 2024-07-18T22:31:29Z

@yousoph Ephemeral environment spinning up at http://34.221.90.242:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

yousoph · 2024-07-18T22:46:26Z

/testenv up FEATURE_ALERT_REPORTS=True

github-actions · 2024-07-19T01:27:14Z

@yousoph Ephemeral environment spinning up at http://35.94.142.6:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

yousoph · 2024-07-22T23:38:42Z

/testenv up FEATURE_ALERT_REPORTS=True

github-actions · 2024-07-22T23:51:07Z

@yousoph Ephemeral environment spinning up at http://34.214.205.147:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

superset-frontend/src/features/alerts/AlertReportModal.test.tsx

superset-frontend/src/features/alerts/AlertReportModal.tsx

geido · 2024-07-23T11:07:00Z

superset-frontend/src/features/alerts/AlertReportModal.tsx

+        const { tab_tree: tabTree, all_tabs: allTabs } = response.json.result;
+        setTabOptions(tabTree);
+        if (currentAlert?.extra?.dashboard?.anchor) {
+          if (!(currentAlert?.extra?.dashboard?.anchor in allTabs)) {


You are already checking above if this exists. Maybe it would help readability to move it inside a const.

const anchor = currentAlert?.extra?.dashboard?.anchor if (anchor && !anchor in allTabs)...

superset-frontend/src/features/alerts/AlertReportModal.tsx

geido · 2024-07-23T11:12:46Z

superset/commands/report/execute.py

-        if dashboard_state := self._report_schedule.extra.get("dashboard"):
+        if (
+            dashboard_state := self._report_schedule.extra.get("dashboard")
+        ) and feature_flag_manager.is_feature_enabled("ALERT_REPORT_TABS"):


What I am saying is that now this is only applied if the ALERT_REPORT_TABS is enabled but shouldn't it be applied anyway if there is an extra parameter? Is there a reason to tie this to the ALERT_REPORT_TABS FF?

superset-frontend/src/features/alerts/AlertReportModal.tsx

kasiazjc · 2024-07-23T12:36:54Z

Functionality wise - looks good! Thanks @fisjac

kgabryje · 2024-07-23T15:32:31Z

superset-frontend/src/features/alerts/AlertReportModal.tsx

@@ -593,6 +605,22 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
    setNotificationAddState('active');
  };

+  const updateAnchorState = (value: any) => {


Can we use more specific type?

Ideally, I'd like for this to be typed as string | undefined, but I'm running into issues from antd's typing for the onSelect prop which is typed as 'RawValueType | LabelValueType' and throws an error on compile when typed as such. I've used any as a workaround.

yousoph · 2024-07-24T00:16:59Z

/testenv up FEATURE_ALERT_REPORTS=True FEATURE_ALERT_REPORT_TABS = True

github-actions · 2024-07-24T00:29:10Z

@yousoph Ephemeral environment spinning up at http://54.213.240.222:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

yousoph · 2024-07-24T03:46:30Z

/testenv up FEATURE_ALERT_REPORTS=True FEATURE_ALERT_REPORT_TABS=True

github-actions · 2024-07-24T03:58:27Z

@yousoph Ephemeral environment spinning up at http://54.213.76.60:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

rusackas · 2024-07-30T04:04:25Z

Any reason not to merge this? Itchy trigger finger kicking in ;)

kasiazjc · 2024-07-30T06:09:17Z

Any reason not to merge this? Itchy trigger finger kicking in ;)

there are only reasons to merge it 🤠

github-actions · 2024-07-30T22:25:55Z

Ephemeral environment shutdown and build artifacts deleted.

pull-request-size bot added the size/L label Jun 5, 2024

github-actions bot added api Related to the REST API dependencies:npm labels Jun 5, 2024

dosubot bot added alert-reports Namespace | Anything related to the Alert & Reports feature dashboard Namespace | Anything related to the Dashboard labels Jun 5, 2024

fisjac marked this pull request as draft June 5, 2024 16:45

john-bodley added review:checkpoint Last PR reviewed during the daily review standup review:draft and removed review:checkpoint Last PR reviewed during the daily review standup labels Jun 5, 2024

geido requested review from kgabryje and geido June 10, 2024 16:18

fisjac force-pushed the ar-tabs/tree-select branch from 7984f28 to 43a0fa6 Compare June 14, 2024 16:30

pull-request-size bot added size/XL and removed size/L labels Jun 18, 2024

fisjac force-pushed the ar-tabs/tree-select branch 2 times, most recently from 0361493 to 0cbe1ac Compare June 25, 2024 17:04

pull-request-size bot added size/L and removed size/XL labels Jun 25, 2024

github-actions bot removed the api Related to the REST API label Jun 25, 2024

fisjac force-pushed the ar-tabs/tree-select branch from ebf1fdd to 9cf1063 Compare June 25, 2024 17:27

pull-request-size bot added size/XL and removed size/L labels Jun 25, 2024

github-actions bot added the api Related to the REST API label Jun 25, 2024

fisjac force-pushed the ar-tabs/tree-select branch from 9cf1063 to 9860098 Compare June 25, 2024 17:33

pull-request-size bot added size/L and removed size/XL labels Jun 25, 2024

github-actions bot removed the api Related to the REST API label Jun 25, 2024

fisjac force-pushed the ar-tabs/tree-select branch from 9860098 to a86e642 Compare June 25, 2024 17:42

fisjac force-pushed the ar-tabs/tree-select branch from a86e642 to 5f88f9b Compare July 9, 2024 17:43

eschutho reviewed Jul 16, 2024

View reviewed changes

superset-frontend/src/features/alerts/AlertReportModal.tsx Outdated Show resolved Hide resolved

fisjac force-pushed the ar-tabs/tree-select branch from 892e267 to 8a14c24 Compare July 16, 2024 15:05

geido reviewed Jul 17, 2024

View reviewed changes

rusackas removed the review:draft label Jul 17, 2024

fisjac force-pushed the ar-tabs/tree-select branch 2 times, most recently from 8b6f683 to db7c487 Compare July 19, 2024 17:07

fisjac added 2 commits July 19, 2024 12:44

implenting AR-tabs for selecting custom tab views in reports

62a7c0c

fixing reportss typo in report listview dangertoast

803ffc0

fisjac force-pushed the ar-tabs/tree-select branch from db7c487 to 803ffc0 Compare July 19, 2024 17:45

geido reviewed Jul 23, 2024

View reviewed changes

kgabryje reviewed Jul 23, 2024

View reviewed changes

AlertReportModal improvements

0001f9f

geido approved these changes Jul 25, 2024

View reviewed changes

rusackas merged commit d21d759 into apache:master Jul 30, 2024
38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(alerts): enable tab selection for dashboard alerts/reports #29096

feat(alerts): enable tab selection for dashboard alerts/reports #29096

fisjac commented Jun 5, 2024 •

edited

Loading

codecov bot commented Jun 5, 2024 •

edited

Loading

geido Jul 17, 2024

fisjac Jul 19, 2024

geido Jul 23, 2024 •

edited

Loading

fisjac Jul 24, 2024

geido Jul 25, 2024

yousoph commented Jul 17, 2024

github-actions bot commented Jul 17, 2024

yousoph commented Jul 18, 2024

github-actions bot commented Jul 18, 2024

yousoph commented Jul 18, 2024

github-actions bot commented Jul 19, 2024

yousoph commented Jul 22, 2024

github-actions bot commented Jul 22, 2024

geido Jul 23, 2024

geido Jul 23, 2024 •

edited

Loading

kasiazjc commented Jul 23, 2024

kgabryje Jul 23, 2024

fisjac Jul 24, 2024 •

edited

Loading

yousoph commented Jul 24, 2024

github-actions bot commented Jul 24, 2024

yousoph commented Jul 24, 2024

github-actions bot commented Jul 24, 2024

rusackas commented Jul 30, 2024

kasiazjc commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

feat(alerts): enable tab selection for dashboard alerts/reports #29096

feat(alerts): enable tab selection for dashboard alerts/reports #29096

Conversation

fisjac commented Jun 5, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jun 5, 2024 • edited Loading

Codecov Report

geido Jul 17, 2024

Choose a reason for hiding this comment

fisjac Jul 19, 2024

Choose a reason for hiding this comment

geido Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

fisjac Jul 24, 2024

Choose a reason for hiding this comment

geido Jul 25, 2024

Choose a reason for hiding this comment

yousoph commented Jul 17, 2024

github-actions bot commented Jul 17, 2024

yousoph commented Jul 18, 2024

github-actions bot commented Jul 18, 2024

yousoph commented Jul 18, 2024

github-actions bot commented Jul 19, 2024

yousoph commented Jul 22, 2024

github-actions bot commented Jul 22, 2024

geido Jul 23, 2024

Choose a reason for hiding this comment

geido Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

kasiazjc commented Jul 23, 2024

kgabryje Jul 23, 2024

Choose a reason for hiding this comment

fisjac Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

yousoph commented Jul 24, 2024

github-actions bot commented Jul 24, 2024

yousoph commented Jul 24, 2024

github-actions bot commented Jul 24, 2024

rusackas commented Jul 30, 2024

kasiazjc commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

fisjac commented Jun 5, 2024 •

edited

Loading

codecov bot commented Jun 5, 2024 •

edited

Loading

geido Jul 23, 2024 •

edited

Loading

geido Jul 23, 2024 •

edited

Loading

fisjac Jul 24, 2024 •

edited

Loading