Intent to ship email fixes #747 (#1147)

* Intent to Ship Email

Fixes #747

* Intent to Ship Email

Part of #747

* Clean up email and code

- No need to pass all these template arguments separately since
  they're largely available on the experiment themselves.

- Do some weird stuff to try to minimize number of extra blank lines
  when risk/technical complexity are absent.

- Disable autoescaping since this is a text email.

- Add tests.

* Help text should not be inside the label

* Add API to send intent-to-ship email

* Add front-end interface to send email

* experiment_url should already include the protocol

Thanks @jaredkerim for pointing this out.

* Move prefetch of locales and countries to models

Thanks @jaredkerim for the suggestion.

* Show button only if email has not been sent

Thanks @jaredkerim for the suggestion.

* Just refresh the page rather than doing anything fancy

Thanks @jaredkerim for the suggestion.

* Fix test coverage

These utility methods in the test suite do not need 100% coverage.

* Send email to release drivers and experiment owner

Adding a CC requires using the EmailMessage API directly instead of
the send_mail wrapper.

* Add setting to test configuration

.env.sample

@@ -12,6 +12,7 @@ REDIS_PORT=
 REDIS_DB=
 OPENIDC_CLIENT_ID=
 OPENIDC_CLIENT_SECRET=
+EMAIL_RELEASE_DRIVERS=
 EMAIL_REVIEW=
 EMAIL_SHIP=
 EMAIL_SENDER=

.env.test

@@ -12,6 +12,7 @@ REDIS_PORT=6379
 REDIS_DB=0
 OPENIDC_CLIENT_ID=
 OPENIDC_CLIENT_SECRET=
+EMAIL_RELEASE_DRIVERS=
 EMAIL_REVIEW=
 EMAIL_SHIP=
 EMAIL_SENDER=

app/experimenter/experiments/api_urls.py

@@ -5,6 +5,7 @@ from experimenter.experiments.api_views import (
     ExperimentDetailView,
     ExperimentListView,
     ExperimentRejectView,
+    ExperimentSendIntentToShipEmailView,
 )
 
 
@@ -19,6 +20,11 @@ urlpatterns = [
         ExperimentRejectView.as_view(),
         name="experiments-api-reject",
     ),
+    url(
+        r"^(?P<slug>[\w-]+)/intent-to-ship-email$",
+        ExperimentSendIntentToShipEmailView.as_view(),
+        name="experiments-api-send-intent-to-ship-email",
+    ),
     url(
         r"^(?P<slug>[\w-]+)/$",
         ExperimentDetailView.as_view(),

app/experimenter/experiments/api_views.py

@@ -1,8 +1,10 @@
 from rest_framework.generics import ListAPIView, UpdateAPIView, RetrieveAPIView
 from rest_framework.response import Response
+from rest_framework import status
 
 from experimenter.experiments.models import Experiment, ExperimentChangeLog
 from experimenter.experiments.serializers import ExperimentSerializer
+from experimenter.experiments import email
 
 
 class ExperimentListView(ListAPIView):
@@ -60,3 +62,24 @@ class ExperimentRejectView(UpdateAPIView):
         )
 
         return Response()
+
+
+class ExperimentSendIntentToShipEmailView(UpdateAPIView):
+    lookup_field = "slug"
+    queryset = Experiment.objects.filter(status=Experiment.STATUS_REVIEW)
+
+    def update(self, request, *args, **kwargs):
+        experiment = self.get_object()
+
+        if experiment.review_intent_to_ship:
+            return Response(
+                {"error": "email-already-sent"},
+                status=status.HTTP_409_CONFLICT,
+            )
+
+        email.send_intent_to_ship_email(experiment.id)
+
+        experiment.review_intent_to_ship = True
+        experiment.save()
+
+        return Response()

app/experimenter/experiments/constants.py

