Merged in work from sasha@dashcare.nl on Review Queue Managemnt:

This abstracts queue management, making it possible to implement different
policies for each team. It provides two concrete policies:
RotateAlphabeticallyReviewerQueuePolicy, which rotates an alphabetically
ordered reviewer list with consideration for skip indications, and is the
default policy; and LeastRecentlyUsedReviewerQueuePolicy, a simple
least-recently-used policy.  Also see issues ietf-tools#2721 and ietf-tools#2656.
 - Legacy-Id: 17121

Author: levkowetz

ietf/doc/tests_review.py

@@ -33,11 +33,10 @@
 from ietf.person.models import Email, Person
 from ietf.review.factories import ReviewRequestFactory, ReviewAssignmentFactory
 from ietf.review.models import (ReviewRequest, ReviewerSettings,
-                                ReviewWish, UnavailablePeriod, NextReviewerInTeam)
-from ietf.review.utils import reviewer_rotation_list, possibly_advance_next_reviewer_for_team
+                                ReviewWish, NextReviewerInTeam)
+from ietf.review.policies import get_reviewer_queue_policy
 
 from ietf.utils.test_utils import TestCase
-from ietf.utils.test_data import create_person
 from ietf.utils.test_utils import login_testing_unauthorized, reload_db_objects
 from ietf.utils.mail import outbox, empty_outbox, parseaddr, on_behalf_of
 from ietf.person.factories import PersonFactory
@@ -235,101 +234,14 @@ def test_close_request(self):
         self.assertIn("closed", mail_content)
         self.assertIn("review_request_close_comment", mail_content)
 
-    def test_possibly_advance_next_reviewer_for_team(self):
-
-        team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
-        doc = WgDraftFactory()
-
-        # make a bunch of reviewers
-        reviewers = [
-            create_person(team, "reviewer", name="Test Reviewer{}".format(i), username="testreviewer{}".format(i))
-            for i in range(5)
-        ]
-
-        self.assertEqual(reviewers, reviewer_rotation_list(team))
-
-        def get_skip_next(person):
-            settings = (ReviewerSettings.objects.filter(team=team, person=person).first()
-                        or ReviewerSettings(team=team))
-            return settings.skip_next
-
-        possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[0].pk, add_skip=False)
-        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[1])
-        self.assertEqual(get_skip_next(reviewers[0]), 0)
-        self.assertEqual(get_skip_next(reviewers[1]), 0)
-
-        possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[1].pk, add_skip=True)
-        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
-        self.assertEqual(get_skip_next(reviewers[1]), 1)
-        self.assertEqual(get_skip_next(reviewers[2]), 0)
-
-        # skip reviewer 2
-        possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[3].pk, add_skip=True)
-        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
-        self.assertEqual(get_skip_next(reviewers[0]), 0)
-        self.assertEqual(get_skip_next(reviewers[1]), 1)
-        self.assertEqual(get_skip_next(reviewers[2]), 0)
-        self.assertEqual(get_skip_next(reviewers[3]), 1)
-
-        # pick reviewer 2, use up reviewer 3's skip_next
-        possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[2].pk, add_skip=False)
-        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4])
-        self.assertEqual(get_skip_next(reviewers[0]), 0)
-        self.assertEqual(get_skip_next(reviewers[1]), 1)
-        self.assertEqual(get_skip_next(reviewers[2]), 0)
-        self.assertEqual(get_skip_next(reviewers[3]), 0)
-        self.assertEqual(get_skip_next(reviewers[4]), 0)
-
-        # check wrap-around
-        possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[4].pk)
-        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[0])
-        self.assertEqual(get_skip_next(reviewers[0]), 0)
-        self.assertEqual(get_skip_next(reviewers[1]), 1)
-        self.assertEqual(get_skip_next(reviewers[2]), 0)
-        self.assertEqual(get_skip_next(reviewers[3]), 0)
-        self.assertEqual(get_skip_next(reviewers[4]), 0)
-
-        # unavailable
-        today = datetime.date.today()
-        UnavailablePeriod.objects.create(team=team, person=reviewers[1], start_date=today, end_date=today, availability="unavailable")
-        possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[0].pk)
-        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
-        self.assertEqual(get_skip_next(reviewers[0]), 0)
-        self.assertEqual(get_skip_next(reviewers[1]), 1) # don't consume that skip while the reviewer is unavailable
-        self.assertEqual(get_skip_next(reviewers[2]), 0)
-        self.assertEqual(get_skip_next(reviewers[3]), 0)
-        self.assertEqual(get_skip_next(reviewers[4]), 0)
-
-        # pick unavailable anyway
-        possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[1].pk, add_skip=False)
-        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
-        self.assertEqual(get_skip_next(reviewers[0]), 0)
-        self.assertEqual(get_skip_next(reviewers[1]), 1)
-        self.assertEqual(get_skip_next(reviewers[2]), 0)
-        self.assertEqual(get_skip_next(reviewers[3]), 0)
-        self.assertEqual(get_skip_next(reviewers[4]), 0)
-
-        # not through min_interval so advance past reviewer[2]
-        settings, _ = ReviewerSettings.objects.get_or_create(team=team, person=reviewers[2])
-        settings.min_interval = 30
-        settings.save()
-        req = ReviewRequest.objects.create(team=team, doc=doc, type_id="early", state_id="assigned", deadline=today, requested_by=reviewers[0])
-        ReviewAssignmentFactory(review_request=req, state_id="accepted", reviewer = reviewers[2].email_set.first(),assigned_on = req.time)
-        possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[3].pk)
-        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4])
-        self.assertEqual(get_skip_next(reviewers[0]), 0)
-        self.assertEqual(get_skip_next(reviewers[1]), 1)
-        self.assertEqual(get_skip_next(reviewers[2]), 0)
-        self.assertEqual(get_skip_next(reviewers[3]), 0)
-        self.assertEqual(get_skip_next(reviewers[4]), 0)
-
     def test_assign_reviewer(self):
         doc = WgDraftFactory(pages=2)
         review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
         rev_role = RoleFactory(group=review_team,person__user__username='reviewer',person__user__email='reviewer@example.com',person__name='Some Reviewer',name_id='reviewer')
         RoleFactory(group=review_team,person__user__username='marschairman',person__name='WG Cháir Man',name_id='reviewer')
         secretary = RoleFactory(group=review_team,person__user__username='reviewsecretary',person__user__email='reviewsecretary@example.com',name_id='secr')
         ReviewerSettings.objects.create(team=review_team, person=rev_role.person, min_interval=14, skip_next=0)
