Fix ietf-tools#2420 - Add reviewer back to top of the queue after rejected/withdrawn reviews.

 - Legacy-Id: 17058

Author: Sasha Romijn

ietf/doc/views_review.py

@@ -363,6 +363,9 @@ def reject_reviewer_assignment(request, name, assignment_id):
                 state=review_assignment.state,
             )
 
+            policy = get_reviewer_queue_policy(review_assignment.review_request.team)
+            policy.return_reviewer_to_top_rotation(review_assignment.reviewer.person)
+
             msg = render_to_string("review/reviewer_assignment_rejected.txt", {
                 "by": request.user.person,
                 "message_to_secretary": form.cleaned_data.get("message_to_secretary")
@@ -409,6 +412,9 @@ def withdraw_reviewer_assignment(request, name, assignment_id):
             state=review_assignment.state,
         )            
 
+        policy = get_reviewer_queue_policy(review_assignment.review_request.team)
+        policy.return_reviewer_to_top_rotation(review_assignment.reviewer.person)
+        
         msg = "Review assignment withdrawn by %s"%request.user.person
 
         email_review_assignment_change(request, review_assignment, "Reviewer assignment withdrawn", msg, by=request.user.person, notify_secretary=True, notify_reviewer=True, notify_requested_by=False)

ietf/review/policies.py

@@ -50,7 +50,14 @@ def default_reviewer_rotation_list(self, dont_skip_person_ids=None):
         Return a list of reviewers (Person objects) in the default reviewer rotation for a policy.
         """
         raise NotImplementedError  # pragma: no cover
-    
+
+    def return_reviewer_to_top_rotation(self, reviewer_person):
+        """
+        Return a reviewer to the top of the rotation, e.g. because they rejected a review,
+        and should retroactively not have been rotated over.
+        """
+        raise NotImplementedError  # pragma: no cover
+
     def update_policy_state_for_assignment(self, assignee_person, add_skip=False):
         """
         Update the skip_count if the assignment was in order, and
@@ -363,7 +370,6 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy):
     NextReviewerInTeam is used to store a pointer to where the queue is currently
     positioned.
     """
-
     def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip_person_ids=None):
         reviewers = list(Person.objects.filter(role__name="reviewer", role__group=self.team))
         reviewers.sort(key=lambda p: p.last_name())
@@ -393,6 +399,14 @@ def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip_pe
 
         return rotation_list
 
+    def return_reviewer_to_top_rotation(self, reviewer_person):
+        # As RotateAlphabetically does not keep a full rotation list,
+        # returning someone to a particular order is complex.
+        # Instead, the "assign me next" flag is set.
+        settings = self._reviewer_settings_for(reviewer_person)
+        settings.request_assignment_next = True
+        settings.save()
+
 
 class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy):
     """
@@ -418,6 +432,12 @@ def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip_pe
 
         return rotation_list
 
+    def return_reviewer_to_top_rotation(self, reviewer_person):
+        # Reviewer rotation for this policy ignored rejected/withdrawn
+        # reviews, so it automatically adjusts the position of someone
+        # who rejected a review and no action is needed.
+        pass
+
 
 QUEUE_POLICY_NAME_MAPPING = {
     'RotateAlphabetically': RotateAlphabeticallyReviewerQueuePolicy,

ietf/review/test_policies.py

@@ -35,9 +35,9 @@ def test_invalid_policy_name(self):
             get_reviewer_queue_policy(team)
 
 
-class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase):
+class RotateAlphabeticallyReviewerAndGenericQueuePolicyTest(TestCase):
     """
-    These tests also cover the common behaviour in RotateAlphabeticallyReviewerQueuePolicy,
+    These tests also cover the common behaviour in AbstractReviewerQueuePolicy,
     as that's difficult to test on it's own.
     """
     def test_default_reviewer_rotation_list(self):
@@ -197,7 +197,16 @@ def get_skip_next(person):
         self.assertEqual(get_skip_next(reviewers[3]), 1)
         self.assertEqual(get_skip_next(reviewers[4]), 0)
 
-
+    def test_return_reviewer_to_top_rotation(self):
+        team = ReviewTeamFactory(acronym="rotationteam", name="Review Team",
+                                 list_email="rotationteam@ietf.org",
+                                 parent=Group.objects.get(acronym="farfut"))
+        reviewer = create_person(team, "reviewer", name="reviewer", username="reviewer")
+        policy = RotateAlphabeticallyReviewerQueuePolicy(team)
+        policy.return_reviewer_to_top_rotation(reviewer)
+        self.assertTrue(ReviewerSettings.objects.get(person=reviewer).request_assignment_next)
+        
+        
 class LeastRecentlyUsedReviewerQueuePolicyTest(TestCase):
     """
     These tests only cover where this policy deviates from
@@ -255,6 +264,14 @@ def test_default_reviewer_rotation_list(self):
         self.assertNotIn(unavailable_reviewer, rotation)
         self.assertEqual(rotation, [reviewers[2], reviewers[3], reviewers[4], reviewers[0], reviewers[1]])
 
+    def test_return_reviewer_to_top_rotation(self):
+        team = ReviewTeamFactory(acronym="rotationteam", name="Review Team",
+                                 list_email="rotationteam@ietf.org",
+                                 parent=Group.objects.get(acronym="farfut"))
+        reviewer = create_person(team, "reviewer", name="reviewer", username="reviewer")
+        policy = LeastRecentlyUsedReviewerQueuePolicy(team)
+        policy.return_reviewer_to_top_rotation(reviewer)
+
 
 class AssignmentOrderResolverTests(TestCase):
     def test_determine_ranking(self):