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
This commit is contained in:
parent
dcf4251363
commit
8759955597
|
@ -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 <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
|
||||
|
|
|
@ -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 <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 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()
|
||||
|
||||
|
|
|
@ -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):
|
||||
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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,
|
||||
|
|
Loading…
Reference in a new issue