From 79a6ce878222495ff2e53cd26f4e7ee619de3473 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Mon, 12 Mar 2018 11:05:05 +0000 Subject: [PATCH] * Updated imports to comply with pep8 for better maintainability * Removed extra log messages so there are not two log messages being generated per exception, as InvalidRequest also logs, updated the InvalidRequest log message to include the exception type and exception information * Added extra asserts to ensure the exception messages are printed --- app/template/rest.py | 38 ++++++++++++++------------------- tests/app/template/test_rest.py | 14 +++++++++--- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 960ce4e60..9db0bed57 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -9,9 +9,11 @@ from flask import ( jsonify, request) from notifications_utils.pdf import extract_page_from_pdf +from notifications_utils.template import SMSMessageTemplate from requests import post as requests_post from app.dao.notifications_dao import get_notification_by_id +from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import ( dao_update_template, dao_create_template, @@ -21,16 +23,14 @@ from app.dao.templates_dao import ( dao_get_template_versions, dao_update_template_reply_to, dao_get_template_by_id) -from notifications_utils.template import SMSMessageTemplate -from app.dao.services_dao import dao_fetch_service_by_id -from app.letters.utils import get_letter_pdf -from app.models import SMS_TYPE -from app.notifications.validators import service_has_permission, check_reply_to -from app.schemas import (template_schema, template_history_schema) from app.errors import ( register_errors, InvalidRequest ) +from app.letters.utils import get_letter_pdf +from app.models import SMS_TYPE +from app.notifications.validators import service_has_permission, check_reply_to +from app.schemas import (template_schema, template_history_schema) from app.utils import get_template_instance, get_public_notify_type_text template_blueprint = Blueprint('template', __name__, url_prefix='/service//template') @@ -205,11 +205,12 @@ 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.exception( - '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) + except botocore.exceptions.ClientError as e: + raise InvalidRequest( + 'Error extracting requested page from PDF file for notification_id {} type {} {}'.format( + notification_id, type(e), e), + status_code=500 + ) content = base64.b64encode(pdf_file).decode('utf-8') @@ -219,11 +220,10 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file page_number = page if page else "0" pdf_page = extract_page_from_pdf(BytesIO(pdf_file), int(page_number) - 1) content = base64.b64encode(pdf_page).decode('utf-8') - except PdfReadError: - current_app.logger.exception( - 'Error extracting requested page from PDF file for notification_id {}'.format(notification_id)) + except PdfReadError as e: raise InvalidRequest( - 'Error extracting requested page from PDF file for notification_id {}'.format(notification_id), + 'Error extracting requested page from PDF file for notification_id {} type {} {}'.format( + notification_id, type(e), e), status_code=500 ) @@ -277,14 +277,8 @@ def _get_png_preview(url, data, notification_id, json=True): ) 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 letter for {}\nStatus code: {}\n{}'.format( + 'Error generating preview letter for {} Status code: {} {}'.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 41d0c0b7f..14145014e 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -946,7 +946,7 @@ def test_preview_letter_template_precompiled_s3_error( 'GetObject' )) - admin_request.get( + request = admin_request.get( 'template.preview_letter_template_by_notification_id', service_id=notification.service_id, notification_id=notification.id, @@ -954,6 +954,10 @@ def test_preview_letter_template_precompiled_s3_error( _expected_status=500 ) + assert request['message'] == "Error extracting requested page from PDF file for notification_id {} type " \ + " An error occurred (403) " \ + "when calling the GetObject operation: Unauthorized".format(notification.id) + def test_preview_letter_template_precompiled_png_file_type( notify_api, @@ -1128,7 +1132,8 @@ def test_preview_letter_template_precompiled_png_template_preview_pdf_error( mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) - mocker.patch('app.template.rest.extract_page_from_pdf', side_effect=PdfReadError()) + error_message = "PDF Error message" + mocker.patch('app.template.rest.extract_page_from_pdf', side_effect=PdfReadError(error_message)) request_mock.post( 'http://localhost/notifications-template-preview/precompiled-preview.png', @@ -1137,10 +1142,13 @@ def test_preview_letter_template_precompiled_png_template_preview_pdf_error( status_code=404 ) - admin_request.get( + request = admin_request.get( 'template.preview_letter_template_by_notification_id', service_id=notification.service_id, notification_id=notification.id, file_type='png', _expected_status=500 ) + + assert request['message'] == "Error extracting requested page from PDF file for notification_id {} type " \ + "{} {}".format(notification.id, type(PdfReadError()), error_message)