From e70b9fff6a7a9b6352120bfdb4a2c5c950ac7d38 Mon Sep 17 00:00:00 2001
From: Jason Davis <32852580+JasonD28@users.noreply.github.com>
Date: Thu, 3 Sep 2020 08:15:05 -0700
Subject: [PATCH] fix: add validator information to email/slack alerts (#10762)

* added validator info to alerts

* adjusted format of messages

* added nits

Co-authored-by: Jason Davis <@dropbox.com>
---
 superset/models/alerts.py                     | 13 ++++++++++
 superset/tasks/schedules.py                   | 24 ++++++++++++-------
 superset/templates/email/alert.txt            |  5 ++--
 superset/templates/slack/alert.txt            |  5 ++--
 .../templates/slack/alert_no_screenshot.txt   |  5 ++--
 tests/alerts_tests.py                         |  9 +++----
 6 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/superset/models/alerts.py b/superset/models/alerts.py
index a62aacc419fe5..95e7c06042d90 100644
--- a/superset/models/alerts.py
+++ b/superset/models/alerts.py
@@ -15,6 +15,7 @@
 # specific language governing permissions and limitations
 # under the License.
 """Models for scheduled execution of jobs"""
+import json
 import textwrap
 from datetime import datetime
 from typing import Any, Optional
@@ -201,3 +202,15 @@ def alert(self) -> RelationshipProperty:
             foreign_keys=[self.alert_id],
             backref=backref("validators", cascade="all, delete-orphan"),
         )
+
+    def pretty_print(self) -> str:
+        """ String representing the comparison that will trigger a validator """
+        config = json.loads(self.config)
+
+        if self.validator_type.lower() == "operator":
+            return f"{config['op']} {config['threshold']}"
+
+        if self.validator_type.lower() == "not null":
+            return "!= Null or 0"
+
+        return ""
diff --git a/superset/tasks/schedules.py b/superset/tasks/schedules.py
index a4a7b5d87be7b..d1fabc9f3e6d4 100644
--- a/superset/tasks/schedules.py
+++ b/superset/tasks/schedules.py
@@ -105,6 +105,7 @@ class AlertContent(NamedTuple):
     label: str  # alert name
     sql: str  # sql statement for alert
     observation_value: str  # value from observation that triggered the alert
+    validation_error_message: str  # a string of the comparison that triggered an alert
     alert_url: str  # url to alert details
     image_data: Optional[ScreenshotData]  # data for the alert screenshot
 
@@ -571,20 +572,21 @@ def deliver_alert(
 
     # Set all the values for the alert report
     # Alternate values are used in the case of a test alert
-    # where an alert has no observations yet
+    # where an alert might not have a validator
     recipients = recipients or alert.recipients
     slack_channel = slack_channel or alert.slack_channel
-    sql = alert.sql_observer[0].sql if alert.sql_observer else ""
-    observation_value = (
-        str(alert.observations[-1].value) if alert.observations else "Value"
+    validation_error_message = (
+        str(alert.observations[-1].value) + " " + alert.validators[0].pretty_print()
+        if alert.validators
+        else ""
     )
 
-    # TODO: add sql query results and validator information to alert content
     if alert.slice:
         alert_content = AlertContent(
             alert.label,
-            sql,
-            observation_value,
+            alert.sql_observer[0].sql,
+            str(alert.observations[-1].value),
+            validation_error_message,
             _get_url_path("AlertModelView.show", user_friendly=True, pk=alert_id),
             _get_slice_screenshot(alert.slice.id),
         )
@@ -592,8 +594,9 @@ def deliver_alert(
         # TODO: dashboard delivery!
         alert_content = AlertContent(
             alert.label,
-            sql,
-            observation_value,
+            alert.sql_observer[0].sql,
+            str(alert.observations[-1].value),
+            validation_error_message,
             _get_url_path("AlertModelView.show", user_friendly=True, pk=alert_id),
             None,
         )
@@ -623,6 +626,7 @@ def deliver_email_alert(alert_content: AlertContent, recipients: str) -> None:
         label=alert_content.label,
         sql=alert_content.sql,
         observation_value=alert_content.observation_value,
+        validation_error_message=alert_content.validation_error_message,
         image_url=image_url,
     )
 
@@ -641,6 +645,7 @@ def deliver_slack_alert(alert_content: AlertContent, slack_channel: str) -> None
             label=alert_content.label,
             sql=alert_content.sql,
             observation_value=alert_content.observation_value,
+            validation_error_message=alert_content.validation_error_message,
             url=alert_content.image_data.url,
             alert_url=alert_content.alert_url,
         )
@@ -651,6 +656,7 @@ def deliver_slack_alert(alert_content: AlertContent, slack_channel: str) -> None
             label=alert_content.label,
             sql=alert_content.sql,
             observation_value=alert_content.observation_value,
+            validation_error_message=alert_content.validation_error_message,
             alert_url=alert_content.alert_url,
         )
 
