Merge pull request #733 from alphagov/refactor-for-code-reuse

Refactor for code reuse

app/celery/tasks.py

@@ -17,15 +17,14 @@ 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.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 (
-    Notification,
     EMAIL_TYPE,
     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 +40,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'
@@ -103,6 +93,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,
@@ -112,6 +117,7 @@ def send_sms(self,
              created_at,
              api_key_id=None,
              key_type=KEY_TYPE_NORMAL):
+    # notification_id is not used, it is created by the db model object
     notification = encryption.decrypt(encrypted_notification)
     service = dao_fetch_service_by_id(service_id)
 
@@ -122,29 +128,38 @@ def send_sms(self,
         return
 
     try:
-        dao_create_notification(
+        saved_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,
+                                                  created_at=created_at,
+                                                  job_id=notification.get('job', None),
+                                                  job_row_number=notification.get('row_number', None),
+                                                  )
+
         provider_tasks.deliver_sms.apply_async(
-            [notification_id],
+            [saved_notification.id],
             queue='send-sms' if not service.research_mode else 'research-mode'
         )
 
         current_app.logger.info(
-            "SMS {} created at {} for job {}".format(notification_id, created_at, notification.get('job', None))
+            "SMS {} created at {} for job {}".format(saved_notification.id, created_at, notification.get('job', None))
         )
 
     except SQLAlchemyError as e:
-        current_app.logger.exception("RETRY: send_sms notification {}".format(notification_id), e)
+        current_app.logger.exception(
+            "RETRY: send_sms notification for job {} row number {}".format(notification.get('job', None),
+                                                                           notification.get('row_number', None)), e)
         try:
             raise self.retry(queue="retry", exc=e)
         except self.MaxRetriesExceededError:
             current_app.logger.exception(
-                "RETRY FAILED: task send_sms failed for notification {}".format(notification.id),
+                "RETRY FAILED: task send_sms failed for notification".format(notification.get('job', None),
-                e
+                                                                             notification.get('row_number', None)), e)
-            )
 
 
 @notify_celery.task(bind=True, name="send-email", max_retries=5, default_retry_delay=300)
@@ -155,6 +170,7 @@ def send_email(self, service_id,
                created_at,
                api_key_id=None,
                key_type=KEY_TYPE_NORMAL):
+    # notification_id is not used, it is created by the db model object
     notification = encryption.decrypt(encrypted_notification)
     service = dao_fetch_service_by_id(service_id)
 
@@ -163,24 +179,33 @@ def send_email(self, service_id,
         return
 
     try:
-        dao_create_notification(
+        saved_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,
+            created_at=created_at,
+            job_id=notification.get('job', None),
+            job_row_number=notification.get('row_number', None),
+
         )
 
         provider_tasks.deliver_email.apply_async(
-            [notification_id],
+            [saved_notification.id],
             queue='send-email' if not service.research_mode else 'research-mode'
         )
 
-        current_app.logger.info("Email {} created at {}".format(notification_id, created_at))
+        current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at))
     except SQLAlchemyError as e:
-        current_app.logger.exception("RETRY: send_email notification {}".format(notification_id), e)
+        current_app.logger.exception("RETRY: send_email notification".format(notification.get('job', None),
+                                                                             notification.get('row_number', None)), e)
         try:
             raise self.retry(queue="retry", exc=e)
         except self.MaxRetriesExceededError:
             current_app.logger.error(
-                "RETRY FAILED: task send_email failed for notification {}".format(notification.id),
+                "RETRY FAILED: task send_email failed for notification".format(notification.get('job', None),
-                e
+                                                                               notification.get('row_number', None)), e)
-            )

app/models.py

@@ -537,55 +537,6 @@ class Notification(db.Model):
         if personalisation:
             self._personalisation = encryption.encrypt(personalisation)
 
