Consolidate and expand statistics page #3544

Merged
merged 50 commits into from Nov 16, 2016

Conversation

Projects
None yet
5 participants
@henare
Contributor

henare commented Oct 17, 2016

This pull request moves the public body statistics page onto a general statistics page and adds two new sections:

  1. The Humans of Right To Know user statistics added to Right To Know as part of openaustralia/righttoknow#645
  2. A chart that tracks hidden requests over time, see #3545

For more information about the reasoning of these changes, please see the issues linked above.

As part of making these changes we've done the following supporting work:

  • 3b43c31 redirect requests to old public body statistics page to new general statistics page
  • Extract a Statistics model, controller and assets files to tidy things up
  • All the statistics are available nested under the same API
  • db72f31 When an administrator uses the backend to only update the prominence of a request to hide it we now treat this as a hide event, not a regular edit event. As mentioned on #3543, we've been using this method exclusively to hide requests - this change (and the following migration) makes them correctly appear as hide events
  • 03a559f Add a migration to update event types of existing hide events
  • fa148ec Small bugfix to allow params yaml of an InfoRequestEvent to be blank instead of raising an exception

Fixes #1107.
Fixes #3545.

alaveteli_statistics

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 17, 2016

Coverage Status

Coverage increased (+0.09%) to 91.78% when pulling 20d5fe3 on openaustralia:statistics into 3e8be2c on mysociety:develop.

coveralls commented Oct 17, 2016

Coverage Status

Coverage increased (+0.09%) to 91.78% when pulling 20d5fe3 on openaustralia:statistics into 3e8be2c on mysociety:develop.

@henare henare changed the title from Add people statistics and consolidate statistics page to Consolidate and expand statistics page Oct 19, 2016

@henare

This comment has been minimized.

Show comment
Hide comment
@henare

henare Oct 19, 2016

Contributor

Apologies for the epic pull request 🗻 but we've decided to update this to also incorporate our recent work on #3545 as it actually seems like it will be easier to review it all in one go :shipit:

We've tried to describe all the major changes we've done along the way but if you have any questions then please ping @equivalentideas or I.

Contributor

henare commented Oct 19, 2016

Apologies for the epic pull request 🗻 but we've decided to update this to also incorporate our recent work on #3545 as it actually seems like it will be easier to review it all in one go :shipit:

We've tried to describe all the major changes we've done along the way but if you have any questions then please ping @equivalentideas or I.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 19, 2016

Coverage Status

Coverage increased (+0.07%) to 91.768% when pulling db2dd72 on openaustralia:statistics into 3e8be2c on mysociety:develop.

coveralls commented Oct 19, 2016

Coverage Status

Coverage increased (+0.07%) to 91.768% when pulling db2dd72 on openaustralia:statistics into 3e8be2c on mysociety:develop.

@lizconlan

Great work! I'm just going to add some notes to the CHANGES file and make a quick tweak for something that's going to be a deployment issue for us (I caused it last time!) before I get this merged in. 🏆

