Change default of ReviewerSettings.min_interval to null - if it's not

specified for a reviewer, we don't take it into account
 - Legacy-Id: 12168

Author: OleLaursen

ietf/doc/tests_review.py

@@ -136,7 +136,9 @@ 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):
+    def test_possibly_advance_next_reviewer_for_team(self):
+        doc = make_test_data()
+
         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"))
 
@@ -148,36 +150,29 @@ def make_data_for_rotation_tests(self, doc):
 
         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)
+        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[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)
+        possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=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)
+        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[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)
+        possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=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)
@@ -186,7 +181,7 @@ def get_skip_next(person):
         self.assertEqual(get_skip_next(reviewers[4]), 0)
 
         # check wrap-around
-        possibly_advance_next_reviewer_for_team(team, reviewers[4].pk)
+        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]), 0)
@@ -197,7 +192,7 @@ def get_skip_next(person):
         # 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)
+        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]), 0)
@@ -206,17 +201,20 @@ def get_skip_next(person):
         self.assertEqual(get_skip_next(reviewers[4]), 0)
 
         # pick unavailable anyway
-        possibly_advance_next_reviewer_for_team(team, reviewers[1].pk)
+        possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=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
+        # 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()
         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)
+        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)

ietf/group/views_review.py

@@ -429,7 +429,7 @@ def change_reviewer_settings(request, acronym, reviewer_email, group_type=None):
 
             changes = []
             if settings.get_min_interval_display() != prev_min_interval:
-                changes.append("Frequency changed to \"{}\" from \"{}\".".format(settings.get_min_interval_display(), prev_min_interval))
+                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))
 

ietf/review/import_from_review_tool.py

@@ -114,22 +114,23 @@ def parse_timestamp(t):
                 if created:
                     print "created role", unicode(role).encode("utf-8")
 
-                reviewer, created = ReviewerSettings.objects.get_or_create(
+                reviewer_settings, created = ReviewerSettings.objects.get_or_create(
                     team=team,
                     person=email.person,
                 )
                 if created:
-                    print "created reviewer", reviewer.pk, unicode(reviewer).encode("utf-8")
+                    print "created reviewer settings", reviewer_settings.pk, unicode(reviewer_settings).encode("utf-8")
 
+                reviewer_settings.min_interval = None
                 if autopolicy_days.get(row.autopolicy):
-                    reviewer.min_interval = autopolicy_days.get(row.autopolicy)
+                    reviewer_settings.min_interval = autopolicy_days.get(row.autopolicy)
 
-                reviewer.filter_re = row.donotassign
+                reviewer_settings.filter_re = row.donotassign
                 try:
-                    reviewer.skip_next = int(row.autopolicy)
+                    reviewer_settings.skip_next = int(row.autopolicy)
                 except ValueError:
                     pass
-                reviewer.save()
+                reviewer_settings.save()
 
                 unavailable_until = parse_timestamp(row.available)
                 if unavailable_until:

ietf/review/models.py

@@ -20,7 +20,7 @@ class ReviewerSettings(models.Model):
         (61, "Once per two months"),
         (91, "Once per quarter"),
     ]
-    min_interval = models.IntegerField(default=30, verbose_name="Can review at most", choices=INTERVALS)
+    min_interval = models.IntegerField(verbose_name="Can review at most", choices=INTERVALS, blank=True, null=True)
     filter_re   = models.CharField(max_length=255, verbose_name="Filter regexp", blank=True, help_text="Draft names matching regular expression should not be assigned")
     skip_next   = models.IntegerField(default=0, verbose_name="Skip next assignments")
     remind_days_before_deadline = models.IntegerField(null=True, blank=True, help_text="To get an email reminder in case you forget to do an assigned review, enter the number of days before a review deadline you want to receive it. Clear the field if you don't want a reminder.")

ietf/review/utils.py

@@ -110,33 +110,33 @@ def reviewer_rotation_list(team, skip_unavailable=False, dont_skip=[]):
 
         days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(team)
         for person_id, days_needed in days_needed_for_reviewers.iteritems():
-            if days_needed > 0 and person_id not in dont_skip:
+            if person_id not in dont_skip:
                 reviewers_to_skip.add(person_id)
 
         rotation_list = [p.pk for p in rotation_list if p.pk not in reviewers_to_skip]
 
     return rotation_list
 
 def days_needed_to_fulfill_min_interval_for_reviewers(team):
-    """Returns person_id -> days needed until min_interval is fulfilled for
-    reviewer."""
+    """Returns person_id -> days needed until min_interval is fulfilled
+    for reviewer (in case it is necessary to wait, otherwise reviewer
+    is absent in result)."""
     latest_assignments = dict(ReviewRequest.objects.filter(
         team=team,
     ).values_list("reviewer__person").annotate(Max("time")))
 
     min_intervals = dict(ReviewerSettings.objects.filter(team=team).values_list("person_id", "min_interval"))
 
-    default_min_interval = ReviewerSettings(team=team).min_interval
-
     now = datetime.datetime.now()
 
     res = {}
     for person_id, latest_assignment_time in latest_assignments.iteritems():
         if latest_assignment_time is not None:
-            min_interval = min_intervals.get(person_id, default_min_interval)
+            min_interval = min_intervals.get(person_id)
+            if min_interval is None:
+                continue
 
             days_needed = max(0, min_interval - (now - latest_assignment_time).days)
-
             if days_needed > 0:
                 res[person_id] = days_needed
 

ietf/templates/group/reviewer_overview.html

@@ -41,8 +41,17 @@ <h2>Reviewers</h2>
               </table>
             </td>
             <td>
-              {{ person.settings.get_min_interval_display }} {% if person.settings.skip_next %}(skip: {{ person.settings.skip_next }}){% endif %}<br>
-              {% if person.settings.filter_re %}Filter: <code title="{{ person.settings.filter_re }}">{{ person.settings.filter_re|truncatechars:15 }}</code><br>{% endif %}
+              {% if person.settings.min_interval %}
+                {{ person.settings.get_min_interval_display }}<br>
+              {% endif %}
+
+              {% if person.settings.skip_next %}
+                Skip: {{ person.settings.skip_next }}<br>
+              {% endif %}
+
+              {% if person.settings.filter_re %}
+                Filter: <code title="{{ person.settings.filter_re }}">{{ person.settings.filter_re|truncatechars:15 }}</code><br>
+              {% endif %}
 
               {% if person.unavailable_periods %}
                 {% include "review/unavailable_table.html" with unavailable_periods=person.unavailable_periods %}

ietf/templates/ietfauth/review_overview.html

@@ -126,7 +126,7 @@ <h2>Settings for {{ t }}</h2>
     <table class="table">
       <tr>
         <th>Can review</th>
-        <td>{{ t.reviewer_settings.get_min_interval_display }}</td>
+        <td>{{ t.reviewer_settings.get_min_interval_display|default:"No max frequency set" }}</td>
       </tr>
       <tr>
         <th>Skip next assignments</th>