From 7c16294b7579437091ec0649dceeb46b9a8830ad Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 16 Sep 2016 13:47:09 +0100 Subject: [PATCH] Exclude test key notifications from: - get all notifications by service - template usage - most recently used templates Ensures that the dashboard shows no test key data. Supplements: https://github.com/alphagov/notifications-api/pull/677 which excludes CSV data. This branches from that so is dependant. --- app/dao/notifications_dao.py | 11 ++- app/dao/services_dao.py | 7 +- tests/app/dao/test_notification_dao.py | 99 +++++++++++++++++++++++++- tests/app/dao/test_services_dao.py | 24 +++++++ 4 files changed, 132 insertions(+), 9 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index efd1cc2a8..fa65e8a72 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -21,7 +21,7 @@ from app.models import ( NOTIFICATION_CREATED, NOTIFICATION_SENDING, NOTIFICATION_PENDING, - NOTIFICATION_TEMPORARY_FAILURE, KEY_TYPE_NORMAL) + NOTIFICATION_TEMPORARY_FAILURE, KEY_TYPE_NORMAL, KEY_TYPE_TEST) from app.dao.dao_utils import transactional from app.statsd_decorators import statsd @@ -97,7 +97,7 @@ def dao_get_template_usage(service_id, limit_days=None): Template.template_type ) - query_filter = [table.service_id == service_id] + query_filter = [table.service_id == service_id, table.key_type != KEY_TYPE_TEST] if limit_days is not None: query_filter.append(table.created_at >= days_ago(limit_days)) @@ -110,7 +110,9 @@ def dao_get_template_usage(service_id, limit_days=None): @statsd(namespace="dao") def dao_get_last_template_usage(template_id): - return NotificationHistory.query.filter(NotificationHistory.template_id == template_id) \ + return NotificationHistory.query.filter( + NotificationHistory.template_id == template_id, + NotificationHistory.key_type != KEY_TYPE_TEST) \ .join(Template) \ .order_by(desc(NotificationHistory.created_at)) \ .first() @@ -248,6 +250,9 @@ def get_notifications_for_service(service_id, if key_type is not None: filters.append(Notification.key_type == key_type) + else: + filters.append(Notification.key_type != KEY_TYPE_TEST) + query = Notification.query.filter(*filters) query = _filter_query(query, filter_dict) if personalisation: diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 031c10939..8e9e8ceb6 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -23,8 +23,8 @@ from app.models import ( Permission, User, InvitedUser, - Service -) + Service, + KEY_TYPE_TEST) from app.statsd_decorators import statsd @@ -156,7 +156,8 @@ def _stats_for_service_query(service_id): Notification.status, func.count(Notification.id).label('count') ).filter( - Notification.service_id == service_id + Notification.service_id == service_id, + Notification.key_type != KEY_TYPE_TEST ).group_by( Notification.notification_type, Notification.status, diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index ca47286ce..39c91912c 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -89,6 +89,36 @@ def test_should_be_able_to_get_all_template_usage_history_order_by_notification_ assert results.id == most_recent.id +def test_template_usage_should_ignore_test_keys( + notify_db, + notify_db_session, + sample_team_api_key, + sample_test_api_key +): + sms = sample_template(notify_db, notify_db_session) + + one_minute_ago = datetime.utcnow() - timedelta(minutes=1) + two_minutes_ago = datetime.utcnow() - timedelta(minutes=2) + + team_key = sample_notification( + notify_db, + notify_db_session, + created_at=two_minutes_ago, + template=sms, + api_key_id=sample_team_api_key.id, + key_type=KEY_TYPE_TEAM) + sample_notification( + notify_db, + notify_db_session, + created_at=one_minute_ago, + template=sms, + api_key_id=sample_test_api_key.id, + key_type=KEY_TYPE_TEST) + + results = dao_get_last_template_usage(sms.id) + assert results.id == team_key.id + + def test_should_be_able_to_get_no_template_usage_history_if_no_notifications_using_template( notify_db, notify_db_session): @@ -117,6 +147,30 @@ def test_should_by_able_to_get_template_count_from_notifications_history(notify_ assert results[1].count == 3 +def test_template_history_should_ignore_test_keys( + notify_db, + notify_db_session, + sample_team_api_key, + sample_test_api_key, + sample_api_key +): + sms = sample_template(notify_db, notify_db_session) + + sample_notification( + notify_db, notify_db_session, template=sms, api_key_id=sample_api_key.id, key_type=KEY_TYPE_NORMAL) + sample_notification( + notify_db, notify_db_session, template=sms, api_key_id=sample_team_api_key.id, key_type=KEY_TYPE_TEAM) + sample_notification( + notify_db, notify_db_session, template=sms, api_key_id=sample_test_api_key.id, key_type=KEY_TYPE_TEST) + sample_notification( + notify_db, notify_db_session, template=sms) + + results = dao_get_template_usage(sms.service_id) + assert results[0].name == 'Template Name' + assert results[0].template_type == 'sms' + assert results[0].count == 3 + + def test_should_by_able_to_get_template_count_from_notifications_history_for_service( notify_db, notify_db_session): @@ -917,7 +971,7 @@ def test_should_return_notifications_including_jobs(notify_db, notify_db_session assert all_notifications[0].id == with_job.id -def test_get_notifications_created_by_api_or_csv_are_returned_correctly( +def test_get_notifications_created_by_api_or_csv_are_returned_correctly_excluding_test_key_notifications( notify_db, notify_db_session, sample_service, @@ -948,11 +1002,11 @@ def test_get_notifications_created_by_api_or_csv_are_returned_correctly( # returns all API derived notifications all_notifications = get_notifications_for_service(sample_service.id).items - assert len(all_notifications) == 3 + assert len(all_notifications) == 2 # all notifications including jobs all_notifications = get_notifications_for_service(sample_service.id, limit_days=1, include_jobs=True).items - assert len(all_notifications) == 4 + assert len(all_notifications) == 3 def test_get_notifications_with_a_live_api_key_type( @@ -1061,3 +1115,42 @@ def test_get_notifications_with_a_team_api_key_type( all_notifications = get_notifications_for_service(sample_service.id, limit_days=1, include_jobs=True, key_type=KEY_TYPE_TEAM).items assert len(all_notifications) == 1 + + +def test_should_exclude_test_key_notifications_by_default( + notify_db, + notify_db_session, + sample_service, + sample_job, + sample_api_key, + sample_team_api_key, + sample_test_api_key +): + sample_notification( + notify_db, notify_db_session, created_at=datetime.utcnow(), job=sample_job + ) + + sample_notification( + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type + ) + sample_notification( + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_team_api_key.id, + key_type=sample_team_api_key.key_type + ) + sample_notification( + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_test_api_key.id, + key_type=sample_test_api_key.key_type + ) + + all_notifications = Notification.query.all() + assert len(all_notifications) == 4 + + all_notifications = get_notifications_for_service(sample_service.id, limit_days=1).items + assert len(all_notifications) == 2 + + all_notifications = get_notifications_for_service(sample_service.id, limit_days=1, include_jobs=True).items + assert len(all_notifications) == 3 + + all_notifications = get_notifications_for_service(sample_service.id, limit_days=1, key_type=KEY_TYPE_TEST).items + assert len(all_notifications) == 1 diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 654c3b686..b14df40cb 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -441,6 +441,30 @@ def test_fetch_stats_counts_correctly(notify_db, notify_db_session, sample_templ assert stats[2].count == 1 +def test_fetch_stats_counts_should_ignore_team_key( + notify_db, + notify_db_session, + sample_template, + sample_api_key, + sample_test_api_key, + sample_team_api_key +): + # two created email, one failed email, and one created sms + create_notification(notify_db, notify_db_session, api_key_id=sample_api_key.id, key_type=sample_api_key.key_type) + create_notification( + notify_db, notify_db_session, api_key_id=sample_test_api_key.id, key_type=sample_test_api_key.key_type) + create_notification( + notify_db, notify_db_session, api_key_id=sample_team_api_key.id, key_type=sample_team_api_key.key_type) + create_notification( + notify_db, notify_db_session) + + stats = dao_fetch_stats_for_service(sample_template.service_id) + assert len(stats) == 1 + assert stats[0].notification_type == 'sms' + assert stats[0].status == 'created' + assert stats[0].count == 3 + + def test_fetch_stats_for_today_only_includes_today(notify_db, notify_db_session, sample_template): # two created email, one failed email, and one created sms with freeze_time('2001-01-01T23:59:00'):