Make old_unclassfied function a bit more flexible for reuse elsewhere e.g. in mailing reminders to requesters

6 files changed, 67 insertions, 39 deletions

diff --git a/app/controllers/admin_general_controller.rb b/app/controllers/admin_general_controller.rb
index cbb381e21..bcfd11e03 100644
--- a/app/controllers/admin_general_controller.rb
+++ b/app/controllers/admin_general_controller.rb
@@ -4,7 +4,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: admin_general_controller.rb,v 1.4 2009-04-08 10:45:33 louise Exp $
+# $Id: admin_general_controller.rb,v 1.5 2009-04-08 16:13:11 louise Exp $
 
 class AdminGeneralController < AdminController
     def index
@@ -19,7 +19,9 @@ class AdminGeneralController < AdminController
         @requires_admin_requests = InfoRequest.find(:all, :select => '*, ' + InfoRequest.last_event_time_clause + ' as last_event_time', :conditions => ["described_state = 'requires_admin'"], :order => "last_event_time")
         @error_message_requests = InfoRequest.find(:all, :select => '*, ' + InfoRequest.last_event_time_clause + ' as last_event_time', :conditions => ["described_state = 'error_message'"], :order => "last_event_time")
         @blank_contacts = PublicBody.find(:all, :conditions => ["request_email = ''"], :order => "updated_at")
-        @ten_days_old_unclassified = InfoRequest.find_old_unclassified(limit=50)
+        @ten_days_old_unclassified = InfoRequest.find_old_unclassified(:limit => 50, 
+                                                                       :conditions => ["prominence != 'backpage'"],
+                                                                       :age_in_days => 10)
         @holding_pen_messages = InfoRequest.holding_pen_request.incoming_messages
     end
 
diff --git a/app/controllers/admin_request_controller.rb b/app/controllers/admin_request_controller.rb
index 15faacac2..dfd44f32b 100644
--- a/app/controllers/admin_request_controller.rb
+++ b/app/controllers/admin_request_controller.rb
@@ -4,7 +4,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: admin_request_controller.rb,v 1.33 2009-04-08 10:45:33 louise Exp $
+# $Id: admin_request_controller.rb,v 1.34 2009-04-08 16:13:11 louise Exp $
 
 class AdminRequestController < AdminController
     def index
@@ -19,7 +19,8 @@ class AdminRequestController < AdminController
     end
 
     def list_old_unclassified
-        @info_requests = InfoRequest.find_old_unclassified
+        @info_requests = InfoRequest.find_old_unclassified(:conditions => ["prominence != 'backpage'"],
+                                                           :age_in_days => 10)
     end
 
     def show
diff --git a/app/models/info_request.rb b/app/models/info_request.rb
index 7ea355a18..fed3d8801 100644
--- a/app/models/info_request.rb
+++ b/app/models/info_request.rb
@@ -23,7 +23,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: info_request.rb,v 1.184 2009-04-08 10:45:34 louise Exp $
+# $Id: info_request.rb,v 1.185 2009-04-08 16:13:11 louise Exp $
 
 require 'digest/sha1'
 require File.join(File.dirname(__FILE__),'../../vendor/plugins/acts_as_xapian/lib/acts_as_xapian')
@@ -74,7 +74,7 @@ class InfoRequest < ActiveRecord::Base
         'eir', # Environmental Information Regulations
     ]
     
-    OLD_AGE_IN_DAYS = 10.days
+    OLD_AGE_IN_DAYS = 14.days
 
     def after_initialize
         if self.described_state.nil?
@@ -672,25 +672,39 @@ public
     end
 
     # Used to find when event last changed
