From 0efa223fb2e644f807a48695395b4c1e4ae09102 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 30 Apr 2018 11:50:56 +0100 Subject: [PATCH] rename days_ago to midnight_n_days_ago also add some more timezone boundary tests and minor code cleanup --- app/dao/jobs_dao.py | 4 +-- app/dao/notifications_dao.py | 4 +-- app/template_statistics/rest.py | 5 +-- app/utils.py | 4 +-- .../test_notification_dao_template_usage.py | 33 +++++++++++++++---- tests/app/test_utils.py | 29 ++++++++++------ 6 files changed, 54 insertions(+), 25 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 8ae51a315..9e7988219 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -10,7 +10,7 @@ from sqlalchemy import ( ) from app import db -from app.utils import days_ago +from app.utils import midnight_n_days_ago from app.models import ( Job, JOB_STATUS_PENDING, @@ -51,7 +51,7 @@ def dao_get_jobs_by_service_id(service_id, limit_days=None, page=1, page_size=50 Job.original_file_name != current_app.config['ONE_OFF_MESSAGE_FILENAME'], ] if limit_days is not None: - query_filter.append(Job.created_at >= days_ago(limit_days)) + query_filter.append(Job.created_at >= midnight_n_days_ago(limit_days)) if statuses is not None and statuses != ['']: query_filter.append( Job.job_status.in_(statuses) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d57ada265..feaad7fea 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -21,7 +21,7 @@ from sqlalchemy.sql import functions from notifications_utils.international_billing_rates import INTERNATIONAL_BILLING_RATES from app import db, create_uuid -from app.utils import days_ago +from app.utils import midnight_n_days_ago from app.errors import InvalidRequest from app.models import ( Notification, @@ -248,7 +248,7 @@ def get_notifications_for_service( filters = [Notification.service_id == service_id] if limit_days is not None: - filters.append(Notification.created_at >= days_ago(limit_days)) + filters.append(Notification.created_at >= midnight_n_days_ago(limit_days)) if older_than is not None: older_than_created_at = db.session.query( diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 019600057..b7d005d55 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -39,7 +39,7 @@ def get_template_statistics_for_service_by_day(service_id): if limit_days < 1 or limit_days > 7: raise InvalidRequest({'limit_days': ['limit_days must be between 1 and 7']}, status_code=400) - return jsonify(data=get_template_statistics_for_last_n_days(service_id, limit_days)) + return jsonify(data=_get_template_statistics_for_last_n_days(service_id, limit_days)) @template_statistics.route('/') @@ -58,7 +58,7 @@ def get_template_statistics_for_template_id(service_id, template_id): return jsonify(data=data) -def get_template_statistics_for_last_n_days(service_id, limit_days): +def _get_template_statistics_for_last_n_days(service_id, limit_days): template_stats_by_id = Counter() for day in last_n_days(limit_days): @@ -77,6 +77,7 @@ def get_template_statistics_for_last_n_days(service_id, limit_days): # if there is data in db, but not in redis - lets put it in redis so we don't have to do # this calc again next time. If there isn't any data, we can't put it in redis. # Zero length hashes aren't a thing in redis. (There'll only be no data if the service has no templates) + # Nothing is stored if redis is down. if stats: redis_store.set_hash_and_expire( key, diff --git a/app/utils.py b/app/utils.py index 5da1e6b27..71468e135 100644 --- a/app/utils.py +++ b/app/utils.py @@ -81,7 +81,7 @@ def cache_key_for_service_template_counter(service_id, limit_days=7): def cache_key_for_service_template_usage_per_day(service_id, datetime): """ - You should probably pass a BST datetime into this function + You should pass a BST datetime into this function """ return "service-{}-template-usage-{}".format(service_id, datetime.date().isoformat()) @@ -95,7 +95,7 @@ def get_public_notify_type_text(notify_type, plural=False): return '{}{}'.format(notify_type_text, 's' if plural else '') -def days_ago(number_of_days): +def midnight_n_days_ago(number_of_days): """ Returns midnight a number of days ago. Takes care of daylight savings etc. """ diff --git a/tests/app/dao/notification_dao/test_notification_dao_template_usage.py b/tests/app/dao/notification_dao/test_notification_dao_template_usage.py index d330997a9..b8ae644c5 100644 --- a/tests/app/dao/notification_dao/test_notification_dao_template_usage.py +++ b/tests/app/dao/notification_dao/test_notification_dao_template_usage.py @@ -72,6 +72,7 @@ def test_last_template_usage_should_be_able_to_get_no_template_usage_history_if_ assert not results +@freeze_time('2018-01-01') def test_should_by_able_to_get_template_count(sample_template, sample_email_template): create_notification(sample_template) create_notification(sample_template) @@ -80,15 +81,16 @@ def test_should_by_able_to_get_template_count(sample_template, sample_email_temp create_notification(sample_email_template) results = dao_get_template_usage(sample_template.service_id, date.today()) - assert results[0].name == 'Email Template Name' - assert results[0].template_type == 'email' + assert results[0].name == sample_email_template.name + assert results[0].template_type == sample_email_template.template_type assert results[0].count == 2 - assert results[1].name == 'Template Name' - assert results[1].template_type == 'sms' + assert results[1].name == sample_template.name + assert results[1].template_type == sample_template.template_type assert results[1].count == 3 +@freeze_time('2018-01-01') def test_template_usage_should_ignore_test_keys( sample_team_api_key, sample_test_api_key, @@ -102,8 +104,8 @@ def test_template_usage_should_ignore_test_keys( create_notification(sample_template) results = dao_get_template_usage(sample_template.service_id, date.today()) - assert results[0].name == 'Template Name' - assert results[0].template_type == 'sms' + assert results[0].name == sample_template.name + assert results[0].template_type == sample_template.template_type assert results[0].count == 3 @@ -148,7 +150,6 @@ def test_template_usage_should_by_able_to_get_zero_count_from_notifications_hist assert len(results) == 0 -@freeze_time('2017-06-10T12:00:00') def test_template_usage_should_by_able_to_get_template_count_for_specific_day(sample_template): # too early create_notification(sample_template, created_at=datetime(2017, 6, 7, 22, 59, 0)) @@ -165,3 +166,21 @@ def test_template_usage_should_by_able_to_get_template_count_for_specific_day(sa assert len(results) == 1 assert results[0].count == 5 + + +def test_template_usage_should_by_able_to_get_template_count_for_specific_timezone_boundary(sample_template): + # too early + create_notification(sample_template, created_at=datetime(2018, 3, 24, 23, 59, 0)) + # just right + create_notification(sample_template, created_at=datetime(2018, 3, 25, 0, 0, 0)) + create_notification(sample_template, created_at=datetime(2018, 3, 25, 0, 0, 0)) + create_notification(sample_template, created_at=datetime(2018, 3, 25, 22, 59, 0)) + create_notification(sample_template, created_at=datetime(2018, 3, 25, 22, 59, 0)) + create_notification(sample_template, created_at=datetime(2018, 3, 25, 22, 59, 0)) + # too late + create_notification(sample_template, created_at=datetime(2018, 3, 25, 23, 0, 0)) + + results = dao_get_template_usage(sample_template.service_id, day=date(2018, 3, 25)) + + assert len(results) == 1 + assert results[0].count == 5 diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 657f0e360..d6488946d 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -8,7 +8,7 @@ from app.utils import ( get_midnight_for_day_before, convert_utc_to_bst, convert_bst_to_utc, - days_ago, + midnight_n_days_ago, last_n_days ) @@ -51,22 +51,26 @@ def test_convert_bst_to_utc(): assert utc == datetime(2017, 5, 12, 12, 15) -@pytest.mark.parametrize('current_time, expected_datetime', [ +@pytest.mark.parametrize('current_time, arg, expected_datetime', [ # winter - ('2018-01-10 23:59', datetime(2018, 1, 9, 0, 0)), - ('2018-01-11 00:00', datetime(2018, 1, 10, 0, 0)), + ('2018-01-10 23:59', 1, datetime(2018, 1, 9, 0, 0)), + ('2018-01-11 00:00', 1, datetime(2018, 1, 10, 0, 0)), # bst switchover at 1am 25th - ('2018-03-25 10:00', datetime(2018, 3, 24, 0, 0)), - ('2018-03-26 10:00', datetime(2018, 3, 25, 0, 0)), - ('2018-03-27 10:00', datetime(2018, 3, 25, 23, 0)), + ('2018-03-25 10:00', 1, datetime(2018, 3, 24, 0, 0)), + ('2018-03-26 10:00', 1, datetime(2018, 3, 25, 0, 0)), + ('2018-03-27 10:00', 1, datetime(2018, 3, 25, 23, 0)), # summer - ('2018-06-05 10:00', datetime(2018, 6, 3, 23, 0)) + ('2018-06-05 10:00', 1, datetime(2018, 6, 3, 23, 0)), + + # zero days ago + ('2018-01-11 00:00', 0, datetime(2018, 1, 11, 0, 0)), + ('2018-06-05 10:00', 0, datetime(2018, 6, 4, 23, 0)), ]) -def test_days_ago(current_time, expected_datetime): +def test_midnight_n_days_ago(current_time, arg, expected_datetime): with freeze_time(current_time): - assert days_ago(1) == expected_datetime + assert midnight_n_days_ago(arg) == expected_datetime def test_last_n_days(): @@ -80,3 +84,8 @@ def test_last_n_days(): datetime(2018, 3, 26, 0, 0), datetime(2018, 3, 27, 0, 0) ] + + +@pytest.mark.parametrize('arg', [0, -1]) +def test_last_n_days_invalid_arg(arg): + assert last_n_days(arg) == []