Skip to content

Commit 871a4b6

Browse files
author
Sasha Romijn
committed
Fix ietf-tools#2217 - Allow submission of unsolicited reviews by secretaries.
- For team secretaries, a button "Submit unsolicited review" will now appear next to "Request review" on the document's main page. - If the secretary is a secretary for multiple teams, they are taken through an intermediate page to select for which team they are submitting their review. - The form is similar (and using the same code) as the usual review completion, with a few extra fields for the review type and reviewer, which would usually already be known. - When submitting the review, a ReviewRequest and ReviewAssignment are automatically created. The assignment is then immediately closed in the usual way. - Other workflows are unchanged. The issues with the review form in ietf-tools#2061 are slightly worse for the unsolicited review scenario, but that will be improved when ietf-tools#2061 is fixed. Commit ready for merge. - Legacy-Id: 16924
1 parent 93797b3 commit 871a4b6

8 files changed

Lines changed: 285 additions & 67 deletions

File tree

‎ietf/doc/tests_review.py‎

Lines changed: 82 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@
2727
from ietf.group.factories import RoleFactory, ReviewTeamFactory
2828
from ietf.group.models import Group
2929
from ietf.message.models import Message
30-
from ietf.name.models import ReviewResultName, ReviewRequestStateName, ReviewAssignmentStateName
30+
from ietf.name.models import ReviewResultName, ReviewRequestStateName, ReviewAssignmentStateName, \
31+
ReviewTypeName
3132
from ietf.person.models import Email, Person
3233
from ietf.review.factories import ReviewRequestFactory, ReviewAssignmentFactory
3334
from ietf.review.models import (ReviewRequest, ReviewerSettings,
@@ -563,7 +564,7 @@ def test_search_mail_archive(self):
563564
assignment = ReviewAssignmentFactory(review_request=review_req, reviewer=rev_role.person.email_set.first(), state_id='accepted')
564565

565566
# test URL construction
566-
query_urls = ietf.review.mailarch.construct_query_urls(review_req)
567+
query_urls = ietf.review.mailarch.construct_query_urls(doc, review_team)
567568
self.assertTrue(review_req.doc.name in query_urls["query_data_url"])
568569

569570
# test parsing
@@ -574,16 +575,21 @@ def test_search_mail_archive(self):
574575
# to work, the module (and not the function) must be
575576
# imported in the view
576577
real_fn = ietf.review.mailarch.construct_query_urls
577-
ietf.review.mailarch.construct_query_urls = lambda review_req, query=None: { "query_data_url": "file://" + os.path.abspath(mbox_path) }
578-
578+
ietf.review.mailarch.construct_query_urls = lambda doc, team, query=None: { "query_data_url": "file://" + os.path.abspath(mbox_path) }
579579
url = urlreverse('ietf.doc.views_review.search_mail_archive', kwargs={ "name": doc.name, "assignment_id": assignment.pk })
580+
url2 = urlreverse('ietf.doc.views_review.search_mail_archive', kwargs={ "name": doc.name, "acronym": review_team.acronym })
580581
login_testing_unauthorized(self, "reviewsecretary", url)
581582

582583
r = self.client.get(url)
583584
self.assertEqual(r.status_code, 200)
584585
messages = r.json()["messages"]
585586
self.assertEqual(len(messages), 2)
586587

588+
r = self.client.get(url2)
589+
self.assertEqual(r.status_code, 200)
590+
messages = r.json()["messages"]
591+
self.assertEqual(len(messages), 2)
592+
587593
today = datetime.date.today()
588594

589595
self.assertEqual(messages[0]["url"], "https://www.example.com/testmessage")
@@ -604,7 +610,7 @@ def test_search_mail_archive(self):
604610
no_result_path = os.path.join(self.review_dir, "mailarch_no_result.html")
605611
with io.open(no_result_path, "w") as f:
606612
f.write('Content-Type: text/html\n\n<html><body><div class="xtr"><div class="xtd no-results">No results found</div></div>')
607-
ietf.review.mailarch.construct_query_urls = lambda review_req, query=None: { "query_data_url": "file://" + os.path.abspath(no_result_path) }
613+
ietf.review.mailarch.construct_query_urls = lambda doc, team, query=None: { "query_data_url": "file://" + os.path.abspath(no_result_path) }
608614

609615
url = urlreverse('ietf.doc.views_review.search_mail_archive', kwargs={ "name": doc.name, "assignment_id": assignment.pk })
610616

@@ -617,6 +623,27 @@ def test_search_mail_archive(self):
617623
finally:
618624
ietf.review.mailarch.construct_query_urls = real_fn
619625

626+
def test_submit_unsolicited_review_choose_team(self):
627+
doc = WgDraftFactory(group__acronym='mars', rev='01')
628+
review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
629+
secretary = RoleFactory(group=review_team,person__user__username='reviewsecretary',person__user__email='reviewsecretary@example.com', name_id='secr')
630+
631+
url = urlreverse('ietf.doc.views_review.submit_unsolicited_review_choose_team',
632+
kwargs={'name': doc.name})
633+
redirect_url = urlreverse("ietf.doc.views_review.complete_review",
634+
kwargs={'name': doc.name, 'acronym': review_team.acronym})
635+
login_testing_unauthorized(self, secretary.person.user.username, url)
636+
637+
r = self.client.get(url)
638+
self.assertEqual(r.status_code, 200)
639+
self.assertContains(r, review_team.name)
640+
641+
r = self.client.post(url, data={'team': review_team.pk})
642+
self.assertRedirects(r, redirect_url)
643+
644+
r = self.client.post(url, data={'team': review_team.pk + 5})
645+
self.assertEqual(r.status_code, 200)
646+
620647
def setup_complete_review_test(self):
621648
doc = WgDraftFactory(group__acronym='mars',rev='01')
622649
NewRevisionDocEventFactory(doc=doc,rev='01')
@@ -872,6 +899,56 @@ def test_complete_review_link_to_mailing_list(self, mock):
872899
self.assertEqual(len(outbox), 0)
873900
self.assertTrue("http://example.com" in assignment.review.external_url)
874901

902+
@patch('requests.get')
903+
def test_complete_unsolicited_review_link_to_mailing_list_by_secretary(self, mock):
904+
# Mock up the url response for the request.get() call to retrieve the mailing list url
905+
response = Response()
906+
response.status_code = 200
907+
response._content = b"This is a review\nwith two lines"
908+
mock.return_value = response
909+
910+
# Run the test
911+
doc = WgDraftFactory(group__acronym='mars', rev='01')
912+
NewRevisionDocEventFactory(doc=doc, rev='01')
913+
review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
914+
rev_role = RoleFactory(group=review_team, person__user__username='reviewer', person__user__email='reviewer@example.com', name_id='reviewer')
915+
secretary_role = RoleFactory(group=review_team, person__user__username='reviewsecretary', person__user__email='reviewsecretary@example.com', name_id='secr')
916+
917+
url = urlreverse('ietf.doc.views_review.complete_review',
918+
kwargs={"name": doc.name, "acronym": review_team.acronym})
919+
920+
login_testing_unauthorized(self, secretary_role.person.user.username, url)
921+
922+
empty_outbox()
923+
924+
r = self.client.post(url, data={
925+
"result": ReviewResultName.objects.get(slug="ready").pk,
926+
"state": ReviewAssignmentStateName.objects.get(slug="completed").pk,
927+
"reviewed_rev": '01',
928+
"review_submission": "link",
929+
"review_content": response.content.decode(),
930+
"review_url": "http://example.com/testreview/",
931+
"review_file": "",
932+
"review_type": ReviewTypeName.objects.get(slug="early").pk,
933+
"reviewer": rev_role.person.pk,
934+
"completion_date": "2012-12-24",
935+
"completion_time": "12:13:14",
936+
})
937+
self.assertEqual(r.status_code, 302)
938+
939+
review_req = doc.reviewrequest_set.get()
940+
assignment = review_req.reviewassignment_set.get()
941+
self.assertEqual(review_req.type_id, "early")
942+
self.assertEqual(review_req.requested_by, secretary_role.person)
943+
self.assertEqual(assignment.reviewer, rev_role.person.role_email('reviewer'))
944+
self.assertEqual(assignment.state_id, "completed")
945+
946+
with io.open(os.path.join(self.review_subdir, assignment.review.name + ".txt")) as f:
947+
self.assertEqual(f.read(), "This is a review\nwith two lines")
948+
949+
self.assertEqual(len(outbox), 0)
950+
self.assertTrue("http://example.com" in assignment.review.external_url)
951+
875952
def test_partially_complete_review(self):
876953
assignment, url = self.setup_complete_review_test()
877954

‎ietf/doc/urls_review.py‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
# Copyright The IETF Trust 2016-2019, All Rights Reserved
2+
from django.conf import settings
3+
14
from ietf.doc import views_review
25
from ietf.utils.urls import url
36

@@ -9,9 +12,12 @@
912
url(r'^(?P<request_id>[0-9]+)/assignreviewer/$', views_review.assign_reviewer),
1013
url(r'^(?P<assignment_id>[0-9]+)/rejectreviewerassignment/$', views_review.reject_reviewer_assignment),
1114
url(r'^(?P<assignment_id>[0-9]+)/complete/$', views_review.complete_review),
15+
url(r'^%(acronym)s/submitunsolicitedreview/$' % settings.URL_REGEXPS, views_review.complete_review),
16+
url(r'^submitunsolicitedreview/$', views_review.submit_unsolicited_review_choose_team),
1217
url(r'^(?P<assignment_id>[0-9]+)/withdraw/$', views_review.withdraw_reviewer_assignment),
1318
url(r'^(?P<assignment_id>[0-9]+)/noresponse/$', views_review.mark_reviewer_assignment_no_response),
14-
url(r'^(?P<assignment_id>[0-9]+)/searchmailarchive/$', views_review.search_mail_archive),
19+
url(r'^assignment/(?P<assignment_id>[0-9]+)/searchmailarchive/$', views_review.search_mail_archive),
20+
url(r'^team/%(acronym)s/searchmailarchive/$' % settings.URL_REGEXPS, views_review.search_mail_archive),
1521
url(r'^(?P<request_id>[0-9]+)/editcomment/$', views_review.edit_comment),
1622
url(r'^(?P<request_id>[0-9]+)/editdeadline/$', views_review.edit_deadline),
1723
]

