From ce812a3a4f432a3a92bd00b8a922305618b2943a Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 11 Nov 2019 14:47:37 +0000 Subject: [PATCH] - Make skipping unavailable reviews the default, except for the reviewer_overview page. - Make default_reviewer_rotation_list use a consistent return type - Legacy-Id: 16986 --- ietf/doc/tests_review.py | 8 -------- ietf/group/views.py | 2 +- ietf/review/policies.py | 16 +++++++--------- ietf/review/test_policies.py | 20 +++++++++----------- 4 files changed, 17 insertions(+), 29 deletions(-) diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index 221dadfd3..8c3832637 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -280,13 +280,6 @@ class ReviewTests(TestCase): another_reviewer = PersonFactory.create(name = "Extra TestReviewer") # needs to be lexically greater than the existing one another_reviewer.role_set.create(name_id='reviewer', email=another_reviewer.email(), group=review_req.team) - UnavailablePeriod.objects.create( - team=review_req.team, - person=reviewer_email.person, - start_date=datetime.date.today() - datetime.timedelta(days=10), - availability="unavailable", - ) - ReviewWish.objects.create(person=reviewer_email.person, team=review_req.team, doc=doc) # pick a non-existing reviewer as next to see that we can @@ -318,7 +311,6 @@ class ReviewTests(TestCase): self.assertIn("wishes to review", reviewer_label) self.assertIn("is author", reviewer_label) self.assertIn("regexp matches", reviewer_label) - self.assertIn("unavailable indefinitely", reviewer_label) self.assertIn("skip next 1", reviewer_label) self.assertIn("#1", reviewer_label) self.assertIn("1 fully completed", reviewer_label) diff --git a/ietf/group/views.py b/ietf/group/views.py index 6e315db00..1fd2ffd7a 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -1391,7 +1391,7 @@ def reviewer_overview(request, acronym, group_type=None): can_manage = can_manage_review_requests_for_team(request.user, group) - reviewers = get_reviewer_queue_policy(group).default_reviewer_rotation_list() + reviewers = get_reviewer_queue_policy(group).default_reviewer_rotation_list(include_unavailable=True) reviewer_settings = { s.person_id: s for s in ReviewerSettings.objects.filter(team=group) } unavailable_periods = defaultdict(list) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 84fd41353..ba64c9910 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -32,10 +32,9 @@ class AbstractReviewerQueuePolicy: def __init__(self, team): self.team = team - def default_reviewer_rotation_list(self, skip_unavailable=False, dont_skip=[]): + def default_reviewer_rotation_list(self, dont_skip=[]): """ Return a list of reviewers in the default reviewer rotation for a policy. - TODO: fix return types """ raise NotImplementedError @@ -283,8 +282,8 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False): assert assignee_person_id is not None - rotation_list = self.default_reviewer_rotation_list(skip_unavailable=True, - dont_skip=[assignee_person_id]) + rotation_list = [p.id for p in self.default_reviewer_rotation_list( + dont_skip=[assignee_person_id])] def reviewer_at_index(i): if not rotation_list: @@ -324,7 +323,7 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): break - def default_reviewer_rotation_list(self, skip_unavailable=False, dont_skip=[]): + def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip=[]): reviewers = list(Person.objects.filter(role__name="reviewer", role__group=self.team)) reviewers.sort(key=lambda p: p.last_name()) next_reviewer_index = 0 @@ -347,10 +346,9 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index] - reviewers_to_skip = [] - if skip_unavailable: + if not include_unavailable: reviewers_to_skip = self._unavailable_reviewers(dont_skip) - rotation_list = [p.pk for p in rotation_list if p.pk not in reviewers_to_skip] - + rotation_list = [p for p in rotation_list if p.pk not in reviewers_to_skip] + return rotation_list diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index d9646864f..5e1290152 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -22,7 +22,6 @@ class RotateWithSkipReviewerPolicyTests(TestCase): create_person(team, "reviewer", name="Test Reviewer{}".format(i), username="testreviewer{}".format(i)) for i in range(5) ] - reviewers_pks = [r.pk for r in reviewers] # This reviewer should never be included. unavailable_reviewer = create_person(team, "reviewer", name="unavailable reviewer", username="unavailablereviewer") @@ -30,27 +29,26 @@ class RotateWithSkipReviewerPolicyTests(TestCase): team=team, person=unavailable_reviewer, start_date='2000-01-01', - end_date='3000-01-01', availability=UnavailablePeriod.AVAILABILITY_CHOICES[0], ) # Default policy without a NextReviewerInTeam - rotation = policy.default_reviewer_rotation_list(skip_unavailable=True) - self.assertNotIn(unavailable_reviewer.pk, rotation) - self.assertEqual(rotation, reviewers_pks) + rotation = policy.default_reviewer_rotation_list() + self.assertNotIn(unavailable_reviewer, rotation) + self.assertEqual(rotation, reviewers) # Policy with a current NextReviewerInTeam NextReviewerInTeam.objects.create(team=team, next_reviewer=reviewers[3]) - rotation = policy.default_reviewer_rotation_list(skip_unavailable=True) - self.assertNotIn(unavailable_reviewer.pk, rotation) - self.assertEqual(rotation, reviewers_pks[3:] + reviewers_pks[:3]) + rotation = policy.default_reviewer_rotation_list() + self.assertNotIn(unavailable_reviewer, rotation) + self.assertEqual(rotation, reviewers[3:] + reviewers[:3]) # Policy with a NextReviewerInTeam that has left the team. Role.objects.get(person=reviewers[1]).delete() NextReviewerInTeam.objects.filter(team=team).update(next_reviewer=reviewers[1]) - rotation = policy.default_reviewer_rotation_list(skip_unavailable=True) - self.assertNotIn(unavailable_reviewer.pk, rotation) - self.assertEqual(rotation, reviewers_pks[2:] + reviewers_pks[:1]) + rotation = policy.default_reviewer_rotation_list() + self.assertNotIn(unavailable_reviewer, rotation) + self.assertEqual(rotation, reviewers[2:] + reviewers[:1]) def test_update_policy_state_for_assignment(self):