From 2caf18dcd3517d60609c40af3b05a09d2cfacbe0 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Thu, 31 Oct 2019 14:55:00 +0000 Subject: [PATCH 01/24] Quick fix to stop mypy from failing - Legacy-Id: 16949 --- ietf/dbtemplate/template.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ietf/dbtemplate/template.py b/ietf/dbtemplate/template.py index ac0de5b88..cdcd4b28f 100644 --- a/ietf/dbtemplate/template.py +++ b/ietf/dbtemplate/template.py @@ -11,7 +11,7 @@ from docutils.utils import SystemMessage import debug # pyflakes:ignore from django.template.loaders.base import Loader as BaseLoader -from django.template.base import Template as DjangoTemplate, TemplateEncodingError # type: ignore (FIXME: remove when Django 2) +from django.template.base import Template as DjangoTemplate, TemplateEncodingError # type: ignore from django.template.exceptions import TemplateDoesNotExist from django.utils.encoding import smart_text From eab14ea1c533ade90475194076a7cec57c8cba8a Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Thu, 31 Oct 2019 15:01:14 +0000 Subject: [PATCH 02/24] Early work to extract reviewer policy from review/utils.py. - Legacy-Id: 16950 --- ietf/doc/tests_review.py | 94 +---------- ietf/doc/views_review.py | 5 +- ietf/group/forms.py | 5 +- ietf/group/tests_review.py | 7 +- ietf/group/views.py | 11 +- ietf/review/policies.py | 291 +++++++++++++++++++++++++++++++++++ ietf/review/test_policies.py | 104 +++++++++++++ ietf/review/utils.py | 256 +----------------------------- 8 files changed, 419 insertions(+), 354 deletions(-) create mode 100644 ietf/review/policies.py create mode 100644 ietf/review/test_policies.py diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index 90974c5d9..37af398ae 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -33,10 +33,9 @@ from ietf.person.models import Email, Person from ietf.review.factories import ReviewRequestFactory, ReviewAssignmentFactory from ietf.review.models import (ReviewRequest, ReviewerSettings, ReviewWish, UnavailablePeriod, NextReviewerInTeam) -from ietf.review.utils import reviewer_rotation_list, possibly_advance_next_reviewer_for_team +from ietf.review.policies import policy_for_team from ietf.utils.test_utils import TestCase -from ietf.utils.test_data import create_person from ietf.utils.test_utils import login_testing_unauthorized, reload_db_objects from ietf.utils.mail import outbox, empty_outbox, parseaddr, on_behalf_of from ietf.person.factories import PersonFactory @@ -232,95 +231,8 @@ class ReviewTests(TestCase): self.assertIn("closed", mail_content) self.assertIn("review_request_close_comment", mail_content) - def test_possibly_advance_next_reviewer_for_team(self): - - team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut")) - doc = WgDraftFactory() - - # make a bunch of reviewers - reviewers = [ - create_person(team, "reviewer", name="Test Reviewer{}".format(i), username="testreviewer{}".format(i)) - for i in range(5) - ] - - self.assertEqual(reviewers, reviewer_rotation_list(team)) - - def get_skip_next(person): - settings = (ReviewerSettings.objects.filter(team=team, person=person).first() - or ReviewerSettings(team=team)) - return settings.skip_next - - possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[0].pk, add_skip=False) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[1]) - self.assertEqual(get_skip_next(reviewers[0]), 0) - self.assertEqual(get_skip_next(reviewers[1]), 0) - - possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[1].pk, add_skip=True) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) - self.assertEqual(get_skip_next(reviewers[1]), 1) - self.assertEqual(get_skip_next(reviewers[2]), 0) - - # skip reviewer 2 - possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[3].pk, add_skip=True) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) - self.assertEqual(get_skip_next(reviewers[0]), 0) - self.assertEqual(get_skip_next(reviewers[1]), 1) - self.assertEqual(get_skip_next(reviewers[2]), 0) - self.assertEqual(get_skip_next(reviewers[3]), 1) - - # pick reviewer 2, use up reviewer 3's skip_next - possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[2].pk, add_skip=False) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4]) - self.assertEqual(get_skip_next(reviewers[0]), 0) - self.assertEqual(get_skip_next(reviewers[1]), 1) - self.assertEqual(get_skip_next(reviewers[2]), 0) - self.assertEqual(get_skip_next(reviewers[3]), 0) - self.assertEqual(get_skip_next(reviewers[4]), 0) - - # check wrap-around - possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[4].pk) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[0]) - self.assertEqual(get_skip_next(reviewers[0]), 0) - self.assertEqual(get_skip_next(reviewers[1]), 1) - self.assertEqual(get_skip_next(reviewers[2]), 0) - self.assertEqual(get_skip_next(reviewers[3]), 0) - self.assertEqual(get_skip_next(reviewers[4]), 0) - - # unavailable - today = datetime.date.today() - UnavailablePeriod.objects.create(team=team, person=reviewers[1], start_date=today, end_date=today, availability="unavailable") - possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[0].pk) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) - self.assertEqual(get_skip_next(reviewers[0]), 0) - self.assertEqual(get_skip_next(reviewers[1]), 1) # don't consume that skip while the reviewer is unavailable - self.assertEqual(get_skip_next(reviewers[2]), 0) - self.assertEqual(get_skip_next(reviewers[3]), 0) - self.assertEqual(get_skip_next(reviewers[4]), 0) - - # pick unavailable anyway - possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[1].pk, add_skip=False) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) - self.assertEqual(get_skip_next(reviewers[0]), 0) - self.assertEqual(get_skip_next(reviewers[1]), 1) - self.assertEqual(get_skip_next(reviewers[2]), 0) - self.assertEqual(get_skip_next(reviewers[3]), 0) - self.assertEqual(get_skip_next(reviewers[4]), 0) - - # not through min_interval so advance past reviewer[2] - settings, _ = ReviewerSettings.objects.get_or_create(team=team, person=reviewers[2]) - settings.min_interval = 30 - settings.save() - req = ReviewRequest.objects.create(team=team, doc=doc, type_id="early", state_id="assigned", deadline=today, requested_by=reviewers[0]) - ReviewAssignmentFactory(review_request=req, state_id="accepted", reviewer = reviewers[2].email_set.first(),assigned_on = req.time) - possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[3].pk) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4]) - self.assertEqual(get_skip_next(reviewers[0]), 0) - self.assertEqual(get_skip_next(reviewers[1]), 1) - self.assertEqual(get_skip_next(reviewers[2]), 0) - self.assertEqual(get_skip_next(reviewers[3]), 0) - self.assertEqual(get_skip_next(reviewers[4]), 0) - def test_assign_reviewer(self): + # TODO: this test overlaps way too much with the reviewer policy doc = WgDraftFactory(pages=2) review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut")) rev_role = RoleFactory(group=review_team,person__user__username='reviewer',person__user__email='reviewer@example.com',person__name='Some Reviewer',name_id='reviewer') @@ -413,7 +325,7 @@ class ReviewTests(TestCase): # assign empty_outbox() - rotation_list = reviewer_rotation_list(review_req.team) + rotation_list = policy_for_team(review_req.team).reviewer_rotation_list(review_req.team) reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[0]).first() r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk }) self.assertEqual(r.status_code, 302) diff --git a/ietf/doc/views_review.py b/ietf/doc/views_review.py index eaffd71fb..abe15dd44 100644 --- a/ietf/doc/views_review.py +++ b/ietf/doc/views_review.py @@ -33,11 +33,12 @@ from ietf.ietfauth.utils import is_authorized_in_doc_stream, user_is_person, has from ietf.message.models import Message from ietf.message.utils import infer_message from ietf.person.fields import PersonEmailChoiceField, SearchablePersonField +from ietf.review.policies import policy_for_team from ietf.review.utils import (active_review_teams, assign_review_request_to_reviewer, can_request_review_of_doc, can_manage_review_requests_for_team, email_review_assignment_change, email_review_request_change, close_review_request_states, - close_review_request, setup_reviewer_field) + close_review_request) from ietf.review import mailarch from ietf.utils.fields import DatepickerDateField from ietf.utils.text import strip_prefix, xslugify @@ -293,7 +294,7 @@ class AssignReviewerForm(forms.Form): def __init__(self, review_req, *args, **kwargs): super(AssignReviewerForm, self).__init__(*args, **kwargs) - setup_reviewer_field(self.fields["reviewer"], review_req) + policy_for_team(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req) @login_required diff --git a/ietf/group/forms.py b/ietf/group/forms.py index 99650eb51..564ddf716 100644 --- a/ietf/group/forms.py +++ b/ietf/group/forms.py @@ -19,7 +19,8 @@ from ietf.group.models import Group, GroupHistory, GroupStateName from ietf.person.fields import SearchableEmailsField, PersonEmailChoiceField from ietf.person.models import Person from ietf.review.models import ReviewerSettings, UnavailablePeriod, ReviewSecretarySettings -from ietf.review.utils import close_review_request_states, setup_reviewer_field +from ietf.review.policies import policy_for_team +from ietf.review.utils import close_review_request_states from ietf.utils.textupload import get_cleaned_text_file_content from ietf.utils.text import strip_suffix #from ietf.utils.ordereddict import insert_after_in_ordered_dict @@ -254,7 +255,7 @@ class ManageReviewRequestForm(forms.Form): self.fields["close"].widget.attrs["class"] = "form-control input-sm" - setup_reviewer_field(self.fields["reviewer"], review_req) + policy_for_team(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req) self.fields["reviewer"].widget.attrs["class"] = "form-control input-sm" if self.is_bound: diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py index 5e92fd015..443f0de19 100644 --- a/ietf/group/tests_review.py +++ b/ietf/group/tests_review.py @@ -12,6 +12,7 @@ from pyquery import PyQuery from django.urls import reverse as urlreverse +from ietf.review.policies import policy_for_team from ietf.utils.test_utils import login_testing_unauthorized, TestCase, reload_db_objects from ietf.doc.models import TelechatDocEvent from ietf.group.models import Role @@ -23,7 +24,6 @@ from ietf.review.utils import ( suggested_review_requests_for_team, review_assignments_needing_reviewer_reminder, email_reviewer_reminder, review_assignments_needing_secretary_reminder, email_secretary_reminder, - reviewer_rotation_list, send_unavaibility_period_ending_reminder, send_reminder_all_open_reviews, send_review_reminder_overdue_assignment, send_reminder_unconfirmed_assignments) from ietf.name.models import ReviewResultName, ReviewRequestStateName, ReviewAssignmentStateName @@ -655,7 +655,8 @@ class BulkAssignmentTests(TestCase): secretary = RoleFactory.create(group=group,name_id='secr') docs = [DocumentFactory.create(type_id='draft',group=None) for i in range(4)] requests = [ReviewRequestFactory(team=group,doc=docs[i]) for i in range(4)] - rot_list = reviewer_rotation_list(group) + policy = policy_for_team(group) + rot_list = policy.reviewer_rotation_list(group) expected_ending_head_of_rotation = rot_list[3] @@ -676,6 +677,6 @@ class BulkAssignmentTests(TestCase): self.client.login(username=secretary.person.user.username,password=secretary.person.user.username+'+password') r = self.client.post(unassigned_url, postdict) self.assertEqual(r.status_code,302) - self.assertEqual(expected_ending_head_of_rotation,reviewer_rotation_list(group)[0]) + self.assertEqual(expected_ending_head_of_rotation, policy.reviewer_rotation_list(group)[0]) self.assertMailboxContains(outbox, subject='Last Call assignment', text='Requested by', count=4) diff --git a/ietf/group/views.py b/ietf/group/views.py index 93df986cd..03e5b1cab 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -92,6 +92,7 @@ from ietf.meeting.utils import group_sessions from ietf.name.models import GroupTypeName, StreamName from ietf.person.models import Email from ietf.review.models import ReviewRequest, ReviewAssignment, ReviewerSettings, ReviewSecretarySettings +from ietf.review.policies import policy_for_team from ietf.review.utils import (can_manage_review_requests_for_team, can_access_review_stats_for_team, @@ -103,7 +104,6 @@ from ietf.review.utils import (can_manage_review_requests_for_team, unavailable_periods_to_list, current_unavailable_periods_for_reviewers, email_reviewer_availability_change, - reviewer_rotation_list, latest_review_assignments_for_reviewers, augment_review_requests_with_events, get_default_filter_re, @@ -1391,7 +1391,7 @@ def reviewer_overview(request, acronym, group_type=None): can_manage = can_manage_review_requests_for_team(request.user, group) - reviewers = reviewer_rotation_list(group) + reviewers = policy_for_team(group).reviewer_rotation_list(group) reviewer_settings = { s.person_id: s for s in ReviewerSettings.objects.filter(team=group) } unavailable_periods = defaultdict(list) @@ -1541,13 +1541,14 @@ def manage_review_requests(request, acronym, group_type=None, assignment_status= # Make sure the any assignments to the person at the head # of the rotation queue are processed first so that the queue # rotates before any more assignments are processed - head_of_rotation = reviewer_rotation_list(group)[0] + reviewer_policy = policy_for_team(group) + head_of_rotation = reviewer_policy.reviewer_rotation_list(group)[0] while head_of_rotation in assignments_by_person: for review_req in assignments_by_person[head_of_rotation]: assign_review_request_to_reviewer(request, review_req, review_req.form.cleaned_data["reviewer"],review_req.form.cleaned_data["add_skip"]) reqs_to_assign.remove(review_req) del assignments_by_person[head_of_rotation] - head_of_rotation = reviewer_rotation_list(group)[0] + head_of_rotation = reviewer_policy.reviewer_rotation_list(group)[0] for review_req in reqs_to_assign: assign_review_request_to_reviewer(request, review_req, review_req.form.cleaned_data["reviewer"],review_req.form.cleaned_data["add_skip"]) @@ -1660,7 +1661,7 @@ def email_open_review_assignments(request, acronym, group_type=None): partial_msg = render_to_string(template.path, { "review_assignments": review_assignments, - "rotation_list": reviewer_rotation_list(group)[:10], + "rotation_list": policy_for_team(group).reviewer_rotation_list(group)[:10], "group" : group, }) diff --git a/ietf/review/policies.py b/ietf/review/policies.py new file mode 100644 index 000000000..86168e688 --- /dev/null +++ b/ietf/review/policies.py @@ -0,0 +1,291 @@ +# Copyright The IETF Trust 2016-2019, All Rights Reserved + +from __future__ import absolute_import, print_function, unicode_literals + +import re + +import six + +from ietf.doc.models import DocumentAuthor, DocAlias +from ietf.group.models import Role +from ietf.person.models import Person +import debug # pyflakes:ignore +from ietf.review.models import NextReviewerInTeam, ReviewerSettings, ReviewWish, ReviewRequest +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) + + +def policy_for_team(team): + return RotateWithSkipReviewerPolicy() + + +class AbstractReviewerPolicy: + + def _unavailable_reviewers(self, team, dont_skip): + # prune reviewers not in the rotation (but not the assigned + # reviewer who must have been available for assignment anyway) + reviewers_to_skip = set() + + unavailable_periods = current_unavailable_periods_for_reviewers(team) + for person_id, periods in unavailable_periods.items(): + if periods and person_id not in dont_skip: + reviewers_to_skip.add(person_id) + + days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(team) + for person_id, days_needed in days_needed_for_reviewers.items(): + if person_id not in dont_skip: + reviewers_to_skip.add(person_id) + return reviewers_to_skip + + +class RotateWithSkipReviewerPolicy(AbstractReviewerPolicy): + # TODO : Change this field to deal with multiple already assigned reviewers??? + def setup_reviewer_field(self, field, review_req): + field.queryset = field.queryset.filter(role__name="reviewer", role__group=review_req.team) + one_assignment = review_req.reviewassignment_set.first() + if one_assignment: + field.initial = one_assignment.reviewer_id + + choices = self._make_assignment_choices(field.queryset, review_req) + if not field.required: + choices = [("", field.empty_label)] + choices + + field.choices = choices + + def _make_assignment_choices(self, email_queryset, review_req): + doc = review_req.doc + team = review_req.team + + possible_emails = list(email_queryset) + possible_person_ids = [e.person_id for e in possible_emails] + + aliases = DocAlias.objects.filter(docs=doc).values_list("name", flat=True) + + # settings + reviewer_settings = { + r.person_id: r + for r in ReviewerSettings.objects.filter(team=team, person__in=possible_person_ids) + } + + for p in possible_person_ids: + if p not in reviewer_settings: + reviewer_settings[p] = ReviewerSettings(team=team, + filter_re=get_default_filter_re(p)) + + # frequency + days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(team) + + # rotation + rotation_index = {p.pk: i for i, p in enumerate(self.reviewer_rotation_list(team))} + + # previous review of document + has_reviewed_previous = ReviewRequest.objects.filter( + doc=doc, + reviewassignment__reviewer__person__in=possible_person_ids, + reviewassignment__state="completed", + 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)) + + # review wishes + wish_to_review = set(ReviewWish.objects.filter(team=team, person__in=possible_person_ids, + doc=doc).values_list("person", flat=True)) + + # connections + connections = {} + # examine the closest connections last to let them override + connections[doc.ad_id] = "is associated Area Director" + for r in Role.objects.filter(group=doc.group_id, + person__in=possible_person_ids).select_related("name"): + connections[r.person_id] = "is group {}".format(r.name) + if doc.shepherd: + connections[doc.shepherd.person_id] = "is shepherd of document" + for author in DocumentAuthor.objects.filter(document=doc, + person__in=possible_person_ids).values_list( + "person", flat=True): + connections[author] = "is author of document" + + # unavailable periods + unavailable_periods = current_unavailable_periods_for_reviewers(team) + + # reviewers statistics + assignment_data_for_reviewers = latest_review_assignments_for_reviewers(team) + + ranking = [] + for e in possible_emails: + settings = reviewer_settings.get(e.person_id) + + # we sort the reviewers by separate axes, listing the most + # important things first + scores = [] + explanations = [] + + def add_boolean_score(direction, expr, explanation=None): + scores.append(direction if expr else -direction) + if expr and explanation: + explanations.append(explanation) + + # unavailable for review periods + periods = unavailable_periods.get(e.person_id, []) + unavailable_at_the_moment = periods and not ( + e.person_id in has_reviewed_previous and all( + p.availability == "canfinish" for p in periods)) + add_boolean_score(-1, unavailable_at_the_moment) + + def format_period(p): + if p.end_date: + res = "unavailable until {}".format(p.end_date.isoformat()) + else: + res = "unavailable indefinitely" + return "{} ({})".format(res, p.get_availability_display()) + + if periods: + explanations.append(", ".join(format_period(p) for p in periods)) + + # misc + add_boolean_score(+1, e.person_id in has_reviewed_previous, "reviewed document before") + add_boolean_score(+1, e.person_id in wish_to_review, "wishes to review document") + add_boolean_score(-1, e.person_id in connections, + connections.get(e.person_id)) # reviewer is somehow connected: bad + add_boolean_score(-1, settings.filter_re and any( + re.search(settings.filter_re, n) for n in aliases), "filter regexp matches") + + # minimum interval between reviews + days_needed = days_needed_for_reviewers.get(e.person_id, 0) + scores.append(-days_needed) + if days_needed > 0: + explanations.append("max frequency exceeded, ready in {} {}".format(days_needed, + "day" if days_needed == 1 else "days")) + + # skip next + scores.append(-settings.skip_next) + if settings.skip_next > 0: + explanations.append("skip next {}".format(settings.skip_next)) + + # index + index = rotation_index.get(e.person_id, 0) + scores.append(-index) + explanations.append("#{}".format(index + 1)) + + # stats + stats = [] + assignment_data = assignment_data_for_reviewers.get(e.person_id, []) + + currently_open = sum(1 for d in assignment_data if d.state in ["assigned", "accepted"]) + pages = sum( + rd.doc_pages for rd in assignment_data if rd.state in ["assigned", "accepted"]) + if currently_open > 0: + stats.append("currently {count} open, {pages} pages".format(count=currently_open, + pages=pages)) + could_have_completed = [d for d in assignment_data if + d.state in ["part-completed", "completed", "no-response"]] + if could_have_completed: + no_response = len([d for d in assignment_data if d.state == 'no-response']) + if no_response: + stats.append("%s no response" % no_response) + part_completed = len([d for d in assignment_data if d.state == 'part-completed']) + if part_completed: + stats.append("%s partially complete" % part_completed) + completed = len([d for d in assignment_data if d.state == 'completed']) + if completed: + stats.append("%s fully completed" % completed) + + if stats: + explanations.append(", ".join(stats)) + + label = six.text_type(e.person) + if explanations: + label = "{}: {}".format(label, "; ".join(explanations)) + + ranking.append({ + "email": e, + "scores": scores, + "label": label, + }) + + ranking.sort(key=lambda r: r["scores"], reverse=True) + + return [(r["email"].pk, r["label"]) for r in ranking] + + def possibly_advance_next_reviewer_for_team(self, team, assigned_review_to_person_id, add_skip=False): + assert assigned_review_to_person_id is not None + + rotation_list = self.reviewer_rotation_list(team, skip_unavailable=True, + dont_skip=[assigned_review_to_person_id]) + + def reviewer_at_index(i): + if not rotation_list: + return None + return rotation_list[i % len(rotation_list)] + + def reviewer_settings_for(person_id): + return (ReviewerSettings.objects.filter(team=team, person=person_id).first() + or ReviewerSettings(team=team, person_id=person_id)) + + current_i = 0 + + if assigned_review_to_person_id == reviewer_at_index(current_i): + # move 1 ahead + current_i += 1 + + if add_skip: + settings = reviewer_settings_for(assigned_review_to_person_id) + settings.skip_next += 1 + settings.save() + + if not rotation_list: + return + + while True: + # as a clean-up step go through any with a skip next > 0 + current_reviewer_person_id = reviewer_at_index(current_i) + settings = reviewer_settings_for(current_reviewer_person_id) + if settings.skip_next > 0: + settings.skip_next -= 1 + settings.save() + current_i += 1 + else: + nr = NextReviewerInTeam.objects.filter(team=team).first() or NextReviewerInTeam( + team=team) + nr.next_reviewer_id = current_reviewer_person_id + nr.save() + + break + + def reviewer_rotation_list(self, team, skip_unavailable=False, dont_skip=[]): + reviewers = list(Person.objects.filter(role__name="reviewer", role__group=team)) + reviewers.sort(key=lambda p: p.last_name()) + next_reviewer_index = 0 + + # now to figure out where the rotation is currently at + saved_reviewer = NextReviewerInTeam.objects.filter(team=team).select_related("next_reviewer").first() + if saved_reviewer: + n = saved_reviewer.next_reviewer + + if n not in reviewers: + # saved reviewer might not still be here, if not just + # insert and use that position (Python will wrap around, + # so no harm done by using the index on the original list + # afterwards) + reviewers_with_next = reviewers[:] + [n] + reviewers_with_next.sort(key=lambda p: p.last_name()) + next_reviewer_index = reviewers_with_next.index(n) + else: + next_reviewer_index = reviewers.index(n) + + rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index] + + reviewers_to_skip = [] + if skip_unavailable: + reviewers_to_skip = self._unavailable_reviewers(team, dont_skip) + rotation_list = [p.pk for p in rotation_list if p.pk not in reviewers_to_skip] + + return rotation_list + diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py new file mode 100644 index 000000000..8f6450957 --- /dev/null +++ b/ietf/review/test_policies.py @@ -0,0 +1,104 @@ +# Copyright The IETF Trust 2016-2019, All Rights Reserved + +import datetime + +from ietf.doc.factories import WgDraftFactory +from ietf.group.factories import ReviewTeamFactory +from ietf.group.models import Group +from ietf.review.factories import ReviewAssignmentFactory +from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, \ + ReviewRequest +from ietf.review.policies import policy_for_team +from ietf.utils.test_data import create_person +from ietf.utils.test_utils import TestCase + + +class RotateWithSkipReviewerPolicyTests(TestCase): + def test_possibly_advance_next_reviewer_for_team(self): + + team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut")) + policy = policy_for_team(team) + doc = WgDraftFactory() + + # make a bunch of reviewers + reviewers = [ + create_person(team, "reviewer", name="Test Reviewer{}".format(i), username="testreviewer{}".format(i)) + for i in range(5) + ] + + self.assertEqual(reviewers, policy.reviewer_rotation_list(team)) + + def get_skip_next(person): + settings = (ReviewerSettings.objects.filter(team=team, person=person).first() + or ReviewerSettings(team=team)) + return settings.skip_next + + policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[0].pk, add_skip=False) + self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[1]) + self.assertEqual(get_skip_next(reviewers[0]), 0) + self.assertEqual(get_skip_next(reviewers[1]), 0) + + policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[1].pk, add_skip=True) + self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) + self.assertEqual(get_skip_next(reviewers[1]), 1) + self.assertEqual(get_skip_next(reviewers[2]), 0) + + # skip reviewer 2 + policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[3].pk, add_skip=True) + self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) + self.assertEqual(get_skip_next(reviewers[0]), 0) + self.assertEqual(get_skip_next(reviewers[1]), 1) + self.assertEqual(get_skip_next(reviewers[2]), 0) + self.assertEqual(get_skip_next(reviewers[3]), 1) + + # pick reviewer 2, use up reviewer 3's skip_next + policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[2].pk, add_skip=False) + self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4]) + self.assertEqual(get_skip_next(reviewers[0]), 0) + self.assertEqual(get_skip_next(reviewers[1]), 1) + self.assertEqual(get_skip_next(reviewers[2]), 0) + self.assertEqual(get_skip_next(reviewers[3]), 0) + self.assertEqual(get_skip_next(reviewers[4]), 0) + + # check wrap-around + policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[4].pk) + self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[0]) + self.assertEqual(get_skip_next(reviewers[0]), 0) + self.assertEqual(get_skip_next(reviewers[1]), 1) + self.assertEqual(get_skip_next(reviewers[2]), 0) + self.assertEqual(get_skip_next(reviewers[3]), 0) + self.assertEqual(get_skip_next(reviewers[4]), 0) + + # unavailable + today = datetime.date.today() + UnavailablePeriod.objects.create(team=team, person=reviewers[1], start_date=today, end_date=today, availability="unavailable") + policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[0].pk) + self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) + self.assertEqual(get_skip_next(reviewers[0]), 0) + self.assertEqual(get_skip_next(reviewers[1]), 1) # don't consume that skip while the reviewer is unavailable + self.assertEqual(get_skip_next(reviewers[2]), 0) + self.assertEqual(get_skip_next(reviewers[3]), 0) + self.assertEqual(get_skip_next(reviewers[4]), 0) + + # pick unavailable anyway + policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[1].pk, add_skip=False) + self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) + self.assertEqual(get_skip_next(reviewers[0]), 0) + self.assertEqual(get_skip_next(reviewers[1]), 1) + self.assertEqual(get_skip_next(reviewers[2]), 0) + self.assertEqual(get_skip_next(reviewers[3]), 0) + self.assertEqual(get_skip_next(reviewers[4]), 0) + + # not through min_interval so advance past reviewer[2] + settings, _ = ReviewerSettings.objects.get_or_create(team=team, person=reviewers[2]) + settings.min_interval = 30 + settings.save() + req = ReviewRequest.objects.create(team=team, doc=doc, type_id="early", state_id="assigned", deadline=today, requested_by=reviewers[0]) + ReviewAssignmentFactory(review_request=req, state_id="accepted", reviewer = reviewers[2].email_set.first(),assigned_on = req.time) + policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[3].pk) + self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4]) + self.assertEqual(get_skip_next(reviewers[0]), 0) + self.assertEqual(get_skip_next(reviewers[1]), 1) + self.assertEqual(get_skip_next(reviewers[2]), 0) + self.assertEqual(get_skip_next(reviewers[3]), 0) + self.assertEqual(get_skip_next(reviewers[4]), 0) \ No newline at end of file diff --git a/ietf/review/utils.py b/ietf/review/utils.py index 370489c91..6d7d919e0 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -6,8 +6,6 @@ from __future__ import absolute_import, print_function, unicode_literals import datetime import itertools -import re -import six from collections import defaultdict, namedtuple @@ -22,15 +20,14 @@ from ietf.dbtemplate.models import DBTemplate from ietf.group.models import Group, Role from ietf.doc.models import (Document, ReviewRequestDocEvent, ReviewAssignmentDocEvent, State, - LastCallDocEvent, TelechatDocEvent, - DocumentAuthor, DocAlias) + LastCallDocEvent, TelechatDocEvent) from ietf.iesg.models import TelechatDate from ietf.mailtrigger.utils import gather_address_lists from ietf.person.models import Person from ietf.ietfauth.utils import has_role, is_authorized_in_doc_stream from ietf.review.models import (ReviewRequest, ReviewAssignment, ReviewRequestStateName, ReviewTypeName, - ReviewerSettings, UnavailablePeriod, ReviewWish, NextReviewerInTeam, - ReviewSecretarySettings, ReviewTeamSettings) + ReviewerSettings, UnavailablePeriod, ReviewSecretarySettings, + ReviewTeamSettings) from ietf.utils.mail import send_mail from ietf.doc.utils import extract_complete_replaces_ancestor_mapping_for_docs from ietf.utils import log @@ -112,49 +109,6 @@ def current_unavailable_periods_for_reviewers(team): return res -def reviewer_rotation_list(team, skip_unavailable=False, dont_skip=[]): - """Returns person id -> index in rotation (next reviewer has index 0).""" - reviewers = list(Person.objects.filter(role__name="reviewer", role__group=team)) - reviewers.sort(key=lambda p: p.last_name()) - - next_reviewer_index = 0 - - # now to figure out where the rotation is currently at - saved_reviewer = NextReviewerInTeam.objects.filter(team=team).select_related("next_reviewer").first() - if saved_reviewer: - n = saved_reviewer.next_reviewer - - if n not in reviewers: - # saved reviewer might not still be here, if not just - # insert and use that position (Python will wrap around, - # so no harm done by using the index on the original list - # afterwards) - reviewers_with_next = reviewers[:] + [n] - reviewers_with_next.sort(key=lambda p: p.last_name()) - next_reviewer_index = reviewers_with_next.index(n) - else: - next_reviewer_index = reviewers.index(n) - - rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index] - - if skip_unavailable: - # prune reviewers not in the rotation (but not the assigned - # reviewer who must have been available for assignment anyway) - reviewers_to_skip = set() - - unavailable_periods = current_unavailable_periods_for_reviewers(team) - for person_id, periods in unavailable_periods.items(): - if periods and person_id not in dont_skip: - reviewers_to_skip.add(person_id) - - days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(team) - for person_id, days_needed in days_needed_for_reviewers.items(): - if person_id not in dont_skip: - reviewers_to_skip.add(person_id) - - rotation_list = [p.pk for p in rotation_list if p.pk not in reviewers_to_skip] - - return rotation_list def days_needed_to_fulfill_min_interval_for_reviewers(team): """Returns person_id -> days needed until min_interval is fulfilled @@ -426,7 +380,8 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa assignment = review_req.reviewassignment_set.create(state_id='assigned', reviewer = reviewer, assigned_on = datetime.datetime.now()) - possibly_advance_next_reviewer_for_team(review_req.team, reviewer.person_id, add_skip) + from ietf.review.policies import policy_for_team + policy_for_team(review_req.team).possibly_advance_next_reviewer_for_team(review_req.team, reviewer.person_id, add_skip) ReviewRequestDocEvent.objects.create( type="assigned_review_request", @@ -477,49 +432,6 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa msg , by=request.user.person, notify_secretary=False, notify_reviewer=True, notify_requested_by=False) -def possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id, add_skip=False): - assert assigned_review_to_person_id is not None - - rotation_list = reviewer_rotation_list(team, skip_unavailable=True, dont_skip=[assigned_review_to_person_id]) - - def reviewer_at_index(i): - if not rotation_list: - return None - - return rotation_list[i % len(rotation_list)] - - def reviewer_settings_for(person_id): - return (ReviewerSettings.objects.filter(team=team, person=person_id).first() - or ReviewerSettings(team=team, person_id=person_id)) - - current_i = 0 - - if assigned_review_to_person_id == reviewer_at_index(current_i): - # move 1 ahead - current_i += 1 - - if add_skip: - settings = reviewer_settings_for(assigned_review_to_person_id) - settings.skip_next += 1 - settings.save() - - if not rotation_list: - return - - while True: - # as a clean-up step go through any with a skip next > 0 - current_reviewer_person_id = reviewer_at_index(current_i) - settings = reviewer_settings_for(current_reviewer_person_id) - if settings.skip_next > 0: - settings.skip_next -= 1 - settings.save() - current_i += 1 - else: - nr = NextReviewerInTeam.objects.filter(team=team).first() or NextReviewerInTeam(team=team) - nr.next_reviewer_id = current_reviewer_person_id - nr.save() - - break def close_review_request(request, review_req, close_state, close_comment=''): suggested_req = review_req.pk is None @@ -774,18 +686,6 @@ def extract_revision_ordered_review_requests_for_documents_and_replaced(review_r return res -# TODO : Change this field to deal with multiple already assigned reviewers -def setup_reviewer_field(field, review_req): - field.queryset = field.queryset.filter(role__name="reviewer", role__group=review_req.team) - one_assignment = review_req.reviewassignment_set.first() - if one_assignment: - field.initial = one_assignment.reviewer_id - - choices = make_assignment_choices(field.queryset, review_req) - if not field.required: - choices = [("", field.empty_label)] + choices - - field.choices = choices def get_default_filter_re(person): if type(person) != Person: @@ -796,152 +696,6 @@ def get_default_filter_re(person): else: return '^draft-(%s|%s)-.*$' % ( person.last_name().lower(), '|'.join(['ietf-%s' % g.acronym for g in groups_to_avoid])) -def make_assignment_choices(email_queryset, review_req): - doc = review_req.doc - team = review_req.team - - possible_emails = list(email_queryset) - possible_person_ids = [e.person_id for e in possible_emails] - - aliases = DocAlias.objects.filter(docs=doc).values_list("name", flat=True) - - # settings - reviewer_settings = { - r.person_id: r - for r in ReviewerSettings.objects.filter(team=team, person__in=possible_person_ids) - } - - for p in possible_person_ids: - if p not in reviewer_settings: - reviewer_settings[p] = ReviewerSettings(team=team, filter_re = get_default_filter_re(p)) - - # frequency - days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(team) - - # rotation - rotation_index = { p.pk: i for i, p in enumerate(reviewer_rotation_list(team)) } - - # previous review of document - has_reviewed_previous = ReviewRequest.objects.filter( - doc=doc, - reviewassignment__reviewer__person__in=possible_person_ids, - reviewassignment__state="completed", - 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)) - - # review wishes - wish_to_review = set(ReviewWish.objects.filter(team=team, person__in=possible_person_ids, doc=doc).values_list("person", flat=True)) - - # connections - connections = {} - # examine the closest connections last to let them override - connections[doc.ad_id] = "is associated Area Director" - for r in Role.objects.filter(group=doc.group_id, person__in=possible_person_ids).select_related("name"): - connections[r.person_id] = "is group {}".format(r.name) - if doc.shepherd: - connections[doc.shepherd.person_id] = "is shepherd of document" - for author in DocumentAuthor.objects.filter(document=doc, person__in=possible_person_ids).values_list("person", flat=True): - connections[author] = "is author of document" - - # unavailable periods - unavailable_periods = current_unavailable_periods_for_reviewers(team) - - # reviewers statistics - assignment_data_for_reviewers = latest_review_assignments_for_reviewers(team) - - ranking = [] - for e in possible_emails: - settings = reviewer_settings.get(e.person_id) - - # we sort the reviewers by separate axes, listing the most - # important things first - scores = [] - explanations = [] - - def add_boolean_score(direction, expr, explanation=None): - scores.append(direction if expr else -direction) - if expr and explanation: - explanations.append(explanation) - - # unavailable for review periods - periods = unavailable_periods.get(e.person_id, []) - unavailable_at_the_moment = periods and not (e.person_id in has_reviewed_previous and all(p.availability == "canfinish" for p in periods)) - add_boolean_score(-1, unavailable_at_the_moment) - - def format_period(p): - if p.end_date: - res = "unavailable until {}".format(p.end_date.isoformat()) - else: - res = "unavailable indefinitely" - return "{} ({})".format(res, p.get_availability_display()) - - if periods: - explanations.append(", ".join(format_period(p) for p in periods)) - - # misc - add_boolean_score(+1, e.person_id in has_reviewed_previous, "reviewed document before") - add_boolean_score(+1, e.person_id in wish_to_review, "wishes to review document") - add_boolean_score(-1, e.person_id in connections, connections.get(e.person_id)) # reviewer is somehow connected: bad - add_boolean_score(-1, settings.filter_re and any(re.search(settings.filter_re, n) for n in aliases), "filter regexp matches") - - # minimum interval between reviews - days_needed = days_needed_for_reviewers.get(e.person_id, 0) - scores.append(-days_needed) - if days_needed > 0: - explanations.append("max frequency exceeded, ready in {} {}".format(days_needed, "day" if days_needed == 1 else "days")) - - # skip next - scores.append(-settings.skip_next) - if settings.skip_next > 0: - explanations.append("skip next {}".format(settings.skip_next)) - - # index - index = rotation_index.get(e.person_id, 0) - scores.append(-index) - explanations.append("#{}".format(index + 1)) - - # stats - stats = [] - assignment_data = assignment_data_for_reviewers.get(e.person_id, []) - - currently_open = sum(1 for d in assignment_data if d.state in ["assigned", "accepted"]) - pages = sum(rd.doc_pages for rd in assignment_data if rd.state in ["assigned", "accepted"]) - if currently_open > 0: - stats.append("currently {count} open, {pages} pages".format(count=currently_open, pages=pages)) - could_have_completed = [d for d in assignment_data if d.state in ["part-completed", "completed", "no-response"]] - if could_have_completed: - no_response = len([d for d in assignment_data if d.state == 'no-response']) - if no_response: - stats.append("%s no response" % no_response) - part_completed = len([d for d in assignment_data if d.state == 'part-completed']) - if part_completed: - stats.append("%s partially complete" % part_completed) - completed = len([d for d in assignment_data if d.state == 'completed']) - if completed: - stats.append("%s fully completed" % completed) - - if stats: - explanations.append(", ".join(stats)) - - label = six.text_type(e.person) - if explanations: - label = "{}: {}".format(label, "; ".join(explanations)) - - ranking.append({ - "email": e, - "scores": scores, - "label": label, - }) - - ranking.sort(key=lambda r: r["scores"], reverse=True) - - return [(r["email"].pk, r["label"]) for r in ranking] - def send_unavaibility_period_ending_reminder(remind_date): reminder_days = 3 From e518824a69470a4cbb4e2c1b4865fdfe5b700d1a Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Tue, 5 Nov 2019 16:39:31 +0000 Subject: [PATCH 03/24] Initial refactoring of the current reviewer assignment code. - Legacy-Id: 16961 --- ietf/doc/tests_review.py | 2 +- ietf/group/tests_review.py | 4 +- ietf/group/views.py | 8 +- ietf/review/policies.py | 406 +++++++++++++++++++---------------- ietf/review/test_policies.py | 18 +- ietf/review/utils.py | 2 +- 6 files changed, 236 insertions(+), 204 deletions(-) diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index 37af398ae..e6d49f9e7 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -325,7 +325,7 @@ class ReviewTests(TestCase): # assign empty_outbox() - rotation_list = policy_for_team(review_req.team).reviewer_rotation_list(review_req.team) + rotation_list = policy_for_team(review_req.team).default_reviewer_rotation_list() reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[0]).first() r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk }) self.assertEqual(r.status_code, 302) diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py index 443f0de19..56a05fa5e 100644 --- a/ietf/group/tests_review.py +++ b/ietf/group/tests_review.py @@ -656,7 +656,7 @@ class BulkAssignmentTests(TestCase): docs = [DocumentFactory.create(type_id='draft',group=None) for i in range(4)] requests = [ReviewRequestFactory(team=group,doc=docs[i]) for i in range(4)] policy = policy_for_team(group) - rot_list = policy.reviewer_rotation_list(group) + rot_list = policy.default_reviewer_rotation_list() expected_ending_head_of_rotation = rot_list[3] @@ -677,6 +677,6 @@ class BulkAssignmentTests(TestCase): self.client.login(username=secretary.person.user.username,password=secretary.person.user.username+'+password') r = self.client.post(unassigned_url, postdict) self.assertEqual(r.status_code,302) - self.assertEqual(expected_ending_head_of_rotation, policy.reviewer_rotation_list(group)[0]) + self.assertEqual(expected_ending_head_of_rotation, policy.default_reviewer_rotation_list()[0]) self.assertMailboxContains(outbox, subject='Last Call assignment', text='Requested by', count=4) diff --git a/ietf/group/views.py b/ietf/group/views.py index 03e5b1cab..83d86a446 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -1391,7 +1391,7 @@ def reviewer_overview(request, acronym, group_type=None): can_manage = can_manage_review_requests_for_team(request.user, group) - reviewers = policy_for_team(group).reviewer_rotation_list(group) + reviewers = policy_for_team(group).default_reviewer_rotation_list() reviewer_settings = { s.person_id: s for s in ReviewerSettings.objects.filter(team=group) } unavailable_periods = defaultdict(list) @@ -1542,13 +1542,13 @@ def manage_review_requests(request, acronym, group_type=None, assignment_status= # of the rotation queue are processed first so that the queue # rotates before any more assignments are processed reviewer_policy = policy_for_team(group) - head_of_rotation = reviewer_policy.reviewer_rotation_list(group)[0] + head_of_rotation = reviewer_policy.default_reviewer_rotation_list()[0] while head_of_rotation in assignments_by_person: for review_req in assignments_by_person[head_of_rotation]: assign_review_request_to_reviewer(request, review_req, review_req.form.cleaned_data["reviewer"],review_req.form.cleaned_data["add_skip"]) reqs_to_assign.remove(review_req) del assignments_by_person[head_of_rotation] - head_of_rotation = reviewer_policy.reviewer_rotation_list(group)[0] + head_of_rotation = reviewer_policy.default_reviewer_rotation_list()[0] for review_req in reqs_to_assign: assign_review_request_to_reviewer(request, review_req, review_req.form.cleaned_data["reviewer"],review_req.form.cleaned_data["add_skip"]) @@ -1661,7 +1661,7 @@ def email_open_review_assignments(request, acronym, group_type=None): partial_msg = render_to_string(template.path, { "review_assignments": review_assignments, - "rotation_list": policy_for_team(group).reviewer_rotation_list(group)[:10], + "rotation_list": policy_for_team(group).default_reviewer_rotation_list()[:10], "group" : group, }) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 86168e688..6c698d600 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -18,207 +18,240 @@ from ietf.review.utils import (current_unavailable_periods_for_reviewers, def policy_for_team(team): - return RotateWithSkipReviewerPolicy() + return RotateWithSkipReviewerPolicy(team) class AbstractReviewerPolicy: + def __init__(self, team): + self.team = team + + def default_reviewer_rotation_list(self, skip_unavailable=False, dont_skip=[]): + """ + Return a list of reviewers in the default reviewer rotation for a policy. + TODO: fix return types + """ + raise NotImplementedError + + # TODO : Change this field to deal with multiple already assigned reviewers??? + def setup_reviewer_field(self, field, review_req): + """ + Fill a choice field with the recommended assignment order of reviewers for a review request. + The field should be an instance similar to + PersonEmailChoiceField(label="Assign Reviewer", empty_label="(None)") + """ + field.queryset = field.queryset.filter(role__name="reviewer", role__group=review_req.team) + one_assignment = review_req.reviewassignment_set.first() + if one_assignment: + field.initial = one_assignment.reviewer_id - def _unavailable_reviewers(self, team, dont_skip): + choices = self._recommended_assignment_order(field.queryset, review_req) + if not field.required: + choices = [("", field.empty_label)] + choices + + field.choices = choices + + def _recommended_assignment_order(self, email_queryset, review_req): + """ + Determine the recommended assignment order for a review request, + choosing from the reviewers in email_queryset, which should be a queryset + returning Email objects. + """ + if review_req.team != self.team: + raise ValueError('Reviewer policy was passed review request belonging to different team.') + resolver = AssignmentOrderResolver(email_queryset, review_req, self.default_reviewer_rotation_list()) + return resolver.determine_ranking() + + def _unavailable_reviewers(self, dont_skip): # prune reviewers not in the rotation (but not the assigned # reviewer who must have been available for assignment anyway) reviewers_to_skip = set() - unavailable_periods = current_unavailable_periods_for_reviewers(team) + 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: reviewers_to_skip.add(person_id) - days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(team) + days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(self.team) for person_id, days_needed in days_needed_for_reviewers.items(): if person_id not in dont_skip: reviewers_to_skip.add(person_id) return reviewers_to_skip -class RotateWithSkipReviewerPolicy(AbstractReviewerPolicy): - # TODO : Change this field to deal with multiple already assigned reviewers??? - def setup_reviewer_field(self, field, review_req): - field.queryset = field.queryset.filter(role__name="reviewer", role__group=review_req.team) - one_assignment = review_req.reviewassignment_set.first() - if one_assignment: - field.initial = one_assignment.reviewer_id +class AssignmentOrderResolver: + def __init__(self, email_queryset, review_req, rotation_list): + self.review_req = review_req + self.doc = review_req.doc + self.team = review_req.team + self.rotation_list = rotation_list - choices = self._make_assignment_choices(field.queryset, review_req) - if not field.required: - choices = [("", field.empty_label)] + choices + self.possible_emails = list(email_queryset) + self.possible_person_ids = [e.person_id for e in self.possible_emails] + self._collect_context() - field.choices = choices + def _collect_context(self): + # Collect all relevant data about this team, document and review request. - def _make_assignment_choices(self, email_queryset, review_req): - doc = review_req.doc - team = review_req.team + self.doc_aliases = DocAlias.objects.filter(docs=self.doc).values_list("name", flat=True) - possible_emails = list(email_queryset) - possible_person_ids = [e.person_id for e in possible_emails] + # This data is collected as a dict, keys being person IDs, values being numbers/objects. + self.reviewer_settings = self._reviewer_settings_for_person_ids(self.possible_person_ids) + self.days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(self.team) + self.rotation_index = {p.pk: i for i, p in enumerate(self.rotation_list)} - aliases = DocAlias.objects.filter(docs=doc).values_list("name", flat=True) + # This data is collected as a set of person IDs. + self.has_reviewed_previous = self._persons_with_previous_review(self.review_req, self.possible_person_ids) + 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)) - # settings - reviewer_settings = { - r.person_id: r - for r in ReviewerSettings.objects.filter(team=team, person__in=possible_person_ids) - } - - for p in possible_person_ids: - if p not in reviewer_settings: - reviewer_settings[p] = ReviewerSettings(team=team, - filter_re=get_default_filter_re(p)) - - # frequency - days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(team) - - # rotation - rotation_index = {p.pk: i for i, p in enumerate(self.reviewer_rotation_list(team))} - - # previous review of document - has_reviewed_previous = ReviewRequest.objects.filter( - doc=doc, - reviewassignment__reviewer__person__in=possible_person_ids, - reviewassignment__state="completed", - 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)) - - # review wishes - wish_to_review = set(ReviewWish.objects.filter(team=team, person__in=possible_person_ids, - doc=doc).values_list("person", flat=True)) - - # connections - connections = {} - # examine the closest connections last to let them override - connections[doc.ad_id] = "is associated Area Director" - for r in Role.objects.filter(group=doc.group_id, - person__in=possible_person_ids).select_related("name"): - connections[r.person_id] = "is group {}".format(r.name) - if doc.shepherd: - connections[doc.shepherd.person_id] = "is shepherd of document" - for author in DocumentAuthor.objects.filter(document=doc, - person__in=possible_person_ids).values_list( - "person", flat=True): - connections[author] = "is author of document" - - # unavailable periods - unavailable_periods = current_unavailable_periods_for_reviewers(team) - - # reviewers statistics - assignment_data_for_reviewers = latest_review_assignments_for_reviewers(team) + self.connections = self._connections_with_doc(self.doc, self.possible_person_ids) + self.unavailable_periods = current_unavailable_periods_for_reviewers(self.team) + self.assignment_data_for_reviewers = latest_review_assignments_for_reviewers(self.team) + def determine_ranking(self): ranking = [] - for e in possible_emails: - settings = reviewer_settings.get(e.person_id) - - # we sort the reviewers by separate axes, listing the most - # important things first - scores = [] - explanations = [] - - def add_boolean_score(direction, expr, explanation=None): - scores.append(direction if expr else -direction) - if expr and explanation: - explanations.append(explanation) - - # unavailable for review periods - periods = unavailable_periods.get(e.person_id, []) - unavailable_at_the_moment = periods and not ( - e.person_id in has_reviewed_previous and all( - p.availability == "canfinish" for p in periods)) - add_boolean_score(-1, unavailable_at_the_moment) - - def format_period(p): - if p.end_date: - res = "unavailable until {}".format(p.end_date.isoformat()) - else: - res = "unavailable indefinitely" - return "{} ({})".format(res, p.get_availability_display()) - - if periods: - explanations.append(", ".join(format_period(p) for p in periods)) - - # misc - add_boolean_score(+1, e.person_id in has_reviewed_previous, "reviewed document before") - add_boolean_score(+1, e.person_id in wish_to_review, "wishes to review document") - add_boolean_score(-1, e.person_id in connections, - connections.get(e.person_id)) # reviewer is somehow connected: bad - add_boolean_score(-1, settings.filter_re and any( - re.search(settings.filter_re, n) for n in aliases), "filter regexp matches") - - # minimum interval between reviews - days_needed = days_needed_for_reviewers.get(e.person_id, 0) - scores.append(-days_needed) - if days_needed > 0: - explanations.append("max frequency exceeded, ready in {} {}".format(days_needed, - "day" if days_needed == 1 else "days")) - - # skip next - scores.append(-settings.skip_next) - if settings.skip_next > 0: - explanations.append("skip next {}".format(settings.skip_next)) - - # index - index = rotation_index.get(e.person_id, 0) - scores.append(-index) - explanations.append("#{}".format(index + 1)) - - # stats - stats = [] - assignment_data = assignment_data_for_reviewers.get(e.person_id, []) - - currently_open = sum(1 for d in assignment_data if d.state in ["assigned", "accepted"]) - pages = sum( - rd.doc_pages for rd in assignment_data if rd.state in ["assigned", "accepted"]) - if currently_open > 0: - stats.append("currently {count} open, {pages} pages".format(count=currently_open, - pages=pages)) - could_have_completed = [d for d in assignment_data if - d.state in ["part-completed", "completed", "no-response"]] - if could_have_completed: - no_response = len([d for d in assignment_data if d.state == 'no-response']) - if no_response: - stats.append("%s no response" % no_response) - part_completed = len([d for d in assignment_data if d.state == 'part-completed']) - if part_completed: - stats.append("%s partially complete" % part_completed) - completed = len([d for d in assignment_data if d.state == 'completed']) - if completed: - stats.append("%s fully completed" % completed) - - if stats: - explanations.append(", ".join(stats)) - - label = six.text_type(e.person) - if explanations: - label = "{}: {}".format(label, "; ".join(explanations)) - - ranking.append({ - "email": e, - "scores": scores, - "label": label, - }) + for e in self.possible_emails: + ranking.append(self._ranking_for_email(e)) ranking.sort(key=lambda r: r["scores"], reverse=True) return [(r["email"].pk, r["label"]) for r in ranking] - def possibly_advance_next_reviewer_for_team(self, team, assigned_review_to_person_id, add_skip=False): - assert assigned_review_to_person_id is not None + def _ranking_for_email(self, email): + settings = self.reviewer_settings.get(email.person_id) + scores = [] + explanations = [] - rotation_list = self.reviewer_rotation_list(team, skip_unavailable=True, - dont_skip=[assigned_review_to_person_id]) + def add_boolean_score(direction, expr, explanation=None): + scores.append(direction if expr else -direction) + if expr and explanation: + explanations.append(explanation) + + # unavailable for review periods + periods = self.unavailable_periods.get(email.person_id, []) + unavailable_at_the_moment = periods and not ( + email.person_id in self.has_reviewed_previous and all( + p.availability == "canfinish" for p in periods)) + add_boolean_score(-1, unavailable_at_the_moment) + + def format_period(p): + if p.end_date: + res = "unavailable until {}".format(p.end_date.isoformat()) + else: + res = "unavailable indefinitely" + return "{} ({})".format(res, p.get_availability_display()) + + if periods: + explanations.append(", ".join(format_period(p) for p in periods)) + + # misc + add_boolean_score(+1, email.person_id in self.has_reviewed_previous, "reviewed document before") + add_boolean_score(+1, email.person_id in self.wish_to_review, "wishes to review document") + add_boolean_score(-1, email.person_id in self.connections, + self.connections.get(email.person_id)) # reviewer is somehow connected: bad + add_boolean_score(-1, settings.filter_re and any( + re.search(settings.filter_re, n) for n in self.doc_aliases), "filter regexp matches") + + # minimum interval between reviews + days_needed = self.days_needed_for_reviewers.get(email.person_id, 0) + scores.append(-days_needed) + if days_needed > 0: + explanations.append("max frequency exceeded, ready in {} {}".format(days_needed, + "day" if days_needed == 1 else "days")) + # skip next + scores.append(-settings.skip_next) + if settings.skip_next > 0: + explanations.append("skip next {}".format(settings.skip_next)) + + # index + index = self.rotation_index.get(email.person_id, 0) + scores.append(-index) + explanations.append("#{}".format(index + 1)) + + # stats (for information, do not affect score) + stats = self._collect_reviewer_stats(email) + if stats: + explanations.append(", ".join(stats)) + + label = six.text_type(email.person) + if explanations: + label = "{}: {}".format(label, "; ".join(explanations)) + return { + "email": email, + "scores": scores, + "label": label, + } + + def _collect_reviewer_stats(self, email): + stats = [] + assignment_data = self.assignment_data_for_reviewers.get(email.person_id, []) + currently_open = sum(1 for d in assignment_data if d.state in ["assigned", "accepted"]) + pages = sum( + rd.doc_pages for rd in assignment_data if rd.state in ["assigned", "accepted"]) + if currently_open > 0: + stats.append("currently {count} open, {pages} pages".format(count=currently_open, + pages=pages)) + could_have_completed = [d for d in assignment_data if + d.state in ["part-completed", "completed", "no-response"]] + if could_have_completed: + no_response = len([d for d in assignment_data if d.state == 'no-response']) + if no_response: + stats.append("%s no response" % no_response) + part_completed = len([d for d in assignment_data if d.state == 'part-completed']) + if part_completed: + stats.append("%s partially complete" % part_completed) + completed = len([d for d in assignment_data if d.state == 'completed']) + if completed: + stats.append("%s fully completed" % completed) + return stats + + def _connections_with_doc(self, doc, person_ids): + connections = {} + # examine the closest connections last to let them override + connections[doc.ad_id] = "is associated Area Director" + for r in Role.objects.filter(group=doc.group_id, + person__in=person_ids).select_related("name"): + connections[r.person_id] = "is group {}".format(r.name) + if doc.shepherd: + connections[doc.shepherd.person_id] = "is shepherd of document" + for author in DocumentAuthor.objects.filter(document=doc, + person__in=person_ids).values_list( + "person", flat=True): + connections[author] = "is author of document" + return connections + + def _persons_with_previous_review(self, review_req, possible_person_ids): + has_reviewed_previous = ReviewRequest.objects.filter( + doc=review_req.doc, + reviewassignment__reviewer__person__in=possible_person_ids, + reviewassignment__state="completed", + 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 + for r in ReviewerSettings.objects.filter(team=self.team, person__in=person_ids) + } + for p in person_ids: + if p not in reviewer_settings: + reviewer_settings[p] = ReviewerSettings(team=self.team, + filter_re=get_default_filter_re(p)) + return reviewer_settings + + +class RotateWithSkipReviewerPolicy(AbstractReviewerPolicy): + + def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False): + assert assignee_person_id is not None + + rotation_list = self.default_reviewer_rotation_list(skip_unavailable=True, + dont_skip=[assignee_person_id]) def reviewer_at_index(i): if not rotation_list: @@ -226,46 +259,45 @@ class RotateWithSkipReviewerPolicy(AbstractReviewerPolicy): return rotation_list[i % len(rotation_list)] def reviewer_settings_for(person_id): - return (ReviewerSettings.objects.filter(team=team, person=person_id).first() - or ReviewerSettings(team=team, person_id=person_id)) - - current_i = 0 - - if assigned_review_to_person_id == reviewer_at_index(current_i): - # move 1 ahead - current_i += 1 + return (ReviewerSettings.objects.filter(team=self.team, person=person_id).first() + or ReviewerSettings(team=self.team, person_id=person_id)) if add_skip: - settings = reviewer_settings_for(assigned_review_to_person_id) + settings = reviewer_settings_for(assignee_person_id) settings.skip_next += 1 settings.save() if not rotation_list: return + + current_idx = 0 + + if assignee_person_id == reviewer_at_index(current_idx): + # Skip the first reviewer in considering who is next. + current_idx += 1 while True: - # as a clean-up step go through any with a skip next > 0 - current_reviewer_person_id = reviewer_at_index(current_i) + current_reviewer_person_id = reviewer_at_index(current_idx) settings = reviewer_settings_for(current_reviewer_person_id) if settings.skip_next > 0: settings.skip_next -= 1 settings.save() - current_i += 1 + current_idx += 1 else: - nr = NextReviewerInTeam.objects.filter(team=team).first() or NextReviewerInTeam( - team=team) + nr = NextReviewerInTeam.objects.filter(team=self.team).first() or NextReviewerInTeam( + team=self.team) nr.next_reviewer_id = current_reviewer_person_id nr.save() break - def reviewer_rotation_list(self, team, skip_unavailable=False, dont_skip=[]): - reviewers = list(Person.objects.filter(role__name="reviewer", role__group=team)) + def default_reviewer_rotation_list(self, skip_unavailable=False, dont_skip=[]): + reviewers = list(Person.objects.filter(role__name="reviewer", role__group=self.team)) reviewers.sort(key=lambda p: p.last_name()) next_reviewer_index = 0 # now to figure out where the rotation is currently at - saved_reviewer = NextReviewerInTeam.objects.filter(team=team).select_related("next_reviewer").first() + saved_reviewer = NextReviewerInTeam.objects.filter(team=self.team).select_related("next_reviewer").first() if saved_reviewer: n = saved_reviewer.next_reviewer @@ -284,7 +316,7 @@ class RotateWithSkipReviewerPolicy(AbstractReviewerPolicy): reviewers_to_skip = [] if skip_unavailable: - reviewers_to_skip = self._unavailable_reviewers(team, dont_skip) + reviewers_to_skip = self._unavailable_reviewers(dont_skip) rotation_list = [p.pk for p in rotation_list if p.pk not in reviewers_to_skip] return rotation_list diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 8f6450957..8624e6769 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -26,25 +26,25 @@ class RotateWithSkipReviewerPolicyTests(TestCase): for i in range(5) ] - self.assertEqual(reviewers, policy.reviewer_rotation_list(team)) + self.assertEqual(reviewers, policy.default_reviewer_rotation_list()) def get_skip_next(person): settings = (ReviewerSettings.objects.filter(team=team, person=person).first() or ReviewerSettings(team=team)) return settings.skip_next - policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[0].pk, add_skip=False) + policy.update_policy_state_for_assignment(assignee_person_id=reviewers[0].pk, add_skip=False) self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[1]) self.assertEqual(get_skip_next(reviewers[0]), 0) self.assertEqual(get_skip_next(reviewers[1]), 0) - policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[1].pk, add_skip=True) + policy.update_policy_state_for_assignment(assignee_person_id=reviewers[1].pk, add_skip=True) self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) self.assertEqual(get_skip_next(reviewers[1]), 1) self.assertEqual(get_skip_next(reviewers[2]), 0) # skip reviewer 2 - policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[3].pk, add_skip=True) + policy.update_policy_state_for_assignment(assignee_person_id=reviewers[3].pk, add_skip=True) self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) self.assertEqual(get_skip_next(reviewers[0]), 0) self.assertEqual(get_skip_next(reviewers[1]), 1) @@ -52,7 +52,7 @@ class RotateWithSkipReviewerPolicyTests(TestCase): self.assertEqual(get_skip_next(reviewers[3]), 1) # pick reviewer 2, use up reviewer 3's skip_next - policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[2].pk, add_skip=False) + policy.update_policy_state_for_assignment(assignee_person_id=reviewers[2].pk, add_skip=False) self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4]) self.assertEqual(get_skip_next(reviewers[0]), 0) self.assertEqual(get_skip_next(reviewers[1]), 1) @@ -61,7 +61,7 @@ class RotateWithSkipReviewerPolicyTests(TestCase): self.assertEqual(get_skip_next(reviewers[4]), 0) # check wrap-around - policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[4].pk) + policy.update_policy_state_for_assignment(assignee_person_id=reviewers[4].pk) self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[0]) self.assertEqual(get_skip_next(reviewers[0]), 0) self.assertEqual(get_skip_next(reviewers[1]), 1) @@ -72,7 +72,7 @@ class RotateWithSkipReviewerPolicyTests(TestCase): # unavailable today = datetime.date.today() UnavailablePeriod.objects.create(team=team, person=reviewers[1], start_date=today, end_date=today, availability="unavailable") - policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[0].pk) + policy.update_policy_state_for_assignment(assignee_person_id=reviewers[0].pk) self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) self.assertEqual(get_skip_next(reviewers[0]), 0) self.assertEqual(get_skip_next(reviewers[1]), 1) # don't consume that skip while the reviewer is unavailable @@ -81,7 +81,7 @@ class RotateWithSkipReviewerPolicyTests(TestCase): self.assertEqual(get_skip_next(reviewers[4]), 0) # pick unavailable anyway - policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[1].pk, add_skip=False) + policy.update_policy_state_for_assignment(assignee_person_id=reviewers[1].pk, add_skip=False) self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) self.assertEqual(get_skip_next(reviewers[0]), 0) self.assertEqual(get_skip_next(reviewers[1]), 1) @@ -95,7 +95,7 @@ class RotateWithSkipReviewerPolicyTests(TestCase): settings.save() req = ReviewRequest.objects.create(team=team, doc=doc, type_id="early", state_id="assigned", deadline=today, requested_by=reviewers[0]) ReviewAssignmentFactory(review_request=req, state_id="accepted", reviewer = reviewers[2].email_set.first(),assigned_on = req.time) - policy.possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[3].pk) + policy.update_policy_state_for_assignment(assignee_person_id=reviewers[3].pk) self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4]) self.assertEqual(get_skip_next(reviewers[0]), 0) self.assertEqual(get_skip_next(reviewers[1]), 1) diff --git a/ietf/review/utils.py b/ietf/review/utils.py index 6d7d919e0..7f7aef568 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -381,7 +381,7 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa assignment = review_req.reviewassignment_set.create(state_id='assigned', reviewer = reviewer, assigned_on = datetime.datetime.now()) from ietf.review.policies import policy_for_team - policy_for_team(review_req.team).possibly_advance_next_reviewer_for_team(review_req.team, reviewer.person_id, add_skip) + policy_for_team(review_req.team).update_policy_state_for_assignment(reviewer.person_id, add_skip) ReviewRequestDocEvent.objects.create( type="assigned_review_request", From b5a31c3c6a4ab939797a21c39c6663a7ea7e5a9f Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 11 Nov 2019 12:31:16 +0000 Subject: [PATCH 04/24] Update some terminology and docstrings. - Legacy-Id: 16983 --- ietf/doc/tests_review.py | 4 +-- ietf/doc/views_review.py | 4 +-- ietf/group/forms.py | 4 +-- ietf/group/tests_review.py | 4 +-- ietf/group/views.py | 8 +++--- ietf/review/policies.py | 51 +++++++++++++++++++++++++++++------- ietf/review/test_policies.py | 4 +-- ietf/review/utils.py | 4 +-- 8 files changed, 58 insertions(+), 25 deletions(-) diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index e6d49f9e7..221dadfd3 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -33,7 +33,7 @@ from ietf.person.models import Email, Person from ietf.review.factories import ReviewRequestFactory, ReviewAssignmentFactory from ietf.review.models import (ReviewRequest, ReviewerSettings, ReviewWish, UnavailablePeriod, NextReviewerInTeam) -from ietf.review.policies import policy_for_team +from ietf.review.policies import get_reviewer_queue_policy from ietf.utils.test_utils import TestCase from ietf.utils.test_utils import login_testing_unauthorized, reload_db_objects @@ -325,7 +325,7 @@ class ReviewTests(TestCase): # assign empty_outbox() - rotation_list = policy_for_team(review_req.team).default_reviewer_rotation_list() + rotation_list = get_reviewer_queue_policy(review_req.team).default_reviewer_rotation_list() reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[0]).first() r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk }) self.assertEqual(r.status_code, 302) diff --git a/ietf/doc/views_review.py b/ietf/doc/views_review.py index abe15dd44..cb7f28499 100644 --- a/ietf/doc/views_review.py +++ b/ietf/doc/views_review.py @@ -33,7 +33,7 @@ from ietf.ietfauth.utils import is_authorized_in_doc_stream, user_is_person, has from ietf.message.models import Message from ietf.message.utils import infer_message from ietf.person.fields import PersonEmailChoiceField, SearchablePersonField -from ietf.review.policies import policy_for_team +from ietf.review.policies import get_reviewer_queue_policy from ietf.review.utils import (active_review_teams, assign_review_request_to_reviewer, can_request_review_of_doc, can_manage_review_requests_for_team, email_review_assignment_change, email_review_request_change, @@ -294,7 +294,7 @@ class AssignReviewerForm(forms.Form): def __init__(self, review_req, *args, **kwargs): super(AssignReviewerForm, self).__init__(*args, **kwargs) - policy_for_team(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req) + get_reviewer_queue_policy(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req) @login_required diff --git a/ietf/group/forms.py b/ietf/group/forms.py index 564ddf716..4ffc9c4b6 100644 --- a/ietf/group/forms.py +++ b/ietf/group/forms.py @@ -19,7 +19,7 @@ from ietf.group.models import Group, GroupHistory, GroupStateName from ietf.person.fields import SearchableEmailsField, PersonEmailChoiceField from ietf.person.models import Person from ietf.review.models import ReviewerSettings, UnavailablePeriod, ReviewSecretarySettings -from ietf.review.policies import policy_for_team +from ietf.review.policies import get_reviewer_queue_policy from ietf.review.utils import close_review_request_states from ietf.utils.textupload import get_cleaned_text_file_content from ietf.utils.text import strip_suffix @@ -255,7 +255,7 @@ class ManageReviewRequestForm(forms.Form): self.fields["close"].widget.attrs["class"] = "form-control input-sm" - policy_for_team(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req) + get_reviewer_queue_policy(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req) self.fields["reviewer"].widget.attrs["class"] = "form-control input-sm" if self.is_bound: diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py index 56a05fa5e..63d2e969c 100644 --- a/ietf/group/tests_review.py +++ b/ietf/group/tests_review.py @@ -12,7 +12,7 @@ from pyquery import PyQuery from django.urls import reverse as urlreverse -from ietf.review.policies import policy_for_team +from ietf.review.policies import get_reviewer_queue_policy from ietf.utils.test_utils import login_testing_unauthorized, TestCase, reload_db_objects from ietf.doc.models import TelechatDocEvent from ietf.group.models import Role @@ -655,7 +655,7 @@ class BulkAssignmentTests(TestCase): secretary = RoleFactory.create(group=group,name_id='secr') docs = [DocumentFactory.create(type_id='draft',group=None) for i in range(4)] requests = [ReviewRequestFactory(team=group,doc=docs[i]) for i in range(4)] - policy = policy_for_team(group) + policy = get_reviewer_queue_policy(group) rot_list = policy.default_reviewer_rotation_list() expected_ending_head_of_rotation = rot_list[3] diff --git a/ietf/group/views.py b/ietf/group/views.py index 83d86a446..6e315db00 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -92,7 +92,7 @@ from ietf.meeting.utils import group_sessions from ietf.name.models import GroupTypeName, StreamName from ietf.person.models import Email from ietf.review.models import ReviewRequest, ReviewAssignment, ReviewerSettings, ReviewSecretarySettings -from ietf.review.policies import policy_for_team +from ietf.review.policies import get_reviewer_queue_policy from ietf.review.utils import (can_manage_review_requests_for_team, can_access_review_stats_for_team, @@ -1391,7 +1391,7 @@ def reviewer_overview(request, acronym, group_type=None): can_manage = can_manage_review_requests_for_team(request.user, group) - reviewers = policy_for_team(group).default_reviewer_rotation_list() + reviewers = get_reviewer_queue_policy(group).default_reviewer_rotation_list() reviewer_settings = { s.person_id: s for s in ReviewerSettings.objects.filter(team=group) } unavailable_periods = defaultdict(list) @@ -1541,7 +1541,7 @@ def manage_review_requests(request, acronym, group_type=None, assignment_status= # Make sure the any assignments to the person at the head # of the rotation queue are processed first so that the queue # rotates before any more assignments are processed - reviewer_policy = policy_for_team(group) + reviewer_policy = get_reviewer_queue_policy(group) head_of_rotation = reviewer_policy.default_reviewer_rotation_list()[0] while head_of_rotation in assignments_by_person: for review_req in assignments_by_person[head_of_rotation]: @@ -1661,7 +1661,7 @@ def email_open_review_assignments(request, acronym, group_type=None): partial_msg = render_to_string(template.path, { "review_assignments": review_assignments, - "rotation_list": policy_for_team(group).default_reviewer_rotation_list()[:10], + "rotation_list": get_reviewer_queue_policy(group).default_reviewer_rotation_list()[:10], "group" : group, }) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 6c698d600..84fd41353 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -16,12 +16,19 @@ from ietf.review.utils import (current_unavailable_periods_for_reviewers, get_default_filter_re, latest_review_assignments_for_reviewers) - -def policy_for_team(team): - return RotateWithSkipReviewerPolicy(team) +""" +This file contains policies regarding reviewer queues. +The policies are documented in more detail on: +https://trac.tools.ietf.org/tools/ietfdb/wiki/ReviewerQueuePolicy +Terminology used here should match terminology used in that document. +""" -class AbstractReviewerPolicy: +def get_reviewer_queue_policy(team): + return RotateWithSkipReviewerQueuePolicy(team) + + +class AbstractReviewerQueuePolicy: def __init__(self, team): self.team = team @@ -32,6 +39,12 @@ class AbstractReviewerPolicy: """ raise NotImplementedError + def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False): + """ + Update the internal state of a policy to reflect an assignment. + """ + raise NotImplementedError + # TODO : Change this field to deal with multiple already assigned reviewers??? def setup_reviewer_field(self, field, review_req): """ @@ -57,7 +70,7 @@ class AbstractReviewerPolicy: returning Email objects. """ if review_req.team != self.team: - raise ValueError('Reviewer policy was passed review request belonging to different team.') + raise ValueError('Reviewer queue policy was passed review request belonging to different team.') resolver = AssignmentOrderResolver(email_queryset, review_req, self.default_reviewer_rotation_list()) return resolver.determine_ranking() @@ -90,7 +103,7 @@ class AssignmentOrderResolver: self._collect_context() def _collect_context(self): - # Collect all relevant data about this team, document and review request. + """Collect all relevant data about this team, document and review request.""" self.doc_aliases = DocAlias.objects.filter(docs=self.doc).values_list("name", flat=True) @@ -109,6 +122,10 @@ class AssignmentOrderResolver: self.assignment_data_for_reviewers = latest_review_assignments_for_reviewers(self.team) def determine_ranking(self): + """ + 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.append(self._ranking_for_email(e)) @@ -118,6 +135,12 @@ class AssignmentOrderResolver: return [(r["email"].pk, r["label"]) for r in ranking] def _ranking_for_email(self, email): + """ + Determine the ranking for a specific Email. + Returns a dict with an email object, the scores and an explanation label. + The scores are a list of individual scores, i.e. they are prioritised, not + cumulative. + """ settings = self.reviewer_settings.get(email.person_id) scores = [] explanations = [] @@ -183,6 +206,7 @@ class AssignmentOrderResolver: } def _collect_reviewer_stats(self, email): + """Collect statistics on past reviews for a particular Email.""" stats = [] assignment_data = self.assignment_data_for_reviewers.get(email.person_id, []) currently_open = sum(1 for d in assignment_data if d.state in ["assigned", "accepted"]) @@ -206,8 +230,13 @@ class AssignmentOrderResolver: return stats def _connections_with_doc(self, doc, person_ids): + """ + Collect any connections any Person in person_ids has with a document. + Returns a dict containing Person IDs that have a connection as keys, + values being an explanation string, + """ connections = {} - # examine the closest connections last to let them override + # examine the closest connections last to let them override the label connections[doc.ad_id] = "is associated Area Director" for r in Role.objects.filter(group=doc.group_id, person__in=person_ids).select_related("name"): @@ -221,6 +250,10 @@ class AssignmentOrderResolver: return connections def _persons_with_previous_review(self, review_req, possible_person_ids): + """ + Collect anyone in possible_person_ids that have reviewed the request before. + Returns a set with Person IDs of anyone who has. + """ has_reviewed_previous = ReviewRequest.objects.filter( doc=review_req.doc, reviewassignment__reviewer__person__in=possible_person_ids, @@ -232,7 +265,7 @@ class AssignmentOrderResolver: 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 @@ -245,7 +278,7 @@ class AssignmentOrderResolver: return reviewer_settings -class RotateWithSkipReviewerPolicy(AbstractReviewerPolicy): +class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False): assert assignee_person_id is not None diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 8624e6769..26637195a 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -8,7 +8,7 @@ from ietf.group.models import Group from ietf.review.factories import ReviewAssignmentFactory from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, \ ReviewRequest -from ietf.review.policies import policy_for_team +from ietf.review.policies import get_reviewer_queue_policy from ietf.utils.test_data import create_person from ietf.utils.test_utils import TestCase @@ -17,7 +17,7 @@ class RotateWithSkipReviewerPolicyTests(TestCase): def test_possibly_advance_next_reviewer_for_team(self): team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut")) - policy = policy_for_team(team) + policy = get_reviewer_queue_policy(team) doc = WgDraftFactory() # make a bunch of reviewers diff --git a/ietf/review/utils.py b/ietf/review/utils.py index 7f7aef568..a58c43583 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -380,8 +380,8 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa assignment = review_req.reviewassignment_set.create(state_id='assigned', reviewer = reviewer, assigned_on = datetime.datetime.now()) - from ietf.review.policies import policy_for_team - policy_for_team(review_req.team).update_policy_state_for_assignment(reviewer.person_id, add_skip) + from ietf.review.policies import get_reviewer_queue_policy + get_reviewer_queue_policy(review_req.team).update_policy_state_for_assignment(reviewer.person_id, add_skip) ReviewRequestDocEvent.objects.create( type="assigned_review_request", From 48f72f2501fb9f0ebd56550080278b04e2e747ba Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 11 Nov 2019 13:48:06 +0000 Subject: [PATCH 05/24] Add tests for default reviewer rotation list. - Legacy-Id: 16984 --- ietf/review/test_policies.py | 42 ++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 26637195a..d9646864f 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -4,7 +4,7 @@ import datetime from ietf.doc.factories import WgDraftFactory from ietf.group.factories import ReviewTeamFactory -from ietf.group.models import Group +from ietf.group.models import Group, Role from ietf.review.factories import ReviewAssignmentFactory from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, \ ReviewRequest @@ -14,7 +14,45 @@ from ietf.utils.test_utils import TestCase class RotateWithSkipReviewerPolicyTests(TestCase): - def test_possibly_advance_next_reviewer_for_team(self): + 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 = get_reviewer_queue_policy(team) + + reviewers = [ + create_person(team, "reviewer", name="Test Reviewer{}".format(i), username="testreviewer{}".format(i)) + for i in range(5) + ] + reviewers_pks = [r.pk for r in reviewers] + + # This reviewer should never be included. + unavailable_reviewer = create_person(team, "reviewer", name="unavailable reviewer", username="unavailablereviewer") + UnavailablePeriod.objects.create( + team=team, + person=unavailable_reviewer, + start_date='2000-01-01', + end_date='3000-01-01', + availability=UnavailablePeriod.AVAILABILITY_CHOICES[0], + ) + + # Default policy without a NextReviewerInTeam + rotation = policy.default_reviewer_rotation_list(skip_unavailable=True) + self.assertNotIn(unavailable_reviewer.pk, rotation) + self.assertEqual(rotation, reviewers_pks) + + # Policy with a current NextReviewerInTeam + NextReviewerInTeam.objects.create(team=team, next_reviewer=reviewers[3]) + rotation = policy.default_reviewer_rotation_list(skip_unavailable=True) + self.assertNotIn(unavailable_reviewer.pk, rotation) + self.assertEqual(rotation, reviewers_pks[3:] + reviewers_pks[:3]) + + # Policy with a NextReviewerInTeam that has left the team. + Role.objects.get(person=reviewers[1]).delete() + NextReviewerInTeam.objects.filter(team=team).update(next_reviewer=reviewers[1]) + rotation = policy.default_reviewer_rotation_list(skip_unavailable=True) + self.assertNotIn(unavailable_reviewer.pk, rotation) + self.assertEqual(rotation, reviewers_pks[2:] + reviewers_pks[:1]) + + def test_update_policy_state_for_assignment(self): team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut")) policy = get_reviewer_queue_policy(team) From ce812a3a4f432a3a92bd00b8a922305618b2943a Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 11 Nov 2019 14:47:37 +0000 Subject: [PATCH 06/24] - Make skipping unavailable reviews the default, except for the reviewer_overview page. - Make default_reviewer_rotation_list use a consistent return type - Legacy-Id: 16986 --- ietf/doc/tests_review.py | 8 -------- ietf/group/views.py | 2 +- ietf/review/policies.py | 16 +++++++--------- ietf/review/test_policies.py | 20 +++++++++----------- 4 files changed, 17 insertions(+), 29 deletions(-) diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index 221dadfd3..8c3832637 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -280,13 +280,6 @@ class ReviewTests(TestCase): another_reviewer = PersonFactory.create(name = "Extra TestReviewer") # needs to be lexically greater than the existing one another_reviewer.role_set.create(name_id='reviewer', email=another_reviewer.email(), group=review_req.team) - UnavailablePeriod.objects.create( - team=review_req.team, - person=reviewer_email.person, - start_date=datetime.date.today() - datetime.timedelta(days=10), - availability="unavailable", - ) - ReviewWish.objects.create(person=reviewer_email.person, team=review_req.team, doc=doc) # pick a non-existing reviewer as next to see that we can @@ -318,7 +311,6 @@ class ReviewTests(TestCase): self.assertIn("wishes to review", reviewer_label) self.assertIn("is author", reviewer_label) self.assertIn("regexp matches", reviewer_label) - self.assertIn("unavailable indefinitely", reviewer_label) self.assertIn("skip next 1", reviewer_label) self.assertIn("#1", reviewer_label) self.assertIn("1 fully completed", reviewer_label) diff --git a/ietf/group/views.py b/ietf/group/views.py index 6e315db00..1fd2ffd7a 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -1391,7 +1391,7 @@ def reviewer_overview(request, acronym, group_type=None): can_manage = can_manage_review_requests_for_team(request.user, group) - reviewers = get_reviewer_queue_policy(group).default_reviewer_rotation_list() + reviewers = get_reviewer_queue_policy(group).default_reviewer_rotation_list(include_unavailable=True) reviewer_settings = { s.person_id: s for s in ReviewerSettings.objects.filter(team=group) } unavailable_periods = defaultdict(list) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 84fd41353..ba64c9910 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -32,10 +32,9 @@ class AbstractReviewerQueuePolicy: def __init__(self, team): self.team = team - def default_reviewer_rotation_list(self, skip_unavailable=False, dont_skip=[]): + def default_reviewer_rotation_list(self, dont_skip=[]): """ Return a list of reviewers in the default reviewer rotation for a policy. - TODO: fix return types """ raise NotImplementedError @@ -283,8 +282,8 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False): assert assignee_person_id is not None - rotation_list = self.default_reviewer_rotation_list(skip_unavailable=True, - dont_skip=[assignee_person_id]) + rotation_list = [p.id for p in self.default_reviewer_rotation_list( + dont_skip=[assignee_person_id])] def reviewer_at_index(i): if not rotation_list: @@ -324,7 +323,7 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): break - def default_reviewer_rotation_list(self, skip_unavailable=False, dont_skip=[]): + def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip=[]): reviewers = list(Person.objects.filter(role__name="reviewer", role__group=self.team)) reviewers.sort(key=lambda p: p.last_name()) next_reviewer_index = 0 @@ -347,10 +346,9 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index] - reviewers_to_skip = [] - if skip_unavailable: + if not include_unavailable: reviewers_to_skip = self._unavailable_reviewers(dont_skip) - rotation_list = [p.pk for p in rotation_list if p.pk not in reviewers_to_skip] - + rotation_list = [p for p in rotation_list if p.pk not in reviewers_to_skip] + return rotation_list diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index d9646864f..5e1290152 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -22,7 +22,6 @@ class RotateWithSkipReviewerPolicyTests(TestCase): create_person(team, "reviewer", name="Test Reviewer{}".format(i), username="testreviewer{}".format(i)) for i in range(5) ] - reviewers_pks = [r.pk for r in reviewers] # This reviewer should never be included. unavailable_reviewer = create_person(team, "reviewer", name="unavailable reviewer", username="unavailablereviewer") @@ -30,27 +29,26 @@ class RotateWithSkipReviewerPolicyTests(TestCase): team=team, person=unavailable_reviewer, start_date='2000-01-01', - end_date='3000-01-01', availability=UnavailablePeriod.AVAILABILITY_CHOICES[0], ) # Default policy without a NextReviewerInTeam - rotation = policy.default_reviewer_rotation_list(skip_unavailable=True) - self.assertNotIn(unavailable_reviewer.pk, rotation) - self.assertEqual(rotation, reviewers_pks) + rotation = policy.default_reviewer_rotation_list() + self.assertNotIn(unavailable_reviewer, rotation) + self.assertEqual(rotation, reviewers) # Policy with a current NextReviewerInTeam NextReviewerInTeam.objects.create(team=team, next_reviewer=reviewers[3]) - rotation = policy.default_reviewer_rotation_list(skip_unavailable=True) - self.assertNotIn(unavailable_reviewer.pk, rotation) - self.assertEqual(rotation, reviewers_pks[3:] + reviewers_pks[:3]) + rotation = policy.default_reviewer_rotation_list() + self.assertNotIn(unavailable_reviewer, rotation) + self.assertEqual(rotation, reviewers[3:] + reviewers[:3]) # Policy with a NextReviewerInTeam that has left the team. Role.objects.get(person=reviewers[1]).delete() NextReviewerInTeam.objects.filter(team=team).update(next_reviewer=reviewers[1]) - rotation = policy.default_reviewer_rotation_list(skip_unavailable=True) - self.assertNotIn(unavailable_reviewer.pk, rotation) - self.assertEqual(rotation, reviewers_pks[2:] + reviewers_pks[:1]) + rotation = policy.default_reviewer_rotation_list() + self.assertNotIn(unavailable_reviewer, rotation) + self.assertEqual(rotation, reviewers[2:] + reviewers[:1]) def test_update_policy_state_for_assignment(self): From 6b85d5aff1938bb5c618d403cf3340e39ac214dd Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 11 Nov 2019 16:05:11 +0000 Subject: [PATCH 07/24] - Remove consideration of unavailability from ranking for recommended assignment order, as it is obsolete - Add test for recommended assignment order - Legacy-Id: 16989 --- ietf/review/policies.py | 33 ++++++------------- ietf/review/test_policies.py | 61 +++++++++++++++++++++++++++++++++--- 2 files changed, 67 insertions(+), 27 deletions(-) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index ba64c9910..74897bb71 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -56,13 +56,13 @@ class AbstractReviewerQueuePolicy: if one_assignment: field.initial = one_assignment.reviewer_id - choices = self._recommended_assignment_order(field.queryset, review_req) + choices = self.recommended_assignment_order(field.queryset, review_req) if not field.required: choices = [("", field.empty_label)] + choices field.choices = choices - def _recommended_assignment_order(self, email_queryset, review_req): + def recommended_assignment_order(self, email_queryset, review_req): """ Determine the recommended assignment order for a review request, choosing from the reviewers in email_queryset, which should be a queryset @@ -71,7 +71,7 @@ class AbstractReviewerQueuePolicy: if review_req.team != self.team: raise ValueError('Reviewer queue policy was passed review request belonging to different team.') resolver = AssignmentOrderResolver(email_queryset, review_req, self.default_reviewer_rotation_list()) - return resolver.determine_ranking() + return [(r['email'].pk, r['label']) for r in resolver.determine_ranking()] def _unavailable_reviewers(self, dont_skip): # prune reviewers not in the rotation (but not the assigned @@ -127,11 +127,12 @@ class AssignmentOrderResolver: """ ranking = [] for e in self.possible_emails: - ranking.append(self._ranking_for_email(e)) + ranking_for_email = self._ranking_for_email(e) + if ranking_for_email: + ranking.append(ranking_for_email) ranking.sort(key=lambda r: r["scores"], reverse=True) - - return [(r["email"].pk, r["label"]) for r in ranking] + return ranking def _ranking_for_email(self, email): """ @@ -149,23 +150,9 @@ class AssignmentOrderResolver: if expr and explanation: explanations.append(explanation) - # unavailable for review periods - periods = self.unavailable_periods.get(email.person_id, []) - unavailable_at_the_moment = periods and not ( - email.person_id in self.has_reviewed_previous and all( - p.availability == "canfinish" for p in periods)) - add_boolean_score(-1, unavailable_at_the_moment) - - def format_period(p): - if p.end_date: - res = "unavailable until {}".format(p.end_date.isoformat()) - else: - res = "unavailable indefinitely" - return "{} ({})".format(res, p.get_availability_display()) - - if periods: - explanations.append(", ".join(format_period(p) for p in periods)) - + if email.person_id not in self.rotation_index: + return + # misc add_boolean_score(+1, email.person_id in self.has_reviewed_previous, "reviewed document before") add_boolean_score(+1, email.person_id in self.wish_to_review, "wishes to review document") diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 5e1290152..6a2119c7b 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -5,10 +5,11 @@ import datetime from ietf.doc.factories import WgDraftFactory from ietf.group.factories import ReviewTeamFactory from ietf.group.models import Group, Role -from ietf.review.factories import ReviewAssignmentFactory +from ietf.person.models import Email +from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, \ - ReviewRequest -from ietf.review.policies import get_reviewer_queue_policy + ReviewRequest, ReviewWish +from ietf.review.policies import get_reviewer_queue_policy, AssignmentOrderResolver from ietf.utils.test_data import create_person from ietf.utils.test_utils import TestCase @@ -49,6 +50,14 @@ class RotateWithSkipReviewerPolicyTests(TestCase): rotation = policy.default_reviewer_rotation_list() self.assertNotIn(unavailable_reviewer, rotation) self.assertEqual(rotation, reviewers[2:] + reviewers[:1]) + + 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 = get_reviewer_queue_policy(team) + + reviewer_high = create_person(team, "reviewer", name="Test Reviewer-high", username="testreviewerhigh") + reviewer_low = create_person(team, "reviewer", name="Test Reviewer-low", username="testreviewerlow") + def test_update_policy_state_for_assignment(self): @@ -137,4 +146,48 @@ class RotateWithSkipReviewerPolicyTests(TestCase): self.assertEqual(get_skip_next(reviewers[1]), 1) self.assertEqual(get_skip_next(reviewers[2]), 0) self.assertEqual(get_skip_next(reviewers[3]), 0) - self.assertEqual(get_skip_next(reviewers[4]), 0) \ No newline at end of file + self.assertEqual(get_skip_next(reviewers[4]), 0) + + +class AssignmentOrderResolverTests(TestCase): + def test_determine_ranking(self): + # reviewer_high is second in the default rotation, reviewer_low is first + # however, reviewer_high hits every score increase, reviewer_low hits every score decrease + 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") + + # Trigger author check, AD check and group check + doc = WgDraftFactory(group__acronym='mars', rev='01', authors=[reviewer_low], ad_id=reviewer_low.pk, shepherd=reviewer_low) + Role.objects.create(group=doc.group, person=reviewer_low, email=reviewer_low.email(), name_id='advisor') + + review_req = ReviewRequestFactory(doc=doc, team=team, type_id='early', state_id='assigned') + rotation_list = [reviewer_low, reviewer_high] + + # Trigger previous review check and completed review stats - TODO: something something related documents + ReviewAssignmentFactory(review_request__team=team, review_request__doc=doc, reviewer=reviewer_high.email(), state_id='completed') + # Trigger other review stats + ReviewAssignmentFactory(review_request__team=team, review_request__doc=doc, reviewer=reviewer_high.email(), state_id='no-response') + ReviewAssignmentFactory(review_request__team=team, review_request__doc=doc, reviewer=reviewer_high.email(), state_id='part-completed') + # Trigger review wish check + ReviewWish.objects.create(team=team, doc=doc, person=reviewer_high) + + # Trigger max frequency and open review stats + ReviewAssignmentFactory(review_request__team=team, reviewer=reviewer_low.email(), state_id='assigned', review_request__doc__pages=10) + # Trigger skip_next, max frequency and filter_re + ReviewerSettings.objects.create( + team=team, + person=reviewer_low, + filter_re='.*draft.*', + skip_next=2, + min_interval=91, + ) + + order = AssignmentOrderResolver(Email.objects.all(), review_req, rotation_list) + ranking = order.determine_ranking() + self.assertEqual(ranking[0]['email'], reviewer_high.email()) + self.assertEqual(ranking[1]['email'], reviewer_low.email()) + self.assertEqual(ranking[0]['scores'], [ 1, 1, 1, 1, 0, 0, -1]) + self.assertEqual(ranking[1]['scores'], [-1, -1, -1, -1, -91, -2, 0]) + self.assertEqual(ranking[0]['label'], 'Test Reviewer-high: reviewed document before; wishes to review document; #2; 1 no response, 1 partially complete, 1 fully completed') + self.assertEqual(ranking[1]['label'], 'Test Reviewer-low: is author of document; filter regexp matches; max frequency exceeded, ready in 91 days; skip next 2; #1; currently 1 open, 10 pages') From 1c84e3c36330fee73c48a0d4b7e54606ea312c87 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 11 Nov 2019 16:32:49 +0000 Subject: [PATCH 08/24] Add additional tests for queue policies and fix unused import. - Legacy-Id: 16990 --- ietf/doc/tests_review.py | 2 +- ietf/review/policies.py | 4 ++-- ietf/review/test_policies.py | 39 +++++++++++++++++++++++++++++++----- 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index 8c3832637..4ad27b79d 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -32,7 +32,7 @@ from ietf.name.models import ReviewResultName, ReviewRequestStateName, ReviewAss from ietf.person.models import Email, Person from ietf.review.factories import ReviewRequestFactory, ReviewAssignmentFactory from ietf.review.models import (ReviewRequest, ReviewerSettings, - ReviewWish, UnavailablePeriod, NextReviewerInTeam) + ReviewWish, NextReviewerInTeam) from ietf.review.policies import get_reviewer_queue_policy from ietf.utils.test_utils import TestCase diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 74897bb71..a58523c51 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -36,13 +36,13 @@ class AbstractReviewerQueuePolicy: """ Return a list of reviewers in the default reviewer rotation for a policy. """ - raise NotImplementedError + raise NotImplementedError # pragma: no cover def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False): """ Update the internal state of a policy to reflect an assignment. """ - raise NotImplementedError + raise NotImplementedError # pragma: no cover # TODO : Change this field to deal with multiple already assigned reviewers??? def setup_reviewer_field(self, field, review_req): diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 6a2119c7b..f3232776d 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -5,6 +5,7 @@ import datetime from ietf.doc.factories import WgDraftFactory from ietf.group.factories import ReviewTeamFactory from ietf.group.models import Group, Role +from ietf.person.fields import PersonEmailChoiceField from ietf.person.models import Email from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, \ @@ -50,17 +51,45 @@ class RotateWithSkipReviewerPolicyTests(TestCase): 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 = get_reviewer_queue_policy(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") + review_req = ReviewRequestFactory(team=team, type_id='early') + ReviewAssignmentFactory(review_request=review_req, reviewer=reviewer_1.email(), state_id='part-completed') + field = PersonEmailChoiceField(label="Assign Reviewer", empty_label="(None)", required=False) + + policy.setup_reviewer_field(field, review_req) + 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 = get_reviewer_queue_policy(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') - reviewer_high = create_person(team, "reviewer", name="Test Reviewer-high", username="testreviewerhigh") - reviewer_low = create_person(team, "reviewer", name="Test Reviewer-low", username="testreviewerlow") + 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_update_policy_state_for_assignment(self): - team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut")) policy = get_reviewer_queue_policy(team) doc = WgDraftFactory() @@ -158,10 +187,10 @@ class AssignmentOrderResolverTests(TestCase): reviewer_low = create_person(team, "reviewer", name="Test Reviewer-low", username="testreviewerlow") # Trigger author check, AD check and group check - doc = WgDraftFactory(group__acronym='mars', rev='01', authors=[reviewer_low], ad_id=reviewer_low.pk, shepherd=reviewer_low) + doc = WgDraftFactory(group__acronym='mars', rev='01', authors=[reviewer_low], ad=reviewer_low, shepherd=reviewer_low.email()) Role.objects.create(group=doc.group, person=reviewer_low, email=reviewer_low.email(), name_id='advisor') - review_req = ReviewRequestFactory(doc=doc, team=team, type_id='early', state_id='assigned') + review_req = ReviewRequestFactory(doc=doc, team=team, type_id='early') rotation_list = [reviewer_low, reviewer_high] # Trigger previous review check and completed review stats - TODO: something something related documents From c8812c719374a4be91f267db344f75beeb7af06c Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Tue, 12 Nov 2019 12:11:52 +0000 Subject: [PATCH 09/24] Account for 'canfinish' unavailabilities. - Legacy-Id: 16999 --- ietf/review/policies.py | 27 +++++++++++++++++++++++---- ietf/review/test_policies.py | 36 ++++++++++++++++++++++++++++++++---- 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index a58523c51..b62e87144 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -73,7 +73,7 @@ class AbstractReviewerQueuePolicy: resolver = AssignmentOrderResolver(email_queryset, review_req, self.default_reviewer_rotation_list()) return [(r['email'].pk, r['label']) for r in resolver.determine_ranking()] - def _unavailable_reviewers(self, dont_skip): + def _entirely_unavailable_reviewers(self, dont_skip): # prune reviewers not in the rotation (but not the assigned # reviewer who must have been available for assignment anyway) reviewers_to_skip = set() @@ -119,7 +119,8 @@ class AssignmentOrderResolver: self.connections = self._connections_with_doc(self.doc, self.possible_person_ids) self.unavailable_periods = current_unavailable_periods_for_reviewers(self.team) self.assignment_data_for_reviewers = latest_review_assignments_for_reviewers(self.team) - + self.unavailable_periods = current_unavailable_periods_for_reviewers(self.team) + def determine_ranking(self): """ Determine the ranking of reviewers. @@ -139,7 +140,7 @@ class AssignmentOrderResolver: Determine the ranking for a specific Email. Returns a dict with an email object, the scores and an explanation label. The scores are a list of individual scores, i.e. they are prioritised, not - cumulative. + cumulative. """ settings = self.reviewer_settings.get(email.person_id) scores = [] @@ -152,7 +153,25 @@ class AssignmentOrderResolver: if email.person_id not in self.rotation_index: return + + # If a reviewer is unavailable, they are ignored. + periods = self.unavailable_periods.get(email.person_id, []) + unavailable_at_the_moment = periods and not ( + email.person_id in self.has_reviewed_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()) + else: + res = "unavailable indefinitely" + return "{} ({})".format(res, p.get_availability_display()) + if periods: + explanations.append(", ".join(format_period(p) for p in periods)) + # misc add_boolean_score(+1, email.person_id in self.has_reviewed_previous, "reviewed document before") add_boolean_score(+1, email.person_id in self.wish_to_review, "wishes to review document") @@ -334,7 +353,7 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index] if not include_unavailable: - reviewers_to_skip = self._unavailable_reviewers(dont_skip) + reviewers_to_skip = self._entirely_unavailable_reviewers(dont_skip) rotation_list = [p for p in rotation_list if p.pk not in reviewers_to_skip] return rotation_list diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index f3232776d..04f67929a 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -31,9 +31,16 @@ class RotateWithSkipReviewerPolicyTests(TestCase): team=team, person=unavailable_reviewer, start_date='2000-01-01', - availability=UnavailablePeriod.AVAILABILITY_CHOICES[0], + 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', ) - # Default policy without a NextReviewerInTeam rotation = policy.default_reviewer_rotation_list() self.assertNotIn(unavailable_reviewer, rotation) @@ -185,13 +192,16 @@ 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 ignored because it is not in the rotation list. + create_person(team, "reviewer", name="Test Reviewer-out-of-rotation", username="testreviewer-out-of-rotation") # Trigger author check, AD check and group check doc = WgDraftFactory(group__acronym='mars', rev='01', authors=[reviewer_low], ad=reviewer_low, shepherd=reviewer_low.email()) Role.objects.create(group=doc.group, person=reviewer_low, email=reviewer_low.email(), name_id='advisor') review_req = ReviewRequestFactory(doc=doc, team=team, type_id='early') - rotation_list = [reviewer_low, reviewer_high] + rotation_list = [reviewer_low, reviewer_high, reviewer_unavailable] # Trigger previous review check and completed review stats - TODO: something something related documents ReviewAssignmentFactory(review_request__team=team, review_request__doc=doc, reviewer=reviewer_high.email(), state_id='completed') @@ -201,6 +211,23 @@ class AssignmentOrderResolverTests(TestCase): # Trigger review wish check ReviewWish.objects.create(team=team, doc=doc, person=reviewer_high) + # This period should not have an impact, because it is the canfinish type, + # and this reviewer has reviewed previously. + UnavailablePeriod.objects.create( + team=team, + person=reviewer_high, + 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 max frequency and open review stats ReviewAssignmentFactory(review_request__team=team, reviewer=reviewer_low.email(), state_id='assigned', review_request__doc__pages=10) # Trigger skip_next, max frequency and filter_re @@ -214,9 +241,10 @@ class AssignmentOrderResolverTests(TestCase): order = AssignmentOrderResolver(Email.objects.all(), review_req, rotation_list) ranking = order.determine_ranking() + self.assertEqual(len(ranking), 2) self.assertEqual(ranking[0]['email'], reviewer_high.email()) self.assertEqual(ranking[1]['email'], reviewer_low.email()) self.assertEqual(ranking[0]['scores'], [ 1, 1, 1, 1, 0, 0, -1]) self.assertEqual(ranking[1]['scores'], [-1, -1, -1, -1, -91, -2, 0]) - self.assertEqual(ranking[0]['label'], 'Test Reviewer-high: reviewed document before; wishes to review document; #2; 1 no response, 1 partially complete, 1 fully completed') + self.assertEqual(ranking[0]['label'], 'Test Reviewer-high: unavailable indefinitely (Can do follow-ups); reviewed document before; wishes to review document; #2; 1 no response, 1 partially complete, 1 fully completed') self.assertEqual(ranking[1]['label'], 'Test Reviewer-low: is author of document; filter regexp matches; max frequency exceeded, ready in 91 days; skip next 2; #1; currently 1 open, 10 pages') From c36fcdc5a7014b6cf863adb089b81308608868c1 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Wed, 13 Nov 2019 14:39:42 +0000 Subject: [PATCH 10/24] Update update_policy_state_for_assignment for new policies, fix tests, fix some other minor things. - Legacy-Id: 17023 --- ietf/doc/tests_review.py | 6 +- ietf/review/policies.py | 87 +++++++++++++++----------- ietf/review/test_policies.py | 115 +++++++++++++++-------------------- ietf/review/utils.py | 2 +- 4 files changed, 102 insertions(+), 108 deletions(-) diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index 4ad27b79d..eb9f47bcc 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -239,6 +239,7 @@ class ReviewTests(TestCase): RoleFactory(group=review_team,person__user__username='marschairman',person__name='WG Cháir Man',name_id='reviewer') secretary = RoleFactory(group=review_team,person__user__username='reviewsecretary',person__user__email='reviewsecretary@example.com',name_id='secr') ReviewerSettings.objects.create(team=review_team, person=rev_role.person, min_interval=14, skip_next=0) + queue_policy = get_reviewer_queue_policy(review_team) # review to assign to review_req = ReviewRequestFactory(team=review_team,doc=doc,state_id='requested') @@ -282,8 +283,6 @@ class ReviewTests(TestCase): ReviewWish.objects.create(person=reviewer_email.person, team=review_req.team, doc=doc) - # pick a non-existing reviewer as next to see that we can - # handle reviewers who have left NextReviewerInTeam.objects.filter(team=review_req.team).delete() NextReviewerInTeam.objects.create( team=review_req.team, @@ -317,7 +316,7 @@ class ReviewTests(TestCase): # assign empty_outbox() - rotation_list = get_reviewer_queue_policy(review_req.team).default_reviewer_rotation_list() + rotation_list = queue_policy.default_reviewer_rotation_list() reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[0]).first() r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk }) self.assertEqual(r.status_code, 302) @@ -328,7 +327,6 @@ class ReviewTests(TestCase): assignment = review_req.reviewassignment_set.first() self.assertEqual(assignment.reviewer, reviewer) self.assertEqual(assignment.state_id, "assigned") - self.assertEqual(NextReviewerInTeam.objects.get(team=review_req.team).next_reviewer, rotation_list[1]) self.assertEqual(len(outbox), 1) self.assertEqual('"Some Reviewer" ', outbox[0]["To"]) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index b62e87144..a6b96691a 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -32,9 +32,9 @@ class AbstractReviewerQueuePolicy: def __init__(self, team): self.team = team - def default_reviewer_rotation_list(self, dont_skip=[]): + def default_reviewer_rotation_list(self, dont_skip_person_ids=None): """ - Return a list of reviewers in the default reviewer rotation for a policy. + Return a list of reviewers (Person objects) in the default reviewer rotation for a policy. """ raise NotImplementedError # pragma: no cover @@ -73,24 +73,29 @@ class AbstractReviewerQueuePolicy: resolver = AssignmentOrderResolver(email_queryset, review_req, self.default_reviewer_rotation_list()) return [(r['email'].pk, r['label']) for r in resolver.determine_ranking()] - def _entirely_unavailable_reviewers(self, dont_skip): - # prune reviewers not in the rotation (but not the assigned - # reviewer who must have been available for assignment anyway) + def _entirely_unavailable_reviewers(self, dont_skip_person_ids=None): + """ + Return a set of PKs of Persons that should not be considered + to be in the rotation list at all. + """ reviewers_to_skip = set() + if not dont_skip_person_ids: + dont_skip_person_ids = [] unavailable_periods = current_unavailable_periods_for_reviewers(self.team) for person_id, periods in unavailable_periods.items(): - if periods and person_id not in dont_skip: + if periods and person_id not in dont_skip_person_ids and not any(p.availability == "canfinish" for p in periods): reviewers_to_skip.add(person_id) - days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(self.team) - for person_id, days_needed in days_needed_for_reviewers.items(): - if person_id not in dont_skip: - reviewers_to_skip.add(person_id) return reviewers_to_skip - + class AssignmentOrderResolver: + """ + The AssignmentOrderResolver resolves the "recommended assignment order", + for a set of possible reviewers (email_queryset), a review request, and a + rotation list. + """ def __init__(self, email_queryset, review_req, rotation_list): self.review_req = review_req self.doc = review_req.doc @@ -285,51 +290,61 @@ class AssignmentOrderResolver: class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): - def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False): - assert assignee_person_id is not None + def update_policy_state_for_assignment(self, assignee_person, add_skip=False): + print('====================') + assert assignee_person is not None - rotation_list = [p.id for p in self.default_reviewer_rotation_list( - dont_skip=[assignee_person_id])] + rotation_list = self.default_reviewer_rotation_list(dont_skip_person_ids=[assignee_person.pk]) def reviewer_at_index(i): if not rotation_list: return None return rotation_list[i % len(rotation_list)] - def reviewer_settings_for(person_id): - return (ReviewerSettings.objects.filter(team=self.team, person=person_id).first() - or ReviewerSettings(team=self.team, person_id=person_id)) - - if add_skip: - settings = reviewer_settings_for(assignee_person_id) - settings.skip_next += 1 - settings.save() + def reviewer_settings_for(person): + return (ReviewerSettings.objects.filter(team=self.team, person=person).first() + or ReviewerSettings(team=self.team, person=person)) if not rotation_list: return + rotation_list_without_skip = [r for r in rotation_list if not reviewer_settings_for(r).skip_next] + print('input: {} assigned'.format(assignee_person)) + print('with skipped {}'.format([r for r in rotation_list])) + print('without skip {}'.format([r for r in rotation_list_without_skip])) + print('skip counts {}'.format([(r, reviewer_settings_for(r).skip_next) for r in rotation_list])) + in_order_assignment = rotation_list_without_skip[0] == assignee_person + print('in order: {}'.format(in_order_assignment)) + + # Loop through the list until finding the first person with skip_next=0, + # who is not the current assignee. Anyone with skip_next>0 encountered before + # has their skip_next decreased. current_idx = 0 - - if assignee_person_id == reviewer_at_index(current_idx): - # Skip the first reviewer in considering who is next. - current_idx += 1 - - while True: - current_reviewer_person_id = reviewer_at_index(current_idx) - settings = reviewer_settings_for(current_reviewer_person_id) + while in_order_assignment: + current_idx_person = reviewer_at_index(current_idx) + settings = reviewer_settings_for(current_idx_person) + print('evaluating {} with skip_next {}, assignee {}'.format(current_idx_person, settings.skip_next, assignee_person)) if settings.skip_next > 0: + print('dropping skip_next') settings.skip_next -= 1 settings.save() - current_idx += 1 - else: + elif current_idx_person != assignee_person: + print('nr appointed') nr = NextReviewerInTeam.objects.filter(team=self.team).first() or NextReviewerInTeam( team=self.team) - nr.next_reviewer_id = current_reviewer_person_id + nr.next_reviewer = current_idx_person nr.save() break + current_idx += 1 + + if add_skip: + print('raising skip count for assignee') + settings = reviewer_settings_for(assignee_person) + settings.skip_next += 1 + settings.save() - def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip=[]): + def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip_person_ids=None): reviewers = list(Person.objects.filter(role__name="reviewer", role__group=self.team)) reviewers.sort(key=lambda p: p.last_name()) next_reviewer_index = 0 @@ -353,7 +368,7 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index] if not include_unavailable: - reviewers_to_skip = self._entirely_unavailable_reviewers(dont_skip) + reviewers_to_skip = self._entirely_unavailable_reviewers(dont_skip_person_ids) rotation_list = [p for p in rotation_list if p.pk not in reviewers_to_skip] return rotation_list diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 04f67929a..aa369def6 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -1,15 +1,12 @@ # Copyright The IETF Trust 2016-2019, All Rights Reserved -import datetime - from ietf.doc.factories import WgDraftFactory from ietf.group.factories import ReviewTeamFactory from ietf.group.models import Group, Role from ietf.person.fields import PersonEmailChoiceField from ietf.person.models import Email from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory -from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, \ - ReviewRequest, ReviewWish +from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, ReviewWish from ietf.review.policies import get_reviewer_queue_policy, AssignmentOrderResolver from ietf.utils.test_data import create_person from ietf.utils.test_utils import TestCase @@ -99,7 +96,6 @@ class RotateWithSkipReviewerPolicyTests(TestCase): def test_update_policy_state_for_assignment(self): team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut")) policy = get_reviewer_queue_policy(team) - doc = WgDraftFactory() # make a bunch of reviewers reviewers = [ @@ -109,81 +105,66 @@ class RotateWithSkipReviewerPolicyTests(TestCase): self.assertEqual(reviewers, policy.default_reviewer_rotation_list()) - def get_skip_next(person): - settings = (ReviewerSettings.objects.filter(team=team, person=person).first() - or ReviewerSettings(team=team)) - return settings.skip_next + def reviewer_settings_for(person): + return (ReviewerSettings.objects.filter(team=team, person=person).first() + or ReviewerSettings(team=team, person=person)) - policy.update_policy_state_for_assignment(assignee_person_id=reviewers[0].pk, add_skip=False) + def get_skip_next(person): + return reviewer_settings_for(person).skip_next + + # Regular in-order assignment without skips + policy.update_policy_state_for_assignment(assignee_person=reviewers[0], add_skip=False) self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[1]) self.assertEqual(get_skip_next(reviewers[0]), 0) self.assertEqual(get_skip_next(reviewers[1]), 0) - - policy.update_policy_state_for_assignment(assignee_person_id=reviewers[1].pk, add_skip=True) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) - self.assertEqual(get_skip_next(reviewers[1]), 1) self.assertEqual(get_skip_next(reviewers[2]), 0) + self.assertEqual(get_skip_next(reviewers[3]), 0) + self.assertEqual(get_skip_next(reviewers[4]), 0) - # skip reviewer 2 - policy.update_policy_state_for_assignment(assignee_person_id=reviewers[3].pk, add_skip=True) + # In-order assignment with add_skip + policy.update_policy_state_for_assignment(assignee_person=reviewers[1], add_skip=True) self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) self.assertEqual(get_skip_next(reviewers[0]), 0) + self.assertEqual(get_skip_next(reviewers[1]), 1) # from current add_skip=True + self.assertEqual(get_skip_next(reviewers[2]), 0) + self.assertEqual(get_skip_next(reviewers[3]), 0) + self.assertEqual(get_skip_next(reviewers[4]), 0) + + # In-order assignment to 2, but 3 has a skip_next, so 4 should be assigned. + # 3 has skip_next decreased as it is skipped over, 1 retains its skip_next + reviewer3_settings = reviewer_settings_for(reviewers[3]) + reviewer3_settings.skip_next = 2 + reviewer3_settings.save() + policy.update_policy_state_for_assignment(assignee_person=reviewers[2], add_skip=False) + self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4]) + self.assertEqual(get_skip_next(reviewers[0]), 0) + self.assertEqual(get_skip_next(reviewers[1]), 1) # from previous add_skip=true + self.assertEqual(get_skip_next(reviewers[2]), 0) + self.assertEqual(get_skip_next(reviewers[3]), 1) # from manually set skip_next - 1 + self.assertEqual(get_skip_next(reviewers[4]), 0) + + # Out of order assignments, nothing should change, + # except the add_skip=True should still apply + policy.update_policy_state_for_assignment(assignee_person=reviewers[3], add_skip=False) + policy.update_policy_state_for_assignment(assignee_person=reviewers[2], add_skip=False) + policy.update_policy_state_for_assignment(assignee_person=reviewers[1], add_skip=False) + policy.update_policy_state_for_assignment(assignee_person=reviewers[0], add_skip=True) + self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4]) + self.assertEqual(get_skip_next(reviewers[0]), 1) # from current add_skip=True self.assertEqual(get_skip_next(reviewers[1]), 1) self.assertEqual(get_skip_next(reviewers[2]), 0) self.assertEqual(get_skip_next(reviewers[3]), 1) - - # pick reviewer 2, use up reviewer 3's skip_next - policy.update_policy_state_for_assignment(assignee_person_id=reviewers[2].pk, add_skip=False) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4]) - self.assertEqual(get_skip_next(reviewers[0]), 0) - self.assertEqual(get_skip_next(reviewers[1]), 1) - self.assertEqual(get_skip_next(reviewers[2]), 0) - self.assertEqual(get_skip_next(reviewers[3]), 0) - self.assertEqual(get_skip_next(reviewers[4]), 0) - - # check wrap-around - policy.update_policy_state_for_assignment(assignee_person_id=reviewers[4].pk) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[0]) - self.assertEqual(get_skip_next(reviewers[0]), 0) - self.assertEqual(get_skip_next(reviewers[1]), 1) - self.assertEqual(get_skip_next(reviewers[2]), 0) - self.assertEqual(get_skip_next(reviewers[3]), 0) - self.assertEqual(get_skip_next(reviewers[4]), 0) - - # unavailable - today = datetime.date.today() - UnavailablePeriod.objects.create(team=team, person=reviewers[1], start_date=today, end_date=today, availability="unavailable") - policy.update_policy_state_for_assignment(assignee_person_id=reviewers[0].pk) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) - self.assertEqual(get_skip_next(reviewers[0]), 0) - self.assertEqual(get_skip_next(reviewers[1]), 1) # don't consume that skip while the reviewer is unavailable - self.assertEqual(get_skip_next(reviewers[2]), 0) - self.assertEqual(get_skip_next(reviewers[3]), 0) - self.assertEqual(get_skip_next(reviewers[4]), 0) - - # pick unavailable anyway - policy.update_policy_state_for_assignment(assignee_person_id=reviewers[1].pk, add_skip=False) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) - self.assertEqual(get_skip_next(reviewers[0]), 0) - self.assertEqual(get_skip_next(reviewers[1]), 1) - self.assertEqual(get_skip_next(reviewers[2]), 0) - self.assertEqual(get_skip_next(reviewers[3]), 0) - self.assertEqual(get_skip_next(reviewers[4]), 0) - - # not through min_interval so advance past reviewer[2] - settings, _ = ReviewerSettings.objects.get_or_create(team=team, person=reviewers[2]) - settings.min_interval = 30 - settings.save() - req = ReviewRequest.objects.create(team=team, doc=doc, type_id="early", state_id="assigned", deadline=today, requested_by=reviewers[0]) - ReviewAssignmentFactory(review_request=req, state_id="accepted", reviewer = reviewers[2].email_set.first(),assigned_on = req.time) - policy.update_policy_state_for_assignment(assignee_person_id=reviewers[3].pk) - self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4]) - self.assertEqual(get_skip_next(reviewers[0]), 0) - self.assertEqual(get_skip_next(reviewers[1]), 1) - self.assertEqual(get_skip_next(reviewers[2]), 0) - self.assertEqual(get_skip_next(reviewers[3]), 0) self.assertEqual(get_skip_next(reviewers[4]), 0) + # Regular assignment, testing wrap-around + policy.update_policy_state_for_assignment(assignee_person=reviewers[4], add_skip=False) + self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2]) + self.assertEqual(get_skip_next(reviewers[0]), 0) # skipped over with this assignment + self.assertEqual(get_skip_next(reviewers[1]), 0) # skipped over with this assignment + self.assertEqual(get_skip_next(reviewers[2]), 0) + self.assertEqual(get_skip_next(reviewers[3]), 1) + self.assertEqual(get_skip_next(reviewers[4]), 0) + class AssignmentOrderResolverTests(TestCase): def test_determine_ranking(self): diff --git a/ietf/review/utils.py b/ietf/review/utils.py index a58c43583..78a09eea2 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -381,7 +381,7 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa assignment = review_req.reviewassignment_set.create(state_id='assigned', reviewer = reviewer, assigned_on = datetime.datetime.now()) from ietf.review.policies import get_reviewer_queue_policy - get_reviewer_queue_policy(review_req.team).update_policy_state_for_assignment(reviewer.person_id, add_skip) + get_reviewer_queue_policy(review_req.team).update_policy_state_for_assignment(reviewer.person, add_skip) ReviewRequestDocEvent.objects.create( type="assigned_review_request", From c5ecfab29f3eb4511940b90a5bdfb3abc6213128 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 18 Nov 2019 10:27:18 +0000 Subject: [PATCH 11/24] Account for previous reviews of ancestor documents (see [16981]) - Legacy-Id: 17046 Note: SVN reference [16981] has been migrated to Git commit f740adcfc1b28f1ce89c48d17f8d4c252dbe23e6 --- ietf/review/policies.py | 44 ++++++++++++++++++++---------------- ietf/review/test_policies.py | 13 +++++++---- 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index a6b96691a..472713035 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -7,6 +7,7 @@ import re import six from ietf.doc.models import DocumentAuthor, DocAlias +from ietf.doc.utils import extract_complete_replaces_ancestor_mapping_for_docs from ietf.group.models import Role from ietf.person.models import Person import debug # pyflakes:ignore @@ -261,11 +262,13 @@ class AssignmentOrderResolver: def _persons_with_previous_review(self, review_req, possible_person_ids): """ - Collect anyone in possible_person_ids that have reviewed the request before. + 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=review_req.doc, + doc__name__in=doc_names, reviewassignment__reviewer__person__in=possible_person_ids, reviewassignment__state="completed", team=self.team, @@ -313,6 +316,8 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): print('with skipped {}'.format([r for r in rotation_list])) print('without skip {}'.format([r for r in rotation_list_without_skip])) print('skip counts {}'.format([(r, reviewer_settings_for(r).skip_next) for r in rotation_list])) + # In order 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 print('in order: {}'.format(in_order_assignment)) @@ -320,23 +325,24 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): # who is not the current assignee. Anyone with skip_next>0 encountered before # has their skip_next decreased. current_idx = 0 - while in_order_assignment: - current_idx_person = reviewer_at_index(current_idx) - settings = reviewer_settings_for(current_idx_person) - print('evaluating {} with skip_next {}, assignee {}'.format(current_idx_person, settings.skip_next, assignee_person)) - if settings.skip_next > 0: - print('dropping skip_next') - settings.skip_next -= 1 - settings.save() - elif current_idx_person != assignee_person: - print('nr appointed') - nr = NextReviewerInTeam.objects.filter(team=self.team).first() or NextReviewerInTeam( - team=self.team) - nr.next_reviewer = current_idx_person - nr.save() - - break - current_idx += 1 + if in_order_assignment: + while True: + current_idx_person = reviewer_at_index(current_idx) + settings = reviewer_settings_for(current_idx_person) + print('evaluating {} with skip_next {}, assignee {}'.format(current_idx_person, settings.skip_next, assignee_person)) + if settings.skip_next > 0: + print('dropping skip_next') + settings.skip_next -= 1 + settings.save() + elif current_idx_person != assignee_person: + print('nr appointed') + nr = NextReviewerInTeam.objects.filter(team=self.team).first() or NextReviewerInTeam( + team=self.team) + nr.next_reviewer = current_idx_person + nr.save() + + break + current_idx += 1 if add_skip: print('raising skip count for assignee') diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index aa369def6..b626ddefa 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -1,6 +1,6 @@ # Copyright The IETF Trust 2016-2019, All Rights Reserved -from ietf.doc.factories import WgDraftFactory +from ietf.doc.factories import WgDraftFactory, IndividualDraftFactory from ietf.group.factories import ReviewTeamFactory from ietf.group.models import Group, Role from ietf.person.fields import PersonEmailChoiceField @@ -177,15 +177,18 @@ class AssignmentOrderResolverTests(TestCase): # This reviewer should be ignored because it is not in the rotation list. create_person(team, "reviewer", name="Test Reviewer-out-of-rotation", username="testreviewer-out-of-rotation") - # Trigger author check, AD check and group check - doc = WgDraftFactory(group__acronym='mars', rev='01', authors=[reviewer_low], ad=reviewer_low, shepherd=reviewer_low.email()) + # Create a document with ancestors, that also triggers author check, AD check and group check + doc_individual = IndividualDraftFactory() + doc_wg = WgDraftFactory(relations=[('replaces', doc_individual)]) + doc_middle_wg = WgDraftFactory(relations=[('replaces', doc_wg)]) + doc = WgDraftFactory(group__acronym='mars', rev='01', authors=[reviewer_low], ad=reviewer_low, shepherd=reviewer_low.email(), relations=[('replaces', doc_middle_wg)]) Role.objects.create(group=doc.group, person=reviewer_low, email=reviewer_low.email(), name_id='advisor') review_req = ReviewRequestFactory(doc=doc, team=team, type_id='early') rotation_list = [reviewer_low, reviewer_high, reviewer_unavailable] - # Trigger previous review check and completed review stats - TODO: something something related documents - ReviewAssignmentFactory(review_request__team=team, review_request__doc=doc, reviewer=reviewer_high.email(), state_id='completed') + # Trigger previous review check (including finding ancestor documents) and completed review stats. + ReviewAssignmentFactory(review_request__team=team, review_request__doc=doc_individual, reviewer=reviewer_high.email(), state_id='completed') # Trigger other review stats ReviewAssignmentFactory(review_request__team=team, review_request__doc=doc, reviewer=reviewer_high.email(), state_id='no-response') ReviewAssignmentFactory(review_request__team=team, review_request__doc=doc, reviewer=reviewer_high.email(), state_id='part-completed') From e188da5214ae1ce0f3436749fc5480fcd7110a9f Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 18 Nov 2019 10:40:59 +0000 Subject: [PATCH 12/24] Remove development print statements and rename policy. This should now be a finished implementation of https://trac.tools.ietf.org/tools/ietfdb/wiki/ReviewerQueuePolicy except for missing LeastRecentlyUsed. - Legacy-Id: 17047 --- ietf/review/policies.py | 14 ++------------ ietf/review/test_policies.py | 2 +- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 472713035..7765340cc 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -26,7 +26,7 @@ Terminology used here should match terminology used in that document. def get_reviewer_queue_policy(team): - return RotateWithSkipReviewerQueuePolicy(team) + return RotateAlphabeticallyReviewerQueuePolicy(team) class AbstractReviewerQueuePolicy: @@ -291,10 +291,9 @@ class AssignmentOrderResolver: return reviewer_settings -class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): +class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy): def update_policy_state_for_assignment(self, assignee_person, add_skip=False): - print('====================') assert assignee_person is not None rotation_list = self.default_reviewer_rotation_list(dont_skip_person_ids=[assignee_person.pk]) @@ -312,14 +311,9 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): return rotation_list_without_skip = [r for r in rotation_list if not reviewer_settings_for(r).skip_next] - print('input: {} assigned'.format(assignee_person)) - print('with skipped {}'.format([r for r in rotation_list])) - print('without skip {}'.format([r for r in rotation_list_without_skip])) - print('skip counts {}'.format([(r, reviewer_settings_for(r).skip_next) for r in rotation_list])) # In order 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 - print('in order: {}'.format(in_order_assignment)) # Loop through the list until finding the first person with skip_next=0, # who is not the current assignee. Anyone with skip_next>0 encountered before @@ -329,13 +323,10 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): while True: current_idx_person = reviewer_at_index(current_idx) settings = reviewer_settings_for(current_idx_person) - print('evaluating {} with skip_next {}, assignee {}'.format(current_idx_person, settings.skip_next, assignee_person)) if settings.skip_next > 0: - print('dropping skip_next') settings.skip_next -= 1 settings.save() elif current_idx_person != assignee_person: - print('nr appointed') nr = NextReviewerInTeam.objects.filter(team=self.team).first() or NextReviewerInTeam( team=self.team) nr.next_reviewer = current_idx_person @@ -345,7 +336,6 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): current_idx += 1 if add_skip: - print('raising skip count for assignee') settings = reviewer_settings_for(assignee_person) settings.skip_next += 1 settings.save() diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index b626ddefa..21a197c14 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -12,7 +12,7 @@ from ietf.utils.test_data import create_person from ietf.utils.test_utils import TestCase -class RotateWithSkipReviewerPolicyTests(TestCase): +class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase): 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 = get_reviewer_queue_policy(team) From 554a839864da0cde66e2f25f1cb1166e40aa3a71 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 18 Nov 2019 12:30:11 +0000 Subject: [PATCH 13/24] Fix #2336 - Add "select me next for an assignment". Reviewers can set this flag in their reviewer settings, which triggers a mail to be sent to the secretary. They are then kept on top of the recommended assignment order. This flag is automatically reset when any assignment is made to the reviewer. - Legacy-Id: 17048 --- ietf/group/forms.py | 2 +- ietf/group/tests_review.py | 2 ++ ietf/group/views.py | 5 +++- .../0020_add_request_assignment_next.py | 26 +++++++++++++++++++ ietf/review/models.py | 1 + ietf/review/policies.py | 25 ++++++++++-------- ietf/review/test_policies.py | 19 +++++++++++--- 7 files changed, 63 insertions(+), 17 deletions(-) create mode 100644 ietf/review/migrations/0020_add_request_assignment_next.py diff --git a/ietf/group/forms.py b/ietf/group/forms.py index 4ffc9c4b6..1722e9506 100644 --- a/ietf/group/forms.py +++ b/ietf/group/forms.py @@ -276,7 +276,7 @@ class ReviewerSettingsForm(forms.ModelForm): class Meta: model = ReviewerSettings fields = ['min_interval', 'filter_re', 'skip_next', 'remind_days_before_deadline', - 'remind_days_open_reviews', 'expertise'] + 'remind_days_open_reviews', 'request_assignment_next', 'expertise'] def __init__(self, *args, **kwargs): exclude_fields = kwargs.pop('exclude_fields', []) diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py index 63d2e969c..5d4f54222 100644 --- a/ietf/group/tests_review.py +++ b/ietf/group/tests_review.py @@ -332,6 +332,7 @@ class ReviewTests(TestCase): "filter_re": "test-[regexp]", "remind_days_before_deadline": "6", "remind_days_open_reviews": "8", + "request_assignment_next": "1", "expertise": "Some expertise", }) self.assertEqual(r.status_code, 302) @@ -347,6 +348,7 @@ class ReviewTests(TestCase): msg_content = outbox[0].get_payload(decode=True).decode("utf-8").lower() self.assertTrue("frequency changed", msg_content) self.assertTrue("skip next", msg_content) + self.assertTrue("requested to be the next person", msg_content) # Normal reviewer should not be able to change skip_next r = self.client.post(url, { diff --git a/ietf/group/views.py b/ietf/group/views.py index 1fd2ffd7a..a53baecb6 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -1732,7 +1732,10 @@ def change_reviewer_settings(request, acronym, reviewer_email, group_type=None): changes.append("Frequency changed to \"{}\" from \"{}\".".format(settings.get_min_interval_display() or "Not specified", prev_min_interval or "Not specified")) if settings.skip_next != prev_skip_next: changes.append("Skip next assignments changed to {} from {}.".format(settings.skip_next, prev_skip_next)) - + if settings.request_assignment_next: + changes.append("Reviewer has requested to be the next person selected for an " + "assignment, as soon as possible, and will be on the top of " + "the queue.") if changes: email_reviewer_availability_change(request, group, reviewer_role, "\n\n".join(changes), request.user.person) diff --git a/ietf/review/migrations/0020_add_request_assignment_next.py b/ietf/review/migrations/0020_add_request_assignment_next.py new file mode 100644 index 000000000..9d61288aa --- /dev/null +++ b/ietf/review/migrations/0020_add_request_assignment_next.py @@ -0,0 +1,26 @@ +# Copyright The IETF Trust 2016-2019, All Rights Reserved +# -*- coding: utf-8 -*- +# Generated by Django 1.11.23 on 2019-11-18 04:26 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('review', '0019_auto_20191023_0829'), + ] + + operations = [ + migrations.AddField( + model_name='historicalreviewersettings', + name='request_assignment_next', + field=models.BooleanField(default=False, help_text='If you would like to be assigned to a review as soon as possible, select this option. It is automatically reset once you receive any assignment.', verbose_name='Select me next for an assignment'), + ), + migrations.AddField( + model_name='reviewersettings', + name='request_assignment_next', + field=models.BooleanField(default=False, help_text='If you would like to be assigned to a review as soon as possible, select this option. It is automatically reset once you receive any assignment.', verbose_name='Select me next for an assignment'), + ), + ] diff --git a/ietf/review/models.py b/ietf/review/models.py index a701e6f0d..522ea08a8 100644 --- a/ietf/review/models.py +++ b/ietf/review/models.py @@ -38,6 +38,7 @@ class ReviewerSettings(models.Model): skip_next = models.IntegerField(default=0, verbose_name="Skip next assignments") remind_days_before_deadline = models.IntegerField(null=True, blank=True, help_text="To get an email reminder in case you forget to do an assigned review, enter the number of days before review deadline you want to receive it. Clear the field if you don't want this reminder.") remind_days_open_reviews = models.PositiveIntegerField(null=True, blank=True, verbose_name="Periodic reminder of open reviews every X days", help_text="To get a periodic email reminder of all your open reviews, enter the number of days between these reminders. Clear the field if you don't want these reminders.") + request_assignment_next = models.BooleanField(default=False, verbose_name="Select me next for an assignment", help_text="If you would like to be assigned to a review as soon as possible, select this option. It is automatically reset once you receive any assignment.") expertise = models.TextField(verbose_name="Reviewer's expertise in this team's area", max_length=2048, blank=True, help_text="Describe the reviewer's expertise in this team's area", default='') def __str__(self): diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 7765340cc..fc679122e 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -39,11 +39,13 @@ class AbstractReviewerQueuePolicy: """ raise NotImplementedError # pragma: no cover - def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False): + def update_policy_state_for_assignment(self, assignee_person, add_skip=False): """ Update the internal state of a policy to reflect an assignment. """ - raise NotImplementedError # pragma: no cover + settings = self._reviewer_settings_for(assignee_person) + settings.request_assignment_next = False + settings.save() # TODO : Change this field to deal with multiple already assigned reviewers??? def setup_reviewer_field(self, field, review_req): @@ -89,8 +91,12 @@ class AbstractReviewerQueuePolicy: reviewers_to_skip.add(person_id) return reviewers_to_skip - + + def _reviewer_settings_for(self, person): + return (ReviewerSettings.objects.filter(team=self.team, person=person).first() + or ReviewerSettings(team=self.team, person=person)) + class AssignmentOrderResolver: """ The AssignmentOrderResolver resolves the "recommended assignment order", @@ -178,7 +184,7 @@ class AssignmentOrderResolver: if periods: explanations.append(", ".join(format_period(p) for p in periods)) - # misc + add_boolean_score(+1, settings.request_assignment_next, "requested to be selected next for assignment") add_boolean_score(+1, email.person_id in self.has_reviewed_previous, "reviewed document before") add_boolean_score(+1, email.person_id in self.wish_to_review, "wishes to review document") add_boolean_score(-1, email.person_id in self.connections, @@ -294,6 +300,7 @@ class AssignmentOrderResolver: class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy): def update_policy_state_for_assignment(self, assignee_person, add_skip=False): + super(RotateAlphabeticallyReviewerQueuePolicy, self).update_policy_state_for_assignment(assignee_person, add_skip) assert assignee_person is not None rotation_list = self.default_reviewer_rotation_list(dont_skip_person_ids=[assignee_person.pk]) @@ -303,14 +310,10 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy): return None return rotation_list[i % len(rotation_list)] - def reviewer_settings_for(person): - return (ReviewerSettings.objects.filter(team=self.team, person=person).first() - or ReviewerSettings(team=self.team, person=person)) - if not rotation_list: return - rotation_list_without_skip = [r for r in rotation_list if not reviewer_settings_for(r).skip_next] + rotation_list_without_skip = [r for r in rotation_list if not self._reviewer_settings_for(r).skip_next] # 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 @@ -322,7 +325,7 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy): if in_order_assignment: while True: current_idx_person = reviewer_at_index(current_idx) - settings = reviewer_settings_for(current_idx_person) + settings = self._reviewer_settings_for(current_idx_person) if settings.skip_next > 0: settings.skip_next -= 1 settings.save() @@ -336,7 +339,7 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy): current_idx += 1 if add_skip: - settings = reviewer_settings_for(assignee_person) + settings = self._reviewer_settings_for(assignee_person) settings.skip_next += 1 settings.save() diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 21a197c14..568905cd5 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -113,6 +113,9 @@ class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase): return reviewer_settings_for(person).skip_next # Regular in-order assignment without skips + reviewer0_settings = 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.assertEqual(get_skip_next(reviewers[0]), 0) @@ -120,6 +123,8 @@ class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase): 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) # In-order assignment with add_skip policy.update_policy_state_for_assignment(assignee_person=reviewers[1], add_skip=True) @@ -214,7 +219,7 @@ class AssignmentOrderResolverTests(TestCase): # Trigger max frequency and open review stats ReviewAssignmentFactory(review_request__team=team, reviewer=reviewer_low.email(), state_id='assigned', review_request__doc__pages=10) - # Trigger skip_next, max frequency and filter_re + # Trigger skip_next, max frequency, filter_re ReviewerSettings.objects.create( team=team, person=reviewer_low, @@ -222,13 +227,19 @@ class AssignmentOrderResolverTests(TestCase): skip_next=2, min_interval=91, ) + # Trigger "assign me next" + ReviewerSettings.objects.create( + team=team, + person=reviewer_high, + request_assignment_next=True, + ) order = AssignmentOrderResolver(Email.objects.all(), review_req, rotation_list) ranking = order.determine_ranking() self.assertEqual(len(ranking), 2) self.assertEqual(ranking[0]['email'], reviewer_high.email()) self.assertEqual(ranking[1]['email'], reviewer_low.email()) - self.assertEqual(ranking[0]['scores'], [ 1, 1, 1, 1, 0, 0, -1]) - self.assertEqual(ranking[1]['scores'], [-1, -1, -1, -1, -91, -2, 0]) - self.assertEqual(ranking[0]['label'], 'Test Reviewer-high: unavailable indefinitely (Can do follow-ups); reviewed document before; wishes to review document; #2; 1 no response, 1 partially complete, 1 fully completed') + self.assertEqual(ranking[0]['scores'], [ 1, 1, 1, 1, 1, 0, 0, -1]) + self.assertEqual(ranking[1]['scores'], [-1, -1, -1, -1, -1, -91, -2, 0]) + self.assertEqual(ranking[0]['label'], 'Test Reviewer-high: unavailable indefinitely (Can do follow-ups); requested to be selected next for assignment; reviewed document before; wishes to review document; #2; 1 no response, 1 partially complete, 1 fully completed') self.assertEqual(ranking[1]['label'], 'Test Reviewer-low: is author of document; filter regexp matches; max frequency exceeded, ready in 91 days; skip next 2; #1; currently 1 open, 10 pages') From 57ec2b3ef8e88e0ff0a7b7ba20bf30352c3a3d97 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 18 Nov 2019 14:47:45 +0000 Subject: [PATCH 14/24] Add LeastRecentlyUsed reviewer queue policy. - Legacy-Id: 17049 --- ietf/review/policies.py | 126 ++++++++++++++++++++++------------- ietf/review/test_policies.py | 67 ++++++++++++++++++- 2 files changed, 145 insertions(+), 48 deletions(-) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index fc679122e..e89c5cd10 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -3,6 +3,7 @@ from __future__ import absolute_import, print_function, unicode_literals import re +from collections import OrderedDict import six @@ -11,7 +12,8 @@ from ietf.doc.utils import extract_complete_replaces_ancestor_mapping_for_docs from ietf.group.models import Role from ietf.person.models import Person import debug # pyflakes:ignore -from ietf.review.models import NextReviewerInTeam, ReviewerSettings, ReviewWish, ReviewRequest +from ietf.review.models import NextReviewerInTeam, ReviewerSettings, ReviewWish, ReviewRequest, \ + ReviewAssignment from ietf.review.utils import (current_unavailable_periods_for_reviewers, days_needed_to_fulfill_min_interval_for_reviewers, get_default_filter_re, @@ -41,12 +43,58 @@ class AbstractReviewerQueuePolicy: def update_policy_state_for_assignment(self, assignee_person, add_skip=False): """ - Update the internal state of a policy to reflect an assignment. + 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 reviewer_at_index(i): + if not rotation_list: + return None + return rotation_list[i % len(rotation_list)] + + if not rotation_list: + return + + rotation_list_without_skip = [r for r in rotation_list if + not self._reviewer_settings_for(r).skip_next] + # 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. + current_idx = 0 + if in_order_assignment: + while True: + 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 add_skip: + settings = self._reviewer_settings_for(assignee_person) + settings.skip_next += 1 + settings.save() + # TODO : Change this field to deal with multiple already assigned reviewers??? def setup_reviewer_field(self, field, review_req): """ @@ -298,50 +346,11 @@ class AssignmentOrderResolver: class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy): - - def update_policy_state_for_assignment(self, assignee_person, add_skip=False): - super(RotateAlphabeticallyReviewerQueuePolicy, self).update_policy_state_for_assignment(assignee_person, add_skip) - assert assignee_person is not None - - rotation_list = self.default_reviewer_rotation_list(dont_skip_person_ids=[assignee_person.pk]) - - def reviewer_at_index(i): - if not rotation_list: - return None - return rotation_list[i % len(rotation_list)] - - if not rotation_list: - return - - rotation_list_without_skip = [r for r in rotation_list if not self._reviewer_settings_for(r).skip_next] - # 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. - current_idx = 0 - if in_order_assignment: - while True: - 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: - 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 add_skip: - settings = self._reviewer_settings_for(assignee_person) - settings.skip_next += 1 - settings.save() + """ + A policy in which the default rotation list is based on last name, alphabetically. + 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)) @@ -372,3 +381,28 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy): return rotation_list + +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)) + assignments = ReviewAssignment.objects.filter( + review_request__team=self.team, + state__in=['accepted', 'assigned', 'completed'], + ).order_by('assigned_on') + + reviewers_with_assignment = [assignment.reviewer.person for assignment in assignments] + reviewers_without_assignment = set(reviewers) - set(reviewers_with_assignment) + + rotation_list = sorted(list(reviewers_without_assignment), key=lambda r: r.pk) + rotation_list += list(OrderedDict.fromkeys(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 + diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 568905cd5..b33921497 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -7,15 +7,20 @@ from ietf.person.fields import PersonEmailChoiceField from ietf.person.models import Email from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, ReviewWish -from ietf.review.policies import get_reviewer_queue_policy, AssignmentOrderResolver +from ietf.review.policies import get_reviewer_queue_policy, AssignmentOrderResolver, \ + LeastRecentlyUsedReviewerQueuePolicy, RotateAlphabeticallyReviewerQueuePolicy from ietf.utils.test_data import create_person from ietf.utils.test_utils import TestCase class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase): + """ + These tests also cover the common behaviour in RotateAlphabeticallyReviewerQueuePolicy, + 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 = get_reviewer_queue_policy(team) + policy = RotateAlphabeticallyReviewerQueuePolicy(team) reviewers = [ create_person(team, "reviewer", name="Test Reviewer{}".format(i), username="testreviewer{}".format(i)) @@ -171,6 +176,64 @@ class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase): self.assertEqual(get_skip_next(reviewers[4]), 0) +class LeastRecentlyUsedReviewerQueuePolicyTest(TestCase): + """ + 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) + + 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', + ) + + # No known assignments + rotation = policy.default_reviewer_rotation_list() + self.assertNotIn(unavailable_reviewer, rotation) + self.assertEqual(rotation, reviewers) + + # Regular accepted assignment + ReviewAssignmentFactory(reviewer=reviewers[1].email(), assigned_on='2019-01-01', + state_id='accepted', review_request__team=team) + # Rejected assignment, should not affect reviewer 2's position + ReviewAssignmentFactory(reviewer=reviewers[2].email(), state_id='rejected', + review_request__team=team) + # Completed assignment, assigned before 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) + rotation = policy.default_reviewer_rotation_list() + self.assertNotIn(unavailable_reviewer, rotation) + self.assertEqual(rotation, [reviewers[2], reviewers[3], reviewers[4], reviewers[0], reviewers[1]]) + + class AssignmentOrderResolverTests(TestCase): def test_determine_ranking(self): # reviewer_high is second in the default rotation, reviewer_low is first From 1e8dda0440a895457f25ab0f940dbe09ac077de3 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 18 Nov 2019 16:10:57 +0000 Subject: [PATCH 15/24] Add request_assignment_next to review overview template, missed in [17048] - Legacy-Id: 17050 Note: SVN reference [17048] has been migrated to Git commit 554a839864da0cde66e2f25f1cb1166e40aa3a71 --- ietf/templates/ietfauth/review_overview.html | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ietf/templates/ietfauth/review_overview.html b/ietf/templates/ietfauth/review_overview.html index 39c2ab326..c7958afea 100644 --- a/ietf/templates/ietfauth/review_overview.html +++ b/ietf/templates/ietfauth/review_overview.html @@ -150,6 +150,10 @@ Periodic reminder of open reviews every X days {{ t.reviewer_settings.remind_days_open_reviews|default:"(Do not remind)" }} + + Select me next for an assignment + {{ t.reviewer_settings.request_assignment_next|yesno }} + Unavailable periods From abedd2d97066e38beec3a29910737fdb49337e5c Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 18 Nov 2019 17:29:25 +0000 Subject: [PATCH 16/24] Add support for setting reviewer queue policies per team. - Legacy-Id: 17052 --- ietf/name/admin.py | 7 +++- ietf/name/fixtures/names.json | 20 +++++++++ .../0008_reviewerqueuepolicyname.py | 38 +++++++++++++++++ ietf/name/models.py | 2 + ietf/name/resources.py | 42 ++++++++++++++----- ietf/review/factories.py | 3 ++ ...eviewteamsettings_reviewer_queue_policy.py | 23 ++++++++++ ietf/review/models.py | 4 +- ietf/review/policies.py | 19 ++++++++- ietf/review/test_policies.py | 34 ++++++++++++--- 10 files changed, 171 insertions(+), 21 deletions(-) create mode 100644 ietf/name/migrations/0008_reviewerqueuepolicyname.py create mode 100644 ietf/review/migrations/0021_reviewteamsettings_reviewer_queue_policy.py diff --git a/ietf/name/admin.py b/ietf/name/admin.py index 3924b7910..33010654c 100644 --- a/ietf/name/admin.py +++ b/ietf/name/admin.py @@ -1,7 +1,9 @@ +# Copyright The IETF Trust 2016-2019, All Rights Reserved from django.contrib import admin from ietf.name.models import ( - AgendaTypeName, BallotPositionName, ConstraintName, ContinentName, CountryName, DBTemplateTypeName, + AgendaTypeName, BallotPositionName, ConstraintName, ContinentName, CountryName, + DBTemplateTypeName, DocRelationshipName, DocReminderTypeName, DocTagName, DocTypeName, DraftSubmissionStateName, FeedbackTypeName, FormalLanguageName, GroupMilestoneStateName, GroupStateName, GroupTypeName, ImportantDateName, IntendedStdLevelName, IprDisclosureStateName, IprEventTypeName, @@ -9,7 +11,7 @@ from ietf.name.models import ( LiaisonStatementState, LiaisonStatementTagName, MeetingTypeName, NomineePositionStateName, ReviewRequestStateName, ReviewResultName, ReviewTypeName, RoleName, RoomResourceName, SessionStatusName, StdLevelName, StreamName, TimeSlotTypeName, TopicAudienceName, - DocUrlTagName, ReviewAssignmentStateName) + DocUrlTagName, ReviewAssignmentStateName, ReviewerQueuePolicyName) from ietf.stats.models import CountryAlias @@ -70,6 +72,7 @@ admin.site.register(NomineePositionStateName, NameAdmin) admin.site.register(ReviewRequestStateName, NameAdmin) admin.site.register(ReviewAssignmentStateName, NameAdmin) admin.site.register(ReviewResultName, NameAdmin) +admin.site.register(ReviewerQueuePolicyName, NameAdmin) admin.site.register(ReviewTypeName, NameAdmin) admin.site.register(RoleName, NameAdmin) admin.site.register(RoomResourceName, NameAdmin) diff --git a/ietf/name/fixtures/names.json b/ietf/name/fixtures/names.json index e0d272bb1..2f193eda7 100644 --- a/ietf/name/fixtures/names.json +++ b/ietf/name/fixtures/names.json @@ -10971,6 +10971,26 @@ "model": "name.reviewresultname", "pk": "serious-issues" }, + { + "fields": { + "desc": "", + "name": "Rotate alphabetically", + "order": 1, + "used": true + }, + "model": "name.reviewerqueuepolicyname", + "pk": "RotateAlphabetically" + }, + { + "fields": { + "desc": "", + "name": "Least recently used", + "order": 2, + "used": true + }, + "model": "name.reviewerqueuepolicyname", + "pk": "LeastRecentlyUsed" + }, { "fields": { "desc": "", diff --git a/ietf/name/migrations/0008_reviewerqueuepolicyname.py b/ietf/name/migrations/0008_reviewerqueuepolicyname.py new file mode 100644 index 000000000..7e9804245 --- /dev/null +++ b/ietf/name/migrations/0008_reviewerqueuepolicyname.py @@ -0,0 +1,38 @@ +# Copyright The IETF Trust 2019, All Rights Reserved +# -*- coding: utf-8 -*- +# Generated by Django 1.11.23 on 2019-11-18 08:35 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + def forward(apps, schema_editor): + ReviewerQueuePolicyName = apps.get_model('name', 'ReviewerQueuePolicyName') + ReviewerQueuePolicyName.objects.create(slug='RotateAlphabetically', name='Rotate alphabetically') + ReviewerQueuePolicyName.objects.create(slug='LeastRecentlyUsed', name='Least recently used') + + def reverse(self, apps): + pass + + dependencies = [ + ('name', '0007_fix_m2m_slug_id_length'), + ] + + operations = [ + migrations.CreateModel( + name='ReviewerQueuePolicyName', + fields=[ + ('slug', models.CharField(max_length=32, primary_key=True, serialize=False)), + ('name', models.CharField(max_length=255)), + ('desc', models.TextField(blank=True)), + ('used', models.BooleanField(default=True)), + ('order', models.IntegerField(default=0)), + ], + options={ + 'ordering': ['order', 'name'], + 'abstract': False, + }, + ), + migrations.RunPython(forward, reverse), + ] diff --git a/ietf/name/models.py b/ietf/name/models.py index 453b7042e..7b3c364ac 100644 --- a/ietf/name/models.py +++ b/ietf/name/models.py @@ -110,6 +110,8 @@ class ReviewResultName(NameModel): """Almost ready, Has issues, Has nits, Not Ready, On the right track, Ready, Ready with issues, Ready with nits, Serious Issues""" +class ReviewerQueuePolicyName(NameModel): + """RotateAlphabetically, LeastRecentlyUsed""" class TopicAudienceName(NameModel): """General, Nominee, Nomcom Member""" class ContinentName(NameModel): diff --git a/ietf/name/resources.py b/ietf/name/resources.py index 83f8d0b6f..9054ccbff 100644 --- a/ietf/name/resources.py +++ b/ietf/name/resources.py @@ -1,3 +1,4 @@ +# Copyright The IETF Trust 2016-2019, All Rights Reserved # Autogenerated by the makeresources management command 2015-08-27 11:01 PDT from ietf.api import ModelResource from ietf.api import ToOneField # pyflakes:ignore @@ -7,16 +8,24 @@ from tastypie.cache import SimpleCache from ietf import api -from ietf.name.models import ( AgendaTypeName, BallotPositionName, ConstraintName, - ContinentName, CountryName, DBTemplateTypeName, DocRelationshipName, DocReminderTypeName, - DocTagName, DocTypeName, DocUrlTagName, DraftSubmissionStateName, FeedbackTypeName, - FormalLanguageName, GroupMilestoneStateName, GroupStateName, GroupTypeName, - ImportantDateName, IntendedStdLevelName, IprDisclosureStateName, IprEventTypeName, - IprLicenseTypeName, LiaisonStatementEventTypeName, LiaisonStatementPurposeName, - LiaisonStatementState, LiaisonStatementTagName, MeetingTypeName, NomineePositionStateName, - ReviewAssignmentStateName, ReviewRequestStateName, ReviewResultName, ReviewTypeName, - RoleName, RoomResourceName, SessionStatusName, StdLevelName, StreamName, TimeSlotTypeName, - TopicAudienceName, ) +from ietf.name.models import (AgendaTypeName, BallotPositionName, ConstraintName, + ContinentName, CountryName, DBTemplateTypeName, DocRelationshipName, + DocReminderTypeName, + DocTagName, DocTypeName, DocUrlTagName, DraftSubmissionStateName, + FeedbackTypeName, + FormalLanguageName, GroupMilestoneStateName, GroupStateName, + GroupTypeName, + ImportantDateName, IntendedStdLevelName, IprDisclosureStateName, + IprEventTypeName, + IprLicenseTypeName, LiaisonStatementEventTypeName, + LiaisonStatementPurposeName, + LiaisonStatementState, LiaisonStatementTagName, MeetingTypeName, + NomineePositionStateName, + ReviewAssignmentStateName, ReviewRequestStateName, ReviewResultName, + ReviewTypeName, + RoleName, RoomResourceName, SessionStatusName, StdLevelName, + StreamName, TimeSlotTypeName, + TopicAudienceName, ReviewerQueuePolicyName) class TimeSlotTypeNameResource(ModelResource): class Meta: @@ -471,6 +480,19 @@ class ReviewResultNameResource(ModelResource): } api.name.register(ReviewResultNameResource()) +class ReviewerQueuePolicyNameResource(ModelResource): + class Meta: + cache = SimpleCache() + queryset = ReviewerQueuePolicyName.objects.all() + filtering = { + "slug": ALL, + "name": ALL, + "desc": ALL, + "used": ALL, + "order": ALL, + } +api.name.register(ReviewerQueuePolicyNameResource()) + class TopicAudienceNameResource(ModelResource): class Meta: cache = SimpleCache() diff --git a/ietf/review/factories.py b/ietf/review/factories.py index 7e7a2480f..1812ae5b0 100644 --- a/ietf/review/factories.py +++ b/ietf/review/factories.py @@ -1,14 +1,17 @@ +# Copyright The IETF Trust 2016-2019, All Rights Reserved import factory import datetime from ietf.review.models import ReviewTeamSettings, ReviewRequest, ReviewAssignment, ReviewerSettings from ietf.name.models import ReviewTypeName, ReviewResultName + class ReviewTeamSettingsFactory(factory.DjangoModelFactory): class Meta: model = ReviewTeamSettings group = factory.SubFactory('ietf.group.factories.GroupFactory',type_id='review') + reviewer_queue_policy_id = 'RotateAlphabetically' @factory.post_generation def review_types(obj, create, extracted, **kwargs): diff --git a/ietf/review/migrations/0021_reviewteamsettings_reviewer_queue_policy.py b/ietf/review/migrations/0021_reviewteamsettings_reviewer_queue_policy.py new file mode 100644 index 000000000..d8d09fe74 --- /dev/null +++ b/ietf/review/migrations/0021_reviewteamsettings_reviewer_queue_policy.py @@ -0,0 +1,23 @@ +# Copyright The IETF Trust 2019, All Rights Reserved +# -*- coding: utf-8 -*- +# Generated by Django 1.11.23 on 2019-11-18 08:50 +from __future__ import unicode_literals + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('name', '0008_reviewerqueuepolicyname'), + ('review', '0020_add_request_assignment_next'), + ] + + operations = [ + migrations.AddField( + model_name='reviewteamsettings', + name='reviewer_queue_policy', + field=models.ForeignKey(default='RotateAlphabetically', on_delete=django.db.models.deletion.PROTECT, to='name.ReviewerQueuePolicyName'), + ), + ] diff --git a/ietf/review/models.py b/ietf/review/models.py index 522ea08a8..cdb9c7106 100644 --- a/ietf/review/models.py +++ b/ietf/review/models.py @@ -14,7 +14,8 @@ from django.utils.encoding import python_2_unicode_compatible from ietf.doc.models import Document from ietf.group.models import Group from ietf.person.models import Person, Email -from ietf.name.models import ReviewTypeName, ReviewRequestStateName, ReviewResultName, ReviewAssignmentStateName +from ietf.name.models import ReviewTypeName, ReviewRequestStateName, ReviewResultName, \ + ReviewAssignmentStateName, ReviewerQueuePolicyName from ietf.utils.validators import validate_regular_expression_string from ietf.utils.models import ForeignKey, OneToOneField @@ -184,6 +185,7 @@ class ReviewTeamSettings(models.Model): """Holds configuration specific to groups that are review teams""" group = OneToOneField(Group) autosuggest = models.BooleanField(default=True, verbose_name="Automatically suggest possible review requests") + reviewer_queue_policy = models.ForeignKey(ReviewerQueuePolicyName, default='RotateAlphabetically', on_delete=models.PROTECT) review_types = models.ManyToManyField(ReviewTypeName, default=get_default_review_types) review_results = models.ManyToManyField(ReviewResultName, default=get_default_review_results, related_name='reviewteamsettings_review_results_set') notify_ad_when = models.ManyToManyField(ReviewResultName, related_name='reviewteamsettings_notify_ad_set', blank=True) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index e89c5cd10..cffde3e51 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -13,7 +13,7 @@ from ietf.group.models import Role from ietf.person.models import Person import debug # pyflakes:ignore from ietf.review.models import NextReviewerInTeam, ReviewerSettings, ReviewWish, ReviewRequest, \ - ReviewAssignment + ReviewAssignment, ReviewTeamSettings from ietf.review.utils import (current_unavailable_periods_for_reviewers, days_needed_to_fulfill_min_interval_for_reviewers, get_default_filter_re, @@ -28,7 +28,17 @@ Terminology used here should match terminology used in that document. def get_reviewer_queue_policy(team): - return RotateAlphabeticallyReviewerQueuePolicy(team) + try: + settings = ReviewTeamSettings.objects.get(group=team) + except ReviewTeamSettings.DoesNotExist: + raise ValueError('Request for a reviewer queue policy for team {} ' + 'which has no ReviewTeamSettings'.format(team)) + try: + policy = QUEUE_POLICY_NAME_MAPPING[settings.reviewer_queue_policy.slug] + except KeyError: + raise ValueError('Team {} has unknown reviewer queue policy: ' + '{}'.format(team, settings.reviewer_queue_policy.slug)) + return policy(team) class AbstractReviewerQueuePolicy: @@ -406,3 +416,8 @@ class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy): return rotation_list + +QUEUE_POLICY_NAME_MAPPING = { + 'RotateAlphabetically': RotateAlphabeticallyReviewerQueuePolicy, + 'LeastRecentlyUsed': LeastRecentlyUsedReviewerQueuePolicy, +} diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index b33921497..24d077db5 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -3,16 +3,38 @@ from ietf.doc.factories import WgDraftFactory, IndividualDraftFactory from ietf.group.factories import ReviewTeamFactory from ietf.group.models import Group, Role +from ietf.name.models import ReviewerQueuePolicyName from ietf.person.fields import PersonEmailChoiceField from ietf.person.models import Email from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory -from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, ReviewWish -from ietf.review.policies import get_reviewer_queue_policy, AssignmentOrderResolver, \ - LeastRecentlyUsedReviewerQueuePolicy, RotateAlphabeticallyReviewerQueuePolicy +from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, ReviewWish, \ + ReviewTeamSettings +from ietf.review.policies import (AssignmentOrderResolver, LeastRecentlyUsedReviewerQueuePolicy, + RotateAlphabeticallyReviewerQueuePolicy, + get_reviewer_queue_policy) from ietf.utils.test_data import create_person from ietf.utils.test_utils import TestCase +class GetReviewerQueuePolicyTest(TestCase): + def test_valid_policy(self): + team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut"), settings__reviewer_queue_policy_id='LeastRecentlyUsed') + policy = get_reviewer_queue_policy(team) + self.assertEqual(policy.__class__, LeastRecentlyUsedReviewerQueuePolicy) + + def test_missing_settings(self): + team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut")) + ReviewTeamSettings.objects.all().delete() + with self.assertRaises(ValueError): + get_reviewer_queue_policy(team) + + def test_invalid_policy_name(self): + ReviewerQueuePolicyName.objects.create(slug='invalid') + team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut"), settings__reviewer_queue_policy_id='invalid') + with self.assertRaises(ValueError): + get_reviewer_queue_policy(team) + + class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase): """ These tests also cover the common behaviour in RotateAlphabeticallyReviewerQueuePolicy, @@ -63,7 +85,7 @@ class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase): 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 = get_reviewer_queue_policy(team) + 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") review_req = ReviewRequestFactory(team=team, type_id='early') @@ -80,7 +102,7 @@ class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase): 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 = get_reviewer_queue_policy(team) + 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") @@ -100,7 +122,7 @@ class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase): def test_update_policy_state_for_assignment(self): team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut")) - policy = get_reviewer_queue_policy(team) + policy = RotateAlphabeticallyReviewerQueuePolicy(team) # make a bunch of reviewers reviewers = [ From 8bb6955d47af62b2a757d10f065812735ecd7330 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 18 Nov 2019 19:50:33 +0000 Subject: [PATCH 17/24] Fix #2418 - Account for previous rejected reviews in recommended assignment order - Legacy-Id: 17053 --- ietf/doc/tests_review.py | 1 - ietf/review/policies.py | 12 +++++++----- ietf/review/test_policies.py | 8 +++++--- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index eb9f47bcc..4dcf66630 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -232,7 +232,6 @@ class ReviewTests(TestCase): self.assertIn("review_request_close_comment", mail_content) def test_assign_reviewer(self): - # TODO: this test overlaps way too much with the reviewer policy doc = WgDraftFactory(pages=2) review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut")) rev_role = RoleFactory(group=review_team,person__user__username='reviewer',person__user__email='reviewer@example.com',person__name='Some Reviewer',name_id='reviewer') diff --git a/ietf/review/policies.py b/ietf/review/policies.py index cffde3e51..69143fc7e 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -182,7 +182,8 @@ class AssignmentOrderResolver: self.rotation_index = {p.pk: i for i, p in enumerate(self.rotation_list)} # This data is collected as a set of person IDs. - self.has_reviewed_previous = self._persons_with_previous_review(self.review_req, self.possible_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.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,7 +228,7 @@ class AssignmentOrderResolver: # If a reviewer is unavailable, they are ignored. periods = self.unavailable_periods.get(email.person_id, []) unavailable_at_the_moment = periods and not ( - email.person_id in self.has_reviewed_previous and + email.person_id in self.has_completed_review_previous and all(p.availability == "canfinish" for p in periods) ) if unavailable_at_the_moment: @@ -242,8 +243,9 @@ class AssignmentOrderResolver: if periods: explanations.append(", ".join(format_period(p) for p in periods)) + add_boolean_score(-1, email.person_id in self.has_rejected_review_previous, "rejected review of document before") add_boolean_score(+1, settings.request_assignment_next, "requested to be selected next for assignment") - add_boolean_score(+1, email.person_id in self.has_reviewed_previous, "reviewed document before") + add_boolean_score(+1, email.person_id in self.has_completed_review_previous, "reviewed document before") add_boolean_score(+1, email.person_id in self.wish_to_review, "wishes to review document") add_boolean_score(-1, email.person_id in self.connections, self.connections.get(email.person_id)) # reviewer is somehow connected: bad @@ -324,7 +326,7 @@ class AssignmentOrderResolver: connections[author] = "is author of document" return connections - def _persons_with_previous_review(self, review_req, possible_person_ids): + 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. @@ -334,7 +336,7 @@ class AssignmentOrderResolver: has_reviewed_previous = ReviewRequest.objects.filter( doc__name__in=doc_names, reviewassignment__reviewer__person__in=possible_person_ids, - reviewassignment__state="completed", + reviewassignment__state=state_id, team=self.team, ).distinct() if review_req.pk is not None: diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 24d077db5..dab5b015a 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -301,6 +301,8 @@ class AssignmentOrderResolverTests(TestCase): 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') # Trigger max frequency and open review stats ReviewAssignmentFactory(review_request__team=team, reviewer=reviewer_low.email(), state_id='assigned', review_request__doc__pages=10) @@ -324,7 +326,7 @@ class AssignmentOrderResolverTests(TestCase): self.assertEqual(len(ranking), 2) self.assertEqual(ranking[0]['email'], reviewer_high.email()) self.assertEqual(ranking[1]['email'], reviewer_low.email()) - self.assertEqual(ranking[0]['scores'], [ 1, 1, 1, 1, 1, 0, 0, -1]) - self.assertEqual(ranking[1]['scores'], [-1, -1, -1, -1, -1, -91, -2, 0]) + self.assertEqual(ranking[0]['scores'], [ 1, 1, 1, 1, 1, 1, 0, 0, -1]) + self.assertEqual(ranking[1]['scores'], [-1, -1, -1, -1, -1, -1, -91, -2, 0]) self.assertEqual(ranking[0]['label'], 'Test Reviewer-high: unavailable indefinitely (Can do follow-ups); requested to be selected next for assignment; reviewed document before; wishes to review document; #2; 1 no response, 1 partially complete, 1 fully completed') - self.assertEqual(ranking[1]['label'], 'Test Reviewer-low: is author of document; filter regexp matches; max frequency exceeded, ready in 91 days; skip next 2; #1; currently 1 open, 10 pages') + self.assertEqual(ranking[1]['label'], 'Test Reviewer-low: rejected review of document before; is author of document; filter regexp matches; max frequency exceeded, ready in 91 days; skip next 2; #1; currently 1 open, 10 pages') From 0c0980c6410ab6a2d1001f2476721da9658466ca Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Tue, 19 Nov 2019 10:57:56 +0000 Subject: [PATCH 18/24] Fix #2420 - Add reviewer back to top of the queue after rejected/withdrawn reviews. - Legacy-Id: 17058 --- ietf/doc/views_review.py | 6 ++++++ ietf/review/policies.py | 24 ++++++++++++++++++++++-- ietf/review/test_policies.py | 23 ++++++++++++++++++++--- 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/ietf/doc/views_review.py b/ietf/doc/views_review.py index cb7f28499..4f7a0e57c 100644 --- a/ietf/doc/views_review.py +++ b/ietf/doc/views_review.py @@ -363,6 +363,9 @@ def reject_reviewer_assignment(request, name, assignment_id): state=review_assignment.state, ) + policy = get_reviewer_queue_policy(review_assignment.review_request.team) + policy.return_reviewer_to_top_rotation(review_assignment.reviewer.person) + msg = render_to_string("review/reviewer_assignment_rejected.txt", { "by": request.user.person, "message_to_secretary": form.cleaned_data.get("message_to_secretary") @@ -409,6 +412,9 @@ def withdraw_reviewer_assignment(request, name, assignment_id): state=review_assignment.state, ) + policy = get_reviewer_queue_policy(review_assignment.review_request.team) + policy.return_reviewer_to_top_rotation(review_assignment.reviewer.person) + msg = "Review assignment withdrawn by %s"%request.user.person email_review_assignment_change(request, review_assignment, "Reviewer assignment withdrawn", msg, by=request.user.person, notify_secretary=True, notify_reviewer=True, notify_requested_by=False) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 69143fc7e..f4417b06b 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -50,7 +50,14 @@ class AbstractReviewerQueuePolicy: Return a list of reviewers (Person objects) in the default reviewer rotation for a policy. """ raise NotImplementedError # pragma: no cover - + + def return_reviewer_to_top_rotation(self, reviewer_person): + """ + Return a reviewer to the top of the rotation, e.g. because they rejected a review, + and should retroactively not have been rotated over. + """ + raise NotImplementedError # pragma: no cover + def update_policy_state_for_assignment(self, assignee_person, add_skip=False): """ Update the skip_count if the assignment was in order, and @@ -363,7 +370,6 @@ 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)) reviewers.sort(key=lambda p: p.last_name()) @@ -393,6 +399,14 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy): return rotation_list + def return_reviewer_to_top_rotation(self, reviewer_person): + # As RotateAlphabetically does not keep a full rotation list, + # returning someone to a particular order is complex. + # Instead, the "assign me next" flag is set. + settings = self._reviewer_settings_for(reviewer_person) + settings.request_assignment_next = True + settings.save() + class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy): """ @@ -418,6 +432,12 @@ class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy): return rotation_list + def return_reviewer_to_top_rotation(self, reviewer_person): + # Reviewer rotation for this policy ignored rejected/withdrawn + # reviews, so it automatically adjusts the position of someone + # who rejected a review and no action is needed. + pass + QUEUE_POLICY_NAME_MAPPING = { 'RotateAlphabetically': RotateAlphabeticallyReviewerQueuePolicy, diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index dab5b015a..88bbbb485 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -35,9 +35,9 @@ class GetReviewerQueuePolicyTest(TestCase): get_reviewer_queue_policy(team) -class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase): +class RotateAlphabeticallyReviewerAndGenericQueuePolicyTest(TestCase): """ - These tests also cover the common behaviour in RotateAlphabeticallyReviewerQueuePolicy, + 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): @@ -197,7 +197,16 @@ class RotateAlphabeticallyReviewerQueuePolicyTest(TestCase): self.assertEqual(get_skip_next(reviewers[3]), 1) self.assertEqual(get_skip_next(reviewers[4]), 0) - + def test_return_reviewer_to_top_rotation(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_top_rotation(reviewer) + self.assertTrue(ReviewerSettings.objects.get(person=reviewer).request_assignment_next) + + class LeastRecentlyUsedReviewerQueuePolicyTest(TestCase): """ These tests only cover where this policy deviates from @@ -255,6 +264,14 @@ class LeastRecentlyUsedReviewerQueuePolicyTest(TestCase): self.assertNotIn(unavailable_reviewer, rotation) self.assertEqual(rotation, [reviewers[2], reviewers[3], reviewers[4], reviewers[0], reviewers[1]]) + def test_return_reviewer_to_top_rotation(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 = LeastRecentlyUsedReviewerQueuePolicy(team) + policy.return_reviewer_to_top_rotation(reviewer) + class AssignmentOrderResolverTests(TestCase): def test_determine_ranking(self): From b1eb2643f02e8888404da3030ff3587c2b2c2380 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Tue, 19 Nov 2019 11:14:29 +0000 Subject: [PATCH 19/24] Cleanup. Branch ready for merge. (see email) - Legacy-Id: 17059 --- ietf/review/policies.py | 43 ++++++++++++++++++------------------ ietf/review/test_policies.py | 9 ++++---- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index f4417b06b..37b6203f4 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -184,20 +184,19 @@ class AssignmentOrderResolver: self.doc_aliases = DocAlias.objects.filter(docs=self.doc).values_list("name", flat=True) # This data is collected as a dict, keys being person IDs, values being numbers/objects. + self.rotation_index = {p.pk: i for i, p in enumerate(self.rotation_list)} self.reviewer_settings = self._reviewer_settings_for_person_ids(self.possible_person_ids) self.days_needed_for_reviewers = days_needed_to_fulfill_min_interval_for_reviewers(self.team) - self.rotation_index = {p.pk: i for i, p in enumerate(self.rotation_list)} + self.connections = self._connections_with_doc(self.doc, self.possible_person_ids) + self.unavailable_periods = current_unavailable_periods_for_reviewers(self.team) + self.assignment_data_for_reviewers = latest_review_assignments_for_reviewers(self.team) + 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.wish_to_review = set(ReviewWish.objects.filter(team=self.team, person__in=self.possible_person_ids, doc=self.doc).values_list("person", flat=True)) - - self.connections = self._connections_with_doc(self.doc, self.possible_person_ids) - self.unavailable_periods = current_unavailable_periods_for_reviewers(self.team) - self.assignment_data_for_reviewers = latest_review_assignments_for_reviewers(self.team) - self.unavailable_periods = current_unavailable_periods_for_reviewers(self.team) def determine_ranking(self): """ @@ -232,7 +231,7 @@ class AssignmentOrderResolver: if email.person_id not in self.rotation_index: return - # If a reviewer is unavailable, they are ignored. + # 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 @@ -265,12 +264,12 @@ class AssignmentOrderResolver: if days_needed > 0: explanations.append("max frequency exceeded, ready in {} {}".format(days_needed, "day" if days_needed == 1 else "days")) - # skip next + # skip next value scores.append(-settings.skip_next) if settings.skip_next > 0: explanations.append("skip next {}".format(settings.skip_next)) - # index + # index in the default rotation order index = self.rotation_index.get(email.person_id, 0) scores.append(-index) explanations.append("#{}".format(index + 1)) @@ -375,21 +374,21 @@ class RotateAlphabeticallyReviewerQueuePolicy(AbstractReviewerQueuePolicy): reviewers.sort(key=lambda p: p.last_name()) next_reviewer_index = 0 - # now to figure out where the rotation is currently at - saved_reviewer = NextReviewerInTeam.objects.filter(team=self.team).select_related("next_reviewer").first() - if saved_reviewer: - n = saved_reviewer.next_reviewer + next_reviewer_in_team = NextReviewerInTeam.objects.filter(team=self.team).select_related("next_reviewer").first() + if next_reviewer_in_team: + next_reviewer = next_reviewer_in_team.next_reviewer - if n not in reviewers: - # saved reviewer might not still be here, if not just - # insert and use that position (Python will wrap around, + if next_reviewer not in reviewers: + # If the next reviewer is no longer on the team, + # advance to the person that would be after them in + # the rotation. (Python will wrap around, # so no harm done by using the index on the original list # afterwards) - reviewers_with_next = reviewers[:] + [n] + reviewers_with_next = reviewers[:] + [next_reviewer] reviewers_with_next.sort(key=lambda p: p.last_name()) - next_reviewer_index = reviewers_with_next.index(n) + next_reviewer_index = reviewers_with_next.index(next_reviewer) else: - next_reviewer_index = reviewers.index(n) + next_reviewer_index = reviewers.index(next_reviewer) rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index] @@ -418,7 +417,7 @@ class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy): assignments = ReviewAssignment.objects.filter( review_request__team=self.team, state__in=['accepted', 'assigned', 'completed'], - ).order_by('assigned_on') + ).order_by('assigned_on').select_related('reviewer') reviewers_with_assignment = [assignment.reviewer.person for assignment in assignments] reviewers_without_assignment = set(reviewers) - set(reviewers_with_assignment) @@ -433,9 +432,9 @@ class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy): return rotation_list def return_reviewer_to_top_rotation(self, reviewer_person): - # Reviewer rotation for this policy ignored rejected/withdrawn + # Reviewer rotation for this policy ignores rejected/withdrawn # reviews, so it automatically adjusts the position of someone - # who rejected a review and no action is needed. + # who rejected a review and no further action is needed. pass diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 88bbbb485..1fcc59d6c 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -281,7 +281,7 @@ class AssignmentOrderResolverTests(TestCase): 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 ignored because it is not in the rotation list. + # 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") # Create a document with ancestors, that also triggers author check, AD check and group check @@ -291,9 +291,6 @@ class AssignmentOrderResolverTests(TestCase): doc = WgDraftFactory(group__acronym='mars', rev='01', authors=[reviewer_low], ad=reviewer_low, shepherd=reviewer_low.email(), relations=[('replaces', doc_middle_wg)]) Role.objects.create(group=doc.group, person=reviewer_low, email=reviewer_low.email(), name_id='advisor') - review_req = ReviewRequestFactory(doc=doc, team=team, type_id='early') - rotation_list = [reviewer_low, reviewer_high, reviewer_unavailable] - # Trigger previous review check (including finding ancestor documents) and completed review stats. ReviewAssignmentFactory(review_request__team=team, review_request__doc=doc_individual, reviewer=reviewer_high.email(), state_id='completed') # Trigger other review stats @@ -338,11 +335,15 @@ class AssignmentOrderResolverTests(TestCase): request_assignment_next=True, ) + review_req = ReviewRequestFactory(doc=doc, team=team, type_id='early') + rotation_list = [reviewer_low, reviewer_high, reviewer_unavailable] + order = AssignmentOrderResolver(Email.objects.all(), review_req, rotation_list) ranking = order.determine_ranking() self.assertEqual(len(ranking), 2) self.assertEqual(ranking[0]['email'], reviewer_high.email()) self.assertEqual(ranking[1]['email'], reviewer_low.email()) + # These scores follow the ordering of https://trac.tools.ietf.org/tools/ietfdb/wiki/ReviewerQueuePolicy, self.assertEqual(ranking[0]['scores'], [ 1, 1, 1, 1, 1, 1, 0, 0, -1]) self.assertEqual(ranking[1]['scores'], [-1, -1, -1, -1, -1, -1, -91, -2, 0]) self.assertEqual(ranking[0]['label'], 'Test Reviewer-high: unavailable indefinitely (Can do follow-ups); requested to be selected next for assignment; reviewed document before; wishes to review document; #2; 1 no response, 1 partially complete, 1 fully completed') From 3db8a0f39d7cbd78e34f258caa5522277c7344bb Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Tue, 19 Nov 2019 11:53:12 +0000 Subject: [PATCH 20/24] Fix issue where queue might not advance correctly while managing unassigned reviews, when some reviewers have a skip count. - Legacy-Id: 17060 --- ietf/group/views.py | 4 ++-- ietf/review/policies.py | 10 ++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/ietf/group/views.py b/ietf/group/views.py index a53baecb6..475dd8215 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -1542,13 +1542,13 @@ def manage_review_requests(request, acronym, group_type=None, assignment_status= # of the rotation queue are processed first so that the queue # rotates before any more assignments are processed reviewer_policy = get_reviewer_queue_policy(group) - head_of_rotation = reviewer_policy.default_reviewer_rotation_list()[0] + head_of_rotation = reviewer_policy.default_reviewer_rotation_list_without_skipped()[0] while head_of_rotation in assignments_by_person: for review_req in assignments_by_person[head_of_rotation]: assign_review_request_to_reviewer(request, review_req, review_req.form.cleaned_data["reviewer"],review_req.form.cleaned_data["add_skip"]) reqs_to_assign.remove(review_req) del assignments_by_person[head_of_rotation] - head_of_rotation = reviewer_policy.default_reviewer_rotation_list()[0] + head_of_rotation = reviewer_policy.default_reviewer_rotation_list_without_skipped()[0] for review_req in reqs_to_assign: assign_review_request_to_reviewer(request, review_req, review_req.form.cleaned_data["reviewer"],review_req.form.cleaned_data["add_skip"]) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 37b6203f4..7a352dd52 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -57,6 +57,13 @@ class AbstractReviewerQueuePolicy: and should retroactively not have been rotated over. """ raise NotImplementedError # pragma: no cover + + def default_reviewer_rotation_list_without_skipped(self): + """ + Return a list of reviewers (Person objects) in the default reviewer rotation for a policy, + while skipping those with a skip_next>0. + """ + 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): """ @@ -78,8 +85,7 @@ class AbstractReviewerQueuePolicy: if not rotation_list: return - rotation_list_without_skip = [r for r in rotation_list if - not self._reviewer_settings_for(r).skip_next] + 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 From 084978e1059c5a8f436b4407079384c42cbb57e2 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Thu, 21 Nov 2019 11:24:33 +0000 Subject: [PATCH 21/24] Fix issue where reviewers that left the team would be included in LeastRecentlyUsed - Legacy-Id: 17086 --- ietf/review/policies.py | 3 ++- ietf/review/test_policies.py | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 7a352dd52..d4e3dc6ab 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -422,7 +422,8 @@ class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy): reviewers = list(Person.objects.filter(role__name="reviewer", role__group=self.team)) assignments = ReviewAssignment.objects.filter( review_request__team=self.team, - state__in=['accepted', 'assigned', 'completed'], + state__in=['accepted', 'assigned', 'completed'], + reviewer__person__in=reviewers, ).order_by('assigned_on').select_related('reviewer') reviewers_with_assignment = [assignment.reviewer.person for assignment in assignments] diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 1fcc59d6c..76176fa7a 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -4,6 +4,7 @@ from ietf.doc.factories import WgDraftFactory, IndividualDraftFactory from ietf.group.factories import ReviewTeamFactory from ietf.group.models import Group, Role from ietf.name.models import ReviewerQueuePolicyName +from ietf.person.factories import PersonFactory from ietf.person.fields import PersonEmailChoiceField from ietf.person.models import Email from ietf.review.factories import ReviewAssignmentFactory, ReviewRequestFactory @@ -243,6 +244,9 @@ class LeastRecentlyUsedReviewerQueuePolicyTest(TestCase): 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 rotation = policy.default_reviewer_rotation_list() From 6bf7d15b70e3861a1527441910520a3093391689 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Thu, 21 Nov 2019 12:37:02 +0000 Subject: [PATCH 22/24] Add a limit to the update_policy_state_for_assignment loop to prevent infinite loops, e.g. when a team has only a single reviewer. - Legacy-Id: 17087 --- ietf/review/policies.py | 7 +++++-- ietf/review/test_policies.py | 9 +++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index d4e3dc6ab..2e123e5b1 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -92,10 +92,13 @@ class AbstractReviewerQueuePolicy: # 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. + # has their skip_next decreased. There is a cap on the number of loops, 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_loops = sum([self._reviewer_settings_for(r).skip_next for r in rotation_list]) + len(rotation_list) if in_order_assignment: - while True: + while current_idx <= max_loops: current_idx_person = reviewer_at_index(current_idx) settings = self._reviewer_settings_for(current_idx_person) if settings.skip_next > 0: diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 76176fa7a..1ca299208 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -198,6 +198,15 @@ class RotateAlphabeticallyReviewerAndGenericQueuePolicyTest(TestCase): self.assertEqual(get_skip_next(reviewers[3]), 1) self.assertEqual(get_skip_next(reviewers[4]), 0) + # 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) + # 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()) + def test_return_reviewer_to_top_rotation(self): team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", From 384c0ac7af602cd42fef200b99aa60e53743334b Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Fri, 22 Nov 2019 10:05:51 +0000 Subject: [PATCH 23/24] Improve LeastRecentlyUsed performance with select_related() fix - Legacy-Id: 17091 --- ietf/review/policies.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 2e123e5b1..22edebeca 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -427,7 +427,7 @@ class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy): review_request__team=self.team, state__in=['accepted', 'assigned', 'completed'], reviewer__person__in=reviewers, - ).order_by('assigned_on').select_related('reviewer') + ).order_by('assigned_on').select_related('reviewer', 'reviewer__person') reviewers_with_assignment = [assignment.reviewer.person for assignment in assignments] reviewers_without_assignment = set(reviewers) - set(reviewers_with_assignment) From b64066b7427f2ec8dc6dcbfd1d373a770b14e692 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Fri, 22 Nov 2019 11:00:58 +0000 Subject: [PATCH 24/24] Fix LeastRecentlyUsed policy ordering. - Legacy-Id: 17092 --- ietf/review/policies.py | 13 +++++++++---- ietf/review/test_policies.py | 17 ++++++++++++----- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 22edebeca..acf069848 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -6,6 +6,7 @@ import re from collections import OrderedDict import six +from django.db.models.aggregates import Max from ietf.doc.models import DocumentAuthor, DocAlias from ietf.doc.utils import extract_complete_replaces_ancestor_mapping_for_docs @@ -423,17 +424,21 @@ class LeastRecentlyUsedReviewerQueuePolicy(AbstractReviewerQueuePolicy): """ def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip_person_ids=None): reviewers = list(Person.objects.filter(role__name="reviewer", role__group=self.team)) + reviewers_dict = {p.pk: p for p in reviewers} assignments = ReviewAssignment.objects.filter( review_request__team=self.team, state__in=['accepted', 'assigned', 'completed'], reviewer__person__in=reviewers, - ).order_by('assigned_on').select_related('reviewer', 'reviewer__person') - - reviewers_with_assignment = [assignment.reviewer.person for assignment in assignments] + ).values('reviewer__person').annotate(most_recent=Max('assigned_on')).order_by('most_recent') + + reviewers_with_assignment = [ + reviewers_dict[assignment['reviewer__person']] + for assignment in assignments + ] reviewers_without_assignment = set(reviewers) - set(reviewers_with_assignment) rotation_list = sorted(list(reviewers_without_assignment), key=lambda r: r.pk) - rotation_list += list(OrderedDict.fromkeys(reviewers_with_assignment)) + rotation_list += reviewers_with_assignment if not include_unavailable: reviewers_to_skip = self._entirely_unavailable_reviewers(dont_skip_person_ids) diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 1ca299208..11f5bf922 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -257,27 +257,34 @@ class LeastRecentlyUsedReviewerQueuePolicyTest(TestCase): out_of_team_reviewer = PersonFactory() ReviewAssignmentFactory(review_request__team=team, reviewer=out_of_team_reviewer.email()) - # No known assignments + # No known assignments, order in PK order. rotation = policy.default_reviewer_rotation_list() self.assertNotIn(unavailable_reviewer, rotation) self.assertEqual(rotation, reviewers) - # Regular accepted assignment + # 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(), state_id='rejected', - review_request__team=team) - # Completed assignment, assigned before reviewer 1, + 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 test_return_reviewer_to_top_rotation(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"))