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

Merged in [18833] from jennifer@painless-security.com:

Browse source 
Refactor reviewer queue policy handling of 'skip' setting. Fixes #3038.
 - Legacy-Id: 18835
Note: SVN reference [18833] has been migrated to Git commit 92c9f856a4
This commit is contained in:
Robert Sparks 2021-02-16 20:58:00 +00:00
parent daf3e7b895 92c9f856a4
commit 9a73cd272e
5 changed files with 986 additions and 337 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

89
ietf/review/migrations/0027_unique_constraint_for_reviewersettings.py Normal file
View file

@ -0,0 +1,89 @@
# Generated by Django 2.2.17 on 2021-01-24 07:24
from django.db import migrations, models
def forward(apps, schema_editor):
"""Forward migration
Attempts to reconcile and remove duplicate ReviewerSettings
"""
ReviewerSettings = apps.get_model('review', 'ReviewerSettings')
HistoricalReviewerSettings = apps.get_model('review', 'HistoricalReviewerSettings')
def reconcile_duplicate_settings(duplicate_settings, histories):
"""Helper to decide how to handle duplicate settings"""
team = duplicate_settings[0].team
person = duplicate_settings[0].person
assert(all((s.person == person and s.team == team for s in duplicate_settings)))
print('\n>> Found duplicate settings for {}'.format(duplicate_settings[0]))
# In the DB as of Dec 2020, the only duplicate settings sets were pairs where the
# earlier PK had a change history and the latter PK did not. Based on this, assuming
# a change history indicates that the settings are important. If only one has history,
# use that one. If multiple have a history, throw an error.
settings_with_history = [s for s in duplicate_settings if histories[s.pk].count() > 0]
if len(settings_with_history) == 0:
duplicate_settings.sort(key=lambda s: s.pk)
keep = duplicate_settings[-1]
reason = 'chosen by pk'
elif len(settings_with_history) == 1:
keep = settings_with_history[0]
reason = 'chosen because has change history'
else:
# Don't try to guess what to do if multiple settings have change histories
raise RuntimeError(
'Multiple ReviewerSettings with change history for {}. Please resolve manually.'.format(
settings_with_history[0]
)
)
print('>> Keeping pk={} ({})'.format(keep.pk, reason))
for settings in duplicate_settings:
if settings.pk != keep.pk:
print('>> Deleting pk={}'.format(settings.pk))
settings.delete()
# forward migration starts here
if ReviewerSettings.objects.count() == 0:
return # nothing to do
records = dict() # list of records, keyed by (person_id, team_id)
for rs in ReviewerSettings.objects.all().order_by('pk'):
key = (rs.person_id, rs.team_id)
if key in records:
records[key].append(rs)
else:
records[key] = [rs]
for duplicate_settings in records.values():
if len(duplicate_settings) > 1:
histories = dict()
for ds in duplicate_settings:
histories[ds.pk] = HistoricalReviewerSettings.objects.filter(
id=ds.pk
)
reconcile_duplicate_settings(duplicate_settings, histories)
def reverse(apps, schema_editor):
"""Reverse migration
Does nothing, but no harm in reverse migration.
"""
pass
class Migration(migrations.Migration):
dependencies = [
('review', '0026_repair_more_assignments'),
]
operations = [
migrations.RunPython(forward, reverse),
migrations.AddConstraint(
model_name='reviewersettings',
constraint=models.UniqueConstraint(fields=('team', 'person'), name='unique_reviewer_settings_per_team_person'),
),
]

2
ietf/review/models.py
View file

@ -45,6 +45,8 @@ class ReviewerSettings(models.Model):
class Meta:
verbose_name_plural = "reviewer settings"
constraints = [models.UniqueConstraint(fields=['team', 'person'],
name='unique_reviewer_settings_per_team_person')]
class ReviewSecretarySettings(models.Model):
"""Keeps track of admin data associated with a secretary in a team."""

355
ietf/review/policies.py
View file

