Add simple email notifications for assigning/rejecting review requests

 - Legacy-Id: 11236

Author: OleLaursen

ietf/doc/tests_review.py

@@ -107,6 +107,7 @@ def test_withdraw_request(self):
         self.assertEqual(r.status_code, 200)
 
         # withdraw
+        empty_outbox()
         r = self.client.post(withdraw_url, { "action": "withdraw" })
         self.assertEqual(r.status_code, 302)
 
@@ -115,6 +116,8 @@ def test_withdraw_request(self):
         e = doc.latest_event()
         self.assertEqual(e.type, "changed_review_request")
         self.assertTrue("Withdrew" in e.desc)
+        self.assertEqual(len(outbox), 1)
+        self.assertTrue("withdrawn" in unicode(outbox[0]))
 
     def test_assign_reviewer(self):
         doc = make_test_data()
@@ -140,24 +143,31 @@ def test_assign_reviewer(self):
         self.assertEqual(r.status_code, 200)
 
         # assign
+        empty_outbox()
         reviewer = Role.objects.filter(name="reviewer", group=review_req.team).first()
         r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk })
         self.assertEqual(r.status_code, 302)
 
         review_req = reload_db_objects(review_req)
         self.assertEqual(review_req.state_id, "requested")
         self.assertEqual(review_req.reviewer, reviewer)
+        self.assertEqual(len(outbox), 1)
+        self.assertTrue("assigned" in unicode(outbox[0]))
 
         # re-assign
+        empty_outbox()
         review_req.state = ReviewRequestStateName.objects.get(slug="accepted")
         review_req.save()
         reviewer = Role.objects.filter(name="reviewer", group=review_req.team).exclude(pk=reviewer.pk).first()
         r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk })
         self.assertEqual(r.status_code, 302)
 
         review_req = reload_db_objects(review_req)
-        self.assertEqual(review_req.state_id, "requested")
+        self.assertEqual(review_req.state_id, "requested") # check that state is reset
         self.assertEqual(review_req.reviewer, reviewer)
+        self.assertEqual(len(outbox), 2)
+        self.assertTrue("cancelled your assignment" in unicode(outbox[0]))
+        self.assertTrue("assigned" in unicode(outbox[1]))
 
     def test_reject_reviewer_assignment(self):
         doc = make_test_data()
@@ -194,4 +204,5 @@ def test_reject_reviewer_assignment(self):
         self.assertTrue("rejected" in e.desc)
         self.assertEqual(ReviewRequest.objects.filter(doc=review_req.doc, team=review_req.team, state="requested").count(), 1)
         self.assertEqual(len(outbox), 1)
-        
+        self.assertTrue("Test message" in unicode(outbox[0]))
+

ietf/doc/views_review.py

@@ -10,8 +10,9 @@
 from ietf.name.models import ReviewRequestStateName
 from ietf.group.models import Role
 from ietf.review.models import ReviewRequest
-from ietf.review.utils import active_review_teams, assign_review_request_to_reviewer
-from ietf.review.utils import can_request_review_of_doc, can_manage_review_requests_for_team
+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)
 from ietf.utils.fields import DatepickerDateField
 
 class RequestReviewForm(forms.ModelForm):
@@ -134,6 +135,7 @@ def withdraw_request(request, name, request_id):
         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()
 
@@ -144,9 +146,12 @@ def withdraw_request(request, name, request_id):
             desc="Withdrew request for {} review by {}".format(review_req.type.name, review_req.team.acronym.upper()),
         )
 
-        if review_req.state_id != "requested":
-            # FIXME: handle this case - by emailing?
-            pass
+        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)
 
         return redirect(review_request, name=review_req.doc.name, request_id=review_req.pk)
 
@@ -187,7 +192,7 @@ def assign_reviewer(request, name, request_id):
         form = AssignReviewerForm(review_req, request.POST)
         if form.is_valid():
             reviewer = form.cleaned_data["reviewer"]
-            assign_review_request_to_reviewer(review_req, reviewer, request.user.person)
+            assign_review_request_to_reviewer(request, review_req, reviewer)
 
             return redirect(review_request, name=review_req.doc.name, request_id=review_req.pk)
     else:
@@ -216,35 +221,48 @@ def reject_reviewer_assignment(request, name, request_id):
         return HttpResponseForbidden("You do not have permission to perform this action")
 
     if request.method == "POST" and request.POST.get("action") == "reject":
-        # reject the request
-        review_req.state = ReviewRequestStateName.objects.get(slug="rejected")
-        review_req.save()
+        form = RejectReviewerAssignmentForm(request.POST)
+        if form.is_valid():
+            # reject the request
+            review_req.state = ReviewRequestStateName.objects.get(slug="rejected")
+            review_req.save()
 
-        DocEvent.objects.create(
-            type="changed_review_request",
-            doc=review_req.doc,
-            by=request.user.person,
-            desc="Assignment of request for {} review by {} to {} was rejected".format(
-                review_req.type.name,
-                review_req.team.acronym.upper(),
-                review_req.reviewer.person,
-            ),
-        )
-        
-        # make a new unassigned review request
-        new_review_req = ReviewRequest.objects.create(
-            time=review_req.time,
-            type=review_req.type,
-            doc=review_req.doc,
-            team=review_req.team,
-            deadline=review_req.deadline,
-            requested_rev=review_req.requested_rev,
-            state=ReviewRequestStateName.objects.get(slug="requested"),
-        )
+            DocEvent.objects.create(
+                type="changed_review_request",
+                doc=review_req.doc,
+                by=request.user.person,
+                desc="Assignment of request for {} review by {} to {} was rejected".format(
+                    review_req.type.name,
+                    review_req.team.acronym.upper(),
+                    review_req.reviewer.person,
+                ),
+            )
+
+            # make a new unassigned review request
+            new_review_req = ReviewRequest.objects.create(
+                time=review_req.time,
+                type=review_req.type,
+                doc=review_req.doc,
+                team=review_req.team,
+                deadline=review_req.deadline,
+                requested_rev=review_req.requested_rev,
+                state=ReviewRequestStateName.objects.get(slug="requested"),
+            )
+
+            msg = u"Reviewer assignment rejected by %s." % request.user.person
 
