From 59d7855a2d01d858a42947167a9705751ef762b9 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Wed, 12 Oct 2016 17:20:31 +1100 Subject: [PATCH 01/50] Move public body statistics to generic statistics page --- app/controllers/general_controller.rb | 119 +++++++++++++++++ app/controllers/public_body_controller.rb | 115 ----------------- .../statistics.html.erb | 2 +- .../public_body/_list_sidebar_extra.html.erb | 2 +- config/routes.rb | 5 +- spec/controllers/general_controller_spec.rb | 119 +++++++++++++++++ .../public_body_controller_spec.rb | 122 ------------------ 7 files changed, 242 insertions(+), 242 deletions(-) rename app/views/{public_body => general}/statistics.html.erb (98%) diff --git a/app/controllers/general_controller.rb b/app/controllers/general_controller.rb index 8c6ced0e046..030a3a42bcf 100644 --- a/app/controllers/general_controller.rb +++ b/app/controllers/general_controller.rb @@ -7,6 +7,7 @@ # Email: hello@mysociety.org; WWW: http://www.mysociety.org/ require 'open-uri' +require 'confidence_intervals' class GeneralController < ApplicationController @@ -233,4 +234,122 @@ def version }} end end + + def statistics + unless AlaveteliConfiguration::public_body_statistics_page + raise ActiveRecord::RecordNotFound.new("Page not enabled") + end + + per_graph = 10 + minimum_requests = AlaveteliConfiguration::minimum_requests_for_statistics + # Make sure minimum_requests is > 0 to avoid division-by-zero + minimum_requests = [minimum_requests, 1].max + total_column = 'info_requests_count' + + @graph_list = [] + + [ + [ + total_column, + [{:title => _('Public bodies with the most requests'), + :y_axis => _('Number of requests'), + :highest => true}] + ], + [ + 'info_requests_successful_count', + [ + {:title => _('Public bodies with the most successful requests'), + :y_axis => _('Percentage of total requests'), + :highest => true}, + {:title => _('Public bodies with the fewest successful requests'), + :y_axis => _('Percentage of total requests'), + :highest => false} + ] + ], + [ + 'info_requests_overdue_count', + [{:title => _('Public bodies with most overdue requests'), + :y_axis => _('Percentage of requests that are overdue'), + :highest => true}] + ], + [ + 'info_requests_not_held_count', + [{:title => _('Public bodies that most frequently replied with "Not Held"'), + :y_axis => _('Percentage of total requests'), + :highest => true}] + ] + ].each do |column, graphs_properties| + graphs_properties.each do |graph_properties| + percentages = (column != total_column) + highest = graph_properties[:highest] + + data = nil + if percentages + data = PublicBody.get_request_percentages(column, + per_graph, + highest, + minimum_requests) + else + data = PublicBody.get_request_totals(per_graph, + highest, + minimum_requests) + end + + if data + @graph_list.push simplify_stats_for_graphs(data, + column, + percentages, + graph_properties) + end + end + end + + respond_to do |format| + format.html + format.json { render :json => @graph_list } + end + end + + # This is a helper method to take data returned by the PublicBody + # model's statistics-generating methods, and converting them to + # simpler data structure that can be rendered by a Javascript + # graph library. (This could be a class method except that we need + # access to the URL helper public_body_path.) + def simplify_stats_for_graphs(data, + column, + percentages, + graph_properties) + # Copy the data, only taking known-to-be-safe keys: + result = Hash.new { |h, k| h[k] = [] } + result.update Hash[data.select do |key, value| + ['y_values', + 'y_max', + 'totals', + 'cis_below', + 'cis_above'].include? key + end] + + # Extract data about the public bodies for the x-axis, + # tooltips, and so on: + data['public_bodies'].each_with_index do |pb, i| + result['x_values'] << i + result['x_ticks'] << [i, pb.name] + result['tooltips'] << "#{pb.name} (#{result['totals'][i]})" + result['public_bodies'] << { + 'name' => pb.name, + 'url' => public_body_path(pb) + } + end + + # Set graph metadata properties, like the title, axis labels, etc. + graph_id = "#{column}-" + graph_id += graph_properties[:highest] ? 'highest' : 'lowest' + result.update({ + 'id' => graph_id, + 'x_axis' => _('Public Bodies'), + 'y_axis' => graph_properties[:y_axis], + 'errorbars' => percentages, + 'title' => graph_properties[:title] + }) + end end diff --git a/app/controllers/public_body_controller.rb b/app/controllers/public_body_controller.rb index 052862c6aa9..8fcf57960ef 100644 --- a/app/controllers/public_body_controller.rb +++ b/app/controllers/public_body_controller.rb @@ -5,7 +5,6 @@ # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved. # Email: hello@mysociety.org; WWW: http://www.mysociety.org/ -require 'confidence_intervals' require 'tempfile' class PublicBodyController < ApplicationController @@ -298,120 +297,6 @@ def list_all_csv :encoding => 'utf8') end - - # This is a helper method to take data returned by the PublicBody - # model's statistics-generating methods, and converting them to - # simpler data structure that can be rendered by a Javascript - # graph library. (This could be a class method except that we need - # access to the URL helper public_body_path.) - def simplify_stats_for_graphs(data, - column, - percentages, - graph_properties) - # Copy the data, only taking known-to-be-safe keys: - result = Hash.new { |h, k| h[k] = [] } - result.update Hash[data.select do |key, value| - ['y_values', - 'y_max', - 'totals', - 'cis_below', - 'cis_above'].include? key - end] - - # Extract data about the public bodies for the x-axis, - # tooltips, and so on: - data['public_bodies'].each_with_index do |pb, i| - result['x_values'] << i - result['x_ticks'] << [i, pb.name] - result['tooltips'] << "#{pb.name} (#{result['totals'][i]})" - result['public_bodies'] << { - 'name' => pb.name, - 'url' => public_body_path(pb) - } - end - - # Set graph metadata properties, like the title, axis labels, etc. - graph_id = "#{column}-" - graph_id += graph_properties[:highest] ? 'highest' : 'lowest' - result.update({ - 'id' => graph_id, - 'x_axis' => _('Public Bodies'), - 'y_axis' => graph_properties[:y_axis], - 'errorbars' => percentages, - 'title' => graph_properties[:title] - }) - end - - def statistics - unless AlaveteliConfiguration::public_body_statistics_page - raise ActiveRecord::RecordNotFound.new("Page not enabled") - end - - per_graph = 10 - minimum_requests = AlaveteliConfiguration::minimum_requests_for_statistics - # Make sure minimum_requests is > 0 to avoid division-by-zero - minimum_requests = [minimum_requests, 1].max - total_column = 'info_requests_count' - - @graph_list = [] - - [[total_column, - [{ - :title => _('Public bodies with the most requests'), - :y_axis => _('Number of requests'), - :highest => true}]], - ['info_requests_successful_count', - [{ - :title => _('Public bodies with the most successful requests'), - :y_axis => _('Percentage of total requests'), - :highest => true}, - { - :title => _('Public bodies with the fewest successful requests'), - :y_axis => _('Percentage of total requests'), - :highest => false}]], - ['info_requests_overdue_count', - [{ - :title => _('Public bodies with most overdue requests'), - :y_axis => _('Percentage of requests that are overdue'), - :highest => true}]], - ['info_requests_not_held_count', - [{ - :title => _('Public bodies that most frequently replied with "Not Held"'), - :y_axis => _('Percentage of total requests'), - :highest => true}]]].each do |column, graphs_properties| - - graphs_properties.each do |graph_properties| - - percentages = (column != total_column) - highest = graph_properties[:highest] - - data = nil - if percentages - data = PublicBody.get_request_percentages(column, - per_graph, - highest, - minimum_requests) - else - data = PublicBody.get_request_totals(per_graph, - highest, - minimum_requests) - end - - if data - @graph_list.push simplify_stats_for_graphs(data, - column, - percentages, - graph_properties) - end - end - end - - respond_to do |format| - format.html { render :template => "public_body/statistics" } - format.json { render :json => @graph_list } - end - end - # Type ahead search def search_typeahead # Since acts_as_xapian doesn't support the Partial match flag, we work around it diff --git a/app/views/public_body/statistics.html.erb b/app/views/general/statistics.html.erb similarity index 98% rename from app/views/public_body/statistics.html.erb rename to app/views/general/statistics.html.erb index 50e62f59c70..b828d9054d3 100644 --- a/app/views/public_body/statistics.html.erb +++ b/app/views/general/statistics.html.erb @@ -1,6 +1,6 @@ <% @title = _("Public Body Statistics") %>
-

<%= @title %>

+

<%= @title %>

<%= _("This page of public body statistics is currently experimental, " \ "so there are some caveats that should be borne in mind:") %>

diff --git a/app/views/public_body/_list_sidebar_extra.html.erb b/app/views/public_body/_list_sidebar_extra.html.erb index 961040537d5..3bf97180146 100644 --- a/app/views/public_body/_list_sidebar_extra.html.erb +++ b/app/views/public_body/_list_sidebar_extra.html.erb @@ -1,6 +1,6 @@ <% if AlaveteliConfiguration::public_body_statistics_page %>

- <%= link_to _('Public authority statistics'), public_bodies_statistics_path %> + <%= link_to _('Public authority statistics'), statistics_path(:anchor => 'public_bodies') %>

<% end %>