@@ -771,6 +771,10 @@ If applicable, link to any relevant test builds / staging information
 {attention}"""
     )
 
+    INTENT_TO_SHIP_EMAIL_SUBJECT = (
+        "SHIELD Study Intent to ship: {name} {version} {channel}"
+    )
+
     BUGZILLA_OVERVIEW_TEMPLATE = (
         """
 {experiment.name}

app/experimenter/experiments/email.py

@@ -2,6 +2,8 @@ from urllib.parse import urljoin
 
 from django.conf import settings
 from django.core.mail import send_mail
+from django.core.mail.message import EmailMessage
+from django.template.loader import render_to_string
 
 from experimenter.experiments.models import Experiment
 
@@ -22,3 +24,38 @@ def send_review_email(experiment_name, experiment_url, needs_attention):
         [settings.EMAIL_REVIEW],
         fail_silently=False,
     )
+
+
+def send_intent_to_ship_email(experiment_id):
+    experiment = Experiment.objects.get(id=experiment_id)
+
+    bug_url = settings.BUGZILLA_DETAIL_URL.format(id=experiment.bugzilla_id)
+
+    # Because that's how it's done in Experiment.population (property)
+    percent_of_population = f"{float(experiment.population_percent):g}%"
+
+    content = render_to_string(
+        "experiments/intent_to_ship.txt",
+        {
+            "experiment": experiment,
+            "bug_url": bug_url,
+            "percent_of_population": percent_of_population,
+            "locales": [str(l) for l in experiment.locales.all()],
+            "countries": [str(c) for c in experiment.countries.all()],
+        },
+    )
+    # Strip extra newlines from autoescape tag
+    content = content.strip() + "\n"
+
+    version = experiment.firefox_version
+    channel = experiment.firefox_channel
+    email = EmailMessage(
+        Experiment.INTENT_TO_SHIP_EMAIL_SUBJECT.format(
+            name=experiment.name, version=version, channel=channel
+        ),
+        content,
+        settings.EMAIL_SENDER,
+        [settings.EMAIL_RELEASE_DRIVERS],
+        cc=[experiment.owner.email],
+    )
+    email.send(fail_silently=False)

app/experimenter/experiments/models.py

@@ -27,6 +27,8 @@ class ExperimentManager(models.Manager):
                 "owner",
                 "comments",
                 "comments__created_by",
+                "locales",
+                "countries",
             )
             .annotate(latest_change=Max("changes__changed_on"))
         )

app/experimenter/experiments/tests/test_api_views.py

@@ -1,6 +1,7 @@
 import json
 
 from django.conf import settings
+from django.core import mail
 from django.test import TestCase
 from django.urls import reverse
 
@@ -191,3 +192,43 @@ class TestExperimentRejectView(TestCase):
         )
 
         self.assertEqual(response.status_code, 404)
+
+
+class TestExperimentSendIntentToShipEmailView(TestCase):
+
+    def test_put_to_view_sends_email(self):
+        user_email = "user@example.com"
+
+        experiment = ExperimentFactory.create_with_variants(
+            review_intent_to_ship=False, status=Experiment.STATUS_REVIEW
+        )
+        old_outbox_len = len(mail.outbox)
+
+        response = self.client.put(
+            reverse(
+                "experiments-api-send-intent-to-ship-email",
+                kwargs={"slug": experiment.slug},
+            ),
+            **{settings.OPENIDC_EMAIL_HEADER: user_email},
+        )
+
+        self.assertEqual(response.status_code, 200)
+
+        experiment = Experiment.objects.get(pk=experiment.pk)
+        self.assertEqual(experiment.review_intent_to_ship, True)
+        self.assertEqual(len(mail.outbox), old_outbox_len + 1)
+
+    def test_put_raises_409_if_email_already_sent(self):
+        experiment = ExperimentFactory.create_with_variants(
+            review_intent_to_ship=True, status=Experiment.STATUS_REVIEW
+        )
+
+        response = self.client.put(
+            reverse(
+                "experiments-api-send-intent-to-ship-email",
+                kwargs={"slug": experiment.slug},
+            ),
+            **{settings.OPENIDC_EMAIL_HEADER: "user@example.com"},
+        )
+
+        self.assertEqual(response.status_code, 409)

