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

@@ -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("/<notification_id>", 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:
+@v2_notification_blueprint.route('/<notification_id>/pdf', methods=['GET'])
-        if notification.status in (NOTIFICATION_CREATED, *NOTIFICATION_STATUS_TYPES_BILLABLE_FOR_LETTERS):
+def get_pdf_for_notification(notification_id):
-            pdf_data = get_letter_pdf(notification)
+    _data = {"notification_id": notification_id}
-        else:
+    validate(_data, notification_by_id)
-            # precompiled letters that are still being virus scanned, or that failed validation/virus scan
+    notification = notifications_dao.get_notification_by_id(
-            pdf_data = b''
+        notification_id, authenticated_service.id, _raise=True
-        response['body'] = base64.b64encode(pdf_data).decode('utf-8')
+    )
 
-    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'])

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', [
+def test_get_pdf_for_notification_returns_pdf_content(
-    ('created', 'Zm9v', True),
+    client,
-    ('sending', 'Zm9v', True),
+    sample_letter_notification,
-    ('pending-virus-check', '', False),
+    mocker,
-    ('virus-scan-failed', '', False),
+):
-    ('validation-failed', '', False),
+    mock_get_letter_pdf = mocker.patch('app.v2.notifications.get_notifications.get_letter_pdf', return_value=b'foo')
-    ('technical-failure', '', False),
+    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(
+        path=url_for('v2_notifications.get_pdf_for_notification', notification_id=sample_letter_notification.id),
-            'v2_notifications.get_notification_by_id',
-            notification_id=sample_letter_notification.id,
-            return_pdf_content='true'
-        ),
         headers=[('Content-Type', 'application/json'), auth_header]
     )
 
-    json_response = json.loads(response.get_data(as_text=True))
+    assert response.status_code == 400
-    assert response.status_code == 200
+    assert response.json['errors'] == [{
-    assert json_response['body'] == expected_body
+        'error': 'BadRequestError',
-    assert mock_get_letter_pdf.called == mock_called
+        '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'}]