From b5d864499f98f8b9c726c45a1f7093e02e35e99f Mon Sep 17 00:00:00 2001 From: Tero Kivinen Date: Sat, 16 Nov 2019 09:41:55 +0000 Subject: [PATCH] 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 --- ietf/group/forms.py | 3 +- ietf/group/tests_review.py | 157 +++++++++++++++++- ietf/group/views.py | 26 ++- ietf/review/admin.py | 2 +- .../migrations/0020_auto_20191115_2059.py | 26 +++ ietf/review/models.py | 2 + 6 files changed, 209 insertions(+), 7 deletions(-) create mode 100644 ietf/review/migrations/0020_auto_20191115_2059.py diff --git a/ietf/group/forms.py b/ietf/group/forms.py index 63353dc8a..d5f02128a 100644 --- a/ietf/group/forms.py +++ b/ietf/group/forms.py @@ -334,6 +334,7 @@ class EndUnavailablePeriodForm(forms.Form): 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'] diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py index 296f99e7d..fb45a3862 100644 --- a/ietf/group/tests_review.py +++ b/ietf/group/tests_review.py @@ -156,6 +156,7 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): # 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 @@ class ReviewTests(TestCase): # 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') diff --git a/ietf/group/views.py b/ietf/group/views.py index e757709f1..ffe8cdb28 100644 --- a/ietf/group/views.py +++ b/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)) diff --git a/ietf/review/admin.py b/ietf/review/admin.py index b540e75cd..290556833 100644 --- a/ietf/review/admin.py +++ b/ietf/review/admin.py @@ -23,7 +23,7 @@ class ReviewerSettingsAdmin(simple_history.admin.SimpleHistoryAdmin): 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) diff --git a/ietf/review/migrations/0020_auto_20191115_2059.py b/ietf/review/migrations/0020_auto_20191115_2059.py new file mode 100644 index 000000000..ccf041800 --- /dev/null +++ b/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), + ), + ] diff --git a/ietf/review/models.py b/ietf/review/models.py index a701e6f0d..d57c14da4 100644 --- a/ietf/review/models.py +++ b/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)