Refactor role handling in group editing slightly and add support for

editing reviewer roles in review teams. Also fix a couple of review
related bugs.
 - Legacy-Id: 11921

Author: OleLaursen

ietf/group/features.py

@@ -11,6 +11,7 @@ class GroupFeatures(object):
     about_page = "group_about"
     default_tab = about_page
     material_types = ["slides"]
+    admin_roles = ["chair"]
 
     def __init__(self, group):
         if group.type_id in ("wg", "rg"):
@@ -31,3 +32,6 @@ def __init__(self, group):
             self.has_reviews = True
             import ietf.group.views
             self.default_tab = ietf.group.views.review_requests
+
+        if group.type_id == "dir":
+            self.admin_roles = ["chair", "secr"]

ietf/group/milestones.py

@@ -94,7 +94,7 @@ def edit_milestones(request, acronym, group_type=None, milestone_set="current"):
 
     needs_review = False
     if not can_manage_group(request.user, group):
-        if group.has_role(request.user, "chair"):
+        if group.has_role(request.user, group.features.admin_roles):
             if milestone_set == "current":
                 needs_review = True
         else:

ietf/group/tests_info.py

@@ -26,7 +26,7 @@
 from ietf.utils.test_utils import TestCase, unicontent
 from ietf.utils.mail import outbox, empty_outbox
 from ietf.utils.test_data import make_test_data, create_person, make_review_data
-from ietf.utils.test_utils import login_testing_unauthorized
+from ietf.utils.test_utils import login_testing_unauthorized, reload_db_objects
 from ietf.group.factories import GroupFactory, RoleFactory, GroupEventFactory
 from ietf.meeting.factories import SessionFactory
 import ietf.group.views
@@ -569,10 +569,10 @@ def test_edit_info(self):
                                   parent=area.pk,
                                   ad=ad.pk,
                                   state=state.pk,
-                                  chairs="aread@ietf.org, ad1@ietf.org",
-                                  secretaries="aread@ietf.org, ad1@ietf.org, ad2@ietf.org",
-                                  techadv="aread@ietf.org",
-                                  delegates="ad2@ietf.org",
+                                  chair_roles="aread@ietf.org, ad1@ietf.org",
+                                  secr_roles="aread@ietf.org, ad1@ietf.org, ad2@ietf.org",
+                                  techadv_roles="aread@ietf.org",
+                                  delegate_roles="ad2@ietf.org",
                                   list_email="mars@mail",
                                   list_subscribe="subscribe.mars",
                                   list_archive="archive.mars",
@@ -598,6 +598,40 @@ def test_edit_info(self):
         for prefix in ['ad1','ad2','aread','marschairman','marsdelegate']:
             self.assertTrue(prefix+'@' in outbox[0]['To'])
 
+    def test_edit_reviewers(self):
+        doc = make_test_data()
+        review_req = make_review_data(doc)
+        group = review_req.team
+
+        url = urlreverse('group_edit', kwargs=dict(group_type=group.type_id, acronym=group.acronym))
+        login_testing_unauthorized(self, "secretary", url)
+
+        # normal get
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 200)
+        q = PyQuery(r.content)
+        self.assertEqual(len(q('form input[name=reviewer_roles]')), 1)
+
+        # set reviewers
+        empty_outbox()
+        r = self.client.post(url,
+                             dict(name=group.name,
+                                  acronym=group.acronym,
+                                  parent=group.parent_id,
+                                  ad=Person.objects.get(name="Areað Irector").pk,
+                                  state=group.state_id,
+                                  reviewer_roles="ad2@ietf.org",
+                                  list_email=group.list_email,
+                                  list_subscribe=group.list_subscribe,
+                                  list_archive=group.list_archive,
+                                  urls=""
+                                  ))
+        self.assertEqual(r.status_code, 302)
+
+        group = reload_db_objects(group)
+        self.assertEqual(list(group.role_set.filter(name="reviewer").values_list("email", flat=True)), ["ad2@ietf.org"])
+        self.assertTrue('Personnel change' in outbox[0]['Subject'])
+
     def test_initial_charter(self):
         make_test_data()
         group = Group.objects.get(acronym="mars")

ietf/group/views.py

@@ -38,7 +38,6 @@
 from tempfile import mkstemp
 import datetime
 from collections import OrderedDict
-import math
 
 import debug              # pyflakes:ignore
 
@@ -364,11 +363,11 @@ def construct_group_menu_context(request, group, selected, group_type, others):
     # actions
     actions = []
 
-    is_chair = group.has_role(request.user, "chair")
+    is_admin = group.has_role(request.user, group.features.admin_roles)
     can_manage = can_manage_group(request.user, group)
 
     if group.features.has_milestones:
