Add support for assigning a reviewer to a review request, still some

corners missing. Fix a couple of other issues.
 - Legacy-Id: 11227
This commit is contained in:
Ole Laursen 2016-05-23 16:42:49 +00:00
parent b5ef179a6e
commit 5dd079e2f8
11 changed files with 246 additions and 107 deletions

View file

@ -13,6 +13,8 @@ from ietf.name.models import ReviewResultName, ReviewRequestStateName
from ietf.utils.test_utils import TestCase
from ietf.utils.test_data import make_test_data
from ietf.utils.test_utils import login_testing_unauthorized, unicontent, reload_db_objects
from ietf.utils.mail import outbox, empty_outbox
def make_review_data(doc):
team = Group.objects.create(state_id="active", acronym="reviewteam", name="Review Team", type_id="team")
@ -20,7 +22,7 @@ def make_review_data(doc):
p = Person.objects.get(user__username="plain")
role = Role.objects.create(name_id="reviewer", person=p, email=p.email_set.first(), group=team)
reviewer = Reviewer.objects.create(role=role, frequency=14, skip_next=0)
Reviewer.objects.create(team=team, person=p, frequency=14, skip_next=0)
review_req = ReviewRequest.objects.create(
doc=doc,
@ -28,10 +30,13 @@ def make_review_data(doc):
type_id="early",
deadline=datetime.datetime.now() + datetime.timedelta(days=20),
state_id="ready",
reviewer=reviewer,
reviewer=role,
reviewed_rev="01",
)
p = Person.objects.get(user__username="marschairman")
role = Role.objects.create(name_id="reviewer", person=p, email=p.email_set.first(), group=team)
return review_req
class ReviewTests(TestCase):
@ -65,29 +70,6 @@ class ReviewTests(TestCase):
self.assertEqual(req.requested_rev, "01")
self.assertEqual(doc.latest_event().type, "requested_review")
def test_request_review_by_reviewer(self):
doc = make_test_data()
review_req = make_review_data(doc)
review_team = review_req.team
url = urlreverse('ietf.doc.views_review.request_review', kwargs={ "name": doc.name })
login_testing_unauthorized(self, "plain", url)
# post request
deadline_date = datetime.date.today() + datetime.timedelta(days=10)
r = self.client.post(url, {
"type": "early",
"team": review_team.pk,
"deadline_date": deadline_date.isoformat(),
"requested_rev": "01"
})
self.assertEqual(r.status_code, 302)
req = ReviewRequest.objects.get(doc=doc, state="requested")
self.assertEqual(req.state_id, "requested")
self.assertEqual(req.team, review_team)
def test_doc_page(self):
# FIXME: fill in
pass
@ -134,13 +116,56 @@ class ReviewTests(TestCase):
self.assertEqual(e.type, "changed_review_request")
self.assertTrue("Withdrew" in e.desc)
def test_reject_request_assignment(self):
def test_assign_reviewer(self):
doc = make_test_data()
review_req = make_review_data(doc)
review_req.state = ReviewRequestStateName.objects.get(slug="requested")
review_req.reviewer = None
review_req.save()
assign_url = urlreverse('ietf.doc.views_review.assign_reviewer', kwargs={ "name": doc.name, "request_id": review_req.pk })
# follow link
req_url = urlreverse('ietf.doc.views_review.review_request', kwargs={ "name": doc.name, "request_id": review_req.pk })
self.client.login(username="secretary", password="secretary+password")
r = self.client.get(req_url)
self.assertEqual(r.status_code, 200)
self.assertTrue(assign_url in unicontent(r))
self.client.logout()
# get assign page
login_testing_unauthorized(self, "secretary", assign_url)
r = self.client.get(assign_url)
self.assertEqual(r.status_code, 200)
# assign
reviewer = Role.objects.filter(name="reviewer", group=review_req.team).first()
r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk })
self.assertEqual(r.status_code, 302)
review_req = reload_db_objects(review_req)
self.assertEqual(review_req.state_id, "requested")
self.assertEqual(review_req.reviewer, reviewer)
# re-assign
review_req.state = ReviewRequestStateName.objects.get(slug="accepted")
review_req.save()
reviewer = Role.objects.filter(name="reviewer", group=review_req.team).exclude(pk=reviewer.pk).first()
r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk })
self.assertEqual(r.status_code, 302)
review_req = reload_db_objects(review_req)
self.assertEqual(review_req.state_id, "requested")
self.assertEqual(review_req.reviewer, reviewer)
def test_reject_reviewer_assignment(self):
doc = make_test_data()
review_req = make_review_data(doc)
review_req.state = ReviewRequestStateName.objects.get(slug="accepted")
review_req.save()
reject_url = urlreverse('ietf.doc.views_review.reject_request_assignment', kwargs={ "name": doc.name, "request_id": review_req.pk })
reject_url = urlreverse('ietf.doc.views_review.reject_reviewer_assignment', kwargs={ "name": doc.name, "request_id": review_req.pk })
# follow link
@ -155,16 +180,18 @@ class ReviewTests(TestCase):
login_testing_unauthorized(self, "secretary", reject_url)
r = self.client.get(reject_url)
self.assertEqual(r.status_code, 200)
self.assertTrue(unicode(review_req.reviewer.role.person) in unicontent(r))
self.assertTrue(unicode(review_req.reviewer.person) in unicontent(r))
# reject
r = self.client.post(reject_url, { "action": "reject" })
empty_outbox()
r = self.client.post(reject_url, { "action": "reject", "message_to_secretary": "Test message" })
self.assertEqual(r.status_code, 302)
review_req = reload_db_objects(review_req)
self.assertEqual(review_req.state_id, "rejected")
e = doc.latest_event()
self.assertEqual(e.type, "changed_review_request")
self.assertTrue("unassigned" in e.desc)
self.assertEqual(ReviewRequest.objects.filter(doc=review_req.doc, team=review_req.team, state="accepted").count(), 1)
self.assertTrue("rejected" in e.desc)
self.assertEqual(ReviewRequest.objects.filter(doc=review_req.doc, team=review_req.team, state="requested").count(), 1)
self.assertEqual(len(outbox), 1)

