diff --git a/requirements/base.txt b/requirements/base.txt index 8f63445ec..0125e9760 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -264,6 +264,8 @@ sqlalchemy-utils==0.36.8 # flask-appbuilder sqlparse==0.3.0 # via apache-superset +tabulate==0.8.9 + # via apache-superset typing-extensions==3.7.4.3 # via # aiohttp diff --git a/setup.cfg b/setup.cfg index 55083c4b9..1e16680ee 100644 --- a/setup.cfg +++ b/setup.cfg @@ -30,7 +30,7 @@ combine_as_imports = true include_trailing_comma = true line_length = 88 known_first_party = superset -known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,cron_descriptor,croniter,cryptography,dateutil,deprecation,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_jwt_extended,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,freezegun,geohash,geopy,graphlib,holidays,humanize,isodate,jinja2,jwt,markdown,markupsafe,marshmallow,marshmallow_enum,msgpack,numpy,pandas,parameterized,parsedatetime,pgsanity,pkg_resources,polyline,prison,progress,pyarrow,pyhive,pyparsing,pytest,pytest_mock,pytz,redis,requests,selenium,setuptools,simplejson,slack,sqlalchemy,sqlalchemy_utils,sqlparse,typing_extensions,werkzeug,wtforms,wtforms_json,yaml +known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,cron_descriptor,croniter,cryptography,dateutil,deprecation,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_jwt_extended,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,freezegun,geohash,geopy,graphlib,holidays,humanize,isodate,jinja2,jwt,markdown,markupsafe,marshmallow,marshmallow_enum,msgpack,numpy,pandas,parameterized,parsedatetime,pgsanity,pkg_resources,polyline,prison,progress,pyarrow,pyhive,pyparsing,pytest,pytest_mock,pytz,redis,requests,selenium,setuptools,simplejson,slack,sqlalchemy,sqlalchemy_utils,sqlparse,tabulate,typing_extensions,werkzeug,wtforms,wtforms_json,yaml multi_line_output = 3 order_by_type = false diff --git a/setup.py b/setup.py index 357f6962d..a38c7d2a2 100644 --- a/setup.py +++ b/setup.py @@ -108,6 +108,7 @@ setup( "sqlalchemy>=1.3.16, <1.4, !=1.3.21", "sqlalchemy-utils>=0.36.6,<0.37", "sqlparse==0.3.0", # PINNED! see https://github.com/andialbrecht/sqlparse/issues/562 + "tabulate==0.8.9", "typing-extensions>=3.7.4.3,<4", # needed to support typing.Literal on py37 "wtforms-json", ], diff --git a/superset/reports/commands/execute.py b/superset/reports/commands/execute.py index c627c9589..536816237 100644 --- a/superset/reports/commands/execute.py +++ b/superset/reports/commands/execute.py @@ -242,13 +242,13 @@ class BaseReportState: raise ReportScheduleCsvFailedError() return csv_data - def _get_embedded_data(self) -> str: + def _get_embedded_data(self) -> pd.DataFrame: """ 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) + return df def _get_notification_content(self) -> NotificationContent: """ diff --git a/superset/reports/notifications/base.py b/superset/reports/notifications/base.py index 311d010e7..40439ef38 100644 --- a/superset/reports/notifications/base.py +++ b/superset/reports/notifications/base.py @@ -18,6 +18,8 @@ from dataclasses import dataclass from typing import Any, List, Optional, Type +import pandas as pd + from superset.models.reports import ReportRecipients, ReportRecipientType @@ -29,7 +31,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] = "" + embedded_data: Optional[pd.DataFrame] = None class BaseNotification: # pylint: disable=too-few-public-methods diff --git a/superset/reports/notifications/email.py b/superset/reports/notifications/email.py index 022ce1144..9a5ab29ae 100644 --- a/superset/reports/notifications/email.py +++ b/superset/reports/notifications/email.py @@ -75,19 +75,25 @@ class EmailNotification(BaseNotification): # pylint: disable=too-few-public-met # 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) + # Strip malicious HTML from embedded data, allowing only table elements + if self._content.embedded_data is not None: + df = self._content.embedded_data + html_table = bleach.clean( + df.to_html(na_rep="", index=False), tags=TABLE_TAGS + ) + else: + html_table = "" body = __( """

%(description)s

Explore in Superset

