Refactor send_sms and send_email to use common code to persist the notificaiton.

app/celery/tasks.py

@@ -17,7 +17,7 @@ from app.dao.jobs_dao import (
     dao_update_job,
     dao_get_job_by_id
 )
-from app.dao.notifications_dao import (dao_create_notification)
+from app.dao.notifications_dao import dao_create_notification
 from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count
 from app.dao.templates_dao import dao_get_template_by_id
 from app.models import (
@@ -26,6 +26,7 @@ from app.models import (
     SMS_TYPE,
     KEY_TYPE_NORMAL
 )
+from app.notifications.process_notifications import persist_notification
 from app.service.utils import service_allowed_to_send_to
 from app.statsd_decorators import statsd
 
@@ -41,16 +42,7 @@ def process_job(job_id):
 
     service = job.service
 
-    total_sent = fetch_todays_total_message_count(service.id)
+    if __sending_limits_for_job_exceeded(service, job, job_id):
-
-    if total_sent + job.notification_count > service.message_limit:
-        job.job_status = 'sending limits exceeded'
-        job.processing_finished = datetime.utcnow()
-        dao_update_job(job)
-        current_app.logger.info(
-            "Job {} size {} error. Sending limits {} exceeded".format(
-                job_id, job.notification_count, service.message_limit)
-        )
         return
 
     job.job_status = 'in progress'
@@ -106,6 +98,21 @@ def process_job(job_id):
     )
 
 
+def __sending_limits_for_job_exceeded(service, job, job_id):
+    total_sent = fetch_todays_total_message_count(service.id)
+
+    if total_sent + job.notification_count > service.message_limit:
+        job.job_status = 'sending limits exceeded'
+        job.processing_finished = datetime.utcnow()
+        dao_update_job(job)
+        current_app.logger.info(
+            "Job {} size {} error. Sending limits {} exceeded".format(
+                job_id, job.notification_count, service.message_limit)
+        )
+        return True
+    return False
+
+
 @notify_celery.task(bind=True, name="send-sms", max_retries=5, default_retry_delay=300)
 @statsd(namespace="tasks")
 def send_sms(self,
@@ -125,11 +132,18 @@ def send_sms(self,
         return
 
     try:
-        dao_create_notification(
+        persist_notification(template_id=notification['template'],
-            Notification.from_api_request(
+                             template_version=notification['template_version'],
-                created_at, notification, notification_id, service.id, SMS_TYPE, api_key_id, key_type
+                             recipient=notification['to'],
-            )
+                             service_id=service.id,
-        )
+                             personalisation=notification.get('personalisation'),
+                             notification_type=SMS_TYPE,
+                             api_key_id=api_key_id,
+                             key_type=key_type,
+                             job_id=notification.get('job', None),
+                             job_row_number=notification.get('row_number', None),
+                             )
+
         provider_tasks.deliver_sms.apply_async(
             [notification_id],
             queue='send-sms' if not service.research_mode else 'research-mode'
@@ -166,10 +180,17 @@ def send_email(self, service_id,
         return
 
     try:
-        dao_create_notification(
+        persist_notification(
-            Notification.from_api_request(
+            template_id=notification['template'],
-                created_at, notification, notification_id, service.id, EMAIL_TYPE, api_key_id, key_type
+            template_version=notification['template_version'],
-            )
+            recipient=notification['to'],
+            service_id=service.id,
+            personalisation=notification.get('personalisation'),
+            notification_type=EMAIL_TYPE,
+            api_key_id=api_key_id,
+            key_type=key_type,
+            job_id=notification.get('job', None),
+            job_row_number=notification.get('row_number', None),
         )
 
         provider_tasks.deliver_email.apply_async(

app/models.py

@@ -572,7 +572,9 @@ class Notification(db.Model):
                             personalisation,
                             notification_type,
                             api_key_id,
-                            key_type):
+                            key_type,
+                            job_id,
+                            job_row_number):
         return cls(
             template_id=template_id,
             template_version=template_version,
@@ -583,7 +585,9 @@ class Notification(db.Model):
             personalisation=personalisation,
             notification_type=notification_type,
             api_key_id=api_key_id,
-            key_type=key_type
+            key_type=key_type,
+            job_id=job_id,
+            job_row_number=job_row_number,
         )
 
 

app/notifications/process_notifications.py

@@ -40,7 +40,9 @@ def persist_notification(template_id,
                          personalisation,
                          notification_type,
                          api_key_id,
-                         key_type):
+                         key_type,
+                         job_id=None,
+                         job_row_number=None):
     notification = Notification.from_v2_api_request(template_id=template_id,
                                                     template_version=template_version,
                                                     recipient=recipient,
@@ -48,7 +50,10 @@ def persist_notification(template_id,
                                                     personalisation=personalisation,
                                                     notification_type=notification_type,
                                                     api_key_id=api_key_id,
-                                                    key_type=key_type)
+                                                    key_type=key_type,
+                                                    job_id=job_id,
+                                                    job_row_number=job_row_number
+                                                    )
     dao_create_notification(notification)
     return notification
 

tests/app/celery/test_tasks.py

@@ -1,11 +1,9 @@
 import uuid
 import pytest
 from datetime import datetime
-
 from freezegun import freeze_time
 from sqlalchemy.exc import SQLAlchemyError
 from sqlalchemy.orm.exc import NoResultFound
-
 from app import (encryption, DATETIME_FORMAT)
 from app.celery import provider_tasks
 from app.celery import tasks
@@ -337,7 +335,7 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi
     notification = _notification_json(sample_template_with_placeholders,
                                       to="+447234123123", personalisation={"name": "Jo"})
 
-    mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
+    mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
 
     notification_id = uuid.uuid4()
 
@@ -352,9 +350,9 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi
         [notification_id],
         queue="send-sms"
     )
-
+    assert mocked_deliver_sms.called
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
+    assert Notification.query.count() == 1
-    assert persisted_notification.id == notification_id
+    persisted_notification = Notification.query.all()[0]
     assert persisted_notification.to == '+447234123123'
     assert persisted_notification.template_id == sample_template_with_placeholders.id
     assert persisted_notification.template_version == sample_template_with_placeholders.version
@@ -377,7 +375,7 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic
 
     notification = _notification_json(template, to="+447234123123")
 
-    mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
+    mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
 
     notification_id = uuid.uuid4()
 
@@ -392,6 +390,7 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic
         [notification_id],
         queue="research-mode"
     )
+    assert mocked_deliver_sms.called
 
 
 def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notify_db_session, mocker):
