Fix ietf-tools#2354 - Make review_completed configurable per team and review type

This includes a migration to change mailtrigger slugs to be up to 64
characters instead of 32, as some slugs would not fit and require clunky
abbreviations.
A data migration creates triggers for existing teams, and they are also
created on the fly if a trigger does not exist yet, providing a safe
fallback for new review teams. The review_completed mailtrigger serves
as the template for new triggers.
This commit also includes tests for gather_address_lists(), as none
existed.

Commit ready for merge.
 - Legacy-Id: 16680

Author: Sasha Romijn

ietf/doc/views_review.py

@@ -571,7 +571,17 @@ def complete_review(request, name, assignment_id):
     if not (is_reviewer or can_manage_request):
         return HttpResponseForbidden("You do not have permission to perform this action")
 
-    (to, cc) = gather_address_lists('review_completed', review_req = assignment.review_request)
+    team_acronym = assignment.review_request.team.acronym.lower()
+    request_type = assignment.review_request.type
+    mailtrigger_slug = 'review_completed_{}_{}'.format(team_acronym, request_type.slug)
+    # Description is only used if the mailtrigger does not exist yet.
+    mailtrigger_desc = 'Recipients when a {} {} review is completed'.format(team_acronym, request_type)
+    to, cc = gather_address_lists(
+        mailtrigger_slug,
+        create_from_slug_if_not_exists='review_completed',
+        desc_if_not_exists=mailtrigger_desc,
+        review_req=assignment.review_request
+    )
 
     if request.method == "POST":
         form = CompleteReviewForm(assignment, is_reviewer,
@@ -588,7 +598,7 @@ def complete_review(request, name, assignment_id):
                         strip_prefix(assignment.review_request.doc.name, "draft-"),
                         form.cleaned_data["reviewed_rev"],
                         assignment.review_request.team.acronym,
-                        assignment.review_request.type.slug,
+                        request_type.slug,
                         xslugify(assignment.reviewer.person.ascii_parts()[3]),
                         datetime.date.today().isoformat(),
                     ]
@@ -761,7 +771,8 @@ def complete_review(request, name, assignment_id):
         }
 
         try:
-            initial['review_content'] = render_to_string('/group/%s/review/content_templates/%s.txt' % (assignment.review_request.team.acronym, assignment.review_request.type.slug), {'assignment':assignment,'today':datetime.date.today()}) 
+            initial['review_content'] = render_to_string('/group/%s/review/content_templates/%s.txt' % (assignment.review_request.team.acronym,
+                                                                                                        request_type.slug), {'assignment':assignment, 'today':datetime.date.today()}) 
         except TemplateDoesNotExist:
             pass
 

ietf/mailtrigger/migrations/0008_lengthen_mailtrigger_slug.py

@@ -0,0 +1,32 @@
+# Copyright The IETF Trust 2015-2019, All Rights Reserved
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.23 on 2019-08-30 09:02
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('mailtrigger', '0007_add_review_mailtriggers'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='mailtrigger',
+            name='slug',
+            field=models.CharField(max_length=64, primary_key=True, serialize=False),
+        ),
+        # The above migration will not update the ManyToMany tables, which also reference
+        # the mailtrigger pk as varchar(32), so manual SQL is used.
+        # https://code.djangoproject.com/ticket/25012
+        migrations.RunSQL(
+            sql='ALTER TABLE `mailtrigger_mailtrigger_to` MODIFY `mailtrigger_id` varchar(64);',
+            reverse_sql='ALTER TABLE `mailtrigger_mailtrigger_to` MODIFY `mailtrigger_id` varchar(32);',
+        ),
+        migrations.RunSQL(
+            sql='ALTER TABLE `mailtrigger_mailtrigger_cc` MODIFY `mailtrigger_id` varchar(64);',
+            reverse_sql='ALTER TABLE `mailtrigger_mailtrigger_cc` MODIFY `mailtrigger_id` varchar(32);',
+        ),
+    ]

ietf/mailtrigger/migrations/0009_custom_review_complete_mailtriggers.py

@@ -0,0 +1,52 @@
+# 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):
+    ReviewTeamSettings = apps.get_model('review', 'ReviewTeamSettings')
+    MailTrigger = apps.get_model('mailtrigger', 'Mailtrigger')
+    Group = apps.get_model('group', 'Group')
+    GroupFeatures = apps.get_model('group', 'GroupFeatures')
+
+    template = MailTrigger.objects.get(slug='review_completed')
+    template.desc = 'Default template for recipients when an review is completed - ' \
+                    'customised mail triggers are used/created per team and review type.'
+    template.save()
+    
+    for group in Group.objects.all().only('pk', 'type', 'acronym'):
+        if not GroupFeatures.objects.get(type=group.type).has_reviews:
+            continue
+        try:
+            review_team = ReviewTeamSettings.objects.get(group=group.pk)
+        except ReviewTeamSettings.DoesNotExist:
+            continue
+        team_acronym = group.acronym.lower()
+        for review_type in review_team.review_types.all():
+            slug = 'review_completed_{}_{}'.format(team_acronym, review_type.slug)
+            desc = 'Recipients when a {} {} review is completed'.format(team_acronym, review_type)
+            if MailTrigger.objects.filter(slug=slug):
+                # Never overwrite existing triggers
+                continue
+            mailtrigger = MailTrigger.objects.create(slug=slug, desc=desc)
+            mailtrigger.to.set(template.to.all())
+            mailtrigger.cc.set(template.cc.all())
+    
+
+def reverse(apps, schema_editor):
+    pass
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('mailtrigger', '0008_lengthen_mailtrigger_slug'),
+        ('review', '0014_document_primary_key_cleanup'),
+        ('group', '0019_rename_field_document2'),
+    ]
+
+    operations = [
+        migrations.RunPython(forward, reverse)
+    ]

