diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 10852929d..1f130f42d 100644 --- a/app/celery/tasks.py +++ b/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: - 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) + + +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.exception('Retry' + retry_msg) + try: + 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 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 2dd70329d..78235001a 100644 --- a/tests/app/celery/test_tasks.py +++ b/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