Rework closing a review request so the logic is reusable, add the more

specific close reasons to the database migration, add
ReviewRequest.requested_by so it's possible to notify the requester of
a review that it has been dropped.
 - Legacy-Id: 11520

Author: OleLaursen

ietf/doc/tests_review.py

@@ -14,7 +14,7 @@
 
 from ietf.review.models import ReviewRequest, ReviewTeamResult
 import ietf.review.mailarch
-from ietf.person.models import Email
+from ietf.person.models import Email, Person
 from ietf.name.models import ReviewResultName, ReviewRequestStateName
 from ietf.utils.test_utils import TestCase
 from ietf.utils.test_data import make_test_data, make_review_data
@@ -57,7 +57,8 @@ def test_request_review(self):
             "type": "early",
             "team": review_team.pk,
             "deadline_date": deadline_date.isoformat(),
-            "requested_rev": "01"
+            "requested_rev": "01",
+            "requested_by": Person.objects.get(user__username="plain").pk,
         })
         self.assertEqual(r.status_code, 302)
 
@@ -82,40 +83,40 @@ def test_review_request(self):
         self.assertEqual(r.status_code, 200)
         self.assertTrue(review_req.team.acronym.upper() in unicontent(r))
 
-    def test_withdraw_request(self):
+    def test_close_request(self):
         doc = make_test_data()
         review_req = make_review_data(doc)
         review_req.state = ReviewRequestStateName.objects.get(slug="accepted")
         review_req.save()
 
-        withdraw_url = urlreverse('ietf.doc.views_review.withdraw_request', kwargs={ "name": doc.name, "request_id": review_req.pk })
+        close_url = urlreverse('ietf.doc.views_review.close_request', kwargs={ "name": doc.name, "request_id": review_req.pk })
 
 
         # follow link
         req_url = urlreverse('ietf.doc.views_review.review_request', kwargs={ "name": doc.name, "request_id": review_req.pk })
         self.client.login(username="secretary", password="secretary+password")
         r = self.client.get(req_url)
         self.assertEqual(r.status_code, 200)
-        self.assertTrue(withdraw_url in unicontent(r))
+        self.assertTrue(close_url in unicontent(r))
         self.client.logout()
 
-        # get withdraw page
-        login_testing_unauthorized(self, "secretary", withdraw_url)
-        r = self.client.get(withdraw_url)
+        # get close page
+        login_testing_unauthorized(self, "secretary", close_url)
+        r = self.client.get(close_url)
         self.assertEqual(r.status_code, 200)
 
-        # withdraw
+        # close
         empty_outbox()
-        r = self.client.post(withdraw_url, { "action": "withdraw" })
+        r = self.client.post(close_url, { "close_reason": "withdrawn" })
         self.assertEqual(r.status_code, 302)
 
         review_req = reload_db_objects(review_req)
         self.assertEqual(review_req.state_id, "withdrawn")
         e = doc.latest_event()
         self.assertEqual(e.type, "changed_review_request")
-        self.assertTrue("Withdrew" in e.desc)
+        self.assertTrue("closed" in e.desc.lower())
         self.assertEqual(len(outbox), 1)
-        self.assertTrue("withdrawn" in unicode(outbox[0]))
+        self.assertTrue("closed" in unicode(outbox[0]).lower())
 
     def test_assign_reviewer(self):
         doc = make_test_data()

ietf/doc/urls_review.py