diff --git a/config/routes.rb b/config/routes.rb index 3eb35fcdcb9..01a9f17662b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -42,6 +42,8 @@ match '/version.:format' => 'general#version', :as => :version, :via => :get + get '/statistics' => 'general#statistics' + get '/body_statistics' => redirect('/statistics#public_bodies'), :as => :public_bodies_statistics ##### ##### Request controller @@ -284,9 +286,6 @@ match '/body/:url_name/:tag/:view' => 'public_body#show', :as => :show_public_body_tag_view, :via => :get - match '/body_statistics' => 'public_body#statistics', - :as => :public_bodies_statistics, - :via => :get #### resource :change_request, :only => [:new, :create], :controller => 'public_body_change_requests' diff --git a/spec/controllers/general_controller_spec.rb b/spec/controllers/general_controller_spec.rb index 3da3050b91f..b82f8054a23 100644 --- a/spec/controllers/general_controller_spec.rb +++ b/spec/controllers/general_controller_spec.rb @@ -410,5 +410,124 @@ end end +end + +describe GeneralController, "when showing public body statistics" do + + it "should render the right template with the right data" do + config = MySociety::Config.load_default + config['MINIMUM_REQUESTS_FOR_STATISTICS'] = 1 + config['PUBLIC_BODY_STATISTICS_PAGE'] = true + get :statistics + expect(response).to render_template('general/statistics') + # There are 5 different graphs we're creating at the moment. + expect(assigns[:graph_list].length).to eq(5) + # The first is the only one with raw values, the rest are + # percentages with error bars: + assigns[:graph_list].each_with_index do |graph, index| + if index == 0 + expect(graph['errorbars']).to be false + expect(graph['x_values'].length).to eq(4) + expect(graph['x_values']).to eq([0, 1, 2, 3]) + expect(graph['y_values']).to eq([1, 2, 2, 4]) + else + expect(graph['errorbars']).to be true + # Just check the first one: + if index == 1 + expect(graph['x_values']).to eq([0, 1, 2, 3]) + expect(graph['y_values']).to eq([0, 50, 100, 100]) + end + # Check that at least every confidence interval value is + # a Float (rather than NilClass, say): + graph['cis_below'].each { |v| expect(v).to be_instance_of(Float) } + graph['cis_above'].each { |v| expect(v).to be_instance_of(Float) } + end + end + end end + +describe GeneralController, "when converting data for graphing" do + + before(:each) do + @raw_count_data = PublicBody.get_request_totals(n=3, + highest=true, + minimum_requests=1) + @percentages_data = PublicBody.get_request_percentages( + column='info_requests_successful_count', + n=3, + highest=false, + minimum_requests=1) + end + + it "should not include the real public body model instance" do + to_draw = controller.simplify_stats_for_graphs(@raw_count_data, + column='blah_blah', + percentages=false, + {} ) + expect(to_draw['public_bodies'][0].class).to eq(Hash) + expect(to_draw['public_bodies'][0].has_key?('request_email')).to be false + end + + it "should generate the expected id" do + to_draw = controller.simplify_stats_for_graphs(@raw_count_data, + column='blah_blah', + percentages=false, + {:highest => true} ) + expect(to_draw['id']).to eq("blah_blah-highest") + to_draw = controller.simplify_stats_for_graphs(@raw_count_data, + column='blah_blah', + percentages=false, + {:highest => false} ) + expect(to_draw['id']).to eq("blah_blah-lowest") + end + + it "should have exactly the expected keys" do + to_draw = controller.simplify_stats_for_graphs(@raw_count_data, + column='blah_blah', + percentages=false, + {} ) + expect(to_draw.keys.sort).to eq(["errorbars", "id", "public_bodies", + "title", "tooltips", "totals", + "x_axis", "x_ticks", "x_values", + "y_axis", "y_max", "y_values"]) + + to_draw = controller.simplify_stats_for_graphs(@percentages_data, + column='whatever', + percentages=true, + {}) + expect(to_draw.keys.sort).to eq(["cis_above", "cis_below", + "errorbars", "id", "public_bodies", + "title", "tooltips", "totals", + "x_axis", "x_ticks", "x_values", + "y_axis", "y_max", "y_values"]) + end + + it "should have values of the expected class and length" do + [controller.simplify_stats_for_graphs(@raw_count_data, + column='blah_blah', + percentages=false, + {}), + controller.simplify_stats_for_graphs(@percentages_data, + column='whatever', + percentages=true, + {})].each do |to_draw| + per_pb_keys = ["cis_above", "cis_below", "public_bodies", + "tooltips", "totals", "x_ticks", "x_values", + "y_values"] + # These should be all be arrays with one element per public body: + per_pb_keys.each do |key| + if to_draw.has_key? key + expect(to_draw[key].class).to eq(Array) + expect(to_draw[key].length).to eq(3), "for key #{key}" + end + end + # Just check that the rest aren't of class Array: + to_draw.keys.each do |key| + unless per_pb_keys.include? key + expect(to_draw[key].class).not_to eq(Array), "for key #{key}" + end + end + end + end +end diff --git a/spec/controllers/public_body_controller_spec.rb b/spec/controllers/public_body_controller_spec.rb index 8d099529b6d..2e8b5df4ccb 100644 --- a/spec/controllers/public_body_controller_spec.rb +++ b/spec/controllers/public_body_controller_spec.rb @@ -411,128 +411,6 @@ def make_single_language_example(locale) end -describe PublicBodyController, "when showing public body statistics" do - - it "should render the right template with the right data" do - config = MySociety::Config.load_default - config['MINIMUM_REQUESTS_FOR_STATISTICS'] = 1 - config['PUBLIC_BODY_STATISTICS_PAGE'] = true - get :statistics - expect(response).to render_template('public_body/statistics') - # There are 5 different graphs we're creating at the moment. - expect(assigns[:graph_list].length).to eq(5) - # The first is the only one with raw values, the rest are - # percentages with error bars: - assigns[:graph_list].each_with_index do |graph, index| - if index == 0 - expect(graph['errorbars']).to be false - expect(graph['x_values'].length).to eq(4) - expect(graph['x_values']).to eq([0, 1, 2, 3]) - expect(graph['y_values']).to eq([1, 2, 2, 4]) - else - expect(graph['errorbars']).to be true - # Just check the first one: - if index == 1 - expect(graph['x_values']).to eq([0, 1, 2, 3]) - expect(graph['y_values']).to eq([0, 50, 100, 100]) - end - # Check that at least every confidence interval value is - # a Float (rather than NilClass, say): - graph['cis_below'].each { |v| expect(v).to be_instance_of(Float) } - graph['cis_above'].each { |v| expect(v).to be_instance_of(Float) } - end - end - end - -end - -describe PublicBodyController, "when converting data for graphing" do - - before(:each) do - @raw_count_data = PublicBody.get_request_totals(n=3, - highest=true, - minimum_requests=1) - @percentages_data = PublicBody.get_request_percentages( - column='info_requests_successful_count', - n=3, - highest=false, - minimum_requests=1) - end - - it "should not include the real public body model instance" do - to_draw = controller.simplify_stats_for_graphs(@raw_count_data, - column='blah_blah', - percentages=false, - {} ) - expect(to_draw['public_bodies'][0].class).to eq(Hash) - expect(to_draw['public_bodies'][0].has_key?('request_email')).to be false - end - - it "should generate the expected id" do - to_draw = controller.simplify_stats_for_graphs(@raw_count_data, - column='blah_blah', - percentages=false, - {:highest => true} ) - expect(to_draw['id']).to eq("blah_blah-highest") - to_draw = controller.simplify_stats_for_graphs(@raw_count_data, - column='blah_blah', - percentages=false, - {:highest => false} ) - expect(to_draw['id']).to eq("blah_blah-lowest") - end - - it "should have exactly the expected keys" do - to_draw = controller.simplify_stats_for_graphs(@raw_count_data, - column='blah_blah', - percentages=false, - {} ) - expect(to_draw.keys.sort).to eq(["errorbars", "id", "public_bodies", - "title", "tooltips", "totals", - "x_axis", "x_ticks", "x_values", - "y_axis", "y_max", "y_values"]) - - to_draw = controller.simplify_stats_for_graphs(@percentages_data, - column='whatever', - percentages=true, - {}) - expect(to_draw.keys.sort).to eq(["cis_above", "cis_below", - "errorbars", "id", "public_bodies", - "title", "tooltips", "totals", - "x_axis", "x_ticks", "x_values", - "y_axis", "y_max", "y_values"]) - end - - it "should have values of the expected class and length" do - [controller.simplify_stats_for_graphs(@raw_count_data, - column='blah_blah', - percentages=false, - {}), - controller.simplify_stats_for_graphs(@percentages_data, - column='whatever', - percentages=true, - {})].each do |to_draw| - per_pb_keys = ["cis_above", "cis_below", "public_bodies", - "tooltips", "totals", "x_ticks", "x_values", - "y_values"] - # These should be all be arrays with one element per public body: - per_pb_keys.each do |key| - if to_draw.has_key? key - expect(to_draw[key].class).to eq(Array) - expect(to_draw[key].length).to eq(3), "for key #{key}" - end - end - # Just check that the rest aren't of class Array: - to_draw.keys.each do |key| - unless per_pb_keys.include? key - expect(to_draw[key].class).not_to eq(Array), "for key #{key}" - end - end - end - end - -end - - describe PublicBodyController, "when doing type ahead searches" do render_views From a3028ea58320493c6e8b2b994c17b6db69b9fe78 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Thu, 13 Oct 2016 11:38:19 +1100 Subject: [PATCH 02/50] Incorporate Humans of Right To Know experiment onto statistics page --- .../responsive/_public_body_stats_layout.scss | 24 ------ .../responsive/_statistics_layout.scss | 82 +++++++++++++++++++ ...tats_style.scss => _statistics_style.scss} | 15 +++- app/assets/stylesheets/responsive/all.scss | 4 +- app/controllers/general_controller.rb | 20 ++++- app/models/user.rb | 44 ++++++++++ .../general/_people_leaderboard.html.erb | 19 +++++ app/views/general/statistics.html.erb | 33 +++++++- 8 files changed, 211 insertions(+), 30 deletions(-) delete mode 100644 app/assets/stylesheets/responsive/_public_body_stats_layout.scss create mode 100644 app/assets/stylesheets/responsive/_statistics_layout.scss rename app/assets/stylesheets/responsive/{_public_body_stats_style.scss => _statistics_style.scss} (68%) create mode 100644 app/views/general/_people_leaderboard.html.erb diff --git a/app/assets/stylesheets/responsive/_public_body_stats_layout.scss b/app/assets/stylesheets/responsive/_public_body_stats_layout.scss deleted file mode 100644 index deaf25f12ff..00000000000 --- a/app/assets/stylesheets/responsive/_public_body_stats_layout.scss +++ /dev/null @@ -1,24 +0,0 @@ -/* Layout for public body stats page */ -.public-body-ranking { - margin-bottom: 40px; -} - -.public-body-ranking-title { - margin-top: 25px; - margin-bottom: 10px; -} - -.public-body-ranking table { - margin-top: 20px; - margin-left: 30px; -} - -.public-body-ranking td, th { - border: 0px; - padding: 5px; - padding-right: 20px; -} - -.public-body-ranking td.statistic { - text-align: center; -} diff --git a/app/assets/stylesheets/responsive/_statistics_layout.scss b/app/assets/stylesheets/responsive/_statistics_layout.scss new file mode 100644 index 00000000000..cd5ed2e5293 --- /dev/null +++ b/app/assets/stylesheets/responsive/_statistics_layout.scss @@ -0,0 +1,82 @@ +/* Layout for statistics page */ +.public-body-ranking { + margin-bottom: 40px; +} + +.public-body-ranking-title { + margin-top: 25px; + margin-bottom: 10px; +} + +.public-body-ranking table { + margin-top: 20px; + margin-left: 30px; +} + +.public-body-ranking td, th { + border: 0px; + padding: 5px; + padding-right: 20px; +} + +.public-body-ranking td.statistic { + text-align: center; +} + +.leaderboard-block { + margin: 0 0 4em; + @include respond-min( 35em ) { + display: flex; + justify-content: space-between; + } +} + +.leaderboard-item { + @include respond-min( 35em ) { + width: 45%; + } +} + +.leaderboard-people { + padding: 0; +} + +.leaderboard-person { + margin: 0 0 1em; + display: flex; + justify-content: space-between; + align-items: flex-start; + + img, + .leaderboard-person-initial { + margin: 0 1rem 0 0; + width: 3rem; + height: 3rem; + } + + .u-name { + font-size: 1.125em; + margin: 0 0 .5em; + } +} + +.leaderboard-person-initial { + padding: .6rem; + font-size: 1.5em; +} + +a.leaderboard-person-details { + display: flex; + justify-content: flex-start; + align-items: flex-start; + + p { + margin: 0; + } +} + +.leaderboard-count { + margin: 0; + font-size: 1.125em; + padding: 0 0 1em 1em; +} diff --git a/app/assets/stylesheets/responsive/_public_body_stats_style.scss b/app/assets/stylesheets/responsive/_statistics_style.scss similarity index 68% rename from app/assets/stylesheets/responsive/_public_body_stats_style.scss rename to app/assets/stylesheets/responsive/_statistics_style.scss index ea3f6f3d564..6c8e7dfda96 100644 --- a/app/assets/stylesheets/responsive/_public_body_stats_style.scss +++ b/app/assets/stylesheets/responsive/_statistics_style.scss @@ -1,4 +1,4 @@ -/* Style for public body stats page */ +/* Style for statistics page */ .public-body-ranking .axisLabels { /* Justification for using !important here: the axis label color is set in the style attribute in Flot's Javascript to the same @@ -9,3 +9,16 @@ the axes black rather than transparent grey for the moment: */ color: #000 !important; } + +.leaderboard-people { + list-style: none; +} + +.leaderboard-person-initial { + text-align: center; + background: lighten(desaturate(#2688dc, 50), 40); +} + +a.leaderboard-person-details { + color: #333; +} diff --git a/app/assets/stylesheets/responsive/all.scss b/app/assets/stylesheets/responsive/all.scss index 9edcfb64752..e64686db88e 100644 --- a/app/assets/stylesheets/responsive/all.scss +++ b/app/assets/stylesheets/responsive/all.scss @@ -62,8 +62,8 @@ @import "_public_body_layout"; @import "_public_body_style"; -@import "_public_body_stats_layout"; -@import "_public_body_stats_style"; +@import "_statistics_layout"; +@import "_statistics_style"; @import "_sidebar_layout"; @import "_sidebar_style"; diff --git a/app/controllers/general_controller.rb b/app/controllers/general_controller.rb index 030a3a42bcf..078802f40f5 100644 --- a/app/controllers/general_controller.rb +++ b/app/controllers/general_controller.rb @@ -304,9 +304,21 @@ def statistics end end + @all_time_requesters = User.all_time_requesters + @last_28_day_requesters = User.last_28_day_requesters + @all_time_commenters = User.all_time_commenters + @last_28_day_commenters = User.last_28_day_commenters + + users = { + all_time_requesters: json_for_api(@all_time_requesters), + last_28_day_requesters: json_for_api(@last_28_day_requesters), + all_time_commenters: json_for_api(@all_time_commenters), + last_28_day_commenters: json_for_api(@last_28_day_commenters) + } + respond_to do |format| format.html - format.json { render :json => @graph_list } + format.json { render :json => { public_bodies: @graph_list, users: users } } end end @@ -352,4 +364,10 @@ def simplify_stats_for_graphs(data, 'title' => graph_properties[:title] }) end + + private + + def json_for_api(data) + data.map { |u,c| { user: u.json_for_api, count: c } } + end end diff --git a/app/models/user.rb b/app/models/user.rb index b2a9a6a6677..3dec62f673f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -213,6 +213,50 @@ def self.find_similar_named_users(user) user.name, true, user.id).order(:created_at) end + def self.all_time_requesters + InfoRequest.visible. + joins(:user). + group(:user). + order("count_all DESC"). + limit(10). + count + end + + def self.last_28_day_requesters + # TODO: Refactor as it's basically the same as all_time_requesters + InfoRequest.visible. + where("info_requests.created_at >= ?", 28.days.ago). + joins(:user). + group(:user). + order("count_all DESC"). + limit(10). + count + end + + def self.all_time_commenters + commenters = Comment.visible. + joins(:user). + group("comments.user_id"). + order("count_all DESC"). + limit(10). + count + # TODO: Have user objects automatically instantiated like the InfoRequest queries above + commenters.map { |u_id,c| [User.find(u_id), c] } + end + + def self.last_28_day_commenters + # TODO: Refactor as it's basically the same as all_time_commenters + commenters = Comment.visible. + where("comments.created_at >= ?", 28.days.ago). + joins(:user). + group("comments.user_id"). + order("count_all DESC"). + limit(10). + count + # TODO: Have user objects automatically instantiated like the InfoRequest queries above + commenters.map { |u_id,c| [User.find(u_id), c] } + end + def transactions(*associations) opts = {} opts[:transaction_associations] = associations if associations.any? diff --git a/app/views/general/_people_leaderboard.html.erb b/app/views/general/_people_leaderboard.html.erb new file mode 100644 index 00000000000..782a6501498 --- /dev/null +++ b/app/views/general/_people_leaderboard.html.erb @@ -0,0 +1,19 @@ +

    +<% users_with_counts.each do |user, count| %> +
  1. + <%= link_to user, class: "leaderboard-person-details" do %> + <% if user.profile_photo %> + Avatar image for <%= user.name %> + <% else %> + <%= user.name.first.upcase %> + <% end %> +
    +

    <%= user.name %>

    +

    <%= _('Joined in {{year}}', :year => user.created_at.year) %>

    +
    + <% end %> +

    <%= count %>

    +
    2. +<% end %> +
+ diff --git a/app/views/general/statistics.html.erb b/app/views/general/statistics.html.erb index b828d9054d3..1ca9bec0ecf 100644 --- a/app/views/general/statistics.html.erb +++ b/app/views/general/statistics.html.erb @@ -1,6 +1,7 @@ -<% @title = _("Public Body Statistics") %> +<% @title = _("Statistics") %>
-

<%= @title %>

+

<%= @title %>

+

Public Bodies

<%= _("This page of public body statistics is currently experimental, " \ "so there are some caveats that should be borne in mind:") %>

@@ -65,6 +66,34 @@
<% end %> +

People

+ +

Most frequent requesters

+ +
+
+

Last 28 days

+ <%= render "people_leaderboard", users_with_counts: @last_28_day_requesters %> +
+
+

