altf4arnold/datatracker
1
0
Fork 0
Code Issues Pull requests Actions 2 Packages Projects Releases Wiki Activity

Fix #2328 - Use mailtriggers to find destinations in review app

Browse source 
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
This commit is contained in:
Sasha Romijn 2019-08-27 15:15:13 +00:00
parent 3942f9acc7
commit 88b7b45b0e
6 changed files with 272 additions and 117 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
ietf/doc/tests_review.py
View file

@ -223,6 +223,8 @@ class ReviewTests(TestCase):
self.assertFalse("review_request_close_comment" in e.desc.lower())
self.assertEqual(len(outbox), 1)
self.assertTrue('<reviewer@example.com>' in outbox[0]["To"])
self.assertFalse("<reviewsecretary@example.com>" 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" <reviewer@example.com>', 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("<reviewsecretary@example.com>" 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("<reviewsecretary@example.com>" 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('<reviewsecretary@example.com>' in outbox[0]["To"])
self.assertTrue('<reviewer@example.com>' 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('<reviewsecretary@example.com>' 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

86
ietf/mailtrigger/migrations/0007_add_review_mailtriggers.py Normal file
View file

@ -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)
]

15
ietf/mailtrigger/models.py
View file

@ -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:

8
ietf/mailtrigger/utils.py
View file

@ -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))

86
ietf/name/fixtures/names.json
View file

@ -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": "",

174
ietf/review/utils.py
View file

@ -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")