View file

@ -5,6 +5,7 @@ urlpatterns = patterns('',
url(r'^$', views_review.request_review),
url(r'^(?P<request_id>[0-9]+)/$', views_review.review_request),
url(r'^(?P<request_id>[0-9]+)/withdraw/$', views_review.withdraw_request),
url(r'^(?P<request_id>[0-9]+)/rejectassignment/$', views_review.reject_request_assignment),
url(r'^(?P<request_id>[0-9]+)/assignreviewer/$', views_review.assign_reviewer),
url(r'^(?P<request_id>[0-9]+)/rejectreviewerassignment/$', views_review.reject_reviewer_assignment),
)

View file

@ -89,22 +89,6 @@ def can_adopt_draft(user, doc):
group__state="active",
person__user=user).exists())
def can_request_review_of_doc(user, doc):
if not user.is_authenticated():
return False
from ietf.review.utils import active_review_teams
if Role.objects.filter(name="reviewer", person__user=user, group__in=active_review_teams()):
return True
return is_authorized_in_doc_stream(user, doc)
def can_manage_review_requests_for_team(user, team):
if not user.is_authenticated():
return False
return Role.objects.filter(name="secretary", person__user=user, group=team).exists() or has_role(user, "Secretariat")
def two_thirds_rule( recused=0 ):
# For standards-track, need positions from 2/3 of the non-recused current IESG.
active = Role.objects.filter(name="ad",group__type="area",group__state="active").count()

View file

