diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index e3db9572e..9c05ab223 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -22,16 +22,16 @@ from app.dao.notifications_dao import ( dao_update_notifications_by_reference, ) from app.letters.utils import ( - delete_pdf_from_letters_scan_bucket, get_reference_from_filename, move_scanned_pdf_to_test_or_live_pdf_bucket, - upload_letter_pdf -) + upload_letter_pdf, + move_failed_pdf, ScanErrorType) from app.models import ( KEY_TYPE_TEST, NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, NOTIFICATION_VIRUS_SCAN_FAILED, + NOTIFICATION_TECHNICAL_FAILURE ) @@ -180,7 +180,7 @@ def process_virus_scan_passed(filename): @notify_celery.task(name='process-virus-scan-failed') def process_virus_scan_failed(filename): current_app.logger.exception('Virus scan failed: {}'.format(filename)) - delete_pdf_from_letters_scan_bucket(filename) + move_failed_pdf(filename, ScanErrorType.FAILURE) reference = get_reference_from_filename(filename) updated_count = update_letter_pdf_status(reference, NOTIFICATION_VIRUS_SCAN_FAILED) @@ -192,6 +192,21 @@ def process_virus_scan_failed(filename): ) +@notify_celery.task(name='process-virus-scan-error') +def process_virus_scan_error(filename): + current_app.logger.exception('Virus scan error: {}'.format(filename)) + move_failed_pdf(filename, ScanErrorType.ERROR) + reference = get_reference_from_filename(filename) + updated_count = update_letter_pdf_status(reference, NOTIFICATION_TECHNICAL_FAILURE) + + if updated_count != 1: + raise Exception( + "There should only be one letter notification for each reference. Found {} notifications".format( + updated_count + ) + ) + + def update_letter_pdf_status(reference, status): return dao_update_notifications_by_reference( references=[reference], diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 2e59541a9..9a543ea23 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -22,10 +22,10 @@ def worker_process_shutdown(sender, signal, pid, exitcode): @statsd(namespace="tasks") def deliver_sms(self, notification_id): try: + current_app.logger.info("Start sending SMS for notification id: {}".format(notification_id)) notification = notifications_dao.get_notification_by_id(notification_id) if not notification: raise NoResultFound() - current_app.logger.info("Start sending SMS for notification id: {}".format(notification_id)) send_to_providers.send_sms_to_provider(notification) except Exception as e: try: @@ -44,10 +44,10 @@ def deliver_sms(self, notification_id): @statsd(namespace="tasks") def deliver_email(self, notification_id): try: + current_app.logger.info("Start sending email for notification id: {}".format(notification_id)) notification = notifications_dao.get_notification_by_id(notification_id) if not notification: raise NoResultFound() - current_app.logger.info("Start sending email for notification id: {}".format(notification_id)) send_to_providers.send_email_to_provider(notification) except InvalidEmailError as e: current_app.logger.exception(e) diff --git a/app/letters/utils.py b/app/letters/utils.py index 16113c69e..4fa041491 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -1,4 +1,5 @@ from datetime import datetime, timedelta +from enum import Enum import boto3 from flask import current_app @@ -9,6 +10,11 @@ from app.models import KEY_TYPE_TEST from app.variables import Retention +class ScanErrorType(Enum): + ERROR = 1 + FAILURE = 2 + + LETTERS_PDF_FILE_LOCATION_STRUCTURE = \ '{folder}NOTIFY.{reference}.{duplex}.{letter_class}.{colour}.{crown}.{date}.pdf' @@ -85,36 +91,22 @@ def upload_letter_pdf(notification, pdf_data): return upload_file_name -def move_scanned_pdf_to_test_or_live_pdf_bucket(filename, is_test_letter=False): +def move_scanned_pdf_to_test_or_live_pdf_bucket(source_filename, is_test_letter=False): source_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] 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] - s3 = boto3.resource('s3') - copy_source = {'Bucket': source_bucket_name, 'Key': filename} - target_filename = get_folder_name(datetime.utcnow(), is_test_letter) + filename + target_filename = get_folder_name(datetime.utcnow(), is_test_letter) + source_filename - target_bucket = s3.Bucket(target_bucket_name) - obj = target_bucket.Object(target_filename) - - # Tags are copied across but the expiration time is reset in the destination bucket - # e.g. if a file has 5 days left to expire on a ONE_WEEK retention in the source bucket, - # in the destination bucket the expiration time will be reset to 7 days left to expire - obj.copy(copy_source, ExtraArgs={'ServerSideEncryption': 'AES256'}) - - s3.Object(source_bucket_name, filename).delete() - - current_app.logger.info("Moved letter PDF: {}/{} to {}/{}".format( - source_bucket_name, filename, target_bucket_name, target_filename)) + _move_s3_object(source_bucket_name, source_filename, target_bucket_name, target_filename) -def delete_pdf_from_letters_scan_bucket(filename): - bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] +def move_failed_pdf(source_filename, scan_error_type): + scan_bucket = current_app.config['LETTERS_SCAN_BUCKET_NAME'] - s3 = boto3.resource('s3') - s3.Object(bucket_name, filename).delete() + target_filename = ('ERROR/' if scan_error_type == ScanErrorType.ERROR else 'FAILURE/') + source_filename - current_app.logger.info("Deleted letter PDF: {}/{}".format(bucket_name, filename)) + _move_s3_object(scan_bucket, source_filename, scan_bucket, target_filename) def get_letter_pdf(notification): @@ -135,3 +127,21 @@ def get_letter_pdf(notification): file_content = obj.get()["Body"].read() return file_content + + +def _move_s3_object(source_bucket, source_filename, target_bucket, target_filename): + s3 = boto3.resource('s3') + copy_source = {'Bucket': source_bucket, 'Key': source_filename} + + target_bucket = s3.Bucket(target_bucket) + obj = target_bucket.Object(target_filename) + + # Tags are copied across but the expiration time is reset in the destination bucket + # e.g. if a file has 5 days left to expire on a ONE_WEEK retention in the source bucket, + # in the destination bucket the expiration time will be reset to 7 days left to expire + obj.copy(copy_source, ExtraArgs={'ServerSideEncryption': 'AES256'}) + + s3.Object(source_bucket, source_filename).delete() + + current_app.logger.info("Moved letter PDF: {}/{} to {}/{}".format( + source_bucket, source_filename, target_bucket, target_filename)) diff --git a/requirements.txt b/requirements.txt index 0a7da8868..24316ae9c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,6 +23,6 @@ notifications-python-client==4.8.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@25.2.1#egg=notifications-utils==25.2.1 +git+https://github.com/alphagov/notifications-utils.git@25.2.2#egg=notifications-utils==25.2.2 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index e492540ed..9d07e7239 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -19,8 +19,8 @@ from app.celery.letters_pdf_tasks import ( letter_in_created_state, process_virus_scan_passed, process_virus_scan_failed, -) -from app.letters.utils import get_letter_pdf_filename + process_virus_scan_error) +from app.letters.utils import get_letter_pdf_filename, ScanErrorType from app.models import ( KEY_TYPE_NORMAL, KEY_TYPE_TEST, @@ -28,8 +28,8 @@ from app.models import ( NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, NOTIFICATION_VIRUS_SCAN_FAILED, - NOTIFICATION_SENDING -) + NOTIFICATION_SENDING, + NOTIFICATION_TECHNICAL_FAILURE) from tests.conftest import set_config_values @@ -341,9 +341,20 @@ def test_process_letter_task_check_virus_scan_passed( 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' - mock_delete_pdf = mocker.patch('app.celery.letters_pdf_tasks.delete_pdf_from_letters_scan_bucket') + mock_move_failed_pdf = mocker.patch('app.celery.letters_pdf_tasks.move_failed_pdf') process_virus_scan_failed(filename) - mock_delete_pdf.assert_called_once_with(filename) + mock_move_failed_pdf.assert_called_once_with(filename, ScanErrorType.FAILURE) assert sample_letter_notification.status == NOTIFICATION_VIRUS_SCAN_FAILED + + +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' + mock_move_failed_pdf = mocker.patch('app.celery.letters_pdf_tasks.move_failed_pdf') + + process_virus_scan_error(filename) + + mock_move_failed_pdf.assert_called_once_with(filename, ScanErrorType.ERROR) + assert sample_letter_notification.status == NOTIFICATION_TECHNICAL_FAILURE diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 79cf03021..c0dfbdc63 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,10 +1,12 @@ import pytest +from botocore.exceptions import ClientError from celery.exceptions import MaxRetriesExceededError from notifications_utils.recipients import InvalidEmailError import app from app.celery import provider_tasks from app.celery.provider_tasks import deliver_sms, deliver_email +from app.clients.email.aws_ses import AwsSesClientException from app.exceptions import NotificationTechnicalFailureException @@ -92,6 +94,24 @@ def test_should_technical_error_and_not_retry_if_invalid_email(sample_notificati assert sample_notification.status == 'technical-failure' +def test_should_retry_and_log_exception(sample_notification, mocker): + error_response = { + 'Error': { + 'Code': 'SomeError', + 'Message': 'some error message from amazon', + 'Type': 'Sender' + } + } + ex = ClientError(error_response=error_response, operation_name='opname') + mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=AwsSesClientException(str(ex))) + mocker.patch('app.celery.provider_tasks.deliver_email.retry') + + deliver_email(sample_notification.id) + + assert provider_tasks.deliver_email.retry.called is True + assert sample_notification.status == 'created' + + def test_send_sms_should_switch_providers_on_provider_failure(sample_notification, mocker): provider_to_use = mocker.patch('app.delivery.send_to_providers.provider_to_use') provider_to_use.return_value.send_sms.side_effect = Exception('Error') diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index 662862e50..400c545dc 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -11,8 +11,8 @@ from app.letters.utils import ( get_letter_pdf_filename, get_letter_pdf, upload_letter_pdf, - move_scanned_pdf_to_test_or_live_pdf_bucket -) + move_scanned_pdf_to_test_or_live_pdf_bucket, + ScanErrorType, move_failed_pdf) from app.models import KEY_TYPE_NORMAL, KEY_TYPE_TEST, PRECOMPILED_TEMPLATE_NAME from app.variables import Retention @@ -163,3 +163,43 @@ def test_move_scanned_letter_pdf_to_processing_bucket( assert folder_date_name + filename in [o.key for o in target_bucket.objects.all()] assert filename not in [o.key for o in source_bucket.objects.all()] + + +@mock_s3 +@freeze_time(FROZEN_DATE_TIME) +def test_move_failed_pdf_error(notify_api): + filename = 'test.pdf' + source_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + target_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + + conn = boto3.resource('s3', region_name='eu-west-1') + source_bucket = conn.create_bucket(Bucket=source_bucket_name) + target_bucket = 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') + + move_failed_pdf(filename, ScanErrorType.ERROR) + + assert 'ERROR/' + filename in [o.key for o in target_bucket.objects.all()] + assert filename not in [o.key for o in source_bucket.objects.all()] + + +@mock_s3 +@freeze_time(FROZEN_DATE_TIME) +def test_move_failed_pdf_scan_failed(notify_api): + filename = 'test.pdf' + source_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + target_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + + conn = boto3.resource('s3', region_name='eu-west-1') + source_bucket = conn.create_bucket(Bucket=source_bucket_name) + target_bucket = 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') + + move_failed_pdf(filename, ScanErrorType.FAILURE) + + assert 'FAILURE/' + filename in [o.key for o in target_bucket.objects.all()] + assert filename not in [o.key for o in source_bucket.objects.all()]