Add statistics section with review statistics. Fix a couple of missing tests.

- Legacy-Id: 12124

ietf/group/tests_review.py

@@ -24,8 +24,7 @@ class ReviewTests(TestCase):
         group = review_req.team
 
         for url in [urlreverse(ietf.group.views_review.review_requests, kwargs={ 'acronym': group.acronym }),
-                     urlreverse(ietf.group.views_review.review_requests, kwargs={ 'acronym': group.acronym , 'group_type': group.type_id}),
-                   ]:
+                    urlreverse(ietf.group.views_review.review_requests, kwargs={ 'acronym': group.acronym , 'group_type': group.type_id})]:
             r = self.client.get(url)
             self.assertEqual(r.status_code, 200)
             self.assertTrue(review_req.doc.name in unicontent(r))
@@ -131,9 +130,9 @@ class ReviewTests(TestCase):
 
         group = review_req1.team
 
-        url = urlreverse(ietf.group.views_review.reviewer_overview, kwargs={ 'acronym': group.acronym, 'group_type': group.type_id })
-
         # get
+        for url in [urlreverse(ietf.group.views_review.reviewer_overview, kwargs={ 'acronym': group.acronym }),
+                    urlreverse(ietf.group.views_review.reviewer_overview, kwargs={ 'acronym': group.acronym, 'group_type': group.type_id })]:
             r = self.client.get(url)
             self.assertEqual(r.status_code, 200)
             self.assertTrue(unicode(reviewer) in unicontent(r))
@@ -149,10 +148,12 @@ class ReviewTests(TestCase):
 
         group = review_req1.team
 
-        url = urlreverse(ietf.group.views_review.manage_review_requests, kwargs={ 'acronym': group.acronym, 'group_type': group.type_id })
+        url = urlreverse(ietf.group.views_review.manage_review_requests, kwargs={ 'acronym': group.acronym })
 
         login_testing_unauthorized(self, "secretary", url)
 
+        url = urlreverse(ietf.group.views_review.manage_review_requests, kwargs={ 'acronym': group.acronym, 'group_type': group.type_id })
+
         review_req2 = ReviewRequest.objects.create(
             doc=review_req1.doc,
             team=review_req1.team,
@@ -265,10 +266,12 @@ class ReviewTests(TestCase):
 
         group = review_req1.team
 
-        url = urlreverse(ietf.group.views_review.email_open_review_assignments, kwargs={ 'acronym': group.acronym, 'group_type': group.type_id })
+        url = urlreverse(ietf.group.views_review.email_open_review_assignments, kwargs={ 'acronym': group.acronym })
 
         login_testing_unauthorized(self, "secretary", url)
 
+        url = urlreverse(ietf.group.views_review.email_open_review_assignments, kwargs={ 'acronym': group.acronym, 'group_type': group.type_id })
+
         r = self.client.get(url)
         self.assertEqual(r.status_code, 200)
         q = PyQuery(r.content)
@@ -299,13 +302,18 @@ class ReviewTests(TestCase):
         review_req.save()
         
         url = urlreverse(ietf.group.views_review.change_reviewer_settings, kwargs={
-            "group_type": review_req.team.type_id,
             "acronym": review_req.team.acronym,
             "reviewer_email": review_req.reviewer_id,
         })
 
         login_testing_unauthorized(self, reviewer.user.username, url)
 
+        url = urlreverse(ietf.group.views_review.change_reviewer_settings, kwargs={
+            "group_type": review_req.team.type_id,
+            "acronym": review_req.team.acronym,
+            "reviewer_email": review_req.reviewer_id,
+        })
+
         # get
         r = self.client.get(url)
         self.assertEqual(r.status_code, 200)

ietf/group/views_review.py

@@ -1,4 +1,4 @@
-import datetime, math
+import datetime, math, itertools
 from collections import defaultdict
 
 from django.shortcuts import render, redirect, get_object_or_404
@@ -105,7 +105,12 @@ def reviewer_overview(request, acronym, group_type=None):
 
     today = datetime.date.today()
 
-    all_req_data = extract_review_request_data(teams=[group], time_from=today - datetime.timedelta(days=365))
+    extracted_data = extract_review_request_data(teams=[group], time_from=today - datetime.timedelta(days=365), ordering=["reviewer"])
+    req_data_for_reviewer = {}
+    for reviewer, req_data_items in itertools.groupby(extracted_data, key=lambda data: data.reviewer):
+        l = list(req_data_items)
+        l.reverse()
+        req_data_for_reviewer[reviewer] = l
     review_state_by_slug = { n.slug: n for n in ReviewRequestStateName.objects.all() }
 
     for person in reviewers:
@@ -120,15 +125,15 @@ def reviewer_overview(request, acronym, group_type=None):
                                        for p in person.unavailable_periods)
 
         MAX_REQS = 5
