diff --git a/ietf/review/migrations/0027_unique_constraint_for_reviewersettings.py b/ietf/review/migrations/0027_unique_constraint_for_reviewersettings.py new file mode 100644 index 000000000..ecd192e1b --- /dev/null +++ b/ietf/review/migrations/0027_unique_constraint_for_reviewersettings.py @@ -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'), + ), + ] diff --git a/ietf/review/models.py b/ietf/review/models.py index a33bda065..96355417d 100644 --- a/ietf/review/models.py +++ b/ietf/review/models.py @@ -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.""" diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 4f0530793..30ded02a7 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -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): diff --git a/ietf/review/tests_policies.py b/ietf/review/tests_policies.py index 55c46ffdb..e1826e9ea 100644 --- a/ietf/review/tests_policies.py +++ b/ietf/review/tests_policies.py @@ -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() diff --git a/ietf/review/utils.py b/ietf/review/utils.py index 932396676..a4c905f2a 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -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(),