diff --git a/superset/templates/email/alert.txt b/superset/templates/email/alert.txt
index 50ca6aa9cd491..964a57e3ccef2 100644
--- a/superset/templates/email/alert.txt
+++ b/superset/templates/email/alert.txt
@@ -17,9 +17,10 @@
  under the License.
 -->
 <h2 style="color: red;">Alert: {{label}} &#x26A0;</h2>
-<p><b>SQL Statement:</b></p>
+<p><b>Query:</b></p>
 <code><mark style="background-color: LightGrey; font-size: 1.1em">{{sql}}</mark></code></p>
-<p><b>SQL Result</b>: {{observation_value}}</p>
+<p><b>Result</b>: {{observation_value}}</p>
+<p><b>Reason</b>: {{validation_error_message}}</p>
 <p><a href="{{alert_url}}">View Alert Details</a></p>
 <p>Click <a href="{{image_url}}">here</a> or the image below to view the chart related to this alert.</p>
 <a href="{{image_url}}">
diff --git a/superset/templates/slack/alert.txt b/superset/templates/slack/alert.txt
index 80cfaa9c2c1ab..df943f5ef090e 100644
--- a/superset/templates/slack/alert.txt
+++ b/superset/templates/slack/alert.txt
@@ -17,7 +17,8 @@
   under the License.
 #}
 *Triggered Alert: {{label}} :redalert:*
-*SQL* *Statement*:```{{sql}}```
-*SQL* *Result*: {{observation_value}}
+*Query*:```{{sql}}```
+*Result*: {{observation_value}}
+*Reason*: {{validation_error_message}}
 <{{alert_url}}|View Alert Details>
 <{{url}}|*Explore in Superset*>
diff --git a/superset/templates/slack/alert_no_screenshot.txt b/superset/templates/slack/alert_no_screenshot.txt
index 4e31f36201754..bca191bb035a2 100644
--- a/superset/templates/slack/alert_no_screenshot.txt
+++ b/superset/templates/slack/alert_no_screenshot.txt
@@ -17,6 +17,7 @@
   under the License.
 #}
 *Triggered Alert: {{label}} :redalert:*
-*SQL* *Statement*:```{{sql}}```
-*SQL* *Result*: {{observation_value}}
+*Query*:```{{sql}}```
+*Result*: {{observation_value}}
+*Reason*: {{validation_error_message}}
 <{{alert_url}}|View Alert Details>
diff --git a/tests/alerts_tests.py b/tests/alerts_tests.py
index 6b5c503edc7e0..a2226107a01d8 100644
--- a/tests/alerts_tests.py
+++ b/tests/alerts_tests.py
@@ -310,7 +310,7 @@ def test_deliver_alert_screenshot(
     screenshot_mock, url_mock, email_mock, file_upload_mock, setup_database
 ):
     dbsession = setup_database
-    alert = create_alert(dbsession, "SELECT 55")
+    alert = create_alert(dbsession, "SELECT 55", "not null", "{}")
     observe(alert.id)
 
     screenshot = read_fixture("sample.png")
@@ -328,9 +328,10 @@ def test_deliver_alert_screenshot(
         "channels": alert.slack_channel,
         "file": screenshot,
         "initial_comment": f"\n*Triggered Alert: {alert.label} :redalert:*\n"
-        f"*SQL* *Statement*:```{alert.sql_observer[0].sql}```\n"
-        f"*SQL* *Result*: {alert.observations[-1].value}"
-        f"\n<http://0.0.0.0:8080/alert/show/{alert.id}"
+        f"*Query*:```{alert.sql_observer[0].sql}```\n"
+        f"*Result*: {alert.observations[-1].value}\n"
+        f"*Reason*: {alert.observations[-1].value} {alert.validators[0].pretty_print()}\n"
+        f"<http://0.0.0.0:8080/alert/show/{alert.id}"
         f"|View Alert Details>\n<http://0.0.0.0:8080/superset/slice/{alert.slice_id}/"
         "|*Explore in Superset*>",
         "title": f"[Alert] {alert.label}",