@@ -4,7 +4,7 @@
 urlpatterns = patterns('',
     url(r'^$', views_review.request_review),
     url(r'^(?P<request_id>[0-9]+)/$', views_review.review_request),
-    url(r'^(?P<request_id>[0-9]+)/withdraw/$', views_review.withdraw_request),
+    url(r'^(?P<request_id>[0-9]+)/close/$', views_review.close_request),
     url(r'^(?P<request_id>[0-9]+)/assignreviewer/$', views_review.assign_reviewer),
     url(r'^(?P<request_id>[0-9]+)/rejectreviewerassignment/$', views_review.reject_reviewer_assignment),
     url(r'^(?P<request_id>[0-9]+)/complete/$', views_review.complete_review),

ietf/doc/views_review.py

@@ -10,13 +10,14 @@
 from django.template.loader import render_to_string
 
 from ietf.doc.models import Document, NewRevisionDocEvent, DocEvent, State, DocAlias
-from ietf.ietfauth.utils import is_authorized_in_doc_stream, user_is_person
+from ietf.ietfauth.utils import is_authorized_in_doc_stream, user_is_person, has_role
 from ietf.name.models import ReviewRequestStateName, ReviewResultName, DocTypeName
 from ietf.review.models import ReviewRequest
-from ietf.person.fields import PersonEmailChoiceField
+from ietf.person.fields import PersonEmailChoiceField, SearchablePersonField
 from ietf.review.utils import (active_review_teams, assign_review_request_to_reviewer,
                                can_request_review_of_doc, can_manage_review_requests_for_team,
-                               email_about_review_request, make_new_review_request_from_existing)
+                               email_review_request_change, make_new_review_request_from_existing,
+                               close_review_request_states, close_review_request)
 from ietf.review import mailarch
 from ietf.utils.fields import DatepickerDateField
 from ietf.utils.text import skip_prefix
@@ -38,7 +39,7 @@ class RequestReviewForm(forms.ModelForm):
 
     class Meta:
         model = ReviewRequest
-        fields = ('type', 'team', 'deadline', 'requested_rev')
+        fields = ('requested_by', 'type', 'team', 'deadline', 'requested_rev')
 
     def __init__(self, user, doc, *args, **kwargs):
         super(RequestReviewForm, self).__init__(*args, **kwargs)
@@ -57,6 +58,12 @@ def __init__(self, user, doc, *args, **kwargs):
         self.fields["deadline"].required = False
         self.fields["requested_rev"].label = "Document revision"
 
+        if has_role(user, "Secretariat"):
+            self.fields["requested_by"] = SearchablePersonField()
+        else:
+            self.fields["requested_by"].widget = forms.HiddenInput()
+            self.fields["requested_by"].initial = user.person.pk
+
     def clean_deadline_date(self):
         v = self.cleaned_data.get('deadline_date')
         if v < datetime.date.today():
@@ -119,9 +126,9 @@ def review_request(request, name, request_id):
     is_reviewer = review_req.reviewer and user_is_person(request.user, review_req.reviewer.person)
     can_manage_request = can_manage_review_requests_for_team(request.user, review_req.team)
 
-    can_withdraw_request = (review_req.state_id in ["requested", "accepted"]
-                            and (is_authorized_in_doc_stream(request.user, doc)
-                                 or can_manage_request))
+    can_close_request = (review_req.state_id in ["requested", "accepted"]
+                         and (is_authorized_in_doc_stream(request.user, doc)
+                              or can_manage_request))
 
     can_assign_reviewer = (review_req.state_id in ["requested", "accepted"]
                            and can_manage_request)
@@ -147,47 +154,55 @@ def review_request(request, name, request_id):
     return render(request, 'doc/review/review_request.html', {
         'doc': doc,
         'review_req': review_req,
-        'can_withdraw_request': can_withdraw_request,
+        'can_close_request': can_close_request,
         'can_reject_reviewer_assignment': can_reject_reviewer_assignment,
         'can_assign_reviewer': can_assign_reviewer,
         'can_accept_reviewer_assignment': can_accept_reviewer_assignment,
         'can_complete_review': can_complete_review,
     })
 
+
+class CloseReviewRequestForm(forms.Form):
+    close_reason = forms.ModelChoiceField(queryset=close_review_request_states(), widget=forms.RadioSelect, empty_label=None)
+
+    def __init__(self, can_manage_request, *args, **kwargs):
+        super(CloseReviewRequestForm, self).__init__(*args, **kwargs)
+
+        if not can_manage_request:
+            self.fields["close_reason"].queryset = self.fields["close_reason"].queryset.filter(slug__in=["withdrawn"])
+
+        if len(self.fields["close_reason"].queryset) == 1:
+            self.fields["close_reason"].initial = self.fields["close_reason"].queryset.first().pk
+            self.fields["close_reason"].widget = forms.HiddenInput()
+
+
 @login_required
