Merge pull request #2295 from alphagov/move-unopenable-precompiled-letters

Move letters which can't be opened to invalid PDF bucket

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]

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):