From f74000f548cb40cde40fc3246ddd474209e5ba35 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 18 Aug 2016 12:06:00 +0100 Subject: [PATCH 1/5] Add query to get template usage from the Notifications History table - groups by template Id and Day. Returns count per day, template name, template id, template type, and day. Ordered by day (desc) and template name (acc) --- app/dao/notifications_dao.py | 23 +++- tests/app/dao/test_notification_dao.py | 144 ++++++++++++++++++++++++- 2 files changed, 164 insertions(+), 3 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d25adac3a..d90427ca6 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -9,7 +9,6 @@ from flask import current_app from werkzeug.datastructures import MultiDict from sqlalchemy import (desc, func, Integer, or_, and_, asc) from sqlalchemy.sql.expression import cast -from notifications_utils.template import get_sms_fragment_count from app import db from app.dao import days_ago @@ -135,6 +134,28 @@ def dao_get_7_day_agg_notification_statistics_for_service(service_id, ) +@statsd(namespace="dao") +def dao_get_template_usage(service_id, limit_days=None): + + query = db.session.query( + func.count(NotificationHistory.template_id).label('count'), + NotificationHistory.template_id, + func.DATE(NotificationHistory.created_at).label('day'), + Template.name, + Template.template_type + ) + + query_filter = [NotificationHistory.service_id == service_id] + if limit_days is not None: + query_filter.append(NotificationHistory.created_at >= days_ago(limit_days)) + + return query.filter(*query_filter) \ + .join(Template)\ + .group_by(NotificationHistory.template_id, func.DATE(NotificationHistory.created_at), Template.name, Template.template_type)\ + .order_by(desc(func.DATE(NotificationHistory.created_at)), asc(Template.name))\ + .all() # noqa + + @statsd(namespace="dao") def dao_get_template_statistics_for_service(service_id, limit_days=None): query_filter = [TemplateStatistics.service_id == service_id] diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 40c5228a2..8affde42b 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -35,15 +35,16 @@ from app.dao.notifications_dao import ( dao_get_template_statistics_for_service, get_notifications_for_service, dao_get_7_day_agg_notification_statistics_for_service, dao_get_potential_notification_statistics_for_day, dao_get_notification_statistics_for_day, - dao_get_template_statistics_for_template, get_notification_by_id) + dao_get_template_statistics_for_template, get_notification_by_id, dao_get_template_usage) from notifications_utils.template import get_sms_fragment_count -from tests.app.conftest import (sample_notification) +from tests.app.conftest import (sample_notification, sample_template, sample_email_template, sample_service) def test_should_have_decorated_notifications_dao_functions(): assert dao_get_notification_statistics_for_service.__wrapped__.__name__ == 'dao_get_notification_statistics_for_service' # noqa + assert dao_get_template_usage.__wrapped__.__name__ == 'dao_get_template_usage' # noqa assert dao_get_notification_statistics_for_service_and_day.__wrapped__.__name__ == 'dao_get_notification_statistics_for_service_and_day' # noqa assert dao_get_notification_statistics_for_day.__wrapped__.__name__ == 'dao_get_notification_statistics_for_day' # noqa assert dao_get_potential_notification_statistics_for_day.__wrapped__.__name__ == 'dao_get_potential_notification_statistics_for_day' # noqa @@ -63,6 +64,145 @@ def test_should_have_decorated_notifications_dao_functions(): assert delete_notifications_created_more_than_a_week_ago.__wrapped__.__name__ == 'delete_notifications_created_more_than_a_week_ago' # noqa +def test_should_by_able_to_get_template_count_from_notifications_history(notify_db, notify_db_session, sample_service): + sms = sample_template(notify_db, notify_db_session) + email = sample_email_template(notify_db, notify_db_session) + sample_notification(notify_db, notify_db_session, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, service=sample_service, template=email) + + results = dao_get_template_usage(sample_service.id) + assert results[0].name == 'Email Template Name' + assert results[0].template_type == 'email' + assert results[0].count == 2 + + assert results[1].name == 'Template Name' + assert results[1].template_type == 'sms' + assert results[1].count == 3 + + +def test_should_by_able_to_get_template_count_from_notifications_history_for_service( + notify_db, + notify_db_session): + service_1 = sample_service(notify_db, notify_db_session, service_name="test1", email_from="test1") + service_2 = sample_service(notify_db, notify_db_session, service_name="test2", email_from="test2") + service_3 = sample_service(notify_db, notify_db_session, service_name="test3", email_from="test3") + + sms = sample_template(notify_db, notify_db_session) + + sample_notification(notify_db, notify_db_session, service=service_1, template=sms) + sample_notification(notify_db, notify_db_session, service=service_1, template=sms) + sample_notification(notify_db, notify_db_session, service=service_2, template=sms) + + assert dao_get_template_usage(service_1.id)[0].count == 2 + assert dao_get_template_usage(service_2.id)[0].count == 1 + assert len(dao_get_template_usage(service_3.id)) == 0 + + +def test_should_by_able_to_get_zero_count_from_notifications_history_if_no_rows(sample_service): + results = dao_get_template_usage(sample_service.id) + assert len(results) == 0 + + +def test_should_by_able_to_get_zero_count_from_notifications_history_if_no_service(): + results = dao_get_template_usage(str(uuid.uuid4())) + assert len(results) == 0 + + +def test_should_by_able_to_get_template_count_from_notifications_history_across_days( + notify_db, + notify_db_session, + sample_service): + sms = sample_template(notify_db, notify_db_session) + email = sample_email_template(notify_db, notify_db_session) + + today = datetime.now() + yesterday = datetime.now() - timedelta(days=1) + one_month_ago = datetime.now() - timedelta(days=30) + + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) + + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=sms) + + sample_notification(notify_db, notify_db_session, created_at=one_month_ago, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=one_month_ago, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=one_month_ago, service=sample_service, template=sms) + + results = dao_get_template_usage(sample_service.id) + + assert len(results) == 5 + + assert [(row.name, row.template_type, row.count, row.day) for row in results] == [ + ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), + ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()), + ('Email Template Name', 'email', 3, datetime(yesterday.year, yesterday.month, yesterday.day).date()), + ('Template Name', 'sms', 1, datetime(yesterday.year, yesterday.month, yesterday.day).date()), + ('Template Name', 'sms', 3, datetime(one_month_ago.year, one_month_ago.month, one_month_ago.day).date()) + ] + + +def test_should_by_able_to_get_template_count_from_notifications_history_with_day_limit( + notify_db, + notify_db_session, + sample_service): + sms = sample_template(notify_db, notify_db_session) + + email = sample_email_template(notify_db, notify_db_session) + + today = datetime.now() + yesterday = datetime.now() - timedelta(days=1) + one_month_ago = datetime.now() - timedelta(days=30) + + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) + + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=yesterday, service=sample_service, template=sms) + + sample_notification(notify_db, notify_db_session, created_at=one_month_ago, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=one_month_ago, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=one_month_ago, service=sample_service, template=sms) + + results_day_one = dao_get_template_usage(sample_service.id, limit_days=0) + assert len(results_day_one) == 2 + + results_day_two = dao_get_template_usage(sample_service.id, limit_days=1) + assert len(results_day_two) == 4 + + results_day_30 = dao_get_template_usage(sample_service.id, limit_days=31) + assert len(results_day_30) == 5 + + assert [(row.name, row.template_type, row.count, row.day) for row in results_day_one] == [ + ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), + ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()) + ] + + assert [(row.name, row.template_type, row.count, row.day) for row in results_day_two] == [ + ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), + ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()), + ('Email Template Name', 'email', 3, datetime(yesterday.year, yesterday.month, yesterday.day).date()), + ('Template Name', 'sms', 1, datetime(yesterday.year, yesterday.month, yesterday.day).date()) + ] + + assert [(row.name, row.template_type, row.count, row.day) for row in results_day_30] == [ + ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), + ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()), + ('Email Template Name', 'email', 3, datetime(yesterday.year, yesterday.month, yesterday.day).date()), + ('Template Name', 'sms', 1, datetime(yesterday.year, yesterday.month, yesterday.day).date()), + ('Template Name', 'sms', 3, datetime(one_month_ago.year, one_month_ago.month, one_month_ago.day).date()) + ] + + def test_should_by_able_to_update_status_by_reference(sample_email_template, ses_provider): data = _notification_json(sample_email_template, status='sending') From 7a5acea71bba7872dfd9be047372a627e2968558 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 18 Aug 2016 14:01:31 +0100 Subject: [PATCH 2/5] New endpoint for template stats to use new query. Breaking change to the returned JSON, so adding on a different url. --- app/template_statistics/rest.py | 26 ++++ tests/app/template_statistics/test_rest.py | 134 ++++++++++++++++++++- 2 files changed, 158 insertions(+), 2 deletions(-) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index d4990ba04..0bed6329e 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -5,6 +5,7 @@ from flask import ( ) from app.dao.notifications_dao import ( + dao_get_template_usage, dao_get_template_statistics_for_service, dao_get_template_statistics_for_template ) @@ -36,6 +37,31 @@ def get_template_statistics_for_service(service_id): return jsonify(data=data) +@template_statistics.route('/replacement') +def get_template_statistics_for_service_by_day(service_id): + if request.args.get('limit_days'): + try: + limit_days = int(request.args['limit_days']) + except ValueError as e: + error = '{} is not an integer'.format(request.args['limit_days']) + message = {'limit_days': [error]} + raise InvalidRequest(message, status_code=400) + else: + limit_days = None + stats = dao_get_template_usage(service_id, limit_days=limit_days) + + def serialize(row): + return { + 'count': row.count, + 'day': str(row.day), + 'template_id': str(row.template_id), + 'template_name': row.name, + 'template_type': row.template_type + } + + return jsonify(data=[serialize(row) for row in stats]) + + @template_statistics.route('/') def get_template_statistics_for_template_id(service_id, template_id): stats = dao_get_template_statistics_for_template(template_id) diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index 4d7b3384a..70191d086 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -1,4 +1,4 @@ -from datetime import datetime +from datetime import datetime, timedelta import json from freezegun import freeze_time @@ -6,7 +6,137 @@ from freezegun import freeze_time from app import db from app.models import TemplateStatistics from tests import create_authorization_header -from tests.app.conftest import sample_template as create_sample_template +from tests.app.conftest import sample_template as create_sample_template, sample_template, sample_notification, \ + sample_email_template + + +def test_get_all_template_statistics_with_bad_arg_returns_400(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/replacement'.format(sample_template.service_id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 'blurk'} + ) + + assert response.status_code == 400 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['result'] == 'error' + assert json_resp['message'] == {'limit_days': ['blurk is not an integer']} + + +@freeze_time('2016-08-18') +def test_get_template_statistics_for_service(notify_db, notify_db_session, notify_api, sample_service): + sms = sample_template(notify_db, notify_db_session, service=sample_service) + email = sample_email_template(notify_db, notify_db_session, service=sample_service) + today = datetime.now() + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/replacement'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 2 + assert json_resp['data'][0]['template_id'] == str(email.id) + assert json_resp['data'][0]['template_name'] == email.name + assert json_resp['data'][0]['template_type'] == email.template_type + assert json_resp['data'][0]['day'] == '2016-08-18' + assert json_resp['data'][1]['count'] == 2 + assert json_resp['data'][1]['template_id'] == str(sms.id) + assert json_resp['data'][1]['template_name'] == sms.name + assert json_resp['data'][1]['template_type'] == sms.template_type + assert json_resp['data'][1]['day'] == '2016-08-18' + + +@freeze_time('2016-08-18') +def test_get_template_statistics_for_service_by_day(notify_db, notify_db_session, notify_api, sample_service): + sms = sample_template(notify_db, notify_db_session, service=sample_service) + email = sample_email_template(notify_db, notify_db_session, service=sample_service) + today = datetime.now() + a_week_ago = datetime.now() - timedelta(days=7) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=a_week_ago, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=a_week_ago, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/replacement'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 1} + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 1 + assert json_resp['data'][0]['template_id'] == str(email.id) + assert json_resp['data'][0]['template_name'] == email.name + assert json_resp['data'][0]['template_type'] == email.template_type + assert json_resp['data'][0]['day'] == '2016-08-18' + assert json_resp['data'][1]['count'] == 1 + assert json_resp['data'][1]['template_id'] == str(sms.id) + assert json_resp['data'][1]['template_name'] == sms.name + assert json_resp['data'][1]['template_type'] == sms.template_type + assert json_resp['data'][1]['day'] == '2016-08-18' + + response_for_a_week = client.get( + '/service/{}/template-statistics/replacement'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 7} + ) + + assert response.status_code == 200 + json_resp = json.loads(response_for_a_week.get_data(as_text=True)) + assert len(json_resp['data']) == 4 + assert json_resp['data'][0]['count'] == 1 + assert json_resp['data'][0]['template_name'] == 'Email Template Name' + assert json_resp['data'][0]['day'] == '2016-08-18' + assert json_resp['data'][1]['count'] == 1 + assert json_resp['data'][1]['template_name'] == 'Template Name' + assert json_resp['data'][1]['day'] == '2016-08-18' + assert json_resp['data'][2]['count'] == 1 + assert json_resp['data'][2]['template_name'] == 'Email Template Name' + assert json_resp['data'][2]['day'] == '2016-08-11' + assert json_resp['data'][3]['count'] == 1 + assert json_resp['data'][3]['template_name'] == 'Template Name' + assert json_resp['data'][3]['day'] == '2016-08-11' + + +@freeze_time('2016-08-18') +def test_returns_empty_list_if_no_templates_used(notify_api, sample_service): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/replacement'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 0 @freeze_time('2016-04-09') From ede7d0cbeaec36016451b599e332d313315b5c63 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 18 Aug 2016 14:06:12 +0100 Subject: [PATCH 3/5] Removed old endpoint. Going to handle the migration in the clients. --- app/template_statistics/rest.py | 16 --- tests/app/template_statistics/test_rest.py | 137 +-------------------- 2 files changed, 6 insertions(+), 147 deletions(-) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 0bed6329e..3b25b4340 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -22,22 +22,6 @@ register_errors(template_statistics) @template_statistics.route('') -def get_template_statistics_for_service(service_id): - if request.args.get('limit_days'): - try: - limit_days = int(request.args['limit_days']) - except ValueError as e: - error = '{} is not an integer'.format(request.args['limit_days']) - message = {'limit_days': [error]} - raise InvalidRequest(message, status_code=400) - else: - limit_days = None - stats = dao_get_template_statistics_for_service(service_id, limit_days=limit_days) - data = template_statistics_schema.dump(stats, many=True).data - return jsonify(data=data) - - -@template_statistics.route('/replacement') def get_template_statistics_for_service_by_day(service_id): if request.args.get('limit_days'): try: diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index 70191d086..65af24166 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -10,14 +10,14 @@ from tests.app.conftest import sample_template as create_sample_template, sample sample_email_template -def test_get_all_template_statistics_with_bad_arg_returns_400(notify_api): +def test_get_all_template_statistics_with_bad_arg_returns_400(notify_api, sample_service): with notify_api.test_request_context(): with notify_api.test_client() as client: auth_header = create_authorization_header() response = client.get( - '/service/{}/template-statistics/replacement'.format(sample_template.service_id), + '/service/{}/template-statistics'.format(sample_service.id), headers=[('Content-Type', 'application/json'), auth_header], query_string={'limit_days': 'blurk'} ) @@ -44,7 +44,7 @@ def test_get_template_statistics_for_service(notify_db, notify_db_session, notif auth_header = create_authorization_header() response = client.get( - '/service/{}/template-statistics/replacement'.format(sample_service.id), + '/service/{}/template-statistics'.format(sample_service.id), headers=[('Content-Type', 'application/json'), auth_header] ) @@ -80,7 +80,7 @@ def test_get_template_statistics_for_service_by_day(notify_db, notify_db_session auth_header = create_authorization_header() response = client.get( - '/service/{}/template-statistics/replacement'.format(sample_service.id), + '/service/{}/template-statistics'.format(sample_service.id), headers=[('Content-Type', 'application/json'), auth_header], query_string={'limit_days': 1} ) @@ -100,7 +100,7 @@ def test_get_template_statistics_for_service_by_day(notify_db, notify_db_session assert json_resp['data'][1]['day'] == '2016-08-18' response_for_a_week = client.get( - '/service/{}/template-statistics/replacement'.format(sample_service.id), + '/service/{}/template-statistics'.format(sample_service.id), headers=[('Content-Type', 'application/json'), auth_header], query_string={'limit_days': 7} ) @@ -130,7 +130,7 @@ def test_returns_empty_list_if_no_templates_used(notify_api, sample_service): auth_header = create_authorization_header() response = client.get( - '/service/{}/template-statistics/replacement'.format(sample_service.id), + '/service/{}/template-statistics'.format(sample_service.id), headers=[('Content-Type', 'application/json'), auth_header] ) @@ -139,131 +139,6 @@ def test_returns_empty_list_if_no_templates_used(notify_api, sample_service): assert len(json_resp['data']) == 0 -@freeze_time('2016-04-09') -def test_get_template_statistics_for_service_for_last_week(notify_api, sample_template): - - # make 9 stats records from 1st to 9th April - for i in range(1, 10): - past_date = '2016-04-0{}'.format(i) - with freeze_time(past_date): - template_stats = TemplateStatistics(template_id=sample_template.id, - service_id=sample_template.service_id) - db.session.add(template_stats) - db.session.commit() - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - - auth_header = create_authorization_header() - - response = client.get( - '/service/{}/template-statistics'.format(sample_template.service_id), - headers=[('Content-Type', 'application/json'), auth_header], - query_string={'limit_days': 7} - ) - - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 8 - assert json_resp['data'][0]['day'] == '2016-04-09' - assert json_resp['data'][6]['day'] == '2016-04-03' - - -@freeze_time('2016-04-30') -def test_get_template_statistics_for_service_for_last_week_with_no_data(notify_api, sample_template): - - # make 9 stats records from 1st to 9th April - for i in range(1, 10): - past_date = '2016-04-0{}'.format(i) - with freeze_time(past_date): - template_stats = TemplateStatistics(template_id=sample_template.id, - service_id=sample_template.service_id) - db.session.add(template_stats) - db.session.commit() - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - - auth_header = create_authorization_header() - - # Date is frozen at 2016-04-30 and no data written since - response = client.get( - '/service/{}/template-statistics'.format(sample_template.service_id), - headers=[('Content-Type', 'application/json'), auth_header], - query_string={'limit_days': 7} - ) - - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 0 - - response = client.get( - '/service/{}/template-statistics'.format(sample_template.service_id), - headers=[('Content-Type', 'application/json'), auth_header], - query_string={'limit_days': 30} - ) - - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 9 - - -def test_get_all_template_statistics_for_service(notify_api, sample_template): - - # make 9 stats records from 1st to 9th April - for i in range(1, 10): - past_date = '2016-04-0{}'.format(i) - with freeze_time(past_date): - template_stats = TemplateStatistics(template_id=sample_template.id, - service_id=sample_template.service_id) - db.session.add(template_stats) - db.session.commit() - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - - auth_header = create_authorization_header() - - response = client.get( - '/service/{}/template-statistics'.format(sample_template.service_id), - headers=[('Content-Type', 'application/json'), auth_header] - ) - - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 9 - assert json_resp['data'][0]['day'] == '2016-04-09' - assert json_resp['data'][8]['day'] == '2016-04-01' - - -def test_get_all_template_statistics_with_bad_limit_arg_returns_400(notify_api, sample_template): - - # make 9 stats records from 1st to 9th April - for i in range(1, 10): - past_date = '2016-04-0{}'.format(i) - with freeze_time(past_date): - template_stats = TemplateStatistics(template_id=sample_template.id, - service_id=sample_template.service_id) - db.session.add(template_stats) - db.session.commit() - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - - auth_header = create_authorization_header() - - response = client.get( - '/service/{}/template-statistics'.format(sample_template.service_id), - headers=[('Content-Type', 'application/json'), auth_header], - query_string={'limit_days': 'blurk'} - ) - - assert response.status_code == 400 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['result'] == 'error' - assert json_resp['message'] == {'limit_days': ['blurk is not an integer']} - - def test_get_template_statistics_for_template_only_returns_for_provided_template( notify_db, notify_db_session, From 9eb559d4b236be5c57927dee4bebed9d47e39a51 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 18 Aug 2016 15:29:56 +0100 Subject: [PATCH 4/5] Renamed the param to avoid shadowing --- app/template_statistics/rest.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 3b25b4340..c044eb70e 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -34,13 +34,13 @@ def get_template_statistics_for_service_by_day(service_id): limit_days = None stats = dao_get_template_usage(service_id, limit_days=limit_days) - def serialize(row): + def serialize(data): return { - 'count': row.count, - 'day': str(row.day), - 'template_id': str(row.template_id), - 'template_name': row.name, - 'template_type': row.template_type + 'count': data.count, + 'day': str(data.day), + 'template_id': str(data.template_id), + 'template_name': data.name, + 'template_type': data.template_type } return jsonify(data=[serialize(row) for row in stats]) From b7476a197512af3ea1510f17f6c43d5def273d6c Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 22 Aug 2016 10:38:44 +0100 Subject: [PATCH 5/5] Removed the group by day aspects of template stats. Not needed. Grouped by template only. --- app/dao/notifications_dao.py | 20 +++++---- app/template_statistics/rest.py | 1 - tests/app/dao/test_notification_dao.py | 38 +++++++--------- tests/app/template_statistics/test_rest.py | 52 +++++++++++++++------- 4 files changed, 62 insertions(+), 49 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d90427ca6..16301e64f 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -137,23 +137,27 @@ def dao_get_7_day_agg_notification_statistics_for_service(service_id, @statsd(namespace="dao") def dao_get_template_usage(service_id, limit_days=None): + table = NotificationHistory + + if limit_days and limit_days <= 7: # can get this data from notifications table + table = Notification + query = db.session.query( - func.count(NotificationHistory.template_id).label('count'), - NotificationHistory.template_id, - func.DATE(NotificationHistory.created_at).label('day'), + func.count(table.template_id).label('count'), + table.template_id, Template.name, Template.template_type ) - query_filter = [NotificationHistory.service_id == service_id] + query_filter = [table.service_id == service_id] if limit_days is not None: - query_filter.append(NotificationHistory.created_at >= days_ago(limit_days)) + query_filter.append(table.created_at >= days_ago(limit_days)) return query.filter(*query_filter) \ .join(Template)\ - .group_by(NotificationHistory.template_id, func.DATE(NotificationHistory.created_at), Template.name, Template.template_type)\ - .order_by(desc(func.DATE(NotificationHistory.created_at)), asc(Template.name))\ - .all() # noqa + .group_by(table.template_id, Template.name, Template.template_type)\ + .order_by(asc(Template.name))\ + .all() @statsd(namespace="dao") diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index c044eb70e..191645b96 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -37,7 +37,6 @@ def get_template_statistics_for_service_by_day(service_id): def serialize(data): return { 'count': data.count, - 'day': str(data.day), 'template_id': str(data.template_id), 'template_name': data.name, 'template_type': data.template_type diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 8affde42b..b75ec5ccf 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -137,14 +137,11 @@ def test_should_by_able_to_get_template_count_from_notifications_history_across_ results = dao_get_template_usage(sample_service.id) - assert len(results) == 5 + assert len(results) == 2 - assert [(row.name, row.template_type, row.count, row.day) for row in results] == [ - ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), - ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()), - ('Email Template Name', 'email', 3, datetime(yesterday.year, yesterday.month, yesterday.day).date()), - ('Template Name', 'sms', 1, datetime(yesterday.year, yesterday.month, yesterday.day).date()), - ('Template Name', 'sms', 3, datetime(one_month_ago.year, one_month_ago.month, one_month_ago.day).date()) + assert [(row.name, row.template_type, row.count) for row in results] == [ + ('Email Template Name', 'email', 5), + ('Template Name', 'sms', 5) ] @@ -177,29 +174,24 @@ def test_should_by_able_to_get_template_count_from_notifications_history_with_da assert len(results_day_one) == 2 results_day_two = dao_get_template_usage(sample_service.id, limit_days=1) - assert len(results_day_two) == 4 + assert len(results_day_two) == 2 results_day_30 = dao_get_template_usage(sample_service.id, limit_days=31) - assert len(results_day_30) == 5 + assert len(results_day_30) == 2 - assert [(row.name, row.template_type, row.count, row.day) for row in results_day_one] == [ - ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), - ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()) + assert [(row.name, row.template_type, row.count) for row in results_day_one] == [ + ('Email Template Name', 'email', 2), + ('Template Name', 'sms', 1) ] - assert [(row.name, row.template_type, row.count, row.day) for row in results_day_two] == [ - ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), - ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()), - ('Email Template Name', 'email', 3, datetime(yesterday.year, yesterday.month, yesterday.day).date()), - ('Template Name', 'sms', 1, datetime(yesterday.year, yesterday.month, yesterday.day).date()) + assert [(row.name, row.template_type, row.count) for row in results_day_two] == [ + ('Email Template Name', 'email', 5), + ('Template Name', 'sms', 2), ] - assert [(row.name, row.template_type, row.count, row.day) for row in results_day_30] == [ - ('Email Template Name', 'email', 2, datetime(today.year, today.month, today.day).date()), - ('Template Name', 'sms', 1, datetime(today.year, today.month, today.day).date()), - ('Email Template Name', 'email', 3, datetime(yesterday.year, yesterday.month, yesterday.day).date()), - ('Template Name', 'sms', 1, datetime(yesterday.year, yesterday.month, yesterday.day).date()), - ('Template Name', 'sms', 3, datetime(one_month_ago.year, one_month_ago.month, one_month_ago.day).date()) + assert [(row.name, row.template_type, row.count) for row in results_day_30] == [ + ('Email Template Name', 'email', 5), + ('Template Name', 'sms', 5), ] diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index 65af24166..1544874f7 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -55,24 +55,25 @@ def test_get_template_statistics_for_service(notify_db, notify_db_session, notif assert json_resp['data'][0]['template_id'] == str(email.id) assert json_resp['data'][0]['template_name'] == email.name assert json_resp['data'][0]['template_type'] == email.template_type - assert json_resp['data'][0]['day'] == '2016-08-18' assert json_resp['data'][1]['count'] == 2 assert json_resp['data'][1]['template_id'] == str(sms.id) assert json_resp['data'][1]['template_name'] == sms.name assert json_resp['data'][1]['template_type'] == sms.template_type - assert json_resp['data'][1]['day'] == '2016-08-18' @freeze_time('2016-08-18') -def test_get_template_statistics_for_service_by_day(notify_db, notify_db_session, notify_api, sample_service): +def test_get_template_statistics_for_service_limited_by_day(notify_db, notify_db_session, notify_api, sample_service): sms = sample_template(notify_db, notify_db_session, service=sample_service) email = sample_email_template(notify_db, notify_db_session, service=sample_service) today = datetime.now() a_week_ago = datetime.now() - timedelta(days=7) + a_month_ago = datetime.now() - timedelta(days=30) sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) sample_notification(notify_db, notify_db_session, created_at=a_week_ago, service=sample_service, template=sms) sample_notification(notify_db, notify_db_session, created_at=a_week_ago, service=sample_service, template=email) - sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) + sample_notification(notify_db, notify_db_session, created_at=a_month_ago, service=sample_service, template=sms) + sample_notification(notify_db, notify_db_session, created_at=a_month_ago, service=sample_service, template=email) with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -92,12 +93,10 @@ def test_get_template_statistics_for_service_by_day(notify_db, notify_db_session assert json_resp['data'][0]['template_id'] == str(email.id) assert json_resp['data'][0]['template_name'] == email.name assert json_resp['data'][0]['template_type'] == email.template_type - assert json_resp['data'][0]['day'] == '2016-08-18' assert json_resp['data'][1]['count'] == 1 assert json_resp['data'][1]['template_id'] == str(sms.id) assert json_resp['data'][1]['template_name'] == sms.name assert json_resp['data'][1]['template_type'] == sms.template_type - assert json_resp['data'][1]['day'] == '2016-08-18' response_for_a_week = client.get( '/service/{}/template-statistics'.format(sample_service.id), @@ -107,19 +106,38 @@ def test_get_template_statistics_for_service_by_day(notify_db, notify_db_session assert response.status_code == 200 json_resp = json.loads(response_for_a_week.get_data(as_text=True)) - assert len(json_resp['data']) == 4 - assert json_resp['data'][0]['count'] == 1 + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 2 assert json_resp['data'][0]['template_name'] == 'Email Template Name' - assert json_resp['data'][0]['day'] == '2016-08-18' - assert json_resp['data'][1]['count'] == 1 + assert json_resp['data'][1]['count'] == 2 + assert json_resp['data'][1]['template_name'] == 'Template Name' + + response_for_a_month = client.get( + '/service/{}/template-statistics'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 30} + ) + + assert response_for_a_month.status_code == 200 + json_resp = json.loads(response_for_a_month.get_data(as_text=True)) + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 3 + assert json_resp['data'][0]['template_name'] == 'Email Template Name' + assert json_resp['data'][1]['count'] == 3 + assert json_resp['data'][1]['template_name'] == 'Template Name' + + response_for_all = client.get( + '/service/{}/template-statistics'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response_for_all.status_code == 200 + json_resp = json.loads(response_for_all.get_data(as_text=True)) + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 3 + assert json_resp['data'][0]['template_name'] == 'Email Template Name' + assert json_resp['data'][1]['count'] == 3 assert json_resp['data'][1]['template_name'] == 'Template Name' - assert json_resp['data'][1]['day'] == '2016-08-18' - assert json_resp['data'][2]['count'] == 1 - assert json_resp['data'][2]['template_name'] == 'Email Template Name' - assert json_resp['data'][2]['day'] == '2016-08-11' - assert json_resp['data'][3]['count'] == 1 - assert json_resp['data'][3]['template_name'] == 'Template Name' - assert json_resp['data'][3]['day'] == '2016-08-11' @freeze_time('2016-08-18')