Merge pull request #1817 from alphagov/rc_add_virus_scan_error_task

Logging refactor to make debugging Antivirus scans easier

app/celery/letters_pdf_tasks.py

@@ -21,6 +21,7 @@ from app.dao.notifications_dao import (
     dao_get_notifications_by_references,
     dao_update_notifications_by_reference,
 )
+from app.errors import VirusScanError
 from app.letters.utils import (
     get_reference_from_filename,
     move_scanned_pdf_to_test_or_live_pdf_bucket,
@@ -162,9 +163,9 @@ def letter_in_created_state(filename):
 
 @notify_celery.task(name='process-virus-scan-passed')
 def process_virus_scan_passed(filename):
-    current_app.logger.info('Virus scan passed: {}'.format(filename))
     reference = get_reference_from_filename(filename)
     notification = dao_get_notification_by_reference(reference)
+    current_app.logger.info('notification id {} Virus scan passed: {}'.format(notification.id, filename))
 
     is_test_key = notification.key_type == KEY_TYPE_TEST
     move_scanned_pdf_to_test_or_live_pdf_bucket(
@@ -179,9 +180,9 @@ 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))
     move_failed_pdf(filename, ScanErrorType.FAILURE)
     reference = get_reference_from_filename(filename)
+    notification = dao_get_notification_by_reference(reference)
     updated_count = update_letter_pdf_status(reference, NOTIFICATION_VIRUS_SCAN_FAILED)
 
     if updated_count != 1:
@@ -191,12 +192,14 @@ def process_virus_scan_failed(filename):
             )
         )
 
+    raise VirusScanError('notification id {} Virus scan failed: {}'.format(notification.id, 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)
+    notification = dao_get_notification_by_reference(reference)
     updated_count = update_letter_pdf_status(reference, NOTIFICATION_TECHNICAL_FAILURE)
 
     if updated_count != 1:
@@ -206,6 +209,8 @@ def process_virus_scan_error(filename):
             )
         )
 
+    raise VirusScanError('notification id {} Virus scan error: {}'.format(notification.id, filename))
+
 
 def update_letter_pdf_status(reference, status):
     return dao_update_notifications_by_reference(

app/errors.py

@@ -10,6 +10,12 @@ from jsonschema import ValidationError as JsonSchemaValidationError
 from app.authentication.auth import AuthError
 
 
+class VirusScanError(Exception):
+    def __init__(self, message):
+
+        super().__init__(message)
+
+
 class InvalidRequest(Exception):
     code = None
     fields = []

tests/app/celery/test_letters_pdf_tasks.py

@@ -10,6 +10,7 @@ from celery.exceptions import MaxRetriesExceededError
 from requests import RequestException
 from sqlalchemy.orm.exc import NoResultFound
 
+from app.errors import VirusScanError
 from app.variables import Retention
 from app.celery.letters_pdf_tasks import (
     create_letters_pdf,
@@ -343,8 +344,10 @@ def test_process_letter_task_check_virus_scan_failed(sample_letter_notification,
     sample_letter_notification.status = 'pending-virus-check'
     mock_move_failed_pdf = mocker.patch('app.celery.letters_pdf_tasks.move_failed_pdf')
 
-    process_virus_scan_failed(filename)
+    with pytest.raises(VirusScanError) as e:
+        process_virus_scan_failed(filename)
 
+    assert "Virus scan failed:" in str(e)
     mock_move_failed_pdf.assert_called_once_with(filename, ScanErrorType.FAILURE)
     assert sample_letter_notification.status == NOTIFICATION_VIRUS_SCAN_FAILED
 
@@ -354,7 +357,9 @@ def test_process_letter_task_check_virus_scan_error(sample_letter_notification,
     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)
+    with pytest.raises(VirusScanError) as e:
+        process_virus_scan_error(filename)
 
+    assert "Virus scan error:" in str(e)
     mock_move_failed_pdf.assert_called_once_with(filename, ScanErrorType.ERROR)
     assert sample_letter_notification.status == NOTIFICATION_TECHNICAL_FAILURE