From cf38ab9cca7843c33a694aaddb715710f248ab6a Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Thu, 16 Nov 2023 11:44:51 +0000
Subject: [PATCH 01/12] Refactor session authentication

---
 app/controllers/application_controller.rb |  5 +----
 app/models/user.rb                        |  6 ++++++
 spec/models/user_spec.rb                  | 24 +++++++++++++++++++++++
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index bb65024c03..9fcdc26004 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -279,10 +279,7 @@ def ask_to_login(as: nil, **reason_params)
   # Return logged in user
   def authenticated_user
     return unless session[:user_id]
-
-    @user ||= User.find_by(
-      id: session[:user_id], login_token: session[:user_login_token]
-    )
+    @user ||= User.authenticate_from_session(session)
   end
 
   # For CanCanCan and other libs which need a Devise-like current_user method
diff --git a/app/models/user.rb b/app/models/user.rb
index ee15eee72e..f3ce6c37af 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -239,6 +239,12 @@ def self.authenticate_from_form(params, specific_user_login = false)
     user
   end
 
+  def self.authenticate_from_session(session)
+    return unless session[:user_id]
+
+    find_by(id: session[:user_id], login_token: session[:user_login_token])
+  end
+
   # Case-insensitively find a user from their email
   def self.find_user_by_email(email)
     return nil if email.blank?
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 79838268a5..4bf9887dad 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -678,6 +678,30 @@ def create_user(options = {})
     end
   end
 
+  describe '.authenticate_from_session' do
+    let(:user) { FactoryBot.create(:user) }
+
+    it 'finds a user by ID and login token' do
+      session = { user_id: user.id, user_login_token: user.login_token }
+      expect(User.authenticate_from_session(session)).to eq(user)
+    end
+
+    it 'returns nil without user_id session' do
+      session = { user_id: nil }
+      expect(User.authenticate_from_session(session)).to be_nil
+    end
+
+    it "returns nil when user ID doesn't match" do
+      session = { user_id: 1, user_login_token: user.login_token }
+      expect(User.authenticate_from_session(session)).to be_nil
+    end
+
+    it "returns nil when user login token doesn't match" do
+      session = { user_id: user.id, user_login_token: 'ABC' }
+      expect(User.authenticate_from_session(session)).to be_nil
+    end
+  end
+
   describe '.stay_logged_in_on_redirect?' do
     it 'is false if the user is nil' do
       expect(User.stay_logged_in_on_redirect?(nil)).to be_falsey

From fd129f7cb23c666d603c9aa6061a3b13f30f29c6 Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Tue, 5 Dec 2023 12:37:13 +0000
Subject: [PATCH 02/12] Update calls to show request routes

Switch to using request URL title paths to save extra redirects.
---
 app/mailers/request_mailer.rb                       | 2 +-
 spec/integration/alaveteli_pro/request_list_spec.rb | 2 +-
 spec/integration/download_request_spec.rb           | 4 ++--
 spec/mailers/request_mailer_spec.rb                 | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/app/mailers/request_mailer.rb b/app/mailers/request_mailer.rb
index d614721967..dfac46dd57 100644
--- a/app/mailers/request_mailer.rb
+++ b/app/mailers/request_mailer.rb
@@ -133,7 +133,7 @@ def very_overdue_alert(info_request, user)
   # Tell the requester that they need to say if the new response
   # contains info or not
   def new_response_reminder_alert(info_request, incoming_message)
-    target = show_request_url(info_request,
+    target = show_request_url(info_request.url_title,
                               anchor: 'describe_state_form_1',
                               only_path: true)
     @url = signin_url(r: target)
diff --git a/spec/integration/alaveteli_pro/request_list_spec.rb b/spec/integration/alaveteli_pro/request_list_spec.rb
index 4f5602bdc0..a7f9827beb 100644
--- a/spec/integration/alaveteli_pro/request_list_spec.rb
+++ b/spec/integration/alaveteli_pro/request_list_spec.rb
@@ -131,7 +131,7 @@
         batch.info_requests.each do |request|
           expect(page).to have_css("info-request-#{request.id}")
           within("info-request-#{request.id}") do
-            request_path = show_request_path(request.id)
+            request_path = show_request_path(request.url_title)
             expect(page).to have_content(request.public_body.name)
             expect(page).to have_content("Awaiting response")
           end
diff --git a/spec/integration/download_request_spec.rb b/spec/integration/download_request_spec.rb
index d722b051bb..d3fb483609 100644
--- a/spec/integration/download_request_spec.rb
+++ b/spec/integration/download_request_spec.rb
@@ -8,7 +8,7 @@
 
   def inspect_zip_download(session, info_request)
     using_session(session) do
-      visit show_request_path(info_request)
+      visit show_request_path(info_request.url_title)
       within('.request-header__action-bar') do
         find_link('Download a zip file of all correspondence').click
       end
@@ -230,7 +230,7 @@ def sleep_and_receive_mail(name, info_request)
 
         # Non-owner can't
         using_session(@non_owner) do
-          visit show_request_path(@info_request)
+          visit show_request_path(@info_request.url_title)
           expect(page).to have_content 'Request has been removed'
           visit download_entire_request_path(@info_request.url_title)
           expect(page.status_code).to eq(403)
diff --git a/spec/mailers/request_mailer_spec.rb b/spec/mailers/request_mailer_spec.rb
index 95328772ac..1fe5859682 100644
--- a/spec/mailers/request_mailer_spec.rb
+++ b/spec/mailers/request_mailer_spec.rb
@@ -645,7 +645,7 @@ def sent_alert_params(request, type)
       mail_url = $1
 
       redirect_target =
-        show_request_path(info_request, anchor: 'describe_state_form_1')
+        show_request_path(info_request.url_title, anchor: 'describe_state_form_1')
 
       expect(mail_url).to eq(signin_url(r: redirect_target))
 

From b145368223abfebcec8cdbd446766cd7b13c00c2 Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Wed, 6 Dec 2023 11:50:54 +0000
Subject: [PATCH 03/12] Update calls to show_request routes

Remove the need to specify `url_title` in the params hash. Rails will
already correctly generate the correct route without this.

Removing means more consistency, shorter line lengths, and will be more
flexible if the route ever changes.
---
 app/controllers/citations_controller.rb       |  2 +-
 app/controllers/request_controller.rb         |  2 +-
 app/controllers/request_game_controller.rb    |  2 +-
 app/helpers/link_to_helper.rb                 |  2 +-
 app/views/request/show.text.erb               |  2 +-
 script/redact-raw-emails.rb                   |  2 +-
 spec/controllers/citations_controller_spec.rb |  2 +-
 .../classifications_controller_spec.rb        |  8 +--
 spec/controllers/followups_controller_spec.rb |  4 +-
 spec/controllers/reports_controller_spec.rb   | 10 ++--
 spec/controllers/request_controller_spec.rb   | 24 ++++----
 spec/integration/admin_censor_rule_spec.rb    | 60 ++++++++-----------
 spec/integration/alaveteli_dsl.rb             |  3 +-
 .../alaveteli_pro/request_list_spec.rb        |  2 +-
 spec/integration/incoming_mail_spec.rb        |  4 +-
 spec/mailers/request_mailer_spec.rb           |  5 +-
 16 files changed, 61 insertions(+), 73 deletions(-)

diff --git a/app/controllers/citations_controller.rb b/app/controllers/citations_controller.rb
index 0034ebca64..5f26fe1ac2 100644
--- a/app/controllers/citations_controller.rb
+++ b/app/controllers/citations_controller.rb
@@ -16,7 +16,7 @@ def create
 
     if @citation.save
       notice = _('Citation successfully created.')
-      redirect_to show_request_path(url_title: info_request.url_title),
+      redirect_to show_request_path(info_request.url_title),
                   notice: notice
     else
       render :new
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index d486b621d1..4623ff5f07 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -331,7 +331,7 @@ def new
       end
     end
 
-    redirect_to show_request_path(url_title: @info_request.url_title)
+    redirect_to show_request_path(@info_request.url_title)
   end
 
   # Used for links from polymorphic URLs e.g. in Atom feeds - just redirect to
diff --git a/app/controllers/request_game_controller.rb b/app/controllers/request_game_controller.rb
index 3f171493ef..12bda32f83 100644
--- a/app/controllers/request_game_controller.rb
+++ b/app/controllers/request_game_controller.rb
@@ -52,7 +52,7 @@ def show
       )
       return
     end
-    redirect_to show_request_url(url_title: url_title)
+    redirect_to show_request_url(url_title)
   end
 
   def stop
diff --git a/app/helpers/link_to_helper.rb b/app/helpers/link_to_helper.rb
index 54004ffc83..9adc7837f8 100755
--- a/app/helpers/link_to_helper.rb
+++ b/app/helpers/link_to_helper.rb
@@ -10,7 +10,7 @@ module LinkToHelper
 
   # Requests
   def request_url(info_request, options = {})
-    show_request_url({ url_title: info_request.url_title }.merge(options))
+    show_request_url(info_request.url_title, options)
   end
 
   def request_path(info_request, options = {})
diff --git a/app/views/request/show.text.erb b/app/views/request/show.text.erb
index 859e98b916..91c22fa1ee 100644
--- a/app/views/request/show.text.erb
+++ b/app/views/request/show.text.erb
@@ -2,7 +2,7 @@
          '"{{request_title}}".  The latest, full version is available online ' \
          'at {{full_url}}',
       :request_title => @info_request.title,
-      :full_url => "http://#{AlaveteliConfiguration::domain}#{show_request_path(:url_title=>@info_request.url_title)}") %>.
+      :full_url => "http://#{AlaveteliConfiguration::domain}#{show_request_path(@info_request.url_title)}") %>.
 
 <% @info_request.info_request_events.each do |info_request_event| %>
 <% if info_request_event.visible %>
diff --git a/script/redact-raw-emails.rb b/script/redact-raw-emails.rb
index 93fe539c13..dca0588601 100755
--- a/script/redact-raw-emails.rb
+++ b/script/redact-raw-emails.rb
@@ -124,7 +124,7 @@ def censor_part(part)
   mail[:bcc]   = censor(mail[:bcc].to_s)
 
   print show_request_url(
-    url_title: @incoming_message.info_request.url_title,
+    @incoming_message.info_request.url_title,
     anchor: "incoming-#{@incoming_message.id}"
   )
 
diff --git a/spec/controllers/citations_controller_spec.rb b/spec/controllers/citations_controller_spec.rb
index 8d837c8ac2..6b262cac2a 100644
--- a/spec/controllers/citations_controller_spec.rb
+++ b/spec/controllers/citations_controller_spec.rb
@@ -167,7 +167,7 @@ def action
         it 'redirects back to request' do
           action
           expect(response).to redirect_to(
-            show_request_url(url_title: info_request.url_title)
+            show_request_url(info_request.url_title)
           )
         end
 
diff --git a/spec/controllers/classifications_controller_spec.rb b/spec/controllers/classifications_controller_spec.rb
index a269341cca..f8c81c9373 100644
--- a/spec/controllers/classifications_controller_spec.rb
+++ b/spec/controllers/classifications_controller_spec.rb
@@ -278,7 +278,7 @@ def post_status(status, message: nil)
               last_event_id_needing_description
           }
           expect(response).to redirect_to(
-            show_request_url(url_title: info_request.url_title)
+            show_request_url(info_request.url_title)
           )
           expect(flash[:error]).to eq(
             'Please choose whether or not you got some of the information ' \
@@ -294,7 +294,7 @@ def post_status(status, message: nil)
             last_info_request_event_id: 1
           }
           expect(response).to redirect_to(
-            show_request_url(url_title: info_request.url_title)
+            show_request_url(info_request.url_title)
           )
           expect(flash[:error]).to match(
             /The request has been updated since you originally loaded this page/
@@ -599,7 +599,7 @@ def post_status(status, message: nil)
           it 'should redirect to the "request url"' do
             post_status('requires_admin', message: 'A message')
             expect(response).to redirect_to(
-              show_request_url(url_title: info_request.url_title)
+              show_request_url(info_request.url_title)
             )
           end
 
@@ -632,7 +632,7 @@ def post_status(status, message: nil)
           it 'should redirect to the "request url"' do
             post_status('error_message', message: 'A message')
             expect(response).to redirect_to(
-              show_request_url(url_title: info_request.url_title)
+              show_request_url(info_request.url_title)
             )
           end
 
diff --git a/spec/controllers/followups_controller_spec.rb b/spec/controllers/followups_controller_spec.rb
index b8fac5810b..ad7b930199 100644
--- a/spec/controllers/followups_controller_spec.rb
+++ b/spec/controllers/followups_controller_spec.rb
@@ -404,7 +404,7 @@
       it 'finds their own embargoed requests' do
         embargoed_request = FactoryBot.create(:embargoed_request,
                                               user: pro_user)
-        expected_url = show_request_url(url_title: embargoed_request.url_title)
+        expected_url = show_request_url(embargoed_request.url_title)
         post :create, params: {
                         outgoing_message: dummy_message,
                         request_id: embargoed_request.id
@@ -514,7 +514,7 @@ def send_request
                     }
 
       expect(response).
-        to redirect_to(show_request_url(url_title: request.url_title))
+        to redirect_to(show_request_url(request.url_title))
     end
 
     it "displays the a confirmation once the message has been sent" do
diff --git a/spec/controllers/reports_controller_spec.rb b/spec/controllers/reports_controller_spec.rb
index 2f30767ba1..7cd912b953 100644
--- a/spec/controllers/reports_controller_spec.rb
+++ b/spec/controllers/reports_controller_spec.rb
@@ -13,7 +13,7 @@
                       }
         expect(flash[:notice]).to match(/You need to be logged in/)
         expect(response).
-          to redirect_to show_request_path(url_title: info_request.url_title)
+          to redirect_to show_request_path(info_request.url_title)
       end
     end
 
@@ -96,7 +96,7 @@
                         reason: "my reason"
                       }
         expect(response).
-          to redirect_to show_request_path(url_title: info_request.url_title)
+          to redirect_to show_request_path(info_request.url_title)
 
         info_request.reload
         expect(info_request.attention_requested).to eq(true)
@@ -121,14 +121,14 @@
                         reason: "my reason"
                       }
         expect(response).
-          to redirect_to show_request_url(url_title: info_request.url_title)
+          to redirect_to show_request_url(info_request.url_title)
 
         post :create, params: {
                         request_id: info_request.url_title,
                         reason: "my reason"
                       }
         expect(response).
-          to redirect_to show_request_url(url_title: info_request.url_title)
+          to redirect_to show_request_url(info_request.url_title)
         expect(flash[:notice]).to match(/has already been reported/)
       end
 
@@ -290,7 +290,7 @@
                       }
 
         expect(response).
-          to redirect_to show_request_path(url_title: info_request.url_title)
+          to redirect_to show_request_path(info_request.url_title)
       end
     end
   end
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index 1e04b54bec..ba993c02d0 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -1009,7 +1009,7 @@ def send_request
     expect(mail.body).
       to match(/This is a silly letter. It is too short to be interesting./)
 
-    expect(response).to redirect_to show_request_url(url_title: ir.url_title)
+    expect(response).to redirect_to show_request_url(ir.url_title)
   end
 
   it "sets the request_sent flash to true if successful" do
@@ -1087,7 +1087,7 @@ def send_request
 
     expect(ir.url_title).not_to eq(ir2.url_title)
 
-    expect(response).to redirect_to show_request_url(url_title: ir2.url_title)
+    expect(response).to redirect_to show_request_url(ir2.url_title)
   end
 
   it 'should respect the rate limit' do
@@ -1108,7 +1108,7 @@ def send_request
       preview: 0
     }
     expect(response).to redirect_to(
-      show_request_url(url_title: 'what_is_the_answer_to_the_ultima')
+      show_request_url('what_is_the_answer_to_the_ultima')
     )
 
     post :new, params: {
@@ -1124,7 +1124,7 @@ def send_request
       preview: 0
     }
     expect(response).to redirect_to(
-      show_request_url(url_title: 'why_did_the_chicken_cross_the_ro')
+      show_request_url('why_did_the_chicken_cross_the_ro')
     )
 
     post :new, params: {
@@ -1163,7 +1163,7 @@ def send_request
       preview: 0
     }
     expect(response).to redirect_to(
-      show_request_url(url_title: 'what_is_the_answer_to_the_ultima')
+      show_request_url('what_is_the_answer_to_the_ultima')
     )
 
     post :new, params: {
@@ -1179,7 +1179,7 @@ def send_request
       preview: 0
     }
     expect(response).to redirect_to(
-      show_request_url(url_title: 'why_did_the_chicken_cross_the_ro')
+      show_request_url('why_did_the_chicken_cross_the_ro')
     )
 
     post :new, params: {
@@ -1196,7 +1196,7 @@ def send_request
       preview: 0
     }
     expect(response).to redirect_to(
-      show_request_url(url_title: 'whats_black_and_white_and_red_al')
+      show_request_url('whats_black_and_white_and_red_al')
     )
   end
 
@@ -1335,7 +1335,7 @@ def send_request
             preview: 0
           }
           expect(response).to redirect_to(
-            show_request_path(url_title: 'some_request_text')
+            show_request_path('some_request_text')
           )
         end
       end
@@ -1477,7 +1477,7 @@ def send_request
                preview: 0
              }
         expect(response).to redirect_to(
-          show_request_path(url_title: 'hd_watch_jason_bourne_online_fre')
+          show_request_path('hd_watch_jason_bourne_online_fre')
         )
       end
     end
@@ -1521,7 +1521,7 @@ def send_request
                preview: 0
              }
         expect(response).to redirect_to(
-          show_request_path(url_title: 'hd_watch_jason_bourne_online_fre')
+          show_request_path('hd_watch_jason_bourne_online_fre')
         )
       end
     end
@@ -1618,7 +1618,7 @@ def send_request
           preview: 0
         }
         expect(response).to redirect_to(
-          show_request_path(url_title: 'some_request_content')
+          show_request_path('some_request_content')
         )
       end
     end
@@ -1663,7 +1663,7 @@ def send_request
           preview: 0
         }
         expect(response).to redirect_to(
-          show_request_path(url_title: 'some_request_content')
+          show_request_path('some_request_content')
         )
       end
     end
diff --git a/spec/integration/admin_censor_rule_spec.rb b/spec/integration/admin_censor_rule_spec.rb
index 7b2e626eb9..24a2f77876 100644
--- a/spec/integration/admin_censor_rule_spec.rb
+++ b/spec/integration/admin_censor_rule_spec.rb
@@ -45,8 +45,7 @@
 
   describe "Authority censor rules" do
     it 'clears the cache for existing requests when a new rule is added' do
-      url_title = request.url_title
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
 
       expect(page).to have_content "I have a rubbish answer for you"
 
@@ -60,19 +59,18 @@
 
       perform_enqueued_jobs
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
       expect(page).not_to have_content "I have a rubbish answer for you"
       expect(page).to have_content "I have [REDACTED] for you"
     end
 
     it 'clears the cache for existing requests when a rule is updated' do
-      url_title = request.url_title
       rule = FactoryBot.create(:public_body_censor_rule,
                                public_body: authority,
                                text: "rubbish",
                                replacement: "[REDACTED]")
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
 
       expect(page).to have_content "I have a [REDACTED] answer for you"
 
@@ -86,19 +84,18 @@
 
       perform_enqueued_jobs
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
       expect(page).not_to have_content "I have a [REDACTED] answer for you"
       expect(page).to have_content "I have a rubbish [REDACTED] for you"
     end
 
     it 'clears the cache for existing requests when a rule is deleted' do
-      url_title = request.url_title
       rule = FactoryBot.create(:public_body_censor_rule,
                                public_body: authority,
                                text: "rubbish",
                                replacement: "[REDACTED]")
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
 
       expect(page).to have_content "I have a [REDACTED] answer for you"
 
@@ -109,7 +106,7 @@
 
       perform_enqueued_jobs
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
       expect(page).not_to have_content "I have a [REDACTED] answer for you"
       expect(page).to have_content "I have a rubbish answer for you"
     end
