diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 3ab7043a2..a4b4fd94b 100644 --- a/app/celery/tasks.py +++ b/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 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) - ) + if __sending_limits_for_job_exceeded(service, job, job_id): 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( - Notification.from_api_request( - created_at, notification, notification_id, service.id, SMS_TYPE, api_key_id, key_type - ) - ) + saved_notification = persist_notification(template_id=notification['template'], + template_version=notification['template_version'], + 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), - e - ) + "RETRY FAILED: task send_sms failed for notification".format(notification.get('job', None), + 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( - Notification.from_api_request( - created_at, notification, notification_id, service.id, EMAIL_TYPE, api_key_id, key_type - ) + saved_notification = persist_notification( + template_id=notification['template'], + 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), - e - ) + "RETRY FAILED: task send_email failed for notification".format(notification.get('job', None), + notification.get('row_number', None)), e) diff --git a/app/models.py b/app/models.py index 699ae2275..5e2d16f6f 100644 --- a/app/models.py +++ b/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' diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 83a3fcf5f..5df9e7b36 100644 --- a/app/notifications/process_notifications.py +++ b/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): - notification = Notification.from_v2_api_request(template_id=template_id, - template_version=template_version, - recipient=recipient, - service_id=service_id, - personalisation=personalisation, - notification_type=notification_type, - api_key_id=api_key_id, - key_type=key_type) + key_type, + created_at=None, + job_id=None, + job_row_number=None): + notification = Notification( + template_id=template_id, + template_version=template_version, + to=recipient, + 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 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 4191857c5..04b449cff 100644 --- a/tests/app/celery/test_tasks.py +++ b/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') - - notification_id = uuid.uuid4() + mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') 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( - [notification_id], - queue="send-sms" - ) - - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + persisted_notification = Notification.query.one() 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( - [notification_id], - queue="send-sms" - ) - - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + persisted_notification = Notification.query.one() 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, - notify_db_session, - mocker): +def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key(notify_db, + notify_db_session, + 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( - [notification_id], + persisted_notification = Notification.query.one() + 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_not_send_email_if_restricted_service_and_invalid_email_address_with_test_key(notify_db, - notify_db_session, - mocker): +def test_should_send_email_if_restricted_service_and_non_team_email_address_with_test_key(notify_db, + notify_db_session, + 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( - [notification_id], + persisted_notification = Notification.query.one() + 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): - Notification.query.filter_by(id=notification_id).one() + assert Notification.query.count() == 0 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): - Notification.query.filter_by(id=notification_id).one() + assert Notification.query.count() == 0 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( - [notification_id], - queue="send-sms" - ) - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + persisted_notification = Notification.query.one() 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): - persisted_notification = Notification.query.filter_by(id=notification_id).one() - print(persisted_notification) + assert Notification.query.count() == 0 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): - Notification.query.filter_by(id=notification_id).one() + assert Notification.query.count() == 0 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() - provider_tasks.deliver_email.apply_async.assert_called_once_with( - [notification_id], queue='send-email') - - assert persisted_notification.id == notification_id + persisted_notification = Notification.query.one() 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.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + persisted_notification = Notification.query.one() 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( - [notification_id], queue='send-email' - ) - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + persisted_notification = Notification.query.one() 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.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + persisted_notification = Notification.query.one() 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: - Notification.query.filter_by(id=notification_id).one() - assert 'No row was found for one' in str(e.value) + assert Notification.query.count() == 0 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: - Notification.query.filter_by(id=notification_id).one() - assert 'No row was found for one' in str(e.value) + assert Notification.query.count() == 0 diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index ddc559201..851865a6b 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/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, - personalisation=None, notification_type='sms', + service_id=sample_api_key.service_id, + 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'), diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 78827235a..abbd167a5 100644 --- a/tests/app/test_model.py +++ b/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'