-    def InfoRequest.last_event_time_clause 
-        '(select created_at from info_request_events where info_request_events.info_request_id = info_requests.id order by created_at desc limit 1)'
-    end
-        
-    def InfoRequest.find_old_unclassified(limit=nil)
-        params = {:select => "*, #{last_event_time_clause} as last_event_time", 
-                  :conditions => ["awaiting_description = ? and #{last_event_time_clause} < ? and prominence != 'backpage'", 
-                                 true, Time.now() - OLD_AGE_IN_DAYS], 
-                                 :order => "last_event_time"}
-        params[:limit] = limit if limit 
+    def InfoRequest.last_event_time_clause(event_type=nil)
+        event_type_clause = ''
+        event_type_clause = " and info_request_events.event_type = '#{event_type}'" if event_type
+        "(select created_at from info_request_events where info_request_events.info_request_id = info_requests.id#{event_type_clause} order by created_at desc limit 1)"
+    end
+
+    def InfoRequest.find_old_unclassified(extra_params={})
+        last_response_created_at = last_event_time_clause('response')
+        age = extra_params[:age_in_days] ? extra_params[:age_in_days].days : OLD_AGE_IN_DAYS
+        params = {:select => "*, #{last_response_created_at} as last_response_time", 
+                  :conditions => ["awaiting_description = ? and #{last_response_created_at} < ? and url_title != 'holding_pen'", 
+                                 true, Time.now() - age], 
+                                 :order => "last_response_time"}
+        params[:limit] = extra_params[:limit] if extra_params[:limit]
+        params[:include] = extra_params[:include] if extra_params[:include]
+        if extra_params[:order]
+            params[:order] = extra_params[:order] 
+            params.delete(:select)
+        end
+        if extra_params[:conditions]
+            condition_string = extra_params[:conditions].shift
+            params[:conditions][0] += " and #{condition_string}"
+            params[:conditions] += extra_params[:conditions]
+        end
         find(:all, params)
     end
     
     def is_old_unclassified?
         return false if !awaiting_description
-        return false if prominence == 'backpage'
-        last_event = get_last_event
-        return false unless last_event
-        return false if last_event.created_at >= Time.now - OLD_AGE_IN_DAYS
+        return false if url_title == 'holding_pen'
+        last_response_event = get_last_response_event
+        return false unless last_response_event
+        return false if last_response_event.created_at >= Time.now - OLD_AGE_IN_DAYS
         return true
     end
 
diff --git a/app/views/admin_general/index.rhtml b/app/views/admin_general/index.rhtml
index 87de83752..636a36c48 100644
--- a/app/views/admin_general/index.rhtml
+++ b/app/views/admin_general/index.rhtml
@@ -39,10 +39,11 @@
     <% for @request in @ten_days_old_unclassified %>
         <li>
             <%= request_both_links(@request) %> 
-            &ndash; <%=simple_date(@request.get_last_event.created_at)%>
+            &ndash; <%=simple_date(@request.get_last_response_event.created_at)%>
         </li>
     <% end %>
     </ul>
+    
     <p>(<%= link_to "Full list", admin_url("unclassified") %>)
 <% end %>
 
diff --git a/app/views/admin_request/list_old_unclassified.rhtml b/app/views/admin_request/list_old_unclassified.rhtml
index 3db40528e..38f2822e0 100644
--- a/app/views/admin_request/list_old_unclassified.rhtml
+++ b/app/views/admin_request/list_old_unclassified.rhtml
@@ -1,4 +1,4 @@
-<% @title = "Unclassfied responses" %>
+<% @title = "Unclassified responses" %>
 
 <h1><%=@title%></h1>
 
@@ -7,7 +7,7 @@
 <% for @request in @info_requests %>
     <li>
         <%= request_both_links(@request) %> 
-        &ndash; <%=simple_date(@request.get_last_event.created_at)%>
+        &ndash; <%=simple_date(@request.get_last_response_event.created_at)%>
     </li>
 <% end %>
 </ul>
diff --git a/spec/models/info_request_spec.rb b/spec/models/info_request_spec.rb
index cbcf7eecc..09b258ee4 100644
--- a/spec/models/info_request_spec.rb
+++ b/spec/models/info_request_spec.rb
@@ -195,7 +195,7 @@ describe InfoRequest do
                                                           :order => anything, 
                                                           :conditions=> anything, 
                                                           :limit => 5})
-            InfoRequest.find_old_unclassified(limit=5)
+            InfoRequest.find_old_unclassified(:limit => 5)
         end
         
         it 'should not limit the number of requests returned by default' do 
