Make changing skip_next on a review assignment an explicit decision of the assigner. Commit ready for merge. Fixes ietf-tools#2148.

 - Legacy-Id: 12670

Author: rjsparks

ietf/doc/tests_review.py

@@ -24,6 +24,7 @@
 from ietf.utils.test_data import make_test_data, make_review_data, create_person
 from ietf.utils.test_utils import login_testing_unauthorized, unicontent, reload_db_objects
 from ietf.utils.mail import outbox, empty_outbox
+from ietf.person.factories import PersonFactory
 
 class ReviewTests(TestCase):
     def setUp(self):
@@ -180,27 +181,29 @@ def get_skip_next(person):
                         or ReviewerSettings(team=team))
             return settings.skip_next
 
-        possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[0].pk)
+        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)
+        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)
+        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]), 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)
+        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]), 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)
@@ -209,7 +212,7 @@ def get_skip_next(person):
         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]), 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)
@@ -220,13 +223,13 @@ def get_skip_next(person):
         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]), 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)
+        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)
@@ -283,6 +286,10 @@ def test_assign_reviewer(self):
         reviewer_settings.skip_next = 1
         reviewer_settings.save()
 
+        # Need one more person in review team one so we can test incrementing skip_count without immediately decrementing it
+        another_reviewer = PersonFactory.create()
+        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,
@@ -345,7 +352,7 @@ def test_assign_reviewer(self):
         review_req.state = ReviewRequestStateName.objects.get(slug="accepted")
         review_req.save()
         reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[1]).first()
-        r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk })
+        r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk, "add_skip": 1 })
         self.assertEqual(r.status_code, 302)
 
         review_req = reload_db_objects(review_req)
@@ -354,6 +361,7 @@ def test_assign_reviewer(self):
         self.assertEqual(len(outbox), 2)
         self.assertTrue("cancelled your assignment" in outbox[0].get_payload(decode=True).decode("utf-8"))
         self.assertTrue("assigned" in outbox[1].get_payload(decode=True).decode("utf-8"))
+        self.assertEqual(ReviewerSettings.objects.get(person=reviewer.person).skip_next, 1)
 
     def test_accept_reviewer_assignment(self):
         doc = make_test_data()

ietf/doc/views_review.py

@@ -253,6 +253,7 @@ def close_request(request, name, request_id):
 
 class AssignReviewerForm(forms.Form):
     reviewer = PersonEmailChoiceField(empty_label="(None)", required=False)
+    add_skip = forms.BooleanField(label='Skip next time', required=False)
 
     def __init__(self, review_req, *args, **kwargs):
         super(AssignReviewerForm, self).__init__(*args, **kwargs)
@@ -271,7 +272,8 @@ def assign_reviewer(request, name, request_id):
         form = AssignReviewerForm(review_req, request.POST)
         if form.is_valid():
             reviewer = form.cleaned_data["reviewer"]
-            assign_review_request_to_reviewer(request, review_req, reviewer)
+            add_skip = form.cleaned_data["add_skip"]
+            assign_review_request_to_reviewer(request, review_req, reviewer, add_skip)
 
             return redirect(review_request, name=review_req.doc.name, request_id=review_req.pk)
     else:

ietf/group/tests_review.py

@@ -20,6 +20,7 @@
 import ietf.group.views_review
 from ietf.utils.mail import outbox, empty_outbox
 from ietf.dbtemplate.factories import DBTemplateFactory
+from ietf.person.factories import PersonFactory
 
 class ReviewTests(TestCase):
     def test_review_requests(self):
@@ -205,6 +206,10 @@ def test_manage_review_requests(self):
             deadline=datetime.date.today() - datetime.timedelta(days=80),
             reviewer=review_req1.reviewer,
         )
+
+        # Need one more person in review team one so we can test incrementing skip_count without immediately decrementing it
+        another_reviewer = PersonFactory.create()
+        another_reviewer.role_set.create(name_id='reviewer', email=another_reviewer.email(), group=review_req1.team)
 
         # get
         r = self.client.get(assigned_url)
@@ -257,6 +262,7 @@ def test_manage_review_requests(self):
             "r{}-existing_reviewer".format(review_req2.pk): review_req2.reviewer_id or "",
             "r{}-action".format(review_req2.pk): "assign",
             "r{}-reviewer".format(review_req2.pk): new_reviewer.pk,
