From e5aa17a30f349e68b918cddb9980f96f07f55c06 Mon Sep 17 00:00:00 2001
From: Tanner Prestegard <tanner.prestegard@ligo.org>
Date: Wed, 12 Dec 2018 19:25:06 -0600
Subject: [PATCH] Updates and bugfixes for phone and email alerts

---
 config/settings/base.py             |   4 +-
 gracedb/alerts/email.py             | 116 +++++++++++++++-------------
 gracedb/alerts/events/utils.py      |   5 ++
 gracedb/alerts/main.py              |  23 ++----
 gracedb/alerts/phone.py             |  16 ++--
 gracedb/alerts/superevents/utils.py |   5 ++
 6 files changed, 90 insertions(+), 79 deletions(-)

diff --git a/config/settings/base.py b/config/settings/base.py
index 21f5029c9..c5fd16f4e 100644
--- a/config/settings/base.py
+++ b/config/settings/base.py
@@ -53,8 +53,8 @@ SECURE_PROXY_SSL_HEADER = ('HTTP_X_FORWARDED_PROTO', 'https')
 TWIML_BASE_URL = 'https://handler.twilio.com/twiml/'
 # TwiML bin SIDs (for Twilio)
 TWIML_BIN = {
-    'create': 'EH761b6a35102737e3d21830a484a98a08',
-    'label': 'EHb596a53b9c92a41950ce1a47335fd834',
+    'new': 'EH761b6a35102737e3d21830a484a98a08',
+    'label_added': 'EHb596a53b9c92a41950ce1a47335fd834',
     'test': 'EH6c0a168b0c6b011047afa1caeb49b241',
 }
 
diff --git a/gracedb/alerts/email.py b/gracedb/alerts/email.py
index f2ffc108d..026827375 100644
--- a/gracedb/alerts/email.py
+++ b/gracedb/alerts/email.py
@@ -1,18 +1,39 @@
 from __future__ import absolute_import
 import logging
 
-from django.core.mail import EmailMessage
 from django.conf import settings
+from django.core.mail import EmailMessage
+from django.urls import reverse
 
 from core.time_utils import gpsToUtc
+from core.urls import build_absolute_uri
 
 # Set up logger
-log = logging.getLogger(__name__)
+logger = logging.getLogger(__name__)
+
+EMAIL_SUBJECT_NEW = "[gracedb] {pipeline} event. ID: {graceid}"
+EMAIL_MESSAGE_NEW = """
+New Event
+{group} / {pipeline}
+GRACEID:   {graceid}
+Info:      {url}
+Data:      {file_url}
+Submitter: {submitter}
+Event Summary:
+{summary}
+"""
+
+EMAIL_SUBJECT_LABEL = "[gracedb] {label} / {pipeline} / {search} / {graceid}"
+EMAIL_SUBJECT_LABEL_NOSEARCH = "[gracedb] {label} / {pipeline} / {graceid}"
+EMAIL_MESSAGE_LABEL = """
+A {pipeline} event with graceid {graceid} was labeled with {label}: {url}
+"""
 
 
 def indent(nindent, text):
     return "\n".join([(nindent*' ')+line for line in text.split('\n')])
 
+
 def prepareSummary(event):
     gpstime = event.gpstime
     utctime = gpsToUtc(gpstime).strftime("%Y-%m-%d %H:%M:%S")