@@ -206,12 +206,21 @@ describe InfoRequest do
             InfoRequest.find_old_unclassified
         end 
         
-        it 'should ask the database for requests that are awaiting description, older than 10 days old and not backpaged' do 
+        it 'should add extra conditions if supplied' do 
             InfoRequest.should_receive(:find).with(:all, 
-                  {:select=>"*, (select created_at from info_request_events where info_request_events.info_request_id = info_requests.id order by created_at desc limit 1) as last_event_time", 
-                   :order=>"last_event_time", 
-                   :conditions=>["awaiting_description = ? and (select created_at from info_request_events where info_request_events.info_request_id = info_requests.id order by created_at desc limit 1) < ? and prominence != 'backpage'", 
-                       true, Time.now - 10.days]})
+                  {:select=> anything, 
+                   :order=> anything, 
+                   :conditions=>["awaiting_description = ? and (select created_at from info_request_events where info_request_events.info_request_id = info_requests.id and info_request_events.event_type = 'response' order by created_at desc limit 1) < ? and url_title != 'holding_pen' and prominence != 'backpage'", 
+                    true, Time.now - 14.days]})
+            InfoRequest.find_old_unclassified({:conditions => ["prominence != 'backpage'"]})
+        end
+        
+        it 'should ask the database for requests that are awaiting description, have a last response older than 14 days old, are not the holding pen and are not backpaged' do 
+            InfoRequest.should_receive(:find).with(:all, 
+                  {:select=>"*, (select created_at from info_request_events where info_request_events.info_request_id = info_requests.id and info_request_events.event_type = 'response' order by created_at desc limit 1) as last_response_time", 
+                   :order=>"last_response_time", 
+                   :conditions=>["awaiting_description = ? and (select created_at from info_request_events where info_request_events.info_request_id = info_requests.id and info_request_events.event_type = 'response' order by created_at desc limit 1) < ? and url_title != 'holding_pen'", 
+                    true, Time.now - 14.days]})
             InfoRequest.find_old_unclassified
         end
         
@@ -221,28 +230,29 @@ describe InfoRequest do
         
         before do 
             Time.stub!(:now).and_return(Time.utc(2007, 11, 12, 23, 59))
-            @mock_event = mock_model(InfoRequestEvent, :created_at => Time.now - 11.days)
+            @mock_comment_event = mock_model(InfoRequestEvent, :created_at => Time.now - 16.days, :event_type => 'comment')
+            @mock_response_event = mock_model(InfoRequestEvent, :created_at => Time.now - 15.days, :event_type => 'response')
             @info_request = InfoRequest.new(:prominence => 'normal', 
                                             :awaiting_description => true, 
-                                            :info_request_events => [@mock_event])
+                                            :info_request_events => [@mock_response_event, @mock_comment_event])
         end
         
-        it 'should return false if it is not awaiting description' do 
-            @info_request.stub!(:awaiting_description).and_return(false)
+        it 'should return false if it is the holding pen' do 
+            @info_request.stub!(:url_title).and_return('holding_pen')
             @info_request.is_old_unclassified?.should be_false
         end
         
-        it 'should return false if it\'s last event occurred less than 10 days ago' do 
-            @mock_event.stub!(:created_at).and_return(Time.now - 9.days)
+        it 'should return false if it is not awaiting description' do 
+            @info_request.stub!(:awaiting_description).and_return(false)
             @info_request.is_old_unclassified?.should be_false
         end
         
-        it 'should return false if it\'s backpaged' do 
-            @info_request.stub!(:prominence).and_return('backpage')
-            @info_request.is_old_unclassified?.should be_false  
+        it 'should return false if it\'s last response event occurred less than 14 days ago' do 
+            @mock_response_event.stub!(:created_at).and_return(Time.now - 13.days)
+            @info_request.is_old_unclassified?.should be_false
         end
         
-        it 'should return true if it is awaiting description, hasn\'t had an event in 10 days and is not backpaged' do 
+        it 'should return true if it is awaiting description, isn\'t the holding pen and hasn\'t had an event in 14 days' do 
             @info_request.is_old_unclassified?.should be_true
         end