refactor exception handling code in job tasks code

it's almost entirely duplicated so share it across.

also clean up retrying - `task.retry(...)` raises a
celery.exceptions.Retry object, so you do not need to `raise` its
response. additionally, cleaned up tests around that since raising
Exception and asserting Exception is raised is dangerous as it could
mask actual programming errors

app/celery/tasks.py

@@ -51,7 +51,7 @@ def process_job(job_id):
 
     db_template = dao_get_template_by_id(job.template_id, job.template_version)
 
-    TemplateClass = SMSMessageTemplate if db_template.template_type == SMS_TYPE else WithSubjectTemplate
+    TemplateClass = get_template_class(db_template)
     template = TemplateClass(db_template.__dict__)
 
     for row_number, recipient, personalisation in RecipientCSV(
@@ -162,25 +162,7 @@ def send_sms(self,
         )
 
     except SQLAlchemyError as e:
-        if not get_notification_by_id(notification_id):
-            # Sometimes, SQS plays the same message twice. We should be able to catch an IntegrityError, but it seems
-            # SQLAlchemy is throwing a FlushError. So we check if the notification id already exists then do not
-            # send to the retry queue.
-            current_app.logger.error(
-                "RETRY: send_sms notification for job {} row number {} and notification id {}".format(
-                    notification.get('job', None),
-                    notification.get('row_number', None),
-                    notification_id))
-            current_app.logger.exception(e)
-            try:
-                raise self.retry(queue="retry", exc=e)
-            except self.MaxRetriesExceededError:
-                current_app.logger.error(
-                    "RETRY FAILED: send_sms notification for job {} row number {} and notification id {}".format(
-                        notification.get('job', None),
-                        notification.get('row_number', None),
-                        notification_id))
-                current_app.logger.exception(e)
+        handle_exception(self, notification, notification_id, e)
 
 
 @notify_celery.task(bind=True, name="send-email", max_retries=5, default_retry_delay=300)
@@ -222,22 +204,31 @@ def send_email(self,
 
         current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at))
     except SQLAlchemyError as e:
+        handle_exception(self, notification, notification_id, e)
+
+
+def handle_exception(task, notification, notification_id, exc):
     if not get_notification_by_id(notification_id):
+        retry_msg = '{task} notification for job {job} row number {row} and notification id {noti}'.format(
+            task=task.__name__,
+            job=notification.get('job', None),
+            row=notification.get('row_number', None),
+            noti=notification_id
+        )
         # Sometimes, SQS plays the same message twice. We should be able to catch an IntegrityError, but it seems
         # SQLAlchemy is throwing a FlushError. So we check if the notification id already exists then do not
         # send to the retry queue.
-            current_app.logger.error(
-                "RETRY: send_sms notification for job {} row number {} and notification id {}".format(
-                    notification.get('job', None),
-                    notification.get('row_number', None),
-                    notification_id))
-            current_app.logger.exception(e)
+        current_app.logger.exception('Retry' + retry_msg)
         try:
-                raise self.retry(queue="retry", exc=e)
-            except self.MaxRetriesExceededError:
-                current_app.logger.error(
-                    "RETRY FAILED: send_sms notification for job {} row number {} and notification id {}".format(
-                        notification.get('job', None),
-                        notification.get('row_number', None),
-                        notification_id))
-                current_app.logger.exception(e)
+            task.retry(queue="retry", exc=exc)
+        except task.MaxRetriesExceededError:
+            current_app.logger.exception('Retry' + retry_msg)
+
+
+def get_template_class(template):
+    if template.template_type == SMS_TYPE:
+        return SMSMessageTemplate
+    elif template.template_type in (EMAIL_TYPE, LETTER_TYPE):
+        # since we don't need rendering capabilities (we only need to extract placeholders) both email and letter can
+        # use the same base template
+        return WithSubjectTemplate

tests/app/celery/test_tasks.py

@@ -1,19 +1,27 @@
 import uuid
-import pytest
 from datetime import datetime
+from unittest.mock import Mock
+
+import pytest
 from freezegun import freeze_time
 from sqlalchemy.exc import SQLAlchemyError
+from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate
+from celery.exceptions import Retry
+
 from app import (encryption, DATETIME_FORMAT)
 from app.celery import provider_tasks
 from app.celery import tasks
 from app.celery.tasks import s3
 from app.celery.tasks import (
-    send_sms,
     process_job,
-    send_email
+    process_row,
+    send_sms,
+    send_email,
+    get_template_class
 )
 from app.dao import jobs_dao, services_dao
-from app.models import Notification, KEY_TYPE_TEAM, KEY_TYPE_TEST, KEY_TYPE_NORMAL
+from app.models import Notification, KEY_TYPE_TEAM, KEY_TYPE_TEST, KEY_TYPE_NORMAL, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE
+
 from tests.app import load_example_csv
 from tests.app.conftest import (
     sample_service,
@@ -715,19 +723,23 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em
                                                                      queue='send-email')
 
 
+class MyException(Exception):
+    pass
+
+
 def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, mocker):
     notification = _notification_json(sample_template, "+447234123123")
 
     expected_exception = SQLAlchemyError()
 
     mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
-    mocker.patch('app.celery.tasks.send_sms.retry', side_effect=Exception())
+    mocker.patch('app.celery.tasks.send_sms.retry', side_effect=Retry)
     mocker.patch('app.notifications.process_notifications.dao_create_notification', side_effect=expected_exception)
     now = datetime.utcnow()
 
     notification_id = uuid.uuid4()
 
-    with pytest.raises(Exception):
+    with pytest.raises(Retry):
         send_sms(
             sample_template.service_id,
             notification_id,
@@ -746,13 +758,13 @@ def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_tem
     expected_exception = SQLAlchemyError()
 
     mocker.patch('app.celery.provider_tasks.deliver_email.apply_async')
-    mocker.patch('app.celery.tasks.send_email.retry', side_effect=Exception())
+    mocker.patch('app.celery.tasks.send_email.retry', side_effect=Retry)
     mocker.patch('app.notifications.process_notifications.dao_create_notification', side_effect=expected_exception)
     now = datetime.utcnow()
 
     notification_id = uuid.uuid4()
 
-    with pytest.raises(Exception):
+    with pytest.raises(Retry):
         send_email(
             sample_email_template.service_id,
             notification_id,
@@ -801,3 +813,13 @@ def test_send_sms_does_not_send_duplicate_and_does_not_put_in_retry_queue(sample
     assert Notification.query.count() == 1
     assert not deliver_sms.called
     assert not retry.called
+
+
+@pytest.mark.parametrize('template_type, expected_class', [
+    (SMS_TYPE, SMSMessageTemplate),
+    (EMAIL_TYPE, WithSubjectTemplate),
+    (LETTER_TYPE, WithSubjectTemplate),
+])
+def test_get_template_class(template_type, expected_class):
+    template = Mock(template_type=template_type)
+    assert get_template_class(template) == expected_class