-def withdraw_request(request, name, request_id):
+def close_request(request, name, request_id):
     doc = get_object_or_404(Document, name=name)
     review_req = get_object_or_404(ReviewRequest, pk=request_id, state__in=["requested", "accepted"])
 
-    if not is_authorized_in_doc_stream(request.user, doc):
-        return HttpResponseForbidden("You do not have permission to perform this action")
-
-    if request.method == "POST" and request.POST.get("action") == "withdraw":
-        prev_state = review_req.state
-        review_req.state = ReviewRequestStateName.objects.get(slug="withdrawn")
-        review_req.save()
+    can_request = is_authorized_in_doc_stream(request.user, doc)
+    can_manage_request = can_manage_review_requests_for_team(request.user, review_req.team)
 
-        DocEvent.objects.create(
-            type="changed_review_request",
-            doc=doc,
-            by=request.user.person,
-            desc="Withdrew request for {} review by {}".format(review_req.type.name, review_req.team.acronym.upper()),
-        )
+    if not (can_request or can_manage_request):
+        return HttpResponseForbidden("You do not have permission to perform this action")
 
-        if prev_state.slug != "requested":
-            email_about_review_request(
-                request, review_req,
-                "Withdrew review request for %s" % review_req.doc.name,
-                "Review request has been withdrawn by %s." % request.user.person,
-                by=request.user.person, notify_secretary=False, notify_reviewer=True)
+    if request.method == "POST":
+        form = CloseReviewRequestForm(can_manage_request, request.POST)
+        if form.is_valid():
+            close_review_request(request, review_req, form.cleaned_data["close_reason"])
 
         return redirect(review_request, name=review_req.doc.name, request_id=review_req.pk)
+    else:
+        form = CloseReviewRequestForm(can_manage_request)
 
-    return render(request, 'doc/review/withdraw_request.html', {
+    return render(request, 'doc/review/close_request.html', {
         'doc': doc,
         'review_req': review_req,
+        'form': form,
     })
 
+
 class AssignReviewerForm(forms.Form):
     reviewer = PersonEmailChoiceField(widget=forms.RadioSelect, empty_label="(None)", required=False)
 
@@ -266,7 +281,7 @@ def reject_reviewer_assignment(request, name, request_id):
                 "message_to_secretary": form.cleaned_data.get("message_to_secretary")
             })
 
-            email_about_review_request(request, review_req, "Reviewer assignment rejected", msg, by=request.user.person, notify_secretary=True, notify_reviewer=True)
+            email_review_request_change(request, review_req, "Reviewer assignment rejected", msg, by=request.user.person, notify_secretary=True, notify_reviewer=True, notify_requested_by=False)
 
             return redirect(review_request, name=new_review_req.doc.name, request_id=new_review_req.pk)
     else:
@@ -438,7 +453,7 @@ def complete_review(request, name, request_id):
                     "new_review_req": new_review_req,
                 })
 
-                email_about_review_request(request, review_req, subject, msg, request.user.person, notify_secretary=True, notify_reviewer=False)
+                email_review_request_change(request, review_req, subject, msg, request.user.person, notify_secretary=True, notify_reviewer=False, notify_requested_by=False)
 
             if review_submission != "link" and review_req.team.list_email:
                 # email the review

ietf/group/tests_review.py

@@ -7,7 +7,7 @@
 from ietf.utils.test_data import make_test_data, make_review_data
 from ietf.utils.test_utils import login_testing_unauthorized, TestCase, unicontent, reload_db_objects
 from ietf.review.models import ReviewRequest
-from ietf.person.models import Email
+from ietf.person.models import Email, Person
 import ietf.group.views_review
 
 class ReviewTests(TestCase):
