feat(alert/report): Added optional CC and BCC fields for email notifi… #29088
Conversation
dosubot
bot
added
the
alert-reports
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
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
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.
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. |
Hi, thanks for this change! In terms of design, some general things:
Flow changes - agree with @villebro that we should save space and use "ghost/inline buttons", visual specs for these are:
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. 
|
Thank you for the changes, this looks good! I have two small things:
|
|
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. 
|
superset-frontend/src/features/alerts/components/NotificationMethod.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/features/alerts/components/NotificationMethod.tsx
Outdated
Show resolved
Hide resolved
|
@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. |
|
restarted some failing tests... hoping they're just flaky 🤞 |
|
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. |
|
/testenv up FLAG_ALERT_REPORTS=True |
|
@villebro Ephemeral environment spinning up at http://34.218.252.68:8080. Credentials are |
|
/testenv up FEATURE_ALERT_REPORTS=true |
|
@eschutho Ephemeral environment spinning up at http://54.184.34.192:8080. Credentials are |
superset-frontend/src/features/alerts/components/NotificationMethod.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/features/alerts/components/NotificationMethod.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/features/alerts/components/NotificationMethod.tsx
Outdated
Show resolved
Hide resolved
|
/testenv up FEATURE_ALERT_REPORTS=true |
|
@kgabryje Ephemeral environment spinning up at http://54.245.193.57:8080. Credentials are |
nsivarajan
force-pushed
the
feature-adding-cc-bcc-in-reports
branch
from
9638baa
to
1bc8426
Compare
| @@ -437,7 +437,7 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({ | |||
| /> | |||
| {index !== 0 && !!onRemove ? ( | |||
| <span | |||
| role="button" | |||
| role="delbutton" | |||
There was a problem hiding this comment.
role attribute should only use aria roles since the application's accessibility depends on it.
There was a problem hiding this comment.
Thanks @kgabryje ! Updated test case and validated to be success.
npm run test -- --silent src/features/alerts/components/NotificationMethod.test.tsx
PASS src/features/alerts/components/NotificationMethod.test.tsx
Test Suites: 1 passed, 1 total
Tests: 7 passed, 7 total
Snapshots: 0 total
Time: 4.149 s, estimated 7 s
There was a problem hiding this comment.
The "Python-Integration / test-mysql (pull_request)" job is failing intermittently and appears to be flaky. A retrigger or reattempt should succeed.
There was a problem hiding this comment.
yeah its definitely flaky... I re-run it, hopefully it passes now
There was a problem hiding this comment.
Thanks again, @kgabryje! All tests are now successful. Please assist with merging this PR.
nsivarajan
force-pushed
the
feature-adding-cc-bcc-in-reports
branch
from
573dc45
to
669e6cb
Compare
|
Merged, thanks for the contribution @nsivarajan! |
|
Ephemeral environment shutdown and build artifacts deleted. |
#29088) Co-authored-by: Sivarajan Narayanan <[email protected]> Co-authored-by: Sivarajan Narayanan <[email protected]>
#29088) Co-authored-by: Sivarajan Narayanan <[email protected]> Co-authored-by: Sivarajan Narayanan <[email protected]> (cherry picked from commit 27dde2a)
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