Merge pull request #4159 from alphagov/catch-missing-letter

Catch error if letter does not exist on send

app/main/views/uploads.py

@@ -7,7 +7,6 @@ from functools import partial
 from io import BytesIO
 from zipfile import BadZipFile
 
-from botocore.exceptions import ClientError
 from flask import (
     abort,
     current_app,
@@ -39,6 +38,7 @@ from app.main import main
 from app.main.forms import CsvUploadForm, LetterUploadPostageForm, PDFUploadForm
 from app.models.contact_list import ContactList
 from app.s3_client.s3_letter_upload_client import (
+    LetterNotFoundError,
     backup_original_letter_to_s3,
     get_letter_metadata,
     get_letter_pdf_and_metadata,
@@ -267,17 +267,18 @@ def uploaded_letter_preview(service_id, file_id):
 
     try:
         metadata = get_letter_metadata(service_id, file_id)
-    except ClientError as e:
-        # if the file's not there, it's probably because we've already created the notification and the letter has been
-        # moved to the normal letters-pdf bucket. So lets just bounce out to the notification page
-        if e.response['Error']['Code'] == 'NoSuchKey':
-            return redirect(url_for(
-                '.view_notification',
-                service_id=service_id,
-                notification_id=file_id,
-            ))
-        else:
-            raise
+    except LetterNotFoundError as e:
+        current_app.logger.warning(e)
+
+        # If the file is missing it could be because this is a duplicate
+        # request, the notification already exists and the file has been
+        # moved to a different bucket. Note that the ID of a precompiled
+        # notification is always set to the file_id.
+        return redirect(url_for(
+            '.view_notification',
+            service_id=service_id,
+            notification_id=file_id,
+        ))
 
     original_filename = metadata.get('filename')
     page_count = metadata.get('page_count')
@@ -352,7 +353,20 @@ def send_uploaded_letter(service_id, file_id):
     if not current_service.has_permission('letter'):
         abort(403)
 
-    metadata = get_letter_metadata(service_id, file_id)
+    try:
+        metadata = get_letter_metadata(service_id, file_id)
+    except LetterNotFoundError as e:
+        current_app.logger.error(e)
+
+        # If the file is missing it could be because this is a duplicate
+        # request, the notification already exists and the file has been
+        # moved to a different bucket. Note that the ID of a precompiled
+        # notification is always set to the file_id.
+        return redirect(url_for(
+            '.view_notification',
+            service_id=service_id,
+            notification_id=file_id,
+        ))
 
     if metadata.get('status') != 'valid':
         abort(403)

app/s3_client/s3_letter_upload_client.py

@@ -1,11 +1,16 @@
 import json
 import urllib
 
+import botocore
 from boto3 import resource
 from flask import current_app
 from notifications_utils.s3 import s3upload as utils_s3upload
 
 
+class LetterNotFoundError(Exception):
+    pass
+
+
 def get_transient_letter_file_location(service_id, upload_id):
     return 'service-{}/{}.pdf'.format(service_id, upload_id)
 
@@ -69,19 +74,24 @@ class LetterMetadata:
         return value
 
 
+def get_letter_s3_object(service_id, file_id):
+    try:
+        file_location = get_transient_letter_file_location(service_id, file_id)
+        s3 = resource('s3')
+        return s3.Object(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location).get()
+    except botocore.exceptions.ClientError as e:
+        if e.response['Error']['Code'] == 'NoSuchKey':
+            raise LetterNotFoundError(f'Letter not found for service {service_id} and file {file_id}')
+
+        raise
+
+
 def get_letter_pdf_and_metadata(service_id, file_id):
-    file_location = get_transient_letter_file_location(service_id, file_id)
-    s3 = resource('s3')
-    s3_object = s3.Object(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location).get()
-
+    s3_object = get_letter_s3_object(service_id, file_id)
     pdf = s3_object['Body'].read()
-
     return pdf, LetterMetadata(s3_object['Metadata'])
 
 
 def get_letter_metadata(service_id, file_id):
-    file_location = get_transient_letter_file_location(service_id, file_id)
-    s3 = resource('s3')
-    s3_object = s3.Object(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location).get()
-
+    s3_object = get_letter_s3_object(service_id, file_id)
     return LetterMetadata(s3_object['Metadata'])

requirements_for_test.txt

@@ -9,6 +9,7 @@ freezegun==1.1.0
 flake8==4.0.1
 flake8-bugbear==21.9.2
 flake8-print==4.0.0
+moto==3.0.4
 requests-mock==1.9.3
 # used for creating manifest file locally
 jinja2-cli[yaml]==0.7.0

tests/app/main/views/uploads/test_upload_letter.py

@@ -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'],

tests/app/s3_client/test_s3_letter_upload_client.py

@@ -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')