diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index f477f3323..f77d1e070 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -24,6 +24,7 @@ from ietf.utils.test_utils import TestCase 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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() diff --git a/ietf/doc/views_review.py b/ietf/doc/views_review.py index 7e9aad465..ae56232f4 100644 --- a/ietf/doc/views_review.py +++ b/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: diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py index 486b421c7..8ef0b3007 100644 --- a/ietf/group/tests_review.py +++ b/ietf/group/tests_review.py @@ -20,6 +20,7 @@ from ietf.name.models import ReviewTypeName, ReviewResultName, ReviewRequestStat 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): "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 @@ class ReviewTests(TestCase): 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): diff --git a/ietf/group/views_review.py b/ietf/group/views_review.py index 41791cb8f..ec319c1a0 100644 --- a/ietf/group/views_review.py +++ b/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"]) diff --git a/ietf/review/utils.py b/ietf/review/utils.py index ae7124fa8..c2772134e 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -417,7 +417,7 @@ def email_reviewer_availability_change(request, team, reviewer_role, msg, by): "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 possibly_advance_next_reviewer_for_team(team, assigned_review_to_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 possibly_advance_next_reviewer_for_team(team, assigned_review_to_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) diff --git a/ietf/templates/group/manage_review_requests.html b/ietf/templates/group/manage_review_requests.html index d52c0f6cf..4991d9d1e 100644 --- a/ietf/templates/group/manage_review_requests.html +++ b/ietf/templates/group/manage_review_requests.html @@ -135,13 +135,17 @@ - {{ r.form.reviewer }} + {{ r.form.reviewer }} + {{ r.form.add_skip }} - {% if r.form.reviewer.errors %} + {% if r.form.reviewer.errors or r.form.add_skip.errors %}
{% for e in r.form.reviewer.errors %} {{ e }} {% endfor %} + {% for e in r.form.add_skip.errors %} + {{ e }} + {% endfor %}
{% endif %}