Merge branch 'master' of https://github.com/alphagov/notifications-api into vb-report-tasks

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],

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)

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')
+    target_filename = get_folder_name(datetime.utcnow(), is_test_letter) + source_filename
-    copy_source = {'Bucket': source_bucket_name, 'Key': filename}
-    target_filename = get_folder_name(datetime.utcnow(), is_test_letter) + filename
 
-    target_bucket = s3.Bucket(target_bucket_name)
+    _move_s3_object(source_bucket_name, source_filename, target_bucket_name, target_filename)
-    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))
 
 
-def delete_pdf_from_letters_scan_bucket(filename):
+def move_failed_pdf(source_filename, scan_error_type):
-    bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME']
+    scan_bucket = current_app.config['LETTERS_SCAN_BUCKET_NAME']
 
-    s3 = boto3.resource('s3')
+    target_filename = ('ERROR/' if scan_error_type == ScanErrorType.ERROR else 'FAILURE/') + source_filename
-    s3.Object(bucket_name, filename).delete()
 
-    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))

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

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,
-)
+    process_virus_scan_error)
-from app.letters.utils import get_letter_pdf_filename
+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

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

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()]