@ -4,6 +4,8 @@
import re
from django.db.models.aggregates import Max
from django.utils import timezone
from simple_history.utils import bulk_update_with_history
from ietf.doc.models import DocumentAuthor, DocAlias
from ietf.doc.utils import extract_complete_replaces_ancestor_mapping_for_docs
@ -16,6 +18,7 @@ from ietf.review.utils import (current_unavailable_periods_for_reviewers,
days_needed_to_fulfill_min_interval_for_reviewers,
get_default_filter_re,
latest_review_assignments_for_reviewers)
from ietf.utils import log
"""
This file contains policies regarding reviewer queues.
@ -39,15 +42,47 @@ def get_reviewer_queue_policy(team):
return policy(team)
def persons_with_previous_review(team, review_req, possible_person_ids, state_id):
""" Collect anyone in possible_person_ids that have reviewed the document before
Also considers ancestor documents. The possible_person_ids elements can be Person objects or PKs
Returns a set of Person IDs.
"""
doc_names = {review_req.doc.name}.union(*extract_complete_replaces_ancestor_mapping_for_docs([review_req.doc.name]).values())
has_reviewed_previous = ReviewRequest.objects.filter(
doc__name__in=doc_names,
reviewassignment__reviewer__person__in=possible_person_ids,
reviewassignment__state=state_id,
team=team,
).distinct()
if review_req.pk is not None:
has_reviewed_previous = has_reviewed_previous.exclude(pk=review_req.pk)
has_reviewed_previous = set(
has_reviewed_previous.values_list("reviewassignment__reviewer__person", flat=True))
return has_reviewed_previous
class AbstractReviewerQueuePolicy:
def __init__(self, team):
self.team = team
def default_reviewer_rotation_list(self, dont_skip_person_ids=None):
def assign_reviewer(self, review_req, reviewer, add_skip):
"""Assign a reviewer to a request and update policy state accordingly"""
# Update policy state first - needed by LRU policy to correctly compute whether assignment was in-order
self.update_policy_state_for_assignment(review_req, reviewer.person, add_skip)
return review_req.reviewassignment_set.create(state_id='assigned', reviewer=reviewer, assigned_on=timezone.now())
def default_reviewer_rotation_list(self, include_unavailable=False):
""" Return a list of reviewers (Person objects) in the default reviewer rotation for a policy.
Subclasses should pretty much always override this. The default implementation provides the filtering
behavior expected of queue policies.
"""
Return a list of reviewers (Person objects) in the default reviewer rotation for a policy.
"""
raise NotImplementedError # pragma: no cover
# Default is just a filtered list of reviewers for the team, in arbitrary order.
rotation_list = list(Person.objects.filter(role__name="reviewer", role__group=self.team))
if not include_unavailable:
rotation_list = self._filter_unavailable_reviewers(rotation_list)
return rotation_list
def return_reviewer_to_rotation_top(self, reviewer_person):
"""
@ -63,62 +98,68 @@ class AbstractReviewerQueuePolicy:
"""
return [r for r in self.default_reviewer_rotation_list() if not self._reviewer_settings_for(r).skip_next]
def update_policy_state_for_assignment(self, assignee_person, add_skip=False):
"""
Update the skip_count if the assignment was in order, and
update NextReviewerInTeam. Note that NextReviewerInTeam is
not used by all policies.
"""
settings = self._reviewer_settings_for(assignee_person)
settings.request_assignment_next = False
settings.save()
rotation_list = self.default_reviewer_rotation_list(
dont_skip_person_ids=[assignee_person.pk])
def update_policy_state_for_assignment(self, review_req, assignee_person, add_skip=False):
"""Update the skip_count if the assignment was in order."""
self._clear_request_next_assignment(assignee_person)
def reviewer_at_index(i):
if not rotation_list:
return None
return rotation_list[i % len(rotation_list)]
rotation = self._filter_unavailable_reviewers(
self.default_reviewer_rotation_list(include_unavailable=True), # we are going to filter for ourselves
review_req,
)
if not rotation_list:
# Use PKs, not objects, to avoid bugs arising from object identity comparisons
rotation_pks = [r.pk for r in rotation]
if len(rotation_pks) == 0:
return
# assignee_person should be in the rotation list, otherwise they should not have been an option
log.assertion('assignee_person.pk in rotation_pks')
rotation_list_without_skip = self.default_reviewer_rotation_list_without_skipped()
# In order means: assigned to the first person in the rotation list with skip_next=0
# If the assignment is not in order, skip_next and NextReviewerInTeam are not modified.
in_order_assignment = rotation_list_without_skip[0] == assignee_person
# 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. There is a cap on the number of iterations, which can be hit e.g. if there
# is only a single reviewer, and the current assignee is excluded from being set as
# NextReviewerInTeam.
current_idx = 0
max_iter = sum([self._reviewer_settings_for(r).skip_next for r in rotation_list]) + len(rotation_list)
if in_order_assignment:
while current_idx <= max_iter:
current_idx_person = reviewer_at_index(current_idx)
settings = self._reviewer_settings_for(current_idx_person)
if settings.skip_next > 0:
settings.skip_next -= 1
settings.save()
elif current_idx_person != assignee_person:
# NextReviewerInTeam is not used by all policies to determine
# default rotation order, but updated regardless.
nr = NextReviewerInTeam.objects.filter(
team=self.team).first() or NextReviewerInTeam(
team=self.team)
nr.next_reviewer = current_idx_person
nr.save()
break
current_idx += 1
if self._assignment_in_order(rotation_pks, assignee_person):
self._update_skip_next(rotation_pks, assignee_person)
if add_skip:
settings = self._reviewer_settings_for(assignee_person)
settings.skip_next += 1
settings.save()
self._add_skip(assignee_person)
def _update_skip_next(self, rotation_pks, assignee_person):
"""Decrement skip_next for all users skipped"""
assignee_index = rotation_pks.index(assignee_person.pk)
skipped = rotation_pks[0:assignee_index]
skipped_settings = self.team.reviewersettings_set.filter(person__in=skipped) # list of PKs is valid here
for ss in skipped_settings:
ss.skip_next = max(0, ss.skip_next - 1) # ensure we don't go negative
bulk_update_with_history(skipped_settings,
ReviewerSettings,
['skip_next'],
default_change_reason='skipped')
def _assignment_in_order(self, rotation_pks, assignee_person):
"""Is this an in-order assignment?"""
if assignee_person.pk not in rotation_pks:
return False # picking from off the list is not in order
# map from person ID to skip_next
skip_next = dict(
self.team.reviewersettings_set.filter(
person__in=rotation_pks
).values_list('person_id', 'skip_next')
)
rotation_skips = [skip_next.get(pk, 0) for pk in rotation_pks]
min_skip = min(rotation_skips) # usually 0, but not guaranteed
assignee_index = rotation_pks.index(assignee_person.pk)
assignee_skip = rotation_skips[assignee_index]
# If the assignee should be skipped, the selection is not in order
if assignee_skip != min_skip:
return False
# If any of the preceding reviewers should not have been skipped, the selection is not in order
for earlier_skip in rotation_skips[0:assignee_index]:
if earlier_skip <= min_skip:
return False
# The selection was in order
return True
# TODO : Change this field to deal with multiple already assigned reviewers???
def setup_reviewer_field(self, field, review_req):
"""
@ -162,28 +203,66 @@ class AbstractReviewerQueuePolicy:
"""
if review_req.team != self.team:
raise ValueError('Reviewer queue policy was passed a review request belonging to a different team.')
resolver = AssignmentOrderResolver(email_queryset, review_req, self.default_reviewer_rotation_list())
resolver = AssignmentOrderResolver(
email_queryset,
review_req,
self._filter_unavailable_reviewers(
self.default_reviewer_rotation_list(include_unavailable=True),
review_req,
)
)
return [(r['email'].pk, r['label']) for r in resolver.determine_ranking()]
def _entirely_unavailable_reviewers(self, dont_skip_person_ids=None):
def _filter_unavailable_reviewers(self, reviewers, review_req=None):
"""Remove any reviewers who are not available for the specified review request
Reviewers who have an unavailability reason of 'unavailable' are always excluded from the
output.
If review_req is specified, 'canfinish' reviewers who have previously completed a review of
the doc in the ReviewRequest will be treated as available.
If no review_req is None, reviewers who are 'canfinish' for *any* review are included in the
output. Only 'unavailable' reviewers are excluded. In this case, the caller must account for
these 'canfinish' reviewers only being available for some reviews.
If multiple UnavailablePeriods apply, a 'canfinish' will take priority over an 'unavailable'.
"""
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_person_ids and not any(p.availability == "canfinish" for p in periods):
reviewers_to_skip.add(person_id)
return reviewers_to_skip
if len(unavailable_periods) == 0:
return reviewers.copy() # nothing to do
available_reviewers = []
if review_req:
previous_reviewers = persons_with_previous_review(self.team,
review_req,
[r.pk for r in reviewers],
'completed')
else:
# treat all reviewers as previous_reviewers if no review_req
previous_reviewers = [r.pk for r in reviewers]
for reviewer in reviewers:
current_periods = unavailable_periods.get(reviewer.pk)
keep = (current_periods is None) or (
'canfinish' in [p.availability for p in current_periods] and reviewer.pk in previous_reviewers
)
if keep:
available_reviewers.append(reviewer)
return available_reviewers
def _clear_request_next_assignment(self, person):
s = self._reviewer_settings_for(person)
s.request_assignment_next = False
s.save()
def _add_skip(self, person):
s = self._reviewer_settings_for(person)
s.skip_next += 1
s.save()
def _reviewer_settings_for(self, person):
return (ReviewerSettings.objects.filter(team=self.team, person=person).first()
or ReviewerSettings(team=self.team, person=person))
return ReviewerSettings.objects.get_or_create(team=self.team, person=person)[0]
class AssignmentOrderResolver:
@ -217,8 +296,12 @@ class AssignmentOrderResolver:
self.unavailable_periods = current_unavailable_periods_for_reviewers(self.team)
# This data is collected as a set of 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.has_completed_review_previous = persons_with_previous_review(
self.team, self.review_req, self.possible_person_ids, 'completed'
)
self.has_rejected_review_previous = persons_with_previous_review(
self.team, 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,12 +310,7 @@ class AssignmentOrderResolver:
Determine the ranking of reviewers.
Returns a list of tuples, each tuple containing an Email pk and an explanation label.
"""
ranking = []
for e in self.possible_emails:
ranking_for_email = self._ranking_for_email(e)
if ranking_for_email:
ranking.append(ranking_for_email)
ranking = [self._ranking_for_email(e) for e in self.possible_emails if e.person_id in self.rotation_index]
ranking.sort(key=lambda r: r["scores"], reverse=True)
return ranking
@ -243,7 +321,11 @@ class AssignmentOrderResolver:
The scores are a list of individual scores, i.e. they are prioritised, not
cumulative; so when comparing scores, elements later in the scores list
will only matter if all earlier scores in the list are equal.
Only valid if email.person_id is in self.rotation_index.
"""
log.assertion('email.person_id in self.rotation_index')
settings = self.reviewer_settings.get(email.person_id)
scores = []
explanations = []
@ -253,18 +335,7 @@ class AssignmentOrderResolver:
if expr and explanation:
explanations.append(explanation)
if email.person_id not in self.rotation_index:
return
# If a reviewer is unavailable at the moment, they are ignored.
periods = self.unavailable_periods.get(email.person_id, [])
unavailable_at_the_moment = periods and not (
email.person_id in self.has_completed_review_previous and
all(p.availability == "canfinish" for p in periods)
)
if unavailable_at_the_moment:
return
def format_period(p):
if p.end_date:
res = "unavailable until {}".format(p.end_date.isoformat())
@ -357,25 +428,6 @@ class AssignmentOrderResolver:
connections[author] = "is author of document"
return connections
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.
Returns a set with Person IDs of anyone who has.
"""
doc_names = {review_req.doc.name}.union(*extract_complete_replaces_ancestor_mapping_for_docs([review_req.doc.name]).values())
has_reviewed_previous = ReviewRequest.objects.filter(
doc__name__in=doc_names,
reviewassignment__reviewer__person__in=possible_person_ids,
reviewassignment__state=state_id,
team=self.team,
).distinct()
if review_req.pk is not None:
has_reviewed_previous = has_reviewed_previous.exclude(pk=review_req.pk)
has_reviewed_previous = set(
has_reviewed_previous.values_list("reviewassignment__reviewer__person", flat=True))
return has_reviewed_previous
def _reviewer_settings_for_person_ids(self, person_ids):
reviewer_settings = {
r.person_id: r
@ -394,8 +446,11 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy):
NextReviewerInTeam is used to store a pointer to where the queue is currently
positioned.
"""
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))
def default_reviewer_rotation_list(self, include_unavailable=False):
reviewers = super(
RotateAlphabeticallyReviewerQueuePolicy, self
).default_reviewer_rotation_list(include_unavailable)
reviewers.sort(key=lambda p: p.last_name())
next_reviewer_index = 0
@ -415,13 +470,7 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy):
else:
next_reviewer_index = reviewers.index(next_reviewer)
rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index]
if not include_unavailable:
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
return reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index]
def return_reviewer_to_rotation_top(self, reviewer_person):
# As RotateAlphabetically does not keep a full rotation list,
@ -431,14 +480,75 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy):
settings.request_assignment_next = True
settings.save()
def _update_skip_next(self, rotation_pks, assignee_person):
"""Decrement skip_next for all users skipped
In addition to the base class behavior, this looks ahead to the next reviewer in the
team and updates NextReviewerInTeam appropriately. Accounts for skip counts along the
way.
"""
super(RotateAlphabeticallyReviewerQueuePolicy, self)._update_skip_next(rotation_pks,
assignee_person)
# All reviewers in the list ahead of the assignee have already had their skip_next
# values decremented. Now need to update NextReviewerInTeam.
# Copy and unfold the rotation list, putting the assignee at the front
unfolded_rotation_pks = rotation_pks.copy()
assignee_index = unfolded_rotation_pks.index(assignee_person.pk)
unfolded_rotation_pks = unfolded_rotation_pks[assignee_index:] + unfolded_rotation_pks[:assignee_index]
# Then remove the assignee
unfolded_rotation_pks.pop(0)
# Nothing to do if the assignee is the only person in the list
if len(unfolded_rotation_pks) == 0:
return
# Get a map from person PK to their settings, if any
rotation_settings = {
settings.person_id: settings
for settings in self.team.reviewersettings_set.filter(person__in=unfolded_rotation_pks)
}
# Update any skip_counts we skip while finding the next reviewer. Handle the case where all skip_count > 0.
if len(rotation_settings) < len(unfolded_rotation_pks):
min_skip_next = 0 # one or more reviewers has no settings object, so they have skip_count=0
else:
min_skip_next = min([rs.skip_next for rs in rotation_settings.values()])
next_reviewer_index = None
for index, pk in enumerate(unfolded_rotation_pks):
rs = rotation_settings.get(pk)
if (rs is None) or (rs.skip_next == min_skip_next):
next_reviewer_index = index
break
else:
rs.skip_next = max(0, rs.skip_next - 1) # ensure never negative
log.assertion('next_reviewer_index is not None') # some entry in the list must have the minimum value
bulk_update_with_history(rotation_settings.values(),
ReviewerSettings,
['skip_next'],
default_change_reason='skipped')
next_reviewer_pk = unfolded_rotation_pks[next_reviewer_index]
NextReviewerInTeam.objects.update_or_create(
team=self.team,
defaults=dict(next_reviewer_id=next_reviewer_pk)
)
class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy):
"""
A policy where the default rotation list is based on the most recent
assigned, accepted or completed review assignment.
"""
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))
def default_reviewer_rotation_list(self, include_unavailable=False):
reviewers = super(
LeastRecentlyUsedReviewerQueuePolicy, self
).default_reviewer_rotation_list(include_unavailable)
reviewers_dict = {p.pk: p for p in reviewers}
assignments = ReviewAssignment.objects.filter(
review_request__team=self.team,
@ -454,11 +564,6 @@ class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy):
rotation_list = sorted(list(reviewers_without_assignment), key=lambda r: r.pk)
rotation_list += reviewers_with_assignment
if not include_unavailable:
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
def return_reviewer_to_rotation_top(self, reviewer_person):

