From b64066b7427f2ec8dc6dcbfd1d373a770b14e692 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Fri, 22 Nov 2019 11:00:58 +0000 Subject: [PATCH] Fix LeastRecentlyUsed policy ordering. - Legacy-Id: 17092 --- ietf/review/policies.py | 13 +++++++++---- ietf/review/test_policies.py | 17 ++++++++++++----- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 22edebeca..acf069848 100644 --- a/ietf/review/policies.py +++ b/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') - - reviewers_with_assignment = [assignment.reviewer.person for assignment in assignments] + ).values('reviewer__person').annotate(most_recent=Max('assigned_on')).order_by('most_recent') + + 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) diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 1ca299208..11f5bf922 100644 --- a/ietf/review/test_policies.py +++ b/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', - review_request__team=team) - # Completed assignment, assigned before reviewer 1, + ReviewAssignmentFactory(reviewer=reviewers[2].email(), assigned_on='2020-01-01', + state_id='rejected', review_request__team=team) + # 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"))