Send experiment review email fixes #397

.env.circle

@@ -13,3 +13,10 @@ REDASH_API_KEY=
 AWS_ACCESS_KEY=
 AWS_SECRET_KEY=
 S3_BUCKET_ID_STATS=
+EMAIL_REVIEW=
+EMAIL_SHIP=
+EMAIL_SENDER=
+EMAIL_HOST=
+EMAIL_PORT=
+EMAIL_HOST_USER=
+EMAIL_HOST_PASSWORD=

.env.sample

@@ -13,3 +13,10 @@ REDASH_API_KEY=
 AWS_ACCESS_KEY=
 AWS_SECRET_KEY=
 S3_BUCKET_ID_STATS=
+EMAIL_REVIEW=
+EMAIL_SHIP=
+EMAIL_SENDER=
+EMAIL_HOST=smtp
+EMAIL_PORT=25
+EMAIL_HOST_USER=
+EMAIL_HOST_PASSWORD=

Makefile

@@ -7,7 +7,10 @@ build:
 compose_build: build ssl
 	docker-compose build
 
-up: compose_build
+compose_kill:
+	docker-compose kill
+
+up: compose_kill compose_build
 	docker-compose up
 
 gunicorn: compose_build
@@ -46,7 +49,6 @@ bash: compose_build
 kill:
 	docker ps -a -q | xargs docker kill;docker ps -a -q | xargs docker rm
 
-
 ssl: nginx/key.pem nginx/cert.pem
 
 nginx/key.pem:

app/experimenter/experiments/constants.py

