Cleanup. Branch ready for merge. (see email)

 - Legacy-Id: 17059

Author: Sasha Romijn

ietf/review/policies.py

@@ -184,20 +184,19 @@ def _collect_context(self):
         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 @@ 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.
+        # 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 @@ def format_period(p):
         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 @@ def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip_pe
         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 @@ def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip_pe
         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 @@ def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip_pe
         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
 
 

ietf/review/test_policies.py

@@ -281,7 +281,7 @@ def test_determine_ranking(self):
         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 @@ def test_determine_ranking(self):
         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 @@ def test_determine_ranking(self):
             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')