From d0df85a6020d55d86bc5e6bec1c161ef9c97ee06 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 6 Mar 2018 14:24:30 +0000 Subject: [PATCH 1/8] Fixed bug where the content header was not being passed onto the post request. Changed data => json. Added extra logging to display the error with more detail --- app/template/rest.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index e49eea762..22f3295c9 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -205,8 +205,9 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file pdf_file = get_letter_pdf(notification) - except botocore.exceptions.ClientError: - current_app.logger.info + except botocore.exceptions.ClientError as e: + current_app.logger.exception( + 'Error getting letter file from S3 notification id {}'.format(notification_id), e) raise InvalidRequest('Error getting letter file from S3 notification id {}'.format(notification_id), status_code=500) @@ -253,13 +254,23 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file def _get_png_preview(url, data, notification_id): resp = requests_post( url, - data=data, + json=data, headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} ) if resp.status_code != 200: + current_app.logger.exception( + 'Error generating preview letter for {} \nStatus code: {}\n{}'.format( + notification_id, + resp.status_code, + resp.content + )) raise InvalidRequest( - 'Error generating preview for {}'.format(notification_id), status_code=500 + 'Error generating preview letter for {} \nStatus code: {}\n{}'.format( + notification_id, + resp.status_code, + resp.content + ), status_code=500 ) return base64.b64encode(resp.content).decode('utf-8') From ed9936bba03dd1a3014644489e9e10285870c7f6 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 6 Mar 2018 14:42:53 +0000 Subject: [PATCH 2/8] Fixed bug where the content header was not being passed onto the post request. Changed data => json. Added extra logging to display the error with more detail --- app/template/rest.py | 2 +- tests/app/template/test_rest.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 22f3295c9..c938b2ea6 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -266,7 +266,7 @@ def _get_png_preview(url, data, notification_id): resp.content )) raise InvalidRequest( - 'Error generating preview letter for {} \nStatus code: {}\n{}'.format( + 'Error generating preview letter for {}\nStatus code: {}\n{}'.format( notification_id, resp.status_code, resp.content diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 14e0aba55..99889e238 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -874,7 +874,9 @@ def test_preview_letter_template_by_id_template_preview_500( _expected_status=500 ) - assert resp['message'] == 'Error generating preview for {}'.format(sample_letter_notification.id) + assert 'Status code: 404' in resp['message'] + assert 'Error generating preview letter for {}'.format(sample_letter_notification.id) in resp['message'] + def test_preview_letter_template_precompiled_pdf_file_type( From 77a3397ce505ffd13258a69e71934bc4c2edef22 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 6 Mar 2018 15:35:00 +0000 Subject: [PATCH 3/8] Added option flag to the _get_png_preview method to determine if the post method content is data or json format. --- app/template/rest.py | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index c938b2ea6..024a2c708 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -8,7 +8,6 @@ from flask import ( request) from requests import post as requests_post - from app.dao.notifications_dao import get_notification_by_id from app.dao.templates_dao import ( dao_update_template, @@ -33,7 +32,6 @@ from app.utils import get_template_instance, get_public_notify_type_text template_blueprint = Blueprint('template', __name__, url_prefix='/service//template') - register_errors(template_blueprint) @@ -189,7 +187,6 @@ def redact_template(template, data): @template_blueprint.route('/preview//', methods=['GET']) def preview_letter_template_by_notification_id(service_id, notification_id, file_type): - if file_type not in ('pdf', 'png'): raise InvalidRequest({'content': ["file_type must be pdf or png"]}, status_code=400) @@ -214,13 +211,12 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file content = base64.b64encode(pdf_file).decode('utf-8') if file_type == 'png': - url = '{}/precompiled-preview.png{}'.format( current_app.config['TEMPLATE_PREVIEW_API_HOST'], '?page={}'.format(page) if page else '' ) - content = _get_png_preview(url, content, notification.id) + content = _get_png_preview(url, content, notification.id, json=False) else: @@ -246,17 +242,24 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file '?page={}'.format(page) if page else '' ) - content = _get_png_preview(url, data, notification.id) + content = _get_png_preview(url, data, notification.id, json=True) return jsonify({"content": content}) -def _get_png_preview(url, data, notification_id): - resp = requests_post( - url, - json=data, - headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} - ) +def _get_png_preview(url, data, notification_id, json=True): + if json: + resp = requests_post( + url, + json=data, + headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} + ) + else: + resp = requests_post( + url, + data=data, + headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} + ) if resp.status_code != 200: current_app.logger.exception( From e1e7f13f2365298ba2dbe675fdcdd335a985c665 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 6 Mar 2018 16:16:29 +0000 Subject: [PATCH 4/8] Added tests to ensure the parameter to the mock post request are the correct type for the call as there are some that require json and some binary. The additional checks ensure that that json decode either fails or succeeds in the correct case. --- tests/app/template/test_rest.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 99889e238..6dac7e084 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -3,6 +3,7 @@ import json import random import string from datetime import datetime, timedelta +from json import JSONDecodeError import botocore import pytest @@ -828,7 +829,7 @@ def test_preview_letter_template_by_id_valid_file_type( with requests_mock.Mocker() as request_mock: content = b'\x00\x01' - request_mock.post( + mock_post = request_mock.post( 'http://localhost/notifications-template-preview/preview.pdf', content=content, headers={'X-pdf-page-count': '1'}, @@ -842,6 +843,7 @@ def test_preview_letter_template_by_id_valid_file_type( file_type='pdf' ) + assert mock_post.last_request.json() assert base64.b64decode(resp['content']) == content @@ -859,7 +861,7 @@ def test_preview_letter_template_by_id_template_preview_500( with requests_mock.Mocker() as request_mock: content = b'\x00\x01' - request_mock.post( + mock_post = request_mock.post( 'http://localhost/notifications-template-preview/preview.pdf', content=content, headers={'X-pdf-page-count': '1'}, @@ -874,11 +876,11 @@ def test_preview_letter_template_by_id_template_preview_500( _expected_status=500 ) + assert mock_post.last_request.json() assert 'Status code: 404' in resp['message'] assert 'Error generating preview letter for {}'.format(sample_letter_notification.id) in resp['message'] - def test_preview_letter_template_precompiled_pdf_file_type( notify_api, client, @@ -980,7 +982,7 @@ def test_preview_letter_template_precompiled_png_file_type( mock_get_letter_pdf = mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) - request_mock.post( + mock_post = request_mock.post( 'http://localhost/notifications-template-preview/precompiled-preview.png', content=png_content, headers={'X-pdf-page-count': '1'}, @@ -994,6 +996,8 @@ def test_preview_letter_template_precompiled_png_file_type( file_type='png' ) + with pytest.raises(JSONDecodeError): + mock_post.last_request.json() assert mock_get_letter_pdf.called_once_with(notification) assert base64.b64decode(resp['content']) == png_content @@ -1025,7 +1029,7 @@ def test_preview_letter_template_precompiled_png_template_preview_500_error( mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) - request_mock.post( + mock_post = request_mock.post( 'http://localhost/notifications-template-preview/precompiled-preview.png', content=png_content, headers={'X-pdf-page-count': '1'}, @@ -1041,6 +1045,9 @@ def test_preview_letter_template_precompiled_png_template_preview_500_error( ) + with pytest.raises(JSONDecodeError): + mock_post.last_request.json() + def test_preview_letter_template_precompiled_png_template_preview_400_error( notify_api, @@ -1069,7 +1076,7 @@ def test_preview_letter_template_precompiled_png_template_preview_400_error( mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) - request_mock.post( + mock_post = request_mock.post( 'http://localhost/notifications-template-preview/precompiled-preview.png', content=png_content, headers={'X-pdf-page-count': '1'}, @@ -1082,5 +1089,7 @@ def test_preview_letter_template_precompiled_png_template_preview_400_error( notification_id=notification.id, file_type='png', _expected_status=500 - ) + + with pytest.raises(JSONDecodeError): + mock_post.last_request.json() From 3f9a37d8a3d22ac54df577b33546d82244e19df7 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 6 Mar 2018 17:31:08 +0000 Subject: [PATCH 5/8] Updated the import for JSONDecodeError to import from from json.decoder as it was failing on Jenkins. --- tests/app/template/test_rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 6dac7e084..099a887c7 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -3,7 +3,7 @@ import json import random import string from datetime import datetime, timedelta -from json import JSONDecodeError +from json.decoder import JSONDecodeError import botocore import pytest From cf2a506b3bb6c768bbebc9e61d7aa61123d1085d Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 6 Mar 2018 18:20:45 +0000 Subject: [PATCH 6/8] Removed the import and directly used json.decoder.JSONDecodeError as it was failing on Jenkins. --- tests/app/template/test_rest.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 099a887c7..c84a86525 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -3,7 +3,6 @@ import json import random import string from datetime import datetime, timedelta -from json.decoder import JSONDecodeError import botocore import pytest @@ -996,7 +995,7 @@ def test_preview_letter_template_precompiled_png_file_type( file_type='png' ) - with pytest.raises(JSONDecodeError): + with pytest.raises(json.decoder.JSONDecodeError): mock_post.last_request.json() assert mock_get_letter_pdf.called_once_with(notification) assert base64.b64decode(resp['content']) == png_content @@ -1045,7 +1044,7 @@ def test_preview_letter_template_precompiled_png_template_preview_500_error( ) - with pytest.raises(JSONDecodeError): + with pytest.raises(json.decoder.JSONDecodeError): mock_post.last_request.json() @@ -1091,5 +1090,5 @@ def test_preview_letter_template_precompiled_png_template_preview_400_error( _expected_status=500 ) - with pytest.raises(JSONDecodeError): + with pytest.raises(json.decoder.JSONDecodeError): mock_post.last_request.json() From 7f32b066154092af007a185f2eb8810289d123fa Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Wed, 7 Mar 2018 09:32:56 +0000 Subject: [PATCH 7/8] Replaced the JSONDecodeError with a ValueError as it was failing on Jenkins. --- tests/app/template/test_rest.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index c84a86525..6611c772d 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -995,7 +995,7 @@ def test_preview_letter_template_precompiled_png_file_type( file_type='png' ) - with pytest.raises(json.decoder.JSONDecodeError): + with pytest.raises(ValueError): mock_post.last_request.json() assert mock_get_letter_pdf.called_once_with(notification) assert base64.b64decode(resp['content']) == png_content @@ -1044,7 +1044,7 @@ def test_preview_letter_template_precompiled_png_template_preview_500_error( ) - with pytest.raises(json.decoder.JSONDecodeError): + with pytest.raises(ValueError): mock_post.last_request.json() @@ -1090,5 +1090,5 @@ def test_preview_letter_template_precompiled_png_template_preview_400_error( _expected_status=500 ) - with pytest.raises(json.decoder.JSONDecodeError): + with pytest.raises(ValueError): mock_post.last_request.json() From d60e802f3568b1e384d7781379028f5a308145ba Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Wed, 7 Mar 2018 09:51:58 +0000 Subject: [PATCH 8/8] Removed the superfluous variable and pass through as it pulls it automatically out of sys.exc_info. --- app/template/rest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 024a2c708..a4bc1b4a9 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -202,9 +202,9 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file pdf_file = get_letter_pdf(notification) - except botocore.exceptions.ClientError as e: + except botocore.exceptions.ClientError: current_app.logger.exception( - 'Error getting letter file from S3 notification id {}'.format(notification_id), e) + 'Error getting letter file from S3 notification id {}'.format(notification_id)) raise InvalidRequest('Error getting letter file from S3 notification id {}'.format(notification_id), status_code=500)