@@ -117,8 +114,7 @@
 
   describe "User censor rules" do
     it 'clears the cache for existing requests when a new rule is added' do
-      url_title = request.url_title
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
 
       expect(page).to have_content "I have a rubbish answer for you"
 
@@ -132,19 +128,18 @@
 
       perform_enqueued_jobs
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
       expect(page).not_to have_content "I have a rubbish answer for you"
       expect(page).to have_content "I have [REDACTED] for you"
     end
 
     it 'clears the cache for existing requests when a rule is updated' do
-      url_title = request.url_title
       rule = FactoryBot.create(:user_censor_rule,
                                user: user,
                                text: "rubbish",
                                replacement: "[REDACTED]")
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
 
       expect(page).to have_content "I have a [REDACTED] answer for you"
 
@@ -158,19 +153,18 @@
 
       perform_enqueued_jobs
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
       expect(page).not_to have_content "I have a [REDACTED] answer for you"
       expect(page).to have_content "I have a rubbish [REDACTED] for you"
     end
 
     it 'clears the cache for existing requests when a rule is deleted' do
-      url_title = request.url_title
       rule = FactoryBot.create(:user_censor_rule,
                                user: user,
                                text: "rubbish",
                                replacement: "[REDACTED]")
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
 
       expect(page).to have_content "I have a [REDACTED] answer for you"
 
@@ -181,7 +175,7 @@
 
       perform_enqueued_jobs
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
       expect(page).not_to have_content "I have a [REDACTED] answer for you"
       expect(page).to have_content "I have a rubbish answer for you"
     end
@@ -190,8 +184,7 @@
   describe "Request censor rules" do
     it 'clears the cache for existing requests when a new rule is added' do
       request_id = request.id
-      url_title = request.url_title
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
 
       expect(page).to have_content "I have a rubbish answer for you"
 
@@ -205,20 +198,19 @@
 
       perform_enqueued_jobs
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
       expect(page).not_to have_content "I have a rubbish answer for you"
       expect(page).to have_content "I have [REDACTED] for you"
     end
 
     it 'clears the cache for existing requests when a rule is updated' do
       request_id = request.id
-      url_title = request.url_title
       rule = FactoryBot.create(:info_request_censor_rule,
                                info_request: request,
                                text: "rubbish",
                                replacement: "[REDACTED]")
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
 
       expect(page).to have_content "I have a [REDACTED] answer for you"
 
@@ -232,20 +224,19 @@
 
       perform_enqueued_jobs
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
       expect(page).not_to have_content "I have a [REDACTED] answer for you"
       expect(page).to have_content "I have a rubbish [REDACTED] for you"
     end
 
     it 'clears the cache for existing requests when a rule is deleted' do
       request_id = request.id
-      url_title = request.url_title
       rule = FactoryBot.create(:info_request_censor_rule,
                                info_request: request,
                                text: "rubbish",
                                replacement: "[REDACTED]")
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
 
       expect(page).to have_content "I have a [REDACTED] answer for you"
 
@@ -256,7 +247,7 @@
 
       perform_enqueued_jobs
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
       expect(page).not_to have_content "I have a [REDACTED] answer for you"
       expect(page).to have_content "I have a rubbish answer for you"
     end
@@ -264,8 +255,7 @@
 
   describe "Global censor rules" do
     it 'clears the cache for existing requests when a new rule is added' do
-      url_title = request.url_title
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
 
       expect(page).to have_content "I have a rubbish answer for you"
 
@@ -279,18 +269,17 @@
 
       perform_enqueued_jobs
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
       expect(page).not_to have_content "I have a rubbish answer for you"
       expect(page).to have_content "I have [REDACTED] for you"
     end
 
     it 'clears the cache for existing requests when a rule is updated' do
-      url_title = request.url_title
       rule = FactoryBot.create(:global_censor_rule,
                                text: "rubbish",
                                replacement: "[REDACTED]")
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
 
       expect(page).to have_content "I have a [REDACTED] answer for you"
 
@@ -304,18 +293,17 @@
 
       perform_enqueued_jobs
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
       expect(page).not_to have_content "I have a [REDACTED] answer for you"
       expect(page).to have_content "I have a rubbish [REDACTED] for you"
     end
 
     it 'clears the cache for existing requests when a rule is deleted' do
-      url_title = request.url_title
       rule = FactoryBot.create(:global_censor_rule,
                                text: "rubbish",
                                replacement: "[REDACTED]")
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
 
       expect(page).to have_content "I have a [REDACTED] answer for you"
 
@@ -326,7 +314,7 @@
 
       perform_enqueued_jobs
 
-      visit show_request_path url_title: url_title
+      visit show_request_path(request.url_title)
       expect(page).not_to have_content "I have a [REDACTED] answer for you"
       expect(page).to have_content "I have a rubbish answer for you"
     end
diff --git a/spec/integration/alaveteli_dsl.rb b/spec/integration/alaveteli_dsl.rb
index 4db60f8cd4..02fce75112 100644
--- a/spec/integration/alaveteli_dsl.rb
+++ b/spec/integration/alaveteli_dsl.rb
@@ -85,8 +85,7 @@ def hide_outgoing_message(outgoing_message, prominence, reason)
 end
 
 def classify_request(request, chosen_option)
-  visit show_request_path url_title: request.url_title,
-                          update_status: 1
+  visit show_request_path(request.url_title, update_status: 1)
   choose(chosen_option)
   click_button('Submit status')
 end
diff --git a/spec/integration/alaveteli_pro/request_list_spec.rb b/spec/integration/alaveteli_pro/request_list_spec.rb
index a7f9827beb..620c10707b 100644
--- a/spec/integration/alaveteli_pro/request_list_spec.rb
+++ b/spec/integration/alaveteli_pro/request_list_spec.rb
@@ -41,7 +41,7 @@
       expect(page).to have_css("#info-request-#{request.id}")
 
       within("#info-request-#{request.id}") do
-        request_path = show_request_path(url_title: request.url_title)
+        request_path = show_request_path(request.url_title)
         expect(page).to have_link(request.title, href: request_path)
         expect(page).to have_content(request.created_at.strftime('%d-%m-%Y'))
         expect(page).to have_content(request.updated_at.strftime('%d-%m-%Y'))
diff --git a/spec/integration/incoming_mail_spec.rb b/spec/integration/incoming_mail_spec.rb
index 79fecfae1a..af586681fe 100644
--- a/spec/integration/incoming_mail_spec.rb
+++ b/spec/integration/incoming_mail_spec.rb
@@ -14,7 +14,7 @@
     mail = deliveries[0]
     expect(mail.to).to eq([info_request.user.email])
     expect(mail.body).to match(/You have a new response to the Freedom of Information request/)
-    visit show_request_path url_title: info_request.url_title
+    visit show_request_path info_request.url_title
     expect(page).to have_content("No way!")
   end
 
@@ -57,7 +57,7 @@
   it "converts message body to UTF8" do
     receive_incoming_mail('iso8859_2_raw_email.email',
                           email_to: info_request.incoming_email)
-    visit show_request_path url_title: info_request.url_title
+    visit show_request_path(info_request.url_title)
     expect(page).to have_content "tënde"
   end
 
diff --git a/spec/mailers/request_mailer_spec.rb b/spec/mailers/request_mailer_spec.rb
index 1fe5859682..44aad8feb8 100644
--- a/spec/mailers/request_mailer_spec.rb
+++ b/spec/mailers/request_mailer_spec.rb
@@ -644,8 +644,9 @@ def sent_alert_params(request, type)
       mail.body.to_s =~ /(http:\/\/.*)/
       mail_url = $1
 
-      redirect_target =
-        show_request_path(info_request.url_title, anchor: 'describe_state_form_1')
+      redirect_target = show_request_path(
+        info_request.url_title, anchor: 'describe_state_form_1'
+      )
 
       expect(mail_url).to eq(signin_url(r: redirect_target))
 

From 889eb71abdfbacf7831f1d1440cbe71285ea8957 Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Wed, 6 Dec 2023 12:03:57 +0000
Subject: [PATCH 04/12] Update calls to followup routes

Remove the need to specify `request_id` in the params hash. Rails will
already correctly generate the correct route without this.

Removing means more consistency, shorter line lengths, and will be more
flexible when the route changes to use request URL title.
---
 app/controllers/refusal_advice_controller.rb          |  4 ++--
 app/helpers/info_request_helper.rb                    |  9 ++++++---
 app/helpers/link_to_helper.rb                         |  6 ++++--
 app/mailers/request_mailer.rb                         |  8 +++++---
 app/models/alaveteli_pro/activity_list/overdue.rb     |  2 +-
 .../alaveteli_pro/activity_list/very_overdue.rb       |  8 +++++---
 .../info_requests/_after_actions.html.erb             |  6 +++---
 app/views/followups/_choose_recipient.html.erb        | 10 ++++------
 app/views/request/_after_actions.html.erb             |  6 +++---
 spec/controllers/classifications_controller_spec.rb   |  8 ++++----
 spec/controllers/refusal_advice_controller_spec.rb    |  7 ++-----
 spec/mailers/request_mailer_spec.rb                   |  9 ++++++---
 .../alaveteli_pro/activity_list/overdue_spec.rb       |  8 +++++---
 .../alaveteli_pro/activity_list/very_overdue_spec.rb  | 11 +++++++----
 spec/views/request/show.html.erb_spec.rb              |  7 ++++---
 15 files changed, 61 insertions(+), 48 deletions(-)

diff --git a/app/controllers/refusal_advice_controller.rb b/app/controllers/refusal_advice_controller.rb
index 84b190f2a7..5ba5660c3d 100644
--- a/app/controllers/refusal_advice_controller.rb
+++ b/app/controllers/refusal_advice_controller.rb
@@ -39,10 +39,10 @@ def internal_redirect_to
 
     case action.target[:internal]
     when 'followup'
-      redirect_to new_request_followup_path(request_id: info_request.id)
+      redirect_to new_request_followup_path(info_request.id)
     when 'internal_review'
       redirect_to new_request_followup_path(
-        request_id: info_request.id, internal_review: '1'
+        info_request.id, internal_review: '1'
       )
     when 'new_request'
       redirect_to new_request_to_body_path(
diff --git a/app/helpers/info_request_helper.rb b/app/helpers/info_request_helper.rb
index 135b65b865..6fc83b33fc 100644
--- a/app/helpers/info_request_helper.rb
+++ b/app/helpers/info_request_helper.rb
@@ -112,9 +112,12 @@ def status_text_waiting_response_very_overdue(info_request, _opts = {})
       str += ' '
       str += _('You can <strong>complain</strong> by')
       str += ' '
-      str += link_to _('requesting an internal review'),
-                    new_request_followup_path(request_id: info_request.id) +
-                    '?internal_review=1'
+      str += link_to(
+        _('requesting an internal review'),
+        new_request_followup_path(
+          info_request.id, internal_review: 1
+        )
+      )
       str += '.'
     end
 
diff --git a/app/helpers/link_to_helper.rb b/app/helpers/link_to_helper.rb
index 9adc7837f8..5c5b031076 100755
--- a/app/helpers/link_to_helper.rb
+++ b/app/helpers/link_to_helper.rb
@@ -80,9 +80,11 @@ def new_response_url(info_request, incoming_message)
   def respond_to_last_url(info_request, options = {})
     last_response = info_request.get_last_public_response
     if last_response.nil?
-      new_request_followup_url(options.merge(request_id: info_request.id))
+      new_request_followup_url(info_request.id, options)
     else
-      new_request_incoming_followup_url(options.merge(request_id: info_request.id, incoming_message_id: last_response.id))
+      new_request_incoming_followup_url(
+        info_request.id, options.merge(incoming_message_id: last_response.id)
+      )
     end
   end
 
diff --git a/app/mailers/request_mailer.rb b/app/mailers/request_mailer.rb
index dfac46dd57..b6403953a9 100644
--- a/app/mailers/request_mailer.rb
+++ b/app/mailers/request_mailer.rb
@@ -162,9 +162,11 @@ def old_unclassified_updated(info_request)
 
   # Tell the requester that they need to clarify their request
   def not_clarified_alert(info_request, incoming_message)
-    respond_url = new_request_incoming_followup_url(request_id: info_request.id,
-                                                    incoming_message_id: incoming_message.id,
-                                                    anchor: 'followup')
+    respond_url = new_request_incoming_followup_url(
+      info_request.id,
+      incoming_message_id: incoming_message.id,
+      anchor: 'followup'
+    )
     @url = respond_url
     @incoming_message = incoming_message
     @info_request = info_request
diff --git a/app/models/alaveteli_pro/activity_list/overdue.rb b/app/models/alaveteli_pro/activity_list/overdue.rb
index 011c15cf7e..7c323d0d6e 100644
--- a/app/models/alaveteli_pro/activity_list/overdue.rb
+++ b/app/models/alaveteli_pro/activity_list/overdue.rb
@@ -10,7 +10,7 @@ def call_to_action
       end
 
       def call_to_action_url
-        new_request_followup_path(request_id: event.info_request.id, anchor: 'followup')
+        new_request_followup_path(event.info_request.id, anchor: 'followup')
       end
     end
   end
diff --git a/app/models/alaveteli_pro/activity_list/very_overdue.rb b/app/models/alaveteli_pro/activity_list/very_overdue.rb
index 3b78bd5db5..935907a881 100644
--- a/app/models/alaveteli_pro/activity_list/very_overdue.rb
+++ b/app/models/alaveteli_pro/activity_list/very_overdue.rb
@@ -10,9 +10,11 @@ def call_to_action
       end
 
       def call_to_action_url
-        new_request_followup_path(request_id: event.info_request.id,
-                                  anchor: 'followup',
-                                  internal_review: 1)
+        new_request_followup_path(
+          event.info_request.id,
+          anchor: 'followup',
+          internal_review: 1
+        )
       end
     end
   end
diff --git a/app/views/alaveteli_pro/info_requests/_after_actions.html.erb b/app/views/alaveteli_pro/info_requests/_after_actions.html.erb
index d9b8306345..b5b43a46ed 100644
--- a/app/views/alaveteli_pro/info_requests/_after_actions.html.erb
+++ b/app/views/alaveteli_pro/info_requests/_after_actions.html.erb
@@ -8,14 +8,14 @@
           <ul class="action-menu__menu__submenu">
             <li>
               <% if last_response.nil? %>
-                <%= link_to _("Send a followup"), new_request_followup_path(:request_id => info_request.id, :anchor => 'followup') %>
+                <%= link_to _("Send a followup"), new_request_followup_path(info_request.id, :anchor => 'followup') %>
               <% else %>
-                <%= link_to _("Write a reply"), new_request_incoming_followup_path(:request_id => info_request.id, :incoming_message_id => last_response.id, :anchor => 'followup') %>
+                <%= link_to _("Write a reply"), new_request_incoming_followup_path(info_request.id, :incoming_message_id => last_response.id, :anchor => 'followup') %>
               <% end %>
             </li>
 
             <li>
-              <%= link_to _("Request an internal review"), new_request_followup_path(:request_id => info_request.id, :internal_review => '1', :anchor => 'followup') %>
+              <%= link_to _("Request an internal review"), new_request_followup_path(info_request.id, :internal_review => '1', :anchor => 'followup') %>
             </li>
 
             <li>
diff --git a/app/views/followups/_choose_recipient.html.erb b/app/views/followups/_choose_recipient.html.erb
index 625562d02f..b8b9f78311 100644
--- a/app/views/followups/_choose_recipient.html.erb
+++ b/app/views/followups/_choose_recipient.html.erb
@@ -10,7 +10,7 @@
         <li>
           <%= link_to(_('the main FOI contact address for {{public_body}}',
                         public_body: name),
-                      new_request_followup_path(request_id: @info_request.id)) %>
+                      new_request_followup_path(@info_request.id)) %>
         </li>
       <% else %>
         <% if id.present? %>
@@ -18,21 +18,19 @@
             <li>
               <%= link_to(_('the main FOI contact address for {{public_body}}',
                             public_body: name),
-                          new_request_followup_path(request_id: @info_request.id)) %>
+                          new_request_followup_path(@info_request.id)) %>
             </li>
           <% else %>
             <li>
               <%= link_to name,
-                          new_request_incoming_followup_path(
-                            request_id: @info_request.id,
-                            incoming_message_id: id) %>
+                          new_request_incoming_followup_path(@info_request.id, incoming_message_id: id) %>
             </li>
           <% end %>
         <% else %>
           <li>
             <%= link_to(_('the main FOI contact address for {{public_body}}',
                           public_body: name),
-                        new_request_followup_path(request_id: @info_request.id)) %>
+                        new_request_followup_path(@info_request.id)) %>
           </li>
         <% end %>
       <% end %>
diff --git a/app/views/request/_after_actions.html.erb b/app/views/request/_after_actions.html.erb
index b05ef17bc2..0bb6c16e18 100644
--- a/app/views/request/_after_actions.html.erb
+++ b/app/views/request/_after_actions.html.erb
@@ -14,9 +14,9 @@
             <ul class="action-menu__menu__submenu owner_actions">
               <li>
                 <% if last_response.nil? %>
-                  <%= link_to _('Send a followup'), new_request_followup_path(request_id: info_request.id) %>
+                  <%= link_to _('Send a followup'), new_request_followup_path(info_request.id) %>
                 <% else %>
-                  <%= link_to _('Write a reply'), new_request_incoming_followup_path(request_id: info_request.id, incoming_message_id: last_response.id) %>
+                  <%= link_to _('Write a reply'), new_request_incoming_followup_path(info_request.id, incoming_message_id: last_response.id) %>
                 <% end %>
               </li>
 
@@ -27,7 +27,7 @@
               <% end %>
 
               <li>
-                <%= link_to _('Request an internal review'), new_request_followup_path(request_id: info_request.id, internal_review: '1') %>
+                <%= link_to _('Request an internal review'), new_request_followup_path(info_request.id, internal_review: '1') %>
               </li>
             </ul>
           </li>
diff --git a/spec/controllers/classifications_controller_spec.rb b/spec/controllers/classifications_controller_spec.rb
index f8c81c9373..49936215c0 100644
--- a/spec/controllers/classifications_controller_spec.rb
+++ b/spec/controllers/classifications_controller_spec.rb
@@ -493,7 +493,7 @@ def post_status(status, message: nil)
               post_status('waiting_clarification')
               expect(response).to redirect_to(
                 new_request_incoming_followup_path(
-                  request_id: info_request.id,
+                  info_request.id,
                   incoming_message_id: info_request.get_last_public_response.id
                 )
               )
@@ -512,7 +512,7 @@ def post_status(status, message: nil)
               post_status('waiting_clarification')
               expect(response).to redirect_to(
                 new_request_followup_path(
-                  request_id: info_request.id,
+                  info_request.id,
                   incoming_message_id: nil
                 )
               )
