Early work to extract reviewer policy from review/utils.py.

 - Legacy-Id: 16950

Author: Sasha Romijn

ietf/doc/tests_review.py

@@ -33,10 +33,9 @@
 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
+from ietf.review.policies import policy_for_team
 
 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
@@ -232,95 +231,8 @@ 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):
+        # TODO: this test overlaps way too much with the reviewer policy
         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')
@@ -413,7 +325,7 @@ def test_assign_reviewer(self):
 
         # assign
         empty_outbox()
-        rotation_list = reviewer_rotation_list(review_req.team)
+        rotation_list = policy_for_team(review_req.team).reviewer_rotation_list(review_req.team)
         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)

ietf/doc/views_review.py

@@ -33,11 +33,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 policy_for_team
 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
@@ -293,7 +294,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)
+        policy_for_team(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req)
 
 
 @login_required

ietf/group/forms.py

@@ -19,7 +19,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 policy_for_team
+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
@@ -254,7 +255,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)
+        policy_for_team(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req)
         self.fields["reviewer"].widget.attrs["class"] = "form-control input-sm"
 
         if self.is_bound:

ietf/group/tests_review.py

@@ -12,6 +12,7 @@
 
 from django.urls import reverse as urlreverse
 
+from ietf.review.policies import policy_for_team
 from ietf.utils.test_utils import login_testing_unauthorized, TestCase, reload_db_objects
 from ietf.doc.models import TelechatDocEvent
 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
@@ -655,7 +655,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 = policy_for_team(group)
+        rot_list = policy.reviewer_rotation_list(group)
 
         expected_ending_head_of_rotation = rot_list[3]
 
@@ -676,6 +677,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.reviewer_rotation_list(group)[0])
         self.assertMailboxContains(outbox, subject='Last Call assignment', text='Requested by', count=4)
 

ietf/group/views.py

@@ -92,6 +92,7 @@
 from ietf.name.models import GroupTypeName, StreamName
 from ietf.person.models import Email
 from ietf.review.models import ReviewRequest, ReviewAssignment, ReviewerSettings, ReviewSecretarySettings
+from ietf.review.policies import policy_for_team
 from ietf.review.utils import (can_manage_review_requests_for_team,
                                can_access_review_stats_for_team,
 
@@ -103,7 +104,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,
@@ -1391,7 +1391,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 = policy_for_team(group).reviewer_rotation_list(group)
 
     reviewer_settings = { s.person_id: s for s in ReviewerSettings.objects.filter(team=group) }
     unavailable_periods = defaultdict(list)
@@ -1541,13 +1541,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 = policy_for_team(group)
+            head_of_rotation = reviewer_policy.reviewer_rotation_list(group)[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.reviewer_rotation_list(group)[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"])
@@ -1660,7 +1661,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": policy_for_team(group).reviewer_rotation_list(group)[:10],
             "group" : group,
         })