Improve calculation of PublicBody statistics columns

On PublicBody, we don't need to update info_requests_count
because that's already done with :counter_cache.  On the
other hand, info_requests_successful_count and
info_requests_not_held_count can't be updated easily with
counter_cache (since they need conditions to be attached).
Instead we update them in post_save and post_destroy,
as suggested here:

   http://blog.douglasfshearer.com/post/17495285851/custom-counter-cache-with-conditions

This also adds tests to ensure that the
after_(save|destroy) callbacks are called and that they
modify the counts correctly.

4 files changed, 81 insertions, 19 deletions

diff --git a/app/models/info_request.rb b/app/models/info_request.rb
index 9bce2ca88..91bd37d9f 100644
--- a/app/models/info_request.rb
+++ b/app/models/info_request.rb
@@ -1125,6 +1125,17 @@ public
         end
     end
 
+    after_save :update_counter_cache
+    after_destroy :update_counter_cache
+    def update_counter_cache
+        self.public_body.info_requests_not_held_count = InfoRequest.where(
+            :public_body_id => self.public_body.id,
+            :described_state => 'not_held').count
+        self.public_body.info_requests_successful_count = InfoRequest.where(
+            :public_body_id => self.public_body.id,
+            :described_state => ['successful', 'partially_successful']).count
+    end
+
     def for_admin_column
       self.class.content_columns.map{|c| c unless %w(title url_title).include?(c.name) }.compact.each do |column|
         yield(column.human_name, self.send(column.name), column.type.to_s, column.name)
diff --git a/lib/tasks/stats.rake b/lib/tasks/stats.rake
index f7a3b07a5..1242575fe 100644
--- a/lib/tasks/stats.rake
+++ b/lib/tasks/stats.rake
@@ -94,25 +94,10 @@ namespace :stats do
   desc 'Update statistics in the public_bodies table'
   task :update_public_bodies_stats => :environment do
     PublicBody.all.each do |public_body|
-      puts "Finding statistics for #{public_body.name}"
-      [["info_requests_count=", nil],
-       ["info_requests_successful_count=", ['successful', 'partially_successful']],
-       ["info_requests_not_held_count=", ['not_held']]].each do |column, states|
-        puts "  Aggregating data for column #{column}"
-        where_clause = 'public_body_id = :pb'
-        parameters = {:pb => public_body.id}
-        if states
-          where_clause += " AND described_state in (:states)"
-          parameters[:states] = states
-        end
-        public_body.send(column,
-                         InfoRequest.where(where_clause,
-                                           parameters).count.to_s)
-      end
-      # Now looking for values of 'waiting_response_overdue' and
+      puts "Counting overdue requests for #{public_body.name}"
+      # Look for values of 'waiting_response_overdue' and
       # 'waiting_response_very_overdue' which aren't directly in the
-      # described_state column, and instead need to
-      puts "  Counting overdue requests"
+      # described_state column, and instead need to be calculated:
       overdue_count = 0
       very_overdue_count = 0
       InfoRequest.find_each(:conditions => {:public_body_id => public_body.id}) do |ir|
diff --git a/spec/controllers/public_body_controller_spec.rb b/spec/controllers/public_body_controller_spec.rb
index 000d1cf80..87544b96a 100644
--- a/spec/controllers/public_body_controller_spec.rb
+++ b/spec/controllers/public_body_controller_spec.rb
@@ -233,7 +233,7 @@ describe PublicBodyController, "when showing public body statistics" do
           graph['y_values'].should == [0, 50, 100, 100]
         end
         # Check that at least every confidence interval value is
-        # numeric:
+        # a Float (rather than NilClass, say):
         graph['cis_below'].each { |v| v.should be_instance_of(Float) }
         graph['cis_above'].each { |v| v.should be_instance_of(Float) }
       end
diff --git a/spec/models/info_request_spec.rb b/spec/models/info_request_spec.rb
index 3451e018f..3f2e02189 100644
--- a/spec/models/info_request_spec.rb
+++ b/spec/models/info_request_spec.rb
@@ -857,4 +857,70 @@ describe InfoRequest do
             end
         end
     end
+
+    describe 'when saving an info_request' do
+
+        before do
+            @info_request = InfoRequest.new(:external_url => 'http://www.example.com',
+                                            :external_user_name => 'Example User',
+                                            :title => 'Some request or other',
+                                            :public_body => public_bodies(:geraldine_public_body))
+        end
+
+        it "should call purge_in_cache and update_counter_cache" do
+            @info_request.should_receive(:purge_in_cache)
+            # Twice - once for save, once for destroy:
+            @info_request.should_receive(:update_counter_cache).twice
+            @info_request.save!
+            @info_request.destroy
+        end
+
+    end
+
+    describe 'when destroying an info_request' do
+
+        before do
+            @info_request = InfoRequest.new(:external_url => 'http://www.example.com',
+                                            :external_user_name => 'Example User',
+                                            :title => 'Some request or other',
+                                            :public_body => public_bodies(:geraldine_public_body))
+        end
+
+        it "should call update_counter_cache" do
+            @info_request.save!
+            @info_request.should_receive(:update_counter_cache)
+            @info_request.destroy
+        end
+
+    end
+
+    describe 'when changing a described_state' do
+
+        it "should change the counts on its PublicBody" do
+            pb = public_bodies(:geraldine_public_body)
+            old_successful_count = pb.info_requests_successful_count
+            old_not_held_count = pb.info_requests_not_held_count
+            ir = InfoRequest.new(:external_url => 'http://www.example.com',
+                                 :external_user_name => 'Example User',
+                                 :title => 'Some request or other',
+                                 :described_state => 'partially_successful',
+                                 :public_body => pb)
+            ir.save!
+            pb.info_requests_successful_count.should == (old_successful_count + 1)
+            ir.described_state = 'not_held'
+            ir.save!
+            pb.info_requests_successful_count.should == old_successful_count
+            pb.info_requests_not_held_count.should == (old_not_held_count + 1)
+            ir.described_state = 'successful'
+            ir.save!
+            pb.info_requests_successful_count.should == (old_successful_count + 1)
+            pb.info_requests_not_held_count.should == old_not_held_count
+            ir.destroy
+            pb.info_requests_successful_count.should == old_successful_count
+            pb.info_requests_successful_count.should == old_not_held_count
+        end
+
+    end
+
+
 end