Merged in [16672] from sasha@dashcare.nl:

    Fix ietf-tools#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.
 - Legacy-Id: 16793
Note: SVN reference [16672] has been migrated to Git commit 88b7b45

Author: levkowetz

ietf/doc/tests_review.py

@@ -223,6 +223,8 @@ def test_close_request(self):
         self.assertNotIn("review_request_close_comment", e.desc.lower())
 
         self.assertEqual(len(outbox), 1)
+        self.assertIn('<reviewer@example.com>', outbox[0]["To"])
+        self.assertNotIn("<reviewsecretary@example.com>", outbox[0]["To"])
         mail_content = outbox[0].get_payload(decode=True).decode("utf-8").lower()
         self.assertIn("closed", mail_content)
         self.assertIn("review_request_close_comment", mail_content)
@@ -420,6 +422,7 @@ def test_assign_reviewer(self):
         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 @@ def test_reject_reviewer_assignment(self):
         self.assertEqual(e.type, "closed_review_assignment")
         self.assertTrue("rejected" in e.desc)
         self.assertEqual(len(outbox), 1)
+        self.assertIn(assignment.reviewer.address, outbox[0]["To"])
+        self.assertNotIn("<reviewsecretary@example.com>", 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 @@ def test_partially_complete_review(self):
         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.assertIn("<reviewsecretary@example.com>", outbox[0]["To"])
+        self.assertNotIn(assignment.reviewer.address, 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 @@ def test_edit_deadline(self):
         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 @@ def test_edit_deadline(self):
         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.assertIn('<reviewsecretary@example.com>', outbox[0]["To"])
+        self.assertIn('<reviewer@example.com>', outbox[0]["To"])
+        self.assertIn('Deadline changed', outbox[0]['Subject'])
 
     def test_mark_no_response(self):
         assignment = ReviewAssignmentFactory()
@@ -1017,12 +1027,18 @@ def test_mark_no_response(self):
         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.assertIn(assignment.reviewer.address,  outbox[0]["To"])
+        self.assertNotIn('<reviewsecretary@example.com>', outbox[0]["To"])
+        self.assertIn('Reviewer assignment marked no-response', 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

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

ietf/mailtrigger/models.py

@@ -184,7 +184,20 @@ def gather_group_secretaries(self, **kwargs):
                 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:

ietf/mailtrigger/utils.py

@@ -1,3 +1,5 @@
+# Copyright The IETF Trust 2015-2019, All Rights Reserved
+
 from collections import namedtuple
 
 import debug                            # pyflakes:ignore
@@ -17,18 +19,22 @@ def as_strings(self,compact=True):
 
         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))
 

ietf/name/fixtures/names.json

@@ -3956,6 +3956,19 @@
     "model": "mailtrigger.mailtrigger",
     "pk": "resurrection_requested"
   },
+  {
+    "fields": {
+      "cc": [],
+      "desc": "Recipients for a change to a review assignment",
+      "to": [
+        "review_assignment_reviewer",
+        "review_assignment_review_req_by",
+        "review_secretaries"
+      ]
+    },
+    "model": "mailtrigger.mailtrigger",
+    "pk": "review_assignment_changed"
+  },
   {
     "fields": {
       "cc": [],
@@ -3967,6 +3980,18 @@
     "model": "mailtrigger.mailtrigger",
     "pk": "review_assignments_summarized"
   },
+  {
+    "fields": {
+      "cc": [],
+      "desc": "Recipients for a change to a reviewer's availability",
+      "to": [
+        "review_reviewer",
+        "group_secretaries"
+      ]
+    },
+    "model": "mailtrigger.mailtrigger",
+    "pk": "review_availability_changed"
+  },
   {
     "fields": {
       "cc": [
@@ -3994,6 +4019,19 @@
     "model": "mailtrigger.mailtrigger",
     "pk": "review_notify_ad"
   },
+  {
+    "fields": {
+      "cc": [],
+      "desc": "Recipients for a change to a review request",
+      "to": [
+        "review_req_requested_by",
+        "review_req_reviewers",
+        "review_secretaries"
+      ]
+    },
+    "model": "mailtrigger.mailtrigger",
+    "pk": "review_req_changed"
+  },
   {
     "fields": {
       "cc": [
@@ -4646,6 +4684,22 @@
     "model": "mailtrigger.recipient",
     "pk": "nominee"
   },
+  {
+    "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_assignment_review_req_by"
+  },
+  {
+    "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_reviewer"
+  },
   {
     "fields": {
       "desc": "The reviewed document's responsible area director",
@@ -4670,6 +4724,38 @@
     "model": "mailtrigger.recipient",
     "pk": "review_doc_group_mail_list"
   },
+  {
+    "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_requested_by"
+  },
+  {
+    "fields": {
+      "desc": "All reviewers assigned to a review request",
+      "template": null
+    },
+    "model": "mailtrigger.recipient",
+    "pk": "review_req_reviewers"
+  },
+  {
+    "fields": {
+      "desc": "A single reviewer",
+      "template": "{{reviewer.email_address}}"
+    },
+    "model": "mailtrigger.recipient",
+    "pk": "review_reviewer"
+  },
+  {
+    "fields": {
+      "desc": "The secretaries of the review team of a review request or assignment",
+      "template": null
+    },
+    "model": "mailtrigger.recipient",
+    "pk": "review_secretaries"
+  },
   {
     "fields": {
       "desc": "The ADs of the team reviewing the document",