feat: Allow review rejections to be undone (#6312)

* feat: Allow reviewer to accept a review they're previously rejected

* feat: Add a reviewer who has previously rejected a review to the list of suggested reviewers.

This largely un-does d105f8b, at the request of at least one team secretary.

* fix: Went a little overboard on the previous commit

one_assignment still has to exclude reviewers who rejected the assignment,
or they could end up being the suggested reviewer.

* fix: Actually do the assignment

* fix: If there's an existing assignment, don't create a new one

* style: Restructure conditional for clarity

* test: Add test cases for accepting or assigning a review assignment after rejecting it

ietf/doc/tests_review.py

@@ -1,4 +1,4 @@
-# Copyright The IETF Trust 2016-2020, All Rights Reserved
+# Copyright The IETF Trust 2016-2023, All Rights Reserved
 # -*- coding: utf-8 -*-
 
 
@@ -355,6 +355,23 @@ class ReviewTests(TestCase):
         request_events = review_req.reviewrequestdocevent_set.all()
         self.assertEqual(request_events.count(), 0)
 
+    def test_assign_reviewer_after_reject(self):
+        doc = WgDraftFactory()
+        review_team = ReviewTeamFactory()
+        rev_role = RoleFactory(group=review_team,person__user__username='reviewer',person__user__email='reviewer@example.com',name_id='reviewer')
+        reviewer_email = Email.objects.get(person__user__username="reviewer")
+        RoleFactory(group=review_team,person__user__username='reviewsecretary',name_id='secr')
+        review_req = ReviewRequestFactory(team=review_team,doc=doc)
+        ReviewAssignmentFactory(review_request=review_req, state_id='rejected', reviewer=rev_role.person.email_set.first())
+
+        url = urlreverse('ietf.doc.views_review.assign_reviewer', kwargs={ "name": doc.name, "request_id": review_req.pk })
+        login_testing_unauthorized(self, "reviewsecretary", url)
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 200)
+        q = PyQuery(r.content)
+        reviewer_label = q("option[value=\"{}\"]".format(reviewer_email.address)).text().lower()
+        self.assertIn("rejected review of document before", reviewer_label)
+
     def test_previously_reviewed_replaced_doc(self):
         review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
         rev_role = RoleFactory(group=review_team,person__user__username='reviewer',person__user__email='reviewer@example.com',person__name='Some Reviewer',name_id='reviewer')
@@ -569,6 +586,29 @@ class ReviewTests(TestCase):
         self.assertContains(r, '<button type="submit"')
 
 
+    def test_accept_reviewer_assignment_after_reject(self):
+        doc = WgDraftFactory()
+        review_team = ReviewTeamFactory()
+        rev_role = RoleFactory(group=review_team,name_id='reviewer')
+        review_req = ReviewRequestFactory(doc=doc,team=review_team)
+        assignment = ReviewAssignmentFactory(review_request=review_req, state_id='rejected', reviewer=rev_role.person.email_set.first())
+
+        url = urlreverse('ietf.doc.views_review.review_request', kwargs={ "name": doc.name, "request_id": review_req.pk })
+        username = assignment.reviewer.person.user.username
+        self.client.login(username=username, password=username + "+password")
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 200)
+        q = PyQuery(r.content)
+        d = q('.reviewer-assignment-not-accepted')
+        self.assertTrue(d("[name=action][value=accept]"))
+
+        # accept
+        r = self.client.post(url, { "action": "accept" })
+        self.assertEqual(r.status_code, 302)
+
+        assignment = reload_db_objects(assignment)
+        self.assertEqual(assignment.state_id, "accepted")
+
     def make_test_mbox_tarball(self, review_req):
         mbox_path = os.path.join(self.review_dir, "testmbox.tar.gz")
         with tarfile.open(mbox_path, "w:gz") as tar:

ietf/doc/views_review.py

@@ -223,7 +223,7 @@ def review_request(request, name, request_id):
     for assignment in assignments:
         assignment.is_reviewer = user_is_person(request.user, assignment.reviewer.person)
 
