diff options
| -rw-r--r-- | app/controllers/request_controller.rb | 12 | ||||
| -rw-r--r-- | app/helpers/application_helper.rb | 13 | ||||
| -rw-r--r-- | app/models/info_request.rb | 17 | ||||
| -rw-r--r-- | app/views/request/_list_results.html.erb | 12 | ||||
| -rw-r--r-- | app/views/request/list.html.erb | 22 | ||||
| -rw-r--r-- | spec/controllers/request_controller_spec.rb | 86 | ||||
| -rw-r--r-- | spec/models/info_request_spec.rb | 96 | ||||
| -rw-r--r-- | spec/views/request/list.html.erb_spec.rb | 49 |
8 files changed, 149 insertions, 158 deletions
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb index 04d8f06b0..649075568 100644 --- a/app/controllers/request_controller.rb +++ b/app/controllers/request_controller.rb @@ -141,7 +141,10 @@ class RequestController < ApplicationController def list medium_cache @view = params[:view] + @locale = self.locale_from_params() @page = get_search_page_from_params if !@page # used in cache case, as perform_search sets @page as side effect + @per_page = PER_PAGE + @max_results = MAX_RESULTS if @view == "recent" return redirect_to request_list_all_url(:action => "list", :view => "all", :page => @page), :status => :moved_permanently end @@ -151,16 +154,11 @@ class RequestController < ApplicationController raise ActiveRecord::RecordNotFound.new("Sorry. No pages after #{MAX_RESULTS / PER_PAGE}.") end - query = InfoRequestEvent.make_query_from_params(params.merge(:latest_status => @view)) + @filters = params.merge(:latest_status => @view) @title = _("View and search requests") - sortby = "newest" - xapian_object = perform_search([InfoRequestEvent], query, sortby, 'request_collapse') - @list_results = xapian_object.results.map { |r| r[:model] } - @matches_estimated = xapian_object.matches_estimated - @show_no_more_than = (@matches_estimated > MAX_RESULTS) ? MAX_RESULTS : @matches_estimated @title = @title + " (page " + @page.to_s + ")" if (@page > 1) - @track_thing = TrackThing.create_track_for_search_query(query) + @track_thing = TrackThing.create_track_for_search_query(InfoRequestEvent.make_query_from_params(@filters)) @feed_autodetect = [ { :url => do_track_url(@track_thing, 'feed'), :title => @track_thing.params[:title_in_rss], :has_json => true } ] # Don't let robots go more than 20 pages in diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 4b603b064..154697377 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -123,5 +123,18 @@ module ApplicationHelper yield end end + + # We only want to cache request lists that have a reasonable chance of not expiring + # before they're requested again. Don't cache lists returned from specific searches + # or anything except the first page of results, just the first page of the default + # views + def request_list_cache_key + cacheable_param_list = ['controller', 'action', 'locale', 'view'] + if params.keys.all?{ |key| cacheable_param_list.include?(key) } + "request-list-#{@view}-#{@locale}" + else + nil + end + end end diff --git a/app/models/info_request.rb b/app/models/info_request.rb index 0a073dc79..4624cefaf 100644 --- a/app/models/info_request.rb +++ b/app/models/info_request.rb @@ -1228,6 +1228,23 @@ public return [xapian_similar, xapian_similar_more] end + def InfoRequest.request_list(filters, page, per_page, max_results) + xapian_object = ActsAsXapian::Search.new([InfoRequestEvent], + InfoRequestEvent.make_query_from_params(filters), + :offset => (page - 1) * per_page, + :limit => 25, + :sort_by_prefix => 'created_at', + :sort_by_ascending => true, + :collapse_by_prefix => 'request_collapse' + ) + list_results = xapian_object.results.map { |r| r[:model] } + matches_estimated = xapian_object.matches_estimated + show_no_more_than = [matches_estimated, max_results].min + return { :results => list_results, + :matches_estimated => matches_estimated, + :show_no_more_than => show_no_more_than } + end + def InfoRequest.recent_requests request_events = [] request_events_all_successful = false diff --git a/app/views/request/_list_results.html.erb b/app/views/request/_list_results.html.erb new file mode 100644 index 000000000..4da042816 --- /dev/null +++ b/app/views/request/_list_results.html.erb @@ -0,0 +1,12 @@ + <% @results = InfoRequest.request_list(@filters, @page, @per_page, @max_results) %> + <% if @results[:results].empty? %> + <p> <%= _('No requests of this sort yet.')%></p> + <% else %> + <h2 class="foi_results"><%= _('{{count}} FOI requests found', :count => @results[:matches_estimated]) %></h2> + <div class="results_block"> + <% @results[:results].each do |result| %> + <%= render :partial => 'request/request_listing_via_event', :locals => { :event => result, :info_request => result.info_request } %> + <% end %> + </div> + <% end %> + <%= will_paginate WillPaginate::Collection.new(@page, @per_page, @results[:show_no_more_than]) %> diff --git a/app/views/request/list.html.erb b/app/views/request/list.html.erb index 062b77c3e..a465f03ba 100644 --- a/app/views/request/list.html.erb +++ b/app/views/request/list.html.erb @@ -14,21 +14,11 @@ <div style="clear:both"></div> <div class="results_section"> - <% # TODO: Cache for 5 minutes %> - <% if @list_results.empty? %> - <p> <%= _('No requests of this sort yet.')%></p> - <% else %> - <h2 class="foi_results"><%= _('{{count}} FOI requests found', :count => @matches_estimated) %></h2> - <div class="results_block"> - <% for result in @list_results%> - <% if result.class.to_s == 'InfoRequestEvent' %> - <%= render :partial => 'request/request_listing_via_event', :locals => { :event => result, :info_request => result.info_request } %> - <% else %> - <p><strong><%= _('Unexpected search result type') %> <%=result.class.to_s%></strong></p> - <% end %> - <% end %> - </div> + <% if key = request_list_cache_key %> + <% cache_if_caching_fragments(key, :expires_in => 5.minutes) do %> + <%= render :partial => 'list_results' %> <% end %> - - <%= will_paginate WillPaginate::Collection.new(@page, @per_page, @show_no_more_than) %> + <% else %> + <%= render :partial => 'list_results' %> + <% end %> </div> diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb index def9dfc7e..23f19f389 100644 --- a/spec/controllers/request_controller_spec.rb +++ b/spec/controllers/request_controller_spec.rb @@ -17,92 +17,6 @@ describe RequestController, "when listing recent requests" do response.should render_template('list') end - it "should filter requests" do - get :list, :view => 'all' - assigns[:list_results].map(&:info_request).should =~ InfoRequest.all - - # default sort order is the request with the most recently created event first - assigns[:list_results].map(&:info_request).should == InfoRequest.all( - :order => "(select max(info_request_events.created_at) from info_request_events where info_request_events.info_request_id = info_requests.id) DESC") - - get :list, :view => 'successful' - assigns[:list_results].map(&:info_request).should =~ InfoRequest.all( - :conditions => "id in ( - select info_request_id - from info_request_events - where not exists ( - select * - from info_request_events later_events - where later_events.created_at > info_request_events.created_at - and later_events.info_request_id = info_request_events.info_request_id - and later_events.described_state is not null - ) - and info_request_events.described_state in ('successful', 'partially_successful') - )") - end - - it "should filter requests by date" do - # The semantics of the search are that it finds any InfoRequest - # that has any InfoRequestEvent created in the specified range - - get :list, :view => 'all', :request_date_before => '13/10/2007' - assigns[:list_results].map(&:info_request).should =~ InfoRequest.all( - :conditions => "id in (select info_request_id from info_request_events where created_at < '2007-10-13'::date)") - - get :list, :view => 'all', :request_date_after => '13/10/2007' - assigns[:list_results].map(&:info_request).should =~ InfoRequest.all( - :conditions => "id in (select info_request_id from info_request_events where created_at > '2007-10-13'::date)") - - get :list, :view => 'all', :request_date_after => '13/10/2007', :request_date_before => '01/11/2007' - assigns[:list_results].map(&:info_request).should =~ InfoRequest.all( - :conditions => "id in (select info_request_id from info_request_events where created_at between '2007-10-13'::date and '2007-11-01'::date)") - end - - it "should list internal_review requests as unresolved ones" do - get :list, :view => 'awaiting' - - # This doesn’t precisely duplicate the logic of the actual - # query, but it is close enough to give the same result with - # the current set of test data. - assigns[:list_results].should =~ InfoRequestEvent.all( - :conditions => "described_state in ( - 'waiting_response', 'waiting_clarification', - 'internal_review', 'gone_postal', 'error_message', 'requires_admin' - ) and not exists ( - select * - from info_request_events later_events - where later_events.created_at > info_request_events.created_at - and later_events.info_request_id = info_request_events.info_request_id - )") - - - get :list, :view => 'awaiting' - assigns[:list_results].map(&:info_request).include?(info_requests(:fancy_dog_request)).should == false - - event = info_request_events(:useless_incoming_message_event) - event.described_state = event.calculated_state = "internal_review" - event.save! - rebuild_xapian_index - - get :list, :view => 'awaiting' - assigns[:list_results].map(&:info_request).include?(info_requests(:fancy_dog_request)).should == true - end - - it "should assign the first page of results" do - xap_results = mock(ActsAsXapian::Search, - :results => (1..25).to_a.map { |m| { :model => m } }, - :matches_estimated => 1000000) - - ActsAsXapian::Search.should_receive(:new). - with([InfoRequestEvent]," (variety:sent OR variety:followup_sent OR variety:response OR variety:comment)", - :sort_by_prefix => "created_at", :offset => 0, :limit => 25, :sort_by_ascending => true, - :collapse_by_prefix => "request_collapse"). - and_return(xap_results) - get :list, :view => 'all' - assigns[:list_results].size.should == 25 - assigns[:show_no_more_than].should == RequestController::MAX_RESULTS - end - it "should return 404 for pages we don't want to serve up" do xap_results = mock(ActsAsXapian::Search, :results => (1..25).to_a.map { |m| { :model => m } }, diff --git a/spec/models/info_request_spec.rb b/spec/models/info_request_spec.rb index dcc94e967..ade75e2cc 100644 --- a/spec/models/info_request_spec.rb +++ b/spec/models/info_request_spec.rb @@ -1177,4 +1177,100 @@ describe InfoRequest do request_events.map(&:info_request).select{|x|x.url_title =~ /^spam/}.length.should == 1 end end + + describe InfoRequest, "when constructing a list of requests by query" do + + before(:each) do + get_fixtures_xapian_index + end + + def apply_filters(filters) + results = InfoRequest.request_list(filters, page=1, per_page=100, max_results=100) + results[:results].map(&:info_request) + end + + it "should filter requests" do + apply_filters(:latest_status => 'all').should =~ InfoRequest.all + + # default sort order is the request with the most recently created event first + apply_filters(:latest_status => 'all').should == InfoRequest.all( + :order => "(SELECT max(info_request_events.created_at) + FROM info_request_events + WHERE info_request_events.info_request_id = info_requests.id) + DESC") + + apply_filters(:latest_status => 'successful').should =~ InfoRequest.all( + :conditions => "id in ( + SELECT info_request_id + FROM info_request_events + WHERE NOT EXISTS ( + SELECT * + FROM info_request_events later_events + WHERE later_events.created_at > info_request_events.created_at + AND later_events.info_request_id = info_request_events.info_request_id + AND later_events.described_state IS NOT null + ) + AND info_request_events.described_state IN ('successful', 'partially_successful') + )") + + end + + it "should filter requests by date" do + # The semantics of the search are that it finds any InfoRequest + # that has any InfoRequestEvent created in the specified range + filters = {:latest_status => 'all', :request_date_before => '13/10/2007'} + apply_filters(filters).should =~ InfoRequest.all( + :conditions => "id IN (SELECT info_request_id + FROM info_request_events + WHERE created_at < '2007-10-13'::date)") + + filters = {:latest_status => 'all', :request_date_after => '13/10/2007'} + apply_filters(filters).should =~ InfoRequest.all( + :conditions => "id IN (SELECT info_request_id + FROM info_request_events + WHERE created_at > '2007-10-13'::date)") + + filters = {:latest_status => 'all', + :request_date_after => '13/10/2007', + :request_date_before => '01/11/2007'} + apply_filters(filters).should =~ InfoRequest.all( + :conditions => "id IN (SELECT info_request_id + FROM info_request_events + WHERE created_at BETWEEN '2007-10-13'::date + AND '2007-11-01'::date)") + end + + + it "should list internal_review requests as unresolved ones" do + + # This doesn’t precisely duplicate the logic of the actual + # query, but it is close enough to give the same result with + # the current set of test data. + results = apply_filters(:latest_status => 'awaiting') + results.should =~ InfoRequest.all( + :conditions => "id IN (SELECT info_request_id + FROM info_request_events + WHERE described_state in ( + 'waiting_response', 'waiting_clarification', + 'internal_review', 'gone_postal', 'error_message', 'requires_admin' + ) and not exists ( + select * + from info_request_events later_events + where later_events.created_at > info_request_events.created_at + and later_events.info_request_id = info_request_events.info_request_id + ))") + + + results.include?(info_requests(:fancy_dog_request)).should == false + + event = info_request_events(:useless_incoming_message_event) + event.described_state = event.calculated_state = "internal_review" + event.save! + rebuild_xapian_index + results = apply_filters(:latest_status => 'awaiting') + results.include?(info_requests(:fancy_dog_request)).should == true + end + + + end end diff --git a/spec/views/request/list.html.erb_spec.rb b/spec/views/request/list.html.erb_spec.rb deleted file mode 100644 index 521d946bc..000000000 --- a/spec/views/request/list.html.erb_spec.rb +++ /dev/null @@ -1,49 +0,0 @@ -require File.expand_path(File.join('..', '..', '..', 'spec_helper'), __FILE__) - -describe "request/list" do - - before do - assign :page, 1 - assign :per_page, 10 - end - - def make_mock_event - return mock_model(InfoRequestEvent, - :info_request => mock_model(InfoRequest, - :title => 'Title', - :url_title => 'title', - :display_status => 'awaiting_response', - :calculate_status => 'awaiting_response', - :public_body => mock_model(PublicBody, :name => 'Test Quango', :url_name => 'testquango'), - :user => mock_model(User, :name => 'Test User', :url_name => 'testuser'), - :is_external? => false - ), - :incoming_message => nil, :is_incoming_message? => false, - :outgoing_message => nil, :is_outgoing_message? => false, - :comment => nil, :is_comment? => false, - :event_type => 'sent', - :created_at => Time.now - 4.days, - :search_text_main => '' - ) - end - - it "should be successful" do - assign :list_results, [ make_mock_event, make_mock_event ] - assign :matches_estimated, 2 - assign :show_no_more_than, 100 - render - response.should have_selector("div.request_listing") - response.should_not have_selector("p", :content => "No requests of this sort yet") - end - - it "should cope with no results" do - assign :list_results, [ ] - assign :matches_estimated, 0 - assign :show_no_more_than, 0 - render - response.should have_selector("p", :content => "No requests of this sort yet") - response.should_not have_selector("div.request_listing") - end - -end - |