-        if group.state_id != "proposed" and (is_chair or can_manage):
+        if group.state_id != "proposed" and (is_admin or can_manage):
             actions.append((u"Edit milestones", urlreverse("group_edit_milestones", kwargs=kwargs)))
 
     if group.features.has_documents:
@@ -384,10 +383,10 @@ def construct_group_menu_context(request, group, selected, group_type, others):
         import ietf.group.views_review
         actions.append((u"Manage review requests", urlreverse(ietf.group.views_review.manage_review_requests, kwargs=kwargs)))
 
-    if group.state_id != "conclude" and (is_chair or can_manage):
+    if group.state_id != "conclude" and (is_admin or can_manage):
         actions.append((u"Edit group", urlreverse("group_edit", kwargs=kwargs)))
 
-    if group.features.customize_workflow and (is_chair or can_manage):
+    if group.features.customize_workflow and (is_admin or can_manage):
         actions.append((u"Customize workflow", urlreverse("ietf.group.views_edit.customize_workflow", kwargs=kwargs)))
 
     if group.state_id in ("active", "dormant") and not group.type_id in ["sdo", "rfcedtyp", "isoc", ] and can_manage:

ietf/group/views_edit.py

@@ -23,19 +23,31 @@
 from ietf.person.models import Person, Email
 from ietf.group.mails import ( email_admin_re_charter, email_personnel_change)
 from ietf.utils.ordereddict import insert_after_in_ordered_dict
+from ietf.utils.text import skip_suffix
+
 
 MAX_GROUP_DELEGATES = 3
 
+def roles_for_group_type(group_type):
+    roles = ["chair", "secr", "techadv", "delegate"]
+    if group_type == "dir":
+        roles.append("reviewer")
+    return roles
+
 class GroupForm(forms.Form):
     name = forms.CharField(max_length=255, label="Name", required=True)
     acronym = forms.CharField(max_length=10, label="Acronym", required=True)
     state = forms.ModelChoiceField(GroupStateName.objects.all(), label="State", required=True)
