From 7bf68e3664f7e9e4676c086a440f56796bcd79ed Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 16 Oct 2018 17:20:34 +0100 Subject: [PATCH] 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 | 16 ++++--- app/letters/utils.py | 4 +- tests/app/celery/test_letters_pdf_tasks.py | 53 ++++++++++++++++++++-- 3 files changed, 61 insertions(+), 12 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index c3a47608d..2891c68db 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/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 diff --git a/app/letters/utils.py b/app/letters/utils.py index 46ff1c4c7..57395fc87 100644 --- a/app/letters/utils.py +++ b/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(invalid_pdf_bucket, invalid_pdf, invalid_pdf_bucket, source_filename) + _move_s3_object(scan_bucket, source_filename, invalid_pdf_bucket, source_filename) def get_file_names_from_error_bucket(): diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index ee770cc99..690c7bb17 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/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})