- %(embedded_data)s + %(html_table)s %(img_tag)s """, description=description, url=self._content.url, - embedded_data=embedded_data, + html_table=html_table, img_tag=''.format(msgid) if self._content.screenshot else "", diff --git a/superset/reports/notifications/slack.py b/superset/reports/notifications/slack.py index ede6b47e2..f3f51b116 100644 --- a/superset/reports/notifications/slack.py +++ b/superset/reports/notifications/slack.py @@ -17,6 +17,7 @@ # under the License. import json import logging +import textwrap from io import IOBase from typing import Optional, Union @@ -24,6 +25,7 @@ import backoff from flask_babel import gettext as __ from slack import WebClient from slack.errors import SlackApiError, SlackClientError +from tabulate import tabulate from superset import app from superset.models.reports import ReportRecipientType @@ -32,6 +34,9 @@ from superset.reports.notifications.exceptions import NotificationError logger = logging.getLogger(__name__) +# Slack only shows ~25 lines in the code block section +MAXIMUM_ROWS_IN_CODE_SECTION = 21 + class SlackNotification(BaseNotification): # pylint: disable=too-few-public-methods """ @@ -45,15 +50,17 @@ class SlackNotification(BaseNotification): # pylint: disable=too-few-public-met @staticmethod def _error_template(name: str, description: str, text: str) -> str: - return __( - """ + return textwrap.dedent( + __( + """ *%(name)s*\n %(description)s\n Error: %(text)s """, - name=name, - description=description, - text=text, + name=name, + description=description, + text=text, + ) ) def _get_body(self) -> str: @@ -61,15 +68,36 @@ class SlackNotification(BaseNotification): # pylint: disable=too-few-public-met return self._error_template( self._content.name, self._content.description or "", self._content.text ) + + # Convert Pandas dataframe into a nice ASCII table + if self._content.embedded_data is not None: + df = self._content.embedded_data + + truncated = len(df) > MAXIMUM_ROWS_IN_CODE_SECTION + message = "(table was truncated)" if truncated else "" + if truncated: + df = df[:MAXIMUM_ROWS_IN_CODE_SECTION].fillna("") + # add a last row with '...' for values + df = df.append({k: "..." for k in df.columns}, ignore_index=True) + + tabulated = tabulate(df, headers="keys", showindex=False) + table = f"```\n{tabulated}\n```\n\n{message}" + else: + table = "" + return __( - """ - *%(name)s*\n - %(description)s\n - <%(url)s|Explore in Superset> + """*%(name)s* + +%(description)s + +<%(url)s|Explore in Superset> + +%(table)s """, name=self._content.name, description=self._content.description or "", url=self._content.url, + table=table, ) def _get_inline_file(self) -> Optional[Union[str, IOBase, bytes]]: diff --git a/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py index b7410d96b..fef19e88e 100644 --- a/tests/integration_tests/reports/commands_tests.py +++ b/tests/integration_tests/reports/commands_tests.py @@ -298,6 +298,21 @@ def create_report_slack_chart_with_csv(): cleanup_report_schedule(report_schedule) +@pytest.fixture() +def create_report_slack_chart_with_text(): + with app.app_context(): + chart = db.session.query(Slice).first() + chart.query_context = '{"mock": "query_context"}' + report_schedule = create_report_notification( + slack_channel="slack_channel", + chart=chart, + report_format=ReportDataFormat.TEXT, + ) + yield report_schedule + + cleanup_report_schedule(report_schedule) + + @pytest.fixture() def create_report_slack_chart_working(): with app.app_context(): @@ -746,7 +761,7 @@ 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 + ExecuteReport Command: Test chart email report schedule with text """ # setup csv mock response = Mock() @@ -887,6 +902,47 @@ def test_slack_chart_report_schedule_with_csv( assert_log(ReportState.SUCCESS) +@pytest.mark.usefixtures( + "load_birth_names_dashboard_with_slices", "create_report_slack_chart_with_text" +) +@patch("superset.reports.notifications.slack.WebClient.chat_postMessage") +@patch("superset.utils.csv.urllib.request.urlopen") +@patch("superset.utils.csv.urllib.request.OpenerDirector.open") +@patch("superset.utils.csv.get_chart_csv_data") +def test_slack_chart_report_schedule_with_text( + csv_mock, + mock_open, + mock_urlopen, + post_message_mock, + create_report_slack_chart_with_text, +): + """ + ExecuteReport Command: Test chart slack report schedule with text + """ + # 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_slack_chart_with_text.id, datetime.utcnow() + ).run() + + table_markdown = """``` +t1 t2 t3__sum +---- ---- --------- +c11 c12 c13 +c21 c22 c23 +```""" + assert table_markdown in post_message_mock.call_args[1]["text"] + + # Assert logs are correct + assert_log(ReportState.SUCCESS) + + @pytest.mark.usefixtures("create_report_slack_chart") def test_report_schedule_not_found(create_report_slack_chart): """