@@ -400,7 +399,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif
     template = sample_template(notify_db, notify_db_session, service=service)
     notification = _notification_json(template, "+447700900890")  # The user’s own number, but in a different format
 
-    mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
+    mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
 
     notification_id = uuid.uuid4()
     encrypt_notification = encryption.encrypt(notification)
@@ -416,8 +415,9 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif
         queue="send-sms"
     )
 
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
+    assert mocked_deliver_sms.called
-    assert persisted_notification.id == notification_id
+    assert Notification.query.count() == 1
+    persisted_notification = Notification.query.all()[0]
     assert persisted_notification.to == '+447700900890'
     assert persisted_notification.template_id == template.id
     assert persisted_notification.template_version == template.version
@@ -430,15 +430,15 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif
     assert persisted_notification.notification_type == 'sms'
 
 
-def test_should_not_send_sms_if_restricted_service_and_invalid_number_with_test_key(notify_db,
+def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key(notify_db,
-                                                                                    notify_db_session,
+                                                                                 notify_db_session,
-                                                                                    mocker):
+                                                                                 mocker):
     user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900205")
     service = sample_service(notify_db, notify_db_session, user=user, restricted=True)
     template = sample_template(notify_db, notify_db_session, service=service)
 
     notification = _notification_json(template, "07700 900849")
