fix: add review team setting allowing reviewers to reject assignments past the deadline (ietf-tools#5418)

* Added a new review team setting
allow_reviewer_to_reject_after_deadline that will allow rejecting
review requests, even after the deadline is past. Also modified that
the secretary, or whoever manages the reviews is always allowed to
reject the review regardless of the deadline as he/she could change
the deadline anyways.

* Fixed but in view_reviews (wrong variable name), added more test
cases to the test_reviews.py for different reject cases.

* test: More thoroughly exercise assignment rejection

* chore: Renumber migration

* test: Unrelated user cannot reject assignments

---------

Co-authored-by: Jennifer Richards <jennifer@staff.ietf.org>

Author: kivinen

ietf/doc/tests_review.py

@@ -418,9 +418,50 @@ def test_reject_reviewer_assignment(self):
         r = self.client.get(req_url)
         self.assertEqual(r.status_code, 200)
         self.assertContains(r, reject_url)
+
+        # anonymous user should not be able to reject
+        self.client.logout()
+        r = self.client.post(reject_url, { "action": "reject", "message_to_secretary": "Test message" })
+        self.assertEqual(r.status_code, 302)  # forwards to login page
+        assignment = reload_db_objects(assignment)
+        self.assertEqual(assignment.state_id, "accepted")
+
+        # unrelated person should not be able to reject
+        other_person = PersonFactory()
+        login_testing_unauthorized(self, other_person.user.username, reject_url)
+        r = self.client.post(reject_url, { "action": "reject", "message_to_secretary": "Test message" })
+        self.assertEqual(r.status_code, 403)
+        assignment = reload_db_objects(assignment)
+        self.assertEqual(assignment.state_id, "accepted")
+
+        # Check that user can reject it
+        login_testing_unauthorized(self, assignment.reviewer.person.user.username, reject_url)
+        r = self.client.get(reject_url)
+        self.assertEqual(r.status_code, 200)
+        self.assertContains(r, escape(assignment.reviewer.person.name))
+        self.assertNotContains(r, 'can not be rejected')
+        self.assertContains(r, '<button type="submit"')
+
+        # reject
+        empty_outbox()
+        r = self.client.post(reject_url, { "action": "reject", "message_to_secretary": "Test message" })
+        self.assertEqual(r.status_code, 302)
+
+        assignment = reload_db_objects(assignment)
+        self.assertEqual(assignment.state_id, "rejected")
+        self.assertNotEqual(assignment.completed_on,None)
+        e = doc.latest_event()
+        self.assertEqual(e.type, "closed_review_assignment")
+        self.assertTrue("rejected" in e.desc)
+        self.assertEqual(len(outbox), 1)
+        self.assertNotIn(assignment.reviewer.address, outbox[0]["To"])
+        self.assertIn("<reviewsecretary@example.com>", outbox[0]["To"])
+        self.assertTrue("Test message" in get_payload_text(outbox[0]))
         self.client.logout()
 
-        # get reject page
+        # Secretary can also reject it
+        assignment.state_id = 'assigned'
+        assignment.save()
         login_testing_unauthorized(self, "reviewsecretary", reject_url)
         r = self.client.get(reject_url)
         self.assertEqual(r.status_code, 200)
@@ -444,12 +485,17 @@ def test_reject_reviewer_assignment(self):
         self.assertNotIn("<reviewsecretary@example.com>", outbox[0]["To"])
         self.assertTrue("Test message" in get_payload_text(outbox[0]))
 
-        # try again, but now with an expired review request, which should not be allowed (#2277)
+        # try again, but now with an expired review request,
+        # which should not be allowed (#2277)
         assignment.state_id = 'assigned'
         assignment.save()
         review_req.deadline = datetime.date(2019, 1, 1)
         review_req.save()
+        self.client.logout()
 
+        # Login as reviewer to do this test, so it should fail, as the
+        # request is past deadline
+        login_testing_unauthorized(self, assignment.reviewer.person.user.username, reject_url)
         r = self.client.get(reject_url)
         self.assertEqual(r.status_code, 200)
         self.assertContains(r, escape(assignment.reviewer.person.name))
@@ -461,10 +507,67 @@ def test_reject_reviewer_assignment(self):
         r = self.client.post(reject_url, { "action": "reject", "message_to_secretary": "Test message" })
         self.assertEqual(r.status_code, 200)
         self.assertContains(r, 'can not be rejected')
+        self.client.logout()
+
+        # Change settings so that even the reviewer should
+        # be allowed to reject the request even after past deadline
+        m = apps.get_model('review', 'ReviewTeamSettings')
+        for row in m.objects.all():
+            if row.group.upcase_acronym == review_team.upcase_acronym:
+               row.allow_reviewer_to_reject_after_deadline = True
+               row.save(update_fields=['allow_reviewer_to_reject_after_deadline'])
+
+        # Test again as user
+        login_testing_unauthorized(self, assignment.reviewer.person.user.username, reject_url)
+        r = self.client.get(reject_url)
+        self.assertEqual(r.status_code, 200)
+        self.assertContains(r, escape(assignment.reviewer.person.name))
+        self.assertNotContains(r, 'can not be rejected')
+        self.assertContains(r, '<button type="submit"')
+
+        # actually reject
+        r = self.client.post(reject_url, { "action": "reject", "message_to_secretary": "Test message" })
+        self.assertEqual(r.status_code, 302)
 
         assignment = reload_db_objects(assignment)
-        self.assertEqual(assignment.state_id, "assigned")
-        self.assertEqual(len(outbox), 0)
+        self.assertEqual(assignment.state_id, "rejected")
+        self.assertNotEqual(len(outbox), 0)
+        self.client.logout()
+
+        # Log in as secretary and that should still allow rejecting the review
+        assignment.state_id = 'assigned'
+        assignment.save()
+        login_testing_unauthorized(self, "reviewsecretary", reject_url)
+        r = self.client.get(reject_url)
+        self.assertEqual(r.status_code, 200)
+        self.assertContains(r, escape(assignment.reviewer.person.name))
+        self.assertNotContains(r, 'can not be rejected')
+        self.assertContains(r, '<button type="submit"')
+
+        # actually reject
+        empty_outbox()
+        r = self.client.post(reject_url, { "action": "reject", "message_to_secretary": "Test message" })
+        self.assertEqual(r.status_code, 302)
+
+        assignment = reload_db_objects(assignment)
+        self.assertEqual(assignment.state_id, "rejected")
+        self.assertNotEqual(len(outbox), 0)
+
+        # Revert the setting of allow_reviewer_to_reject_after_deadline
+        # This should not affect the secretary's ability to reject.
+        m = apps.get_model('review', 'ReviewTeamSettings')
+        for row in m.objects.all():
+            if row.group.upcase_acronym == review_team.upcase_acronym:
+               row.allow_reviewer_to_reject_after_deadline = False
+               row.save(update_fields=['allow_reviewer_to_reject_after_deadline'])
+        assignment.state_id = 'assigned'
+        assignment.save()
+        r = self.client.get(reject_url)
+        self.assertEqual(r.status_code, 200)
+        self.assertContains(r, escape(assignment.reviewer.person.name))
+        self.assertNotContains(r, 'can not be rejected')
+        self.assertContains(r, '<button type="submit"')
+
 
     def make_test_mbox_tarball(self, review_req):
         mbox_path = os.path.join(self.review_dir, "testmbox.tar.gz")

ietf/doc/views_review.py

@@ -354,8 +354,13 @@ class RejectReviewerAssignmentForm(forms.Form):
 def reject_reviewer_assignment(request, name, assignment_id):
     doc = get_object_or_404(Document, name=name)
     review_assignment = get_object_or_404(ReviewAssignment, pk=assignment_id, state__in=["assigned", "accepted"])
-    review_request_past_deadline = review_assignment.review_request.deadline < date_today(DEADLINE_TZINFO)
 
+    allow_reject_request = True
+    # Only check deadline if the group does not allow rejecting always
+    if not review_assignment.review_request.team.reviewteamsettings.allow_reviewer_to_reject_after_deadline:
+        if review_assignment.review_request.deadline < date_today(DEADLINE_TZINFO):
+            allow_reject_request = False
+    
     if not review_assignment.reviewer:
         return redirect(review_request, name=review_assignment.review_request.doc.name, request_id=review_assignment.review_request.pk)
 
@@ -365,7 +370,12 @@ def reject_reviewer_assignment(request, name, assignment_id):
     if not (is_reviewer or can_manage_request):
         permission_denied(request, "You do not have permission to perform this action")
 
-    if request.method == "POST" and request.POST.get("action") == "reject" and not review_request_past_deadline:
+    # Secretary or whoever can manage review request, has permission
+    # to reject requests even if the deadline is in the past
+    if can_manage_request:
+        allow_reject_request = True
+
+    if request.method == "POST" and request.POST.get("action") == "reject" and allow_reject_request:
         form = RejectReviewerAssignmentForm(request.POST)
         if form.is_valid():
             # reject the assignment
@@ -406,7 +416,7 @@ def reject_reviewer_assignment(request, name, assignment_id):
         'review_req': review_assignment.review_request,
         'assignments': review_assignment.review_request.reviewassignment_set.all(),
         'form': form,
-        'review_request_past_deadline': review_request_past_deadline,
+        'allow_reject_request': allow_reject_request,
     })
 
 @login_required

