From c8812c719374a4be91f267db344f75beeb7af06c Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Tue, 12 Nov 2019 12:11:52 +0000 Subject: [PATCH] Account for 'canfinish' unavailabilities. - Legacy-Id: 16999 --- ietf/review/policies.py | 27 +++++++++++++++++++++++---- ietf/review/test_policies.py | 36 ++++++++++++++++++++++++++++++++---- 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index a58523c51..b62e87144 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -73,7 +73,7 @@ class AbstractReviewerQueuePolicy: 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 @@ class AssignmentOrderResolver: 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 @@ class AssignmentOrderResolver: 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 @@ class AssignmentOrderResolver: 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 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): 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 diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index f3232776d..04f67929a 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -31,9 +31,16 @@ class RotateWithSkipReviewerPolicyTests(TestCase): 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 @@ class AssignmentOrderResolverTests(TestCase): 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 @@ class AssignmentOrderResolverTests(TestCase): # 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 @@ class AssignmentOrderResolverTests(TestCase): 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')