Implement a round-robin rotation scheme for reviewers instead of

relying on when they last reviewed. In-order assignments automatically
move the rotation forwards while out-of-order assignments increment
the skip next of the assigned reviewer. Include rotation in open
review assignments email. Fix a couple of issues in the importer.
 - Legacy-Id: 12015

Author: OleLaursen

ietf/doc/tests_review.py

@@ -12,13 +12,16 @@
 
 import debug                            # pyflakes:ignore
 
-from ietf.review.models import ReviewRequest, ReviewTeamResult, ReviewerSettings, ReviewWish, UnavailablePeriod
+from ietf.review.models import (ReviewRequest, ReviewTeamResult, ReviewerSettings,
+                                ReviewWish, UnavailablePeriod, NextReviewerInTeam)
+from ietf.review.utils import reviewer_rotation_list, possibly_advance_next_reviewer_for_team
 import ietf.review.mailarch
 from ietf.person.models import Email, Person
 from ietf.name.models import ReviewResultName, ReviewRequestStateName, ReviewTypeName, DocRelationshipName
+from ietf.group.models import Group
 from ietf.doc.models import DocumentAuthor, Document, DocAlias, RelatedDocument, DocEvent
 from ietf.utils.test_utils import TestCase
-from ietf.utils.test_data import make_test_data, make_review_data
+from ietf.utils.test_data import make_test_data, make_review_data, create_person
 from ietf.utils.test_utils import login_testing_unauthorized, unicontent, reload_db_objects
 from ietf.utils.mail import outbox, empty_outbox
 
@@ -133,6 +136,94 @@ def test_close_request(self):
         self.assertEqual(len(outbox), 1)
         self.assertTrue("closed" in unicode(outbox[0]).lower())
 
+    def make_data_for_rotation_tests(self, doc):
+        team = Group.objects.create(state_id="active", acronym="rotationteam", name="Review Team", type_id="dir",
+                                    list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
+
+        # 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))
+
+        return team, reviewers
+
+    def test_possibly_advance_next_reviewer_for_team(self):
+        doc = make_test_data()
+
+        team, reviewers = self.make_data_for_rotation_tests(doc)
+
+        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, reviewers[0].pk)
+        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, reviewers[1].pk)
+        self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
+
+        # skip reviewer 2
+        possibly_advance_next_reviewer_for_team(team, reviewers[3].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]), 0)
+        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, reviewers[2].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]), 0)
+        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, 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]), 0)
+        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, 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]), 0)
+        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, reviewers[1].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)
+        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
+        ReviewRequest.objects.create(team=team, doc=doc, type_id="early", state_id="accepted", deadline=today, requested_by=reviewers[0], reviewer=reviewers[2].email_set.first())
+        possibly_advance_next_reviewer_for_team(team, 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 = make_test_data()
 
@@ -166,6 +257,7 @@ def test_assign_reviewer(self):
 
         reviewer_settings = ReviewerSettings.objects.get(person__email=plain_email)
         reviewer_settings.filter_re = doc.name
+        reviewer_settings.skip_next = 1
         reviewer_settings.save()
 
         UnavailablePeriod.objects.create(
@@ -177,6 +269,14 @@ def test_assign_reviewer(self):
 
         ReviewWish.objects.create(person=plain_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,
+            next_reviewer=Person.objects.exclude(pk=plain_email.person_id).first(),
+        )
+
         assign_url = urlreverse('ietf.doc.views_review.assign_reviewer', kwargs={ "name": doc.name, "request_id": review_req.pk })
 
 
@@ -194,16 +294,18 @@ def test_assign_reviewer(self):
         self.assertEqual(r.status_code, 200)
         q = PyQuery(r.content)
         plain_label = q("option[value=\"{}\"]".format(plain_email.address)).text().lower()
-        self.assertIn("ready for", plain_label)
         self.assertIn("reviewed document before", plain_label)
         self.assertIn("wishes to review", plain_label)
         self.assertIn("is author", plain_label)
         self.assertIn("regexp matches", plain_label)
-        self.assertIn("unavailable", plain_label)
+        self.assertIn("unavailable indefinitely", plain_label)
+        self.assertIn("skip next 1", plain_label)
+        self.assertIn("#1", plain_label)
 
         # assign
         empty_outbox()
-        reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team).first()
+        rotation_list = 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)
 
@@ -212,12 +314,13 @@ def test_assign_reviewer(self):
         self.assertEqual(review_req.reviewer, reviewer)
         self.assertEqual(len(outbox), 1)
         self.assertTrue("assigned" in unicode(outbox[0]))
+        self.assertEqual(NextReviewerInTeam.objects.get(team=review_req.team).next_reviewer, rotation_list[1])
 
         # re-assign
         empty_outbox()
         review_req.state = ReviewRequestStateName.objects.get(slug="accepted")
         review_req.save()
-        reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team).exclude(pk=reviewer.pk).first()
+        reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[1]).first()
         r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk })
         self.assertEqual(r.status_code, 302)
 

ietf/group/tests_review.py

@@ -1,6 +1,6 @@
 import datetime
 
-#from pyquery import PyQuery
+from pyquery import PyQuery
 
 from django.core.urlresolvers import reverse as urlreverse
 
@@ -175,7 +175,10 @@ def test_email_open_review_assignments(self):
 
         r = self.client.get(url)
         self.assertEqual(r.status_code, 200)
