Merge pull request #3140 from alphagov/store-upload-details-in-metadata

Store all data about uploaded letters in the S3 metadata

app/main/views/uploads.py

@@ -20,6 +20,7 @@ from app.extensions import antivirus_client
 from app.main import main
 from app.main.forms import PDFUploadForm
 from app.s3_client.s3_letter_upload_client import (
+    get_letter_metadata,
     get_letter_pdf_and_metadata,
     get_transient_letter_file_location,
     upload_letter_to_s3,
@@ -43,6 +44,7 @@ def upload_letter(service_id):
 
     if form.validate_on_submit():
         pdf_file_bytes = form.file.data.read()
+        original_filename = form.file.data.filename
 
         virus_free = antivirus_client.scan(BytesIO(pdf_file_bytes))
         if not virus_free:
@@ -67,22 +69,29 @@ def upload_letter(service_id):
         except RequestException as ex:
             if ex.response is not None and ex.response.status_code == 400:
                 status = 'invalid'
-                upload_letter_to_s3(pdf_file_bytes, file_location, status)
+                upload_letter_to_s3(
+                    pdf_file_bytes,
+                    file_location=file_location,
+                    status=status,
+                    page_count=page_count,
+                    filename=original_filename)
             else:
                 raise ex
         else:
             status = 'valid'
             file_contents = base64.b64decode(response.json()['file'].encode())
-            upload_letter_to_s3(file_contents, file_location, status)
+            upload_letter_to_s3(
+                file_contents,
+                file_location=file_location,
+                status=status,
+                page_count=page_count,
+                filename=original_filename)
 
         return redirect(
             url_for(
                 'main.uploaded_letter_preview',
                 service_id=current_service.id,
                 file_id=upload_id,
-                original_filename=form.file.data.filename,
-                page_count=page_count,
-                status=status,
             )
         )
 
@@ -97,9 +106,10 @@ def invalid_upload_error(message):
 @main.route("/services/<service_id>/preview-letter/<file_id>")
 @user_has_permissions('send_messages')
 def uploaded_letter_preview(service_id, file_id):
-    original_filename = request.args.get('original_filename')
+    metadata = get_letter_metadata(service_id, file_id)
-    page_count = request.args.get('page_count')
+    original_filename = metadata.get('filename')
-    status = request.args.get('status')
+    page_count = metadata.get('page_count')
+    status = metadata.get('status')
 
     template_dict = service_api_client.get_precompiled_template(service_id)
 
@@ -126,8 +136,7 @@ def uploaded_letter_preview(service_id, file_id):
 @main.route("/services/<service_id>/preview-letter-image/<file_id>")
 @user_has_permissions('send_messages')
 def view_letter_upload_as_preview(service_id, file_id):
-    file_location = get_transient_letter_file_location(service_id, file_id)
+    pdf_file, metadata = get_letter_pdf_and_metadata(service_id, file_id)
-    pdf_file, metadata = get_letter_pdf_and_metadata(file_location)
 
     page = request.args.get('page')
 
@@ -140,14 +149,12 @@ def view_letter_upload_as_preview(service_id, file_id):
 @main.route("/services/<service_id>/upload-letter/send", methods=['POST'])
 @user_has_permissions('send_messages', restrict_admin_usage=True)
 def send_uploaded_letter(service_id):
-    filename = request.form['filename']
-    file_id = request.form['file_id']
-
     if not (current_service.has_permission('letter') and current_service.has_permission('upload_letters')):
         abort(403)
 
-    file_location = get_transient_letter_file_location(service_id, file_id)
+    file_id = request.form['file_id']
-    _, metadata = get_letter_pdf_and_metadata(file_location)
+    metadata = get_letter_metadata(service_id, file_id)
+    filename = metadata.get('filename')
 
     if metadata.get('status') != 'valid':
         abort(403)

app/s3_client/s3_letter_upload_client.py

@@ -7,17 +7,22 @@ def get_transient_letter_file_location(service_id, upload_id):
     return 'service-{}/{}.pdf'.format(service_id, upload_id)
 
 
-def upload_letter_to_s3(data, file_location, status):
+def upload_letter_to_s3(data, *, file_location, status, page_count, filename):
     utils_s3upload(
         filedata=data,
         region=current_app.config['AWS_REGION'],
         bucket_name=current_app.config['TRANSIENT_UPLOADED_LETTERS'],
         file_location=file_location,
-        metadata={'status': status}
+        metadata={
+            'status': status,
+            'page_count': str(page_count),
+            'filename': filename,
+        }
     )
 
 
-def get_letter_pdf_and_metadata(file_location):
+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()
 
@@ -25,3 +30,11 @@ def get_letter_pdf_and_metadata(file_location):
     metadata = s3_object['Metadata']
 
     return pdf, 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()
+
+    return s3_object['Metadata']

app/templates/views/uploads/preview.html

@@ -28,7 +28,6 @@
           service_id=current_service.id,
         )}}" class='page-footer'>
         <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
