altf4arnold/datatracker
1
0
Fork 0
Code Issues Pull requests Actions 3 Packages Projects Releases Wiki Activity

- Make skipping unavailable reviews the default, except for the

Browse source 
reviewer_overview page.
- Make default_reviewer_rotation_list use a consistent return type
 - Legacy-Id: 16986
This commit is contained in:
Sasha Romijn 2019-11-11 14:47:37 +00:00
parent 48f72f2501
commit ce812a3a4f
4 changed files with 17 additions and 29 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
ietf/doc/tests_review.py
View file

@ -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)

2
ietf/group/views.py
View file

@ -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)

16
ietf/review/policies.py
View file

@ -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

20
ietf/review/test_policies.py
View file

@ -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):