-        assignment.can_accept_reviewer_assignment = (assignment.state_id == "assigned"
+        assignment.can_accept_reviewer_assignment = (assignment.state_id in ["assigned", "rejected"]
                                                      and (assignment.is_reviewer or can_manage_request))
 
         assignment.can_reject_reviewer_assignment = (assignment.state_id in ["assigned", "accepted"]

ietf/group/tests_review.py

@@ -1,4 +1,4 @@
-# Copyright The IETF Trust 2016-2020, All Rights Reserved
+# Copyright The IETF Trust 2016-2023, All Rights Reserved
 # -*- coding: utf-8 -*-
 
 
@@ -711,6 +711,22 @@ class ReviewTests(TestCase):
         self.assertEqual(settings.max_items_to_show_in_reviewer_list, 10)
         self.assertEqual(settings.days_to_show_in_reviewer_list, 365)
 
+    def test_assign_reviewer_after_reject(self):
+        team = ReviewTeamFactory()
+        reviewer = RoleFactory(name_id='reviewer', group=team).person
+        ReviewerSettingsFactory(person=reviewer, team=team)
+        review_req = ReviewRequestFactory(team=team)
+        ReviewAssignmentFactory(review_request=review_req, state_id='rejected', reviewer=reviewer.email())
+
+        unassigned_url = urlreverse(ietf.group.views.manage_review_requests, kwargs={ 'acronym': team.acronym, 'group_type': team.type_id, "assignment_status": "unassigned" })
+        login_testing_unauthorized(self, "secretary", unassigned_url)
+
+        r = self.client.get(unassigned_url)
+        self.assertEqual(r.status_code, 200)
+        q = PyQuery(r.content)
+        reviewer_label = q("option[value=\"{}\"]".format(reviewer.email())).text().lower()
+        self.assertIn("rejected review of document before", reviewer_label)
+
 
 class BulkAssignmentTests(TestCase):
 

ietf/review/policies.py

@@ -1,4 +1,4 @@
-# Copyright The IETF Trust 2019-2021, All Rights Reserved
+# Copyright The IETF Trust 2019-2023, All Rights Reserved
 
 
 import re
@@ -10,6 +10,7 @@ from simple_history.utils import bulk_update_with_history
 from ietf.doc.models import DocumentAuthor, DocAlias
 from ietf.doc.utils import extract_complete_replaces_ancestor_mapping_for_docs
 from ietf.group.models import Role
+from ietf.name.models import ReviewAssignmentStateName
 from ietf.person.models import Person
 import debug                            # pyflakes:ignore
 from ietf.review.models import NextReviewerInTeam, ReviewerSettings, ReviewWish, ReviewRequest, \
@@ -55,8 +56,6 @@ def persons_with_previous_review(team, review_req, possible_person_ids, state_id
         reviewassignment__state=state_id,
         team=team,
     ).distinct()
-    if review_req.pk is not None:
-        has_reviewed_previous = has_reviewed_previous.exclude(pk=review_req.pk)
     has_reviewed_previous = set(
         has_reviewed_previous.values_list("reviewassignment__reviewer__person", flat=True))
     return has_reviewed_previous
@@ -70,7 +69,14 @@ class AbstractReviewerQueuePolicy:
         """Assign a reviewer to a request and update policy state accordingly"""
         # Update policy state first - needed by LRU policy to correctly compute whether assignment was in-order
         self.update_policy_state_for_assignment(review_req, reviewer.person, add_skip)
-        return review_req.reviewassignment_set.create(state_id='assigned', reviewer=reviewer, assigned_on=timezone.now())
+        assignment = review_req.reviewassignment_set.filter(reviewer=reviewer).first()
+        if assignment:
+            assignment.state = ReviewAssignmentStateName.objects.get(slug='assigned', used=True)
+            assignment.assigned_on = timezone.now()
+            assignment.save()
+            return assignment
+        else:
+            return review_req.reviewassignment_set.create(state_id='assigned', reviewer=reviewer, assigned_on=timezone.now())
 
     def default_reviewer_rotation_list(self, include_unavailable=False):
         """ Return a list of reviewers (Person objects) in the default reviewer rotation for a policy.
@@ -168,15 +174,15 @@ class AbstractReviewerQueuePolicy:
             PersonEmailChoiceField(label="Assign Reviewer", empty_label="(None)")
         """
 
-        # Collect a set of person IDs for people who have either not responded
+        # Collect a set of person IDs for people who have not responded
-        # to or outright rejected reviewing this document in the past
+        # to this document in the past
         rejecting_reviewer_ids = review_req.doc.reviewrequest_set.filter(
-            reviewassignment__state__slug__in=('rejected', 'no-response')
+            reviewassignment__state__slug='no-response'
         ).values_list(
             'reviewassignment__reviewer__person_id', flat=True
         )
 
-        # Query the Email objects for reviewers who haven't rejected or
+        # Query the Email objects for reviewers who haven't
         # not responded to this document in the past
         field.queryset = field.queryset.filter(
             role__name="reviewer",

ietf/review/tests_policies.py

@@ -1,4 +1,4 @@
-# Copyright The IETF Trust 2016-2021, All Rights Reserved
+# Copyright The IETF Trust 2016-2023, All Rights Reserved
 
 import debug                            # pyflakes:ignore
 import datetime
@@ -471,9 +471,6 @@ class _Wrapper(TestCase):
             
             addresses = list( map( lambda choice: choice[0], field.choices ) )
             
-            self.assertNotIn(
-                str(rejected_reviewer.email()), addresses,
-                "Reviews should not suggest people who have rejected this request in the past")
             self.assertNotIn(
                 str(no_response_reviewer.email()), addresses,
                 "Reviews should not suggest people who have not responded to this request in the past.")

ietf/review/utils.py

@@ -388,8 +388,10 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa
     log.assertion('reviewer is not None')
 
     # cannot reference reviewassignment_set relation until pk exists
-    if review_req.pk is not None and review_req.reviewassignment_set.filter(reviewer=reviewer).exists():
+    if review_req.pk is not None:
-        return
+        reviewassignment_set = review_req.reviewassignment_set.filter(reviewer=reviewer)
+        if reviewassignment_set.exists() and not reviewassignment_set.filter(state_id='rejected').exists():
+            return
 
     # Note that assigning a review no longer unassigns other reviews
 

ietf/templates/doc/review/request_info.html

@@ -177,6 +177,8 @@
                         <div class="reviewer-assignment-not-accepted">
                             {% if assignment.state_id == "assigned" %}
                                 Assignment not accepted yet
+                            {% elif assignment.state_id == "rejected" %}
+                                <span class="text-danger">Assignment rejected</span>
                             {% else %}
                                 <span class="text-success">Assignment accepted</span>
                             {% endif %}