ietf/review/migrations/0002_reviewteamsettings_allow_reviewer_to_reject_after_deadline.py

@@ -0,0 +1,18 @@
+# Generated by Django 2.2.28 on 2023-03-25 06:34
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('review', '0001_initial'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='reviewteamsettings',
+            name='allow_reviewer_to_reject_after_deadline',
+            field=models.BooleanField(default=False, verbose_name='Allow reviewer to reject request after deadline.'),
+        ),
+    ]

ietf/review/models.py

@@ -191,6 +191,7 @@ class ReviewTeamSettings(models.Model):
     """Holds configuration specific to groups that are review teams"""
     group = OneToOneField(Group)
     autosuggest = models.BooleanField(default=True, verbose_name="Automatically suggest possible review requests")
+    allow_reviewer_to_reject_after_deadline = models.BooleanField(default=False, verbose_name="Allow reviewer to reject request after deadline.")
     reviewer_queue_policy = models.ForeignKey(ReviewerQueuePolicyName, default='RotateAlphabetically', on_delete=models.PROTECT)
     review_types = models.ManyToManyField(ReviewTypeName, default=get_default_review_types)
     review_results = models.ManyToManyField(ReviewResultName, default=get_default_review_results, related_name='reviewteamsettings_review_results_set')

ietf/templates/doc/review/reject_reviewer_assignment.html

@@ -10,7 +10,7 @@ <h1>
         <small class="text-body-secondary">{{ review_req.doc.name }}</small>
     </h1>
     {% include "doc/review/request_info.html" %}
-    {% if not review_request_past_deadline %}
+    {% if allow_reject_request %}
         <p class="alert alert-danger my-3">
             Do you want to reject this assignment?
         </p>