-        self.assertTrue(review_req1.doc.name in unicontent(r))
+        q = PyQuery(r.content)
+        generated_text = q("[name=body]").text()
+        self.assertTrue(review_req1.doc.name in generated_text)
+        self.assertTrue(unicode(Person.objects.get(user__username="marschairman")) in generated_text)
 
         empty_outbox()
         r = self.client.post(url, {

ietf/group/views_review.py

@@ -16,7 +16,8 @@
                                suggested_review_requests_for_team,
                                unavailability_periods_to_list,
                                current_unavailable_periods_for_reviewers,
-                               email_reviewer_availability_change)
+                               email_reviewer_availability_change,
+                               reviewer_rotation_list)
 from ietf.group.models import Role
 from ietf.group.utils import get_group_or_404
 from ietf.person.fields import PersonEmailChoiceField
@@ -221,9 +222,10 @@ def email_open_review_assignments(request, acronym, group_type=None):
     else:
         to = group.list_email
         subject = "Open review assignments in {}".format(group.acronym)
-        # FIXME: add rotation info
+
         body = render_to_string("group/email_open_review_assignments.txt", {
             "review_requests": review_requests,
+            "rotation_list": reviewer_rotation_list(group)[:10],
         })
 
         form = EmailOpenAssignmentsForm(initial={

ietf/review/import_from_review_tool.py

@@ -18,7 +18,7 @@
 from django.db import connections
 from ietf.review.models import (ReviewRequest, ReviewerSettings, ReviewResultName,
                                 ReviewRequestStateName, ReviewTypeName, ReviewTeamResult,
-                                UnavailablePeriod)
+                                UnavailablePeriod, NextReviewerInTeam)
 from ietf.group.models import Group, Role, RoleName
 from ietf.person.models import Person, Email, Alias
 import argparse
@@ -138,9 +138,6 @@ def parse_timestamp(t):
                             availability="unavailable",
                         )
 
-
-# review requests
-
 # check that we got the needed names
 results = { n.name.lower(): n for n in ReviewResultName.objects.all() }
 
@@ -150,8 +147,23 @@ def parse_timestamp(t):
     missing_result_names = set(summaries) - set(results.keys())
     assert not missing_result_names, "missing result names: {} {}".format(missing_result_names, results.keys())
 
-    for s in summaries:
-        ReviewTeamResult.objects.get_or_create(team=team, result=results[s])
+
+# configuration options
+with db_con.cursor() as c:
+    c.execute("select * from config;")
+
+    for row in namedtuplefetchall(c):
+        if row.name == "next": # next reviewer
+            NextReviewerInTeam.objects.filter(team=team).delete()
+            NextReviewerInTeam.objects.create(team=team, next_reviewer=known_personnel[row.value].person)
+
+        if row.name == "summary-list": # review results used in team
+            summaries = [v.strip().lower() for v in row.value.split(";") if v.strip()]
+
+            for s in summaries:
+                ReviewTeamResult.objects.get_or_create(team=team, result=results[s])
+
+# review requests
 
 states = { n.slug: n for n in ReviewRequestStateName.objects.all() }
 # map some names

ietf/review/migrations/0001_initial.py

@@ -15,6 +15,17 @@ class Migration(migrations.Migration):
     ]
 
     operations = [
+        migrations.CreateModel(
+            name='NextReviewerInTeam',
+            fields=[
+                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
+                ('next_reviewer', models.ForeignKey(to='person.Person')),
+                ('team', models.ForeignKey(to='group.Group')),
+            ],
+            options={
+            },
+            bases=(models.Model,),
+        ),
         migrations.CreateModel(
             name='ReviewerSettings',
             fields=[

ietf/review/models.py

@@ -77,6 +77,13 @@ class ReviewTeamResult(models.Model):
     def __unicode__(self):
         return u"{} in {}".format(self.result.name, self.group.acronym)
 
+class NextReviewerInTeam(models.Model):
+    team        = models.ForeignKey(Group)
+    next_reviewer = models.ForeignKey(Person)
+
+    def __unicode__(self):
+        return u"{} next in {}".format(self.next_reviewer, self.team)
+
 class ReviewRequest(models.Model):
     """Represents a request for a review and the process it goes through.
     There should be one ReviewRequest entered for each combination of

ietf/review/resources.py

@@ -8,7 +8,7 @@
 from ietf.api import ToOneField                         # pyflakes:ignore
 
 from ietf.review.models import (ReviewerSettings, ReviewRequest, ReviewTeamResult,
-                                UnavailablePeriod, ReviewWish)
+                                UnavailablePeriod, ReviewWish, NextReviewerInTeam)
 
 
 from ietf.person.resources import PersonResource
@@ -125,3 +125,22 @@ class Meta:
         }
 api.review.register(ReviewWishResource())
 
+
+
+from ietf.person.resources import PersonResource
+from ietf.group.resources import GroupResource
+class NextReviewerInTeamResource(ModelResource):
+    team             = ToOneField(GroupResource, 'team')
+    next_reviewer    = ToOneField(PersonResource, 'next_reviewer')
+    class Meta:
+        queryset = NextReviewerInTeam.objects.all()
+        serializer = api.Serializer()
+        cache = SimpleCache()
+        #resource_name = 'nextreviewerinteam'
+        filtering = { 
+            "id": ALL,
+            "team": ALL_WITH_RELATIONS,
+            "next_reviewer": ALL_WITH_RELATIONS,
+        }
+api.review.register(NextReviewerInTeamResource())
+