@ -48,8 +48,7 @@ from ietf.doc.models import ( Document, DocAlias, DocHistory, DocEvent, BallotDo
from ietf.doc.utils import ( add_links_in_new_revision_events, augment_events_with_revision,
can_adopt_draft, get_chartering_type, get_document_content, get_tags_for_stream_id,
needed_ballot_positions, nice_consensus, prettify_std_name, update_telechat, has_same_ballot,
get_initial_notify, make_notify_changed_event, crawl_history, default_consensus,
can_request_review_of_doc )
get_initial_notify, make_notify_changed_event, crawl_history, default_consensus )
from ietf.community.utils import augment_docs_with_tracking_info
from ietf.group.models import Role
from ietf.group.utils import can_manage_group, can_manage_materials
@ -63,6 +62,7 @@ from ietf.mailtrigger.utils import gather_relevant_expansions
from ietf.meeting.models import Session
from ietf.meeting.utils import group_sessions, get_upcoming_manageable_sessions, sort_sessions
from ietf.review.models import ReviewRequest
from ietf.review.utils import can_request_review_of_doc
def render_document_top(request, doc, tab, name):
tabs = []

View file

@ -6,10 +6,12 @@ from django import forms
from django.contrib.auth.decorators import login_required
from ietf.doc.models import Document, NewRevisionDocEvent, DocEvent
from ietf.doc.utils import can_request_review_of_doc, can_manage_review_requests_for_team
from ietf.ietfauth.utils import is_authorized_in_doc_stream, user_is_person
from ietf.review.models import ReviewRequest, ReviewRequestStateName
from ietf.review.utils import active_review_teams
from ietf.name.models import ReviewRequestStateName
from ietf.group.models import Role
from ietf.review.models import ReviewRequest
from ietf.review.utils import active_review_teams, assign_review_request_to_reviewer
from ietf.review.utils import can_request_review_of_doc, can_manage_review_requests_for_team
from ietf.utils.fields import DatepickerDateField
class RequestReviewForm(forms.ModelForm):
@ -89,7 +91,6 @@ def request_review(request, name):
time=review_req.time,
)
# FIXME: if I'm a reviewer, auto-assign to myself?
return redirect('doc_view', name=doc.name)
else:
@ -104,21 +105,25 @@ def review_request(request, name, request_id):
doc = get_object_or_404(Document, name=name)
review_req = get_object_or_404(ReviewRequest, pk=request_id)
is_reviewer = review_req.reviewer and user_is_person(request.user, review_req.reviewer.role.person)
is_reviewer = review_req.reviewer and user_is_person(request.user, review_req.reviewer.person)
can_manage_req = can_manage_review_requests_for_team(request.user, review_req.team)
can_withdraw_request = (review_req.state_id in ["requested", "accepted"]
and is_authorized_in_doc_stream(request.user, doc))
can_reject_request_assignment = (review_req.state_id in ["requested", "accepted"]
and review_req.reviewer_id is not None
and (is_reviewer or can_manage_req))
can_assign_reviewer = (review_req.state_id in ["requested", "accepted"]
and is_authorized_in_doc_stream(request.user, doc))
can_reject_reviewer_assignment = (review_req.state_id in ["requested", "accepted"]
and review_req.reviewer_id is not None
and (is_reviewer or can_manage_req))
return render(request, 'doc/review/review_request.html', {
'doc': doc,
'review_req': review_req,
'can_withdraw_request': can_withdraw_request,
'can_reject_request_assignment': can_reject_request_assignment,
'can_reject_reviewer_assignment': can_reject_reviewer_assignment,
'can_assign_reviewer': can_assign_reviewer,
})
def withdraw_request(request, name, request_id):
@ -150,52 +155,96 @@ def withdraw_request(request, name, request_id):
'review_req': review_req,
})
class RejectRequestAssignmentForm(forms.Form):
class PersonEmailLabeledRoleModelChoiceField(forms.ModelChoiceField):
def __init__(self, *args, **kwargs):
if not "queryset" in kwargs:
kwargs["queryset"] = Role.objects.select_related("person", "email")
super(PersonEmailLabeledRoleModelChoiceField, self).__init__(*args, **kwargs)
def label_from_instance(self, role):
return u"{} <{}>".format(role.person.name, role.email.address)
class AssignReviewerForm(forms.Form):
reviewer = PersonEmailLabeledRoleModelChoiceField(widget=forms.RadioSelect, empty_label="(None)", required=False)
def __init__(self, review_req, *args, **kwargs):
super(AssignReviewerForm, self).__init__(*args, **kwargs)
f = self.fields["reviewer"]
f.queryset = f.queryset.filter(name="reviewer", group=review_req.team)
if review_req.reviewer:
f.initial = review_req.reviewer_id
def assign_reviewer(request, name, request_id):
doc = get_object_or_404(Document, name=name)
review_req = get_object_or_404(ReviewRequest, pk=request_id, state__in=["requested", "accepted"])
can_manage_req = can_manage_review_requests_for_team(request.user, review_req.team)
if not can_manage_req:
return HttpResponseForbidden("You do not have permission to perform this action")
if request.method == "POST" and request.POST.get("action") == "assign":
form = AssignReviewerForm(review_req, request.POST)
if form.is_valid():
reviewer = form.cleaned_data["reviewer"]
assign_review_request_to_reviewer(review_req, reviewer, request.user.person)
return redirect(review_request, name=review_req.doc.name, request_id=review_req.pk)
else:
form = AssignReviewerForm(review_req)
return render(request, 'doc/review/assign_reviewer.html', {
'doc': doc,
'review_req': review_req,
'form': form,
})
class RejectReviewerAssignmentForm(forms.Form):
message_to_secretary = forms.CharField(widget=forms.Textarea, required=False, help_text="Optional explanation of rejection, will be emailed to team secretary")
def reject_request_assignment(request, name, request_id):
def reject_reviewer_assignment(request, name, request_id):
doc = get_object_or_404(Document, name=name)
review_req = get_object_or_404(ReviewRequest, pk=request_id, state__in=["requested", "accepted"])
if not review_req.reviewer:
return redirect(review_request, name=review_req.doc.name, request_id=review_req.pk)
is_reviewer = user_is_person(request.user, review_req.reviewer.role.person)
is_reviewer = user_is_person(request.user, review_req.reviewer.person)
can_manage_req = can_manage_review_requests_for_team(request.user, review_req.team)
if not (is_reviewer or can_manage_req):
return HttpResponseForbidden("You do not have permission to perform this action")
if request.method == "POST" and request.POST.get("action") == "reject":
# reject the old request
prev_state = review_req.state
# reject the request
review_req.state = ReviewRequestStateName.objects.get(slug="rejected")
review_req.save()
# assignment of reviewer is currently considered minutia, so
# not reported in the log
if prev_state.slug == "accepted":
DocEvent.objects.create(
type="changed_review_request",
doc=doc,
by=request.user.person,
desc="Request for {} review by {} is unassigned".format(review_req.type.name, review_req.team.acronym.upper()),
)
# make a new, open review request
ReviewRequest.objects.create(
DocEvent.objects.create(
type="changed_review_request",
doc=review_req.doc,
by=request.user.person,
desc="Assignment of request for {} review by {} to {} was rejected".format(
review_req.type.name,
review_req.team.acronym.upper(),
review_req.reviewer.person,
),
)
# make a new unassigned review request
new_review_req = ReviewRequest.objects.create(
time=review_req.time,
type=review_req.type,
doc=review_req.doc,
team=review_req.team,
deadline=review_req.deadline,
requested_rev=review_req.requested_rev,
state=prev_state,
state=ReviewRequestStateName.objects.get(slug="requested"),
)
return redirect(review_request, name=review_req.doc.name, request_id=review_req.pk)
return redirect(review_request, name=new_review_req.doc.name, request_id=new_review_req.pk)
return render(request, 'doc/review/reject_request_assignment.html', {
return render(request, 'doc/review/reject_reviewer_assignment.html', {
'doc': doc,
'review_req': review_req,
})

View file

@ -10,6 +10,7 @@ class Migration(migrations.Migration):
('group', '0008_auto_20160505_0523'),
('name', '0012_insert_review_name_data'),
('doc', '0012_auto_20160207_0537'),
('person', '0006_auto_20160503_0937'),
]
operations = [
@ -17,11 +18,12 @@ class Migration(migrations.Migration):
name='Reviewer',
fields=[
('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
('frequency', models.IntegerField(help_text=b'Can review every N days')),
('available', models.DateTimeField(help_text=b'When will this reviewer be available again', null=True, blank=True)),
('frequency', models.IntegerField(default=30, help_text=b'Can review every N days')),
('unavailable_until', models.DateTimeField(help_text=b'When will this reviewer be available again', null=True, blank=True)),
('filter_re', models.CharField(max_length=255, blank=True)),
('skip_next', models.IntegerField(help_text=b'Skip the next N review assignments')),
('role', models.OneToOneField(to='group.Role')),
('person', models.ForeignKey(to='person.Person')),
('team', models.ForeignKey(to='group.Group')),
],
options={
},
@ -38,7 +40,7 @@ class Migration(migrations.Migration):
('doc', models.ForeignKey(related_name='review_request_set', to='doc.Document')),
('result', models.ForeignKey(blank=True, to='name.ReviewResultName', null=True)),
('review', models.OneToOneField(null=True, blank=True, to='doc.Document')),
('reviewer', models.ForeignKey(blank=True, to='review.Reviewer', null=True)),
('reviewer', models.ForeignKey(blank=True, to='group.Role', null=True)),
('state', models.ForeignKey(to='name.ReviewRequestStateName')),
('team', models.ForeignKey(to='group.Group')),
('type', models.ForeignKey(to='name.ReviewTypeName')),

View file

@ -2,36 +2,42 @@ from django.db import models
from ietf.doc.models import Document
from ietf.group.models import Group, Role
from ietf.person.models import Person
from ietf.name.models import ReviewTypeName, ReviewRequestStateName, ReviewResultName
class Reviewer(models.Model):
"""
These records associate reviewers with review teams and keep track
of admin data associated with the reviewer in the particular team.
There will be one record for each combination of reviewer and team.
"""
role = models.OneToOneField(Role)
frequency = models.IntegerField(help_text="Can review every N days")
available = models.DateTimeField(blank=True, null=True, help_text="When will this reviewer be available again")
"""Keeps track of admin data associated with the reviewer in the
particular team. There will be one record for each combination of
reviewer and team."""
team = models.ForeignKey(Group)
person = models.ForeignKey(Person)
frequency = models.IntegerField(help_text="Can review every N days", default=30)
unavailable_until = models.DateTimeField(blank=True, null=True, help_text="When will this reviewer be available again")
filter_re = models.CharField(max_length=255, blank=True)
skip_next = models.IntegerField(help_text="Skip the next N review assignments")
class ReviewRequest(models.Model):
"""
"""Represents a request for a review and the process it goes through.
There should be one ReviewRequest entered for each combination of
document, rev, and reviewer.
"""
# Fields filled in on the initial record creation:
document, rev, and reviewer."""
state = models.ForeignKey(ReviewRequestStateName)
# Fields filled in on the initial record creation - these
# constitute the request part.
time = models.DateTimeField(auto_now_add=True)
type = models.ForeignKey(ReviewTypeName)
doc = models.ForeignKey(Document, related_name='review_request_set')
team = models.ForeignKey(Group)
deadline = models.DateTimeField()
requested_rev = models.CharField(verbose_name="requested revision", max_length=16, blank=True, help_text="Fill in if a specific revision is to be reviewed, e.g. 02")
state = models.ForeignKey(ReviewRequestStateName)
# Fields filled in as reviewer is assigned, and as the review
# is uploaded
reviewer = models.ForeignKey(Reviewer, blank=True, null=True)
# Fields filled in as reviewer is assigned and as the review is
# uploaded. Once these are filled in and we progress beyond the
# states requested/assigned, any changes to the assignment happens
# by closing down the current request and making a new one,
# copying the request-part fields above.
reviewer = models.ForeignKey(Role, blank=True, null=True)
review = models.OneToOneField(Document, blank=True, null=True)
reviewed_rev = models.CharField(verbose_name="reviewed revision", max_length=16, blank=True)
result = models.ForeignKey(ReviewResultName, blank=True, null=True)

View file

@ -1,6 +1,48 @@
from ietf.group.models import Group
from ietf.group.models import Group, Role
from ietf.doc.models import DocEvent
from ietf.ietfauth.utils import has_role, is_authorized_in_doc_stream
from ietf.review.models import ReviewRequestStateName
def active_review_teams():
# if there's a ReviewResultName defined, it's a review team
return Group.objects.filter(state="active").exclude(reviewresultname=None)
def can_request_review_of_doc(user, doc):
if not user.is_authenticated():
return False
return is_authorized_in_doc_stream(user, doc)
def can_manage_review_requests_for_team(user, team):
if not user.is_authenticated():
return False
return Role.objects.filter(name="secretary", person__user=user, group=team).exists() or has_role(user, "Secretariat")
def assign_review_request_to_reviewer(review_req, reviewer, by):
assert review_req.state_id in ("requested", "accepted")
if review_req.reviewer == reviewer:
return
prev_state = review_req.state
prev_reviewer = review_req.reviewer
review_req.state = ReviewRequestStateName.objects.get(slug="requested")
review_req.reviewer = reviewer
review_req.save()
DocEvent.objects.create(
type="changed_review_request",
doc=review_req.doc,
by=by,
desc="Request for {} review by {} is assigned to {}".format(
review_req.type.name,
review_req.team.acronym.upper(),
review_req.reviewer.person if review_req.reviewer else "(None)",
),
)
if prev_state.slug != "requested" and prev_reviewer:
# FIXME: email old reviewer?
pass

View file

@ -0,0 +1,22 @@
{% extends "base.html" %}
{# Copyright The IETF Trust 2016, All Rights Reserved #}
{% load origin bootstrap3 static %}
{% block title %}Assign reviewer for {{ review_req.doc.name }}{% endblock %}
{% block content %}
{% origin %}
<h1>Assign reviewer<br><small>{{ review_req.doc.name }}</small></h1>
<form method="post">
{% csrf_token %}
{% bootstrap_form form %}
{% buttons %}
<a class="btn btn-default" href="{% url "ietf.doc.views_review.review_request" name=doc.canonical_name request_id=review_req.pk %}">Cancel</a>
<button type="submit" class="btn btn-primary" name="action" value="assign">Assign reviewer</button>
{% endbuttons %}
</form>
{% endblock %}

View file

@ -8,7 +8,7 @@
{% origin %}
<h1>Reject review assignment<br><small>{{ review_req.doc.name }}</small></h1>
<p>{{ review_req.reviewer.role.person }} is currently assigned to do the review. Do you want to reject this assignment?</p>
<p>{{ review_req.reviewer.person }} is currently assigned to do the review. Do you want to reject this assignment?</p>
<form method="post">
{% csrf_token %}

View file

@ -60,19 +60,25 @@
<td>{{ review_req.state.name }}</td>
</tr>
{% if review_req.reviewer %}
<tr>
<th></th>
<th>Reviewer</th>
<td>
{{ review_req.reviewer.role.person }}
{% if review_req.reviewer %}
{{ review_req.reviewer.person }}
{% else %}
None assigned yet
{% endif %}
{% if can_reject_request_assignment %}
<a class="btn btn-default btn-xs" href="{% url "ietf.doc.views_review.reject_request_assignment" name=doc.name request_id=review_req.pk %}"><span class="fa fa-ban"></span> Reject request assignment</a>
{% if can_assign_reviewer %}
<a class="btn btn-default btn-xs" href="{% url "ietf.doc.views_review.assign_reviewer" name=doc.name request_id=review_req.pk %}"><span class="fa fa-user"></span> {% if review_req.reviewer %}Reassign{% else %}Assign{% endif %} reviewer</a>
{% endif %}
{% if can_reject_reviewer_assignment %}
<a class="btn btn-default btn-xs" href="{% url "ietf.doc.views_review.reject_reviewer_assignment" name=doc.name request_id=review_req.pk %}"><span class="fa fa-ban"></span> Reject reviewer assignment</a>
{% endif %}
</td>
</tr>
{% endif %}
{% if review_req.review %}
<tr>
@ -94,7 +100,7 @@
<tr>
<th></th>
<th>Result of review</th>
<td>{{ review_req.result.name }</td>
<td>{{ review_req.result.name }}</td>
</tr>
{% endif %}
</tbody>