Skip to content

Commit 10a7bee

Browse files
author
Sasha Romijn
committed
Merge reviewer-queue-management on top of 6.111.1.dev0
 - Legacy-Id: 17094
2 parents 0af4504 + b64066b commit 10a7bee

18 files changed

+1023
-381
lines changed

‎ietf/doc/tests_review.py‎

Lines changed: 4 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,10 @@
3333
from ietf.person.models import Email, Person
3434
from ietf.review.factories import ReviewRequestFactory, ReviewAssignmentFactory
3535
from ietf.review.models import (ReviewRequest, ReviewerSettings,
36-
ReviewWish, UnavailablePeriod, NextReviewerInTeam)
37-
from ietf.review.utils import reviewer_rotation_list, possibly_advance_next_reviewer_for_team
36+
ReviewWish, NextReviewerInTeam)
37+
from ietf.review.policies import get_reviewer_queue_policy
3838

3939
from ietf.utils.test_utils import TestCase
40-
from ietf.utils.test_data import create_person
4140
from ietf.utils.test_utils import login_testing_unauthorized, reload_db_objects
4241
from ietf.utils.mail import outbox, empty_outbox, parseaddr, on_behalf_of
4342
from ietf.person.factories import PersonFactory
@@ -233,101 +232,14 @@ def test_close_request(self):
233232
self.assertIn("closed", mail_content)
234233
self.assertIn("review_request_close_comment", mail_content)
235234