@@ -31,60 +52,47 @@ def prepareSummary(event):
     Component masses: %.2f, %.2f """ % (si.mass1, si.mass2)
     return summary
 
-def issue_email_alerts(event, event_url):
 
-    # Check settings switch for turning off email alerts
-    if not settings.SEND_EMAIL_ALERTS:
-        return
+def issue_email_alerts(event, recips, label=None):
 
-    # The right way of doing this is to make the email alerts filter-able
-    # by search. But this is a low priority dev task. For now, we simply 
-    # short-circuit in case this is an MDC event.
-    if event.search and event.search.name == 'MDC':
-        return
+    # Prepare URLs for email message body
+    event_url = build_absolute_uri(reverse("view", args=[event.graceid]))
+    file_url = build_absolute_uri(reverse("file_list", args=[event.graceid]))
 
-    # Gather Recipients
-    if event.group.name == 'Test':
-        fromaddress = settings.ALERT_TEST_EMAIL_FROM
-        toaddresses = settings.ALERT_TEST_EMAIL_TO
-        bccaddresses = []
+    # Compile subject and message content
+    if label is None:
+        # Alert for new event
+        subject = EMAIL_SUBJECT_NEW.format(pipeline=event.pipeline.name,
+            graceid=event.graceid)
+        message = EMAIL_MESSAGE_NEW.format(group=event.group.name,
+            pipeline=event.pipeline.name, graceid=event.graceid,
+            url=event_url, file_url=file_url,
+            submitter=event.submitter.get_full_name(),
+            summary=indent(3, prepareSummary(event)))
     else:
-        fromaddress = settings.ALERT_EMAIL_FROM
-        toaddresses = settings.ALERT_EMAIL_TO
-        # XXX Bizarrely, this settings.ALERT_EMAIL_BCC seems to be overwritten in a 
-        # persistent way between calls, so that you can get alerts going out to the 
-        # wrong contacts. I find that it works if you just start with an empty list
-        # See: https://bugs.ligo.org/redmine/issues/2185
-        #bccaddresses = settings.ALERT_EMAIL_BCC
-        bccaddresses = []
-        pipeline = event.pipeline
-        triggers = pipeline.trigger_set.filter(labels=None)
-        for trigger in triggers:
-            for recip in trigger.contacts.all():
-                if ((event.far and event.far < trigger.farThresh)
-                    or not trigger.farThresh):
-                    if recip.email:
-                        bccaddresses.append(recip.email)
-
-    subject = "[gracedb] %s event. ID: %s" % (event.pipeline.name, event.graceid)
-    message = """
-New Event
-%s / %s
-GRACEID:   %s
-Info:      %s
-Data:      %s
-Submitter: %s
-Event Summary:
-%s
-"""
-    message %= (event.group.name,
-                event.pipeline.name,
-                event.graceid,
-                event_url,
-                event.weburl(),
-                "%s %s" % (event.submitter.first_name, event.submitter.last_name),
-                indent(3, prepareSummary(event))
-               )
-
-    email = EmailMessage(subject, message, fromaddress, toaddresses, bccaddresses)
+        # Alert for label
+        if event.search:
+            subject = EMAIL_SUBJECT_LABEL.format(label=label.name,
+                pipeline=event.pipeline.name, search=event.search.name,
+                graceid=event.graceid)
+        else:
+            subject = EMAIL_SUBJECT_LABEL_NOSEARCH.format(label=label.name,
+                pipeline=event.pipeline.name, graceid=event.graceid)
+        message = EMAIL_MESSAGE_LABEL.format(pipeline=event.pipeline.name,
+            graceid=event.graceid, label=label.name, url=event_url)
+
+    # Actual recipients should be BCC'd
+    bcc_addresses = [recip.email for recip in recips]
+
+    # Compile from/to addresses
+    from_address = settings.ALERT_EMAIL_FROM
+    to_addresses = settings.ALERT_EMAIL_TO
+
+    # Log email recipients
+    logger.debug("Sending email to {recips}".format(
+        recips=", ".join(bcc_addresses)))
+
+    # Send email
+    email = EmailMessage(subject, message, from_address, to_addresses,
+        bcc_addresses)
     email.send()
diff --git a/gracedb/alerts/events/utils.py b/gracedb/alerts/events/utils.py
index 74c0db790..4bc9824fb 100644
--- a/gracedb/alerts/events/utils.py
+++ b/gracedb/alerts/events/utils.py
@@ -66,6 +66,11 @@ class EventLabelAlertIssuer(AlertIssuerWithParentEvent):
     serializer_class = staticmethod(labelToDict)
     alert_types = ['label_added', 'label_removed']
 
+    def issue_alerts(self):
+        issue_alerts(self.get_parent_obj(), self.alert_type,
+            self.serialize_obj(), self.serialize_parent(),
+            label=self.obj.label)
+
 
 class EventVOEventAlertIssuer(AlertIssuerWithParentEvent):
     serializer_class = staticmethod(voeventToDict)
diff --git a/gracedb/alerts/main.py b/gracedb/alerts/main.py
index 5aa68e42d..475c8dd6c 100644
--- a/gracedb/alerts/main.py
+++ b/gracedb/alerts/main.py
@@ -39,7 +39,7 @@ def get_alert_recips(event_or_superevent):
         # Filter on FAR threshold requirements
         query = Q(farThresh__isnull=True)
         if event.far:
-            query |= Q(farThresh__lt=event.far)
+            query |= Q(farThresh__gt=event.far)
         triggers = triggers.filter(query)
         # Contacts for all triggers, make sure user is in LVC group (safeguard)
         contacts = Contact.objects.filter(trigger__in=triggers,
@@ -52,8 +52,8 @@ def get_alert_recips(event_or_superevent):
 
 def get_alert_recips_for_label(event_or_superevent, label):
     # Blank QuerySets for recipients
-    email_recips = QuerySet()
-    phone_recips = QuerySet()
+    email_recips = Contact.objects.none()
+    phone_recips = Contact.objects.none()
 
     # Construct a queryset containing only this object; needed for
     # call to filter_for_labels
@@ -97,7 +97,7 @@ def get_alert_recips_for_label(event_or_superevent, label):
 
 
 def issue_alerts(event_or_superevent, alert_type, serialized_object,
-    serialized_parent=None):
+    serialized_parent=None, label=None):
 
     # Send XMPP alert
     if settings.SEND_XMPP_ALERTS:
@@ -128,21 +128,14 @@ def issue_alerts(event_or_superevent, alert_type, serialized_object,
         if event.offline:
             return
 
-    # Compile phone and email recipients for new or label alerts
-    if alert_type not in ["new", "label"]:
-        return
-
     if alert_type == "new":
         email_recips, phone_recips = get_alert_recips(event_or_superevent)
-        # Force label = None for new alerts
-        label = None
-    elif alert_type == "label":
+    elif alert_type == "label_added":
         email_recips, phone_recips = \
             get_alert_recips_for_label(event_or_superevent, label)
 
-    if settings.SEND_EMAIL_ALERTS:
-        url = reverse('view', args=[event_or_superevent.graceid])
-        issue_email_alerts(event_or_superevent, url)
+    if settings.SEND_EMAIL_ALERTS and email_recips.exists():
+        issue_email_alerts(event_or_superevent, email_recips, label=label)
 
-    if settings.SEND_PHONE_ALERTS and phone_recips:
+    if settings.SEND_PHONE_ALERTS and phone_recips.exists():
         issue_phone_alerts(event_or_superevent, phone_recips, label=label)
diff --git a/gracedb/alerts/phone.py b/gracedb/alerts/phone.py
index f1d22956e..118975393 100644
--- a/gracedb/alerts/phone.py
+++ b/gracedb/alerts/phone.py
@@ -3,7 +3,11 @@ import logging
 import socket
 
 from django.conf import settings
+from django.urls import reverse
+
 from django_twilio.client import twilio_client
+
+from core.urls import build_absolute_uri
 from events.permission_utils import is_external
 
 # Set up logger
@@ -20,13 +24,11 @@ TWIML_ARG_STR = {
               '&server={server}'),
 }
 
-# TODO: fix these by using reverse
 # Dict for managing Twilio message contents.
 TWILIO_MSG_CONTENT = {
-    'new': ('A {pipeline} event with GraceDB ID {graceid} was created.'
-               ' https://{server}.ligo.org/events/view/{graceid}'),
+    'new': 'A {pipeline} event with GraceDB ID {graceid} was created. {url}',
     'label_added': ('A {pipeline} event with GraceDB ID {graceid} was labeled '
-              'with {label}. https://{server}.ligo.org/events/view/{graceid}')
+              'with {label}. {url}'),
 }
 
 
@@ -55,9 +57,6 @@ def issue_phone_alerts(event, contacts, label=None):
     else:
         alert_type = "new"
 
-    # Get server name.
-    hostname = socket.gethostname()
-
     # Get "from" phone number.
     from_ = get_twilio_from()
 
@@ -65,7 +64,8 @@ def issue_phone_alerts(event, contacts, label=None):
     msg_params = {
         'pipeline': event.pipeline.name,
         'graceid': event.graceid,
-        'server': hostname,
+        'server': settings.SERVER_HOSTNAME,
+        'url': build_absolute_uri(reverse('view', args=[event.graceid])),
     }
     if alert_type == "label_added":
         msg_params['label'] = label.name
diff --git a/gracedb/alerts/superevents/utils.py b/gracedb/alerts/superevents/utils.py
index b36931796..10e92a8e1 100644
--- a/gracedb/alerts/superevents/utils.py
+++ b/gracedb/alerts/superevents/utils.py
@@ -43,6 +43,11 @@ class SupereventLabelAlertIssuer(AlertIssuerWithParentSuperevent):
     serializer_class = SupereventLabelSerializer
     alert_types = ['label_added', 'label_removed']
 
+    def issue_alerts(self):
+        issue_alerts(self.get_parent_obj(), self.alert_type,
+            self.serialize_obj(), self.serialize_parent(),
+            label=self.obj.label)
+
 
 class SupereventVOEventAlertIssuer(AlertIssuerWithParentSuperevent):
     serializer_class = SupereventVOEventSerializer
-- 
GitLab