From 1390ae073c224780aef895d57e665ef4fdfa7c18 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Fri, 30 Aug 2019 17:40:55 +0000 Subject: [PATCH] Fix #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 --- ietf/doc/views_review.py | 17 ++++-- .../0008_lengthen_mailtrigger_slug.py | 32 ++++++++++++ ...009_custom_review_complete_mailtriggers.py | 52 +++++++++++++++++++ ietf/mailtrigger/models.py | 2 +- ietf/mailtrigger/test_utils.py | 46 ++++++++++++++++ ietf/mailtrigger/utils.py | 21 +++++++- ietf/name/fixtures/names.json | 2 +- 7 files changed, 165 insertions(+), 7 deletions(-) create mode 100644 ietf/mailtrigger/migrations/0008_lengthen_mailtrigger_slug.py create mode 100644 ietf/mailtrigger/migrations/0009_custom_review_complete_mailtriggers.py create mode 100644 ietf/mailtrigger/test_utils.py diff --git a/ietf/doc/views_review.py b/ietf/doc/views_review.py index 0d7f66ec0..8bbe5c6ab 100644 --- a/ietf/doc/views_review.py +++ b/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 diff --git a/ietf/mailtrigger/migrations/0008_lengthen_mailtrigger_slug.py b/ietf/mailtrigger/migrations/0008_lengthen_mailtrigger_slug.py new file mode 100644 index 000000000..fe5f83dd9 --- /dev/null +++ b/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);', + ), + ] diff --git a/ietf/mailtrigger/migrations/0009_custom_review_complete_mailtriggers.py b/ietf/mailtrigger/migrations/0009_custom_review_complete_mailtriggers.py new file mode 100644 index 000000000..ec0acfdf5 --- /dev/null +++ b/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) + ] diff --git a/ietf/mailtrigger/models.py b/ietf/mailtrigger/models.py index 2a72b622b..b642a9f13 100644 --- a/ietf/mailtrigger/models.py +++ b/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') diff --git a/ietf/mailtrigger/test_utils.py b/ietf/mailtrigger/test_utils.py new file mode 100644 index 000000000..91007ebdf --- /dev/null +++ b/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 ', 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 ', 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 ', 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) diff --git a/ietf/mailtrigger/utils.py b/ietf/mailtrigger/utils.py index 86455c61d..2391592e8 100644 --- a/ietf/mailtrigger/utils.py +++ b/ietf/mailtrigger/utils.py @@ -19,8 +19,10 @@ class AddrLists(namedtuple('AddrLists',['to','cc'])): 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): diff --git a/ietf/name/fixtures/names.json b/ietf/name/fixtures/names.json index 4cac5770b..509fcd23e 100644 --- a/ietf/name/fixtures/names.json +++ b/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" ]