From 0ac6375b51093b4ae32f97f1f6dbe6ed02353fda Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Thu, 11 Apr 2024 12:30:08 +0100 Subject: [PATCH 1/7] Refactor RequestController#list specs Remove unused double and there is no reason to load the raw emails. --- spec/controllers/request_controller_spec.rb | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb index f18b69692b..2d80c2a517 100644 --- a/spec/controllers/request_controller_spec.rb +++ b/spec/controllers/request_controller_spec.rb @@ -1,11 +1,6 @@ require 'spec_helper' RSpec.describe RequestController, "when listing recent requests" do - before(:each) do - load_raw_emails_data - update_xapian_index - end - it "should be successful" do get :list, params: { view: 'all' } expect(response).to be_successful @@ -17,11 +12,6 @@ end it "should return 404 for pages we don't want to serve up" do - xap_results = double( - ActsAsXapian::Search, - results: (1..25).to_a.map { |m| { model: m } }, - matches_estimated: 1_000_000 - ) expect { get :list, params: { view: 'all', page: 100 } }.to raise_error(ActiveRecord::RecordNotFound) From 01241b625f628b99be85748c1853557eb3465cc6 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Wed, 10 Apr 2024 16:37:13 +0100 Subject: [PATCH 2/7] Update request pagination setup Extract setup into before action hook. This will be reused in a new list by tag action. --- app/controllers/request_controller.rb | 36 ++++++++++++--------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb index cb23b6ce70..05c834f96d 100644 --- a/app/controllers/request_controller.rb +++ b/app/controllers/request_controller.rb @@ -14,6 +14,7 @@ class RequestController < ApplicationController before_action :set_info_request, only: [:show] before_action :redirect_new_form_to_pro_version, only: [:select_authority, :new] before_action :set_in_pro_area, only: [:select_authority, :show] + before_action :setup_results_pagination, only: [:list, :similar] helper_method :state_transitions_empty? @@ -110,13 +111,6 @@ def details # Requests similar to this one def similar short_cache - @per_page = 25 - @page = (params[:page] || "1").to_i - - # Later pages are very expensive to load - if @page > MAX_RESULTS / PER_PAGE - raise ActiveRecord::RecordNotFound, "Sorry. No pages after #{MAX_RESULTS / PER_PAGE}." - end @info_request = InfoRequest.find_by_url_title!(params[:url_title]) @@ -134,20 +128,10 @@ def similar def list medium_cache @view = params[:view] - unless @page # used in cache case, as perform_search sets @page as side effect - @page = get_search_page_from_params - end - @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 - # Later pages are very expensive to load - if @page > MAX_RESULTS / PER_PAGE - raise ActiveRecord::RecordNotFound, "Sorry. No pages after #{MAX_RESULTS / PER_PAGE}." - end - @filters = params.merge(latest_status: @view) if @page > 1 @@ -158,9 +142,6 @@ def list @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 - @no_crawl = true if @page > 20 end # Page new form posts to @@ -474,6 +455,21 @@ def outgoing_message_params params.require(:outgoing_message).permit(:body, :what_doing) end + def setup_results_pagination + @page ||= get_search_page_from_params + @per_page = PER_PAGE + @max_results = MAX_RESULTS + + # Don't let robots go more than 20 pages in + @no_crawl = true if @page > 20 + + # Later pages are very expensive to load + return if @page <= MAX_RESULTS / PER_PAGE + + raise ActiveRecord::RecordNotFound, + "Sorry. No pages after #{MAX_RESULTS / PER_PAGE}." + end + def can_update_status(info_request) # Don't allow status update on external requests, otherwise accept param info_request.is_external? ? false : params[:update_status] == "1" From 7b0d758a20dc38c4e998cbc90c98bdb685379a66 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Thu, 11 Apr 2024 09:37:55 +0100 Subject: [PATCH 3/7] Refactor request list routes Remove routes for "all" and "recent" views. The "recent" view already is redirected in the main `list` action to the "all" view. The "all" view should really be the default for the `list` action as currently the `/list` endpoint will show no requests because there is no view is selected. --- app/controllers/request_controller.rb | 7 ++----- app/views/general/_frontpage_requests_list.html.erb | 2 +- config/routes.rb | 11 +++-------- spec/controllers/track_controller_spec.rb | 4 ++-- 4 files changed, 8 insertions(+), 16 deletions(-) diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb index 05c834f96d..5d08d989db 100644 --- a/app/controllers/request_controller.rb +++ b/app/controllers/request_controller.rb @@ -127,12 +127,9 @@ def similar def list medium_cache - @view = params[:view] - if @view == "recent" - return redirect_to request_list_all_url(action: "list", view: "all", page: @page), status: :moved_permanently - end - @filters = params.merge(latest_status: @view) + @filters = params.slice(:query, :request_date_after, :request_date_before) + @filters[:latest_status] = params[:view] if @page > 1 @title = _("Browse and search requests (page {{count}})", count: @page) diff --git a/app/views/general/_frontpage_requests_list.html.erb b/app/views/general/_frontpage_requests_list.html.erb index 6b8e8a0dd8..3fb6d82f87 100644 --- a/app/views/general/_frontpage_requests_list.html.erb +++ b/app/views/general/_frontpage_requests_list.html.erb @@ -41,5 +41,5 @@ <% end %> - <%= link_to _("Browse all requests →"), request_list_all_path, :class => 'button-secondary' %> + <%= link_to _("Browse all requests →"), request_list_path, :class => 'button-secondary' %> diff --git a/config/routes.rb b/config/routes.rb index 87ab3dbc7a..eda303eb43 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -75,14 +75,8 @@ def matches?(request) get '/body_statistics' => redirect('/statistics#public_bodies'), :as => :public_bodies_statistics ##### Request controller - match '/list/recent' => 'request#list', - :as => :request_list_recent, - :view => 'recent', - :via => :get - match '/list/all' => 'request#list', - :as => :request_list_all, - :view => 'all', - :via => :get + get '/list/all' => redirect('/list') + get '/list/recent' => redirect('/list') match '/list/successful' => 'request#list', :as => :request_list_successful, :view => 'successful', @@ -97,6 +91,7 @@ def matches?(request) :via => :get match '/list' => 'request#list', :as => :request_list, + :view => 'all', :via => :get match '/select_authority' => 'request#select_authority', diff --git a/spec/controllers/track_controller_spec.rb b/spec/controllers/track_controller_spec.rb index 2c3c6018a6..1d8f600b92 100644 --- a/spec/controllers/track_controller_spec.rb +++ b/spec/controllers/track_controller_spec.rb @@ -367,7 +367,7 @@ and_return(track_thing) expect(track_thing).to receive(:save).and_call_original get :track_list, params: { view: 'recent', feed: 'track' } - expect(response).to redirect_to("/list?view=recent") + expect(response).to redirect_to("/list") end it "should redirect with an error message if the query is too long" do @@ -378,7 +378,7 @@ and_return(long_track) get :track_list, params: { view: 'recent', feed: 'track' } expect(flash[:error]).to match('too long') - expect(response).to redirect_to("/list?view=recent") + expect(response).to redirect_to("/list") end end From 6fc79007be94762236abc1c1568a8f0b67a28f3b Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 12 Apr 2024 15:35:03 +0100 Subject: [PATCH 4/7] Move request searching into controller action Having access to the results here will allow us to check for the number of results before rendering. --- app/controllers/request_controller.rb | 4 ++++ app/views/request/_list_results.html.erb | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb index 5d08d989db..a5c16521ef 100644 --- a/app/controllers/request_controller.rb +++ b/app/controllers/request_controller.rb @@ -131,6 +131,10 @@ def list @filters = params.slice(:query, :request_date_after, :request_date_before) @filters[:latest_status] = params[:view] + @results = InfoRequest.request_list( + @filters, @page, @per_page, @max_results + ) + if @page > 1 @title = _("Browse and search requests (page {{count}})", count: @page) else diff --git a/app/views/request/_list_results.html.erb b/app/views/request/_list_results.html.erb index 8824ff0245..382fa9b2fe 100644 --- a/app/views/request/_list_results.html.erb +++ b/app/views/request/_list_results.html.erb @@ -1,4 +1,3 @@ -<% @results = InfoRequest.request_list(@filters, @page, @per_page, @max_results) %> <% if @results[:results].empty? %>

