diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index ebe541cd9..366a9f954 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, @@ -209,17 +208,8 @@ 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=notification, filename=filename, scan_pdf_object=scan_pdf_object - ) - return - - sanitise_response = _sanitise_precompiled_pdf(self, notification, old_pdf) - if sanitise_response["message"]: # is response without message attribute possible now? I think not? + sanitise_response, result = _sanitise_precompiled_pdf(self, notification, old_pdf) + if result == "validation_failed": new_pdf = None else: new_pdf = base64.b64decode(sanitise_response["file"].encode()) @@ -231,6 +221,8 @@ 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 @@ -322,19 +314,19 @@ def _sanitise_precompiled_pdf(self, notification, precompiled_pdf): 'Notification-ID': str(notification.id)} ) response.raise_for_status() - return response.json() + 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 "invalid_pages" in response.json(): - message += (" on pages: " + ", ".join(map(str, response.json()["invalid_pages"]))) + if response.json().get("invalid_pages"): + message += (" on pages: " + ", ".join(map(str, response.json()["invalid_pages"]))) current_app.logger.info( message ) - return response.json() + return response.json(), "validation_failed" try: current_app.logger.exception( diff --git a/app/letters/utils.py b/app/letters/utils.py index 916274e2f..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 @@ -130,9 +132,9 @@ def move_error_pdf_to_scan_bucket(source_filename): def move_scan_to_invalid_pdf_bucket(source_filename, message=None, invalid_pages=None, page_count=None): metadata = {} if message: - metadata["validation_failed_message"] = message + metadata["message"] = message if invalid_pages: - metadata["invalid_pages"] = "-".join(map(str, invalid_pages)) + metadata["invalid_pages"] = json.dumps(invalid_pages) if page_count: metadata["page_count"] = str(page_count) @@ -228,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 41bc7bb69..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: @@ -455,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') @@ -486,9 +483,8 @@ def test_process_letter_task_check_virus_scan_passed_when_sanitise_fails( "page_count": 1 } mock_sanitise = mocker.patch( - 'app.celery.letters_pdf_tasks._sanitise_precompiled_pdf', return_value=sanitise_response + 'app.celery.letters_pdf_tasks._sanitise_precompiled_pdf', return_value=(sanitise_response, "validation_failed") ) - mock_get_page_count = mocker.patch('app.celery.letters_pdf_tasks.get_page_count', return_value=2) process_virus_scan_passed(filename) @@ -502,14 +498,12 @@ def test_process_letter_task_check_virus_scan_passed_when_sanitise_fails( mock_move_s3.assert_called_once_with( source_bucket=source_bucket_name, source_filename=filename, target_bucket=target_bucket_name, target_filename=filename, metadata={ - "validation_failed_message": "content-outside-printable-area", - "invalid_pages": "1-2", + "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 @@ -534,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: @@ -547,7 +540,7 @@ def test_process_letter_task_check_virus_scan_passed_when_redaction_fails( "redaction_failed_message": "No matches for address block during redaction procedure", "message": "", "invalid_pages": "", - "page_count": 2 + "page_count": 3 }, status_code=200 ) @@ -585,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=source_bucket_name, source_filename=filename, - target_bucket=target_bucket_name, target_filename=filename, metadata={} + 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 @@ -617,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', @@ -741,10 +741,11 @@ def test_sanitise_precompiled_pdf_returns_data_from_template_preview(rmock, samp 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 result == "validation_passed" assert base64.b64decode(response["file"].encode()) == b"new_pdf" assert rmock.last_request.text == 'old_pdf' @@ -768,10 +769,11 @@ def test_sanitise_precompiled_pdf_return_validation_error(rmock, sample_letter_n 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 result == "validation_failed" assert response == response_json