From c36fcdc5a7014b6cf863adb089b81308608868c1 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Wed, 13 Nov 2019 14:39:42 +0000 Subject: [PATCH] Update update_policy_state_for_assignment for new policies, fix tests, fix some other minor things. - Legacy-Id: 17023 --- ietf/doc/tests_review.py | 6 +- ietf/review/policies.py | 87 +++++++++++++++----------- ietf/review/test_policies.py | 115 +++++++++++++++-------------------- ietf/review/utils.py | 2 +- 4 files changed, 102 insertions(+), 108 deletions(-) diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index 4ad27b79d..eb9f47bcc 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -239,6 +239,7 @@ class ReviewTests(TestCase): 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') 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_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) - # 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.create( team=review_req.team, @@ -317,7 +316,7 @@ class ReviewTests(TestCase): # assign 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() r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk }) self.assertEqual(r.status_code, 302) @@ -328,7 +327,6 @@ class ReviewTests(TestCase): assignment = review_req.reviewassignment_set.first() self.assertEqual(assignment.reviewer, reviewer) 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('"Some Reviewer" ', outbox[0]["To"]) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index b62e87144..a6b96691a 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -32,9 +32,9 @@ class AbstractReviewerQueuePolicy: def __init__(self, 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 @@ -73,24 +73,29 @@ class AbstractReviewerQueuePolicy: resolver = AssignmentOrderResolver(email_queryset, review_req, self.default_reviewer_rotation_list()) return [(r['email'].pk, r['label']) for r in resolver.determine_ranking()] - def _entirely_unavailable_reviewers(self, dont_skip): - # prune reviewers not in the rotation (but not the assigned - # reviewer who must have been available for assignment anyway) + def _entirely_unavailable_reviewers(self, dont_skip_person_ids=None): + """ + Return a set of PKs of Persons that should not be considered + to be in the rotation list at all. + """ reviewers_to_skip = set() + if not dont_skip_person_ids: + dont_skip_person_ids = [] unavailable_periods = current_unavailable_periods_for_reviewers(self.team) 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) - 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 - + 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): self.review_req = review_req self.doc = review_req.doc @@ -285,51 +290,61 @@ class AssignmentOrderResolver: class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): - def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False): - assert assignee_person_id is not None + def update_policy_state_for_assignment(self, assignee_person, add_skip=False): + print('====================') + assert assignee_person is not None - rotation_list = [p.id for p in self.default_reviewer_rotation_list( - dont_skip=[assignee_person_id])] + rotation_list = self.default_reviewer_rotation_list(dont_skip_person_ids=[assignee_person.pk]) def reviewer_at_index(i): if not rotation_list: return None return rotation_list[i % len(rotation_list)] - def reviewer_settings_for(person_id): - return (ReviewerSettings.objects.filter(team=self.team, person=person_id).first() - or ReviewerSettings(team=self.team, person_id=person_id)) - - if add_skip: - settings = reviewer_settings_for(assignee_person_id) - settings.skip_next += 1 - settings.save() + def reviewer_settings_for(person): + return (ReviewerSettings.objects.filter(team=self.team, person=person).first() + or ReviewerSettings(team=self.team, person=person)) if not rotation_list: 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 - - if assignee_person_id == reviewer_at_index(current_idx): - # Skip the first reviewer in considering who is next. - current_idx += 1 - - while True: - current_reviewer_person_id = reviewer_at_index(current_idx) - settings = reviewer_settings_for(current_reviewer_person_id) + while in_order_assignment: + current_idx_person = reviewer_at_index(current_idx) + settings = reviewer_settings_for(current_idx_person) + print('evaluating {} with skip_next {}, assignee {}'.format(current_idx_person, settings.skip_next, assignee_person)) if settings.skip_next > 0: + print('dropping skip_next') settings.skip_next -= 1 settings.save() - current_idx += 1 - else: + elif current_idx_person != assignee_person: + print('nr appointed') nr = NextReviewerInTeam.objects.filter(team=self.team).first() or NextReviewerInTeam( team=self.team) - nr.next_reviewer_id = current_reviewer_person_id + nr.next_reviewer = current_idx_person nr.save() 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.sort(key=lambda p: p.last_name()) next_reviewer_index = 0 @@ -353,7 +368,7 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index] 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] return rotation_list diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 04f67929a..aa369def6 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -1,15 +1,12 @@ # Copyright The IETF Trust 2016-2019, All Rights Reserved -import datetime - from ietf.doc.factories import WgDraftFactory from ietf.group.factories import ReviewTeamFactory from ietf.group.models import Group, Role from ietf.person.fields import PersonEmailChoiceField from ietf.person.models import Email from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory -from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, \ - ReviewRequest, ReviewWish +from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, ReviewWish from ietf.review.policies import get_reviewer_queue_policy, AssignmentOrderResolver from ietf.utils.test_data import create_person from ietf.utils.test_utils import TestCase @@ -99,7 +96,6 @@ class RotateWithSkipReviewerPolicyTests(TestCase): 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")) policy = get_reviewer_queue_policy(team) - doc = WgDraftFactory() # make a bunch of reviewers reviewers = [ @@ -109,81 +105,66 @@ class RotateWithSkipReviewerPolicyTests(TestCase): self.assertEqual(reviewers, policy.default_reviewer_rotation_list()) - def get_skip_next(person): - settings = (ReviewerSettings.objects.filter(team=team, person=person).first() - or ReviewerSettings(team=team)) - return settings.skip_next + def reviewer_settings_for(person): + return (ReviewerSettings.objects.filter(team=team, person=person).first() + or ReviewerSettings(team=team, person=person)) - 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(get_skip_next(reviewers[0]), 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[3]), 0) + self.assertEqual(get_skip_next(reviewers[4]), 0) - # skip reviewer 2 - policy.update_policy_state_for_assignment(assignee_person_id=reviewers[3].pk, add_skip=True) + # In-order assignment with add_skip + 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(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[2]), 0) 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) + # 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): def test_determine_ranking(self): diff --git a/ietf/review/utils.py b/ietf/review/utils.py index a58c43583..78a09eea2 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -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()) 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( type="assigned_review_request",