From 58dd25ff7936bed1c1cbbba470b50bafae8bebfb Mon Sep 17 00:00:00 2001 From: Ole Laursen Date: Fri, 4 Nov 2016 17:19:33 +0000 Subject: [PATCH] Split up open review requests on the review team page in assigned and unassigned requests to make it easier to just work with the unassigned ones. Use same split on the manage reviews page which is now two pages. - Legacy-Id: 12266 --- ietf/group/tests_review.py | 37 ++++++-- ietf/group/urls_info_details.py | 2 +- ietf/group/utils.py | 3 +- ietf/group/views_review.py | 95 ++++++++++++------- .../group/email_open_review_assignments.html | 2 +- .../group/manage_review_requests.html | 9 +- ietf/templates/group/review_requests.html | 75 +++++++-------- 7 files changed, 137 insertions(+), 86 deletions(-) diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py index 0d5e90a92..563ecac3f 100644 --- a/ietf/group/tests_review.py +++ b/ietf/group/tests_review.py @@ -148,11 +148,12 @@ class ReviewTests(TestCase): group = review_req1.team - url = urlreverse(ietf.group.views_review.manage_review_requests, kwargs={ 'acronym': group.acronym }) + url = urlreverse(ietf.group.views_review.manage_review_requests, kwargs={ 'acronym': group.acronym, "assignment_status": "assigned" }) login_testing_unauthorized(self, "secretary", url) - url = urlreverse(ietf.group.views_review.manage_review_requests, kwargs={ 'acronym': group.acronym, 'group_type': group.type_id }) + assigned_url = urlreverse(ietf.group.views_review.manage_review_requests, kwargs={ 'acronym': group.acronym, 'group_type': group.type_id, "assignment_status": "assigned" }) + unassigned_url = urlreverse(ietf.group.views_review.manage_review_requests, kwargs={ 'acronym': group.acronym, 'group_type': group.type_id, "assignment_status": "unassigned" }) review_req2 = ReviewRequest.objects.create( doc=review_req1.doc, @@ -201,14 +202,14 @@ class ReviewTests(TestCase): ) # get - r = self.client.get(url) + r = self.client.get(assigned_url) self.assertEqual(r.status_code, 200) self.assertTrue(review_req1.doc.name in unicontent(r)) - # can't save: conflict + # can't save assigned: conflict new_reviewer = Email.objects.get(role__name="reviewer", role__group=group, person__user__username="marschairman") # provoke conflict by posting bogus data - r = self.client.post(url, { + r = self.client.post(assigned_url, { "reviewrequest": [str(review_req1.pk), str(review_req2.pk), str(123456)], # close @@ -226,13 +227,21 @@ class ReviewTests(TestCase): self.assertEqual(r.status_code, 200) content = unicontent(r).lower() self.assertTrue("1 request closed" in content) - self.assertTrue("1 request opened" in content) self.assertTrue("2 requests changed assignment" in content) - # close and assign + # can't save unassigned: conflict + r = self.client.post(unassigned_url, { + "reviewrequest": [str(123456)], + "action": "save-continue", + }) + self.assertEqual(r.status_code, 200) + content = unicontent(r).lower() + self.assertTrue("1 request opened" in content) + + # close and reassign assigned new_reviewer = Email.objects.get(role__name="reviewer", role__group=group, person__user__username="marschairman") - r = self.client.post(url, { - "reviewrequest": [str(review_req1.pk), str(review_req2.pk), str(review_req3.pk)], + r = self.client.post(assigned_url, { + "reviewrequest": [str(review_req1.pk), str(review_req2.pk)], # close "r{}-existing_reviewer".format(review_req1.pk): review_req1.reviewer_id or "", @@ -244,12 +253,20 @@ class ReviewTests(TestCase): "r{}-action".format(review_req2.pk): "assign", "r{}-reviewer".format(review_req2.pk): new_reviewer.pk, + "action": "save", + }) + self.assertEqual(r.status_code, 302) + + # no change on unassigned + r = self.client.post(unassigned_url, { + "reviewrequest": [str(review_req3.pk)], + # no change "r{}-existing_reviewer".format(review_req3.pk): review_req3.reviewer_id or "", "r{}-action".format(review_req3.pk): "", "r{}-close".format(review_req3.pk): "no-response", "r{}-reviewer".format(review_req3.pk): "", - + "action": "save", }) self.assertEqual(r.status_code, 302) diff --git a/ietf/group/urls_info_details.py b/ietf/group/urls_info_details.py index ff7d08e7e..c0c47a8d5 100644 --- a/ietf/group/urls_info_details.py +++ b/ietf/group/urls_info_details.py @@ -31,7 +31,7 @@ urlpatterns = patterns('', (r'^archives/$', 'ietf.group.views.derived_archives'), (r'^photos/$', views.group_photos), (r'^reviews/$', views_review.review_requests), - (r'^reviews/manage/$', views_review.manage_review_requests), + (r'^reviews/manage/(?Passigned|unassigned)/$', views_review.manage_review_requests), (r'^reviews/email-assignments/$', views_review.email_open_review_assignments), (r'^reviewers/$', views_review.reviewer_overview), (r'^reviewers/(?P[\w%+-.@]+)/settings/$', views_review.change_reviewer_settings), diff --git a/ietf/group/utils.py b/ietf/group/utils.py index e7c8c5af9..1cc41ca77 100644 --- a/ietf/group/utils.py +++ b/ietf/group/utils.py @@ -213,7 +213,8 @@ def construct_group_menu_context(request, group, selected, group_type, others): if group.features.has_reviews and can_manage_review_requests_for_team(request.user, group): import ietf.group.views_review - actions.append((u"Manage review requests", urlreverse(ietf.group.views_review.manage_review_requests, kwargs=kwargs))) + actions.append((u"Manage unassigned reviews", urlreverse(ietf.group.views_review.manage_review_requests, kwargs=dict(assignment_status="unassigned", **kwargs)))) + actions.append((u"Manage assigned reviews", urlreverse(ietf.group.views_review.manage_review_requests, kwargs=dict(assignment_status="assigned", **kwargs)))) if group.state_id != "conclude" and (is_admin or can_manage): actions.append((u"Edit group", urlreverse("group_edit", kwargs=kwargs))) diff --git a/ietf/group/views_review.py b/ietf/group/views_review.py index f6421a283..b45e990e7 100644 --- a/ietf/group/views_review.py +++ b/ietf/group/views_review.py @@ -30,33 +30,55 @@ from ietf.utils.mail import send_mail_text from ietf.utils.fields import DatepickerDateField from ietf.ietfauth.utils import user_is_person +def get_open_review_requests_for_team(team, assignment_status=None): + open_review_requests = ReviewRequest.objects.filter( + team=team, + state__in=("requested", "accepted") + ).prefetch_related( + "reviewer__person", "type", "state" + ).order_by("-time", "-id") + + if assignment_status == "unassigned": + open_review_requests = suggested_review_requests_for_team(team) + list(open_review_requests.filter(reviewer=None)) + elif assignment_status == "assigned": + open_review_requests = list(open_review_requests.exclude(reviewer=None)) + else: + open_review_requests = suggested_review_requests_for_team(team) + list(open_review_requests) + + today = datetime.date.today() + unavailable_periods = current_unavailable_periods_for_reviewers(team) + for r in open_review_requests: + if r.reviewer: + r.reviewer_unavailable = any(p.availability == "unavailable" + for p in unavailable_periods.get(r.reviewer.person_id, [])) + r.due = max(0, (today - r.deadline).days) + + return open_review_requests def review_requests(request, acronym, group_type=None): group = get_group_or_404(acronym, group_type) if not group.features.has_reviews: raise Http404 - open_review_requests = list(ReviewRequest.objects.filter( - team=group, state__in=("requested", "accepted") - ).prefetch_related("reviewer", "type", "state").order_by("-time", "-id")) + assigned_review_requests = [] + unassigned_review_requests = [] - unavailable_periods = current_unavailable_periods_for_reviewers(group) - for review_req in open_review_requests: - if review_req.reviewer: - review_req.reviewer_unavailable = any(p.availability == "unavailable" - for p in unavailable_periods.get(review_req.reviewer.person_id, [])) + for r in get_open_review_requests_for_team(group): + if r.reviewer: + assigned_review_requests.append(r) + else: + unassigned_review_requests.append(r) - open_review_requests = suggested_review_requests_for_team(group) + open_review_requests - - today = datetime.date.today() - for r in open_review_requests: - r.due = max(0, (today - r.deadline).days) + open_review_requests = [ + ("Unassigned", unassigned_review_requests), + ("Assigned", assigned_review_requests), + ] closed_review_requests = ReviewRequest.objects.filter( team=group, ).exclude( state__in=("requested", "accepted") - ).prefetch_related("reviewer", "type", "state", "doc").order_by("-time", "-id") + ).prefetch_related("reviewer__person", "type", "state", "doc", "result").order_by("-time", "-id") since_choices = [ (None, "1 month"), @@ -192,7 +214,7 @@ class ManageReviewRequestForm(forms.Form): @login_required -def manage_review_requests(request, acronym, group_type=None): +def manage_review_requests(request, acronym, group_type=None, assignment_status=None): group = get_group_or_404(acronym, group_type) if not group.features.has_reviews: raise Http404 @@ -200,18 +222,7 @@ def manage_review_requests(request, acronym, group_type=None): if not can_manage_review_requests_for_team(request.user, group): return HttpResponseForbidden("You do not have permission to perform this action") - unavailable_periods = current_unavailable_periods_for_reviewers(group) - - open_review_requests = list(ReviewRequest.objects.filter( - team=group, state__in=("requested", "accepted") - ).prefetch_related("reviewer", "type", "state").order_by("-time", "-id")) - - for review_req in open_review_requests: - if review_req.reviewer: - review_req.reviewer_unavailable = any(p.availability == "unavailable" - for p in unavailable_periods.get(review_req.reviewer.person_id, [])) - - review_requests = suggested_review_requests_for_team(group) + open_review_requests + review_requests = get_open_review_requests_for_team(group, assignment_status=assignment_status) document_requests = extract_revision_ordered_review_requests_for_documents_and_replaced( ReviewRequest.objects.filter(state__in=("part-completed", "completed"), team=group).prefetch_related("result"), @@ -221,10 +232,12 @@ def manage_review_requests(request, acronym, group_type=None): # we need a mutable query dict for resetting upon saving with # conflicts query_dict = request.POST.copy() if request.method == "POST" else None + for req in review_requests: + # add previous requests l = [] - # take all on the latest reviewed rev for r in document_requests.get(req.doc_id, []): + # take all on the latest reviewed rev if l and l[0].reviewed_rev: if r.doc_id == l[0].doc_id and r.reviewed_rev: if int(r.reviewed_rev) > int(l[0].reviewed_rev): @@ -292,11 +305,19 @@ def manage_review_requests(request, acronym, group_type=None): kwargs["group_type"] = group_type if form_action == "save-continue": + if assignment_status: + kwargs["assignment_status"] = assignment_status + return redirect(manage_review_requests, **kwargs) else: import ietf.group.views_review return redirect(ietf.group.views_review.review_requests, **kwargs) + other_assignment_status = { + "unassigned": "assigned", + "assigned": "unassigned", + }.get(assignment_status) + return render(request, 'group/manage_review_requests.html', { 'group': group, 'review_requests': review_requests, @@ -304,6 +325,8 @@ def manage_review_requests(request, acronym, group_type=None): 'newly_opened': newly_opened, 'newly_assigned': newly_assigned, 'saving': saving, + 'assignment_status': assignment_status, + 'other_assignment_status': other_assignment_status, }) class EmailOpenAssignmentsForm(forms.Form): @@ -327,16 +350,21 @@ def email_open_review_assignments(request, acronym, group_type=None): reviewer=None, ).prefetch_related("reviewer", "type", "state", "doc").distinct().order_by("deadline", "reviewer")) + back_url = request.GET.get("next") + if not back_url: + kwargs = { "acronym": group.acronym } + if group_type: + kwargs["group_type"] = group_type + + import ietf.group.views_review + back_url = urlreverse(ietf.group.views_review.review_requests, kwargs=kwargs) + if request.method == "POST" and request.POST.get("action") == "email": form = EmailOpenAssignmentsForm(request.POST) if form.is_valid(): send_mail_text(request, form.cleaned_data["to"], None, form.cleaned_data["subject"], form.cleaned_data["body"]) - kwargs = { "acronym": group.acronym } - if group_type: - kwargs["group_type"] = group_type - - return redirect(manage_review_requests, **kwargs) + return HttpResponseRedirect(back_url) else: to = group.list_email subject = "Open review assignments in {}".format(group.acronym) @@ -356,6 +384,7 @@ def email_open_review_assignments(request, acronym, group_type=None): 'group': group, 'review_requests': review_requests, 'form': form, + 'back_url': back_url, }) diff --git a/ietf/templates/group/email_open_review_assignments.html b/ietf/templates/group/email_open_review_assignments.html index c5883eb6c..70083a618 100644 --- a/ietf/templates/group/email_open_review_assignments.html +++ b/ietf/templates/group/email_open_review_assignments.html @@ -16,7 +16,7 @@ {% bootstrap_form form %} {% buttons %} - Cancel + Cancel {% endbuttons %} diff --git a/ietf/templates/group/manage_review_requests.html b/ietf/templates/group/manage_review_requests.html index 1433ccad5..c820b89b1 100644 --- a/ietf/templates/group/manage_review_requests.html +++ b/ietf/templates/group/manage_review_requests.html @@ -13,11 +13,14 @@ {% block content %} {% origin %} -

