From d0877a0aa96575a7971d930caa0be40d40c0e05f Mon Sep 17 00:00:00 2001 From: Ole Laursen Date: Tue, 18 Oct 2016 10:36:42 +0000 Subject: [PATCH] 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 --- ietf/doc/tests_review.py | 32 +++++++++----------- ietf/group/views_review.py | 2 +- ietf/review/import_from_review_tool.py | 13 ++++---- ietf/review/models.py | 2 +- ietf/review/utils.py | 14 ++++----- ietf/templates/group/reviewer_overview.html | 13 ++++++-- ietf/templates/ietfauth/review_overview.html | 2 +- 7 files changed, 43 insertions(+), 35 deletions(-) diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index 39eebf21d..9099d2fba 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -136,7 +136,9 @@ class ReviewTests(TestCase): 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,28 +150,21 @@ class ReviewTests(TestCase): 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) @@ -177,7 +172,7 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): # 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,7 +201,7 @@ class ReviewTests(TestCase): 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) @@ -214,9 +209,12 @@ class ReviewTests(TestCase): 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) diff --git a/ietf/group/views_review.py b/ietf/group/views_review.py index 73c572398..20885ea48 100644 --- a/ietf/group/views_review.py +++ b/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)) diff --git a/ietf/review/import_from_review_tool.py b/ietf/review/import_from_review_tool.py index 984936919..e8c4411e0 100755 --- a/ietf/review/import_from_review_tool.py +++ b/ietf/review/import_from_review_tool.py @@ -114,22 +114,23 @@ with db_con.cursor() as c: 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: diff --git a/ietf/review/models.py b/ietf/review/models.py index 08d5a4c3b..8bba30e36 100644 --- a/ietf/review/models.py +++ b/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.") diff --git a/ietf/review/utils.py b/ietf/review/utils.py index 894d3ce89..93ef74410 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -110,7 +110,7 @@ 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] @@ -118,25 +118,25 @@ def reviewer_rotation_list(team, skip_unavailable=False, dont_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 diff --git a/ietf/templates/group/reviewer_overview.html b/ietf/templates/group/reviewer_overview.html index 9a25c8f3b..814be277e 100644 --- a/ietf/templates/group/reviewer_overview.html +++ b/ietf/templates/group/reviewer_overview.html @@ -41,8 +41,17 @@ - {{ person.settings.get_min_interval_display }} {% if person.settings.skip_next %}(skip: {{ person.settings.skip_next }}){% endif %}
- {% if person.settings.filter_re %}Filter: {{ person.settings.filter_re|truncatechars:15 }}
{% endif %} + {% if person.settings.min_interval %} + {{ person.settings.get_min_interval_display }}
+ {% endif %} + + {% if person.settings.skip_next %} + Skip: {{ person.settings.skip_next }}
+ {% endif %} + + {% if person.settings.filter_re %} + Filter: {{ person.settings.filter_re|truncatechars:15 }}
+ {% endif %} {% if person.unavailable_periods %} {% include "review/unavailable_table.html" with unavailable_periods=person.unavailable_periods %} diff --git a/ietf/templates/ietfauth/review_overview.html b/ietf/templates/ietfauth/review_overview.html index edf39f11f..3ea396c22 100644 --- a/ietf/templates/ietfauth/review_overview.html +++ b/ietf/templates/ietfauth/review_overview.html @@ -126,7 +126,7 @@ - +
Can review{{ t.reviewer_settings.get_min_interval_display }}{{ t.reviewer_settings.get_min_interval_display|default:"No max frequency set" }}
Skip next assignments