app/experimenter/experiments/tests/test_email.py

@@ -2,7 +2,10 @@ from django.test import TestCase
 from django.conf import settings
 from django.core import mail
 
-from experimenter.experiments.email import send_review_email
+from experimenter.experiments.email import (
+    send_review_email,
+    send_intent_to_ship_email,
+)
 from experimenter.experiments.tests.factories import ExperimentFactory
 
 
@@ -48,3 +51,139 @@ class TestSendReviewEmail(TestCase):
         )
         self.assertEqual(sent_email.from_email, settings.EMAIL_SENDER)
         self.assertEqual(sent_email.recipients(), [settings.EMAIL_REVIEW])
+
+
+class TestIntentToShipEmail(TestCase):
+    maxDiff = None
+
+    def test_send_intent_to_ship_email_with_risk_fields(self):
+        risk_description = "Hardcoded fictitious technical challenge"
+        experiment = ExperimentFactory.create(
+            name="Experiment",
+            slug="experiment",
+            risks="Hardcoded fictitious risk",
+            risk_technical_description=risk_description,
+            population_percent=10.0,
+        )
+        send_intent_to_ship_email(experiment.id)
+
+        sent_email = mail.outbox[-1]
+        self.verify_subject(experiment, sent_email)
+
+        bug_url = settings.BUGZILLA_DETAIL_URL.format(
+            id=experiment.bugzilla_id
+        )
+        expected_locales = self.format_locales(experiment)
+        expected_countries = self.format_countries(experiment)
+        expected_body = (
+            f"""
+Hello Release Drivers,
+
+This request is coming from information entered in Experimenter.
+Please reach out to the person(s) on cc: with any questions, details,
+or discussion. They will email an update if any of the key information
+changes. Otherwise they will reach out once the study has fully passed
+QA for Release Management sign-off.
+
+Experimenter Bug: {bug_url}
+Experimenter URL: {experiment.experiment_url}
+Study owner: {experiment.owner.email}
+Description: {experiment.short_description}
+Timeline & Channel: {experiment.firefox_version} {experiment.firefox_channel}
+Intended study dates: {experiment.dates}
+Percent of Population: 10%
+Platforms: {experiment.platform}
+Locales: {expected_locales}; {expected_countries}
+QA Status: {experiment.qa_status}
+Meta Bug: {experiment.feature_bugzilla_url}
+Related links: {experiment.related_work}
+Risk: Hardcoded fictitious risk
+Technical Complexity: Hardcoded fictitious technical challenge
+
+Thank you!!
+""".lstrip()
+        )
+
+        self.assertEqual(expected_body, sent_email.body)
+        self.assertEqual(sent_email.from_email, settings.EMAIL_SENDER)
+        self.assertEqual(
+            sent_email.recipients(),
+            [settings.EMAIL_RELEASE_DRIVERS, experiment.owner.email],
+        )
+
+    def test_send_intent_to_ship_email_without_risk_fields(self):
+        experiment = ExperimentFactory.create(
+            name="Experiment",
+            slug="experiment",
+            risks="",
+            risk_technical_description="",
+            population_percent=10.0,
+        )
+        send_intent_to_ship_email(experiment.id)
+
+        sent_email = mail.outbox[-1]
+        self.verify_subject(experiment, sent_email)
+
+        bug_url = settings.BUGZILLA_DETAIL_URL.format(
+            id=experiment.bugzilla_id
+        )
+        expected_locales = self.format_locales(experiment)
+        expected_countries = self.format_countries(experiment)
+        expected_body = (
+            f"""
+Hello Release Drivers,
+
+This request is coming from information entered in Experimenter.
+Please reach out to the person(s) on cc: with any questions, details,
+or discussion. They will email an update if any of the key information
+changes. Otherwise they will reach out once the study has fully passed
+QA for Release Management sign-off.
+
+Experimenter Bug: {bug_url}
+Experimenter URL: {experiment.experiment_url}
+Study owner: {experiment.owner.email}
+Description: {experiment.short_description}
+Timeline & Channel: {experiment.firefox_version} {experiment.firefox_channel}
+Intended study dates: {experiment.dates}
+Percent of Population: 10%
+Platforms: {experiment.platform}
+Locales: {expected_locales}; {expected_countries}
+QA Status: {experiment.qa_status}
+Meta Bug: {experiment.feature_bugzilla_url}
+Related links: {experiment.related_work}
+
+Thank you!!
+""".lstrip()
+        )
+
+        self.assertEqual(expected_body, sent_email.body)
+        self.assertEqual(sent_email.from_email, settings.EMAIL_SENDER)
+        self.assertEqual(
+            sent_email.recipients(),
+            [settings.EMAIL_RELEASE_DRIVERS, experiment.owner.email],
+        )
+
+    def format_locales(self, experiment):
+        locales = "All locales"
+        if experiment.locales.exists():  # pragma: no branch
+            locales = ", ".join(
+                str(locale) for locale in experiment.locales.all()
+            )
+        return locales
+
+    def format_countries(self, experiment):
+        countries = "All countries"
+        if experiment.countries.exists():  # pragma: no branch
+            countries = ", ".join(
+                str(country) for country in experiment.countries.all()
+            )
+        return countries
+
+    def verify_subject(self, experiment, email):
+        expected_subject = "".join(
+            [
+                "SHIELD Study Intent to ship: Experiment ",
+                f"{experiment.firefox_version} {experiment.firefox_channel}",
+            ]
+        )
+        self.assertEqual(email.subject, expected_subject)

