From b5a31c3c6a4ab939797a21c39c6663a7ea7e5a9f Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 11 Nov 2019 12:31:16 +0000 Subject: [PATCH] Update some terminology and docstrings. - Legacy-Id: 16983 --- ietf/doc/tests_review.py | 4 +-- ietf/doc/views_review.py | 4 +-- ietf/group/forms.py | 4 +-- ietf/group/tests_review.py | 4 +-- ietf/group/views.py | 8 +++--- ietf/review/policies.py | 51 +++++++++++++++++++++++++++++------- ietf/review/test_policies.py | 4 +-- ietf/review/utils.py | 4 +-- 8 files changed, 58 insertions(+), 25 deletions(-) diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index e6d49f9e7..221dadfd3 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -33,7 +33,7 @@ from ietf.person.models import Email, Person from ietf.review.factories import ReviewRequestFactory, ReviewAssignmentFactory from ietf.review.models import (ReviewRequest, ReviewerSettings, ReviewWish, UnavailablePeriod, NextReviewerInTeam) -from ietf.review.policies import policy_for_team +from ietf.review.policies import get_reviewer_queue_policy from ietf.utils.test_utils import TestCase from ietf.utils.test_utils import login_testing_unauthorized, reload_db_objects @@ -325,7 +325,7 @@ class ReviewTests(TestCase): # assign empty_outbox() - rotation_list = policy_for_team(review_req.team).default_reviewer_rotation_list() + rotation_list = get_reviewer_queue_policy(review_req.team).default_reviewer_rotation_list() reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[0]).first() r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk }) self.assertEqual(r.status_code, 302) diff --git a/ietf/doc/views_review.py b/ietf/doc/views_review.py index abe15dd44..cb7f28499 100644 --- a/ietf/doc/views_review.py +++ b/ietf/doc/views_review.py @@ -33,7 +33,7 @@ from ietf.ietfauth.utils import is_authorized_in_doc_stream, user_is_person, has from ietf.message.models import Message from ietf.message.utils import infer_message from ietf.person.fields import PersonEmailChoiceField, SearchablePersonField -from ietf.review.policies import policy_for_team +from ietf.review.policies import get_reviewer_queue_policy from ietf.review.utils import (active_review_teams, assign_review_request_to_reviewer, can_request_review_of_doc, can_manage_review_requests_for_team, email_review_assignment_change, email_review_request_change, @@ -294,7 +294,7 @@ class AssignReviewerForm(forms.Form): def __init__(self, review_req, *args, **kwargs): super(AssignReviewerForm, self).__init__(*args, **kwargs) - policy_for_team(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req) + get_reviewer_queue_policy(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req) @login_required diff --git a/ietf/group/forms.py b/ietf/group/forms.py index 564ddf716..4ffc9c4b6 100644 --- a/ietf/group/forms.py +++ b/ietf/group/forms.py @@ -19,7 +19,7 @@ from ietf.group.models import Group, GroupHistory, GroupStateName from ietf.person.fields import SearchableEmailsField, PersonEmailChoiceField from ietf.person.models import Person from ietf.review.models import ReviewerSettings, UnavailablePeriod, ReviewSecretarySettings -from ietf.review.policies import policy_for_team +from ietf.review.policies import get_reviewer_queue_policy from ietf.review.utils import close_review_request_states from ietf.utils.textupload import get_cleaned_text_file_content from ietf.utils.text import strip_suffix @@ -255,7 +255,7 @@ class ManageReviewRequestForm(forms.Form): self.fields["close"].widget.attrs["class"] = "form-control input-sm" - policy_for_team(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req) + get_reviewer_queue_policy(review_req.team).setup_reviewer_field(self.fields["reviewer"], review_req) self.fields["reviewer"].widget.attrs["class"] = "form-control input-sm" if self.is_bound: diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py index 56a05fa5e..63d2e969c 100644 --- a/ietf/group/tests_review.py +++ b/ietf/group/tests_review.py @@ -12,7 +12,7 @@ from pyquery import PyQuery from django.urls import reverse as urlreverse -from ietf.review.policies import policy_for_team +from ietf.review.policies import get_reviewer_queue_policy from ietf.utils.test_utils import login_testing_unauthorized, TestCase, reload_db_objects from ietf.doc.models import TelechatDocEvent from ietf.group.models import Role @@ -655,7 +655,7 @@ class BulkAssignmentTests(TestCase): secretary = RoleFactory.create(group=group,name_id='secr') docs = [DocumentFactory.create(type_id='draft',group=None) for i in range(4)] requests = [ReviewRequestFactory(team=group,doc=docs[i]) for i in range(4)] - policy = policy_for_team(group) + policy = get_reviewer_queue_policy(group) rot_list = policy.default_reviewer_rotation_list() expected_ending_head_of_rotation = rot_list[3] diff --git a/ietf/group/views.py b/ietf/group/views.py index 83d86a446..6e315db00 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -92,7 +92,7 @@ from ietf.meeting.utils import group_sessions from ietf.name.models import GroupTypeName, StreamName from ietf.person.models import Email from ietf.review.models import ReviewRequest, ReviewAssignment, ReviewerSettings, ReviewSecretarySettings -from ietf.review.policies import policy_for_team +from ietf.review.policies import get_reviewer_queue_policy from ietf.review.utils import (can_manage_review_requests_for_team, can_access_review_stats_for_team, @@ -1391,7 +1391,7 @@ def reviewer_overview(request, acronym, group_type=None): can_manage = can_manage_review_requests_for_team(request.user, group) - reviewers = policy_for_team(group).default_reviewer_rotation_list() + reviewers = get_reviewer_queue_policy(group).default_reviewer_rotation_list() reviewer_settings = { s.person_id: s for s in ReviewerSettings.objects.filter(team=group) } unavailable_periods = defaultdict(list) @@ -1541,7 +1541,7 @@ def manage_review_requests(request, acronym, group_type=None, assignment_status= # Make sure the any assignments to the person at the head # of the rotation queue are processed first so that the queue # rotates before any more assignments are processed - reviewer_policy = policy_for_team(group) + reviewer_policy = get_reviewer_queue_policy(group) head_of_rotation = reviewer_policy.default_reviewer_rotation_list()[0] while head_of_rotation in assignments_by_person: for review_req in assignments_by_person[head_of_rotation]: @@ -1661,7 +1661,7 @@ def email_open_review_assignments(request, acronym, group_type=None): partial_msg = render_to_string(template.path, { "review_assignments": review_assignments, - "rotation_list": policy_for_team(group).default_reviewer_rotation_list()[:10], + "rotation_list": get_reviewer_queue_policy(group).default_reviewer_rotation_list()[:10], "group" : group, }) diff --git a/ietf/review/policies.py b/ietf/review/policies.py index 6c698d600..84fd41353 100644 --- a/ietf/review/policies.py +++ b/ietf/review/policies.py @@ -16,12 +16,19 @@ from ietf.review.utils import (current_unavailable_periods_for_reviewers, get_default_filter_re, latest_review_assignments_for_reviewers) - -def policy_for_team(team): - return RotateWithSkipReviewerPolicy(team) +""" +This file contains policies regarding reviewer queues. +The policies are documented in more detail on: +https://trac.tools.ietf.org/tools/ietfdb/wiki/ReviewerQueuePolicy +Terminology used here should match terminology used in that document. +""" -class AbstractReviewerPolicy: +def get_reviewer_queue_policy(team): + return RotateWithSkipReviewerQueuePolicy(team) + + +class AbstractReviewerQueuePolicy: def __init__(self, team): self.team = team @@ -32,6 +39,12 @@ class AbstractReviewerPolicy: """ raise NotImplementedError + def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False): + """ + Update the internal state of a policy to reflect an assignment. + """ + raise NotImplementedError + # TODO : Change this field to deal with multiple already assigned reviewers??? def setup_reviewer_field(self, field, review_req): """ @@ -57,7 +70,7 @@ class AbstractReviewerPolicy: returning Email objects. """ if review_req.team != self.team: - raise ValueError('Reviewer policy was passed review request belonging to different team.') + raise ValueError('Reviewer queue policy was passed review request belonging to different team.') resolver = AssignmentOrderResolver(email_queryset, review_req, self.default_reviewer_rotation_list()) return resolver.determine_ranking() @@ -90,7 +103,7 @@ class AssignmentOrderResolver: self._collect_context() def _collect_context(self): - # Collect all relevant data about this team, document and review request. + """Collect all relevant data about this team, document and review request.""" self.doc_aliases = DocAlias.objects.filter(docs=self.doc).values_list("name", flat=True) @@ -109,6 +122,10 @@ class AssignmentOrderResolver: self.assignment_data_for_reviewers = latest_review_assignments_for_reviewers(self.team) def determine_ranking(self): + """ + Determine the ranking of reviewers. + Returns a list of tuples, each tuple containing an Email pk and an explanation label. + """ ranking = [] for e in self.possible_emails: ranking.append(self._ranking_for_email(e)) @@ -118,6 +135,12 @@ class AssignmentOrderResolver: return [(r["email"].pk, r["label"]) for r in ranking] def _ranking_for_email(self, email): + """ + Determine the ranking for a specific Email. + Returns a dict with an email object, the scores and an explanation label. + The scores are a list of individual scores, i.e. they are prioritised, not + cumulative. + """ settings = self.reviewer_settings.get(email.person_id) scores = [] explanations = [] @@ -183,6 +206,7 @@ class AssignmentOrderResolver: } def _collect_reviewer_stats(self, email): + """Collect statistics on past reviews for a particular Email.""" stats = [] assignment_data = self.assignment_data_for_reviewers.get(email.person_id, []) currently_open = sum(1 for d in assignment_data if d.state in ["assigned", "accepted"]) @@ -206,8 +230,13 @@ class AssignmentOrderResolver: return stats def _connections_with_doc(self, doc, person_ids): + """ + Collect any connections any Person in person_ids has with a document. + Returns a dict containing Person IDs that have a connection as keys, + values being an explanation string, + """ connections = {} - # examine the closest connections last to let them override + # examine the closest connections last to let them override the label connections[doc.ad_id] = "is associated Area Director" for r in Role.objects.filter(group=doc.group_id, person__in=person_ids).select_related("name"): @@ -221,6 +250,10 @@ class AssignmentOrderResolver: return connections def _persons_with_previous_review(self, review_req, possible_person_ids): + """ + Collect anyone in possible_person_ids that have reviewed the request before. + Returns a set with Person IDs of anyone who has. + """ has_reviewed_previous = ReviewRequest.objects.filter( doc=review_req.doc, reviewassignment__reviewer__person__in=possible_person_ids, @@ -232,7 +265,7 @@ class AssignmentOrderResolver: has_reviewed_previous = set( has_reviewed_previous.values_list("reviewassignment__reviewer__person", flat=True)) return has_reviewed_previous - + def _reviewer_settings_for_person_ids(self, person_ids): reviewer_settings = { r.person_id: r @@ -245,7 +278,7 @@ class AssignmentOrderResolver: return reviewer_settings -class RotateWithSkipReviewerPolicy(AbstractReviewerPolicy): +class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy): def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False): assert assignee_person_id is not None diff --git a/ietf/review/test_policies.py b/ietf/review/test_policies.py index 8624e6769..26637195a 100644 --- a/ietf/review/test_policies.py +++ b/ietf/review/test_policies.py @@ -8,7 +8,7 @@ from ietf.group.models import Group from ietf.review.factories import ReviewAssignmentFactory from ietf.review.models import ReviewerSettings, NextReviewerInTeam, UnavailablePeriod, \ ReviewRequest -from ietf.review.policies import policy_for_team +from ietf.review.policies import get_reviewer_queue_policy from ietf.utils.test_data import create_person from ietf.utils.test_utils import TestCase @@ -17,7 +17,7 @@ class RotateWithSkipReviewerPolicyTests(TestCase): def test_possibly_advance_next_reviewer_for_team(self): team = ReviewTeamFactory(acronym="rotationteam", name="Review Team", list_email="rotationteam@ietf.org", parent=Group.objects.get(acronym="farfut")) - policy = policy_for_team(team) + policy = get_reviewer_queue_policy(team) doc = WgDraftFactory() # make a bunch of reviewers diff --git a/ietf/review/utils.py b/ietf/review/utils.py index 7f7aef568..a58c43583 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -380,8 +380,8 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa assignment = review_req.reviewassignment_set.create(state_id='assigned', reviewer = reviewer, assigned_on = datetime.datetime.now()) - from ietf.review.policies import policy_for_team - policy_for_team(review_req.team).update_policy_state_for_assignment(reviewer.person_id, add_skip) + from ietf.review.policies import get_reviewer_queue_policy + get_reviewer_queue_policy(review_req.team).update_policy_state_for_assignment(reviewer.person_id, add_skip) ReviewRequestDocEvent.objects.create( type="assigned_review_request",