ietf/mailtrigger/models.py

@@ -35,7 +35,7 @@ def clean_duplicates(addrlist):
 
 @python_2_unicode_compatible
 class MailTrigger(models.Model):
-    slug = models.CharField(max_length=32, primary_key=True)
+    slug = models.CharField(max_length=64, primary_key=True)
     desc = models.TextField(blank=True)
     to   = models.ManyToManyField('Recipient', blank=True, related_name='used_in_to')
     cc   = models.ManyToManyField('Recipient', blank=True, related_name='used_in_cc')

ietf/mailtrigger/test_utils.py

@@ -0,0 +1,46 @@
+# Copyright The IETF Trust 2015-2019, All Rights Reserved
+# -*- coding: utf-8 -*-
+
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+from ietf.doc.factories import WgDraftFactory
+from ietf.mailtrigger.models import MailTrigger
+from .utils import gather_address_lists
+from ietf.utils.test_utils import TestCase
+import six
+
+
+class GatherAddressListsTests(TestCase):
+    def setUp(self):
+        self.doc = WgDraftFactory(group__acronym='mars', rev='01')
+        self.author_address = self.doc.name + '@ietf.org'
+
+    def test_regular_trigger(self):
+        to, cc = gather_address_lists('doc_pulled_from_rfc_queue', doc=self.doc)
+        # Despite its name, assertCountEqual also compares content, but does not care for ordering
+        six.assertCountEqual(self, to, ['iana@iana.org', 'rfc-editor@rfc-editor.org'])
+        six.assertCountEqual(self, cc, ['The IESG <iesg@ietf.org>', self.author_address,
+                                        'mars-chairs@ietf.org', 'iesg-secretary@ietf.org'])
+
+    def test_skipped_recipient(self):
+        to, cc = gather_address_lists('doc_pulled_from_rfc_queue', doc=self.doc,
+                                      skipped_recipients=['mars-chairs@ietf.org', 'iana@iana.org'])
+        six.assertCountEqual(self, to, ['rfc-editor@rfc-editor.org'])
+        six.assertCountEqual(self, cc, ['The IESG <iesg@ietf.org>', self.author_address,
+                                        'iesg-secretary@ietf.org'])
+
+    def test_trigger_does_not_exist(self):
+        with self.assertRaises(MailTrigger.DoesNotExist):
+            gather_address_lists('this-does-not-exist______', doc=self.doc)
+
+    def test_create_if_not_exists(self):
+        new_slug = 'doc_pulled_from_rfc_special_autocreated'
+        new_desc = 'Autocreated mailtrigger from doc_pulled_from_rfc_queue'
+        to, cc = gather_address_lists(new_slug, doc=self.doc, desc_if_not_exists=new_desc,
+                                      create_from_slug_if_not_exists='doc_pulled_from_rfc_queue')
+        six.assertCountEqual(self, to, ['iana@iana.org', 'rfc-editor@rfc-editor.org'])
+        six.assertCountEqual(self, cc, ['The IESG <iesg@ietf.org>', self.author_address,
+                                        'mars-chairs@ietf.org', 'iesg-secretary@ietf.org'])
+        new_trigger = MailTrigger.objects.get(slug=new_slug)
+        self.assertEqual(new_trigger.desc, new_desc)

ietf/mailtrigger/utils.py

@@ -19,8 +19,10 @@ def as_strings(self,compact=True):
 
         return namedtuple('AddrListsAsStrings',['to','cc'])(to=to_string,cc=cc_string)
 
-def gather_address_lists(slug, skipped_recipients=None, **kwargs):
-    mailtrigger = MailTrigger.objects.get(slug=slug)
+
+def gather_address_lists(slug, skipped_recipients=None, create_from_slug_if_not_exists=None, 
+                         desc_if_not_exists=None, **kwargs):
+    mailtrigger = get_mailtrigger(slug, create_from_slug_if_not_exists, desc_if_not_exists)
 
     to = set()
     for recipient in mailtrigger.to.all():
@@ -38,6 +40,21 @@ def gather_address_lists(slug, skipped_recipients=None, **kwargs):
 
     return AddrLists(to=list(to),cc=list(cc))
 
+
+def get_mailtrigger(slug, create_from_slug_if_not_exists, desc_if_not_exists):
+    try:
+        mailtrigger = MailTrigger.objects.get(slug=slug)
+    except MailTrigger.DoesNotExist:
+        if create_from_slug_if_not_exists and desc_if_not_exists:
+            template = MailTrigger.objects.get(slug=create_from_slug_if_not_exists)
+            mailtrigger = MailTrigger.objects.create(slug=slug, desc=desc_if_not_exists)
+            mailtrigger.to.set(template.to.all())
+            mailtrigger.cc.set(template.cc.all())
+        else:
+            raise
+    return mailtrigger
+
+
 def gather_relevant_expansions(**kwargs):
 
     def starts_with(prefix):

ietf/name/fixtures/names.json

@@ -3902,7 +3902,7 @@
         "review_doc_all_parties",
         "review_doc_group_mail_list"
       ],
-      "desc": "Recipients when an review is completed",
+      "desc": "Default template for recipients when an review is completed - customised mail triggers are used/created per team and review type.",
       "to": [
         "review_team_mail_list"
       ]