Skip to content

Commit

Permalink
refactor: don't use filesystem for group aliases (#7556)
Browse files Browse the repository at this point in the history 
* 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 <rjsparks@nostrum.com>
  • Loading branch information
@jennifer-richards @rjsparks
jennifer-richards and rjsparks committed Jun 18, 2024
1 parent 6338f45 commit 0ac2ae1
Show file tree
Hide file tree
Showing 7 changed files with 129 additions and 118 deletions.
28 changes: 0 additions & 28 deletions ietf/checks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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):
#
Expand Down
84 changes: 62 additions & 22 deletions ietf/group/tests.py
View file
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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,
)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"], ["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):

Expand Down
63 changes: 38 additions & 25 deletions ietf/group/tests_info.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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')
Expand Down
30 changes: 27 additions & 3 deletions ietf/group/utils.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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
Expand All @@ -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
Expand All @@ -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:
Expand All @@ -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 = [
Expand Down
35 changes: 2 additions & 33 deletions ietf/group/views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,7 @@
import copy
import datetime
import itertools
import io
import math
import re
import json
import types

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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)

Expand Down
Loading

0 comments on commit 0ac2ae1

Please sign in to comment.