app/experimenter/settings.py

@@ -253,6 +253,9 @@ EMAIL_REVIEW = config("EMAIL_REVIEW")
 # Email to send to when an experiment is ready to ship
 EMAIL_SHIP = config("EMAIL_SHIP")
 
+# Email to send to when an experiment is being signed-off
+EMAIL_RELEASE_DRIVERS = config("EMAIL_RELEASE_DRIVERS")
+
 # Bugzilla API Integration
 BUGZILLA_HOST = config("BUGZILLA_HOST")
 BUGZILLA_API_KEY = config("BUGZILLA_API_KEY")

app/experimenter/settings_test.py

@@ -15,5 +15,6 @@ HOSTNAME = "experimenter.moz"
 EMAIL_REVIEW = "testreview@example.com"
 EMAIL_SHIP = "testship@example.com"
 EMAIL_SENDER = "sender@example.com"
+EMAIL_RELEASE_DRIVERS = "release.drivers@example.com"
 
 USE_GOOGLE_ANALYTICS = True

app/experimenter/static/js/detail-review.js

@@ -0,0 +1,25 @@
+// Hook up intent-to-ship email
+jQuery(function($) {
+  let sendUrl;
+  $("button.send-intent-to-ship").on("click", function(e) {
+    sendUrl = this.dataset.url;
+    // Let Bootstrap handle showing the modal itself
+  });
+
+  const modal = $("#send-intent-to-ship-modal");
+
+  modal.find("button.send").on("click", async function(e) {
+    this.innerHTML = "Sending...";
+    this.disabled = true;
+    const resp = await fetch(sendUrl, {
+      method: "PUT",
+    });
+    if (resp.status == 200) {
+      // Rather than trying to update the DOM to match the new state,
+      // just refresh the page.
+      location.reload();
+    } else {
+      modal.find(".sending-failed").toggleClass("d-none");
+    }
+  });
+});

app/experimenter/templates/experiments/detail_review.html

@@ -1,5 +1,7 @@
 {% extends "experiments/detail_base.html" %}
 
