diff --git a/ietf/review/policies.py b/ietf/review/policies.py index f4417b06b..37b6203f4 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -184,20 +184,19 @@ class AssignmentOrderResolver: self.doc_aliases = DocAlias.objects.filter(docs=self.doc).values_list("name", flat=True) # This data is collected as a dict, keys being person IDs, values being numbers/objects. + self.rotation_index = {p.pk: i for i, p in enumerate(self.rotation_list)} self.reviewer_settings = self._reviewer_settings_for_person_ids(self.possible_person_ids) self.days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(self.team) - self.rotation_index = {p.pk: i for i, p in enumerate(self.rotation_list)} + 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) # This data is collected as a set of person IDs. self.has_completed_review_previous = self._persons_with_previous_review(self.review_req, self.possible_person_ids, 'completed') self.has_rejected_review_previous = self._persons_with_previous_review(self.review_req, self.possible_person_ids, 'rejected') self.wish_to_review = set(ReviewWish.objects.filter(team=self.team, person__in=self.possible_person_ids, doc=self.doc).values_list("person", flat=True)) - - 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): """ @@ -232,7 +231,7 @@ class AssignmentOrderResolver: if email.person_id not in self.rotation_index: return - # If a reviewer is unavailable, they are ignored. + # If a reviewer is unavailable at the moment, they are ignored. periods = self.unavailable_periods.get(email.person_id, []) unavailable_at_the_moment = periods and not ( email.person_id in self.has_completed_review_previous and @@ -265,12 +264,12 @@ class AssignmentOrderResolver: if days_needed > 0: explanations.append("max frequency exceeded, ready in {} {}".format(days_needed, "day" if days_needed == 1 else "days")) - # skip next + # skip next value scores.append(-settings.skip_next) if settings.skip_next > 0: explanations.append("skip next {}".format(settings.skip_next)) - # index + # index in the default rotation order index = self.rotation_index.get(email.person_id, 0) scores.append(-index) explanations.append("#{}".format(index + 1)) @@ -375,21 +374,21 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy): reviewers.sort(key=lambda p: p.last_name()) next_reviewer_index = 0 - # now to figure out where the rotation is currently at - saved_reviewer = NextReviewerInTeam.objects.filter(team=self.team).select_related("next_reviewer").first() - if saved_reviewer: - n = saved_reviewer.next_reviewer + next_reviewer_in_team = NextReviewerInTeam.objects.filter(team=self.team).select_related("next_reviewer").first() + if next_reviewer_in_team: + next_reviewer = next_reviewer_in_team.next_reviewer - if n not in reviewers: - # saved reviewer might not still be here, if not just - # insert and use that position (Python will wrap around, + if next_reviewer not in reviewers: + # If the next reviewer is no longer on the team, + # advance to the person that would be after them in + # the rotation. (Python will wrap around, # so no harm done by using the index on the original list # afterwards) - reviewers_with_next = reviewers[:] + [n] + reviewers_with_next = reviewers[:] + [next_reviewer] reviewers_with_next.sort(key=lambda p: p.last_name()) - next_reviewer_index = reviewers_with_next.index(n) + next_reviewer_index = reviewers_with_next.index(next_reviewer) else: - next_reviewer_index = reviewers.index(n) + next_reviewer_index = reviewers.index(next_reviewer) rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index] @@ -418,7 +417,7 @@ class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy): assignments = ReviewAssignment.objects.filter( review_request__team=self.team, state__in=['accepted', 'assigned', 'completed'], - ).order_by('assigned_on') + ).order_by('assigned_on').select_related('reviewer') reviewers_with_assignment = [assignment.reviewer.person for assignment in assignments] reviewers_without_assignment = set(reviewers) - set(reviewers_with_assignment) @@ -433,9 +432,9 @@ class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy): return rotation_list def return_reviewer_to_top_rotation(self, reviewer_person): - # Reviewer rotation for this policy ignored rejected/withdrawn + # Reviewer rotation for this policy ignores rejected/withdrawn # reviews, so it automatically adjusts the position of someone - # who rejected a review and no action is needed. + # who rejected a review and no further action is needed. pass diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 88bbbb485..1fcc59d6c 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -281,7 +281,7 @@ class AssignmentOrderResolverTests(TestCase): 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. + # This reviewer should be entirely ignored because it is not in the rotation list. create_person(team, "reviewer", name="Test Reviewer-out-of-rotation", username="testreviewer-out-of-rotation") # Create a document with ancestors, that also triggers author check, AD check and group check @@ -291,9 +291,6 @@ class AssignmentOrderResolverTests(TestCase): doc = WgDraftFactory(group__acronym='mars', rev='01', authors=[reviewer_low], ad=reviewer_low, shepherd=reviewer_low.email(), relations=[('replaces', doc_middle_wg)]) 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, reviewer_unavailable] - # Trigger previous review check (including finding ancestor documents) and completed review stats. ReviewAssignmentFactory(review_request__team=team, review_request__doc=doc_individual, reviewer=reviewer_high.email(), state_id='completed') # Trigger other review stats @@ -338,11 +335,15 @@ class AssignmentOrderResolverTests(TestCase): request_assignment_next=True, ) + review_req = ReviewRequestFactory(doc=doc, team=team, type_id='early') + rotation_list = [reviewer_low, reviewer_high, reviewer_unavailable] + 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()) + # These scores follow the ordering of https://trac.tools.ietf.org/tools/ietfdb/wiki/ReviewerQueuePolicy, self.assertEqual(ranking[0]['scores'], [ 1, 1, 1, 1, 1, 1, 0, 0, -1]) self.assertEqual(ranking[1]['scores'], [-1, -1, -1, -1, -1, -1, -91, -2, 0]) self.assertEqual(ranking[0]['label'], 'Test Reviewer-high: unavailable indefinitely (Can do follow-ups); requested to be selected next for assignment; reviewed document before; wishes to review document; #2; 1 no response, 1 partially complete, 1 fully completed')