236-
def test_possibly_advance_next_reviewer_for_team(self):
237-
238-
team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
239-
doc = WgDraftFactory()
240-
241-
# make a bunch of reviewers
242-
reviewers = [
243-
create_person(team, "reviewer", name="Test Reviewer{}".format(i), username="testreviewer{}".format(i))
244-
for i in range(5)
245-
]
246-
247-
self.assertEqual(reviewers, reviewer_rotation_list(team))
248-
249-
def get_skip_next(person):
250-
settings = (ReviewerSettings.objects.filter(team=team, person=person).first()
251-
or ReviewerSettings(team=team))
252-
return settings.skip_next
253-
254-
possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[0].pk, add_skip=False)
255-
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[1])
256-
self.assertEqual(get_skip_next(reviewers[0]), 0)
257-
self.assertEqual(get_skip_next(reviewers[1]), 0)
258-
259-
possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[1].pk, add_skip=True)
260-
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
261-
self.assertEqual(get_skip_next(reviewers[1]), 1)
262-
self.assertEqual(get_skip_next(reviewers[2]), 0)
263-
264-
# skip reviewer 2
265-
possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[3].pk, add_skip=True)
266-
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
267-
self.assertEqual(get_skip_next(reviewers[0]), 0)
268-
self.assertEqual(get_skip_next(reviewers[1]), 1)
269-
self.assertEqual(get_skip_next(reviewers[2]), 0)
270-
self.assertEqual(get_skip_next(reviewers[3]), 1)
271-
272-
# pick reviewer 2, use up reviewer 3's skip_next
273-
possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[2].pk, add_skip=False)
274-
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4])
275-
self.assertEqual(get_skip_next(reviewers[0]), 0)
276-
self.assertEqual(get_skip_next(reviewers[1]), 1)
277-
self.assertEqual(get_skip_next(reviewers[2]), 0)
278-
self.assertEqual(get_skip_next(reviewers[3]), 0)
279-
self.assertEqual(get_skip_next(reviewers[4]), 0)
280-
281-
# check wrap-around
282-
possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[4].pk)
283-
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[0])
284-
self.assertEqual(get_skip_next(reviewers[0]), 0)
285-
self.assertEqual(get_skip_next(reviewers[1]), 1)
286-
self.assertEqual(get_skip_next(reviewers[2]), 0)
287-
self.assertEqual(get_skip_next(reviewers[3]), 0)
288-
self.assertEqual(get_skip_next(reviewers[4]), 0)
289-
290-
# unavailable
291-
today = datetime.date.today()
292-
UnavailablePeriod.objects.create(team=team, person=reviewers[1], start_date=today, end_date=today, availability="unavailable")
293-
possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[0].pk)
294-
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
295-
self.assertEqual(get_skip_next(reviewers[0]), 0)
296-
self.assertEqual(get_skip_next(reviewers[1]), 1) # don't consume that skip while the reviewer is unavailable
297-
self.assertEqual(get_skip_next(reviewers[2]), 0)
298-
self.assertEqual(get_skip_next(reviewers[3]), 0)
299-
self.assertEqual(get_skip_next(reviewers[4]), 0)
300-
301-
# pick unavailable anyway
302-
possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[1].pk, add_skip=False)
303-
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[2])
304-
self.assertEqual(get_skip_next(reviewers[0]), 0)
305-
self.assertEqual(get_skip_next(reviewers[1]), 1)
306-
self.assertEqual(get_skip_next(reviewers[2]), 0)
307-
self.assertEqual(get_skip_next(reviewers[3]), 0)
308-
self.assertEqual(get_skip_next(reviewers[4]), 0)
309-
310-
# not through min_interval so advance past reviewer[2]
311-
settings, _ = ReviewerSettings.objects.get_or_create(team=team, person=reviewers[2])
312-
settings.min_interval = 30
313-
settings.save()
314-
req = ReviewRequest.objects.create(team=team, doc=doc, type_id="early", state_id="assigned", deadline=today, requested_by=reviewers[0])
315-
ReviewAssignmentFactory(review_request=req, state_id="accepted", reviewer = reviewers[2].email_set.first(),assigned_on = req.time)
316-
possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[3].pk)
317-
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4])
318-
self.assertEqual(get_skip_next(reviewers[0]), 0)
319-
self.assertEqual(get_skip_next(reviewers[1]), 1)
320-
self.assertEqual(get_skip_next(reviewers[2]), 0)
321-
self.assertEqual(get_skip_next(reviewers[3]), 0)
322-
self.assertEqual(get_skip_next(reviewers[4]), 0)
323-
324235
def test_assign_reviewer(self):
325236
doc = WgDraftFactory(pages=2)
326237
review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
327238
rev_role = RoleFactory(group=review_team,person__user__username='reviewer',person__user__email='reviewer@example.com',person__name='Some Reviewer',name_id='reviewer')
328239
RoleFactory(group=review_team,person__user__username='marschairman',person__name='WG Cháir Man',name_id='reviewer')
329240
secretary = RoleFactory(group=review_team,person__user__username='reviewsecretary',person__user__email='reviewsecretary@example.com',name_id='secr')
330241
ReviewerSettings.objects.create(team=review_team, person=rev_role.person, min_interval=14, skip_next=0)
242+
queue_policy = get_reviewer_queue_policy(review_team)
331243

332244
# review to assign to
333245
review_req = ReviewRequestFactory(team=review_team,doc=doc,state_id='requested')
@@ -369,17 +281,8 @@ def test_assign_reviewer(self):
369281
another_reviewer = PersonFactory.create(name = "Extra TestReviewer") # needs to be lexically greater than the existing one
370282
another_reviewer.role_set.create(name_id='reviewer', email=another_reviewer.email(), group=review_req.team)
371283

372-
UnavailablePeriod.objects.create(
373-
team=review_req.team,
374-
person=reviewer_email.person,
375-
start_date=datetime.date.today() - datetime.timedelta(days=10),
376-
availability="unavailable",
377-
)
378-
379284
ReviewWish.objects.create(person=reviewer_email.person, team=review_req.team, doc=doc)
380285

