From ec095bb901d970c102d3a71102e78c5572ab6731 Mon Sep 17 00:00:00 2001 From: lizconlan Date: Thu, 5 Nov 2015 17:25:32 +0000 Subject: [PATCH] Simplify the way InfoRequest looks for old_unclassified requests --- app/mailers/request_mailer.rb | 2 +- app/models/info_request.rb | 31 ++- .../admin_request/_some_requests.html.erb | 2 +- app/views/admin_request/show.html.erb | 2 +- spec/fixtures/info_requests.yml | 5 + spec/mailers/request_mailer_spec.rb | 27 +-- spec/models/info_request_spec.rb | 176 ++++++++++++------ 7 files changed, 157 insertions(+), 88 deletions(-) diff --git a/app/mailers/request_mailer.rb b/app/mailers/request_mailer.rb index 5221ef09a7..625ae2065b 100644 --- a/app/mailers/request_mailer.rb +++ b/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? diff --git a/app/models/info_request.rb b/app/models/info_request.rb index 9b725726a6..36ca97e1ea 100644 --- a/app/models/info_request.rb +++ b/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 diff --git a/app/views/admin_request/_some_requests.html.erb b/app/views/admin_request/_some_requests.html.erb index 3cc05e3e3c..1bf07a67cc 100644 --- a/app/views/admin_request/_some_requests.html.erb +++ b/app/views/admin_request/_some_requests.html.erb @@ -17,7 +17,7 @@ <%= h name %> - <% 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 %> diff --git a/app/views/admin_request/show.html.erb b/app/views/admin_request/show.html.erb index c72addda57..0d57cd0f19 100644 --- a/app/views/admin_request/show.html.erb +++ b/app/views/admin_request/show.html.erb @@ -30,7 +30,7 @@ <%= name %>: - <% 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 %> diff --git a/spec/fixtures/info_requests.yml b/spec/fixtures/info_requests.yml index d523236b5e..78790b3962 100644 --- a/spec/fixtures/info_requests.yml +++ b/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 diff --git a/spec/mailers/request_mailer_spec.rb b/spec/mailers/request_mailer_spec.rb index e5f23a7679..94ae299c3f 100644 --- a/spec/mailers/request_mailer_spec.rb +++ b/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 diff --git a/spec/models/info_request_spec.rb b/spec/models/info_request_spec.rb index 13b0264a0d..b2b4a33511 100644 --- a/spec/models/info_request_spec.rb +++ b/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