diff --git a/.env.sample b/.env.sample index e07942d084..707286a66f 100644 --- a/.env.sample +++ b/.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= diff --git a/.env.test b/.env.test index fa675f6706..95639fd15d 100644 --- a/.env.test +++ b/.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= diff --git a/app/experimenter/experiments/api_urls.py b/app/experimenter/experiments/api_urls.py index 3385f33a9b..89ac72aed2 100644 --- a/app/experimenter/experiments/api_urls.py +++ b/app/experimenter/experiments/api_urls.py @@ -5,6 +5,7 @@ ExperimentDetailView, ExperimentListView, ExperimentRejectView, + ExperimentSendIntentToShipEmailView, ) @@ -19,6 +20,11 @@ ExperimentRejectView.as_view(), name="experiments-api-reject", ), + url( + r"^(?P[\w-]+)/intent-to-ship-email$", + ExperimentSendIntentToShipEmailView.as_view(), + name="experiments-api-send-intent-to-ship-email", + ), url( r"^(?P[\w-]+)/$", ExperimentDetailView.as_view(), diff --git a/app/experimenter/experiments/api_views.py b/app/experimenter/experiments/api_views.py index f0c97e687f..acc3857865 100644 --- a/app/experimenter/experiments/api_views.py +++ b/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 @@ def update(self, request, *args, **kwargs): ) 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() diff --git a/app/experimenter/experiments/constants.py b/app/experimenter/experiments/constants.py index 5dfce150d5..3e051a1f3a 100644 --- a/app/experimenter/experiments/constants.py +++ b/app/experimenter/experiments/constants.py @@ -758,6 +758,10 @@ class ExperimentConstants(object): {attention}""" ) + INTENT_TO_SHIP_EMAIL_SUBJECT = ( + "SHIELD Study Intent to ship: {name} {version} {channel}" + ) + BUGZILLA_OVERVIEW_TEMPLATE = ( """ {experiment.name} diff --git a/app/experimenter/experiments/email.py b/app/experimenter/experiments/email.py index 62d0f6641e..ffba2151e0 100644 --- a/app/experimenter/experiments/email.py +++ b/app/experimenter/experiments/email.py @@ -2,6 +2,8 @@ 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) diff --git a/app/experimenter/experiments/models.py b/app/experimenter/experiments/models.py index 2264c9eb0a..cc1b5657fd 100644 --- a/app/experimenter/experiments/models.py +++ b/app/experimenter/experiments/models.py @@ -28,6 +28,8 @@ def get_queryset(self): "owner", "comments", "comments__created_by", + "locales", + "countries", ) .annotate(latest_change=Max("changes__changed_on")) ) diff --git a/app/experimenter/experiments/tests/test_api_views.py b/app/experimenter/experiments/tests/test_api_views.py index 7d8e5ae2b1..84b48d3181 100644 --- a/app/experimenter/experiments/tests/test_api_views.py +++ b/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 @@ def test_post_to_reject_raises_404_for_non_pending_experiment(self): ) 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) diff --git a/app/experimenter/experiments/tests/test_email.py b/app/experimenter/experiments/tests/test_email.py index f2173617ac..eb9a2449d1 100644 --- a/app/experimenter/experiments/tests/test_email.py +++ b/app/experimenter/experiments/tests/test_email.py @@ -2,7 +2,10 @@ 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 @@ def test_send_review_email_with_needs_attention(self): ) 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) diff --git a/app/experimenter/settings.py b/app/experimenter/settings.py index d423545eaa..bdff4875da 100644 --- a/app/experimenter/settings.py +++ b/app/experimenter/settings.py @@ -253,6 +253,9 @@ # 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") diff --git a/app/experimenter/settings_test.py b/app/experimenter/settings_test.py index 5ec0a03e89..90938558e7 100644 --- a/app/experimenter/settings_test.py +++ b/app/experimenter/settings_test.py @@ -15,5 +15,6 @@ 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 diff --git a/app/experimenter/static/js/detail-review.js b/app/experimenter/static/js/detail-review.js new file mode 100644 index 0000000000..44b482aee6 --- /dev/null +++ b/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"); + } + }); +}); diff --git a/app/experimenter/templates/experiments/detail_review.html b/app/experimenter/templates/experiments/detail_review.html index a7dbc3617a..25a739cc53 100644 --- a/app/experimenter/templates/experiments/detail_review.html +++ b/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 %}
Sign-Off Checklist {% for field in form.required_reviews %}
- + {% if field.name == 'review_intent_to_ship' and not experiment.review_intent_to_ship %} + + {% else %} + + {% endif %} + {{ field.help_text|safe }}
{% endfor %} @@ -49,8 +60,8 @@
Sign-Off Checklist
+ {{ field.help_text|safe }} {% endfor %} @@ -77,3 +88,35 @@
Sign-Off Checklist
{% endblock %} + +{% block main_content %} + {{ block.super }} + +{% endblock %} + +{% block extrascripts %} + +{% endblock %} diff --git a/app/experimenter/templates/experiments/intent_to_ship.txt b/app/experimenter/templates/experiments/intent_to_ship.txt new file mode 100644 index 0000000000..c3d03c4bcb --- /dev/null +++ b/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 %}