From e91a0efc43dd57e13277eb1cba8e815dcf15fbe4 Mon Sep 17 00:00:00 2001
From: Richard Chapman <richard.chapman@digital.cabinet-office.gov.uk>
Date: Mon, 5 Mar 2018 14:54:18 +0000
Subject: [PATCH] Refactored code to make it more maintainable and changed an
 error type

* Rather than an abort 404 returned a 500 and InvalidRequest so that the
error is more easily handled on the admin console. If the file is
missing but expected to be there is actually an internal error for admin
* Refactored the code to remove duplicate code in calls to template
preview by creating a new private method which is called with specific
parameters
---
 app/template/rest.py            | 52 +++++++++++++++------------------
 tests/app/template/test_rest.py |  2 +-
 2 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/app/template/rest.py b/app/template/rest.py
index 5f36557e0..e49eea762 100644
--- a/app/template/rest.py
+++ b/app/template/rest.py
@@ -7,7 +7,6 @@ from flask import (
     jsonify,
     request)
 from requests import post as requests_post
-from werkzeug.exceptions import abort
 
 
 from app.dao.notifications_dao import get_notification_by_id
@@ -207,7 +206,9 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file
             pdf_file = get_letter_pdf(notification)
 
         except botocore.exceptions.ClientError:
-            abort(404)
+            current_app.logger.info
+            raise InvalidRequest('Error getting letter file from S3 notification id {}'.format(notification_id),
+                                 status_code=500)
 
         content = base64.b64encode(pdf_file).decode('utf-8')
 
@@ -218,18 +219,7 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file
                 '?page={}'.format(page) if page else ''
             )
 
-            resp = requests_post(
-                url,
-                data=content,
-                headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])}
-            )
-
-            if resp.status_code != 200:
-                raise InvalidRequest(
-                    'Error generating preview for {}'.format(notification_id), status_code=500
-                )
-
-            content = base64.b64encode(resp.content).decode('utf-8')
+            content = _get_png_preview(url, content, notification.id)
 
     else:
 
@@ -249,21 +239,27 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file
             'dvla_org_id': service.dvla_organisation_id,
         }
 
-        resp = requests_post(
-            '{}/preview.{}{}'.format(
-                current_app.config['TEMPLATE_PREVIEW_API_HOST'],
-                file_type,
-                '?page={}'.format(page) if page else ''
-            ),
-            json=data,
-            headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])}
+        url = '{}/preview.{}{}'.format(
+            current_app.config['TEMPLATE_PREVIEW_API_HOST'],
+            file_type,
+            '?page={}'.format(page) if page else ''
         )
 
-        if resp.status_code != 200:
-            raise InvalidRequest(
-                'Error generating preview for {}'.format(notification_id), status_code=500
-            )
-
-        content = base64.b64encode(resp.content).decode('utf-8')
+        content = _get_png_preview(url, data, notification.id)
 
     return jsonify({"content": content})
+
+
+def _get_png_preview(url, data, notification_id):
+    resp = requests_post(
+        url,
+        data=data,
+        headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])}
+    )
+
+    if resp.status_code != 200:
+        raise InvalidRequest(
+            'Error generating preview for {}'.format(notification_id), status_code=500
+        )
+
+    return base64.b64encode(resp.content).decode('utf-8')
diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py
index 34192148d..cfd92f0d3 100644
--- a/tests/app/template/test_rest.py
+++ b/tests/app/template/test_rest.py
@@ -949,7 +949,7 @@ def test_preview_letter_template_precompiled_s3_error(
                 service_id=notification.service_id,
                 notification_id=notification.id,
                 file_type='pdf',
-                _expected_status=404
+                _expected_status=500
             )