From b9f4b7005e2d715fbd6c4e82a006da22f71c0005 Mon Sep 17 00:00:00 2001 From: Ole Laursen Date: Tue, 24 May 2016 14:02:59 +0000 Subject: [PATCH] Add simple email notifications for assigning/rejecting review requests - Legacy-Id: 11236 --- ietf/doc/tests_review.py | 15 +++- ietf/doc/views_review.py | 80 ++++++++++++------- ietf/review/models.py | 2 +- ietf/review/utils.py | 58 +++++++++++--- .../doc/mail/review_request_changed.txt | 7 ++ 5 files changed, 117 insertions(+), 45 deletions(-) create mode 100644 ietf/templates/doc/mail/review_request_changed.txt diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index b6058ad0d..c2fe46549 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -107,6 +107,7 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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,6 +143,7 @@ class ReviewTests(TestCase): 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) @@ -147,8 +151,11 @@ class ReviewTests(TestCase): 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() @@ -156,8 +163,11 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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])) + diff --git a/ietf/doc/views_review.py b/ietf/doc/views_review.py index a0722a276..eabc02beb 100644 --- a/ietf/doc/views_review.py +++ b/ietf/doc/views_review.py @@ -10,8 +10,9 @@ from ietf.ietfauth.utils import is_authorized_in_doc_stream, user_is_person 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, + ), + ) - return redirect(review_request, name=new_review_req.doc.name, request_id=new_review_req.pk) + # 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 + + 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, }) diff --git a/ietf/review/models.py b/ietf/review/models.py index 530e0c939..79092e8b1 100644 --- a/ietf/review/models.py +++ b/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") diff --git a/ietf/review/utils.py b/ietf/review/utils.py index 43d495e15..8cc692d35 100644 --- a/ietf/review/utils.py +++ b/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 assign_review_request_to_reviewer(review_req, reviewer, by): - assert review_req.state_id in ("requested", "accepted") +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.""" - if review_req.reviewer == reviewer: + 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]) + + if not to: return - prev_state = review_req.state - prev_reviewer = review_req.reviewer + 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 reviewer == review_req.reviewer: + return + + 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) diff --git a/ietf/templates/doc/mail/review_request_changed.txt b/ietf/templates/doc/mail/review_request_changed.txt new file mode 100644 index 000000000..5c8f36d51 --- /dev/null +++ b/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 %}