Add LeastRecentlyUsed reviewer queue policy.

 - Legacy-Id: 17049

Author: Sasha Romijn

ietf/review/policies.py

@@ -3,6 +3,7 @@
 from __future__ import absolute_import, print_function, unicode_literals
 
 import re
+from collections import OrderedDict
 
 import six
 
@@ -11,7 +12,8 @@
 from ietf.group.models import Role
 from ietf.person.models import Person
 import debug                            # pyflakes:ignore
-from ietf.review.models import NextReviewerInTeam, ReviewerSettings, ReviewWish, ReviewRequest
+from ietf.review.models import NextReviewerInTeam, ReviewerSettings, ReviewWish, ReviewRequest, \
+    ReviewAssignment
 from ietf.review.utils import (current_unavailable_periods_for_reviewers,
                                days_needed_to_fulfill_min_interval_for_reviewers, 
                                get_default_filter_re,
@@ -41,12 +43,58 @@ def default_reviewer_rotation_list(self, dont_skip_person_ids=None):
 
     def update_policy_state_for_assignment(self, assignee_person, add_skip=False):
         """
-        Update the internal state of a policy to reflect an assignment. 
+        Update the skip_count if the assignment was in order, and
+        update NextReviewerInTeam. Note that NextReviewerInTeam is
+        not used by all policies. 
         """
         settings = self._reviewer_settings_for(assignee_person)
         settings.request_assignment_next = False
         settings.save()
+        rotation_list = self.default_reviewer_rotation_list(
+            dont_skip_person_ids=[assignee_person.pk])
 
+        def reviewer_at_index(i):
+            if not rotation_list:
+                return None
+            return rotation_list[i % len(rotation_list)]
+
+        if not rotation_list:
+            return
+
+        rotation_list_without_skip = [r for r in rotation_list if
+                                      not self._reviewer_settings_for(r).skip_next]
+        # In order means: assigned to the first person in the rotation list with skip_next=0
+        # If the assignment is not in order, skip_next and NextReviewerInTeam are not modified.
+        in_order_assignment = rotation_list_without_skip[0] == assignee_person
+
+        # Loop through the list until finding the first person with skip_next=0,
+        # who is not the current assignee. Anyone with skip_next>0 encountered before
+        # has their skip_next decreased.
+        current_idx = 0
+        if in_order_assignment:
+            while True:
+                current_idx_person = reviewer_at_index(current_idx)
+                settings = self._reviewer_settings_for(current_idx_person)
+                if settings.skip_next > 0:
+                    settings.skip_next -= 1
+                    settings.save()
+                elif current_idx_person != assignee_person:
+                    # NextReviewerInTeam is not used by all policies to determine
+                    # default rotation order, but updated regardless.
+                    nr = NextReviewerInTeam.objects.filter(
+                        team=self.team).first() or NextReviewerInTeam(
+                        team=self.team)
+                    nr.next_reviewer = current_idx_person
+                    nr.save()
+
+                    break
+                current_idx += 1
+
+        if add_skip:
+            settings = self._reviewer_settings_for(assignee_person)
+            settings.skip_next += 1
+            settings.save()
+            
     # TODO : Change this field to deal with multiple already assigned reviewers???
     def setup_reviewer_field(self, field, review_req):
         """
@@ -298,50 +346,11 @@ def _reviewer_settings_for_person_ids(self, person_ids):
 
 
 class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy):
-
-    def update_policy_state_for_assignment(self, assignee_person, add_skip=False):
-        super(RotateAlphabeticallyReviewerQueuePolicy, self).update_policy_state_for_assignment(assignee_person, add_skip)
-        assert assignee_person is not None
-
-        rotation_list = self.default_reviewer_rotation_list(dont_skip_person_ids=[assignee_person.pk])
-
-        def reviewer_at_index(i):
-            if not rotation_list:
-                return None
-            return rotation_list[i % len(rotation_list)]
-
-        if not rotation_list:
-            return
-        
-        rotation_list_without_skip = [r for r in rotation_list if not self._reviewer_settings_for(r).skip_next]
-        # In order means: assigned to the first person in the rotation list with skip_next=0
-        # If the assignment is not in order, skip_next and NextReviewerInTeam are not modified.
-        in_order_assignment = rotation_list_without_skip[0] == assignee_person
-        
-        # Loop through the list until finding the first person with skip_next=0,
-        # who is not the current assignee. Anyone with skip_next>0 encountered before
-        # has their skip_next decreased.
-        current_idx = 0
-        if in_order_assignment:
-            while True:
-                current_idx_person = reviewer_at_index(current_idx)
-                settings = self._reviewer_settings_for(current_idx_person)
-                if settings.skip_next > 0:
-                    settings.skip_next -= 1
-                    settings.save()
-                elif current_idx_person != assignee_person:
-                    nr = NextReviewerInTeam.objects.filter(team=self.team).first() or NextReviewerInTeam(
-                        team=self.team)
-                    nr.next_reviewer = current_idx_person
-                    nr.save()
-    
-                    break
-                current_idx += 1
-            
-        if add_skip:
-            settings = self._reviewer_settings_for(assignee_person)
-            settings.skip_next += 1
-            settings.save()
+    """
+    A policy in which the default rotation list is based on last name, alphabetically.
+    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))
@@ -372,3 +381,28 @@ def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip_pe
 
         return rotation_list
 
