Added two new configuration settings for the review team secretary,

one to set how many days to include in the reviewers list, and
another one to limit the number of completed items in the list for
each person. This version replaces the one I did earlier, and includes
much more test cases to test different limits on the reviewers page.
Commit ready for merge.
 - Legacy-Id: 17034

Author: kivinen

ietf/group/forms.py

@@ -334,6 +334,7 @@ def clean_end_date(self):
 class ReviewSecretarySettingsForm(forms.ModelForm):
     class Meta:
         model = ReviewSecretarySettings
-        fields = ['remind_days_before_deadline']
+        fields = ['remind_days_before_deadline', 'max_items_to_show_in_reviewer_list',
+                  'days_to_show_in_reviewer_list']
 
 

ietf/group/tests_review.py

@@ -156,6 +156,7 @@ def test_reviewer_overview(self):
         reviewer = RoleFactory(name_id='reviewer',group=team,person__user__username='reviewer').person
         ReviewerSettingsFactory(person=reviewer,team=team)
         review_req1 = ReviewRequestFactory(state_id='completed',team=team)
+        secretary = Person.objects.get(user__username="secretary")
         ReviewAssignmentFactory(review_request = review_req1, reviewer=reviewer.email())
         PersonFactory(user__username='plain')
 
@@ -212,6 +213,156 @@ def test_reviewer_overview(self):
         # secretariat can see reason for being unavailable
         self.assertContains(r, "Availability")
 
