From a32d00af0a8712c862f790b90cb5ba8038423069 Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Thu, 24 Jun 2021 15:44:55 +0000 Subject: [PATCH] When removing reviewers from a review team, change the state of only that team's assignments. Check to make sure the person being removed doesn't have another reviewer Role (with a different Email) in the same team. Change the disposition of any assignments from that team for the removed reviewer to 'withdrawn' rather than 'rejected'. Fixes #3320. Commit ready for merge. - Legacy-Id: 19151 --- ietf/group/tests_info.py | 61 +++++++++++++++++++++++++++++----------- ietf/group/views.py | 27 ++++++++++-------- 2 files changed, 59 insertions(+), 29 deletions(-) diff --git a/ietf/group/tests_info.py b/ietf/group/tests_info.py index e89eb43ac..8470981cc 100644 --- a/ietf/group/tests_info.py +++ b/ietf/group/tests_info.py @@ -762,11 +762,20 @@ class GroupEditTests(TestCase): def test_edit_reviewers(self): group=GroupFactory(type_id='review',parent=GroupFactory(type_id='area')) + other_group=GroupFactory(type_id='review',parent=GroupFactory(type_id='area')) review_req = ReviewRequestFactory(team=group) - ad_email = Email.objects.get(address='ad2@example.org') + other_review_req = ReviewRequestFactory(team=other_group) - url = urlreverse('ietf.group.views.edit', kwargs=dict(group_type=group.type_id, acronym=group.acronym, action="edit")) - login_testing_unauthorized(self, "secretary", url) + # Set up a reviewer that has two email addresses + reviewer = PersonFactory() + EmailFactory(person=reviewer) + first_email = reviewer.email_set.first() + last_email = reviewer.email_set.last() + + RoleFactory(group=other_group, name_id='reviewer', person=reviewer, email=first_email) + + url = urlreverse('ietf.group.views.edit', kwargs=dict(group_type=group.type_id, acronym=group.acronym, action='edit')) + login_testing_unauthorized(self, 'secretary', url) # normal get r = self.client.get(url) @@ -780,33 +789,36 @@ class GroupEditTests(TestCase): name=group.name, acronym=group.acronym, parent=group.parent_id, - ad=Person.objects.get(name="Areaư Irector").pk, + ad=Person.objects.get(name='Areaư Irector').pk, state=group.state_id, list_email=group.list_email, list_subscribe=group.list_subscribe, list_archive=group.list_archive, - urls="" + urls='' ) - r = self.client.post(url, dict(post_data, reviewer_roles=ad_email.address)) + r = self.client.post(url, dict(post_data, reviewer_roles=first_email.address)) 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)), [ad_email.address]) + self.assertEqual(group.role_set.get(name='reviewer').email.address, first_email.address) self.assertTrue('Personnel change' in outbox[0]['Subject']) - # Assign a review to the reviewer, then remove the reviewer from the group - # As the request deadline has not passed, the assignment should be set to rejected - review_assignment = ReviewAssignmentFactory(review_request=review_req, state_id='assigned', reviewer=ad_email) + # Assign reviews to the reviewer, then remove the reviewer from the group + # As the request deadline has not passed, the assignment should be set to withdrawn + # Reviews assigned to other groups must not be affected + review_assignment = ReviewAssignmentFactory(review_request=review_req, state_id='assigned', reviewer=first_email) + other_review_assignment = ReviewAssignmentFactory(review_request=other_review_req, state_id='assigned', reviewer=first_email) r = self.client.post(url, post_data) self.assertEqual(r.status_code, 302) - group = reload_db_objects(group) - self.assertFalse(group.role_set.filter(name="reviewer")) - review_assignment = reload_db_objects(review_assignment) - self.assertEqual(review_assignment.state_id, 'rejected') + self.assertFalse(group.role_set.filter(name='reviewer').exists()) + self.assertEqual(other_group.role_set.get(name='reviewer').email.address, first_email.address) + + review_assignment, other_review_assignment = reload_db_objects(review_assignment, other_review_assignment) + self.assertEqual(review_assignment.state_id, 'withdrawn') + self.assertEqual(other_review_assignment.state_id, 'assigned') # Repeat after adding reviewer again, but now beyond request deadline - r = self.client.post(url, dict(post_data, reviewer_roles=ad_email.address)) + r = self.client.post(url, dict(post_data, reviewer_roles=first_email.address)) self.assertEqual(r.status_code, 302) review_assignment.state_id = 'accepted' review_assignment.save() @@ -816,8 +828,23 @@ class GroupEditTests(TestCase): r = self.client.post(url, post_data) self.assertEqual(r.status_code, 302) - review_assignment = reload_db_objects(review_assignment) + review_assignment, other_review_assignment = reload_db_objects(review_assignment, other_review_assignment) self.assertEqual(review_assignment.state_id, 'no-response') + self.assertEqual(other_review_assignment.state_id, 'assigned') + + # Configure group with two reviewer Roles for the same person with different email addresses + # then remove one of the roles. The result should be no change to the review assignments + group.role_set.filter(name_id='reviewer').delete() + for email in reviewer.email_set.all(): + group.role_set.create(name_id='reviewer', person=reviewer, email=email) + review_assignment.state_id = 'accepted' + review_assignment.save() + r = self.client.post(url, dict(post_data, reviewer_roles=last_email.address)) + self.assertEqual(group.role_set.get(name='reviewer').email.address, last_email.address) + review_assignment, other_review_assignment = reload_db_objects(review_assignment, other_review_assignment) + self.assertEqual(review_assignment.state_id, 'accepted') + self.assertEqual(other_review_assignment.state_id, 'assigned') + def test_conclude(self): group = GroupFactory(acronym="mars") diff --git a/ietf/group/views.py b/ietf/group/views.py index 0eae9f11c..51a0f3108 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -974,18 +974,21 @@ def edit(request, group_type=None, acronym=None, action="edit", field=None): today = datetime.date.today() for deleted_email in deleted: - active_assignments = ReviewAssignment.objects.filter( - reviewer__person=deleted_email.person, - state_id__in=['accepted', 'assigned'], - ) - for assignment in active_assignments: - if assignment.review_request.deadline > today: - assignment.state_id = 'rejected' - 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() + # 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))