@@ -566,7 +566,7 @@ def post_status(status, message: nil)
             post_status('gone_postal')
             expect(response).to redirect_to(
               new_request_incoming_followup_path(
-                request_id: info_request.id,
+                info_request.id,
                 incoming_message_id: info_request.get_last_public_response.id,
                 gone_postal: 1
               )
@@ -668,7 +668,7 @@ def post_status(status, message: nil)
             post_status('user_withdrawn')
             expect(response).to redirect_to(
               new_request_incoming_followup_path(
-                request_id: info_request.id,
+                info_request.id,
                 incoming_message_id: info_request.get_last_public_response.id
               )
             )
diff --git a/spec/controllers/refusal_advice_controller_spec.rb b/spec/controllers/refusal_advice_controller_spec.rb
index 4bd03600bd..21fe5a455c 100644
--- a/spec/controllers/refusal_advice_controller_spec.rb
+++ b/spec/controllers/refusal_advice_controller_spec.rb
@@ -38,7 +38,7 @@
         it 'redirects to new followup action' do
           post :create, params: params
           expect(response).to redirect_to(
-            new_request_followup_path(request_id: info_request.id)
+            new_request_followup_path(info_request.id)
           )
         end
       end
@@ -49,10 +49,7 @@
         it 'redirects to new internal review action' do
           post :create, params: params
           expect(response).to redirect_to(
-            new_request_followup_path(
-              request_id: info_request.id,
-              internal_review: '1'
-            )
+            new_request_followup_path(info_request.id, internal_review: '1')
           )
         end
       end
diff --git a/spec/mailers/request_mailer_spec.rb b/spec/mailers/request_mailer_spec.rb
index 44aad8feb8..4ee111cfa7 100644
--- a/spec/mailers/request_mailer_spec.rb
+++ b/spec/mailers/request_mailer_spec.rb
@@ -936,9 +936,12 @@ def force_updated_at_to_past(request)
       mail.body.to_s =~ /(http:\/\/.*)/
       mail_url = $1
 
-      expect(mail_url).
-        to match(new_request_incoming_followup_path(request_id: ir.id,
-                                    incoming_message_id: ir.incoming_messages.last.id))
+      expect(mail_url).to match(
+        new_request_incoming_followup_path(
+          ir.id,
+          incoming_message_id: ir.incoming_messages.last.id
+        )
+      )
     end
 
     it "skips requests that don't have a public last response" do
diff --git a/spec/models/alaveteli_pro/activity_list/overdue_spec.rb b/spec/models/alaveteli_pro/activity_list/overdue_spec.rb
index e33cd1ea62..68cd201bba 100644
--- a/spec/models/alaveteli_pro/activity_list/overdue_spec.rb
+++ b/spec/models/alaveteli_pro/activity_list/overdue_spec.rb
@@ -24,9 +24,11 @@
 
   describe '#call_to_action_url' do
     it 'returns the url of the info_request' do
-      expect(activity.call_to_action_url).
-        to eq new_request_followup_path(request_id: event.info_request.id,
-                                        anchor: 'followup')
+      expect(activity.call_to_action_url).to eq(
+        new_request_followup_path(
+          event.info_request.id, anchor: 'followup'
+        )
+      )
     end
   end
 end
diff --git a/spec/models/alaveteli_pro/activity_list/very_overdue_spec.rb b/spec/models/alaveteli_pro/activity_list/very_overdue_spec.rb
index 524ee12c8b..14783ec007 100644
--- a/spec/models/alaveteli_pro/activity_list/very_overdue_spec.rb
+++ b/spec/models/alaveteli_pro/activity_list/very_overdue_spec.rb
@@ -24,10 +24,13 @@
 
   describe '#call_to_action_url' do
     it 'returns the url of the info_request' do
-      expect(activity.call_to_action_url).
-        to eq new_request_followup_path(request_id: event.info_request.id,
-                                        anchor: 'followup',
-                                        internal_review: 1)
+      expect(activity.call_to_action_url).to eq(
+        new_request_followup_path(
+          event.info_request.id,
+          anchor: 'followup',
+          internal_review: 1
+        )
+      )
     end
   end
 end
diff --git a/spec/views/request/show.html.erb_spec.rb b/spec/views/request/show.html.erb_spec.rb
index 9c47185e42..93556e1ead 100644
--- a/spec/views/request/show.html.erb_spec.rb
+++ b/spec/views/request/show.html.erb_spec.rb
@@ -88,8 +88,9 @@ def request_page
             and_return(mock_response)
           request_page
           expected_url = new_request_incoming_followup_path(
-                          request_id: mock_request.id,
-                          incoming_message_id: mock_response.id)
+            mock_request.id,
+            incoming_message_id: mock_response.id
+          )
           expect(response.body).
             to have_css(
               "a[href='#{expected_url}']",
@@ -106,7 +107,7 @@ def request_page
 
         it "should show a link to follow up the request without reference to a specific response" do
           request_page
-          expected_url = new_request_followup_path(request_id: mock_request.id)
+          expected_url = new_request_followup_path(mock_request.id)
           expect(response.body).
             to have_css(
               "a[href='#{expected_url}']",

From 2b2dd2ac1480b088c052fbbd98b0743ca7957e72 Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Thu, 7 Dec 2023 08:22:53 +0000
Subject: [PATCH 05/12] Update calls to attachment routes

Remove the need to specify `id` in the params hash. Rails will already
correctly generate the correct route without this.

Removing means more consistency, shorter line lengths, and will be more
flexible when the route changes to use request URL title.
---
 app/helpers/info_request_helper.rb                   | 10 +++++-----
 spec/controllers/attachments_controller_spec.rb      |  4 ++--
 spec/integration/incoming_mail_spec.rb               | 12 ++++++------
 .../prominence/viewing_attachment_as_html_spec.rb    |  4 ++--
 .../prominence/viewing_raw_attachment_spec.rb        |  4 ++--
 5 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/app/helpers/info_request_helper.rb b/app/helpers/info_request_helper.rb
index 6fc83b33fc..6b6de66d50 100644
--- a/app/helpers/info_request_helper.rb
+++ b/app/helpers/info_request_helper.rb
@@ -289,13 +289,13 @@ def attachment_url(attachment, options = {})
     attach_params = attachment_params(attachment, options)
 
     if options[:html] && public_token?
-      share_attachment_as_html_url(attach_params)
+      share_attachment_as_html_url(*attach_params)
     elsif options[:html]
-      get_attachment_as_html_url(attach_params)
+      get_attachment_as_html_url(*attach_params)
     elsif public_token?
-      share_attachment_url(attach_params)
+      share_attachment_url(*attach_params)
     else
-      get_attachment_url(attach_params)
+      get_attachment_url(*attach_params)
     end
   end
 
@@ -328,7 +328,7 @@ def attachment_params(attachment, options = {})
 
     attach_params[:project_id] = @project.id if @project
 
-    attach_params
+    [attach_params.delete(:id), attach_params]
   end
 
   def public_token?
diff --git a/spec/controllers/attachments_controller_spec.rb b/spec/controllers/attachments_controller_spec.rb
index 7eb25cc749..f9f1d128eb 100644
--- a/spec/controllers/attachments_controller_spec.rb
+++ b/spec/controllers/attachments_controller_spec.rb
@@ -167,8 +167,8 @@ def show(params = {})
           allow(controller).to receive(:verifier).and_return(verifier)
           allow(verifier).to receive(:generate).with(
             get_attachment_path(
+              info_request.id,
               incoming_message_id: attachment.incoming_message_id,
-              id: info_request.id,
               part: attachment.url_part_number,
               file_name: attachment.filename
             )
@@ -542,8 +542,8 @@ def show_as_html(params = {})
           allow(controller).to receive(:verifier).and_return(verifier)
           allow(verifier).to receive(:generate).with(
             get_attachment_as_html_path(
+              info_request.id,
               incoming_message_id: attachment.incoming_message_id,
-              id: info_request.id,
               part: attachment.url_part_number,
               file_name: attachment.filename
             )
diff --git a/spec/integration/incoming_mail_spec.rb b/spec/integration/incoming_mail_spec.rb
index af586681fe..98844c2890 100644
--- a/spec/integration/incoming_mail_spec.rb
+++ b/spec/integration/incoming_mail_spec.rb
@@ -23,15 +23,15 @@
                           email_to: info_request.incoming_email)
 
     attachment_1_path = get_attachment_path(
+      info_request.id,
       incoming_message_id: info_request.incoming_messages.first.id,
-      id: info_request.id,
       part: 2,
       file_name: 'hello world.txt',
       skip_cache: 1
     )
     attachment_2_path = get_attachment_path(
+      info_request.id,
       incoming_message_id: info_request.incoming_messages.first.id,
-      id: info_request.id,
       part: 3,
       file_name: 'hello world.txt',
       skip_cache: 1
@@ -65,8 +65,8 @@
     receive_incoming_mail('incoming-request-two-same-name.email',
                           email_to: info_request.incoming_email)
     attachment_path = get_attachment_as_html_path(
+      info_request.id,
       incoming_message_id: info_request.incoming_messages.first.id,
-      id: info_request.id,
       part: 2,
       file_name: 'hello world.txt.html',
       skip_cache: 1)
@@ -83,8 +83,8 @@
     receive_incoming_mail('incoming-request-pdf-attachment.email',
                           email_to: info_request.incoming_email)
     attachment_path = get_attachment_as_html_path(
+      info_request.id,
       incoming_message_id: info_request.incoming_messages.first.id,
-      id: info_request.id,
       part: 2,
       file_name: 'fs 50379341.pdf.html',
       skip_cache: 1)
@@ -103,8 +103,8 @@
     # asking for an attachment by the wrong filename should result in
     # redirecting back to the incoming message
     visit get_attachment_as_html_path(
+      info_request.id,
       incoming_message_id: info_request.incoming_messages.first.id,
-      id: info_request.id,
       part: 2,
       file_name: 'hello world.txt.baz.html',
       skip_cache: 1
@@ -118,8 +118,8 @@
                           email_to: info_request.incoming_email)
 
     attachment_path = get_attachment_path(
+      info_request.id,
       incoming_message_id: info_request.incoming_messages.first.id,
-      id: info_request.id,
       part: 2,
       file_name: 'hello.qwglhm',
       skip_cache: 1
diff --git a/spec/integration/prominence/viewing_attachment_as_html_spec.rb b/spec/integration/prominence/viewing_attachment_as_html_spec.rb
index 92267d8b31..beeb7d3604 100644
--- a/spec/integration/prominence/viewing_attachment_as_html_spec.rb
+++ b/spec/integration/prominence/viewing_attachment_as_html_spec.rb
@@ -8,10 +8,10 @@
   let(:within_session) do
     -> {
       visit get_attachment_as_html_url(
+        info_request.id,
         incoming_message_id: attachment.incoming_message_id,
         part: attachment.url_part_number,
-        file_name: "#{attachment.display_filename}.html",
-        id: info_request.id
+        file_name: "#{attachment.display_filename}.html"
       )
     }
   end
diff --git a/spec/integration/prominence/viewing_raw_attachment_spec.rb b/spec/integration/prominence/viewing_raw_attachment_spec.rb
index a432f31c45..7d49a6cd0f 100644
--- a/spec/integration/prominence/viewing_raw_attachment_spec.rb
+++ b/spec/integration/prominence/viewing_raw_attachment_spec.rb
@@ -10,10 +10,10 @@
       rebuild_raw_emails(info_request)
 
       visit get_attachment_url(
+        info_request.id,
         incoming_message_id: attachment.incoming_message_id,
         part: attachment.url_part_number,
-        file_name: attachment.display_filename,
-        id: info_request.id
+        file_name: attachment.display_filename
       )
     }
   end

From 1afd3586ce8818091e59259736031cca7803a6ab Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Thu, 7 Dec 2023 08:27:50 +0000
Subject: [PATCH 06/12] Update calls to report routes

Remove the need to specify `request_id` in the params hash. Rails will
already correctly generate the correct route without this.

Removing means more consistency, shorter line lengths, and will be more
flexible when the route changes to use request URL title.
---
 app/views/comment/_single_comment.html.erb          | 2 +-
 app/views/reports/new.html.erb                      | 2 +-
 app/views/request/_after_actions.html.erb           | 2 +-
 app/views/request/_incoming_correspondence.html.erb | 2 +-
 app/views/request/_outgoing_correspondence.html.erb | 2 +-
 spec/integration/reports_controller_spec.rb         | 4 ++--
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/app/views/comment/_single_comment.html.erb b/app/views/comment/_single_comment.html.erb
index 286e6c9eaa..292132f38c 100644
--- a/app/views/comment/_single_comment.html.erb
+++ b/app/views/comment/_single_comment.html.erb
@@ -35,7 +35,7 @@
     <% link_to_this_url = comment_url(comment) %>
 
     <% report_path =
-         new_request_report_path(request_id: @info_request.url_title,
+         new_request_report_path(@info_request.url_title,
                                  comment_id: comment.id) %>
 
     <%= render partial: 'request/correspondence_footer',
diff --git a/app/views/reports/new.html.erb b/app/views/reports/new.html.erb
index 0f52a0d6ce..da9e16c781 100644
--- a/app/views/reports/new.html.erb
+++ b/app/views/reports/new.html.erb
@@ -11,7 +11,7 @@
     </p>
     <p><%= _("Why specifically do you consider this request unsuitable?") %></p>
 
-    <%= form_tag request_report_path(:request_id => @info_request.url_title) do %>
+    <%= form_tag request_report_path(@info_request.url_title) do %>
       <p>
         <label class="form_label" for="reason"><%= _('Reason:') %></label>
         <%= select_tag :reason, options_for_select(@report_reasons, @reason), :prompt => _("Choose a reason") %>
diff --git a/app/views/request/_after_actions.html.erb b/app/views/request/_after_actions.html.erb
index 0bb6c16e18..e596d8db41 100644
--- a/app/views/request/_after_actions.html.erb
+++ b/app/views/request/_after_actions.html.erb
@@ -61,7 +61,7 @@
               </li>
             <% else %>
               <li>
-               <%= link_to _("Report this request"), new_request_report_path(:request_id => info_request.url_title) %><span class="action-menu__info-link">
+                <%= link_to _("Report this request"), new_request_report_path(info_request.url_title) %><span class="action-menu__info-link">
                 <%= link_to _("Help"), help_about_path(:anchor => "reporting") %>
               </span>
               </li>
diff --git a/app/views/request/_incoming_correspondence.html.erb b/app/views/request/_incoming_correspondence.html.erb
index ade5c8a466..6d05b5178c 100644
--- a/app/views/request/_incoming_correspondence.html.erb
+++ b/app/views/request/_incoming_correspondence.html.erb
@@ -37,7 +37,7 @@
       <% link_to_this_url = incoming_message_url(incoming_message) %>
 
       <% report_path =
-           new_request_report_path(request_id: @info_request.url_title,
+           new_request_report_path(@info_request.url_title,
                                    incoming_message_id: incoming_message.id) %>
 
       <%= render partial: 'request/correspondence_footer',
diff --git a/app/views/request/_outgoing_correspondence.html.erb b/app/views/request/_outgoing_correspondence.html.erb
index 43e0fbdc4a..16d332c781 100644
--- a/app/views/request/_outgoing_correspondence.html.erb
+++ b/app/views/request/_outgoing_correspondence.html.erb
@@ -46,7 +46,7 @@
       <% link_to_this_url = outgoing_message_url(outgoing_message) %>
 
       <% report_path =
-           new_request_report_path(request_id: @info_request.url_title,
+           new_request_report_path(@info_request.url_title,
                                    outgoing_message_id: outgoing_message.id) %>
 
       <%= render partial: 'request/correspondence_footer',
diff --git a/spec/integration/reports_controller_spec.rb b/spec/integration/reports_controller_spec.rb
index d9494fcc7d..fd698f1738 100644
--- a/spec/integration/reports_controller_spec.rb
+++ b/spec/integration/reports_controller_spec.rb
@@ -9,14 +9,14 @@
 
     describe 'when not logged in' do
       it "should redirect to the login page" do
-        visit new_request_report_path(request_id: request.url_title,
+        visit new_request_report_path(request.url_title,
                                       comment_id: comment.id)
 
         expect(page).to have_content "create an account or sign in"
       end
 
       it "should not lose the comment_id post login" do
-        visit new_request_report_path(request_id: request.url_title,
+        visit new_request_report_path(request.url_title,
                                       comment_id: comment.id)
 
         fill_in :user_signin_email, with: user.email

From dabda2d05a7c815d74c9d16bd01f17999ad46a37 Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Thu, 16 Nov 2023 11:45:10 +0000
Subject: [PATCH 07/12] Move request controller numerical redirect

Now we can a redirects routing file with similar redirects we can move
the before action callback which redirects request numerical IDs to the
more friendly URL titles.
---
 app/controllers/request_controller.rb       | 13 -----------
 config/routes.rb                            |  2 ++
 config/routes/redirects.rb                  | 26 +++++++++++++++++++++
 spec/controllers/request_controller_spec.rb | 10 --------
 spec/integration/request_controller_spec.rb |  9 +++++++
 spec/routing/redirects_spec.rb              | 16 +++++++++++++
 6 files changed, 53 insertions(+), 23 deletions(-)
 create mode 100644 config/routes/redirects.rb
 create mode 100644 spec/routing/redirects_spec.rb

diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index 4623ff5f07..a5abeec4ef 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -11,7 +11,6 @@ class RequestController < ApplicationController
 
   before_action :check_read_only, only: [:new, :upload_response]
   before_action :set_render_recaptcha, only: [ :new ]
-  before_action :redirect_numeric_id_to_url_title, only: [:show]
   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]
@@ -712,18 +711,6 @@ def set_render_recaptcha
                         (!@user || !@user.confirmed_not_spam?)
   end
 
-  def redirect_numeric_id_to_url_title
-    # Look up by old style numeric identifiers
-    if params[:url_title].match(/^[0-9]+$/)
-      @info_request = InfoRequest.find(params[:url_title].to_i)
-      # We don't want to leak the title of embargoed or hidden requests, so
-      # don't even redirect on if the user can't access the request
-      return render_hidden if cannot?(:read, @info_request)
-
-      redirect_to request_url(@info_request, format: params[:format])
-    end
-  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
diff --git a/config/routes.rb b/config/routes.rb
index 8bd8f2b419..1dff81202f 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -24,6 +24,8 @@ def matches?(request)
 end
 
 Rails.application.routes.draw do
+  draw :redirects
+
   mount Sidekiq::Web => '/sidekiq', constraints: AdminConstraint.new
 
   root to: 'general#frontpage'
diff --git a/config/routes/redirects.rb b/config/routes/redirects.rb
new file mode 100644
index 0000000000..207e3d2044
--- /dev/null
+++ b/config/routes/redirects.rb
@@ -0,0 +1,26 @@
+info_request_redirect = redirect do |params, request|
+  # find request
+  info_request = InfoRequest.find(params.fetch(:id))
+
+  # check if current user can read the request
+  ability = Ability.new(User.authenticate_from_session(request.session))
+  raise ActiveRecord::RecordNotFound if ability.cannot?(:read, info_request)
+
+  # encode path components
+  encoded_parts = [
+    *params[:locale], 'request', info_request.url_title
+  ].map do |part|
+    if RUBY_VERSION < '3.1'
+      URI.encode_www_form_component(part).gsub('+', '%20')
+    else
+      URI.encode_uri_component(part)
+    end
+  end
+
+  # join encoded parts together with slashes
+  encoded_parts.join('/')
+end
+
+get '/request/:id',
+  constraints: { id: /\d+/ },
+  to: info_request_redirect
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index ba993c02d0..53fc2eb439 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -61,16 +61,6 @@
     expect(assigns[:info_request]).to eq(info_requests(:fancy_dog_request))
   end
 
-  it "should redirect from a numeric URL to pretty one" do
-    get :show, params: {
-      url_title: info_requests(:naughty_chicken_request).id.to_s
-    }
-    expect(response).to redirect_to(
-      action: 'show',
-      url_title: info_requests(:naughty_chicken_request).url_title
-    )
-  end
-
   it 'should return a 404 for GET requests to a malformed request URL' do
     expect {
       get :show, params: { url_title: '228%85' }
diff --git a/spec/integration/request_controller_spec.rb b/spec/integration/request_controller_spec.rb
index 593405c6c6..10d3f55356 100644
--- a/spec/integration/request_controller_spec.rb
+++ b/spec/integration/request_controller_spec.rb
@@ -63,4 +63,13 @@
       end
     end
   end
+
+  it "should redirect from a numeric URL to pretty one" do
+    visit show_request_path(info_requests(:naughty_chicken_request).id)
+    expect(current_path).to eq(
+      show_request_path(
+        info_requests(:naughty_chicken_request).url_title
+      )
+    )
+  end
 end
diff --git a/spec/routing/redirects_spec.rb b/spec/routing/redirects_spec.rb
new file mode 100644
index 0000000000..a784a575dc
--- /dev/null
+++ b/spec/routing/redirects_spec.rb
@@ -0,0 +1,16 @@
+require 'spec_helper'
+
+RSpec.describe 'routing redirects', type: :request do
+  it 'routes numerical request route to URL title route' do
+    get('/request/105')
+    expect(response).to redirect_to('/request/the_cost_of_boring')
+  end
+
+  it 'redirects numerical request routes with locales' do
+    get('/fr/request/105')
+    expect(response).to redirect_to('/fr/request/the_cost_of_boring')
+
+    get('/en_GB/request/105')
+    expect(response).to redirect_to('/en_GB/request/the_cost_of_boring')
+  end
+end

From 4d58ba26332b6999c8b42da7801525477a54b5b7 Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Thu, 7 Dec 2023 12:41:57 +0000
Subject: [PATCH 08/12] Minor cleanup

Fix pre-existing linting issues with code lines which will be changing.

Fixes:
- line length
- indentation
- leading periods
---
 app/controllers/reports_controller.rb         |   5 +-
 config/routes.rb                              |   3 +-
 spec/controllers/followups_controller_spec.rb |  68 +++---
 spec/controllers/reports_controller_spec.rb   | 226 +++++++++---------
 4 files changed, 151 insertions(+), 151 deletions(-)

diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb
index 9913f6f721..5daecd8ed2 100644
--- a/app/controllers/reports_controller.rb
+++ b/app/controllers/reports_controller.rb
@@ -49,9 +49,8 @@ def new
   private
 
   def set_info_request
-    @info_request = InfoRequest
-                      .not_embargoed
-                        .find_by_url_title!(params[:request_id])
+    @info_request = InfoRequest.not_embargoed.
+      find_by_url_title!(params[:request_id])
   end
 
   def set_comment
diff --git a/config/routes.rb b/config/routes.rb
index 1dff81202f..7636a67c28 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -192,7 +192,8 @@ def matches?(request)
   match '/request/:request_id/followups/new' => 'followups#new',
         :as => :new_request_followup,
         :via => :get
-  match '/request/:request_id/followups/new/:incoming_message_id' => 'followups#new',
+  match '/request/:request_id/followups/new/:incoming_message_id' =>
+          'followups#new',
         :as => :new_request_incoming_followup,
         :via => :get
   match '/request/:request_id/followups/preview' => 'followups#preview',
diff --git a/spec/controllers/followups_controller_spec.rb b/spec/controllers/followups_controller_spec.rb
index ad7b930199..ae7d2318c3 100644
--- a/spec/controllers/followups_controller_spec.rb
+++ b/spec/controllers/followups_controller_spec.rb
@@ -34,9 +34,9 @@
 
       it "displays 'wrong user' message when not logged in as the request owner" do
         get :new, params: {
-                    request_id: request.id,
-                    incoming_message_id: message_id
-                  }
+          request_id: request.id,
+          incoming_message_id: message_id
+        }
         expect(response).to render_template('user/wrong_user')
       end
 
@@ -51,9 +51,9 @@
     it "displays 'wrong user' message when not logged in as the request owner" do
       sign_in FactoryBot.create(:user)
       get :new, params: {
-                  request_id: request.id,
-                  incoming_message_id: message_id
-                }
+        request_id: request.id,
+        incoming_message_id: message_id
+      }
       expect(response).to render_template('user/wrong_user')
     end
 
@@ -154,10 +154,10 @@
           incoming_message_id = hidden_request.incoming_messages[0].id
           expect do
             get :new, params: {
-                        request_id: hidden_request.id,
-                        incoming_message_id: incoming_message_id,
-                        format: 'json'
-                      }
+              request_id: hidden_request.id,
+              incoming_message_id: incoming_message_id,
+              format: 'json'
+            }
           end.to raise_error ActionController::UnknownFormat
         end
       end
@@ -233,18 +233,18 @@
         embargoed_request = FactoryBot.create(:embargoed_request)
         expect {
           post :preview, params: {
-                           outgoing_message: dummy_message,
-                           request_id: embargoed_request.id
-                         }
+            outgoing_message: dummy_message,
+            request_id: embargoed_request.id
+          }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
       it "redirects to the signin page" do
         post :preview, params: {
-                         outgoing_message: dummy_message,
-                         request_id: request.id,
-                         incoming_message_id: message_id
-                       }
+          outgoing_message: dummy_message,
+          request_id: request.id,
+          incoming_message_id: message_id
+        }
         expect(response).
           to redirect_to(signin_url(token: get_last_post_redirect.token))
       end
@@ -259,9 +259,9 @@
         embargoed_request = FactoryBot.create(:embargoed_request,
                                               user: pro_user)
         post :preview, params: {
-                         outgoing_message: dummy_message,
-                         request_id: embargoed_request.id
-                       }
+          outgoing_message: dummy_message,
+          request_id: embargoed_request.id
+        }
         expect(response).to be_successful
       end
 
@@ -269,9 +269,9 @@
         embargoed_request = FactoryBot.create(:embargoed_request)
         expect {
           post :preview, params: {
-                           outgoing_message: dummy_message,
-                           request_id: embargoed_request.id
-                         }
+            outgoing_message: dummy_message,
+            request_id: embargoed_request.id
+          }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
     end
@@ -279,10 +279,10 @@
     it "displays a wrong user message when not logged in as the request owner" do
       sign_in FactoryBot.create(:user)
       post :preview, params: {
-                       outgoing_message: dummy_message,
-                       request_id: request.id,
-                       incoming_message_id: message_id
-                     }
+        outgoing_message: dummy_message,
+        request_id: request.id,
+        incoming_message_id: message_id
+      }
       expect(response).to render_template('user/wrong_user')
     end
 
@@ -293,13 +293,13 @@
 
       it "displays the edit form with an error when the message body is blank" do
         post :preview, params: {
-                         request_id: request.id,
-                         outgoing_message: {
-                           body: "",
-                           what_doing: "normal_sort"
-                         },
-                         incoming_message_id: message_id
-                       }
+          request_id: request.id,
+          outgoing_message: {
+            body: "",
+            what_doing: "normal_sort"
+          },
+          incoming_message_id: message_id
+        }
 
         expect(response).to render_template("new")
         expect(response.body).to include("Please enter your follow up message")
diff --git a/spec/controllers/reports_controller_spec.rb b/spec/controllers/reports_controller_spec.rb
index 7cd912b953..893eb786e8 100644
--- a/spec/controllers/reports_controller_spec.rb
+++ b/spec/controllers/reports_controller_spec.rb
@@ -8,9 +8,9 @@
     context "when reporting a request when not logged in" do
       it "should only allow logged-in users to report requests" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          reason: "my reason"
+        }
         expect(flash[:notice]).to match(/You need to be logged in/)
         expect(response).
           to redirect_to show_request_path(info_request.url_title)
@@ -24,36 +24,36 @@
 
       it "finds the expected request" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          reason: "my reason"
+        }
 
         expect(assigns(:info_request)).to eq(info_request)
       end
 
       it "sets reportable to the request" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          reason: "my reason"