381-
# pick a non-existing reviewer as next to see that we can
382-
# handle reviewers who have left
383286
NextReviewerInTeam.objects.filter(team=review_req.team).delete()
384287
NextReviewerInTeam.objects.create(
385288
team=review_req.team,
@@ -407,14 +310,13 @@ def test_assign_reviewer(self):
407310
self.assertIn("wishes to review", reviewer_label)
408311
self.assertIn("is author", reviewer_label)
409312
self.assertIn("regexp matches", reviewer_label)
410-
self.assertIn("unavailable indefinitely", reviewer_label)
411313
self.assertIn("skip next 1", reviewer_label)
412314
self.assertIn("#1", reviewer_label)
413315
self.assertIn("1 fully completed", reviewer_label)
414316

415317
# assign
416318
empty_outbox()
417-
rotation_list = reviewer_rotation_list(review_req.team)
319+
rotation_list = queue_policy.default_reviewer_rotation_list()
418320
reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[0]).first()
419321
r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk })
420322
self.assertEqual(r.status_code, 302)
@@ -425,7 +327,6 @@ def test_assign_reviewer(self):
425327
assignment = review_req.reviewassignment_set.first()
426328
self.assertEqual(assignment.reviewer, reviewer)
427329
self.assertEqual(assignment.state_id, "assigned")
428-
self.assertEqual(NextReviewerInTeam.objects.get(team=review_req.team).next_reviewer, rotation_list[1])
429330

430331
self.assertEqual(len(outbox), 1)
431332
self.assertEqual('"Some Reviewer" <reviewer@example.com>', outbox[0]["To"])

‎ietf/doc/views_review.py‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,12 @@
3636
from ietf.message.models import Message
3737
from ietf.message.utils import infer_message
3838
from ietf.person.fields import PersonEmailChoiceField, SearchablePersonField
39+
from ietf.review.policies import get_reviewer_queue_policy
3940
from ietf.review.utils import (active_review_teams, assign_review_request_to_reviewer,
4041
can_request_review_of_doc, can_manage_review_requests_for_team,
4142
email_review_assignment_change, email_review_request_change,
4243
close_review_request_states,
43-
close_review_request, setup_reviewer_field)
44+
close_review_request)
4445
from ietf.review import mailarch
4546
from ietf.utils.fields import DatepickerDateField
4647
from ietf.utils.text import strip_prefix, xslugify
@@ -296,7 +297,7 @@ class AssignReviewerForm(forms.Form):
296297

297298
def __init__(self, review_req, *args, **kwargs):
298299
super(AssignReviewerForm, self).__init__(*args, **kwargs)
299-
setup_reviewer_field(self.fields["reviewer"], review_req)
300+
get_reviewer_queue_policy(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req)
300301

301302

302303
@login_required
@@ -365,6 +366,9 @@ def reject_reviewer_assignment(request, name, assignment_id):
365366
state=review_assignment.state,
366367
)
367368

369+
policy = get_reviewer_queue_policy(review_assignment.review_request.team)
370+
policy.return_reviewer_to_top_rotation(review_assignment.reviewer.person)
371+
368372
msg = render_to_string("review/reviewer_assignment_rejected.txt", {
369373
"by": request.user.person,
370374
"message_to_secretary": form.cleaned_data.get("message_to_secretary")
@@ -411,6 +415,9 @@ def withdraw_reviewer_assignment(request, name, assignment_id):
411415
state=review_assignment.state,
412416
)
413417

418+
policy = get_reviewer_queue_policy(review_assignment.review_request.team)
419+
policy.return_reviewer_to_top_rotation(review_assignment.reviewer.person)
420+
414421
msg = "Review assignment withdrawn by %s"%request.user.person
415422

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

‎ietf/group/forms.py‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@
2020
from ietf.person.fields import SearchableEmailsField, PersonEmailChoiceField
2121
from ietf.person.models import Person
2222
from ietf.review.models import ReviewerSettings, UnavailablePeriod, ReviewSecretarySettings
23-
from ietf.review.utils import close_review_request_states, setup_reviewer_field
23+
from ietf.review.policies import get_reviewer_queue_policy
24+
from ietf.review.utils import close_review_request_states
2425
from ietf.utils.textupload import get_cleaned_text_file_content
2526
from ietf.utils.text import strip_suffix
2627
#from ietf.utils.ordereddict import insert_after_in_ordered_dict
@@ -256,7 +257,7 @@ def __init__(self, review_req, *args, **kwargs):
256257

257258
self.fields["close"].widget.attrs["class"] = "form-control input-sm"
258259

259-
setup_reviewer_field(self.fields["reviewer"], review_req)
260+
get_reviewer_queue_policy(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req)
260261
self.fields["reviewer"].widget.attrs["class"] = "form-control input-sm"
261262

262263
if not getattr(review_req, 'in_lc_and_telechat', False):
@@ -280,7 +281,7 @@ class ReviewerSettingsForm(forms.ModelForm):
280281
class Meta:
281282
model = ReviewerSettings
282283
fields = ['min_interval', 'filter_re', 'skip_next', 'remind_days_before_deadline',
283-
'remind_days_open_reviews', 'expertise']
284+
'remind_days_open_reviews', 'request_assignment_next', 'expertise']
284285