-    @classmethod
-    def from_api_request(
-            cls,
-            created_at,
-            notification,
-            notification_id,
-            service_id,
-            notification_type,
-            api_key_id,
-            key_type):
-        return cls(
-            id=notification_id,
-            template_id=notification['template'],
-            template_version=notification['template_version'],
-            to=notification['to'],
-            service_id=service_id,
-            job_id=notification.get('job', None),
-            job_row_number=notification.get('row_number', None),
-            status='created',
-            created_at=datetime.datetime.strptime(created_at, DATETIME_FORMAT),
-            personalisation=notification.get('personalisation'),
-            notification_type=notification_type,
-            api_key_id=api_key_id,
-            key_type=key_type
-        )
-
-    @classmethod
-    def from_v2_api_request(cls,
-                            template_id,
-                            template_version,
-                            recipient,
-                            service_id,
-                            personalisation,
-                            notification_type,
-                            api_key_id,
-                            key_type):
-        return cls(
-            template_id=template_id,
-            template_version=template_version,
-            to=recipient,
-            service_id=service_id,
-            status='created',
-            created_at=datetime.datetime.utcnow(),
-            personalisation=personalisation,
-            notification_type=notification_type,
-            api_key_id=api_key_id,
-            key_type=key_type
-        )
-
 
 class NotificationHistory(db.Model):
     __tablename__ = 'notification_history'

app/notifications/process_notifications.py

@@ -1,7 +1,10 @@
+from datetime import datetime
+
 from flask import current_app
 from notifications_utils.renderers import PassThrough
 from notifications_utils.template import Template
 
+from app import DATETIME_FORMAT
 from app.celery import provider_tasks
 from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id
 from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE
@@ -40,15 +43,23 @@ def persist_notification(template_id,
                          personalisation,
                          notification_type,
                          api_key_id,
-                         key_type):
+                         key_type,
-    notification = Notification.from_v2_api_request(template_id=template_id,
+                         created_at=None,
-                                                    template_version=template_version,
+                         job_id=None,
-                                                    recipient=recipient,
+                         job_row_number=None):
-                                                    service_id=service_id,
+    notification = Notification(
-                                                    personalisation=personalisation,
+        template_id=template_id,
-                                                    notification_type=notification_type,
+        template_version=template_version,
-                                                    api_key_id=api_key_id,
+        to=recipient,
-                                                    key_type=key_type)
+        service_id=service_id,
+        personalisation=personalisation,
+        notification_type=notification_type,
+        api_key_id=api_key_id,
+        key_type=key_type,
+        created_at=created_at if created_at else datetime.utcnow().strftime(DATETIME_FORMAT),
+        job_id=job_id,
+        job_row_number=job_row_number
+    )
     dao_create_notification(notification)
     return notification
 
@@ -66,7 +77,7 @@ def send_notification_to_queue(notification, research_mode):
                 [str(notification.id)],
                 queue='send-email' if not research_mode else 'research-mode'
             )
-    except Exception as e:
+    except Exception:
         current_app.logger.exception("Failed to send to SQS exception")
         dao_delete_notifications_and_history_by_id(notification.id)
         raise

tests/app/celery/test_tasks.py

@@ -1,11 +1,8 @@
 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
@@ -191,8 +188,6 @@ def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(noti
 
     mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('email'))
     mocker.patch('app.celery.tasks.send_email.apply_async')
-    mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
-    mocker.patch('app.celery.tasks.create_uuid', return_value="uuid")
 
     process_job(job.id)
 
@@ -210,8 +205,6 @@ def test_should_not_process_email_job_if_would_exceed_send_limits(notify_db, not
 
     mocker.patch('app.celery.tasks.s3.get_job_from_s3')
     mocker.patch('app.celery.tasks.send_email.apply_async')
-    mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
-    mocker.patch('app.celery.tasks.create_uuid', return_value="uuid")
 
     process_job(job.id)
 
@@ -337,24 +330,16 @@ 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()
 
     send_sms(
         sample_template_with_placeholders.service_id,
-        notification_id,
+        uuid.uuid4(),
         encryption.encrypt(notification),
         datetime.utcnow().strftime(DATETIME_FORMAT)
     )
 
-    provider_tasks.deliver_sms.apply_async.assert_called_once_with(
+    persisted_notification = Notification.query.one()
-        [notification_id],
-        queue="send-sms"
-    )
-
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
-    assert persisted_notification.id == notification_id
     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
@@ -366,6 +351,10 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi
     assert persisted_notification.personalisation == {'name': 'Jo'}
     assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"})
     assert persisted_notification.notification_type == 'sms'
+    mocked_deliver_sms.assert_called_once_with(
+        [persisted_notification.id],
+        queue="send-sms"
+    )
 
 
 def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_service(notify_db, notify_db_session, mocker):
@@ -377,7 +366,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()
 
@@ -387,11 +376,12 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic
         encryption.encrypt(notification),
         datetime.utcnow().strftime(DATETIME_FORMAT)
     )