+        }
 
         expect(assigns(:reportable)).to eq(info_request)
       end
 
       it "sets report_reasons to the request report reasons" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          reason: "my reason"
+        }
 
         expect(assigns(:report_reasons)).to eq(info_request.report_reasons)
       end
 
       it 'logs an event' do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        reason: 'my reason'
-                      }
+          request_id: info_request.url_title,
+          reason: 'my reason'
+        }
         last_event = info_request.reload.last_event
         expect(last_event.event_type).to eq('report_request')
         expect(last_event.params).to eq(
@@ -68,10 +68,10 @@
 
       it 'ignores an empty comment_id param' do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        comment_id: '',
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          comment_id: '',
+          reason: "my reason"
+        }
         expect(assigns[:comment]).to be_nil
       end
 
@@ -92,9 +92,9 @@
         expect(info_request.attention_requested).to eq(false)
 
         post :create, params: {
-                        request_id: info_request.url_title,
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          reason: "my reason"
+        }
         expect(response).
           to redirect_to show_request_path(info_request.url_title)
 
@@ -107,26 +107,26 @@
         expected = "This request has been reported for administrator attention"
 
         post :create, params: {
-                        request_id: info_request.url_title,
-                        reason: "my reason",
-                        message: "It's just not"
-                      }
+          request_id: info_request.url_title,
+          reason: "my reason",
+          message: "It's just not"
+        }
 
         expect(flash[:notice]).to eq(expected)
       end
 
       it "should not allow a request to be reported twice" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          reason: "my reason"
+        }
         expect(response).
           to redirect_to show_request_url(info_request.url_title)
 
         post :create, params: {
-                        request_id: info_request.url_title,
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          reason: "my reason"
+        }
         expect(response).
           to redirect_to show_request_url(info_request.url_title)
         expect(flash[:notice]).to match(/has already been reported/)
@@ -134,10 +134,10 @@
 
       it "should send an email from the reporter to admins" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        reason: "my reason",
-                        message: "It's just not"
-                      }
+          request_id: info_request.url_title,
+          reason: "my reason",
+          message: "It's just not"
+        }
         deliveries = ActionMailer::Base.deliveries
         expect(deliveries.size).to eq(1)
         mail = deliveries[0]
@@ -150,9 +150,9 @@
 
       it "should force the user to pick a reason" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        reason: ""
-                      }
+          request_id: info_request.url_title,
+          reason: ""
+        }
         expect(response).to render_template("new")
         expect(flash[:error]).to eq("Please choose a reason")
       end
@@ -170,40 +170,40 @@
 
       it "finds the expected request" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        comment_id: comment.id,
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          comment_id: comment.id,
+          reason: "my reason"
+        }
 
         expect(assigns(:info_request)).to eq(info_request)
       end
 
       it "finds the expected comment" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        comment_id: comment.id,
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          comment_id: comment.id,
+          reason: "my reason"
+        }
 
         expect(assigns(:comment)).to eq(comment)
       end
 
       it "sets reportable to the comment" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        comment_id: comment.id,
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          comment_id: comment.id,
+          reason: "my reason"
+        }
 
         expect(assigns(:reportable)).to eq(comment)
       end
 
       it "sets report_reasons to the comment report reasons" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        comment_id: comment.id,
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          comment_id: comment.id,
+          reason: "my reason"
+        }
 
         expect(assigns(:report_reasons)).to eq(comment.report_reasons)
       end
@@ -212,19 +212,19 @@
         new_comment = FactoryBot.create(:comment)
         expect {
           post :create, params: {
-                          request_id: info_request.url_title,
-                          comment_id: new_comment.id,
-                          reason: "my reason"
-                        }
+            request_id: info_request.url_title,
+            comment_id: new_comment.id,
+            reason: "my reason"
+          }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
       it "marks the comment as having been reported" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        comment_id: comment.id,
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          comment_id: comment.id,
+          reason: "my reason"
+        }
 
         comment.reload
         expect(comment.attention_requested).to eq(true)
@@ -232,10 +232,10 @@
 
       it "does not mark the parent request as having been reported" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        comment_id: comment.id,
-                        reason: "my reason"
-                      }
+          request_id: info_request.url_title,
+          comment_id: comment.id,
+          reason: "my reason"
+        }
 
         info_request.reload
         expect(info_request.attention_requested).to eq(false)
@@ -244,11 +244,11 @@
 
       it "sends an email alerting admins to the report" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        comment_id: comment.id,
-                        reason: "my reason",
-                        message: "It's just not"
-                      }
+          request_id: info_request.url_title,
+          comment_id: comment.id,
+          reason: "my reason",
+          message: "It's just not"
+        }
         deliveries = ActionMailer::Base.deliveries
 
         expect(deliveries.size).to eq(1)
@@ -272,22 +272,22 @@
                    "administrator attention"
 
         post :create, params: {
-                        request_id: info_request.url_title,
-                        comment_id: comment.id,
-                        reason: "my reason",
-                        message: "It's just not"
-                      }
+          request_id: info_request.url_title,
+          comment_id: comment.id,
+          reason: "my reason",
+          message: "It's just not"
+        }
 
         expect(flash[:notice]).to eq(expected)
       end
 
       it "redirects to the parent info_request page" do
         post :create, params: {
-                        request_id: info_request.url_title,
-                        comment_id: comment.id,
-                        reason: "my reason",
-                        message: "It's just not"
-                      }
+          request_id: info_request.url_title,
+          comment_id: comment.id,
+          reason: "my reason",
+          message: "It's just not"
+        }
 
         expect(response).
           to redirect_to show_request_path(info_request.url_title)
@@ -367,45 +367,45 @@
 
       it "finds the expected request" do
         get :new, params: {
-                    request_id: info_request.url_title,
-                    comment_id: comment.id
-                  }
+          request_id: info_request.url_title,
+          comment_id: comment.id
+        }
         expect(assigns(:info_request)).to eq(info_request)
       end
 
       it "finds the expected comment" do
         get :new, params: {
-                    request_id: info_request.url_title,
-                    comment_id: comment.id,
-                    reason: "my reason"
-                  }
+          request_id: info_request.url_title,
+          comment_id: comment.id,
+          reason: "my reason"
+        }
 
         expect(assigns(:comment)).to eq(comment)
       end
 
       it "sets reportable to the comment" do
         get :new, params: {
-                    request_id: info_request.url_title,
-                    comment_id: comment.id
-                  }
+          request_id: info_request.url_title,
+          comment_id: comment.id
+        }
 
         expect(assigns(:reportable)).to eq(comment)
       end
 
       it "sets report_reasons to the comment report reasons" do
         get :new, params: {
-                    request_id: info_request.url_title,
-                    comment_id: comment.id
-                  }
+          request_id: info_request.url_title,
+          comment_id: comment.id
+        }
 
         expect(assigns(:report_reasons)).to eq(comment.report_reasons)
       end
 
       it "sets the page title" do
         get :new, params: {
-                    request_id: info_request.url_title,
-                    comment_id: comment.id
-                  }
+          request_id: info_request.url_title,
+          comment_id: comment.id
+        }
 
         expect(assigns(:title)).
           to eq("Report annotation on request: #{ info_request.title }")
@@ -415,25 +415,25 @@
         new_comment = FactoryBot.create(:comment)
         expect {
           get :new, params: {
-                      request_id: info_request.url_title,
-                      comment_id: new_comment.id
-                    }
+            request_id: info_request.url_title,
+            comment_id: new_comment.id
+          }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
       it "should show the form" do
         get :new, params: {
-                    request_id: info_request.url_title,
-                    comment_id: comment.id
-                  }
+          request_id: info_request.url_title,
+          comment_id: comment.id
+        }
         expect(response).to render_template("new")
       end
 
       it "copies the comment id into a hidden form field" do
         get :new, params: {
-                    request_id: info_request.url_title,
-                    comment_id: comment.id
-                  }
+          request_id: info_request.url_title,
+          comment_id: comment.id
+        }
         expect(response.body).
           to have_selector("input#comment_id[value=\"#{comment.id}\"]",
                            visible: false)
@@ -442,9 +442,9 @@
       it "should 404 for non-existent requests" do
         expect {
           get :new, params: {
-                      request_id: "hjksfdhjk_louytu_qqxxx",
-                      comment_id: comment.id
-                    }
+            request_id: "hjksfdhjk_louytu_qqxxx",
+            comment_id: comment.id
+          }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
@@ -452,9 +452,9 @@
         info_request = FactoryBot.create(:embargoed_request)
         expect {
           get :new, params: {
-                      request_id: info_request.url_title,
-                      comment_id: comment.id
-                    }
+            request_id: info_request.url_title,
+            comment_id: comment.id
+          }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
     end

From 34fbb4c4743739a0c8ab301c9760086ac51dafbd Mon Sep 17 00:00:00 2001
From: Alexander Griffen <alexander@mysociety.org>
Date: Fri, 27 Oct 2023 13:12:54 +0100
Subject: [PATCH 09/12] URL Perms: Followups controller

Replace request IDs in URLs with titles
---
 app/controllers/followups_controller.rb       |   7 +-
 app/controllers/refusal_advice_controller.rb  |   4 +-
 app/helpers/info_request_helper.rb            |   2 +-
 app/helpers/link_to_helper.rb                 |   5 +-
 app/mailers/request_mailer.rb                 |   2 +-
 .../alaveteli_pro/activity_list/overdue.rb    |   4 +-
 .../activity_list/very_overdue.rb             |   2 +-
 .../info_requests/_after_actions.html.erb     |   6 +-
 .../followups/_choose_recipient.html.erb      |   8 +-
 app/views/followups/_followup.html.erb        |   9 +-
 app/views/request/_after_actions.html.erb     |   6 +-
 config/routes.rb                              |   8 +-
 config/routes/redirects.rb                    |   9 +-
 .../classifications_controller_spec.rb        |   8 +-
 spec/controllers/followups_controller_spec.rb | 103 +++++++++---------
 .../refusal_advice_controller_spec.rb         |   6 +-
 .../notification_mailer/daily-summary.txt     |  12 +-
 .../files/notification_mailer/overdue.txt     |   2 +-
 .../notification_mailer/very_overdue.txt      |   2 +-
 spec/helpers/info_request_helper_spec.rb      |  20 ++--
 spec/mailers/notification_mailer_spec.rb      |  10 +-
 spec/mailers/request_mailer_spec.rb           |  16 ++-
 .../activity_list/overdue_spec.rb             |   2 +-
 .../activity_list/very_overdue_spec.rb        |   2 +-
 spec/routing/redirects_spec.rb                |   5 +
 spec/views/request/show.html.erb_spec.rb      |   4 +-
 26 files changed, 148 insertions(+), 116 deletions(-)

diff --git a/app/controllers/followups_controller.rb b/app/controllers/followups_controller.rb
index 0e33ba77be..11cd2439bb 100644
--- a/app/controllers/followups_controller.rb
+++ b/app/controllers/followups_controller.rb
@@ -190,11 +190,12 @@ def set_incoming_message
 
   def set_info_request
     if current_user
-      @info_request =
-        current_user.info_requests.find_by(id: params[:request_id].to_i)
+      @info_request = current_user.info_requests.
+        find_by(url_title: params[:request_url_title])
     end
 
-    @info_request ||= InfoRequest.not_embargoed.find(params[:request_id].to_i)
+    @info_request ||= InfoRequest.not_embargoed.
+      find_by!(url_title: params[:request_url_title])
   end
 
   def set_last_request_data
diff --git a/app/controllers/refusal_advice_controller.rb b/app/controllers/refusal_advice_controller.rb
index 5ba5660c3d..a8b99c6a14 100644
--- a/app/controllers/refusal_advice_controller.rb
+++ b/app/controllers/refusal_advice_controller.rb
@@ -39,10 +39,10 @@ def internal_redirect_to
 
     case action.target[:internal]
     when 'followup'
