From eab14ea1c533ade90475194076a7cec57c8cba8a Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Thu, 31 Oct 2019 15:01:14 +0000 Subject: [PATCH] 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