+        # add one closed review with no response and see it is visible
+        review_req2 = ReviewRequestFactory(state_id='completed',team=team)
+        ReviewAssignmentFactory(
+            review_request__doc=review_req2.doc,
+            review_request__team=review_req2.team,
+            review_request__type_id="lc",
+            review_request__deadline=datetime.date.today() - datetime.timedelta(days=30),
+            review_request__state_id="assigned",
+            review_request__requested_by=Person.objects.get(user__username="reviewer"),
+            state_id = "no-response",
+            reviewer=reviewer.email_set.first(),
+        )
+        self.client.login(username="secretary", password="secretary+password")
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 200)
+        # We should see the new document with status of no response
+        self.assertContains(r, "No Response")
+        self.assertContains(r, review_req1.doc.name)
+        self.assertContains(r, review_req2.doc.name)
+        # None of the reviews should be completed this time,
+        # note that "Days Since Completed has soft hypens in it, so it
+        # will not match
+        self.assertNotContains(r, "Completed")
+        # add multiple completed reviews
+        review_req3 = ReviewRequestFactory(state_id='completed', team=team)
+        ReviewAssignmentFactory(
+            review_request__doc=review_req3.doc,
+            review_request__time=datetime.date.today() - datetime.timedelta(days=30),
+            review_request__team=review_req3.team,
+            review_request__type_id="telechat",
+            review_request__deadline=datetime.date.today() - datetime.timedelta(days=25),
+            review_request__state_id="completed",
+            review_request__requested_by=Person.objects.get(user__username="reviewer"),
+            state_id = "completed",
+            reviewer=reviewer.email_set.first(),
+            assigned_on=datetime.date.today() - datetime.timedelta(days=30)
+        )
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 200)
+        self.assertContains(r, "Completed")
+        self.assertContains(r, review_req1.doc.name)
+        self.assertContains(r, review_req2.doc.name)
+        self.assertContains(r, review_req3.doc.name)
+        # Few more to make sure we hit the limit
+        reqs = [ReviewRequestFactory(state_id='completed', team=team) for i in range(10)]
+        for i in range(10):
+            ReviewAssignmentFactory(
+                review_request__doc=reqs[i].doc,
+                review_request__time=datetime.date.today() - datetime.timedelta(days=i*30),
+                review_request__team=reqs[i].team,
+                review_request__type_id="telechat",
+                review_request__deadline=datetime.date.today() - datetime.timedelta(days=i*20),
+                review_request__state_id="completed",
+                review_request__requested_by=Person.objects.get(user__username="reviewer"),
+                state_id = "completed",
+                reviewer=reviewer.email_set.first(),
+                assigned_on=datetime.date.today() - datetime.timedelta(days=i*30)
+            )
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 200)
+        self.assertContains(r, "Completed")
+        self.assertContains(r, review_req1.doc.name)
+        self.assertContains(r, review_req2.doc.name)
+        self.assertContains(r, review_req3.doc.name)
+        # The default is 10 completed entries, we had one before + one No response
+        # so we should have 8 more here
+        for i in range(10):
+            if i < 8:
+                self.assertContains(r, reqs[i].doc.name)
+            else:
+                self.assertNotContains(r, reqs[i].doc.name)
+        # Change the limit to be 15 instead of 10, so we should get all
+        secretary_settings = ReviewSecretarySettings(team=team, person=secretary)
+        secretary_settings.max_items_to_show_in_reviewer_list = 15
+        secretary_settings.save()
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 200)
+        self.assertContains(r, review_req1.doc.name)
+        self.assertContains(r, review_req2.doc.name)
+        self.assertContains(r, review_req3.doc.name)
+        for i in range(10):
+            self.assertContains(r, reqs[i].doc.name)
+        # Change the time limit to 100 days, so we should only get 3 completed
+        secretary_settings.days_to_show_in_reviewer_list = 100
+        secretary_settings.save()
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 200)
+        self.assertContains(r, review_req1.doc.name)
+        self.assertContains(r, review_req2.doc.name)
+        self.assertContains(r, review_req3.doc.name)
+        for i in range(10):
+            if i < 4:
+                self.assertContains(r, reqs[i].doc.name)
+            else:
+                self.assertNotContains(r, reqs[i].doc.name)
+        # Add assigned items, they should be visible as long as they
+        # are withing time period
+        review_req4 = ReviewRequestFactory(state_id='completed', team=team)
+        ReviewAssignmentFactory(
+            review_request__doc=review_req4.doc,
+            review_request__time=datetime.date.today() - datetime.timedelta(days=80),
+            review_request__team=review_req4.team,
+            review_request__type_id="lc",
+            review_request__deadline=datetime.date.today() - datetime.timedelta(days=60),
+            review_request__state_id="assigned",
+            review_request__requested_by=Person.objects.get(user__username="reviewer"),
+            state_id = "accepted",
+            reviewer=reviewer.email_set.first(),
+            assigned_on=datetime.date.today() - datetime.timedelta(days=80)
+        )
+        review_req5 = ReviewRequestFactory(state_id='completed', team=team)
+        ReviewAssignmentFactory(
+            review_request__doc=review_req5.doc,
+            review_request__time=datetime.date.today() - datetime.timedelta(days=120),
+            review_request__team=review_req5.team,
+            review_request__type_id="lc",
+            review_request__deadline=datetime.date.today() - datetime.timedelta(days=100),
+            review_request__state_id="assigned",
+            review_request__requested_by=Person.objects.get(user__username="reviewer"),
+            state_id = "accepted",
+            reviewer=reviewer.email_set.first(),
+            assigned_on=datetime.date.today() - datetime.timedelta(days=120)
+        )
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 200)
+        self.assertContains(r, review_req1.doc.name)
+        self.assertContains(r, review_req2.doc.name)
+        self.assertContains(r, review_req3.doc.name)
+        self.assertContains(r, review_req4.doc.name)
+        self.assertNotContains(r, review_req5.doc.name)
+        for i in range(10):
+            if i < 4:
+                self.assertContains(r, reqs[i].doc.name)
+            else:
+                self.assertNotContains(r, reqs[i].doc.name)
+        # Change the limit to be 1 instead of 10, so we should only one entry
+        secretary_settings.max_items_to_show_in_reviewer_list = 1
+        secretary_settings.save()
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 200)
+        self.assertContains(r, review_req1.doc.name)
+        self.assertContains(r, review_req2.doc.name)
+        self.assertNotContains(r, review_req3.doc.name)
+        for i in range(10):
+            self.assertNotContains(r, reqs[i].doc.name)
+        self.assertContains(r, review_req4.doc.name)
+        self.assertNotContains(r, review_req5.doc.name)
+
+        # print(r.content)
+
     def test_manage_review_requests(self):
         group = ReviewTeamFactory()
         RoleFactory(name_id='reviewer',group=group,person__user__username='reviewer').person
@@ -539,11 +690,15 @@ def test_change_review_secretary_settings(self):
 
         # set settings
         r = self.client.post(url, {
-            "remind_days_before_deadline": "6"
+            "remind_days_before_deadline": "6",
+            "max_items_to_show_in_reviewer_list": 10,
+            "days_to_show_in_reviewer_list": 365
         })
         self.assertEqual(r.status_code, 302)
         settings = ReviewSecretarySettings.objects.get(person=secretary, team=review_req.team)
         self.assertEqual(settings.remind_days_before_deadline, 6)
+        self.assertEqual(settings.max_items_to_show_in_reviewer_list, 10)
+        self.assertEqual(settings.days_to_show_in_reviewer_list, 365)
 
     def test_review_reminders(self):
         review_req = ReviewRequestFactory(state_id='assigned')

ietf/group/views.py

@@ -1402,7 +1402,24 @@ def reviewer_overview(request, acronym, group_type=None):
 
     today = datetime.date.today()
 
