From c2825e10b1b96021cae83492a5038e2404d384bd Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 29 Oct 2019 16:19:50 +0000 Subject: [PATCH] Grab metadata when getting pdf letter preview from S3 Also use this metadata to decide whether preview pages need overlay or not. So far we have always added overlay when validation has failed. Now we will only show it when validation failed due to content being outside of printable area. --- app/letters/utils.py | 6 +- app/template/rest.py | 9 +-- app/v2/notifications/get_notifications.py | 4 +- tests/app/celery/test_letters_pdf_tasks.py | 4 +- tests/app/letters/test_letter_utils.py | 6 +- tests/app/template/test_rest.py | 65 ++++++++++++++----- .../notifications/test_get_notifications.py | 20 ++++-- 7 files changed, 81 insertions(+), 33 deletions(-) diff --git a/app/letters/utils.py b/app/letters/utils.py index 4aa4ee5c7..272cd9f92 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -164,7 +164,7 @@ def get_file_names_from_error_bucket(): return bucket.objects.filter(Prefix="ERROR") -def get_letter_pdf(notification): +def get_letter_pdf_and_metadata(notification): bucket_name, prefix = get_bucket_name_and_prefix_for_notification(notification) s3 = boto3.resource('s3') @@ -174,8 +174,8 @@ def get_letter_pdf(notification): obj = s3.Object( bucket_name=bucket_name, key=item.key - ) - return obj.get()["Body"].read() + ).get() + return obj["Body"].read(), obj["Metadata"] def _move_s3_object(source_bucket, source_filename, target_bucket, target_filename, metadata=None): diff --git a/app/template/rest.py b/app/template/rest.py index 85b610ebb..b77df5741 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -32,7 +32,7 @@ from app.errors import ( register_errors, InvalidRequest ) -from app.letters.utils import get_letter_pdf +from app.letters.utils import get_letter_pdf_and_metadata from app.models import SMS_TYPE, Template, SECOND_CLASS, LETTER_TYPE from app.notifications.validators import service_has_permission, check_reply_to from app.schema_validation import validate @@ -231,11 +231,12 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file notification = get_notification_by_id(notification_id) template = dao_get_template_by_id(notification.template_id) + metadata = {} if template.is_precompiled_letter: try: - pdf_file = get_letter_pdf(notification) + pdf_file, metadata = get_letter_pdf_and_metadata(notification) except botocore.exceptions.ClientError as e: raise InvalidRequest( @@ -245,7 +246,7 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file ) content = base64.b64encode(pdf_file).decode('utf-8') - overlay = request.args.get('overlay') + overlay = metadata.get("message") == "content-outside-printable-area" page_number = page if page else "1" if overlay: @@ -300,7 +301,7 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file ) response_content = _get_png_preview_or_overlaid_pdf(url, data, notification.id, json=True) - return jsonify({"content": response_content}) + return jsonify({"content": response_content, "metadata": metadata}) def _get_png_preview_or_overlaid_pdf(url, data, notification_id, json=True): diff --git a/app/v2/notifications/get_notifications.py b/app/v2/notifications/get_notifications.py index e5d267024..95cf1aa51 100644 --- a/app/v2/notifications/get_notifications.py +++ b/app/v2/notifications/get_notifications.py @@ -4,7 +4,7 @@ from flask import jsonify, request, url_for, current_app, send_file from app import api_user, authenticated_service from app.dao import notifications_dao -from app.letters.utils import get_letter_pdf +from app.letters.utils import get_letter_pdf_and_metadata from app.schema_validation import validate from app.v2.errors import BadRequestError, PDFNotReadyError from app.v2.notifications import v2_notification_blueprint @@ -48,7 +48,7 @@ def get_pdf_for_notification(notification_id): raise PDFNotReadyError() try: - pdf_data = get_letter_pdf(notification) + pdf_data, metadata = get_letter_pdf_and_metadata(notification) except Exception: raise PDFNotReadyError() diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 38e61deef..fd41f4b00 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -480,7 +480,7 @@ def test_process_letter_task_check_virus_scan_passed_when_sanitise_fails( "validation_passed": False, "message": "content-outside-printable-area", "invalid_pages": [1, 2], - "page_count": 1 + "page_count": 2 } mock_sanitise = mocker.patch( 'app.celery.letters_pdf_tasks._sanitise_precompiled_pdf', return_value=(sanitise_response, "validation_failed") @@ -500,7 +500,7 @@ def test_process_letter_task_check_virus_scan_passed_when_sanitise_fails( target_bucket=target_bucket_name, target_filename=filename, metadata={ "message": "content-outside-printable-area", "invalid_pages": "[1, 2]", - "page_count": "1" + "page_count": "2" } ) diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index 844ada741..99cc4fbe1 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -10,7 +10,7 @@ from app.letters.utils import ( copy_redaction_failed_pdf, get_bucket_name_and_prefix_for_notification, get_letter_pdf_filename, - get_letter_pdf, + get_letter_pdf_and_metadata, letter_print_day, upload_letter_pdf, ScanErrorType, move_failed_pdf, get_folder_name @@ -194,9 +194,9 @@ def test_get_letter_pdf_gets_pdf_from_correct_bucket( s3 = boto3.client('s3', region_name='eu-west-1') s3.put_object(Bucket=bucket_name, Key=filename, Body=b'pdf_content') - ret = get_letter_pdf(sample_precompiled_letter_notification_using_test_key) + file_data, metadata = get_letter_pdf_and_metadata(sample_precompiled_letter_notification_using_test_key) - assert ret == b'pdf_content' + assert file_data == b'pdf_content' @pytest.mark.parametrize('is_precompiled_letter,bucket_config_name', [ diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index ee6dbb29c..ecce789c5 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -1088,7 +1088,14 @@ def test_preview_letter_template_precompiled_pdf_file_type( content = b'\x00\x01' - mock_get_letter_pdf = mocker.patch('app.template.rest.get_letter_pdf', return_value=content) + mock_get_letter_pdf = mocker.patch( + 'app.template.rest.get_letter_pdf_and_metadata', + return_value=(content, { + "message": "", + "invalid_pages": "", + "page_count": "1" + }) + ) resp = admin_request.get( 'template.preview_letter_template_by_notification_id', @@ -1123,7 +1130,7 @@ def test_preview_letter_template_precompiled_s3_error( }): with requests_mock.Mocker(): - mocker.patch('app.template.rest.get_letter_pdf', + mocker.patch('app.template.rest.get_letter_pdf_and_metadata', side_effect=botocore.exceptions.ClientError( {'Error': {'Code': '403', 'Message': 'Unauthorized'}}, 'GetObject' @@ -1143,11 +1150,11 @@ def test_preview_letter_template_precompiled_s3_error( @pytest.mark.parametrize( - "filetype, post_url, overlay", + "filetype, post_url, message", [ - ('png', 'precompiled-preview.png', None), - ('png', 'precompiled/overlay.png?page_number=1', 1), - ('pdf', 'precompiled/overlay.pdf', 1) + ('png', 'precompiled-preview.png', ""), + ('png', 'precompiled/overlay.png?page_number=1', "content-outside-printable-area"), + ('pdf', 'precompiled/overlay.pdf', "content-outside-printable-area") ] ) def test_preview_letter_template_precompiled_png_file_type_or_pdf_with_overlay( @@ -1158,7 +1165,7 @@ def test_preview_letter_template_precompiled_png_file_type_or_pdf_with_overlay( mocker, filetype, post_url, - overlay + message ): template = create_template(sample_service, @@ -1178,7 +1185,16 @@ def test_preview_letter_template_precompiled_png_file_type_or_pdf_with_overlay( pdf_content = b'\x00\x01' expected_returned_content = b'\x00\x02' - mock_get_letter_pdf = mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) + metadata = { + "message": message, + "invalid_pages": "[1]", + "page_count": "1" + } + + mock_get_letter_pdf = mocker.patch( + 'app.template.rest.get_letter_pdf_and_metadata', + return_value=(pdf_content, metadata) + ) mocker.patch('app.template.rest.extract_page_from_pdf', return_value=pdf_content) @@ -1189,18 +1205,18 @@ def test_preview_letter_template_precompiled_png_file_type_or_pdf_with_overlay( status_code=200 ) - resp = admin_request.get( + response = admin_request.get( 'template.preview_letter_template_by_notification_id', service_id=notification.service_id, notification_id=notification.id, file_type=filetype, - overlay=overlay, ) with pytest.raises(ValueError): mock_post.last_request.json() assert mock_get_letter_pdf.called_once_with(notification) - assert base64.b64decode(resp['content']) == expected_returned_content + assert base64.b64decode(response['content']) == expected_returned_content + assert response["metadata"] == metadata @pytest.mark.parametrize('page_number,expect_preview_url', [ @@ -1234,7 +1250,14 @@ def test_preview_letter_template_precompiled_png_file_type_hide_notify_tag_only_ png_content = b'\x00\x02' encoded = base64.b64encode(png_content).decode('utf-8') - mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) + mocker.patch( + 'app.template.rest.get_letter_pdf_and_metadata', + return_value=(pdf_content, { + "message": "", + "invalid_pages": "", + "page_count": "2" + }) + ) mocker.patch('app.template.rest.extract_page_from_pdf', return_value=png_content) mock_get_png_preview = mocker.patch('app.template.rest._get_png_preview_or_overlaid_pdf', return_value=encoded) @@ -1276,7 +1299,11 @@ def test_preview_letter_template_precompiled_png_template_preview_500_error( pdf_content = b'\x00\x01' png_content = b'\x00\x02' - mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) + mocker.patch('app.template.rest.get_letter_pdf_and_metadata', return_value=(pdf_content, { + "message": "", + "invalid_pages": "", + "page_count": "1" + })) mocker.patch('app.template.rest.extract_page_from_pdf', return_value=pdf_content) @@ -1325,7 +1352,11 @@ def test_preview_letter_template_precompiled_png_template_preview_400_error( pdf_content = b'\x00\x01' png_content = b'\x00\x02' - mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) + mocker.patch('app.template.rest.get_letter_pdf_and_metadata', return_value=(pdf_content, { + "message": "", + "invalid_pages": "", + "page_count": "1" + })) mocker.patch('app.template.rest.extract_page_from_pdf', return_value=pdf_content) @@ -1373,7 +1404,11 @@ def test_preview_letter_template_precompiled_png_template_preview_pdf_error( pdf_content = b'\x00\x01' png_content = b'\x00\x02' - mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) + mocker.patch('app.template.rest.get_letter_pdf_and_metadata', return_value=(pdf_content, { + "message": "", + "invalid_pages": "", + "page_count": "1" + })) error_message = "PDF Error message" mocker.patch('app.template.rest.extract_page_from_pdf', side_effect=PdfReadError(error_message)) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index d38d95460..91f5bd752 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -656,7 +656,13 @@ def test_get_pdf_for_notification_returns_pdf_content( sample_letter_notification, mocker, ): - mock_get_letter_pdf = mocker.patch('app.v2.notifications.get_notifications.get_letter_pdf', return_value=b'foo') + mock_get_letter_pdf = mocker.patch( + 'app.v2.notifications.get_notifications.get_letter_pdf_and_metadata', return_value=(b'foo', { + "message": "", + "invalid_pages": "", + "page_count": "1" + }) + ) sample_letter_notification.status = 'created' auth_header = create_authorization_header(service_id=sample_letter_notification.service_id) @@ -677,8 +683,8 @@ def test_get_pdf_for_notification_returns_400_if_pdf_not_found( ): # if no files are returned get_letter_pdf throws StopIteration as the iterator runs out mock_get_letter_pdf = mocker.patch( - 'app.v2.notifications.get_notifications.get_letter_pdf', - side_effect=StopIteration + 'app.v2.notifications.get_notifications.get_letter_pdf_and_metadata', + side_effect=(StopIteration, {}) ) sample_letter_notification.status = 'created' @@ -707,7 +713,13 @@ def test_get_pdf_for_notification_only_returns_pdf_content_if_right_status( status, expected_message ): - mock_get_letter_pdf = mocker.patch('app.v2.notifications.get_notifications.get_letter_pdf', return_value=b'foo') + mock_get_letter_pdf = mocker.patch( + 'app.v2.notifications.get_notifications.get_letter_pdf_and_metadata', return_value=(b'foo', { + "message": "", + "invalid_pages": "", + "page_count": "1" + }) + ) sample_letter_notification.status = status auth_header = create_authorization_header(service_id=sample_letter_notification.service_id)