All time

+ <%= render "people_leaderboard", users_with_counts: @all_time_requesters %> +
+
+ +

Most frequent annotators

+ +
+
+

Last 28 days

+ <%= render "people_leaderboard", users_with_counts: @last_28_day_commenters %> +
+
+

All time

+ <%= render "people_leaderboard", users_with_counts: @all_time_commenters %> +
+
+ <% content_for :javascript do %> <%= javascript_include_tag "stats" %> From 40876c7a287277900629ff3ff346b9113591e44b Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Thu, 13 Oct 2016 16:35:02 +1100 Subject: [PATCH 05/50] Extract user statistic logic into the statistic model --- app/controllers/general_controller.rb | 26 +++++++------------------- app/models/statistics.rb | 15 +++++++++++++++ app/views/general/statistics.html.erb | 8 ++++---- 3 files changed, 26 insertions(+), 23 deletions(-) diff --git a/app/controllers/general_controller.rb b/app/controllers/general_controller.rb index ea1de9e4ecb..7715d03c0fe 100644 --- a/app/controllers/general_controller.rb +++ b/app/controllers/general_controller.rb @@ -241,28 +241,16 @@ def statistics end @public_bodies = Statistics.public_bodies - - @all_time_requesters = User.all_time_requesters - @last_28_day_requesters = User.last_28_day_requesters - @all_time_commenters = User.all_time_commenters - @last_28_day_commenters = User.last_28_day_commenters - - users = { - all_time_requesters: json_for_api(@all_time_requesters), - last_28_day_requesters: json_for_api(@last_28_day_requesters), - all_time_commenters: json_for_api(@all_time_commenters), - last_28_day_commenters: json_for_api(@last_28_day_commenters) - } + @users = Statistics.users respond_to do |format| format.html - format.json { render :json => { public_bodies: @public_bodies, users: users } } + format.json do + render json: { + public_bodies: @public_bodies, + users: Statistics.user_json_for_api(@users) + } + end end end - - private - - def json_for_api(data) - data.map { |u,c| { user: u.json_for_api, count: c } } - end end diff --git a/app/models/statistics.rb b/app/models/statistics.rb index f141e748fe9..0500a250354 100644 --- a/app/models/statistics.rb +++ b/app/models/statistics.rb @@ -110,5 +110,20 @@ def simplify_stats_for_graphs(data, 'title' => graph_properties[:title] }) end + + def users + { + all_time_requesters: User.all_time_requesters, + last_28_day_requesters: User.last_28_day_requesters, + all_time_commenters: User.all_time_commenters, + last_28_day_commenters: User.last_28_day_commenters + } + end + + def user_json_for_api(user_statistics) + user_statistics.each do |k,v| + user_statistics[k] = v.map { |u,c| { user: u.json_for_api, count: c } } + end + end end end diff --git a/app/views/general/statistics.html.erb b/app/views/general/statistics.html.erb index 44fa32db340..30104dde181 100644 --- a/app/views/general/statistics.html.erb +++ b/app/views/general/statistics.html.erb @@ -73,11 +73,11 @@

Last 28 days

- <%= render "people_leaderboard", users_with_counts: @last_28_day_requesters %> + <%= render "people_leaderboard", users_with_counts: @users[:last_28_day_requesters] %>

All time

- <%= render "people_leaderboard", users_with_counts: @all_time_requesters %> + <%= render "people_leaderboard", users_with_counts: @users[:all_time_requesters] %>
@@ -86,11 +86,11 @@

Last 28 days

- <%= render "people_leaderboard", users_with_counts: @last_28_day_commenters %> + <%= render "people_leaderboard", users_with_counts: @users[:last_28_day_commenters] %>

All time

- <%= render "people_leaderboard", users_with_counts: @all_time_commenters %> + <%= render "people_leaderboard", users_with_counts: @users[:all_time_commenters] %>
From eb609a530c4fb0a63f096be3d209a8758b4d6348 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Thu, 13 Oct 2016 16:51:07 +1100 Subject: [PATCH 06/50] Move and update tests for extracted Statistics model methods --- spec/controllers/general_controller_spec.rb | 90 +-------------------- spec/models/statistics_spec.rb | 87 ++++++++++++++++++++ 2 files changed, 89 insertions(+), 88 deletions(-) create mode 100644 spec/models/statistics_spec.rb diff --git a/spec/controllers/general_controller_spec.rb b/spec/controllers/general_controller_spec.rb index b82f8054a23..0f3ccffd207 100644 --- a/spec/controllers/general_controller_spec.rb +++ b/spec/controllers/general_controller_spec.rb @@ -421,10 +421,10 @@ get :statistics expect(response).to render_template('general/statistics') # There are 5 different graphs we're creating at the moment. - expect(assigns[:graph_list].length).to eq(5) + expect(assigns[:public_bodies].length).to eq(5) # The first is the only one with raw values, the rest are # percentages with error bars: - assigns[:graph_list].each_with_index do |graph, index| + assigns[:public_bodies].each_with_index do |graph, index| if index == 0 expect(graph['errorbars']).to be false expect(graph['x_values'].length).to eq(4) @@ -444,90 +444,4 @@ end end end - -end - -describe GeneralController, "when converting data for graphing" do - - before(:each) do - @raw_count_data = PublicBody.get_request_totals(n=3, - highest=true, - minimum_requests=1) - @percentages_data = PublicBody.get_request_percentages( - column='info_requests_successful_count', - n=3, - highest=false, - minimum_requests=1) - end - - it "should not include the real public body model instance" do - to_draw = controller.simplify_stats_for_graphs(@raw_count_data, - column='blah_blah', - percentages=false, - {} ) - expect(to_draw['public_bodies'][0].class).to eq(Hash) - expect(to_draw['public_bodies'][0].has_key?('request_email')).to be false - end - - it "should generate the expected id" do - to_draw = controller.simplify_stats_for_graphs(@raw_count_data, - column='blah_blah', - percentages=false, - {:highest => true} ) - expect(to_draw['id']).to eq("blah_blah-highest") - to_draw = controller.simplify_stats_for_graphs(@raw_count_data, - column='blah_blah', - percentages=false, - {:highest => false} ) - expect(to_draw['id']).to eq("blah_blah-lowest") - end - - it "should have exactly the expected keys" do - to_draw = controller.simplify_stats_for_graphs(@raw_count_data, - column='blah_blah', - percentages=false, - {} ) - expect(to_draw.keys.sort).to eq(["errorbars", "id", "public_bodies", - "title", "tooltips", "totals", - "x_axis", "x_ticks", "x_values", - "y_axis", "y_max", "y_values"]) - - to_draw = controller.simplify_stats_for_graphs(@percentages_data, - column='whatever', - percentages=true, - {}) - expect(to_draw.keys.sort).to eq(["cis_above", "cis_below", - "errorbars", "id", "public_bodies", - "title", "tooltips", "totals", - "x_axis", "x_ticks", "x_values", - "y_axis", "y_max", "y_values"]) - end - - it "should have values of the expected class and length" do - [controller.simplify_stats_for_graphs(@raw_count_data, - column='blah_blah', - percentages=false, - {}), - controller.simplify_stats_for_graphs(@percentages_data, - column='whatever', - percentages=true, - {})].each do |to_draw| - per_pb_keys = ["cis_above", "cis_below", "public_bodies", - "tooltips", "totals", "x_ticks", "x_values", - "y_values"] - # These should be all be arrays with one element per public body: - per_pb_keys.each do |key| - if to_draw.has_key? key - expect(to_draw[key].class).to eq(Array) - expect(to_draw[key].length).to eq(3), "for key #{key}" - end - end - # Just check that the rest aren't of class Array: - to_draw.keys.each do |key| - unless per_pb_keys.include? key - expect(to_draw[key].class).not_to eq(Array), "for key #{key}" - end - end - end - end end diff --git a/spec/models/statistics_spec.rb b/spec/models/statistics_spec.rb new file mode 100644 index 00000000000..0676e101b06 --- /dev/null +++ b/spec/models/statistics_spec.rb @@ -0,0 +1,87 @@ +require "spec_helper" + +describe Statistics do + describe ".simplify_stats_for_graphs" do + before(:each) do + @raw_count_data = PublicBody.get_request_totals(n=3, + highest=true, + minimum_requests=1) + @percentages_data = PublicBody.get_request_percentages( + column='info_requests_successful_count', + n=3, + highest=false, + minimum_requests=1) + end + + it "should not include the real public body model instance" do + to_draw = Statistics.simplify_stats_for_graphs(@raw_count_data, + column='blah_blah', + percentages=false, + {} ) + expect(to_draw['public_bodies'][0].class).to eq(Hash) + expect(to_draw['public_bodies'][0].has_key?('request_email')).to be false + end + + it "should generate the expected id" do + to_draw = Statistics.simplify_stats_for_graphs(@raw_count_data, + column='blah_blah', + percentages=false, + {:highest => true} ) + expect(to_draw['id']).to eq("blah_blah-highest") + to_draw = Statistics.simplify_stats_for_graphs(@raw_count_data, + column='blah_blah', + percentages=false, + {:highest => false} ) + expect(to_draw['id']).to eq("blah_blah-lowest") + end + + it "should have exactly the expected keys" do + to_draw = Statistics.simplify_stats_for_graphs(@raw_count_data, + column='blah_blah', + percentages=false, + {} ) + expect(to_draw.keys.sort).to eq(["errorbars", "id", "public_bodies", + "title", "tooltips", "totals", + "x_axis", "x_ticks", "x_values", + "y_axis", "y_max", "y_values"]) + + to_draw = Statistics.simplify_stats_for_graphs(@percentages_data, + column='whatever', + percentages=true, + {}) + expect(to_draw.keys.sort).to eq(["cis_above", "cis_below", + "errorbars", "id", "public_bodies", + "title", "tooltips", "totals", + "x_axis", "x_ticks", "x_values", + "y_axis", "y_max", "y_values"]) + end + + it "should have values of the expected class and length" do + [Statistics.simplify_stats_for_graphs(@raw_count_data, + column='blah_blah', + percentages=false, + {}), + Statistics.simplify_stats_for_graphs(@percentages_data, + column='whatever', + percentages=true, + {})].each do |to_draw| + per_pb_keys = ["cis_above", "cis_below", "public_bodies", + "tooltips", "totals", "x_ticks", "x_values", + "y_values"] + # These should be all be arrays with one element per public body: + per_pb_keys.each do |key| + if to_draw.has_key? key + expect(to_draw[key].class).to eq(Array) + expect(to_draw[key].length).to eq(3), "for key #{key}" + end + end + # Just check that the rest aren't of class Array: + to_draw.keys.each do |key| + unless per_pb_keys.include? key + expect(to_draw[key].class).not_to eq(Array), "for key #{key}" + end + end + end + end + end +end From 4878c6353aae97a8ff28a2a7f57ef9d20fe325a6 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Thu, 13 Oct 2016 17:00:40 +1100 Subject: [PATCH 07/50] Use let instead of a before block in tests --- spec/models/statistics_spec.rb | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/spec/models/statistics_spec.rb b/spec/models/statistics_spec.rb index 0676e101b06..dc448214cd4 100644 --- a/spec/models/statistics_spec.rb +++ b/spec/models/statistics_spec.rb @@ -2,19 +2,21 @@ describe Statistics do describe ".simplify_stats_for_graphs" do - before(:each) do - @raw_count_data = PublicBody.get_request_totals(n=3, - highest=true, - minimum_requests=1) - @percentages_data = PublicBody.get_request_percentages( + let(:raw_count_data) do + PublicBody.get_request_totals(n=3, highest=true, minimum_requests=1) + end + + let(:percentages_data) do + PublicBody.get_request_percentages( column='info_requests_successful_count', n=3, highest=false, - minimum_requests=1) + minimum_requests=1 + ) end it "should not include the real public body model instance" do - to_draw = Statistics.simplify_stats_for_graphs(@raw_count_data, + to_draw = Statistics.simplify_stats_for_graphs(raw_count_data, column='blah_blah', percentages=false, {} ) @@ -23,12 +25,12 @@ end it "should generate the expected id" do - to_draw = Statistics.simplify_stats_for_graphs(@raw_count_data, + to_draw = Statistics.simplify_stats_for_graphs(raw_count_data, column='blah_blah', percentages=false, {:highest => true} ) expect(to_draw['id']).to eq("blah_blah-highest") - to_draw = Statistics.simplify_stats_for_graphs(@raw_count_data, + to_draw = Statistics.simplify_stats_for_graphs(raw_count_data, column='blah_blah', percentages=false, {:highest => false} ) @@ -36,7 +38,7 @@ end it "should have exactly the expected keys" do - to_draw = Statistics.simplify_stats_for_graphs(@raw_count_data, + to_draw = Statistics.simplify_stats_for_graphs(raw_count_data, column='blah_blah', percentages=false, {} ) @@ -45,7 +47,7 @@ "x_axis", "x_ticks", "x_values", "y_axis", "y_max", "y_values"]) - to_draw = Statistics.simplify_stats_for_graphs(@percentages_data, + to_draw = Statistics.simplify_stats_for_graphs(percentages_data, column='whatever', percentages=true, {}) @@ -57,11 +59,11 @@ end it "should have values of the expected class and length" do - [Statistics.simplify_stats_for_graphs(@raw_count_data, + [Statistics.simplify_stats_for_graphs(raw_count_data, column='blah_blah', percentages=false, {}), - Statistics.simplify_stats_for_graphs(@percentages_data, + Statistics.simplify_stats_for_graphs(percentages_data, column='whatever', percentages=true, {})].each do |to_draw| From fd9a25616c7348473ff9fcdbc89441591e58bfd4 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Thu, 13 Oct 2016 17:21:43 +1100 Subject: [PATCH 08/50] Improve title and add introduction to statistics page --- app/views/general/statistics.html.erb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/app/views/general/statistics.html.erb b/app/views/general/statistics.html.erb index 30104dde181..0063762ad21 100644 --- a/app/views/general/statistics.html.erb +++ b/app/views/general/statistics.html.erb @@ -1,6 +1,14 @@ <% @title = _("Statistics") %>
-