+
+class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy):
+    """
+    A policy where the default rotation list is based on the most recent
+    assigned, accepted or completed review assignment.
+    """
+    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))
+        assignments = ReviewAssignment.objects.filter(
+            review_request__team=self.team,
+            state__in=['accepted', 'assigned', 'completed'],   
+        ).order_by('assigned_on')
+        
+        reviewers_with_assignment = [assignment.reviewer.person for assignment in assignments]
+        reviewers_without_assignment = set(reviewers) - set(reviewers_with_assignment)
+        
+        rotation_list = sorted(list(reviewers_without_assignment), key=lambda r: r.pk)
+        rotation_list += list(OrderedDict.fromkeys(reviewers_with_assignment))
+        
+        if not include_unavailable:
+            reviewers_to_skip = self._entirely_unavailable_reviewers(dont_skip_person_ids)
+            rotation_list = [p for p in rotation_list if p.pk not in reviewers_to_skip]
+
+        return rotation_list
+

ietf/review/test_policies.py

@@ -7,15 +7,20 @@
 from ietf.person.models import Email
 from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory
 from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, ReviewWish
-from ietf.review.policies import get_reviewer_queue_policy, AssignmentOrderResolver
+from ietf.review.policies import get_reviewer_queue_policy, AssignmentOrderResolver, \
+    LeastRecentlyUsedReviewerQueuePolicy, RotateAlphabeticallyReviewerQueuePolicy
 from ietf.utils.test_data import create_person
 from ietf.utils.test_utils import TestCase
 
 
 class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase):
+    """
+    These tests also cover the common behaviour in RotateAlphabeticallyReviewerQueuePolicy,
+    as that's difficult to test on it's own.
+    """
     def test_default_reviewer_rotation_list(self):
         team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
-        policy = get_reviewer_queue_policy(team)
+        policy = RotateAlphabeticallyReviewerQueuePolicy(team)
 
         reviewers = [
             create_person(team, "reviewer", name="Test Reviewer{}".format(i), username="testreviewer{}".format(i))
@@ -171,6 +176,64 @@ def get_skip_next(person):
         self.assertEqual(get_skip_next(reviewers[4]), 0)
 
 
+class LeastRecentlyUsedReviewerQueuePolicyTest(TestCase):
+    """
+    These tests only cover where this policy deviates from
+    RotateAlphabeticallyReviewerQueuePolicy - the common behaviour
+    inherited from AbstractReviewerQueuePolicy is covered in
+    RotateAlphabeticallyReviewerQueuePolicyTest.
+    """
+    def test_default_reviewer_rotation_list(self):
+        team = ReviewTeamFactory(acronym="rotationteam", name="Review Team",
+                                 list_email="rotationteam@ietf.org",
+                                 parent=Group.objects.get(acronym="farfut"))
+        policy = LeastRecentlyUsedReviewerQueuePolicy(team)
+
+        reviewers = [
+            create_person(team, "reviewer", name="Test Reviewer{}".format(i),
+                          username="testreviewer{}".format(i))
+            for i in range(5)
+        ]
+
+        # This reviewer should never be included.
+        unavailable_reviewer = create_person(team, "reviewer", name="unavailable reviewer",
+                                             username="unavailablereviewer")
+        UnavailablePeriod.objects.create(
+            team=team,
+            person=unavailable_reviewer,
+            start_date='2000-01-01',
+            availability='unavailable',
+        )
+        # This should not have any impact. Canfinish unavailable reviewers are included in
+        # the default rotation, and filtered further when making assignment choices.
+        UnavailablePeriod.objects.create(
+            team=team,
+            person=reviewers[1],
+            start_date='2000-01-01',
+            availability='canfinish',
+        )
+
+        # No known assignments
+        rotation = policy.default_reviewer_rotation_list()
+        self.assertNotIn(unavailable_reviewer, rotation)
+        self.assertEqual(rotation, reviewers)
+        
+        # Regular accepted assignment
+        ReviewAssignmentFactory(reviewer=reviewers[1].email(), assigned_on='2019-01-01',
+                                state_id='accepted', review_request__team=team)
+        # Rejected assignment, should not affect reviewer 2's position
+        ReviewAssignmentFactory(reviewer=reviewers[2].email(), state_id='rejected',
+                                review_request__team=team)
+        # Completed assignment, assigned before reviewer 1,
+        # but completed after (assign date should count).
+        ReviewAssignmentFactory(reviewer=reviewers[0].email(), assigned_on='2018-01-01',
+                                completed_on='2020-01-01', state_id='completed',
+                                review_request__team=team)
+        rotation = policy.default_reviewer_rotation_list()
+        self.assertNotIn(unavailable_reviewer, rotation)
+        self.assertEqual(rotation, [reviewers[2], reviewers[3], reviewers[4], reviewers[0], reviewers[1]])
+
+
 class AssignmentOrderResolverTests(TestCase):
     def test_determine_ranking(self):
         # reviewer_high is second in the default rotation, reviewer_low is first