diff options
| -rw-r--r-- | app/controllers/application_controller.rb | 94 | ||||
| -rw-r--r-- | app/controllers/general_controller.rb | 2 | ||||
| -rw-r--r-- | app/controllers/public_body_controller.rb | 2 | ||||
| -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/models/info_request_event.rb | 4 | ||||
| -rw-r--r-- | app/views/request/_list_results.html.erb | 12 | ||||
| -rw-r--r-- | app/views/request/list.html.erb | 22 | ||||
| -rw-r--r-- | config/initializers/alaveteli.rb | 1 | ||||
| -rw-r--r-- | lib/xapian_queries.rb | 85 | ||||
| -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 |
14 files changed, 241 insertions, 254 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 161a82b26..7c122917d 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -428,100 +428,6 @@ class ApplicationController < ActionController::Base end end - def get_request_variety_from_params(params) - query = "" - sortby = "newest" - varieties = [] - if params[:request_variety] && !(query =~ /variety:/) - if params[:request_variety].include? "sent" - varieties -= ['variety:sent', 'variety:followup_sent', 'variety:response', 'variety:comment'] - varieties << ['variety:sent', 'variety:followup_sent'] - end - if params[:request_variety].include? "response" - varieties << ['variety:response'] - end - if params[:request_variety].include? "comment" - varieties << ['variety:comment'] - end - end - if !varieties.empty? - query = " (#{varieties.join(' OR ')})" - end - return query - end - - def get_status_from_params(params) - query = "" - if params[:latest_status] - statuses = [] - if params[:latest_status].class == String - params[:latest_status] = [params[:latest_status]] - end - if params[:latest_status].include?("recent") || params[:latest_status].include?("all") - query += " (variety:sent OR variety:followup_sent OR variety:response OR variety:comment)" - end - if params[:latest_status].include? "successful" - statuses << ['latest_status:successful', 'latest_status:partially_successful'] - end - if params[:latest_status].include? "unsuccessful" - statuses << ['latest_status:rejected', 'latest_status:not_held'] - end - if params[:latest_status].include? "awaiting" - statuses << ['latest_status:waiting_response', 'latest_status:waiting_clarification', 'waiting_classification:true', 'latest_status:internal_review','latest_status:gone_postal', 'latest_status:error_message', 'latest_status:requires_admin'] - end - if params[:latest_status].include? "internal_review" - statuses << ['status:internal_review'] - end - if params[:latest_status].include? "other" - statuses << ['latest_status:gone_postal', 'latest_status:error_message', 'latest_status:requires_admin', 'latest_status:user_withdrawn'] - end - if params[:latest_status].include? "gone_postal" - statuses << ['latest_status:gone_postal'] - end - if !statuses.empty? - query = " (#{statuses.join(' OR ')})" - end - end - return query - end - - def get_date_range_from_params(params) - query = "" - if params.has_key?(:request_date_after) && !params.has_key?(:request_date_before) - params[:request_date_before] = Time.now.strftime("%d/%m/%Y") - query += " #{params[:request_date_after]}..#{params[:request_date_before]}" - elsif !params.has_key?(:request_date_after) && params.has_key?(:request_date_before) - params[:request_date_after] = "01/01/2001" - end - if params.has_key?(:request_date_after) - query = " #{params[:request_date_after]}..#{params[:request_date_before]}" - end - return query - end - - def get_tags_from_params(params) - query = "" - tags = [] - if params.has_key?(:tags) - params[:tags].split().each do |tag| - tags << "tag:#{tag}" - end - end - if !tags.empty? - query = " (#{tags.join(' OR ')})" - end - return query - end - - def make_query_from_params(params) - query = params[:query] || "" if query.nil? - query += get_date_range_from_params(params) - query += get_request_variety_from_params(params) - query += get_status_from_params(params) - query += get_tags_from_params(params) - return query - end - def country_from_ip country = "" if !AlaveteliConfiguration::gaze_url.empty? diff --git a/app/controllers/general_controller.rb b/app/controllers/general_controller.rb index b01a67027..6f0d29889 100644 --- a/app/controllers/general_controller.rb +++ b/app/controllers/general_controller.rb @@ -103,7 +103,7 @@ class GeneralController < ApplicationController params[:query] = @query end if @variety_postfix != "all" && @requests - @query, _ = make_query_from_params(params) + @query = InfoRequestEvent.make_query_from_params(params) end @inputted_sortby = @sortby if @sortby.nil? diff --git a/app/controllers/public_body_controller.rb b/app/controllers/public_body_controller.rb index 308d38e4c..862f4b318 100644 --- a/app/controllers/public_body_controller.rb +++ b/app/controllers/public_body_controller.rb @@ -40,7 +40,7 @@ class PublicBodyController < ApplicationController @searched_to_send_request = true end @view = params[:view] - query = make_query_from_params(params.merge(:latest_status => @view)) + query = InfoRequestEvent.make_query_from_params(params.merge(:latest_status => @view)) query += " requested_from:#{@public_body.url_name}" # Use search query for this so can collapse and paginate easily # XXX really should just use SQL query here rather than Xapian. diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb index 6f80be7a6..b79ead73d 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 = 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 4eb64dc13..eadb66d54 100644 --- a/app/models/info_request.rb +++ b/app/models/info_request.rb @@ -1232,6 +1232,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/models/info_request_event.rb b/app/models/info_request_event.rb index e268b28ca..5eed5ba83 100644 --- a/app/models/info_request_event.rb +++ b/app/models/info_request_event.rb @@ -21,6 +21,9 @@ # Email: hello@mysociety.org; WWW: http://www.mysociety.org/ class InfoRequestEvent < ActiveRecord::Base + + extend XapianQueries + belongs_to :info_request validates_presence_of :info_request @@ -416,4 +419,5 @@ class InfoRequestEvent < ActiveRecord::Base yield(column.human_name, self.send(column.name), column.type.to_s, column.name) end end + end 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/config/initializers/alaveteli.rb b/config/initializers/alaveteli.rb index cabe96efa..6df1823a5 100644 --- a/config/initializers/alaveteli.rb +++ b/config/initializers/alaveteli.rb @@ -51,6 +51,7 @@ require 'alaveteli_file_types' require 'alaveteli_localization' require 'message_prominence' require 'theme' +require 'xapian_queries' AlaveteliLocalization.set_locales(AlaveteliConfiguration::available_locales, AlaveteliConfiguration::default_locale) diff --git a/lib/xapian_queries.rb b/lib/xapian_queries.rb new file mode 100644 index 000000000..b3599740a --- /dev/null +++ b/lib/xapian_queries.rb @@ -0,0 +1,85 @@ +module XapianQueries + + # These methods take some filter criteria expressed in a hash and convert them + # into a xapian query referencing the terms and values stored by InfoRequestEvent. + # Note that the params are request params and may contain irrelevant keys + + def get_request_variety_from_params(params) + query = "" + sortby = "newest" + varieties = [] + if params[:request_variety] && !(query =~ /variety:/) + if params[:request_variety].include? "sent" + varieties -= ['variety:sent', 'variety:followup_sent', 'variety:response', 'variety:comment'] + varieties << ['variety:sent', 'variety:followup_sent'] + end + if params[:request_variety].include? "response" + varieties << ['variety:response'] + end + if params[:request_variety].include? "comment" + varieties << ['variety:comment'] + end + end + if !varieties.empty? + query = " (#{varieties.join(' OR ')})" + end + return query + end + + def get_status_from_params(params) + query = "" + if params[:latest_status] + statuses = [] + if params[:latest_status].class == String + params[:latest_status] = [params[:latest_status]] + end + if params[:latest_status].include?("recent") || params[:latest_status].include?("all") + query += " (variety:sent OR variety:followup_sent OR variety:response OR variety:comment)" + end + if params[:latest_status].include? "successful" + statuses << ['latest_status:successful', 'latest_status:partially_successful'] + end + if params[:latest_status].include? "unsuccessful" + statuses << ['latest_status:rejected', 'latest_status:not_held'] + end + if params[:latest_status].include? "awaiting" + statuses << ['latest_status:waiting_response', 'latest_status:waiting_clarification', 'waiting_classification:true', 'latest_status:internal_review','latest_status:gone_postal', 'latest_status:error_message', 'latest_status:requires_admin'] + end + if params[:latest_status].include? "internal_review" + statuses << ['status:internal_review'] + end + if params[:latest_status].include? "other" + statuses << ['latest_status:gone_postal', 'latest_status:error_message', 'latest_status:requires_admin', 'latest_status:user_withdrawn'] + end + if params[:latest_status].include? "gone_postal" + statuses << ['latest_status:gone_postal'] + end + if !statuses.empty? + query = " (#{statuses.join(' OR ')})" + end + end + return query + end + + def get_date_range_from_params(params) + query = "" + if params.has_key?(:request_date_after) && !params.has_key?(:request_date_before) + params[:request_date_before] = Time.now.strftime("%d/%m/%Y") + query += " #{params[:request_date_after]}..#{params[:request_date_before]}" + elsif !params.has_key?(:request_date_after) && params.has_key?(:request_date_before) + params[:request_date_after] = "01/01/2001" + end + if params.has_key?(:request_date_after) + query = " #{params[:request_date_after]}..#{params[:request_date_before]}" + end + return query + end + + def make_query_from_params(params) + query = params[:query] || "" if query.nil? + query += get_date_range_from_params(params) + query += get_request_variety_from_params(params) + query += get_status_from_params(params) + return query + end +end 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 - |