-
+    persisted_notification = Notification.query.one()
     provider_tasks.deliver_sms.apply_async.assert_called_once_with(
-        [notification_id],
+        [persisted_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):
@@ -411,13 +401,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif
         datetime.utcnow().strftime(DATETIME_FORMAT)
     )
 
-    provider_tasks.deliver_sms.apply_async.assert_called_once_with(
+    persisted_notification = Notification.query.one()
-        [notification_id],
-        queue="send-sms"
-    )
-
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
-    assert persisted_notification.id == notification_id
     assert persisted_notification.to == '+447700900890'
     assert persisted_notification.template_id == template.id
     assert persisted_notification.template_version == template.version
@@ -428,17 +412,21 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif
     assert not persisted_notification.job_id
     assert not persisted_notification.personalisation
     assert persisted_notification.notification_type == 'sms'
+    provider_tasks.deliver_sms.apply_async.assert_called_once_with(
+        [persisted_notification.id],
+        queue="send-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(
@@ -449,18 +437,16 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number_with_test_
         key_type=KEY_TYPE_TEST
     )
 
-    provider_tasks.deliver_sms.apply_async.assert_called_once_with(
+    persisted_notification = Notification.query.one()
-        [notification_id],
+    mocked_deliver_sms.assert_called_once_with(
+        [persisted_notification.id],
         queue="send-sms"
     )
 
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
-    assert persisted_notification.id == notification_id
 
-
+def test_should_send_email_if_restricted_service_and_non_team_email_address_with_test_key(notify_db,
-def test_should_not_send_email_if_restricted_service_and_invalid_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 +454,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(
@@ -479,14 +465,12 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address_w
         key_type=KEY_TYPE_TEST
     )
 
-    provider_tasks.deliver_email.apply_async.assert_called_once_with(
+    persisted_notification = Notification.query.one()
-        [notification_id],
+    mocked_deliver_email.assert_called_once_with(
+        [persisted_notification.id],
         queue="send-email"
     )
 
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
-    assert persisted_notification.id == notification_id
-
 
 def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker):
     user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900205")
@@ -504,8 +488,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 +507,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(
@@ -550,8 +532,9 @@ def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_serv
         datetime.utcnow().strftime(DATETIME_FORMAT)
     )
 
+    persisted_notification = Notification.query.one()
     provider_tasks.deliver_email.apply_async.assert_called_once_with(
-        [notification_id],
+        [persisted_notification.id],
         queue="research-mode"
     )
 
@@ -565,32 +548,33 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_
     mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
 
     notification_id = uuid.uuid4()
+    now = datetime.utcnow()
     send_sms(
         sample_job.service.id,
         notification_id,
         encryption.encrypt(notification),
-        datetime.utcnow().strftime(DATETIME_FORMAT),
+        now.strftime(DATETIME_FORMAT),
         api_key_id=str(sample_api_key.id),
         key_type=KEY_TYPE_NORMAL
     )
-    provider_tasks.deliver_sms.apply_async.assert_called_once_with(
+    persisted_notification = Notification.query.one()
-        [notification_id],
-        queue="send-sms"
-    )
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
-    assert persisted_notification.id == notification_id
     assert persisted_notification.to == '+447234123123'
     assert persisted_notification.job_id == sample_job.id
     assert persisted_notification.template_id == sample_job.template.id
     assert persisted_notification.status == 'created'
     assert not persisted_notification.sent_at
-    assert persisted_notification.created_at <= datetime.utcnow()
+    assert persisted_notification.created_at <= now
     assert not persisted_notification.sent_by
     assert persisted_notification.job_row_number == 2
     assert persisted_notification.api_key_id == sample_api_key.id
     assert persisted_notification.key_type == KEY_TYPE_NORMAL
     assert persisted_notification.notification_type == 'sms'
 
+    provider_tasks.deliver_sms.apply_async.assert_called_once_with(
+        [persisted_notification.id],
+        queue="send-sms"
+    )
+
 
 def test_should_not_send_email_if_team_key_and_recipient_not_in_team(sample_email_template_with_placeholders,
                                                                      sample_team_api_key,
@@ -618,14 +602,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 +627,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,11 +653,7 @@ 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()
+    persisted_notification = Notification.query.one()
-    provider_tasks.deliver_email.apply_async.assert_called_once_with(
-        [notification_id], queue='send-email')
-
-    assert persisted_notification.id == notification_id
     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
@@ -690,6 +668,9 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh
     assert persisted_notification.key_type == KEY_TYPE_NORMAL
     assert persisted_notification.notification_type == 'email'
 
+    provider_tasks.deliver_email.apply_async.assert_called_once_with(
+        [persisted_notification.id], queue='send-email')
+
 
 def test_send_email_should_use_template_version_from_job_not_latest(sample_email_template, mocker):
     notification = _notification_json(sample_email_template, 'my_email@my_email.com')
@@ -701,19 +682,15 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email
     dao_update_template(sample_email_template)
     t = dao_get_template_by_id(sample_email_template.id)
     assert t.version > version_on_notification
-    notification_id = uuid.uuid4()
     now = datetime.utcnow()
     send_email(
         sample_email_template.service_id,
-        notification_id,
+        uuid.uuid4(),
         encryption.encrypt(notification),
         now.strftime(DATETIME_FORMAT)
     )
 
-    provider_tasks.deliver_email.apply_async.assert_called_once_with([notification_id], queue='send-email')
+    persisted_notification = Notification.query.one()
-
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
-    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.template_version == version_on_notification
@@ -722,6 +699,7 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email
     assert persisted_notification.status == 'created'
     assert not persisted_notification.sent_by
     assert persisted_notification.notification_type == 'email'
+    provider_tasks.deliver_email.apply_async.assert_called_once_with([persisted_notification.id], queue='send-email')
 
 
 def test_should_use_email_template_subject_placeholders(sample_email_template_with_placeholders, mocker):
@@ -737,18 +715,18 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi
         encryption.encrypt(notification),
         now.strftime(DATETIME_FORMAT)
     )
-    provider_tasks.deliver_email.apply_async.assert_called_once_with(
+    persisted_notification = Notification.query.one()
-        [notification_id], queue='send-email'
-    )
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
-    assert persisted_notification.id == notification_id
     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
     assert persisted_notification.notification_type == 'email'
+    provider_tasks.deliver_email.apply_async.assert_called_once_with(
+        [persisted_notification.id], queue='send-email'
+    )
 
 
 def test_should_use_email_template_and_persist_without_personalisation(sample_email_template, mocker):
@@ -764,10 +742,7 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em
         encryption.encrypt(notification),
         now.strftime(DATETIME_FORMAT)
     )
-    provider_tasks.deliver_email.apply_async.assert_called_once_with([notification_id], queue='send-email')
+    persisted_notification = Notification.query.one()
-
-    persisted_notification = Notification.query.filter_by(id=notification_id).one()
-    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
@@ -777,6 +752,8 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em
     assert not persisted_notification.personalisation
     assert not persisted_notification.reference
     assert persisted_notification.notification_type == 'email'
+    provider_tasks.deliver_email.apply_async.assert_called_once_with([persisted_notification.id],
+                                                                     queue='send-email')
 
 
 def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, mocker):
