Catch error if letter does not exist on send

This repeats the pattern we already have for previewing a letter,
where we assume the error is because the notification has already
been sent and redirect the user to see it.

I've improved the original pattern a bit:

- I've DRYed-up the low-level boto code and moved the error handler
there so it can be reused.

- I've introduced a custom exception, which the calling code can
choose to log.

- I've introduced the moto library, which we use elsewhere, to make
it easier to test S3 code.

I've used an error level log when sending a notification - now that
we have a more descriptive log, we can verify the assumption is true
and then make an informed decision to downgrade the log.

In future we may want to merge this handler with the similar code
in utils [1], but we'll need to be careful as the utils handler is
superficial - it doesn't check the reason for the error.

[1]: bce0f4e596/notifications_utils/s3.py (L52)
This commit is contained in:
Ben Thorner
2022-02-18 14:37:09 +00:00
parent 73cc034676
commit 4fef2861c6
5 changed files with 103 additions and 24 deletions

View File

@@ -1,12 +1,14 @@
from unittest.mock import ANY, Mock
import pytest
from botocore.exceptions import ClientError
from flask import make_response, url_for
from requests import RequestException
from app.formatters import normalize_spaces
from app.s3_client.s3_letter_upload_client import LetterMetadata
from app.s3_client.s3_letter_upload_client import (
LetterMetadata,
LetterNotFoundError,
)
from tests.conftest import SERVICE_ONE_ID
@@ -465,11 +467,11 @@ def test_uploaded_letter_preview_redirects_if_file_not_in_s3(
client_request,
fake_uuid
):
boto_error_json = {'Error': {'Code': 'NoSuchKey', 'Message': 'The specified key does not exist.'}}
mocker.patch(
'app.main.views.uploads.get_letter_metadata',
side_effect=ClientError(boto_error_json, 'operation_name')
side_effect=LetterNotFoundError
)
client_request.get(
'main.uploaded_letter_preview',
service_id=SERVICE_ONE_ID,
@@ -646,6 +648,33 @@ def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page(
)
def test_send_uploaded_letter_redirects_if_file_not_in_s3(
mocker,
client_request,
fake_uuid,
service_one,
):
mocker.patch(
'app.main.views.uploads.get_letter_metadata',
side_effect=LetterNotFoundError
)
service_one['permissions'] = ['letter', 'upload_letters']
client_request.post(
'main.send_uploaded_letter',
service_id=SERVICE_ONE_ID,
file_id=fake_uuid,
_data={'filename': 'my_file.pdf'},
_expected_redirect=url_for(
'main.view_notification',
service_id=SERVICE_ONE_ID,
notification_id=fake_uuid,
_external=True
)
)
@pytest.mark.parametrize('permissions', [
['email'],
['sms'],

View File

@@ -1,11 +1,17 @@
import urllib
import uuid
import boto3
import botocore
import pytest
from flask import current_app
from moto import mock_s3
from app.s3_client.s3_letter_upload_client import (
LetterMetadata,
LetterNotFoundError,
backup_original_letter_to_s3,
get_letter_metadata,
upload_letter_to_s3,
)
@@ -89,3 +95,26 @@ def test_lettermetadata_unquotes_special_keys():
metadata = LetterMetadata({"filename": "%C2%A3hello", "recipient": "%C2%A3hi"})
assert metadata.get("filename") == "£hello"
assert metadata.get("recipient") == "£hi"
@mock_s3
@pytest.mark.parametrize('will_raise_custom_error,expected_exception', [
(True, LetterNotFoundError),
(False, botocore.exceptions.ClientError)
])
def test_get_letter_s3_object_raises_custom_error(
will_raise_custom_error,
expected_exception
):
bucket_name = current_app.config['TRANSIENT_UPLOADED_LETTERS']
s3 = boto3.client('s3', region_name='eu-west-1')
# bucket not existing will trigger some other error
if will_raise_custom_error:
s3.create_bucket(
Bucket=bucket_name,
CreateBucketConfiguration={'LocationConstraint': 'eu-west-1'}
)
with pytest.raises(expected_exception):
get_letter_metadata('service', 'file')