Merged in [19710] from jennifer@painless-security.com:

    Allow nomcom chair to edit liaisons as well as members and generate GroupEvents when changed. Share code between group and nomcom for this purpose. Fixes ietf-tools#3376.
 - Legacy-Id: 19730
Note: SVN reference [19710] has been migrated to Git commit 8759955

Author: rjsparks

ietf/group/utils.py

@@ -7,6 +7,7 @@
 
 from django.db.models import Q
 from django.shortcuts import get_object_or_404
+from django.utils.html import format_html
 from django.utils.safestring import mark_safe
 from django.urls import reverse as urlreverse
 
@@ -15,9 +16,9 @@
 from ietf.community.models import CommunityList, SearchRule
 from ietf.community.utils import reset_name_contains_index_for_rule, can_manage_community_list
 from ietf.doc.models import Document, State
-from ietf.group.models import Group, RoleHistory, Role, GroupFeatures
+from ietf.group.models import Group, RoleHistory, Role, GroupFeatures, GroupEvent
 from ietf.ietfauth.utils import has_role
-from ietf.name.models import GroupTypeName
+from ietf.name.models import GroupTypeName, RoleName
 from ietf.person.models import Email
 from ietf.review.utils import can_manage_review_requests_for_team
 from ietf.utils import log
@@ -279,4 +280,45 @@ def group_features_role_filter(roles, person, feature):
         return roles.none()
     q = reduce(lambda a,b:a|b, [ Q(person=person, name__slug__in=getattr(t.features, feature)) for t in group_types ])
     return roles.filter(q)
-    
+
+
+def group_attribute_change_desc(attr, new, old=None):
+    if old is None:
+        return format_html('{} changed to <b>{}</b>', attr, new)
+    else:
+        return format_html('{} changed to <b>{}</b> from {}', attr, new, old)
+
+
+def update_role_set(group, role_name, new_value, by):
+    """Alter role_set for a group
+
+    Updates the value and creates history events.
+    """
+    if isinstance(role_name, str):
+        role_name = RoleName.objects.get(slug=role_name)
+    new = set(new_value)
+    old = set(r.email for r in group.role_set.filter(name=role_name).distinct().select_related("person"))
+    removed = old - new
+    added = new - old
+    if added or removed:
+        GroupEvent.objects.create(
+            group=group,
+            by=by,
+            type='info_changed',
+            desc=group_attribute_change_desc(
+                role_name.name,
+                ", ".join(sorted(x.get_name() for x in new)),
+                ", ".join(sorted(x.get_name() for x in old)),
+            )
+        )
+
+        group.role_set.filter(name=role_name, email__in=removed).delete()
+        for email in added:
+            group.role_set.create(name=role_name, email=email, person=email.person)
+
+        for e in new:
+            if not e.origin or (e.person.user and e.origin == e.person.user.username):
+                e.origin = "role: %s %s" % (group.acronym, role_name.slug)
+                e.save()
+
+    return added, removed

ietf/group/views.py

@@ -77,9 +77,9 @@
                               ChangeStateGroupEvent, GroupFeatures )
 from ietf.group.utils import (get_charter_text, can_manage_all_groups_of_type, 
                               milestone_reviewer_for_group_type, can_provide_status_update,
-                              can_manage_materials, 
+                              can_manage_materials, group_attribute_change_desc,
                               construct_group_menu_context, get_group_materials,
-                              save_group_in_history, can_manage_group, 
+                              save_group_in_history, can_manage_group, update_role_set,
                               get_group_or_404, setup_default_community_list_for_group, )                              
 #
 from ietf.ietfauth.utils import has_role, is_authorized_in_group
@@ -873,13 +873,6 @@ def group_photos(request, group_type=None, acronym=None):
 def edit(request, group_type=None, acronym=None, action="edit", field=None):
     """Edit or create a group, notifying parties as
     necessary and logging changes as group events."""