@@ -786,7 +763,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 +778,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 +788,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()
@@ -825,9 +800,7 @@ def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_tem
             encryption.encrypt(notification),
             now.strftime(DATETIME_FORMAT)
         )
-    assert provider_tasks.deliver_email.apply_async.called is False
+    assert not provider_tasks.deliver_email.apply_async.called
     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)

tests/app/notifications/test_process_notification.py

@@ -1,3 +1,5 @@
+import datetime
+
 import pytest
 from boto3.exceptions import Boto3Error
 from sqlalchemy.exc import SQLAlchemyError
@@ -46,21 +48,44 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api
     assert NotificationHistory.query.count() == 1
 
 
-def test_persist_notification_throws_exception_when_missing_template(sample_template, sample_api_key):
+def test_persist_notification_throws_exception_when_missing_template(sample_api_key):
     assert Notification.query.count() == 0
     assert NotificationHistory.query.count() == 0
     with pytest.raises(SQLAlchemyError):
         persist_notification(template_id=None,
                              template_version=None,
                              recipient='+447111111111',
-                             service_id=sample_template.service.id,
+                             service_id=sample_api_key.service_id,
-                             personalisation=None, notification_type='sms',
+                             personalisation=None,
+                             notification_type='sms',
                              api_key_id=sample_api_key.id,
                              key_type=sample_api_key.key_type)
     assert Notification.query.count() == 0
     assert NotificationHistory.query.count() == 0
 
 
