Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(alerts): Select tabs to send backend #17749

Merged

Conversation

m-ajay
Copy link
Contributor

@m-ajay m-ajay commented Dec 14, 2021

SUMMARY

For a large dashboard that have multiple tabs, users want to get email report on a specific tabs (or nested tabs). So we proposed a feature request to allow user select tab(s) when setup dashboard report. This PR is a backend piece for validating tab selection, store the selected tab with report schedule, and taking a list of selected tabs screenshot for a dashboard report.

TESTING INSTRUCTIONS

added integration tests.

ADDITIONAL INFORMATION

  • Has associated issue: [alert/report] Solution for dashboards with multiple tabs #14056
  • Required feature flags:
  • 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

@m-ajay m-ajay marked this pull request as ready for review December 21, 2021 23:18
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome tests! a few minor comments, but everything makes sense to me

@@ -16,6 +16,8 @@
# under the License.
"""A collection of ORM sqlalchemy models for Superset"""
import enum
import json
import typing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think in most of the code we do from typing import Any, Optional, etc.

tab_id for tab_id in dashboard_tab_ids if tab_id not in position_data
]
if invalid_tab_ids:
exceptions.append(ValidationError("Invalid tab IDs selected", "extra"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we append the invalid tab ids to the error message? If not, then we don't need to find all of them, and can instead just check if dashboard_tab_ids is a subset of position_data. Like this:

if not set(dashboard_tab_ids).issubset.(set(position_data.keys())):
  raise Exception

Copy link

@graceguo-supercat graceguo-supercat Jan 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think let owners know which tab_id is invalid is important. So i prefer to keep invalid_tab_ids and add it to error message.

for screenshot in screenshots:
image = screenshot.get_screenshot(user=user)
if image is not None:
image_data.append(image)
except SoftTimeLimitExceeded as ex:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit concerning, because now we're asking you to complete all the screenshots in the same time originally alotted to a single one. I guess we could either increase the time limit, or try to do them in parallel

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we try add time limit on each of screenshot?

@graceguo-supercat graceguo-supercat force-pushed the feat/alerts-select-tabs-to-send-backend branch from 04a154f to 4683f5c Compare January 7, 2022 00:13
@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #17749 (ae4699e) into master (07998fe) will decrease coverage by 0.02%.
The diff coverage is 98.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17749      +/-   ##
==========================================
- Coverage   67.07%   67.05%   -0.03%     
==========================================
  Files        1609     1610       +1     
  Lines       64899    65073     +174     
  Branches     6866     6866              
==========================================
+ Hits        43533    43634     +101     
- Misses      19500    19573      +73     
  Partials     1866     1866              
Flag Coverage Δ
hive 53.23% <18.46%> (-0.06%) ⬇️
mysql ?
postgres 82.14% <98.46%> (-0.11%) ⬇️
presto 53.07% <18.46%> (-0.06%) ⬇️
python 82.54% <98.46%> (-0.15%) ⬇️
sqlite 81.82% <98.46%> (-0.10%) ⬇️

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

Impacted Files Coverage Δ
superset/reports/commands/create.py 89.18% <92.30%> (+0.66%) ⬆️
superset/models/reports.py 100.00% <100.00%> (ø)
superset/reports/commands/execute.py 91.51% <100.00%> (+0.22%) ⬆️
superset/reports/notifications/base.py 95.83% <100.00%> (ø)
superset/reports/notifications/email.py 100.00% <100.00%> (ø)
superset/reports/notifications/slack.py 87.32% <100.00%> (+0.18%) ⬆️
superset/reports/schemas.py 98.83% <100.00%> (+0.02%) ⬆️
superset/utils/encrypt.py 41.46% <0.00%> (-46.54%) ⬇️
superset/db_engine_specs/mysql.py 94.04% <0.00%> (-3.58%) ⬇️
superset/models/core.py 88.75% <0.00%> (-0.76%) ⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07998fe...ae4699e. Read the comment docs.

@graceguo-supercat graceguo-supercat force-pushed the feat/alerts-select-tabs-to-send-backend branch 8 times, most recently from 7163167 to ed53f56 Compare January 7, 2022 18:36
@graceguo-supercat graceguo-supercat force-pushed the feat/alerts-select-tabs-to-send-backend branch from ed53f56 to d954835 Compare January 7, 2022 19:16
@graceguo-supercat
Copy link

@dpgaspar @betodealmeida we are trying to implements an improvement for dashboard email report: allow user to select one or more dashboard tabs, and send them in a single email report. see description in this issue. This PR is backend part and we have another one for front-end tab selector. Ajay started the project but left, so I will take over the rest and finish it.

i saw you had some commits for the Alert and Report feature. please let me know if you have any concerns on our solution. Thanks.

@graceguo-supercat
Copy link

@m-ajay can you update the summary with more context on what we're trying to achieve here?

Ajay will not work for Superset anymore...i will update description.

@graceguo-supercat
Copy link

ping @etr2460 @betodealmeida @dpgaspar

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I left a few minor nits, but this looks solid!

else ""
)
img_tag = ""
for msgid, _ in images.items():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
for msgid, _ in images.items():
for msgid in images.keys():

(Or just images instead of images.keys(), though I think explicit is better.)

@@ -27,7 +27,7 @@
class NotificationContent:
name: str
csv: Optional[bytes] = None # bytes for csv file
screenshot: Optional[bytes] = None # bytes for the screenshot
screenshots: Optional[List[bytes]] = None # bytes for the screenshot
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
screenshots: Optional[List[bytes]] = None # bytes for the screenshot
screenshots: Optional[List[bytes]] = None # bytes for the screenshots

)
img_tag = ""
for msgid, _ in images.items():
img_tag = img_tag + (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, it's more idiomatic to do this by joining a List[str]:

img_tags = []
for msgid in images.keys():
    img_tags.append('<div ...>')
img_tag = ''.join(img_tags)

@graceguo-supercat
Copy link

Thanks @betodealmeida! i added extra fixes per comments, will merge after CI passed.

@graceguo-supercat graceguo-supercat merged commit bdc35a2 into apache:master Jan 11, 2022
shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
* Adding the extra config and validation

* wip

* reports working

* Tests working

* fix type

* Fix lint errors

* Fixing type issues

* add licence header

* fix the fixture deleting problem

* scope to session

* fix integration test

* fix review comments

* fix review comments patch 2

Co-authored-by: Grace Guo <grace.guo@airbnb.com>
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
* Adding the extra config and validation

* wip

* reports working

* Tests working

* fix type

* Fix lint errors

* Fixing type issues

* add licence header

* fix the fixture deleting problem

* scope to session

* fix integration test

* fix review comments

* fix review comments patch 2

Co-authored-by: Grace Guo <grace.guo@airbnb.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants