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

Update update_policy_state_for_assignment for new policies, fix tests,

Browse source 
fix some other minor things.
 - Legacy-Id: 17023
This commit is contained in:
Sasha Romijn 2019-11-13 14:39:42 +00:00
parent c8812c7193
commit c36fcdc5a7
4 changed files with 102 additions and 108 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
ietf/doc/tests_review.py
View file

@ -239,6 +239,7 @@ class ReviewTests(TestCase):
RoleFactory(group=review_team,person__user__username='marschairman',person__name='WG Cháir Man',name_id='reviewer') RoleFactory(group=review_team,person__user__username='marschairman',person__name='WG Cháir Man',name_id='reviewer')
secretary = RoleFactory(group=review_team,person__user__username='reviewsecretary',person__user__email='reviewsecretary@example.com',name_id='secr') secretary = RoleFactory(group=review_team,person__user__username='reviewsecretary',person__user__email='reviewsecretary@example.com',name_id='secr')
ReviewerSettings.objects.create(team=review_team, person=rev_role.person, min_interval=14, skip_next=0) ReviewerSettings.objects.create(team=review_team, person=rev_role.person, min_interval=14, skip_next=0)
queue_policy = get_reviewer_queue_policy(review_team)
# review to assign to # review to assign to
review_req = ReviewRequestFactory(team=review_team,doc=doc,state_id='requested') review_req = ReviewRequestFactory(team=review_team,doc=doc,state_id='requested')
@ -282,8 +283,6 @@ class ReviewTests(TestCase):
ReviewWish.objects.create(person=reviewer_email.person, team=review_req.team, doc=doc) 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
# handle reviewers who have left
NextReviewerInTeam.objects.filter(team=review_req.team).delete() NextReviewerInTeam.objects.filter(team=review_req.team).delete()
NextReviewerInTeam.objects.create( NextReviewerInTeam.objects.create(
team=review_req.team, team=review_req.team,
@ -317,7 +316,7 @@ class ReviewTests(TestCase):
# assign # assign
empty_outbox() empty_outbox()
rotation_list = get_reviewer_queue_policy(review_req.team).default_reviewer_rotation_list() rotation_list = queue_policy.default_reviewer_rotation_list()
reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[0]).first() reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[0]).first()
r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk }) r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk })
self.assertEqual(r.status_code, 302) self.assertEqual(r.status_code, 302)
@ -328,7 +327,6 @@ class ReviewTests(TestCase):
assignment = review_req.reviewassignment_set.first() assignment = review_req.reviewassignment_set.first()
self.assertEqual(assignment.reviewer, reviewer) self.assertEqual(assignment.reviewer, reviewer)
self.assertEqual(assignment.state_id, "assigned") self.assertEqual(assignment.state_id, "assigned")
self.assertEqual(NextReviewerInTeam.objects.get(team=review_req.team).next_reviewer, rotation_list[1])
self.assertEqual(len(outbox), 1) self.assertEqual(len(outbox), 1)
self.assertEqual('"Some Reviewer" <reviewer@example.com>', outbox[0]["To"]) self.assertEqual('"Some Reviewer" <reviewer@example.com>', outbox[0]["To"])

87
ietf/review/policies.py
View file

@ -32,9 +32,9 @@ class AbstractReviewerQueuePolicy:
def __init__(self, team): def __init__(self, team):
self.team = team self.team = team
def default_reviewer_rotation_list(self, dont_skip=[]): def default_reviewer_rotation_list(self, dont_skip_person_ids=None):
""" """
Return a list of reviewers in the default reviewer rotation for a policy. Return a list of reviewers (Person objects) in the default reviewer rotation for a policy.
""" """
raise NotImplementedError # pragma: no cover raise NotImplementedError # pragma: no cover
@ -73,24 +73,29 @@ class AbstractReviewerQueuePolicy:
resolver = AssignmentOrderResolver(email_queryset, review_req, self.default_reviewer_rotation_list()) resolver = AssignmentOrderResolver(email_queryset, review_req, self.default_reviewer_rotation_list())
return [(r['email'].pk, r['label']) for r in resolver.determine_ranking()] return [(r['email'].pk, r['label']) for r in resolver.determine_ranking()]
def _entirely_unavailable_reviewers(self, dont_skip): def _entirely_unavailable_reviewers(self, dont_skip_person_ids=None):
# prune reviewers not in the rotation (but not the assigned """
# reviewer who must have been available for assignment anyway) Return a set of PKs of Persons that should not be considered
to be in the rotation list at all.
"""
reviewers_to_skip = set() reviewers_to_skip = set()
if not dont_skip_person_ids:
dont_skip_person_ids = []
unavailable_periods = current_unavailable_periods_for_reviewers(self.team) unavailable_periods = current_unavailable_periods_for_reviewers(self.team)
for person_id, periods in unavailable_periods.items(): for person_id, periods in unavailable_periods.items():
if periods and person_id not in dont_skip: if periods and person_id not in dont_skip_person_ids and not any(p.availability == "canfinish" for p in periods):
reviewers_to_skip.add(person_id) reviewers_to_skip.add(person_id)
days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(self.team)
for person_id, days_needed in days_needed_for_reviewers.items():
if person_id not in dont_skip:
reviewers_to_skip.add(person_id)
return reviewers_to_skip return reviewers_to_skip
class AssignmentOrderResolver: class AssignmentOrderResolver:
"""
The AssignmentOrderResolver resolves the "recommended assignment order",
for a set of possible reviewers (email_queryset), a review request, and a
rotation list.
"""
def __init__(self, email_queryset, review_req, rotation_list): def __init__(self, email_queryset, review_req, rotation_list):
self.review_req = review_req self.review_req = review_req
self.doc = review_req.doc self.doc = review_req.doc
@ -285,51 +290,61 @@ class AssignmentOrderResolver:
class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy):
def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False): def update_policy_state_for_assignment(self, assignee_person, add_skip=False):
assert assignee_person_id is not None print('====================')
assert assignee_person is not None
rotation_list = [p.id for p in self.default_reviewer_rotation_list( rotation_list = self.default_reviewer_rotation_list(dont_skip_person_ids=[assignee_person.pk])
dont_skip=[assignee_person_id])]
def reviewer_at_index(i): def reviewer_at_index(i):
if not rotation_list: if not rotation_list:
return None return None
return rotation_list[i % len(rotation_list)] return rotation_list[i % len(rotation_list)]
def reviewer_settings_for(person_id): def reviewer_settings_for(person):
return (ReviewerSettings.objects.filter(team=self.team, person=person_id).first() return (ReviewerSettings.objects.filter(team=self.team, person=person).first()
or ReviewerSettings(team=self.team, person_id=person_id)) or ReviewerSettings(team=self.team, person=person))
if add_skip:
settings = reviewer_settings_for(assignee_person_id)
settings.skip_next += 1
settings.save()
if not rotation_list: if not rotation_list:
return return
rotation_list_without_skip = [r for r in rotation_list if not reviewer_settings_for(r).skip_next]
print('input: {} assigned'.format(assignee_person))
print('with skipped {}'.format([r for r in rotation_list]))
print('without skip {}'.format([r for r in rotation_list_without_skip]))
print('skip counts {}'.format([(r, reviewer_settings_for(r).skip_next) for r in rotation_list]))
in_order_assignment = rotation_list_without_skip[0] == assignee_person
print('in order: {}'.format(in_order_assignment))
# Loop through the list until finding the first person with skip_next=0,
# who is not the current assignee. Anyone with skip_next>0 encountered before
# has their skip_next decreased.
current_idx = 0 current_idx = 0
while in_order_assignment:
if assignee_person_id == reviewer_at_index(current_idx): current_idx_person = reviewer_at_index(current_idx)
# Skip the first reviewer in considering who is next. settings = reviewer_settings_for(current_idx_person)
current_idx += 1 print('evaluating {} with skip_next {}, assignee {}'.format(current_idx_person, settings.skip_next, assignee_person))
while True:
current_reviewer_person_id = reviewer_at_index(current_idx)
settings = reviewer_settings_for(current_reviewer_person_id)
if settings.skip_next > 0: if settings.skip_next > 0:
print('dropping skip_next')
settings.skip_next -= 1 settings.skip_next -= 1
settings.save() settings.save()
current_idx += 1 elif current_idx_person != assignee_person:
else: print('nr appointed')
nr = NextReviewerInTeam.objects.filter(team=self.team).first() or NextReviewerInTeam( nr = NextReviewerInTeam.objects.filter(team=self.team).first() or NextReviewerInTeam(
team=self.team) team=self.team)
nr.next_reviewer_id = current_reviewer_person_id nr.next_reviewer = current_idx_person
nr.save() nr.save()
break break
current_idx += 1
if add_skip:
print('raising skip count for assignee')
settings = reviewer_settings_for(assignee_person)
settings.skip_next += 1
settings.save()
def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip=[]): 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 = list(Person.objects.filter(role__name="reviewer", role__group=self.team))
reviewers.sort(key=lambda p: p.last_name()) reviewers.sort(key=lambda p: p.last_name())
next_reviewer_index = 0 next_reviewer_index = 0
@ -353,7 +368,7 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy):
rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index] rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index]
if not include_unavailable: if not include_unavailable:
reviewers_to_skip = self._entirely_unavailable_reviewers(dont_skip) reviewers_to_skip = self._entirely_unavailable_reviewers(dont_skip_person_ids)
rotation_list = [p 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 return rotation_list

115
ietf/review/test_policies.py
View file

@ -1,15 +1,12 @@
# Copyright The IETF Trust 2016-2019, All Rights Reserved # Copyright The IETF Trust 2016-2019, All Rights Reserved
import datetime
from ietf.doc.factories import WgDraftFactory from ietf.doc.factories import WgDraftFactory
from ietf.group.factories import ReviewTeamFactory from ietf.group.factories import ReviewTeamFactory
from ietf.group.models import Group, Role from ietf.group.models import Group, Role
from ietf.person.fields import PersonEmailChoiceField from ietf.person.fields import PersonEmailChoiceField
from ietf.person.models import Email from ietf.person.models import Email
from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory
from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, \ from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, ReviewWish
ReviewRequest, ReviewWish
from ietf.review.policies import get_reviewer_queue_policy, AssignmentOrderResolver from ietf.review.policies import get_reviewer_queue_policy, AssignmentOrderResolver
from ietf.utils.test_data import create_person from ietf.utils.test_data import create_person
from ietf.utils.test_utils import TestCase from ietf.utils.test_utils import TestCase
@ -99,7 +96,6 @@ class RotateWithSkipReviewerPolicyTests(TestCase):
def test_update_policy_state_for_assignment(self): def test_update_policy_state_for_assignment(self):
team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut")) team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
policy = get_reviewer_queue_policy(team) policy = get_reviewer_queue_policy(team)
doc = WgDraftFactory()
# make a bunch of reviewers # make a bunch of reviewers
reviewers = [ reviewers = [
@ -109,81 +105,66 @@ class RotateWithSkipReviewerPolicyTests(TestCase):
self.assertEqual(reviewers, policy.default_reviewer_rotation_list()) self.assertEqual(reviewers, policy.default_reviewer_rotation_list())
def get_skip_next(person): def reviewer_settings_for(person):
settings = (ReviewerSettings.objects.filter(team=team, person=person).first() return (ReviewerSettings.objects.filter(team=team, person=person).first()
or ReviewerSettings(team=team)) or ReviewerSettings(team=team, person=person))
return settings.skip_next
policy.update_policy_state_for_assignment(assignee_person_id=reviewers[0].pk, add_skip=False) def get_skip_next(person):
return reviewer_settings_for(person).skip_next
# Regular in-order assignment without skips
policy.update_policy_state_for_assignment(assignee_person=reviewers[0], add_skip=False)
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[1]) self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[1])
self.assertEqual(get_skip_next(reviewers[0]), 0) self.assertEqual(get_skip_next(reviewers[0]), 0)
self.assertEqual(get_skip_next(reviewers[1]), 0) self.assertEqual(get_skip_next(reviewers[1]), 0)
policy.update_policy_state_for_assignment(assignee_person_id=reviewers[1].pk, add_skip=True)
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
self.assertEqual(get_skip_next(reviewers[1]), 1)
self.assertEqual(get_skip_next(reviewers[2]), 0) self.assertEqual(get_skip_next(reviewers[2]), 0)
self.assertEqual(get_skip_next(reviewers[3]), 0)
self.assertEqual(get_skip_next(reviewers[4]), 0)
# skip reviewer 2 # In-order assignment with add_skip
policy.update_policy_state_for_assignment(assignee_person_id=reviewers[3].pk, add_skip=True) policy.update_policy_state_for_assignment(assignee_person=reviewers[1], add_skip=True)
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
self.assertEqual(get_skip_next(reviewers[0]), 0) self.assertEqual(get_skip_next(reviewers[0]), 0)
self.assertEqual(get_skip_next(reviewers[1]), 1) # from current add_skip=True
self.assertEqual(get_skip_next(reviewers[2]), 0)
self.assertEqual(get_skip_next(reviewers[3]), 0)
self.assertEqual(get_skip_next(reviewers[4]), 0)
# In-order assignment to 2, but 3 has a skip_next, so 4 should be assigned.
# 3 has skip_next decreased as it is skipped over, 1 retains its skip_next
reviewer3_settings = reviewer_settings_for(reviewers[3])
reviewer3_settings.skip_next = 2
reviewer3_settings.save()
policy.update_policy_state_for_assignment(assignee_person=reviewers[2], add_skip=False)
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4])
self.assertEqual(get_skip_next(reviewers[0]), 0)
self.assertEqual(get_skip_next(reviewers[1]), 1) # from previous add_skip=true
self.assertEqual(get_skip_next(reviewers[2]), 0)
self.assertEqual(get_skip_next(reviewers[3]), 1) # from manually set skip_next - 1
self.assertEqual(get_skip_next(reviewers[4]), 0)
# Out of order assignments, nothing should change,
# except the add_skip=True should still apply
policy.update_policy_state_for_assignment(assignee_person=reviewers[3], add_skip=False)
policy.update_policy_state_for_assignment(assignee_person=reviewers[2], add_skip=False)
policy.update_policy_state_for_assignment(assignee_person=reviewers[1], add_skip=False)
policy.update_policy_state_for_assignment(assignee_person=reviewers[0], add_skip=True)
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4])
self.assertEqual(get_skip_next(reviewers[0]), 1) # from current add_skip=True
self.assertEqual(get_skip_next(reviewers[1]), 1) self.assertEqual(get_skip_next(reviewers[1]), 1)
self.assertEqual(get_skip_next(reviewers[2]), 0) self.assertEqual(get_skip_next(reviewers[2]), 0)
self.assertEqual(get_skip_next(reviewers[3]), 1) self.assertEqual(get_skip_next(reviewers[3]), 1)
# pick reviewer 2, use up reviewer 3's skip_next
policy.update_policy_state_for_assignment(assignee_person_id=reviewers[2].pk, add_skip=False)
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4])
self.assertEqual(get_skip_next(reviewers[0]), 0)
self.assertEqual(get_skip_next(reviewers[1]), 1)
self.assertEqual(get_skip_next(reviewers[2]), 0)
self.assertEqual(get_skip_next(reviewers[3]), 0)
self.assertEqual(get_skip_next(reviewers[4]), 0)
# check wrap-around
policy.update_policy_state_for_assignment(assignee_person_id=reviewers[4].pk)
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[0])
self.assertEqual(get_skip_next(reviewers[0]), 0)
self.assertEqual(get_skip_next(reviewers[1]), 1)
self.assertEqual(get_skip_next(reviewers[2]), 0)
self.assertEqual(get_skip_next(reviewers[3]), 0)
self.assertEqual(get_skip_next(reviewers[4]), 0)
# unavailable
today = datetime.date.today()
UnavailablePeriod.objects.create(team=team, person=reviewers[1], start_date=today, end_date=today, availability="unavailable")
policy.update_policy_state_for_assignment(assignee_person_id=reviewers[0].pk)
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
self.assertEqual(get_skip_next(reviewers[0]), 0)
self.assertEqual(get_skip_next(reviewers[1]), 1) # don't consume that skip while the reviewer is unavailable
self.assertEqual(get_skip_next(reviewers[2]), 0)
self.assertEqual(get_skip_next(reviewers[3]), 0)
self.assertEqual(get_skip_next(reviewers[4]), 0)
# pick unavailable anyway
policy.update_policy_state_for_assignment(assignee_person_id=reviewers[1].pk, add_skip=False)
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
self.assertEqual(get_skip_next(reviewers[0]), 0)
self.assertEqual(get_skip_next(reviewers[1]), 1)
self.assertEqual(get_skip_next(reviewers[2]), 0)
self.assertEqual(get_skip_next(reviewers[3]), 0)
self.assertEqual(get_skip_next(reviewers[4]), 0)
# not through min_interval so advance past reviewer[2]
settings, _ = ReviewerSettings.objects.get_or_create(team=team, person=reviewers[2])
settings.min_interval = 30
settings.save()
req = ReviewRequest.objects.create(team=team, doc=doc, type_id="early", state_id="assigned", deadline=today, requested_by=reviewers[0])
ReviewAssignmentFactory(review_request=req, state_id="accepted", reviewer = reviewers[2].email_set.first(),assigned_on = req.time)
policy.update_policy_state_for_assignment(assignee_person_id=reviewers[3].pk)
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4])
self.assertEqual(get_skip_next(reviewers[0]), 0)
self.assertEqual(get_skip_next(reviewers[1]), 1)
self.assertEqual(get_skip_next(reviewers[2]), 0)
self.assertEqual(get_skip_next(reviewers[3]), 0)
self.assertEqual(get_skip_next(reviewers[4]), 0) self.assertEqual(get_skip_next(reviewers[4]), 0)
# Regular assignment, testing wrap-around
policy.update_policy_state_for_assignment(assignee_person=reviewers[4], add_skip=False)
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
self.assertEqual(get_skip_next(reviewers[0]), 0) # skipped over with this assignment
self.assertEqual(get_skip_next(reviewers[1]), 0) # skipped over with this assignment
self.assertEqual(get_skip_next(reviewers[2]), 0)
self.assertEqual(get_skip_next(reviewers[3]), 1)
self.assertEqual(get_skip_next(reviewers[4]), 0)
class AssignmentOrderResolverTests(TestCase): class AssignmentOrderResolverTests(TestCase):
def test_determine_ranking(self): def test_determine_ranking(self):

2
ietf/review/utils.py
View file

@ -381,7 +381,7 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa
assignment = review_req.reviewassignment_set.create(state_id='assigned', reviewer = reviewer, assigned_on = datetime.datetime.now()) assignment = review_req.reviewassignment_set.create(state_id='assigned', reviewer = reviewer, assigned_on = datetime.datetime.now())
from ietf.review.policies import get_reviewer_queue_policy from ietf.review.policies import get_reviewer_queue_policy
get_reviewer_queue_policy(review_req.team).update_policy_state_for_assignment(reviewer.person_id, add_skip) get_reviewer_queue_policy(review_req.team).update_policy_state_for_assignment(reviewer.person, add_skip)
ReviewRequestDocEvent.objects.create( ReviewRequestDocEvent.objects.create(
type="assigned_review_request", type="assigned_review_request",