From 44e135345c9ff741d4b49a1b4fa425ef4746f0a3 Mon Sep 17 00:00:00 2001 From: Ole Laursen Date: Fri, 20 May 2016 14:14:31 +0000 Subject: [PATCH] Add a page for displaying a review request, add support for withdrawing requests, add tests for these two pages - Legacy-Id: 11217 --- ietf/doc/models.py | 1 + ietf/doc/tests_review.py | 69 +++++++++--- ietf/doc/urls.py | 4 +- ietf/doc/urls_review.py | 9 ++ ietf/doc/views_review.py | 44 +++++++- ietf/name/models.py | 4 +- ietf/review/migrations/0001_initial.py | 2 +- ietf/review/models.py | 2 +- ietf/templates/doc/document_draft.html | 2 +- ietf/templates/doc/review/request_review.html | 2 +- ietf/templates/doc/review/review_request.html | 103 ++++++++++++++++++ .../doc/review/withdraw_request.html | 22 ++++ ietf/utils/test_utils.py | 13 ++- 13 files changed, 247 insertions(+), 30 deletions(-) create mode 100644 ietf/doc/urls_review.py create mode 100644 ietf/templates/doc/review/review_request.html create mode 100644 ietf/templates/doc/review/withdraw_request.html diff --git a/ietf/doc/models.py b/ietf/doc/models.py index 157255dfd..26d610c03 100644 --- a/ietf/doc/models.py +++ b/ietf/doc/models.py @@ -707,6 +707,7 @@ EVENT_TYPES = [ # review ("requested_review", "Requested review"), + ("withdrew_review_request", "Withdrew review"), ] class DocEvent(models.Model): diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index fa0893587..01c27711f 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -1,33 +1,44 @@ # -*- coding: utf-8 -*- import datetime -from pyquery import PyQuery from django.core.urlresolvers import reverse as urlreverse import debug # pyflakes:ignore -from ietf.review.models import ReviewRequest +from ietf.review.models import ReviewRequest, Reviewer from ietf.person.models import Person from ietf.group.models import Group, Role -from ietf.name.models import ReviewResultName +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 +from ietf.utils.test_utils import login_testing_unauthorized, unicontent, reload_db_objects -def make_review_data(): - review_team = Group.objects.create(state_id="active", acronym="reviewteam", name="Review Team", type_id="team") - review_team.reviewresultname_set.add(ReviewResultName.objects.filter(slug__in=["issues", "ready-issues", "ready", "not-ready"])) +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.objects.create(name_id="reviewer", person=p, email=p.email_set.first(), group=review_team) + 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) - return review_team + 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, + reviewed_rev="01", + ) + + return review_req class ReviewTests(TestCase): def test_request_review(self): doc = make_test_data() - review_team = make_review_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, "secretary", url) @@ -47,17 +58,17 @@ class ReviewTests(TestCase): }) self.assertEqual(r.status_code, 302) - req = ReviewRequest.objects.get(doc=doc) + req = ReviewRequest.objects.get(doc=doc, state="requested") self.assertEqual(req.deadline.date(), deadline_date) self.assertEqual(req.deadline.time(), datetime.time(23, 59, 59)) - self.assertEqual(req.state_id, "requested") self.assertEqual(req.team, review_team) 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_team = make_review_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) @@ -73,10 +84,40 @@ class ReviewTests(TestCase): }) self.assertEqual(r.status_code, 302) - req = ReviewRequest.objects.get(doc=doc) + 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 + def test_review_request(self): + doc = make_test_data() + review_req = make_review_data(doc) + + url = urlreverse('ietf.doc.views_review.review_request', kwargs={ "name": doc.name, "request_id": review_req.pk }) + + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + self.assertTrue(review_req.team.acronym.upper() in unicontent(r)) + + def test_withdraw_request(self): + doc = make_test_data() + review_req = make_review_data(doc) + review_req.state = ReviewRequestStateName.objects.get(slug="accepted") + review_req.save() + + url = urlreverse('ietf.doc.views_review.withdraw_request', kwargs={ "name": doc.name, "request_id": review_req.pk }) + login_testing_unauthorized(self, "secretary", url) + + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + + # withdraw + r = self.client.post(url, { "action": "withdraw" }) + self.assertEqual(r.status_code, 302) + + review_req = reload_db_objects(review_req) + self.assertEqual(review_req.state_id, "withdrawn") + self.assertEqual(doc.latest_event().type, "withdrew_review_request") diff --git a/ietf/doc/urls.py b/ietf/doc/urls.py index 188ccbb6d..d75459813 100644 --- a/ietf/doc/urls.py +++ b/ietf/doc/urls.py @@ -36,7 +36,6 @@ from django.views.generic import RedirectView from ietf.doc import views_search, views_draft, views_ballot from ietf.doc import views_status_change from ietf.doc import views_doc -from ietf.doc import views_review session_patterns = [ url(r'^add$', views_doc.add_sessionpresentation), @@ -74,8 +73,7 @@ urlpatterns = patterns('', url(r'^(?P[A-Za-z0-9._+-]+)/ballot/$', views_doc.document_ballot, name="doc_ballot"), (r'^(?P[A-Za-z0-9._+-]+)/(?:(?P[0-9-]+)/)?doc.json$', views_doc.document_json), (r'^(?P[A-Za-z0-9._+-]+)/ballotpopup/(?P[0-9]+)/$', views_doc.ballot_popup), - url(r'^(?P[A-Za-z0-9._+-]+)/requestreview/$', views_review.request_review), - url(r'^(?P[A-Za-z0-9._+-]+)/review/(?P[0-9]+)/$', views_review.review), + url(r'^(?P[A-Za-z0-9._+-]+)/reviewrequest/', include("ietf.doc.urls_review")), url(r'^(?P[A-Za-z0-9._+-]+)/email-aliases/$', RedirectView.as_view(pattern_name='doc_email', permanent=False),name='doc_specific_email_aliases'), diff --git a/ietf/doc/urls_review.py b/ietf/doc/urls_review.py new file mode 100644 index 000000000..afc2f6536 --- /dev/null +++ b/ietf/doc/urls_review.py @@ -0,0 +1,9 @@ +from django.conf.urls import patterns, url +from ietf.doc import views_review + +urlpatterns = patterns('', + url(r'^$', views_review.request_review), + url(r'^(?P[0-9]+)/$', views_review.review_request), + url(r'^(?P[0-9]+)/withdraw/$', views_review.withdraw_request), +) + diff --git a/ietf/doc/views_review.py b/ietf/doc/views_review.py index 5578dcd97..d1f7861bb 100644 --- a/ietf/doc/views_review.py +++ b/ietf/doc/views_review.py @@ -2,7 +2,6 @@ import datetime from django.http import HttpResponseForbidden from django.shortcuts import render, get_object_or_404, redirect -from django.core.urlresolvers import reverse as urlreverse from django import forms from django.contrib.auth.decorators import login_required @@ -13,7 +12,6 @@ from ietf.review.models import ReviewRequest, ReviewRequestStateName from ietf.review.utils import active_review_teams from ietf.utils.fields import DatepickerDateField - class RequestReviewForm(forms.ModelForm): deadline_date = DatepickerDateField(date_format="yyyy-mm-dd", picker_settings={ "autoclose": "1", "start-date": "+0d" }) deadline_time = forms.TimeField(widget=forms.TextInput(attrs={ 'placeholder': "HH:MM" }), help_text="If time is not specified, end of day is assumed", required=False) @@ -87,7 +85,8 @@ def request_review(request, name): type="requested_review", doc=doc, by=request.user.person, - desc="{} review by {} requested".format(review_req.type.name, review_req.team.acronym.upper()), + desc="Requested {} review by {}".format(review_req.type.name, review_req.team.acronym.upper()), + time=review_req.time, ) # FIXME: if I'm a reviewer, auto-assign to myself? @@ -101,8 +100,41 @@ def request_review(request, name): 'form': form, }) -def review(request, name, request_id): +def review_request(request, name, request_id): doc = get_object_or_404(Document, name=name) - review_request = get_object_or_404(ReviewRequest, pk=request_id) + review_req = get_object_or_404(ReviewRequest, pk=request_id) - print doc, review_request + return render(request, 'doc/review/review_request.html', { + 'doc': doc, + 'review_req': review_req, + 'can_withdraw_request': review_req.state_id in ["requested", "accepted"] and is_authorized_in_doc_stream(request.user, doc), + }) + +def withdraw_request(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 is_authorized_in_doc_stream(request.user, doc): + return HttpResponseForbidden("You do not have permission to perform this action") + + if request.method == "POST" and request.POST.get("action") == "withdraw": + review_req.state = ReviewRequestStateName.objects.get(slug="withdrawn") + review_req.save() + + DocEvent.objects.create( + type="withdrew_review_request", + doc=doc, + by=request.user.person, + desc="Withdrew request for {} review by {}".format(review_req.type.name, review_req.team.acronym.upper()), + ) + + if review_req.state_id != "requested": + # FIXME: handle this case - by emailing? + pass + + return redirect(review_request, name=review_req.doc.name, request_id=review_req.pk) + + return render(request, 'doc/review/withdraw_request.html', { + 'doc': doc, + 'review_req': review_req, + }) diff --git a/ietf/name/models.py b/ietf/name/models.py index dad83c97f..733f957b3 100644 --- a/ietf/name/models.py +++ b/ietf/name/models.py @@ -89,12 +89,12 @@ class LiaisonStatementTagName(NameModel): "Action Required, Action Taken" class ReviewRequestStateName(NameModel): """Requested, Accepted, Rejected, Withdrawn, Overtaken By Events, - No Response , Completed""" + No Response, Completed""" class ReviewTypeName(NameModel): """Early Review, Last Call, Telechat""" class ReviewResultName(NameModel): """Almost ready, Has issues, Has nits, Not Ready, On the right track, Ready, Ready with issues, Ready with nits, Serious Issues""" - teams = models.ManyToManyField("group.Group", help_text="Which teams this result can be set for. This also implicitly defines which teams are review teams - if there are no possible review results defined for a given team, it can't be a review team.", blank=True) + teams = models.ManyToManyField("group.Group", help_text="Which teams this result can be set for. This also implicitly defines which teams are review teams - if there are no possible review results defined for a given team, it can't be a review team.", blank=True) diff --git a/ietf/review/migrations/0001_initial.py b/ietf/review/migrations/0001_initial.py index 523b73c5e..2c19afa60 100644 --- a/ietf/review/migrations/0001_initial.py +++ b/ietf/review/migrations/0001_initial.py @@ -21,7 +21,7 @@ class Migration(migrations.Migration): ('available', 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.ForeignKey(to='group.Role')), + ('role', models.OneToOneField(to='group.Role')), ], options={ }, diff --git a/ietf/review/models.py b/ietf/review/models.py index c4a1f5475..7e166333c 100644 --- a/ietf/review/models.py +++ b/ietf/review/models.py @@ -10,7 +10,7 @@ class Reviewer(models.Model): 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.ForeignKey(Role) + 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") filter_re = models.CharField(max_length=255, blank=True) diff --git a/ietf/templates/doc/document_draft.html b/ietf/templates/doc/document_draft.html index 20f4a2761..853d6fcc6 100644 --- a/ietf/templates/doc/document_draft.html +++ b/ietf/templates/doc/document_draft.html @@ -200,7 +200,7 @@ {% for r in review_requests %} {% endfor %} diff --git a/ietf/templates/doc/review/request_review.html b/ietf/templates/doc/review/request_review.html index d05b27733..aa1700aab 100644 --- a/ietf/templates/doc/review/request_review.html +++ b/ietf/templates/doc/review/request_review.html @@ -23,7 +23,7 @@ {% bootstrap_field form.requested_rev layout="horizontal" %} {% buttons %} - + Back {% endbuttons %} diff --git a/ietf/templates/doc/review/review_request.html b/ietf/templates/doc/review/review_request.html new file mode 100644 index 000000000..f1087cd84 --- /dev/null +++ b/ietf/templates/doc/review/review_request.html @@ -0,0 +1,103 @@ +{% extends "base.html" %} +{# Copyright The IETF Trust 2016, All Rights Reserved #} +{% load origin bootstrap3 static %} + +{% block title %}Review request for {{ review_req.doc.name }}{% endblock %} + +{% block content %} + {% origin %} +

Review request
{{ review_req.doc.name }}

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + {% if review_req.reviewer %} + + + + + + {% endif %} + + {% if review_req.review %} + + + + + + {% endif %} + + {% if review_req.reviewed_rev %} + + + + + + {% endif %} + + {% if review_req.result %} + + + + + + {% endif %} + +
RequestReview of + {% if review_req.requested_rev %} + {{ review_req.doc.name }}-{{ review_req.requested_rev }} + {% else %} + {{ review_req.doc.name }} + {% endif %} +
Type{{ review_req.type.name }} Review
Team{{ review_req.team.acronym|upper }}
Deadline + {% if review_req.deadline|date:"H:i" != "23:59" %} + {{ review_req.deadline|date:"Y-m-d H:i" }} + {% else %} + {{ review_req.deadline|date:"Y-m-d" }} + {% endif %} +
Requested{{ review_req.time|date:"Y-m-d" }}
ReviewState{{ review_req.state.name }}
Reviewer{{ review_req.reviewer.role.person }}
Review{{ review_req.review.name }}
Reviewed revision{{ review_req.reviewed_rev }}
Result of review{{ review_req.result.name }
+ +
+ {% if can_withdraw_request %} + Withdraw request + {% endif %} +
+ +{% endblock %} diff --git a/ietf/templates/doc/review/withdraw_request.html b/ietf/templates/doc/review/withdraw_request.html new file mode 100644 index 000000000..191f236dc --- /dev/null +++ b/ietf/templates/doc/review/withdraw_request.html @@ -0,0 +1,22 @@ +{% extends "base.html" %} +{# Copyright The IETF Trust 2016, All Rights Reserved #} +{% load origin bootstrap3 static %} + +{% block title %}Withdraw review request for {{ review_req.doc.name }}{% endblock %} + +{% block content %} + {% origin %} +

Withdraw review request
{{ review_req.doc.name }}

+ +

Do you want to withdraw the review request?

+ +
+ {% csrf_token %} + + {% buttons %} + Cancel + + {% endbuttons %} +
+ +{% endblock %} diff --git a/ietf/utils/test_utils.py b/ietf/utils/test_utils.py index bc4ed9b77..b3253aa14 100644 --- a/ietf/utils/test_utils.py +++ b/ietf/utils/test_utils.py @@ -255,7 +255,7 @@ def canonicalize_sitemap(s): def login_testing_unauthorized(test_case, username, url, password=None): r = test_case.client.get(url) - test_case.assertTrue(r.status_code in (302, 403)) + test_case.assertIn(r.status_code, (302, 403)) if r.status_code == 302: test_case.assertTrue("/accounts/login" in r['Location']) if not password: @@ -272,6 +272,17 @@ def unicontent(r): encoding = 'utf-8' return r.content.decode(encoding) +def reload_db_objects(*objects): + """Rerequest the given arguments from the database so they're refreshed, to be used like + + foo, bar = reload_objects(foo, bar)""" + + t = tuple(o.__class__.objects.get(pk=o.pk) for o in objects) + if len(objects) == 1: + return t[0] + else: + return t + class ReverseLazyTest(django.test.TestCase): def test_redirect_with_lazy_reverse(self): response = self.client.get('/ipr/update/')