+            "r{}-add_skip".format(review_req2.pk) : 1,
 
             "action": "save",
         })
@@ -280,6 +286,8 @@ def test_manage_review_requests(self):
         self.assertEqual(review_req1.state_id, "no-response")
         self.assertEqual(review_req2.state_id, "requested")
         self.assertEqual(review_req2.reviewer, new_reviewer)
+        settings = ReviewerSettings.objects.filter(team=review_req2.team, person=new_reviewer.person).first()
+        self.assertEqual(settings.skip_next,1)
         self.assertEqual(review_req3.state_id, "requested")
 
     def test_email_open_review_assignments(self):

ietf/group/views_review.py

@@ -188,6 +188,7 @@ class ManageReviewRequestForm(forms.Form):
     action = forms.ChoiceField(choices=ACTIONS, widget=forms.HiddenInput, required=False)
     close = forms.ModelChoiceField(queryset=close_review_request_states(), required=False)
     reviewer = PersonEmailChoiceField(empty_label="(None)", required=False, label_with="person")
+    add_skip = forms.BooleanField(required=False)
 
     def __init__(self, review_req, *args, **kwargs):
         if not "prefix" in kwargs:
@@ -307,7 +308,7 @@ def manage_review_requests(request, acronym, group_type=None, assignment_status=
             for review_req in review_requests:
                 action = review_req.form.cleaned_data.get("action")
                 if action == "assign":
-                    assign_review_request_to_reviewer(request, review_req, review_req.form.cleaned_data["reviewer"])
+                    assign_review_request_to_reviewer(request, review_req, review_req.form.cleaned_data["reviewer"],review_req.form.cleaned_data["add_skip"])
                 elif action == "close":
                     close_review_request(request, review_req, review_req.form.cleaned_data["close"])
 

ietf/review/utils.py

@@ -417,7 +417,7 @@ def extract_email_addresses(objs):
         "by": by,
     })
 
-def assign_review_request_to_reviewer(request, review_req, reviewer):
+def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=False):
     assert review_req.state_id in ("requested", "accepted")
 
     if reviewer == review_req.reviewer:
@@ -435,7 +435,7 @@ def assign_review_request_to_reviewer(request, review_req, reviewer):
     review_req.save()
 
     if review_req.reviewer:
-        possibly_advance_next_reviewer_for_team(review_req.team, review_req.reviewer.person_id)
+        possibly_advance_next_reviewer_for_team(review_req.team, review_req.reviewer.person_id, add_skip)
 
     ReviewRequestDocEvent.objects.create(
         type="assigned_review_request",
@@ -456,7 +456,7 @@ def assign_review_request_to_reviewer(request, review_req, reviewer):
         "%s has assigned you as a reviewer for this document." % request.user.person,
         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):
+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])
@@ -476,7 +476,8 @@ def reviewer_settings_for(person_id):
     if assigned_review_to_person_id == reviewer_at_index(current_i):
         # move 1 ahead
         current_i += 1
-    else:
+
+    if add_skip:
         settings = reviewer_settings_for(assigned_review_to_person_id)
         settings.skip_next += 1
         settings.save()
@@ -491,7 +492,6 @@ def reviewer_settings_for(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)

ietf/templates/group/manage_review_requests.html

@@ -135,13 +135,17 @@ <h3 class="panel-title">
 
             <span class="reviewer-controls form-inline">
               <label for="{{ r.form.reviewer.id_for_label }}">Assign:</label>
-              {{ r.form.reviewer }}
+              {{ r.form.reviewer }} 
+              {{ r.form.add_skip }} <label for="{{ r.form.add_skip.id_for_label }}">Skip next time</label>
               <button type="button" class="btn btn-default undo" title="Cancel assignment" data-initial="{{ r.form.fields.reviewer.initial|default:"" }}">Cancel</button>
-              {% if r.form.reviewer.errors %}
+              {% if r.form.reviewer.errors or r.form.add_skip.errors %}
                 <div class="alert alert-danger">
                   {% for e in r.form.reviewer.errors %}
                     {{ e }}
                   {% endfor %}
+                  {% for e in r.form.add_skip.errors %}
+                    {{ e }}
+                  {% endfor %}
                 </div>
               {% endif %}
             </span>