-    def desc(attr, new, old):
-        entry = "%(attr)s changed to <b>%(new)s</b> from %(old)s"
-        if new_group:
-            entry = "%(attr)s changed to <b>%(new)s</b>"
-
-        return entry % dict(attr=attr, new=new, old=old)
-
     def format_resources(resources, fs="\n"):
         res = []
         for r in resources:
@@ -896,7 +889,11 @@ def diff(attr, name):
             return
         v = getattr(group, attr)
         if clean[attr] != v:
-            changes.append((attr, clean[attr], desc(name, clean[attr], v)))
+            changes.append((
+                attr,
+                clean[attr],
+                group_attribute_change_desc(name, clean[attr], v if v else None)
+            ))
             setattr(group, attr, clean[attr])
 
     if action == "edit":
@@ -972,47 +969,33 @@ def diff(attr, name):
 
                 title = f.label
 
-                new = clean[attr]
-                old = Email.objects.filter(role__group=group, role__name=slug).select_related("person")
-                if set(new) != set(old):
-                    changes.append((attr, new, desc(title,
-                                        ", ".join(sorted(x.get_name() for x in new)),
-                                        ", ".join(sorted(x.get_name() for x in old)))))
-                    group.role_set.filter(name=slug).delete()
-                    for e in new:
-                        Role.objects.get_or_create(name_id=slug, email=e, group=group, person=e.person)
-                        if not e.origin or (e.person.user and e.origin == e.person.user.username):
-                            e.origin = "role: %s %s" % (group.acronym, slug)
-                            e.save()
-
-                    added = set(new) - set(old)
-                    deleted = set(old) - set(new)
-                    if added:
-                        change_text=title + ' added: ' + ", ".join(x.name_and_email() for x in added)
-                        personnel_change_text+=change_text+"\n"
-                    if deleted:
-                        change_text=title + ' deleted: ' + ", ".join(x.name_and_email() for x in deleted)
-                        personnel_change_text+=change_text+"\n"
-                        
-                        today = datetime.date.today()
-                        for deleted_email in deleted:
-                            # Verify the person doesn't have a separate reviewer role for the group with a different address
-                            if not group.role_set.filter(name_id='reviewer',person=deleted_email.person).exists():
-                                active_assignments = ReviewAssignment.objects.filter(
-                                    review_request__team=group,
-                                    reviewer__person=deleted_email.person,
-                                    state_id__in=['accepted', 'assigned'],
-                                )
-                                for assignment in active_assignments:
-                                    if assignment.review_request.deadline > today:
-                                        assignment.state_id = 'withdrawn'
-                                    else:
-                                        assignment.state_id = 'no-response'
-                                    # save() will update review_request state to 'requested'
-                                    # if needed, so that the review can be assigned to someone else
-                                    assignment.save()
-                                
-                    changed_personnel.update(set(old)^set(new))
+                added, deleted = update_role_set(group, slug, clean[attr], request.user.person)
+                changed_personnel.update(added | deleted)
+                if added:
+                    change_text=title + ' added: ' + ", ".join(x.name_and_email() for x in added)
+                    personnel_change_text+=change_text+"\n"
+                if deleted:
+                    change_text=title + ' deleted: ' + ", ".join(x.name_and_email() for x in deleted)
+                    personnel_change_text+=change_text+"\n"
+
+                    today = datetime.date.today()
+                    for deleted_email in deleted:
+                        # Verify the person doesn't have a separate reviewer role for the group with a different address
+                        if not group.role_set.filter(name_id='reviewer',person=deleted_email.person).exists():
+                            active_assignments = ReviewAssignment.objects.filter(
+                                review_request__team=group,
+                                reviewer__person=deleted_email.person,
+                                state_id__in=['accepted', 'assigned'],
+                            )
+                            for assignment in active_assignments:
+                                if assignment.review_request.deadline > today:
+                                    assignment.state_id = 'withdrawn'
+                                else:
+                                    assignment.state_id = 'no-response'
+                                # save() will update review_request state to 'requested'
+                                # if needed, so that the review can be assigned to someone else
+                                assignment.save()
+
 
             if personnel_change_text!="":
                 changed_personnel = [ str(p) for p in changed_personnel ]