‎ietf/doc/views_doc.py‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@
6464
get_initial_notify, make_notify_changed_event, make_rev_history, default_consensus,
6565
add_events_message_info, get_unicode_document_content, build_doc_meta_block)
6666
from ietf.community.utils import augment_docs_with_tracking_info
67-
from ietf.group.models import Role
67+
from ietf.group.models import Role, Group
6868
from ietf.group.utils import can_manage_group_type, can_manage_materials, group_features_role_filter
6969
from ietf.ietfauth.utils import ( has_role, is_authorized_in_doc_stream, user_is_person,
7070
role_required, is_individual_draft_author)
@@ -317,6 +317,10 @@ def document_main(request, name, rev=None):
317317
consensus = nice_consensus(e and e.consensus)
318318

319319
can_request_review = can_request_review_of_doc(request.user, doc)
320+
can_submit_unsolicited_review_for_teams = None
321+
if request.user.is_authenticated:
322+
can_submit_unsolicited_review_for_teams = Group.objects.filter(
323+
reviewteamsettings__isnull=False, role__person__user=request.user, role__name='secr')
320324

321325
# mailing list search archive
322326
search_archive = "www.ietf.org/mail-archive/web/"
@@ -419,6 +423,7 @@ def document_main(request, name, rev=None):
419423
can_edit_replaces=can_edit_replaces,
420424
can_view_possibly_replaces=can_view_possibly_replaces,
421425
can_request_review=can_request_review,
426+
can_submit_unsolicited_review_for_teams=can_submit_unsolicited_review_for_teams,
422427

423428
rfc_number=rfc_number,
424429
draft_name=draft_name,

0 commit comments

Comments
 (0)