+        queue_policy = get_reviewer_queue_policy(review_team)
 
         # review to assign to
         review_req = ReviewRequestFactory(team=review_team,doc=doc,state_id='requested')
@@ -371,17 +283,8 @@ def test_assign_reviewer(self):
         another_reviewer = PersonFactory.create(name = "Extra TestReviewer") # needs to be lexically greater than the existing one
         another_reviewer.role_set.create(name_id='reviewer', email=another_reviewer.email(), group=review_req.team)
 
-        UnavailablePeriod.objects.create(
-            team=review_req.team,
-            person=reviewer_email.person,
-            start_date=datetime.date.today() - datetime.timedelta(days=10),
-            availability="unavailable",
-        )
-
         ReviewWish.objects.create(person=reviewer_email.person, team=review_req.team, doc=doc)
 
-        # pick a non-existing reviewer as next to see that we can
-        # handle reviewers who have left
         NextReviewerInTeam.objects.filter(team=review_req.team).delete()
         NextReviewerInTeam.objects.create(
             team=review_req.team,
@@ -409,14 +312,13 @@ def test_assign_reviewer(self):
         self.assertIn("wishes to review", reviewer_label)
         self.assertIn("is author", reviewer_label)
         self.assertIn("regexp matches", reviewer_label)
-        self.assertIn("unavailable indefinitely", reviewer_label)
         self.assertIn("skip next 1", reviewer_label)
         self.assertIn("#1", reviewer_label)
         self.assertIn("1 fully completed", reviewer_label)
 
         # assign
         empty_outbox()
-        rotation_list = reviewer_rotation_list(review_req.team)
+        rotation_list = queue_policy.default_reviewer_rotation_list()
         reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[0]).first()
         r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk })
         self.assertEqual(r.status_code, 302)
@@ -427,7 +329,6 @@ def test_assign_reviewer(self):
         assignment = review_req.reviewassignment_set.first()
         self.assertEqual(assignment.reviewer, reviewer)
         self.assertEqual(assignment.state_id, "assigned")
-        self.assertEqual(NextReviewerInTeam.objects.get(team=review_req.team).next_reviewer, rotation_list[1])
 
         self.assertEqual(len(outbox), 1)
         self.assertEqual('"Some Reviewer" <reviewer@example.com>', outbox[0]["To"])

ietf/doc/views_review.py

@@ -38,11 +38,12 @@
 from ietf.message.models import Message
 from ietf.message.utils import infer_message
 from ietf.person.fields import PersonEmailChoiceField, SearchablePersonField
+from ietf.review.policies import get_reviewer_queue_policy
 from ietf.review.utils import (active_review_teams, assign_review_request_to_reviewer,
                                can_request_review_of_doc, can_manage_review_requests_for_team,
                                email_review_assignment_change, email_review_request_change,
                                close_review_request_states,
-                               close_review_request, setup_reviewer_field)
+                               close_review_request)
 from ietf.review import mailarch
 from ietf.utils.fields import DatepickerDateField
 from ietf.utils.text import strip_prefix, xslugify