-    mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
+    mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
 
     notification_id = uuid.uuid4()
     send_sms(
@@ -454,13 +454,13 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number_with_test_
         queue="send-sms"
     )
 
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
+    assert mocked_deliver_sms.called
-    assert persisted_notification.id == notification_id
+    assert Notification.query.count() == 1
 
 
-def test_should_not_send_email_if_restricted_service_and_invalid_email_address_with_test_key(notify_db,
+def test_should_send_email_if_restricted_service_and_non_team_email_address_with_test_key(notify_db,
-                                                                                             notify_db_session,
+                                                                                          notify_db_session,
-                                                                                             mocker):
+                                                                                          mocker):
     user = sample_user(notify_db, notify_db_session)
     service = sample_service(notify_db, notify_db_session, user=user, restricted=True)
     template = sample_template(
@@ -468,7 +468,7 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address_w
     )
 
     notification = _notification_json(template, to="test@example.com")
-    mocker.patch('app.celery.provider_tasks.deliver_email.apply_async')
+    mocked_deliver_email = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async')
 
     notification_id = uuid.uuid4()
     send_email(
@@ -483,9 +483,8 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address_w
         [notification_id],
         queue="send-email"
     )
-
+    assert Notification.query.count() == 1
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
+    assert mocked_deliver_email.called
-    assert persisted_notification.id == notification_id
 
 
 def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker):
@@ -504,8 +503,7 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db,
         datetime.utcnow().strftime(DATETIME_FORMAT)
     )
     assert provider_tasks.deliver_sms.apply_async.called is False
-    with pytest.raises(NoResultFound):
+    assert Notification.query.count() == 0
-        Notification.query.filter_by(id=notification_id).one()
 
 
 def test_should_not_send_email_if_restricted_service_and_invalid_email_address(notify_db, notify_db_session, mocker):
@@ -524,8 +522,7 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address(n
         datetime.utcnow().strftime(DATETIME_FORMAT)
     )
 
-    with pytest.raises(NoResultFound):
+    assert Notification.query.count() == 0
-        Notification.query.filter_by(id=notification_id).one()
 
 
 def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_service(
@@ -577,8 +574,8 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_
         [notification_id],
         queue="send-sms"
     )
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
+    assert Notification.query.count() == 1
-    assert persisted_notification.id == notification_id
+    persisted_notification = Notification.query.all()[0]
     assert persisted_notification.to == '+447234123123'
     assert persisted_notification.job_id == sample_job.id
     assert persisted_notification.template_id == sample_job.template.id
@@ -618,14 +615,13 @@ def test_should_not_send_email_if_team_key_and_recipient_not_in_team(sample_emai
             key_type=KEY_TYPE_TEAM
         )
 
-        with pytest.raises(NoResultFound):
+        assert Notification.query.count() == 0
-            persisted_notification = Notification.query.filter_by(id=notification_id).one()
-            print(persisted_notification)
 
     apply_async.not_called()
 
 
 def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, notify_db_session, mocker):
+    assert Notification.query.count() == 0
     user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900205")
     service = sample_service(notify_db, notify_db_session, user=user, restricted=True)
     template = sample_template(notify_db, notify_db_session, service=service)
@@ -644,8 +640,7 @@ def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, no
         datetime.utcnow().strftime(DATETIME_FORMAT)
     )
     assert provider_tasks.deliver_sms.apply_async.called is False
-    with pytest.raises(NoResultFound):
+    assert Notification.query.count() == 0
-        Notification.query.filter_by(id=notification_id).one()
 
 
 def test_should_use_email_template_and_persist(sample_email_template_with_placeholders, sample_api_key, mocker):
@@ -671,15 +666,14 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh
             key_type=sample_api_key.key_type
         )
 
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
     provider_tasks.deliver_email.apply_async.assert_called_once_with(
         [notification_id], queue='send-email')
-
+    assert Notification.query.count() == 1
-    assert persisted_notification.id == notification_id
+    persisted_notification = Notification.query.all()[0]
     assert persisted_notification.to == 'my_email@my_email.com'
     assert persisted_notification.template_id == sample_email_template_with_placeholders.id
     assert persisted_notification.template_version == sample_email_template_with_placeholders.version