-    chairs = SearchableEmailsField(label="Chairs", required=False, only_users=True)
-    secretaries = SearchableEmailsField(label="Secretaries", required=False, only_users=True)
-    techadv = SearchableEmailsField(label="Technical Advisors", required=False, only_users=True)
-    delegates = SearchableEmailsField(label="Delegates", required=False, only_users=True, max_entries=MAX_GROUP_DELEGATES,
+
+    # roles
+    chair_roles = SearchableEmailsField(label="Chairs", required=False, only_users=True)
+    secr_roles = SearchableEmailsField(label="Secretaries", required=False, only_users=True)
+    techadv_roles = SearchableEmailsField(label="Technical Advisors", required=False, only_users=True)
+    delegate_roles = SearchableEmailsField(label="Delegates", required=False, only_users=True, max_entries=MAX_GROUP_DELEGATES,
                                       help_text=mark_safe("Chairs can delegate the authority to update the state of group documents - at most %s persons at a given time." % MAX_GROUP_DELEGATES))
+    reviewer_roles = SearchableEmailsField(label="Reviewers", required=False, only_users=True)
     ad = forms.ModelChoiceField(Person.objects.filter(role__name="ad", role__group__state="active", role__group__type='area').order_by('name'), label="Shepherding AD", empty_label="(None)", required=False)
+
     parent = forms.ModelChoiceField(Group.objects.filter(state="active").order_by('name'), empty_label="(None)", required=False)
     list_email = forms.CharField(max_length=64, required=False)
     list_subscribe = forms.CharField(max_length=255, required=False)
@@ -69,6 +81,11 @@ def __init__(self, *args, **kwargs):
             self.fields['parent'].queryset = self.fields['parent'].queryset.filter(type="area")
             self.fields['parent'].label = "IETF Area"
 
+        role_fields_to_remove = (set(roles_for_group_type(self.group_type))
+                                 - set(skip_suffix(attr, "_roles") for attr in self.fields if attr.endswith("_roles")))
+        for r in role_fields_to_remove:
+            del self.fields[r + "_roles"]
+
     def clean_acronym(self):
         # Changing the acronym of an already existing group will cause 404s all
         # over the place, loose history, and generally muck up a lot of
@@ -211,7 +228,8 @@ def edit(request, group_type=None, acronym=None, action="edit"):
         group = get_group_or_404(acronym, group_type)
         if not group_type and group:
             group_type = group.type_id
-        if not (can_manage_group(request.user, group) or group.has_role(request.user, "chair")):
+        if not (can_manage_group(request.user, group)
+                or group.has_role(request.user, group.features.admin_roles)):
             return HttpResponseForbidden("You don't have permission to access this view")
 
     if request.method == 'POST':
@@ -274,10 +292,18 @@ def diff(attr, name):
             personnel_change_text=""
             changed_personnel = set()
             # update roles
-            for attr, slug, title in [('ad','ad','Shepherding AD'), ('chairs', 'chair', "Chairs"), ('secretaries', 'secr', "Secretaries"), ('techadv', 'techadv', "Tech Advisors"), ('delegates', 'delegate', "Delegates")]:
+            for attr, f in form.fields.iteritems():
+                if not (attr.endswith("_roles") or attr == "ad"):
+                    continue
+
+                slug = attr
+                slug = skip_suffix(slug, "_roles")
+
+                title = f.label
+
                 new = clean[attr]
                 if attr == 'ad':
-                    new = [ new.role_email('ad'),] if new else []
+                    new = [ new.role_email('ad') ] if new else []
                 old = Email.objects.filter(role__group=group, role__name=slug).select_related("person")
                 if set(new) != set(old):
                     changes.append((attr, new, desc(title,
@@ -336,17 +362,16 @@ def diff(attr, name):
             init = dict(name=group.name,
                         acronym=group.acronym,
                         state=group.state,
-                        chairs=Email.objects.filter(role__group=group, role__name="chair"),
-                        secretaries=Email.objects.filter(role__group=group, role__name="secr"),
-                        techadv=Email.objects.filter(role__group=group, role__name="techadv"),
-                        delegates=Email.objects.filter(role__group=group, role__name="delegate"),
                         ad=ad_role and ad_role.person and ad_role.person.id,
                         parent=group.parent.id if group.parent else None,
                         list_email=group.list_email if group.list_email else None,
                         list_subscribe=group.list_subscribe if group.list_subscribe else None,
                         list_archive=group.list_archive if group.list_archive else None,
                         urls=format_urls(group.groupurl_set.all()),
                         )
+
+            for slug in roles_for_group_type(group_type):
+                init[slug + "_roles"] = Email.objects.filter(role__group=group, role__name=slug)
         else:
             init = dict(ad=request.user.person.id if group_type == "wg" and has_role(request.user, "Area Director") else None,
                         )
@@ -400,8 +425,8 @@ def customize_workflow(request, group_type=None, acronym=None):
     if not group.features.customize_workflow:
         raise Http404
 
-    if (not has_role(request.user, "Secretariat") and
-        not group.role_set.filter(name="chair", person__user=request.user)):
+    if not (can_manage_group(request.user, group)
+            or group.has_role(request.user, group.features.admin_roles)):
         return HttpResponseForbidden("You don't have permission to access this view")
 
     if group_type == "rg":

ietf/name/fixtures/names.json

@@ -1827,7 +1827,7 @@
  "fields": {
   "order": 7,
   "used": true,
-  "name": "No Review of Version",
+  "name": "Team Will not Review Version",
   "desc": ""
  },
  "model": "name.reviewrequeststatename",
@@ -1837,7 +1837,7 @@
  "fields": {
   "order": 8,
   "used": true,
-  "name": "No Review of Document",
+  "name": "Team Will not Review Document",
   "desc": ""
  },
  "model": "name.reviewrequeststatename",

ietf/utils/test_data.py

@@ -360,7 +360,7 @@ def other_doc_factory(type_id,name):
     return draft
 
 def make_review_data(doc):
-    team = Group.objects.create(state_id="active", acronym="reviewteam", name="Review Team", type_id="team", list_email="reviewteam@ietf.org")
+    team = Group.objects.create(state_id="active", acronym="reviewteam", name="Review Team", type_id="dir", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
     for r in ReviewResultName.objects.filter(slug__in=["issues", "ready-issues", "ready", "not-ready"]):
         ReviewTeamResult.objects.create(team=team, result=r)
 
@@ -383,7 +383,7 @@ def make_review_data(doc):
     Role.objects.create(name_id="reviewer", person=p, email=p.email_set.first(), group=team)
 
     p = Person.objects.get(user__username="secretary")
-    Role.objects.create(name_id="secretary", person=p, email=p.email_set.first(), group=team)
+    Role.objects.create(name_id="secr", person=p, email=p.email_set.first(), group=team)
 
     return review_req
 

ietf/utils/text.py

@@ -4,8 +4,8 @@ def skip_prefix(text, prefix):
     else:
         return text
 
-def skip_suffix(text, prefix):
-    if text.endswith(prefix):
-        return text[:-len(prefix)]
+def skip_suffix(text, suffix):
+    if text.endswith(suffix):
+        return text[:-len(suffix)]
     else:
         return text