@@ -307,7 +308,7 @@ class AssignReviewerForm(forms.Form):
 
     def __init__(self, review_req, *args, **kwargs):
         super(AssignReviewerForm, self).__init__(*args, **kwargs)
-        setup_reviewer_field(self.fields["reviewer"], review_req)
+        get_reviewer_queue_policy(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req)
 
 
 @login_required
@@ -378,6 +379,9 @@ def reject_reviewer_assignment(request, name, assignment_id):
                 state=review_assignment.state,
             )
 
+            policy = get_reviewer_queue_policy(review_assignment.review_request.team)
+            policy.return_reviewer_to_rotation_top(review_assignment.reviewer.person)
+
             msg = render_to_string("review/reviewer_assignment_rejected.txt", {
                 "by": request.user.person,
                 "message_to_secretary": form.cleaned_data.get("message_to_secretary")
@@ -425,6 +429,9 @@ def withdraw_reviewer_assignment(request, name, assignment_id):
             state=review_assignment.state,
         )            
 
+        policy = get_reviewer_queue_policy(review_assignment.review_request.team)
+        policy.return_reviewer_to_rotation_top(review_assignment.reviewer.person)
+        
         msg = "Review assignment withdrawn by %s"%request.user.person
 
         email_review_assignment_change(request, review_assignment, "Reviewer assignment withdrawn", msg, by=request.user.person, notify_secretary=True, notify_reviewer=True, notify_requested_by=False)

ietf/group/forms.py

@@ -20,7 +20,8 @@
 from ietf.person.fields import SearchableEmailsField, PersonEmailChoiceField
 from ietf.person.models import Person
 from ietf.review.models import ReviewerSettings, UnavailablePeriod, ReviewSecretarySettings
-from ietf.review.utils import close_review_request_states, setup_reviewer_field
+from ietf.review.policies import get_reviewer_queue_policy
+from ietf.review.utils import close_review_request_states
 from ietf.utils.textupload import get_cleaned_text_file_content
 from ietf.utils.text import strip_suffix
 #from ietf.utils.ordereddict import insert_after_in_ordered_dict
@@ -256,7 +257,7 @@ def __init__(self, review_req, *args, **kwargs):
 
         self.fields["close"].widget.attrs["class"] = "form-control input-sm"
 
-        setup_reviewer_field(self.fields["reviewer"], review_req)
+        get_reviewer_queue_policy(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req)
         self.fields["reviewer"].widget.attrs["class"] = "form-control input-sm"
 
         if not getattr(review_req, 'in_lc_and_telechat', False):
@@ -280,7 +281,7 @@ class ReviewerSettingsForm(forms.ModelForm):
     class Meta:
         model = ReviewerSettings
         fields = ['min_interval', 'filter_re', 'skip_next', 'remind_days_before_deadline', 
-                  'remind_days_open_reviews', 'expertise']
+                  'remind_days_open_reviews', 'request_assignment_next', 'expertise']
 
     def __init__(self, *args, **kwargs):
        exclude_fields = kwargs.pop('exclude_fields', [])

ietf/group/tests_review.py

@@ -12,6 +12,7 @@
 
 from django.urls import reverse as urlreverse
 
+from ietf.review.policies import get_reviewer_queue_policy
 from ietf.utils.test_utils import login_testing_unauthorized, TestCase, reload_db_objects
 from ietf.doc.models import TelechatDocEvent, LastCallDocEvent, State
 from ietf.group.models import Role
@@ -23,7 +24,6 @@
     suggested_review_requests_for_team,
     review_assignments_needing_reviewer_reminder, email_reviewer_reminder,
     review_assignments_needing_secretary_reminder, email_secretary_reminder,
-    reviewer_rotation_list,
     send_unavaibility_period_ending_reminder, send_reminder_all_open_reviews,
     send_review_reminder_overdue_assignment, send_reminder_unconfirmed_assignments)
 from ietf.name.models import ReviewResultName, ReviewRequestStateName, ReviewAssignmentStateName, \
@@ -576,6 +576,7 @@ def test_change_reviewer_settings(self):
             "filter_re": "test-[regexp]",
             "remind_days_before_deadline": "6",
             "remind_days_open_reviews": "8",
+            "request_assignment_next": "1",
             "expertise": "Some expertise",
         })
         self.assertEqual(r.status_code, 302)
@@ -591,6 +592,7 @@ def test_change_reviewer_settings(self):
         msg_content = outbox[0].get_payload(decode=True).decode("utf-8").lower()
         self.assertTrue("frequency changed", msg_content)
         self.assertTrue("skip next", msg_content)
