From b793bbef62e6934370a26b55fa62794a88d55763 Mon Sep 17 00:00:00 2001 From: Alexander Griffen Date: Fri, 1 Mar 2024 10:04:27 +0000 Subject: [PATCH 1/5] Update Alaveteli Pro request routing This commit does the following: - Removes a redirect in the request controller to redirect embargoed requests for pro specific URL - Replaces Alaveteli Pro request URLs and routes with the standard route - Disables spec examples testing pro context redirection as these tests will need to be rewritten once the old routing is updated --- .../alaveteli_pro/classifications_controller.rb | 2 +- .../embargo_extensions_controller.rb | 4 ++-- .../alaveteli_pro/embargoes_controller.rb | 2 +- .../alaveteli_pro/info_requests_controller.rb | 2 +- app/controllers/request_controller.rb | 17 +---------------- app/services/info_request_batch_metrics.rb | 2 +- app/views/admin_request/show.html.erb | 2 +- .../classifications_controller_spec.rb | 6 +++--- .../alaveteli_pro/embargo_controller_spec.rb | 2 +- .../embargo_extensions_controller_spec.rb | 6 +++--- spec/controllers/request_controller_spec.rb | 6 +++--- spec/integration/alaveteli_dsl.rb | 2 +- .../services/info_request_batch_metrics_spec.rb | 3 +-- spec/views/admin_request/show.html.erb_spec.rb | 2 +- 14 files changed, 21 insertions(+), 37 deletions(-) diff --git a/app/controllers/alaveteli_pro/classifications_controller.rb b/app/controllers/alaveteli_pro/classifications_controller.rb index 035974b48c..2fe10f33c5 100644 --- a/app/controllers/alaveteli_pro/classifications_controller.rb +++ b/app/controllers/alaveteli_pro/classifications_controller.rb @@ -10,7 +10,7 @@ def create set_described_state flash[:notice] = _('Your request has been updated!') - redirect_to show_alaveteli_pro_request_path( + redirect_to show_request_path( url_title: @info_request.url_title ) end diff --git a/app/controllers/alaveteli_pro/embargo_extensions_controller.rb b/app/controllers/alaveteli_pro/embargo_extensions_controller.rb index 5faa105ed6..643b4f9197 100644 --- a/app/controllers/alaveteli_pro/embargo_extensions_controller.rb +++ b/app/controllers/alaveteli_pro/embargo_extensions_controller.rb @@ -32,7 +32,7 @@ def create "request's privacy settings, please try again.") end - redirect_to show_alaveteli_pro_request_path( + redirect_to show_request_path( url_title: @info_request.url_title ) end @@ -70,7 +70,7 @@ def create_batch if params[:info_request_id] @info_request = InfoRequest.find(params[:info_request_id]) - redirect_to show_alaveteli_pro_request_path( + redirect_to show_request_path( url_title: @info_request.url_title ) else diff --git a/app/controllers/alaveteli_pro/embargoes_controller.rb b/app/controllers/alaveteli_pro/embargoes_controller.rb index 987fa3d3f0..7dd7b6dcb9 100644 --- a/app/controllers/alaveteli_pro/embargoes_controller.rb +++ b/app/controllers/alaveteli_pro/embargoes_controller.rb @@ -66,7 +66,7 @@ def destroy_batch end if params[:info_request_id] @info_request = InfoRequest.find(params[:info_request_id]) - redirect_to show_alaveteli_pro_request_path( + redirect_to show_request_path( url_title: @info_request.url_title) else redirect_to show_alaveteli_pro_batch_request_path(@info_request_batch) diff --git a/app/controllers/alaveteli_pro/info_requests_controller.rb b/app/controllers/alaveteli_pro/info_requests_controller.rb index 76f5760ca4..6adb6b37fc 100644 --- a/app/controllers/alaveteli_pro/info_requests_controller.rb +++ b/app/controllers/alaveteli_pro/info_requests_controller.rb @@ -47,7 +47,7 @@ def create @embargo.save! if @embargo.present? send_initial_message(@outgoing_message) destroy_draft - redirect_to show_alaveteli_pro_request_path( + redirect_to show_request_path( url_title: @info_request.url_title) else show_errors diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb index a5abeec4ef..0759904554 100644 --- a/app/controllers/request_controller.rb +++ b/app/controllers/request_controller.rb @@ -12,7 +12,6 @@ class RequestController < ApplicationController before_action :check_read_only, only: [:new, :upload_response] before_action :set_render_recaptcha, only: [ :new ] before_action :set_info_request, only: [:show] - before_action :redirect_embargoed_requests_for_pro_users, only: [:show] before_action :redirect_public_requests_from_pro_context, only: [:show] before_action :redirect_new_form_to_pro_version, only: [:select_authority, :new] before_action :set_in_pro_area, only: [:select_authority, :show] @@ -52,8 +51,7 @@ def show # Always show the pro livery if a request is embargoed. This makes it # clear to admins and ex-pro users that the `InfoRequest` is still - # private. Users who are not permitted to view the request are redirected - # so we don't need to consider the `current_user` here. + # private. @in_pro_area = true if @info_request.embargo set_last_request(@info_request) @@ -711,19 +709,6 @@ def set_render_recaptcha (!@user || !@user.confirmed_not_spam?) end - def redirect_embargoed_requests_for_pro_users - # Pro users should see their embargoed requests in the pro page, so that - # if other site functions send them to a request page, they end up back in - # the pro area - if feature_enabled?(:alaveteli_pro) && params[:pro] != "1" && current_user - @info_request = InfoRequest.find_by_url_title!(params[:url_title]) - if @info_request.is_actual_owning_user?(current_user) && @info_request.embargo - redirect_to show_alaveteli_pro_request_url( - url_title: @info_request.url_title) - end - end - end - def redirect_public_requests_from_pro_context # Requests which aren't embargoed should always go to the normal request # page, so that pro's seem them in that context after they publish them diff --git a/app/services/info_request_batch_metrics.rb b/app/services/info_request_batch_metrics.rb index f90670a186..73d0b2eb6b 100644 --- a/app/services/info_request_batch_metrics.rb +++ b/app/services/info_request_batch_metrics.rb @@ -16,7 +16,7 @@ def initialize(info_request_batch) def metrics @metrics ||= @info_request_batch.info_requests. includes(public_body: :translations).map do |info_request| - url = show_alaveteli_pro_request_url(info_request.url_title) + url = show_request_url(info_request.url_title) status = InfoRequest::State.short_description( info_request.calculate_status(true) ) diff --git a/app/views/admin_request/show.html.erb b/app/views/admin_request/show.html.erb index 4946874df6..191544d993 100644 --- a/app/views/admin_request/show.html.erb +++ b/app/views/admin_request/show.html.erb @@ -34,7 +34,7 @@ <% if @info_request.embargo %> Request page: - <%= link_to show_alaveteli_pro_request_url(@info_request.url_title), show_alaveteli_pro_request_path(@info_request.url_title) %> + <%= link_to show_request_url(@info_request.url_title), show_request_path(@info_request.url_title) %> <% else %> Public page: <%= link_to request_url(@info_request), request_path(@info_request) %> diff --git a/spec/controllers/alaveteli_pro/classifications_controller_spec.rb b/spec/controllers/alaveteli_pro/classifications_controller_spec.rb index fc94929d49..814fd3b769 100644 --- a/spec/controllers/alaveteli_pro/classifications_controller_spec.rb +++ b/spec/controllers/alaveteli_pro/classifications_controller_spec.rb @@ -75,7 +75,7 @@ def post_status(status, message: nil) it 'should redirect back to the request' do post_status('successful') expect(response).to redirect_to( - show_alaveteli_pro_request_path(url_title: info_request.url_title) + show_request_path(url_title: info_request.url_title) ) end end @@ -128,7 +128,7 @@ def post_status(status, message: nil) it 'should redirect back to the request' do post_status('error_message', message: 'A message') expect(response).to redirect_to( - show_alaveteli_pro_request_path(url_title: info_request.url_title) + show_request_path(url_title: info_request.url_title) ) end end @@ -169,7 +169,7 @@ def post_status(status, message: nil) it 'should redirect back to the request' do post_status('requires_admin', message: 'A message') expect(response).to redirect_to( - show_alaveteli_pro_request_path(url_title: info_request.url_title) + show_request_path(url_title: info_request.url_title) ) end end diff --git a/spec/controllers/alaveteli_pro/embargo_controller_spec.rb b/spec/controllers/alaveteli_pro/embargo_controller_spec.rb index 60cf5863b3..0cf46fb8c6 100644 --- a/spec/controllers/alaveteli_pro/embargo_controller_spec.rb +++ b/spec/controllers/alaveteli_pro/embargo_controller_spec.rb @@ -306,7 +306,7 @@ end it "redirects to that request, not the batch" do - expected_path = show_alaveteli_pro_request_path( + expected_path = show_request_path( url_title: info_request_batch.info_requests.first.url_title) expect(response).to redirect_to(expected_path) end diff --git a/spec/controllers/alaveteli_pro/embargo_extensions_controller_spec.rb b/spec/controllers/alaveteli_pro/embargo_extensions_controller_spec.rb index 04f0c95346..46e4671109 100644 --- a/spec/controllers/alaveteli_pro/embargo_extensions_controller_spec.rb +++ b/spec/controllers/alaveteli_pro/embargo_extensions_controller_spec.rb @@ -40,7 +40,7 @@ it 'redirects to the request show page' do expect(response). - to redirect_to show_alaveteli_pro_request_path( + to redirect_to show_request_path( url_title: info_request.url_title) end end @@ -71,7 +71,7 @@ it 'redirects to the request show page' do expect(response). - to redirect_to show_alaveteli_pro_request_path( + to redirect_to show_request_path( url_title: info_request.url_title) end end @@ -307,7 +307,7 @@ end it 'redirects to that request, not the batch' do - expected_path = show_alaveteli_pro_request_path( + expected_path = show_request_path( url_title: info_request_batch.info_requests.first.url_title) expect(response).to redirect_to(expected_path) end diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb index 53fc2eb439..cacdfedbde 100644 --- a/spec/controllers/request_controller_spec.rb +++ b/spec/controllers/request_controller_spec.rb @@ -113,7 +113,7 @@ end end - describe "redirecting pro users to the pro context" do + xdescribe "redirecting pro users to the pro context" do let(:pro_user) { FactoryBot.create(:pro_user) } context "when showing pros their own requests" do @@ -127,7 +127,7 @@ sign_in pro_user get :show, params: { url_title: info_request.url_title } expect(response).to redirect_to( - show_alaveteli_pro_request_path(url_title: info_request.url_title) + show_request_path(url_title: info_request.url_title) ) end end @@ -161,7 +161,7 @@ with_feature_enabled(:alaveteli_pro) do sign_in pro_user get :show, params: { url_title: info_request.url_title } - expect(response).to redirect_to show_alaveteli_pro_request_path( + expect(response).to redirect_to show_request_path( url_title: info_request.url_title) end end diff --git a/spec/integration/alaveteli_dsl.rb b/spec/integration/alaveteli_dsl.rb index 8abccda7de..40da2ed5de 100644 --- a/spec/integration/alaveteli_dsl.rb +++ b/spec/integration/alaveteli_dsl.rb @@ -4,7 +4,7 @@ def browse_request(url_title) end def browse_pro_request(url_title) - visit "/alaveteli_pro/info_requests/#{url_title}" + browse_request(url_title) end def create_request(public_body) diff --git a/spec/services/info_request_batch_metrics_spec.rb b/spec/services/info_request_batch_metrics_spec.rb index bf111359ec..50dab8e0e0 100644 --- a/spec/services/info_request_batch_metrics_spec.rb +++ b/spec/services/info_request_batch_metrics_spec.rb @@ -8,8 +8,7 @@ subject(:metrics) { described_class.new(batch).metrics } it 'generates info request batch metrics' do - request_url = 'http://test.host/alaveteli_pro/info_requests/' + - request.url_title + request_url = 'http://test.host/request/' + request.url_title authority_name = request.public_body.name is_expected.to match_array( diff --git a/spec/views/admin_request/show.html.erb_spec.rb b/spec/views/admin_request/show.html.erb_spec.rb index 0727ff1368..a12de51a20 100644 --- a/spec/views/admin_request/show.html.erb_spec.rb +++ b/spec/views/admin_request/show.html.erb_spec.rb @@ -25,7 +25,7 @@ it 'links to the pro request page' do render expect(rendered). - to match(show_alaveteli_pro_request_url(info_request.url_title)) + to match(show_request_url(info_request.url_title)) end it 'includes embargo information' do From 7ac7c55d29b7a8faba029713b4b25e537752f740 Mon Sep 17 00:00:00 2001 From: Alexander Griffen Date: Fri, 1 Mar 2024 10:05:15 +0000 Subject: [PATCH 2/5] Remove old Alaveteli Pro routes This commit removes the Alaveteli Pro info request route and also sets up a redirect from this route to the standard request route. --- config/routes.rb | 7 ------- config/routes/redirects.rb | 6 ++++++ spec/routing/redirects_spec.rb | 8 ++++++++ 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index 9076e52b31..72d227db92 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -907,13 +907,6 @@ def matches?(request) end scope path: :alaveteli_pro do - # So that we can show a request using the existing controller from the - # pro context - match '/info_requests/:url_title' => 'request#show', - :as => :show_alaveteli_pro_request, - :via => :get, - :defaults => { :pro => '1' } - # So that we can show a batch request using the existing controller from # the pro context match '/info_request_batches/:id' => 'info_request_batch#show', diff --git a/config/routes/redirects.rb b/config/routes/redirects.rb index 1fa67f1fe7..708cae33f2 100644 --- a/config/routes/redirects.rb +++ b/config/routes/redirects.rb @@ -95,3 +95,9 @@ get ':locale/*path', constraints: locale_constraint, to: redirect('%{path}?locale=%{locale}') + +constraints FeatureConstraint.new(:alaveteli_pro) do + get '/alaveteli_pro/info_requests/:url_title', + constraints: { url_title: /(?!new).*/ }, + to: redirect('/request/%{url_title}') +end diff --git a/spec/routing/redirects_spec.rb b/spec/routing/redirects_spec.rb index 74f8c32446..70c8ccac05 100644 --- a/spec/routing/redirects_spec.rb +++ b/spec/routing/redirects_spec.rb @@ -87,4 +87,12 @@ get('/help/about?locale=en_GB') expect(response).to redirect_to('/help/about') end + + it 'redirects old pro request routes', feature: :alaveteli_pro do + get('/alaveteli_pro/info_requests/my-request') + expect(response).to redirect_to('/request/my-request') + + get('/alaveteli_pro/info_requests/new') + expect(response).to_not redirect_to('/request/new') + end end From 7ade97491f06603316836105726269913bdc7903 Mon Sep 17 00:00:00 2001 From: Alexander Griffen Date: Fri, 1 Mar 2024 10:06:05 +0000 Subject: [PATCH 3/5] Remove no longer useful redirect This commit removes the redirect for public requests from the pro context as it no longer can be ran --- app/controllers/request_controller.rb | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb index 0759904554..cb23b6ce70 100644 --- a/app/controllers/request_controller.rb +++ b/app/controllers/request_controller.rb @@ -12,7 +12,6 @@ class RequestController < ApplicationController before_action :check_read_only, only: [:new, :upload_response] before_action :set_render_recaptcha, only: [ :new ] before_action :set_info_request, only: [:show] - before_action :redirect_public_requests_from_pro_context, only: [:show] before_action :redirect_new_form_to_pro_version, only: [:select_authority, :new] before_action :set_in_pro_area, only: [:select_authority, :show] @@ -709,15 +708,6 @@ def set_render_recaptcha (!@user || !@user.confirmed_not_spam?) end - def redirect_public_requests_from_pro_context - # Requests which aren't embargoed should always go to the normal request - # page, so that pro's seem them in that context after they publish them - if feature_enabled?(:alaveteli_pro) && params[:pro] == "1" - @info_request = InfoRequest.find_by_url_title!(params[:url_title]) - redirect_to request_url(@info_request) unless @info_request.embargo - end - end - def redirect_new_form_to_pro_version # Pros should use the pro version of the form if feature_enabled?(:alaveteli_pro) && From 975f3d6004302019a1d67d3d1757eccc2df50e80 Mon Sep 17 00:00:00 2001 From: Alexander Griffen Date: Fri, 15 Mar 2024 10:29:15 +0000 Subject: [PATCH 4/5] Update pro context redirection tests We're not redirecting to the pro specific request route any more, instead update these tests to check when the pro livery is used or not. --- spec/controllers/request_controller_spec.rb | 49 ++++++--------------- 1 file changed, 14 insertions(+), 35 deletions(-) diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb index cacdfedbde..f18b69692b 100644 --- a/spec/controllers/request_controller_spec.rb +++ b/spec/controllers/request_controller_spec.rb @@ -113,23 +113,20 @@ end end - xdescribe "redirecting pro users to the pro context" do + describe "livery used", feature: :alaveteli_pro do let(:pro_user) { FactoryBot.create(:pro_user) } + before { sign_in pro_user } + context "when showing pros their own requests" do context "when the request is embargoed" do let(:info_request) do FactoryBot.create(:embargoed_request, user: pro_user) end - it "should always redirect to the pro version of the page" do - with_feature_enabled(:alaveteli_pro) do - sign_in pro_user - get :show, params: { url_title: info_request.url_title } - expect(response).to redirect_to( - show_request_path(url_title: info_request.url_title) - ) - end + it 'uses the pro livery' do + get :show, params: { url_title: info_request.url_title } + expect(assigns[:in_pro_area]).to be true end end @@ -138,12 +135,9 @@ FactoryBot.create(:info_request, user: pro_user) end - it "should not redirect to the pro version of the page" do - with_feature_enabled(:alaveteli_pro) do - sign_in pro_user - get :show, params: { url_title: info_request.url_title } - expect(response).to be_successful - end + it "should not use the pro livery" do + get :show, params: { url_title: info_request.url_title } + expect(assigns[:in_pro_area]).to be false end end end @@ -157,31 +151,16 @@ FactoryBot.create(:embargoed_request, user: pro_user) end - it 'redirects to the pro version of the page' do - with_feature_enabled(:alaveteli_pro) do - sign_in pro_user - get :show, params: { url_title: info_request.url_title } - expect(response).to redirect_to show_request_path( - url_title: info_request.url_title) - end - end - it 'uses the pro livery' do - with_feature_enabled(:alaveteli_pro) do - sign_in pro_user - get :show, params: { url_title: info_request.url_title, pro: '1' } - expect(assigns[:in_pro_area]).to be true - end + get :show, params: { url_title: info_request.url_title } + expect(assigns[:in_pro_area]).to be true end end context "when showing pros a someone else's request" do - it "should not redirect to the pro version of the page" do - with_feature_enabled(:alaveteli_pro) do - sign_in pro_user - get :show, params: { url_title: 'why_do_you_have_such_a_fancy_dog' } - expect(response).to be_successful - end + it "should not user the pro livery" do + get :show, params: { url_title: 'why_do_you_have_such_a_fancy_dog' } + expect(assigns[:in_pro_area]).to be false end end end From 74ed2614768d5752fe41ebe1d87f1211fada5a71 Mon Sep 17 00:00:00 2001 From: Alexander Griffen Date: Mon, 25 Mar 2024 17:50:48 +0000 Subject: [PATCH 5/5] Update changelog --- doc/CHANGES.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/CHANGES.md b/doc/CHANGES.md index 6cdbfa4b9e..bda5287b7e 100644 --- a/doc/CHANGES.md +++ b/doc/CHANGES.md @@ -2,6 +2,8 @@ ## Highlighted Features +* Change use of `/alaveteli_pro/info_requests/{request}` to instead appear as + `/request/{request}` (Alexander Griffen, Graeme Porteous) * Remove locale prefixes from URLs (Alexander Griffen, Graeme Porteous) * Fix missing headers when exporting Project data (Gareth Rees) * Reduce amount of storage related background jobs (Graeme Porteous)