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

Early work to extract reviewer policy from review/utils.py.

Browse source 
- Legacy-Id: 16950
This commit is contained in:
Sasha Romijn 2019-10-31 15:01:14 +00:00
parent 2caf18dcd3
commit eab14ea1c5
8 changed files with 419 additions and 354 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

94
ietf/doc/tests_review.py
View file

@ -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)

5
ietf/doc/views_review.py
View file

@ -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

5
ietf/group/forms.py
View file

@ -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:

7
ietf/group/tests_review.py
View file

@ -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)

11
ietf/group/views.py
View file

@ -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,
})

291
ietf/review/policies.py Normal file
View file

@ -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

104
ietf/review/test_policies.py Normal file
View file

@ -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)

256
ietf/review/utils.py
View file

@ -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