872
ietf/review/tests_policies.py
View file

@ -1,6 +1,9 @@
# Copyright The IETF Trust 2016-2019, All Rights Reserved
import debug # pyflakes:ignore
import datetime
from django.utils import timezone
from ietf.doc.factories import WgDraftFactory, IndividualDraftFactory
from ietf.group.factories import ReviewTeamFactory
@ -13,8 +16,7 @@ from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory
from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, ReviewWish, \
ReviewTeamSettings
from ietf.review.policies import (AssignmentOrderResolver, LeastRecentlyUsedReviewerQueuePolicy,
RotateAlphabeticallyReviewerQueuePolicy,
get_reviewer_queue_policy)
get_reviewer_queue_policy, QUEUE_POLICY_NAME_MAPPING)
from ietf.utils.test_data import create_person
from ietf.utils.test_utils import TestCase
@ -38,140 +40,506 @@ class GetReviewerQueuePolicyTest(TestCase):
get_reviewer_queue_policy(team)
class RotateAlphabeticallyReviewerAndGenericQueuePolicyTest(TestCase):
"""
These tests also cover the common behaviour in AbstractReviewerQueuePolicy,
as that's difficult to test on it's own.
"""
def test_default_reviewer_rotation_list(self):
team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
policy = RotateAlphabeticallyReviewerQueuePolicy(team)
class _Wrapper(TestCase):
"""Wrapper class - exists to prevent UnitTest from trying to run the base class tests"""
reviewers = [
create_person(team, "reviewer", name="Test Reviewer{}".format(i), username="testreviewer{}".format(i))
for i in range(5)
]
def test_all_reviewer_queue_policies_have_tests(self):
"""Every ReviewerQueuePolicy should be tested"""
rqp_test_classes = self.ReviewerQueuePolicyTestCase.__subclasses__()
# This reviewer should never be included.
unavailable_reviewer = create_person(team, "reviewer", name="unavailable reviewer", username="unavailablereviewer")
UnavailablePeriod.objects.create(
team=team,
person=unavailable_reviewer,
start_date='2000-01-01',
availability='unavailable',
self.assertCountEqual(
QUEUE_POLICY_NAME_MAPPING.keys(),
[cls.reviewer_queue_policy_id for cls in rqp_test_classes],
)
# This should not have any impact. Canfinish unavailable reviewers are included in
# the default rotation, and filtered further when making assignment choices.
UnavailablePeriod.objects.create(
team=team,
person=reviewers[1],
start_date='2000-01-01',
availability='canfinish',
)
# Default policy without a NextReviewerInTeam
rotation = policy.default_reviewer_rotation_list()
self.assertNotIn(unavailable_reviewer, rotation)
self.assertEqual(rotation, reviewers)
class ReviewerQueuePolicyTestCase(TestCase):
"""Parent class to define interface / default tests for QueuePolicy implementation tests
To add tests for a new AbstractReviewerQueuePolicy class, you need to:
1. Subclass _Wrapper.ReviewerQueuePolicyTestCase (i.e., this class)
2. Define the reviewer_queue_policy_id class variable in your new class
3. (Maybe) implement a class-specific append_reviewer() method to add a new
reviewer that sorts to the end of default_reviewer_rotation_list()
4. Fill in any tests that raise NotImplemented exceptions
5. Override any other tests that should have different behavior for your new policy
6. Add any policy-specific tests
When adding tests to this default class, be careful not to make assumptions about
the ordering of reviewers. The only guarantee is that append_reviewer() adds a
new reviewer who is later in the default rotation for the next assignment. Once that
assignment is made, the rotation order is entirely unknown! If you need to make
such assumptions, call policy.default_reviewer_rotation_list() or move the test
into a policy-specific subclass.
"""
# Must define reviewer_queue_policy_id in test subclass
reviewer_queue_policy_id = ''
def setUp(self):
self.team = ReviewTeamFactory(acronym="rotationteam",
name="Review Team",
list_email="rotationteam@ietf.org",
parent=Group.objects.get(acronym="farfut"))
self.team.reviewteamsettings.reviewer_queue_policy_id = self.reviewer_queue_policy_id
self.team.reviewteamsettings.save()
self.policy = get_reviewer_queue_policy(self.team)
self.reviewers = []
def append_reviewer(self, skip_count=None):
"""Create a reviewer who will appear in the assignee options list
Newly added reviewer must come later in the default_reviewer_rotation_list. The default
implementation creates users whose names are in lexicographic order.
"""
index = len(self.reviewers)
assert(index < 100) # ordering by label will fail if > 100 reviewers are created
label = '{:02d}'.format(index)
reviewer = create_person(self.team,
'reviewer',
name='Test Reviewer{}'.format(label),
username='testreviewer{}'.format(label))
self.reviewers.append(reviewer)
if skip_count is not None:
settings = self.reviewer_settings_for(reviewer)
settings.skip_next = skip_count
settings.save()
return reviewer
def create_old_review_assignment(self, reviewer, **kwargs):
"""Create a review that won't disturb the ordering of reviewers"""
return ReviewAssignmentFactory(reviewer=reviewer.email(), **kwargs)
def reviewer_settings_for(self, person):
return (ReviewerSettings.objects.filter(team=self.team, person=person).first()
or ReviewerSettings(team=self.team, person=person))
def test_return_reviewer_to_rotation_top(self):
# Subclass must implement this
raise NotImplementedError
def test_default_reviewer_rotation_list_ignores_out_of_team_reviewers(self):
available_reviewers, _ = self.set_up_default_reviewer_rotation_list_test()
# This reviewer has an assignment, but is no longer in the team and should not be in rotation.
out_of_team_reviewer = PersonFactory()
ReviewAssignmentFactory(review_request__team=self.team, reviewer=out_of_team_reviewer.email())
# No known assignments, order in PK order.
rotation = self.policy.default_reviewer_rotation_list()
self.assertNotIn(out_of_team_reviewer, rotation)
self.assertEqual(rotation, available_reviewers)
def test_assign_reviewer(self):
"""assign_reviewer() should create a review assignment for the correct user"""
review_req = ReviewRequestFactory(team=self.team)
for _ in range(3):
self.append_reviewer()
self.assertFalse(review_req.reviewassignment_set.exists())
reviewer = self.reviewers[0]
self.policy.assign_reviewer(review_req, reviewer.email(), add_skip=False)
self.assertCountEqual(
review_req.reviewassignment_set.all().values_list('reviewer', flat=True),
[str(reviewer.email())]
)
self.assertEqual(self.reviewer_settings_for(reviewer).skip_next, 0)
def test_assign_reviewer_and_add_skip(self):
"""assign_reviewer() should create a review assignment for the correct user"""
review_req = ReviewRequestFactory(team=self.team)
for _ in range(3):
self.append_reviewer()
self.assertFalse(review_req.reviewassignment_set.exists())
reviewer = self.reviewers[0]
self.policy.assign_reviewer(review_req, reviewer.email(), add_skip=True)
self.assertCountEqual(
review_req.reviewassignment_set.all().values_list('reviewer', flat=True),
[str(reviewer.email())]
)
self.assertEqual(self.reviewer_settings_for(reviewer).skip_next, 1)
def test_assign_reviewer_updates_skip_next_minimal(self):
"""If we skip the first reviewer, their skip_next value should decrement
Different policies handle skipping in different ways.
The only assumption we make in the base test class is that an in-order assignment
to a non-skipped reviewer will decrement the skip_next for any reviewers we skipped.
Any other tests are policy-specific (e.g., the RotateAlphabetically policy will
also decrement any users skipped between the assignee and the next reviewer in the
rotation)
"""
review_req = ReviewRequestFactory(team=self.team)
reviewer_to_skip = self.append_reviewer()
settings = self.reviewer_settings_for(reviewer_to_skip)
settings.skip_next = 1
settings.save()
another_reviewer_to_skip = self.append_reviewer()
settings = self.reviewer_settings_for(another_reviewer_to_skip)
settings.skip_next = 1
settings.save()
reviewer_to_assign = self.append_reviewer()
reviewer_to_ignore = self.append_reviewer()
# Check test assumptions
self.assertEqual(
self.policy.default_reviewer_rotation_list(),
[
reviewer_to_skip,
another_reviewer_to_skip,
reviewer_to_assign,
reviewer_to_ignore,
],
)
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip).skip_next, 1)
self.assertEqual(self.reviewer_settings_for(another_reviewer_to_skip).skip_next, 1)
self.assertEqual(self.reviewer_settings_for(reviewer_to_assign).skip_next, 0)
self.policy.assign_reviewer(review_req, reviewer_to_assign.email(), add_skip=False)
# Check results
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip).skip_next, 0,
'skip_next not updated for first skipped reviewer')
self.assertEqual(self.reviewer_settings_for(another_reviewer_to_skip).skip_next, 0,
'skip_next not updated for second skipped reviewer')
def test_assign_reviewer_updates_skip_next_with_add_skip(self):
"""Skipping reviewers with add_skip=True should update skip_counts properly
Subclasses must implement
"""
raise NotImplementedError
def test_assign_reviewer_updates_skip_next_without_add_skip(self):
"""Skipping reviewers with add_skip=False should update skip_counts properly
Subclasses must implement
"""
raise NotImplementedError
def test_assign_reviewer_ignores_skip_next_on_out_of_order_assignment(self):
"""If assignment is not in-order, skip_next values should not change"""
review_req = ReviewRequestFactory(team=self.team)
reviewer_to_skip = self.append_reviewer()
settings = self.reviewer_settings_for(reviewer_to_skip)
settings.skip_next = 1
settings.save()
reviewer_to_ignore = self.append_reviewer()
reviewer_to_assign = self.append_reviewer()
another_reviewer_to_skip = self.append_reviewer()
settings = self.reviewer_settings_for(another_reviewer_to_skip)
settings.skip_next = 3
settings.save()
# Check test assumptions
self.assertEqual(
self.policy.default_reviewer_rotation_list(),
[
reviewer_to_skip,
reviewer_to_ignore,
reviewer_to_assign,
another_reviewer_to_skip,
],
)
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip).skip_next, 1)
self.assertEqual(self.reviewer_settings_for(reviewer_to_ignore).skip_next, 0)
self.assertEqual(self.reviewer_settings_for(reviewer_to_assign).skip_next, 0)
self.assertEqual(self.reviewer_settings_for(another_reviewer_to_skip).skip_next, 3)
self.policy.assign_reviewer(review_req, reviewer_to_assign.email(), add_skip=False)
# Check results
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip).skip_next, 1,
'skip_next changed unexpectedly for first skipped reviewer')
self.assertEqual(self.reviewer_settings_for(reviewer_to_ignore).skip_next, 0,
'skip_next changed unexpectedly for ignored reviewer')
self.assertEqual(self.reviewer_settings_for(reviewer_to_assign).skip_next, 0,
'skip_next changed unexpectedly for assigned reviewer')
self.assertEqual(self.reviewer_settings_for(another_reviewer_to_skip).skip_next, 3,
'skip_next changed unexpectedly for second skipped reviewer')
def test_assign_reviewer_updates_skip_next_when_canfinish_other_doc(self):
"""Should update skip_next when 'canfinish' set for someone unrelated to this doc"""
completed_req = ReviewRequestFactory(team=self.team, state_id='assigned')
assigned_req = ReviewRequestFactory(team=self.team, state_id='assigned')
new_req = ReviewRequestFactory(team=self.team, doc=assigned_req.doc)
reviewer_to_skip = self.append_reviewer()
settings = self.reviewer_settings_for(reviewer_to_skip)
settings.skip_next = 1
settings.save()
# Has completed a review of some other document - unavailable for current req
canfinish_reviewer = self.append_reviewer()
UnavailablePeriod.objects.create(
team=self.team,
person=canfinish_reviewer,
start_date='2000-01-01',
availability='canfinish',
)
self.create_old_review_assignment(
reviewer=canfinish_reviewer,
review_request=completed_req,
state_id='completed',
)
# Has no review assignments at all
canfinish_reviewer_no_review = self.append_reviewer()
UnavailablePeriod.objects.create(
team=self.team,
person=canfinish_reviewer_no_review,
start_date='2000-01-01',
availability='canfinish',
)
# Has accepted but not completed a review of this document
canfinish_reviewer_no_completed = self.append_reviewer()
UnavailablePeriod.objects.create(
team=self.team,
person=canfinish_reviewer_no_completed,
start_date='2000-01-01',
availability='canfinish',
)
self.create_old_review_assignment(
reviewer=canfinish_reviewer_no_completed,
review_request=assigned_req,
state_id='accepted',
)
reviewer_to_assign = self.append_reviewer()
self.assertEqual(
self.policy.default_reviewer_rotation_list(),
[
reviewer_to_skip,
canfinish_reviewer,
canfinish_reviewer_no_review,
canfinish_reviewer_no_completed,
reviewer_to_assign
],
'Test logic error - reviewers not in expected starting order'
)
# assign the review
self.policy.assign_reviewer(new_req, reviewer_to_assign.email(), add_skip=False)
# Check results
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip).skip_next, 0,
'skip_next not updated for skipped reviewer')
self.assertEqual(self.reviewer_settings_for(canfinish_reviewer).skip_next, 0,
'skip_next changed unexpectedly for "canfinish" unavailable reviewer')
self.assertEqual(self.reviewer_settings_for(canfinish_reviewer_no_review).skip_next, 0,
'skip_next changed unexpectedly for "canfinish" unavailable reviewer with no review')
self.assertEqual(self.reviewer_settings_for(canfinish_reviewer_no_completed).skip_next, 0,
'skip_next changed unexpectedly for "canfinish" unavailable reviewer with no completed review')
self.assertEqual(self.reviewer_settings_for(reviewer_to_assign).skip_next, 0,
'skip_next changed unexpectedly for assigned reviewer')
def test_assign_reviewer_ignores_skip_next_when_canfinish_this_doc(self):
"""Should not update skip_next when 'canfinish' set for prior reviewer of current req
If a reviewer is unavailable but 'canfinish' and has previously completed a review of this
doc, they are a candidate to be assigned to it. In that case, when skip_next == 0, skipping
over them means the assignment was not 'in order' and skip_next should not be updated.
"""
completed_req = ReviewRequestFactory(team=self.team, state_id='assigned')
new_req = ReviewRequestFactory(team=self.team, doc=completed_req.doc)
reviewer_to_skip = self.append_reviewer()
settings = self.reviewer_settings_for(reviewer_to_skip)
settings.skip_next = 1
settings.save()
canfinish_reviewer = self.append_reviewer()
UnavailablePeriod.objects.create(
team=self.team,
person=canfinish_reviewer,
start_date='2000-01-01',
availability='canfinish',
)
self.create_old_review_assignment(
reviewer=canfinish_reviewer,
review_request=completed_req,
state_id='completed',
)
reviewer_to_assign = self.append_reviewer()
self.assertEqual(self.policy.default_reviewer_rotation_list(),
[reviewer_to_skip, canfinish_reviewer, reviewer_to_assign],
'Test logic error - reviewers not in expected starting order')
# assign the review
self.policy.assign_reviewer(new_req, reviewer_to_assign.email(), add_skip=False)
# Check results
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip).skip_next, 1,
'skip_next changed unexpectedly for skipped reviewer')
self.assertEqual(self.reviewer_settings_for(canfinish_reviewer).skip_next, 0,
'skip_next changed unexpectedly for "canfinish" reviewer')
self.assertEqual(self.reviewer_settings_for(reviewer_to_assign).skip_next, 0,
'skip_next changed unexpectedly for assigned reviewer')
def set_up_default_reviewer_rotation_list_test(self):
"""Helper to set up for the test_default_reviewer_rotation_list test and related tests"""
for i in range(5):
self.append_reviewer()
# This reviewer should never be included.
unavailable_reviewer = self.append_reviewer()
UnavailablePeriod.objects.create(
team=self.team,
person=unavailable_reviewer,
start_date='2000-01-01',
availability='unavailable',
)
# This should not have any impact. Canfinish unavailable reviewers are included in
# the default rotation, and filtered further when making assignment choices.
UnavailablePeriod.objects.create(
team=self.team,
person=self.reviewers[1],
start_date='2000-01-01',
availability='canfinish',
)
return (
[r for r in self.reviewers if r is not unavailable_reviewer], # available reviewers
unavailable_reviewer,
)
def test_default_reviewer_rotation_list(self):
available_reviewers, unavailable_reviewer = self.set_up_default_reviewer_rotation_list_test()
rotation = self.policy.default_reviewer_rotation_list()
self.assertNotIn(unavailable_reviewer, rotation)
self.assertEqual(rotation, available_reviewers)
def test_recommended_assignment_order(self):
reviewer_low = self.append_reviewer()
reviewer_high = self.append_reviewer()
# reviewer_high appears later in the default rotation, but reviewer_low is the author
doc = WgDraftFactory(group__acronym='mars', rev='01', authors=[reviewer_low])
review_req = ReviewRequestFactory(doc=doc, team=self.team, type_id='early')
order = self.policy.recommended_assignment_order(Email.objects.all(), review_req)
self.assertEqual(order[0][0], str(reviewer_high.email()))
self.assertEqual(order[1][0], str(reviewer_low.email()))
self.assertIn('{}: #2'.format(reviewer_high.name), order[0][1])
self.assertIn('{}: is author of document; #1'.format(reviewer_low.name), order[1][1])
with self.assertRaises(ValueError):
review_req_other_team = ReviewRequestFactory(doc=doc, type_id='early')
self.policy.recommended_assignment_order(Email.objects.all(), review_req_other_team)
def test_setup_reviewer_field(self):
review_req = ReviewRequestFactory(team=self.team, type_id='early')
reviewer = self.append_reviewer()
partial_reviewer = self.append_reviewer()
ReviewAssignmentFactory(review_request=review_req,
reviewer=partial_reviewer.email(),
state_id='part-completed')
rejected_reviewer = self.append_reviewer()
ReviewAssignmentFactory(review_request=ReviewRequestFactory(team=self.team,
type_id='early',
doc=review_req.doc),
reviewer=rejected_reviewer.email(),
state_id='rejected')
no_response_reviewer = self.append_reviewer()
ReviewAssignmentFactory(review_request=ReviewRequestFactory(team=self.team,
type_id='early',
doc=review_req.doc),
reviewer=no_response_reviewer.email(),
state_id='no-response')
field = PersonEmailChoiceField(label="Assign Reviewer", empty_label="(None)", required=False)
self.policy.setup_reviewer_field(field, review_req)
addresses = list( map( lambda choice: choice[0], field.choices ) )
self.assertNotIn(
str(rejected_reviewer.email()), addresses,
"Reviews should not suggest people who have rejected this request in the past")
self.assertNotIn(
str(no_response_reviewer.email()), addresses,
"Reviews should not suggest people who have not responded to this request in the past.")
self.assertEqual(field.initial, str(partial_reviewer.email()))
self.assertEqual(field.choices[0], ('', '(None)'))
self.assertEqual(field.choices[1][0], str(reviewer.email()))
self.assertEqual(field.choices[2][0], str(partial_reviewer.email()))
self.assertIn('{}: #1'.format(reviewer.name), field.choices[1][1])
self.assertIn('{}: #2'.format(partial_reviewer.name), field.choices[2][1])
self.assertIn('1 partially complete', field.choices[2][1])
class RotateAlphabeticallyReviewerQueuePolicyTest(_Wrapper.ReviewerQueuePolicyTestCase):
reviewer_queue_policy_id = 'RotateAlphabetically'
def test_default_reviewer_rotation_list_with_nextreviewerinteam(self):
available_reviewers, _ = self.set_up_default_reviewer_rotation_list_test()
assert(len(available_reviewers) > 4)
# Policy with a current NextReviewerInTeam
NextReviewerInTeam.objects.create(team=team, next_reviewer=reviewers[3])
rotation = policy.default_reviewer_rotation_list()
self.assertNotIn(unavailable_reviewer, rotation)
self.assertEqual(rotation, reviewers[3:] + reviewers[:3])
NextReviewerInTeam.objects.create(team=self.team, next_reviewer=available_reviewers[3])
rotation = self.policy.default_reviewer_rotation_list()
self.assertEqual(rotation, available_reviewers[3:] + available_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()
self.assertNotIn(unavailable_reviewer, rotation)
self.assertEqual(rotation, reviewers[2:] + reviewers[:1])
def test_setup_reviewer_field(self):
team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
policy = RotateAlphabeticallyReviewerQueuePolicy(team)
reviewer_0 = create_person(team, "reviewer", name="Test Reviewer-0", username="testreviewer0")
reviewer_1 = create_person(team, "reviewer", name="Test Reviewer-1", username="testreviewer1")
reviewer_2 = create_person(team, "reviewer", name="Test Reviewer-2", username="testreviewer2")
reviewer_3 = create_person(team, "reviewer", name="Test Reviewer-3", username="testreviewer3")
review_req = ReviewRequestFactory(team=team, type_id='early')
review_req_2 = ReviewRequestFactory(team=team, type_id='early', doc=review_req.doc)
review_req_3 = ReviewRequestFactory(team=team, type_id='early', doc=review_req.doc)
ReviewAssignmentFactory(review_request=review_req, reviewer=reviewer_1.email(), state_id='part-completed')
ReviewAssignmentFactory(review_request=review_req_2, reviewer=reviewer_2.email(), state_id='rejected')
ReviewAssignmentFactory(review_request=review_req_3, reviewer=reviewer_3.email(), state_id='no-response')
field = PersonEmailChoiceField(label="Assign Reviewer", empty_label="(None)", required=False)
policy.setup_reviewer_field(field, review_req)
addresses = list( map( lambda choice: choice[0], field.choices ) )
self.assertNotIn(
str(reviewer_2.email()), addresses,
"Reviews should not suggest people who have rejected this request in the past")
self.assertNotIn(
str(reviewer_3.email()), addresses,
"Reviews should not suggest people who have not responded to this request in the past.")
self.assertEqual(field.choices[0], ('', '(None)'))
self.assertEqual(field.choices[1][0], str(reviewer_0.email()))
self.assertEqual(field.choices[2][0], str(reviewer_1.email()))
self.assertEqual(field.choices[1][1], 'Test Reviewer-0: #1')
self.assertEqual(field.choices[2][1], 'Test Reviewer-1: #2; 1 partially complete')
self.assertEqual(field.initial, str(reviewer_1.email()))
def test_recommended_assignment_order(self):
team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
policy = RotateAlphabeticallyReviewerQueuePolicy(team)
reviewer_high = create_person(team, "reviewer", name="Test Reviewer-1-high", username="testreviewerhigh")
reviewer_low = create_person(team, "reviewer", name="Test Reviewer-0-low", username="testreviewerlow")
# reviewer_high appears later in the default rotation, but reviewer_low is the author
doc = WgDraftFactory(group__acronym='mars', rev='01', authors=[reviewer_low])
review_req = ReviewRequestFactory(doc=doc, team=team, type_id='early')
Role.objects.get(person=available_reviewers[1]).delete()
NextReviewerInTeam.objects.filter(team=self.team).update(next_reviewer=available_reviewers[1])
rotation = self.policy.default_reviewer_rotation_list()
self.assertEqual(rotation, available_reviewers[2:] + available_reviewers[:1])
order = policy.recommended_assignment_order(Email.objects.all(), review_req)
self.assertEqual(order[0][0], str(reviewer_high.email()))
self.assertEqual(order[1][0], str(reviewer_low.email()))
self.assertEqual(order[0][1], 'Test Reviewer-1-high: #2')
self.assertEqual(order[1][1], 'Test Reviewer-0-low: is author of document; #1')
with self.assertRaises(ValueError):
review_req_other_team = ReviewRequestFactory(doc=doc, type_id='early')
policy.recommended_assignment_order(Email.objects.all(), review_req_other_team)
def test_return_reviewer_to_rotation_top(self):
reviewer = self.append_reviewer()
self.policy.return_reviewer_to_rotation_top(reviewer)
self.assertTrue(ReviewerSettings.objects.get(person=reviewer).request_assignment_next)
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 = RotateAlphabeticallyReviewerQueuePolicy(team)
# make a bunch of reviewers
reviewers = [
create_person(team, "reviewer", name="Test Reviewer{}".format(i), username="testreviewer{}".format(i))
for i in range(5)
]
review_req = ReviewRequestFactory(team=self.team)
for i in range(5):
self.append_reviewer()
reviewers = self.reviewers
self.assertEqual(reviewers, policy.default_reviewer_rotation_list())
def reviewer_settings_for(person):
return (ReviewerSettings.objects.filter(team=team, person=person).first()
or ReviewerSettings(team=team, person=person))
self.assertEqual(reviewers, self.policy.default_reviewer_rotation_list())
def get_skip_next(person):
return reviewer_settings_for(person).skip_next
return self.reviewer_settings_for(person).skip_next
# Regular in-order assignment without skips
reviewer0_settings = reviewer_settings_for(reviewers[0])
reviewer0_settings = self.reviewer_settings_for(reviewers[0])
reviewer0_settings.request_assignment_next = True
reviewer0_settings.save()
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.policy.update_policy_state_for_assignment(review_req, assignee_person=reviewers[0], add_skip=False)
self.assertEqual(NextReviewerInTeam.objects.get(team=self.team).next_reviewer, reviewers[1])
self.assertEqual(get_skip_next(reviewers[0]), 0)
self.assertEqual(get_skip_next(reviewers[1]), 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)
# request_assignment_next should be reset after any assignment
self.assertFalse(reviewer_settings_for(reviewers[0]).request_assignment_next)
self.assertFalse(self.reviewer_settings_for(reviewers[0]).request_assignment_next)
# 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.policy.update_policy_state_for_assignment(review_req, assignee_person=reviewers[1], add_skip=True)
self.assertEqual(NextReviewerInTeam.objects.get(team=self.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)
@ -180,11 +548,11 @@ class RotateAlphabeticallyReviewerAndGenericQueuePolicyTest(TestCase):
# 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 = self.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.policy.update_policy_state_for_assignment(review_req, assignee_person=reviewers[2], add_skip=False)
self.assertEqual(NextReviewerInTeam.objects.get(team=self.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)
@ -193,20 +561,20 @@ class RotateAlphabeticallyReviewerAndGenericQueuePolicyTest(TestCase):
# 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.policy.update_policy_state_for_assignment(review_req, assignee_person=reviewers[3], add_skip=False)
self.policy.update_policy_state_for_assignment(review_req, assignee_person=reviewers[2], add_skip=False)
self.policy.update_policy_state_for_assignment(review_req, assignee_person=reviewers[1], add_skip=False)
self.policy.update_policy_state_for_assignment(review_req, assignee_person=reviewers[0], add_skip=True)
self.assertEqual(NextReviewerInTeam.objects.get(team=self.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)
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.policy.update_policy_state_for_assignment(review_req, assignee_person=reviewers[4], add_skip=False)
self.assertEqual(NextReviewerInTeam.objects.get(team=self.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)
@ -216,96 +584,193 @@ class RotateAlphabeticallyReviewerAndGenericQueuePolicyTest(TestCase):
# Leave only a single reviewer remaining, which should not trigger an infinite loop.
# The deletion also causes NextReviewerInTeam to be deleted.
[reviewer.delete() for reviewer in reviewers[1:]]
self.assertEqual([reviewers[0]], policy.default_reviewer_rotation_list())
policy.update_policy_state_for_assignment(assignee_person=reviewers[0], add_skip=False)
self.assertEqual([reviewers[0]], self.policy.default_reviewer_rotation_list())
self.policy.update_policy_state_for_assignment(review_req, assignee_person=reviewers[0], add_skip=False)
# No NextReviewerInTeam should be created, the only possible next is the excluded assignee.
self.assertFalse(NextReviewerInTeam.objects.filter(team=team))
self.assertEqual([reviewers[0]], policy.default_reviewer_rotation_list())
self.assertFalse(NextReviewerInTeam.objects.filter(team=self.team))
self.assertEqual([reviewers[0]], self.policy.default_reviewer_rotation_list())
def test_return_reviewer_to_rotation_top(self):
team = ReviewTeamFactory(acronym="rotationteam", name="Review Team",
list_email="rotationteam@ietf.org",
parent=Group.objects.get(acronym="farfut"))
reviewer = create_person(team, "reviewer", name="reviewer", username="reviewer")
policy = RotateAlphabeticallyReviewerQueuePolicy(team)
policy.return_reviewer_to_rotation_top(reviewer)
self.assertTrue(ReviewerSettings.objects.get(person=reviewer).request_assignment_next)
class LeastRecentlyUsedReviewerQueuePolicyTest(TestCase):
def test_assign_reviewer_updates_skip_next_without_add_skip(self):
"""Skipping reviewers with add_skip=False should update skip_counts properly"""
review_req = ReviewRequestFactory(team=self.team)
reviewer_to_skip = self.append_reviewer(skip_count=1)
reviewer_to_assign = self.append_reviewer(skip_count=0)
reviewer_to_skip_later = self.append_reviewer(skip_count=1)
# Check test assumptions
self.assertEqual(
self.policy.default_reviewer_rotation_list(),
[reviewer_to_skip, reviewer_to_assign, reviewer_to_skip_later],
)
self.policy.assign_reviewer(review_req, reviewer_to_assign.email(), add_skip=False)
# Check results
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip).skip_next, 0,
'skip_next not updated for skipped reviewer')
self.assertEqual(self.reviewer_settings_for(reviewer_to_assign).skip_next, 0,
'skip_next changed unexpectedly for assigned reviewer')
# Expect to skip the later reviewer when updating NextReviewerInTeam
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip_later).skip_next, 0,
'skip_next not updated for reviewer to skip later')
def test_assign_reviewer_updates_skip_next_with_add_skip(self):
"""Skipping reviewers with add_skip=True should update skip_counts properly"""
review_req = ReviewRequestFactory(team=self.team)
reviewer_to_skip = self.append_reviewer(skip_count=1)
reviewer_to_assign = self.append_reviewer(skip_count=0)
reviewer_to_skip_later = self.append_reviewer(skip_count=1)
# Check test assumptions
self.assertEqual(
self.policy.default_reviewer_rotation_list(),
[reviewer_to_skip, reviewer_to_assign, reviewer_to_skip_later],
)
self.policy.assign_reviewer(review_req, reviewer_to_assign.email(), add_skip=True)
# Check results
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip).skip_next, 0,
'skip_next not updated for skipped reviewer')
self.assertEqual(self.reviewer_settings_for(reviewer_to_assign).skip_next, 1,
'skip_next not updated for assigned reviewer')
# Expect to skip the later reviewer when updating NextReviewerInTeam
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip_later).skip_next, 0,
'skip_next not updated for reviewer to skip later')
class LeastRecentlyUsedReviewerQueuePolicyTest(_Wrapper.ReviewerQueuePolicyTestCase):
"""
These tests only cover where this policy deviates from
RotateAlphabeticallyReviewerQueuePolicy - the common behaviour
inherited from AbstractReviewerQueuePolicy is covered in
RotateAlphabeticallyReviewerQueuePolicyTest.
"""
def test_default_reviewer_rotation_list(self):
team = ReviewTeamFactory(acronym="rotationteam", name="Review Team",
list_email="rotationteam@ietf.org",
parent=Group.objects.get(acronym="farfut"))
policy = LeastRecentlyUsedReviewerQueuePolicy(team)
reviewer_queue_policy_id = 'LeastRecentlyUsed'
reviewers = [
create_person(team, "reviewer", name="Test Reviewer{}".format(i),
username="testreviewer{}".format(i))
for i in range(5)
]
# This reviewer should never be included.
unavailable_reviewer = create_person(team, "reviewer", name="unavailable reviewer",
username="unavailablereviewer")
UnavailablePeriod.objects.create(
team=team,
person=unavailable_reviewer,
start_date='2000-01-01',
availability='unavailable',
)
# This should not have any impact. Canfinish unavailable reviewers are included in
# the default rotation, and filtered further when making assignment choices.
UnavailablePeriod.objects.create(
team=team,
person=reviewers[1],
start_date='2000-01-01',
availability='canfinish',
)
# This reviewer has an assignment, but is no longer in the team and should not be in rotation.
out_of_team_reviewer = PersonFactory()
ReviewAssignmentFactory(review_request__team=team, reviewer=out_of_team_reviewer.email())
# No known assignments, order in PK order.
rotation = policy.default_reviewer_rotation_list()
self.assertNotIn(unavailable_reviewer, rotation)
self.assertEqual(rotation, reviewers)
def setUp(self):
super(LeastRecentlyUsedReviewerQueuePolicyTest, self).setUp()
self.last_assigned_on = timezone.now() - datetime.timedelta(days=365)
# Regular accepted assignments. Note that one is older and one is newer than reviewer 0's,
# the newest one should count for ordering, i.e. reviewer 1 should be later in the list.
ReviewAssignmentFactory(reviewer=reviewers[1].email(), assigned_on='2019-01-01',
state_id='accepted', review_request__team=team)
ReviewAssignmentFactory(reviewer=reviewers[1].email(), assigned_on='2017-01-01',
state_id='accepted', review_request__team=team)
# Rejected assignment, should not affect reviewer 2's position
ReviewAssignmentFactory(reviewer=reviewers[2].email(), assigned_on='2020-01-01',
state_id='rejected', review_request__team=team)
# Completed assignments, assigned before the most recent assignment of reviewer 1,
# but completed after (assign date should count).
ReviewAssignmentFactory(reviewer=reviewers[0].email(), assigned_on='2018-01-01',
completed_on='2020-01-01', state_id='completed',
review_request__team=team)
ReviewAssignmentFactory(reviewer=reviewers[0].email(), assigned_on='2018-05-01',
completed_on='2020-01-01', state_id='completed',
review_request__team=team)
rotation = policy.default_reviewer_rotation_list()
self.assertNotIn(unavailable_reviewer, rotation)
self.assertEqual(rotation, [reviewers[2], reviewers[3], reviewers[4], reviewers[0], reviewers[1]])
def append_reviewer(self, skip_count=None):
"""Create a reviewer who will appear in the assignee options list
New reviewer will be last in the default reviewer rotation list.
"""
reviewer = super(LeastRecentlyUsedReviewerQueuePolicyTest, self).append_reviewer(skip_count)
self.create_reviewer_assignment(reviewer)
return reviewer
def create_reviewer_assignment(self, reviewer):
"""Assign reviewer as most recent assignee
Calling this will move a reviewer to the end of the LRU order.
"""
if self.last_assigned_on is None:
assignment = ReviewAssignmentFactory(review_request__team=self.team, reviewer=reviewer.email())
else:
assignment = ReviewAssignmentFactory(
review_request__team=self.team,
reviewer=reviewer.email(),
assigned_on=self.last_assigned_on + datetime.timedelta(days=1)
)
self.last_assigned_on = assignment.assigned_on
return assignment
def create_old_review_assignment(self, reviewer, **kwargs):
"""Create a review that won't disturb the ordering of reviewers"""
# Make a review older than our oldest review
assert('assigned_on' not in kwargs)
kwargs['assigned_on'] = timezone.now() - datetime.timedelta(days=400)
return super(LeastRecentlyUsedReviewerQueuePolicyTest, self).create_old_review_assignment(reviewer, **kwargs)
def test_default_reviewer_rotation_list_uses_latest_assignment(self):
available_reviewers, _ = self.set_up_default_reviewer_rotation_list_test()
assert(len(available_reviewers) > 2) # need enough to avoid wrapping around in a way that invalidates tests
# Give the first reviewer, who would normally appear at the top of the list, a newer assignment
first_reviewer = available_reviewers[0]
self.create_reviewer_assignment(first_reviewer) # creates a new assignment, later assigned_on than all others
self.assertEqual(self.policy.default_reviewer_rotation_list(), available_reviewers[1:] + [first_reviewer])
def test_default_reviewer_rotation_list_ignores_rejected(self):
available_reviewers, _ = self.set_up_default_reviewer_rotation_list_test()
assert(len(available_reviewers) > 2) # need enough to avoid wrapping around in a way that invalidates tests
first_reviewer = available_reviewers[0]
rejected_assignment = self.create_reviewer_assignment(first_reviewer) # assigned_on later than all others...
rejected_assignment.state_id = 'rejected' #... but marked as rejected
rejected_assignment.save()
self.assertEqual(self.policy.default_reviewer_rotation_list(), available_reviewers) # order unchanged
def test_default_review_rotation_list_uses_assigned_on_date(self):
available_reviewers, _ = self.set_up_default_reviewer_rotation_list_test()
assert(len(available_reviewers) > 2) # need enough to avoid wrapping around in a way that invalidates tests
first_reviewer, second_reviewer = available_reviewers[:2]
completed_assignment = self.create_reviewer_assignment(first_reviewer) # moves to the end...
second_reviewer_assignment = self.create_reviewer_assignment(second_reviewer) # moves to the end...
# Mark first_reviewer's assignment as completed after second_reviewer's was assigned
completed_assignment.state_id = 'completed'
completed_assignment.completed_on = second_reviewer_assignment.assigned_on + datetime.timedelta(days=1)
completed_assignment.save()
# The completed_on timestamp should not have changed the order - second_reviewer still at the end
self.assertEqual(self.policy.default_reviewer_rotation_list(),
available_reviewers[2:] + [first_reviewer, second_reviewer])
def test_return_reviewer_to_rotation_top(self):
# Should do nothing, this is implicit in this policy, no state change is needed.
team = ReviewTeamFactory(acronym="rotationteam", name="Review Team",
list_email="rotationteam@ietf.org",
parent=Group.objects.get(acronym="farfut"))
reviewer = create_person(team, "reviewer", name="reviewer", username="reviewer")
policy = LeastRecentlyUsedReviewerQueuePolicy(team)
policy.return_reviewer_to_rotation_top(reviewer)
self.policy.return_reviewer_to_rotation_top(self.append_reviewer())
def test_assign_reviewer_updates_skip_next_without_add_skip(self):
"""Skipping reviewers with add_skip=False should update skip_counts properly"""
review_req = ReviewRequestFactory(team=self.team)
reviewer_to_skip = self.append_reviewer(skip_count=1)
reviewer_to_assign = self.append_reviewer(skip_count=0)
reviewer_to_skip_later = self.append_reviewer(skip_count=1)
# Check test assumptions
self.assertEqual(
self.policy.default_reviewer_rotation_list(),
[reviewer_to_skip, reviewer_to_assign, reviewer_to_skip_later],
)
self.policy.assign_reviewer(review_req, reviewer_to_assign.email(), add_skip=False)
# Check results
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip).skip_next, 0,
'skip_next not updated for skipped reviewer')
self.assertEqual(self.reviewer_settings_for(reviewer_to_assign).skip_next, 0,
'skip_next changed unexpectedly for assigned reviewer')
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip_later).skip_next, 1,
'skip_next changed unexpectedly for reviewer to skip later')
def test_assign_reviewer_updates_skip_next_with_add_skip(self):
"""Skipping reviewers with add_skip=True should update skip_counts properly"""
review_req = ReviewRequestFactory(team=self.team)
reviewer_to_skip = self.append_reviewer(skip_count=1)
reviewer_to_assign = self.append_reviewer(skip_count=0)
reviewer_to_skip_later = self.append_reviewer(skip_count=1)
# Check test assumptions
self.assertEqual(
self.policy.default_reviewer_rotation_list(),
[reviewer_to_skip, reviewer_to_assign, reviewer_to_skip_later],
)
self.policy.assign_reviewer(review_req, reviewer_to_assign.email(), add_skip=True)
# Check results
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip).skip_next, 0,
'skip_next not updated for skipped reviewer')
self.assertEqual(self.reviewer_settings_for(reviewer_to_assign).skip_next, 1,
'skip_next not updated for assigned reviewer')
self.assertEqual(self.reviewer_settings_for(reviewer_to_skip_later).skip_next, 1,
'skip_next changed unexpectedly for reviewer to skip later')
class AssignmentOrderResolverTests(TestCase):
@ -315,7 +780,6 @@ class AssignmentOrderResolverTests(TestCase):
team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
reviewer_high = create_person(team, "reviewer", name="Test Reviewer-high", username="testreviewerhigh")
reviewer_low = create_person(team, "reviewer", name="Test Reviewer-low", username="testreviewerlow")
reviewer_unavailable = create_person(team, "reviewer", name="Test Reviewer-unavailable", username="testreviewerunavailable")
# This reviewer should be entirely ignored because it is not in the rotation list.
create_person(team, "reviewer", name="Test Reviewer-out-of-rotation", username="testreviewer-out-of-rotation")
@ -342,14 +806,6 @@ class AssignmentOrderResolverTests(TestCase):
start_date='2000-01-01',
availability='canfinish',
)
# This period should exclude this reviewer entirely, as it is 'canfinish',
# but this reviewer has not reviewed previously.
UnavailablePeriod.objects.create(
team=team,
person=reviewer_unavailable,
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')
@ -371,7 +827,7 @@ class AssignmentOrderResolverTests(TestCase):
)
review_req = ReviewRequestFactory(doc=doc, team=team, type_id='early')
rotation_list = [reviewer_low, reviewer_high, reviewer_unavailable]
rotation_list = [reviewer_low, reviewer_high]
order = AssignmentOrderResolver(Email.objects.all(), review_req, rotation_list)
ranking = order.determine_ranking()

5
ietf/review/utils.py
View file

@ -381,11 +381,8 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa
review_req.state_id = 'assigned'
review_req.save()
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, add_skip)
assignment = get_reviewer_queue_policy(review_req.team).assign_reviewer(review_req, reviewer, add_skip)
descr = "Request for {} review by {} is assigned to {}".format(
review_req.type.name,
review_req.team.acronym.upper(),