Account for 'canfinish' unavailabilities.

 - Legacy-Id: 16999

Author: Sasha Romijn

ietf/review/policies.py

@@ -73,7 +73,7 @@ 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 _unavailable_reviewers(self, dont_skip):
+    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)
         reviewers_to_skip = set()
@@ -119,7 +119,8 @@ def _collect_context(self):
         self.connections = self._connections_with_doc(self.doc, self.possible_person_ids)
         self.unavailable_periods = current_unavailable_periods_for_reviewers(self.team)
         self.assignment_data_for_reviewers = latest_review_assignments_for_reviewers(self.team)
-
+        self.unavailable_periods = current_unavailable_periods_for_reviewers(self.team)
+        
     def determine_ranking(self):
         """
         Determine the ranking of reviewers.
@@ -139,7 +140,7 @@ def _ranking_for_email(self, email):
         Determine the ranking for a specific Email.
         Returns a dict with an email object, the scores and an explanation label.
         The scores are a list of individual scores, i.e. they are prioritised, not
-        cumulative. 
+        cumulative.
         """
         settings = self.reviewer_settings.get(email.person_id)
         scores = []
@@ -152,7 +153,25 @@ def add_boolean_score(direction, expr, explanation=None):
 
         if email.person_id not in self.rotation_index:
             return
+
+        # If a reviewer is unavailable, they are ignored.
+        periods = self.unavailable_periods.get(email.person_id, [])
+        unavailable_at_the_moment = periods and not (
+            email.person_id in self.has_reviewed_previous and
+            all(p.availability == "canfinish" for p in periods)
+        )
+        if unavailable_at_the_moment:
+            return
 
+        def format_period(p):
+            if p.end_date:
+                res = "unavailable until {}".format(p.end_date.isoformat())
+            else:
+                res = "unavailable indefinitely"
+            return "{} ({})".format(res, p.get_availability_display())
+        if periods:
+            explanations.append(", ".join(format_period(p) for p in periods))
+            
         # misc
         add_boolean_score(+1, email.person_id in self.has_reviewed_previous, "reviewed document before")
         add_boolean_score(+1, email.person_id in self.wish_to_review, "wishes to review document")
@@ -334,7 +353,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._unavailable_reviewers(dont_skip)
+            reviewers_to_skip = self._entirely_unavailable_reviewers(dont_skip)
             rotation_list = [p for p in rotation_list if p.pk not in reviewers_to_skip]
 
         return rotation_list

ietf/review/test_policies.py

@@ -31,9 +31,16 @@ def test_default_reviewer_rotation_list(self):
             team=team,
             person=unavailable_reviewer,
             start_date='2000-01-01',
-            availability=UnavailablePeriod.AVAILABILITY_CHOICES[0],
+            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',
         )
-
         # Default policy without a NextReviewerInTeam
         rotation = policy.default_reviewer_rotation_list()
         self.assertNotIn(unavailable_reviewer, rotation)
@@ -185,13 +192,16 @@ def test_determine_ranking(self):
         team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
         reviewer_high = create_person(team, "reviewer", name="Test Reviewer-high", username="testreviewerhigh")
         reviewer_low = create_person(team, "reviewer", name="Test Reviewer-low", username="testreviewerlow")
+        reviewer_unavailable = create_person(team, "reviewer", name="Test Reviewer-unavailable", username="testreviewerunavailable")
+        # This reviewer should be ignored because it is not in the rotation list.
+        create_person(team, "reviewer", name="Test Reviewer-out-of-rotation", username="testreviewer-out-of-rotation")
 
         # Trigger author check, AD check and group check
         doc = WgDraftFactory(group__acronym='mars', rev='01', authors=[reviewer_low], ad=reviewer_low, shepherd=reviewer_low.email())
         Role.objects.create(group=doc.group, person=reviewer_low, email=reviewer_low.email(), name_id='advisor')
 
         review_req = ReviewRequestFactory(doc=doc, team=team, type_id='early')
-        rotation_list = [reviewer_low, reviewer_high]
+        rotation_list = [reviewer_low, reviewer_high, reviewer_unavailable]
 
         # Trigger previous review check and completed review stats - TODO: something something related documents
         ReviewAssignmentFactory(review_request__team=team, review_request__doc=doc, reviewer=reviewer_high.email(), state_id='completed')
@@ -201,6 +211,23 @@ def test_determine_ranking(self):
         # Trigger review wish check
         ReviewWish.objects.create(team=team, doc=doc, person=reviewer_high)
 
+        # This period should not have an impact, because it is the canfinish type,
+        # and this reviewer has reviewed previously.
+        UnavailablePeriod.objects.create(
+            team=team,
+            person=reviewer_high,
+            start_date='2000-01-01',
+            availability='canfinish',
+        )
+        # This period should exclude this reviewer entirely, as it is 'canfinish',
+        # but this reviewer has not reviewed previously.
+        UnavailablePeriod.objects.create(
+            team=team,
+            person=reviewer_unavailable,
+            start_date='2000-01-01',
+            availability='canfinish',
+        )
+
         # Trigger max frequency and open review stats
         ReviewAssignmentFactory(review_request__team=team, reviewer=reviewer_low.email(), state_id='assigned', review_request__doc__pages=10)
         # Trigger skip_next, max frequency and filter_re
@@ -214,9 +241,10 @@ def test_determine_ranking(self):
 
         order = AssignmentOrderResolver(Email.objects.all(), review_req, rotation_list)
         ranking = order.determine_ranking()
+        self.assertEqual(len(ranking), 2)
         self.assertEqual(ranking[0]['email'], reviewer_high.email())
         self.assertEqual(ranking[1]['email'], reviewer_low.email())
         self.assertEqual(ranking[0]['scores'], [ 1,  1,  1,  1,   0,  0, -1])
         self.assertEqual(ranking[1]['scores'], [-1, -1, -1, -1, -91, -2,  0])
-        self.assertEqual(ranking[0]['label'], 'Test Reviewer-high: reviewed document before; wishes to review document; #2; 1 no response, 1 partially complete, 1 fully completed')
+        self.assertEqual(ranking[0]['label'], 'Test Reviewer-high: unavailable indefinitely (Can do follow-ups); reviewed document before; wishes to review document; #2; 1 no response, 1 partially complete, 1 fully completed')
         self.assertEqual(ranking[1]['label'], 'Test Reviewer-low: is author of document; filter regexp matches; max frequency exceeded, ready in 91 days; skip next 2; #1; currently 1 open, 10 pages')