@@ -452,3 +452,18 @@ If additional QA is required, provide a plan for
 testing each branch of this study:
     """
     )
+
+    ATTENTION_MESSAGE = (
+        "This experiment requires special attention "
+        "and should be reviewed ASAP"
+    )
+
+    REVIEW_EMAIL_SUBJECT = "Experimenter Review Request: {name}"
+
+    REVIEW_EMAIL_TEMPLATE = (
+        """Please add the following experiment to the Shield review queue:
+
+{experiment_url}
+
+{attention}"""
+    )

app/experimenter/experiments/email.py

@@ -0,0 +1,24 @@
+from urllib.parse import urljoin
+
+from django.conf import settings
+from django.core.mail import send_mail
+
+
+def send_review_email(experiment, needs_attention):
+    attention = experiment.ATTENTION_MESSAGE if needs_attention else ""
+
+    content = experiment.REVIEW_EMAIL_TEMPLATE.format(
+        experiment_url=urljoin(
+            "https://{}".format(settings.HOSTNAME),
+            experiment.get_absolute_url(),
+        ),
+        attention=attention,
+    )
+
+    send_mail(
+        experiment.REVIEW_EMAIL_SUBJECT.format(name=experiment.name),
+        content,
+        settings.EMAIL_SENDER,
+        [settings.EMAIL_REVIEW],
+        fail_silently=False,
+    )

app/experimenter/experiments/forms.py

@@ -4,14 +4,15 @@ from django import forms
 from django.contrib.auth import get_user_model
 from django.utils.text import slugify
 
-from experimenter.projects.forms import AutoNameSlugFormMixin
-from experimenter.projects.models import Project
+from experimenter.experiments.constants import ExperimentConstants
+from experimenter.experiments.email import send_review_email
 from experimenter.experiments.models import (
     Experiment,
     ExperimentChangeLog,
     ExperimentVariant,
 )
-from experimenter.experiments.constants import ExperimentConstants
+from experimenter.projects.forms import AutoNameSlugFormMixin
+from experimenter.projects.models import Project
 
 
 class JSONField(forms.CharField):
@@ -380,21 +381,45 @@ class ExperimentStatusForm(
     ExperimentConstants, ChangeLogMixin, forms.ModelForm
 ):
 
+    attention = forms.CharField(required=False)
+
     class Meta:
         model = Experiment
-        fields = ("status",)
+        fields = ("status", "attention")
+
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.old_status = self.instance.status
+
+    @property
+    def new_status(self):
+        return self.cleaned_data["status"]
 
     def clean_status(self):
-        old_status = self.instance.status
-        new_status = self.cleaned_data["status"]
-        expected_new_status = new_status in self.STATUS_TRANSITIONS[old_status]
+        expected_status = (
+            self.new_status in self.STATUS_TRANSITIONS[self.old_status]
+        )
 
-        if old_status != new_status and not expected_new_status:
+        if self.old_status != self.new_status and not expected_status:
             raise forms.ValidationError(
                 (
                     "You can not change an Experiment's status "
                     "from {old_status} to {new_status}"
-                ).format(old_status=old_status, new_status=new_status)
+                ).format(
+                    old_status=self.old_status, new_status=self.new_status
+                )
             )
 
-        return new_status
+        return self.new_status
+
+    def save(self, *args, **kwargs):
+        experiment = super().save(*args, **kwargs)
+
+        if (
+            self.old_status == Experiment.STATUS_DRAFT
+            and self.new_status == Experiment.STATUS_REVIEW
+        ):
+            needs_attention = len(self.cleaned_data.get("attention", "")) > 0
+            send_review_email(experiment, needs_attention)
+
+        return experiment

app/experimenter/experiments/models.py

@@ -104,6 +104,10 @@ class Experiment(ExperimentConstants, models.Model):
         verbose_name = "Experiment"
         verbose_name_plural = "Experiments"
 
+    @models.permalink
+    def get_absolute_url(self):
+        return ("experiments-detail", (), {"slug": self.slug})
+
     @cached_property
     def control(self):
         return self.variants.filter(is_control=True).first()

app/experimenter/experiments/tests/test_email.py

@@ -0,0 +1,51 @@
+import mock
+from django.test import TestCase
+from django.conf import settings
+
+from experimenter.experiments.email import send_review_email
+from experimenter.experiments.tests.factories import ExperimentFactory
+
+
+class TestSendReviewEmail(TestCase):
+
+    def setUp(self):
+        super().setUp()
+        mock_send_mail_patcher = mock.patch(
+            "experimenter.experiments.email.send_mail"
+        )
+        self.mock_send_mail = mock_send_mail_patcher.start()
+        self.addCleanup(mock_send_mail_patcher.stop)
+
+    def test_send_review_email_without_needs_attention(self):
+        experiment = ExperimentFactory.create(
+            name="Experiment", slug="experiment"
+        )
+        send_review_email(experiment, False)
+        self.mock_send_mail.assert_called_with(
+            "Experimenter Review Request: Experiment",
+            (
+                "Please add the following experiment to the Shield review "
+                "queue:\n\nhttps://localhost/experiments/experiment/\n\n"
+            ),
+            settings.EMAIL_SENDER,
+            [settings.EMAIL_REVIEW],
+            fail_silently=False,
+        )
+
+    def test_send_review_email_with_needs_attention(self):
+        experiment = ExperimentFactory.create(
+            name="Experiment", slug="experiment"
+        )
+        send_review_email(experiment, True)
+        self.mock_send_mail.assert_called_with(
+            "Experimenter Review Request: Experiment",
+            (
+                "Please add the following experiment to the Shield review "
+                "queue:\n\nhttps://localhost/experiments/experiment/\n\n"
+                "This experiment requires special attention and "
+                "should be reviewed ASAP"
+            ),
+            settings.EMAIL_SENDER,
+            [settings.EMAIL_REVIEW],
+            fail_silently=False,
+        )

app/experimenter/experiments/tests/test_forms.py

@@ -411,6 +411,14 @@ class TestExperimentRisksForm(MockRequestMixin, TestCase):
 
 class TestExperimentStatusForm(MockRequestMixin, TestCase):
 
+    def setUp(self):
+        super().setUp()
+        mock_send_mail_patcher = mock.patch(
+            "experimenter.experiments.email.send_mail"
+        )
+        self.mock_send_mail = mock_send_mail_patcher.start()
+        self.addCleanup(mock_send_mail_patcher.stop)
+
     def test_form_allows_valid_state_transition_and_creates_changelog(self):
         experiment = ExperimentFactory.create_with_status(
             Experiment.STATUS_DRAFT
@@ -437,3 +445,16 @@ class TestExperimentStatusForm(MockRequestMixin, TestCase):
             instance=experiment,
         )
         self.assertFalse(form.is_valid())
+
+    def test_sends_review_mail_when_draft_becomes_review(self):
+        experiment = ExperimentFactory.create_with_status(
+            Experiment.STATUS_DRAFT
+        )
+        form = ExperimentStatusForm(
+            request=self.request,
+            data={"status": experiment.STATUS_REVIEW},
+            instance=experiment,
+        )
+        self.assertTrue(form.is_valid())
+        form.save()
+        self.mock_send_mail.assert_called()

app/experimenter/settings.py

@@ -38,6 +38,7 @@ HOSTNAME = config("HOSTNAME")
 
 ALLOWED_HOSTS = [HOSTNAME]
 
+SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https")
 
 # Application definition
 
@@ -213,3 +214,20 @@ CORS_ORIGIN_ALLOW_ALL = True
 # reDash Rate Limit
 # Number of dashboards to deploy per hour
 DASHBOARD_RATE_LIMIT = 2
+
+# Automated email destinations
+
+# SMTP configuration
+EMAIL_SENDER = config("EMAIL_SENDER")
+EMAIL_HOST = config("EMAIL_HOST")
+EMAIL_PORT = config("EMAIL_PORT")
+EMAIL_HOST_USER = config("EMAIL_HOST_USER")
+EMAIL_HOST_PASSWORD = config("EMAIL_HOST_PASSWORD")
+EMAIL_USE_TLS = not DEBUG
+EMAIL_USE_SSL = False
+
+# Email to send to when an experiment is ready for review
+EMAIL_REVIEW = config("EMAIL_REVIEW")
+
+# Email to send to when an experiment is ready to ship
+EMAIL_SHIP = config("EMAIL_SHIP")

app/experimenter/templates/experiments/detail.html

@@ -26,7 +26,6 @@
         <h4 class="col-md-8 md-mt-0">
           Overview
         </h4>
-
       </div>
 
       <div class="row card md-pt-15 md-pb-10">
@@ -64,7 +63,6 @@
         <h4 class="col-md-8 md-mt-0">
           Population
         </h4>
-
       </div>
 
       <div class="row card md-pt-15 md-pb-10">
@@ -90,7 +88,6 @@
           {% endif %}
           Firefox Pref &amp; Branches
         </h4>
-
       </div>
 
       <div class="row card md-pt-15 md-pb-10">
@@ -132,7 +129,6 @@
           {% endif %}
           Objectives
         </h4>
-
       </div>
 
       <div class="row card md-pt-15 md-pb-10">
@@ -162,7 +158,6 @@
           {% endif %}
           Analysis
         </h4>
-
       </div>
 
       <div class="row card md-pt-15 md-pb-10">
@@ -275,7 +270,6 @@
           {% endif %}
           Testing Plan
         </h4>
-
       </div>
 
       <div class="row card md-pt-15 md-pb-10">
@@ -311,6 +305,13 @@
 
             <input type="hidden" name="status" value="{{ experiment.STATUS_REVIEW }}">
 
+            <div class="checkbox">
+              <label>
+                <input type="checkbox" name="attention" value="{{ experiment.ATTENTION_MESSAGE }}"> 
+                {{ experiment.ATTENTION_MESSAGE }}
+              </label>
+            </div> 
+
             <input
               type="submit"
               class="col-md-12 md-mb-10 btn btn-success"

docker-compose.yml

@@ -8,6 +8,7 @@ services:
     tty: true
     links:
       - db
+      - smtp
     volumes:
       - ./app:/app
       - static_volume:/app/experimenter/served/
@@ -38,6 +39,16 @@ services:
     networks:
       - private_nw
 
+  smtp:
+    image: python:3.6
+    ports:
+      - "25:25"
+    networks:
+      - private_nw
+    environment:
+      - PYTHONUNBUFFERED=1
+    command: python -m smtpd -n -c DebuggingServer 0.0.0.0:25
+
 volumes:
   db_volume:
   static_volume:

nginx/nginx.conf

@@ -32,11 +32,12 @@ http {
         server_name localhost;
 
         ssl_certificate     cert.pem;
-        ssl_certificate_key key.pem;				
+        ssl_certificate_key key.pem;
         client_max_body_size 20M;
 
         location / {
             proxy_pass http://app:7001/;
+            proxy_set_header X-Forwarded-Proto $scheme;
             proxy_set_header Host $host;
             proxy_set_header x-forwarded-user "dev@example.com";
         }