diff --git a/ietf/review/policies.py b/ietf/review/policies.py
index fc679122e..e89c5cd10 100644
--- a/ietf/review/policies.py
+++ b/ietf/review/policies.py
@@ -3,6 +3,7 @@
from __future__ import absolute_import, print_function, unicode_literals
import re
+from collections import OrderedDict
import six
@@ -11,7 +12,8 @@ from ietf.doc.utils import extract_complete_replaces_ancestor_mapping_for_docs
from ietf.group.models import Role
from ietf.person.models import Person
import debug # pyflakes:ignore
-from ietf.review.models import NextReviewerInTeam, ReviewerSettings, ReviewWish, ReviewRequest
+from ietf.review.models import NextReviewerInTeam, ReviewerSettings, ReviewWish, ReviewRequest, \
+ ReviewAssignment
from ietf.review.utils import (current_unavailable_periods_for_reviewers,
days_needed_to_fulfill_min_interval_for_reviewers,
get_default_filter_re,
@@ -41,12 +43,58 @@ class AbstractReviewerQueuePolicy:
def update_policy_state_for_assignment(self, assignee_person, add_skip=False):
"""
- Update the internal state of a policy to reflect an assignment.
+ Update the skip_count if the assignment was in order, and
+ update NextReviewerInTeam. Note that NextReviewerInTeam is
+ not used by all policies.
"""
settings = self._reviewer_settings_for(assignee_person)
settings.request_assignment_next = False
settings.save()
+ rotation_list = self.default_reviewer_rotation_list(
+ dont_skip_person_ids=[assignee_person.pk])
+ def reviewer_at_index(i):
+ if not rotation_list:
+ return None
+ return rotation_list[i % len(rotation_list)]
+
+ if not rotation_list:
+ return
+
+ rotation_list_without_skip = [r for r in rotation_list if
+ not self._reviewer_settings_for(r).skip_next]
+ # In order means: assigned to the first person in the rotation list with skip_next=0
+ # If the assignment is not in order, skip_next and NextReviewerInTeam are not modified.
+ in_order_assignment = rotation_list_without_skip[0] == assignee_person
+
+ # Loop through the list until finding the first person with skip_next=0,
+ # who is not the current assignee. Anyone with skip_next>0 encountered before
+ # has their skip_next decreased.
+ current_idx = 0
+ if in_order_assignment:
+ while True:
+ current_idx_person = reviewer_at_index(current_idx)
+ settings = self._reviewer_settings_for(current_idx_person)
+ if settings.skip_next > 0:
+ settings.skip_next -= 1
+ settings.save()
+ elif current_idx_person != assignee_person:
+ # NextReviewerInTeam is not used by all policies to determine
+ # default rotation order, but updated regardless.
+ nr = NextReviewerInTeam.objects.filter(
+ team=self.team).first() or NextReviewerInTeam(
+ team=self.team)
+ nr.next_reviewer = current_idx_person
+ nr.save()
+
+ break
+ current_idx += 1
+
+ if add_skip:
+ settings = self._reviewer_settings_for(assignee_person)
+ settings.skip_next += 1
+ settings.save()
+
# TODO : Change this field to deal with multiple already assigned reviewers???
def setup_reviewer_field(self, field, review_req):
"""
@@ -298,50 +346,11 @@ class AssignmentOrderResolver:
class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy):
-
- def update_policy_state_for_assignment(self, assignee_person, add_skip=False):
- super(RotateAlphabeticallyReviewerQueuePolicy, self).update_policy_state_for_assignment(assignee_person, add_skip)
- assert assignee_person is not None
-
- rotation_list = self.default_reviewer_rotation_list(dont_skip_person_ids=[assignee_person.pk])
-
- def reviewer_at_index(i):
- if not rotation_list:
- return None
- return rotation_list[i % len(rotation_list)]
-
- if not rotation_list:
- return
-
- rotation_list_without_skip = [r for r in rotation_list if not self._reviewer_settings_for(r).skip_next]
- # In order means: assigned to the first person in the rotation list with skip_next=0
- # If the assignment is not in order, skip_next and NextReviewerInTeam are not modified.
- in_order_assignment = rotation_list_without_skip[0] == assignee_person
-
- # Loop through the list until finding the first person with skip_next=0,
- # who is not the current assignee. Anyone with skip_next>0 encountered before
- # has their skip_next decreased.
- current_idx = 0
- if in_order_assignment:
- while True:
- current_idx_person = reviewer_at_index(current_idx)
- settings = self._reviewer_settings_for(current_idx_person)
- if settings.skip_next > 0:
- settings.skip_next -= 1
- settings.save()
- elif current_idx_person != assignee_person:
- nr = NextReviewerInTeam.objects.filter(team=self.team).first() or NextReviewerInTeam(
- team=self.team)
- nr.next_reviewer = current_idx_person
- nr.save()
-
- break
- current_idx += 1
-
- if add_skip:
- settings = self._reviewer_settings_for(assignee_person)
- settings.skip_next += 1
- settings.save()
+ """
+ A policy in which the default rotation list is based on last name, alphabetically.
+ NextReviewerInTeam is used to store a pointer to where the queue is currently
+ positioned.
+ """
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))
@@ -372,3 +381,28 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy):
return rotation_list
+
+class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy):
+ """
+ A policy where the default rotation list is based on the most recent
+ assigned, accepted or completed review assignment.
+ """
+ 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))
+ assignments = ReviewAssignment.objects.filter(
+ review_request__team=self.team,
+ state__in=['accepted', 'assigned', 'completed'],
+ ).order_by('assigned_on')
+
+ reviewers_with_assignment = [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))
+
+ if not include_unavailable:
+ reviewers_to_skip = self._entirely_unavailable_reviewers(dont_skip_person_ids)
+ 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 568905cd5..b33921497 100644
--- a/ietf/review/test_policies.py
+++ b/ietf/review/test_policies.py
@@ -7,15 +7,20 @@ from ietf.person.fields import PersonEmailChoiceField
from ietf.person.models import Email
from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory
from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, ReviewWish
-from ietf.review.policies import get_reviewer_queue_policy, AssignmentOrderResolver
+from ietf.review.policies import get_reviewer_queue_policy, AssignmentOrderResolver, \
+ LeastRecentlyUsedReviewerQueuePolicy, RotateAlphabeticallyReviewerQueuePolicy
from ietf.utils.test_data import create_person
from ietf.utils.test_utils import TestCase
class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase):
+ """
+ These tests also cover the common behaviour in RotateAlphabeticallyReviewerQueuePolicy,
+ as that's difficult to test on it's own.
+ """
def test_default_reviewer_rotation_list(self):
team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
- policy = get_reviewer_queue_policy(team)
+ policy = RotateAlphabeticallyReviewerQueuePolicy(team)
reviewers = [
create_person(team, "reviewer", name="Test Reviewer{}".format(i), username="testreviewer{}".format(i))
@@ -171,6 +176,64 @@ class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase):
self.assertEqual(get_skip_next(reviewers[4]), 0)
+class LeastRecentlyUsedReviewerQueuePolicyTest(TestCase):
+ """
+ These tests only cover where this policy deviates from
+ RotateAlphabeticallyReviewerQueuePolicy - the common behaviour
+ inherited from AbstractReviewerQueuePolicy is covered in
+ RotateAlphabeticallyReviewerQueuePolicyTest.
+ """
+ def test_default_reviewer_rotation_list(self):
+ team = ReviewTeamFactory(acronym="rotationteam", name="Review Team",
+ list_email="rotationteam@ietf.org",
+ parent=Group.objects.get(acronym="farfut"))
+ policy = LeastRecentlyUsedReviewerQueuePolicy(team)
+
+ reviewers = [
+ create_person(team, "reviewer", name="Test Reviewer{}".format(i),
+ username="testreviewer{}".format(i))
+ for i in range(5)
+ ]
+
+ # This reviewer should never be included.
+ unavailable_reviewer = create_person(team, "reviewer", name="unavailable reviewer",
+ username="unavailablereviewer")
+ UnavailablePeriod.objects.create(
+ team=team,
+ person=unavailable_reviewer,
+ start_date='2000-01-01',
+ 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',
+ )
+
+ # No known assignments
+ rotation = policy.default_reviewer_rotation_list()
+ self.assertNotIn(unavailable_reviewer, rotation)
+ self.assertEqual(rotation, reviewers)
+
+ # Regular accepted assignment
+ ReviewAssignmentFactory(reviewer=reviewers[1].email(), assigned_on='2019-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,
+ # 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)
+ rotation = policy.default_reviewer_rotation_list()
+ self.assertNotIn(unavailable_reviewer, rotation)
+ self.assertEqual(rotation, [reviewers[2], reviewers[3], reviewers[4], reviewers[0], reviewers[1]])
+
+
class AssignmentOrderResolverTests(TestCase):
def test_determine_ranking(self):
# reviewer_high is second in the default rotation, reviewer_low is first