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