diff --git a/ietf/bin/send-review-reminders b/ietf/bin/send-review-reminders index c662da38d..eb21325fa 100755 --- a/ietf/bin/send-review-reminders +++ b/ietf/bin/send-review-reminders @@ -19,18 +19,19 @@ django.setup() import datetime from ietf.review.utils import ( - review_requests_needing_reviewer_reminder, email_reviewer_reminder, - review_requests_needing_secretary_reminder, email_secretary_reminder, + review_assignments_needing_reviewer_reminder, email_reviewer_reminder, + review_assignments_needing_secretary_reminder, email_secretary_reminder, ) today = datetime.date.today() -for review_req in review_requests_needing_reviewer_reminder(today): - email_reviewer_reminder(review_req) - for review_assignment in review_req.reviewassignment_set.all(): - print("Emailed reminder to {} for review of {} in {} (req. id {})".format(review_assignment.reviewer.address, review_req.doc_id, review_req.team.acronym, review_req.pk)) +for assignment in review_assignments_needing_reviewer_reminder(today): + email_reviewer_reminder(assignment.review_request) + for review_assignment in assignments.review_req.reviewassignment_set.all(): + print("Emailed reminder to {} for review of {} in {} (req. id {})".format(review_assignment.reviewer.address, assignment.review_req.doc_id, assignment.review_req.team.acronym, assignment.review_req.pk)) -for review_req, secretary_role in review_requests_needing_secretary_reminder(today): - email_secretary_reminder(review_req, secretary_role) - print("Emailed reminder to {} for review of {} in {} (req. id {})".format(review_req.secretary_role.email.address, review_req.doc_id, review_req.team.acronym, review_req.pk)) +for assignment, secretary_role in review_assignments_needing_secretary_reminder(today): + email_secretary_reminder(assignment.review_request, secretary_role) + review_req = assignment.review_request + print("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)) diff --git a/ietf/dbtemplate/migrations/0004_adjust_assignment_email_summary_templates.py b/ietf/dbtemplate/migrations/0004_adjust_assignment_email_summary_templates.py new file mode 100644 index 000000000..0c4463a74 --- /dev/null +++ b/ietf/dbtemplate/migrations/0004_adjust_assignment_email_summary_templates.py @@ -0,0 +1,289 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-03-13 13:41 +from __future__ import unicode_literals + +from django.db import migrations + +def forward(apps, schema_editor): + DBTemplate = apps.get_model('dbtemplate','DBTemplate') + + DBTemplate.objects.filter(pk=182).update(content="""{% autoescape off %}Subject: Open review assignments in {{group.acronym}} + +The following reviewers have assignments:{% for r in review_assignments %}{% ifchanged r.section %} + +{{r.section}} + +{% if r.section == 'Early review requests:' %}Reviewer Due Draft{% else %}Reviewer LC end Draft{% endif %}{% endifchanged %} +{{ r.reviewer.person.plain_name|ljust:"22" }} {% if r.section == 'Early review requests:' %}{{ r.review_request.deadline|date:"Y-m-d" }}{% else %}{{ r.lastcall_ends|default:"None " }}{% endif %} {{ r.review_request.doc_id }}-{% if r.review_request..requested_rev %}{{ r.review_request.requested_rev }}{% else %}{{ r.review_request..doc.rev }}{% endif %} {{ r.earlier_review_mark }}{% endfor %} + +* Other revision previously reviewed +** This revision already reviewed + +{% if rotation_list %}Next in the reviewer rotation: + +{% for p in rotation_list %} {{ p }} +{% endfor %}{% endif %}{% endautoescape %} +""") + + DBTemplate.objects.filter(pk=183).update(content="""{% autoescape off %}Subject: Review Assignments + +Hi all, + +The following reviewers have assignments:{% for r in review_assignments %}{% ifchanged r.section %} + +{{r.section}} + +{% if r.section == 'Early review requests:' %}Reviewer Due Draft{% else %}Reviewer Type LC end Draft{% endif %}{% endifchanged %} +{{ r.reviewer.person.plain_name|ljust:"22" }} {% if r.section == 'Early review requests:' %}{{ r.review_request.deadline|date:"Y-m-d" }}{% else %}{{r.review_request.type.name|ljust:"10"}}{{ r.lastcall_ends|default:"None " }}{% endif %} {{ r.review_request.doc_id }}-{% if r.review_request.requested_rev %}{{ r.review_request.requested_rev }}{% else %}{{ r.review_request.doc.rev }}{% endif %}{% if r.earlier_review_mark %} {{ r.earlier_review_mark }}{% endif %}{% endfor %} + +* Other revision previously reviewed +** This revision already reviewed + +{% if rotation_list %}Next in the reviewer rotation: + +{% for p in rotation_list %} {{ p }} +{% endfor %}{% endif %} +The LC and Telechat review templates are included below: +------------------------------------------------------- + +-- Begin LC Template -- +I am the assigned Gen-ART reviewer for this draft. The General Area +Review Team (Gen-ART) reviews all IETF documents being processed +by the IESG for the IETF Chair. Please treat these comments just +like any other last call comments. + +For more information, please see the FAQ at + +. + +Document: +Reviewer: +Review Date: +IETF LC End Date: +IESG Telechat date: (if known) + +Summary: + +Major issues: + +Minor issues: + +Nits/editorial comments: + +-- End LC Template -- + +-- Begin Telechat Template -- +I am the assigned Gen-ART reviewer for this draft. The General Area +Review Team (Gen-ART) reviews all IETF documents being processed +by the IESG for the IETF Chair. Please wait for direction from your +document shepherd or AD before posting a new version of the draft. + +For more information, please see the FAQ at + +. + +Document: +Reviewer: +Review Date: +IETF LC End Date: +IESG Telechat date: (if known) + +Summary: + +Major issues: + +Minor issues: + +Nits/editorial comments: + +-- End Telechat Template -- +{% endautoescape %} + +""") + + DBTemplate.objects.filter(pk=184).update(content="""{% autoescape off %}Subject: Assignments + +Review instructions and related resources are at: +http://tools.ietf.org/area/sec/trac/wiki/SecDirReview{% for r in review_assignments %}{% ifchanged r.section %} + +{{r.section}} + +{% if r.section == 'Early review requests:' %}Reviewer Due Draft{% else %}Reviewer LC end Draft{% endif %}{% endifchanged %} +{{ r.reviewer.person.plain_name|ljust:"22" }}{{ r.earlier_review|yesno:'R, , ' }}{% if r.section == 'Early review requests:' %}{{ r.review_request.deadline|date:"Y-m-d" }}{% else %}{{ r.lastcall_ends|default:"None " }}{% endif %} {{ r.review_request.doc_id }}-{% if r.review_request.requested_rev %}{{ r.review_request.requested_rev }}{% else %}{{ r.review_request.doc.rev }}{% endif %}{% endfor %} + +{% if rotation_list %}Next in the reviewer rotation: + +{% for p in rotation_list %} {{ p }} +{% endfor %}{% endif %}{% endautoescape %} + +""") + + DBTemplate.objects.filter(pk=185).update(content="""{% autoescape off %}Subject: Open review assignments in {{group.acronym}} + +Review instructions and related resources are at: + + +The following reviewers have assignments:{% for r in review_assignments %}{% ifchanged r.section %} + +{{r.section}} + +{% if r.section == 'Early review requests:' %}Reviewer Due Draft{% else %}Reviewer LC end Draft{% endif %}{% endifchanged %} +{{ r.reviewer.person.plain_name|ljust:"22" }} {% if r.section == 'Early review requests:' %}{{ r.review_request.deadline|date:"Y-m-d" }}{% else %}{{ r.lastcall_ends|default:"None " }}{% endif %} {{ r.review_request.doc_id }}-{% if r.review_request.requested_rev %}{{ r.review_request.requested_rev }}{% else %}{{ r.review_request.doc.rev }}{% endif %} {{ r.earlier_review_mark }}{% endfor %} + +* Other revision previously reviewed +** This revision already reviewed + +{% if rotation_list %}Next in the reviewer rotation: + +{% for p in rotation_list %} {{ p }} +{% endfor %}{% endif %}{% endautoescape %} + +""") + + +def reverse(apps, schema_editor): + DBTemplate = apps.get_model('dbtemplate','DBTemplate') + + DBTemplate.objects.filter(pk=182).update(content="""{% autoescape off %}Subject: Open review assignments in {{group.acronym}} + +The following reviewers have assignments:{% for r in review_requests %}{% ifchanged r.section %} + +{{r.section}} + +{% if r.section == 'Early review requests:' %}Reviewer Due Draft{% else %}Reviewer LC end Draft{% endif %}{% endifchanged %} +{{ r.reviewer.person.plain_name|ljust:"22" }} {% if r.section == 'Early review requests:' %}{{ r.deadline|date:"Y-m-d" }}{% else %}{{ r.lastcall_ends|default:"None " }}{% endif %} {{ r.doc_id }}-{% if r.requested_rev %}{{ r.requested_rev }}{% else %}{{ r.doc.rev }}{% endif %} {{ r.earlier_review_mark }}{% endfor %} + +* Other revision previously reviewed +** This revision already reviewed + +{% if rotation_list %}Next in the reviewer rotation: + +{% for p in rotation_list %} {{ p }} +{% endfor %}{% endif %}{% endautoescape %} +""") + + DBTemplate.objects.filter(pk=183).update(content="""{% autoescape off %}Subject: Review Assignments + +Hi all, + +The following reviewers have assignments:{% for r in review_requests %}{% ifchanged r.section %} + +{{r.section}} + +{% if r.section == 'Early review requests:' %}Reviewer Due Draft{% else %}Reviewer Type LC end Draft{% endif %}{% endifchanged %} +{{ r.reviewer.person.plain_name|ljust:"22" }} {% if r.section == 'Early review requests:' %}{{ r.deadline|date:"Y-m-d" }}{% else %}{{r.type.name|ljust:"10"}}{{ r.lastcall_ends|default:"None " }}{% endif %} {{ r.doc_id }}-{% if r.requested_rev %}{{ r.requested_rev }}{% else %}{{ r.doc.rev }}{% endif %}{% if r.earlier_review_mark %} {{ r.earlier_review_mark }}{% endif %}{% endfor %} + +* Other revision previously reviewed +** This revision already reviewed + +{% if rotation_list %}Next in the reviewer rotation: + +{% for p in rotation_list %} {{ p }} +{% endfor %}{% endif %} +The LC and Telechat review templates are included below: +------------------------------------------------------- + +-- Begin LC Template -- +I am the assigned Gen-ART reviewer for this draft. The General Area +Review Team (Gen-ART) reviews all IETF documents being processed +by the IESG for the IETF Chair. Please treat these comments just +like any other last call comments. + +For more information, please see the FAQ at + +. + +Document: +Reviewer: +Review Date: +IETF LC End Date: +IESG Telechat date: (if known) + +Summary: + +Major issues: + +Minor issues: + +Nits/editorial comments: + +-- End LC Template -- + +-- Begin Telechat Template -- +I am the assigned Gen-ART reviewer for this draft. The General Area +Review Team (Gen-ART) reviews all IETF documents being processed +by the IESG for the IETF Chair. Please wait for direction from your +document shepherd or AD before posting a new version of the draft. + +For more information, please see the FAQ at + +. + +Document: +Reviewer: +Review Date: +IETF LC End Date: +IESG Telechat date: (if known) + +Summary: + +Major issues: + +Minor issues: + +Nits/editorial comments: + +-- End Telechat Template -- +{% endautoescape %} + +""") + + DBTemplate.objects.filter(pk=184).update(content="""{% autoescape off %}Subject: Assignments + +Review instructions and related resources are at: +http://tools.ietf.org/area/sec/trac/wiki/SecDirReview{% for r in review_requests %}{% ifchanged r.section %} + +{{r.section}} + +{% if r.section == 'Early review requests:' %}Reviewer Due Draft{% else %}Reviewer LC end Draft{% endif %}{% endifchanged %} +{{ r.reviewer.person.plain_name|ljust:"22" }}{{ r.earlier_review|yesno:'R, , ' }}{% if r.section == 'Early review requests:' %}{{ r.deadline|date:"Y-m-d" }}{% else %}{{ r.lastcall_ends|default:"None " }}{% endif %} {{ r.doc_id }}-{% if r.requested_rev %}{{ r.requested_rev }}{% else %}{{ r.doc.rev }}{% endif %}{% endfor %} + +{% if rotation_list %}Next in the reviewer rotation: + +{% for p in rotation_list %} {{ p }} +{% endfor %}{% endif %}{% endautoescape %} + +""") + + DBTemplate.objects.filter(pk=185).update(content="""{% autoescape off %}Subject: Open review assignments in {{group.acronym}} + +Review instructions and related resources are at: + + +The following reviewers have assignments:{% for r in review_requests %}{% ifchanged r.section %} + +{{r.section}} + +{% if r.section == 'Early review requests:' %}Reviewer Due Draft{% else %}Reviewer LC end Draft{% endif %}{% endifchanged %} +{{ r.reviewer.person.plain_name|ljust:"22" }} {% if r.section == 'Early review requests:' %}{{ r.deadline|date:"Y-m-d" }}{% else %}{{ r.lastcall_ends|default:"None " }}{% endif %} {{ r.doc_id }}-{% if r.requested_rev %}{{ r.requested_rev }}{% else %}{{ r.doc.rev }}{% endif %} {{ r.earlier_review_mark }}{% endfor %} + +* Other revision previously reviewed +** This revision already reviewed + +{% if rotation_list %}Next in the reviewer rotation: + +{% for p in rotation_list %} {{ p }} +{% endfor %}{% endif %}{% endautoescape %} + +""") + + +class Migration(migrations.Migration): + + dependencies = [ + ('dbtemplate', '0003_adjust_review_templates'), + ] + + operations = [ + migrations.RunPython(forward,reverse), + ] diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index 2598979f9..4799afea2 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -414,7 +414,7 @@ class ReviewTests(TestCase): review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut")) rev_role = RoleFactory(group=review_team,person__user__username='reviewer',person__user__email='reviewer@example.com',name_id='reviewer') review_req = ReviewRequestFactory(doc=doc,team=review_team,type_id='early',state_id='assigned',requested_by=rev_role.person,deadline=datetime.datetime.now()+datetime.timedelta(days=20)) - assignment = ReviewAssignmentFactory(review_request=review_req, state_id='requested', reviewer=rev_role.person.email_set.first()) + assignment = ReviewAssignmentFactory(review_request=review_req, state_id='assigned', reviewer=rev_role.person.email_set.first()) url = urlreverse('ietf.doc.views_review.review_request', kwargs={ "name": doc.name, "request_id": review_req.pk }) username = assignment.reviewer.person.user.username diff --git a/ietf/group/forms.py b/ietf/group/forms.py index 1cbc56ee8..635312263 100644 --- a/ietf/group/forms.py +++ b/ietf/group/forms.py @@ -239,8 +239,6 @@ class ManageReviewRequestForm(forms.Form): close_initial = None if review_req.pk is None: close_initial = "no-review-version" - elif review_req.reviewer: - close_initial = "no-response" else: close_initial = "overtaken" diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py index 666beeb40..7bc6c86ac 100644 --- a/ietf/group/tests_review.py +++ b/ietf/group/tests_review.py @@ -13,22 +13,23 @@ from ietf.person.models import Email, Person from ietf.review.models import ReviewRequest, ReviewerSettings, UnavailablePeriod, ReviewSecretarySettings from ietf.review.utils import ( suggested_review_requests_for_team, - review_requests_needing_reviewer_reminder, email_reviewer_reminder, - review_requests_needing_secretary_reminder, email_secretary_reminder, + review_assignments_needing_reviewer_reminder, email_reviewer_reminder, + review_assignments_needing_secretary_reminder, email_secretary_reminder, reviewer_rotation_list, ) -from ietf.name.models import ReviewTypeName, ReviewResultName, ReviewRequestStateName +from ietf.name.models import ReviewTypeName, ReviewResultName, ReviewRequestStateName, ReviewAssignmentStateName import ietf.group.views from ietf.utils.mail import outbox, empty_outbox from ietf.dbtemplate.factories import DBTemplateFactory from ietf.person.factories import PersonFactory, EmailFactory from ietf.doc.factories import DocumentFactory from ietf.group.factories import RoleFactory, ReviewTeamFactory -from ietf.review.factories import ReviewRequestFactory, ReviewerSettingsFactory +from ietf.review.factories import ReviewRequestFactory, ReviewerSettingsFactory, ReviewAssignmentFactory class ReviewTests(TestCase): def test_review_requests(self): - review_req = ReviewRequestFactory(reviewer=EmailFactory()) + review_req = ReviewRequestFactory(state_id='assigned') + assignment = ReviewAssignmentFactory(review_request=review_req, state_id='assigned', reviewer=EmailFactory(), assigned_on = review_req.time) group = review_req.team for url in [urlreverse(ietf.group.views.review_requests, kwargs={ 'acronym': group.acronym }), @@ -36,7 +37,7 @@ class ReviewTests(TestCase): r = self.client.get(url) self.assertEqual(r.status_code, 200) self.assertIn(review_req.doc.name, unicontent(r)) - self.assertIn(unicode(review_req.reviewer.person), unicontent(r)) + self.assertIn(assignment.reviewer.person.__unicode__(), unicontent(r)) url = urlreverse(ietf.group.views.review_requests, kwargs={ 'acronym': group.acronym }) @@ -50,7 +51,8 @@ class ReviewTests(TestCase): self.assertTrue(review_req.doc.name in unicontent(r)) def test_suggested_review_requests(self): - review_req = ReviewRequestFactory() + review_req = ReviewRequestFactory(state_id='assigned') + assignment = ReviewAssignmentFactory(review_request=review_req, state_id='assigned') doc = review_req.doc team = review_req.team @@ -89,22 +91,22 @@ class ReviewTests(TestCase): self.assertEqual(list(suggested_review_requests_for_team(team)), []) # blocked by versioned refusal - review_req.reviewed_rev = doc.rev - review_req.state = ReviewRequestStateName.objects.get(slug="no-review-document") + review_req.state = ReviewRequestStateName.objects.get(slug="no-review-version") review_req.save() self.assertEqual(list(suggested_review_requests_for_team(team)), []) # blocked by completion - review_req.state = ReviewRequestStateName.objects.get(slug="completed") + review_req.state = ReviewRequestStateName.objects.get(slug="assigned") review_req.save() + assignment.state = ReviewAssignmentStateName.objects.get(slug="completed") + assignment.save() self.assertEqual(list(suggested_review_requests_for_team(team)), []) # ... but not to previous version - review_req.reviewed_rev = prev_rev - review_req.state = ReviewRequestStateName.objects.get(slug="completed") - review_req.save() + assignment.reviewed_rev = prev_rev + assignment.save() self.assertEqual(len(suggested_review_requests_for_team(team)), 1) @@ -112,17 +114,19 @@ class ReviewTests(TestCase): team = ReviewTeamFactory() 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,reviewer=reviewer.email()) + review_req1 = ReviewRequestFactory(state_id='completed',team=team) + ReviewAssignmentFactory(review_request = review_req1, reviewer=reviewer.email()) PersonFactory(user__username='plain') - ReviewRequest.objects.create( - doc=review_req1.doc, - team=review_req1.team, - type_id="early", - deadline=datetime.date.today() + datetime.timedelta(days=30), - state_id="accepted", - reviewer=review_req1.reviewer, - requested_by=Person.objects.get(user__username="reviewer"), + ReviewAssignmentFactory( + review_request__doc=review_req1.doc, + review_request__team=review_req1.team, + review_request__type_id="early", + 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 = "accepted", + reviewer=reviewer.email_set.first(), ) UnavailablePeriod.objects.create( @@ -171,167 +175,69 @@ class ReviewTests(TestCase): group = ReviewTeamFactory() reviewer = RoleFactory(name_id='reviewer',group=group,person__user__username='reviewer').person marsperson = RoleFactory(name_id='reviewer',group=group,person=PersonFactory(name=u"Mars Anders Chairman",user__username='marschairman')).person - review_req1 = ReviewRequestFactory(doc__pages=2,doc__shepherd=marsperson.email(),reviewer=reviewer.email(),team=group) + review_req1 = ReviewRequestFactory(doc__pages=2,doc__shepherd=marsperson.email(),team=group) + review_req2 = ReviewRequestFactory(team=group) + review_req3 = ReviewRequestFactory(team=group) RoleFactory(name_id='chair',group=review_req1.doc.group,person=marsperson) doc = review_req1.doc - url = urlreverse(ietf.group.views.manage_review_requests, kwargs={ 'acronym': group.acronym, "assignment_status": "assigned" }) - - login_testing_unauthorized(self, "secretary", url) - - assigned_url = urlreverse(ietf.group.views.manage_review_requests, kwargs={ 'acronym': group.acronym, 'group_type': group.type_id, "assignment_status": "assigned" }) unassigned_url = urlreverse(ietf.group.views.manage_review_requests, kwargs={ 'acronym': group.acronym, 'group_type': group.type_id, "assignment_status": "unassigned" }) - - review_req2 = ReviewRequest.objects.create( - doc=review_req1.doc, - team=review_req1.team, - type_id="early", - deadline=datetime.date.today() + datetime.timedelta(days=30), - state_id="accepted", - reviewer=review_req1.reviewer, - requested_by=Person.objects.get(user__username="reviewer"), - ) - - review_req3 = ReviewRequest.objects.create( - doc=review_req1.doc, - team=review_req1.team, - type_id="early", - deadline=datetime.date.today() + datetime.timedelta(days=30), - state_id="requested", - requested_by=Person.objects.get(user__username="reviewer"), - ) - - # previous reviews - ReviewRequest.objects.create( - time=datetime.datetime.now() - datetime.timedelta(days=100), - requested_by=Person.objects.get(name="(System)"), - doc=doc, - type=ReviewTypeName.objects.get(slug="early"), - team=review_req1.team, - state=ReviewRequestStateName.objects.get(slug="completed"), - result=ReviewResultName.objects.get(slug="ready-nits"), - reviewed_rev="01", - deadline=datetime.date.today() - datetime.timedelta(days=80), - reviewer=review_req1.reviewer, - ) - - ReviewRequest.objects.create( - time=datetime.datetime.now() - datetime.timedelta(days=100), - requested_by=Person.objects.get(name="(System)"), - doc=doc, - type=ReviewTypeName.objects.get(slug="early"), - team=review_req1.team, - state=ReviewRequestStateName.objects.get(slug="completed"), - result=ReviewResultName.objects.get(slug="ready"), - reviewed_rev="01", - deadline=datetime.date.today() - datetime.timedelta(days=80), - reviewer=review_req1.reviewer, - ) + login_testing_unauthorized(self, "secretary", unassigned_url) # Need one more person in review team one so we can test incrementing skip_count without immediately decrementing it another_reviewer = PersonFactory.create(name = u"Extra TestReviewer") # needs to be lexically greater than the exsting one another_reviewer.role_set.create(name_id='reviewer', email=another_reviewer.email(), group=review_req1.team) - + ReviewerSettingsFactory(team=review_req3.team, person = another_reviewer) + yet_another_reviewer = PersonFactory.create(name = u"YetAnotherExtra TestReviewer") # needs to be lexically greater than the exsting one + yet_another_reviewer.role_set.create(name_id='reviewer', email=yet_another_reviewer.email(), group=review_req1.team) + ReviewerSettingsFactory(team=review_req3.team, person = yet_another_reviewer) + # get - r = self.client.get(assigned_url) + r = self.client.get(unassigned_url) self.assertEqual(r.status_code, 200) self.assertTrue(review_req1.doc.name in unicontent(r)) - # can't save assigned: conflict - new_reviewer = Email.objects.get(role__name="reviewer", role__group=group, person__user__username="marschairman") - # provoke conflict by posting bogus data - r = self.client.post(assigned_url, { - "reviewrequest": [str(review_req1.pk), str(review_req2.pk), str(123456)], - - # close - "r{}-existing_reviewer".format(review_req1.pk): "123456", - "r{}-action".format(review_req1.pk): "close", - "r{}-close".format(review_req1.pk): "no-response", - - # assign - "r{}-existing_reviewer".format(review_req2.pk): "123456", - "r{}-action".format(review_req2.pk): "assign", - "r{}-reviewer".format(review_req2.pk): new_reviewer.pk, - - "action": "save-continue", - }) - self.assertEqual(r.status_code, 200) - content = unicontent(r).lower() - self.assertTrue("1 request closed" in content) - self.assertTrue("2 requests changed assignment" in content) - - # can't save unassigned: conflict - r = self.client.post(unassigned_url, { - "reviewrequest": [str(123456)], - "action": "save-continue", - }) - self.assertEqual(r.status_code, 200) - content = unicontent(r).lower() - self.assertTrue("1 request opened" in content) - - # close and reassign assigned - new_reviewer = Email.objects.get(role__name="reviewer", role__group=group, person__user__username="marschairman") - r = self.client.post(assigned_url, { - "reviewrequest": [str(review_req1.pk), str(review_req2.pk)], - - # close - "r{}-existing_reviewer".format(review_req1.pk): review_req1.reviewer_id or "", - "r{}-action".format(review_req1.pk): "close", - "r{}-close".format(review_req1.pk): "no-response", - - # assign - "r{}-existing_reviewer".format(review_req2.pk): review_req2.reviewer_id or "", - "r{}-action".format(review_req2.pk): "assign", - "r{}-reviewer".format(review_req2.pk): new_reviewer.pk, - "r{}-add_skip".format(review_req2.pk) : 1, - - "action": "save", - }) - self.assertEqual(r.status_code, 302) - - # no change on unassigned + # Test that conflicts are detected r = self.client.post(unassigned_url, { "reviewrequest": [str(review_req3.pk)], - # no change - "r{}-existing_reviewer".format(review_req3.pk): review_req3.reviewer_id or "", - "r{}-action".format(review_req3.pk): "", - "r{}-close".format(review_req3.pk): "no-response", - "r{}-reviewer".format(review_req3.pk): "", + "r{}-existing_reviewer".format(review_req3.pk): "", + "r{}-action".format(review_req3.pk): "assign", + "r{}-reviewer".format(review_req3.pk): another_reviewer.email_set.first().pk, + "r{}-add_skip".format(review_req3.pk): 1, + + "action": "save", + }) + self.assertEqual(r.status_code, 200) + content = unicontent(r).lower() + self.assertTrue("2 requests opened" in content) + + r = self.client.post(unassigned_url, { + "reviewrequest": [str(review_req1.pk),str(review_req2.pk),str(review_req3.pk)], + + "r{}-existing_reviewer".format(review_req3.pk): "", + "r{}-action".format(review_req3.pk): "assign", + "r{}-reviewer".format(review_req3.pk): another_reviewer.email_set.first().pk, + "r{}-add_skip".format(review_req3.pk): 1, "action": "save", }) self.assertEqual(r.status_code, 302) - review_req1, review_req2, review_req3 = reload_db_objects(review_req1, review_req2, review_req3) - self.assertEqual(review_req1.state_id, "no-response") - self.assertEqual(review_req2.state_id, "requested") - self.assertEqual(review_req2.reviewer, new_reviewer) - settings = ReviewerSettings.objects.filter(team=review_req2.team, person=new_reviewer.person).first() + review_req3 = reload_db_objects(review_req3) + settings = ReviewerSettings.objects.filter(team=review_req3.team, person=another_reviewer).first() self.assertEqual(settings.skip_next,1) - self.assertEqual(review_req3.state_id, "requested") - - r = self.client.post(assigned_url, { - "reviewrequest": [str(review_req2.pk)], - "r{}-existing_reviewer".format(review_req2.pk): review_req2.reviewer_id or "", - "r{}-action".format(review_req2.pk): "assign", - "r{}-reviewer".format(review_req2.pk): "", - "r{}-add_skip".format(review_req2.pk) : 0, - "action": "save", - }) - self.assertEqual(r.status_code, 302) - review_req2 = reload_db_objects(review_req2) - self.assertEqual(review_req2.state_id, "requested") - self.assertEqual(review_req2.reviewer, None) + self.assertEqual(review_req3.state_id, "assigned") def test_email_open_review_assignments(self): - review_req1 = ReviewRequestFactory(reviewer=EmailFactory(person__user__username='marschairman')) + review_req1 = ReviewRequestFactory() + ReviewAssignmentFactory(review_request=review_req1,reviewer=EmailFactory(person__user__username='marschairman')) DBTemplateFactory.create(path='/group/defaults/email/open_assignments.txt', type_id='django', content = """ {% autoescape off %} Reviewer Deadline Draft - {% for r in review_requests %}{{ r.reviewer.person.plain_name|ljust:"22" }} {{ r.deadline|date:"Y-m-d" }} {{ r.doc_id }}-{% if r.requested_rev %}{{ r.requested_rev }}{% else %}{{ r.doc.rev }}{% endif %} + {% for r in review_assignments %}{{ r.reviewer.person.plain_name|ljust:"22" }} {{ r.review_request.deadline|date:"Y-m-d" }} {{ r.review_request.doc_id }}-{% if r.review_request.requested_rev %}{{ r.review_request.requested_rev }}{% else %}{{ r.review_request.doc.rev }}{% endif %} {% endfor %} {% if rotation_list %}Next in the reviewer rotation: @@ -376,15 +282,14 @@ class ReviewTests(TestCase): def test_change_reviewer_settings(self): reviewer = ReviewerSettingsFactory(person__user__username='reviewer',expertise='Some expertise').person - review_req = ReviewRequestFactory(reviewer=reviewer.email()) - RoleFactory(name_id='reviewer',group=review_req.team,person=review_req.reviewer.person) + review_req = ReviewRequestFactory() + assignment = ReviewAssignmentFactory(review_request=review_req,reviewer=reviewer.email()) + RoleFactory(name_id='reviewer',group=review_req.team,person=assignment.reviewer.person) RoleFactory(name_id='secr',group=review_req.team) - reviewer = review_req.reviewer.person - url = urlreverse(ietf.group.views.change_reviewer_settings, kwargs={ "acronym": review_req.team.acronym, - "reviewer_email": review_req.reviewer_id, + "reviewer_email": assignment.reviewer_id, }) login_testing_unauthorized(self, reviewer.user.username, url) @@ -392,7 +297,7 @@ class ReviewTests(TestCase): url = urlreverse(ietf.group.views.change_reviewer_settings, kwargs={ "group_type": review_req.team.type_id, "acronym": review_req.team.acronym, - "reviewer_email": review_req.reviewer_id, + "reviewer_email": assignment.reviewer_id, }) # get @@ -451,7 +356,7 @@ class ReviewTests(TestCase): msg_content = outbox[0].get_payload(decode=True).decode("utf-8").lower() self.assertTrue(start_date.isoformat(), msg_content) self.assertTrue("indefinite", msg_content) - self.assertEqual(period.reason, "Whimsy") + self.assertEqual(period.reason, "Whimsy") # end unavailable period empty_outbox() @@ -526,8 +431,9 @@ class ReviewTests(TestCase): self.assertEqual(settings.remind_days_before_deadline, 6) def test_review_reminders(self): - review_req = ReviewRequestFactory() + 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) @@ -551,23 +457,23 @@ class ReviewTests(TestCase): review_req.save() # reviewer - needing_reminders = review_requests_needing_reviewer_reminder(today - datetime.timedelta(days=1)) + needing_reminders = review_assignments_needing_reviewer_reminder(today - datetime.timedelta(days=1)) self.assertEqual(list(needing_reminders), []) - needing_reminders = review_requests_needing_reviewer_reminder(today) - self.assertEqual(list(needing_reminders), [review_req]) + needing_reminders = review_assignments_needing_reviewer_reminder(today) + self.assertEqual(list(needing_reminders), [assignment]) - needing_reminders = review_requests_needing_reviewer_reminder(today + datetime.timedelta(days=1)) + needing_reminders = review_assignments_needing_reviewer_reminder(today + datetime.timedelta(days=1)) self.assertEqual(list(needing_reminders), []) # secretary - needing_reminders = review_requests_needing_secretary_reminder(today - datetime.timedelta(days=1)) + needing_reminders = review_assignments_needing_secretary_reminder(today - datetime.timedelta(days=1)) self.assertEqual(list(needing_reminders), []) - needing_reminders = review_requests_needing_secretary_reminder(today) - self.assertEqual(list(needing_reminders), [(review_req, secretary_role)]) + needing_reminders = review_assignments_needing_secretary_reminder(today) + self.assertEqual(list(needing_reminders), [(assignment, secretary_role)]) - needing_reminders = review_requests_needing_secretary_reminder(today + datetime.timedelta(days=1)) + needing_reminders = review_assignments_needing_secretary_reminder(today + datetime.timedelta(days=1)) self.assertEqual(list(needing_reminders), []) # email reviewer diff --git a/ietf/group/urls.py b/ietf/group/urls.py index 6f02b8b27..3a9a70235 100644 --- a/ietf/group/urls.py +++ b/ietf/group/urls.py @@ -41,7 +41,7 @@ info_detail_urls = [ url(r'^archives/$', views.derived_archives), url(r'^photos/$', views.group_photos), url(r'^reviews/$', views.review_requests), - url(r'^reviews/manage/(?Passigned|unassigned)/$', views.manage_review_requests), + url(r'^reviews/manage/(?Punassigned)/$', views.manage_review_requests), url(r'^reviews/email-assignments/$', views.email_open_review_assignments), url(r'^reviewers/$', views.reviewer_overview), url(r'^reviewers/(?P[\w%+-.@]+)/settings/$', views.change_reviewer_settings), diff --git a/ietf/group/utils.py b/ietf/group/utils.py index f504cef21..fd41fecd6 100644 --- a/ietf/group/utils.py +++ b/ietf/group/utils.py @@ -223,7 +223,7 @@ def construct_group_menu_context(request, group, selected, group_type, others): if group.features.has_reviews and can_manage_review_requests_for_team(request.user, group): import ietf.group.views actions.append((u"Manage unassigned reviews", urlreverse(ietf.group.views.manage_review_requests, kwargs=dict(assignment_status="unassigned", **kwargs)))) - actions.append((u"Manage assigned reviews", urlreverse(ietf.group.views.manage_review_requests, kwargs=dict(assignment_status="assigned", **kwargs)))) + #actions.append((u"Manage assigned reviews", urlreverse(ietf.group.views.manage_review_requests, kwargs=dict(assignment_status="assigned", **kwargs)))) if Role.objects.filter(name="secr", group=group, person__user=request.user).exists(): actions.append((u"Secretary settings", urlreverse(ietf.group.views.change_review_secretary_settings, kwargs=kwargs))) diff --git a/ietf/group/views.py b/ietf/group/views.py index 5c96263cc..7a1466f4a 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -1461,23 +1461,6 @@ def manage_review_requests(request, acronym, group_type=None, assignment_status= for r in opened_reqs: review_requests_dict[r].form.add_error(None, "New request.") - for req in review_requests: - existing_reviewer = request.POST.get(req.form.prefix + "-existing_reviewer") - if existing_reviewer is None: - continue - - if existing_reviewer != unicode(req.reviewer_id or ""): - msg = "Assignment was changed." - a = req.form["action"].value() - if a == "assign": - msg += " Didn't assign reviewer." - elif a == "close": - msg += " Didn't close request." - req.form.add_error(None, msg) - req.form.data[req.form.prefix + "-action"] = "" # cancel the action - - newly_assigned += 1 - form_results = [] for req in review_requests: form_results.append(req.form.is_valid()) @@ -1552,37 +1535,35 @@ def email_open_review_assignments(request, acronym, group_type=None): if not can_manage_review_requests_for_team(request.user, group): return HttpResponseForbidden("You do not have permission to perform this action") - review_requests = list(ReviewRequest.objects.filter( - team=group, - state__in=("requested", "accepted"), - ).exclude( - reviewer=None, - ).prefetch_related("reviewer", "type", "state", "doc").distinct().order_by("reviewer","-deadline")) + review_assignments = list(ReviewAssignment.objects.filter( + review_request__team=group, + state__in=("assigned", "accepted"), + ).prefetch_related("reviewer", "review_request__type", "state", "review_request__doc").distinct().order_by("reviewer","-review_request__deadline")) - review_requests.sort(key=lambda r:r.reviewer.person.last_name()+r.reviewer.person.first_name()) + review_assignments.sort(key=lambda r:r.reviewer.person.last_name()+r.reviewer.person.first_name()) - for r in review_requests: - if r.doc.telechat_date(): - r.section = 'For telechat %s' % r.doc.telechat_date().isoformat() + for r in review_assignments: + if r.review_request.doc.telechat_date(): + r.section = 'For telechat %s' % r.review_request.doc.telechat_date().isoformat() r.section_order='0'+r.section - elif r.type_id == 'early': + elif r.review_request.type_id == 'early': r.section = 'Early review requests:' r.section_order='2' else: r.section = 'Last calls:' r.section_order='1' - e = r.doc.latest_event(LastCallDocEvent, type="sent_last_call") + e = r.review_request.doc.latest_event(LastCallDocEvent, type="sent_last_call") r.lastcall_ends = e and e.expires.date().isoformat() - r.earlier_review = ReviewRequest.objects.filter(doc=r.doc,reviewer__in=r.reviewer.person.email_set.all(),state="completed") + r.earlier_review = ReviewAssignment.objects.filter(review_request__doc=r.review_request.doc,reviewer__in=r.reviewer.person.email_set.all(),state="completed") if r.earlier_review: - req_rev = r.requested_rev or r.doc.rev + req_rev = r.review_request.requested_rev or r.review_request.doc.rev earlier_review_rev = r.earlier_review.aggregate(Max('reviewed_rev'))['reviewed_rev__max'] if req_rev == earlier_review_rev: r.earlier_review_mark = '**' else: r.earlier_review_mark = '*' - review_requests.sort(key=lambda r: r.section_order) + review_assignments.sort(key=lambda r: r.section_order) back_url = request.GET.get("next") if not back_url: @@ -1610,7 +1591,7 @@ def email_open_review_assignments(request, acronym, group_type=None): template = DBTemplate.objects.get(path="/group/defaults/email/open_assignments.txt") partial_msg = render_to_string(template.path, { - "review_requests": review_requests, + "review_assignments": review_assignments, "rotation_list": reviewer_rotation_list(group)[:10], "group" : group, }) @@ -1631,7 +1612,7 @@ def email_open_review_assignments(request, acronym, group_type=None): return render(request, 'group/email_open_review_assignments.html', { 'group': group, - 'review_requests': review_requests, + 'review_assignments': review_assignments, 'form': form, 'back_url': back_url, }) @@ -1717,14 +1698,14 @@ def change_reviewer_settings(request, acronym, reviewer_email, group_type=None): # the secretary might need to reassign # assignments, so mention the current ones - review_reqs = ReviewRequest.objects.filter(state__in=["requested", "accepted"], reviewer=reviewer_role.email, team=group) + review_assignments = ReviewAssignment.objects.filter(state__in=["assigned", "accepted"], reviewer=reviewer_role.email, review_request__team=group) msg += "\n\n" - if review_reqs: + if review_assignments: msg += "{} is currently assigned to review:".format(reviewer_role.person) - for r in review_reqs: + for r in review_assignments: msg += "\n\n" - msg += "{} (deadline: {})".format(r.doc_id, r.deadline.isoformat()) + msg += "{} (deadline: {})".format(r.review_request.doc_id, r.review_request.deadline.isoformat()) else: msg += "{} does not have any assignments currently.".format(reviewer_role.person) diff --git a/ietf/review/factories.py b/ietf/review/factories.py index b304cf412..7e7a2480f 100644 --- a/ietf/review/factories.py +++ b/ietf/review/factories.py @@ -44,7 +44,7 @@ class ReviewAssignmentFactory(factory.DjangoModelFactory): model = ReviewAssignment review_request = factory.SubFactory('ietf.review.factories.ReviewRequestFactory') - state_id = 'requested' + state_id = 'assigned' reviewer = factory.SubFactory('ietf.person.factories.EmailFactory') assigned_on = datetime.datetime.now() diff --git a/ietf/review/models.py b/ietf/review/models.py index cf6f997d8..8d9b92cce 100644 --- a/ietf/review/models.py +++ b/ietf/review/models.py @@ -142,15 +142,6 @@ class ReviewRequest(models.Model): def other_completed_requests(self): return self.other_requests().filter(state_id__in=['completed','part-completed']) - #def review_done_time(self): - # # First check if this is completed review having review and if so take time from there. - # if self.review and self.review.time: - # return self.review.time - # # If not, then it is closed review, so it either has event in doc or if not then take - # # time from the request. - # time = self.doc.request_closed_time(self) - # return time if time else self.time - def request_closed_time(self): return self.doc.request_closed_time(self) or self.time diff --git a/ietf/review/utils.py b/ietf/review/utils.py index 1b6eba37c..c31270acf 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -559,7 +559,7 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa # Note that assigning a review no longer unassigns other reviews - review_req.reviewassignment_set.create(state_id='requested', reviewer = reviewer, assigned_on = datetime.datetime.now()) + review_req.reviewassignment_set.create(state_id='assigned', reviewer = reviewer, assigned_on = datetime.datetime.now()) if review_req.state_id != 'assigned': review_req.state_id = 'assigned' review_req.save() @@ -668,7 +668,7 @@ def close_review_request(request, review_req, close_state): state=review_req.state, ) - for assignment in review_req.reviewassignment_set.filter(state_id__in=['requested','accepted']): + for assignment in review_req.reviewassignment_set.filter(state_id__in=['assigned','accepted']): assignment.state_id = 'withdrawn' assignment.save() ReviewAssignmentDocEvent.objects.create( @@ -790,9 +790,10 @@ def suggested_review_requests_for_team(team): and existing.reviewassignment_set.filter(state_id__in=("assigned", "accepted")).exists() and (not existing.requested_rev or existing.requested_rev == request.doc.rev)) request_closed = existing.state_id not in ('requested','assigned') - all_assignments_completed = not existing.reviewassignment_set.filter(state_id__in=('assigned','accepted')).exists() + # at least one assignment was completed for the requested version: + some_assignment_completed = existing.reviewassignment_set.filter(reviewed_rev=existing.requested_rev,state_id='completed').exists() - return any([no_review_document, no_review_rev, pending, request_closed, all_assignments_completed]) + return any([no_review_document, no_review_rev, pending, request_closed, some_assignment_completed]) res = [r for r in requests.itervalues() if not any(blocks(e, r) for e in existing_requests[r.doc_id])] @@ -1061,21 +1062,19 @@ def make_assignment_choices(email_queryset, review_req): return [(r["email"].pk, r["label"]) for r in ranking] -def review_requests_needing_reviewer_reminder(remind_date): - reqs_qs = ReviewRequest.objects.filter( - state__in=("requested", "accepted"), +def review_assignments_needing_reviewer_reminder(remind_date): + assignment_qs = ReviewAssignment.objects.filter( + state__in=("assigned", "accepted"), reviewer__person__reviewersettings__remind_days_before_deadline__isnull=False, - reviewer__person__reviewersettings__team=F("team"), - ).exclude( - reviewer=None - ).values_list("pk", "deadline", "reviewer__person__reviewersettings__remind_days_before_deadline").distinct() + reviewer__person__reviewersettings__team=F("review_request__team"), + ).values_list("pk", "review_request__deadline", "reviewer__person__reviewersettings__remind_days_before_deadline").distinct() - req_pks = [] - for r_pk, deadline, remind_days in reqs_qs: + assignment_pks = [] + for a_pk, deadline, remind_days in assignment_qs: if (deadline - remind_date).days == remind_days: - req_pks.append(r_pk) + assignment_pks.append(a_pk) - return ReviewRequest.objects.filter(pk__in=req_pks).select_related("reviewer", "reviewer__person", "state", "team") + return ReviewAssignment.objects.filter(pk__in=assignment_pks).select_related("reviewer", "reviewer__person", "state", "review_request__team") def email_reviewer_reminder(review_request): team = review_request.team @@ -1102,24 +1101,24 @@ def email_reviewer_reminder(review_request): "remind_days": remind_days, }) -def review_requests_needing_secretary_reminder(remind_date): - reqs_qs = ReviewRequest.objects.filter( - state__in=("requested", "accepted"), - team__role__person__reviewsecretarysettings__remind_days_before_deadline__isnull=False, - team__role__person__reviewsecretarysettings__team=F("team"), +def review_assignments_needing_secretary_reminder(remind_date): + assignment_qs = ReviewAssignment.objects.filter( + state__in=("assigned", "accepted"), + 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", "deadline", "team__role", "team__role__person__reviewsecretarysettings__remind_days_before_deadline").distinct() + ).values_list("pk", "review_request__deadline", "review_request__team__role", "review_request__team__role__person__reviewsecretarysettings__remind_days_before_deadline").distinct() - req_pks = {} - for r_pk, deadline, secretary_role_pk, remind_days in reqs_qs: + assignment_pks = {} + for a_pk, deadline, secretary_role_pk, remind_days in assignment_qs: if (deadline - remind_date).days == remind_days: - req_pks[r_pk] = secretary_role_pk + assignment_pks[a_pk] = secretary_role_pk - review_reqs = { r.pk: r for r in ReviewRequest.objects.filter(pk__in=req_pks.keys()).select_related("reviewer", "reviewer__person", "state", "team") } - secretary_roles = { r.pk: r for r in Role.objects.filter(pk__in=req_pks.values()).select_related("email", "person") } + review_assignments = { a.pk: a for a in ReviewAssignment.objects.filter(pk__in=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=assignment_pks.values()).select_related("email", "person") } - return [ (review_reqs[req_pk], secretary_roles[secretary_role_pk]) for req_pk, secretary_role_pk in req_pks.iteritems() ] + return [ (review_assignments[a_pk], secretary_roles[secretary_role_pk]) for a_pk, secretary_role_pk in assignment_pks.iteritems() ] def email_secretary_reminder(review_request, secretary_role): team = review_request.team diff --git a/ietf/templates/group/email_open_review_assignments.html b/ietf/templates/group/email_open_review_assignments.html index 74dcc5b03..4ac55fc46 100644 --- a/ietf/templates/group/email_open_review_assignments.html +++ b/ietf/templates/group/email_open_review_assignments.html @@ -11,7 +11,7 @@

Email summary of assigned review requests for {{ group.acronym }}

- {% if review_requests %} + {% if review_assignments %}