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

corners missing. Fix a couple of other issues.
 - Legacy-Id: 11227

Author: OleLaursen

ietf/doc/tests_review.py

@@ -13,25 +13,30 @@
 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")
     team.reviewresultname_set.add(ReviewResultName.objects.filter(slug__in=["issues", "ready-issues", "ready", "not-ready"]))
 
     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,
         team=team,
         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 @@ def test_request_review(self):
         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 @@ def test_withdraw_request(self):
         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 @@ def test_reject_request_assignment(self):
         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)
 

ietf/doc/urls_review.py

@@ -5,6 +5,7 @@
     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),
 )
 

ietf/doc/utils.py

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

ietf/doc/views_doc.py

@@ -48,8 +48,7 @@
 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.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 = []

ietf/doc/views_review.py

@@ -6,10 +6,12 @@
 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,
     })

ietf/review/migrations/0001_initial.py

@@ -10,18 +10,20 @@ 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 = [
         migrations.CreateModel(
             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')),