From d168dd106ef1b5d7fbe1c5c20e7e99dce8f0bf27 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 9 Mar 2020 13:35:00 +0000 Subject: [PATCH 1/4] Add uncovered test cases We want to know that the overlay is added regardless of which page number you are requesting when a pdf contains pages that go beyond the print area --- tests/app/template/test_rest.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index c8fef9766..286684afd 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -1143,11 +1143,13 @@ def test_preview_letter_template_precompiled_s3_error( @pytest.mark.parametrize( - "filetype, post_url, message", + "filetype, post_url, message, requested_page", [ - ('png', 'precompiled-preview.png', ""), - ('png', 'precompiled/overlay.png?page_number=1', "content-outside-printable-area"), - ('pdf', 'precompiled/overlay.pdf', "content-outside-printable-area") + ('png', 'precompiled-preview.png', "", ""), + ('png', 'precompiled/overlay.png?page_number=1', "content-outside-printable-area", "1"), + ('png', 'precompiled/overlay.png?page_number=2', "content-outside-printable-area", "2"), + ('png', 'precompiled/overlay.png?page_number=3', "content-outside-printable-area", "3"), + ('pdf', 'precompiled/overlay.pdf', "content-outside-printable-area", "") ] ) def test_preview_letter_template_precompiled_png_file_type_or_pdf_with_overlay( @@ -1158,7 +1160,8 @@ def test_preview_letter_template_precompiled_png_file_type_or_pdf_with_overlay( mocker, filetype, post_url, - message + message, + requested_page, ): template = create_template(sample_service, @@ -1180,8 +1183,8 @@ def test_preview_letter_template_precompiled_png_file_type_or_pdf_with_overlay( metadata = { "message": message, - "invalid_pages": "[1]", - "page_count": "1" + "invalid_pages": "[1,3]", + "page_count": "4" } mock_get_letter_pdf = mocker.patch( @@ -1194,12 +1197,13 @@ def test_preview_letter_template_precompiled_png_file_type_or_pdf_with_overlay( mock_post = request_mock.post( 'http://localhost/notifications-template-preview/{}'.format(post_url), content=expected_returned_content, - headers={'X-pdf-page-count': '1'}, + headers={'X-pdf-page-count': '4'}, status_code=200 ) response = admin_request.get( 'template.preview_letter_template_by_notification_id', + page=requested_page, service_id=notification.service_id, notification_id=notification.id, file_type=filetype, From 9eda30f4fdcf81fbac84ecce76682616b50ae47c Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 9 Mar 2020 13:42:07 +0000 Subject: [PATCH 2/4] Refactor tests to be a bit more readable Make arguments more verbose and have pytest params as inputs at the start and expected things at the end --- tests/app/template/test_rest.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 286684afd..e9f6ea04d 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -1143,13 +1143,13 @@ def test_preview_letter_template_precompiled_s3_error( @pytest.mark.parametrize( - "filetype, post_url, message, requested_page", + "requested_page, filetype, message, expected_post_url", [ - ('png', 'precompiled-preview.png', "", ""), - ('png', 'precompiled/overlay.png?page_number=1', "content-outside-printable-area", "1"), - ('png', 'precompiled/overlay.png?page_number=2', "content-outside-printable-area", "2"), - ('png', 'precompiled/overlay.png?page_number=3', "content-outside-printable-area", "3"), - ('pdf', 'precompiled/overlay.pdf', "content-outside-printable-area", "") + ("", 'png', "", 'precompiled-preview.png'), + ("1", 'png', "content-outside-printable-area", 'precompiled/overlay.png?page_number=1'), + ("2", 'png', "content-outside-printable-area", 'precompiled/overlay.png?page_number=2'), + ("3", 'png', "content-outside-printable-area", 'precompiled/overlay.png?page_number=3'), + ("", 'pdf', "content-outside-printable-area", 'precompiled/overlay.pdf') ] ) def test_preview_letter_template_precompiled_png_file_type_or_pdf_with_overlay( @@ -1158,10 +1158,10 @@ def test_preview_letter_template_precompiled_png_file_type_or_pdf_with_overlay( admin_request, sample_service, mocker, - filetype, - post_url, - message, requested_page, + filetype, + message, + expected_post_url, ): template = create_template(sample_service, @@ -1195,7 +1195,7 @@ def test_preview_letter_template_precompiled_png_file_type_or_pdf_with_overlay( mocker.patch('app.template.rest.extract_page_from_pdf', return_value=pdf_content) mock_post = request_mock.post( - 'http://localhost/notifications-template-preview/{}'.format(post_url), + 'http://localhost/notifications-template-preview/{}'.format(expected_post_url), content=expected_returned_content, headers={'X-pdf-page-count': '4'}, status_code=200 From d9243b8b8a3bcf4b9670a5315acc86ab225ebbc0 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 9 Mar 2020 13:43:06 +0000 Subject: [PATCH 3/4] When requesting a png for a single page, show overlay as appropriate If the png page is invalid then show the overlay for that page If the png page is not invalid, even if other pages are invalid, then don't shown the overlay for that page These keeps the behaviour were if you get the pdf for a pdf which has invalid pages then the whole PDF is requested (not just a single page) and all of the pages include the overlay Tests have been improved to be more explicit about what we are testing --- app/template/rest.py | 12 +++-- tests/app/template/test_rest.py | 89 +++++++++++++++++++++++++++++---- 2 files changed, 88 insertions(+), 13 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 61f07d105..e8fba9fab 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -245,11 +245,15 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file status_code=500 ) - content = base64.b64encode(pdf_file).decode('utf-8') - overlay = metadata.get("message") == "content-outside-printable-area" page_number = page if page else "1" + content = base64.b64encode(pdf_file).decode('utf-8') + content_outside_printable_area = metadata.get("message") == "content-outside-printable-area" - if overlay: + show_overlay_for_page = False + if content_outside_printable_area and page_number in metadata.get('invalid_pages', '[]'): + show_overlay_for_page = True + + if show_overlay_for_page or (content_outside_printable_area and file_type == "pdf"): path = '/precompiled/overlay.{}'.format(file_type) query_string = '?page_number={}'.format(page_number) if file_type == 'png' else '' content = pdf_file @@ -262,7 +266,7 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file if file_type == 'png': try: pdf_page = extract_page_from_pdf(BytesIO(pdf_file), int(page_number) - 1) - content = pdf_page if overlay else base64.b64encode(pdf_page).decode('utf-8') + content = pdf_page if show_overlay_for_page else base64.b64encode(pdf_page).decode('utf-8') except PdfReadError as e: raise InvalidRequest( 'Error extracting requested page from PDF file for notification_id {} type {} {}'.format( diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index e9f6ea04d..ffe660cea 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -1143,23 +1143,27 @@ def test_preview_letter_template_precompiled_s3_error( @pytest.mark.parametrize( - "requested_page, filetype, message, expected_post_url", + "requested_page, message, expected_post_url", [ - ("", 'png', "", 'precompiled-preview.png'), - ("1", 'png', "content-outside-printable-area", 'precompiled/overlay.png?page_number=1'), - ("2", 'png', "content-outside-printable-area", 'precompiled/overlay.png?page_number=2'), - ("3", 'png', "content-outside-printable-area", 'precompiled/overlay.png?page_number=3'), - ("", 'pdf', "content-outside-printable-area", 'precompiled/overlay.pdf') + # page defaults to 1, page is valid, no overlay shown + ("", "", 'precompiled-preview.png'), + # page is valid, no overlay shown + ("1", "", 'precompiled-preview.png'), + # page is invalid, overlay shown + ("1", "content-outside-printable-area", 'precompiled/overlay.png?page_number=1'), + # page is valid, no overlay shown + ("2", "content-outside-printable-area", 'precompiled-preview.png'), + # page is invalid, overlay shown + ("3", "content-outside-printable-area", 'precompiled/overlay.png?page_number=3'), ] ) -def test_preview_letter_template_precompiled_png_file_type_or_pdf_with_overlay( +def test_preview_letter_template_precompiled_for_png_file_shows_overlay_where_appropriate( notify_api, client, admin_request, sample_service, mocker, requested_page, - filetype, message, expected_post_url, ): @@ -1206,7 +1210,74 @@ def test_preview_letter_template_precompiled_png_file_type_or_pdf_with_overlay( page=requested_page, service_id=notification.service_id, notification_id=notification.id, - file_type=filetype, + file_type="png", + ) + + with pytest.raises(ValueError): + mock_post.last_request.json() + assert mock_get_letter_pdf.called_once_with(notification) + assert base64.b64decode(response['content']) == expected_returned_content + assert response["metadata"] == metadata + + +@pytest.mark.parametrize( + "invalid_pages", + [ + "[1,3]", + "[2,4]", # it shouldn't make a difference if the error was on the first page or not + ] +) +def test_preview_letter_template_precompiled_for_pdf_file_shows_overlay_if_content_outside_printable_area( + notify_api, + client, + admin_request, + sample_service, + mocker, + invalid_pages, +): + + template = create_template(sample_service, + template_type='letter', + template_name='Pre-compiled PDF', + subject='Pre-compiled PDF', + hidden=True) + + notification = create_notification(template) + + with set_config_values(notify_api, { + 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', + 'TEMPLATE_PREVIEW_API_KEY': 'test-key' + }): + with requests_mock.Mocker() as request_mock: + + pdf_content = b'\x00\x01' + expected_returned_content = b'\x00\x02' + + metadata = { + "message": "content-outside-printable-area", + "invalid_pages": invalid_pages, + "page_count": "4" + } + + 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) + + mock_post = request_mock.post( + 'http://localhost/notifications-template-preview/precompiled/overlay.pdf', + content=expected_returned_content, + headers={'X-pdf-page-count': '4'}, + status_code=200 + ) + + response = admin_request.get( + 'template.preview_letter_template_by_notification_id', + service_id=notification.service_id, + notification_id=notification.id, + file_type="pdf", ) with pytest.raises(ValueError): From 34d571405ebce02aef435bb6c1ec57837f28c9bf Mon Sep 17 00:00:00 2001 From: David McDonald Date: Tue, 10 Mar 2020 13:38:20 +0000 Subject: [PATCH 4/4] Refactor of logic to make code more readable Functionality remains as it was --- app/template/rest.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index e8fba9fab..411cd6c82 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -248,12 +248,9 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file page_number = page if page else "1" content = base64.b64encode(pdf_file).decode('utf-8') content_outside_printable_area = metadata.get("message") == "content-outside-printable-area" + page_is_in_invalid_pages = page_number in metadata.get('invalid_pages', '[]') - show_overlay_for_page = False - if content_outside_printable_area and page_number in metadata.get('invalid_pages', '[]'): - show_overlay_for_page = True - - if show_overlay_for_page or (content_outside_printable_area and file_type == "pdf"): + if content_outside_printable_area and (file_type == "pdf" or page_is_in_invalid_pages): path = '/precompiled/overlay.{}'.format(file_type) query_string = '?page_number={}'.format(page_number) if file_type == 'png' else '' content = pdf_file @@ -266,7 +263,7 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file if file_type == 'png': try: pdf_page = extract_page_from_pdf(BytesIO(pdf_file), int(page_number) - 1) - content = pdf_page if show_overlay_for_page else base64.b64encode(pdf_page).decode('utf-8') + content = pdf_page if page_is_in_invalid_pages else base64.b64encode(pdf_page).decode('utf-8') except PdfReadError as e: raise InvalidRequest( 'Error extracting requested page from PDF file for notification_id {} type {} {}'.format(