Simplify the way InfoRequest looks for old_unclassified requests

app/mailers/request_mailer.rb

@@ -318,7 +318,7 @@ def self.alert_new_response_reminders_internal(days_since, type_code)
                                                       :include => [:user],
                                                       :age_in_days => days_since)
 
-    for info_request in info_requests
+    info_requests.each do |info_request|
       alert_event_id = info_request.get_last_public_response_event_id
       last_response_message = info_request.get_last_public_response
       if alert_event_id.nil?

app/models/info_request.rb

@@ -927,12 +927,14 @@ def self.last_event_time_clause(event_type=nil, join_table=nil, join_clause=nil)
   end
 
   def self.last_public_response_clause
+    # TODO: Deprecate this method
     join_clause = "incoming_messages.id = info_request_events.incoming_message_id
                        AND incoming_messages.prominence = 'normal'"
     last_event_time_clause('response', 'incoming_messages', join_clause)
   end
 
-  def self.old_unclassified_params(extra_params, include_last_response_time=false)
+  def self.old_unclassified_params_old(extra_params, include_last_response_time=false)
+    # TODO: Remove this method post benchmark testing
     last_response_created_at = last_public_response_clause
     age = extra_params[:age_in_days] ? extra_params[:age_in_days].days : OLD_AGE_IN_DAYS
     params = { :conditions => ["awaiting_description = ?
@@ -947,6 +949,19 @@ def self.old_unclassified_params(extra_params, include_last_response_time=false)
     params
   end
 
+  def self.old_unclassified_params(extra_params, include_last_response_time=false)
+    age = extra_params[:age_in_days] ? extra_params[:age_in_days].days : OLD_AGE_IN_DAYS
+    params = { :conditions => ["awaiting_description = ?
+                                    AND last_public_response_at < ?
+                                    AND url_title != 'holding_pen'
+                                    AND user_id IS NOT NULL",
+                                      true, Time.zone.now - age] }
+    if include_last_response_time
+      params[:order] = 'last_public_response_at'
+    end
+    return params
+  end
+
   def self.count_old_unclassified(extra_params={})
     params = old_unclassified_params(extra_params)
     add_conditions_from_extra_params(params, extra_params)
@@ -974,6 +989,20 @@ def self.find_old_unclassified(extra_params={})
     find(:all, params)
   end
 
+  def self.find_old_unclassified_old(extra_params={})
+    # TODO: Remove this method post benchmark testing
+    params = old_unclassified_params_old(extra_params, include_last_response_time=true)
+    [:limit, :include, :offset].each do |extra|
+      params[extra] = extra_params[extra] if extra_params[extra]
+    end
+    if extra_params[:order]
+      params[:order] = extra_params[:order]
+      params.delete(:select)
+    end
+    add_conditions_from_extra_params(params, extra_params)
+    find(:all, params)
+  end
+
   def self.download_zip_dir
     File.join(Rails.root, "cache", "zips", "#{Rails.env}")
   end

app/views/admin_request/_some_requests.html.erb

@@ -17,7 +17,7 @@
             <%= h name %>
           </span>
           <span class="span6">
-            <% if type == 'datetime' %>
+            <% if type == 'datetime' && value %>
               <%= I18n.l(value, :format => "%e %B %Y %H:%M:%S") %>
               (<%= _('{{length_of_time}} ago', :length_of_time => time_ago_in_words(value)) %>)
             <% else %>

app/views/admin_request/show.html.erb

@@ -30,7 +30,7 @@
                   <b><%= name %>:</b>
                 </td>
                 <td>
-                  <% if type == 'datetime' %>
+                  <% if type == 'datetime' && value %>
                     <%= I18n.l(value, :format => "%e %B %Y %H:%M:%S") %>
                     (<%= _('{{length_of_time}} ago', :length_of_time => time_ago_in_words(value)) %>)
                   <% else %>

spec/fixtures/info_requests.yml

@@ -34,6 +34,7 @@ fancy_dog_request:
     described_state: waiting_response
     awaiting_description: true
     comments_allowed: true
+    last_public_response_at: 2007-11-13 18:09:20.042061
     idhash: 50929748
 naughty_chicken_request:
     id: 103
@@ -70,6 +71,7 @@ boring_request:
     described_state: successful
     awaiting_description: false
     comments_allowed: true
+    last_public_response_at: 2007-11-13 18:00:20
     idhash: 173fd003
 another_boring_request:
     id: 106
@@ -82,6 +84,7 @@ another_boring_request:
     described_state: successful
     awaiting_description: false
     comments_allowed: true
+    last_public_response_at: 2007-11-13 18:09:20.042061
     idhash: 173fd004
 
 # A pair of identical requests (with url_title differing only in the numeric suffix)
@@ -97,6 +100,7 @@ spam_1_request:
     described_state: successful
     awaiting_description: false
     comments_allowed: false
+    last_public_response_at: 2001-01-03 01:23:45.6789100
     idhash: 173fd005
 spam_2_request:
     id: 108
@@ -120,6 +124,7 @@ external_request:
     described_state: waiting_response
     awaiting_description: false
     comments_allowed: true
+    last_public_response_at: 2001-01-03 02:23:45.6789100
     idhash: a1234567
 anonymous_external_request:
     id: 110

spec/mailers/request_mailer_spec.rb

@@ -289,28 +289,11 @@ def send_alerts
     RequestMailer.alert_new_response_reminders_internal(7, 'new_response_reminder_1')
   end
 
-  it 'should ask for all requests that are awaiting description and whose latest response is older
-        than the number of days given and that are not the holding pen' do
-    expected_conditions = [ "awaiting_description = ?
-                                 AND (SELECT info_request_events.created_at
-                                      FROM info_request_events, incoming_messages
-                                       WHERE info_request_events.info_request_id = info_requests.id
-                                       AND info_request_events.event_type = 'response'
-                                       AND incoming_messages.id = info_request_events.incoming_message_id
-                                       AND incoming_messages.prominence = 'normal'
-                                      ORDER BY created_at desc LIMIT 1) < ?
-                                 AND url_title != 'holding_pen'
-                                 AND user_id IS NOT NULL".split(' ').join(' '),
-                            true, Time.now - 7.days ]
-
-    # compare the query string ignoring any spacing differences
-    expect(InfoRequest).to receive(:find) { |all, query_params|
-      query_string = query_params[:conditions][0]
-      query_params[:conditions][0] = query_string.split(' ').join(' ')
-      expect(query_params[:conditions]).to eq(expected_conditions)
-      expect(query_params[:include]).to eq([ :user ])
-      expect(query_params[:order]).to eq('info_requests.id')
-    }.and_return [@mock_request]
+  it 'should pass the RequestMailer specific params to InfoRequest' do
+    expect(InfoRequest).to receive(:find_old_unclassified).with(
+      :order => 'info_requests.id',
+      :include => [:user],
+      :age_in_days => 7).and_call_original
 
     send_alerts
   end

spec/models/info_request_spec.rb

@@ -1203,85 +1203,137 @@
 
   describe 'when asked for old unclassified requests' do
 
-    before do
-      allow(Time).to receive(:now).and_return(Time.utc(2007, 11, 9, 23, 59))
-    end
-
     it 'asks for requests using any limit param supplied' do
-      expect(InfoRequest).to receive(:find).with(:all, {:select => anything,
-                                                    :order => anything,
-                                                    :conditions=> anything,
-                                                    :limit => 5})
+      expect(InfoRequest).to receive(:find).
+        with(:all, hash_including(:limit => 5))
       InfoRequest.find_old_unclassified(:limit => 5)
     end
 
     it 'asks for requests using any offset param supplied' do
-      expect(InfoRequest).to receive(:find).with(:all, {:select => anything,
-                                                    :order => anything,
-                                                    :conditions=> anything,
-                                                    :offset => 100})
+      expect(InfoRequest).to receive(:find).
+        with(:all, hash_including(:offset => 100))
       InfoRequest.find_old_unclassified(:offset => 100)
     end
 
     it 'does not limit the number of requests returned by default' do
-      expect(InfoRequest).not_to receive(:find).with(:all, {:select => anything,
-                                                        :order => anything,
-                                                        :conditions=> anything,
-                                                        :limit => anything})
+      expect(InfoRequest).to receive(:find).
+        with(:all, hash_excluding(:limit => anything))
       InfoRequest.find_old_unclassified
     end
 
     it 'adds extra conditions if supplied' do
-      expected_conditions = ["awaiting_description = ?
-                                    AND (SELECT info_request_events.created_at
-                                         FROM info_request_events, incoming_messages
-                                         WHERE info_request_events.info_request_id = info_requests.id
-                                         AND info_request_events.event_type = 'response'
-                                         AND incoming_messages.id = info_request_events.incoming_message_id
-                                         AND incoming_messages.prominence = 'normal'
-                                         ORDER BY created_at desc LIMIT 1) < ?
-                                    AND url_title != 'holding_pen'
-                                    AND user_id IS NOT NULL
-                                    AND prominence != 'backpage'".split(' ').join(' '),
-                             true, Time.now - 21.days]
-      # compare conditions ignoring whitespace differences
-      expect(InfoRequest).to receive(:find) do |all, query_params|
-        query_string = query_params[:conditions][0]
-        query_params[:conditions][0] = query_string.split(' ').join(' ')
-        expect(query_params[:conditions]).to eq(expected_conditions)
-      end
+      expect(InfoRequest).to receive(:find).
+        with(:all, hash_including(
+          {:conditions => include(/prominence != 'backpage'/)}))
       InfoRequest.find_old_unclassified({:conditions => ["prominence != 'backpage'"]})
     end
 
-    it 'asks the database for requests that are awaiting description, have a last public response older
-        than 21 days old, have a user, are not the holding pen and are not backpaged' do
-      expected_conditions = ["awaiting_description = ?
-                                    AND (SELECT info_request_events.created_at
-                                         FROM info_request_events, incoming_messages
-                                         WHERE info_request_events.info_request_id = info_requests.id
-                                         AND info_request_events.event_type = 'response'
-                                         AND incoming_messages.id = info_request_events.incoming_message_id
-                                         AND incoming_messages.prominence = 'normal'
-                                         ORDER BY created_at desc LIMIT 1) < ?
-                                    AND url_title != 'holding_pen'
-                                    AND user_id IS NOT NULL".split(' ').join(' '),
-                             true, Time.now - 21.days]
-      expected_select = "*, (SELECT info_request_events.created_at
-                                   FROM info_request_events, incoming_messages
-                                   WHERE info_request_events.info_request_id = info_requests.id
-                                   AND info_request_events.event_type = 'response'
-                                   AND incoming_messages.id = info_request_events.incoming_message_id
-                                   AND incoming_messages.prominence = 'normal'
-                                   ORDER BY created_at desc LIMIT 1)
-                                   AS last_response_time".split(' ').join(' ')
-      expect(InfoRequest).to receive(:find) do |all, query_params|
-        query_string = query_params[:conditions][0]
-        query_params[:conditions][0] = query_string.split(' ').join(' ')
-        expect(query_params[:conditions]).to eq(expected_conditions)
-        expect(query_params[:select].split(' ').join(' ')).to eq(expected_select)
-        expect(query_params[:order]).to eq("last_response_time")
+    context "returning records" do
+      let(:recent_date) { Time.zone.now - 20.days }
+      let(:old_date) { Time.zone.now - 22.days }
+      let(:user) { FactoryGirl.create(:user) }
+
+      def create_recent_unclassified_request
+        request = FactoryGirl.create(:info_request, :user => user,
+                                                    :created_at => recent_date)
+        message = FactoryGirl.create(:incoming_message, :created_at => recent_date,
+                                                        :info_request => request)
+        FactoryGirl.create(:info_request_event, :incoming_message => message,
+                                                :event_type => "response",
+                                                :info_request => request,
+                                                :created_at => recent_date)
+        request.awaiting_description = true
+        request.save
+        request
+      end
+
+      def create_old_unclassified_request
+        request = FactoryGirl.create(:info_request, :user => user,
+                                                    :created_at => old_date)
+        message = FactoryGirl.create(:incoming_message, :created_at => old_date,
+                                                        :info_request => request)
+        FactoryGirl.create(:info_request_event, :incoming_message => message,
+                                                :event_type => "response",
+                                                :info_request => request,
+                                                :created_at => old_date)
+        request.awaiting_description = true
+        request.save
+        request
+      end
+
+      def create_old_unclassified_described
+        request = FactoryGirl.create(:info_request, :user => user,
+                                                    :created_at => old_date)
+        message = FactoryGirl.create(:incoming_message, :created_at => old_date,
+                                                        :info_request => request)
+        FactoryGirl.create(:info_request_event, :incoming_message => message,
+                                                :event_type => "response",
+                                                :info_request => request,
+                                                :created_at => old_date)
+        request
+      end
+
+      def create_old_unclassified_no_user
+        request = FactoryGirl.create(:info_request, :user => nil,
+                                                    :external_user_name => 'test_user',
+                                                    :external_url => 'test',
+                                                    :created_at => old_date)
+        message = FactoryGirl.create(:incoming_message, :created_at => old_date,
+                                                        :info_request => request)
+        FactoryGirl.create(:info_request_event, :incoming_message => message,
+                                                :event_type => "response",
+                                                :info_request => request,
+                                                :created_at => old_date)
+        request.awaiting_description = true
+        request.save
+        request
+      end
+
+      def create_old_unclassified_holding_pen
+        request = FactoryGirl.create(:info_request, :user => user,
+                                                    :url_title => 'holding_pen',
+                                                    :created_at => old_date)
+        message = FactoryGirl.create(:incoming_message, :created_at => old_date,
+                                                        :info_request => request)
+        FactoryGirl.create(:info_request_event, :incoming_message => message,
+                                                :event_type => "response",
+                                                :info_request => request,
+                                                :created_at => old_date)
+        request.awaiting_description = true
+        request.save
+        request
+      end
+
+
+      it "returns records over 21 days old" do
+        old_unclassified_request = create_old_unclassified_request
+        results = InfoRequest.find_old_unclassified
+        expect(results).to include(old_unclassified_request)
+      end
+
+      it "does not return records less than 21 days old" do
+        recent_unclassified_request = create_recent_unclassified_request
+        results = InfoRequest.find_old_unclassified
+        expect(results).not_to include(recent_unclassified_request)
+      end
+
+      it "only returns records with an associated user" do
+        old_unclassified_no_user = create_old_unclassified_no_user
+        results = InfoRequest.find_old_unclassified
+        expect(results).not_to include(old_unclassified_no_user)
+      end
+
+      it "only returns records which are awaiting description" do
+        old_unclassified_described = create_old_unclassified_described
+        results = InfoRequest.find_old_unclassified
+        expect(results).not_to include(old_unclassified_described)
+      end
+
+      it "does not return anything which is in the holding pen" do
+        old_unclassified_holding_pen = create_old_unclassified_holding_pen
+        results = InfoRequest.find_old_unclassified
+        expect(results).not_to include(old_unclassified_holding_pen)
       end
-      InfoRequest.find_old_unclassified
     end
 
   end