feat(alert/report): add ALERTS_ATTACH_REPORTS feature flags + feature (#13894)

* Add a feature flag ALERTS_ATTACH_REPORTS

* update test

* update feature flag

* add comment for feature flag

* add unit tests for alerts with attachments disabled

* fix lint

Co-authored-by: samtfm <sam@preset.io>
(cherry picked from commit 762101018b)

setup.py

@@ -118,10 +118,11 @@ setup(
             "pybigquery>=0.4.10",
             "google-cloud-bigquery>=2.4.0",
         ],
-        "clickhouse": ["clickhouse-sqlalchemy>= 0.1.4, <0.2"],
+        "clickhouse": ["clickhouse-sqlalchemy>=0.1.4, <0.2"],
         "cockroachdb": ["cockroachdb>=0.3.5, <0.4"],
         "cors": ["flask-cors>=2.0.0"],
         "crate": ["crate[sqlalchemy]>=0.26.0, <0.27"],
+        "databricks": ["databricks-dbapi[sqlalchemy]>=0.5.0, <0.6"],
         "db2": ["ibm-db-sa>=0.3.5, <0.4"],
         "dremio": ["sqlalchemy-dremio>=1.1.5, <1.2"],
         "drill": ["sqlalchemy-drill==0.1.dev"],

superset/config.py

@@ -346,6 +346,13 @@ DEFAULT_FEATURE_FLAGS: Dict[str, bool] = {
     "OMNIBAR": False,
     "DASHBOARD_RBAC": False,
     "ENABLE_EXPLORE_DRAG_AND_DROP": False,
+    # Enabling ALERTS_ATTACH_REPORTS, the system sends email and slack message
+    # with screenshot and link
+    # Disables ALERTS_ATTACH_REPORTS, the system DOES NOT generate screenshot
+    # for report with type 'alert' and sends email and slack message with only link;
+    # for report with type 'report' still send with email and slack message with
+    # screenshot and link
+    "ALERTS_ATTACH_REPORTS": True,
 }
 
 # Set the default view to card/grid view if thumbnail support is enabled.

superset/db_engine_specs/databricks.py

@@ -0,0 +1,23 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.o
+from superset.db_engine_specs.hive import HiveEngineSpec
+
+
+class DatabricksHiveEngineSpec(HiveEngineSpec):
+    engine = "databricks"
+    engine_name = "Databricks Hive"
+    driver = "pyhive"

superset/reports/commands/execute.py

@@ -25,6 +25,7 @@ from sqlalchemy.orm import Session
 from superset import app
 from superset.commands.base import BaseCommand
 from superset.commands.exceptions import CommandException
+from superset.extensions import feature_flag_manager
 from superset.models.reports import (
     ReportExecutionLog,
     ReportSchedule,
@@ -51,7 +52,7 @@ from superset.reports.dao import (
     ReportScheduleDAO,
 )
 from superset.reports.notifications import create_notification
-from superset.reports.notifications.base import NotificationContent, ScreenshotData
+from superset.reports.notifications.base import NotificationContent
 from superset.reports.notifications.exceptions import NotificationError
 from superset.utils.celery import session_scope
 from superset.utils.screenshots import (
@@ -149,7 +150,7 @@ class BaseReportState:
             raise ReportScheduleSelleniumUserNotFoundError()
         return user
 
-    def _get_screenshot(self) -> ScreenshotData:
+    def _get_screenshot(self) -> bytes:
         """
         Get a chart or dashboard screenshot
 
@@ -172,7 +173,6 @@ class BaseReportState:
                 window_size=app.config["WEBDRIVER_WINDOW"]["dashboard"],
                 thumb_size=app.config["WEBDRIVER_WINDOW"]["dashboard"],
             )
-        image_url = self._get_url(user_friendly=True)
         user = self._get_screenshot_user()
         try:
             image_data = screenshot.get_screenshot(user=user)
@@ -184,7 +184,7 @@ class BaseReportState:
             )
         if not image_data:
             raise ReportScheduleScreenshotFailedError()
-        return ScreenshotData(url=image_url, image=image_data)
+        return image_data
 
     def _get_notification_content(self) -> NotificationContent:
         """
@@ -192,7 +192,19 @@ class BaseReportState:
 
         :raises: ReportScheduleScreenshotFailedError
         """
-        screenshot_data = self._get_screenshot()
+        screenshot_data = None
+        url = self._get_url(user_friendly=True)
+        if (
+            feature_flag_manager.is_feature_enabled("ALERTS_ATTACH_REPORTS")
+            or self._report_schedule.type == ReportScheduleType.REPORT
+        ):
+            screenshot_data = self._get_screenshot()
+            if not screenshot_data:
+                return NotificationContent(
+                    name=self._report_schedule.name,
+                    text="Unexpected missing screenshot",
+                )
+
         if self._report_schedule.chart:
             name = (
                 f"{self._report_schedule.name}: "
@@ -203,7 +215,7 @@ class BaseReportState:
                 f"{self._report_schedule.name}: "
                 f"{self._report_schedule.dashboard.dashboard_title}"
             )
-        return NotificationContent(name=name, screenshot=screenshot_data)
+        return NotificationContent(name=name, url=url, screenshot=screenshot_data)
 
     def _send(self, notification_content: NotificationContent) -> None:
         """

superset/reports/notifications/base.py

@@ -21,16 +21,11 @@ from typing import Any, List, Optional, Type
 from superset.models.reports import ReportRecipients, ReportRecipientType
 
 
-@dataclass
-class ScreenshotData:
-    url: str  # url to chart/dashboard for this screenshot
-    image: bytes  # bytes for the screenshot
-
-
 @dataclass
 class NotificationContent:
     name: str
-    screenshot: Optional[ScreenshotData] = None
+    url: Optional[str] = None  # url to chart/dashboard for this screenshot
+    screenshot: Optional[bytes] = None  # bytes for the screenshot
     text: Optional[str] = None
 
 

superset/reports/notifications/email.py

@@ -63,21 +63,21 @@ class EmailNotification(BaseNotification):  # pylint: disable=too-few-public-met
             return EmailContent(body=self._error_template(self._content.text))
         # Get the domain from the 'From' address ..
         # and make a message id without the < > in the end
+        image = None
+        domain = self._get_smtp_domain()
+        msgid = make_msgid(domain)[1:-1]
+        body = __(
+            """
+            <b><a href="%(url)s">Explore in Superset</a></b><p></p>
+            <img src="cid:%(msgid)s">
+            """,
+            url=self._content.url,
+            msgid=msgid,
+        )
         if self._content.screenshot:
-            domain = self._get_smtp_domain()
-            msgid = make_msgid(domain)[1:-1]
+            image = {msgid: self._content.screenshot}
 
-            image = {msgid: self._content.screenshot.image}
-            body = __(
-                """
-                <b><a href="%(url)s">Explore in Superset</a></b><p></p>
-                <img src="cid:%(msgid)s">
-                """,
-                url=self._content.screenshot.url,
-                msgid=msgid,
-            )
-            return EmailContent(body=body, images=image)
-        return EmailContent(body=self._error_template("Unexpected missing screenshot"))
+        return EmailContent(body=body, images=image)
 
     def _get_subject(self) -> str:
         return __(

superset/reports/notifications/slack.py

@@ -57,20 +57,18 @@ class SlackNotification(BaseNotification):  # pylint: disable=too-few-public-met
     def _get_body(self) -> str:
         if self._content.text:
             return self._error_template(self._content.name, self._content.text)
-        if self._content.screenshot:
-            return __(
-                """
-                *%(name)s*\n
-                <%(url)s|Explore in Superset>
-                """,
-                name=self._content.name,
-                url=self._content.screenshot.url,
-            )
-        return self._error_template(self._content.name, "Unexpected missing screenshot")
+        return __(
+            """
+            *%(name)s*\n
+            <%(url)s|Explore in Superset>
+            """,
+            name=self._content.name,
+            url=self._content.url,
+        )
 
     def _get_inline_screenshot(self) -> Optional[Union[str, IOBase, bytes]]:
         if self._content.screenshot:
-            return self._content.screenshot.image
+            return self._content.screenshot
         return None
 
     @retry(SlackApiError, delay=10, backoff=2, tries=5)

superset/reports/schemas.py

@@ -148,8 +148,8 @@ class ReportSchedulePostSchema(Schema):
     sql = fields.String(
         description=sql_description, example="SELECT value FROM time_series_table"
     )
-    chart = fields.Integer(required=False)
-    dashboard = fields.Integer(required=False)
+    chart = fields.Integer(required=False, allow_none=True)
+    dashboard = fields.Integer(required=False, allow_none=True)
     database = fields.Integer(required=False)
     owners = fields.List(fields.Integer(description=owners_description))
     validator_type = fields.String(

tests/reports/api_tests.py

@@ -496,13 +496,16 @@ class TestReportSchedulesApi(SupersetTestCase):
         db.session.delete(created_model)
         db.session.commit()
 
-    @pytest.mark.usefixtures("create_report_schedules")
+    @pytest.mark.usefixtures(
+        "load_birth_names_dashboard_with_slices", "create_report_schedules"
+    )
     def test_create_report_schedule_schema(self):
         """
         ReportSchedule Api: Test create report schedule schema check
         """
         self.login(username="admin")
         chart = db.session.query(Slice).first()
+        dashboard = db.session.query(Dashboard).first()
         example_db = get_example_database()
 
         # Check that a report does not have a database reference
@@ -590,6 +593,56 @@ class TestReportSchedulesApi(SupersetTestCase):
         rv = self.client.post(uri, json=report_schedule_data)
         assert rv.status_code == 400
 
+        # Test that report can be created with null dashboard
+        report_schedule_data = {
+            "type": ReportScheduleType.ALERT,
+            "name": "new4",
+            "description": "description",
+            "crontab": "0 9 * * *",
+            "recipients": [
+                {
+                    "type": ReportRecipientType.EMAIL,
+                    "recipient_config_json": {"target": "target@superset.org"},
+                },
+                {
+                    "type": ReportRecipientType.SLACK,
+                    "recipient_config_json": {"target": "channel"},
+                },
+            ],
+            "working_timeout": 3600,
+            "chart": chart.id,
+            "dashboard": None,
+            "database": example_db.id,
+        }
+        uri = "api/v1/report/"
+        rv = self.client.post(uri, json=report_schedule_data)
+        assert rv.status_code == 201
+
+        # Test that report can be created with null chart
+        report_schedule_data = {
+            "type": ReportScheduleType.ALERT,
+            "name": "new5",
+            "description": "description",
+            "crontab": "0 9 * * *",
+            "recipients": [
+                {
+                    "type": ReportRecipientType.EMAIL,
+                    "recipient_config_json": {"target": "target@superset.org"},
+                },
+                {
+                    "type": ReportRecipientType.SLACK,
+                    "recipient_config_json": {"target": "channel"},
+                },
+            ],
+            "working_timeout": 3600,
+            "chart": None,
+            "dashboard": dashboard.id,
+            "database": example_db.id,
+        }
+        uri = "api/v1/report/"
+        rv = self.client.post(uri, json=report_schedule_data)
+        assert rv.status_code == 201
+
     @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
     def test_create_report_schedule_chart_dash_validation(self):
         """

tests/reports/commands_tests.py

@@ -747,6 +747,10 @@ def test_email_dashboard_report_fails(
 )
 @patch("superset.reports.notifications.email.send_email_smtp")
 @patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+@patch.dict(
+    "superset.extensions.feature_flag_manager._feature_flags",
+    ALERTS_ATTACH_REPORTS=True,
+)
 def test_slack_chart_alert(screenshot_mock, email_mock, create_alert_email_chart):
     """
     ExecuteReport Command: Test chart slack alert
@@ -770,6 +774,34 @@ def test_slack_chart_alert(screenshot_mock, email_mock, create_alert_email_chart
         assert_log(ReportState.SUCCESS)
 
 
+@pytest.mark.usefixtures(
+    "load_birth_names_dashboard_with_slices", "create_alert_email_chart"
+)
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch.dict(
+    "superset.extensions.feature_flag_manager._feature_flags",
+    ALERTS_ATTACH_REPORTS=False,
+)
+def test_slack_chart_alert_no_attachment(email_mock, create_alert_email_chart):
+    """
+    ExecuteReport Command: Test chart slack alert
+    """
+    # setup screenshot mock
+
+    with freeze_time("2020-01-01T00:00:00Z"):
+        AsyncExecuteReportScheduleCommand(
+            test_id, create_alert_email_chart.id, datetime.utcnow()
+        ).run()
+
+        notification_targets = get_target_from_report_schedule(create_alert_email_chart)
+        # Assert the email smtp address
+        assert email_mock.call_args[0][0] == notification_targets[0]
+        # Assert the there is no attached image
+        assert email_mock.call_args[1]["images"] is None
+        # Assert logs are correct
+        assert_log(ReportState.SUCCESS)
+
+
 @pytest.mark.usefixtures(
     "load_birth_names_dashboard_with_slices", "create_report_slack_chart"
 )
@@ -856,6 +888,10 @@ def test_soft_timeout_alert(email_mock, create_alert_email_chart):
 )
 @patch("superset.reports.notifications.email.send_email_smtp")
 @patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+@patch.dict(
+    "superset.extensions.feature_flag_manager._feature_flags",
+    ALERTS_ATTACH_REPORTS=True,
+)
 def test_soft_timeout_screenshot(screenshot_mock, email_mock, create_alert_email_chart):
     """
     ExecuteReport Command: Test soft timeout on screenshot
@@ -879,11 +915,11 @@ def test_soft_timeout_screenshot(screenshot_mock, email_mock, create_alert_email
 
 
 @pytest.mark.usefixtures(
-    "load_birth_names_dashboard_with_slices", "create_alert_email_chart"
+    "load_birth_names_dashboard_with_slices", "create_report_email_chart"
 )
 @patch("superset.reports.notifications.email.send_email_smtp")
 @patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
-def test_fail_screenshot(screenshot_mock, email_mock, create_alert_email_chart):
+def test_fail_screenshot(screenshot_mock, email_mock, create_report_email_chart):
     """
     ExecuteReport Command: Test soft timeout on screenshot
     """
@@ -896,7 +932,7 @@ def test_fail_screenshot(screenshot_mock, email_mock, create_alert_email_chart):
             create_alert_email_chart.id, datetime.utcnow()
         ).run()
 
-    notification_targets = get_target_from_report_schedule(create_alert_email_chart)
+    notification_targets = get_target_from_report_schedule(create_report_email_chart)
     # Assert the email smtp address, asserts a notification was sent with the error
     assert email_mock.call_args[0][0] == notification_targets[0]
 
@@ -905,6 +941,32 @@ def test_fail_screenshot(screenshot_mock, email_mock, create_alert_email_chart):
     )
 
 
+@pytest.mark.usefixtures(
+    "load_birth_names_dashboard_with_slices", "create_alert_email_chart"
+)
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch.dict(
+    "superset.extensions.feature_flag_manager._feature_flags",
+    ALERTS_ATTACH_REPORTS=False,
+)
+def test_email_disable_screenshot(email_mock, create_alert_email_chart):
+    """
+    ExecuteReport Command: Test soft timeout on screenshot
+    """
+
+    AsyncExecuteReportScheduleCommand(
+        create_alert_email_chart.id, datetime.utcnow()
+    ).run()
+
+    notification_targets = get_target_from_report_schedule(create_alert_email_chart)
+    # Assert the email smtp address, asserts a notification was sent with the error
+    assert email_mock.call_args[0][0] == notification_targets[0]
+    # Assert the there is no attached image
+    assert email_mock.call_args[1]["images"] is None
+
+    assert_log(ReportState.SUCCESS)
+
+
 @pytest.mark.usefixtures("create_invalid_sql_alert_email_chart")
 @patch("superset.reports.notifications.email.send_email_smtp")
 def test_invalid_sql_alert(email_mock, create_invalid_sql_alert_email_chart):