+        self.assertTrue("requested to be the next person", msg_content)
 
         # Normal reviewer should not be able to change skip_next
         r = self.client.post(url, {
@@ -903,7 +905,8 @@ def test_rotation_queue_update(self):
         secretary = RoleFactory.create(group=group,name_id='secr')
         docs = [DocumentFactory.create(type_id='draft',group=None) for i in range(4)]
         requests = [ReviewRequestFactory(team=group,doc=docs[i]) for i in range(4)]
-        rot_list = reviewer_rotation_list(group)
+        policy = get_reviewer_queue_policy(group)
+        rot_list = policy.default_reviewer_rotation_list()
 
         expected_ending_head_of_rotation = rot_list[3]
 
@@ -924,6 +927,6 @@ def test_rotation_queue_update(self):
         self.client.login(username=secretary.person.user.username,password=secretary.person.user.username+'+password')
         r = self.client.post(unassigned_url, postdict)
         self.assertEqual(r.status_code,302)
-        self.assertEqual(expected_ending_head_of_rotation,reviewer_rotation_list(group)[0])
+        self.assertEqual(expected_ending_head_of_rotation, policy.default_reviewer_rotation_list()[0])
         self.assertMailboxContains(outbox, subject='Last Call assignment', text='Requested by', count=4)
 

ietf/group/views.py

@@ -93,6 +93,7 @@
 from ietf.person.models import Email
 from ietf.review.models import (ReviewRequest, ReviewAssignment, ReviewerSettings, 
                                 ReviewSecretarySettings, UnavailablePeriod )
+from ietf.review.policies import get_reviewer_queue_policy
 from ietf.review.utils import (can_manage_review_requests_for_team,
                                can_access_review_stats_for_team,
 
@@ -104,7 +105,6 @@
                                unavailable_periods_to_list,
                                current_unavailable_periods_for_reviewers,
                                email_reviewer_availability_change,
-                               reviewer_rotation_list,
                                latest_review_assignments_for_reviewers,
                                augment_review_requests_with_events,
                                get_default_filter_re,
@@ -1395,7 +1395,7 @@ def reviewer_overview(request, acronym, group_type=None):
 
     can_manage = can_manage_review_requests_for_team(request.user, group)
 
-    reviewers = reviewer_rotation_list(group)
+    reviewers = get_reviewer_queue_policy(group).default_reviewer_rotation_list(include_unavailable=True)
 
     reviewer_settings = { s.person_id: s for s in ReviewerSettings.objects.filter(team=group) }
     unavailable_periods = defaultdict(list)
@@ -1566,13 +1566,14 @@ def manage_review_requests(request, acronym, group_type=None, assignment_status=
             # Make sure the any assignments to the person at the head
             # of the rotation queue are processed first so that the queue
             # rotates before any more assignments are processed
-            head_of_rotation = reviewer_rotation_list(group)[0]
+            reviewer_policy = get_reviewer_queue_policy(group)
+            head_of_rotation = reviewer_policy.default_reviewer_rotation_list_without_skipped()[0]
             while head_of_rotation in assignments_by_person:
                 for review_req in assignments_by_person[head_of_rotation]:
                     assign_review_request_to_reviewer(request, review_req, review_req.form.cleaned_data["reviewer"],review_req.form.cleaned_data["add_skip"])
                     reqs_to_assign.remove(review_req)
                 del assignments_by_person[head_of_rotation]
-                head_of_rotation = reviewer_rotation_list(group)[0]
+                head_of_rotation = reviewer_policy.default_reviewer_rotation_list_without_skipped()[0]
 
             for review_req in reqs_to_assign:
                 assign_review_request_to_reviewer(request, review_req, review_req.form.cleaned_data["reviewer"],review_req.form.cleaned_data["add_skip"])
@@ -1685,7 +1686,7 @@ def should_be_replicated_in_last_call_section(r):
 
         partial_msg = render_to_string(template.path, {
             "review_assignments": review_assignments,
-            "rotation_list": reviewer_rotation_list(group)[:10],
+            "rotation_list": get_reviewer_queue_policy(group).default_reviewer_rotation_list()[:10],
             "group" : group,
         })
 
@@ -1756,7 +1757,10 @@ def change_reviewer_settings(request, acronym, reviewer_email, group_type=None):
                 changes.append("Frequency changed to \"{}\" from \"{}\".".format(settings.get_min_interval_display() or "Not specified", prev_min_interval or "Not specified"))
             if settings.skip_next != prev_skip_next:
                 changes.append("Skip next assignments changed to {} from {}.".format(settings.skip_next, prev_skip_next))
-
+            if settings.request_assignment_next:
+                changes.append("Reviewer has requested to be the next person selected for an "
+                               "assignment, as soon as possible, and will be on the top of "
+                               "the queue.") 
             if changes:
                 email_reviewer_availability_change(request, group, reviewer_role, "\n\n".join(changes), request.user.person)