fix failed sanitise flow

the move from virus scan to validation failed function was called with
the wrong variables, and had some internal logic that was slightly
wrong.

Also, Don't use `update_notification_by_id` for notifications if they
are not in `created`, `sending`, `pending`, or `sent`. It silently
doesn't update them. I didn't want to do a deeper investigation into
the reasons behind this terrifying state machine as part of this commit
so I just changed the functions to call `dao_update_notification`
manually

app/celery/letters_pdf_tasks.py

@@ -27,8 +27,9 @@ from app.letters.utils import (
     get_reference_from_filename,
     get_folder_name,
     upload_letter_pdf,
-    move_failed_pdf,
     ScanErrorType,
+    move_failed_pdf,
+    move_scan_to_invalid_pdf_bucket,
     move_error_pdf_to_scan_bucket,
     get_file_names_from_error_bucket
 )
@@ -41,8 +42,6 @@ from app.models import (
     NOTIFICATION_VALIDATION_FAILED
 )
 
-from app.letters.utils import move_scan_to_invalid_pdf_bucket
-
 
 @notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300)
 @statsd(namespace="tasks")
@@ -194,8 +193,11 @@ def process_virus_scan_passed(self, filename):
 
     if not new_pdf:
         current_app.logger.info('Invalid precompiled pdf received {} ({})'.format(notification.id, filename))
-        update_notification_status_by_id(notification.id, NOTIFICATION_VALIDATION_FAILED)
+
-        move_scan_to_invalid_pdf_bucket(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:
@@ -257,7 +259,9 @@ def _sanitise_precomiled_pdf(self, notification, precompiled_pdf):
             current_app.logger.exception(
                 "RETRY FAILED: sanitise_precomiled_pdf failed for notification {}".format(notification.id),
             )
-            update_notification_status_by_id(notification.id, NOTIFICATION_TECHNICAL_FAILURE)
+
+            notification.status = NOTIFICATION_TECHNICAL_FAILURE
+            dao_update_notification(notification)
             raise
 
 

app/letters/utils.py

@@ -108,9 +108,9 @@ def move_error_pdf_to_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']
-    invalid_pdf = source_filename
+    _move_s3_object(scan_bucket, source_filename, invalid_pdf_bucket, source_filename)
-    _move_s3_object(invalid_pdf_bucket, invalid_pdf, invalid_pdf_bucket, source_filename)
 
 
 def get_file_names_from_error_bucket():

tests/app/celery/test_letters_pdf_tasks.py

@@ -32,8 +32,11 @@ from app.models import (
     NOTIFICATION_CREATED,
     NOTIFICATION_DELIVERED,
     NOTIFICATION_VIRUS_SCAN_FAILED,
+    NOTIFICATION_VALIDATION_FAILED,
     NOTIFICATION_SENDING,
-    NOTIFICATION_TECHNICAL_FAILURE)
+    NOTIFICATION_TECHNICAL_FAILURE,
+    NOTIFICATION_PENDING_VIRUS_CHECK,
+)
 
 from tests.conftest import set_config_values
 
@@ -351,7 +354,7 @@ 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'pdf_content')
 
-    sample_letter_notification.status = 'pending-virus-check'
+    sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK
     sample_letter_notification.key_type = key_type
     mock_s3upload = mocker.patch('app.celery.letters_pdf_tasks.s3upload')
     mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precomiled_pdf', return_value="success")
@@ -359,22 +362,60 @@ def test_process_letter_task_check_virus_scan_passed(
     process_virus_scan_passed(filename)
 
     assert sample_letter_notification.status == noti_status
+    mock_sanitise.assert_called_once_with(
+        ANY,
+        sample_letter_notification,
+        b'pdf_content'
+    )
     mock_s3upload.assert_called_once_with(
         bucket_name=target_bucket_name,
         filedata="success",
         file_location=destination_folder + filename,
         region='eu-west-1',
     )
+
+
+@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_sanitise_fails(
+    sample_letter_notification, mocker, key_type, is_test_letter
+):
+    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_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precomiled_pdf', return_value=None)
+
+    process_virus_scan_passed(filename)
+
+    assert sample_letter_notification.status == NOTIFICATION_VALIDATION_FAILED
     mock_sanitise.assert_called_once_with(
         ANY,
         sample_letter_notification,
         b'pdf_content'
     )
+    mock_move_s3.assert_called_once_with(
+        source_bucket_name, filename,
+        target_bucket_name, filename
+    )
 
 
 def test_process_letter_task_check_virus_scan_failed(sample_letter_notification, mocker):
     filename = 'NOTIFY.{}'.format(sample_letter_notification.reference)
-    sample_letter_notification.status = 'pending-virus-check'
+    sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK
     mock_move_failed_pdf = mocker.patch('app.celery.letters_pdf_tasks.move_failed_pdf')
 
     with pytest.raises(VirusScanError) as e:
@@ -387,7 +428,7 @@ def test_process_letter_task_check_virus_scan_failed(sample_letter_notification,
 
 def test_process_letter_task_check_virus_scan_error(sample_letter_notification, mocker):
     filename = 'NOTIFY.{}'.format(sample_letter_notification.reference)
-    sample_letter_notification.status = 'pending-virus-check'
+    sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK
     mock_move_failed_pdf = mocker.patch('app.celery.letters_pdf_tasks.move_failed_pdf')
 
     with pytest.raises(VirusScanError) as e:
@@ -419,6 +460,7 @@ def test_replay_letters_in_error_for_one_file(notify_api, mocker):
 
 
 def test_sanitise_precompiled_pdf_returns_data_from_template_preview(rmock, sample_letter_notification):
+    sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK
     rmock.post('http://localhost:9999/precompiled/sanitise', content=b'new_pdf', status_code=200)
     mock_celery = Mock(**{'retry.side_effect': Retry})
 
@@ -429,6 +471,7 @@ def test_sanitise_precompiled_pdf_returns_data_from_template_preview(rmock, samp
 
 
 def test_sanitise_precompiled_pdf_returns_none_on_validation_error(rmock, sample_letter_notification):
+    sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK
     rmock.post('http://localhost:9999/precompiled/sanitise', content=b'new_pdf', status_code=400)
     mock_celery = Mock(**{'retry.side_effect': Retry})
 
@@ -438,6 +481,7 @@ def test_sanitise_precompiled_pdf_returns_none_on_validation_error(rmock, sample
 
 
 def test_sanitise_precompiled_pdf_retries_on_http_error(rmock, sample_letter_notification):
+    sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK
     rmock.post('http://localhost:9999/precompiled/sanitise', content=b'new_pdf', status_code=500)
     mock_celery = Mock(**{'retry.side_effect': Retry})
 
@@ -449,6 +493,7 @@ def test_sanitise_precompiled_pdf_sets_notification_to_technical_failure_after_t
     rmock,
     sample_letter_notification
 ):
+    sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK
     rmock.post('http://localhost:9999/precompiled/sanitise', content=b'new_pdf', status_code=500)
     mock_celery = Mock(**{'retry.side_effect': MaxRetriesExceededError})