-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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: send data embedded in report email #15805
Changes from all commits
139e4fe
9a0c4c5
f6f2e05
67cc8cd
ea9e11c
ec0f57b
3ad198b
2c58e4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ type user = { | |
export type ChartObject = { | ||
id: number; | ||
slice_name: string; | ||
viz_type: string; | ||
}; | ||
|
||
export type DashboardObject = { | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what did you think about calling this 'HTML' instead of TEXT? cc @yousoph There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think for email HTML makes sense, but not for Slack (since it's not technically HTML). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's stick with "text" then :) |
||
type?: string; | ||
validator_config_json?: { | ||
op?: Operator; | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
ok, gotcha. sounds good.