+def test_persist_notification_with_job_and_created(sample_job, sample_api_key):
+    assert Notification.query.count() == 0
+    assert NotificationHistory.query.count() == 0
+    created_at = datetime.datetime(2016, 11, 11, 16, 8, 18)
+    persist_notification(template_id=sample_job.template.id,
+                         template_version=sample_job.template.version,
+                         recipient='+447111111111',
+                         service_id=sample_job.service.id,
+                         personalisation=None, notification_type='sms',
+                         api_key_id=sample_api_key.id,
+                         key_type=sample_api_key.key_type,
+                         created_at=created_at,
+                         job_id=sample_job.id,
+                         job_row_number=10)
+    assert Notification.query.count() == 1
+    assert NotificationHistory.query.count() == 1
+    persisted_notification = Notification.query.all()[0]
+    assert persisted_notification.job_id == sample_job.id
+    assert persisted_notification.job_row_number == 10
+    assert persisted_notification.created_at == created_at
+
+
 @pytest.mark.parametrize('research_mode, queue, notification_type, key_type',
                          [(True, 'research-mode', 'sms', 'normal'),
                           (True, 'research-mode', 'email', 'normal'),

tests/app/test_model.py

@@ -1,3 +1,4 @@
+import uuid
 from datetime import datetime
 
 import pytest
@@ -9,74 +10,6 @@ from app.models import (
     MOBILE_TYPE, EMAIL_TYPE)
 
 
-def test_should_build_notification_from_minimal_set_of_api_derived_params(notify_api):
-    now = datetime.utcnow()
-
-    notification = {
-        'template': 'template',
-        'template_version': '1',
-        'to': 'someone',
-        'personalisation': {}
-    }
-    notification = Notification.from_api_request(
-        created_at=now.strftime(DATETIME_FORMAT),
-        notification=notification,
-        notification_id="notification_id",
-        service_id="service_id",
-        notification_type='SMS',
-        api_key_id='api_key_id',
-        key_type='key_type'
-    )
-    assert notification.created_at == now
-    assert notification.id == "notification_id"
-    assert notification.template_id == 'template'
-    assert notification.template_version == '1'
-    assert not notification.job_row_number
-    assert not notification.job_id
-    assert notification.to == 'someone'
-    assert notification.service_id == 'service_id'
-    assert notification.status == 'created'
-    assert not notification.personalisation
-    assert notification.notification_type == 'SMS'
-    assert notification.api_key_id == 'api_key_id'
-    assert notification.key_type == 'key_type'
-
-
-def test_should_build_notification_from_full_set_of_api_derived_params(notify_api):
-    now = datetime.utcnow()
-
-    notification = {
-        'template': 'template',
-        'template_version': '1',
-        'to': 'someone',
-        'personalisation': {'key': 'value'},
-        'job': 'job_id',
-        'row_number': 100
-    }
-    notification = Notification.from_api_request(
-        created_at=now.strftime(DATETIME_FORMAT),
-        notification=notification,
-        notification_id="notification_id",
-        service_id="service_id",
-        notification_type='SMS',
-        api_key_id='api_key_id',
-        key_type='key_type'
-    )
-    assert notification.created_at == now
-    assert notification.id == "notification_id"
-    assert notification.template_id == 'template'
-    assert notification.template_version == '1'
-    assert notification.job_row_number == 100
-    assert notification.job_id == 'job_id'
-    assert notification.to == 'someone'
-    assert notification.service_id == 'service_id'
-    assert notification.status == 'created'
-    assert notification.personalisation == {'key': 'value'}
-    assert notification.notification_type == 'SMS'
-    assert notification.api_key_id == 'api_key_id'
-    assert notification.key_type == 'key_type'
-
-
 @pytest.mark.parametrize('mobile_number', [
     '07700 900678',
     '+44 7700 900678'