For percentage stats, exclude hidden or unclassified requests

The WDTK volunteers pointed out that it's not fair to include
hidden requests in the denominator, since they're typically hidden
for a good reason (e.g. being vexatious, spam, etc.), and we have
no information about those that are awaiting_description (i.e.
unclassified) so they should be excluded as well.

3 files changed, 56 insertions, 1 deletions

diff --git a/app/models/public_body.rb b/app/models/public_body.rb
index 7f8356dcf..ac2daf279 100644
--- a/app/models/public_body.rb
+++ b/app/models/public_body.rb
@@ -677,7 +677,7 @@ class PublicBody < ActiveRecord::Base
     # percentage.  This only returns data for those public bodies with
     # at least 'minimum_requests' requests.
     def self.get_request_percentages(column, n, highest, minimum_requests)
-        total_column = "info_requests_count"
+        total_column = "info_requests_visible_classified_count"
         ordering = "y_value"
         ordering += " DESC" if highest
         y_value_column = "(cast(#{column} as float) / #{total_column})"
diff --git a/spec/models/public_body_spec.rb b/spec/models/public_body_spec.rb
index 714934ee3..3ef4c71e7 100644
--- a/spec/models/public_body_spec.rb
+++ b/spec/models/public_body_spec.rb
@@ -530,3 +530,33 @@ describe PublicBody, " when override all public body request emails set" do
         @geraldine.request_email.should == "catch_all_test_email@foo.com"
     end
 end
+
+describe PublicBody, "when calculating statistics" do
+    it "should not include unclassified or hidden requests in percentages" do
+        with_hidden_and_successful_requests do
+            totals_data = PublicBody.get_request_totals(n=3,
+                                                        highest=true,
+                                                        minimum_requests=1)
+            # For the total number of requests, we still include
+            # hidden or unclassified requests:
+            totals_data['public_bodies'][-1].name.should == "Geraldine Quango"
+            totals_data['totals'][-1].should == 4
+
+            # However, for percentages, don't include the hidden or
+            # unclassified requests.  So, for the Geraldine Quango
+            # we've made sure that there are only two visible and
+            # classified requests, one of which is successful, so the
+            # percentage should be 50%:
+
+            percentages_data = PublicBody.get_request_percentages(column='info_requests_successful_count',
+                                                                  n=3,
+                                                                  highest=false,
+                                                                  minimum_requests=1)
+            geraldine_index = percentages_data['public_bodies'].index do |pb|
+                pb.name == "Geraldine Quango"
+            end
+
+            percentages_data['y_values'][geraldine_index].should == 50
+        end
+    end
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 6e65018f1..9d16f6387 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -158,6 +158,31 @@ Spork.prefork do
     ActiveRecord::Base.default_timezone = old_zone
   end
 
+  # To test the statistics calculations, it's helpful to have the
+  # request fixtures in different states, but changing the fixtures
+  # themselves disrupts many other tests.  This function takes a
+  # block, and runs that block with the info requests for the
+  # Geraldine Quango altered so that one is hidden and there's a
+  # successful one.
+  def with_hidden_and_successful_requests
+    external = info_requests(:external_request)
+    chicken = info_requests(:naughty_chicken_request)
+    old_external_prominence = external.prominence
+    old_chicken_described_state = chicken.described_state
+    begin
+      external.prominence = 'hidden'
+      external.save!
+      chicken.described_state = 'successful'
+      chicken.save!
+      yield
+    ensure
+      external.prominence = old_external_prominence
+      external.save!
+      chicken.described_state = old_chicken_described_state
+      chicken.save!
+    end
+  end
+
   def load_test_categories
       PublicBodyCategories.add(:en, [
           "Local and regional",