Update update_policy_state_for_assignment for new policies, fix tests,

fix some other minor things.
 - Legacy-Id: 17023

Author: Sasha Romijn

ietf/doc/tests_review.py

@@ -239,6 +239,7 @@ def test_assign_reviewer(self):
         RoleFactory(group=review_team,person__user__username='marschairman',person__name='WG Cháir Man',name_id='reviewer')
         secretary = RoleFactory(group=review_team,person__user__username='reviewsecretary',person__user__email='reviewsecretary@example.com',name_id='secr')
         ReviewerSettings.objects.create(team=review_team, person=rev_role.person, min_interval=14, skip_next=0)
+        queue_policy = get_reviewer_queue_policy(review_team)
 
         # review to assign to
         review_req = ReviewRequestFactory(team=review_team,doc=doc,state_id='requested')
@@ -282,8 +283,6 @@ def test_assign_reviewer(self):
 
         ReviewWish.objects.create(person=reviewer_email.person, team=review_req.team, doc=doc)
 
-        # pick a non-existing reviewer as next to see that we can
-        # handle reviewers who have left
         NextReviewerInTeam.objects.filter(team=review_req.team).delete()
         NextReviewerInTeam.objects.create(
             team=review_req.team,
@@ -317,7 +316,7 @@ def test_assign_reviewer(self):
 
         # assign
         empty_outbox()
-        rotation_list = get_reviewer_queue_policy(review_req.team).default_reviewer_rotation_list()
+        rotation_list = queue_policy.default_reviewer_rotation_list()
         reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[0]).first()
         r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk })
         self.assertEqual(r.status_code, 302)
@@ -328,7 +327,6 @@ def test_assign_reviewer(self):
         assignment = review_req.reviewassignment_set.first()
         self.assertEqual(assignment.reviewer, reviewer)
         self.assertEqual(assignment.state_id, "assigned")
-        self.assertEqual(NextReviewerInTeam.objects.get(team=review_req.team).next_reviewer, rotation_list[1])
 
         self.assertEqual(len(outbox), 1)
         self.assertEqual('"Some Reviewer" <reviewer@example.com>', outbox[0]["To"])

ietf/review/policies.py

@@ -32,9 +32,9 @@ class AbstractReviewerQueuePolicy:
     def __init__(self, team):
         self.team = team
 
-    def default_reviewer_rotation_list(self, dont_skip=[]):
+    def default_reviewer_rotation_list(self, dont_skip_person_ids=None):
         """
-        Return a list of reviewers in the default reviewer rotation for a policy.
+        Return a list of reviewers (Person objects) in the default reviewer rotation for a policy.
         """
         raise NotImplementedError  # pragma: no cover
 
@@ -73,24 +73,29 @@ def recommended_assignment_order(self, email_queryset, review_req):
         resolver = AssignmentOrderResolver(email_queryset, review_req, self.default_reviewer_rotation_list())
         return [(r['email'].pk, r['label']) for r in resolver.determine_ranking()]
 
-    def _entirely_unavailable_reviewers(self, dont_skip):
-        # prune reviewers not in the rotation (but not the assigned
-        # reviewer who must have been available for assignment anyway)
+    def _entirely_unavailable_reviewers(self, dont_skip_person_ids=None):
+        """
+        Return a set of PKs of Persons that should not be considered
+        to be in the rotation list at all. 
+        """
         reviewers_to_skip = set()
+        if not dont_skip_person_ids:
+            dont_skip_person_ids = []
 
         unavailable_periods = current_unavailable_periods_for_reviewers(self.team)
         for person_id, periods in unavailable_periods.items():
-            if periods and person_id not in dont_skip:
+            if periods and person_id not in dont_skip_person_ids and not any(p.availability == "canfinish" for p in periods):
                 reviewers_to_skip.add(person_id)
 
-        days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(self.team)
-        for person_id, days_needed in days_needed_for_reviewers.items():
-            if person_id not in dont_skip:
-                reviewers_to_skip.add(person_id)
         return reviewers_to_skip
-
+                
 
 class AssignmentOrderResolver:
+    """
+    The AssignmentOrderResolver resolves the "recommended assignment order",
+    for a set of possible reviewers (email_queryset), a review request, and a
+    rotation list.
+    """
     def __init__(self, email_queryset, review_req, rotation_list):
         self.review_req = review_req
         self.doc = review_req.doc
@@ -285,51 +290,61 @@ def _reviewer_settings_for_person_ids(self, person_ids):
 
 class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy):
 
-    def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False):
-        assert assignee_person_id is not None
+    def update_policy_state_for_assignment(self, assignee_person, add_skip=False):
+        print('====================')
+        assert assignee_person is not None
 
-        rotation_list = [p.id for p in self.default_reviewer_rotation_list(
-                                                            dont_skip=[assignee_person_id])]
+        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)]
 
-        def reviewer_settings_for(person_id):
-            return (ReviewerSettings.objects.filter(team=self.team, person=person_id).first()
-                    or ReviewerSettings(team=self.team, person_id=person_id))
-
-        if add_skip:
-            settings = reviewer_settings_for(assignee_person_id)
-            settings.skip_next += 1
-            settings.save()
+        def reviewer_settings_for(person):
+            return (ReviewerSettings.objects.filter(team=self.team, person=person).first()
+                    or ReviewerSettings(team=self.team, person=person))
 
         if not rotation_list:
             return
 
+        rotation_list_without_skip = [r for r in rotation_list if not reviewer_settings_for(r).skip_next]
+        print('input: {} assigned'.format(assignee_person))
+        print('with skipped {}'.format([r for r in rotation_list]))
+        print('without skip {}'.format([r for r in rotation_list_without_skip]))
+        print('skip counts {}'.format([(r, reviewer_settings_for(r).skip_next) for r in rotation_list]))
+        in_order_assignment = rotation_list_without_skip[0] == assignee_person
+        print('in order: {}'.format(in_order_assignment))
+        
+        # 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 assignee_person_id == reviewer_at_index(current_idx):
-            # Skip the first reviewer in considering who is next.
-            current_idx += 1
-
-        while True:
-            current_reviewer_person_id = reviewer_at_index(current_idx)
-            settings = reviewer_settings_for(current_reviewer_person_id)
+        while in_order_assignment:
+            current_idx_person = reviewer_at_index(current_idx)
+            settings = reviewer_settings_for(current_idx_person)
+            print('evaluating {} with skip_next {}, assignee {}'.format(current_idx_person, settings.skip_next, assignee_person))
             if settings.skip_next > 0:
+                print('dropping skip_next')
                 settings.skip_next -= 1
                 settings.save()
-                current_idx += 1
-            else:
+            elif current_idx_person != assignee_person:
+                print('nr appointed')
                 nr = NextReviewerInTeam.objects.filter(team=self.team).first() or NextReviewerInTeam(
                     team=self.team)
-                nr.next_reviewer_id = current_reviewer_person_id
+                nr.next_reviewer = current_idx_person
                 nr.save()
 
                 break
+            current_idx += 1
+            
+        if add_skip:
+            print('raising skip count for assignee')
+            settings = reviewer_settings_for(assignee_person)
+            settings.skip_next += 1
+            settings.save()
 
-    def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip=[]):
+    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())
         next_reviewer_index = 0
@@ -353,7 +368,7 @@ def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip=[]
         rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index]
 
         if not include_unavailable:
-            reviewers_to_skip = self._entirely_unavailable_reviewers(dont_skip)
+            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

@@ -1,15 +1,12 @@
 # Copyright The IETF Trust 2016-2019, All Rights Reserved
 
-import datetime
-
 from ietf.doc.factories import WgDraftFactory
 from ietf.group.factories import ReviewTeamFactory
 from ietf.group.models import Group, Role
 from ietf.person.fields import PersonEmailChoiceField
 from ietf.person.models import Email
 from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory
-from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, \
-    ReviewRequest, ReviewWish
+from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, ReviewWish
 from ietf.review.policies import get_reviewer_queue_policy, AssignmentOrderResolver
 from ietf.utils.test_data import create_person
 from ietf.utils.test_utils import TestCase