+{% load static %}
+
 {% block main_sidebar_extra %}
   {% if experiment.is_ready_to_launch %}
     <form
@@ -34,11 +36,20 @@
 
         {% for field in form.required_reviews %}
             <div class="checkbox">
-              <label>
-                {{ field }}
-                {{ field.label }}
-                {{ field.help_text|safe }}
-              </label>
+              {% if field.name == 'review_intent_to_ship' and not experiment.review_intent_to_ship %}
+                <button type="button" class="btn btn-info send-intent-to-ship"
+                  data-toggle="modal" data-target="#send-intent-to-ship-modal"
+                  data-url="{% url 'experiments-api-send-intent-to-ship-email' experiment.slug %}"
+                >
+                  <span class="fas fa-envelope"></span> Send intent-to-ship email
+                </button>
+              {% else %}
+                <label>
+                  {{ field }}
+                  {{ field.label }}
+                </label>
+              {% endif %}
+              {{ field.help_text|safe }}
             </div>
         {% endfor %}
 
@@ -49,8 +60,8 @@
               <label>
                 {{ field }}
                 {{ field.label }}
-                {{ field.help_text|safe }}
               </label>
+              {{ field.help_text|safe }}
             </div>
         {% endfor %}
 
@@ -77,3 +88,35 @@
     </button>
   </form>
 {% endblock %}
+
+{% block main_content %}
+  {{ block.super }}
+  <div id="send-intent-to-ship-modal" class="modal" tabindex="-1" role="dialog">
+    <div class="modal-dialog" role="document">
+      <div class="modal-content">
+        <div class="modal-header">
+          <h5 class="modal-title">Send email?</h5>
+          <button type="button" class="close" data-dismiss="modal" aria-label="Close">
+            <span aria-hidden="true">&times;</span>
+          </button>
+        </div>
+
+        <div class="modal-body">
+          <p>Are you sure you want to send an intent-to-ship email for this experiment?</p>
+          <p class="sending-failed d-none text-danger">
+            We're sorry, something went wrong with sending this email. Please refresh the page and try again.
+          </p>
+        </div>
+
+        <div class="modal-footer">
+          <button type="button" class="btn btn-secondary" data-dismiss="modal">Cancel</button>
+          <button type="button" class="btn btn-primary send">Send</button>
+        </div>
+      </div>
+    </div>
+  </div>
+{% endblock %}
+
+{% block extrascripts %}
+  <script src="{% static "js/detail-review.js" %}"></script>
+{% endblock %}

app/experimenter/templates/experiments/intent_to_ship.txt

@@ -0,0 +1,26 @@
+{% autoescape off %}
+Hello Release Drivers,
+
+This request is coming from information entered in Experimenter.
+Please reach out to the person(s) on cc: with any questions, details,
+or discussion. They will email an update if any of the key information
+changes. Otherwise they will reach out once the study has fully passed
+QA for Release Management sign-off.
+
+Experimenter Bug: {{ bug_url }}
+Experimenter URL: {{ experiment.experiment_url }}
+Study owner: {{ experiment.owner.email }}
+Description: {{ experiment.short_description }}
+Timeline & Channel: {{ experiment.firefox_version }} {{ experiment.firefox_channel }}
+Intended study dates: {{ experiment.dates }}
+Percent of Population: {{ percent_of_population }}
+Platforms: {{ experiment.platform }}
+Locales: {{ locales|join:", "|default:"All locales" }}; {{ countries|join:", "|default:"All countries" }}
+QA Status: {{ experiment.qa_status }}
+Meta Bug: {{ experiment.feature_bugzilla_url }}
+Related links: {{ experiment.related_work }}{% if experiment.risks %}
+Risk: {{ experiment.risks }}{% endif %}{% if experiment.risk_technical_description %}
+Technical Complexity: {{ experiment.risk_technical_description }}{% endif %}
+
+Thank you!!
+{% endautoescape %}