spec/models/statistics_spec.rb
- highest=true,
- minimum_requests=1)
- @percentages_data = PublicBody.get_request_percentages(
+ let(:raw_count_data) do

This comment has been minimized.

@lizconlan

lizconlan Nov 14, 2016

Member

yay! 🎉

@lizconlan

lizconlan Nov 14, 2016

Member

yay! 🎉

app/views/general/statistics.html.erb
@@ -1,6 +1,14 @@
<% @title = _("Statistics") %>
<div id="main_content" class="main_content">
- <h1><%= @title %></h1>
+ <h1><%= site_name %> <%= @title %></h1>

This comment has been minimized.

@lizconlan

lizconlan Nov 14, 2016

Member

Nice!

@@ -0,0 +1,11 @@
+class UpdateEventTypeWhenOnlyEditingProminenceToHide < ActiveRecord::Migration
+ def up
+ InfoRequestEvent.find_each do |event|

This comment has been minimized.

@lizconlan

lizconlan Nov 14, 2016

Member

This is a completely valid approach but it's proved problematic on WDTK recently as the database is large enough for an update migration to hold up a deploy. So on that basis I'm going to copy this code into a rake task instead.

@lizconlan

lizconlan Nov 14, 2016

Member

This is a completely valid approach but it's proved problematic on WDTK recently as the database is large enough for an update migration to hold up a deploy. So on that basis I'm going to copy this code into a rake task instead.

henare added some commits Oct 12, 2016

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.
Add a migration to update event types of existing hide events
In db72f31 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.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 14, 2016

Coverage Status

Coverage increased (+0.07%) to 92.043% when pulling 05dbf5c on openaustralia:statistics into dba64ca on mysociety:develop.

Coverage Status

Coverage increased (+0.07%) to 92.043% when pulling 05dbf5c on openaustralia:statistics into dba64ca on mysociety:develop.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 14, 2016

Coverage Status

Coverage increased (+0.07%) to 92.043% when pulling 05dbf5c on openaustralia:statistics into dba64ca on mysociety:develop.

Coverage Status

Coverage increased (+0.07%) to 92.043% when pulling 05dbf5c on openaustralia:statistics into dba64ca on mysociety:develop.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 14, 2016

Coverage Status

Coverage increased (+0.02%) to 91.987% when pulling 05dbf5c on openaustralia:statistics into dba64ca on mysociety:develop.

coveralls commented Nov 14, 2016

Coverage Status

Coverage increased (+0.02%) to 91.987% when pulling 05dbf5c on openaustralia:statistics into dba64ca on mysociety:develop.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 14, 2016

Coverage Status

Coverage increased (+0.04%) to 92.006% when pulling d5cf036 on openaustralia:statistics into dba64ca on mysociety:develop.

coveralls commented Nov 14, 2016

Coverage Status

Coverage increased (+0.04%) to 92.006% when pulling d5cf036 on openaustralia:statistics into dba64ca on mysociety:develop.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 14, 2016

Coverage Status

Coverage increased (+0.02%) to 91.987% when pulling d5cf036 on openaustralia:statistics into dba64ca on mysociety:develop.

coveralls commented Nov 14, 2016

Coverage Status

Coverage increased (+0.02%) to 91.987% when pulling d5cf036 on openaustralia:statistics into dba64ca on mysociety:develop.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 14, 2016

Coverage Status

Coverage increased (+0.03%) to 92.0% when pulling 267de79 on openaustralia:statistics into dba64ca on mysociety:develop.

coveralls commented Nov 14, 2016

Coverage Status

Coverage increased (+0.03%) to 92.0% when pulling 267de79 on openaustralia:statistics into dba64ca on mysociety:develop.

doc/CHANGES.md
+ annotators, and hidden requests. Includes a new event type of "hide" to
+ make tracking and reporting on hidden requests much simpler. Need to run
+ `rake temp:update_hide_event_type` to set up the data for this feature
+ (Henare Degan)

This comment has been minimized.

@henare

henare Nov 14, 2016

Contributor

Can we please change this to (Henare Degan, Luke Bacon)? Although the commits are attributed to me we paired on this entire feature.

@henare

henare Nov 14, 2016

Contributor

Can we please change this to (Henare Degan, Luke Bacon)? Although the commits are attributed to me we paired on this entire feature.

This comment has been minimized.

@lizconlan

lizconlan Nov 15, 2016

Member

Sure, I'll fix that right up - sorry @equivalentideas!

@lizconlan

lizconlan Nov 15, 2016

Member

Sure, I'll fix that right up - sorry @equivalentideas!

@henare

This comment has been minimized.

Show comment
Hide comment
@henare

henare Nov 14, 2016

Contributor

Amazing, thanks @lizconlan! 🌟 I've just left a little comment about attribution that I hope can be changed before this is merged 😄

Contributor

henare commented Nov 14, 2016

Amazing, thanks @lizconlan! 🌟 I've just left a little comment about attribution that I hope can be changed before this is merged 😄

@@ -1,6 +1,15 @@
# -*- coding: utf-8 -*-
namespace :temp do
+ desc 'Update EventType when only editing prominence to hide'

This comment has been minimized.

@equivalentideas

equivalentideas Nov 14, 2016

Collaborator

@lizconlan I'm interested in why you've moved this to a rake task? Is it a convention we should be following for future contributions, or something specific to what this does?

@equivalentideas

equivalentideas Nov 14, 2016

Collaborator

@lizconlan I'm interested in why you've moved this to a rake task? Is it a convention we should be following for future contributions, or something specific to what this does?

This comment has been minimized.

@lizconlan

lizconlan Nov 15, 2016

Member

Yeah, this is something we've only found really recently (when I did this) that affects large sites. If we do data updates in migrations on a large site then the deploy process has to wait until every row is updated, which turned out to be a longer wait than we realised. We ended up scrapping the deploy and moving the update part of the migration to a rake task so we could run it without holding up the deploy process.

So, yes please - data updates in lib/tasks/temp.rake to be run as one-offs but db structure changes in migrations as usual. (I think that makes sense, only one coffee in!)

But a really recent find so we've not written it up anywhere yet. Your commit predates our findings!

@lizconlan

lizconlan Nov 15, 2016

Member

Yeah, this is something we've only found really recently (when I did this) that affects large sites. If we do data updates in migrations on a large site then the deploy process has to wait until every row is updated, which turned out to be a longer wait than we realised. We ended up scrapping the deploy and moving the update part of the migration to a rake task so we could run it without holding up the deploy process.

So, yes please - data updates in lib/tasks/temp.rake to be run as one-offs but db structure changes in migrations as usual. (I think that makes sense, only one coffee in!)

But a really recent find so we've not written it up anywhere yet. Your commit predates our findings!

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 15, 2016

Coverage Status

Coverage decreased (-0.007%) to 91.963% when pulling 0eaba55 on openaustralia:statistics into dba64ca on mysociety:develop.

coveralls commented Nov 15, 2016

Coverage Status

Coverage decreased (-0.007%) to 91.963% when pulling 0eaba55 on openaustralia:statistics into dba64ca on mysociety:develop.

@lizconlan lizconlan merged commit 0eaba55 into mysociety:develop Nov 16, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.007%) to 91.963%
Details
hound No violations found. Woof!

@henare henare deleted the openaustralia:statistics branch Nov 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment