diff --git a/app/clients/document_download.py b/app/clients/document_download.py index b40184452..f6d2968e6 100644 --- a/app/clients/document_download.py +++ b/app/clients/document_download.py @@ -12,13 +12,8 @@ class DocumentDownloadError(Exception): @classmethod def from_exception(cls, e): - try: - message = e.response.json()['error'] - status_code = e.response.status_code - except (TypeError, ValueError, AttributeError, KeyError): - message = 'connection error' - status_code = 503 - + message = e.response.json()['error'] + status_code = e.response.status_code return cls(message, status_code) @@ -45,11 +40,16 @@ class DocumentDownloadClient: response.raise_for_status() except requests.RequestException as e: - error = DocumentDownloadError.from_exception(e) - current_app.logger.warning( - 'Document download request failed with error: {}'.format(error.message) - ) - - raise error + # if doc dl responds with a non-400, (eg 403) it's referring to credentials that the API and Doc DL use. + # we don't want to tell users about that, so anything that isn't a 400 (virus scan failed or file type + # unrecognised) should be raised as a 500 internal server error here. + if e.response.status_code == 400: + error = DocumentDownloadError.from_exception(e) + current_app.logger.info( + 'Document download request failed with error: {}'.format(error.message) + ) + raise error + else: + raise Exception(f'Unhandled document download error: {e.response.text}') return response.json()['document']['url'] diff --git a/app/v2/notifications/get_notifications.py b/app/v2/notifications/get_notifications.py index 95cf1aa51..7f90f7a9d 100644 --- a/app/v2/notifications/get_notifications.py +++ b/app/v2/notifications/get_notifications.py @@ -39,7 +39,7 @@ def get_pdf_for_notification(notification_id): raise BadRequestError(message="Notification is not a letter") if notification.status == NOTIFICATION_VIRUS_SCAN_FAILED: - raise BadRequestError(message='Document did not pass the virus scan') + raise BadRequestError(message='File did not pass the virus scan') if notification.status == NOTIFICATION_TECHNICAL_FAILURE: raise BadRequestError(message='PDF not available for letters in status {}'.format(notification.status)) diff --git a/tests/app/clients/test_document_download.py b/tests/app/clients/test_document_download.py index 8a15ba1c1..ca4f5a89f 100644 --- a/tests/app/clients/test_document_download.py +++ b/tests/app/clients/test_document_download.py @@ -1,4 +1,3 @@ -import requests import requests_mock import pytest @@ -33,26 +32,27 @@ def test_upload_document(document_download): assert resp == 'https://document-download/services/service-id/documents/uploaded-url' -def test_should_raise_for_status(document_download): +def test_should_raise_400s_as_DocumentDownloadErrors(document_download): with pytest.raises(DocumentDownloadError) as excinfo, requests_mock.Mocker() as request_mock: request_mock.post('https://document-download/services/service-id/documents', json={ - 'error': 'Invalid encoding' - }, status_code=403) + 'error': 'Invalid mime type' + }, status_code=400) document_download.upload_document('service-id', 'abababab') - assert excinfo.value.message == 'Invalid encoding' - assert excinfo.value.status_code == 403 + assert excinfo.value.message == 'Invalid mime type' + assert excinfo.value.status_code == 400 -def test_should_raise_for_connection_errors(document_download): - with pytest.raises(DocumentDownloadError) as excinfo, requests_mock.Mocker() as request_mock: +def test_should_raise_non_400_statuses_as_exceptions(document_download): + with pytest.raises(Exception) as excinfo, requests_mock.Mocker() as request_mock: request_mock.post( 'https://document-download/services/service-id/documents', - exc=requests.exceptions.ConnectTimeout + json={'error': 'Auth Error Of Some Kind'}, + status_code=403 ) document_download.upload_document('service-id', 'abababab') - assert excinfo.value.message == 'connection error' - assert excinfo.value.status_code == 503 + assert type(excinfo.value) == Exception # make sure it's a base exception, so will be handled as a 500 by v2 api + assert str(excinfo.value) == 'Unhandled document download error: {"error": "Auth Error Of Some Kind"}' diff --git a/tests/app/db.py b/tests/app/db.py index 6f5feed9f..f3540f8ac 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -250,7 +250,7 @@ def create_notification( updated_at = updated_at or datetime.utcnow() if not one_off and (job is None and api_key is None): - # we didn't specify in test - lets create it + # we did not specify in test - lets create it api_key = ApiKey.query.filter(ApiKey.service == template.service, ApiKey.key_type == key_type).first() if not api_key: api_key = create_api_key(template.service, key_type=key_type) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 91f5bd752..be90a137b 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -703,7 +703,7 @@ def test_get_pdf_for_notification_returns_400_if_pdf_not_found( @pytest.mark.parametrize('status, expected_message', [ - ('virus-scan-failed', 'Document did not pass the virus scan'), + ('virus-scan-failed', 'File did not pass the virus scan'), ('technical-failure', 'PDF not available for letters in status technical-failure'), ]) def test_get_pdf_for_notification_only_returns_pdf_content_if_right_status(