altf4arnold/datatracker
1
0
Fork 0
Code Issues Pull requests Actions 3 Packages Projects Releases Wiki Activity

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

Browse source 
- Legacy-Id: 12670
This commit is contained in:
Robert Sparks 2017-01-19 14:23:09 +00:00
parent 49dcf67fd5
commit 8e007ce50b
6 changed files with 42 additions and 19 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

28
ietf/doc/tests_review.py
View file

@ -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()

4
ietf/doc/views_review.py
View file

@ -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:

8
ietf/group/tests_review.py
View file

@ -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):

3
ietf/group/views_review.py
View file

@ -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"])

10
ietf/review/utils.py
View file

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

8
ietf/templates/group/manage_review_requests.html
View file

@ -135,13 +135,17 @@
<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>