<%= _('No requests of this sort yet.')%>

<% else %> From 926390818ff00f9b9bf0cd60fd0fafd73bd1d7cb Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 12 Apr 2024 11:56:34 +0100 Subject: [PATCH 5/7] Update RequestController#list action to handle tags Allow users to view requests by tag. In the tag belongs to a category then, load the category and render a different title. --- app/controllers/request_controller.rb | 16 ++++++++- .../request/_request_filter_form.html.erb | 2 +- config/routes.rb | 8 ++--- lib/xapian_queries.rb | 7 ++++ spec/controllers/request_controller_spec.rb | 35 +++++++++++++++++++ spec/factories/categories.rb | 4 +++ 6 files changed, 66 insertions(+), 6 deletions(-) diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb index a5c16521ef..5115c5a8c2 100644 --- a/app/controllers/request_controller.rb +++ b/app/controllers/request_controller.rb @@ -130,12 +130,26 @@ def list @filters = params.slice(:query, :request_date_after, :request_date_before) @filters[:latest_status] = params[:view] + @tag = @filters[:tag] = params[:tag] @results = InfoRequest.request_list( @filters, @page, @per_page, @max_results ) - if @page > 1 + @category = InfoRequest.category_list.find_by(category_tag: @tag) if @tag + if @category + @title = n_('Found {{count}} request in the category ‘{{category}}’', + 'Found {{count}} requests in the category ‘{{category}}’', + @results[:matches_estimated], + count: @results[:matches_estimated], + category: @category.title) + elsif @tag + @title = n_('Found {{count}} request tagged ‘{{tag_name}}’', + 'Found {{count}} requests tagged ‘{{tag_name}}’', + @results[:matches_estimated], + count: @results[:matches_estimated], + tag_name: @tag) + elsif @page > 1 @title = _("Browse and search requests (page {{count}})", count: @page) else @title = _('Browse and search requests') diff --git a/app/views/request/_request_filter_form.html.erb b/app/views/request/_request_filter_form.html.erb index f0770b210d..6155d0a174 100644 --- a/app/views/request/_request_filter_form.html.erb +++ b/app/views/request/_request_filter_form.html.erb @@ -11,7 +11,7 @@ <% if controller?("public_body") %> <%= link_to label, url_for(:controller => "public_body", :action => "show", :view => status, :url_name => @public_body.url_name) + "?" + request.query_string + '#results' %> <% else %> - <%= link_to label, url_for(:controller => "request", :action => "list", :view => status) + "?" + request.query_string + '#results' %> + <%= link_to label, url_for(:controller => "request", :action => "list", :view => status, :tag => @tag) + "?" + request.query_string + '#results' %> <% end %> <% else %> <%= label %> diff --git a/config/routes.rb b/config/routes.rb index eda303eb43..50699549f3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -77,19 +77,19 @@ def matches?(request) ##### Request controller get '/list/all' => redirect('/list') get '/list/recent' => redirect('/list') - match '/list/successful' => 'request#list', + match '/list(/:tag)/successful' => 'request#list', :as => :request_list_successful, :view => 'successful', :via => :get - match '/list/unsuccessful' => 'request#list', + match '/list(/:tag)/unsuccessful' => 'request#list', :as => :request_list_unsuccessful, :view => 'unsuccessful', :via => :get - match '/list/awaiting' => 'request#list', + match '/list(/:tag)/awaiting' => 'request#list', :as => :request_list_awaiting, :view => 'awaiting', :via => :get - match '/list' => 'request#list', + match '/list(/:tag)' => 'request#list', :as => :request_list, :view => 'all', :via => :get diff --git a/lib/xapian_queries.rb b/lib/xapian_queries.rb index 0eee8284c7..dfdf45efdd 100644 --- a/lib/xapian_queries.rb +++ b/lib/xapian_queries.rb @@ -70,11 +70,18 @@ def get_date_range_from_params(params) query end + def get_tag_params(params) + return '' unless params[:tag] + + " tag:#{params[:tag]}" + end + def make_query_from_params(params) query = params[:query] || '' query += get_date_range_from_params(params) query += get_request_variety_from_params(params) query += get_status_from_params(params) + query += get_tag_params(params) query end end diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb index 2d80c2a517..351d5c0204 100644 --- a/spec/controllers/request_controller_spec.rb +++ b/spec/controllers/request_controller_spec.rb @@ -27,6 +27,41 @@ expect { get :list, params: { view: 'all', page: "-1" } }.not_to raise_error expect(assigns[:page]).to eq(1) end + + it 'sets title based on page' do + get :list, params: { view: 'all' } + expect(assigns[:title]).to eq('Browse and search requests') + + get :list, params: { view: 'all', page: 2 } + expect(assigns[:title]).to eq('Browse and search requests (page 2)') + end + + it 'sets title based on if tag matches an request category' do + FactoryBot.create(:category, :info_request, + title: 'Climate requests', category_tag: 'climate') + + update_xapian_index + get :list, params: { view: 'all', tag: 'climate' } + expect(assigns[:title]). + to eq("Found 0 requests in the category ‘Climate requests’") + + FactoryBot.create(:info_request, tag_string: 'climate') + update_xapian_index + get :list, params: { view: 'all', tag: 'climate' } + expect(assigns[:title]). + to eq("Found 1 request in the category ‘Climate requests’") + end + + it 'sets title based on if tag does not match an request category' do + update_xapian_index + get :list, params: { view: 'all', tag: 'other' } + expect(assigns[:title]).to eq("Found 0 requests tagged ‘other’") + + FactoryBot.create(:info_request, tag_string: 'other') + update_xapian_index + get :list, params: { view: 'all', tag: 'other' } + expect(assigns[:title]).to eq("Found 1 request tagged ‘other’") + end end RSpec.describe RequestController, "when showing one request" do diff --git a/spec/factories/categories.rb b/spec/factories/categories.rb index 180df1d690..72685e174c 100644 --- a/spec/factories/categories.rb +++ b/spec/factories/categories.rb @@ -20,5 +20,9 @@ trait :public_body do parents { [PublicBody.category_root] } end + + trait :info_request do + parents { [InfoRequest.category_root] } + end end end From 9bf08a2a0d1aa73c71c966d37b3a746a7b953fa6 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Thu, 11 Apr 2024 13:07:28 +0100 Subject: [PATCH 6/7] Update RequestController#list view Render any category notes and it's rich text body. --- app/views/request/_category.html.erb | 2 ++ app/views/request/list.html.erb | 2 ++ 2 files changed, 4 insertions(+) create mode 100644 app/views/request/_category.html.erb diff --git a/app/views/request/_category.html.erb b/app/views/request/_category.html.erb new file mode 100644 index 0000000000..2f96b70660 --- /dev/null +++ b/app/views/request/_category.html.erb @@ -0,0 +1,2 @@ +<%= render_notes(@category.all_notes) %> +<%= @category.body %> diff --git a/app/views/request/list.html.erb b/app/views/request/list.html.erb index 498c45e7ee..5a89b11565 100644 --- a/app/views/request/list.html.erb +++ b/app/views/request/list.html.erb @@ -2,6 +2,8 @@

<%= @title %>

<%= render :partial => 'request/request_search_form', :locals => { :after_form_fields => render(:partial => 'request/request_filter_form') } %> + + <%= render partial: 'category' if @category %>