Ctskf 831 cccd refactor attachment links actual ticket

app/controllers/case_workers/admin/management_information_controller.rb

@@ -5,9 +5,9 @@ module Admin
     class ManagementInformationController < CaseWorkers::Admin::ApplicationController
       include ActiveStorage::SetCurrent
 
-      skip_load_and_authorize_resource only: %i[index download generate create]
-      before_action -> { authorize! :view, :management_information }, only: %i[index download generate create]
-      before_action :validate_report_type, only: %i[download generate create]
+      skip_load_and_authorize_resource only: %i[index generate create]
+      before_action -> { authorize! :view, :management_information }, only: %i[index generate create]
+      before_action :validate_report_type, only: %i[generate create]
       before_action :validate_report_type_is_updatable, only: %i[generate create]
       before_action :validate_and_set_date, only: %i[create]
 
@@ -17,17 +17,6 @@ def index
         end
       end
 
-      def download
-        log_download_start
-        record = Stats::StatsReport.most_recent_by_type(params[:report_type])
-
-        if record.document.attached?
-          redirect_to record.document.blob.url(disposition: 'attachment'), allow_other_host: true
-        else
-          redirect_to case_workers_admin_management_information_url, alert: t('.missing_report')
-        end
-      end
-
       def generate
         StatsReportGenerationJob.perform_later(report_type: params[:report_type])
         message = t('case_workers.admin.management_information.job_scheduled')
@@ -69,13 +58,13 @@ def validate_and_set_date
         redirect_to case_workers_admin_management_information_url, alert: t('.invalid_report_date')
       end
 
-      def log_download_start
-        LogStuff.info(class: 'CaseWorkers::Admin::ManagementInformationController',
-                      action: 'download',
-                      downloading_user_id: @current_user&.id) do
-          'MI Report download started'
-        end
-      end
+      # def log_download_start
+      #   LogStuff.info(class: 'CaseWorkers::Admin::ManagementInformationController',
+      #                 action: 'download',
+      #                 downloading_user_id: @current_user&.id) do
+      #     'MI Report download started'
+      #   end
+      # end
     end
   end
 end

app/controllers/csp_reports_controller.rb

@@ -3,13 +3,13 @@ class CspReportsController < ApplicationController
   skip_forgery_protection
 
   def create
-    slack_notifier.build_payload(
-      icon: ':security:',
-      title: 'Content Security Policy violation',
-      message: report.map { |key, value| "#{key}: #{value}" }.join("\n") + "\n\nUser agent: #{request.env['HTTP_USER_AGENT']}",
-      status: :fail
-    )
-    slack_notifier.send_message
+    # slack_notifier.build_payload(
+    #   icon: ':security:',
+    #   title: 'Content Security Policy violation',
+    #   message: report.map { |key, value| "#{key}: #{value}" }.join("\n") + "\n\nUser agent: #{request.env['HTTP_USER_AGENT']}",
+    #   status: :fail
+    # )
+    # slack_notifier.send_message
 
     head :ok
   end

app/controllers/documents_controller.rb

@@ -2,7 +2,7 @@ class DocumentsController < ApplicationController
   include ActiveStorage::SetCurrent
 
   respond_to :html
-  before_action :document, only: %i[show download destroy]
+  before_action :document, only: %i[show destroy]
 
   def index
     return render json: [] if params[:form_id].blank?
@@ -12,11 +12,8 @@ def index
   def show
     raise ActiveRecord::RecordNotFound, 'Preview not found' unless document.converted_preview_document.attached?
 
-    redirect_to document.converted_preview_document.blob.url(disposition: :inline), allow_other_host: true
-  end
-
-  def download
-    redirect_to document.document.blob.url(disposition: :attachment), allow_other_host: true
+    redirect_to document.converted_preview_document.rails_blob_path(message.attachment, disposition: 'attachment'),
+                allow_other_host: true
   end
 
   def create

app/controllers/messages_controller.rb

@@ -34,12 +34,6 @@ def create
     end
   end
 
-  def download_attachment
-    raise 'No attachment present on this message' unless message.attachment.attached?
-
-    redirect_to message.attachment.blob.url(disposition: 'attachment'), allow_other_host: true
-  end
-
   private
 
   def message

app/models/ability.rb

@@ -22,7 +22,7 @@ def initialize(user)
     end
 
     # applies to all external users and case workers
-    can %i[create download_attachment], Message
+    can %i[create], Message
     can %i[index update], UserMessageStatus
     can [:update_settings], User, id: user.id
 
@@ -46,7 +46,7 @@ def external_user(persona)
 
   def case_worker_admin(persona)
     can %i[index show update archived], Claim::BaseClaim
-    can %i[show download], Document
+    can %i[show], Document
     can %i[index new create], CaseWorker
     can %i[show show_message_controls edit change_password update_password update destroy], CaseWorker
     can %i[new create], Allocation
@@ -61,7 +61,7 @@ def case_worker(persona)
       claim.case_workers.include?(persona)
     end
     can_administer_any_provider if persona.roles.include?('provider_management')
-    can %i[show download], Document
+    can %i[show], Document
     can_manage_own_password(persona)
     can %i[dismiss], InjectionAttempt
   end
@@ -87,7 +87,7 @@ def can_administer_documents_in_provider(persona)
     can %i[index create], Document
 
     # NOTE: for destroy action, at least, the document may not be persisted/saved
-    can %i[show download destroy], Document do |document|
+    can %i[show destroy], Document do |document|
       if document.external_user_id.nil?
         User.active.find(document.creator_id).persona.provider.id == persona.provider.id
       else
@@ -127,7 +127,7 @@ def can_manage_own_claims_of_class(persona, claim_klass)
   def can_manage_own_documents(persona)
     can %i[index create], Document
 
-    can %i[show download destroy], Document do |document|
+    can %i[show destroy], Document do |document|
       if document.external_user_id.nil?
         document.creator_id == persona.user.id
       else

app/presenters/message_presenter.rb

@@ -1,4 +1,5 @@
 class MessagePresenter < BasePresenter
+  include Rails.application.routes.url_helpers
   presents :message
 
   def sender_is_a?(klass)
@@ -32,7 +33,9 @@ def download_file_link
     h.concat(
       h.tag.a(
         "#{attachment_file_name} (#{attachment_file_size})",
-        href: "/messages/#{message.id}/download_attachment",
+        href: rails_blob_path(message.attachment,
+                              disposition: 'attachment',
+                              host: Rails.application.config.action_mailer.default_url_options[:host]),
         title: 'Download ' + attachment_file_name
       )
     )

app/views/case_workers/admin/management_information/_stats_report.html.haml

@@ -12,7 +12,7 @@
 
 - if report_rec&.started_at.present?
   = govuk_link_button(t('download_html', scope: i18n_scope, report_name: report_name_html),
-                    case_workers_admin_management_information_download_url(report_type: report_object.name, format: :csv))
+                    rails_blob_path(report_rec.document, disposition: "attachment"))
 
 - if report_object.updatable?
   = govuk_link_button_secondary(t('update_report_html', scope: i18n_scope, report_name: report_name_html),

app/views/case_workers/admin/management_information/_stats_report_with_date_form.html.haml

@@ -8,7 +8,7 @@
         time: report_rec.started_at.strftime(Settings.report_date_format))
 
   = govuk_link_button(t('download_html', scope: i18n_scope, report_name: report_name_html),
-                      case_workers_admin_management_information_download_url(report_type: report_object.name, format: :csv))
+                       rails_blob_path(report_rec.document, disposition: "attachment"))
 - else
   %p
     = t('unavailable_report', scope: i18n_scope)

app/views/external_users/claims/supporting_documents/_existing.html.haml

@@ -17,10 +17,10 @@
           = govuk_table_td('data-label': t('.action')) do
             .app-link-group
               = govuk_link_to t('common.download_html', context: "#{document.document_file_name}"),
-                        download_document_path(document),
+                        rails_blob_url(document.document, disposition: 'inline'),
                         class: 'download'
               = govuk_link_to t('common.remove_html', context: "#{document.document_file_name}"),
-                        document_path(document),
+                       rails_blob_url(document.document, disposition: 'inline'),
                         method: :delete,
                         remote: true,
                         data: { confirm: 'Are you sure?' }

app/views/external_users/claims/supporting_evidence/_summary.html.haml

@@ -15,7 +15,7 @@
           - claim.documents.includes(:document_blob, :converted_preview_document_attachment).each do |document|
             %li
               = govuk_link_to document.document_file_name,
-                        download_document_path(document),
+                       rails_blob_url(document.document, disposition: 'inline'),
                         class: 'download',
                         'aria-label': "Download document: #{document.document_file_name}"
 

app/views/shared/_evidence_list.html.haml

@@ -46,5 +46,4 @@
               .app-link-group
                 - if document.converted_preview_document.present?
                   = govuk_link_to t('common.view_html', context: "#{document.document_file_name}"), document_path(document)
-
-                = govuk_link_to t('common.download_html', context: "#{document.document_file_name}"), download_document_path(document)
+                = govuk_link_to t('common.download_html', context: "#{document.document_file_name}"),rails_blob_path(document.attachment, disposition: "attachment")

config/routes.rb

@@ -58,13 +58,9 @@
 
   resources :claim_intentions, only: [:create], format: :json
 
-  resources :documents do
-    get 'download', on: :member
-  end
+  resources :documents
 
-  resources :messages, only: [:create] do
-    get 'download_attachment', on: :member
-  end
+  resources :messages, only: [:create]
 
   resources :establishments, only: %i[index], format: :js
   resources :offences, only: [:index], format: :js
@@ -189,7 +185,6 @@
       end
 
       get 'management_information', to: 'management_information#index', as: :management_information
-      get 'management_information/download', to: 'management_information#download', as: :management_information_download
       get 'management_information/generate', to: 'management_information#generate', as: :management_information_generate
       post 'management_information/create', to: 'management_information#create', as: :management_information_create
     end
@@ -213,7 +208,14 @@
   # catch-all route
   # -------------------------------------------------
   # WARNING: do not put routes below this point
-  unless Rails.env.development?
-    match '*path', to: 'errors#not_found', via: :all
+  # unless Rails.env.development?
+  #   match '*path', to: 'errors#not_found', via: :all
+  # end
+
+  if Rails.env.development?
+    scope format: true, constraints: { format: /jpg|png|gif|PNG/ } do
+      get '/*anything', to: proc { [404, {}, ['']] }, constraints: lambda { |request| !request.path_parameters[:anything].start_with?('rails/') }
+    end
   end
+
 end

spec/controllers/messages_controller_spec.rb

@@ -75,31 +75,6 @@
         end
       end
     end
-
-    describe 'GET #download_attachment' do
-      subject(:download_attachment) { get :download_attachment, params: { id: message.id } }
-
-      context 'when message has attachment' do
-        let(:message) { create(:message) }
-        let(:test_url) { 'https://document.storage/attachment.doc#123abc' }
-
-        before do
-          message.attachment.attach(io: StringIO.new, filename: 'attachment.doc')
-          allow(Message).to receive(:find).with(message[:id].to_s).and_return(message)
-          allow(message.attachment.blob).to receive(:url).and_return(test_url)
-        end
-
-        it { is_expected.to redirect_to test_url }
-      end
-
-      context 'when message does not have attachment' do
-        let(:message) { create(:message) }
-
-        it 'redirects to 500 page' do
-          expect { download_attachment }.to raise_exception('No attachment present on this message')
-        end
-      end
-    end
   end
 
   context 'email notifications' do

spec/requests/active_storage_spec.rb

@@ -0,0 +1,37 @@
+# spec/requests/active_storage_spec.rb
+
+require 'rails_helper'
+
+RSpec.describe 'ActiveStorage', type: :request do
+  describe 'GET /rails/active_storage/blobs/:signed_id/:filename' do
+    let(:message) { create(:message) }
+    let(:attachment_content) { 'dummy content' }
+    let(:attachment_filename) { 'attachment.doc' }
+    let(:attachment_content_type) { 'application/msword' }
+
+    before do
+      message.attachment.attach(io: StringIO.new(attachment_content), filename: attachment_filename, content_type: attachment_content_type)
+    end
+
+    context 'when the message has an attachment' do
+      subject(:get_attachment) { get rails_blob_path(message.attachment, disposition: 'attachment') }
+
+      it 'redirects to the ActiveStorage blob URL' do
+        get_attachment
+        expect(response).to have_http_status(:redirect)
+        expect(response.headers['Location']).to include('rails/active_storage/blobs/redirect/')
+        expect(response.headers['Location']).to include('?disposition=attachment')
+      end
+    end
+
+    context 'when the message does not have an attachment' do
+      let(:message_without_attachment) { create(:message) }
+
+      it 'raises an ActiveRecord::RecordNotFound error' do
+        expect {
+          get rails_blob_path(message_without_attachment.attachment, disposition: 'attachment', only_path: true)
+        }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+    end
+  end
+end