-    assert persisted_notification.created_at == now
+    assert persisted_notification.created_at >= now
     assert not persisted_notification.sent_at
     assert persisted_notification.status == 'created'
     assert not persisted_notification.sent_by
@@ -712,12 +706,12 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email
 
     provider_tasks.deliver_email.apply_async.assert_called_once_with([notification_id], queue='send-email')
 
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
+    assert Notification.query.count() == 1
-    assert persisted_notification.id == notification_id
+    persisted_notification = Notification.query.all()[0]
     assert persisted_notification.to == 'my_email@my_email.com'
     assert persisted_notification.template_id == sample_email_template.id
     assert persisted_notification.template_version == version_on_notification
-    assert persisted_notification.created_at == now
+    assert persisted_notification.created_at >= now
     assert not persisted_notification.sent_at
     assert persisted_notification.status == 'created'
     assert not persisted_notification.sent_by
@@ -740,11 +734,12 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi
     provider_tasks.deliver_email.apply_async.assert_called_once_with(
         [notification_id], queue='send-email'
     )
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
+    assert Notification.query.count() == 1
-    assert persisted_notification.id == notification_id
+    persisted_notification = Notification.query.all()[0]
     assert persisted_notification.to == 'my_email@my_email.com'
     assert persisted_notification.template_id == sample_email_template_with_placeholders.id
     assert persisted_notification.status == 'created'
+    assert persisted_notification.created_at >= now
     assert not persisted_notification.sent_by
     assert persisted_notification.personalisation == {"name": "Jo"}
     assert not persisted_notification.reference
@@ -755,6 +750,8 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em
     notification = _notification_json(sample_email_template, "my_email@my_email.com")
     mocker.patch('app.celery.provider_tasks.deliver_email.apply_async')
 
+    assert Notification.query.count() == 0
+
     notification_id = uuid.uuid4()
 
     now = datetime.utcnow()
@@ -765,12 +762,11 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em
         now.strftime(DATETIME_FORMAT)
     )
     provider_tasks.deliver_email.apply_async.assert_called_once_with([notification_id], queue='send-email')
-
+    assert Notification.query.count() == 1
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
+    persisted_notification = Notification.query.all()[0]
-    assert persisted_notification.id == notification_id
     assert persisted_notification.to == 'my_email@my_email.com'
     assert persisted_notification.template_id == sample_email_template.id
-    assert persisted_notification.created_at == now
+    assert persisted_notification.created_at >= now
     assert not persisted_notification.sent_at
     assert persisted_notification.status == 'created'
     assert not persisted_notification.sent_by
@@ -786,7 +782,7 @@ def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, m
 
     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.dao_create_notification', side_effect=expected_exception)
+    mocker.patch('app.notifications.process_notifications.dao_create_notification', side_effect=expected_exception)
     now = datetime.utcnow()
 
     notification_id = uuid.uuid4()
@@ -801,9 +797,7 @@ def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, m
     assert provider_tasks.deliver_sms.apply_async.called is False
     tasks.send_sms.retry.assert_called_with(exc=expected_exception, queue='retry')
 
-    with pytest.raises(NoResultFound) as e:
+    assert Notification.query.count() == 0
-        Notification.query.filter_by(id=notification_id).one()
-    assert 'No row was found for one' in str(e.value)
 
 
 def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_template, mocker):
@@ -813,7 +807,7 @@ def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_tem
 
     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.dao_create_notification', side_effect=expected_exception)
+    mocker.patch('app.notifications.process_notifications.dao_create_notification', side_effect=expected_exception)
     now = datetime.utcnow()
 
     notification_id = uuid.uuid4()
@@ -828,6 +822,4 @@ def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_tem
     assert provider_tasks.deliver_email.apply_async.called is False
     tasks.send_email.retry.assert_called_with(exc=expected_exception, queue='retry')
 
-    with pytest.raises(NoResultFound) as e:
+    assert Notification.query.count() == 0
-        Notification.query.filter_by(id=notification_id).one()
-    assert 'No row was found for one' in str(e.value)