From 0c0980c6410ab6a2d1001f2476721da9658466ca Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Tue, 19 Nov 2019 10:57:56 +0000 Subject: [PATCH] Fix #2420 - Add reviewer back to top of the queue after rejected/withdrawn reviews. - Legacy-Id: 17058 --- ietf/doc/views_review.py | 6 ++++++ ietf/review/policies.py | 24 ++++++++++++++++++++++-- ietf/review/test_policies.py | 23 ++++++++++++++++++++--- 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/ietf/doc/views_review.py b/ietf/doc/views_review.py index cb7f28499..4f7a0e57c 100644 --- a/ietf/doc/views_review.py +++ b/ietf/doc/views_review.py @@ -363,6 +363,9 @@ def reject_reviewer_assignment(request, name, assignment_id): state=review_assignment.state, ) + policy = get_reviewer_queue_policy(review_assignment.review_request.team) + policy.return_reviewer_to_top_rotation(review_assignment.reviewer.person) + msg = render_to_string("review/reviewer_assignment_rejected.txt", { "by": request.user.person, "message_to_secretary": form.cleaned_data.get("message_to_secretary") @@ -409,6 +412,9 @@ def withdraw_reviewer_assignment(request, name, assignment_id): state=review_assignment.state, ) + policy = get_reviewer_queue_policy(review_assignment.review_request.team) + policy.return_reviewer_to_top_rotation(review_assignment.reviewer.person) + msg = "Review assignment withdrawn by %s"%request.user.person email_review_assignment_change(request, review_assignment, "Reviewer assignment withdrawn", msg, by=request.user.person, notify_secretary=True, notify_reviewer=True, notify_requested_by=False) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 69143fc7e..f4417b06b 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -50,7 +50,14 @@ class AbstractReviewerQueuePolicy: Return a list of reviewers (Person objects) in the default reviewer rotation for a policy. """ raise NotImplementedError # pragma: no cover - + + def return_reviewer_to_top_rotation(self, reviewer_person): + """ + Return a reviewer to the top of the rotation, e.g. because they rejected a review, + and should retroactively not have been rotated over. + """ + raise NotImplementedError # pragma: no cover + def update_policy_state_for_assignment(self, assignee_person, add_skip=False): """ Update the skip_count if the assignment was in order, and @@ -363,7 +370,6 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy): NextReviewerInTeam is used to store a pointer to where the queue is currently positioned. """ - def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip_person_ids=None): reviewers = list(Person.objects.filter(role__name="reviewer", role__group=self.team)) reviewers.sort(key=lambda p: p.last_name()) @@ -393,6 +399,14 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy): return rotation_list + def return_reviewer_to_top_rotation(self, reviewer_person): + # 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() + class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy): """ @@ -418,6 +432,12 @@ class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy): return rotation_list + def return_reviewer_to_top_rotation(self, reviewer_person): + # Reviewer rotation for this policy ignored rejected/withdrawn + # reviews, so it automatically adjusts the position of someone + # who rejected a review and no action is needed. + pass + QUEUE_POLICY_NAME_MAPPING = { 'RotateAlphabetically': RotateAlphabeticallyReviewerQueuePolicy, diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index dab5b015a..88bbbb485 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -35,9 +35,9 @@ class GetReviewerQueuePolicyTest(TestCase): get_reviewer_queue_policy(team) -class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase): +class RotateAlphabeticallyReviewerAndGenericQueuePolicyTest(TestCase): """ - These tests also cover the common behaviour in RotateAlphabeticallyReviewerQueuePolicy, + These tests also cover the common behaviour in AbstractReviewerQueuePolicy, as that's difficult to test on it's own. """ def test_default_reviewer_rotation_list(self): @@ -197,7 +197,16 @@ class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase): self.assertEqual(get_skip_next(reviewers[3]), 1) self.assertEqual(get_skip_next(reviewers[4]), 0) - + def test_return_reviewer_to_top_rotation(self): + team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", + list_email="rotationteam@ietf.org", + parent=Group.objects.get(acronym="farfut")) + reviewer = create_person(team, "reviewer", name="reviewer", username="reviewer") + policy = RotateAlphabeticallyReviewerQueuePolicy(team) + policy.return_reviewer_to_top_rotation(reviewer) + self.assertTrue(ReviewerSettings.objects.get(person=reviewer).request_assignment_next) + + class LeastRecentlyUsedReviewerQueuePolicyTest(TestCase): """ These tests only cover where this policy deviates from @@ -255,6 +264,14 @@ class LeastRecentlyUsedReviewerQueuePolicyTest(TestCase): self.assertNotIn(unavailable_reviewer, rotation) self.assertEqual(rotation, [reviewers[2], reviewers[3], reviewers[4], reviewers[0], reviewers[1]]) + def test_return_reviewer_to_top_rotation(self): + team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", + list_email="rotationteam@ietf.org", + parent=Group.objects.get(acronym="farfut")) + reviewer = create_person(team, "reviewer", name="reviewer", username="reviewer") + policy = LeastRecentlyUsedReviewerQueuePolicy(team) + policy.return_reviewer_to_top_rotation(reviewer) + class AssignmentOrderResolverTests(TestCase): def test_determine_ranking(self):