fix: Do not add user to the top of queue after reject (fixes #4505) (#5427)

* fix: Do not add user to the top of queue after reject (fixes #4505)
Added a checkbox in the reject review dialog to ask whether user
wants to be added to the top of the queue or not. Default is off.

* Do not change request_assignment_next if wants_to_be_next is not True.
Set the request_assignment_next also in LeastRecentlyUsedReviewerQueuePolicy
so it can be used to override the assignment policy rules (i.e., if
someone has once per month, but he rejects review with wants_to_be_next set
to true, he will get new assignment immediately, not after one month).

* Added wants_to_be_next to test cases too.

* Fixed test function prototypes, they can't have any parameters, only
self. Added test cases for test_return_reviewer_to_rotation_top both
for RotateAlphabeticallyReviewerQueuePolicyTest and
LeastRecentlyUsedReviewerQueuePolicyTest.

ietf/doc/views_review.py

@@ -347,6 +347,7 @@ def assign_reviewer(request, name, request_id):
 
 class RejectReviewerAssignmentForm(forms.Form):
     message_to_secretary = forms.CharField(widget=forms.Textarea, required=False, help_text="Optional explanation of rejection, will be emailed to team secretary if filled in", strip=False)
+    wants_to_be_next = forms.BooleanField(label="I want to be assigned new document immediately", required=False)
 
 @login_required
 def reject_reviewer_assignment(request, name, assignment_id):
@@ -388,11 +389,12 @@ def reject_reviewer_assignment(request, name, assignment_id):
             )
 
             policy = get_reviewer_queue_policy(review_assignment.review_request.team)
-            policy.return_reviewer_to_rotation_top(review_assignment.reviewer.person)
+            policy.return_reviewer_to_rotation_top(review_assignment.reviewer.person, form.cleaned_data['wants_to_be_next'])
 
             msg = render_to_string("review/reviewer_assignment_rejected.txt", {
                 "by": request.user.person,
-                "message_to_secretary": form.cleaned_data.get("message_to_secretary")
+                "message_to_secretary": form.cleaned_data.get("message_to_secretary"),
+                "wants_to_be_next" : form.cleaned_data['wants_to_be_next']
             })
 
             email_review_assignment_change(request, review_assignment, "Reviewer assignment rejected", msg, by=request.user.person, notify_secretary=True, notify_reviewer=True, notify_requested_by=False)
@@ -438,7 +440,7 @@ def withdraw_reviewer_assignment(request, name, assignment_id):
         )            
 
         policy = get_reviewer_queue_policy(review_assignment.review_request.team)
-        policy.return_reviewer_to_rotation_top(review_assignment.reviewer.person)
+        policy.return_reviewer_to_rotation_top(review_assignment.reviewer.person, True)
         
         msg = "Review assignment withdrawn by %s"%request.user.person
 
@@ -1093,4 +1095,4 @@ def _generate_ajax_or_redirect_response(request, doc):
     elif url_is_safe:
         return HttpResponseRedirect(redirect_url)
     else:
-        return HttpResponseRedirect(doc.get_absolute_url())
+        return HttpResponseRedirect(doc.get_absolute_url())

ietf/review/policies.py

@@ -84,7 +84,7 @@ class AbstractReviewerQueuePolicy:
             rotation_list = self._filter_unavailable_reviewers(rotation_list)
         return rotation_list
 
-    def return_reviewer_to_rotation_top(self, reviewer_person):
+    def return_reviewer_to_rotation_top(self, reviewer_person, wants_to_be_next):
         """
         Return a reviewer to the top of the rotation, e.g. because they rejected a review,
         and should retroactively not have been rotated over.
@@ -472,13 +472,14 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy):
     
         return reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index]
 
-    def return_reviewer_to_rotation_top(self, reviewer_person):
+    def return_reviewer_to_rotation_top(self, reviewer_person, wants_to_be_next):
         # As RotateAlphabetically does not keep a full rotation list,
         # returning someone to a particular order is complex.
         # Instead, the "assign me next" flag is set.
-        settings = self._reviewer_settings_for(reviewer_person)
-        settings.request_assignment_next = True
-        settings.save()
+        if wants_to_be_next:
+            settings = self._reviewer_settings_for(reviewer_person)
+            settings.request_assignment_next = wants_to_be_next
+            settings.save()
 
     def _update_skip_next(self, rotation_pks, assignee_person):
         """Decrement skip_next for all users skipped
@@ -566,11 +567,14 @@ class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy):
         rotation_list += reviewers_with_assignment
         return rotation_list
 
-    def return_reviewer_to_rotation_top(self, reviewer_person):
+    def return_reviewer_to_rotation_top(self, reviewer_person, wants_to_be_next):
         # Reviewer rotation for this policy ignores rejected/withdrawn
         # reviews, so it automatically adjusts the position of someone
         # who rejected a review and no further action is needed.
-        pass
+        if wants_to_be_next:
+            settings = self._reviewer_settings_for(reviewer_person)
+            settings.request_assignment_next = wants_to_be_next
+            settings.save()
 
 
 QUEUE_POLICY_NAME_MAPPING = {

ietf/review/tests_policies.py

@@ -509,8 +509,10 @@ class RotateAlphabeticallyReviewerQueuePolicyTest(_Wrapper.ReviewerQueuePolicyTe
 
     def test_return_reviewer_to_rotation_top(self):
         reviewer = self.append_reviewer()
-        self.policy.return_reviewer_to_rotation_top(reviewer)
-        self.assertTrue(ReviewerSettings.objects.get(person=reviewer).request_assignment_next)
+        self.policy.return_reviewer_to_rotation_top(reviewer, False)
+        self.assertFalse(self.reviewer_settings_for(reviewer).request_assignment_next)
+        self.policy.return_reviewer_to_rotation_top(reviewer, True)
+        self.assertTrue(self.reviewer_settings_for(reviewer).request_assignment_next)
 
     def test_update_policy_state_for_assignment(self):
         # make a bunch of reviewers
@@ -724,8 +726,11 @@ class LeastRecentlyUsedReviewerQueuePolicyTest(_Wrapper.ReviewerQueuePolicyTestC
                          available_reviewers[2:] + [first_reviewer, second_reviewer])
 
     def test_return_reviewer_to_rotation_top(self):
-        # Should do nothing, this is implicit in this policy, no state change is needed.
-        self.policy.return_reviewer_to_rotation_top(self.append_reviewer())
+        reviewer = self.append_reviewer()
+        self.policy.return_reviewer_to_rotation_top(reviewer, False)
+        self.assertFalse(self.reviewer_settings_for(reviewer).request_assignment_next)
+        self.policy.return_reviewer_to_rotation_top(reviewer, True)
+        self.assertTrue(self.reviewer_settings_for(reviewer).request_assignment_next)
 
     def test_assign_reviewer_updates_skip_next_without_add_skip(self):
         """Skipping reviewers with add_skip=False should update skip_counts properly"""

ietf/templates/review/reviewer_assignment_rejected.txt

@@ -3,4 +3,9 @@
 Explanation:
 
 {{ message_to_secretary }}
-{% endif %}{% endautoescape %}
+
+{% endif %}
+{% if wants_to_be_next %}
+User requested to be next in queue.
+{% endif %}
+{% endautoescape %}