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

@@ -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))
-
+        _move_invalid_letter_and_update_status(notification.reference, filename, scan_pdf_object)
-        notification.status = NOTIFICATION_VALIDATION_FAILED
-        dao_update_notification(notification)
-
-        move_scan_to_invalid_pdf_bucket(filename)
-        scan_pdf_object.delete()
         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(
+@freeze_time('2018-01-01 18:00')
-        sample_letter_notification
+@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):
+    filename = 'NOTIFY.{}'.format(sample_letter_notification.reference)
-        _get_page_count(sample_letter_notification, b'pdf_content')
+    source_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME']
-    updated_notification = Notification.query.filter_by(id=sample_letter_notification.id).first()
+    target_bucket_name = current_app.config['INVALID_PDF_BUCKET_NAME']
-    assert updated_notification.status == NOTIFICATION_VALIDATION_FAILED
+
+    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):