-        <input type="hidden" name="filename" value="{{ original_filename }}" />
         <input type="hidden" name="file_id" value="{{ file_id }}" />
         <button type="submit" class="button">Send 1 letter</button>
       </form>

tests/app/main/views/test_uploads.py

@@ -33,6 +33,8 @@ def test_post_upload_letter_redirects_for_valid_file(mocker, client_request):
         return_value=Mock(content='The sanitised content', json=lambda: {'file': 'VGhlIHNhbml0aXNlZCBjb250ZW50'})
     )
     mock_s3 = mocker.patch('app.main.views.uploads.upload_letter_to_s3')
+    mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={
+        'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '1', 'status': 'valid'})
     mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template')
 
     with open('tests/test_pdf_files/one_page_pdf.pdf', 'rb') as file:
@@ -46,14 +48,15 @@ def test_post_upload_letter_redirects_for_valid_file(mocker, client_request):
 
     mock_s3.assert_called_once_with(
         b'The sanitised content',
-        'service-{}/fake-uuid.pdf'.format(SERVICE_ONE_ID),
+        file_location='service-{}/fake-uuid.pdf'.format(SERVICE_ONE_ID),
-        'valid',
+        status='valid',
+        page_count=1,
+        filename='tests/test_pdf_files/one_page_pdf.pdf'
     )
 
     assert page.find('h1').text == 'tests/test_pdf_files/one_page_pdf.pdf'
     assert not page.find(id='validation-error-message')
 
-    assert page.find('input', {'type': 'hidden', 'name': 'filename', 'value': 'tests/test_pdf_files/one_page_pdf.pdf'})
     assert page.find('input', {'type': 'hidden', 'name': 'file_id', 'value': 'fake-uuid'})
     assert page.find('button', {'type': 'submit'}).text == 'Send 1 letter'
 
@@ -73,6 +76,8 @@ def test_post_upload_letter_shows_letter_preview_for_valid_file(mocker, client_r
     )
     mocker.patch('app.main.views.uploads.upload_letter_to_s3')
     mocker.patch('app.main.views.uploads.pdf_page_count', return_value=3)
+    mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={
+        'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '3', 'status': 'valid'})
     mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template', return_value=letter_template)
 
     with open('tests/test_pdf_files/one_page_pdf.pdf', 'rb') as file:
@@ -170,6 +175,8 @@ def test_post_upload_letter_with_invalid_file(mocker, client_request):
     mock_sanitise_response.raise_for_status.side_effect = RequestException(response=Mock(status_code=400))
     mocker.patch('app.main.views.uploads.sanitise_letter', return_value=mock_sanitise_response)
     mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template')
+    mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={
+        'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '1', 'status': 'invalid'})
 
     with open('tests/test_pdf_files/one_page_pdf.pdf', 'rb') as file:
         file_contents = file.read()
@@ -184,8 +191,10 @@ def test_post_upload_letter_with_invalid_file(mocker, client_request):
 
         mock_s3.assert_called_once_with(
             file_contents,
-            'service-{}/fake-uuid.pdf'.format(SERVICE_ONE_ID),
+            file_location='service-{}/fake-uuid.pdf'.format(SERVICE_ONE_ID),
-            'invalid',
+            status='invalid',
+            page_count=1,
+            filename='tests/test_pdf_files/one_page_pdf.pdf'
         )
 
     assert page.find('h1').text == 'tests/test_pdf_files/one_page_pdf.pdf'
@@ -209,6 +218,8 @@ def test_post_upload_letter_shows_letter_preview_for_invalid_file(mocker, client
     mock_sanitise_response.raise_for_status.side_effect = RequestException(response=Mock(status_code=400))
     mocker.patch('app.main.views.uploads.sanitise_letter', return_value=mock_sanitise_response)
     mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template', return_value=letter_template)
+    mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={
+        'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '1', 'status': 'invalid'})
 
     with open('tests/test_pdf_files/one_page_pdf.pdf', 'rb') as file:
         page = client_request.post(
@@ -253,6 +264,8 @@ def test_post_upload_letter_does_not_upload_to_s3_if_template_preview_raises_unk
 
 def test_uploaded_letter_preview(mocker, client_request):
     mocker.patch('app.main.views.uploads.service_api_client')
+    mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={
+        'filename': 'my_letter.pdf', 'page_count': '1', 'status': 'valid'})
 
     page = client_request.get(
         'main.uploaded_letter_preview',
@@ -268,8 +281,11 @@ def test_uploaded_letter_preview(mocker, client_request):
 
 
 def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page(mocker, service_one, client_request):
-    mocker.patch('app.main.views.uploads.get_letter_pdf_and_metadata', return_value=('file', {'status': 'valid'}))
+    metadata = {'filename': 'my_file.pdf', 'page_count': '1', 'status': 'valid'}
+
+    mocker.patch('app.main.views.uploads.get_letter_pdf_and_metadata', return_value=('file', metadata))
     mock_send = mocker.patch('app.main.views.uploads.notification_api_client.send_precompiled_letter')
+    mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=metadata)
 
     service_one['permissions'] = ['letter', 'upload_letters']
     file_id = 'abcd-1234'
@@ -315,8 +331,11 @@ def test_send_uploaded_letter_when_service_does_not_have_correct_permissions(
 
 
 def test_send_uploaded_letter_when_metadata_states_pdf_is_invalid(mocker, service_one, client_request):
-    mocker.patch('app.main.views.uploads.get_letter_pdf_and_metadata', return_value=('file', {'status': 'invalid'}))
     mock_send = mocker.patch('app.main.views.uploads.notification_api_client.send_precompiled_letter')
+    mocker.patch(
+        'app.main.views.uploads.get_letter_metadata',
+        return_value={'filename': 'my_file.pdf', 'page_count': '3', 'status': 'invalid'}
+    )
 
     service_one['permissions'] = ['letter', 'upload_letters']
     file_id = 'abcd-1234'

tests/app/s3_client/test_s3_letter_upload_client.py

@@ -6,12 +6,17 @@ from app.s3_client.s3_letter_upload_client import upload_letter_to_s3
 def test_upload_letter_to_s3(mocker):
     s3_mock = mocker.patch('app.s3_client.s3_letter_upload_client.utils_s3upload')
 
-    upload_letter_to_s3('pdf_data', 'service_id/upload_id.pdf', 'valid')
+    upload_letter_to_s3(
+        'pdf_data',
+        file_location='service_id/upload_id.pdf',
+        status='valid',
+        page_count=3,
+        filename='my_doc')
 
     s3_mock.assert_called_once_with(
         bucket_name=current_app.config['TRANSIENT_UPLOADED_LETTERS'],
         file_location='service_id/upload_id.pdf',
         filedata='pdf_data',
-        metadata={'status': 'valid'},
+        metadata={'status': 'valid', 'page_count': '3', 'filename': 'my_doc'},
         region=current_app.config['AWS_REGION']
     )