-      redirect_to new_request_followup_path(info_request.id)
+      redirect_to new_request_followup_path(info_request.url_title)
     when 'internal_review'
       redirect_to new_request_followup_path(
-        info_request.id, internal_review: '1'
+        info_request.url_title, internal_review: '1'
       )
     when 'new_request'
       redirect_to new_request_to_body_path(
diff --git a/app/helpers/info_request_helper.rb b/app/helpers/info_request_helper.rb
index 6b6de66d50..1314ad220b 100644
--- a/app/helpers/info_request_helper.rb
+++ b/app/helpers/info_request_helper.rb
@@ -115,7 +115,7 @@ def status_text_waiting_response_very_overdue(info_request, _opts = {})
       str += link_to(
         _('requesting an internal review'),
         new_request_followup_path(
-          info_request.id, internal_review: 1
+          info_request.url_title, internal_review: 1
         )
       )
       str += '.'
diff --git a/app/helpers/link_to_helper.rb b/app/helpers/link_to_helper.rb
index 5c5b031076..4c0f860745 100755
--- a/app/helpers/link_to_helper.rb
+++ b/app/helpers/link_to_helper.rb
@@ -80,10 +80,11 @@ def new_response_url(info_request, incoming_message)
   def respond_to_last_url(info_request, options = {})
     last_response = info_request.get_last_public_response
     if last_response.nil?
-      new_request_followup_url(info_request.id, options)
+      new_request_followup_url(info_request.url_title, options)
     else
       new_request_incoming_followup_url(
-        info_request.id, options.merge(incoming_message_id: last_response.id)
+        info_request.url_title,
+        options.merge(incoming_message_id: last_response.id)
       )
     end
   end
diff --git a/app/mailers/request_mailer.rb b/app/mailers/request_mailer.rb
index b6403953a9..b464a88d61 100644
--- a/app/mailers/request_mailer.rb
+++ b/app/mailers/request_mailer.rb
@@ -163,7 +163,7 @@ def old_unclassified_updated(info_request)
   # Tell the requester that they need to clarify their request
   def not_clarified_alert(info_request, incoming_message)
     respond_url = new_request_incoming_followup_url(
-      info_request.id,
+      info_request.url_title,
       incoming_message_id: incoming_message.id,
       anchor: 'followup'
     )
diff --git a/app/models/alaveteli_pro/activity_list/overdue.rb b/app/models/alaveteli_pro/activity_list/overdue.rb
index 7c323d0d6e..c70d9220a4 100644
--- a/app/models/alaveteli_pro/activity_list/overdue.rb
+++ b/app/models/alaveteli_pro/activity_list/overdue.rb
@@ -10,7 +10,9 @@ def call_to_action
       end
 
       def call_to_action_url
-        new_request_followup_path(event.info_request.id, anchor: 'followup')
+        new_request_followup_path(
+          event.info_request.url_title, anchor: 'followup'
+        )
       end
     end
   end
diff --git a/app/models/alaveteli_pro/activity_list/very_overdue.rb b/app/models/alaveteli_pro/activity_list/very_overdue.rb
index 935907a881..a862c187fe 100644
--- a/app/models/alaveteli_pro/activity_list/very_overdue.rb
+++ b/app/models/alaveteli_pro/activity_list/very_overdue.rb
@@ -11,7 +11,7 @@ def call_to_action
 
       def call_to_action_url
         new_request_followup_path(
-          event.info_request.id,
+          event.info_request.url_title,
           anchor: 'followup',
           internal_review: 1
         )
diff --git a/app/views/alaveteli_pro/info_requests/_after_actions.html.erb b/app/views/alaveteli_pro/info_requests/_after_actions.html.erb
index b5b43a46ed..e2af845947 100644
--- a/app/views/alaveteli_pro/info_requests/_after_actions.html.erb
+++ b/app/views/alaveteli_pro/info_requests/_after_actions.html.erb
@@ -8,14 +8,14 @@
           <ul class="action-menu__menu__submenu">
             <li>
               <% if last_response.nil? %>
-                <%= link_to _("Send a followup"), new_request_followup_path(info_request.id, :anchor => 'followup') %>
+                <%= link_to _("Send a followup"), new_request_followup_path(info_request.url_title, :anchor => 'followup') %>
               <% else %>
-                <%= link_to _("Write a reply"), new_request_incoming_followup_path(info_request.id, :incoming_message_id => last_response.id, :anchor => 'followup') %>
+                <%= link_to _("Write a reply"), new_request_incoming_followup_path(info_request.url_title, :incoming_message_id => last_response.id, :anchor => 'followup') %>
               <% end %>
             </li>
 
             <li>
-              <%= link_to _("Request an internal review"), new_request_followup_path(info_request.id, :internal_review => '1', :anchor => 'followup') %>
+              <%= link_to _("Request an internal review"), new_request_followup_path(info_request.url_title, :internal_review => '1', :anchor => 'followup') %>
             </li>
 
             <li>
diff --git a/app/views/followups/_choose_recipient.html.erb b/app/views/followups/_choose_recipient.html.erb
index b8b9f78311..300157b6a9 100644
--- a/app/views/followups/_choose_recipient.html.erb
+++ b/app/views/followups/_choose_recipient.html.erb
@@ -10,7 +10,7 @@
         <li>
           <%= link_to(_('the main FOI contact address for {{public_body}}',
                         public_body: name),
-                      new_request_followup_path(@info_request.id)) %>
+                      new_request_followup_path(@info_request.url_title)) %>
         </li>
       <% else %>
         <% if id.present? %>
@@ -18,19 +18,19 @@
             <li>
               <%= link_to(_('the main FOI contact address for {{public_body}}',
                             public_body: name),
-                          new_request_followup_path(@info_request.id)) %>
+                          new_request_followup_path(@info_request.url_title)) %>
             </li>
           <% else %>
             <li>
               <%= link_to name,
-                          new_request_incoming_followup_path(@info_request.id, incoming_message_id: id) %>
+                          new_request_incoming_followup_path(@info_request.url_title, incoming_message_id: id) %>
             </li>
           <% end %>
         <% else %>
           <li>
             <%= link_to(_('the main FOI contact address for {{public_body}}',
                           public_body: name),
-                        new_request_followup_path(@info_request.id)) %>
+                        new_request_followup_path(@info_request.url_title)) %>
           </li>
         <% end %>
       <% end %>
diff --git a/app/views/followups/_followup.html.erb b/app/views/followups/_followup.html.erb
index 5a62a85a7f..47e25c9681 100644
--- a/app/views/followups/_followup.html.erb
+++ b/app/views/followups/_followup.html.erb
@@ -117,11 +117,14 @@
 
     <% form_url =
          if incoming_message.nil?
-           preview_request_followups_url(request_id: @info_request.id)
+           preview_request_followups_url(
+             request_url_title: @info_request.url_title
+           )
          else
            preview_request_followups_url(
-             request_id: @info_request.id,
-             incoming_message_id: incoming_message.id)
+             request_url_title: @info_request.url_title,
+             incoming_message_id: incoming_message.id
+           )
          end -%>
     <%= form_for @outgoing_message,
                  html: { id: 'followup_form' },
diff --git a/app/views/request/_after_actions.html.erb b/app/views/request/_after_actions.html.erb
index e596d8db41..0b187ecd7c 100644
--- a/app/views/request/_after_actions.html.erb
+++ b/app/views/request/_after_actions.html.erb
@@ -14,9 +14,9 @@
             <ul class="action-menu__menu__submenu owner_actions">
               <li>
                 <% if last_response.nil? %>
-                  <%= link_to _('Send a followup'), new_request_followup_path(info_request.id) %>
+                  <%= link_to _('Send a followup'), new_request_followup_path(info_request.url_title) %>
                 <% else %>
-                  <%= link_to _('Write a reply'), new_request_incoming_followup_path(info_request.id, incoming_message_id: last_response.id) %>
+                  <%= link_to _('Write a reply'), new_request_incoming_followup_path(info_request.url_title, incoming_message_id: last_response.id) %>
                 <% end %>
               </li>
 
@@ -27,7 +27,7 @@
               <% end %>
 
               <li>
-                <%= link_to _('Request an internal review'), new_request_followup_path(info_request.id, internal_review: '1') %>
+                <%= link_to _('Request an internal review'), new_request_followup_path(info_request.url_title, internal_review: '1') %>
               </li>
             </ul>
           </li>
diff --git a/config/routes.rb b/config/routes.rb
index 7636a67c28..9cd68aaaab 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -189,17 +189,17 @@ def matches?(request)
   end
 
   #### Followups controller
-  match '/request/:request_id/followups/new' => 'followups#new',
+  match '/request/:request_url_title/followups/new' => 'followups#new',
         :as => :new_request_followup,
         :via => :get
-  match '/request/:request_id/followups/new/:incoming_message_id' =>
+  match '/request/:request_url_title/followups/new/:incoming_message_id' =>
           'followups#new',
         :as => :new_request_incoming_followup,
         :via => :get
-  match '/request/:request_id/followups/preview' => 'followups#preview',
+  match '/request/:request_url_title/followups/preview' => 'followups#preview',
         :as => :preview_request_followups,
         :via => :post
-  match '/request/:request_id/followups' => 'followups#create',
+  match '/request/:request_url_title/followups' => 'followups#create',
         :as => :request_followups,
         :via => :post
   ####
diff --git a/config/routes/redirects.rb b/config/routes/redirects.rb
index 207e3d2044..20f7fdc0fc 100644
--- a/config/routes/redirects.rb
+++ b/config/routes/redirects.rb
@@ -6,9 +6,12 @@
   ability = Ability.new(User.authenticate_from_session(request.session))
   raise ActiveRecord::RecordNotFound if ability.cannot?(:read, info_request)
 
+  # split path components
+  suffix = params[:suffix]&.split('/') # suffix is optional
+
   # encode path components
   encoded_parts = [
-    *params[:locale], 'request', info_request.url_title
+    *params[:locale], 'request', info_request.url_title, *suffix
   ].map do |part|
     if RUBY_VERSION < '3.1'
       URI.encode_www_form_component(part).gsub('+', '%20')
@@ -24,3 +27,7 @@
 get '/request/:id',
   constraints: { id: /\d+/ },
   to: info_request_redirect
+
+get '/request/:id(/*suffix)',
+  constraints: { id: /\d+/, suffix: %r(followups/new(/\d+)?) },
+  to: info_request_redirect
diff --git a/spec/controllers/classifications_controller_spec.rb b/spec/controllers/classifications_controller_spec.rb
index 49936215c0..aea327dc94 100644
--- a/spec/controllers/classifications_controller_spec.rb
+++ b/spec/controllers/classifications_controller_spec.rb
@@ -493,7 +493,7 @@ def post_status(status, message: nil)
               post_status('waiting_clarification')
               expect(response).to redirect_to(
                 new_request_incoming_followup_path(
-                  info_request.id,
+                  info_request.url_title,
                   incoming_message_id: info_request.get_last_public_response.id
                 )
               )
@@ -512,7 +512,7 @@ def post_status(status, message: nil)
               post_status('waiting_clarification')
               expect(response).to redirect_to(
                 new_request_followup_path(
-                  info_request.id,
+                  info_request.url_title,
                   incoming_message_id: nil
                 )
               )
@@ -566,7 +566,7 @@ def post_status(status, message: nil)
             post_status('gone_postal')
             expect(response).to redirect_to(
               new_request_incoming_followup_path(
-                info_request.id,
+                info_request.url_title,
                 incoming_message_id: info_request.get_last_public_response.id,
                 gone_postal: 1
               )
@@ -668,7 +668,7 @@ def post_status(status, message: nil)
             post_status('user_withdrawn')
             expect(response).to redirect_to(
               new_request_incoming_followup_path(
-                info_request.id,
+                info_request.url_title,
                 incoming_message_id: info_request.get_last_public_response.id
               )
             )
diff --git a/spec/controllers/followups_controller_spec.rb b/spec/controllers/followups_controller_spec.rb
index ae7d2318c3..e17833eb3c 100644
--- a/spec/controllers/followups_controller_spec.rb
+++ b/spec/controllers/followups_controller_spec.rb
@@ -15,7 +15,7 @@
       it 'raises an ActiveRecord::RecordNotFound error for an embargoed request' do
         embargoed_request = FactoryBot.create(:embargoed_request)
         expect {
-          get :new, params: { request_id: embargoed_request.id }
+          get :new, params: { request_url_title: embargoed_request.url_title }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
     end
@@ -28,13 +28,13 @@
       it 'finds their own embargoed requests' do
         embargoed_request = FactoryBot.create(:embargoed_request,
                                               user: pro_user)
-        get :new, params: { request_id: embargoed_request.id }
+        get :new, params: { request_url_title: embargoed_request.url_title }
         expect(response).to be_successful
       end
 
       it "displays 'wrong user' message when not logged in as the request owner" do
         get :new, params: {
-          request_id: request.id,
+          request_url_title: request.url_title,
           incoming_message_id: message_id
         }
         expect(response).to render_template('user/wrong_user')
@@ -43,7 +43,7 @@
       it 'raises an ActiveRecord::RecordNotFound error for other embargoed requests' do
         embargoed_request = FactoryBot.create(:embargoed_request)
         expect {
-          get :new, params: { request_id: embargoed_request.id }
+          get :new, params: { request_url_title: embargoed_request.url_title }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
     end
@@ -51,7 +51,7 @@
     it "displays 'wrong user' message when not logged in as the request owner" do
       sign_in FactoryBot.create(:user)
       get :new, params: {
-        request_id: request.id,
+        request_url_title: request.url_title,
         incoming_message_id: message_id
       }
       expect(response).to render_template('user/wrong_user')
@@ -60,13 +60,13 @@
     it "does not allow follow ups to external requests" do
       sign_in FactoryBot.create(:user)
       external_request = FactoryBot.create(:external_request)
-      get :new, params: { request_id: external_request.id }
+      get :new, params: { request_url_title: external_request.url_title }
       expect(response).to render_template('followup_bad')
       expect(assigns[:reason]).to eq('external')
     end
 
     it "redirects to the signin page if not logged in" do
-      get :new, params: { request_id: request.id }
+      get :new, params: { request_url_title: request.url_title }
       expect(response).
         to redirect_to(signin_url(token: get_last_post_redirect.token))
     end
@@ -74,14 +74,14 @@
     it "calls the message a followup if there is an incoming message" do
       expected_reason = "To send a follow up message " \
         "to #{request.public_body.name}"
-      get :new, params: { request_id: request.id,
+      get :new, params: { request_url_title: request.url_title,
                           incoming_message_id: message_id }
       expect(get_last_post_redirect.reason_params[:web]).to eq(expected_reason)
     end
 
     it "calls the message a reply if there is no incoming message" do
       expected_reason = "To reply to #{request.public_body.name}."
-      get :new, params: { request_id: request.id }
+      get :new, params: { request_url_title: request.url_title }
       expect(get_last_post_redirect.reason_params[:web]).to eq(expected_reason)
     end
 
@@ -91,12 +91,12 @@
       end
 
       it "shows the followup form" do
-        get :new, params: { request_id: request.id }
+        get :new, params: { request_url_title: request.url_title }
         expect(response).to render_template('new')
       end
 
       it "shows the followup form when replying to an incoming message" do
-        get :new, params: { request_id: request.id,
+        get :new, params: { request_url_title: request.url_title,
                             incoming_message_id: message_id }
         expect(response).to render_template('new')
       end
@@ -105,7 +105,7 @@
         before { request.create_embargo!(embargo_duration: '3_months') }
 
         it 'shows the followup form' do
-          get :new, params: { request_id: request.id }
+          get :new, params: { request_url_title: request.url_title }
           expect(response).to render_template('new')
         end
       end
@@ -122,7 +122,7 @@
         end
 
         it "offers the opportunity to reply to the main address" do
-          get :new, params: { request_id: request.id,
+          get :new, params: { request_url_title: request.url_title,
                               incoming_message_id: message_id }
           expect(response.body).
             to have_css("div#other_recipients ul li",
@@ -131,9 +131,9 @@
 
         it "offers an opportunity to reply to another address" do
           get :new, params: {
-                      request_id: request.id,
-                      incoming_message_id: message_id
-                    }
+            request_url_title: request.url_title,
+            incoming_message_id: message_id
+          }
           expect(response.body).
             to have_css("div#other_recipients ul li", text: "Frob")
         end
@@ -146,7 +146,7 @@
         end
 
         it "does not show the form, even to the request owner" do
-          get :new, params: { request_id: hidden_request.id }
+          get :new, params: { request_url_title: hidden_request.url_title }
           expect(response).to render_template('request/hidden')
         end
 
@@ -154,7 +154,7 @@
           incoming_message_id = hidden_request.incoming_messages[0].id
           expect do
             get :new, params: {
-              request_id: hidden_request.id,
+              request_url_title: hidden_request.url_title,
               incoming_message_id: incoming_message_id,
               format: 'json'
             }
@@ -167,7 +167,7 @@
       it "does not allow follow ups to external requests" do
         sign_in FactoryBot.create(:user)
         external_request = FactoryBot.create(:external_request)
-        get :new, params: { request_id: external_request.id }
+        get :new, params: { request_url_title: external_request.url_title }
         expect(response).to render_template('followup_bad')
         expect(assigns[:reason]).to eq('external')
       end
@@ -175,8 +175,8 @@
       it 'the response code should be successful' do
         sign_in FactoryBot.create(:user)
         get :new, params: {
-                    request_id: FactoryBot.create(:external_request).id
-                  }
+          request_url_title: FactoryBot.create(:external_request).url_title
+        }
         expect(response).to be_successful
       end
     end
@@ -190,7 +190,7 @@
       it "sets @in_pro_area" do
         sign_in pro_user
         with_feature_enabled(:alaveteli_pro) do
-          get :new, params: { request_id: embargoed_request.id }
+          get :new, params: { request_url_title: embargoed_request.url_title }
           expect(assigns[:in_pro_area]).to eq true
         end
       end
@@ -204,7 +204,7 @@
           request, internal_review: false, user: request.user
         ).and_call_original
 
-        get :new, params: { request_id: request.id }
+        get :new, params: { request_url_title: request.url_title }
       end
 
       it 'initialise with internal_review option' do
@@ -212,11 +212,14 @@
           request, internal_review: true, user: request.user
         ).and_call_original
 
-        get :new, params: { request_id: request.id, internal_review: 1 }
+        get :new, params: {
+          request_url_title: request.url_title,
+          internal_review: 1
+        }
       end
 
       it 'assigns @refusal_advice' do
-        get :new, params: { request_id: request.id }
+        get :new, params: { request_url_title: request.url_title }
         expect(assigns[:refusal_advice]).to be_a(RefusalAdvice)
       end
     end
@@ -234,7 +237,7 @@
         expect {
           post :preview, params: {
             outgoing_message: dummy_message,
-            request_id: embargoed_request.id
+            request_url_title: embargoed_request.url_title
           }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
@@ -242,7 +245,7 @@
       it "redirects to the signin page" do
         post :preview, params: {
           outgoing_message: dummy_message,
-          request_id: request.id,
+          request_url_title: request.url_title,
           incoming_message_id: message_id
         }
         expect(response).
@@ -260,7 +263,7 @@
                                               user: pro_user)
         post :preview, params: {
           outgoing_message: dummy_message,
-          request_id: embargoed_request.id
+          request_url_title: embargoed_request.url_title
         }
         expect(response).to be_successful
       end
@@ -270,7 +273,7 @@
         expect {
           post :preview, params: {
             outgoing_message: dummy_message,
-            request_id: embargoed_request.id
+            request_url_title: embargoed_request.url_title
           }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
@@ -280,7 +283,7 @@
       sign_in FactoryBot.create(:user)
       post :preview, params: {
         outgoing_message: dummy_message,
-        request_id: request.id,
+        request_url_title: request.url_title,
         incoming_message_id: message_id
       }
       expect(response).to render_template('user/wrong_user')
@@ -293,7 +296,7 @@
 
       it "displays the edit form with an error when the message body is blank" do
         post :preview, params: {
-          request_id: request.id,
+          request_url_title: request.url_title,
           outgoing_message: {
             body: "",
             what_doing: "normal_sort"
@@ -308,7 +311,7 @@
       it "shows a preview when input is good" do
         post :preview, params: {
                          outgoing_message: dummy_message,
-                         request_id: request.id,
+                         request_url_title: request.url_title,
                          incoming_message_id: message_id,
                          preview: 1
                        }
@@ -318,7 +321,7 @@
       it "allows re-editing of a preview" do
         post :preview, params: {
                          outgoing_message: dummy_message,
-                         request_id: request.id,
+                         request_url_title: request.url_title,
                          incoming_message_id: message_id,
                          reedit: "Re-edit this request"
                        }
@@ -335,7 +338,7 @@
       it "sets @in_pro_area" do
         sign_in pro_user
         with_feature_enabled(:alaveteli_pro) do
-          get :new, params: { request_id: embargoed_request.id }
+          get :new, params: { request_url_title: embargoed_request.url_title }
           expect(assigns[:in_pro_area]).to eq true
         end
       end
@@ -356,7 +359,7 @@
       it 'sends the followup message' do
         post :create, params: {
                         outgoing_message: dummy_message,
-                        request_id: request.id,
+                        request_url_title: request.url_title,
                         incoming_message_id: message_id
                       }
 
@@ -380,7 +383,7 @@
         expect {
           post :create, params: {
                           outgoing_message: dummy_message,
-                          request_id: embargoed_request.id
+                          request_url_title: embargoed_request.url_title
                         }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
@@ -388,7 +391,7 @@
       it "redirects to the signin page" do
         post :create, params: {
                         outgoing_message: dummy_message,
-                        request_id: request.id,
+                        request_url_title: request.url_title,
                         incoming_message_id: message_id
                       }
         expect(response).
@@ -407,7 +410,7 @@
         expected_url = show_request_url(embargoed_request.url_title)
         post :create, params: {
                         outgoing_message: dummy_message,
-                        request_id: embargoed_request.id
+                        request_url_title: embargoed_request.url_title
                       }
         expect(response).to redirect_to(expected_url)
       end
@@ -417,7 +420,7 @@
         expect {
           post :create, params: {
                           outgoing_message: dummy_message,
-                          request_id: embargoed_request.id
+                          request_url_title: embargoed_request.url_title
                         }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
@@ -427,7 +430,7 @@
       sign_in FactoryBot.create(:user)
       post :create, params: {
                       outgoing_message: dummy_message,
-                      request_id: request.id,
+                      request_url_title: request.url_title,
                       incoming_message_id: message_id
                     }
       expect(response).to render_template('user/wrong_user')
@@ -436,7 +439,7 @@
     it "gives an error and renders 'show_response' when a body isn't given" do
       post :create, params: {
                       outgoing_message: dummy_message.merge(body: ''),
-                      request_id: request.id,
+                      request_url_title: request.url_title,
                       incoming_message_id: message_id
                     }
 
@@ -449,7 +452,7 @@
       def send_request
         post :create, params: {
                outgoing_message: dummy_message,
-               request_id: request.id,
+               request_url_title: request.url_title,
                incoming_message_id: message_id
              }
       end
@@ -464,7 +467,7 @@ def send_request
     it "updates the status for successful followup sends" do
       post :create, params: {
                       outgoing_message: dummy_message,
-                      request_id: request.id,
+                      request_url_title: request.url_title,
                       incoming_message_id: message_id
                     }
 
@@ -482,7 +485,7 @@ def send_request
       it 'reopens the parent request to new responses' do
         post :create, params: {
                         outgoing_message: dummy_message,
-                        request_id: request.id,
+                        request_url_title: request.url_title,
                         incoming_message_id: message_id
                       }
 
@@ -498,7 +501,7 @@ def send_request
 
       post :create, params: {
                       outgoing_message: dummy_message,
-                      request_id: request.id,
+                      request_url_title: request.url_title,
                       incoming_message_id: message_id
                     }
 
@@ -509,7 +512,7 @@ def send_request
     it "redirects to the request page" do
       post :create, params: {
                       outgoing_message: dummy_message,
-                      request_id: request.id,
+                      request_url_title: request.url_title,
                       incoming_message_id: message_id
                     }
 
@@ -520,7 +523,7 @@ def send_request
     it "displays the a confirmation once the message has been sent" do
       post :create, params: {
                       outgoing_message: dummy_message,
-                      request_id: request.id,
+                      request_url_title: request.url_title,
                       incoming_message_id: message_id
                     }
       expect(flash[:notice]).
@@ -535,7 +538,7 @@ def send_request
       post :create,
            params: {
              outgoing_message: dummy_message,
-             request_id: closed_request.id,
+             request_url_title: closed_request.url_title,
              incoming_message_id: closed_request.incoming_messages[0].id
            }
       deliveries = ActionMailer::Base.deliveries
@@ -554,13 +557,13 @@ def send_request
       before(:each) do
         post :create, params: {
                         outgoing_message: dummy_message,
-                        request_id: request.id,
+                        request_url_title: request.url_title,
                         incoming_message_id: message_id
                       }
 
         post :create, params: {
                         outgoing_message: dummy_message,
-                        request_id: request.id,
+                        request_url_title: request.url_title,
                         incoming_message_id: message_id
                       }
       end
diff --git a/spec/controllers/refusal_advice_controller_spec.rb b/spec/controllers/refusal_advice_controller_spec.rb
index 21fe5a455c..ed3fc319e2 100644
--- a/spec/controllers/refusal_advice_controller_spec.rb
+++ b/spec/controllers/refusal_advice_controller_spec.rb
@@ -38,7 +38,7 @@
         it 'redirects to new followup action' do
           post :create, params: params
           expect(response).to redirect_to(
-            new_request_followup_path(info_request.id)
+            new_request_followup_path(info_request.url_title)
           )
         end
       end
@@ -49,7 +49,9 @@
         it 'redirects to new internal review action' do
           post :create, params: params
           expect(response).to redirect_to(
-            new_request_followup_path(info_request.id, internal_review: '1')
+            new_request_followup_path(
+              info_request.url_title, internal_review: '1'
+            )
           )
         end
       end
diff --git a/spec/fixtures/files/notification_mailer/daily-summary.txt b/spec/fixtures/files/notification_mailer/daily-summary.txt
index b34f2ba69f..0ea9ab0450 100644
--- a/spec/fixtures/files/notification_mailer/daily-summary.txt
+++ b/spec/fixtures/files/notification_mailer/daily-summary.txt
@@ -41,7 +41,7 @@ Late expenses claims
 What's happened:
 Ministry of fact keeping have not replied to your FOI request Late expenses claims promptly, as normally required by law. Click on the link below to remind them to reply.
 
-http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$LATE_EXPENSES_CLAIMS_MOF_ID$%2Ffollowups%2Fnew
+http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$LATE_EXPENSES_CLAIMS_MOF_URL_TITLE$%2Ffollowups%2Fnew
 
 
 Extremely late expenses claims
@@ -50,7 +50,7 @@ Extremely late expenses claims
 What's happened:
 Ministry of fact keeping have still not replied to your FOI request Extremely late expenses claims, as normally required by law. Click on the link below to tell them to reply. You might like to ask for an internal review, asking them to find out why their response to the request has been so slow.
 
-http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$EXTREMELY_LATE_EXPENSES_CLAIMS_MOF_ID$%2Ffollowups%2Fnew
+http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$EXTREMELY_LATE_EXPENSES_CLAIMS_MOF_URL_TITLE$%2Ffollowups%2Fnew
 
 
 Misdelivered letters
@@ -114,8 +114,8 @@ What's happened:
 
 You can see the requests and remind the bodies to respond with the following links:
 
-  Ministry of fact keeping: http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$LATE_FOI_REQUESTS_MOF_ID$%2Ffollowups%2Fnew
-  Minor infractions quango: http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$LATE_FOI_REQUESTS_2_MIQ_ID$%2Ffollowups%2Fnew
+  Ministry of fact keeping: http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$LATE_FOI_REQUESTS_MOF_URL_TITLE$%2Ffollowups%2Fnew
+  Minor infractions quango: http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$LATE_FOI_REQUESTS_2_MIQ_URL_TITLE$%2Ffollowups%2Fnew
 
 
 Ignored FOI requests
@@ -130,8 +130,8 @@ What's happened:
 
 You can see the requests and tell the bodies to respond with the following links. You might like to ask for internal reviews, asking the bodies to find out why responses to the requests have been so slow.
 
-  Ministry of fact keeping: http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$IGNORED_FOI_REQUESTS_MOF_ID$%2Ffollowups%2Fnew
-  Minor infractions quango: http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$IGNORED_FOI_REQUESTS_2_MIQ_ID$%2Ffollowups%2Fnew
+  Ministry of fact keeping: http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$IGNORED_FOI_REQUESTS_MOF_URL_TITLE$%2Ffollowups%2Fnew
+  Minor infractions quango: http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$IGNORED_FOI_REQUESTS_2_MIQ_URL_TITLE$%2Ffollowups%2Fnew
 
 
 
diff --git a/spec/fixtures/files/notification_mailer/overdue.txt b/spec/fixtures/files/notification_mailer/overdue.txt
index 2ad08008cb..d849554ab1 100644
--- a/spec/fixtures/files/notification_mailer/overdue.txt
+++ b/spec/fixtures/files/notification_mailer/overdue.txt
@@ -4,7 +4,7 @@ They have not replied to your FOI request Here is a character that needs quoting
 
 Click on the link below to send a message to Test public body reminding them to reply to your request.
 
-http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2FINFO_REQUEST_ID%2Ffollowups%2Fnew
+http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2FINFO_REQUEST_URL_TITLE%2Ffollowups%2Fnew
 
 
 -- the Alaveteli team
diff --git a/spec/fixtures/files/notification_mailer/very_overdue.txt b/spec/fixtures/files/notification_mailer/very_overdue.txt
index 43a11522ed..2df84ef4b8 100644
--- a/spec/fixtures/files/notification_mailer/very_overdue.txt
+++ b/spec/fixtures/files/notification_mailer/very_overdue.txt
@@ -4,6 +4,6 @@ They have still not replied to your FOI request Here is a character that needs q
 
 Click on the link below to send a message to Test public body telling them to reply to your request. You might like to ask for an internal review, asking them to find out why their response to the request has been so slow.
 
-http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2FINFO_REQUEST_ID%2Ffollowups%2Fnew
+http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2FINFO_REQUEST_URL_TITLE%2Ffollowups%2Fnew
 
 -- the Alaveteli team
diff --git a/spec/helpers/info_request_helper_spec.rb b/spec/helpers/info_request_helper_spec.rb
index 57b3c88d1e..12931c3a9c 100644
--- a/spec/helpers/info_request_helper_spec.rb
+++ b/spec/helpers/info_request_helper_spec.rb
@@ -158,14 +158,15 @@
                         'title="2014-12-31 00:00:00 UTC">' \
                         'December 31, 2014</time>'
 
+        path = new_request_followup_path(
+          info_request.url_title, internal_review: 1
+        )
         expected = "Response to this request is <strong>long overdue" \
                    "</strong>. By law, under all circumstances, " \
                    "#{ body_link } should have responded by now " \
                    "(<a href=\"/help/requesting#quickly_response\">details" \
                    "</a>). You can <strong>complain</strong> by " \
-                   "<a href=\"/request/#{info_request.id}/followups/new?" \
-                   "internal_review=1\">requesting an internal " \
-                   "review</a>."
+                   "<a href=\"#{path}\">requesting an internal review</a>."
 
         expect(status_text(info_request)).to eq(expected)
 
@@ -189,15 +190,16 @@
                           'title="2014-12-31 00:00:00 UTC">' \
                           'December 31, 2014</time>'
 
+          path = new_request_followup_path(
+            info_request.url_title, internal_review: 1
+          )
           expected = "Response to this request is <strong>long overdue" \
                      "</strong>. " \
                      "Although not legally required to do so, we would have " \
                      "expected #{ body_link } to have responded by now " \
                      "(<a href=\"/help/requesting#authorities\">details" \
                      "</a>). You can <strong>complain</strong> by " \
-                     "<a href=\"/request/#{info_request.id}/followups/new?" \
-                     "internal_review=1\">requesting an internal " \
-                     "review</a>."
+                     "<a href=\"#{path}\">requesting an internal review</a>."
 
           expect(status_text(info_request)).to eq(expected)
 
@@ -293,10 +295,10 @@
           to receive(:get_last_public_response).
           and_return(nil)
 
+        path = new_request_followup_path(info_request.url_title)
         expected = "#{ body.name } is <strong>waiting for your clarification" \
-                   "</strong>. Please " \
-                   "<a href=\"/request/#{info_request.id}/followups/new\">" \
-                   "send a follow up message</a>."
+                   "</strong>. Please <a href=\"#{path}\">send a follow up " \
+                   "message</a>."
 
         actual = status_text(info_request, is_owning_user: true)
 
diff --git a/spec/mailers/notification_mailer_spec.rb b/spec/mailers/notification_mailer_spec.rb
index 72c68d5418..a57b34a797 100644
--- a/spec/mailers/notification_mailer_spec.rb
+++ b/spec/mailers/notification_mailer_spec.rb
@@ -351,7 +351,7 @@
     # HACK: We can't control the IDs of the requests or incoming messages create
     # a data structure of mappings here so that we can replace keys in fixture
     # files with the ID that will end up in the URL.
-    let(:id_mappings) do
+    let(:url_id_mappings) do
       all_notifications.each_with_object({}) do |notification, data|
         event = notification.info_request_event
         case event.event_type
@@ -362,7 +362,7 @@
         else
           request = event.info_request
           key = "#{request.url_title}_#{request.public_body.url_name}".upcase
-          data["#{key}_ID"] = request.id
+          data["#{key}_URL_TITLE"] = request.url_title
         end
       end
     end
@@ -387,7 +387,7 @@
       mail = NotificationMailer.daily_summary(user, all_notifications)
       expected_message = load_file_fixture(
         "notification_mailer/daily-summary.txt", 'r:utf-8')
-      id_mappings.each do |key, id|
+      url_id_mappings.each do |key, id|
         expected_message.gsub!("$#{key}$", id.to_s)
       end
       expect(mail.body.encoded).to eq(expected_message)
@@ -719,7 +719,7 @@
       mail = NotificationMailer.overdue_notification(notification)
       expected_message = load_file_fixture(
         "notification_mailer/overdue.txt", 'r:utf-8')
-      expected_message.gsub!(/INFO_REQUEST_ID/, info_request.id.to_s)
+      expected_message.gsub!(/INFO_REQUEST_URL_TITLE/, info_request.url_title)
       expect(mail.body.encoded).to eq(expected_message)
     end
   end
@@ -790,7 +790,7 @@
       mail = NotificationMailer.very_overdue_notification(notification)
       expected_message = load_file_fixture(
         "notification_mailer/very_overdue.txt", 'r:utf-8')
-      expected_message.gsub!(/INFO_REQUEST_ID/, info_request.id.to_s)
+      expected_message.gsub!(/INFO_REQUEST_URL_TITLE/, info_request.url_title)
       expect(mail.body.encoded).to eq(expected_message)
     end
   end
diff --git a/spec/mailers/request_mailer_spec.rb b/spec/mailers/request_mailer_spec.rb
index 4ee111cfa7..653db16fe0 100644
--- a/spec/mailers/request_mailer_spec.rb
+++ b/spec/mailers/request_mailer_spec.rb
@@ -746,8 +746,11 @@ def kitten_mails
         mail.body.to_s =~ /(http:\/\/.*)/
         mail_url = $1
 
-        expect(mail_url).
-          to match(new_request_followup_path(@kitten_request.id))
+        expect(mail_url).to match(
+          new_request_followup_path(
+            request_url_title: @kitten_request.url_title
+          )
+        )
       end
     end
 
@@ -856,8 +859,11 @@ def kitten_mails
           mail.body.to_s =~ /(http:\/\/.*)/
           mail_url = $1
 
-          expect(mail_url).
-            to match(new_request_followup_path(@kitten_request.id))
+          expect(mail_url).to match(
+            new_request_followup_path(
+              request_url_title: @kitten_request.url_title
+            )
+          )
         end
       end
 
@@ -938,7 +944,7 @@ def force_updated_at_to_past(request)
 
       expect(mail_url).to match(
         new_request_incoming_followup_path(
-          ir.id,
+          ir.url_title,
           incoming_message_id: ir.incoming_messages.last.id
         )
       )
diff --git a/spec/models/alaveteli_pro/activity_list/overdue_spec.rb b/spec/models/alaveteli_pro/activity_list/overdue_spec.rb
index 68cd201bba..ced9bcf270 100644
--- a/spec/models/alaveteli_pro/activity_list/overdue_spec.rb
+++ b/spec/models/alaveteli_pro/activity_list/overdue_spec.rb
@@ -26,7 +26,7 @@
     it 'returns the url of the info_request' do
       expect(activity.call_to_action_url).to eq(
         new_request_followup_path(
-          event.info_request.id, anchor: 'followup'
+          event.info_request.url_title, anchor: 'followup'
         )
       )
     end
diff --git a/spec/models/alaveteli_pro/activity_list/very_overdue_spec.rb b/spec/models/alaveteli_pro/activity_list/very_overdue_spec.rb
index 14783ec007..8b5cebd159 100644
--- a/spec/models/alaveteli_pro/activity_list/very_overdue_spec.rb
+++ b/spec/models/alaveteli_pro/activity_list/very_overdue_spec.rb
@@ -26,7 +26,7 @@
     it 'returns the url of the info_request' do
       expect(activity.call_to_action_url).to eq(
         new_request_followup_path(
-          event.info_request.id,
+          event.info_request.url_title,
           anchor: 'followup',
           internal_review: 1
         )
diff --git a/spec/routing/redirects_spec.rb b/spec/routing/redirects_spec.rb
index a784a575dc..f4d94a31ec 100644
--- a/spec/routing/redirects_spec.rb
+++ b/spec/routing/redirects_spec.rb
@@ -13,4 +13,9 @@
     get('/en_GB/request/105')
     expect(response).to redirect_to('/en_GB/request/the_cost_of_boring')
   end
+
+  it 'routes numerical request member routes to URL title member routes' do
+    get('/request/105/followups/new')
+    expect(response).to redirect_to('/request/the_cost_of_boring/followups/new')
+  end
 end
diff --git a/spec/views/request/show.html.erb_spec.rb b/spec/views/request/show.html.erb_spec.rb
index 93556e1ead..1507e3a4b0 100644
--- a/spec/views/request/show.html.erb_spec.rb
+++ b/spec/views/request/show.html.erb_spec.rb
@@ -88,7 +88,7 @@ def request_page
             and_return(mock_response)
           request_page
           expected_url = new_request_incoming_followup_path(
-            mock_request.id,
+            mock_request.url_title,
             incoming_message_id: mock_response.id
           )
           expect(response.body).
@@ -107,7 +107,7 @@ def request_page
 
         it "should show a link to follow up the request without reference to a specific response" do
           request_page
-          expected_url = new_request_followup_path(mock_request.id)
+          expected_url = new_request_followup_path(mock_request.url_title)
           expect(response.body).
             to have_css(
               "a[href='#{expected_url}']",

From e0cd4afc814efc5e0d88c7a7c417754a321c1759 Mon Sep 17 00:00:00 2001
From: Alexander Griffen <alexander@mysociety.org>
Date: Fri, 27 Oct 2023 17:03:38 +0100
Subject: [PATCH 10/12] URL Perms: Attachments controller

Replace request IDs in URLs with titles

This does remove a few tests that were looking specifically at
IDs, or 'ugly' IDs)
---
 app/controllers/attachments_controller.rb     |  2 +-
 app/helpers/info_request_helper.rb            |  3 +-
 config/routes.rb                              |  7 +--
 config/routes/redirects.rb                    |  5 ++
 .../attachments_controller_spec.rb            | 52 +++----------------
 spec/helpers/info_request_helper_spec.rb      |  8 +--
 spec/integration/errors_spec.rb               |  5 +-
 spec/integration/incoming_mail_spec.rb        | 12 ++---
 .../viewing_attachment_as_html_spec.rb        |  2 +-
 .../prominence/viewing_raw_attachment_spec.rb |  2 +-
 spec/integration/view_request_spec.rb         |  2 +-
 spec/routing/redirects_spec.rb                | 12 +++++
 12 files changed, 48 insertions(+), 64 deletions(-)

diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb
index 1c1fdedfac..0c02a76e80 100644
--- a/app/controllers/attachments_controller.rb
+++ b/app/controllers/attachments_controller.rb
@@ -58,7 +58,7 @@ def find_info_request
       if public_token?
         InfoRequest.find_by!(public_token: public_token)
       else
-        InfoRequest.find(params[:id])
+        InfoRequest.find_by!(url_title: params[:request_url_title])
       end
   end
 
diff --git a/app/helpers/info_request_helper.rb b/app/helpers/info_request_helper.rb
index 1314ad220b..a39c74a113 100644
--- a/app/helpers/info_request_helper.rb
+++ b/app/helpers/info_request_helper.rb
@@ -315,7 +315,8 @@ def attachment_params(attachment, options = {})
     if public_token?
       attach_params[:public_token] = public_token
     else
-      attach_params[:id] = attachment.incoming_message.info_request_id
+      attach_params[:request_url_title] = attachment.incoming_message.
+        info_request.url_title
     end
     if options[:html]
       attach_params[:file_name] = "#{attachment.display_filename}.html"
diff --git a/config/routes.rb b/config/routes.rb
index 9cd68aaaab..442fc4357b 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -127,13 +127,14 @@ def matches?(request)
         :as => :similar_request,
         :via => :get
 
-  match '/request/:id/response/:incoming_message_id/attach/html' \
-        '/(:part(/*file_name))' => 'attachments#show_as_html',
+  match '/request/:request_url_title/response/:incoming_message_id/attach' \
+        '/html/(:part(/*file_name))' => 'attachments#show_as_html',
         :format => false,
         :as => :get_attachment_as_html,
         :via => :get,
         :constraints => { :part => /\d+/ }
-  match '/request/:id/response/:incoming_message_id/attach/:part(/*file_name)' => 'attachments#show',
+  match '/request/:request_url_title/response/:incoming_message_id/attach' \
+        '/:part(/*file_name)' => 'attachments#show',
         :format => false,
         :as => :get_attachment,
         :via => :get,
diff --git a/config/routes/redirects.rb b/config/routes/redirects.rb
index 20f7fdc0fc..cd004ca778 100644
--- a/config/routes/redirects.rb
+++ b/config/routes/redirects.rb
@@ -31,3 +31,8 @@
 get '/request/:id(/*suffix)',
   constraints: { id: /\d+/, suffix: %r(followups/new(/\d+)?) },
   to: info_request_redirect
+
+get '/request/:id(/*suffix)',
+  format: false,
+  constraints: { id: /\d+/, suffix: %r(response/\d+/attach(/html)?/\d+/.*) },
+  to: info_request_redirect
diff --git a/spec/controllers/attachments_controller_spec.rb b/spec/controllers/attachments_controller_spec.rb
index f9f1d128eb..cf2b870806 100644
--- a/spec/controllers/attachments_controller_spec.rb
+++ b/spec/controllers/attachments_controller_spec.rb
@@ -43,39 +43,13 @@ def show(params = {})
         part: attachment.url_part_number,
         file_name: attachment.display_filename
       }
-      default_params[:id] = info_request.id unless params[:public_token]
+      unless params[:public_token]
+        default_params[:request_url_title] = info_request.url_title
+      end
       rebuild_raw_emails(info_request)
       get :show, params: default_params.merge(params)
     end
 
-    # This is a regression test for a bug where URLs of this form were causing
-    # 500 errors instead of 404s.
-    #
-    # (Note that in fact only the integer-prefix of the URL part is used, so
-    # there are *some* “ugly URLs containing a request id that isn\'t an
-    # integer” that actually return a 200 response. The point is that IDs of
-    # this sort were triggering an error in the error-handling path, causing
-    # the wrong sort of error response to be returned in the case where the
-    # integer prefix referred to the wrong request.)
-    #
-    # https://github.com/mysociety/alaveteli/issues/351
-    it 'should return 404 for ugly URLs containing a request id that isn\'t an integer' do
-      ugly_id = '55195'
-      expect { show(id: ugly_id) }
-        .to raise_error(ActiveRecord::RecordNotFound)
-    end
-
-    it 'should return 404 when incoming message and request ids don\'t match' do
-      expect { show(id: info_request.id + 1) }
-        .to raise_error(ActiveRecord::RecordNotFound)
-    end
-
-    it 'should return 404 for ugly URLs contain a request id that isn\'t an integer, even if the integer prefix refers to an actual request' do
-      ugly_id = '#{FactoryBot.create(:info_request).id}95'
-      expect { show(id: ugly_id) }
-        .to raise_error(ActiveRecord::RecordNotFound)
-    end
-
     it 'should redirect to the incoming message if there\'s a wrong part number and an ambiguous filename' do
       show(
         part: attachment.url_part_number + 1,
@@ -167,7 +141,7 @@ def show(params = {})
           allow(controller).to receive(:verifier).and_return(verifier)
           allow(verifier).to receive(:generate).with(
             get_attachment_path(
-              info_request.id,
+              info_request.url_title,
               incoming_message_id: attachment.incoming_message_id,
               part: attachment.url_part_number,
               file_name: attachment.filename
@@ -460,7 +434,9 @@ def show_as_html(params = {})
         part: attachment.url_part_number,
         file_name: attachment.display_filename
       }
-      default_params[:id] = info_request.id unless params[:public_token]
+      unless params[:public_token]
+        default_params[:request_url_title] = info_request.url_title
+      end
       get :show_as_html, params: default_params.merge(params)
     end
 
@@ -482,18 +458,6 @@ def show_as_html(params = {})
       expect(response.headers['X-Robots-Tag']).to eq 'noindex'
     end
 
-    it 'should return 404 for ugly URLs containing a request id that isn\'t an integer' do
-      ugly_id = '55195'
-      expect { show_as_html(id: ugly_id) }
-        .to raise_error(ActiveRecord::RecordNotFound)
-    end
-
-    it 'should return 404 for ugly URLs contain a request id that isn\'t an integer, even if the integer prefix refers to an actual request' do
-      ugly_id = FactoryBot.create(:info_request).id.to_s + '95'
-      expect { show_as_html(id: ugly_id) }
-        .to raise_error(ActiveRecord::RecordNotFound)
-    end
-
     context 'when attachment has not been masked' do
       let(:attachment) do
         FactoryBot.create(
@@ -542,7 +506,7 @@ def show_as_html(params = {})
           allow(controller).to receive(:verifier).and_return(verifier)
           allow(verifier).to receive(:generate).with(
             get_attachment_as_html_path(
-              info_request.id,
+              info_request.url_title,
               incoming_message_id: attachment.incoming_message_id,
               part: attachment.url_part_number,
               file_name: attachment.filename
diff --git a/spec/helpers/info_request_helper_spec.rb b/spec/helpers/info_request_helper_spec.rb
index 12931c3a9c..65e868f90a 100644
--- a/spec/helpers/info_request_helper_spec.rb
+++ b/spec/helpers/info_request_helper_spec.rb
@@ -644,7 +644,7 @@
     context 'when given no format options' do
       it 'returns the path to the attachment with a cookie cookie_passthrough param' do
         expect(attachment_path(jpeg_attachment)).to eq(
-          "/request/#{incoming_message.info_request_id}" \
+          "/request/#{incoming_message.info_request.url_title}" \
           "/response/#{incoming_message.id}/" \
           "attach/#{jpeg_attachment.url_part_number}" \
           "/interesting.jpg?cookie_passthrough=1"
@@ -655,7 +655,7 @@
     context 'when given an html format option' do
       it 'returns the path to the HTML version of the attachment' do
         expect(attachment_path(jpeg_attachment, html: true)).to eq(
-          "/request/#{incoming_message.info_request_id}" \
+          "/request/#{incoming_message.info_request.url_title}" \
           "/response/#{incoming_message.id}" \
           "/attach/html/#{jpeg_attachment.url_part_number}" \
           "/interesting.jpg.html"
@@ -686,7 +686,7 @@
       it 'returns the URL to the attachment with a cookie cookie_passthrough param' do
         expect(attachment_url(jpeg_attachment)).to eq(
           "http://test.host" \
-          "/request/#{incoming_message.info_request_id}" \
+          "/request/#{incoming_message.info_request.url_title}" \
           "/response/#{incoming_message.id}" \
           "/attach/#{jpeg_attachment.url_part_number}" \
           "/interesting.jpg?cookie_passthrough=1"
@@ -698,7 +698,7 @@
       it 'returns the URL to the HTML version of the attachment' do
         expect(attachment_url(jpeg_attachment, html: true)).to eq(
           "http://test.host" \
-          "/request/#{incoming_message.info_request_id}" \
+          "/request/#{incoming_message.info_request.url_title}" \
           "/response/#{incoming_message.id}" \
           "/attach/html/#{jpeg_attachment.url_part_number}" \
           "/interesting.jpg.html"
diff --git a/spec/integration/errors_spec.rb b/spec/integration/errors_spec.rb
index 228bd9298f..7254826b68 100644
--- a/spec/integration/errors_spec.rb
+++ b/spec/integration/errors_spec.rb
@@ -90,6 +90,7 @@
     it 'should render a 403 with text body for attempts at directory listing for attachments' do
       info_request = FactoryBot.create(:info_request_with_pdf_attachment)
       id = info_request.id
+      url_title = info_request.url_title
       prefix = id.to_s[0..2]
       msg_id = info_request.incoming_messages.first.id
 
@@ -99,11 +100,11 @@
       )
       FileUtils.mkdir_p(cache_key_path)
 
-      get "/request/#{id}/response/#{msg_id}/attach/html/1/"
+      get "/request/#{url_title}/response/#{msg_id}/attach/html/1/"
       expect(response.body).to include('Directory listing not allowed')
       expect(response.code).to eq('403')
 
-      get "/request/#{id}/response/#{msg_id}/attach/html"
+      get "/request/#{url_title}/response/#{msg_id}/attach/html"
       expect(response.body).to include('Directory listing not allowed')
       expect(response.code).to eq('403')
     end
diff --git a/spec/integration/incoming_mail_spec.rb b/spec/integration/incoming_mail_spec.rb
index 98844c2890..40451ba816 100644
--- a/spec/integration/incoming_mail_spec.rb
+++ b/spec/integration/incoming_mail_spec.rb
@@ -23,14 +23,14 @@
                           email_to: info_request.incoming_email)
 
     attachment_1_path = get_attachment_path(
-      info_request.id,
+      info_request.url_title,
       incoming_message_id: info_request.incoming_messages.first.id,
       part: 2,
       file_name: 'hello world.txt',
       skip_cache: 1
     )
     attachment_2_path = get_attachment_path(
-      info_request.id,
+      info_request.url_title,
       incoming_message_id: info_request.incoming_messages.first.id,
       part: 3,
       file_name: 'hello world.txt',
@@ -65,7 +65,7 @@
     receive_incoming_mail('incoming-request-two-same-name.email',
                           email_to: info_request.incoming_email)
     attachment_path = get_attachment_as_html_path(
-      info_request.id,
+      info_request.url_title,
       incoming_message_id: info_request.incoming_messages.first.id,
       part: 2,
       file_name: 'hello world.txt.html',
@@ -83,7 +83,7 @@
     receive_incoming_mail('incoming-request-pdf-attachment.email',
                           email_to: info_request.incoming_email)
     attachment_path = get_attachment_as_html_path(
-      info_request.id,
+      info_request.url_title,
       incoming_message_id: info_request.incoming_messages.first.id,
       part: 2,
       file_name: 'fs 50379341.pdf.html',
@@ -103,7 +103,7 @@
     # asking for an attachment by the wrong filename should result in
     # redirecting back to the incoming message
     visit get_attachment_as_html_path(
-      info_request.id,
+      info_request.url_title,
       incoming_message_id: info_request.incoming_messages.first.id,
       part: 2,
       file_name: 'hello world.txt.baz.html',
@@ -118,7 +118,7 @@
                           email_to: info_request.incoming_email)
 
     attachment_path = get_attachment_path(
-      info_request.id,
+      info_request.url_title,
       incoming_message_id: info_request.incoming_messages.first.id,
       part: 2,
       file_name: 'hello.qwglhm',
diff --git a/spec/integration/prominence/viewing_attachment_as_html_spec.rb b/spec/integration/prominence/viewing_attachment_as_html_spec.rb
index beeb7d3604..3f7e01b14e 100644
--- a/spec/integration/prominence/viewing_attachment_as_html_spec.rb
+++ b/spec/integration/prominence/viewing_attachment_as_html_spec.rb
@@ -8,7 +8,7 @@
   let(:within_session) do
     -> {
       visit get_attachment_as_html_url(
-        info_request.id,
+        info_request.url_title,
         incoming_message_id: attachment.incoming_message_id,
         part: attachment.url_part_number,
         file_name: "#{attachment.display_filename}.html"
diff --git a/spec/integration/prominence/viewing_raw_attachment_spec.rb b/spec/integration/prominence/viewing_raw_attachment_spec.rb
index 7d49a6cd0f..2e1ec5922a 100644
--- a/spec/integration/prominence/viewing_raw_attachment_spec.rb
+++ b/spec/integration/prominence/viewing_raw_attachment_spec.rb
@@ -10,7 +10,7 @@
       rebuild_raw_emails(info_request)
 
       visit get_attachment_url(
-        info_request.id,
+        info_request.url_title,
         incoming_message_id: attachment.incoming_message_id,
         part: attachment.url_part_number,
         file_name: attachment.display_filename
diff --git a/spec/integration/view_request_spec.rb b/spec/integration/view_request_spec.rb
index 20424be918..0d35264dfe 100644
--- a/spec/integration/view_request_spec.rb
+++ b/spec/integration/view_request_spec.rb
@@ -51,7 +51,7 @@
       )
       rebuild_raw_emails(info_request)
 
-      attachment_url = "/es/request/#{info_request.id}/response/" \
+      attachment_url = "/es/request/#{info_request.url_title}/response/" \
         "#{incoming_message.id}/attach/#{attachment.url_part_number}/" \
         "#{attachment.filename}"
       using_session(non_owner) { visit(attachment_url) }
diff --git a/spec/routing/redirects_spec.rb b/spec/routing/redirects_spec.rb
index f4d94a31ec..6a3cf109ee 100644
--- a/spec/routing/redirects_spec.rb
+++ b/spec/routing/redirects_spec.rb
@@ -18,4 +18,16 @@
     get('/request/105/followups/new')
     expect(response).to redirect_to('/request/the_cost_of_boring/followups/new')
   end
+
+  it 'routes numerical request attachment routes to URL title attachment routes' do
+    get('/request/105/response/1/attach/2/filename.txt')
+    expect(response).to redirect_to(
+      '/request/the_cost_of_boring/response/1/attach/2/filename.txt'
+    )
+
+    get('/request/105/response/1/attach/html/2/filename.txt.html')
+    expect(response).to redirect_to(
+      '/request/the_cost_of_boring/response/1/attach/html/2/filename.txt.html'
+    )
+  end
 end

From f23496da2941f14092ea64a4bd49cde642a9186b Mon Sep 17 00:00:00 2001
From: Alexander Griffen <alexander@mysociety.org>
Date: Mon, 20 Nov 2023 13:30:35 +0000
Subject: [PATCH 11/12] URL Perms: Reports, widgets controllers

Replace request IDs in URLs with titles
---
 app/controllers/reports_controller.rb         |  2 +-
 app/controllers/widget_votes_controller.rb    |  3 +-
 app/controllers/widgets_controller.rb         |  2 +-
 app/views/request/_act.html.erb               |  2 +-
 app/views/widgets/new.html.erb                |  4 +-
 app/views/widgets/show.html.erb               |  2 +-
 config/routes.rb                              |  2 +-
 config/routes/redirects.rb                    |  8 ++
 spec/controllers/reports_controller_spec.rb   | 84 +++++++++----------
 .../widget_votes_controller_spec.rb           | 20 +++--
 spec/controllers/widgets_controller_spec.rb   | 48 +++++------
 spec/routing/redirects_spec.rb                |  9 ++
 12 files changed, 103 insertions(+), 83 deletions(-)

diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb
index 5daecd8ed2..30e590bf5b 100644
--- a/app/controllers/reports_controller.rb
+++ b/app/controllers/reports_controller.rb
@@ -50,7 +50,7 @@ def new
 
   def set_info_request
     @info_request = InfoRequest.not_embargoed.
-      find_by_url_title!(params[:request_id])
+      find_by_url_title!(params[:request_url_title])
   end
 
   def set_comment
diff --git a/app/controllers/widget_votes_controller.rb b/app/controllers/widget_votes_controller.rb
index d5338ddc0c..7e097b4df0 100644
--- a/app/controllers/widget_votes_controller.rb
+++ b/app/controllers/widget_votes_controller.rb
@@ -37,7 +37,8 @@ def check_widget_config
   end
 
   def find_info_request
-    @info_request = InfoRequest.not_embargoed.find(params[:request_id])
+    @info_request = InfoRequest.not_embargoed.
+      find_by!(url_title: params[:request_url_title])
   end
 
   def check_prominence
diff --git a/app/controllers/widgets_controller.rb b/app/controllers/widgets_controller.rb
index 4484734993..1a57827989 100644
--- a/app/controllers/widgets_controller.rb
+++ b/app/controllers/widgets_controller.rb
@@ -42,7 +42,7 @@ def check_widget_config
   end
 
   def find_info_request
-    @info_request = InfoRequest.find(params[:request_id])
+    @info_request = InfoRequest.find_by!(url_title: params[:request_url_title])
   end
 
   def check_prominence
diff --git a/app/views/request/_act.html.erb b/app/views/request/_act.html.erb
index a055c6148a..844306e5f1 100644
--- a/app/views/request/_act.html.erb
+++ b/app/views/request/_act.html.erb
@@ -44,7 +44,7 @@
 <% if AlaveteliConfiguration::enable_widgets %>
   <div class="act_link">
     <i class="act-link-icon act-link-icon--widget"></i>
-    <%= link_to _("Create a widget for this request"), new_request_widget_path(info_request) %>
+    <%= link_to _("Create a widget for this request"), new_request_widget_path(info_request.url_title) %>
   </div>
 <% end %>
 
diff --git a/app/views/widgets/new.html.erb b/app/views/widgets/new.html.erb
index 0d24d2ff66..6e13b67972 100644
--- a/app/views/widgets/new.html.erb
+++ b/app/views/widgets/new.html.erb
@@ -4,12 +4,12 @@
   <%= _("To add a widget for <b>{{info_request_title}}</b>, copy and paste " \
         "the following code to your web page:",
         :info_request_title => @info_request.title) %>
-  <textarea autofocus readonly rows='4' cols='60' id='widgetbox'><iframe src='<%= request_widget_url(@info_request) %>' width='320' height='215' frameborder='0' marginwidth='0' marginheight='0'></iframe></textarea>
+  <textarea autofocus readonly rows='4' cols='60' id='widgetbox'><iframe src='<%= request_widget_url(@info_request.url_title) %>' width='320' height='215' frameborder='0' marginwidth='0' marginheight='0'></iframe></textarea>
 
 </p>
 
 <p>
   <%= _("The widget will look like this:") %>
   <br>
-  <iframe src='<%= request_widget_url(@info_request) %>' width='320' height='215' frameborder='0' marginwidth='0' marginheight='0'></iframe>
+  <iframe src='<%= request_widget_url(@info_request.url_title) %>' width='320' height='215' frameborder='0' marginwidth='0' marginheight='0'></iframe>
 </p>
diff --git a/app/views/widgets/show.html.erb b/app/views/widgets/show.html.erb
index d0a1207720..2786b6f69d 100644
--- a/app/views/widgets/show.html.erb
+++ b/app/views/widgets/show.html.erb
@@ -57,7 +57,7 @@
           <%= _('I also want to know!') %>
         <% end %>
       <% else %>
-        <%= form_tag request_widget_votes_url(@info_request),
+        <%= form_tag request_widget_votes_url(request_url_title: @info_request.url_title),
               :method => 'post', :target => '_top' do %>
           <%= submit_tag _('I also want to know!'),
                 :class => 'alaveteli-widget__bottom__action alaveteli-widget__button--create-vote',
diff --git a/config/routes.rb b/config/routes.rb
index 442fc4357b..3bc4c9d9a9 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -234,7 +234,7 @@ def matches?(request)
   end
   get '/health_checks' => redirect('/health/checks')
 
-  resources :request, :only => [] do
+  resources :request, :only => [], param: :url_title do
     resource :report, :only => [:new, :create]
     resource :widget, :only => [:new, :show]
     resources :widget_votes, :only => [:create]
diff --git a/config/routes/redirects.rb b/config/routes/redirects.rb
index cd004ca778..4c6f977ce7 100644
--- a/config/routes/redirects.rb
+++ b/config/routes/redirects.rb
@@ -36,3 +36,11 @@
   format: false,
   constraints: { id: /\d+/, suffix: %r(response/\d+/attach(/html)?/\d+/.*) },
   to: info_request_redirect
+
+get '/request/:id(/*suffix)',
+  constraints: { id: /\d+/, suffix: %r(report/new) },
+  to: info_request_redirect
+
+get '/request/:id(/*suffix)',
+  constraints: { id: /\d+/, suffix: %r(widget(/new)?) },
+  to: info_request_redirect
diff --git a/spec/controllers/reports_controller_spec.rb b/spec/controllers/reports_controller_spec.rb
index 893eb786e8..e4bee67f6b 100644
--- a/spec/controllers/reports_controller_spec.rb
+++ b/spec/controllers/reports_controller_spec.rb
@@ -8,7 +8,7 @@
     context "when reporting a request when not logged in" do
       it "should only allow logged-in users to report requests" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           reason: "my reason"
         }
         expect(flash[:notice]).to match(/You need to be logged in/)
@@ -24,7 +24,7 @@
 
       it "finds the expected request" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           reason: "my reason"
         }
 
@@ -33,7 +33,7 @@
 
       it "sets reportable to the request" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           reason: "my reason"
         }
 
@@ -42,7 +42,7 @@
 
       it "sets report_reasons to the request report reasons" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           reason: "my reason"
         }
 
@@ -51,7 +51,7 @@
 
       it 'logs an event' do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           reason: 'my reason'
         }
         last_event = info_request.reload.last_event
@@ -68,7 +68,7 @@
 
       it 'ignores an empty comment_id param' do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: '',
           reason: "my reason"
         }
@@ -77,14 +77,14 @@
 
       it "should 404 for non-existent requests" do
         expect {
-          post :create, params: { request_id: "hjksfdhjk_louytu_qqxxx" }
+          post :create, params: { request_url_title: "hjksfdhjk_louytu_qqxxx" }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
       it 'should 404 for embargoed requests' do
         info_request = FactoryBot.create(:embargoed_request)
         expect {
-          post :create, params: { request_id: info_request.url_title }
+          post :create, params: { request_url_title: info_request.url_title }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
@@ -92,7 +92,7 @@
         expect(info_request.attention_requested).to eq(false)
 
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           reason: "my reason"
         }
         expect(response).
@@ -107,7 +107,7 @@
         expected = "This request has been reported for administrator attention"
 
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           reason: "my reason",
           message: "It's just not"
         }
@@ -117,14 +117,14 @@
 
       it "should not allow a request to be reported twice" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           reason: "my reason"
         }
         expect(response).
           to redirect_to show_request_url(info_request.url_title)
 
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           reason: "my reason"
         }
         expect(response).
@@ -134,7 +134,7 @@
 
       it "should send an email from the reporter to admins" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           reason: "my reason",
           message: "It's just not"
         }
@@ -150,7 +150,7 @@
 
       it "should force the user to pick a reason" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           reason: ""
         }
         expect(response).to render_template("new")
@@ -170,7 +170,7 @@
 
       it "finds the expected request" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id,
           reason: "my reason"
         }
@@ -180,7 +180,7 @@
 
       it "finds the expected comment" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id,
           reason: "my reason"
         }
@@ -190,7 +190,7 @@
 
       it "sets reportable to the comment" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id,
           reason: "my reason"
         }
@@ -200,7 +200,7 @@
 
       it "sets report_reasons to the comment report reasons" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id,
           reason: "my reason"
         }
@@ -212,7 +212,7 @@
         new_comment = FactoryBot.create(:comment)
         expect {
           post :create, params: {
-            request_id: info_request.url_title,
+            request_url_title: info_request.url_title,
             comment_id: new_comment.id,
             reason: "my reason"
           }
@@ -221,7 +221,7 @@
 
       it "marks the comment as having been reported" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id,
           reason: "my reason"
         }
@@ -232,7 +232,7 @@
 
       it "does not mark the parent request as having been reported" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id,
           reason: "my reason"
         }
@@ -244,7 +244,7 @@
 
       it "sends an email alerting admins to the report" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id,
           reason: "my reason",
           message: "It's just not"
@@ -272,7 +272,7 @@
                    "administrator attention"
 
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id,
           reason: "my reason",
           message: "It's just not"
@@ -283,7 +283,7 @@
 
       it "redirects to the parent info_request page" do
         post :create, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id,
           reason: "my reason",
           message: "It's just not"
@@ -301,7 +301,7 @@
 
     context "not logged in" do
       it "should require the user to be logged in" do
-        get :new, params: { request_id: info_request.url_title }
+        get :new, params: { request_url_title: info_request.url_title }
         expect(response).
           to redirect_to(signin_path(token: PostRedirect.last.token))
       end
@@ -313,42 +313,42 @@
       end
 
       it "finds the expected request" do
-        get :new, params: { request_id: info_request.url_title }
+        get :new, params: { request_url_title: info_request.url_title }
         expect(assigns(:info_request)).to eq(info_request)
       end
 
       it "sets reportable to the request" do
-        get :new, params: { request_id: info_request.url_title }
+        get :new, params: { request_url_title: info_request.url_title }
         expect(assigns(:reportable)).to eq(info_request)
       end
 
       it "sets report_reasons to the request report reasons" do
-        get :new, params: { request_id: info_request.url_title }
+        get :new, params: { request_url_title: info_request.url_title }
         expect(assigns(:report_reasons)).to eq(info_request.report_reasons)
       end
 
       it "sets the page title" do
-        get :new, params: { request_id: info_request.url_title }
+        get :new, params: { request_url_title: info_request.url_title }
 
         expect(assigns(:title)).
           to eq("Report request: #{ info_request.title }")
       end
 
       it "should show the form" do
-        get :new, params: { request_id: info_request.url_title }
+        get :new, params: { request_url_title: info_request.url_title }
         expect(response).to render_template("new")
       end
 
       it "should 404 for non-existent requests" do
         expect {
-          get :new, params: { request_id: "hjksfdhjk_louytu_qqxxx" }
+          get :new, params: { request_url_title: "hjksfdhjk_louytu_qqxxx" }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
       it 'should 404 for embargoed requests' do
         info_request = FactoryBot.create(:embargoed_request)
         expect {
-          get :new, params: { request_id: info_request.url_title }
+          get :new, params: { request_url_title: info_request.url_title }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
     end
@@ -367,7 +367,7 @@
 
       it "finds the expected request" do
         get :new, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id
         }
         expect(assigns(:info_request)).to eq(info_request)
@@ -375,7 +375,7 @@
 
       it "finds the expected comment" do
         get :new, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id,
           reason: "my reason"
         }
@@ -385,7 +385,7 @@
 
       it "sets reportable to the comment" do
         get :new, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id
         }
 
@@ -394,7 +394,7 @@
 
       it "sets report_reasons to the comment report reasons" do
         get :new, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id
         }
 
@@ -403,7 +403,7 @@
 
       it "sets the page title" do
         get :new, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id
         }
 
@@ -415,7 +415,7 @@
         new_comment = FactoryBot.create(:comment)
         expect {
           get :new, params: {
-            request_id: info_request.url_title,
+            request_url_title: info_request.url_title,
             comment_id: new_comment.id
           }
         }.to raise_error(ActiveRecord::RecordNotFound)
@@ -423,7 +423,7 @@
 
       it "should show the form" do
         get :new, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id
         }
         expect(response).to render_template("new")
@@ -431,7 +431,7 @@
 
       it "copies the comment id into a hidden form field" do
         get :new, params: {
-          request_id: info_request.url_title,
+          request_url_title: info_request.url_title,
           comment_id: comment.id
         }
         expect(response.body).
@@ -442,7 +442,7 @@
       it "should 404 for non-existent requests" do
         expect {
           get :new, params: {
-            request_id: "hjksfdhjk_louytu_qqxxx",
+            request_url_title: "hjksfdhjk_louytu_qqxxx",
             comment_id: comment.id
           }
         }.to raise_error(ActiveRecord::RecordNotFound)
@@ -452,7 +452,7 @@
         info_request = FactoryBot.create(:embargoed_request)
         expect {
           get :new, params: {
-            request_id: info_request.url_title,
+            request_url_title: info_request.url_title,
             comment_id: comment.id
           }
         }.to raise_error(ActiveRecord::RecordNotFound)
diff --git a/spec/controllers/widget_votes_controller_spec.rb b/spec/controllers/widget_votes_controller_spec.rb
index 88028d5502..ab9f4ef4c4 100644
--- a/spec/controllers/widget_votes_controller_spec.rb
+++ b/spec/controllers/widget_votes_controller_spec.rb
@@ -11,12 +11,12 @@
     end
 
     it 'should find the info request' do
-      post :create, params: { request_id: info_request.id }
+      post :create, params: { request_url_title: info_request.url_title }
       expect(assigns[:info_request]).to eq(info_request)
     end
 
     it 'should redirect to the track path for the info request' do
-      post :create, params: { request_id: info_request.id }
+      post :create, params: { request_url_title: info_request.url_title }
       track_thing = TrackThing.create_track_for_request(info_request)
       expect(response).to redirect_to(do_track_path(track_thing))
     end
@@ -24,7 +24,7 @@
     context 'for a non-logged-in user without a tracking cookie' do
       it 'sets a tracking cookie' do
         allow(SecureRandom).to receive(:hex).and_return(mock_cookie)
-        post :create, params: { request_id: info_request.id }
+        post :create, params: { request_url_title: info_request.url_title }
         expect(cookies[:widget_vote]).to eq(mock_cookie)
       end
 
@@ -34,7 +34,7 @@
           widget_votes.
           where(cookie: mock_cookie)
 
-        post :create, params: { request_id: info_request.id }
+        post :create, params: { request_url_title: info_request.url_title }
 
         expect(votes.size).to eq(1)
       end
@@ -43,7 +43,7 @@
     context 'for a non-logged-in user with a tracking cookie' do
       it 'retains the existing tracking cookie' do
         request.cookies['widget_vote'] = mock_cookie
-        post :create, params: { request_id: info_request.id }
+        post :create, params: { request_url_title: info_request.url_title }
         expect(cookies[:widget_vote]).to eq(mock_cookie)
       end
 
@@ -53,7 +53,7 @@
           widget_votes.
           where(cookie: mock_cookie)
 
-        post :create, params: { request_id: info_request.id }
+        post :create, params: { request_url_title: info_request.url_title }
 
         expect(votes.size).to eq(1)
       end
@@ -65,7 +65,7 @@
           to receive(:enable_widgets).
           and_return(false)
         expect {
-          post :create, params: { request_id: info_request.id }
+          post :create, params: { request_url_title: info_request.url_title }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
     end
@@ -74,7 +74,7 @@
       it 'should return a 403' do
         info_request.prominence = 'hidden'
         info_request.save!
-        post :create, params: { request_id: info_request.id }
+        post :create, params: { request_url_title: info_request.url_title }
         expect(response.code).to eq("403")
       end
     end
@@ -83,7 +83,9 @@
       it 'should raise an ActiveRecord::RecordNotFound error' do
         embargoed_request = FactoryBot.create(:embargoed_request)
         expect {
-          post :create, params: { request_id: embargoed_request.id }
+          post :create, params: {
+            request_url_title: embargoed_request.url_title
+          }
         }.to raise_error ActiveRecord::RecordNotFound
       end
     end
diff --git a/spec/controllers/widgets_controller_spec.rb b/spec/controllers/widgets_controller_spec.rb
index 44ab9996f3..7930e163db 100644
--- a/spec/controllers/widgets_controller_spec.rb
+++ b/spec/controllers/widgets_controller_spec.rb
@@ -10,22 +10,22 @@
     end
 
     it 'should render the widget template' do
-      get :show, params: { request_id: @info_request.id }
+      get :show, params: { request_url_title: @info_request.url_title }
       expect(response).to render_template('show')
     end
 
     it 'should find the info request' do
-      get :show, params: { request_id: @info_request.id }
+      get :show, params: { request_url_title: @info_request.url_title }
       expect(assigns[:info_request]).to eq(@info_request)
     end
 
     it 'should create a track thing for the request' do
-      get :show, params: { request_id: @info_request.id }
+      get :show, params: { request_url_title: @info_request.url_title }
       expect(assigns[:track_thing].info_request).to eq(@info_request)
     end
 
     it 'should assign the request status' do
-      get :show, params: { request_id: @info_request.id }
+      get :show, params: { request_url_title: @info_request.url_title }
       expect(assigns[:status]).to eq(@info_request.calculate_status)
     end
 
@@ -42,7 +42,7 @@
         @info_request.widget_votes.create(cookie: SecureRandom.hex(10))
       end
 
-      get :show, params: { request_id: @info_request.id }
+      get :show, params: { request_url_title: @info_request.url_title }
 
       # Count should be 5
       # 1 for the request's owning user
@@ -53,25 +53,25 @@
 
     it 'sets user_owns_request to true if the user owns the request' do
       sign_in @info_request.user
-      get :show, params: { request_id: @info_request.id }
+      get :show, params: { request_url_title: @info_request.url_title }
       expect(assigns[:user_owns_request]).to be true
     end
 
     it 'sets user_owns_request to false if the user does not own the request' do
       sign_in FactoryBot.create(:user)
-      get :show, params: { request_id: @info_request.id }
+      get :show, params: { request_url_title: @info_request.url_title }
       expect(assigns[:user_owns_request]).to be false
     end
 
     it 'should not send an x-frame-options header' do
-      get :show, params: { request_id: @info_request.id }
+      get :show, params: { request_url_title: @info_request.url_title }
       expect(response.headers["X-Frame-Options"]).to be_nil
     end
 
     context 'for a non-logged-in user with a tracking cookie' do
       it 'will not find existing tracks' do
         request.cookies['widget_vote'] = mock_cookie
-        get :show, params: { request_id: @info_request.id }
+        get :show, params: { request_url_title: @info_request.url_title }
         expect(assigns[:existing_track]).to be_nil
       end
 
@@ -80,14 +80,14 @@
                                  info_request: @info_request,
                                  cookie: mock_cookie)
         request.cookies['widget_vote'] = vote.cookie
-        get :show, params: { request_id: @info_request.id }
+        get :show, params: { request_url_title: @info_request.url_title }
         expect(assigns[:existing_vote]).to be true
       end
 
       it 'will not find any existing votes if none exist' do
         WidgetVote.delete_all
         request.cookies['widget_vote'] = mock_cookie
-        get :show, params: { request_id: @info_request.id }
+        get :show, params: { request_url_title: @info_request.url_title }
         expect(assigns[:existing_vote]).to be false
       end
     end
@@ -95,13 +95,13 @@
     context 'for a non-logged-in user without a tracking cookie' do
       it 'will not find existing tracks' do
         request.cookies['widget_vote'] = nil
-        get :show, params: { request_id: @info_request.id }
+        get :show, params: { request_url_title: @info_request.url_title }
         expect(assigns[:existing_track]).to be_nil
       end
 
       it 'will not find any existing votes' do
         request.cookies['widget_vote'] = nil
-        get :show, params: { request_id: @info_request.id }
+        get :show, params: { request_url_title: @info_request.url_title }
         expect(assigns[:existing_vote]).to be false
       end
     end
@@ -115,7 +115,7 @@
         track.save!
         sign_in user
 
-        get :show, params: { request_id: @info_request.id }
+        get :show, params: { request_url_title: @info_request.url_title }
 
         expect(assigns[:existing_track]).to eq(track)
       end
@@ -127,7 +127,7 @@
         user = FactoryBot.create(:user)
         sign_in user
 
-        get :show, params: { request_id: @info_request.id }
+        get :show, params: { request_url_title: @info_request.url_title }
 
         expect(assigns[:existing_track]).to be_nil
       end
@@ -140,7 +140,7 @@
         sign_in @info_request.user
         request.cookies['widget_vote'] = mock_cookie
 
-        get :show, params: { request_id: @info_request.id }
+        get :show, params: { request_url_title: @info_request.url_title }
 
         expect(assigns[:existing_vote]).to be true
       end
@@ -151,7 +151,7 @@
         sign_in @info_request.user
         request.cookies['widget_vote'] = mock_cookie
 
-        get :show, params: { request_id: @info_request.id }
+        get :show, params: { request_url_title: @info_request.url_title }
 
         expect(assigns[:existing_vote]).to be false
       end
@@ -163,7 +163,7 @@
           to receive(:enable_widgets).
           and_return(false)
         expect {
-          get :show, params: { request_id: @info_request.id }
+          get :show, params: { request_url_title: @info_request.url_title }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
     end
@@ -172,7 +172,7 @@
       it 'should return a 403' do
         @info_request.prominence = 'hidden'
         @info_request.save!
-        get :show, params: { request_id: @info_request.id }
+        get :show, params: { request_url_title: @info_request.url_title }
         expect(response.code).to eq("403")
       end
 
@@ -182,7 +182,7 @@
                                  cookie: mock_cookie)
         sign_in @info_request.user
 
-        get :show, params: { request_id: @info_request.id }
+        get :show, params: { request_url_title: @info_request.url_title }
 
         expect(assigns[:existing_vote]).to be false
       end
@@ -198,12 +198,12 @@
     end
 
     it 'should render the create widget template' do
-      get :new, params: { request_id: @info_request.id }
+      get :new, params: { request_url_title: @info_request.url_title }
       expect(response).to render_template('new')
     end
 
     it 'should find the info request' do
-      get :new, params: { request_id: @info_request.id }
+      get :new, params: { request_url_title: @info_request.url_title }
       expect(assigns[:info_request]).to eq(@info_request)
     end
 
@@ -213,7 +213,7 @@
           to receive(:enable_widgets).
           and_return(false)
         expect {
-          get :new, params: { request_id: @info_request.id }
+          get :new, params: { request_url_title: @info_request.url_title }
         }.to raise_error(ActiveRecord::RecordNotFound)
       end
     end
@@ -222,7 +222,7 @@
       it 'should return a 403' do
         @info_request.prominence = 'hidden'
         @info_request.save!
-        get :show, params: { request_id: @info_request.id }
+        get :show, params: { request_url_title: @info_request.url_title }
         expect(response.code).to eq("403")
       end
     end
diff --git a/spec/routing/redirects_spec.rb b/spec/routing/redirects_spec.rb
index 6a3cf109ee..bc02d8026c 100644
--- a/spec/routing/redirects_spec.rb
+++ b/spec/routing/redirects_spec.rb
@@ -17,6 +17,15 @@
   it 'routes numerical request member routes to URL title member routes' do
     get('/request/105/followups/new')
     expect(response).to redirect_to('/request/the_cost_of_boring/followups/new')
+
+    get('/request/105/report/new')
+    expect(response).to redirect_to('/request/the_cost_of_boring/report/new')
+
+    get('/request/105/widget')
+    expect(response).to redirect_to('/request/the_cost_of_boring/widget')
+
+    get('/request/105/widget/new')
+    expect(response).to redirect_to('/request/the_cost_of_boring/widget/new')
   end
 
   it 'routes numerical request attachment routes to URL title attachment routes' do

From dcca4ed3481fdc427b9e1293d65debc92109837c Mon Sep 17 00:00:00 2001
From: Alexander Griffen <alexander@mysociety.org>
Date: Mon, 27 Nov 2023 15:23:44 +0000
Subject: [PATCH 12/12] Update changelog

---
 doc/CHANGES.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/doc/CHANGES.md b/doc/CHANGES.md
index 3194f69996..973df430b6 100644
--- a/doc/CHANGES.md
+++ b/doc/CHANGES.md
@@ -2,6 +2,8 @@
 
 ## Highlighted Features
 
+* Change request URL patterns to only use titles rather than request IDs
+  (Alexander Griffen, Graeme Porteous)
 * Colourise holding pen guess scores (Gareth Rees)
 * Fix default holding pen guess scores (Gareth Rees)
 * Add support for Debian 12 "Bookworm" (Graeme Porteous)