285286
def __init__(self, *args, **kwargs):
286287
exclude_fields = kwargs.pop('exclude_fields', [])

‎ietf/group/tests_review.py‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
from django.urls import reverse as urlreverse
1414

15+
from ietf.review.policies import get_reviewer_queue_policy
1516
from ietf.utils.test_utils import login_testing_unauthorized, TestCase, reload_db_objects
1617
from ietf.doc.models import TelechatDocEvent, LastCallDocEvent, State
1718
from ietf.group.models import Role
@@ -23,7 +24,6 @@
2324
suggested_review_requests_for_team,
2425
review_assignments_needing_reviewer_reminder, email_reviewer_reminder,
2526
review_assignments_needing_secretary_reminder, email_secretary_reminder,
26-
reviewer_rotation_list,
2727
send_unavaibility_period_ending_reminder, send_reminder_all_open_reviews,
2828
send_review_reminder_overdue_assignment, send_reminder_unconfirmed_assignments)
2929
from ietf.name.models import ReviewResultName, ReviewRequestStateName, ReviewAssignmentStateName, \
@@ -576,6 +576,7 @@ def test_change_reviewer_settings(self):
576576
"filter_re": "test-[regexp]",
577577
"remind_days_before_deadline": "6",
578578
"remind_days_open_reviews": "8",
579+
"request_assignment_next": "1",
579580
"expertise": "Some expertise",
580581
})
581582
self.assertEqual(r.status_code, 302)
@@ -591,6 +592,7 @@ def test_change_reviewer_settings(self):
591592
msg_content = outbox[0].get_payload(decode=True).decode("utf-8").lower()
592593
self.assertTrue("frequency changed", msg_content)
593594
self.assertTrue("skip next", msg_content)
595+
self.assertTrue("requested to be the next person", msg_content)
594596