@@ -99,7 +96,6 @@ def test_recommended_assignment_order(self):
     def test_update_policy_state_for_assignment(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)
-        doc = WgDraftFactory()
 
         # make a bunch of reviewers
         reviewers = [
@@ -109,81 +105,66 @@ def test_update_policy_state_for_assignment(self):
 
         self.assertEqual(reviewers, policy.default_reviewer_rotation_list())
 
+        def reviewer_settings_for(person):
+            return (ReviewerSettings.objects.filter(team=team, person=person).first()
+                    or ReviewerSettings(team=team, person=person))
+
         def get_skip_next(person):
-            settings = (ReviewerSettings.objects.filter(team=team, person=person).first()
-                        or ReviewerSettings(team=team))
-            return settings.skip_next
+            return reviewer_settings_for(person).skip_next
 
-        policy.update_policy_state_for_assignment(assignee_person_id=reviewers[0].pk, add_skip=False)
+        # Regular in-order assignment without skips
+        policy.update_policy_state_for_assignment(assignee_person=reviewers[0], add_skip=False)
         self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[1])
         self.assertEqual(get_skip_next(reviewers[0]), 0)
         self.assertEqual(get_skip_next(reviewers[1]), 0)
-
-        policy.update_policy_state_for_assignment(assignee_person_id=reviewers[1].pk, add_skip=True)
-        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
-        self.assertEqual(get_skip_next(reviewers[1]), 1)
-        self.assertEqual(get_skip_next(reviewers[2]), 0)
-
-        # skip reviewer 2
-        policy.update_policy_state_for_assignment(assignee_person_id=reviewers[3].pk, add_skip=True)
-        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
-        self.assertEqual(get_skip_next(reviewers[0]), 0)
-        self.assertEqual(get_skip_next(reviewers[1]), 1)
-        self.assertEqual(get_skip_next(reviewers[2]), 0)
-        self.assertEqual(get_skip_next(reviewers[3]), 1)
-
-        # pick reviewer 2, use up reviewer 3's skip_next
-        policy.update_policy_state_for_assignment(assignee_person_id=reviewers[2].pk, add_skip=False)
-        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4])
-        self.assertEqual(get_skip_next(reviewers[0]), 0)
-        self.assertEqual(get_skip_next(reviewers[1]), 1)
-        self.assertEqual(get_skip_next(reviewers[2]), 0)
-        self.assertEqual(get_skip_next(reviewers[3]), 0)
-        self.assertEqual(get_skip_next(reviewers[4]), 0)
-
-        # check wrap-around
-        policy.update_policy_state_for_assignment(assignee_person_id=reviewers[4].pk)
-        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[0])
-        self.assertEqual(get_skip_next(reviewers[0]), 0)
-        self.assertEqual(get_skip_next(reviewers[1]), 1)
         self.assertEqual(get_skip_next(reviewers[2]), 0)
         self.assertEqual(get_skip_next(reviewers[3]), 0)
         self.assertEqual(get_skip_next(reviewers[4]), 0)
 
-        # unavailable
-        today = datetime.date.today()
-        UnavailablePeriod.objects.create(team=team, person=reviewers[1], start_date=today, end_date=today, availability="unavailable")
-        policy.update_policy_state_for_assignment(assignee_person_id=reviewers[0].pk)
+        # In-order assignment with add_skip
+        policy.update_policy_state_for_assignment(assignee_person=reviewers[1], add_skip=True)
         self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
         self.assertEqual(get_skip_next(reviewers[0]), 0)
-        self.assertEqual(get_skip_next(reviewers[1]), 1) # don't consume that skip while the reviewer is unavailable
+        self.assertEqual(get_skip_next(reviewers[1]), 1)  # from current add_skip=True
         self.assertEqual(get_skip_next(reviewers[2]), 0)
         self.assertEqual(get_skip_next(reviewers[3]), 0)
         self.assertEqual(get_skip_next(reviewers[4]), 0)
 
