Add refresh button to manage reviews page, make save detect changes in

the requests and pop the page back up for confirmation if so
 - Legacy-Id: 11827

ietf/group/tests_review.py

@@ -107,21 +107,52 @@ class ReviewTests(TestCase):
         self.assertEqual(r.status_code, 200)
         self.assertTrue(review_req1.doc.name in unicontent(r))
 
-        # close and assign
+        # can't save: 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, {
+            "reviewrequest": [str(review_req1.pk), str(review_req2.pk), str(123456)],
+
             # close
+            "r{}-existing_reviewer".format(review_req1.pk): "123456",
             "r{}-action".format(review_req1.pk): "close",
             "r{}-close".format(review_req1.pk): "no-response",
 
             # assign
+            "r{}-existing_reviewer".format(review_req2.pk): "123456",
+            "r{}-action".format(review_req2.pk): "assign",
+            "r{}-reviewer".format(review_req2.pk): new_reviewer.pk,
+
+            "action": "save",
+        })
+        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
+        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)],
+
+            # close
+            "r{}-existing_reviewer".format(review_req1.pk): review_req1.reviewer_id or "",
+            "r{}-action".format(review_req1.pk): "close",
+            "r{}-close".format(review_req1.pk): "no-response",
+
+            # assign
+            "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,
 
             # 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)
 
@@ -130,7 +161,3 @@ class ReviewTests(TestCase):
         self.assertEqual(review_req2.state_id, "requested")
         self.assertEqual(review_req2.reviewer, new_reviewer)
         self.assertEqual(review_req3.state_id, "requested")
-
-        # FIXME: test suggested
-
-

ietf/group/views_review.py

@@ -22,9 +22,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")
 
     def __init__(self, review_req, *args, **kwargs):
@@ -83,6 +81,8 @@ def manage_review_requests(request, acronym, group_type=None):
         set(r.doc_id for r in review_requests),
     )
 
+    # we need a mutable query dict
+    query_dict = request.POST.copy() if request.method == "POST" else None
     for req in review_requests:
         l = []
         # take all on the latest reviewed rev
@@ -98,14 +98,49 @@ def manage_review_requests(request, acronym, group_type=None):
 
         req.latest_reqs = l
 
-        req.form = ManageReviewRequestForm(req, request.POST if request.method == "POST" else None)
+        req.form = ManageReviewRequestForm(req, query_dict)
+
+    saving = False
+    newly_closed = newly_opened = newly_assigned = 0
 
     if request.method == "POST":
+        saving = request.POST.get("action") == "save"
+
+        # check for conflicts
+        review_requests_dict = { unicode(r.pk): r for r in review_requests }
+        posted_reqs = set(request.POST.getlist("reviewrequest", []))
+        current_reqs = set(review_requests_dict.iterkeys())
+
+        closed_reqs = posted_reqs - current_reqs
+        newly_closed += len(closed_reqs)
+
+        opened_reqs = current_reqs - posted_reqs
+        newly_opened += len(opened_reqs)
+        for r in opened_reqs:
+            review_requests_dict[r].form.add_error(None, "New request.")
+
+        for req in review_requests:
+            existing_reviewer = request.POST.get(req.form.prefix + "-existing_reviewer")
+            if existing_reviewer is None:
+                continue
+
+            if existing_reviewer != unicode(req.reviewer_id or ""):
+                msg = "Assignment was changed."
+                a = req.form["action"].value()
+                if a == "assign":
+                    msg += " Didn't assign reviewer."
+                elif a == "close":
+                    msg += " Didn't close request."
+                req.form.add_error(None, msg)
+                req.form.data[req.form.prefix + "-action"] = "" # cancel the action
+
+                newly_assigned += 1
+
         form_results = []
         for req in review_requests:
             form_results.append(req.form.is_valid())
 
-        if all(form_results):
+        if saving and all(form_results) and not (newly_closed > 0 or newly_opened > 0 or newly_assigned > 0):
             for review_req in review_requests:
                 action = review_req.form.cleaned_data.get("action")
                 if action == "assign":
@@ -113,16 +148,18 @@ def manage_review_requests(request, acronym, group_type=None):
                 elif action == "close":
                     close_review_request(request, review_req, review_req.form.cleaned_data["close"])
 
-
             kwargs = { "acronym": group.acronym }
             if group_type:
                 kwargs["group_type"] = group_type
             import ietf.group.views
             return redirect(ietf.group.views.review_requests, **kwargs)
 
-
     return render(request, 'group/manage_review_requests.html', {
         'group': group,
         'review_requests': review_requests,
+        'newly_closed': newly_closed,
+        'newly_opened': newly_opened,
+        'newly_assigned': newly_assigned,
+        'saving': saving,
     })
 

ietf/static/ietf/js/manage-review-requests.js

@@ -32,6 +32,15 @@ $(document).ready(function () {
     });
 
     form.find("[name$=\"-action\"]").each(function () {
-        console.log(this);
+        var v = $(this).val();
+        if (!v)
+            return;
+
+        var row = $(this).closest("tr");
+
+        if (v == "assign")
+            row.find(".reviewer-action").click();
+        else if (v == "close")
+            row.find(".close-action").click();
     });
 });