595597
# Normal reviewer should not be able to change skip_next
596598
r = self.client.post(url, {
@@ -903,7 +905,8 @@ def test_rotation_queue_update(self):
903905
secretary = RoleFactory.create(group=group,name_id='secr')
904906
docs = [DocumentFactory.create(type_id='draft',group=None) for i in range(4)]
905907
requests = [ReviewRequestFactory(team=group,doc=docs[i]) for i in range(4)]
906-
rot_list = reviewer_rotation_list(group)
908+
policy = get_reviewer_queue_policy(group)
909+
rot_list = policy.default_reviewer_rotation_list()
907910

908911
expected_ending_head_of_rotation = rot_list[3]
909912

@@ -924,6 +927,6 @@ def test_rotation_queue_update(self):
924927
self.client.login(username=secretary.person.user.username,password=secretary.person.user.username+'+password')
925928
r = self.client.post(unassigned_url, postdict)
926929
self.assertEqual(r.status_code,302)
927-
self.assertEqual(expected_ending_head_of_rotation,reviewer_rotation_list(group)[0])
930+
self.assertEqual(expected_ending_head_of_rotation, policy.default_reviewer_rotation_list()[0])
928931
self.assertMailboxContains(outbox, subject='Last Call assignment', text='Requested by', count=4)
929932

‎ietf/group/views.py‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@
9292
from ietf.name.models import GroupTypeName, StreamName
9393
from ietf.person.models import Email
9494
from ietf.review.models import ReviewRequest, ReviewAssignment, ReviewerSettings, ReviewSecretarySettings
95+
from ietf.review.policies import get_reviewer_queue_policy
9596
from ietf.review.utils import (can_manage_review_requests_for_team,
9697
can_access_review_stats_for_team,
9798

@@ -103,7 +104,6 @@
103104
unavailable_periods_to_list,
104105
current_unavailable_periods_for_reviewers,
105106
email_reviewer_availability_change,
106-
reviewer_rotation_list,
107107
latest_review_assignments_for_reviewers,
108108
augment_review_requests_with_events,
109109
get_default_filter_re,
@@ -1394,7 +1394,7 @@ def reviewer_overview(request, acronym, group_type=None):
13941394

13951395
can_manage = can_manage_review_requests_for_team(request.user, group)
13961396

1397-
reviewers = reviewer_rotation_list(group)
1397+
reviewers = get_reviewer_queue_policy(group).default_reviewer_rotation_list(include_unavailable=True)
13981398

13991399
reviewer_settings = { s.person_id: s for s in ReviewerSettings.objects.filter(team=group) }
14001400
unavailable_periods = defaultdict(list)
@@ -1565,13 +1565,14 @@ def manage_review_requests(request, acronym, group_type=None, assignment_status=
15651565
# Make sure the any assignments to the person at the head
15661566
# of the rotation queue are processed first so that the queue
15671567
# rotates before any more assignments are processed
1568-
head_of_rotation = reviewer_rotation_list(group)[0]
1568+
reviewer_policy = get_reviewer_queue_policy(group)
1569+
head_of_rotation = reviewer_policy.default_reviewer_rotation_list_without_skipped()[0]
15691570
while head_of_rotation in assignments_by_person:
15701571
for review_req in assignments_by_person[head_of_rotation]:
15711572
assign_review_request_to_reviewer(request, review_req, review_req.form.cleaned_data["reviewer"],review_req.form.cleaned_data["add_skip"])
15721573
reqs_to_assign.remove(review_req)
15731574
del assignments_by_person[head_of_rotation]
1574-
head_of_rotation = reviewer_rotation_list(group)[0]
1575+
head_of_rotation = reviewer_policy.default_reviewer_rotation_list_without_skipped()[0]
15751576

15761577
for review_req in reqs_to_assign:
15771578
assign_review_request_to_reviewer(request, review_req, review_req.form.cleaned_data["reviewer"],review_req.form.cleaned_data["add_skip"])
@@ -1684,7 +1685,7 @@ def should_be_replicated_in_last_call_section(r):
16841685

16851686
partial_msg = render_to_string(template.path, {
16861687
"review_assignments": review_assignments,
1687-
"rotation_list": reviewer_rotation_list(group)[:10],
1688+
"rotation_list": get_reviewer_queue_policy(group).default_reviewer_rotation_list()[:10],
16881689
"group" : group,
16891690
})
16901691

@@ -1755,7 +1756,10 @@ def change_reviewer_settings(request, acronym, reviewer_email, group_type=None):
17551756
changes.append("Frequency changed to \"{}\" from \"{}\".".format(settings.get_min_interval_display() or "Not specified", prev_min_interval or "Not specified"))
17561757
if settings.skip_next != prev_skip_next:
17571758
changes.append("Skip next assignments changed to {} from {}.".format(settings.skip_next, prev_skip_next))
1758-
1759+
if settings.request_assignment_next:
1760+
changes.append("Reviewer has requested to be the next person selected for an "
1761+
"assignment, as soon as possible, and will be on the top of "
1762+
"the queue.")
17591763
if changes:
17601764
email_reviewer_availability_change(request, group, reviewer_role, "\n\n".join(changes), request.user.person)
17611765

0 commit comments

Comments
 (0)