From 276ca159192ff8b0256443ca38d5eecb212cf6ae Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Wed, 1 Jun 2016 12:42:19 +0100 Subject: [PATCH 1/2] Provider stats only updated if the provider successfully sends the message. --- app/celery/tasks.py | 15 +++++- app/dao/notifications_dao.py | 46 +++++++++---------- tests/app/conftest.py | 4 +- tests/app/dao/test_notification_dao.py | 29 +++++++++--- ...t_notifications_dao_provider_statistics.py | 38 ++++++++++----- 5 files changed, 85 insertions(+), 47 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index f9540fa9d..d40620e13 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -36,7 +36,7 @@ from app.dao.notifications_dao import ( dao_update_notification, delete_notifications_created_more_than_a_week_ago, dao_get_notification_statistics_for_service_and_day, - update_notification_reference_by_id + update_notification_after_sent_to_provider ) from app.dao.jobs_dao import ( @@ -262,6 +262,12 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): reference=str(notification_id) ) + update_notification_after_sent_to_provider( + notification_id, + 'sms', + provider.get_name() + ) + except SmsClientException as e: current_app.logger.error( "SMS notification {} failed".format(notification_id) @@ -331,7 +337,12 @@ def send_email(service_id, notification_id, from_address, encrypted_notification reply_to_addresses=reply_to_addresses, ) - update_notification_reference_by_id(notification_id, reference) + update_notification_after_sent_to_provider( + notification_id, + 'email', + provider.get_name(), + reference=reference + ) except EmailClientException as e: current_app.logger.exception(e) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index f93e062ad..eecee12a6 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -176,22 +176,6 @@ def dao_create_notification(notification, notification_type, provider_identifier service_id=notification.service_id) db.session.add(template_stats) - update_count = db.session.query(ProviderStatistics).filter_by( - day=date.today(), - service_id=notification.service_id, - provider_id=provider.id - ).update({'unit_count': ProviderStatistics.unit_count + ( - 1 if notification_type == TEMPLATE_TYPE_EMAIL else get_sms_fragment_count(notification.content_char_count))}) - - if update_count == 0: - provider_stats = ProviderStatistics( - day=notification.created_at.date(), - service_id=notification.service_id, - provider_id=provider.id, - unit_count=1 if notification_type == TEMPLATE_TYPE_EMAIL else get_sms_fragment_count( - notification.content_char_count)) - db.session.add(provider_stats) - db.session.add(notification) @@ -293,14 +277,28 @@ def dao_update_notification(notification): db.session.commit() -def update_notification_reference_by_id(id, reference): - count = db.session.query(Notification).filter_by( - id=id - ).update({ - Notification.reference: reference - }) - db.session.commit() - return count +@transactional +def update_notification_after_sent_to_provider(id_, notification_type, provider_name, reference=None): + provider = ProviderDetails.query.filter_by(identifier=provider_name).one() + notification = Notification.query.filter(Notification.id == id_).one() + if reference: + notification.reference = reference + db.session.add(notification) + update_count = db.session.query(ProviderStatistics).filter_by( + day=date.today(), + service_id=notification.service_id, + provider_id=provider.id + ).update({'unit_count': ProviderStatistics.unit_count + ( + 1 if notification_type == TEMPLATE_TYPE_EMAIL else get_sms_fragment_count(notification.content_char_count))}) + + if update_count == 0: + provider_stats = ProviderStatistics( + day=notification.created_at.date(), + service_id=notification.service_id, + provider_id=provider.id, + unit_count=1 if notification_type == TEMPLATE_TYPE_EMAIL else get_sms_fragment_count( + notification.content_char_count)) + db.session.add(provider_stats) def get_notification_for_job(service_id, job_id, notification_id): diff --git a/tests/app/conftest.py b/tests/app/conftest.py index cdb4cb0ec..bd49294b3 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -328,7 +328,7 @@ def sample_notification(notify_db, notification_id = uuid.uuid4() if provider_name is None: - provider = mmg_provider() if template.template_type == 'sms' else ses_provider() + provider_name = mmg_provider().identifier if template.template_type == 'sms' else ses_provider().identifier if to_field: to = to_field @@ -352,7 +352,7 @@ def sample_notification(notify_db, data['job_row_number'] = job_row_number notification = Notification(**data) if create: - dao_create_notification(notification, template.template_type, provider.identifier) + dao_create_notification(notification, template.template_type, provider_name) return notification diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 1a55e3849..a26dbbaa6 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -25,7 +25,7 @@ from app.dao.notifications_dao import ( delete_notifications_created_more_than_a_week_ago, dao_get_notification_statistics_for_service_and_day, update_notification_status_by_id, - update_notification_reference_by_id, + update_notification_after_sent_to_provider, update_notification_status_by_reference, dao_get_template_statistics_for_service, get_notifications_for_service @@ -36,10 +36,13 @@ from notifications_utils.template import get_sms_fragment_count from tests.app.conftest import (sample_notification) -def test_should_by_able_to_update_reference_by_id(sample_notification): +def test_should_by_able_to_update_reference_by_notification_id(sample_notification, mmg_provider): assert not Notification.query.get(sample_notification.id).reference - count = update_notification_reference_by_id(sample_notification.id, 'reference') - assert count == 1 + update_notification_after_sent_to_provider( + sample_notification.id, + 'sms', + mmg_provider.identifier, + reference='reference') assert Notification.query.get(sample_notification.id).reference == 'reference' @@ -53,7 +56,11 @@ def test_should_by_able_to_update_status_by_reference(sample_email_template, ses ses_provider.identifier) assert Notification.query.get(notification.id).status == "sending" - update_notification_reference_by_id(notification.id, 'reference') + update_notification_after_sent_to_provider( + notification.id, + 'email', + ses_provider.identifier, + reference="reference") update_notification_status_by_reference('reference', 'delivered', 'delivered') assert Notification.query.get(notification.id).status == 'delivered' _assert_notification_stats(notification.service_id, emails_delivered=1, emails_requested=1, emails_failed=0) @@ -140,7 +147,11 @@ def test_should_not_update_status_one_notification_status_is_delivered(sample_em ses_provider.identifier) assert Notification.query.get(notification.id).status == "sending" - update_notification_reference_by_id(notification.id, 'reference') + update_notification_after_sent_to_provider( + notification.id, + 'email', + ses_provider.identifier, + reference='reference') update_notification_status_by_reference('reference', 'delivered', 'delivered') assert Notification.query.get(notification.id).status == 'delivered' @@ -163,7 +174,11 @@ def test_should_be_able_to_record_statistics_failure_for_email(sample_email_temp notification = Notification(**data) dao_create_notification(notification, sample_email_template.template_type, ses_provider.identifier) - update_notification_reference_by_id(notification.id, 'reference') + update_notification_after_sent_to_provider( + notification.id, + 'email', + ses_provider.identifier, + reference='reference') count = update_notification_status_by_reference('reference', 'failed', 'failure') assert count == 1 assert Notification.query.get(notification.id).status == 'failed' diff --git a/tests/app/dao/test_notifications_dao_provider_statistics.py b/tests/app/dao/test_notifications_dao_provider_statistics.py index cc166edd1..24d2fc6dd 100644 --- a/tests/app/dao/test_notifications_dao_provider_statistics.py +++ b/tests/app/dao/test_notifications_dao_provider_statistics.py @@ -1,6 +1,6 @@ from datetime import (date, timedelta) from app.models import ProviderStatistics - +from app.dao.notifications_dao import update_notification_after_sent_to_provider from app.dao.provider_statistics_dao import ( get_provider_statistics, get_fragment_count) from tests.app.conftest import sample_notification as create_sample_notification @@ -10,10 +10,11 @@ def test_should_update_provider_statistics_sms(notify_db, notify_db_session, sample_template, mmg_provider): - create_sample_notification( + n1 = create_sample_notification( notify_db, notify_db_session, template=sample_template) + update_notification_after_sent_to_provider(n1.id, 'sms', mmg_provider.identifier) provider_stats = get_provider_statistics( sample_template.service, providers=[mmg_provider.identifier]).one() @@ -24,10 +25,11 @@ def test_should_update_provider_statistics_email(notify_db, notify_db_session, sample_email_template, ses_provider): - create_sample_notification( + n1 = create_sample_notification( notify_db, notify_db_session, template=sample_email_template) + update_notification_after_sent_to_provider(n1.id, 'email', ses_provider.identifier, reference="reference") provider_stats = get_provider_statistics( sample_email_template.service, providers=[ses_provider.identifier]).one() @@ -38,21 +40,27 @@ def test_should_update_provider_statistics_sms_multi(notify_db, notify_db_session, sample_template, mmg_provider): - create_sample_notification( + n1 = create_sample_notification( notify_db, notify_db_session, template=sample_template, + provider_name=mmg_provider.identifier, content_char_count=160) - create_sample_notification( + update_notification_after_sent_to_provider(n1.id, 'sms', mmg_provider.identifier) + n2 = create_sample_notification( notify_db, notify_db_session, template=sample_template, + provider_name=mmg_provider.identifier, content_char_count=161) - create_sample_notification( + update_notification_after_sent_to_provider(n2.id, 'sms', mmg_provider.identifier) + n3 = create_sample_notification( notify_db, notify_db_session, template=sample_template, + provider_name=mmg_provider.identifier, content_char_count=307) + update_notification_after_sent_to_provider(n3.id, 'sms', mmg_provider.identifier) provider_stats = get_provider_statistics( sample_template.service, providers=[mmg_provider.identifier]).one() @@ -63,18 +71,24 @@ def test_should_update_provider_statistics_email_multi(notify_db, notify_db_session, sample_email_template, ses_provider): - create_sample_notification( + n1 = create_sample_notification( notify_db, notify_db_session, - template=sample_email_template) - create_sample_notification( + template=sample_email_template, + provider_name=ses_provider.identifier) + update_notification_after_sent_to_provider(n1.id, 'email', ses_provider.identifier, reference="reference") + n2 = create_sample_notification( notify_db, notify_db_session, - template=sample_email_template) - create_sample_notification( + template=sample_email_template, + provider_name=ses_provider.identifier) + update_notification_after_sent_to_provider(n2.id, 'email', ses_provider.identifier, reference="reference") + n3 = create_sample_notification( notify_db, notify_db_session, - template=sample_email_template) + template=sample_email_template, + provider_name=ses_provider.identifier) + update_notification_after_sent_to_provider(n3.id, 'email', ses_provider.identifier, reference="reference") provider_stats = get_provider_statistics( sample_email_template.service, providers=[ses_provider.identifier]).one() From 49f386e7c5c0d866bc2b35a1e6c47dacf14a895c Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 2 Jun 2016 09:17:06 +0100 Subject: [PATCH 2/2] Refactored to put notifications update last --- app/dao/notifications_dao.py | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index eecee12a6..c5db52329 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -281,25 +281,33 @@ def dao_update_notification(notification): def update_notification_after_sent_to_provider(id_, notification_type, provider_name, reference=None): provider = ProviderDetails.query.filter_by(identifier=provider_name).one() notification = Notification.query.filter(Notification.id == id_).one() - if reference: - notification.reference = reference - db.session.add(notification) + + def unit_count(): + if notification_type == TEMPLATE_TYPE_EMAIL: + return 1 + else: + return get_sms_fragment_count(notification.content_char_count) + update_count = db.session.query(ProviderStatistics).filter_by( day=date.today(), service_id=notification.service_id, provider_id=provider.id - ).update({'unit_count': ProviderStatistics.unit_count + ( - 1 if notification_type == TEMPLATE_TYPE_EMAIL else get_sms_fragment_count(notification.content_char_count))}) + ).update({'unit_count': ProviderStatistics.unit_count + unit_count()}) if update_count == 0: provider_stats = ProviderStatistics( day=notification.created_at.date(), service_id=notification.service_id, provider_id=provider.id, - unit_count=1 if notification_type == TEMPLATE_TYPE_EMAIL else get_sms_fragment_count( - notification.content_char_count)) + unit_count=unit_count() + ) + db.session.add(provider_stats) + if reference: + notification.reference = reference + db.session.add(notification) + def get_notification_for_job(service_id, job_id, notification_id): return Notification.query.filter_by(service_id=service_id, job_id=job_id, id=notification_id).one()