From 8759955597d19fc636ff6279dfb8fe23cfebc5bd Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Wed, 24 Nov 2021 16:40:05 +0000 Subject: [PATCH] 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 #3376. Commit ready for merge. - Legacy-Id: 19710 --- ietf/group/utils.py | 48 +++++++++++++++++++++-- ietf/group/views.py | 93 ++++++++++++++++++++------------------------ ietf/nomcom/forms.py | 18 ++++++++- ietf/nomcom/tests.py | 59 ++++++++++++++++++++++++++-- ietf/nomcom/views.py | 17 ++++---- 5 files changed, 166 insertions(+), 69 deletions(-) diff --git a/ietf/group/utils.py b/ietf/group/utils.py index dbce474db..8d27507db 100644 --- a/ietf/group/utils.py +++ b/ietf/group/utils.py @@ -7,6 +7,7 @@ import os 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 @@ import debug # pyflakes:ignore 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 {}', attr, new) + else: + return format_html('{} changed to {} 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 diff --git a/ietf/group/views.py b/ietf/group/views.py index fcb3318ae..0cfd38259 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -77,9 +77,9 @@ from ietf.group.models import ( Group, Role, GroupEvent, GroupStateTransitions, 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 %(new)s from %(old)s" - if new_group: - entry = "%(attr)s changed to %(new)s" - - 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 edit(request, group_type=None, acronym=None, action="edit", field=None): 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 edit(request, group_type=None, acronym=None, action="edit", field=None): 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, 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() - 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)) if personnel_change_text!="": changed_personnel = [ str(p) for p in changed_personnel ] @@ -1030,7 +1013,15 @@ def edit(request, group_type=None, acronym=None, action="edit", field=None): 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() diff --git a/ietf/nomcom/forms.py b/ietf/nomcom/forms.py index 642ec5d5f..6e5d3fa45 100644 --- a/ietf/nomcom/forms.py +++ b/ietf/nomcom/forms.py @@ -86,9 +86,23 @@ class MultiplePositionNomineeField(forms.MultipleChoiceField, PositionNomineeFie 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): diff --git a/ietf/nomcom/tests.py b/ietf/nomcom/tests.py index bca655c51..a498a9e56 100644 --- a/ietf/nomcom/tests.py +++ b/ietf/nomcom/tests.py @@ -408,9 +408,14 @@ class NomcomViewsTest(TestCase): 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 @@ class NomcomViewsTest(TestCase): 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) diff --git a/ietf/nomcom/views.py b/ietf/nomcom/views.py index 5ddeec24d..f69d65910 100644 --- a/ietf/nomcom/views.py +++ b/ietf/nomcom/views.py @@ -22,7 +22,8 @@ from django.utils.encoding import force_bytes, force_text 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 @@ from ietf.nomcom.forms import (NominateForm, NominateNewPersonForm, FeedbackForm 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,