@@ -1030,7 +1013,15 @@ def diff(attr, name):
                         value = parts[1]
                         display_name = ' '.join(parts[2:]).strip('()')
                         group.groupextresource_set.create(value=value, name_id=name, display_name=display_name)
-                    changes.append(('resources', new_resources, desc('Resources', ", ".join(new_resources), ", ".join(old_resources))))
+                    changes.append((
+                        'resources',
+                        new_resources,
+                        group_attribute_change_desc(
+                            'Resources',
+                            ", ".join(new_resources),
+                            ", ".join(old_resources) if old_resources else None
+                        )
+                    ))
 
             group.time = datetime.datetime.now()
 

ietf/nomcom/forms.py

@@ -86,9 +86,23 @@ def clean(self, value):
         return result
 
 
-class NewEditMembersForm(forms.Form):
+class EditMembersForm(forms.Form):
+    members = SearchableEmailsField(only_users=True, all_emails=True, required=False)
+    liaisons = SearchableEmailsField(only_users=True, all_emails=True, required=False)
+
+    def __init__(self, nomcom, *args, **kwargs):
+        initial = kwargs.setdefault('initial', {})
+        roles = nomcom.group.role_set.filter(
+            name__slug__in=('member', 'liaison')
+        ).order_by('email__person__name').select_related('email')
+        initial['members'] = [
+            r.email for r in roles if r.name.slug == 'member'
+        ]
+        initial['liaisons'] = [
+                r.email for r in roles if r.name.slug =='liaison'
+        ]
+        super().__init__(*args, **kwargs)
 
-    members = SearchableEmailsField(only_users=True,all_emails=True)
 
 class EditNomcomForm(forms.ModelForm):
 

ietf/nomcom/tests.py

@@ -408,9 +408,14 @@ def test_private_merge_view(self):
 
         self.client.logout()
 
-    def change_members(self, members):
-        members_emails = ','.join(['%s%s' % (member, EMAIL_DOMAIN) for member in members])
-        test_data = {'members': members_emails,}
+    def change_members(self, members=None, liaisons=None):
+        test_data = {}
+        if members is not None:
+            members_emails = ','.join(['%s%s' % (member, EMAIL_DOMAIN) for member in members])
+            test_data['members'] = members_emails
+        if liaisons is not None:
+            liaisons_emails = ','.join(['%s%s' % (liaison, EMAIL_DOMAIN) for liaison in liaisons])
+            test_data['liaisons'] = liaisons_emails
         self.client.post(self.edit_members_url, test_data)
 
     def test_edit_members_view(self):
@@ -432,6 +437,54 @@ def test_edit_members_view(self):
         self.check_url_status(self.private_index_url, 403)
         self.client.logout()
 
