diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index c599313ab..ec2866e1d 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -48,28 +48,26 @@ def retry_iteration_to_delay(retry=0): @notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5) -def send_sms_to_provider(self, service_id, notification_id, encrypted_notification): +def send_sms_to_provider(self, service_id, notification_id, encrypted_notification=None): task_start = monotonic() service = dao_fetch_service_by_id(service_id) provider = provider_to_use('sms', notification_id) notification = get_notification_by_id(notification_id) - notification_json = encryption.decrypt(encrypted_notification) - template = Template( dao_get_template_by_id(notification.template_id, notification.template_version).__dict__, - values=notification_json.get('personalisation', {}), + values={} if not notification.personalisation else notification.personalisation, prefix=service.name ) try: if service.research_mode: send_sms_response.apply_async( - (provider.get_name(), str(notification_id), notification_json['to']), queue='research-mode' + (provider.get_name(), str(notification_id), notification.to), queue='research-mode' ) else: provider.send_sms( - to=validate_and_format_phone_number(notification_json['to']), + to=validate_and_format_phone_number(notification.to), content=template.replaced, reference=str(notification_id) ) @@ -84,6 +82,7 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati notification.sent_at = datetime.utcnow() notification.sent_by = provider.get_name(), notification.content_char_count = template.replaced_content_count + notification.status = 'sending' dao_update_notification(notification) except SmsClientException as e: try: diff --git a/app/celery/tasks.py b/app/celery/tasks.py index b2090aa7e..ff74c98bb 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -155,7 +155,7 @@ def send_sms(self, service_id, notification_id, encrypted_notification, created_ service_id=service_id, job_id=notification.get('job', None), job_row_number=notification.get('row_number', None), - status='sending', + status='created', created_at=datetime.strptime(created_at, DATETIME_FORMAT), personalisation=notification.get('personalisation') ) diff --git a/app/models.py b/app/models.py index 6d8ed05e5..cca4b7d32 100644 --- a/app/models.py +++ b/app/models.py @@ -348,7 +348,7 @@ class Notification(db.Model): nullable=True, onupdate=datetime.datetime.utcnow) status = db.Column( - db.Enum(*NOTIFICATION_STATUS_TYPES, name='notify_status_type'), nullable=False, default='sending') + db.Enum(*NOTIFICATION_STATUS_TYPES, name='notify_status_type'), nullable=False, default='created') reference = db.Column(db.String, nullable=True, index=True) _personalisation = db.Column(db.String, nullable=True) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index f4a899ffd..45524a130 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -2,7 +2,7 @@ from celery.exceptions import MaxRetriesExceededError from mock import ANY, call -from app import statsd_client, mmg_client +from app import statsd_client, mmg_client, encryption from app.celery import provider_tasks from app.celery.provider_tasks import send_sms_to_provider from app.celery.tasks import provider_to_use @@ -83,94 +83,33 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( notify_db_session, sample_template_with_placeholders, mocker): - db_notification = sample_notification(notify_db, notify_db_session, template=sample_template_with_placeholders) + db_notification = sample_notification(notify_db, notify_db_session, template=sample_template_with_placeholders, + to_field="+447234123123", personalisation={"name": "Jo"}, + status='created') - notification = _notification_json( - sample_template_with_placeholders, - to="+447234123123", - personalisation={"name": "Jo"} - ) - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing_with_dates') mocker.patch('app.statsd_client.timing') - freezer = freeze_time("2016-01-01 11:09:00.00000") - freezer.start() - now = datetime.utcnow() - freezer.stop() - - freezer = freeze_time("2016-01-01 11:10:00.00000") - freezer.start() - send_sms_to_provider( db_notification.service_id, - db_notification.id, - "encrypted-in-reality" + db_notification.id ) - freezer.stop() mmg_client.send_sms.assert_called_once_with( to=format_phone_number(validate_phone_number("+447234123123")), content="Sample service: Hello Jo", reference=str(db_notification.id) ) - - statsd_client.incr.assert_called_once_with("notifications.tasks.send-sms-to-provider") - statsd_client.timing.assert_has_calls([ - call("notifications.tasks.send-sms-to-provider.task-time", ANY), - call("notifications.sms.total-time", ANY) - ]) - - notification = notifications_dao.get_notification( - db_notification.service_id, db_notification.id - ) + notification = Notification.query.filter_by(id=db_notification.id).one() assert notification.status == 'sending' - assert notification.sent_at > now + assert notification.sent_at <= datetime.utcnow() assert notification.sent_by == 'mmg' - - -def test_should_send_template_to_correct_sms_provider_and_persist( - notify_db, - notify_db_session, - sample_template, - mocker): - db_notification = sample_notification(notify_db, notify_db_session, template=sample_template) - - notification = _notification_json( - sample_template, - to="+447234123123" - ) - mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") - mocker.patch('app.statsd_client.incr') - mocker.patch('app.statsd_client.timing_with_dates') - mocker.patch('app.statsd_client.timing') - - freezer = freeze_time("2016-01-01 11:09:00.00000") - freezer.start() - now = datetime.utcnow() - freezer.stop() - - freezer = freeze_time("2016-01-01 11:10:00.00000") - freezer.start() - - send_sms_to_provider( - db_notification.service_id, - db_notification.id, - "encrypted-in-reality" - ) - freezer.stop() - - mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), - content="Sample service: This is a template", - reference=str(db_notification.id) - ) + assert notification.content_char_count == 24 + assert notification.personalisation == {"name": "Jo"} def test_send_sms_should_use_template_version_from_notification_not_latest( @@ -178,10 +117,9 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( notify_db_session, sample_template, mocker): - db_notification = sample_notification(notify_db, notify_db_session, template=sample_template) + db_notification = sample_notification(notify_db, notify_db_session, + template=sample_template, to_field='+447234123123') - notification = _notification_json(sample_template, '+447234123123') - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") version_on_notification = sample_template.version @@ -193,12 +131,9 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( t = dao_get_template_by_id(sample_template.id) assert t.version > version_on_notification - now = datetime.utcnow() - send_sms_to_provider( db_notification.service_id, - db_notification.id, - "encrypted-in-reality" + db_notification.id ) mmg_client.send_sms.assert_called_once_with( @@ -212,18 +147,12 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( assert persisted_notification.template_id == sample_template.id assert persisted_notification.template_version == version_on_notification assert persisted_notification.template_version != sample_template.version - assert persisted_notification.sent_at > now - assert persisted_notification.status == 'sending' - assert persisted_notification.sent_by == 'mmg' assert persisted_notification.content_char_count == len("Sample service: This is a template") + assert not persisted_notification.personalisation def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_service, sample_notification, mocker): - notification = _notification_json( - sample_notification.template, - to=sample_notification.to - ) - mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') @@ -232,11 +161,9 @@ def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_s notify_db.session.add(sample_service) notify_db.session.commit() - now = datetime.utcnow() send_sms_to_provider( sample_notification.service_id, - sample_notification.id, - "encrypted-in-reality" + sample_notification.id ) assert not mmg_client.send_sms.called send_sms_response.apply_async.assert_called_once_with( @@ -247,27 +174,22 @@ def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_s assert persisted_notification.to == sample_notification.to assert persisted_notification.template_id == sample_notification.template_id assert persisted_notification.status == 'sending' - assert persisted_notification.sent_at > now + assert persisted_notification.sent_at <= datetime.utcnow() assert persisted_notification.sent_by == 'mmg' + assert not persisted_notification.personalisation def test_should_update_provider_stats_on_success(notify_db, sample_service, sample_notification, mocker): provider_stats = provider_statistics_dao.get_provider_statistics(sample_service).all() assert len(provider_stats) == 0 - notification = _notification_json( - sample_notification.template, - to=sample_notification.to - ) - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') send_sms_to_provider( sample_notification.service_id, - sample_notification.id, - "encrypted-in-reality" + sample_notification.id ) updated_provider_stats = provider_statistics_dao.get_provider_statistics(sample_service).all() @@ -275,15 +197,11 @@ def test_should_update_provider_stats_on_success(notify_db, sample_service, samp assert updated_provider_stats[0].unit_count == 1 -def test_not_should_update_provider_stats_on_success(notify_db, sample_service, sample_notification, mocker): +def test_not_should_update_provider_stats_on_success_in_research_mode(notify_db, sample_service, sample_notification, + mocker): provider_stats = provider_statistics_dao.get_provider_statistics(sample_service).all() assert len(provider_stats) == 0 - notification = _notification_json( - sample_notification.template, - to=sample_notification.to - ) - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') @@ -294,8 +212,7 @@ def test_not_should_update_provider_stats_on_success(notify_db, sample_service, send_sms_to_provider( sample_notification.service_id, - sample_notification.id, - "encrypted-in-reality" + sample_notification.id ) updated_provider_stats = provider_statistics_dao.get_provider_statistics(sample_service).all() @@ -303,22 +220,15 @@ def test_not_should_update_provider_stats_on_success(notify_db, sample_service, def test_statsd_updates(notify_db, notify_db_session, sample_service, sample_notification, mocker): - notification = _notification_json( - sample_notification.template, - to=sample_notification.to - ) - mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing') - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') send_sms_to_provider( sample_notification.service_id, - sample_notification.id, - "encrypted-in-reality" + sample_notification.id ) statsd_client.incr.assert_called_once_with("notifications.tasks.send-sms-to-provider") @@ -332,50 +242,30 @@ def test_should_go_into_technical_error_if_exceeds_retries( notify_db, notify_db_session, sample_service, - sample_notification, mocker): - notification = _notification_json( - sample_notification.template, - to=sample_notification.to - ) + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + service=sample_service, status='created') mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing') - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms', side_effect=SmsClientException("EXPECTED")) mocker.patch('app.celery.provider_tasks.send_sms_to_provider.retry', side_effect=MaxRetriesExceededError()) send_sms_to_provider( - sample_notification.service_id, - sample_notification.id, - "encrypted-in-reality" + notification.service_id, + notification.id ) provider_tasks.send_sms_to_provider.retry.assert_called_with(queue='retry', countdown=10) assert statsd_client.incr.assert_not_called assert statsd_client.timing.assert_not_called - db_notification = Notification.query.get(sample_notification.id) + db_notification = Notification.query.filter_by(id=notification.id).one() assert db_notification.status == 'technical-failure' - notification_stats = NotificationStatistics.query.filter_by(service_id=sample_notification.service.id).first() + notification_stats = NotificationStatistics.query.filter_by(service_id=notification.service.id).first() assert notification_stats.sms_requested == 1 assert notification_stats.sms_failed == 1 - job = Job.query.get(sample_notification.job.id) + job = Job.query.get(notification.job.id) assert job.notification_count == 1 assert job.notifications_failed == 1 - - -def _notification_json(template, to, personalisation=None, job_id=None, row_number=None): - notification = { - "template": template.id, - "template_version": template.version, - "to": to, - } - if personalisation: - notification.update({"personalisation": personalisation}) - if job_id: - notification.update({"job": job_id}) - if row_number: - notification['row_number'] = row_number - return notification diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index fa6ca8efa..39452a2de 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -19,8 +19,9 @@ from app.celery.tasks import ( send_email ) from app.clients.email.aws_ses import AwsSesClientException -from app.dao import notifications_dao, jobs_dao, provider_details_dao +from app.dao import jobs_dao, provider_details_dao from app.dao.provider_statistics_dao import get_provider_statistics +from app.models import Notification from tests.app import load_example_csv from tests.app.conftest import ( sample_service, @@ -312,7 +313,7 @@ def test_should_process_all_sms_job(sample_job, def test_should_send_template_to_correct_sms_task_and_persist(sample_template_with_placeholders, mocker): notification = _notification_json(sample_template_with_placeholders, to="+447234123123", personalisation={"name": "Jo"}) - mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing_with_dates') mocker.patch('app.statsd_client.timing') @@ -320,72 +321,73 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi notification_id = uuid.uuid4() - freezer = freeze_time("2016-01-01 11:09:00.00000") - freezer.start() - now = datetime.utcnow() - freezer.stop() - - freezer = freeze_time("2016-01-01 11:10:00.00000") - freezer.start() - send_sms( sample_template_with_placeholders.service_id, notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) + encryption.encrypt(notification), + datetime.utcnow().strftime(DATETIME_FORMAT) ) - freezer.stop() statsd_client.timing.assert_called_once_with("notifications.tasks.send-sms.task-time", ANY) provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( (sample_template_with_placeholders.service_id, notification_id, - "encrypted-in-reality"), + encryption.encrypt(notification)), queue="sms" ) statsd_client.incr.assert_called_once_with("notifications.tasks.send-sms") - persisted_notification = notifications_dao.get_notification( - sample_template_with_placeholders.service_id, notification_id - ) + 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 - assert persisted_notification.status == 'sending' - assert persisted_notification.created_at == now + assert persisted_notification.status == 'created' + assert persisted_notification.created_at <= datetime.utcnow() assert not persisted_notification.sent_at assert not persisted_notification.sent_by assert not persisted_notification.job_id + assert persisted_notification.personalisation == {'name': 'Jo'} + assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notify_db_session, mocker): user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900890") 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, "+447700900890") # The user’s own number, but in a different format - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') notification_id = uuid.uuid4() - now = datetime.utcnow() + encrypt_notification = encryption.encrypt(notification) send_sms( service.id, notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) + encrypt_notification, + datetime.utcnow().strftime(DATETIME_FORMAT) ) provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( (service.id, notification_id, - "encrypted-in-reality"), + encrypt_notification), queue="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 + assert persisted_notification.status == 'created' + assert persisted_notification.created_at <= datetime.utcnow() + assert not persisted_notification.sent_at + assert not persisted_notification.sent_by + assert not persisted_notification.job_id + assert not persisted_notification.personalisation + 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") @@ -393,20 +395,18 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, template = sample_template(notify_db, notify_db_session, service=service) notification = _notification_json(template, "07700 900849") - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') notification_id = uuid.uuid4() - now = datetime.utcnow() send_sms( service.id, notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) + encryption.encrypt(notification), + datetime.utcnow().strftime(DATETIME_FORMAT) ) provider_tasks.send_sms_to_provider.apply_async.assert_not_called() with pytest.raises(NoResultFound): - notifications_dao.get_notification(service.id, notification_id) + Notification.query.filter_by(id=notification_id).one() def test_should_send_email_if_restricted_service_and_valid_email(notify_db, notify_db_session, mocker): @@ -419,7 +419,6 @@ def test_should_send_email_if_restricted_service_and_valid_email(notify_db, noti template_type='email') notification = _notification_json(template, "test@restricted.com") - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email', return_value="1234") notification_id = uuid.uuid4() @@ -427,7 +426,7 @@ def test_should_send_email_if_restricted_service_and_valid_email(notify_db, noti send_email( service.id, notification_id, - "encrypted-in-reality", + encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) @@ -449,54 +448,50 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address(n ) notification = _notification_json(template, to="test@example.com") - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email') notification_id = uuid.uuid4() - now = datetime.utcnow() send_email( service.id, notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) + encryption.encrypt(notification), + datetime.utcnow().strftime(DATETIME_FORMAT) ) aws_ses_client.send_email.assert_not_called() with pytest.raises(NoResultFound): - notifications_dao.get_notification(service.id, notification_id) + Notification.query.filter_by(id=notification_id).one() -def test_should_send_template_to_and_persist_with_job_id(sample_job, mocker): +def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, mocker): notification = _notification_json( sample_job.template, to="+447234123123", job_id=sample_job.id, row_number=2) - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') notification_id = uuid.uuid4() - now = datetime.utcnow() send_sms( sample_job.service.id, notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) + encryption.encrypt(notification), + datetime.utcnow().strftime(DATETIME_FORMAT) ) provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( (sample_job.service.id, notification_id, - "encrypted-in-reality"), + encryption.encrypt(notification)), queue="sms" ) - persisted_notification = notifications_dao.get_notification(sample_job.template.service_id, notification_id) + 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 == 'sending' + assert persisted_notification.status == 'created' assert not persisted_notification.sent_at - assert persisted_notification.created_at == now + assert persisted_notification.created_at <= datetime.utcnow() assert not persisted_notification.sent_by assert persisted_notification.job_row_number == 2 @@ -507,7 +502,6 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh "my_email@my_email.com", {"name": "Jo"}, row_number=1) - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing_with_dates') mocker.patch('app.statsd_client.timing') @@ -523,11 +517,10 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh freezer = freeze_time("2016-01-01 11:10:00.00000") freezer.start() - send_email( sample_email_template_with_placeholders.service_id, notification_id, - "encrypted-in-reality", + encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) freezer.stop() @@ -535,7 +528,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh aws_ses_client.send_email.assert_called_once_with( '"Sample service" ', "my_email@my_email.com", - notification['personalisation']['name'], + "Jo", body="Hello Jo", html_body=AnyStringWith("Hello Jo"), reply_to_addresses=None @@ -548,9 +541,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh datetime(2016, 1, 1, 11, 9, 0, 00000) ) statsd_client.timing.assert_called_once_with("notifications.tasks.send-email.task-time", ANY) - persisted_notification = notifications_dao.get_notification( - sample_email_template_with_placeholders.service_id, notification_id - ) + 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' @@ -561,6 +552,8 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh assert persisted_notification.status == 'sending' assert persisted_notification.sent_by == 'ses' assert persisted_notification.job_row_number == 1 + assert persisted_notification.personalisation == {'name': 'Jo'} + assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) def test_send_email_should_use_template_version_from_job_not_latest(sample_email_template, mocker): @@ -593,7 +586,7 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email reply_to_addresses=None ) - persisted_notification = notifications_dao.get_notification(sample_email_template.service_id, notification_id) + 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 @@ -607,7 +600,6 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email def test_should_use_email_template_subject_placeholders(sample_email_template_with_placeholders, mocker): notification = _notification_json(sample_email_template_with_placeholders, "my_email@my_email.com", {"name": "Jo"}) - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email', return_value="1234") mocker.patch('app.aws_ses_client.get_name', return_value='ses') @@ -616,7 +608,7 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi send_email( sample_email_template_with_placeholders.service_id, notification_id, - "encrypted-in-reality", + encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) aws_ses_client.send_email.assert_called_once_with( @@ -627,9 +619,7 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi html_body=AnyStringWith("Hello Jo"), reply_to_addresses=None ) - persisted_notification = notifications_dao.get_notification( - sample_email_template_with_placeholders.service_id, notification_id - ) + 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 @@ -637,30 +627,12 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi assert persisted_notification.sent_at > now assert persisted_notification.status == 'sending' assert persisted_notification.sent_by == 'ses' - - -def test_should_use_email_template_and_persist_ses_reference(sample_email_template_with_placeholders, mocker): - notification = _notification_json(sample_email_template_with_placeholders, "my_email@my_email.com", {"name": "Jo"}) - mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.aws_ses_client.send_email', return_value='reference') - - notification_id = uuid.uuid4() - now = datetime.utcnow() - send_email( - sample_email_template_with_placeholders.service_id, - notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) - ) - persisted_notification = notifications_dao.get_notification( - sample_email_template_with_placeholders.service_id, notification_id - ) - assert persisted_notification.reference == 'reference' + assert persisted_notification.personalisation == {"name": "Jo"} + assert persisted_notification.reference == '1234' def test_should_use_email_template_and_persist_without_personalisation(sample_email_template, mocker): - mocker.patch('app.encryption.decrypt', - return_value=_notification_json(sample_email_template, "my_email@my_email.com")) + notification = _notification_json(sample_email_template, "my_email@my_email.com") mocker.patch('app.aws_ses_client.send_email', return_value="ref") mocker.patch('app.aws_ses_client.get_name', return_value='ses') @@ -669,7 +641,7 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em send_email( sample_email_template.service_id, notification_id, - "encrypted-in-reality", + encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) aws_ses_client.send_email.assert_called_once_with( @@ -680,14 +652,23 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em html_body=AnyStringWith("This is a template"), reply_to_addresses=None ) + 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 + assert persisted_notification.sent_at > now + assert persisted_notification.status == 'sending' + assert persisted_notification.sent_by == 'ses' + assert not persisted_notification.personalisation + assert persisted_notification.reference == 'ref' -def test_should_persist_notification_as_failed_if_database_fails(sample_template, mocker): +def test_should_go_to_retry_queue_if_database_errors(sample_template, mocker): notification = _notification_json(sample_template, "+447234123123") expected_exception = SQLAlchemyError() - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.celery.provider_tasks.send_sms_to_provider.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) @@ -699,20 +680,19 @@ def test_should_persist_notification_as_failed_if_database_fails(sample_template send_sms( sample_template.service_id, notification_id, - "encrypted-in-reality", + encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) provider_tasks.send_sms_to_provider.apply_async.assert_not_called() tasks.send_sms.retry.assert_called_with(exc=expected_exception, queue='retry') with pytest.raises(NoResultFound) as e: - notifications_dao.get_notification(sample_template.service_id, notification_id) + Notification.query.filter_by(id=notification_id).one() assert 'No row was found for one' in str(e.value) def test_should_persist_notification_as_failed_if_email_client_fails(sample_email_template, mocker): notification = _notification_json(sample_email_template, "my_email@my_email.com") - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email', side_effect=AwsSesClientException()) mocker.patch('app.aws_ses_client.get_name', return_value="ses") @@ -723,7 +703,7 @@ def test_should_persist_notification_as_failed_if_email_client_fails(sample_emai send_email( sample_email_template.service_id, notification_id, - "encrypted-in-reality", + encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) aws_ses_client.send_email.assert_called_once_with( @@ -734,7 +714,7 @@ def test_should_persist_notification_as_failed_if_email_client_fails(sample_emai html_body=AnyStringWith(sample_email_template.content), reply_to_addresses=None ) - persisted_notification = notifications_dao.get_notification(sample_email_template.service_id, notification_id) + 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 @@ -742,12 +722,11 @@ def test_should_persist_notification_as_failed_if_email_client_fails(sample_emai assert persisted_notification.status == 'technical-failure' assert persisted_notification.created_at == now assert persisted_notification.sent_by == 'ses' - assert persisted_notification.sent_at > now + assert persisted_notification.sent_at >= now -def test_should_not_send_email_if_db_peristance_failed(sample_email_template, mocker): +def test_should_not_send_email_if_db_persistance_failed(sample_email_template, mocker): notification = _notification_json(sample_email_template, "my_email@my_email.com") - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email') mocker.patch('app.db.session.add', side_effect=SQLAlchemyError()) now = datetime.utcnow() @@ -757,13 +736,10 @@ def test_should_not_send_email_if_db_peristance_failed(sample_email_template, mo send_email( sample_email_template.service_id, notification_id, - "encrypted-in-reality", + encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) aws_ses_client.send_email.assert_not_called() - with pytest.raises(NoResultFound) as e: - notifications_dao.get_notification(sample_email_template.service_id, notification_id) - assert 'No row was found for one' in str(e.value) def test_process_email_job_should_use_reply_to_email_if_present(sample_email_job, mocker, mock_celery_remove_job): @@ -823,7 +799,7 @@ def test_should_call_send_email_response_task_if_research_mode( ('ses', str(reference), 'john@smith.com'), queue="research-mode" ) - persisted_notification = notifications_dao.get_notification(sample_service.id, notification_id) + persisted_notification = Notification.query.filter_by(id=notification_id).one() assert persisted_notification.id == notification_id assert persisted_notification.to == 'john@smith.com' assert persisted_notification.template_id == sample_email_template.id @@ -834,7 +810,7 @@ def test_should_call_send_email_response_task_if_research_mode( assert persisted_notification.reference == str(reference) -def test_should_call_send_not_update_provider_email_stats_if_research_mode( +def test_should_use_research_mode_task_and_not_update_provider_stats( notify_db, sample_service, sample_email_template, @@ -848,7 +824,6 @@ def test_should_call_send_not_update_provider_email_stats_if_research_mode( reference = uuid.uuid4() mocker.patch('app.uuid.uuid4', return_value=reference) - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email') mocker.patch('app.aws_ses_client.get_name', return_value="ses") mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async') @@ -866,7 +841,7 @@ def test_should_call_send_not_update_provider_email_stats_if_research_mode( send_email( sample_service.id, notification_id, - "encrypted-in-reality", + encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) assert not aws_ses_client.send_email.called @@ -877,69 +852,3 @@ def test_should_call_send_not_update_provider_email_stats_if_research_mode( assert not get_provider_statistics( sample_email_template.service, providers=[ses_provider.identifier]).first() - - -def test_email_template_personalisation_persisted(sample_email_template_with_placeholders, mocker): - encrypted_notification = encryption.encrypt(_notification_json( - sample_email_template_with_placeholders, - "my_email@my_email.com", - {"name": "Jo"}, - row_number=1)) - mocker.patch('app.statsd_client.incr') - mocker.patch('app.statsd_client.timing_with_dates') - mocker.patch('app.statsd_client.timing') - mocker.patch('app.aws_ses_client.get_name', return_value='ses') - mocker.patch('app.aws_ses_client.send_email', return_value='ses') - - notification_id = uuid.uuid4() - - send_email( - sample_email_template_with_placeholders.service_id, - notification_id, - encrypted_notification, - datetime.utcnow().strftime(DATETIME_FORMAT) - ) - - persisted_notification = notifications_dao.get_notification( - sample_email_template_with_placeholders.service_id, notification_id - ) - - assert persisted_notification.id == notification_id - assert persisted_notification.personalisation == {"name": "Jo"} - # personalisation data is encrypted in db - assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) - - -def test_sms_template_personalisation_persisted(sample_template_with_placeholders, mocker): - - encrypted_notification = encryption.encrypt(_notification_json(sample_template_with_placeholders, - to="+447234123123", personalisation={"name": "Jo"})) - mocker.patch('app.statsd_client.incr') - mocker.patch('app.statsd_client.timing_with_dates') - mocker.patch('app.statsd_client.timing') - mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') - - notification_id = uuid.uuid4() - - send_sms( - sample_template_with_placeholders.service_id, - notification_id, - encrypted_notification, - datetime.utcnow().strftime(DATETIME_FORMAT) - ) - - provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( - (sample_template_with_placeholders.service.id, - notification_id, - encrypted_notification), - queue="sms" - ) - persisted_notification = notifications_dao.get_notification( - sample_template_with_placeholders.service_id, notification_id - ) - - assert persisted_notification.id == notification_id - assert persisted_notification.to == '+447234123123' - assert persisted_notification.personalisation == {"name": "Jo"} - # personalisation data is encrypted in db - assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 1b1a74c58..d134118df 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -39,7 +39,7 @@ from tests.app.conftest import (sample_notification) def test_should_by_able_to_update_status_by_reference(sample_email_template, ses_provider): - data = _notification_json(sample_email_template) + data = _notification_json(sample_email_template, status='sending') notification = Notification(**data) dao_create_notification( @@ -56,7 +56,7 @@ def test_should_by_able_to_update_status_by_reference(sample_email_template, ses def test_should_by_able_to_update_status_by_id(sample_template, sample_job, mmg_provider): - data = _notification_json(sample_template, job_id=sample_job.id) + data = _notification_json(sample_template, job_id=sample_job.id, status='sending') notification = Notification(**data) dao_create_notification(notification, sample_template.template_type) assert Notification.query.get(notification.id).status == 'sending' @@ -82,7 +82,7 @@ def test_should_update_status_if_created(notify_db, notify_db_session): def test_should_by_able_to_update_status_by_id_from_pending_to_delivered(sample_template, sample_job): - data = _notification_json(sample_template, job_id=sample_job.id) + data = _notification_json(sample_template, job_id=sample_job.id, status='sending') notification = Notification(**data) dao_create_notification(notification, sample_template.template_type) assert Notification.query.get(notification.id).status == 'sending' @@ -98,7 +98,7 @@ def test_should_by_able_to_update_status_by_id_from_pending_to_delivered(sample_ def test_should_by_able_to_update_status_by_id_from_pending_to_temporary_failure(sample_template, sample_job): - data = _notification_json(sample_template, job_id=sample_job.id) + data = _notification_json(sample_template, job_id=sample_job.id, status='sending') notification = Notification(**data) dao_create_notification(notification, sample_template.template_type) assert Notification.query.get(notification.id).status == 'sending' @@ -117,7 +117,7 @@ def test_should_by_able_to_update_status_by_id_from_pending_to_temporary_failure def test_should_by_able_to_update_status_by_id_from_sending_to_permanent_failure(sample_template, sample_job): - data = _notification_json(sample_template, job_id=sample_job.id) + data = _notification_json(sample_template, job_id=sample_job.id, status='sending') notification = Notification(**data) dao_create_notification(notification, sample_template.template_type) assert Notification.query.get(notification.id).status == 'sending' @@ -132,15 +132,14 @@ def test_should_by_able_to_update_status_by_id_from_sending_to_permanent_failure _assert_job_stats(sample_job.id, sent=1, count=1, delivered=0, failed=1) -def test_should_not_update_status_one_notification_status_is_delivered(sample_email_template, sample_job, ses_provider): - data = _notification_json(sample_email_template, job_id=sample_job.id) - - notification = Notification(**data) - dao_create_notification( - notification, - sample_email_template.template_type) - +def test_should_not_update_status_one_notification_status_is_delivered(notify_db, notify_db_session, + sample_email_template, + ses_provider): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_email_template, + status='sending') assert Notification.query.get(notification.id).status == "sending" + update_provider_stats( notification.id, 'email', @@ -156,16 +155,18 @@ def test_should_not_update_status_one_notification_status_is_delivered(sample_em _assert_job_stats(notification.job_id, sent=1, count=1, delivered=1, failed=0) -def test_should_be_able_to_record_statistics_failure_for_sms(sample_notification): - assert Notification.query.get(sample_notification.id).status == 'sending' - assert update_notification_status_by_id(sample_notification.id, 'permanent-failure', 'failure') - assert Notification.query.get(sample_notification.id).status == 'permanent-failure' - _assert_notification_stats(sample_notification.service_id, sms_requested=1, sms_delivered=0, sms_failed=1) - _assert_job_stats(sample_notification.job_id, sent=1, count=1, delivered=0, failed=1) +def test_should_be_able_to_record_statistics_failure_for_sms(notify_db, notify_db_session,): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, status='sending') + assert Notification.query.get(notification.id).status == 'sending' + + assert update_notification_status_by_id(notification.id, 'permanent-failure', 'failure') + assert Notification.query.get(notification.id).status == 'permanent-failure' + _assert_notification_stats(notification.service_id, sms_requested=1, sms_delivered=0, sms_failed=1) + _assert_job_stats(notification.job_id, sent=1, count=1, delivered=0, failed=1) def test_should_be_able_to_record_statistics_failure_for_email(sample_email_template, sample_job, ses_provider): - data = _notification_json(sample_email_template, job_id=sample_job.id) + data = _notification_json(sample_email_template, job_id=sample_job.id, status='sending') notification = Notification(**data) dao_create_notification(notification, sample_email_template.template_type) @@ -301,6 +302,46 @@ def test_should_be_able_to_get_all_statistics_for_a_service_for_several_days_pre assert stats[1].day == seven_days_ago.date() +def test_create_notification_creates_notification_with_personalisation(notify_db, notify_db_session, + sample_template_with_placeholders, + sample_job, mmg_provider): + + assert Notification.query.count() == 0 + assert NotificationStatistics.query.count() == 0 + assert TemplateStatistics.query.count() == 0 + + data = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_template_with_placeholders, + job=sample_job, + personalisation={'name': 'Jo'}, + status='created') + + assert Notification.query.count() == 1 + notification_from_db = Notification.query.all()[0] + assert notification_from_db.id + assert data.to == notification_from_db.to + assert data.job_id == notification_from_db.job_id + assert data.service == notification_from_db.service + assert data.template == notification_from_db.template + assert data.template_version == notification_from_db.template_version + assert data.created_at == notification_from_db.created_at + assert 'created' == notification_from_db.status + assert {'name': 'Jo'} == notification_from_db.personalisation + _assert_job_stats(sample_job.id, sent=1, count=1, delivered=0, failed=0) + + stats = NotificationStatistics.query.filter( + NotificationStatistics.service_id == sample_template_with_placeholders.service.id).first() + assert stats.emails_requested == 0 + assert stats.sms_requested == 1 + + template_stats = TemplateStatistics.query.filter( + TemplateStatistics.service_id == sample_template_with_placeholders.service.id, + TemplateStatistics.template_id == sample_template_with_placeholders.id).first() + assert template_stats.service_id == sample_template_with_placeholders.service.id + assert template_stats.template_id == sample_template_with_placeholders.id + assert template_stats.usage_count == 1 + + def test_save_notification_creates_sms_and_template_stats(sample_template, sample_job, mmg_provider): assert Notification.query.count() == 0 assert NotificationStatistics.query.count() == 0 @@ -320,7 +361,7 @@ def test_save_notification_creates_sms_and_template_stats(sample_template, sampl assert data['template'] == notification_from_db.template assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at - assert 'sending' == notification_from_db.status + assert 'created' == notification_from_db.status _assert_job_stats(sample_job.id, sent=1, count=1, delivered=0, failed=0) stats = NotificationStatistics.query.filter(NotificationStatistics.service_id == sample_template.service.id).first() @@ -353,7 +394,7 @@ def test_save_notification_and_create_email_and_template_stats(sample_email_temp assert data['template'] == notification_from_db.template assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at - assert 'sending' == notification_from_db.status + assert 'created' == notification_from_db.status _assert_job_stats(sample_job.id, sent=1, count=1, delivered=0, failed=0) stats = NotificationStatistics.query.filter( @@ -490,7 +531,7 @@ def test_save_notification_and_increment_job(sample_template, sample_job, mmg_pr assert data['template'] == notification_from_db.template assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at - assert 'sending' == notification_from_db.status + assert 'created' == notification_from_db.status assert Job.query.get(sample_job.id).notifications_sent == 1 notification_2 = Notification(**data) @@ -541,7 +582,7 @@ def test_save_notification_and_increment_correct_job(notify_db, notify_db_sessio assert data['template'] == notification_from_db.template assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at - assert 'sending' == notification_from_db.status + assert 'created' == notification_from_db.status assert job_1.id != job_2.id _assert_job_stats(job_id=job_1.id, sent=1, count=1) _assert_job_stats(job_id=job_2.id, sent=0, count=1) @@ -562,7 +603,7 @@ def test_save_notification_with_no_job(sample_template, mmg_provider): assert data['template'] == notification_from_db.template assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at - assert 'sending' == notification_from_db.status + assert 'created' == notification_from_db.status def test_get_notification(sample_notification): @@ -586,7 +627,8 @@ def test_save_notification_no_job_id(sample_template, mmg_provider): assert data['service'] == notification_from_db.service assert data['template'] == notification_from_db.template assert data['template_version'] == notification_from_db.template_version - assert 'sending' == notification_from_db.status + assert 'created' == notification_from_db.status + assert data.get('job_id') is None def test_get_notification_for_job(sample_notification): @@ -924,7 +966,7 @@ def test_sms_fragment_count(char_count, expected_sms_fragment_count): assert get_sms_fragment_count(char_count) == expected_sms_fragment_count -def _notification_json(sample_template, job_id=None, id=None): +def _notification_json(sample_template, job_id=None, id=None, status=None): data = { 'to': '+44709123456', 'service': sample_template.service, @@ -933,12 +975,14 @@ def _notification_json(sample_template, job_id=None, id=None): 'template_id': sample_template.id, 'template_version': sample_template.version, 'created_at': datetime.utcnow(), - 'content_char_count': 160 + 'content_char_count': 160, } if job_id: data.update({'job_id': job_id}) if id: data.update({'id': id}) + if status: + data.update({'status': status}) return data diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 76157ccad..a7fc33593 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -1,4 +1,4 @@ -from datetime import datetime +from datetime import datetime, timedelta import uuid import pytest @@ -1283,7 +1283,8 @@ def test_get_notifications_for_service_returns_merged_template_content(notify_ap notify_db_session, service=sample_template_with_placeholders.service, template=sample_template_with_placeholders, - personalisation={"name": "merged with first"}) + personalisation={"name": "merged with first"}, + created_at=datetime.utcnow() - timedelta(seconds=1)) create_sample_notification(notify_db, notify_db_session,