WIP: Attachments controller

This does remove a few tests that were looking specifically at IDs (or, 'ugly' IDs)
mysociety · Nov 3, 2023 · e8e4929 · e8e4929
1 parent f80bea2
commit e8e4929
Show file tree

Hide file tree

Showing 3 changed files with 7 additions and 47 deletions.
diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb
@@ -57,7 +57,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[:url_title])
       end
   end
 

diff --git a/config/routes.rb b/config/routes.rb
@@ -122,13 +122,13 @@ def matches?(request)
         :as => :similar_request,
         :via => :get
 
-  match '/request/:id/response/:incoming_message_id/attach/html' \
+  match '/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/:url_title/response/:incoming_message_id/attach/:part(/*file_name)' => 'attachments#show',
         :format => false,
         :as => :get_attachment,
         :via => :get,

diff --git a/spec/controllers/attachments_controller_spec.rb b/spec/controllers/attachments_controller_spec.rb
@@ -43,39 +43,11 @@ def show(params = {})
         part: attachment.url_part_number,
         file_name: attachment.display_filename
       }
-      default_params[:id] = info_request.id unless params[:public_token]
+      default_params[:url_title] = info_request.url_title unless params[:public_token]
       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,
@@ -144,7 +116,7 @@ def show(params = {})
           allow(verifier).to receive(:generate).with(
             get_attachment_path(
               incoming_message_id: attachment.incoming_message_id,
-              id: info_request.id,
+              url_title: info_request.url_title,
               part: attachment.url_part_number,
               file_name: attachment.filename
             )
@@ -416,7 +388,7 @@ 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]
+      default_params[:url_title] = info_request.url_title unless params[:public_token]
       get :show_as_html, params: default_params.merge(params)
     end
 
@@ -438,18 +410,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(
@@ -499,7 +459,7 @@ def show_as_html(params = {})
           allow(verifier).to receive(:generate).with(
             get_attachment_as_html_path(
               incoming_message_id: attachment.incoming_message_id,
-              id: info_request.id,
+              url_title: info_request.url_title,
               part: attachment.url_part_number,
               file_name: attachment.filename
             )