-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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: add descriptions to report emails #13827
feat: add descriptions to report emails #13827
Conversation
@@ -70,9 +70,11 @@ def _get_content(self) -> EmailContent: | |||
image = {msgid: self._content.screenshot.image} | |||
body = __( | |||
""" | |||
<p>%(description)s</p> |
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.
Is the description just for emails?
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.
that was the ask. Should I also add the description to slack notifications while I'm at it?
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.
@junlincc ^^^
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.
Yes, let's add description to slack notifs as well.
Codecov Report
@@ Coverage Diff @@
## master #13827 +/- ##
========================================
Coverage 79.21% 79.22%
========================================
Files 936 940 +4
Lines 47408 47598 +190
Branches 5940 5979 +39
========================================
+ Hits 37554 37708 +154
- Misses 9727 9764 +37
+ Partials 127 126 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
665c9c4
to
5443ace
Compare
@samtfm you may want to rebase or merge from master - there are some conflicts here :) |
a5261fa
to
a7d9da5
Compare
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.
LGTM once CI is green.
It looks like the failing python test |
@samtfm looks like master is green, a rebase will likely solve your build issue. |
667374b
to
82a2455
Compare
I was getting failures running this test on my local master, but I'll try rebasing in case that was some local anomaly |
@samtfm looks like most required checks are passing, I think you just need to run |
1 similar comment
@samtfm looks like most required checks are passing, I think you just need to run |
* master: fix: unable to apply logging format (#14074) refactor: Bootstrap to AntD - Slider (#13989) chore(spa refactor): refactoring dashboard to use api's instead of bootstrapdata (#13306) fix(listview): update listview feature flag (#13906) Add docs for configuring Docker Compose setup (#13961) feat: invalid password error message (Postgres) (#14038) fix: flacky test in test_update_dataset_item_w_override_columns (#14082) feat: Implement Celery SoftTimeLimit handling (#13740) feat: only send alert error emails to owners of the alert (#13862) feat: add descriptions to report emails (#13827) Make chart exclude itself from cross filtering (#14046) fix: fix bug when remove chart not removing it's related cross filter data (#14081) feat(native-filters): Add default first value to select filter (#13726) feat: Make async query JWT cookie domain configurable (#14007) fix: add exception to catch session not having JWT (#14036) # Conflicts: # superset-frontend/src/dashboard/actions/hydrate.js # superset/views/core.py
* master: (53 commits) test: Adds tests to the UndoRedoKeyListeners component (#13919) chore: Adds dataMask reducer to reducerIndex (#13951) test: Tests audit for the Dashboard FilterBar (#13916) fix: unable to apply logging format (#14074) refactor: Bootstrap to AntD - Slider (#13989) chore(spa refactor): refactoring dashboard to use api's instead of bootstrapdata (#13306) fix(listview): update listview feature flag (#13906) Add docs for configuring Docker Compose setup (#13961) feat: invalid password error message (Postgres) (#14038) fix: flacky test in test_update_dataset_item_w_override_columns (#14082) feat: Implement Celery SoftTimeLimit handling (#13740) feat: only send alert error emails to owners of the alert (#13862) feat: add descriptions to report emails (#13827) Make chart exclude itself from cross filtering (#14046) fix: fix bug when remove chart not removing it's related cross filter data (#14081) feat(native-filters): Add default first value to select filter (#13726) feat: Make async query JWT cookie domain configurable (#14007) fix: add exception to catch session not having JWT (#14036) Use consistent chart value (#14031) fix: Use superset generic db to catch external_metadata queries (#13974) ...
* add description to report email * add report description to slack notification * fix issues caught by linter * fix long line * remove extra line * handle missing descriptions * run python black * properly run python black
* add description to report email * add report description to slack notification * fix issues caught by linter * fix long line * remove extra line * handle missing descriptions * run python black * properly run python black
SUMMARY
Adds the report description to the report email body and slack notification
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION