From eafe8269ef94446b77397689ff3e75ff08ceb491 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 24 Feb 2017 13:39:58 +0000 Subject: [PATCH] Simplify dao method and update tests and fixtures --- app/dao/notifications_dao.py | 20 +-- tests/app/conftest.py | 48 +++---- tests/app/dao/test_notification_dao.py | 167 ++++++------------------- tests/app/db.py | 19 ++- 4 files changed, 73 insertions(+), 181 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 13fcee367..bc05a5509 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -11,13 +11,11 @@ from sqlalchemy.orm import joinedload from app import db, create_uuid from app.dao import days_ago -from app.dao.provider_details_dao import get_provider_details_by_identifier from app.models import ( Service, Notification, NotificationHistory, NotificationStatistics, - ProviderDetails, Template, NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, @@ -426,8 +424,7 @@ def get_total_sent_notifications_in_date_range(start_date, end_date, notificatio return result or 0 -def get_count_of_slow_delivery_sms_notifications_for_provider( - created_at, +def is_delivery_slow_for_provider( sent_at, provider, threshold, @@ -435,21 +432,12 @@ def get_count_of_slow_delivery_sms_notifications_for_provider( service_id, template_id ): - count = db.session.query( - func.count().label('total'), - Notification.sent_by - ).filter( + count = db.session.query(Notification).filter( Notification.service_id == service_id, Notification.template_id == template_id, - Notification.created_at >= created_at, Notification.sent_at >= sent_at, Notification.status == NOTIFICATION_DELIVERED, Notification.sent_by == provider, (Notification.updated_at - Notification.sent_at) >= delivery_time, - ).group_by( - Notification.sent_by - ).having( - func.count().label('total') >= threshold, - ).first() - - return count + ).count() + return count >= threshold diff --git a/tests/app/conftest.py b/tests/app/conftest.py index c946153ec..23e7bcede 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -124,8 +124,7 @@ def sample_service( user=None, restricted=False, limit=1000, - email_from=None, - service_id=None + email_from=None ): if user is None: user = create_user() @@ -141,7 +140,7 @@ def sample_service( service = Service.query.filter_by(name=service_name).first() if not service: service = Service(**data) - dao_create_service(service, user, service_id) + dao_create_service(service, user) else: if user not in service.users: dao_add_user_to_service(service, user) @@ -160,8 +159,7 @@ def sample_template( user=None, service=None, created_by=None, - process_type='normal', - template_id=None + process_type='normal' ): if user is None: user = create_user() @@ -169,24 +167,21 @@ def sample_template( service = sample_service(notify_db, notify_db_session) if created_by is None: created_by = create_user() - - template = Template.query.filter_by(id=template_id).first() - if not template: - data = { - 'name': template_name, - 'template_type': template_type, - 'content': content, - 'service': service, - 'created_by': created_by, - 'archived': archived, - 'process_type': process_type - } - if template_type in ['email', 'letter']: - data.update({ - 'subject': subject_line - }) - template = Template(**data) - dao_create_template(template, template_id) + data = { + 'name': template_name, + 'template_type': template_type, + 'content': content, + 'service': service, + 'created_by': created_by, + 'archived': archived, + 'process_type': process_type + } + if template_type in ['email', 'letter']: + data.update({ + 'subject': subject_line + }) + template = Template(**data) + dao_create_template(template) return template @@ -447,8 +442,7 @@ def sample_notification( api_key_id=None, key_type=KEY_TYPE_NORMAL, sent_by=None, - client_reference=None, - updated_at=None + client_reference=None ): if created_at is None: created_at = datetime.utcnow() @@ -456,8 +450,6 @@ def sample_notification( service = sample_service(notify_db, notify_db_session) if template is None: template = sample_template(notify_db, notify_db_session, service=service) - if not updated_at and status in NOTIFICATION_STATUS_TYPES_COMPLETED: - updated_at = created_at notification_id = uuid.uuid4() @@ -486,7 +478,7 @@ def sample_notification( 'api_key_id': api_key_id, 'key_type': key_type, 'sent_by': sent_by, - 'updated_at': updated_at, + 'updated_at': created_at if status in NOTIFICATION_STATUS_TYPES_COMPLETED else None, 'client_reference': client_reference } if job_row_number: diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index d9aa249a4..bdcef6dcd 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -41,10 +41,11 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, get_financial_year, get_april_fools, - get_count_of_slow_delivery_sms_notifications_for_provider + is_delivery_slow_for_provider ) from app.dao.services_dao import dao_update_service +from tests.app.db import create_notification from tests.app.conftest import ( sample_notification, sample_template, @@ -1427,47 +1428,7 @@ def test_get_total_sent_notifications_for_email_excludes_sms_counts( @freeze_time("2016-01-10 12:00:00.000000") -def test_get_count_of_slow_delivery_sms_notifications_returns_after_created_time( - notify_db, - notify_db_session, - sample_template -): - now = datetime.utcnow() - a_second_ago = now - timedelta(seconds=1) - five_minutes_from_now = now + timedelta(minutes=5) - - notification_five_minutes_to_deliver = partial( - sample_notification, - notify_db, - notify_db_session, - template=sample_template, - status='delivered', - sent_at=now, - sent_by='mmg', - updated_at=five_minutes_from_now - ) - - noti1 = notification_five_minutes_to_deliver(created_at=a_second_ago) - notification_five_minutes_to_deliver(created_at=a_second_ago) - notification_five_minutes_to_deliver(created_at=now) - - count = get_count_of_slow_delivery_sms_notifications_for_provider( - created_at=now, - sent_at=now, - provider='mmg', - threshold=1, - delivery_time=timedelta(minutes=5), - service_id=noti1.service_id, - template_id=noti1.template_id - ) - - assert count.total == 1 - - -@freeze_time("2016-01-10 12:00:00.000000") -def test_get_count_of_slow_delivery_sms_notifications_returns_after_sent_time( - notify_db, - notify_db_session, +def test_slow_provider_delivery_returns_for_sent_notifications( sample_template ): now = datetime.utcnow() @@ -1475,45 +1436,38 @@ def test_get_count_of_slow_delivery_sms_notifications_returns_after_sent_time( five_minutes_from_now = now + timedelta(minutes=5) notification_five_minutes_to_deliver = partial( - sample_notification, - notify_db, - notify_db_session, + create_notification, template=sample_template, status='delivered', sent_by='mmg', updated_at=five_minutes_from_now ) - noti1 = notification_five_minutes_to_deliver(created_at=now, sent_at=now) - notification_five_minutes_to_deliver(created_at=now, sent_at=one_minute_from_now) - notification_five_minutes_to_deliver(created_at=now, sent_at=one_minute_from_now) + notification_five_minutes_to_deliver(sent_at=now) + notification_five_minutes_to_deliver(sent_at=one_minute_from_now) + notification_five_minutes_to_deliver(sent_at=one_minute_from_now) - count = get_count_of_slow_delivery_sms_notifications_for_provider( - created_at=now, + slow_delivery = is_delivery_slow_for_provider( sent_at=one_minute_from_now, provider='mmg', threshold=2, delivery_time=timedelta(minutes=3), - service_id=noti1.service_id, - template_id=noti1.template_id + service_id=sample_template.service.id, + template_id=sample_template.id ) - assert count.total == 2 + assert slow_delivery @freeze_time("2016-01-10 12:00:00.000000") -def test_get_count_of_slow_delivery_sms_notifications_observes_threshold( - notify_db, - notify_db_session, +def test_slow_provider_delivery_observes_threshold( sample_template ): now = datetime.utcnow() five_minutes_from_now = now + timedelta(minutes=5) notification_five_minutes_to_deliver = partial( - sample_notification, - notify_db, - notify_db_session, + create_notification, template=sample_template, status='delivered', sent_at=now, @@ -1521,35 +1475,30 @@ def test_get_count_of_slow_delivery_sms_notifications_observes_threshold( updated_at=five_minutes_from_now ) - noti1 = notification_five_minutes_to_deliver(created_at=now) - notification_five_minutes_to_deliver(created_at=now) + notification_five_minutes_to_deliver() + notification_five_minutes_to_deliver() - count = get_count_of_slow_delivery_sms_notifications_for_provider( - created_at=now, + slow_delivery = is_delivery_slow_for_provider( sent_at=now, provider='mmg', threshold=3, delivery_time=timedelta(minutes=5), - service_id=noti1.service_id, - template_id=noti1.template_id + service_id=sample_template.service.id, + template_id=sample_template.id ) - assert not count + assert not slow_delivery @freeze_time("2016-01-10 12:00:00.000000") -def test_get_count_of_slow_delivery_sms_notifications_returns_status_delivered_only( - notify_db, - notify_db_session, +def test_slow_provider_delivery_returns_for_delivered_notifications_only( sample_template ): now = datetime.utcnow() five_minutes_from_now = now + timedelta(minutes=5) notification_five_minutes_to_deliver = partial( - sample_notification, - notify_db, - notify_db_session, + create_notification, template=sample_template, sent_at=now, sent_by='firetext', @@ -1557,37 +1506,32 @@ def test_get_count_of_slow_delivery_sms_notifications_returns_status_delivered_o updated_at=five_minutes_from_now ) - noti1 = notification_five_minutes_to_deliver(status='created') + notification_five_minutes_to_deliver(status='created') notification_five_minutes_to_deliver(status='sending') notification_five_minutes_to_deliver(status='delivered') notification_five_minutes_to_deliver(status='delivered') - count = get_count_of_slow_delivery_sms_notifications_for_provider( - created_at=now, + slow_delivery = is_delivery_slow_for_provider( sent_at=now, provider='firetext', threshold=1, delivery_time=timedelta(minutes=5), - service_id=noti1.service_id, - template_id=noti1.template_id + service_id=sample_template.service.id, + template_id=sample_template.id ) - assert count.total == 2 + assert slow_delivery @freeze_time("2016-01-10 12:00:00.000000") -def test_get_count_of_slow_delivery_sms_notifications_returns_slow_delivery_time_only( - notify_db, - notify_db_session, +def test_slow_provider_delivery_does_not_return_for_standard_delivery_time( sample_template ): now = datetime.utcnow() five_minutes_from_now = now + timedelta(minutes=5) notification = partial( - sample_notification, - notify_db, - notify_db_session, + create_notification, template=sample_template, created_at=now, sent_at=now, @@ -1595,58 +1539,17 @@ def test_get_count_of_slow_delivery_sms_notifications_returns_slow_delivery_time status='delivered' ) - noti1 = notification(updated_at=five_minutes_from_now - timedelta(seconds=1)) - noti1 = notification(updated_at=five_minutes_from_now - timedelta(seconds=1)) - notification(updated_at=five_minutes_from_now) - notification(updated_at=five_minutes_from_now) + notification(updated_at=five_minutes_from_now - timedelta(seconds=1)) + notification(updated_at=five_minutes_from_now - timedelta(seconds=1)) notification(updated_at=five_minutes_from_now) - count = get_count_of_slow_delivery_sms_notifications_for_provider( - created_at=now, + slow_delivery = is_delivery_slow_for_provider( sent_at=now, provider='mmg', - threshold=1, + threshold=2, delivery_time=timedelta(minutes=5), - service_id=noti1.service_id, - template_id=noti1.template_id + service_id=sample_template.service.id, + template_id=sample_template.id ) - assert count.total == 3 - - -@freeze_time("2016-01-10 12:00:00.000000") -def test_get_count_of_slow_delivery_sms_notifications_returns_provider_only( - notify_db, - notify_db_session, - sample_template -): - now = datetime.utcnow() - five_minutes_from_now = now + timedelta(minutes=5) - - notification = partial( - sample_notification, - notify_db, - notify_db_session, - template=sample_template, - created_at=now, - sent_at=now, - updated_at=five_minutes_from_now, - status='delivered' - ) - - noti1 = notification(sent_by='mmg') - notification(sent_by='firetext') - notification(sent_by='loadtesting') - notification(sent_by='loadtesting') - - count = get_count_of_slow_delivery_sms_notifications_for_provider( - created_at=now, - sent_at=now, - provider='mmg', - threshold=1, - delivery_time=timedelta(minutes=5), - service_id=noti1.service_id, - template_id=noti1.template_id - ) - - assert count.sent_by == 'mmg' + assert not slow_delivery diff --git a/tests/app/db.py b/tests/app/db.py index b30eaa7bb..4b0e9185c 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -23,7 +23,7 @@ def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-off return user -def create_service(user=None, service_name="Sample service"): +def create_service(user=None, service_name="Sample service", service_id=None): service = Service( name=service_name, message_limit=1000, @@ -31,14 +31,15 @@ def create_service(user=None, service_name="Sample service"): email_from=service_name.lower().replace(' ', '.'), created_by=user or create_user() ) - dao_create_service(service, service.created_by) + dao_create_service(service, service.created_by, service_id) return service def create_template( service, template_type=SMS_TYPE, - content='Dear Sir/Madam, Hello. Yours Truly, The Government.' + content='Dear Sir/Madam, Hello. Yours Truly, The Government.', + template_id=None ): data = { 'name': '{} Template Name'.format(template_type), @@ -50,7 +51,7 @@ def create_template( if template_type != SMS_TYPE: data['subject'] = 'Template subject' template = Template(**data) - dao_create_template(template) + dao_create_template(template, template_id) return template @@ -63,6 +64,7 @@ def create_notification( reference=None, created_at=None, sent_at=None, + updated_at=None, billable_units=1, personalisation=None, api_key_id=None, @@ -72,6 +74,13 @@ def create_notification( ): if created_at is None: created_at = datetime.utcnow() + + if sent_at is None: + sent_at = datetime.utcnow() + + if updated_at is None: + updated_at = datetime.utcnow() + data = { 'id': uuid.uuid4(), 'to': to_field, @@ -91,7 +100,7 @@ def create_notification( 'api_key_id': api_key_id, 'key_type': key_type, 'sent_by': sent_by, - 'updated_at': None, + 'updated_at': updated_at, 'client_reference': client_reference, 'job_row_number': job_row_number }