From 5af90c72c172f76019516bcac0cdccefdbee1fa5 Mon Sep 17 00:00:00 2001 From: Peter Bengtsson Date: Fri, 1 Mar 2019 11:05:43 -0500 Subject: [PATCH 01/13] Intent to Ship Email Fixes #747 --- app/experimenter/experiments/constants.py | 30 +++++++++++ app/experimenter/experiments/email.py | 63 +++++++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/app/experimenter/experiments/constants.py b/app/experimenter/experiments/constants.py index 5dfce150d5..f08795d197 100644 --- a/app/experimenter/experiments/constants.py +++ b/app/experimenter/experiments/constants.py @@ -758,6 +758,36 @@ class ExperimentConstants(object): {attention}""" ) + INTENT_TO_SHIP_EMAIL_SUBJECT = ( + "SHIELD Study Intent to ship: {name} {version} {channel}" + ) + + INTENT_TO_SHIP_EMAIL_TEMPLATE = """ +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 Project: {project_url} +Study owner: {experiment_owner} +Description: {short_description} +Timeline & Channel: {version} {channel} +Intended study dates: {proposed_start_date} - {proposed_end_date} +Percent of Population: {percent_of_population} +Platforms: {platforms} +Locales: {locales} +QA Status: {qa_status} +Meta Bug: {feature_bugzilla_url} +Related links: {related_work} +Risk: {Risk Box - only if it exists} +Technical Complexity: {Technical Complexity Box - only if it exists} + +Thank you!!""" + BUGZILLA_OVERVIEW_TEMPLATE = ( """ {experiment.name} diff --git a/app/experimenter/experiments/email.py b/app/experimenter/experiments/email.py index 62d0f6641e..0dcec9afff 100644 --- a/app/experimenter/experiments/email.py +++ b/app/experimenter/experiments/email.py @@ -1,7 +1,9 @@ +import datetime from urllib.parse import urljoin from django.conf import settings from django.core.mail import send_mail +from django.urls import reverse from experimenter.experiments.models import Experiment @@ -22,3 +24,64 @@ 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_url): + + def make_url(uri): + return urljoin("https://{}".format(settings.HOSTNAME), uri) + + # XXX Is there already a routine for doing this? + def format_date(date, format="%Y-%m-%d"): + assert isinstance(date, datetime.date) + return date.strftime(format) + + experiment = Experiment.objects.get(id=experiment_id) + + version = experiment.firefox_version + channel = experiment.firefox_channel + experiment_url = make_url(experiment.experiment_url) + + bug_url = settings.BUGZILLA_DETAIL_URL.format(id=experiment.bugzilla_id) + + project_url = make_url( + reverse("projects-detail", kwargs={"slug": experiment.project.slug}) + ) + + proposed_start_date = experiment.proposed_start_date + proposed_end_date = proposed_start_date + datetime.timedelta( + days=experiment.proposed_duration + ) + # Because that's how it's done in Experiment.population (property) + percent_of_population = f"{experiment.population_percent:g}%" + # XXX https://github.com/mozilla/experimenter/issues/747#issuecomment-468404348 + platforms = "(unknown)" + # XXX https://github.com/mozilla/experimenter/issues/747#issuecomment-468407307 + locales = "(unknown)" + + content = Experiment.INTENT_TO_SHIP_EMAIL_TEMPLATE.format( + experiment_url=experiment_url, + bug_url=bug_url, + project_url=project_url, + experiment_owner=experiment.owner.email, + short_description=experiment.short_description, + version=version, + channel=channel, + proposed_start_date=format_date(proposed_start_date), + proposed_end_date=format_date(proposed_end_date), + percent_of_population=percent_of_population, + platforms=platforms, + locales=locales, + qa_status=experiment.qa_status, + feature_bugzilla_url=experiment.feature_bugzilla_url, + related_work=experiment.related_work, + ).lstrip() + send_mail( + Experiment.INTENT_TO_SHIP_EMAIL_SUBJECT.format( + name=experiment.name, version=version, channel=channel + ), + content, + settings.EMAIL_SENDER, + [settings.EMAIL_REVIEW], + fail_silently=False, + ) From e557485961c1dd0898f5c7ad4e8f84f5e1608159 Mon Sep 17 00:00:00 2001 From: Peter Bengtsson Date: Fri, 22 Mar 2019 17:13:42 -0400 Subject: [PATCH 02/13] Intent to Ship Email Part of #747 --- app/experimenter/experiments/constants.py | 26 ------------- app/experimenter/experiments/email.py | 37 +++++++++++-------- .../templates/experiments/intent_to_ship.txt | 24 ++++++++++++ 3 files changed, 45 insertions(+), 42 deletions(-) create mode 100644 app/experimenter/templates/experiments/intent_to_ship.txt diff --git a/app/experimenter/experiments/constants.py b/app/experimenter/experiments/constants.py index f08795d197..3e051a1f3a 100644 --- a/app/experimenter/experiments/constants.py +++ b/app/experimenter/experiments/constants.py @@ -762,32 +762,6 @@ class ExperimentConstants(object): "SHIELD Study Intent to ship: {name} {version} {channel}" ) - INTENT_TO_SHIP_EMAIL_TEMPLATE = """ -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 Project: {project_url} -Study owner: {experiment_owner} -Description: {short_description} -Timeline & Channel: {version} {channel} -Intended study dates: {proposed_start_date} - {proposed_end_date} -Percent of Population: {percent_of_population} -Platforms: {platforms} -Locales: {locales} -QA Status: {qa_status} -Meta Bug: {feature_bugzilla_url} -Related links: {related_work} -Risk: {Risk Box - only if it exists} -Technical Complexity: {Technical Complexity Box - only if it exists} - -Thank you!!""" - BUGZILLA_OVERVIEW_TEMPLATE = ( """ {experiment.name} diff --git a/app/experimenter/experiments/email.py b/app/experimenter/experiments/email.py index 0dcec9afff..1f974c895e 100644 --- a/app/experimenter/experiments/email.py +++ b/app/experimenter/experiments/email.py @@ -4,6 +4,7 @@ from django.conf import settings from django.core.mail import send_mail from django.urls import reverse +from django.template.loader import render_to_string from experimenter.experiments.models import Experiment @@ -44,6 +45,7 @@ def format_date(date, format="%Y-%m-%d"): bug_url = settings.BUGZILLA_DETAIL_URL.format(id=experiment.bugzilla_id) + # XXX Not all experiments have a Project! project_url = make_url( reverse("projects-detail", kwargs={"slug": experiment.project.slug}) ) @@ -59,22 +61,25 @@ def format_date(date, format="%Y-%m-%d"): # XXX https://github.com/mozilla/experimenter/issues/747#issuecomment-468407307 locales = "(unknown)" - content = Experiment.INTENT_TO_SHIP_EMAIL_TEMPLATE.format( - experiment_url=experiment_url, - bug_url=bug_url, - project_url=project_url, - experiment_owner=experiment.owner.email, - short_description=experiment.short_description, - version=version, - channel=channel, - proposed_start_date=format_date(proposed_start_date), - proposed_end_date=format_date(proposed_end_date), - percent_of_population=percent_of_population, - platforms=platforms, - locales=locales, - qa_status=experiment.qa_status, - feature_bugzilla_url=experiment.feature_bugzilla_url, - related_work=experiment.related_work, + content = render_to_string( + "my_template.html", + { + "experiment_url": experiment_url, + "bug_url": bug_url, + "project_url": project_url, + "experiment_owner": experiment.owner.email, + "short_description": experiment.short_description, + "version": version, + "channel": channel, + "proposed_start_date": format_date(proposed_start_date), + "proposed_end_date": format_date(proposed_end_date), + "percent_of_population": percent_of_population, + "platforms": platforms, + "locales": locales, + "qa_status": experiment.qa_status, + "feature_bugzilla_url": experiment.feature_bugzilla_url, + "related_work": experiment.related_work, + }, ).lstrip() send_mail( Experiment.INTENT_TO_SHIP_EMAIL_SUBJECT.format( 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..07aecd3851 --- /dev/null +++ b/app/experimenter/templates/experiments/intent_to_ship.txt @@ -0,0 +1,24 @@ +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 Project: {{ project_url }} +Study owner: {{ experiment_owner }} +Description: {{ short_description }} +Timeline & Channel: {{ version }} {{ channel }} +Intended study dates: {{ proposed_start_date }} - {{ proposed_end_date }} +Percent of Population: {{ percent_of_population }} +Platforms: {{ platforms }} +Locales: {{ locales }} +QA Status: {{ qa_status }} +Meta Bug: {{ feature_bugzilla_url }} +Related links: {{ related_work }} +Risk: {{ Risk Box - only if it exists }} +Technical Complexity: {{ Technical Complexity Box - only if it exists }} + +Thank you!! From b751a068b452ae4dc83d23a6294ec44fef9c8ee5 Mon Sep 17 00:00:00 2001 From: Ethan Glasser-Camp Date: Thu, 28 Mar 2019 17:07:33 -0400 Subject: [PATCH 03/13] 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. --- app/experimenter/experiments/email.py | 54 ++----- .../experiments/tests/test_email.py | 143 +++++++++++++++++- .../templates/experiments/intent_to_ship.txt | 28 ++-- 3 files changed, 172 insertions(+), 53 deletions(-) diff --git a/app/experimenter/experiments/email.py b/app/experimenter/experiments/email.py index 1f974c895e..6d3f9ba3bd 100644 --- a/app/experimenter/experiments/email.py +++ b/app/experimenter/experiments/email.py @@ -1,9 +1,7 @@ -import datetime from urllib.parse import urljoin from django.conf import settings from django.core.mail import send_mail -from django.urls import reverse from django.template.loader import render_to_string from experimenter.experiments.models import Experiment @@ -27,60 +25,38 @@ def send_review_email(experiment_name, experiment_url, needs_attention): ) -def send_intent_to_ship_email(experiment_id, experiment_url): +def send_intent_to_ship_email(experiment_id): def make_url(uri): return urljoin("https://{}".format(settings.HOSTNAME), uri) - # XXX Is there already a routine for doing this? - def format_date(date, format="%Y-%m-%d"): - assert isinstance(date, datetime.date) - return date.strftime(format) + experiment = Experiment.objects.prefetch_related( + "locales", "countries" + ).get(id=experiment_id) - experiment = Experiment.objects.get(id=experiment_id) - - version = experiment.firefox_version - channel = experiment.firefox_channel experiment_url = make_url(experiment.experiment_url) bug_url = settings.BUGZILLA_DETAIL_URL.format(id=experiment.bugzilla_id) - # XXX Not all experiments have a Project! - project_url = make_url( - reverse("projects-detail", kwargs={"slug": experiment.project.slug}) - ) - - proposed_start_date = experiment.proposed_start_date - proposed_end_date = proposed_start_date + datetime.timedelta( - days=experiment.proposed_duration - ) # Because that's how it's done in Experiment.population (property) - percent_of_population = f"{experiment.population_percent:g}%" - # XXX https://github.com/mozilla/experimenter/issues/747#issuecomment-468404348 - platforms = "(unknown)" - # XXX https://github.com/mozilla/experimenter/issues/747#issuecomment-468407307 - locales = "(unknown)" + percent_of_population = f"{float(experiment.population_percent):g}%" content = render_to_string( - "my_template.html", + "experiments/intent_to_ship.txt", { + "experiment": experiment, "experiment_url": experiment_url, "bug_url": bug_url, - "project_url": project_url, - "experiment_owner": experiment.owner.email, - "short_description": experiment.short_description, - "version": version, - "channel": channel, - "proposed_start_date": format_date(proposed_start_date), - "proposed_end_date": format_date(proposed_end_date), "percent_of_population": percent_of_population, - "platforms": platforms, - "locales": locales, - "qa_status": experiment.qa_status, - "feature_bugzilla_url": experiment.feature_bugzilla_url, - "related_work": experiment.related_work, + "locales": [str(l) for l in experiment.locales.all()], + "countries": [str(c) for c in experiment.countries.all()], }, - ).lstrip() + ) + # Strip extra newlines from autoescape tag + content = content.strip() + "\n" + + version = experiment.firefox_version + channel = experiment.firefox_channel send_mail( Experiment.INTENT_TO_SHIP_EMAIL_SUBJECT.format( name=experiment.name, version=version, channel=channel diff --git a/app/experimenter/experiments/tests/test_email.py b/app/experimenter/experiments/tests/test_email.py index f2173617ac..ce77b5f335 100644 --- a/app/experimenter/experiments/tests/test_email.py +++ b/app/experimenter/experiments/tests/test_email.py @@ -1,8 +1,13 @@ +from urllib.parse import urljoin + 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 +53,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) + + experiment_url = urljoin( + "https://{}".format(settings.HOSTNAME), experiment.experiment_url + ) + 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_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_REVIEW]) + + 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) + + experiment_url = urljoin( + "https://{}".format(settings.HOSTNAME), experiment.experiment_url + ) + 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_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_REVIEW]) + + def format_locales(self, experiment): + locales = "All locales" + if experiment.locales.exists(): + locales = ", ".join( + str(locale) for locale in experiment.locales.all() + ) + return locales + + def format_countries(self, experiment): + countries = "All countries" + if experiment.countries.exists(): + 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/templates/experiments/intent_to_ship.txt b/app/experimenter/templates/experiments/intent_to_ship.txt index 07aecd3851..1479394200 100644 --- a/app/experimenter/templates/experiments/intent_to_ship.txt +++ b/app/experimenter/templates/experiments/intent_to_ship.txt @@ -1,3 +1,4 @@ +{% autoescape off %} Hello Release Drivers, This request is coming from information entered in Experimenter. @@ -6,19 +7,20 @@ 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 Project: {{ project_url }} -Study owner: {{ experiment_owner }} -Description: {{ short_description }} -Timeline & Channel: {{ version }} {{ channel }} -Intended study dates: {{ proposed_start_date }} - {{ proposed_end_date }} +Experimenter Bug: {{ bug_url }} +Experimenter URL: {{ 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: {{ platforms }} -Locales: {{ locales }} -QA Status: {{ qa_status }} -Meta Bug: {{ feature_bugzilla_url }} -Related links: {{ related_work }} -Risk: {{ Risk Box - only if it exists }} -Technical Complexity: {{ Technical Complexity Box - only if it exists }} +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 %} From 02b48c428b5b44fc166361857854398fbd0e00b4 Mon Sep 17 00:00:00 2001 From: Ethan Glasser-Camp Date: Tue, 2 Apr 2019 12:58:31 -0400 Subject: [PATCH 04/13] Help text should not be inside the label --- app/experimenter/templates/experiments/detail_review.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/experimenter/templates/experiments/detail_review.html b/app/experimenter/templates/experiments/detail_review.html index a7dbc3617a..0fcbecb3aa 100644 --- a/app/experimenter/templates/experiments/detail_review.html +++ b/app/experimenter/templates/experiments/detail_review.html @@ -37,8 +37,8 @@
Sign-Off Checklist
+ {{ field.help_text|safe }} {% endfor %} @@ -49,8 +49,8 @@
Sign-Off Checklist
+ {{ field.help_text|safe }} {% endfor %} From 8b0b8f57261f750a8a30a687beaf774da7165c67 Mon Sep 17 00:00:00 2001 From: Ethan Glasser-Camp Date: Thu, 4 Apr 2019 13:43:10 -0400 Subject: [PATCH 05/13] Add API to send intent-to-ship email --- app/experimenter/experiments/api_urls.py | 6 +++ app/experimenter/experiments/api_views.py | 23 +++++++++++ .../experiments/tests/test_api_views.py | 41 +++++++++++++++++++ 3 files changed, 70 insertions(+) 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/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) From fad8c7494da3219b98a408bead7cfb7462650473 Mon Sep 17 00:00:00 2001 From: Ethan Glasser-Camp Date: Thu, 4 Apr 2019 13:43:33 -0400 Subject: [PATCH 06/13] Add front-end interface to send email --- app/experimenter/static/js/detail-review.js | 26 ++++++++++++ .../templates/experiments/detail_review.html | 42 +++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 app/experimenter/static/js/detail-review.js diff --git a/app/experimenter/static/js/detail-review.js b/app/experimenter/static/js/detail-review.js new file mode 100644 index 0000000000..1b4644d875 --- /dev/null +++ b/app/experimenter/static/js/detail-review.js @@ -0,0 +1,26 @@ +// 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) { + modal.modal("hide"); + // Update the checkbox to match our out-of-band knowledge of + // what happened. + $("#id_review_intent_to_ship").prop("checked", true); + } 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 0fcbecb3aa..1a5d9745f6 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 {{ field }} {{ field.label }} + {% if field.name == 'review_intent_to_ship' %} + + {% endif %} {{ field.help_text|safe }} {% endfor %} @@ -77,3 +87,35 @@
Sign-Off Checklist
{% endblock %} + +{% block main_content %} + {{ block.super }} + +{% endblock %} + +{% block extrascripts %} + +{% endblock %} From 1c3a4b438af9a2aab6a58b1457eeea23b8b5f70f Mon Sep 17 00:00:00 2001 From: Ethan Glasser-Camp Date: Mon, 8 Apr 2019 11:32:20 -0400 Subject: [PATCH 07/13] experiment_url should already include the protocol Thanks @jaredkerim for pointing this out. --- app/experimenter/experiments/email.py | 7 ------- app/experimenter/experiments/tests/test_email.py | 12 ++---------- .../templates/experiments/intent_to_ship.txt | 2 +- 3 files changed, 3 insertions(+), 18 deletions(-) diff --git a/app/experimenter/experiments/email.py b/app/experimenter/experiments/email.py index 6d3f9ba3bd..e2523c16a8 100644 --- a/app/experimenter/experiments/email.py +++ b/app/experimenter/experiments/email.py @@ -26,16 +26,10 @@ def send_review_email(experiment_name, experiment_url, needs_attention): def send_intent_to_ship_email(experiment_id): - - def make_url(uri): - return urljoin("https://{}".format(settings.HOSTNAME), uri) - experiment = Experiment.objects.prefetch_related( "locales", "countries" ).get(id=experiment_id) - experiment_url = make_url(experiment.experiment_url) - bug_url = settings.BUGZILLA_DETAIL_URL.format(id=experiment.bugzilla_id) # Because that's how it's done in Experiment.population (property) @@ -45,7 +39,6 @@ def make_url(uri): "experiments/intent_to_ship.txt", { "experiment": experiment, - "experiment_url": experiment_url, "bug_url": bug_url, "percent_of_population": percent_of_population, "locales": [str(l) for l in experiment.locales.all()], diff --git a/app/experimenter/experiments/tests/test_email.py b/app/experimenter/experiments/tests/test_email.py index ce77b5f335..29b948a993 100644 --- a/app/experimenter/experiments/tests/test_email.py +++ b/app/experimenter/experiments/tests/test_email.py @@ -1,5 +1,3 @@ -from urllib.parse import urljoin - from django.test import TestCase from django.conf import settings from django.core import mail @@ -72,9 +70,6 @@ def test_send_intent_to_ship_email_with_risk_fields(self): sent_email = mail.outbox[-1] self.verify_subject(experiment, sent_email) - experiment_url = urljoin( - "https://{}".format(settings.HOSTNAME), experiment.experiment_url - ) bug_url = settings.BUGZILLA_DETAIL_URL.format( id=experiment.bugzilla_id ) @@ -91,7 +86,7 @@ def test_send_intent_to_ship_email_with_risk_fields(self): QA for Release Management sign-off. Experimenter Bug: {bug_url} -Experimenter URL: {experiment_url} +Experimenter URL: {experiment.experiment_url} Study owner: {experiment.owner.email} Description: {experiment.short_description} Timeline & Channel: {experiment.firefox_version} {experiment.firefox_channel} @@ -126,9 +121,6 @@ def test_send_intent_to_ship_email_without_risk_fields(self): sent_email = mail.outbox[-1] self.verify_subject(experiment, sent_email) - experiment_url = urljoin( - "https://{}".format(settings.HOSTNAME), experiment.experiment_url - ) bug_url = settings.BUGZILLA_DETAIL_URL.format( id=experiment.bugzilla_id ) @@ -145,7 +137,7 @@ def test_send_intent_to_ship_email_without_risk_fields(self): QA for Release Management sign-off. Experimenter Bug: {bug_url} -Experimenter URL: {experiment_url} +Experimenter URL: {experiment.experiment_url} Study owner: {experiment.owner.email} Description: {experiment.short_description} Timeline & Channel: {experiment.firefox_version} {experiment.firefox_channel} diff --git a/app/experimenter/templates/experiments/intent_to_ship.txt b/app/experimenter/templates/experiments/intent_to_ship.txt index 1479394200..c3d03c4bcb 100644 --- a/app/experimenter/templates/experiments/intent_to_ship.txt +++ b/app/experimenter/templates/experiments/intent_to_ship.txt @@ -8,7 +8,7 @@ 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_url }} +Experimenter URL: {{ experiment.experiment_url }} Study owner: {{ experiment.owner.email }} Description: {{ experiment.short_description }} Timeline & Channel: {{ experiment.firefox_version }} {{ experiment.firefox_channel }} From a9f2c0dbe1a0b5eef7c1f222b1ba5523ecbf0d50 Mon Sep 17 00:00:00 2001 From: Ethan Glasser-Camp Date: Mon, 8 Apr 2019 11:33:57 -0400 Subject: [PATCH 08/13] Move prefetch of locales and countries to models Thanks @jaredkerim for the suggestion. --- app/experimenter/experiments/email.py | 4 +--- app/experimenter/experiments/models.py | 2 ++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/experimenter/experiments/email.py b/app/experimenter/experiments/email.py index e2523c16a8..062518843e 100644 --- a/app/experimenter/experiments/email.py +++ b/app/experimenter/experiments/email.py @@ -26,9 +26,7 @@ def send_review_email(experiment_name, experiment_url, needs_attention): def send_intent_to_ship_email(experiment_id): - experiment = Experiment.objects.prefetch_related( - "locales", "countries" - ).get(id=experiment_id) + experiment = Experiment.objects.get(id=experiment_id) bug_url = settings.BUGZILLA_DETAIL_URL.format(id=experiment.bugzilla_id) 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")) ) From 54105362a4bdf9b20592c65d741b0ab74f46f34b Mon Sep 17 00:00:00 2001 From: Ethan Glasser-Camp Date: Mon, 8 Apr 2019 11:57:19 -0400 Subject: [PATCH 09/13] Show button only if email has not been sent Thanks @jaredkerim for the suggestion. --- .../templates/experiments/detail_review.html | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/app/experimenter/templates/experiments/detail_review.html b/app/experimenter/templates/experiments/detail_review.html index 1a5d9745f6..25a739cc53 100644 --- a/app/experimenter/templates/experiments/detail_review.html +++ b/app/experimenter/templates/experiments/detail_review.html @@ -36,17 +36,18 @@
Sign-Off Checklist
{% for field in form.required_reviews %}
- - {% if field.name == 'review_intent_to_ship' %} + {% if field.name == 'review_intent_to_ship' and not experiment.review_intent_to_ship %} + {% else %} + {% endif %} {{ field.help_text|safe }}
From 24df88f97d22201991315071a9872268f1ca46bf Mon Sep 17 00:00:00 2001 From: Ethan Glasser-Camp Date: Mon, 8 Apr 2019 12:00:43 -0400 Subject: [PATCH 10/13] Just refresh the page rather than doing anything fancy Thanks @jaredkerim for the suggestion. --- app/experimenter/static/js/detail-review.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/experimenter/static/js/detail-review.js b/app/experimenter/static/js/detail-review.js index 1b4644d875..44b482aee6 100644 --- a/app/experimenter/static/js/detail-review.js +++ b/app/experimenter/static/js/detail-review.js @@ -15,10 +15,9 @@ jQuery(function($) { method: "PUT", }); if (resp.status == 200) { - modal.modal("hide"); - // Update the checkbox to match our out-of-band knowledge of - // what happened. - $("#id_review_intent_to_ship").prop("checked", true); + // 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"); } From 8ab8bdf3350b914afb4b3692b521e14b39d219c2 Mon Sep 17 00:00:00 2001 From: Ethan Glasser-Camp Date: Mon, 8 Apr 2019 12:57:27 -0400 Subject: [PATCH 11/13] Fix test coverage These utility methods in the test suite do not need 100% coverage. --- app/experimenter/experiments/tests/test_email.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/experimenter/experiments/tests/test_email.py b/app/experimenter/experiments/tests/test_email.py index 29b948a993..e8555d7a97 100644 --- a/app/experimenter/experiments/tests/test_email.py +++ b/app/experimenter/experiments/tests/test_email.py @@ -159,7 +159,7 @@ def test_send_intent_to_ship_email_without_risk_fields(self): def format_locales(self, experiment): locales = "All locales" - if experiment.locales.exists(): + if experiment.locales.exists(): # pragma: no branch locales = ", ".join( str(locale) for locale in experiment.locales.all() ) @@ -167,7 +167,7 @@ def format_locales(self, experiment): def format_countries(self, experiment): countries = "All countries" - if experiment.countries.exists(): + if experiment.countries.exists(): # pragma: no branch countries = ", ".join( str(country) for country in experiment.countries.all() ) From 4e7a7604b76ba0c3ee63c5ed40bdc37a65fc034d Mon Sep 17 00:00:00 2001 From: Ethan Glasser-Camp Date: Tue, 9 Apr 2019 13:28:29 -0400 Subject: [PATCH 12/13] Send email to release drivers and experiment owner Adding a CC requires using the EmailMessage API directly instead of the send_mail wrapper. --- .env.sample | 1 + app/experimenter/experiments/email.py | 8 +++++--- app/experimenter/experiments/tests/test_email.py | 10 ++++++++-- app/experimenter/settings.py | 3 +++ 4 files changed, 17 insertions(+), 5 deletions(-) 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/app/experimenter/experiments/email.py b/app/experimenter/experiments/email.py index 062518843e..ffba2151e0 100644 --- a/app/experimenter/experiments/email.py +++ b/app/experimenter/experiments/email.py @@ -2,6 +2,7 @@ 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 @@ -48,12 +49,13 @@ def send_intent_to_ship_email(experiment_id): version = experiment.firefox_version channel = experiment.firefox_channel - send_mail( + email = EmailMessage( Experiment.INTENT_TO_SHIP_EMAIL_SUBJECT.format( name=experiment.name, version=version, channel=channel ), content, settings.EMAIL_SENDER, - [settings.EMAIL_REVIEW], - fail_silently=False, + [settings.EMAIL_RELEASE_DRIVERS], + cc=[experiment.owner.email], ) + email.send(fail_silently=False) diff --git a/app/experimenter/experiments/tests/test_email.py b/app/experimenter/experiments/tests/test_email.py index e8555d7a97..eb9a2449d1 100644 --- a/app/experimenter/experiments/tests/test_email.py +++ b/app/experimenter/experiments/tests/test_email.py @@ -106,7 +106,10 @@ def test_send_intent_to_ship_email_with_risk_fields(self): self.assertEqual(expected_body, sent_email.body) self.assertEqual(sent_email.from_email, settings.EMAIL_SENDER) - self.assertEqual(sent_email.recipients(), [settings.EMAIL_REVIEW]) + 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( @@ -155,7 +158,10 @@ def test_send_intent_to_ship_email_without_risk_fields(self): self.assertEqual(expected_body, sent_email.body) self.assertEqual(sent_email.from_email, settings.EMAIL_SENDER) - self.assertEqual(sent_email.recipients(), [settings.EMAIL_REVIEW]) + self.assertEqual( + sent_email.recipients(), + [settings.EMAIL_RELEASE_DRIVERS, experiment.owner.email], + ) def format_locales(self, experiment): locales = "All locales" 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") From 81e4536a1295756dd9b8aebafc3f5ccede55bb1c Mon Sep 17 00:00:00 2001 From: Ethan Glasser-Camp Date: Tue, 9 Apr 2019 13:41:39 -0400 Subject: [PATCH 13/13] Add setting to test configuration --- .env.test | 1 + app/experimenter/settings_test.py | 1 + 2 files changed, 2 insertions(+) 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/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