Merge pull request #2631 from alphagov/add-validation-message-to-s3-as-metadata

Put pdf letter validation failed message in S3 metadata

app/celery/letters_pdf_tasks.py

@@ -5,7 +5,6 @@ from uuid import UUID
 from hashlib import sha512
 from base64 import urlsafe_b64encode
 
-from PyPDF2.utils import PdfReadError
 from botocore.exceptions import ClientError as BotoClientError
 from flask import current_app
 from requests import (
@@ -30,6 +29,7 @@ from app.dao.notifications_dao import (
 from app.errors import VirusScanError
 from app.letters.utils import (
     copy_redaction_failed_pdf,
+    get_billable_units_for_letter_page_count,
     get_reference_from_filename,
     get_folder_name,
     upload_letter_pdf,
@@ -38,7 +38,6 @@ from app.letters.utils import (
     move_scan_to_invalid_pdf_bucket,
     move_error_pdf_to_scan_bucket,
     get_file_names_from_error_bucket,
-    get_page_count,
 )
 from app.models import (
     KEY_TYPE_TEST,
@@ -49,7 +48,6 @@ from app.models import (
     NOTIFICATION_VIRUS_SCAN_FAILED,
 )
 from app.cronitor import cronitor
-from json import JSONDecodeError
 
 
 @notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300)
@@ -210,22 +208,10 @@ def process_virus_scan_passed(self, filename):
     scan_pdf_object = s3.get_s3_object(current_app.config['LETTERS_SCAN_BUCKET_NAME'], filename)
     old_pdf = scan_pdf_object.get()['Body'].read()
 
-    try:
-        billable_units = get_page_count(old_pdf)
-    except PdfReadError:
-        current_app.logger.exception(msg='Invalid PDF received for notification_id: {}'.format(notification.id))
-        _move_invalid_letter_and_update_status(notification, filename, scan_pdf_object)
-        return
-
-    sanitise_response = _sanitise_precompiled_pdf(self, notification, old_pdf)
-    if not sanitise_response:
-        new_pdf = None
-    else:
-        sanitise_response = sanitise_response.json()
-        try:
-            new_pdf = base64.b64decode(sanitise_response["file"].encode())
-        except JSONDecodeError:
-            new_pdf = sanitise_response.content
+    sanitise_response, result = _sanitise_precompiled_pdf(self, notification, old_pdf)
+    new_pdf = None
+    if result == 'validation_passed':
+        new_pdf = base64.b64decode(sanitise_response["file"].encode())
 
         redaction_failed_message = sanitise_response.get("redaction_failed_message")
         if redaction_failed_message and not is_test_key:
@@ -234,18 +220,24 @@ def process_virus_scan_passed(self, filename):
             )
             copy_redaction_failed_pdf(filename)
 
+    billable_units = get_billable_units_for_letter_page_count(sanitise_response.get("page_count"))
+
     # TODO: Remove this once CYSP update their template to not cross over the margins
     if notification.service_id == UUID('fe44178f-3b45-4625-9f85-2264a36dd9ec'):  # CYSP
         # Check your state pension submit letters with good addresses and notify tags, so just use their supplied pdf
         new_pdf = old_pdf
 
-    if not new_pdf:
+    if result == 'validation_failed' and not new_pdf:
         current_app.logger.info('Invalid precompiled pdf received {} ({})'.format(notification.id, filename))
-        _move_invalid_letter_and_update_status(notification, filename, scan_pdf_object)
+        _move_invalid_letter_and_update_status(
+            notification=notification,
+            filename=filename,
+            scan_pdf_object=scan_pdf_object,
+            message=sanitise_response["message"],
+            invalid_pages=sanitise_response.get("invalid_pages"),
+            page_count=sanitise_response.get("page_count")
+        )
         return
-    else:
-        current_app.logger.info(
-            "Validation was successful for precompiled pdf {} ({})".format(notification.id, filename))
 
     current_app.logger.info('notification id {} ({}) sanitised and ready to send'.format(notification.id, filename))
 
@@ -270,9 +262,16 @@ def process_virus_scan_passed(self, filename):
         update_notification_status_by_id(notification.id, NOTIFICATION_TECHNICAL_FAILURE)
 
 
-def _move_invalid_letter_and_update_status(notification, filename, scan_pdf_object):
+def _move_invalid_letter_and_update_status(
+    *, notification, filename, scan_pdf_object, message=None, invalid_pages=None, page_count=None
+):
     try:
-        move_scan_to_invalid_pdf_bucket(filename)
+        move_scan_to_invalid_pdf_bucket(
+            source_filename=filename,
+            message=message,
+            invalid_pages=invalid_pages,
+            page_count=page_count
+        )
         scan_pdf_object.delete()
 
         update_letter_pdf_status(
@@ -311,17 +310,19 @@ def _sanitise_precompiled_pdf(self, notification, precompiled_pdf):
                      'Notification-ID': str(notification.id)}
         )
         response.raise_for_status()
-        return response
+        return response.json(), "validation_passed"
     except RequestException as ex:
         if ex.response is not None and ex.response.status_code == 400:
             message = "sanitise_precompiled_pdf validation error for notification: {}. ".format(notification.id)
-            if "message" in response.json():
+            if response.json().get("message"):
                 message += response.json()["message"]
+                if response.json().get("invalid_pages"):
+                    message += (" on pages: " + ", ".join(map(str, response.json()["invalid_pages"])))
 
             current_app.logger.info(
                 message
             )
-            return None
+            return response.json(), "validation_failed"
 
         try:
             current_app.logger.exception(

app/letters/utils.py

@@ -1,9 +1,13 @@
+import boto3
 import io
+import json
 import math
+
+from app.models import KEY_TYPE_TEST, SECOND_CLASS, RESOLVE_POSTAGE_FOR_FILE_NAME, NOTIFICATION_VALIDATION_FAILED
+
 from datetime import datetime, timedelta
 from enum import Enum
 
-import boto3
 from flask import current_app
 
 from notifications_utils.letter_timings import LETTER_PROCESSING_DEADLINE
@@ -11,8 +15,6 @@ from notifications_utils.pdf import pdf_page_count
 from notifications_utils.s3 import s3upload
 from notifications_utils.timezones import convert_utc_to_bst
 
-from app.models import KEY_TYPE_TEST, SECOND_CLASS, RESOLVE_POSTAGE_FOR_FILE_NAME, NOTIFICATION_VALIDATION_FAILED
-
 
 class ScanErrorType(Enum):
     ERROR = 1
@@ -127,10 +129,22 @@ def move_error_pdf_to_scan_bucket(source_filename):
     _move_s3_object(scan_bucket, error_file, scan_bucket, source_filename)
 
 
-def move_scan_to_invalid_pdf_bucket(source_filename):
-    scan_bucket = current_app.config['LETTERS_SCAN_BUCKET_NAME']
-    invalid_pdf_bucket = current_app.config['INVALID_PDF_BUCKET_NAME']
-    _move_s3_object(scan_bucket, source_filename, invalid_pdf_bucket, source_filename)
+def move_scan_to_invalid_pdf_bucket(source_filename, message=None, invalid_pages=None, page_count=None):
+    metadata = {}
+    if message:
+        metadata["message"] = message
+    if invalid_pages:
+        metadata["invalid_pages"] = json.dumps(invalid_pages)
+    if page_count:
+        metadata["page_count"] = str(page_count)
+
+    _move_s3_object(
+        source_bucket=current_app.config['LETTERS_SCAN_BUCKET_NAME'],
+        source_filename=source_filename,
+        target_bucket=current_app.config['INVALID_PDF_BUCKET_NAME'],
+        target_filename=source_filename,
+        metadata=metadata
+    )
 
 
 def move_uploaded_pdf_to_letters_bucket(source_filename, upload_filename):
@@ -138,7 +152,7 @@ def move_uploaded_pdf_to_letters_bucket(source_filename, upload_filename):
         source_bucket=current_app.config['TRANSIENT_UPLOADED_LETTERS'],
         source_filename=source_filename,
         target_bucket=current_app.config['LETTERS_PDF_BUCKET_NAME'],
-        target_filename=upload_filename
+        target_filename=upload_filename,
     )
 
 
@@ -164,7 +178,7 @@ def get_letter_pdf(notification):
     return obj.get()["Body"].read()
 
 
-def _move_s3_object(source_bucket, source_filename, target_bucket, target_filename):
+def _move_s3_object(source_bucket, source_filename, target_bucket, target_filename, metadata=None):
     s3 = boto3.resource('s3')
     copy_source = {'Bucket': source_bucket, 'Key': source_filename}
 
@@ -174,7 +188,11 @@ def _move_s3_object(source_bucket, source_filename, target_bucket, target_filena
     # Tags are copied across but the expiration time is reset in the destination bucket
     # e.g. if a file has 5 days left to expire on a ONE_WEEK retention in the source bucket,
     # in the destination bucket the expiration time will be reset to 7 days left to expire
-    obj.copy(copy_source, ExtraArgs={'ServerSideEncryption': 'AES256'})
+    put_args = {'ServerSideEncryption': 'AES256'}
+    if metadata:
+        put_args['Metadata'] = metadata
+        put_args["MetadataDirective"] = "REPLACE"
+    obj.copy(copy_source, ExtraArgs=put_args)
 
     s3.Object(source_bucket, source_filename).delete()
 
@@ -212,7 +230,12 @@ def letter_print_day(created_at):
 
 
 def get_page_count(pdf):
-    pages = pdf_page_count(io.BytesIO(pdf))
+    return pdf_page_count(io.BytesIO(pdf))
+
+
+def get_billable_units_for_letter_page_count(page_count):
+    if not page_count:
+        return 0
     pages_per_sheet = 2
-    billable_units = math.ceil(pages / pages_per_sheet)
+    billable_units = math.ceil(page_count / pages_per_sheet)
     return billable_units

app/service/send_notification.py

@@ -30,6 +30,7 @@ from app.dao.services_dao import dao_fetch_service_by_id
 from app.dao.templates_dao import dao_get_template_by_id_and_service_id, get_precompiled_letter_template
 from app.dao.users_dao import get_user_by_id
 from app.letters.utils import (
+    get_billable_units_for_letter_page_count,
     get_letter_pdf_filename,
     get_page_count,
     move_uploaded_pdf_to_letters_bucket,
@@ -152,7 +153,8 @@ def send_pdf_letter_notification(service_id, post_data):
         raise e
 
     # Getting the page count won't raise an error since admin has already checked the PDF is valid
-    billable_units = get_page_count(letter.read())
+    page_count = get_page_count(letter.read())
+    billable_units = get_billable_units_for_letter_page_count(page_count)
 
     personalisation = {
         'address_line_1': post_data['filename']