From 1ad32c91683934bb8383956ff88ccdcd287803a1 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 12 Sep 2019 17:06:22 +0100 Subject: [PATCH] move get_pdf functionality to its own endpoint it felt very awkward when the body of a pdf might be empty, might have things in it, and whether it is empty or not can change even when the status is the same (a created template notification might have a pdf, but might not, we don't know). So move it to its own endpoint, so we can hand craft some 400 errors that appropriately explain what's going on. --- app/v2/notifications/get_notifications.py | 55 +++++++++--- .../notifications/test_get_notifications.py | 87 +++++++++++++++---- 2 files changed, 111 insertions(+), 31 deletions(-) diff --git a/app/v2/notifications/get_notifications.py b/app/v2/notifications/get_notifications.py index 00e2adff0..12156e7f9 100644 --- a/app/v2/notifications/get_notifications.py +++ b/app/v2/notifications/get_notifications.py @@ -1,14 +1,20 @@ -import base64 +from io import BytesIO -from flask import jsonify, request, url_for, current_app +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.schema_validation import validate +from app.v2.errors import BadRequestError from app.v2.notifications import v2_notification_blueprint from app.v2.notifications.notification_schemas import get_notifications_request, notification_by_id -from app.models import NOTIFICATION_CREATED, NOTIFICATION_STATUS_TYPES_BILLABLE_FOR_LETTERS, LETTER_TYPE +from app.models import ( + NOTIFICATION_PENDING_VIRUS_CHECK, + NOTIFICATION_VIRUS_SCAN_FAILED, + NOTIFICATION_TECHNICAL_FAILURE, + LETTER_TYPE, +) @v2_notification_blueprint.route("/", methods=['GET']) @@ -18,18 +24,43 @@ def get_notification_by_id(notification_id): notification = notifications_dao.get_notification_with_personalisation( authenticated_service.id, notification_id, key_type=None ) + return jsonify(notification.serialize()), 200 - response = notification.serialize() - if request.args.get('return_pdf_content') and notification.notification_type == LETTER_TYPE: - if notification.status in (NOTIFICATION_CREATED, *NOTIFICATION_STATUS_TYPES_BILLABLE_FOR_LETTERS): - pdf_data = get_letter_pdf(notification) - else: - # precompiled letters that are still being virus scanned, or that failed validation/virus scan - pdf_data = b'' - response['body'] = base64.b64encode(pdf_data).decode('utf-8') +@v2_notification_blueprint.route('//pdf', methods=['GET']) +def get_pdf_for_notification(notification_id): + _data = {"notification_id": notification_id} + validate(_data, notification_by_id) + notification = notifications_dao.get_notification_by_id( + notification_id, authenticated_service.id, _raise=True + ) - return jsonify(response), 200 + if notification.notification_type != LETTER_TYPE: + raise BadRequestError(message="Notification is not a letter", status_code=400) + + if notification.status in { + NOTIFICATION_PENDING_VIRUS_CHECK, + NOTIFICATION_VIRUS_SCAN_FAILED, + NOTIFICATION_TECHNICAL_FAILURE, + }: + raise BadRequestError( + message='PDF not available for letters in status {}'.format(notification.status), + status_code=400 + ) + + try: + pdf_data = get_letter_pdf(notification) + except Exception: + # this probably means it's a templated letter that hasn't been created yet + current_app.logger.info('PDF not found for notification id {} status {}'.format( + notification.id, + notification.status + )) + raise BadRequestError( + message='PDF not available for letter, try again later', + status_code=400) + + return send_file(filename_or_fp=BytesIO(pdf_data), mimetype='application/pdf') @v2_notification_blueprint.route("", methods=['GET']) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 699f0fb0b..d2b126493 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -651,36 +651,85 @@ def test_get_notifications_renames_letter_statuses(client, sample_letter_templat assert json_response['status'] == expected_status -@pytest.mark.parametrize('status,expected_body,mock_called', [ - ('created', 'Zm9v', True), - ('sending', 'Zm9v', True), - ('pending-virus-check', '', False), - ('virus-scan-failed', '', False), - ('validation-failed', '', False), - ('technical-failure', '', False), +def test_get_pdf_for_notification_returns_pdf_content( + client, + sample_letter_notification, + mocker, +): + mock_get_letter_pdf = mocker.patch('app.v2.notifications.get_notifications.get_letter_pdf', return_value=b'foo') + sample_letter_notification.status = 'created' + + auth_header = create_authorization_header(service_id=sample_letter_notification.service_id) + response = client.get( + path=url_for('v2_notifications.get_pdf_for_notification', notification_id=sample_letter_notification.id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response.status_code == 200 + assert response.get_data() == b'foo' + mock_get_letter_pdf.assert_called_once_with(sample_letter_notification) + + +def test_get_pdf_for_notification_returns_400_if_pdf_not_found( + client, + sample_letter_notification, + mocker, +): + # 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 + ) + sample_letter_notification.status = 'created' + + auth_header = create_authorization_header(service_id=sample_letter_notification.service_id) + response = client.get( + path=url_for('v2_notifications.get_pdf_for_notification', notification_id=sample_letter_notification.id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response.status_code == 400 + assert response.json['errors'] == [{ + 'error': 'BadRequestError', + 'message': 'PDF not available for letter, try again later' + }] + mock_get_letter_pdf.assert_called_once_with(sample_letter_notification) + + +@pytest.mark.parametrize('status', [ + 'pending-virus-check', + 'virus-scan-failed', + 'technical-failure', ]) -def test_get_notification_only_returns_pdf_content_if_right_status( +def test_get_pdf_for_notification_only_returns_pdf_content_if_right_status( client, sample_letter_notification, mocker, status, - expected_body, - mock_called ): mock_get_letter_pdf = mocker.patch('app.v2.notifications.get_notifications.get_letter_pdf', return_value=b'foo') sample_letter_notification.status = status auth_header = create_authorization_header(service_id=sample_letter_notification.service_id) response = client.get( - path=url_for( - 'v2_notifications.get_notification_by_id', - notification_id=sample_letter_notification.id, - return_pdf_content='true' - ), + path=url_for('v2_notifications.get_pdf_for_notification', notification_id=sample_letter_notification.id), headers=[('Content-Type', 'application/json'), auth_header] ) - json_response = json.loads(response.get_data(as_text=True)) - assert response.status_code == 200 - assert json_response['body'] == expected_body - assert mock_get_letter_pdf.called == mock_called + assert response.status_code == 400 + assert response.json['errors'] == [{ + 'error': 'BadRequestError', + 'message': 'PDF not available for letters in status {}'.format(status) + }] + assert mock_get_letter_pdf.called is False + + +def test_get_pdf_for_notification_fails_for_non_letters(client, sample_notification): + auth_header = create_authorization_header(service_id=sample_notification.service_id) + response = client.get( + path=url_for('v2_notifications.get_pdf_for_notification', notification_id=sample_notification.id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response.status_code == 400 + assert response.json['errors'] == [{'error': 'BadRequestError', 'message': 'Notification is not a letter'}]