diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 434f2fdb7..761a97c78 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/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( diff --git a/app/letters/utils.py b/app/letters/utils.py index 98e954fb5..4aa4ee5c7 100644 --- a/app/letters/utils.py +++ b/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 diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 00c78936b..4589cff40 100644 --- a/app/service/send_notification.py +++ b/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'] diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 441e3bffc..38e61deef 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -2,7 +2,6 @@ from unittest.mock import Mock, call, ANY import base64 import boto3 -from PyPDF2.utils import PdfReadError from moto import mock_s3 from flask import current_app from freezegun import freeze_time @@ -426,7 +425,6 @@ def test_process_letter_task_check_virus_scan_passed( s3 = boto3.client('s3', region_name='eu-west-1') s3.put_object(Bucket=source_bucket_name, Key=filename, Body=b'old_pdf') - mock_get_page_count = mocker.patch('app.celery.letters_pdf_tasks.get_page_count', return_value=1) mock_s3upload = mocker.patch('app.celery.letters_pdf_tasks.s3upload') endpoint = 'http://localhost:9999/precompiled/sanitise' with requests_mock.mock() as rmock: @@ -436,10 +434,9 @@ def test_process_letter_task_check_virus_scan_passed( json={ "file": base64.b64encode(b"new_pdf").decode("utf-8"), "validation_passed": True, - "errors": { - "content_outside_of_printable_area": [], - "document_not_a4_size_portrait_orientation": [], - } + "message": "", + "invalid_pages": [], + "page_count": 1 }, status_code=200 ) @@ -456,7 +453,6 @@ def test_process_letter_task_check_virus_scan_passed( file_location=destination_folder + filename, region='eu-west-1', ) - mock_get_page_count.assert_called_once_with(b'old_pdf') @freeze_time('2018-01-01 18:00') @@ -479,8 +475,16 @@ def test_process_letter_task_check_virus_scan_passed_when_sanitise_fails( sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK sample_letter_notification.key_type = key_type mock_move_s3 = mocker.patch('app.letters.utils._move_s3_object') - mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precompiled_pdf', return_value=None) - mock_get_page_count = mocker.patch('app.celery.letters_pdf_tasks.get_page_count', return_value=2) + sanitise_response = { + "file": base64.b64encode(b"nyan").decode("utf-8"), + "validation_passed": False, + "message": "content-outside-printable-area", + "invalid_pages": [1, 2], + "page_count": 1 + } + mock_sanitise = mocker.patch( + 'app.celery.letters_pdf_tasks._sanitise_precompiled_pdf', return_value=(sanitise_response, "validation_failed") + ) process_virus_scan_passed(filename) @@ -492,12 +496,14 @@ def test_process_letter_task_check_virus_scan_passed_when_sanitise_fails( b'pdf_content' ) mock_move_s3.assert_called_once_with( - source_bucket_name, filename, - target_bucket_name, filename + source_bucket=source_bucket_name, source_filename=filename, + target_bucket=target_bucket_name, target_filename=filename, metadata={ + "message": "content-outside-printable-area", + "invalid_pages": "[1, 2]", + "page_count": "1" + } ) - mock_get_page_count.assert_called_once_with(b'pdf_content') - @freeze_time('2018-01-01 18:00') @mock_s3 @@ -522,7 +528,6 @@ def test_process_letter_task_check_virus_scan_passed_when_redaction_fails( sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK sample_letter_notification.key_type = key_type mock_copy_s3 = mocker.patch('app.letters.utils._copy_s3_object') - mocker.patch('app.celery.letters_pdf_tasks.get_page_count', return_value=2) endpoint = 'http://localhost:9999/precompiled/sanitise' with requests_mock.mock() as rmock: @@ -533,10 +538,9 @@ def test_process_letter_task_check_virus_scan_passed_when_redaction_fails( "file": base64.b64encode(b"new_pdf").decode("utf-8"), "validation_passed": True, "redaction_failed_message": "No matches for address block during redaction procedure", - "errors": { - "content_outside_of_printable_area": [], - "document_not_a4_size_portrait_orientation": [] - } + "message": "", + "invalid_pages": "", + "page_count": 3 }, status_code=200 ) @@ -574,16 +578,25 @@ def test_process_letter_task_check_virus_scan_passed_when_file_cannot_be_opened( sample_letter_notification.key_type = key_type mock_move_s3 = mocker.patch('app.letters.utils._move_s3_object') - mock_get_page_count = mocker.patch('app.celery.letters_pdf_tasks.get_page_count', side_effect=PdfReadError) - mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precompiled_pdf') + endpoint = 'http://localhost:9999/precompiled/sanitise' + with requests_mock.mock() as rmock: + rmock.request( + "POST", + endpoint, + json={ + "page_count": None, + "recipient_address": None, + "message": 'unable-to-read-the-file', + "invalid_pages": None, + "file": None + }, + status_code=400 + ) + process_virus_scan_passed(filename) - process_virus_scan_passed(filename) - - mock_sanitise.assert_not_called() - mock_get_page_count.assert_called_once_with(b'pdf_content') mock_move_s3.assert_called_once_with( - source_bucket_name, filename, - target_bucket_name, filename + source_bucket=source_bucket_name, source_filename=filename, + target_bucket=target_bucket_name, target_filename=filename, metadata={'message': 'unable-to-read-the-file'} ) assert sample_letter_notification.status == NOTIFICATION_VALIDATION_FAILED assert sample_letter_notification.billable_units == 0 @@ -606,8 +619,6 @@ def test_process_virus_scan_passed_logs_error_and_sets_tech_failure_if_s3_error_ s3 = boto3.client('s3', region_name='eu-west-1') s3.put_object(Bucket=source_bucket_name, Key=filename, Body=b'pdf_content') - mocker.patch('app.celery.letters_pdf_tasks.get_page_count', return_value=1) - error_response = { 'Error': { 'Code': 'InvalidParameterValue', @@ -626,10 +637,9 @@ def test_process_virus_scan_passed_logs_error_and_sets_tech_failure_if_s3_error_ json={ "file": base64.b64encode(b"new_pdf").decode("utf-8"), "validation_passed": True, - "errors": { - "content_outside_of_printable_area": [], - "document_not_a4_size_portrait_orientation": [], - } + "message": "", + "invalid_pages": [], + "page_count": 1 }, status_code=200 ) @@ -656,7 +666,11 @@ def test_move_invalid_letter_and_update_status_logs_error_and_sets_tech_failure_ side_effect=ClientError(error_response, 'operation_name')) mock_logger = mocker.patch('app.celery.tasks.current_app.logger.exception') - _move_invalid_letter_and_update_status(sample_letter_notification, 'filename', mocker.Mock()) + _move_invalid_letter_and_update_status( + notification=sample_letter_notification, + filename='filename', + scan_pdf_object=mocker.Mock() + ) assert sample_letter_notification.status == NOTIFICATION_TECHNICAL_FAILURE mock_logger.assert_called_once_with( @@ -720,46 +734,47 @@ def test_sanitise_precompiled_pdf_returns_data_from_template_preview(rmock, samp json={ "file": base64.b64encode(b"new_pdf").decode("utf-8"), "validation_passed": True, - "errors": { - "content_outside_of_printable_area": [], - "document_not_a4_size_portrait_orientation": [], - } + "message": "", + "invalid_pages": [], + "page_count": 1 }, status_code=200 ) mock_celery = Mock(**{'retry.side_effect': Retry}) - response = _sanitise_precompiled_pdf(mock_celery, sample_letter_notification, b'old_pdf') + response, result = _sanitise_precompiled_pdf(mock_celery, sample_letter_notification, b'old_pdf') assert rmock.called assert rmock.request_history[0].url == endpoint - assert base64.b64decode(response.json()["file"].encode()) == b"new_pdf" + assert result == "validation_passed" + assert base64.b64decode(response["file"].encode()) == b"new_pdf" assert rmock.last_request.text == 'old_pdf' -def test_sanitise_precompiled_pdf_returns_none_on_validation_error(rmock, sample_letter_notification): +def test_sanitise_precompiled_pdf_return_validation_error(rmock, sample_letter_notification): sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK endpoint = 'http://localhost:9999/precompiled/sanitise' + response_json = { + "file": base64.b64encode(b"nyan").decode("utf-8"), + "validation_passed": False, + "message": "content-outside-printable-area", + "invalid_pages": [1], + "page_count": 1 + } with requests_mock.mock() as rmock: rmock.request( "POST", endpoint, - json={ - "file": base64.b64encode(b"nyan").decode("utf-8"), - "validation_passed": False, - "errors": { - "content_outside_of_printable_area": [1], - "document_not_a4_size_portrait_orientation": [], - } - }, + json=response_json, status_code=400 ) mock_celery = Mock(**{'retry.side_effect': Retry}) - response = _sanitise_precompiled_pdf(mock_celery, sample_letter_notification, b'old_pdf') + response, result = _sanitise_precompiled_pdf(mock_celery, sample_letter_notification, b'old_pdf') assert rmock.called assert rmock.request_history[0].url == endpoint - assert response is None + assert result == "validation_failed" + assert response == response_json def test_sanitise_precompiled_pdf_passes_the_service_id_and_notification_id_to_template_preview(