From 8bb6955d47af62b2a757d10f065812735ecd7330 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 18 Nov 2019 19:50:33 +0000 Subject: [PATCH] Fix #2418 - Account for previous rejected reviews in recommended assignment order - Legacy-Id: 17053 --- ietf/doc/tests_review.py | 1 - ietf/review/policies.py | 12 +++++++----- ietf/review/test_policies.py | 8 +++++--- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index eb9f47bcc..4dcf66630 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -232,7 +232,6 @@ class ReviewTests(TestCase): 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') diff --git a/ietf/review/policies.py b/ietf/review/policies.py index cffde3e51..69143fc7e 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -182,7 +182,8 @@ class AssignmentOrderResolver: 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 @@ class AssignmentOrderResolver: # 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 @@ class AssignmentOrderResolver: 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 @@ class AssignmentOrderResolver: 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 @@ class AssignmentOrderResolver: 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: diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 24d077db5..dab5b015a 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -301,6 +301,8 @@ class AssignmentOrderResolverTests(TestCase): 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 @@ class AssignmentOrderResolverTests(TestCase): 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')