-        return redirect(review_request, name=new_review_req.doc.name, request_id=new_review_req.pk)
+            m = form.cleaned_data.get("message_to_secretary")
+            if m:
+                msg += "\n\n" + "Explanation:" + "\n" + m
+
+            email_about_review_request(request, review_req, "Reviewer assignment rejected", msg, by=request.user.person, notify_secretary=True, notify_reviewer=True)
+
+            return redirect(review_request, name=new_review_req.doc.name, request_id=new_review_req.pk)
+    else:
+        form = RejectReviewerAssignmentForm()
 
     return render(request, 'doc/review/reject_reviewer_assignment.html', {
         'doc': doc,
         'review_req': review_req,
+        'form': form,
     })

ietf/review/models.py

@@ -27,7 +27,7 @@ class ReviewRequest(models.Model):
     time          = models.DateTimeField(auto_now_add=True)
     type          = models.ForeignKey(ReviewTypeName)
     doc           = models.ForeignKey(Document, related_name='review_request_set')
-    team          = models.ForeignKey(Group)
+    team          = models.ForeignKey(Group, limit_choices_to=~models.Q(reviewresultname=None))
     deadline      = models.DateTimeField()
     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")
 

ietf/review/utils.py

@@ -1,7 +1,10 @@
+from django.contrib.sites.models import Site
+
 from ietf.group.models import Group, Role
 from ietf.doc.models import DocEvent
 from ietf.ietfauth.utils import has_role, is_authorized_in_doc_stream
 from ietf.review.models import ReviewRequestStateName
+from ietf.utils.mail import send_mail
 
 def active_review_teams():
     # if there's a ReviewResultName defined, it's a review team
@@ -17,16 +20,47 @@ def can_manage_review_requests_for_team(user, team):
     if not user.is_authenticated():
         return False
 
-    return Role.objects.filter(name="secretary", person__user=user, group=team).exists() or has_role(user, "Secretariat")
+    return Role.objects.filter(name__in=["secretary", "delegate"], person__user=user, group=team).exists() or has_role(user, "Secretariat")
+
+def email_about_review_request(request, review_req, subject, msg, by, notify_secretary, notify_reviewer):
+    """Notify possibly both secretary and reviewer about change, skipping
+    a party if the change was done by that party."""
+
+    def extract_email_addresses(roles):
+        if any(r.person == by for r in roles if r):
+            return []
+        else:
+            return [r.formatted_email() for r in roles if r]
+
+    to = []
+
+    if notify_secretary:
+        to += extract_email_addresses(Role.objects.filter(name__in=["secretary", "delegate"], group=review_req.team).distinct())
+    if notify_reviewer:
+        to += extract_email_addresses([review_req.reviewer])
 
-def assign_review_request_to_reviewer(review_req, reviewer, by):
+    if not to:
+        return
+
+    send_mail(request, list(set(to)), None, subject, "doc/mail/review_request_changed.txt", {
+        "domain": Site.objects.get_current().domain,
+        "review_req": review_req,
+        "msg": msg,
+    })
+
+
+def assign_review_request_to_reviewer(request, review_req, reviewer):
     assert review_req.state_id in ("requested", "accepted")
 
-    if review_req.reviewer == reviewer:
+    if reviewer == review_req.reviewer:
         return
 
-    prev_state = review_req.state
-    prev_reviewer = review_req.reviewer
+    if review_req.reviewer:
+        email_about_review_request(
+            request, review_req,
+            "Unassigned from review of %s" % review_req.doc.name,
+            "%s has cancelled your assignment to the review." % request.user.person,
+            by=request.user.person, notify_secretary=False, notify_reviewer=True)
 
     review_req.state = ReviewRequestStateName.objects.get(slug="requested")
     review_req.reviewer = reviewer
@@ -35,14 +69,16 @@ def assign_review_request_to_reviewer(review_req, reviewer, by):
     DocEvent.objects.create(
         type="changed_review_request",
         doc=review_req.doc,
-        by=by,
+        by=request.user.person,
         desc="Request for {} review by {} is assigned to {}".format(
             review_req.type.name,
             review_req.team.acronym.upper(),
             review_req.reviewer.person if review_req.reviewer else "(None)",
         ),
     )
-    
-    if prev_state.slug != "requested" and prev_reviewer:
-        # FIXME: email old reviewer?
-        pass
+
+    email_about_review_request(
+        request, review_req,
+        "Assigned to review of %s" % review_req.doc.name,
+        "%s has assigned you to review the document." % request.user.person,
+        by=request.user.person, notify_secretary=False, notify_reviewer=True)

ietf/templates/doc/mail/review_request_changed.txt

@@ -0,0 +1,7 @@
+{% autoescape off %}
+  {{ review_req.type.name }} review of: {{ review_req.doc.name }}{% if review_req.requested_rev %}-{{ review_req.requested_rev }}{% endif %}
+  https://{{ domain }}{% url "ietf.doc.views_review.review_request" name=review_req.doc.name request_id=review_req.pk %}
+
+{{ msg|wordwrap:72 }}
+
+{% endautoescape %}