<%= @title %>

+

<%= site_name %> <%= @title %>

+ +

+ Here you can + <%= link_to "see how well public bodies have responded", "#public_bodies" %> + to requests on <%= site_name %> and + <%= link_to "meet the people most actively using it", "#people" %>. +

+

Public Bodies

<%= _("This page of public body statistics is currently experimental, " \ From 1726f0a82473a6b63f2b93d93f69cce61fcfd52a Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Thu, 13 Oct 2016 17:27:32 +1100 Subject: [PATCH 09/50] Move confidence_intervals require to where it's actually being used --- app/controllers/general_controller.rb | 1 - app/models/public_body.rb | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/general_controller.rb b/app/controllers/general_controller.rb index 7715d03c0fe..a2e769653e3 100644 --- a/app/controllers/general_controller.rb +++ b/app/controllers/general_controller.rb @@ -7,7 +7,6 @@ # Email: hello@mysociety.org; WWW: http://www.mysociety.org/ require 'open-uri' -require 'confidence_intervals' class GeneralController < ApplicationController diff --git a/app/models/public_body.rb b/app/models/public_body.rb index 4f4ef6c6797..7a973aadc5a 100644 --- a/app/models/public_body.rb +++ b/app/models/public_body.rb @@ -30,6 +30,7 @@ require 'csv' require 'securerandom' require 'set' +require 'confidence_intervals' class PublicBody < ActiveRecord::Base include AdminColumn From 2b68cfb70d6bfa543c993ce4b03c5768a6133815 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Fri, 14 Oct 2016 15:18:00 +1100 Subject: [PATCH 10/50] Update intro text to public body statistics section --- app/views/general/statistics.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/general/statistics.html.erb b/app/views/general/statistics.html.erb index 0063762ad21..84408987d1d 100644 --- a/app/views/general/statistics.html.erb +++ b/app/views/general/statistics.html.erb @@ -11,7 +11,7 @@

Public Bodies

-

<%= _("This page of public body statistics is currently experimental, " \ +

<%= _("This section on public body statistics is currently experimental, " \ "so there are some caveats that should be borne in mind:") %>

    From 25a9d96cc711ee159a401076d98e00e2ff8d2042 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Fri, 14 Oct 2016 15:27:28 +1100 Subject: [PATCH 11/50] Make paragraph translatable --- app/views/general/statistics.html.erb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/app/views/general/statistics.html.erb b/app/views/general/statistics.html.erb index 84408987d1d..628817f86c0 100644 --- a/app/views/general/statistics.html.erb +++ b/app/views/general/statistics.html.erb @@ -2,12 +2,11 @@

    <%= site_name %> <%= @title %>

    -

    - Here you can - <%= link_to "see how well public bodies have responded", "#public_bodies" %> - to requests on <%= site_name %> and - <%= link_to "meet the people most actively using it", "#people" %>. -

    +

    <%= _('Here you can ' \ + 'see how well public bodies have responded ' \ + 'to requests on {{site_name}} and ' \ + 'meet the people most actively using it.', + site_name: site_name) %>

    Public Bodies

    From a5dd9e2f8bd9ca75b8ed97de17a2f9ab71686128 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Fri, 14 Oct 2016 15:37:48 +1100 Subject: [PATCH 12/50] Remove custom link style so themes can choose it themselves --- app/assets/stylesheets/responsive/_statistics_style.scss | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/assets/stylesheets/responsive/_statistics_style.scss b/app/assets/stylesheets/responsive/_statistics_style.scss index 6c8e7dfda96..ea7d4cf6185 100644 --- a/app/assets/stylesheets/responsive/_statistics_style.scss +++ b/app/assets/stylesheets/responsive/_statistics_style.scss @@ -18,7 +18,3 @@ text-align: center; background: lighten(desaturate(#2688dc, 50), 40); } - -a.leaderboard-person-details { - color: #333; -} From 8081720de043988c879c06b7a0ac107834112c0f Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Fri, 14 Oct 2016 15:45:00 +1100 Subject: [PATCH 13/50] Move statistics onto their own controller for better organisation --- app/controllers/general_controller.rb | 19 ---------- app/controllers/statistics_controller.rb | 20 +++++++++++ .../_people_leaderboard.html.erb | 0 .../index.html.erb} | 0 config/routes.rb | 6 ++-- spec/controllers/general_controller_spec.rb | 34 ------------------ .../controllers/statistics_controller_spec.rb | 36 +++++++++++++++++++ 7 files changed, 60 insertions(+), 55 deletions(-) create mode 100644 app/controllers/statistics_controller.rb rename app/views/{general => statistics}/_people_leaderboard.html.erb (100%) rename app/views/{general/statistics.html.erb => statistics/index.html.erb} (100%) create mode 100644 spec/controllers/statistics_controller_spec.rb diff --git a/app/controllers/general_controller.rb b/app/controllers/general_controller.rb index a2e769653e3..8c6ced0e046 100644 --- a/app/controllers/general_controller.rb +++ b/app/controllers/general_controller.rb @@ -233,23 +233,4 @@ def version }} end end - - def statistics - unless AlaveteliConfiguration::public_body_statistics_page - raise ActiveRecord::RecordNotFound.new("Page not enabled") - end - - @public_bodies = Statistics.public_bodies - @users = Statistics.users - - respond_to do |format| - format.html - format.json do - render json: { - public_bodies: @public_bodies, - users: Statistics.user_json_for_api(@users) - } - end - end - end end diff --git a/app/controllers/statistics_controller.rb b/app/controllers/statistics_controller.rb new file mode 100644 index 00000000000..a5ce8a995b9 --- /dev/null +++ b/app/controllers/statistics_controller.rb @@ -0,0 +1,20 @@ +class StatisticsController < ApplicationController + def index + unless AlaveteliConfiguration::public_body_statistics_page + raise ActiveRecord::RecordNotFound.new("Page not enabled") + end + + @public_bodies = Statistics.public_bodies + @users = Statistics.users + + respond_to do |format| + format.html + format.json do + render json: { + public_bodies: @public_bodies, + users: Statistics.user_json_for_api(@users) + } + end + end + end +end diff --git a/app/views/general/_people_leaderboard.html.erb b/app/views/statistics/_people_leaderboard.html.erb similarity index 100% rename from app/views/general/_people_leaderboard.html.erb rename to app/views/statistics/_people_leaderboard.html.erb diff --git a/app/views/general/statistics.html.erb b/app/views/statistics/index.html.erb similarity index 100% rename from app/views/general/statistics.html.erb rename to app/views/statistics/index.html.erb diff --git a/config/routes.rb b/config/routes.rb index 01a9f17662b..8d73a9cd409 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -42,10 +42,12 @@ match '/version.:format' => 'general#version', :as => :version, :via => :get - get '/statistics' => 'general#statistics' - get '/body_statistics' => redirect('/statistics#public_bodies'), :as => :public_bodies_statistics ##### + ##### Statistics controller + get '/statistics' => 'statistics#index' + get '/body_statistics' => redirect('/statistics#public_bodies'), :as => :public_bodies_statistics + ##### Request controller match '/list/recent' => 'request#list', :as => :request_list_recent, diff --git a/spec/controllers/general_controller_spec.rb b/spec/controllers/general_controller_spec.rb index 0f3ccffd207..c076401e6de 100644 --- a/spec/controllers/general_controller_spec.rb +++ b/spec/controllers/general_controller_spec.rb @@ -411,37 +411,3 @@ end end - -describe GeneralController, "when showing public body statistics" do - - it "should render the right template with the right data" do - config = MySociety::Config.load_default - config['MINIMUM_REQUESTS_FOR_STATISTICS'] = 1 - config['PUBLIC_BODY_STATISTICS_PAGE'] = true - get :statistics - expect(response).to render_template('general/statistics') - # There are 5 different graphs we're creating at the moment. - expect(assigns[:public_bodies].length).to eq(5) - # The first is the only one with raw values, the rest are - # percentages with error bars: - assigns[:public_bodies].each_with_index do |graph, index| - if index == 0 - expect(graph['errorbars']).to be false - expect(graph['x_values'].length).to eq(4) - expect(graph['x_values']).to eq([0, 1, 2, 3]) - expect(graph['y_values']).to eq([1, 2, 2, 4]) - else - expect(graph['errorbars']).to be true - # Just check the first one: - if index == 1 - expect(graph['x_values']).to eq([0, 1, 2, 3]) - expect(graph['y_values']).to eq([0, 50, 100, 100]) - end - # Check that at least every confidence interval value is - # a Float (rather than NilClass, say): - graph['cis_below'].each { |v| expect(v).to be_instance_of(Float) } - graph['cis_above'].each { |v| expect(v).to be_instance_of(Float) } - end - end - end -end diff --git a/spec/controllers/statistics_controller_spec.rb b/spec/controllers/statistics_controller_spec.rb new file mode 100644 index 00000000000..e75278af0ab --- /dev/null +++ b/spec/controllers/statistics_controller_spec.rb @@ -0,0 +1,36 @@ +require "spec_helper" + +describe StatisticsController do + describe "#index" do + it "should render the right template with the right data" do + config = MySociety::Config.load_default + config['MINIMUM_REQUESTS_FOR_STATISTICS'] = 1 + config['PUBLIC_BODY_STATISTICS_PAGE'] = true + get :index + expect(response).to render_template('statistics/index') + # There are 5 different graphs we're creating at the moment. + expect(assigns[:public_bodies].length).to eq(5) + # The first is the only one with raw values, the rest are + # percentages with error bars: + assigns[:public_bodies].each_with_index do |graph, index| + if index == 0 + expect(graph['errorbars']).to be false + expect(graph['x_values'].length).to eq(4) + expect(graph['x_values']).to eq([0, 1, 2, 3]) + expect(graph['y_values']).to eq([1, 2, 2, 4]) + else + expect(graph['errorbars']).to be true + # Just check the first one: + if index == 1 + expect(graph['x_values']).to eq([0, 1, 2, 3]) + expect(graph['y_values']).to eq([0, 50, 100, 100]) + end + # Check that at least every confidence interval value is + # a Float (rather than NilClass, say): + graph['cis_below'].each { |v| expect(v).to be_instance_of(Float) } + graph['cis_above'].each { |v| expect(v).to be_instance_of(Float) } + end + end + end + end +end From badfb17bc91ba8183d8957538fe8d257bf0e97b3 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Fri, 14 Oct 2016 16:03:42 +1100 Subject: [PATCH 14/50] Add test TODOs for methods we've added --- spec/models/statistics_spec.rb | 12 ++++++++++++ spec/models/user_spec.rb | 16 ++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/spec/models/statistics_spec.rb b/spec/models/statistics_spec.rb index dc448214cd4..9478a24e1cd 100644 --- a/spec/models/statistics_spec.rb +++ b/spec/models/statistics_spec.rb @@ -1,6 +1,10 @@ require "spec_helper" describe Statistics do + describe ".public_bodies" do + # TODO + end + describe ".simplify_stats_for_graphs" do let(:raw_count_data) do PublicBody.get_request_totals(n=3, highest=true, minimum_requests=1) @@ -86,4 +90,12 @@ end end end + + describe ".users" do + # TODO + end + + describe ".user_json_for_api" do + # TODO + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 4c77cff836b..53ff65d908a 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -442,6 +442,22 @@ end + describe '.all_time_requesters' do + # TODO + end + + describe '.last_28_day_requesters' do + # TODO + end + + describe '.all_time_commenters' do + # TODO + end + + describe '.last_28_day_commenters' do + # TODO + end + describe '#transactions' do it 'returns a TransactionCalculator with the default transaction set' do From 7e5ad51337e87af6553ddb2a9f99e09b55419c4a Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Fri, 14 Oct 2016 16:16:40 +1100 Subject: [PATCH 15/50] Add a test to ensure user statistics hash is correctly formed --- spec/models/statistics_spec.rb | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/spec/models/statistics_spec.rb b/spec/models/statistics_spec.rb index 9478a24e1cd..c14ba2bbe83 100644 --- a/spec/models/statistics_spec.rb +++ b/spec/models/statistics_spec.rb @@ -92,7 +92,21 @@ end describe ".users" do - # TODO + it "creates a hash of user statistics" do + allow(User).to receive(:all_time_requesters).and_return([]) + allow(User).to receive(:last_28_day_requesters).and_return([]) + allow(User).to receive(:all_time_commenters).and_return([]) + allow(User).to receive(:last_28_day_commenters).and_return([]) + + expect(Statistics.users).to eql( + { + all_time_requesters: [], + last_28_day_requesters: [], + all_time_commenters: [], + last_28_day_commenters: [] + } + ) + end end describe ".user_json_for_api" do From 2c40b694a5349793bfde26fae46e8ddf04c4792a Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Fri, 14 Oct 2016 17:32:07 +1100 Subject: [PATCH 16/50] Add a test for User.last_28_day_requesters --- spec/models/user_spec.rb | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 53ff65d908a..07efc80db42 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -447,7 +447,24 @@ end describe '.last_28_day_requesters' do - # TODO + it 'gets recent frequent requesters' do + user_with_3_requests = FactoryGirl.create(:user) + 3.times { FactoryGirl.create(:info_request, user: user_with_3_requests) } + user_with_2_requests = FactoryGirl.create(:user) + 2.times { FactoryGirl.create(:info_request, user: user_with_2_requests) } + user_with_1_request = FactoryGirl.create(:user) + FactoryGirl.create(:info_request, user: user_with_1_request) + user_with_an_old_request = FactoryGirl.create(:user) + FactoryGirl.create(:info_request, user: user_with_an_old_request, created_at: 2.months.ago) + + expect(User.last_28_day_requesters).to eql( + { + user_with_3_requests => 3, + user_with_2_requests => 2, + user_with_1_request => 1 + } + ) + end end describe '.all_time_commenters' do From c020c5d35bb585835289c64c4cb3966c241c245c Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Mon, 17 Oct 2016 12:34:18 +1100 Subject: [PATCH 17/50] Fix how we return results so it's the same as the InfoRequest results --- app/models/user.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 3dec62f673f..d394099b587 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -241,7 +241,9 @@ def self.all_time_commenters limit(10). count # TODO: Have user objects automatically instantiated like the InfoRequest queries above - commenters.map { |u_id,c| [User.find(u_id), c] } + result = {} + commenters.each { |user_id,count| result[User.find(user_id)] = count } + result end def self.last_28_day_commenters @@ -254,7 +256,9 @@ def self.last_28_day_commenters limit(10). count # TODO: Have user objects automatically instantiated like the InfoRequest queries above - commenters.map { |u_id,c| [User.find(u_id), c] } + result = {} + commenters.each { |user_id,count| result[User.find(user_id)] = count } + result end def transactions(*associations) From 74f96b9816e9493734a01636d5f305f0464c3b7d Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Mon, 17 Oct 2016 12:35:05 +1100 Subject: [PATCH 18/50] Add test for User.last_28_day_commenters --- spec/models/user_spec.rb | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 07efc80db42..3ecd3c20cdf 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -472,7 +472,24 @@ end describe '.last_28_day_commenters' do - # TODO + it 'gets recent frequent commenters' do + user_with_3_comments = FactoryGirl.create(:user) + 3.times { FactoryGirl.create(:comment, user: user_with_3_comments) } + user_with_2_comments = FactoryGirl.create(:user) + 2.times { FactoryGirl.create(:comment, user: user_with_2_comments) } + user_with_1_comment = FactoryGirl.create(:user) + FactoryGirl.create(:comment, user: user_with_1_comment) + user_with_an_old_comment = FactoryGirl.create(:user) + FactoryGirl.create(:comment, user: user_with_an_old_comment, created_at: 2.months.ago) + + expect(User.last_28_day_commenters).to eql( + { + user_with_3_comments => 3, + user_with_2_comments => 2, + user_with_1_comment => 1 + } + ) + end end describe '#transactions' do From a6eebb64822756891c3a7e5ddd614beadb0a3b71 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Mon, 17 Oct 2016 12:35:48 +1100 Subject: [PATCH 19/50] Add test for Statistics.user_json_for_api --- spec/models/statistics_spec.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/spec/models/statistics_spec.rb b/spec/models/statistics_spec.rb index c14ba2bbe83..a81f6780162 100644 --- a/spec/models/statistics_spec.rb +++ b/spec/models/statistics_spec.rb @@ -110,6 +110,13 @@ end describe ".user_json_for_api" do - # TODO + it "creates more descriptive and sanitised JSON" do + user = FactoryGirl.create(:user) + test_data = { a_test_key: { user => 123 } } + + expect(Statistics.user_json_for_api(test_data)).to eql( + { a_test_key: [{ user: user.json_for_api, count: 123 }] } + ) + end end end From 7066cca0e31d693eaea7970dfe67f5804c374ac0 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Mon, 17 Oct 2016 12:40:29 +1100 Subject: [PATCH 20/50] Add test for User.all_time_requesters --- spec/models/user_spec.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3ecd3c20cdf..f1ff4d3e374 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -443,7 +443,16 @@ end describe '.all_time_requesters' do - # TODO + it 'gets most frequent requesters' do + # FIXME: This uses fixtures. Change it to use factories when we can. + expect(User.all_time_requesters).to eql( + { + users(:bob_smith_user) => 5, + users(:robin_user) => 2, + users(:another_user) => 1 + } + ) + end end describe '.last_28_day_requesters' do From 2aa71c7bbbeabf2da15fcc82f4ce204abefc3bd5 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Mon, 17 Oct 2016 12:42:20 +1100 Subject: [PATCH 21/50] Add test for User.all_time_commenters --- spec/models/user_spec.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f1ff4d3e374..d0b695bee99 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -477,7 +477,15 @@ end describe '.all_time_commenters' do - # TODO + it 'gets most frequent commenters' do + # FIXME: This uses fixtures. Change it to use factories when we can. + expect(User.all_time_commenters).to eql( + { + users(:bob_smith_user) => 1, + users(:silly_name_user) => 1 + } + ) + end end describe '.last_28_day_commenters' do From 007ff53c826b0dc026b9b5a14fd7521619242388 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Mon, 17 Oct 2016 12:46:41 +1100 Subject: [PATCH 22/50] Update comment now this method has been moved --- app/models/statistics.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/models/statistics.rb b/app/models/statistics.rb index 0500a250354..0bc0126c6fe 100644 --- a/app/models/statistics.rb +++ b/app/models/statistics.rb @@ -68,10 +68,9 @@ def public_bodies graph_list end - # This is a helper method to take data returned by the PublicBody - # model's statistics-generating methods, and converting them to - # simpler data structure that can be rendered by a Javascript - # graph library. + # This is a helper method to take data returned by the above method and + # converting them to simpler data structure that can be rendered by a + # Javascript graph library. def simplify_stats_for_graphs(data, column, percentages, From 475d599e9855b4421d2c0d7e0aa3835fde6c4a8b Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Mon, 17 Oct 2016 14:57:59 +1100 Subject: [PATCH 23/50] Add an explanation when there's no one to show on leaderboard --- .../statistics/_people_leaderboard.html.erb | 37 ++++++++++--------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/app/views/statistics/_people_leaderboard.html.erb b/app/views/statistics/_people_leaderboard.html.erb index 782a6501498..b2122054be2 100644 --- a/app/views/statistics/_people_leaderboard.html.erb +++ b/app/views/statistics/_people_leaderboard.html.erb @@ -1,19 +1,22 @@ -
      -<% users_with_counts.each do |user, count| %> -
    1. - <%= link_to user, class: "leaderboard-person-details" do %> - <% if user.profile_photo %> - Avatar image for <%= user.name %> - <% else %> - <%= user.name.first.upcase %> +<% if users_with_counts.any? %> +
        + <% users_with_counts.each do |user, count| %> +
      1. + <%= link_to user, class: "leaderboard-person-details" do %> + <% if user.profile_photo %> + Avatar image for <%= user.name %> + <% else %> + <%= user.name.first.upcase %> + <% end %> +
        +

        <%= user.name %>

        +

        <%= _('Joined in {{year}}', :year => user.created_at.year) %>

        +
        <% end %> -
        -

        <%= user.name %>

        -

        <%= _('Joined in {{year}}', :year => user.created_at.year) %>

        -
        - <% end %> -

        <%= count %>

        -
        2. +

        <%= count %>

        + + <% end %> +
      +<% else %> +

      Crickets. No one is here. It's time to get the word out.

      <% end %> -
    - From 8ea616ea29f1c30f17e21981d0460a8bfd21ee96 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Mon, 17 Oct 2016 14:59:59 +1100 Subject: [PATCH 24/50] Use consistent hash style within the same file --- app/views/statistics/_people_leaderboard.html.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/statistics/_people_leaderboard.html.erb b/app/views/statistics/_people_leaderboard.html.erb index b2122054be2..16ea6341122 100644 --- a/app/views/statistics/_people_leaderboard.html.erb +++ b/app/views/statistics/_people_leaderboard.html.erb @@ -4,13 +4,13 @@
  • <%= link_to user, class: "leaderboard-person-details" do %> <% if user.profile_photo %> - Avatar image for <%= user.name %> + Avatar image for <%= user.name %> <% else %> <%= user.name.first.upcase %> <% end %>

    <%= user.name %>

    -

    <%= _('Joined in {{year}}', :year => user.created_at.year) %>

    +

    <%= _('Joined in {{year}}', year: user.created_at.year) %>

    <% end %>

    <%= count %>

    From c96cda8f9e5a58af80af6b5eb6ed5f0b1b95662a Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Mon, 17 Oct 2016 15:10:13 +1100 Subject: [PATCH 25/50] Use smart quotes just for @equivalentideas --- app/views/statistics/_people_leaderboard.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/statistics/_people_leaderboard.html.erb b/app/views/statistics/_people_leaderboard.html.erb index 16ea6341122..1daafd60858 100644 --- a/app/views/statistics/_people_leaderboard.html.erb +++ b/app/views/statistics/_people_leaderboard.html.erb @@ -18,5 +18,5 @@ <% end %> <% else %> -

    Crickets. No one is here. It's time to get the word out.

    +

    Crickets. No one is here. It’s time to get the word out.

    <% end %> From 9873b7c11152cefb065c8fc456f06b9e46704ec7 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Mon, 17 Oct 2016 16:42:53 +1100 Subject: [PATCH 26/50] Change event type to hide when only editing prominence Admins sometimes hide requests by editing the prominence metadata rather than using the hide event action. This is commonly used when hiding a request for, or containing, personal information. This change will make it much easier to find these events because they will be tracked under the same "hide" type. --- app/models/info_request_event.rb | 9 ++ spec/models/info_request_event_spec.rb | 117 +++++++++++++++++++++++++ 2 files changed, 126 insertions(+) diff --git a/app/models/info_request_event.rb b/app/models/info_request_event.rb index fe60d72f357..86e44e2a33c 100644 --- a/app/models/info_request_event.rb +++ b/app/models/info_request_event.rb @@ -60,6 +60,9 @@ class InfoRequestEvent < ActiveRecord::Base validates_presence_of :event_type + before_save(:if => :only_editing_prominence_to_hide?) do + self.event_type = "hide" + end after_create :update_request, :if => :response? def self.enumerate_event_types @@ -388,6 +391,12 @@ def response? event_type == 'response' end + def only_editing_prominence_to_hide? + event_type == 'edit' && + params_diff[:new].keys == [:prominence] && + %w(hidden requester_only backpage).include?(params_diff[:new][:prominence]) + end + # This method updates the cached column of the InfoRequest that # stores the last created_at date of relevant events # when saving or destroying an InfoRequestEvent associated with the request diff --git a/spec/models/info_request_event_spec.rb b/spec/models/info_request_event_spec.rb index e834432bbda..8297cdaace3 100644 --- a/spec/models/info_request_event_spec.rb +++ b/spec/models/info_request_event_spec.rb @@ -491,4 +491,121 @@ end end + + describe "editing requests" do + let(:unchanged_params) do + { :editor => "henare", + :old_title => "How much wood does a woodpecker peck?", + :title => "How much wood does a woodpecker peck?", + :old_described_state => "rejected", + :described_state => "rejected", + :old_awaiting_description => false, + :awaiting_description => false, + :old_allow_new_responses_from => "anybody", + :allow_new_responses_from => "anybody", + :old_handle_rejected_responses => "bounce", + :handle_rejected_responses => "bounce", + :old_tag_string => "", + :tag_string => "", + :old_comments_allowed => true, + :comments_allowed => true } + end + + it "should change type to hidden when only editing prominence to hidden" do + params = unchanged_params.merge({:old_prominence => "normal", :prominence => "hidden"}) + + ire = InfoRequestEvent.create!(:info_request => FactoryGirl.create(:info_request), + :event_type => "edit", + :params => params) + + expect(ire.event_type).to eql "hide" + end + + it "should change type to hidden when only editing prominence to requester_only" do + params = unchanged_params.merge({:old_prominence => "normal", :prominence => "requester_only"}) + + ire = InfoRequestEvent.create!(:info_request => FactoryGirl.create(:info_request), + :event_type => "edit", + :params => params) + + expect(ire.event_type).to eql "hide" + end + + it "should change type to hidden when only editing prominence to backpage" do + params = unchanged_params.merge({:old_prominence => "normal", :prominence => "backpage"}) + + ire = InfoRequestEvent.create!(:info_request => FactoryGirl.create(:info_request), + :event_type => "edit", + :params => params) + + expect(ire.event_type).to eql "hide" + end + end + + describe "#only_editing_prominence_to_hide?" do + let(:unchanged_params) do + { :editor => "henare", + :old_title => "How much wood does a woodpecker peck?", + :title => "How much wood does a woodpecker peck?", + :old_described_state => "rejected", + :described_state => "rejected", + :old_awaiting_description => false, + :awaiting_description => false, + :old_allow_new_responses_from => "anybody", + :allow_new_responses_from => "anybody", + :old_handle_rejected_responses => "bounce", + :handle_rejected_responses => "bounce", + :old_tag_string => "", + :tag_string => "", + :old_comments_allowed => true, + :comments_allowed => true } + end + + it "should be true if only editing prominence to hidden" do + params = unchanged_params.merge({:old_prominence => "normal", :prominence => "hidden"}) + + ire = InfoRequestEvent.new(:event_type => "edit", :params => params) + + expect(ire.only_editing_prominence_to_hide?).to be true + end + + it "should be true if only editing prominence to requester_only" do + params = unchanged_params.merge({:old_prominence => "normal", :prominence => "requester_only"}) + + ire = InfoRequestEvent.new(:event_type => "edit", :params => params) + + expect(ire.only_editing_prominence_to_hide?).to be true + end + + it "should be true if only editing prominence to backpage" do + params = unchanged_params.merge({:old_prominence => "normal", :prominence => "backpage"}) + + ire = InfoRequestEvent.new(:event_type => "edit", :params => params) + + expect(ire.only_editing_prominence_to_hide?).to be true + end + + it "should be false if editing multiple conditions" do + params = unchanged_params.merge({ :old_prominence => "normal", + :prominence => "backpage", + :old_comments_allowed => true, + :comments_allowed => false }) + + ire = InfoRequestEvent.new(:event_type => "edit", :params => params) + + expect(ire.only_editing_prominence_to_hide?).to be false + end + + it "should be false it's not an edit" do + ire = InfoRequestEvent.new(:event_type => "resent") + + expect(ire.only_editing_prominence_to_hide?).to be false + end + + it "should be false it's already a hide event" do + ire = InfoRequestEvent.new(:event_type => "hide") + + expect(ire.only_editing_prominence_to_hide?).to be false + end + end end From 2f1951073a9ea0ffff2644963b26be0dba904362 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Mon, 17 Oct 2016 16:58:48 +1100 Subject: [PATCH 27/50] Add a migration to update event types of existing hide events In db72f3119d66e10a57688cd0993bee74f99b286a we made it so that events that only edit the prominence of a request to hide it are saved with the event type "hide". This updates all existing events to match this new behaviour. --- ...event_type_when_only_editing_prominence_to_hide.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 db/migrate/20161017054639_update_event_type_when_only_editing_prominence_to_hide.rb diff --git a/db/migrate/20161017054639_update_event_type_when_only_editing_prominence_to_hide.rb b/db/migrate/20161017054639_update_event_type_when_only_editing_prominence_to_hide.rb new file mode 100644 index 00000000000..48eb0369f57 --- /dev/null +++ b/db/migrate/20161017054639_update_event_type_when_only_editing_prominence_to_hide.rb @@ -0,0 +1,11 @@ +class UpdateEventTypeWhenOnlyEditingProminenceToHide < ActiveRecord::Migration + def up + InfoRequestEvent.find_each do |event| + event.update_attributes!(event_type: "hide") if event.only_editing_prominence_to_hide? + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end From aa456c6cc260c3075b1925707b144a2bf82b95b9 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Tue, 18 Oct 2016 11:27:12 +1100 Subject: [PATCH 28/50] Fix typo in spec description --- spec/models/info_request_event_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/info_request_event_spec.rb b/spec/models/info_request_event_spec.rb index 8297cdaace3..b6502a28811 100644 --- a/spec/models/info_request_event_spec.rb +++ b/spec/models/info_request_event_spec.rb @@ -596,13 +596,13 @@ expect(ire.only_editing_prominence_to_hide?).to be false end - it "should be false it's not an edit" do + it "should be false if it's not an edit" do ire = InfoRequestEvent.new(:event_type => "resent") expect(ire.only_editing_prominence_to_hide?).to be false end - it "should be false it's already a hide event" do + it "should be false if it's already a hide event" do ire = InfoRequestEvent.new(:event_type => "hide") expect(ire.only_editing_prominence_to_hide?).to be false From 8a52d1f30ab4bd675bdf0b30971ed3d86032f393 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Tue, 18 Oct 2016 12:02:16 +1100 Subject: [PATCH 29/50] Don't change event type to hidden if it's already hidden --- app/models/info_request_event.rb | 1 + spec/models/info_request_event_spec.rb | 30 ++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/app/models/info_request_event.rb b/app/models/info_request_event.rb index 86e44e2a33c..4c6caf4d4c8 100644 --- a/app/models/info_request_event.rb +++ b/app/models/info_request_event.rb @@ -394,6 +394,7 @@ def response? def only_editing_prominence_to_hide? event_type == 'edit' && params_diff[:new].keys == [:prominence] && + params_diff[:old][:prominence] == "normal" && %w(hidden requester_only backpage).include?(params_diff[:new][:prominence]) end diff --git a/spec/models/info_request_event_spec.rb b/spec/models/info_request_event_spec.rb index b6502a28811..628e3696c15 100644 --- a/spec/models/info_request_event_spec.rb +++ b/spec/models/info_request_event_spec.rb @@ -607,5 +607,35 @@ expect(ire.only_editing_prominence_to_hide?).to be false end + + context "when the old prominence was hidden" do + let(:params) { unchanged_params.merge({:old_prominence => "hidden", :prominence => "requester_only"}) } + + it do + ire = InfoRequestEvent.new(:event_type => "edit", :params => params) + + expect(ire.only_editing_prominence_to_hide?).to be false + end + end + + context "when the old prominence was requester_only" do + let(:params) { unchanged_params.merge({:old_prominence => "requester_only", :prominence => "hidden"}) } + + it do + ire = InfoRequestEvent.new(:event_type => "edit", :params => params) + + expect(ire.only_editing_prominence_to_hide?).to be false + end + end + + context "when the old prominence was backpage" do + let(:params) { unchanged_params.merge({:old_prominence => "backpage", :prominence => "hidden"}) } + + it do + ire = InfoRequestEvent.new(:event_type => "edit", :params => params) + + expect(ire.only_editing_prominence_to_hide?).to be false + end + end end end From 65d2568ed2a07bd0ceac3cbf62eba6ae6c3af8e7 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Tue, 18 Oct 2016 12:07:30 +1100 Subject: [PATCH 30/50] Refactor tests only for clarity --- spec/models/info_request_event_spec.rb | 54 ++++++++++++++------------ 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/spec/models/info_request_event_spec.rb b/spec/models/info_request_event_spec.rb index 628e3696c15..bc159b985e0 100644 --- a/spec/models/info_request_event_spec.rb +++ b/spec/models/info_request_event_spec.rb @@ -561,28 +561,16 @@ :comments_allowed => true } end - it "should be true if only editing prominence to hidden" do - params = unchanged_params.merge({:old_prominence => "normal", :prominence => "hidden"}) - - ire = InfoRequestEvent.new(:event_type => "edit", :params => params) - - expect(ire.only_editing_prominence_to_hide?).to be true - end - - it "should be true if only editing prominence to requester_only" do - params = unchanged_params.merge({:old_prominence => "normal", :prominence => "requester_only"}) - - ire = InfoRequestEvent.new(:event_type => "edit", :params => params) + it "should be false if it's not an edit" do + ire = InfoRequestEvent.new(:event_type => "resent") - expect(ire.only_editing_prominence_to_hide?).to be true + expect(ire.only_editing_prominence_to_hide?).to be false end - it "should be true if only editing prominence to backpage" do - params = unchanged_params.merge({:old_prominence => "normal", :prominence => "backpage"}) - - ire = InfoRequestEvent.new(:event_type => "edit", :params => params) + it "should be false if it's already a hide event" do + ire = InfoRequestEvent.new(:event_type => "hide") - expect(ire.only_editing_prominence_to_hide?).to be true + expect(ire.only_editing_prominence_to_hide?).to be false end it "should be false if editing multiple conditions" do @@ -596,16 +584,34 @@ expect(ire.only_editing_prominence_to_hide?).to be false end - it "should be false if it's not an edit" do - ire = InfoRequestEvent.new(:event_type => "resent") + context "when only editing prominence to hidden" do + let(:params) { unchanged_params.merge({:old_prominence => "normal", :prominence => "hidden"}) } - expect(ire.only_editing_prominence_to_hide?).to be false + it do + ire = InfoRequestEvent.new(:event_type => "edit", :params => params) + + expect(ire.only_editing_prominence_to_hide?).to be true + end end - it "should be false if it's already a hide event" do - ire = InfoRequestEvent.new(:event_type => "hide") + context "when only editing prominence to requester_only" do + let(:params) { unchanged_params.merge({:old_prominence => "normal", :prominence => "requester_only"}) } - expect(ire.only_editing_prominence_to_hide?).to be false + it "should be true if only editing prominence to requester_only" do + ire = InfoRequestEvent.new(:event_type => "edit", :params => params) + + expect(ire.only_editing_prominence_to_hide?).to be true + end + end + + context "when only editing prominence to backpage" do + let(:params) { unchanged_params.merge({:old_prominence => "normal", :prominence => "backpage"}) } + + it "should be true if only editing prominence to backpage" do + ire = InfoRequestEvent.new(:event_type => "edit", :params => params) + + expect(ire.only_editing_prominence_to_hide?).to be true + end end context "when the old prominence was hidden" do From 4ea651c3359f72c65adf6d23594a7e42c072cf4c Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Tue, 18 Oct 2016 12:43:27 +1100 Subject: [PATCH 31/50] Allow params yaml to be blank There's no code path that currently makes this happen but it seems more correct to return an empty hash instead of throwing an exception. --- app/models/info_request_event.rb | 2 +- spec/models/info_request_event_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/models/info_request_event.rb b/app/models/info_request_event.rb index 4c6caf4d4c8..69b59b59449 100644 --- a/app/models/info_request_event.rb +++ b/app/models/info_request_event.rb @@ -305,7 +305,7 @@ def params=(params) end def params - param_hash = YAML.load(params_yaml) + param_hash = YAML.load(params_yaml) || {} param_hash.each do |key, value| param_hash[key] = value.force_encoding('UTF-8') if value.respond_to?(:force_encoding) end diff --git a/spec/models/info_request_event_spec.rb b/spec/models/info_request_event_spec.rb index bc159b985e0..fd32512077d 100644 --- a/spec/models/info_request_event_spec.rb +++ b/spec/models/info_request_event_spec.rb @@ -56,6 +56,12 @@ ire.params = example_params expect(ire.params).to eq(example_params) end + + it "should allow params_yaml to be blank" do + ire.params_yaml = '' + + expect(ire.params).to eql({}) + end end describe 'when deciding if it is indexed by search' do From a732bfaa555d513cbfd37cb1b49db952dc30c558 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Tue, 18 Oct 2016 14:40:19 +1100 Subject: [PATCH 32/50] Add a way to get number of hide events by week --- app/models/info_request_event.rb | 4 ++++ spec/factories/info_request_events.rb | 6 ++++++ spec/models/info_request_event_spec.rb | 17 +++++++++++++++++ 3 files changed, 27 insertions(+) diff --git a/app/models/info_request_event.rb b/app/models/info_request_event.rb index 69b59b59449..95ec571adc7 100644 --- a/app/models/info_request_event.rb +++ b/app/models/info_request_event.rb @@ -107,6 +107,10 @@ def must_be_valid_state :if => :indexed_by_search?, :eager_load => [ :outgoing_message, :comment, { :info_request => [ :user, :public_body, :censor_rules ] } ] + def self.count_of_hides_by_week + where(event_type: "hide").group("date(date_trunc('week', created_at))").count + end + def requested_by info_request.user_name_slug end diff --git a/spec/factories/info_request_events.rb b/spec/factories/info_request_events.rb index d617890905f..5d01e26ef4a 100644 --- a/spec/factories/info_request_events.rb +++ b/spec/factories/info_request_events.rb @@ -25,6 +25,12 @@ factory :sent_event do event_type 'sent' end + factory :edit_event do + event_type 'edit' + end + factory :hide_event do + event_type 'hide' + end end end diff --git a/spec/models/info_request_event_spec.rb b/spec/models/info_request_event_spec.rb index fd32512077d..4b4b2e1a4f1 100644 --- a/spec/models/info_request_event_spec.rb +++ b/spec/models/info_request_event_spec.rb @@ -119,6 +119,23 @@ end end + describe '.count_of_hides_by_week' do + it do + FactoryGirl.create(:hide_event, created_at: Time.utc(2016, 1, 4)) + FactoryGirl.create(:hide_event, created_at: Time.utc(2016, 1, 7)) + FactoryGirl.create(:edit_event, created_at: Time.utc(2016, 1, 11)) + FactoryGirl.create(:edit_event, created_at: Time.utc(2016, 1, 18)) + FactoryGirl.create(:hide_event, created_at: Time.utc(2016, 1, 24)) + + expect(InfoRequestEvent.count_of_hides_by_week).to eql( + { + "2016-01-04" => 2, + "2016-01-18" => 1 + } + ) + end + end + describe '#described_at' do let(:ire) { FactoryGirl.create(:info_request_event) } From 432c28cd30e97cf74d11c7211e5ee759503e4a6a Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Tue, 18 Oct 2016 15:14:12 +1100 Subject: [PATCH 33/50] Sort results --- app/models/info_request_event.rb | 2 +- spec/models/info_request_event_spec.rb | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/models/info_request_event.rb b/app/models/info_request_event.rb index 95ec571adc7..dd2084cf412 100644 --- a/app/models/info_request_event.rb +++ b/app/models/info_request_event.rb @@ -108,7 +108,7 @@ def must_be_valid_state :eager_load => [ :outgoing_message, :comment, { :info_request => [ :user, :public_body, :censor_rules ] } ] def self.count_of_hides_by_week - where(event_type: "hide").group("date(date_trunc('week', created_at))").count + where(event_type: "hide").group("date(date_trunc('week', created_at))").count.sort end def requested_by diff --git a/spec/models/info_request_event_spec.rb b/spec/models/info_request_event_spec.rb index 4b4b2e1a4f1..258760a7d5a 100644 --- a/spec/models/info_request_event_spec.rb +++ b/spec/models/info_request_event_spec.rb @@ -121,17 +121,17 @@ describe '.count_of_hides_by_week' do it do - FactoryGirl.create(:hide_event, created_at: Time.utc(2016, 1, 4)) - FactoryGirl.create(:hide_event, created_at: Time.utc(2016, 1, 7)) - FactoryGirl.create(:edit_event, created_at: Time.utc(2016, 1, 11)) - FactoryGirl.create(:edit_event, created_at: Time.utc(2016, 1, 18)) FactoryGirl.create(:hide_event, created_at: Time.utc(2016, 1, 24)) + FactoryGirl.create(:edit_event, created_at: Time.utc(2016, 1, 18)) + FactoryGirl.create(:edit_event, created_at: Time.utc(2016, 1, 11)) + FactoryGirl.create(:hide_event, created_at: Time.utc(2016, 1, 7)) + FactoryGirl.create(:hide_event, created_at: Time.utc(2016, 1, 4)) expect(InfoRequestEvent.count_of_hides_by_week).to eql( - { - "2016-01-04" => 2, - "2016-01-18" => 1 - } + [ + ["2016-01-04", 2], + ["2016-01-18", 1] + ] ) end end From a13f5524d3d58cc8e48c6a747111f4751222fc20 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Tue, 18 Oct 2016 15:22:30 +1100 Subject: [PATCH 34/50] Add request hides by week to statistics API --- app/controllers/statistics_controller.rb | 6 +++++- app/models/statistics.rb | 11 +++++++++++ spec/models/statistics_spec.rb | 17 +++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/app/controllers/statistics_controller.rb b/app/controllers/statistics_controller.rb index a5ce8a995b9..5b0998e1a81 100644 --- a/app/controllers/statistics_controller.rb +++ b/app/controllers/statistics_controller.rb @@ -6,13 +6,17 @@ def index @public_bodies = Statistics.public_bodies @users = Statistics.users + @request_hides_by_week = Statistics.by_week_with_noughts(InfoRequestEvent.count_of_hides_by_week) respond_to do |format| format.html format.json do render json: { public_bodies: @public_bodies, - users: Statistics.user_json_for_api(@users) + users: Statistics.user_json_for_api(@users), + requests: { + hides_by_week: @request_hides_by_week + } } end end diff --git a/app/models/statistics.rb b/app/models/statistics.rb index 0bc0126c6fe..1aaa8128637 100644 --- a/app/models/statistics.rb +++ b/app/models/statistics.rb @@ -124,5 +124,16 @@ def user_json_for_api(user_statistics) user_statistics[k] = v.map { |u,c| { user: u.json_for_api, count: c } } end end + + def by_week_with_noughts(counts_by_week) + earliest_week = counts_by_week.first.first.to_date.at_beginning_of_week + latest_week = counts_by_week.to_a.last.first.to_date.at_beginning_of_week + + (earliest_week..latest_week).step(7) do |date| + counts_by_week << [date.to_s, 0] unless counts_by_week.any? { |c| c.first == date.to_s } + end + + counts_by_week.sort_by(&:first) + end end end diff --git a/spec/models/statistics_spec.rb b/spec/models/statistics_spec.rb index a81f6780162..be26fb9088f 100644 --- a/spec/models/statistics_spec.rb +++ b/spec/models/statistics_spec.rb @@ -119,4 +119,21 @@ ) end end + + describe ".by_week_with_noughts" do + it "adds missing weeks with noughts" do + data = [ + ["2016-01-04", 2], + ["2016-01-18", 1] + ] + + expect(Statistics.by_week_with_noughts(data)).to eql( + [ + ["2016-01-04", 2], + ["2016-01-11", 0], + ["2016-01-18", 1] + ] + ) + end + end end From 79c4c4febcfa657d01ce9bdda217bbd462e4d112 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Tue, 18 Oct 2016 15:42:12 +1100 Subject: [PATCH 35/50] Show every week since the first request was made until today --- app/controllers/statistics_controller.rb | 5 ++++- app/models/statistics.rb | 6 +++--- spec/models/statistics_spec.rb | 22 ++++++++++++++-------- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/app/controllers/statistics_controller.rb b/app/controllers/statistics_controller.rb index 5b0998e1a81..edb2f7e7e40 100644 --- a/app/controllers/statistics_controller.rb +++ b/app/controllers/statistics_controller.rb @@ -6,7 +6,10 @@ def index @public_bodies = Statistics.public_bodies @users = Statistics.users - @request_hides_by_week = Statistics.by_week_with_noughts(InfoRequestEvent.count_of_hides_by_week) + @request_hides_by_week = Statistics.by_week_to_today_with_noughts( + InfoRequestEvent.count_of_hides_by_week, + InfoRequest.visible.order(:created_at).first.created_at + ) respond_to do |format| format.html diff --git a/app/models/statistics.rb b/app/models/statistics.rb index 1aaa8128637..e77378a997e 100644 --- a/app/models/statistics.rb +++ b/app/models/statistics.rb @@ -125,9 +125,9 @@ def user_json_for_api(user_statistics) end end - def by_week_with_noughts(counts_by_week) - earliest_week = counts_by_week.first.first.to_date.at_beginning_of_week - latest_week = counts_by_week.to_a.last.first.to_date.at_beginning_of_week + def by_week_to_today_with_noughts(counts_by_week, start_date) + earliest_week = start_date.to_date.at_beginning_of_week + latest_week = Date.today.at_beginning_of_week (earliest_week..latest_week).step(7) do |date| counts_by_week << [date.to_s, 0] unless counts_by_week.any? { |c| c.first == date.to_s } diff --git a/spec/models/statistics_spec.rb b/spec/models/statistics_spec.rb index be26fb9088f..e2323395dd5 100644 --- a/spec/models/statistics_spec.rb +++ b/spec/models/statistics_spec.rb @@ -120,20 +120,26 @@ end end - describe ".by_week_with_noughts" do + describe ".by_week_to_today_with_noughts" do it "adds missing weeks with noughts" do data = [ ["2016-01-04", 2], ["2016-01-18", 1] ] + date_from = Date.new(2015, 12, 28) + fake_current_date = Date.new(2016, 1, 31) - expect(Statistics.by_week_with_noughts(data)).to eql( - [ - ["2016-01-04", 2], - ["2016-01-11", 0], - ["2016-01-18", 1] - ] - ) + time_travel_to(fake_current_date) do + expect(Statistics.by_week_to_today_with_noughts(data, date_from)).to eql( + [ + ["2015-12-28", 0], + ["2016-01-04", 2], + ["2016-01-11", 0], + ["2016-01-18", 1], + ["2016-01-25", 0] + ] + ) + end end end end From 5fdf56148a3a5163bcd6295e7ee061a186f569c0 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Tue, 18 Oct 2016 16:20:25 +1100 Subject: [PATCH 36/50] Add start of the chart about hidden request events --- app/assets/javascripts/time_series.js | 206 ++++++++++++++++++ .../stylesheets/responsive/_time_series.scss | 113 ++++++++++ app/assets/stylesheets/responsive/all.scss | 2 + app/views/statistics/index.html.erb | 45 ++++ 4 files changed, 366 insertions(+) create mode 100644 app/assets/javascripts/time_series.js create mode 100644 app/assets/stylesheets/responsive/_time_series.scss diff --git a/app/assets/javascripts/time_series.js b/app/assets/javascripts/time_series.js new file mode 100644 index 00000000000..e2b2243e99d --- /dev/null +++ b/app/assets/javascripts/time_series.js @@ -0,0 +1,206 @@ +function timeSeries(selector, url, metric) { + + // Add the title + var wrapper_element = document.querySelectorAll(selector)[0]; + + var title_element = document.createElement('h4'); + title_element.classList.add("chart-title"); + title_element.textContent = "Number of " + metric + " over time"; + + wrapper_element.insertBefore(title_element, wrapper_element.firstChild); + + // Add the chart + d3.json(url, function(data) { + + // FIXME: Don't hardcode this to request hides by week + data = data["requests"]["hides_by_week"].map(function(d, i) { + return {key: new Date(d[0]), values: d[1]}; + }); + + var width = 600; + var barWidth = 5; + var height = 100; + var margin = { top: 20, right: 30, bottom: 100, left: 50 }; + var viewPortWidth = margin.left + width + margin.right; + var viewPortHeight = margin.top + height + margin.bottom; + + var maxYValue = d3.max(data, function(datum) { return datum.values; }); + + var x = d3.time.scale() + .domain( + d3.extent(data, function(datum) { return datum.key; }) + ) + .range([0, width]); + + var y = d3.scale + .linear() + .domain([0, maxYValue]) + .rangeRound([height, 0]); + + var bisectDate = d3.bisector(function(d) { return d.key; }).left + + var yTickCount; + if (maxYValue < 3) { + yTickCount = Math.floor(maxYValue); + } else { + yTickCount = 3; + } + + var yAxis = d3.svg.axis() + .scale(y) + .ticks(yTickCount) + .tickFormat(d3.format("s")) + .tickPadding(8) + .orient("left"); + + var xTickCount, + xDomainMonths = d3.time.months(x.domain()[0], x.domain()[1]).length; + if (xDomainMonths < 9) { + xTickCount = 2; + } else { + xTickCount = (d3.time.months, 10); + } + + var xAxisDateFormats = d3.time.format.multi([ + ["%_d %b", function(d) { return d.getDate() != 1; }], + ["%b", function(d) { return d.getMonth(); }], + ["%Y", function() { return true; }] + ]); + + var xAxis = d3.svg.axis() + .scale(x) + .ticks(xTickCount) + .tickFormat(xAxisDateFormats) + .tickPadding(8); + + var focusCallout = d3.select(selector) + .append("div") + .attr("class", "chart-callout") + .append("h5"), + focusCalloutValue = focusCallout.append("span") + .attr("class", "chart-callout-heading"), + focusCalloutDate = focusCallout.append("span") + .attr("class", "chart-callout-subheading"); + + // add the canvas to the DOM + var chart = d3.select(selector) + .attr("class", "chart chart-with-callout") + .append("svg:svg") + .attr("width", "100%") + .attr("height", "100%") + .attr("viewBox", "0 0 " + viewPortWidth + " " + viewPortHeight ) + .append("g") + .attr("transform", "translate(" + margin.left + "," + margin.top + ")"); + + var areaValues = d3.svg.area() + .x(function(d) { return x(d.key); }) + .y0(height) + .y1(function(d) { return y(d.values); }) + .interpolate("monotone"); + + var lineValues = d3.svg.line() + .x(function(d) { return x(d.key); }) + .y(function(d) { return y(d.values); }) + .interpolate("monotone"); + + chart.append("g") + .attr("class", "x-axis") + .attr("transform", "translate(0, " + (height + 8) + ")") + .call(xAxis); + + chart.append("g") + .attr("class", "y-axis") + .attr("transform", "translate(-8, 0)") + .call(yAxis); + + chart.append("line") + .attr("class", "x-axis-baseline") + .attr("x1", 0) + .attr("y1", height) + .attr("x2", width) + .attr("y2", height); + + chart.append("svg:path") + .attr("d", areaValues(data)) + .attr("class", "chart-area"); + + // Clip the line a y(0) so 0 values are more prominent + chart.append("clipPath") + .attr("id", "clip-above") + .append("rect") + .attr("width", width) + .attr("height", y(0) - 1); + + chart.append("clipPath") + .attr("id", "clip-below") + .append("rect") + .attr("y", y(0)) + .attr("width", width) + .attr("height", height); + + chart.selectAll(".chart-line") + .data(["above", "below"]) + .enter() + .append("path") + .attr("class", function(d) { return "chart-line chart-clipping-" + d; }) + .attr("clip-path", function(d) { return "url(#clip-" + d + ")"; }) + .datum(data) + .attr("d", lineValues); + + var focus = chart.append("g") + .attr("class", "focus"); + + focus.append("circle") + .attr("r", 5); + + focus.append("text") + .attr("x", 9) + .attr("dy", -10); + + focusDefault(); + + chart.append("rect") + .attr("class", "chart-overlay") + .attr("width", width) + .attr("height", height) + .on("mouseout", focusDefault) + .on("mousemove", mousemove); + + function focusDefault() { + var finalPoint = data[data.length - 1], + focusDefaultPosition = { + x: x(finalPoint.key), + y: y(finalPoint.values) + }; + + focus.attr("transform", "translate(" + focusDefaultPosition.x + "," + focusDefaultPosition.y + ")"); + focus.select("text").text(finalPoint.values); + + setCalloutText(finalPoint); + } + + function setCalloutText(point) { + var weekEnd = d3.time.week.offset(point.key, 1), + weekStartFormat = d3.time.format("%d %b"), + weekEndFormat = d3.time.format("%d %b %Y"), + dateSpan = weekStartFormat(point.key) + " – " + weekEndFormat(weekEnd); + + focusCalloutValue.text(point.values + " " + metric); + focusCalloutDate.text(dateSpan); + } + + function mousemove() { + var x0 = x.invert(d3.mouse(this)[0]), + i = bisectDate(data, x0, 1), + d0 = data[i - 1], + d1 = data[i], + d = x0 - d0.date > d1.date - x0 ? d1 : d0, + yValue = height - y(d.values); + focus.attr("transform", "translate(" + x(d.key) + "," + y(d.values) + ")"); + focus.select("text").text(d.values); + + setCalloutText(d); + } + }); + +} diff --git a/app/assets/stylesheets/responsive/_time_series.scss b/app/assets/stylesheets/responsive/_time_series.scss new file mode 100644 index 00000000000..6940155d45c --- /dev/null +++ b/app/assets/stylesheets/responsive/_time_series.scss @@ -0,0 +1,113 @@ +$chart-fill-color: lighten(#2688dc, 65%); +$chart-line-color: lighten(#2688dc, 20%); +.chart { + @include clearfix(); + + svg { + overflow: visible; + } + + text { + fill: #333; + font-size: .875em; + } + + .chart-area { + fill: $chart-fill-color; + } + + .y-axis, + .x-axis { + line { + stroke: #888; + stroke-width: 1px; + } + + .domain { + display: none; + } + } +} + +.chart-title { + width: 100%; + float: none; + clear: both; +} + +.x-axis-baseline { + fill: none; + stroke: #ccc; + stroke-width: 2px; +} + +.chart-line { + fill: none; + stroke: $chart-line-color; + stroke-width: 2px; + + &.chart-clipping-below { + display: none; + } +} + +.chart-area-stacked { + .axis { + line, + path { + fill: none; + stroke: #888; + stroke-width: 2px; + } + } +} + +.chart-with-callout { + svg { + width: 100%; + height: auto; + + @include respond-min(40em) { + float: left; + clear: left; + width: 80%; + } + } + + .chart-callout { + @include respond-min(40em) { + float: right; + width: 20%; + } + } + + text { + font-size: 1em; + + @include respond-min(40em) { + font-size: .875em; + } + } +} + +.chart-callout-heading { + display: block; +} + +.chart-callout-subheading { + display: block; + font-weight: normal; +} + +.chart-overlay { + fill: none; + pointer-events: all; +} + +.focus { + circle { + stroke: $chart-line-color; + stroke-width: 2px; + fill: transparent; + } +} diff --git a/app/assets/stylesheets/responsive/all.scss b/app/assets/stylesheets/responsive/all.scss index e64686db88e..3aeff8128d5 100644 --- a/app/assets/stylesheets/responsive/all.scss +++ b/app/assets/stylesheets/responsive/all.scss @@ -71,4 +71,6 @@ @import "_user_style"; @import "_user_layout"; +@import "_time_series"; + @import "custom"; diff --git a/app/views/statistics/index.html.erb b/app/views/statistics/index.html.erb index 628817f86c0..b309d45355c 100644 --- a/app/views/statistics/index.html.erb +++ b/app/views/statistics/index.html.erb @@ -7,6 +7,7 @@ 'to requests on {{site_name}} and ' \ 'meet the people most actively using it.', site_name: site_name) %>

    + <% # TODO: Add bit about hidden requests %>

    Public Bodies

    @@ -101,11 +102,55 @@
+

Requests

+ +

Hidden

+ +

TODO: explain what this means

+ +
+ + + + + + + + + <% @request_hides_by_week.each do |row| %> + + + + + <% end %> + +
Week startingRequest hide events
<%= row.first %><%= row.last %>
+
+ <% content_for :javascript do %> <%= javascript_include_tag "stats" %> + + <%= javascript_include_tag "https://cdnjs.cloudflare.com/ajax/libs/d3/3.5.12/d3.min.js" %> + <%= javascript_include_tag "time_series" %> + <% end %>
From 176e4be48a8a3a0affa58a00df6efcd44291a085 Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Tue, 18 Oct 2016 16:27:43 +1100 Subject: [PATCH 37/50] Don't hide table if JavaScript chart function isn't available --- app/views/statistics/index.html.erb | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/app/views/statistics/index.html.erb b/app/views/statistics/index.html.erb index b309d45355c..ca382adc4e5 100644 --- a/app/views/statistics/index.html.erb +++ b/app/views/statistics/index.html.erb @@ -137,19 +137,21 @@ <%= javascript_include_tag "https://cdnjs.cloudflare.com/ajax/libs/d3/3.5.12/d3.min.js" %> <%= javascript_include_tag "time_series" %> <% end %> From 38328153cb01d1bea5dbffb99c78863baabb0f6c Mon Sep 17 00:00:00 2001 From: Henare Degan Date: Tue, 18 Oct 2016 16:36:52 +1100 Subject: [PATCH 38/50] Don't hardcode time series chart to request hides by week We're now passing in JSON instead of a URL. --- app/assets/javascripts/time_series.js | 380 +++++++++++++------------- app/views/statistics/index.html.erb | 12 +- 2 files changed, 194 insertions(+), 198 deletions(-) diff --git a/app/assets/javascripts/time_series.js b/app/assets/javascripts/time_series.js index e2b2243e99d..2d0787a347b 100644 --- a/app/assets/javascripts/time_series.js +++ b/app/assets/javascripts/time_series.js @@ -1,4 +1,4 @@ -function timeSeries(selector, url, metric) { +function timeSeries(selector, data, metric) { // Add the title var wrapper_element = document.querySelectorAll(selector)[0]; @@ -9,198 +9,192 @@ function timeSeries(selector, url, metric) { wrapper_element.insertBefore(title_element, wrapper_element.firstChild); - // Add the chart - d3.json(url, function(data) { - - // FIXME: Don't hardcode this to request hides by week - data = data["requests"]["hides_by_week"].map(function(d, i) { - return {key: new Date(d[0]), values: d[1]}; - }); - - var width = 600; - var barWidth = 5; - var height = 100; - var margin = { top: 20, right: 30, bottom: 100, left: 50 }; - var viewPortWidth = margin.left + width + margin.right; - var viewPortHeight = margin.top + height + margin.bottom; - - var maxYValue = d3.max(data, function(datum) { return datum.values; }); - - var x = d3.time.scale() - .domain( - d3.extent(data, function(datum) { return datum.key; }) - ) - .range([0, width]); - - var y = d3.scale - .linear() - .domain([0, maxYValue]) - .rangeRound([height, 0]); - - var bisectDate = d3.bisector(function(d) { return d.key; }).left - - var yTickCount; - if (maxYValue < 3) { - yTickCount = Math.floor(maxYValue); - } else { - yTickCount = 3; - } - - var yAxis = d3.svg.axis() - .scale(y) - .ticks(yTickCount) - .tickFormat(d3.format("s")) - .tickPadding(8) - .orient("left"); - - var xTickCount, - xDomainMonths = d3.time.months(x.domain()[0], x.domain()[1]).length; - if (xDomainMonths < 9) { - xTickCount = 2; - } else { - xTickCount = (d3.time.months, 10); - } - - var xAxisDateFormats = d3.time.format.multi([ - ["%_d %b", function(d) { return d.getDate() != 1; }], - ["%b", function(d) { return d.getMonth(); }], - ["%Y", function() { return true; }] - ]); - - var xAxis = d3.svg.axis() - .scale(x) - .ticks(xTickCount) - .tickFormat(xAxisDateFormats) - .tickPadding(8); - - var focusCallout = d3.select(selector) - .append("div") - .attr("class", "chart-callout") - .append("h5"), - focusCalloutValue = focusCallout.append("span") - .attr("class", "chart-callout-heading"), - focusCalloutDate = focusCallout.append("span") - .attr("class", "chart-callout-subheading"); - - // add the canvas to the DOM - var chart = d3.select(selector) - .attr("class", "chart chart-with-callout") - .append("svg:svg") - .attr("width", "100%") - .attr("height", "100%") - .attr("viewBox", "0 0 " + viewPortWidth + " " + viewPortHeight ) - .append("g") - .attr("transform", "translate(" + margin.left + "," + margin.top + ")"); - - var areaValues = d3.svg.area() - .x(function(d) { return x(d.key); }) - .y0(height) - .y1(function(d) { return y(d.values); }) - .interpolate("monotone"); - - var lineValues = d3.svg.line() - .x(function(d) { return x(d.key); }) - .y(function(d) { return y(d.values); }) - .interpolate("monotone"); - - chart.append("g") - .attr("class", "x-axis") - .attr("transform", "translate(0, " + (height + 8) + ")") - .call(xAxis); - - chart.append("g") - .attr("class", "y-axis") - .attr("transform", "translate(-8, 0)") - .call(yAxis); - - chart.append("line") - .attr("class", "x-axis-baseline") - .attr("x1", 0) - .attr("y1", height) - .attr("x2", width) - .attr("y2", height); - - chart.append("svg:path") - .attr("d", areaValues(data)) - .attr("class", "chart-area"); - - // Clip the line a y(0) so 0 values are more prominent - chart.append("clipPath") - .attr("id", "clip-above") - .append("rect") - .attr("width", width) - .attr("height", y(0) - 1); - - chart.append("clipPath") - .attr("id", "clip-below") - .append("rect") - .attr("y", y(0)) - .attr("width", width) - .attr("height", height); - - chart.selectAll(".chart-line") - .data(["above", "below"]) - .enter() - .append("path") - .attr("class", function(d) { return "chart-line chart-clipping-" + d; }) - .attr("clip-path", function(d) { return "url(#clip-" + d + ")"; }) - .datum(data) - .attr("d", lineValues); - - var focus = chart.append("g") - .attr("class", "focus"); - - focus.append("circle") - .attr("r", 5); - - focus.append("text") - .attr("x", 9) - .attr("dy", -10); - - focusDefault(); - - chart.append("rect") - .attr("class", "chart-overlay") - .attr("width", width) - .attr("height", height) - .on("mouseout", focusDefault) - .on("mousemove", mousemove); - - function focusDefault() { - var finalPoint = data[data.length - 1], - focusDefaultPosition = { - x: x(finalPoint.key), - y: y(finalPoint.values) - }; - - focus.attr("transform", "translate(" + focusDefaultPosition.x + "," + focusDefaultPosition.y + ")"); - focus.select("text").text(finalPoint.values); - - setCalloutText(finalPoint); - } - - function setCalloutText(point) { - var weekEnd = d3.time.week.offset(point.key, 1), - weekStartFormat = d3.time.format("%d %b"), - weekEndFormat = d3.time.format("%d %b %Y"), - dateSpan = weekStartFormat(point.key) + " – " + weekEndFormat(weekEnd); - - focusCalloutValue.text(point.values + " " + metric); - focusCalloutDate.text(dateSpan); - } - - function mousemove() { - var x0 = x.invert(d3.mouse(this)[0]), - i = bisectDate(data, x0, 1), - d0 = data[i - 1], - d1 = data[i], - d = x0 - d0.date > d1.date - x0 ? d1 : d0, - yValue = height - y(d.values); - focus.attr("transform", "translate(" + x(d.key) + "," + y(d.values) + ")"); - focus.select("text").text(d.values); - - setCalloutText(d); - } + data = data.map(function(d, i) { + return {key: new Date(d[0]), values: d[1]}; }); + var width = 600; + var barWidth = 5; + var height = 100; + var margin = { top: 20, right: 30, bottom: 100, left: 50 }; + var viewPortWidth = margin.left + width + margin.right; + var viewPortHeight = margin.top + height + margin.bottom; + + var maxYValue = d3.max(data, function(datum) { return datum.values; }); + + var x = d3.time.scale() + .domain( + d3.extent(data, function(datum) { return datum.key; }) + ) + .range([0, width]); + + var y = d3.scale + .linear() + .domain([0, maxYValue]) + .rangeRound([height, 0]); + + var bisectDate = d3.bisector(function(d) { return d.key; }).left + + var yTickCount; + if (maxYValue < 3) { + yTickCount = Math.floor(maxYValue); + } else { + yTickCount = 3; + } + + var yAxis = d3.svg.axis() + .scale(y) + .ticks(yTickCount) + .tickFormat(d3.format("s")) + .tickPadding(8) + .orient("left"); + + var xTickCount, + xDomainMonths = d3.time.months(x.domain()[0], x.domain()[1]).length; + if (xDomainMonths < 9) { + xTickCount = 2; + } else { + xTickCount = (d3.time.months, 10); + } + + var xAxisDateFormats = d3.time.format.multi([ + ["%_d %b", function(d) { return d.getDate() != 1; }], + ["%b", function(d) { return d.getMonth(); }], + ["%Y", function() { return true; }] + ]); + + var xAxis = d3.svg.axis() + .scale(x) + .ticks(xTickCount) + .tickFormat(xAxisDateFormats) + .tickPadding(8); + + var focusCallout = d3.select(selector) + .append("div") + .attr("class", "chart-callout") + .append("h5"), + focusCalloutValue = focusCallout.append("span") + .attr("class", "chart-callout-heading"), + focusCalloutDate = focusCallout.append("span") + .attr("class", "chart-callout-subheading"); + + // add the canvas to the DOM + var chart = d3.select(selector) + .attr("class", "chart chart-with-callout") + .append("svg:svg") + .attr("width", "100%") + .attr("height", "100%") + .attr("viewBox", "0 0 " + viewPortWidth + " " + viewPortHeight ) + .append("g") + .attr("transform", "translate(" + margin.left + "," + margin.top + ")"); + + var areaValues = d3.svg.area() + .x(function(d) { return x(d.key); }) + .y0(height) + .y1(function(d) { return y(d.values); }) + .interpolate("monotone"); + + var lineValues = d3.svg.line() + .x(function(d) { return x(d.key); }) + .y(function(d) { return y(d.values); }) + .interpolate("monotone"); + + chart.append("g") + .attr("class", "x-axis") + .attr("transform", "translate(0, " + (height + 8) + ")") + .call(xAxis); + + chart.append("g") + .attr("class", "y-axis") + .attr("transform", "translate(-8, 0)") + .call(yAxis); + + chart.append("line") + .attr("class", "x-axis-baseline") + .attr("x1", 0) + .attr("y1", height) + .attr("x2", width) + .attr("y2", height); + + chart.append("svg:path") + .attr("d", areaValues(data)) + .attr("class", "chart-area"); + + // Clip the line a y(0) so 0 values are more prominent + chart.append("clipPath") + .attr("id", "clip-above") + .append("rect") + .attr("width", width) + .attr("height", y(0) - 1); + + chart.append("clipPath") + .attr("id", "clip-below") + .append("rect") + .attr("y", y(0)) + .attr("width", width) + .attr("height", height); + + chart.selectAll(".chart-line") + .data(["above", "below"]) + .enter() + .append("path") + .attr("class", function(d) { return "chart-line chart-clipping-" + d; }) + .attr("clip-path", function(d) { return "url(#clip-" + d + ")"; }) + .datum(data) + .attr("d", lineValues); + + var focus = chart.append("g") + .attr("class", "focus"); + + focus.append("circle") + .attr("r", 5); + + focus.append("text") + .attr("x", 9) + .attr("dy", -10); + + focusDefault(); + + chart.append("rect") + .attr("class", "chart-overlay") + .attr("width", width) + .attr("height", height) + .on("mouseout", focusDefault) + .on("mousemove", mousemove); + + function focusDefault() { + var finalPoint = data[data.length - 1], + focusDefaultPosition = { + x: x(finalPoint.key), + y: y(finalPoint.values) + }; + + focus.attr("transform", "translate(" + focusDefaultPosition.x + "," + focusDefaultPosition.y + ")"); + focus.select("text").text(finalPoint.values); + + setCalloutText(finalPoint); + } + + function setCalloutText(point) { + var weekEnd = d3.time.week.offset(point.key, 1), + weekStartFormat = d3.time.format("%d %b"), + weekEndFormat = d3.time.format("%d %b %Y"), + dateSpan = weekStartFormat(point.key) + " – " + weekEndFormat(weekEnd); + + focusCalloutValue.text(point.values + " " + metric); + focusCalloutDate.text(dateSpan); + } + + function mousemove() { + var x0 = x.invert(d3.mouse(this)[0]), + i = bisectDate(data, x0, 1), + d0 = data[i - 1], + d1 = data[i], + d = x0 - d0.date > d1.date - x0 ? d1 : d0, + yValue = height - y(d.values); + focus.attr("transform", "translate(" + x(d.key) + "," + y(d.values) + ")"); + focus.select("text").text(d.values); + + setCalloutText(d); + } } diff --git a/app/views/statistics/index.html.erb b/app/views/statistics/index.html.erb index ca382adc4e5..efc7fe0aefd 100644 --- a/app/views/statistics/index.html.erb +++ b/app/views/statistics/index.html.erb @@ -138,11 +138,13 @@ <%= javascript_include_tag "time_series" %>