From 051aee57a82d870463962d3e429b9f1bd3dbc0bb Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Mon, 17 Jun 2024 23:19:38 -0300 Subject: [PATCH 1/7] refactor: generate group aliases on the fly --- ietf/group/utils.py | 12 +++++++++--- ietf/group/views.py | 29 ++++++++++++++++------------- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/ietf/group/utils.py b/ietf/group/utils.py index 51696eb39b1..b04f34f3fe7 100644 --- a/ietf/group/utils.py +++ b/ietf/group/utils.py @@ -373,6 +373,12 @@ class GroupAliasGenerator: ] # This should become groupfeature driven... no_ad_group_types = ["rg", "rag", "team", "program", "rfcedtyp", "edappr", "edwg"] + def __init__(self, group_queryset=None): + if group_queryset is None: + self.group_queryset = Group.objects.all() + else: + self.group_queryset = group_queryset + def __iter__(self): show_since = timezone.now() - datetime.timedelta(days=self.days) @@ -384,7 +390,7 @@ def __iter__(self): if g == "program": domains.append("iab") - entries = Group.objects.filter(type=g).all() + entries = self.group_queryset.filter(type=g).all() active_entries = entries.filter(state__in=self.active_states) inactive_recent_entries = entries.exclude( state__in=self.active_states @@ -405,7 +411,7 @@ def __iter__(self): yield name + "-chairs", domains, list(chair_emails) # The area lists include every chair in active working groups in the area - areas = Group.objects.filter(type="area").all() + areas = self.group_queryset.filter(type="area").all() active_areas = areas.filter(state__in=self.active_states) for area in active_areas: name = area.acronym @@ -418,7 +424,7 @@ def __iter__(self): # Other groups with chairs that require Internet-Draft submission approval gtypes = GroupTypeName.objects.values_list("slug", flat=True) - special_groups = Group.objects.filter( + special_groups = self.group_queryset.filter( type__features__req_subm_approval=True, acronym__in=gtypes, state="active" ) for group in special_groups: diff --git a/ietf/group/views.py b/ietf/group/views.py index 09b1ac933fa..77e0f42736c 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -81,7 +81,8 @@ can_manage_materials, group_attribute_change_desc, construct_group_menu_context, get_group_materials, save_group_in_history, can_manage_group, update_role_set, - get_group_or_404, setup_default_community_list_for_group, fill_in_charter_info) + get_group_or_404, setup_default_community_list_for_group, fill_in_charter_info, + GroupAliasGenerator) # from ietf.ietfauth.utils import has_role, is_authorized_in_group from ietf.mailtrigger.utils import gather_relevant_expansions @@ -581,19 +582,21 @@ def group_about_status_edit(request, acronym, group_type=None): ) def get_group_email_aliases(acronym, group_type): - if acronym: - pattern = re.compile(r'expand-(%s)(-\w+)@.*? +(.*)$'%acronym) - else: - pattern = re.compile(r'expand-(.*?)(-\w+)@.*? +(.*)$') - aliases = [] - with io.open(settings.GROUP_VIRTUAL_PATH,"r") as virtual_file: - for line in virtual_file.readlines(): - m = pattern.match(line) - if m: - if acronym or not group_type or Group.objects.filter(acronym=m.group(1),type__slug=group_type): - aliases.append({'acronym':m.group(1),'alias_type':m.group(2),'expansion':m.group(3)}) - return aliases + group_queryset = Group.objects.all() + if acronym: + group_queryset = group_queryset.filter(acronym=acronym) + if group_type: + group_queryset = group_queryset.filter(type__slug=group_type) + for (alias, domains, alist) in GroupAliasGenerator(group_queryset): + acro, _hyphen, alias_type = alias.partition("-") + expansion = ", ".join(sorted(alist)) + aliases.append({ + "acronym": acro, + "alias_type": f"-{alias_type}" if alias_type else "", + "expansion": expansion, + }) + return sorted(aliases, key=lambda a: a["acronym"]) def email(request, acronym, group_type=None): group = get_group_or_404(acronym, group_type) From f53549440b819167ccd534c513bb9c54d93a0e59 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Mon, 17 Jun 2024 23:20:17 -0300 Subject: [PATCH 2/7] chore: remove group alias file check --- ietf/checks.py | 27 --------------------------- ietf/group/views.py | 15 --------------- 2 files changed, 42 deletions(-) diff --git a/ietf/checks.py b/ietf/checks.py index f66182a027a..073598746d6 100644 --- a/ietf/checks.py +++ b/ietf/checks.py @@ -29,33 +29,6 @@ def already_ran(): return False -@checks.register('files') -def check_group_email_aliases_exists(app_configs, **kwargs): - from ietf.group.views import check_group_email_aliases - # - if already_ran(): - return [] - # - errors = [] - try: - ok = check_group_email_aliases() - if not ok: - errors.append(checks.Error( - "Found no aliases in the group email aliases file\n'%s'."%settings.GROUP_ALIASES_PATH, - hint="These should be created by the infrastructure using ietf/bin/aliases-from-json.py.", - obj=None, - id="datatracker.E0002", - )) - except IOError as e: - errors.append(checks.Error( - "Could not read group email aliases:\n %s" % e, - hint="These should be created by the infrastructure using ietf/bin/aliases-from-json.py.", - obj=None, - id="datatracker.E0003", - )) - - return errors - @checks.register('files') def check_doc_email_aliases_exists(app_configs, **kwargs): from ietf.doc.views_doc import check_doc_email_aliases diff --git a/ietf/group/views.py b/ietf/group/views.py index 77e0f42736c..0df94298686 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -138,21 +138,6 @@ def extract_last_name(role): return role.person.name_parts()[3] -def check_group_email_aliases(): - pattern = re.compile(r'expand-(.*?)(-\w+)@.*? +(.*)$') - tot_count = 0 - good_count = 0 - with io.open(settings.GROUP_VIRTUAL_PATH,"r") as virtual_file: - for line in virtual_file.readlines(): - m = pattern.match(line) - tot_count += 1 - if m: - good_count += 1 - if good_count > 50 and tot_count < 3*good_count: - return True - return False - - def response_from_file(fpath: Path) -> HttpResponse: """Helper to shovel a file back in an HttpResponse""" try: From fdced99ed7184e7381f5fe2252b945aceed5ebbf Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Mon, 17 Jun 2024 23:28:55 -0300 Subject: [PATCH 3/7] chore: drop group alias settings, fix lint --- ietf/group/tests.py | 22 ---------------------- ietf/group/views.py | 2 -- ietf/settings.py | 4 ---- k8s/settings_local.py | 2 -- 4 files changed, 30 deletions(-) diff --git a/ietf/group/tests.py b/ietf/group/tests.py index 0d5cddf105e..38d821ab6ac 100644 --- a/ietf/group/tests.py +++ b/ietf/group/tests.py @@ -1,13 +1,9 @@ # Copyright The IETF Trust 2013-2020, All Rights Reserved # -*- coding: utf-8 -*- -import os import datetime import json -from tempfile import NamedTemporaryFile - -from django.conf import settings from django.urls import reverse as urlreverse from django.db.models import Q from django.test import Client @@ -132,24 +128,6 @@ def test_group_document_dependencies(self): class GenerateGroupAliasesTests(TestCase): - def setUp(self): - super().setUp() - self.doc_aliases_file = NamedTemporaryFile(delete=False, mode='w+') - self.doc_aliases_file.close() - self.doc_virtual_file = NamedTemporaryFile(delete=False, mode='w+') - self.doc_virtual_file.close() - self.saved_draft_aliases_path = settings.GROUP_ALIASES_PATH - self.saved_draft_virtual_path = settings.GROUP_VIRTUAL_PATH - settings.GROUP_ALIASES_PATH = self.doc_aliases_file.name - settings.GROUP_VIRTUAL_PATH = self.doc_virtual_file.name - - def tearDown(self): - settings.GROUP_ALIASES_PATH = self.saved_draft_aliases_path - settings.GROUP_VIRTUAL_PATH = self.saved_draft_virtual_path - os.unlink(self.doc_aliases_file.name) - os.unlink(self.doc_virtual_file.name) - super().tearDown() - def test_generator_class(self): """The GroupAliasGenerator should generate the same lists as the old mgmt cmd""" # clean out test fixture group roles we don't need for this test diff --git a/ietf/group/views.py b/ietf/group/views.py index 0df94298686..21be6e39620 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -37,9 +37,7 @@ import copy import datetime import itertools -import io import math -import re import json import types diff --git a/ietf/settings.py b/ietf/settings.py index f04dd15bcaf..059084602c7 100644 --- a/ietf/settings.py +++ b/ietf/settings.py @@ -1070,10 +1070,6 @@ def skip_unreadable_post(record): DRAFT_VIRTUAL_PATH = os.path.join(TEST_DATA_DIR, "draft-virtual") DRAFT_VIRTUAL_DOMAIN = "virtual.ietf.org" -GROUP_ALIASES_PATH = os.path.join(TEST_DATA_DIR, "group-aliases") -GROUP_VIRTUAL_PATH = os.path.join(TEST_DATA_DIR, "group-virtual") -GROUP_VIRTUAL_DOMAIN = "virtual.ietf.org" - POSTCONFIRM_PATH = "/a/postconfirm/wrapper" USER_PREFERENCE_DEFAULTS = { diff --git a/k8s/settings_local.py b/k8s/settings_local.py index b3bb0f6d190..5a852e4db28 100644 --- a/k8s/settings_local.py +++ b/k8s/settings_local.py @@ -195,8 +195,6 @@ def _multiline_to_list(s): # Path to the email alias lists. Used by ietf.utils.aliases DRAFT_ALIASES_PATH = "/a/postfix/draft-aliases" DRAFT_VIRTUAL_PATH = "/a/postfix/draft-virtual" -GROUP_ALIASES_PATH = "/a/postfix/group-aliases" -GROUP_VIRTUAL_PATH = "/a/postfix/group-virtual" STATIC_URL = os.environ.get("DATATRACKER_STATIC_URL", None) if STATIC_URL is None: From 62d2e5c648d1dadeae4c5793a1bde34a9d6d1782 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Mon, 17 Jun 2024 23:33:55 -0300 Subject: [PATCH 4/7] refactor: rename var to hint it's ignored --- ietf/group/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ietf/group/views.py b/ietf/group/views.py index 21be6e39620..898e7c1c165 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -571,7 +571,7 @@ def get_group_email_aliases(acronym, group_type): group_queryset = group_queryset.filter(acronym=acronym) if group_type: group_queryset = group_queryset.filter(type__slug=group_type) - for (alias, domains, alist) in GroupAliasGenerator(group_queryset): + for (alias, _, alist) in GroupAliasGenerator(group_queryset): acro, _hyphen, alias_type = alias.partition("-") expansion = ", ".join(sorted(alist)) aliases.append({ From 85f594add6cf3e7ff4580733f5cb974d5dfc103b Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 18 Jun 2024 00:01:02 -0300 Subject: [PATCH 5/7] test: update tests --- ietf/group/tests_info.py | 63 ++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/ietf/group/tests_info.py b/ietf/group/tests_info.py index 29a45a32eb8..561f542f420 100644 --- a/ietf/group/tests_info.py +++ b/ietf/group/tests_info.py @@ -2,16 +2,15 @@ # -*- coding: utf-8 -*- -import os import calendar import datetime import io import bleach +import mock from unittest.mock import call, patch from pathlib import Path from pyquery import PyQuery -from tempfile import NamedTemporaryFile import debug # pyflakes:ignore @@ -1925,58 +1924,72 @@ def setUp(self): PersonFactory(user__username='plain') GroupFactory(acronym='mars',parent=GroupFactory(type_id='area')) GroupFactory(acronym='ames',parent=GroupFactory(type_id='area')) - self.group_alias_file = NamedTemporaryFile(delete=False) - self.group_alias_file.write(b"""# Generated by hand at 2015-02-12_16:30:52 -virtual.ietf.org anything -mars-ads@ietf.org xfilter-mars-ads -expand-mars-ads@virtual.ietf.org aread@example.org -mars-chairs@ietf.org xfilter-mars-chairs -expand-mars-chairs@virtual.ietf.org mars_chair@ietf.org -ames-ads@ietf.org xfilter-mars-ads -expand-ames-ads@virtual.ietf.org aread@example.org -ames-chairs@ietf.org xfilter-mars-chairs -expand-ames-chairs@virtual.ietf.org mars_chair@ietf.org -""") - self.group_alias_file.close() - self.saved_group_virtual_path = settings.GROUP_VIRTUAL_PATH - settings.GROUP_VIRTUAL_PATH = self.group_alias_file.name - - def tearDown(self): - settings.GROUP_VIRTUAL_PATH = self.saved_group_virtual_path - os.unlink(self.group_alias_file.name) - super().tearDown() - - def testAliases(self): + + @mock.patch("ietf.group.views.get_group_email_aliases") + def testAliases(self, mock_get_aliases): url = urlreverse('ietf.group.urls_info_details.redirect.email', kwargs=dict(acronym="mars")) r = self.client.get(url) self.assertEqual(r.status_code, 302) + mock_get_aliases.return_value = [ + {"acronym": "mars", "alias_type": "-ads", "expansion": "aread@example.org"}, + {"acronym": "mars", "alias_type": "-chairs", "expansion": "mars_chair@ietf.org"}, + ] for testdict in [dict(acronym="mars"),dict(acronym="mars",group_type="wg")]: url = urlreverse('ietf.group.urls_info_details.redirect.email', kwargs=testdict) r = self.client.get(url,follow=True) + self.assertEqual( + mock_get_aliases.call_args, + mock.call(testdict.get("acronym", None), testdict.get("group_type", None)), + ) self.assertTrue(all([x in unicontent(r) for x in ['mars-ads@','mars-chairs@']])) self.assertFalse(any([x in unicontent(r) for x in ['ames-ads@','ames-chairs@']])) url = urlreverse('ietf.group.views.email_aliases', kwargs=dict()) login_testing_unauthorized(self, "plain", url) + + mock_get_aliases.return_value = [ + {"acronym": "mars", "alias_type": "-ads", "expansion": "aread@example.org"}, + {"acronym": "mars", "alias_type": "-chairs", "expansion": "mars_chair@ietf.org"}, + {"acronym": "ames", "alias_type": "-ads", "expansion": "aread@example.org"}, + {"acronym": "ames", "alias_type": "-chairs", "expansion": "mars_chair@ietf.org"}, + ] r = self.client.get(url) self.assertTrue(r.status_code,200) + self.assertEqual(mock_get_aliases.call_args, mock.call(None, None)) self.assertTrue(all([x in unicontent(r) for x in ['mars-ads@','mars-chairs@','ames-ads@','ames-chairs@']])) url = urlreverse('ietf.group.views.email_aliases', kwargs=dict(group_type="wg")) + mock_get_aliases.return_value = [ + {"acronym": "mars", "alias_type": "-ads", "expansion": "aread@example.org"}, + {"acronym": "mars", "alias_type": "-chairs", "expansion": "mars_chair@ietf.org"}, + {"acronym": "ames", "alias_type": "-ads", "expansion": "aread@example.org"}, + {"acronym": "ames", "alias_type": "-chairs", "expansion": "mars_chair@ietf.org"}, + ] r = self.client.get(url) self.assertEqual(r.status_code,200) + self.assertEqual(mock_get_aliases.call_args, mock.call(None, "wg")) self.assertContains(r, 'mars-ads@') url = urlreverse('ietf.group.views.email_aliases', kwargs=dict(group_type="rg")) + mock_get_aliases.return_value = [] r = self.client.get(url) self.assertEqual(r.status_code,200) + self.assertEqual(mock_get_aliases.call_args, mock.call(None, "rg")) self.assertNotContains(r, 'mars-ads@') - def testExpansions(self): + @mock.patch("ietf.group.views.get_group_email_aliases") + def testExpansions(self, mock_get_aliases): + mock_get_aliases.return_value = [ + {"acronym": "mars", "alias_type": "-ads", "expansion": "aread@example.org"}, + {"acronym": "mars", "alias_type": "-chairs", "expansion": "mars_chair@ietf.org"}, + {"acronym": "ames", "alias_type": "-ads", "expansion": "aread@example.org"}, + {"acronym": "ames", "alias_type": "-chairs", "expansion": "mars_chair@ietf.org"}, + ] url = urlreverse('ietf.group.views.email', kwargs=dict(acronym="mars")) r = self.client.get(url) self.assertEqual(r.status_code,200) + self.assertEqual(mock_get_aliases.call_args, mock.call("mars", None)) self.assertContains(r, 'Email aliases') self.assertContains(r, 'mars-ads@ietf.org') self.assertContains(r, 'group_personnel_change') From 0d83d036ab52efaff8b234e284f782410f2329d5 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 18 Jun 2024 09:31:25 -0300 Subject: [PATCH 6/7] refactor: move utility to utils --- ietf/group/utils.py | 18 ++++++++++++++++++ ietf/group/views.py | 19 +------------------ 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/ietf/group/utils.py b/ietf/group/utils.py index b04f34f3fe7..68b618120bb 100644 --- a/ietf/group/utils.py +++ b/ietf/group/utils.py @@ -433,6 +433,24 @@ def __iter__(self): yield group.acronym + "-chairs", ["ietf"], list(chair_emails) +def get_group_email_aliases(acronym, group_type): + aliases = [] + group_queryset = Group.objects.all() + if acronym: + group_queryset = group_queryset.filter(acronym=acronym) + if group_type: + group_queryset = group_queryset.filter(type__slug=group_type) + for (alias, _, alist) in GroupAliasGenerator(group_queryset): + acro, _hyphen, alias_type = alias.partition("-") + expansion = ", ".join(sorted(alist)) + aliases.append({ + "acronym": acro, + "alias_type": f"-{alias_type}" if alias_type else "", + "expansion": expansion, + }) + return sorted(aliases, key=lambda a: a["acronym"]) + + def role_holder_emails(): """Get queryset of active Emails for group role holders""" group_types_of_interest = [ diff --git a/ietf/group/views.py b/ietf/group/views.py index 898e7c1c165..f909a31b6d7 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -80,7 +80,7 @@ construct_group_menu_context, get_group_materials, save_group_in_history, can_manage_group, update_role_set, get_group_or_404, setup_default_community_list_for_group, fill_in_charter_info, - GroupAliasGenerator) + get_group_email_aliases) # from ietf.ietfauth.utils import has_role, is_authorized_in_group from ietf.mailtrigger.utils import gather_relevant_expansions @@ -564,23 +564,6 @@ def group_about_status_edit(request, acronym, group_type=None): } ) -def get_group_email_aliases(acronym, group_type): - aliases = [] - group_queryset = Group.objects.all() - if acronym: - group_queryset = group_queryset.filter(acronym=acronym) - if group_type: - group_queryset = group_queryset.filter(type__slug=group_type) - for (alias, _, alist) in GroupAliasGenerator(group_queryset): - acro, _hyphen, alias_type = alias.partition("-") - expansion = ", ".join(sorted(alist)) - aliases.append({ - "acronym": acro, - "alias_type": f"-{alias_type}" if alias_type else "", - "expansion": expansion, - }) - return sorted(aliases, key=lambda a: a["acronym"]) - def email(request, acronym, group_type=None): group = get_group_or_404(acronym, group_type) From 70aa17c39f5413a0f4fb53d7260cbbcc3ea4f6cb Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 18 Jun 2024 09:51:42 -0300 Subject: [PATCH 7/7] test: add test --- ietf/group/tests.py | 62 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/ietf/group/tests.py b/ietf/group/tests.py index 38d821ab6ac..130c68b3fcc 100644 --- a/ietf/group/tests.py +++ b/ietf/group/tests.py @@ -3,6 +3,7 @@ import datetime import json +import mock from django.urls import reverse as urlreverse from django.db.models import Q @@ -18,6 +19,7 @@ get_group_role_emails, get_child_group_role_emails, get_group_ad_emails, + get_group_email_aliases, GroupAliasGenerator, role_holder_emails, ) @@ -186,6 +188,66 @@ def test_generator_class(self): {k: (sorted(doms), sorted(addrs)) for k, (doms, addrs) in expected_dict.items()}, ) + @mock.patch("ietf.group.utils.GroupAliasGenerator") + def test_get_group_email_aliases(self, mock_alias_gen_cls): + GroupFactory(name="agroup", type_id="rg") + GroupFactory(name="bgroup") + GroupFactory(name="cgroup", type_id="rg") + GroupFactory(name="dgroup") + + mock_alias_gen_cls.return_value = [ + ("bgroup-chairs", ["ietf"], ["c1@example.com", "c2@example.com"]), + ("agroup-ads", ["ietf", "irtf"], ["ad@example.com"]), + ("bgroup-ads", ["ietf"], ["ad@example.com"]), + ] + # order is important - should be by acronym, otherwise left in order returned by generator + self.assertEqual( + get_group_email_aliases(None, None), + [ + { + "acronym": "agroup", + "alias_type": "-ads", + "expansion": "ad@example.com", + }, + { + "acronym": "bgroup", + "alias_type": "-chairs", + "expansion": "c1@example.com, c2@example.com", + }, + { + "acronym": "bgroup", + "alias_type": "-ads", + "expansion": "ad@example.com", + }, + ], + ) + self.assertQuerySetEqual( + mock_alias_gen_cls.call_args[0][0], + Group.objects.all(), + ordered=False, + ) + + # test other parameter combinations but we already checked that the alias generator's + # output will be passed through, so don't re-test the processing + get_group_email_aliases("agroup", None) + self.assertQuerySetEqual( + mock_alias_gen_cls.call_args[0][0], + Group.objects.filter(acronym="agroup"), + ordered=False, + ) + get_group_email_aliases(None, "wg") + self.assertQuerySetEqual( + mock_alias_gen_cls.call_args[0][0], + Group.objects.filter(type_id="wg"), + ordered=False, + ) + get_group_email_aliases("agroup", "wg") + self.assertQuerySetEqual( + mock_alias_gen_cls.call_args[0][0], + Group.objects.none(), + ordered=False, + ) + class GroupRoleEmailTests(TestCase):