feat: send data embedded in report email #15805
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15805 +/- ##
==========================================
- Coverage 76.91% 76.85% -0.06%
==========================================
Files 986 986
Lines 51993 52008 +15
Branches 7090 7090
==========================================
- Hits 39988 39973 -15
- Misses 11779 11809 +30
Partials 226 226
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
betodealmeida
force-pushed
the
ch16684
branch
3 times, most recently
from
7c0477f
to
66cb8e7
Compare
| @@ -74,7 +75,7 @@ export type AlertObject = { | |||
| owners?: Array<Owner | MetaObject>; | |||
| sql?: string; | |||
| recipients?: Array<Recipient>; | |||
| report_format?: 'PNG' | 'CSV'; | |||
| report_format?: 'PNG' | 'CSV' | 'TEXT'; | |||
There was a problem hiding this comment.
what did you think about calling this 'HTML' instead of TEXT? cc @yousoph
There was a problem hiding this comment.
I think for email HTML makes sense, but not for Slack (since it's not technically HTML).
There was a problem hiding this comment.
Let's stick with "text" then :)
| @@ -770,6 +784,7 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({ | |||
| }; | |||
|
|
|||
| const onChartChange = (chart: SelectValue) => { | |||
| getChartVisualizationType(chart); | |||
There was a problem hiding this comment.
would it be easier to update the api that fetches the list of charts to include the chart type?
There was a problem hiding this comment.
That was my initial attempt, but it seems like the related objects come back only with ID and name, and it seems I would need to change FAB itself.
* feat: send data embedded in report email * Prettify table * Change post-processing to use new endpoint * Show text option only for text viz * Show TEXT option only to text-based vizs * Fix test * Add email test * Add unit test
* feat: send data embedded in report email * Prettify table * Change post-processing to use new endpoint * Show text option only for text viz * Show TEXT option only to text-based vizs * Fix test * Add email test * Add unit test
* feat: send data embedded in report email * Prettify table * Change post-processing to use new endpoint * Show text option only for text viz * Show TEXT option only to text-based vizs * Fix test * Add email test * Add unit test
* feat: send data embedded in report email * Prettify table * Change post-processing to use new endpoint * Show text option only for text viz * Show TEXT option only to text-based vizs * Fix test * Add email test * Add unit test
mistercrunch
added
🏷️ bot
SUMMARY
This PR introduces a new category of reports for charts, "TEXT":
The option shows up for a small subset of the charts types that present data in textual format: t-test, pivot table and table.
When the report is sent as text the data is embedded directly in the email as an HTML table:
Finally, I also noticed that we were passing HTML from the description directly to the email without escaping it. This was fixed by using the
bleachlibrary.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION