From e2979dfaef1057d946f7a4632818428d4624ac3a Mon Sep 17 00:00:00 2001 From: "hibo.abdilaahi" <51047911+hiboabd@users.noreply.github.com> Date: Tue, 31 Oct 2023 13:27:47 +0000 Subject: [PATCH 1/2] CRIMAP-687 display message when evidence download fails --- app/controllers/casework/documents_controller.rb | 8 +++++++- config/environments/test.rb | 2 +- config/locales/en/casework.yml | 1 + spec/shared_contexts/when_downloading_a_document.rb | 4 +++- .../that_is_assigned_to_me_spec.rb | 11 +++++++++++ 5 files changed, 23 insertions(+), 3 deletions(-) diff --git a/app/controllers/casework/documents_controller.rb b/app/controllers/casework/documents_controller.rb index c3fa69558..07274e656 100644 --- a/app/controllers/casework/documents_controller.rb +++ b/app/controllers/casework/documents_controller.rb @@ -10,7 +10,13 @@ def show; end def download presign_download = Datastore::Documents::Download.new(document: @document).call - redirect_to(presign_download.url, allow_other_host: true) if presign_download + + if presign_download.respond_to?(:url) + redirect_to(presign_download.url, allow_other_host: true) + else + set_flash(:cannot_download_try_again, file_name: @document.filename, success: false) + redirect_to crime_application_path(params[:crime_application_id]) + end end private diff --git a/config/environments/test.rb b/config/environments/test.rb index ab4841238..76a2a19db 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -29,7 +29,7 @@ config.cache_store = :null_store # Raise exceptions instead of rendering exception templates. - config.action_dispatch.show_exceptions = true + config.action_dispatch.show_exceptions = :all # Disable request forgery protection in test environment. config.action_controller.allow_forgery_protection = false diff --git a/config/locales/en/casework.yml b/config/locales/en/casework.yml index 19bed84f2..a70b2078c 100644 --- a/config/locales/en/casework.yml +++ b/config/locales/en/casework.yml @@ -38,6 +38,7 @@ en: cannot_send_back_when_marked_as_ready: This application was already marked as ready for assessment cannot_download_unless_assigned: You must assign this application to your list to download files cannot_download_doc_uploaded_to_another_app: File must be uploaded to current application to download + cannot_download_try_again: "%{file_name} could not be downloaded – try again" primary_navigation: your_list: "Your list (%{count})" diff --git a/spec/shared_contexts/when_downloading_a_document.rb b/spec/shared_contexts/when_downloading_a_document.rb index bc2ca8132..bc110b4e4 100644 --- a/spec/shared_contexts/when_downloading_a_document.rb +++ b/spec/shared_contexts/when_downloading_a_document.rb @@ -11,9 +11,11 @@ 'https://localhost.localstack.cloud:4566/crime-apply-documents-dev/42/WtpJTOwsQ2?response-content-disposition=attachment%3B%20filename%3Dtest.pdf&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=test%2F20231005%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20231005T112229Z&X-Amz-Expires=15&X-Amz-SignedHeaders=host&X-Amz-Signature=ca6081d4060ab8fc692f53b1746740f13a6bf3e46a6df0cf0acc06ed2367084e' end + let(:expected_return) { { status: 201, body: { url: presign_download_url }.to_json } } + before do stub_request(:put, 'https://datastore-api-stub.test/api/v1/documents/presign_download') .with(body: expected_query) - .to_return(status: 201, body: { url: presign_download_url }.to_json) + .to_return(expected_return) end end diff --git a/spec/system/casework/viewing_an_application/that_is_assigned_to_me_spec.rb b/spec/system/casework/viewing_an_application/that_is_assigned_to_me_spec.rb index 927f33983..1dcfff8f1 100644 --- a/spec/system/casework/viewing_an_application/that_is_assigned_to_me_spec.rb +++ b/spec/system/casework/viewing_an_application/that_is_assigned_to_me_spec.rb @@ -39,7 +39,9 @@ expect(page).not_to have_content('Mark as ready for MAAT') end end + end + describe 'Evidence download' do context 'when a user attempts to download supporting evidence' do it 'raises error if document is not part of current application' do visit download_documents_path(crime_application_id: application_id, id: '321/hijklm5678') @@ -52,5 +54,14 @@ expect(page).to have_current_path(presign_download_url) end end + + context 'when there is an error with obtaining the download url' do + let(:expected_return) { { status: 500 } } + + it 'displays a message if there is an error obtaining the url' do + click_on 'Download file (pdf, 12 Bytes)' + expect(page).to have_content('test.pdf could not be downloaded – try again') + end + end end end From eabc4f1343c263cabb195a88b4c5cfd63a6ac557 Mon Sep 17 00:00:00 2001 From: "hibo.abdilaahi" <51047911+hiboabd@users.noreply.github.com> Date: Tue, 31 Oct 2023 13:59:22 +0000 Subject: [PATCH 2/2] Add logging for evidence download failures --- app/controllers/casework/documents_controller.rb | 7 ++++++- app/services/datastore/documents/download.rb | 7 ++++--- spec/services/datastore/documents/download_spec.rb | 3 ++- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/app/controllers/casework/documents_controller.rb b/app/controllers/casework/documents_controller.rb index 07274e656..b9a2489a7 100644 --- a/app/controllers/casework/documents_controller.rb +++ b/app/controllers/casework/documents_controller.rb @@ -9,7 +9,7 @@ class CannotDownloadDocNotPartOfApp < StandardError; end def show; end def download - presign_download = Datastore::Documents::Download.new(document: @document).call + presign_download = Datastore::Documents::Download.new(document: @document, log_context: log_context).call if presign_download.respond_to?(:url) redirect_to(presign_download.url, allow_other_host: true) @@ -21,6 +21,11 @@ def download private + def log_context + { caseworker_id: current_user_id, caseworker_ip: request.remote_ip, file_type: @document.content_type, + s3_object_key: @document.s3_object_key } + end + def set_crime_application @crime_application = ::CrimeApplication.find(params[:crime_application_id]) end diff --git a/app/services/datastore/documents/download.rb b/app/services/datastore/documents/download.rb index 6d8506f3b..e21e73227 100644 --- a/app/services/datastore/documents/download.rb +++ b/app/services/datastore/documents/download.rb @@ -3,14 +3,15 @@ module Documents class Download PRESIGNED_URL_EXPIRES_IN = 15 # seconds - attr_accessor :document + attr_accessor :document, :log_context - def initialize(document:) + def initialize(document:, log_context:) @document = document + @log_context = log_context end def call - Rails.error.handle(fallback: -> { false }) do + Rails.error.handle(fallback: -> { false }, context: @log_context, severity: :error) do DatastoreApi::Requests::Documents::PresignDownload.new( object_key:, expires_in:, response_content_disposition: ).call diff --git a/spec/services/datastore/documents/download_spec.rb b/spec/services/datastore/documents/download_spec.rb index fda744124..85500e5d7 100644 --- a/spec/services/datastore/documents/download_spec.rb +++ b/spec/services/datastore/documents/download_spec.rb @@ -1,11 +1,12 @@ require 'rails_helper' RSpec.describe Datastore::Documents::Download do - subject(:download_service) { described_class.new(document:) } + subject(:download_service) { described_class.new(document:, log_context:) } include_context 'when downloading a document' let(:document) { instance_double(Document, filename:, s3_object_key:) } + let(:log_context) { { caseworker_id: 1, caseworker_ip: '123.0001' } } describe '#call' do context 'when a document download link is retrieved successfully' do