+    def test_edit_members_only_removes_member_roles(self):
+        """Removing a member or liaison should not affect other roles"""
+        # log in and set up members/liaisons lists
+        self.access_chair_url(self.edit_members_url)
+        self.change_members(
+            members=[CHAIR_USER, COMMUNITY_USER],
+            liaisons=[CHAIR_USER, COMMUNITY_USER],
+        )
+        nomcom_group = Group.objects.get(acronym=f'nomcom{self.year}')
+        self.assertCountEqual(
+            nomcom_group.role_set.filter(name='member').values_list('email__address', flat=True),
+            [CHAIR_USER + EMAIL_DOMAIN, COMMUNITY_USER + EMAIL_DOMAIN],
+        )
+        self.assertCountEqual(
+            nomcom_group.role_set.filter(name='liaison').values_list('email__address', flat=True),
+            [CHAIR_USER + EMAIL_DOMAIN, COMMUNITY_USER + EMAIL_DOMAIN],
+        )
+
+        # remove a member who is also a liaison and check that the liaisons list is unchanged
+        self.change_members(
+            members=[COMMUNITY_USER],
+            liaisons=[CHAIR_USER, COMMUNITY_USER],
+        )
+        nomcom_group = Group.objects.get(pk=nomcom_group.pk)  # refresh from db
+        self.assertCountEqual(
+            nomcom_group.role_set.filter(name='member').values_list('email__address', flat=True),
+            [COMMUNITY_USER + EMAIL_DOMAIN],
+        )
+        self.assertCountEqual(
+            nomcom_group.role_set.filter(name='liaison').values_list('email__address', flat=True),
+            [CHAIR_USER + EMAIL_DOMAIN, COMMUNITY_USER + EMAIL_DOMAIN],
+        )
+
+        # remove a liaison who is also a member and check that the members list is unchanged
+        self.change_members(
+            members=[COMMUNITY_USER],
+            liaisons=[CHAIR_USER],
+        )
+        nomcom_group = Group.objects.get(pk=nomcom_group.pk)  # refresh from db
+        self.assertCountEqual(
+            nomcom_group.role_set.filter(name='member').values_list('email__address', flat=True),
+            [COMMUNITY_USER + EMAIL_DOMAIN],
+        )
+        self.assertCountEqual(
+            nomcom_group.role_set.filter(name='liaison').values_list('email__address', flat=True),
+            [CHAIR_USER + EMAIL_DOMAIN],
+        )
+
     def test_edit_nomcom_view(self):
         r = self.access_chair_url(self.edit_nomcom_url)
         q = PyQuery(r.content)

ietf/nomcom/views.py

@@ -22,7 +22,8 @@
 from ietf.dbtemplate.models import DBTemplate
 from ietf.dbtemplate.views import group_template_edit, group_template_show
 from ietf.name.models import NomineePositionStateName, FeedbackTypeName
-from ietf.group.models import Group, GroupEvent, Role 
+from ietf.group.models import Group, GroupEvent, Role
+from ietf.group.utils import update_role_set
 from ietf.message.models import Message
 
 from ietf.nomcom.decorators import nomcom_private_key_required
@@ -31,7 +32,7 @@
                                PrivateKeyForm, EditNomcomForm, EditNomineeForm,
                                PendingFeedbackForm, ReminderDatesForm, FullFeedbackFormSet,
                                FeedbackEmailForm, NominationResponseCommentForm, TopicForm,
-                               NewEditMembersForm, VolunteerForm, )
+                               EditMembersForm, VolunteerForm, )
 from ietf.nomcom.models import (Position, NomineePosition, Nominee, Feedback, NomCom, ReminderDates,
                                 FeedbackLastSeen, Topic, TopicFeedbackLastSeen, )
 from ietf.nomcom.utils import (get_nomcom_by_year, store_nomcom_private_key, suggest_affiliation,
@@ -1230,18 +1231,14 @@ def edit_members(request, year):
     if nomcom.group.state_id=='conclude':
         permission_denied(request, 'This nomcom is closed.')
 
-    old_members_email = [r.email for r in nomcom.group.role_set.filter(name='member')]
-
     if request.method=='POST':
-        form = NewEditMembersForm(data=request.POST)
+        form = EditMembersForm(nomcom, data=request.POST)
         if form.is_valid():
-            new_members_email = form.cleaned_data['members']
-            nomcom.group.role_set.filter( email__in=set(old_members_email)-set(new_members_email) ).delete()
-            for email in set(new_members_email)-set(old_members_email):
-                nomcom.group.role_set.create(email=email,person=email.person,name_id='member')
+            update_role_set(nomcom.group, 'member', form.cleaned_data['members'], request.user.person)
+            update_role_set(nomcom.group, 'liaison', form.cleaned_data['liaisons'], request.user.person)
             return HttpResponseRedirect(reverse('ietf.nomcom.views.private_index',kwargs={'year':year}))
     else:
-        form = NewEditMembersForm(initial={ 'members' : old_members_email })
+        form = EditMembersForm(nomcom)
 
     return render(request, 'nomcom/new_edit_members.html',
                               {'nomcom' : nomcom,