diff --git a/ietf/bin/send-review-reminders b/ietf/bin/send-review-reminders index f20cc52d4..e74694c8a 100755 --- a/ietf/bin/send-review-reminders +++ b/ietf/bin/send-review-reminders @@ -23,7 +23,7 @@ django.setup() from ietf.review.utils import ( review_assignments_needing_reviewer_reminder, email_reviewer_reminder, review_assignments_needing_secretary_reminder, email_secretary_reminder, - send_unavaibility_period_ending_reminder, send_reminder_all_open_reviews, + send_unavailability_period_ending_reminder, send_reminder_all_open_reviews, send_review_reminder_overdue_assignment, send_reminder_unconfirmed_assignments) from ietf.utils.log import log @@ -38,7 +38,7 @@ for assignment, secretary_role in review_assignments_needing_secretary_reminder( review_req = assignment.review_request log("Emailed reminder to {} for review of {} in {} (req. id {})".format(secretary_role.email.address, review_req.doc_id, review_req.team.acronym, review_req.pk)) -period_end_reminders_sent = send_unavaibility_period_ending_reminder(today) +period_end_reminders_sent = send_unavailability_period_ending_reminder(today) for msg in period_end_reminders_sent: log(msg) diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py index 7af311959..6486fa105 100644 --- a/ietf/group/tests_review.py +++ b/ietf/group/tests_review.py @@ -12,17 +12,10 @@ from django.urls import reverse as urlreverse from ietf.review.policies import get_reviewer_queue_policy from ietf.utils.test_utils import login_testing_unauthorized, TestCase, reload_db_objects from ietf.doc.models import TelechatDocEvent, LastCallDocEvent, State -from ietf.group.models import Role from ietf.iesg.models import TelechatDate from ietf.person.models import Person -from ietf.review.models import ( ReviewerSettings, UnavailablePeriod, ReviewSecretarySettings, - ReviewTeamSettings, NextReviewerInTeam ) -from ietf.review.utils import ( - suggested_review_requests_for_team, - review_assignments_needing_reviewer_reminder, email_reviewer_reminder, - review_assignments_needing_secretary_reminder, email_secretary_reminder, - send_unavaibility_period_ending_reminder, send_reminder_all_open_reviews, - send_review_reminder_overdue_assignment, send_reminder_unconfirmed_assignments) +from ietf.review.models import ReviewerSettings, UnavailablePeriod, ReviewSecretarySettings,NextReviewerInTeam +from ietf.review.utils import suggested_review_requests_for_team from ietf.name.models import ReviewResultName, ReviewRequestStateName, ReviewAssignmentStateName, \ ReviewTypeName import ietf.group.views @@ -703,199 +696,6 @@ class ReviewTests(TestCase): 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') - reviewer = RoleFactory(name_id='reviewer',group=review_req.team,person__user__username='reviewer').person - assignment = ReviewAssignmentFactory(review_request=review_req, state_id='assigned', assigned_on = review_req.time, reviewer=reviewer.email_set.first()) - RoleFactory(name_id='secr',group=review_req.team,person__user__username='reviewsecretary') - ReviewerSettingsFactory(team = review_req.team, person = reviewer) - - remind_days = 6 - - reviewer_settings = ReviewerSettings.objects.get(team=review_req.team, person=reviewer) - reviewer_settings.remind_days_before_deadline = remind_days - reviewer_settings.save() - - secretary = Person.objects.get(user__username="reviewsecretary") - secretary_role = Role.objects.get(group=review_req.team, name="secr", person=secretary) - - secretary_settings = ReviewSecretarySettings(team=review_req.team, person=secretary) - secretary_settings.remind_days_before_deadline = remind_days - secretary_settings.save() - - today = datetime.date.today() - - review_req.reviewer = reviewer.email_set.first() - review_req.deadline = today + datetime.timedelta(days=remind_days) - review_req.save() - - # reviewer - needing_reminders = review_assignments_needing_reviewer_reminder(today - datetime.timedelta(days=1)) - self.assertEqual(list(needing_reminders), []) - - needing_reminders = review_assignments_needing_reviewer_reminder(today) - self.assertEqual(list(needing_reminders), [assignment]) - - needing_reminders = review_assignments_needing_reviewer_reminder(today + datetime.timedelta(days=1)) - self.assertEqual(list(needing_reminders), []) - - # secretary - needing_reminders = review_assignments_needing_secretary_reminder(today - datetime.timedelta(days=1)) - self.assertEqual(list(needing_reminders), []) - - needing_reminders = review_assignments_needing_secretary_reminder(today) - self.assertEqual(list(needing_reminders), [(assignment, secretary_role)]) - - needing_reminders = review_assignments_needing_secretary_reminder(today + datetime.timedelta(days=1)) - self.assertEqual(list(needing_reminders), []) - - # email reviewer - empty_outbox() - email_reviewer_reminder(assignment) - self.assertEqual(len(outbox), 1) - self.assertTrue(review_req.doc.name in get_payload_text(outbox[0])) - - # email secretary - empty_outbox() - email_secretary_reminder(assignment, secretary_role) - self.assertEqual(len(outbox), 1) - self.assertTrue(review_req.doc.name in get_payload_text(outbox[0])) - - def test_send_unavaibility_period_ending_reminder(self): - review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", - list_email="reviewteam@ietf.org") - reviewer = RoleFactory(group=review_team, person__user__username='reviewer', - person__user__email='reviewer@example.com', - person__name='Some Reviewer', name_id='reviewer') - secretary = RoleFactory(group=review_team, person__user__username='reviewsecretary', - person__user__email='reviewsecretary@example.com', name_id='secr') - empty_outbox() - today = datetime.date.today() - UnavailablePeriod.objects.create( - team=review_team, - person=reviewer.person, - start_date=today - datetime.timedelta(days=40), - end_date=today + datetime.timedelta(days=3), - availability="unavailable", - ) - UnavailablePeriod.objects.create( - team=review_team, - person=reviewer.person, - # This object should be ignored, length is too short - start_date=today - datetime.timedelta(days=20), - end_date=today + datetime.timedelta(days=3), - availability="unavailable", - ) - UnavailablePeriod.objects.create( - team=review_team, - person=reviewer.person, - start_date=today - datetime.timedelta(days=40), - # This object should be ignored, end date is too far away - end_date=today + datetime.timedelta(days=4), - availability="unavailable", - ) - UnavailablePeriod.objects.create( - team=review_team, - person=reviewer.person, - # This object should be ignored, end date is too close - start_date=today - datetime.timedelta(days=40), - end_date=today + datetime.timedelta(days=2), - availability="unavailable", - ) - log = send_unavaibility_period_ending_reminder(today) - - self.assertEqual(len(outbox), 1) - self.assertTrue(reviewer.person.email_address() in outbox[0]["To"]) - self.assertTrue(secretary.person.email_address() in outbox[0]["To"]) - message = get_payload_text(outbox[0]) - self.assertTrue(reviewer.person.name in message) - self.assertTrue(review_team.acronym in message) - self.assertEqual(len(log), 1) - self.assertTrue(reviewer.person.name in log[0]) - self.assertTrue(review_team.acronym in log[0]) - - def test_send_review_reminder_overdue_assignment(self): - today = datetime.date.today() - - # An assignment that's exactly on the date at which the grace period expires - review_req = ReviewRequestFactory(state_id='assigned', deadline=today - datetime.timedelta(5)) - reviewer = RoleFactory(name_id='reviewer', group=review_req.team,person__user__username='reviewer').person - ReviewAssignmentFactory(review_request=review_req, state_id='assigned', assigned_on=review_req.time, reviewer=reviewer.email_set.first()) - secretary = RoleFactory(name_id='secr', group=review_req.team, person__user__username='reviewsecretary') - - # A assignment that is not yet overdue - not_overdue = today + datetime.timedelta(days=1) - ReviewAssignmentFactory(review_request__team=review_req.team, review_request__state_id='assigned', review_request__deadline=not_overdue, state_id='assigned', assigned_on=not_overdue, reviewer=reviewer.email_set.first()) - - # An assignment that is overdue but is not past the grace period - in_grace_period = today - datetime.timedelta(days=1) - ReviewAssignmentFactory(review_request__team=review_req.team, review_request__state_id='assigned', review_request__deadline=in_grace_period, state_id='assigned', assigned_on=in_grace_period, reviewer=reviewer.email_set.first()) - - empty_outbox() - log = send_review_reminder_overdue_assignment(today) - self.assertEqual(len(log), 1) - - self.assertEqual(len(outbox), 1) - self.assertTrue(secretary.person.email_address() in outbox[0]["To"]) - self.assertEqual(outbox[0]["Subject"], "1 Overdue review for team {}".format(review_req.team.acronym)) - message = get_payload_text(outbox[0]) - self.assertIn(review_req.team.acronym + ' has 1 accepted or assigned review overdue by at least 5 days.', message) - self.assertIn('Review of {} by {}'.format(review_req.doc.name, reviewer.plain_name()), message) - self.assertEqual(len(log), 1) - self.assertIn(secretary.person.email_address(), log[0]) - self.assertIn('1 overdue review', log[0]) - - def test_send_reminder_all_open_reviews(self): - review_req = ReviewRequestFactory(state_id='assigned') - reviewer = RoleFactory(name_id='reviewer', group=review_req.team,person__user__username='reviewer').person - ReviewAssignmentFactory(review_request=review_req, state_id='assigned', assigned_on=review_req.time, reviewer=reviewer.email_set.first()) - RoleFactory(name_id='secr', group=review_req.team, person__user__username='reviewsecretary') - ReviewerSettingsFactory(team=review_req.team, person=reviewer, remind_days_open_reviews=1) - - empty_outbox() - today = datetime.date.today() - log = send_reminder_all_open_reviews(today) - - self.assertEqual(len(outbox), 1) - self.assertTrue(reviewer.email_address() in outbox[0]["To"]) - self.assertEqual(outbox[0]["Subject"], "Reminder: you have 1 open review assignment") - message = get_payload_text(outbox[0]) - self.assertTrue(review_req.team.acronym in message) - self.assertTrue('you have 1 open review' in message) - self.assertTrue(review_req.doc.name in message) - self.assertTrue(review_req.deadline.strftime('%Y-%m-%d') in message) - self.assertEqual(len(log), 1) - self.assertTrue(reviewer.email_address() in log[0]) - self.assertTrue('1 open review' in log[0]) - - def test_send_reminder_unconfirmed_assignments(self): - review_req = ReviewRequestFactory(state_id='assigned') - reviewer = RoleFactory(name_id='reviewer', group=review_req.team, person__user__username='reviewer').person - ReviewAssignmentFactory(review_request=review_req, state_id='assigned', assigned_on=review_req.time, reviewer=reviewer.email_set.first()) - RoleFactory(name_id='secr', group=review_req.team, person__user__username='reviewsecretary') - today = datetime.date.today() - - # By default, these reminders are disabled for all teams. - empty_outbox() - log = send_reminder_unconfirmed_assignments(today) - self.assertEqual(len(outbox), 0) - self.assertFalse(log) - - ReviewTeamSettings.objects.update(remind_days_unconfirmed_assignments=1) - - empty_outbox() - log = send_reminder_unconfirmed_assignments(today) - self.assertEqual(len(outbox), 1) - self.assertIn(reviewer.email_address(), outbox[0]["To"]) - self.assertEqual(outbox[0]["Subject"], "Reminder: you have not responded to a review assignment") - message = get_payload_text(outbox[0]) - self.assertIn(review_req.team.acronym, message) - self.assertIn('accept or reject the assignment on', message) - self.assertIn(review_req.doc.name, message) - self.assertEqual(len(log), 1) - self.assertIn(reviewer.email_address(), log[0]) - self.assertIn('not accepted/rejected review assignment', log[0]) - class BulkAssignmentTests(TestCase): diff --git a/ietf/review/tests.py b/ietf/review/tests.py index e33148d86..7dcbefe17 100644 --- a/ietf/review/tests.py +++ b/ietf/review/tests.py @@ -1,10 +1,17 @@ # Copyright The IETF Trust 2019-2020, All Rights Reserved # -*- coding: utf-8 -*- +import datetime - -from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory +from ietf.group.factories import RoleFactory +from ietf.utils.mail import empty_outbox, get_payload_text, outbox from ietf.utils.test_utils import TestCase, reload_db_objects +from .factories import ReviewAssignmentFactory, ReviewRequestFactory, ReviewerSettingsFactory from .mailarch import hash_list_message_id +from .models import ReviewerSettings, ReviewSecretarySettings, ReviewTeamSettings, UnavailablePeriod +from .utils import (email_secretary_reminder, review_assignments_needing_secretary_reminder, + email_reviewer_reminder, review_assignments_needing_reviewer_reminder, + send_reminder_unconfirmed_assignments, send_review_reminder_overdue_assignment, + send_reminder_all_open_reviews, send_unavailability_period_ending_reminder) class HashTest(TestCase): @@ -63,3 +70,434 @@ class ReviewAssignmentTest(TestCase): assignment.save() review_req = reload_db_objects(review_req) self.assertEqual(review_req.state_id, 'withdrawn') + + +class ReviewAssignmentReminderTests(TestCase): + today = datetime.date.today() + deadline = today + datetime.timedelta(days=6) + + def setUp(self): + super().setUp() + self.review_req = ReviewRequestFactory( + state_id='assigned', + deadline=self.deadline, + ) + self.team = self.review_req.team + self.reviewer = RoleFactory( + name_id='reviewer', + group=self.team, + person__user__username='reviewer', + ).person + self.assignment = ReviewAssignmentFactory( + review_request=self.review_req, + state_id='assigned', + assigned_on=self.review_req.time, + reviewer=self.reviewer.email_set.first(), + ) + + def make_secretary(self, username, remind_days=None): + secretary_role = RoleFactory( + name_id='secr', + group=self.team, + person__user__username=username, + ) + ReviewSecretarySettings.objects.create( + team=self.team, + person=secretary_role.person, + remind_days_before_deadline=remind_days, + ) + return secretary_role + + def make_non_secretary(self, username, remind_days=None): + """Make a non-secretary role that has a ReviewSecretarySettings + + This is a little odd, but might come up if an ex-secretary takes on another role and still + has a ReviewSecretarySettings record. + """ + role = RoleFactory( + name_id='reviewer', + group=self.team, + person__user__username=username, + ) + ReviewSecretarySettings.objects.create( + team=self.team, + person=role.person, + remind_days_before_deadline=remind_days, + ) + return role + + def test_review_assignments_needing_secretary_reminder(self): + """Notification sent to multiple secretaries""" + # Set up two secretaries with the same remind_days one with a different, and one with None. + secretary_roles = [ + self.make_secretary(username='reviewsecretary0', remind_days=6), + self.make_secretary(username='reviewsecretary1', remind_days=6), + self.make_secretary(username='reviewsecretary2', remind_days=5), + self.make_secretary(username='reviewsecretary3', remind_days=None), # never notified + ] + self.make_non_secretary(username='nonsecretary', remind_days=6) # never notified + + # Check from more than remind_days before the deadline all the way through the day before. + # Should only get reminders on the expected days. + self.assertCountEqual( + review_assignments_needing_secretary_reminder(self.deadline - datetime.timedelta(days=7)), + [], + 'No reminder needed when deadline is more than remind_days away', + ) + self.assertCountEqual( + review_assignments_needing_secretary_reminder(self.deadline - datetime.timedelta(days=6)), + [(self.assignment, secretary_roles[0]), (self.assignment, secretary_roles[1])], + 'Reminders needed for all secretaries when deadline is exactly remind_days away', + ) + self.assertCountEqual( + review_assignments_needing_secretary_reminder(self.deadline - datetime.timedelta(days=5)), + [(self.assignment, secretary_roles[2])], + 'Reminder needed when deadline is exactly remind_days away', + ) + for days in range(1, 5): + self.assertCountEqual( + review_assignments_needing_secretary_reminder(self.deadline - datetime.timedelta(days=days)), + [], + f'No reminder needed when deadline is less than remind_days away (tried {days})', + ) + + def test_email_secretary_reminder_emails_secretaries(self): + """Secretary review assignment reminders are sent to secretaries""" + secretary_role = self.make_secretary(username='reviewsecretary') + # create a couple other roles for the team to check that only the requested secretary is reminded + self.make_secretary(username='ignoredsecretary') + self.make_non_secretary(username='nonsecretary') + + empty_outbox() + email_secretary_reminder(self.assignment, secretary_role) + self.assertEqual(len(outbox), 1) + msg = outbox[0] + text = get_payload_text(msg) + self.assertIn(secretary_role.email.address, msg['to']) + self.assertIn(self.review_req.doc.name, msg['subject']) + self.assertIn(self.review_req.doc.name, text) + self.assertIn(self.team.acronym, msg['subject']) + self.assertIn(self.team.acronym, text) + + def test_review_assignments_needing_reviewer_reminder(self): + # method should find lists of assignments + reviewer_settings = ReviewerSettings.objects.create( + team=self.team, + person=self.reviewer, + remind_days_before_deadline=6, + ) + + # Give this reviewer another team with a review to be sure + # we don't have cross-talk between teams. + second_req = ReviewRequestFactory(state_id='assigned', deadline=self.deadline) + second_team = second_req.team + second_assignment = ReviewAssignmentFactory( + review_request=second_req, + state_id='assigned', + assigned_on=second_req.time, + reviewer=self.reviewer.email(), + ) + ReviewerSettingsFactory( + team=second_team, + person=self.reviewer, + remind_days_before_deadline=5, + ) + + self.assertCountEqual( + review_assignments_needing_reviewer_reminder(self.deadline - datetime.timedelta(days=7)), + [], + 'No reminder needed when deadline is more than remind_days away' + ) + self.assertCountEqual( + review_assignments_needing_reviewer_reminder(self.deadline - datetime.timedelta(days=6)), + [self.assignment], + 'Reminder needed when deadline is exactly remind_days away', + ) + self.assertCountEqual( + review_assignments_needing_reviewer_reminder(self.deadline - datetime.timedelta(days=5)), + [second_assignment], + 'Reminder needed for other assignment' + ) + self.assertCountEqual( + review_assignments_needing_reviewer_reminder(self.deadline - datetime.timedelta(days=4)), + [], + 'No reminder needed when deadline is less than remind_days away' + ) + + # should never send a reminder when disabled + reviewer_settings.remind_days_before_deadline = None + reviewer_settings.save() + second_assignment.delete() # get rid of this one for the second test + + # test over a range that includes when we *did* send a reminder above + for days in range(1, 8): + self.assertCountEqual( + review_assignments_needing_reviewer_reminder(self.deadline - datetime.timedelta(days=days)), + [], + f'No reminder should be sent when reminders are disabled (sent for days={days})', + ) + + def test_email_review_reminder_emails_reviewers(self): + """Reviewer assignment reminders are sent to the reviewers""" + empty_outbox() + email_reviewer_reminder(self.assignment) + self.assertEqual(len(outbox), 1) + msg = outbox[0] + text = get_payload_text(msg) + self.assertIn(self.reviewer.email_address(), msg['to']) + self.assertIn(self.review_req.doc.name, msg['subject']) + self.assertIn(self.review_req.doc.name, text) + self.assertIn(self.team.acronym, msg['subject']) + + def test_send_reminder_unconfirmed_assignments(self): + """Unconfirmed assignment reminders are sent to reviewer and team secretary""" + assigned_on = self.assignment.assigned_on.date() + secretaries = [ + self.make_secretary(username='reviewsecretary0').person, + self.make_secretary(username='reviewsecretary1').person, + ] + + # assignments that should be ignored (will result in extra emails being sent if not) + ReviewAssignmentFactory( + review_request=self.review_req, + state_id='accepted', + assigned_on=self.review_req.time, + ) + ReviewAssignmentFactory( + review_request=self.review_req, + state_id='completed', + assigned_on=self.review_req.time, + ) + ReviewAssignmentFactory( + review_request=self.review_req, + state_id='rejected', + assigned_on=self.review_req.time, + ) + + # Create a second review for a different team to test for cross-talk between teams. + ReviewAssignmentFactory( + state_id='completed', # something that does not need a reminder + reviewer=self.reviewer.email(), + ) + + # By default, these reminders are disabled for all teams. + ReviewTeamSettings.objects.update(remind_days_unconfirmed_assignments=1) + + empty_outbox() + log = send_reminder_unconfirmed_assignments(assigned_on + datetime.timedelta(days=1)) + self.assertEqual(len(outbox), 1) + self.assertIn(self.reviewer.email_address(), outbox[0]["To"]) + for secretary in secretaries: + self.assertIn( + secretary.email_address(), + outbox[0]["Cc"], + f'Secretary {secretary.user.username} was not copied on the reminder', + ) + self.assertEqual(outbox[0]["Subject"], "Reminder: you have not responded to a review assignment") + message = get_payload_text(outbox[0]) + self.assertIn(self.team.acronym, message) + self.assertIn('accept or reject the assignment on', message) + self.assertIn(self.review_req.doc.name, message) + self.assertEqual(len(log), 1) + self.assertIn(self.reviewer.email_address(), log[0]) + self.assertIn('not accepted/rejected review assignment', log[0]) + + def test_send_reminder_unconfirmed_assignments_respects_remind_days(self): + """Unconfirmed assignment reminders should respect the team settings""" + assigned_on = self.assignment.assigned_on.date() + + # By default, these reminders are disabled for all teams. + empty_outbox() + for days in range(10): + send_reminder_unconfirmed_assignments(assigned_on + datetime.timedelta(days=days)) + self.assertEqual(len(outbox), 0) + + # expect a notification every day except the day of assignment + ReviewTeamSettings.objects.update(remind_days_unconfirmed_assignments=1) + send_reminder_unconfirmed_assignments(assigned_on + datetime.timedelta(days=0)) + self.assertEqual(len(outbox), 0) # no message + send_reminder_unconfirmed_assignments(assigned_on + datetime.timedelta(days=1)) + self.assertEqual(len(outbox), 1) # one new message + send_reminder_unconfirmed_assignments(assigned_on + datetime.timedelta(days=2)) + self.assertEqual(len(outbox), 2) # one new message + send_reminder_unconfirmed_assignments(assigned_on + datetime.timedelta(days=3)) + self.assertEqual(len(outbox), 3) # one new message + + # expect a notification every other day + empty_outbox() + ReviewTeamSettings.objects.update(remind_days_unconfirmed_assignments=2) + send_reminder_unconfirmed_assignments(assigned_on + datetime.timedelta(days=0)) + self.assertEqual(len(outbox), 0) # no message + send_reminder_unconfirmed_assignments(assigned_on + datetime.timedelta(days=1)) + self.assertEqual(len(outbox), 0) # no message + send_reminder_unconfirmed_assignments(assigned_on + datetime.timedelta(days=2)) + self.assertEqual(len(outbox), 1) # one new message + send_reminder_unconfirmed_assignments(assigned_on + datetime.timedelta(days=3)) + self.assertEqual(len(outbox), 1) # no new message + send_reminder_unconfirmed_assignments(assigned_on + datetime.timedelta(days=4)) + self.assertEqual(len(outbox), 2) # one new message + send_reminder_unconfirmed_assignments(assigned_on + datetime.timedelta(days=5)) + self.assertEqual(len(outbox), 2) # no new message + send_reminder_unconfirmed_assignments(assigned_on + datetime.timedelta(days=6)) + self.assertEqual(len(outbox), 3) # no new message + + def test_send_unavailability_period_ending_reminder(self): + secretary = self.make_secretary(username='reviewsecretary') + empty_outbox() + today = datetime.date.today() + UnavailablePeriod.objects.create( + team=self.team, + person=self.reviewer, + start_date=today - datetime.timedelta(days=40), + end_date=today + datetime.timedelta(days=3), + availability="unavailable", + ) + UnavailablePeriod.objects.create( + team=self.team, + person=self.reviewer, + # This object should be ignored, length is too short + start_date=today - datetime.timedelta(days=20), + end_date=today + datetime.timedelta(days=3), + availability="unavailable", + ) + UnavailablePeriod.objects.create( + team=self.team, + person=self.reviewer, + start_date=today - datetime.timedelta(days=40), + # This object should be ignored, end date is too far away + end_date=today + datetime.timedelta(days=4), + availability="unavailable", + ) + UnavailablePeriod.objects.create( + team=self.team, + person=self.reviewer, + # This object should be ignored, end date is too close + start_date=today - datetime.timedelta(days=40), + end_date=today + datetime.timedelta(days=2), + availability="unavailable", + ) + log = send_unavailability_period_ending_reminder(today) + + self.assertEqual(len(outbox), 1) + self.assertTrue(self.reviewer.email_address() in outbox[0]["To"]) + self.assertTrue(secretary.person.email_address() in outbox[0]["To"]) + message = get_payload_text(outbox[0]) + self.assertTrue(self.reviewer.name in message) + self.assertTrue(self.team.acronym in message) + self.assertEqual(len(log), 1) + self.assertTrue(self.reviewer.name in log[0]) + self.assertTrue(self.team.acronym in log[0]) + + def test_send_review_reminder_overdue_assignment(self): + """An overdue assignment reminder should be sent to the secretary + + This tests that a second set of assignments for the same reviewer but a different + review team does not cause cross-talk between teams. To do this, it removes the + ReviewTeamSettings instance for the second review team. At the moment, this has + the effect of disabling these reminders. This is a bit of a hack, because I'm not + sure that review teams without the ReviewTeamSettings should exist. It has the + needed effect but might require rethinking in the future. + """ + secretary = self.make_secretary(username='reviewsecretary') + + # Set the remind_date to be exactly one grace period after self.deadline + remind_date = self.deadline + datetime.timedelta(days=5) + # Create a second request for a second team that will not be sent reminders + second_team = ReviewAssignmentFactory( + review_request__state_id='assigned', + review_request__deadline=self.deadline, + state_id='assigned', + assigned_on=self.deadline, + reviewer=self.reviewer.email_set.first(), + ).review_request.team + second_team.reviewteamsettings.delete() # prevent it from being sent reminders + + # An assignment that is not yet overdue + not_overdue = remind_date + datetime.timedelta(days=1) + ReviewAssignmentFactory( + review_request__team=self.team, + review_request__state_id='assigned', + review_request__deadline=not_overdue, + state_id='assigned', + assigned_on=not_overdue, + reviewer=self.reviewer.email_set.first(), + ) + ReviewAssignmentFactory( + review_request__team=second_team, + review_request__state_id='assigned', + review_request__deadline=not_overdue, + state_id='assigned', + assigned_on=not_overdue, + reviewer=self.reviewer.email_set.first(), + ) + + # An assignment that is overdue but is not past the grace period + in_grace_period = remind_date - datetime.timedelta(days=1) + ReviewAssignmentFactory( + review_request__team=self.team, + review_request__state_id='assigned', + review_request__deadline=in_grace_period, + state_id='assigned', + assigned_on=in_grace_period, + reviewer=self.reviewer.email_set.first(), + ) + ReviewAssignmentFactory( + review_request__team=second_team, + review_request__state_id='assigned', + review_request__deadline=in_grace_period, + state_id='assigned', + assigned_on=in_grace_period, + reviewer=self.reviewer.email_set.first(), + ) + + empty_outbox() + log = send_review_reminder_overdue_assignment(remind_date) + self.assertEqual(len(log), 1) + + self.assertEqual(len(outbox), 1) + self.assertTrue(secretary.person.email_address() in outbox[0]["To"]) + self.assertEqual(outbox[0]["Subject"], "1 Overdue review for team {}".format(self.team.acronym)) + message = get_payload_text(outbox[0]) + self.assertIn( + self.team.acronym + ' has 1 accepted or assigned review overdue by at least 5 days.', + message, + ) + self.assertIn('Review of {} by {}'.format(self.review_req.doc.name, self.reviewer.plain_name()), message) + self.assertEqual(len(log), 1) + self.assertIn(secretary.person.email_address(), log[0]) + self.assertIn('1 overdue review', log[0]) + + def test_send_reminder_all_open_reviews(self): + self.make_secretary(username='reviewsecretary') + ReviewerSettingsFactory(team=self.team, person=self.reviewer, remind_days_open_reviews=1) + + # Create another assignment for this reviewer in a different team. + # Configure so that a reminder should not be sent for the date we test. It should not + # be included in the reminder that's sent - only one open review assignment should be + # reported. + second_req = ReviewRequestFactory(state_id='assigned', deadline=self.deadline) + second_team = second_req.team + ReviewAssignmentFactory( + review_request=second_req, + state_id='assigned', + assigned_on=second_req.time, + reviewer=self.reviewer.email(), + ) + ReviewerSettingsFactory(team=second_team, person=self.reviewer, remind_days_open_reviews=13) + + empty_outbox() + today = datetime.date.today() + log = send_reminder_all_open_reviews(today) + + self.assertEqual(len(outbox), 1) + self.assertTrue(self.reviewer.email_address() in outbox[0]["To"]) + self.assertEqual(outbox[0]["Subject"], "Reminder: you have 1 open review assignment") + message = get_payload_text(outbox[0]) + self.assertTrue(self.team.acronym in message) + self.assertTrue('you have 1 open review' in message) + self.assertTrue(self.review_req.doc.name in message) + self.assertTrue(self.review_req.deadline.strftime('%Y-%m-%d') in message) + self.assertEqual(len(log), 1) + self.assertTrue(self.reviewer.email_address() in log[0]) + self.assertTrue('1 open review' in log[0]) + diff --git a/ietf/review/utils.py b/ietf/review/utils.py index a4c905f2a..67d5d41bb 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -702,7 +702,7 @@ def get_default_filter_re(person): return '^draft-(%s|%s)-.*$' % ( person.last_name().lower(), '|'.join(['ietf-%s' % g.acronym for g in groups_to_avoid])) -def send_unavaibility_period_ending_reminder(remind_date): +def send_unavailability_period_ending_reminder(remind_date): reminder_days = 3 end_date = remind_date + datetime.timedelta(days=reminder_days) min_start_date = end_date - datetime.timedelta(days=30) @@ -771,6 +771,7 @@ def send_reminder_all_open_reviews(remind_date): assignments = ReviewAssignment.objects.filter( state__in=("assigned", "accepted"), reviewer__person=reviewer_settings.person, + review_request__team=reviewer_settings.team, ) if not assignments: continue @@ -800,14 +801,10 @@ def send_reminder_unconfirmed_assignments(remind_date): accepted or rejected, if enabled in ReviewTeamSettings. """ log = [] - days_since_origin = (remind_date - ORIGIN_DATE_PERIODIC_REMINDERS).days relevant_review_team_settings = ReviewTeamSettings.objects.filter( remind_days_unconfirmed_assignments__isnull=False) for review_team_settings in relevant_review_team_settings: - if days_since_origin % review_team_settings.remind_days_unconfirmed_assignments != 0: - continue - assignments = ReviewAssignment.objects.filter( state='assigned', review_request__team=review_team_settings.group, @@ -816,6 +813,9 @@ def send_reminder_unconfirmed_assignments(remind_date): continue for assignment in assignments: + days_old = (remind_date - assignment.assigned_on.date()).days + if days_old == 0 or (days_old % review_team_settings.remind_days_unconfirmed_assignments) != 0: + continue # skip those created today or not due for a reminder today to = assignment.reviewer.formatted_email() subject = "Reminder: you have not responded to a review assignment" domain = Site.objects.get_current().domain @@ -823,19 +823,33 @@ def send_reminder_unconfirmed_assignments(remind_date): "name": assignment.review_request.doc.name, "request_id": assignment.review_request.pk }) + cc = [secr_role.formatted_email() + for secr_role in assignment.review_request.team.role_set.filter(name__slug='secr')] - send_mail(None, to, None, subject, "review/reviewer_reminder_unconfirmed_assignments.txt", { - "review_request_url": "https://{}{}".format(domain, review_request_url), - "assignment": assignment, - "team": assignment.review_request.team, - "remind_days": review_team_settings.remind_days_unconfirmed_assignments, - }) + send_mail( + request=None, + to=to, + cc=cc, + frm=None, + subject=subject, + template="review/reviewer_reminder_unconfirmed_assignments.txt", + context={ + "review_request_url": "https://{}{}".format(domain, review_request_url), + "assignment": assignment, + "team": assignment.review_request.team, + "remind_days": review_team_settings.remind_days_unconfirmed_assignments, + }, + ) log.append("Emailed reminder to {} about not accepted/rejected review assignment {}".format(to, assignment.pk)) return log def review_assignments_needing_reviewer_reminder(remind_date): + """Get review assignments needing reviewer reminders + + Returns a queryset of ReviewAssignments whose reviewers should be notified. + """ assignment_qs = ReviewAssignment.objects.filter( state__in=("assigned", "accepted"), reviewer__person__reviewersettings__remind_days_before_deadline__isnull=False, @@ -876,23 +890,44 @@ def email_reviewer_reminder(assignment): }) def review_assignments_needing_secretary_reminder(remind_date): + """Find ReviewAssignments whose secretary should be sent a reminder today""" + # Get ReviewAssignments for teams whose secretaries have a non-null remind_days_before_deadline + # setting. assignment_qs = ReviewAssignment.objects.filter( state__in=("assigned", "accepted"), + review_request__team__role__name__slug='secr', review_request__team__role__person__reviewsecretarysettings__remind_days_before_deadline__isnull=False, review_request__team__role__person__reviewsecretarysettings__team=F("review_request__team"), ).exclude( reviewer=None ).values_list("pk", "review_request__deadline", "review_request__team__role", "review_request__team__role__person__reviewsecretarysettings__remind_days_before_deadline").distinct() - assignment_pks = {} + # For each assignment, find all secretaries who should be reminded today + assignment_pks = set() + secretary_pks = set() + notifications = [] for a_pk, deadline, secretary_role_pk, remind_days in assignment_qs: if (deadline - remind_date).days == remind_days: - assignment_pks[a_pk] = secretary_role_pk + notifications.append((a_pk, secretary_role_pk)) + assignment_pks.add(a_pk) + secretary_pks.add(secretary_role_pk) - review_assignments = { a.pk: a for a in ReviewAssignment.objects.filter(pk__in=list(assignment_pks.keys())).select_related("reviewer", "reviewer__person", "state", "review_request__team") } - secretary_roles = { r.pk: r for r in Role.objects.filter(pk__in=list(assignment_pks.values())).select_related("email", "person") } + review_assignments = { + a.pk: a + for a in ReviewAssignment.objects.filter(pk__in=assignment_pks).select_related( + "reviewer", "reviewer__person", "state", "review_request__team" + ) + } + secretary_roles = { + r.pk: r + for r in Role.objects.filter(pk__in=secretary_pks).select_related("email", "person") + } + + return [ + (review_assignments[a_pk], secretary_roles[secretary_role_pk]) + for a_pk, secretary_role_pk in notifications + ] - return [ (review_assignments[a_pk], secretary_roles[secretary_role_pk]) for a_pk, secretary_role_pk in assignment_pks.items() ] def email_secretary_reminder(assignment, secretary_role): review_request = assignment.review_request @@ -912,7 +947,7 @@ def email_secretary_reminder(assignment, secretary_role): settings = ReviewSecretarySettings.objects.filter(person=secretary_role.person_id, team=team).first() remind_days = settings.remind_days_before_deadline if settings else 0 - send_mail(None, [assignment.reviewer.formatted_email()], None, subject, "review/secretary_reminder.txt", { + send_mail(None, [secretary_role.email.formatted_email()], None, subject, "review/secretary_reminder.txt", { "review_request_url": "https://{}{}".format(domain, request_url), "settings_url": "https://{}{}".format(domain, settings_url), "review_request": review_request,