-
Notifications
You must be signed in to change notification settings - Fork 13k
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(alert/report): Added optional CC and BCC fields for email notifi… #29088
base: master
Are you sure you want to change the base?
feat(alert/report): Added optional CC and BCC fields for email notifi… #29088
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #29088 +/- ##
==========================================
+ Coverage 60.48% 70.36% +9.87%
==========================================
Files 1931 1956 +25
Lines 76236 78422 +2186
Branches 8568 8960 +392
==========================================
+ Hits 46114 55178 +9064
+ Misses 28017 21043 -6974
- Partials 2105 2201 +96
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM. However, given all the design work on this modal lately, it'd be nice to get @kasiazjc feedback on the new elements. One thing I believe I've seen elsewhere is CC and BCC are text/button like objects, and they expand into an input field when clicked, so as to save real estate when not needed.
@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. |
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
CONTRIBUTORS
nsivarajan, episkey24
ADDITIONAL INFORMATION