-        req_data = all_req_data.get((group.pk, person.pk), [])
-        open_reqs = sum(1 for _, _, _, _, state, _, _, _, _, _ in req_data if state in ("requested", "accepted"))
+        req_data = req_data_for_reviewer.get(person.pk, [])
+        open_reqs = sum(1 for d in req_data if d.state in ("requested", "accepted"))
         latest_reqs = []
-        for req_pk, doc, req_time, state, deadline, result, late_days, request_to_assignment_days, assignment_to_closure_days, request_to_closure_days in req_data:
+        for d in req_data:
             # any open requests pushes the others out
-            if ((state in ("requested", "accepted") and len(latest_reqs) < MAX_REQS) or (len(latest_reqs) + open_reqs < MAX_REQS)):
-                if assignment_to_closure_days is not None:
-                    assignment_to_closure_days = int(math.ceil(assignment_to_closure_days))
-                latest_reqs.append((req_pk, doc, deadline, review_state_by_slug.get(state), assignment_to_closure_days))
+            if ((d.state in ("requested", "accepted") and len(latest_reqs) < MAX_REQS) or (len(latest_reqs) + open_reqs < MAX_REQS)):
+                latest_reqs.append((d.req_pk, d.doc, d.deadline,
+                                    review_state_by_slug.get(d.state),
+                                    int(math.ceil(d.assignment_to_closure_days)) if d.assignment_to_closure_days is not None else None))
         person.latest_reqs = latest_reqs
 
     return render(request, 'group/reviewer_overview.html',

ietf/name/migrations/0015_insert_review_name_data.py

@@ -10,7 +10,7 @@ def insert_initial_review_data(apps, schema_editor):
     ReviewRequestStateName.objects.get_or_create(slug="accepted", name="Accepted", order=2)
     ReviewRequestStateName.objects.get_or_create(slug="rejected", name="Rejected", order=3)
     ReviewRequestStateName.objects.get_or_create(slug="withdrawn", name="Withdrawn", order=4)
-    ReviewRequestStateName.objects.get_or_create(slug="overtaken", name="Overtaken By Events", order=5)
+    ReviewRequestStateName.objects.get_or_create(slug="overtaken", name="Overtaken by Events", order=5)
     ReviewRequestStateName.objects.get_or_create(slug="no-response", name="No Response", order=6)
     ReviewRequestStateName.objects.get_or_create(slug="no-review-version", name="Team Will not Review Version", order=7)
     ReviewRequestStateName.objects.get_or_create(slug="no-review-document", name="Team Will not Review Document", order=8)

ietf/review/utils.py

@@ -1,5 +1,5 @@
 import datetime, re, itertools
-from collections import defaultdict
+from collections import defaultdict, namedtuple
 
 from django.db.models import Q, Max, F
 from django.core.urlresolvers import reverse as urlreverse
@@ -142,8 +142,15 @@ def days_needed_to_fulfill_min_interval_for_reviewers(team):
 
     return res
 
-def extract_review_request_data(teams=None, reviewers=None, time_from=None, time_to=None):
-    """Returns a dict keyed on (team.pk, reviewer_person.pk) which lists data on each review request."""
+ReviewRequestData = namedtuple("ReviewRequestData", [
+    "req_pk", "doc", "doc_pages", "req_time", "state", "deadline", "result", "team", "reviewer",
+    "late_days",
+    "request_to_assignment_days", "assignment_to_closure_days", "request_to_closure_days"])
+
+
+def extract_review_request_data(teams=None, reviewers=None, time_from=None, time_to=None, ordering=[]):
+    """Yield data on each review request, sorted by (*ordering, time)
+    for easy use with itertools.groupby. Valid entries in *ordering are "team" and "reviewer"."""
 
     filters = Q()
 
@@ -159,13 +166,16 @@ def extract_review_request_data(teams=None, reviewers=None, time_from=None, time
     if time_to:
         filters &= Q(time__lte=time_to)
 
-    res = defaultdict(list)
-
     # we may be dealing with a big bunch of data, so treat it carefully
     event_qs = ReviewRequest.objects.filter(filters)
 
     # left outer join with RequestRequestDocEvent for request/assign/close time
-    event_qs = event_qs.values_list("pk", "doc", "time", "state", "deadline", "result", "team", "reviewer__person", "reviewrequestdocevent__time", "reviewrequestdocevent__type").order_by("-time", "-pk", "-reviewrequestdocevent__time")
+    event_qs = event_qs.values_list(
+        "pk", "doc", "doc__pages", "time", "state", "deadline", "result", "team",
+        "reviewer__person", "reviewrequestdocevent__time", "reviewrequestdocevent__type"
+    )
+
+    event_qs = event_qs.order_by(*[o.replace("reviewer", "reviewer__person") for o in ordering] + ["time", "pk", "-reviewrequestdocevent__time"])
 
     def positive_days(time_from, time_to):
         if time_from is None or time_to is None:
@@ -182,7 +192,7 @@ def extract_review_request_data(teams=None, reviewers=None, time_from=None, time
         requested_time = assigned_time = closed_time = None
 
         for e in events:
-            req_pk, doc, req_time, state, deadline, result, team, reviewer, event_time, event_type = e
+            req_pk, doc, doc_pages, req_time, state, deadline, result, team, reviewer, event_time, event_type = e
 
             if event_type == "requested_review" and requested_time is None:
                 requested_time = event_time
@@ -196,12 +206,58 @@ def extract_review_request_data(teams=None, reviewers=None, time_from=None, time
         assignment_to_closure_days = positive_days(assigned_time, closed_time)
         request_to_closure_days = positive_days(requested_time, closed_time)
 
-        res[(team, reviewer)].append((req_pk, doc, req_time, state, deadline, result,
+        d = ReviewRequestData(req_pk, doc, doc_pages, req_time, state, deadline, result, team, reviewer,
                               late_days, request_to_assignment_days, assignment_to_closure_days,
-                                      request_to_closure_days))
+                              request_to_closure_days)
+
+        yield d
+
+def aggregate_review_request_stats(review_request_data, count=None):
+    """Take a sequence of review request data from
+    extract_review_request_data and compute aggregated statistics."""
+
+    state_dict = defaultdict(int)
+    late_state_dict = defaultdict(int)
+    result_dict = defaultdict(int)
+    assignment_to_closure_days_list = []
+    assignment_to_closure_days_count = 0
+
+    for (req_pk, doc, doc_pages, req_time, state, deadline, result, team, reviewer,
+         late_days, request_to_assignment_days, assignment_to_closure_days, request_to_closure_days) in review_request_data:
+        if count == "pages":
+            c = doc_pages
+        else:
+            c = 1
+
+        state_dict[state] += c
+
+        if late_days is not None and late_days > 0:
+            late_state_dict[state] += c
+
+        if state in ("completed", "part-completed"):
+            result_dict[result] += c
+            if assignment_to_closure_days is not None:
+                assignment_to_closure_days_list.append(assignment_to_closure_days)
+                assignment_to_closure_days_count += c
+
+    res = {}
+    res["state"] = state_dict
+    res["result"] = result_dict
+
+    res["open"] = sum(state_dict.get(s, 0) for s in ("requested", "accepted"))
+    res["completed"] = sum(state_dict.get(s, 0) for s in ("completed", "part-completed"))
+    res["not_completed"] = sum(state_dict.get(s, 0) for s in state_dict if s in ("rejected", "withdrawn", "overtaken", "no-response"))
+
+    res["open_late"] = sum(late_state_dict.get(s, 0) for s in ("requested", "accepted"))
+    res["open_in_time"] = res["open"] - res["open_late"]
+    res["completed_late"] = sum(late_state_dict.get(s, 0) for s in ("completed", "part-completed"))
+    res["completed_in_time"] = res["completed"] - res["completed_late"]
+
+    res["average_assignment_to_closure_days"] = float(sum(assignment_to_closure_days_list)) / (assignment_to_closure_days_count or 1) if assignment_to_closure_days_list else None
 
     return res
 
+
 def make_new_review_request_from_existing(review_req):
     obj = ReviewRequest()
     obj.time = review_req.time
@@ -215,6 +271,7 @@ def make_new_review_request_from_existing(review_req):
     return obj
 
 def email_review_request_change(request, review_req, subject, msg, by, notify_secretary, notify_reviewer, notify_requested_by):
+
     """Notify stakeholders about change, skipping a party if the change
     was done by that party."""
 

ietf/static/ietf/css/ietf.css

@@ -535,6 +535,43 @@ table.simple-table td:last-child {
   opacity: 0.6;
 }
 
+/* === Statistics =========================================================== */
+
+.stats-options > * {
+  margin-bottom: 1em;
+}
+
+.stats-options > *:last-child {
+  margin-bottom: 0;
+}
+
+.stats-options .date-range input.form-control {
+  display: inline-block;
+  width: 7em;
+}
+
+.stats-time-graph {
+  height: 15em;
+}
+
+.review-stats th:first-child, .review-stats td:first-child {
+ text-align: left;
+}
+
+.review-stats th, .review-stats td {
+  text-align: center;
+}
+
+.review-stats-teams {
+  -moz-column-width: 18em;
+  -webkit-column-width: 18em;
+  column-width: 18em;
+}
+
+.review-stats-teams a {
+  display: block;
+}
+
 /* === Photo pages ========================================================== */
 
 .photo-name {

ietf/static/ietf/js/review-stats.js

@@ -0,0 +1,8 @@
+$(document).ready(function () {
+    if (window.timeSeriesData && window.timeSeriesOptions) {
+        var placeholder = $(".stats-time-graph");
+        placeholder.height(Math.round(placeholder.width() * 1 / 3));
+
+        $.plot(placeholder, window.timeSeriesData, window.timeSeriesOptions);
+    }
+});

ietf/templates/base/menu.html

@@ -102,6 +102,7 @@
   <li><a href="/ipr/">IPR disclosures</a></li>
   <li><a href="/liaison/">Liaison statements</a></li>
   <li><a href="/iesg/agenda/">IESG agenda</a></li>
+  <li><a href="{% url "ietf.stats.views.stats_index" %}">Statistics</a></li>
   <li><a href="/group/edu/materials/">Tutorials</a></li>
   {% if flavor == "top" %}<li class="divider hidden-xs"></li>{% endif %}
   <li><a href="https://tools.ietf.org/tools/ietfdb/newticket"><span class="fa fa-bug"></span> Report a bug</a></li>

ietf/templates/stats/index.html

@@ -0,0 +1,18 @@
+{% extends "base.html" %}
+
+{% load origin %}{% origin %}
+
+{% load ietf_filters staticfiles bootstrap3 %}
+
+{% block content %}
+  {% origin %}
+
+  <h1>{% block title %}Statistics{% endblock %}</h1>
+
+  <p>Currently, there are statistics for:</p>
+
+  <ul>
+    <li><a rel="nofollow" href="{% url "ietf.stats.views.review_stats" %}">Reviews in review teams</a> (requires login)</li>
+  </ul>
+
+{% endblock %}

ietf/templates/stats/review_stats.html

@@ -0,0 +1,260 @@
+{% extends "base.html" %}
+
+{% load origin %}{% origin %}
+
+{% load ietf_filters staticfiles bootstrap3 %}
+
+{% block pagehead %}
+  <link rel="stylesheet" href="{% static 'bootstrap-datepicker/css/bootstrap-datepicker3.min.css' %}">
+{% endblock %}
+
+{% block content %}
+  {% origin %}
+
+  <h1>
+    {% block title %}
+      {% if level == "team" %}
+        Statistics for review teams
+      {% elif level == "reviewer" %}
+        Statistics for reviewers in {{ reviewers_for_team.name }}
+      {% endif %}
+    {% endblock %}
+  </h1>
+
+  {% if level == "reviewer" %}
+    <p><a href="{{ team_level_url }}">&laquo; Back to teams</a></p>
+  {% endif %}
+
+  <div class="stats-options well">
+    <div>
+      Show:
+      <div class="btn-group">
+        {% for slug, label, url in possible_stats_types %}
+          <a class="btn btn-default {% if slug == stats_type %}active{% endif %}" href="{{ url }}">{{ label }}</a>
+        {% endfor %}
+      </div>
+    </div>
+
+    <div>
+      Count:
+      <div class="btn-group">
+        {% for slug, label, url in possible_count_choices %}
+          <a class="btn btn-default {% if slug == count %}active{% endif %}" href="{{ url }}">{{ label }}</a>
+        {% endfor %}
+      </div>
+    </div>
+
+    <form class="form-inline date-range">
+      Request time:
+      
+      <input class="form-control" type="text" name="from" value="{{ from_date }}" data-provide="datepicker" data-date-format="yyyy-mm-dd" data-date-autoclose="1" data-date-end-date="{{ today.isoformat }}" data-date-start-view="months">
+      -
+      <input class="form-control" type="text" name="to" value="{{ to_date }}" data-provide="datepicker" data-date-format="yyyy-mm-dd" data-date-autoclose="1" data-date-end-date="{{ today.isoformat }}" data-date-start-view="months">
+
+      {% for name, value in request.GET.iteritems %}
+        {% if name != "from" and name != "to" %}
+          <input type="hidden" name="{{ name }}" value="{{ value }}">
+        {% endif %}
+      {% endfor %}
+      <button class="btn btn-default" type="submit">Set</button>
+    </form>
+
+
+    {% if stats_type == "time" %}
+      <hr>
+
+      <div>
+        Team:
+        <div class="btn-group">
+          {% for slug, label, url in possible_teams %}
+            <a class="btn btn-default {% if slug == selected_team %}active{% endif %}" href="{{ url }}">{{ label }}</a>
+          {% endfor %}
+        </div>
+      </div>
+
+      {% if selected_team %}
+        <div>
+          Completion:
+          <div class="btn-group">
+            {% for slug, label, url in possible_completion_types %}
+              <a class="btn btn-default {% if slug == selected_completion_type %}active{% endif %}" href="{{ url }}">{{ label }}</a>
+            {% endfor %}
+          </div>
+        </div>
+
+        <div>
+          Result:
+          <div class="btn-group">
+            {% for slug, label, url in possible_results %}
+              <a class="btn btn-default {% if slug == selected_result %}active{% endif %}" href="{{ url }}">{{ label }}</a>
+            {% endfor %}
+          </div>
+        </div>
+
+        <div>
+          State:
+          <div class="btn-group">
+            {% for slug, label, url in possible_states %}
+              <a class="btn btn-default {% if slug == selected_state %}active{% endif %}" href="{{ url }}">{{ label }}</a>
+            {% endfor %}
+          </div>
+        </div>
+      {% endif %}
+      
+    {% endif %}
+
+  </div>
+
+  {% if stats_type == "completion" %}
+
+    <h3>Completion status and completion time</h3>
+
+    <table class="review-stats table">
+      <thead>
+        <th>
+          {% if level == "team" %}
+            Team
+          {% elif level == "reviewer" %}
+            Reviewer
+          {% endif %}
+        </th>
+        <th title="Requests that are currently requested or accepted by reviewer">Open in time</th>
+        <th title="Requests that are currently requested or accepted by reviewer and past the deadline">Open late</th>
+        <th title="Requests that have been completed partially or completely">Completed in time</th>
+        <th title="Requests that have been completed partially or completely past the deadline">Completed late</th>
+        <th title="Requests that are rejected by the reviewer, withdrawn, overtaken by events or with no response from reviewer">Not completed</th>
+        <th title="Average time between assignment and completion for completed reviews, in days">Avg. compl. days{% if count == "pages" %}/page{% endif %}</th>
+      </thead>
+      <tbody>
+        {% for row in data %}
+          <tr>
+            <td>{{ row.obj }}</td>
+            <td>{{ row.open_in_time }}</td>
+            <td>{{ row.open_late }}</td>
+            <td>{{ row.completed_in_time }}</td>
+            <td>{{ row.completed_late }}</td>
+            <td>{{ row.not_completed }}</td>
+            <td>
+              {% if row.average_assignment_to_closure_days != None %}
+                {{ row.average_assignment_to_closure_days|floatformat }}
+              {% endif %}
+            </td>
+          </tr>
+        {% endfor %}
+      </tbody>
+    </table>
+
+  {% elif stats_type == "results" %}
+
+    <h3>Results of completed reviews</h3>
+
+    <table class="review-stats table">
+      <thead>
+        <th>
+          {% if level == "team" %}
+            Team
+          {% elif level == "reviewer" %}
+            Reviewer
+          {% endif %}
+        </th>
+        {% for r in results %}
+          <th>{{ r.name }}</th>
+        {% endfor %}
+      </thead>
+      <tbody>
+        {% for row in data %}
+          <tr>
+            <td>{{ row.obj }}</td>
+            {% for c in row.result_list %}
+              <td>{{ c }}</td>
+            {% endfor %}
+          </tr>
+        {% endfor %}
+      </tbody>
+    </table>
+
+  {% elif stats_type == "states" %}
+
+    <h3>Specific request states</h3>
+
+    <table class="review-stats table">
+      <thead>
+        <th>
+          {% if level == "team" %}
+            Team
+          {% elif level == "reviewer" %}
+            Reviewer
+          {% endif %}
+        </th>
+        {% for s in states %}
+          <th>{{ s.name }}</th>
+        {% endfor %}
+      </thead>
+      <tbody>
+        {% for row in data %}
+          <tr>
+            <td>{{ row.obj }}</td>
+            {% for c in row.state_list %}
+              <td>{{ c }}</td>
+            {% endfor %}
+          </tr>
+        {% endfor %}
+      </tbody>
+    </table>
+
+  {% elif stats_type == "time" and selected_team %}
+
+    <h3>Counts per month</h3>
+
+    <div class="stats-time-graph"></div>
+
+    <script>
+     var timeSeriesData = {{ data|safe }};
+     var timeSeriesOptions = {
+         xaxis: {
+             mode: "time",
+             tickLength: 0
+         },
+         yaxis: {
+             tickDecimals: {% if selected_completion_type == "average_assignment_to_closure_days" %}null{% else %}0{% endif %}
+         },
+         series: {
+             color: "#3d22b3",
+             bars: {
+                 show: true,
+                 barWidth: 20 * 24 * 60 * 60 * 1000,
+                 align: "center",
+                 lineWidth: 1,
+                 fill: 0.6
+             }
+         }
+     };
+    </script>
+
+  {% endif %}
+
+  {% if stats_type != "time" %}
+    <p class="text-muted text-right">Note: {% if level == "team" %}teams{% elif level == "reviewer" %}reviewers{% endif %}
+      with no requests in the period are omitted.</p>
+  {% endif %}
+
+  {% if level == "team" and stats_type != "time" %}
+    <p>Statistics for individual reviewers:</p>
+
+    <div class="review-stats-teams">
+      {% for t in teams %}
+        <a href="{{ t.reviewer_stats_url }}">{{ t.name }}</a>
+      {% endfor %}
+    </div>
+  {% endif %}
+
+{% endblock %}
+
+{% block js %}
+  <script src="{% static 'bootstrap-datepicker/js/bootstrap-datepicker.min.js' %}"></script>
+  {% if stats_type == "time" %}
+    <script src="{% static 'flot/jquery.flot.min.js' %}"></script>
+    <script src="{% static 'flot/jquery.flot.time.min.js' %}"></script>
+    <script src="{% static 'ietf/js/review-stats.js' %}"></script>
+  {% endif %}
+{% endblock %}

ietf/urls.py

@@ -54,9 +54,10 @@ urlpatterns = patterns('',
     (r'^secr/', include('ietf.secr.urls')),
     (r'^sitemap-(?P<section>.+).xml$', 'django.contrib.sitemaps.views.sitemap', {'sitemaps': sitemaps}),
     (r'^sitemap.xml$', 'django.contrib.sitemaps.views.index', { 'sitemaps': sitemaps}),
+    (r'^stats/', include('ietf.stats.urls')),
+    (r'^stream/', include('ietf.group.urls_stream')),
     (r'^submit/', include('ietf.submit.urls')),
     (r'^sync/', include('ietf.sync.urls')),
-    (r'^stream/', include('ietf.group.urls_stream')),
     (r'^templates/', include('ietf.dbtemplate.urls')),
     (r'^(?P<group_type>(wg|rg|ag|team|dir|area))/', include('ietf.group.urls_info')),