-    req_data_for_reviewers = latest_review_assignments_for_reviewers(group)
+    max_closed_reqs = 10
+    days_back = 365
+    if can_manage:
+        secretary_settings = (ReviewSecretarySettings.objects.filter(person=
+                                                                     request.user.person,
+                                                                     team=group).first()
+                              or ReviewSecretarySettings(person=request.user.person,
+                                                         team=group))
+        if secretary_settings:
+            max_closed_reqs = secretary_settings.max_items_to_show_in_reviewer_list
+            days_back = secretary_settings.days_to_show_in_reviewer_list
+
+    if max_closed_reqs == None:
+        max_closed_reqs = 10
+
+    if days_back == None:
+        days_back = 365
+    req_data_for_reviewers = latest_review_assignments_for_reviewers(group, days_back)
     assignment_state_by_slug = { n.slug: n for n in ReviewAssignmentStateName.objects.all() }
 
     days_needed = days_needed_to_fulfill_min_interval_for_reviewers(group)
@@ -1424,13 +1441,14 @@ def reviewer_overview(request, acronym, group_type=None):
             person.busy = person.id in days_needed 
 
 
-        MAX_CLOSED_REQS = 10 # This keeps the overview page with being filled with too many closed requests, since the focus should be on open or recently closed per reviewer
         days_since = 9999
         req_data = req_data_for_reviewers.get(person.pk, [])
-        open_reqs = sum(1 for d in req_data if d.state in ["assigned", "accepted"])
+        closed_reqs = 0
         latest_reqs = []
         for d in req_data:
-            if d.state in ["assigned", "accepted"] or len(latest_reqs) < MAX_CLOSED_REQS + open_reqs:
+            if d.state in ["assigned", "accepted"] or closed_reqs < max_closed_reqs:
+                if not d.state in ["assigned", "accepted"]:
+                    closed_reqs += 1
                 latest_reqs.append((d.assignment_pk, d.request_pk, d.doc_name, d.reviewed_rev, d.assigned_time, d.deadline,
                                     assignment_state_by_slug.get(d.state),
                                     int(math.ceil(d.assignment_to_closure_days)) if d.assignment_to_closure_days is not None else None))

ietf/review/admin.py

@@ -23,7 +23,7 @@ def acronym(self, obj):
 admin.site.register(ReviewerSettings, ReviewerSettingsAdmin)
 
 class ReviewSecretarySettingsAdmin(admin.ModelAdmin):
-    list_display = ['id', 'team', 'person', 'remind_days_before_deadline']
+    list_display = ['id', 'team', 'person', 'remind_days_before_deadline', 'max_items_to_show_in_reviewer_list', 'days_to_show_in_reviewer_list']
     raw_id_fields = ['team', 'person']
 admin.site.register(ReviewSecretarySettings, ReviewSecretarySettingsAdmin)
 

ietf/review/migrations/0020_auto_20191115_2059.py

@@ -0,0 +1,26 @@
+# Copyright The IETF Trust 2019, All Rights Reserved
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.26 on 2019-11-15 20:59
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('review', '0019_auto_20191023_0829'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='reviewsecretarysettings',
+            name='days_to_show_in_reviewer_list',
+            field=models.IntegerField(blank=True, help_text='Maximum number of days to show in reviewer list for completed items.', null=True),
+        ),
+        migrations.AddField(
+            model_name='reviewsecretarysettings',
+            name='max_items_to_show_in_reviewer_list',
+            field=models.IntegerField(blank=True, help_text='Maximum number of completed items to show for one reviewer in the reviewer list view, the list is also filtered by the days to show in reviews list setting.', null=True),
+        ),
+    ]

ietf/review/models.py

@@ -52,6 +52,8 @@ class ReviewSecretarySettings(models.Model):
     team        = ForeignKey(Group, limit_choices_to=~models.Q(reviewteamsettings=None))
     person      = ForeignKey(Person)
     remind_days_before_deadline = models.IntegerField(null=True, blank=True, help_text="To get an email reminder in case a reviewer forgets to do an assigned review, enter the number of days before review deadline you want to receive it. Clear the field if you don't want a reminder.")
+    max_items_to_show_in_reviewer_list = models.IntegerField(null=True, blank=True, help_text="Maximum number of completed items to show for one reviewer in the reviewer list view, the list is also filtered by the days to show in reviews list setting.")
+    days_to_show_in_reviewer_list = models.IntegerField(null=True, blank=True, help_text="Maximum number of days to show in reviewer list for completed items.")
 
     def __str__(self):
         return "{} in {}".format(self.person, self.team)