- Make skipping unavailable reviews the default, except for the

  reviewer_overview page.
- Make default_reviewer_rotation_list use a consistent return type
 - Legacy-Id: 16986

Author: Sasha Romijn

ietf/doc/tests_review.py

@@ -280,13 +280,6 @@ def test_assign_reviewer(self):
         another_reviewer = PersonFactory.create(name = "Extra TestReviewer") # needs to be lexically greater than the existing one
         another_reviewer.role_set.create(name_id='reviewer', email=another_reviewer.email(), group=review_req.team)
 
-        UnavailablePeriod.objects.create(
-            team=review_req.team,
-            person=reviewer_email.person,
-            start_date=datetime.date.today() - datetime.timedelta(days=10),
-            availability="unavailable",
-        )
-
         ReviewWish.objects.create(person=reviewer_email.person, team=review_req.team, doc=doc)
 
         # pick a non-existing reviewer as next to see that we can
@@ -318,7 +311,6 @@ def test_assign_reviewer(self):
         self.assertIn("wishes to review", reviewer_label)
         self.assertIn("is author", reviewer_label)
         self.assertIn("regexp matches", reviewer_label)
-        self.assertIn("unavailable indefinitely", reviewer_label)
         self.assertIn("skip next 1", reviewer_label)
         self.assertIn("#1", reviewer_label)
         self.assertIn("1 fully completed", reviewer_label)

ietf/group/views.py

@@ -1391,7 +1391,7 @@ def reviewer_overview(request, acronym, group_type=None):
 
     can_manage = can_manage_review_requests_for_team(request.user, group)
 
-    reviewers = get_reviewer_queue_policy(group).default_reviewer_rotation_list()
+    reviewers = get_reviewer_queue_policy(group).default_reviewer_rotation_list(include_unavailable=True)
 
     reviewer_settings = { s.person_id: s for s in ReviewerSettings.objects.filter(team=group) }
     unavailable_periods = defaultdict(list)

ietf/review/policies.py

@@ -32,10 +32,9 @@ class AbstractReviewerQueuePolicy:
     def __init__(self, team):
         self.team = team
 
-    def default_reviewer_rotation_list(self, skip_unavailable=False, dont_skip=[]):
+    def default_reviewer_rotation_list(self, dont_skip=[]):
         """
         Return a list of reviewers in the default reviewer rotation for a policy.
-        TODO: fix return types
         """
         raise NotImplementedError
 
@@ -283,8 +282,8 @@ class RotateWithSkipReviewerQueuePolicy(AbstractReviewerQueuePolicy):
     def update_policy_state_for_assignment(self, assignee_person_id, add_skip=False):
         assert assignee_person_id is not None
 
-        rotation_list = self.default_reviewer_rotation_list(skip_unavailable=True,
-                                                            dont_skip=[assignee_person_id])
+        rotation_list = [p.id for p in self.default_reviewer_rotation_list(
+                                                            dont_skip=[assignee_person_id])]
 
         def reviewer_at_index(i):
             if not rotation_list:
@@ -324,7 +323,7 @@ def reviewer_settings_for(person_id):
 
                 break
 
-    def default_reviewer_rotation_list(self, skip_unavailable=False, dont_skip=[]):
+    def default_reviewer_rotation_list(self, include_unavailable=False, dont_skip=[]):
         reviewers = list(Person.objects.filter(role__name="reviewer", role__group=self.team))
         reviewers.sort(key=lambda p: p.last_name())
         next_reviewer_index = 0
@@ -347,10 +346,9 @@ def default_reviewer_rotation_list(self, skip_unavailable=False, dont_skip=[]):
 
         rotation_list = reviewers[next_reviewer_index:] + reviewers[:next_reviewer_index]
 
-        reviewers_to_skip = []            
-        if skip_unavailable:
+        if not include_unavailable:
             reviewers_to_skip = self._unavailable_reviewers(dont_skip)
-            rotation_list = [p.pk for p in rotation_list if p.pk not in reviewers_to_skip]
-    
+            rotation_list = [p for p in rotation_list if p.pk not in reviewers_to_skip]
+        
         return rotation_list
 

ietf/review/test_policies.py

@@ -22,35 +22,33 @@ def test_default_reviewer_rotation_list(self):
             create_person(team, "reviewer", name="Test Reviewer{}".format(i), username="testreviewer{}".format(i))
             for i in range(5)
         ]
-        reviewers_pks = [r.pk for r in reviewers]
 
         # This reviewer should never be included.
         unavailable_reviewer = create_person(team, "reviewer", name="unavailable reviewer", username="unavailablereviewer")
         UnavailablePeriod.objects.create(
             team=team,
             person=unavailable_reviewer,
             start_date='2000-01-01',
-            end_date='3000-01-01',
             availability=UnavailablePeriod.AVAILABILITY_CHOICES[0],
         )
 
         # Default policy without a NextReviewerInTeam
-        rotation = policy.default_reviewer_rotation_list(skip_unavailable=True)
-        self.assertNotIn(unavailable_reviewer.pk, rotation)
-        self.assertEqual(rotation, reviewers_pks)
+        rotation = policy.default_reviewer_rotation_list()
+        self.assertNotIn(unavailable_reviewer, rotation)
+        self.assertEqual(rotation, reviewers)
 
         # Policy with a current NextReviewerInTeam
         NextReviewerInTeam.objects.create(team=team, next_reviewer=reviewers[3])
-        rotation = policy.default_reviewer_rotation_list(skip_unavailable=True)
-        self.assertNotIn(unavailable_reviewer.pk, rotation)
-        self.assertEqual(rotation, reviewers_pks[3:] + reviewers_pks[:3])
+        rotation = policy.default_reviewer_rotation_list()
+        self.assertNotIn(unavailable_reviewer, rotation)
+        self.assertEqual(rotation, reviewers[3:] + reviewers[:3])
 
         # Policy with a NextReviewerInTeam that has left the team.
         Role.objects.get(person=reviewers[1]).delete()
         NextReviewerInTeam.objects.filter(team=team).update(next_reviewer=reviewers[1])
-        rotation = policy.default_reviewer_rotation_list(skip_unavailable=True)
-        self.assertNotIn(unavailable_reviewer.pk, rotation)
-        self.assertEqual(rotation, reviewers_pks[2:] + reviewers_pks[:1])
+        rotation = policy.default_reviewer_rotation_list()
+        self.assertNotIn(unavailable_reviewer, rotation)
+        self.assertEqual(rotation, reviewers[2:] + reviewers[:1])
 
     def test_update_policy_state_for_assignment(self):