@@ -28,6 +28,7 @@ def test_manage_review_requests(self):
             deadline=datetime.datetime.combine(datetime.date.today() + datetime.timedelta(days=30), datetime.time(23, 59, 59)),
             state_id="accepted",
             reviewer=review_req1.reviewer,
+            requested_by=Person.objects.get(user__username="plain"),
         )
 
         review_req3 = ReviewRequest.objects.create(
@@ -36,6 +37,7 @@ def test_manage_review_requests(self):
             type_id="early",
             deadline=datetime.datetime.combine(datetime.date.today() + datetime.timedelta(days=30), datetime.time(23, 59, 59)),
             state_id="requested",
+            requested_by=Person.objects.get(user__username="plain"),
         )
 
         # get

ietf/group/views_review.py

@@ -4,10 +4,11 @@
 from django import forms
 
 from ietf.review.models import ReviewRequest, ReviewRequestStateName
-from ietf.review.utils import (can_manage_review_requests_for_team,
+from ietf.review.utils import (can_manage_review_requests_for_team, close_review_request_states,
                                extract_revision_ordered_review_requests_for_documents,
                                assign_review_request_to_reviewer,
-#                               email_about_review_request, make_new_review_request_from_existing,
+                               close_review_request,
+#                               email_review_request_change, make_new_review_request_from_existing,
                                suggested_review_requests_for_team)
 from ietf.group.utils import get_group_or_404
 from ietf.person.fields import PersonEmailChoiceField
@@ -21,14 +22,7 @@ class ManageReviewRequestForm(forms.Form):
 
     action = forms.ChoiceField(choices=ACTIONS, widget=forms.HiddenInput, required=False)
 
-    CLOSE_OPTIONS = [
-        ("noreviewversion", "No review of this version"),
-        ("noreviewdocument", "No review of document"),
-        ("withdraw", "Withdraw request"),
-        ("no-response", "No response"),
-        ("overtaken", "Overtaken by events"),
-    ]
-    close = forms.ChoiceField(choices=CLOSE_OPTIONS, required=False)
+    close = forms.ModelChoiceField(queryset=close_review_request_states(), required=False)
 
     reviewer = PersonEmailChoiceField(empty_label="(None)", required=False, label_with="person")
 
@@ -44,9 +38,9 @@ def __init__(self, review_req, *args, **kwargs):
         close_initial = None
         if review_req.pk is None:
             if review_req.latest_reqs:
-                close_initial = "noreviewversion"
+                close_initial = "no-review-version"
             else:
-                close_initial = "noreviewdocument"
+                close_initial = "no-review-document"
         elif review_req.reviewer:
             close_initial = "no-response"
         else:
@@ -55,6 +49,9 @@ def __init__(self, review_req, *args, **kwargs):
         if close_initial:
             self.fields["close"].initial = close_initial
 
+        if review_req.pk is None:
+            self.fields["close"].queryset = self.fields["close"].queryset.filter(slug__in=["noreviewversion", "noreviewdocument"])
+
         self.fields["close"].widget.attrs["class"] = "form-control input-sm"
 
         self.fields["reviewer"].queryset = self.fields["reviewer"].queryset.filter(
@@ -115,18 +112,13 @@ def manage_review_requests(request, acronym, group_type=None):
             form_results.append(req.form.is_valid())
 
         if all(form_results):
-            for req in review_requests:
-                action = req.form.cleaned_data.get("action")
+            for review_req in review_requests:
+                action = review_req.form.cleaned_data.get("action")
                 if action == "assign":
-                    assign_review_request_to_reviewer(request, req, req.form.cleaned_data["reviewer"])
+                    assign_review_request_to_reviewer(request, review_req, review_req.form.cleaned_data["reviewer"])
                 elif action == "close":
-                    close_reason = req.form.cleaned_data["close"]
-                    if close_reason in ("withdraw", "no-response", "overtaken"):
-                        req.state = ReviewRequestStateName.objects.get(slug=close_reason, used=True)
-                        req.save()
-                        # FIXME: notify?
-                    else:
-                        FIXME
+                    close_review_request(request, review_req, review_req.form.cleaned_data["close"])
+
 
             kwargs = { "acronym": group.acronym }
             if group_type:

ietf/name/migrations/0012_insert_review_name_data.py

@@ -12,8 +12,10 @@ def insert_initial_review_data(apps, schema_editor):
     ReviewRequestStateName.objects.get_or_create(slug="withdrawn", name="Withdrawn", order=4)
     ReviewRequestStateName.objects.get_or_create(slug="overtaken", name="Overtaken By Events", order=5)
     ReviewRequestStateName.objects.get_or_create(slug="no-response", name="No Response", order=6)
-    ReviewRequestStateName.objects.get_or_create(slug="part-completed", name="Partially Completed", order=7)
-    ReviewRequestStateName.objects.get_or_create(slug="completed", name="Completed", order=8)
+    ReviewRequestStateName.objects.get_or_create(slug="no-review-version", name="No Review of Version", order=7)
+    ReviewRequestStateName.objects.get_or_create(slug="no-review-document", name="No Review of Document", order=8)
+    ReviewRequestStateName.objects.get_or_create(slug="part-completed", name="Partially Completed", order=9)
+    ReviewRequestStateName.objects.get_or_create(slug="completed", name="Completed", order=10)
 
     ReviewTypeName = apps.get_model("name", "ReviewTypeName")
     ReviewTypeName.objects.get_or_create(slug="early", name="Early", order=1)

ietf/name/models.py

@@ -89,7 +89,7 @@ class LiaisonStatementTagName(NameModel):
     "Action Required, Action Taken"
 class ReviewRequestStateName(NameModel):
     """Requested, Accepted, Rejected, Withdrawn, Overtaken By Events,
-    No Response, Partially Completed, Completed"""
+    No Response, No Review of Version, No Review of Document, Partially Completed, Completed"""
 class ReviewTypeName(NameModel):
     """Early Review, Last Call, Telechat"""
 class ReviewResultName(NameModel):

ietf/review/import_from_review_tool.py

@@ -161,6 +161,8 @@ def parse_timestamp(t):
         doc_metadata[(row.docname, row.version)] = doc_metadata[row.docname] = (parse_timestamp(row.deadline), parse_timestamp(row.telechat), parse_timestamp(row.lcend), row.status)
 
 
+system_person = Person.objects.get(name="(System)")
+
 with db_con.cursor() as c:
     c.execute("select * from reviews order by reviewid;")
 
@@ -207,6 +209,7 @@ def parse_timestamp(t):
                 "state": states["requested"],
                 "type": type_name,
                 "deadline": deadline,
+                "requested_by": system_person,
             }
         )
 

ietf/review/migrations/0001_initial.py

@@ -42,6 +42,7 @@ class Migration(migrations.Migration):
                 ('result', models.ForeignKey(blank=True, to='name.ReviewResultName', null=True)),
                 ('review', models.OneToOneField(null=True, blank=True, to='doc.Document')),
                 ('reviewer', models.ForeignKey(blank=True, to='person.Email', null=True)),
+                ('requested_by', models.ForeignKey(to='person.Person')),
                 ('state', models.ForeignKey(to='name.ReviewRequestStateName')),
                 ('team', models.ForeignKey(to='group.Group')),
                 ('type', models.ForeignKey(to='name.ReviewTypeName')),

ietf/review/models.py

@@ -42,6 +42,7 @@ class ReviewRequest(models.Model):
     doc           = models.ForeignKey(Document, related_name='review_request_set')
     team          = models.ForeignKey(Group, limit_choices_to=~models.Q(reviewteamresult=None))
     deadline      = models.DateTimeField()
+    requested_by  = models.ForeignKey(Person)
     requested_rev = models.CharField(verbose_name="requested revision", max_length=16, blank=True, help_text="Fill in if a specific revision is to be reviewed, e.g. 02")
 
     # Fields filled in as reviewer is assigned and as the review is