From 88b7b45b0e51d8243b7e61b6f480e8e7a48567fa Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Tue, 27 Aug 2019 15:15:13 +0000 Subject: [PATCH] Fix #2328 - Use mailtriggers to find destinations in review app As the review app has several conditionals that don't fit entirely well within mailtriggers, the templates use a bit of extra context to figure out who exactly to include. This also extends the tests for review, to check for correct recipients. It also adds a tiny feature to mailtrigger to entirely exclude certain addresses, as required by the review-generated mails. Commit ready for merge. - Legacy-Id: 16672 --- ietf/doc/tests_review.py | 20 +- .../0007_add_review_mailtriggers.py | 86 +++++++++ ietf/mailtrigger/models.py | 15 +- ietf/mailtrigger/utils.py | 8 +- ietf/name/fixtures/names.json | 86 +++++++++ ietf/review/utils.py | 174 ++++++------------ 6 files changed, 272 insertions(+), 117 deletions(-) create mode 100644 ietf/mailtrigger/migrations/0007_add_review_mailtriggers.py diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index 4e4d00672..184c0a40b 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -223,6 +223,8 @@ class ReviewTests(TestCase): self.assertFalse("review_request_close_comment" in e.desc.lower()) self.assertEqual(len(outbox), 1) + self.assertTrue('' in outbox[0]["To"]) + self.assertFalse("" in outbox[0]["To"]) mail_content = outbox[0].get_payload(decode=True).decode("utf-8").lower() self.assertTrue("closed" in mail_content) self.assertTrue("review_request_close_comment" in mail_content) @@ -420,6 +422,7 @@ class ReviewTests(TestCase): self.assertEqual(assignment.reviewer, reviewer) self.assertEqual(assignment.state_id, "assigned") self.assertEqual(len(outbox), 1) + self.assertEqual('"Some Reviewer" ', outbox[0]["To"]) self.assertTrue("assigned" in outbox[0].get_payload(decode=True).decode("utf-8")) self.assertEqual(NextReviewerInTeam.objects.get(team=review_req.team).next_reviewer, rotation_list[1]) @@ -482,6 +485,8 @@ class ReviewTests(TestCase): self.assertEqual(e.type, "closed_review_assignment") self.assertTrue("rejected" in e.desc) self.assertEqual(len(outbox), 1) + self.assertTrue(assignment.reviewer.address in outbox[0]["To"]) + self.assertFalse("" in outbox[0]["To"]) self.assertTrue("Test message" in outbox[0].get_payload(decode=True).decode("utf-8")) def make_test_mbox_tarball(self, review_req): @@ -858,7 +863,8 @@ class ReviewTests(TestCase): self.assertTrue(assignment.review_request.doc.rev in assignment.review.name) self.assertEqual(len(outbox), 2) - self.assertTrue("reviewsecretary@example.com" in outbox[0]["To"]) + self.assertTrue("" in outbox[0]["To"]) + self.assertFalse(assignment.reviewer.address in outbox[0]["To"]) self.assertTrue("partially" in outbox[0]["Subject"].lower()) self.assertTrue(assignment.review_request.team.list_email in outbox[1]["To"]) @@ -998,6 +1004,7 @@ class ReviewTests(TestCase): r = self.client.get(url) self.assertEqual(r.status_code, 200) + empty_outbox() old_deadline = review_req.deadline.date() new_deadline = old_deadline + datetime.timedelta(days=1) r = self.client.post(url, data={ @@ -1006,7 +1013,10 @@ class ReviewTests(TestCase): self.assertEqual(r.status_code, 302) review_req = reload_db_objects(review_req) self.assertEqual(review_req.deadline,new_deadline) - self.assertTrue('Deadline changed' in outbox[-1]['Subject']) + self.assertEqual(len(outbox), 1) + self.assertTrue('' in outbox[0]["To"]) + self.assertTrue('' in outbox[0]["To"]) + self.assertTrue('Deadline changed' in outbox[0]['Subject']) def test_mark_no_response(self): assignment = ReviewAssignmentFactory() @@ -1017,12 +1027,18 @@ class ReviewTests(TestCase): r = self.client.get(url) self.assertEqual(r.status_code, 200) + empty_outbox() r=self.client.post(url, data={"action":"noresponse"}) self.assertEqual(r.status_code, 302) assignment = reload_db_objects(assignment) self.assertEqual(assignment.state_id, 'no-response') + self.assertEqual(len(outbox), 1) + self.assertTrue(assignment.reviewer.address in outbox[0]["To"]) + self.assertFalse('' in outbox[0]["To"]) + self.assertTrue('Reviewer assignment marked no-response' in outbox[0]['Subject']) + def test_withdraw_assignment(self): assignment = ReviewAssignmentFactory() secr = RoleFactory(group=assignment.review_request.team,person__user__username='reviewsecretary',person__user__email='reviewsecretary@example.com',name_id='secr').person diff --git a/ietf/mailtrigger/migrations/0007_add_review_mailtriggers.py b/ietf/mailtrigger/migrations/0007_add_review_mailtriggers.py new file mode 100644 index 000000000..8d41037d1 --- /dev/null +++ b/ietf/mailtrigger/migrations/0007_add_review_mailtriggers.py @@ -0,0 +1,86 @@ +# Copyright The IETF Trust 2019, All Rights Reserved +# -*- coding: utf-8 -*- + +from __future__ import absolute_import, print_function, unicode_literals + +from django.db import migrations + +def forward(apps, schema_editor): + MailTrigger = apps.get_model('mailtrigger','MailTrigger') + Recipient = apps.get_model('mailtrigger', 'Recipient') + + review_assignment_reviewer = Recipient.objects.create( + slug="review_assignment_reviewer", + desc="The reviewer assigned to a review assignment", + template="{% if not skip_review_reviewer %}{{review_assignment.reviewer.email_address}}{% endif %}", + ) + review_assignment_review_req_by = Recipient.objects.create( + slug="review_assignment_review_req_by", + desc="The requester of an assigned review", + template="{% if not skip_review_requested_by %}{{review_assignment.review_request.requested_by.email_address}}{% endif %}", + ) + review_req_requested_by = Recipient.objects.create( + slug="review_req_requested_by", + desc="The requester of a review", + template="{% if not skip_review_requested_by %}{{review_req.requested_by.email_address}}{% endif %}", + ) + review_req_reviewers = Recipient.objects.create( + slug="review_req_reviewers", + desc="All reviewers assigned to a review request", + template=None, + ) + review_secretaries = Recipient.objects.create( + slug="review_secretaries", + desc="The secretaries of the review team of a review request or assignment", + template=None, + ) + Recipient.objects.create( + slug="review_reviewer", + desc="A single reviewer", + template="{{reviewer.email_address}}", + ) + + review_assignment_changed = MailTrigger.objects.create( + slug="review_assignment_changed", + desc="Recipients for a change to a review assignment", + ) + review_assignment_changed.to.set([review_assignment_review_req_by, review_assignment_reviewer, + review_secretaries]) + + review_req_changed = MailTrigger.objects.create( + slug="review_req_changed", + desc="Recipients for a change to a review request", + ) + review_req_changed.to.set([review_req_requested_by, review_req_reviewers, review_secretaries]) + + review_availability_changed = MailTrigger.objects.create( + slug="review_availability_changed", + desc="Recipients for a change to a reviewer's availability", + ) + review_availability_changed.to.set( + Recipient.objects.filter(slug__in=['review_reviewer', 'group_secretaries']) + ) + + +def reverse(apps, schema_editor): + MailTrigger = apps.get_model('mailtrigger','MailTrigger') + Recipient = apps.get_model('mailtrigger', 'Recipient') + + MailTrigger.objects.filter(slug__in=[ + 'review_assignment_changed', 'review_req_changed', 'review_availability_changed', + ]).delete() + Recipient.objects.filter(slug__in=[ + 'review_assignment_reviewer', 'review_assignment_review_req_by', 'review_req_requested_by', + 'review_req_reviewers', 'review_secretaries', 'review_reviewer', + ]).delete() + + +class Migration(migrations.Migration): + + dependencies = [ + ('mailtrigger', '0006_sub_new_wg_00'), + ] + + operations = [ + migrations.RunPython(forward, reverse) + ] diff --git a/ietf/mailtrigger/models.py b/ietf/mailtrigger/models.py index 29c899e86..2a72b622b 100644 --- a/ietf/mailtrigger/models.py +++ b/ietf/mailtrigger/models.py @@ -184,7 +184,20 @@ class Recipient(models.Model): else: addrs.extend(group.role_set.filter(name='secr').values_list('email__address',flat=True)) return addrs - + + def gather_review_req_reviewers(self, **kwargs): + addrs = [] + if 'review_request' in kwargs: + review_request = kwargs['review_request'] + for assignment in review_request.reviewassignment_set.all(): + addrs.append(assignment.reviewer.formatted_email()) + return addrs + + def gather_review_secretaries(self, **kwargs): + if not kwargs.get('skip_secretary'): + return self.gather_group_secretaries(**kwargs) + return [] + def gather_doc_group_responsible_directors(self, **kwargs): addrs = [] if 'doc' in kwargs: diff --git a/ietf/mailtrigger/utils.py b/ietf/mailtrigger/utils.py index ccc21d199..86455c61d 100644 --- a/ietf/mailtrigger/utils.py +++ b/ietf/mailtrigger/utils.py @@ -1,3 +1,5 @@ +# Copyright The IETF Trust 2019, All Rights Reserved + from collections import namedtuple import debug # pyflakes:ignore @@ -17,18 +19,22 @@ class AddrLists(namedtuple('AddrLists',['to','cc'])): return namedtuple('AddrListsAsStrings',['to','cc'])(to=to_string,cc=cc_string) -def gather_address_lists(slug, **kwargs): +def gather_address_lists(slug, skipped_recipients=None, **kwargs): mailtrigger = MailTrigger.objects.get(slug=slug) to = set() for recipient in mailtrigger.to.all(): to.update(recipient.gather(**kwargs)) to.discard('') + if skipped_recipients: + to -= set(skipped_recipients) cc = set() for recipient in mailtrigger.cc.all(): cc.update(recipient.gather(**kwargs)) cc.discard('') + if skipped_recipients: + cc -= set(skipped_recipients) return AddrLists(to=list(to),cc=list(cc)) diff --git a/ietf/name/fixtures/names.json b/ietf/name/fixtures/names.json index d0ebca2b5..4cac5770b 100644 --- a/ietf/name/fixtures/names.json +++ b/ietf/name/fixtures/names.json @@ -4126,6 +4126,44 @@ "model": "mailtrigger.mailtrigger", "pk": "sub_new_wg_00" }, + { + "model": "mailtrigger.mailtrigger", + "pk": "review_assignment_changed", + "fields": { + "desc": "Recipients for a change to a review assignment", + "to": [ + "review_assignment_reviewer", + "review_assignment_review_req_by", + "review_secretaries" + ], + "cc": [] + } + }, + { + "model": "mailtrigger.mailtrigger", + "pk": "review_req_changed", + "fields": { + "desc": "Recipients for a change to a review request", + "to": [ + "review_req_requested_by", + "review_req_reviewers", + "review_secretaries" + ], + "cc": [] + } + }, + { + "model": "mailtrigger.mailtrigger", + "pk": "review_availability_changed", + "fields": { + "desc": "Recipients for a change to a reviewer's availability", + "to": [ + "review_reviewer", + "group_secretaries" + ], + "cc": [] + } + }, { "fields": { "desc": "The person providing a comment to nomcom", @@ -4702,6 +4740,54 @@ "model": "mailtrigger.recipient", "pk": "submission_submitter" }, + { + "model": "mailtrigger.recipient", + "pk": "review_assignment_reviewer", + "fields": { + "desc": "The reviewer assigned to a review assignment", + "template": "{% if not skip_review_reviewer %}{{review_assignment.reviewer.email_address}}{% endif %}" + } + }, + { + "model": "mailtrigger.recipient", + "pk": "review_assignment_review_req_by", + "fields": { + "desc": "The requester of an assigned review", + "template": "{% if not skip_review_requested_by %}{{review_assignment.review_request.requested_by.email_address}}{% endif %}" + } + }, + { + "model": "mailtrigger.recipient", + "pk": "review_req_requested_by", + "fields": { + "desc": "The requester of a review", + "template": "{% if not skip_review_requested_by %}{{review_req.requested_by.email_address}}{% endif %}" + } + }, + { + "model": "mailtrigger.recipient", + "pk": "review_req_reviewers", + "fields": { + "desc": "All reviewers assigned to a review request", + "template": null + } + }, + { + "model": "mailtrigger.recipient", + "pk": "review_secretaries", + "fields": { + "desc": "The secretaries of the review team of a review request or assignment", + "template": null + } + }, + { + "model": "mailtrigger.recipient", + "pk": "review_reviewer", + "fields": { + "desc": "A single reviewer", + "template": "{{reviewer.email_address}}" + } + }, { "fields": { "desc": "", diff --git a/ietf/review/utils.py b/ietf/review/utils.py index 24f1e02c5..e26c075df 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -22,12 +22,13 @@ from ietf.doc.models import (Document, ReviewRequestDocEvent, ReviewAssignmentDo LastCallDocEvent, TelechatDocEvent, DocumentAuthor, DocAlias) from ietf.iesg.models import TelechatDate +from ietf.mailtrigger.utils import gather_address_lists from ietf.person.models import Person from ietf.ietfauth.utils import has_role, is_authorized_in_doc_stream from ietf.review.models import (ReviewRequest, ReviewAssignment, ReviewRequestStateName, ReviewTypeName, ReviewerSettings, UnavailablePeriod, ReviewWish, NextReviewerInTeam, - ReviewTeamSettings, ReviewSecretarySettings) -from ietf.utils.mail import send_mail, get_email_addresses_from_text + ReviewSecretarySettings) +from ietf.utils.mail import send_mail from ietf.doc.utils import extract_complete_replaces_ancestor_mapping_for_docs def active_review_teams(): @@ -329,128 +330,75 @@ def latest_review_assignments_for_reviewers(team, days_back=365): return assignment_data_for_reviewers def email_review_assignment_change(request, review_assignment, subject, msg, by, notify_secretary, notify_reviewer, notify_requested_by): + to, cc = gather_address_lists( + 'review_assignment_changed', + skipped_recipients=[Person.objects.get(name="(System)").formatted_email(), by.email_address()], + doc=review_assignment.review_request.doc, + group=review_assignment.review_request.team, + review_assignment=review_assignment, + skip_review_secretary=not notify_secretary, + skip_review_reviewer=not notify_reviewer, + skip_review_requested_by=not notify_requested_by, + ) - system_email = Person.objects.get(name="(System)").formatted_email() - - to = set() - - def extract_email_addresses(objs): - for o in objs: - if o and o.person!=by: - e = o.formatted_email() - if e != system_email: - to.add(e) - - if notify_secretary: - rts = ReviewTeamSettings.objects.filter(group=review_assignment.review_request.team).first() - if rts and rts.secr_mail_alias and rts.secr_mail_alias.strip() != '': - for addr in get_email_addresses_from_text(rts.secr_mail_alias): - to.add(addr) - else: - extract_email_addresses(Role.objects.filter(name="secr", group=review_assignment.review_request.team).distinct()) - if notify_reviewer: - extract_email_addresses([review_assignment.reviewer]) - if notify_requested_by: - extract_email_addresses([review_assignment.review_request.requested_by.email()]) - - if not to: - return - - to = list(to) - - url = urlreverse("ietf.doc.views_review.review_request_forced_login", kwargs={ "name": review_assignment.review_request.doc.name, "request_id": review_assignment.review_request.pk }) - url = request.build_absolute_uri(url) - send_mail(request, to, request.user.person.formatted_email(), subject, "review/review_request_changed.txt", { - "review_req_url": url, - "review_req": review_assignment.review_request, - "msg": msg, - }) + if to or cc: + url = urlreverse("ietf.doc.views_review.review_request_forced_login", kwargs={ "name": review_assignment.review_request.doc.name, "request_id": review_assignment.review_request.pk }) + url = request.build_absolute_uri(url) + send_mail(request, to, request.user.person.formatted_email(), subject, "review/review_request_changed.txt", { + "review_req_url": url, + "review_req": review_assignment.review_request, + "msg": msg, + }, cc=cc) def email_review_request_change(request, review_req, subject, msg, by, notify_secretary, notify_reviewer, notify_requested_by): - """Notify stakeholders about change, skipping a party if the change - was done by that party.""" - - system_email = Person.objects.get(name="(System)").formatted_email() - - to = set() - - def extract_email_addresses(objs): - for o in objs: - if o and o.person!=by: - e = o.formatted_email() - if e != system_email: - to.add(e) - - if notify_secretary: - rts = ReviewTeamSettings.objects.filter(group=review_req.team).first() - if rts and rts.secr_mail_alias and rts.secr_mail_alias.strip() != '': - for addr in get_email_addresses_from_text(rts.secr_mail_alias): - to.add(addr) - else: - extract_email_addresses(Role.objects.filter(name="secr", group=review_req.team).distinct()) - if notify_reviewer: - for assignment in review_req.reviewassignment_set.all(): - extract_email_addresses([assignment.reviewer]) - if notify_requested_by: - extract_email_addresses([review_req.requested_by.email()]) - - if not to: - return - - to = list(to) - - url = urlreverse("ietf.doc.views_review.review_request_forced_login", kwargs={ "name": review_req.doc.name, "request_id": review_req.pk }) - url = request.build_absolute_uri(url) - send_mail(request, to, request.user.person.formatted_email(), subject, "review/review_request_changed.txt", { - "review_req_url": url, - "review_req": review_req, - "msg": msg, - }, - ) + was done by that party.""" + (to, cc) = gather_address_lists( + 'review_req_changed', + skipped_recipients=[Person.objects.get(name="(System)").formatted_email(), by.email_address()], + doc=review_req.doc, + group=review_req.team, + review_request=review_req, + skip_review_secretary=not notify_secretary, + skip_review_reviewer=not notify_reviewer, + skip_review_requested_by=not notify_requested_by, + ) + + if to or cc: + url = urlreverse("ietf.doc.views_review.review_request_forced_login", kwargs={ "name": review_req.doc.name, "request_id": review_req.pk }) + url = request.build_absolute_uri(url) + send_mail(request, to, request.user.person.formatted_email(), subject, "review/review_request_changed.txt", { + "review_req_url": url, + "review_req": review_req, + "msg": msg, + }, + cc=cc, + ) def email_reviewer_availability_change(request, team, reviewer_role, msg, by): """Notify possibly both secretary and reviewer about change, skipping a party if the change was done by that party.""" + (to, cc) = gather_address_lists( + 'review_availability_changed', + skipped_recipients=[Person.objects.get(name="(System)").formatted_email(), by.email_address()], + group=team, + reviewer=reviewer_role, + ) - system_email = Person.objects.get(name="(System)").formatted_email() + if to or cc: + subject = "Reviewer availability of {} changed in {}".format(reviewer_role.person, team.acronym) + + url = urlreverse("ietf.group.views.reviewer_overview", kwargs={ "group_type": team.type_id, "acronym": team.acronym }) + url = request.build_absolute_uri(url) + send_mail(request, to, None, subject, "review/reviewer_availability_changed.txt", { + "reviewer_overview_url": url, + "reviewer": reviewer_role.person, + "team": team, + "msg": msg, + "by": by, + }, cc=cc) - to = [] - - def extract_email_addresses(objs): - if any(o.person == by for o in objs if o): - l = [] - else: - l = [] - for o in objs: - if o: - e = o.formatted_email() - if e != system_email: - l.append(e) - - for e in l: - if e not in to: - to.append(e) - - extract_email_addresses(Role.objects.filter(name="secr", group=team).distinct()) - - extract_email_addresses([reviewer_role]) - - if not to: - return - - subject = "Reviewer availability of {} changed in {}".format(reviewer_role.person, team.acronym) - - url = urlreverse("ietf.group.views.reviewer_overview", kwargs={ "group_type": team.type_id, "acronym": team.acronym }) - url = request.build_absolute_uri(url) - send_mail(request, to, None, subject, "review/reviewer_availability_changed.txt", { - "reviewer_overview_url": url, - "reviewer": reviewer_role.person, - "team": team, - "msg": msg, - "by": by, - }) def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=False): assert review_req.state_id in ("requested", "assigned")