From a9b755b08cc11da64905a4666b43d821783f6d9f Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 11 Jan 2019 09:23:05 +0000 Subject: [PATCH] Move letters which can't be opened to invalid PDF bucket If a precompiled letter can't be opened (e.g. because it isn't a valid PDF) we were setting its billable units to 0, but not moving it to the invalid PDF bucket. If a precompiled letter failed sanitisation, we were moving it to the invalid PDF bucket but not setting its billable units to 0. This commit makes sure that we always set the billable units to 0 and move the PDF to the right bucket if it fails sanitisation or can't be opened. --- app/celery/letters_pdf_tasks.py | 29 ++++++++------ tests/app/celery/test_letters_pdf_tasks.py | 45 ++++++++++++++++++---- 2 files changed, 55 insertions(+), 19 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 485f258bf..a57be4044 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -188,7 +188,12 @@ 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() - billable_units = _get_page_count(notification, old_pdf) + try: + billable_units = _get_page_count(notification, old_pdf) + except PdfReadError: + _move_invalid_letter_and_update_status(notification.reference, filename, scan_pdf_object) + return + new_pdf = _sanitise_precompiled_pdf(self, notification, old_pdf) # TODO: Remove this once CYSP update their template to not cross over the margins @@ -198,12 +203,7 @@ def process_virus_scan_passed(self, filename): if not new_pdf: current_app.logger.info('Invalid precompiled pdf received {} ({})'.format(notification.id, filename)) - - notification.status = NOTIFICATION_VALIDATION_FAILED - dao_update_notification(notification) - - move_scan_to_invalid_pdf_bucket(filename) - scan_pdf_object.delete() + _move_invalid_letter_and_update_status(notification.reference, filename, scan_pdf_object) return else: current_app.logger.info( @@ -233,14 +233,19 @@ def _get_page_count(notification, old_pdf): return billable_units except PdfReadError as e: current_app.logger.exception(msg='Invalid PDF received for notification_id: {}'.format(notification.id)) - update_letter_pdf_status( - reference=notification.reference, - status=NOTIFICATION_VALIDATION_FAILED, - billable_units=0 - ) raise e +def _move_invalid_letter_and_update_status(notification_reference, filename, scan_pdf_object): + move_scan_to_invalid_pdf_bucket(filename) + scan_pdf_object.delete() + + update_letter_pdf_status( + reference=notification_reference, + status=NOTIFICATION_VALIDATION_FAILED, + billable_units=0) + + def _upload_pdf_to_test_or_live_pdf_bucket(pdf_data, filename, is_test_letter): target_bucket_config = 'TEST_LETTERS_BUCKET_NAME' if is_test_letter else 'LETTERS_PDF_BUCKET_NAME' target_bucket_name = current_app.config[target_bucket_config] diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 13815ce05..ce7c9cd79 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -23,7 +23,6 @@ from app.celery.letters_pdf_tasks import ( process_virus_scan_failed, process_virus_scan_error, replay_letters_in_error, - _get_page_count, _sanitise_precompiled_pdf ) from app.letters.utils import get_letter_pdf_filename, ScanErrorType @@ -417,6 +416,7 @@ def test_process_letter_task_check_virus_scan_passed_when_sanitise_fails( process_virus_scan_passed(filename) assert sample_letter_notification.status == NOTIFICATION_VALIDATION_FAILED + assert sample_letter_notification.billable_units == 0 mock_sanitise.assert_called_once_with( ANY, sample_letter_notification, @@ -432,13 +432,44 @@ def test_process_letter_task_check_virus_scan_passed_when_sanitise_fails( ) -def test_get_page_count_set_notification_to_permanent_failure_when_not_pdf( - sample_letter_notification +@freeze_time('2018-01-01 18:00') +@mock_s3 +@pytest.mark.parametrize('key_type,is_test_letter', [ + (KEY_TYPE_NORMAL, False), (KEY_TYPE_TEST, True) +]) +def test_process_letter_task_check_virus_scan_passed_when_file_cannot_be_opened( + sample_letter_notification, mocker, key_type, is_test_letter ): - with pytest.raises(expected_exception=PdfReadError): - _get_page_count(sample_letter_notification, b'pdf_content') - updated_notification = Notification.query.filter_by(id=sample_letter_notification.id).first() - assert updated_notification.status == NOTIFICATION_VALIDATION_FAILED + filename = 'NOTIFY.{}'.format(sample_letter_notification.reference) + source_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + target_bucket_name = current_app.config['INVALID_PDF_BUCKET_NAME'] + + conn = boto3.resource('s3', region_name='eu-west-1') + conn.create_bucket(Bucket=source_bucket_name) + conn.create_bucket(Bucket=target_bucket_name) + + s3 = boto3.client('s3', region_name='eu-west-1') + s3.put_object(Bucket=source_bucket_name, Key=filename, Body=b'pdf_content') + + 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_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') + + process_virus_scan_passed(filename) + + mock_sanitise.assert_not_called() + mock_get_page_count.assert_called_once_with( + sample_letter_notification, b'pdf_content' + ) + mock_move_s3.assert_called_once_with( + source_bucket_name, filename, + target_bucket_name, filename + ) + assert sample_letter_notification.status == NOTIFICATION_VALIDATION_FAILED + assert sample_letter_notification.billable_units == 0 def test_process_letter_task_check_virus_scan_failed(sample_letter_notification, mocker):