apache
feat(alert/report): Added optional CC and BCC fields for email notifi…
SUMMARY
Addition of an option to provide cc and bcc list for email notifications along with email validation under Alerts and Reports.
SAMPLE SCREENSHOT
TESTING INSTRUCTIONS
- Open Superset in browser > Click on the "Settings" menu on top right > Click on "Alerts & Reports"
- Create a new alert/report or edit an existing alert/report. Enter the required details.
- Select "Email" as the preferred notification method.
- Enter valid/invalid email ids in the To field, CC and BCC fields and hit add/save to test the feature.
CONTRIBUTORS
nsivarajan, episkey24
ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [X] 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
- [X] Introduces new feature or API
- [ ] Removes existing feature or API
Codecov Report
Attention: Patch coverage is 53.22581% with 29 lines in your changes missing coverage. Please review.
Project coverage is 70.40%. Comparing base (
76d897e) to head (17b2cbe). Report is 1701 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #29088 +/- ##
==========================================
+ Coverage 60.48% 70.40% +9.91%
==========================================
Files 1931 1969 +38
Lines 76236 78901 +2665
Branches 8568 9009 +441
==========================================
+ Hits 46114 55547 +9433
+ Misses 28017 21142 -6875
- Partials 2105 2212 +107
| Flag | Coverage Δ | |
|---|---|---|
| hive | 49.10% <44.44%> (-0.06%) |
:arrow_down: |
| javascript | 58.10% <45.28%> (∅) |
|
| mysql | 76.91% <100.00%> (?) |
|
| postgres | 76.99% <100.00%> (?) |
|
| presto | 53.66% <44.44%> (-0.15%) |
:arrow_down: |
| python | 83.69% <100.00%> (+20.19%) |
:arrow_up: |
| sqlite | 76.48% <100.00%> (?) |
|
| unit | 59.88% <44.44%> (+2.25%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
@nsivarajan can you please describe why this features is useful and what problem it's solving? I'm not sure it's clear why having someone cc'd on the notification email would be any different than just adding directly to the recipients list. For BCC I can see a use case where someone would silently like to know if the report is delivered as expected, but are there any others?
Please address the failing checks and add some unit/integration tests for this functionality
@nytai Thanks for raising these points. In our use of Superset's alerts and reports feature, TO recipients receive actionable items, CC recipients—including leadership, hierarchical levels, and cross-functional teams—benefit from increased visibility into the communication without being directly responsible for action, while BCC recipients discreetly monitor email delivery and processes.
I've addressed the failing checks and added test cases,Could you please assist with triggering the workflow/checks again?
@kasiazjc Could you please assist with the review of this PR when you have a moment? Your feedback would be greatly appreciated.
@nsivarajan curious if you're looking for cc/bcc for each report, or if you want the same cc/bcc list for all reports. If it's the latter, there's a recent feature that would allow you to add cc to all emails. We could also add a bcc there.
@eschutho Thanks for checking. Each report will have its own configurable CC and BCC recipients, allowing for specific recipients to be designated as needed.
For example, the Sales dashboard will have its own CC and BCC recipients, and similarly, the Manufacturing dashboard will have its own CC and BCC recipients. This approach provides flexibility and ensures that the appropriate people are included for each report.
@kasiazjc Could you please assist with the review of this PR when you have a moment? Your feedback would be greatly appreciated.
Hi, thanks for this change! In terms of design, some general things:
- we don't need "optional" in the label, as already the required fields are marked with "*", so it's one or the other
- metadata text should be under the email field, not subject
Flow changes - agree with @villebro that we should save space and use "ghost/inline buttons", visual specs for these are:
- 12px font
- icon "mail" from ant-d; size of the icon 12px x 12px
- spacing between the label and icon 4px
- spacing between two buttons 16px
In terms of behavior: when you click on the button, it shows the field. There is no way to close it (not to overcomplicate), but if it's saved empty, when you open the modal back again it goes back to being a button
Hope this works, let me know!
@kasiazjc Thank you for your great suggestions and detailed review! I've incorporated the feedback and completed the changes with a commit.
Could you please assist with reviewing the PR to ensure everything aligns correctly? Your help in getting this merged would be greatly appreciated.
@kasiazjc Thank you for your great suggestions and detailed review! I've incorporated the feedback and completed the changes with a commit.
Could you please assist with reviewing the PR to ensure everything aligns correctly? Your help in getting this merged would be greatly appreciated.
Thank you for the changes, this looks good! I have two small things:
- I think envelope icon might be bigger than 12px, can you make it smaller? Also align both text and icon to the center (should be looking similar to the "button" below with "+ add new notification method"
- when you add one field, the button is not aligned to the left "add bcc..." and there is some space left, can it be in one line with all of the other fields etc aligned to the left?
@kasiazjc
Thank you for your feedback! I've incorporated the requested adjustments into the PR.
Could you please review the changes once more to confirm they meet the requirements? Additionally, I would appreciate your assistance in merging this PR.
@nsivarajan I've started CI and I'll start an eph env once the Docker image is ready so we can poke around on the modal.
Thanks @rusackas,
The Python-Integration tests for test-mysql (pull_request) and test-postgres (current) (pull_request) are failing, although both passed successfully on my local setup. A retrigger should resolve this.
Appreciate your assistance.
@villebro Ephemeral environment spinning up at http://34.218.252.68:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@eschutho Ephemeral environment spinning up at http://54.184.34.192:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.
@villebro @eschutho Thank you for spinning up the ephemeral environment. Could you please review and assist with merging this PR? Your support in moving this forward would be greatly appreciated.
@kgabryje Ephemeral environment spinning up at http://54.245.193.57:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.