refactor: don't use filesystem for group aliases (#7556)

* refactor: generate group aliases on the fly * chore: remove group alias file check * chore: drop group alias settings, fix lint * refactor: rename var to hint it's ignored * test: update tests * refactor: move utility to utils * test: add test --------- Co-authored-by: Robert Sparks <[email protected]>
ietf-tools · Jun 18, 2024 · 0ac2ae1 · 0ac2ae1
1 parent 6338f45
commit 0ac2ae1
Show file tree

Hide file tree

Showing 7 changed files with 129 additions and 118 deletions.
diff --git a/ietf/checks.py b/ietf/checks.py
@@ -28,34 +28,6 @@ def already_ran():
         checks_run.append(name)
         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('directories')
 def check_id_submission_directories(app_configs, **kwargs):
     #

diff --git a/ietf/group/tests.py b/ietf/group/tests.py
@@ -1,13 +1,10 @@
 # Copyright The IETF Trust 2013-2020, All Rights Reserved
 # -*- coding: utf-8 -*-
 
-import os
 import datetime
 import json
+import mock
 
-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
@@ -22,6 +19,7 @@
     get_group_role_emails,
     get_child_group_role_emails,
     get_group_ad_emails,
+    get_group_email_aliases,
     GroupAliasGenerator,
     role_holder_emails,
 )
@@ -132,24 +130,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
@@ -208,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"], ["[email protected]", "[email protected]"]),
+            ("agroup-ads", ["ietf", "irtf"], ["[email protected]"]),
+            ("bgroup-ads", ["ietf"], ["[email protected]"]),
+        ]
+        # 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": "[email protected]",
+                },
+                {
+                    "acronym": "bgroup",
+                    "alias_type": "-chairs",
+                    "expansion": "[email protected], [email protected]",
+                },
+                {
+                    "acronym": "bgroup",
+                    "alias_type": "-ads",
+                    "expansion": "[email protected]",
+                },
+            ],
+        )
+        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):
 

diff --git 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
-[email protected]                                                xfilter-mars-ads
-[email protected]                                 [email protected]
-[email protected]                                             xfilter-mars-chairs
-[email protected]                              [email protected]
-[email protected]                                                xfilter-mars-ads
-[email protected]                                 [email protected]
-[email protected]                                             xfilter-mars-chairs
-[email protected]                              [email protected]
-""")
-        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": "[email protected]"},
+            {"acronym": "mars", "alias_type": "-chairs", "expansion": "[email protected]"},
+        ]
         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": "[email protected]"},
+            {"acronym": "mars", "alias_type": "-chairs", "expansion": "[email protected]"},
+            {"acronym": "ames", "alias_type": "-ads", "expansion": "[email protected]"},
+            {"acronym": "ames", "alias_type": "-chairs", "expansion": "[email protected]"},
+        ]
         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": "[email protected]"},
+            {"acronym": "mars", "alias_type": "-chairs", "expansion": "[email protected]"},
+            {"acronym": "ames", "alias_type": "-ads", "expansion": "[email protected]"},
+            {"acronym": "ames", "alias_type": "-chairs", "expansion": "[email protected]"},
+        ]
         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": "[email protected]"},
+            {"acronym": "mars", "alias_type": "-chairs", "expansion": "[email protected]"},
+            {"acronym": "ames", "alias_type": "-ads", "expansion": "[email protected]"},
+            {"acronym": "ames", "alias_type": "-chairs", "expansion": "[email protected]"},
+        ]
         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, '[email protected]')
         self.assertContains(r, 'group_personnel_change')

diff --git 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:
@@ -427,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
@@ -37,9 +37,7 @@
 import copy
 import datetime
 import itertools
-import io
 import math
-import re
 import json
 import types
 
@@ -81,7 +79,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,
+                              get_group_email_aliases)                              
 #
 from ietf.ietfauth.utils import has_role, is_authorized_in_group
 from ietf.mailtrigger.utils import gather_relevant_expansions
@@ -137,21 +136,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:
@@ -580,21 +564,6 @@ 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
-
 def email(request, acronym, group_type=None):
     group = get_group_or_404(acronym, group_type)