Skip to content
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

Merged
merged 8 commits into from Jul 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 18 additions & 2 deletions superset-frontend/src/views/CRUD/alert/AlertReportModal.test.jsx
Expand Up @@ -36,7 +36,7 @@ const mockData = {
id: 1,
name: 'test report',
description: 'test report description',
chart: { id: 1, slice_name: 'test chart' },
chart: { id: 1, slice_name: 'test chart', viz_type: 'table' },
database: { id: 1, database_name: 'test database' },
sql: 'SELECT NaN',
};
Expand Down Expand Up @@ -76,7 +76,7 @@ fetchMock.get(dashboardEndpoint, {
});

fetchMock.get(chartEndpoint, {
result: [],
result: [{ text: 'table chart', value: 1 }],
});

async function mountAndWait(props = mockedProps) {
Expand Down Expand Up @@ -260,6 +260,22 @@ describe('AlertReportModal', () => {
expect(wrapper.find(Radio)).toHaveLength(2);
});

it('renders text option for text-based charts', async () => {
const props = {
...mockedProps,
alert: mockData,
};
const textWrapper = await mountAndWait(props);

const chartOption = textWrapper.find('input[value="chart"]');
act(() => {
chartOption.props().onChange({ target: { value: 'chart' } });
});
await waitForComponentToPaint(textWrapper);

expect(textWrapper.find('input[value="TEXT"]')).toExist();
});

it('renders input element for working timeout', () => {
expect(wrapper.find('input[name="working_timeout"]')).toExist();
});
Expand Down
44 changes: 33 additions & 11 deletions superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx
Expand Up @@ -51,6 +51,12 @@ import {

const SELECT_PAGE_SIZE = 2000; // temporary fix for paginated query
const TIMEOUT_MIN = 1;
const TEXT_BASED_VISUALIZATION_TYPES = [
'pivot_table',
'pivot_table_v2',
'table',
'paired_ttest',
];

type SelectValue = {
value: string;
Expand Down Expand Up @@ -416,6 +422,9 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
const [dashboardOptions, setDashboardOptions] = useState<MetaObject[]>([]);
const [chartOptions, setChartOptions] = useState<MetaObject[]>([]);

// Chart metadata
const [chartVizType, setChartVizType] = useState<string>('');

const isEditMode = alert !== null;
const formatOptionEnabled =
contentType === 'chart' &&
Expand Down Expand Up @@ -718,6 +727,11 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
return result;
};

const getChartVisualizationType = (chart: SelectValue) =>
SupersetClient.get({
endpoint: `/api/v1/chart/${chart.value}`,
}).then(response => setChartVizType(response.json.result.viz_type));

// Updating alert/report state
const updateAlertState = (name: string, value: any) => {
setCurrentAlert(currentAlertData => ({
Expand Down Expand Up @@ -770,6 +784,7 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
};

const onChartChange = (chart: SelectValue) => {
getChartVisualizationType(chart);
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, gotcha. sounds good.

updateAlertState('chart', chart || undefined);
updateAlertState('dashboard', null);
};
Expand Down Expand Up @@ -920,6 +935,10 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({

setConditionNotNull(resource.validator_type === 'not null');

if (resource.chart) {
setChartVizType((resource.chart as ChartObject).viz_type);
}

setCurrentAlert({
...resource,
chart: resource.chart
Expand Down Expand Up @@ -1251,17 +1270,6 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
<StyledRadio value="dashboard">{t('Dashboard')}</StyledRadio>
<StyledRadio value="chart">{t('Chart')}</StyledRadio>
</Radio.Group>
{formatOptionEnabled && (
<div className="inline-container">
<StyledRadioGroup
onChange={onFormatChange}
value={reportFormat}
>
<StyledRadio value="PNG">{t('Send as PNG')}</StyledRadio>
<StyledRadio value="CSV">{t('Send as CSV')}</StyledRadio>
</StyledRadioGroup>
</div>
)}
<AsyncSelect
className={
contentType === 'chart'
Expand Down Expand Up @@ -1302,6 +1310,20 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
cacheOptions
onChange={onDashboardChange}
/>
{formatOptionEnabled && (
<div className="inline-container">
<StyledRadioGroup
onChange={onFormatChange}
value={reportFormat}
>
<StyledRadio value="PNG">{t('Send as PNG')}</StyledRadio>
<StyledRadio value="CSV">{t('Send as CSV')}</StyledRadio>
{TEXT_BASED_VISUALIZATION_TYPES.includes(chartVizType) && (
<StyledRadio value="TEXT">{t('Send as text')}</StyledRadio>
)}
</StyledRadioGroup>
</div>
)}
<StyledSectionTitle>
<h4>{t('Notification method')}</h4>
<span className="required">*</span>
Expand Down
3 changes: 2 additions & 1 deletion superset-frontend/src/views/CRUD/alert/types.ts
Expand Up @@ -27,6 +27,7 @@ type user = {
export type ChartObject = {
id: number;
slice_name: string;
viz_type: string;
};

export type DashboardObject = {
Expand Down Expand Up @@ -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';
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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).

Copy link
Member

Choose a reason for hiding this comment

The 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;
Expand Down
1 change: 1 addition & 0 deletions superset/models/reports.py
Expand Up @@ -72,6 +72,7 @@ class ReportState(str, enum.Enum):
class ReportDataFormat(str, enum.Enum):
VISUALIZATION = "PNG"
DATA = "CSV"
TEXT = "TEXT"


class ReportCreationMethodType(str, enum.Enum):
Expand Down
1 change: 1 addition & 0 deletions superset/reports/api.py
Expand Up @@ -83,6 +83,7 @@ def ensure_alert_reports_enabled(self) -> Optional[Response]:
"active",
"chart.id",
"chart.slice_name",
"chart.viz_type",
"context_markdown",
"creation_method",
"crontab",
Expand Down
18 changes: 18 additions & 0 deletions superset/reports/commands/execute.py
Expand Up @@ -17,9 +17,11 @@
import json
import logging
from datetime import datetime, timedelta
from io import BytesIO
from typing import Any, List, Optional
from uuid import UUID

import pandas as pd
from celery.exceptions import SoftTimeLimitExceeded
from flask_appbuilder.security.sqla.models import User
from sqlalchemy.orm import Session
Expand Down Expand Up @@ -240,13 +242,22 @@ def _get_csv_data(self) -> bytes:
raise ReportScheduleCsvFailedError()
return csv_data

def _get_embedded_data(self) -> str:
"""
Return data as an HTML table, to embed in the email.
"""
buf = BytesIO(self._get_csv_data())
df = pd.read_csv(buf)
return df.to_html(na_rep="", index=False)

def _get_notification_content(self) -> NotificationContent:
"""
Gets a notification content, this is composed by a title and a screenshot

:raises: ReportScheduleScreenshotFailedError
"""
csv_data = None
embedded_data = None
error_text = None
screenshot_data = None
url = self._get_url(user_friendly=True)
Expand All @@ -270,6 +281,12 @@ def _get_notification_content(self) -> NotificationContent:
name=self._report_schedule.name, text=error_text
)

if (
self._report_schedule.chart
and self._report_schedule.report_format == ReportDataFormat.TEXT
):
embedded_data = self._get_embedded_data()

if self._report_schedule.chart:
name = (
f"{self._report_schedule.name}: "
Expand All @@ -286,6 +303,7 @@ def _get_notification_content(self) -> NotificationContent:
screenshot=screenshot_data,
description=self._report_schedule.description,
csv=csv_data,
embedded_data=embedded_data,
)

def _send(
Expand Down
1 change: 1 addition & 0 deletions superset/reports/notifications/base.py
Expand Up @@ -29,6 +29,7 @@ class NotificationContent:
text: Optional[str] = None
description: Optional[str] = ""
url: Optional[str] = None # url to chart/dashboard for this screenshot
embedded_data: Optional[str] = ""


class BaseNotification: # pylint: disable=too-few-public-methods
Expand Down
14 changes: 13 additions & 1 deletion superset/reports/notifications/email.py
Expand Up @@ -21,6 +21,7 @@
from email.utils import make_msgid, parseaddr
from typing import Any, Dict, Optional

import bleach
from flask_babel import gettext as __

from superset import app
Expand All @@ -31,6 +32,8 @@

logger = logging.getLogger(__name__)

TABLE_TAGS = ["table", "th", "tr", "td", "thead", "tbody", "tfoot"]


@dataclass
class EmailContent:
Expand Down Expand Up @@ -68,14 +71,23 @@ def _get_content(self) -> EmailContent:
csv_data = None
domain = self._get_smtp_domain()
msgid = make_msgid(domain)[1:-1]

# Strip any malicious HTML from the description
description = bleach.clean(self._content.description or "")

# Strip malicious HTML from embedded data, allowing table elements
embedded_data = bleach.clean(self._content.embedded_data or "", tags=TABLE_TAGS)

body = __(
"""
<p>%(description)s</p>
<b><a href="%(url)s">Explore in Superset</a></b><p></p>
%(embedded_data)s
%(img_tag)s
""",
description=self._content.description or "",
description=description,
url=self._content.url,
embedded_data=embedded_data,
img_tag='<img width="1000px" src="cid:{}">'.format(msgid)
if self._content.screenshot
else "",
Expand Down
1 change: 1 addition & 0 deletions tests/integration_tests/reports/api_tests.py
Expand Up @@ -181,6 +181,7 @@ def test_get_report_schedule(self):
"chart": {
"id": report_schedule.chart.id,
"slice_name": report_schedule.chart.slice_name,
"viz_type": report_schedule.chart.viz_type,
},
"context_markdown": report_schedule.context_markdown,
"crontab": report_schedule.crontab,
Expand Down
67 changes: 67 additions & 0 deletions tests/integration_tests/reports/commands_tests.py
Expand Up @@ -230,6 +230,20 @@ def create_report_email_chart_with_csv():
cleanup_report_schedule(report_schedule)


@pytest.fixture()
def create_report_email_chart_with_text():
with app.app_context():
chart = db.session.query(Slice).first()
chart.query_context = '{"mock": "query_context"}'
report_schedule = create_report_notification(
email_target="target@email.com",
chart=chart,
report_format=ReportDataFormat.TEXT,
)
yield report_schedule
cleanup_report_schedule(report_schedule)


@pytest.fixture()
def create_report_email_chart_with_csv_no_query_context():
with app.app_context():
Expand Down Expand Up @@ -721,6 +735,59 @@ def test_email_chart_report_schedule_with_csv_no_query_context(
screenshot_mock.assert_called_once()


@pytest.mark.usefixtures(
"load_birth_names_dashboard_with_slices", "create_report_email_chart_with_text"
)
@patch("superset.utils.csv.urllib.request.urlopen")
@patch("superset.utils.csv.urllib.request.OpenerDirector.open")
@patch("superset.reports.notifications.email.send_email_smtp")
@patch("superset.utils.csv.get_chart_csv_data")
def test_email_chart_report_schedule_with_text(
csv_mock, email_mock, mock_open, mock_urlopen, create_report_email_chart_with_text,
):
"""
ExecuteReport Command: Test chart email report schedule with CSV
"""
# setup csv mock
response = Mock()
mock_open.return_value = response
mock_urlopen.return_value = response
mock_urlopen.return_value.getcode.return_value = 200
response.read.return_value = CSV_FILE

with freeze_time("2020-01-01T00:00:00Z"):
AsyncExecuteReportScheduleCommand(
TEST_ID, create_report_email_chart_with_text.id, datetime.utcnow()
).run()

# assert that the data is embedded correctly
table_html = """<table>
<thead>
<tr>
<th>t1</th>
<th>t2</th>
<th>t3__sum</th>
</tr>
</thead>
<tbody>
<tr>
<td>c11</td>
<td>c12</td>
<td>c13</td>
</tr>
<tr>
<td>c21</td>
<td>c22</td>
<td>c23</td>
</tr>
</tbody>
</table>"""
assert table_html in email_mock.call_args[0][2]

# Assert logs are correct
assert_log(ReportState.SUCCESS)


@pytest.mark.usefixtures(
"load_birth_names_dashboard_with_slices", "create_report_email_dashboard"
)
Expand Down