-        # pick unavailable anyway
-        policy.update_policy_state_for_assignment(assignee_person_id=reviewers[1].pk, add_skip=False)
-        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
+        # In-order assignment to 2, but 3 has a skip_next, so 4 should be assigned.
+        # 3 has skip_next decreased as it is skipped over, 1 retains its skip_next
+        reviewer3_settings = reviewer_settings_for(reviewers[3])
+        reviewer3_settings.skip_next = 2
+        reviewer3_settings.save()
+        policy.update_policy_state_for_assignment(assignee_person=reviewers[2], add_skip=False)
+        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4])
         self.assertEqual(get_skip_next(reviewers[0]), 0)
-        self.assertEqual(get_skip_next(reviewers[1]), 1)
+        self.assertEqual(get_skip_next(reviewers[1]), 1)  # from previous add_skip=true
         self.assertEqual(get_skip_next(reviewers[2]), 0)
-        self.assertEqual(get_skip_next(reviewers[3]), 0)
+        self.assertEqual(get_skip_next(reviewers[3]), 1)  # from manually set skip_next - 1
         self.assertEqual(get_skip_next(reviewers[4]), 0)
 
-        # not through min_interval so advance past reviewer[2]
-        settings, _ = ReviewerSettings.objects.get_or_create(team=team, person=reviewers[2])
-        settings.min_interval = 30
-        settings.save()
-        req = ReviewRequest.objects.create(team=team, doc=doc, type_id="early", state_id="assigned", deadline=today, requested_by=reviewers[0])
-        ReviewAssignmentFactory(review_request=req, state_id="accepted", reviewer = reviewers[2].email_set.first(),assigned_on = req.time)
-        policy.update_policy_state_for_assignment(assignee_person_id=reviewers[3].pk)
+        # Out of order assignments, nothing should change,
+        # except the add_skip=True should still apply
+        policy.update_policy_state_for_assignment(assignee_person=reviewers[3], add_skip=False)
+        policy.update_policy_state_for_assignment(assignee_person=reviewers[2], add_skip=False)
+        policy.update_policy_state_for_assignment(assignee_person=reviewers[1], add_skip=False)
+        policy.update_policy_state_for_assignment(assignee_person=reviewers[0], add_skip=True)
         self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4])
-        self.assertEqual(get_skip_next(reviewers[0]), 0)
+        self.assertEqual(get_skip_next(reviewers[0]), 1)  # from current add_skip=True
         self.assertEqual(get_skip_next(reviewers[1]), 1)
         self.assertEqual(get_skip_next(reviewers[2]), 0)
-        self.assertEqual(get_skip_next(reviewers[3]), 0)
+        self.assertEqual(get_skip_next(reviewers[3]), 1)
         self.assertEqual(get_skip_next(reviewers[4]), 0)
 
+        # Regular assignment, testing wrap-around
+        policy.update_policy_state_for_assignment(assignee_person=reviewers[4], add_skip=False)
+        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
+        self.assertEqual(get_skip_next(reviewers[0]), 0)  # skipped over with this assignment
+        self.assertEqual(get_skip_next(reviewers[1]), 0)  # skipped over with this assignment
+        self.assertEqual(get_skip_next(reviewers[2]), 0)
+        self.assertEqual(get_skip_next(reviewers[3]), 1)
+        self.assertEqual(get_skip_next(reviewers[4]), 0)
+
 
 class AssignmentOrderResolverTests(TestCase):
     def test_determine_ranking(self):

ietf/review/utils.py

@@ -381,7 +381,7 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa
     assignment = review_req.reviewassignment_set.create(state_id='assigned', reviewer = reviewer, assigned_on = datetime.datetime.now())
 
     from ietf.review.policies import get_reviewer_queue_policy
-    get_reviewer_queue_policy(review_req.team).update_policy_state_for_assignment(reviewer.person_id, add_skip)
+    get_reviewer_queue_policy(review_req.team).update_policy_state_for_assignment(reviewer.person, add_skip)
 
     ReviewRequestDocEvent.objects.create(
         type="assigned_review_request",