Fix LeastRecentlyUsed policy ordering.

- Legacy-Id: 17092

ietf/review/policies.py

@@ -6,6 +6,7 @@ import re
 from collections import OrderedDict
 
 import six
+from django.db.models.aggregates import Max
 
 from ietf.doc.models import DocumentAuthor, DocAlias
 from ietf.doc.utils import extract_complete_replaces_ancestor_mapping_for_docs
@@ -423,17 +424,21 @@ class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy):
     """
     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_dict = {p.pk: p for p in reviewers}
         assignments = ReviewAssignment.objects.filter(
             review_request__team=self.team,
             state__in=['accepted', 'assigned', 'completed'],
             reviewer__person__in=reviewers,
-        ).order_by('assigned_on').select_related('reviewer', 'reviewer__person')
+        ).values('reviewer__person').annotate(most_recent=Max('assigned_on')).order_by('most_recent')
-        
+
-        reviewers_with_assignment = [assignment.reviewer.person for assignment in assignments]
+        reviewers_with_assignment = [
+            reviewers_dict[assignment['reviewer__person']]
+            for assignment in assignments
+        ] 
         reviewers_without_assignment = set(reviewers) - set(reviewers_with_assignment)
         
         rotation_list = sorted(list(reviewers_without_assignment), key=lambda r: r.pk)
-        rotation_list += list(OrderedDict.fromkeys(reviewers_with_assignment))
+        rotation_list += reviewers_with_assignment
         
         if not include_unavailable:
             reviewers_to_skip = self._entirely_unavailable_reviewers(dont_skip_person_ids)

ietf/review/test_policies.py

@@ -257,27 +257,34 @@ class LeastRecentlyUsedReviewerQueuePolicyTest(TestCase):
         out_of_team_reviewer = PersonFactory()
         ReviewAssignmentFactory(review_request__team=team, reviewer=out_of_team_reviewer.email())
 
-        # No known assignments
+        # No known assignments, order in PK order.
         rotation = policy.default_reviewer_rotation_list()
         self.assertNotIn(unavailable_reviewer, rotation)
         self.assertEqual(rotation, reviewers)
         
-        # Regular accepted assignment
+        # Regular accepted assignments. Note that one is older and one is newer than reviewer 0's,
+        # the newest one should count for ordering, i.e. reviewer 1 should be later in the list.
         ReviewAssignmentFactory(reviewer=reviewers[1].email(), assigned_on='2019-01-01',
                                 state_id='accepted', review_request__team=team)
+        ReviewAssignmentFactory(reviewer=reviewers[1].email(), assigned_on='2017-01-01',
+                                state_id='accepted', review_request__team=team)
         # Rejected assignment, should not affect reviewer 2's position
-        ReviewAssignmentFactory(reviewer=reviewers[2].email(), state_id='rejected',
+        ReviewAssignmentFactory(reviewer=reviewers[2].email(), assigned_on='2020-01-01',
-                                review_request__team=team)
+                                state_id='rejected', review_request__team=team)
-        # Completed assignment, assigned before reviewer 1,
+        # Completed assignments, assigned before the most recent assignment of reviewer 1,
         # but completed after (assign date should count).
         ReviewAssignmentFactory(reviewer=reviewers[0].email(), assigned_on='2018-01-01',
                                 completed_on='2020-01-01', state_id='completed',
                                 review_request__team=team)
+        ReviewAssignmentFactory(reviewer=reviewers[0].email(), assigned_on='2018-05-01',
+                                completed_on='2020-01-01', state_id='completed',
+                                review_request__team=team)
         rotation = policy.default_reviewer_rotation_list()
         self.assertNotIn(unavailable_reviewer, rotation)
         self.assertEqual(rotation, [reviewers[2], reviewers[3], reviewers[4], reviewers[0], reviewers[1]])
 
     def test_return_reviewer_to_top_rotation(self):
+        # Should do nothing, this is implicit in this policy, no state change is needed.
         team = ReviewTeamFactory(acronym="rotationteam", name="Review Team",
                                  list_email="rotationteam@ietf.org",
                                  parent=Group.objects.get(acronym="farfut"))