ietf/templates/group/manage_review_requests.html

@@ -4,7 +4,7 @@
 
 {% load ietf_filters staticfiles bootstrap3 %}
 
-{% block title %}Manage pending review requests for {{ group.acronym }}{% endblock %}
+{% block title %}Manage open review requests for {{ group.acronym }}{% endblock %}
 
 {% block pagehead %}
   <link rel="stylesheet" href="{% static "jquery.tablesorter/css/theme.bootstrap.min.css" %}">
@@ -15,7 +15,23 @@
 
   <h1>Manage open review requests for {{ group.acronym }}</h1>
 
-  <p>For reference: <a href="{% url "ietf.group.views.review_requests" group_type=group.type_id acronym=group.acronym %}#closed-review-requests">closed review requests</a>
+  <p>Other options:
+    <a href="{% url "ietf.group.views.review_requests" group_type=group.type_id acronym=group.acronym %}#closed-review-requests">Closed review requests</a>
+    - <a href="FIXME">Email open assignments summary</a>
+  </p>
+
+  {% if newly_closed > 0 or newly_opened > 0 or newly_assigned > 0 %}
+    <p class="alert alert-danger">
+      Changes since last refresh:
+      {% if newly_closed %}{{ newly_closed }} request{{ newly_closed|pluralize }} closed.{% endif %}
+      {% if newly_opened %}{{ newly_opened }} request{{ newly_opened|pluralize }} opened.{% endif %}
+      {% if newly_assigned %}{{ newly_assigned }} request{{ newly_assigned|pluralize }} changed assignment.{% endif %}
+
+      {% if saving %}
+        Check that you are happy with the results, then re-save.
+      {% endif %}
+    </p>
+  {% endif %}
 
   {% if review_requests %}
     <form class="review-requests" method="post">{% csrf_token %}
@@ -33,7 +49,8 @@
         <tbody>
           {% for r in review_requests %}
             <tr>
-              <td><a href="{% if r.requested_rev %}{% url "doc_view" name=r.doc.name rev=r.requested_rev %}{% else %}{% url "doc_view" name=r.doc.name %}{% endif %}">{{ r.doc.name }}{% if r.requested_rev %}-{{ r.requested_rev }}{% endif %}</a>
+              <td>
+                <a href="{% if r.requested_rev %}{% url "doc_view" name=r.doc.name rev=r.requested_rev %}{% else %}{% url "doc_view" name=r.doc.name %}{% endif %}">{{ r.doc.name }}{% if r.requested_rev %}-{{ r.requested_rev }}{% endif %}</a>
                 {% if r.latest_reqs %}
                   <br>
                   <small>- prev. review:
@@ -43,6 +60,14 @@
                     {% endfor %}
                   </small>
                 {% endif %}
+
+                {% if r.form.non_field_errors %}
+                  <div class="alert alert-danger">
+                    {% for e in r.form.non_field_errors %}
+                      {{ e }}
+                    {% endfor %}
+                  </div>
+                {% endif %}
               </td>
               <td>{{ r.type.name }}</td>
               <td>{% if r.time %}{{ r.time|date:"Y-m-d" }}{% else %}<em>auto-suggested</em>{% endif %}</td>
@@ -51,6 +76,9 @@
                 {% if r.due %}<span class="label label-warning">{{ r.due }} day{{ r.due|pluralize }}</span>{% endif %}
               </td>
               <td>
+                <input type="hidden" name="reviewrequest" value="{{ r.pk }}">
+                <input type="hidden" name="{{ r.form.prefix }}-existing_reviewer" value="{{ r.reviewer_id }}">
+
                 {% if r.reviewer %}
                   <button type="button" class="btn btn-default btn-sm reviewer-action" title="Click to reassign request">{{ r.reviewer.person }} {% if r.state_id == "accepted" %}<span class="label label-default">accp</span>{% endif %}</button>
                 {% else %}
@@ -64,8 +92,11 @@
                     {{ r.form.reviewer }}
                     <button type="button" class="btn btn-default btn-sm undo" title="Undo assignment"><span class="fa fa-times"></span></button>
                     {% if r.form.reviewer.errors %}
-                      <br>
-                      {{ r.form.reviewer.errors }}
+                      <div class="alert alert-danger">
+                        {% for e in r.form.reviewer.errors %}
+                          {{ e }}
+                        {% endfor %}
+                      </div>
                     {% endif %}
                   {% endspaceless %}
                 </span>
@@ -91,7 +122,8 @@
 
       {% buttons %}
         <a href="{% url "ietf.group.views.review_requests" group_type=group.type_id acronym=group.acronym %}" class="btn btn-default pull-right">Cancel</a>
-        <button class="btn btn-primary" type="submit">Save changes</button>
+        <button class="btn btn-primary" type="submit" name="action" value="save">Save changes</button>
+        <button class="btn btn-default" type="submit" name="action" value="refresh">Refresh (keeping changes)</button>
       {% endbuttons %}
     </form>
   {% else %}