Manage open review requests for {{ group.acronym }}

+

Manage {{ assignment_status }} open review requests for {{ group.acronym }}

Other options: Reviewers in team - - Email open assignments summary + - Email open assignments summary + {% if other_assignment_status %} + - Manage {{ other_assignment_status }} reviews + {% endif %}

{% if newly_closed > 0 or newly_opened > 0 or newly_assigned > 0 %} @@ -136,7 +139,7 @@ {% endbuttons %} {% else %} -

There are currently no open requests.

+

There are currently no {{ assignment_status }} open requests.

{% endif %} {% endblock %} diff --git a/ietf/templates/group/review_requests.html b/ietf/templates/group/review_requests.html index 64d7c22b4..87c48bd0f 100644 --- a/ietf/templates/group/review_requests.html +++ b/ietf/templates/group/review_requests.html @@ -17,46 +17,47 @@

  

{% endif %} -

Open review requests

+ {% for label, review_requests in open_review_requests %} + {% if review_requests %} - {% if open_review_requests %} - - - - - - - - - - - - {% for r in open_review_requests %} +

{{ label }} open review requests

+ +
RequestTypeRequestedDeadlineReviewer
+ - - - - - + + + + + {% if review_requests.0.reviewer %} + + {% endif %} - {% endfor %} - -
{% if r.pk != None %}{% endif %}{{ r.doc.name }}-{% if r.requested_rev %}{{ r.requested_rev }}{% else %}{{ r.doc.rev }}{% endif %}{% if r.pk != None %}{% endif %}{{ r.type.name }}{% if r.pk %}{{ r.time|date:"Y-m-d" }}{% else %}auto-suggested{% endif %} - {{ r.deadline|date:"Y-m-d" }} - {% if r.due %}{{ r.due }} day{{ r.due|pluralize }}{% endif %} - - {% if r.reviewer %} - {{ r.reviewer.person }} - {% if r.state_id == "accepted" %}Accepted{% endif %} - {% if r.reviewer_unavailable %}Unavailable{% endif %} - {% elif r.pk != None %} - not yet assigned - {% endif %} - RequestTypeRequestedDeadlineReviewer
- - {% else %} -

There are currently no open requests.

- {% endif %} + + + {% for r in review_requests %} + + {% if r.pk != None %}{% endif %}{{ r.doc.name }}-{% if r.requested_rev %}{{ r.requested_rev }}{% else %}{{ r.doc.rev }}{% endif %}{% if r.pk != None %}{% endif %} + {{ r.type.name }} + {% if r.pk %}{{ r.time|date:"Y-m-d" }}{% else %}auto-suggested{% endif %} + + {{ r.deadline|date:"Y-m-d" }} + {% if r.due %}{{ r.due }} day{{ r.due|pluralize }}{% endif %} + + {% if r.reviewer %} + + {{ r.reviewer.person }} + {% if r.state_id == "accepted" %}Accepted{% endif %} + {% if r.reviewer_unavailable %}Unavailable{% endif %} + + {% endif %} + + {% endfor %} + + + + {% endif %} + {% endfor %}

Closed review requests