Fix ietf-tools#2418 - Account for previous rejected reviews in recommended assignment order

 - Legacy-Id: 17053

Author: Sasha Romijn

ietf/doc/tests_review.py

@@ -232,7 +232,6 @@ def test_close_request(self):
         self.assertIn("review_request_close_comment", mail_content)
 
     def test_assign_reviewer(self):
-        # TODO: this test overlaps way too much with the reviewer policy
         doc = WgDraftFactory(pages=2)
         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')

ietf/review/policies.py

@@ -182,7 +182,8 @@ def _collect_context(self):
         self.rotation_index = {p.pk: i for i, p in enumerate(self.rotation_list)}
 
         # This data is collected as a set of person IDs.
-        self.has_reviewed_previous = self._persons_with_previous_review(self.review_req, self.possible_person_ids)
+        self.has_completed_review_previous = self._persons_with_previous_review(self.review_req, self.possible_person_ids, 'completed')
+        self.has_rejected_review_previous = self._persons_with_previous_review(self.review_req, self.possible_person_ids, 'rejected')
         self.wish_to_review = set(ReviewWish.objects.filter(team=self.team, person__in=self.possible_person_ids,
                                                        doc=self.doc).values_list("person", flat=True))
 
@@ -227,7 +228,7 @@ def add_boolean_score(direction, expr, explanation=None):
         # If a reviewer is unavailable, they are ignored.
         periods = self.unavailable_periods.get(email.person_id, [])
         unavailable_at_the_moment = periods and not (
-            email.person_id in self.has_reviewed_previous and
+            email.person_id in self.has_completed_review_previous and
             all(p.availability == "canfinish" for p in periods)
         )
         if unavailable_at_the_moment:
@@ -242,8 +243,9 @@ def format_period(p):
         if periods:
             explanations.append(", ".join(format_period(p) for p in periods))
 
+        add_boolean_score(-1, email.person_id in self.has_rejected_review_previous, "rejected review of document before")
         add_boolean_score(+1, settings.request_assignment_next, "requested to be selected next for assignment")
-        add_boolean_score(+1, email.person_id in self.has_reviewed_previous, "reviewed document before")
+        add_boolean_score(+1, email.person_id in self.has_completed_review_previous, "reviewed document before")
         add_boolean_score(+1, email.person_id in self.wish_to_review, "wishes to review document")
         add_boolean_score(-1, email.person_id in self.connections,
                           self.connections.get(email.person_id))  # reviewer is somehow connected: bad
@@ -324,7 +326,7 @@ def _connections_with_doc(self, doc, person_ids):
             connections[author] = "is author of document"
         return connections
 
-    def _persons_with_previous_review(self, review_req, possible_person_ids):
+    def _persons_with_previous_review(self, review_req, possible_person_ids, state_id):
         """
         Collect anyone in possible_person_ids that have reviewed the document before,
         or an ancestor document.
@@ -334,7 +336,7 @@ def _persons_with_previous_review(self, review_req, possible_person_ids):
         has_reviewed_previous = ReviewRequest.objects.filter(
             doc__name__in=doc_names,
             reviewassignment__reviewer__person__in=possible_person_ids,
-            reviewassignment__state="completed",
+            reviewassignment__state=state_id,
             team=self.team,
         ).distinct()
         if review_req.pk is not None:

ietf/review/test_policies.py

@@ -301,6 +301,8 @@ def test_determine_ranking(self):
             start_date='2000-01-01',
             availability='canfinish',
         )
+        # Trigger "reviewer has rejected before"
+        ReviewAssignmentFactory(review_request__team=team, review_request__doc=doc, reviewer=reviewer_low.email(), state_id='rejected')
 
         # Trigger max frequency and open review stats
         ReviewAssignmentFactory(review_request__team=team, reviewer=reviewer_low.email(), state_id='assigned', review_request__doc__pages=10)
@@ -324,7 +326,7 @@ def test_determine_ranking(self):
         self.assertEqual(len(ranking), 2)
         self.assertEqual(ranking[0]['email'], reviewer_high.email())
         self.assertEqual(ranking[1]['email'], reviewer_low.email())
-        self.assertEqual(ranking[0]['scores'], [ 1,  1,  1,  1,  1,   0,  0, -1])
-        self.assertEqual(ranking[1]['scores'], [-1, -1, -1, -1, -1, -91, -2,  0])
+        self.assertEqual(ranking[0]['scores'], [ 1,  1,  1,  1,  1,  1,   0,  0, -1])
+        self.assertEqual(ranking[1]['scores'], [-1, -1, -1, -1, -1, -1, -91, -2,  0])
         self.assertEqual(ranking[0]['label'], 'Test Reviewer-high: unavailable indefinitely (Can do follow-ups); requested to be selected next for assignment; reviewed document before; wishes to review document; #2; 1 no response, 1 partially complete, 1 fully completed')
-        self.assertEqual(ranking[1]['label'], 'Test Reviewer-low: is author of document; filter regexp matches; max frequency exceeded, ready in 91 days; skip next 2; #1; currently 1 open, 10 pages')
+        self.assertEqual(ranking[1]['label'], 'Test Reviewer-low: rejected review of document before; is author of document; filter regexp matches; max frequency exceeded, ready in 91 days; skip next 2; #1; currently 1 open, 10 pages')