From 7fda32b7cf2efb5fb14405ae8a20b8a9ce4b3a87 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 6 Jul 2018 18:09:41 +0100 Subject: [PATCH 1/2] dashboard functions should return data for 8 days, not 7 really, it'll be somewhere btween 7 and 8 depending on what time of day you request it at. But if today is monday, then seven days ago is last tuesday - but we should return data for last monday as well so that users see a full week's worth of data also update/clarify the tests to make sure this is being honored for all the different widgets on the dashboard --- app/dao/services_dao.py | 6 ++-- tests/app/dao/test_services_dao.py | 35 +++++++++------------ tests/app/job/test_rest.py | 36 ++++++++++------------ tests/app/template_statistics/test_rest.py | 30 +++++++++--------- 4 files changed, 49 insertions(+), 58 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 4aff00390..d1afdc3d5 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -37,7 +37,7 @@ from app.models import ( SMS_TYPE, LETTER_TYPE, ) -from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc +from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc, midnight_n_days_ago DEFAULT_SERVICE_PERMISSIONS = [ SMS_TYPE, @@ -247,8 +247,8 @@ def delete_service_and_all_associated_db_objects(service): @statsd(namespace="dao") def dao_fetch_stats_for_service(service_id): - # We want 7 days inclusive - start_date = get_london_midnight_in_utc(date.today() - timedelta(days=6)) + # We always want between seven and eight days + start_date = midnight_n_days_ago(7) return _stats_for_service_query(service_id).filter( Notification.created_at >= start_date ).all() diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index e9a85a2eb..2720db90c 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -620,30 +620,23 @@ def test_fetch_stats_for_today_only_includes_today(notify_db, notify_db_session, assert stats['created'] == 1 -def test_fetch_stats_should_not_gather_notifications_older_than_7_days(notify_db, notify_db_session, sample_template): +@pytest.mark.parametrize('created_at, rows_returned', [ + ('Sunday 8th July 2018 12:00', 0), + ('Sunday 8th July 2018 22:59', 0), + ('Sunday 8th July 2018 23:00', 1), + ('Monday 9th July 2018 09:00', 1), + ('Monday 9th July 2018 15:00', 1), + ('Monday 16th July 2018 12:00', 1), +]) +def test_fetch_stats_should_not_gather_notifications_older_than_7_days(sample_template, created_at, rows_returned): + # It's monday today. Things made last monday should still show + with freeze_time(created_at): + create_notification_db(sample_template,) - # 8 days ago - create_notification(notify_db, None, to_field='1', status='delivered', created_at='2001-04-02T12:00:00') - create_notification(notify_db, None, to_field='1', status='delivered', created_at='2001-04-02T22:59:59') - - # 7 days ago BST midnight - create_notification(notify_db, None, to_field='1', status='failed', created_at='2001-04-02T23:00:00') - # 7 days ago, 2hours ago - create_notification(notify_db, None, to_field='2', status='failed', created_at='2001-04-03T10:00:00') - - # 7 days ago - create_notification(notify_db, None, to_field='2', status='failed', created_at='2001-04-03T12:00:00') - - # right_now - create_notification(notify_db, None, to_field='3', status='created', created_at='2001-04-09T12:00:00') - - with freeze_time('2001-04-09T12:00:00'): + with freeze_time('Monday 16th July 2018 12:00'): stats = dao_fetch_stats_for_service(sample_template.service_id) - stats = {row.status: row.count for row in stats} - assert 'delivered' not in stats - assert stats['failed'] == 3 - assert stats['created'] == 1 + assert len(stats) == rows_returned def test_dao_fetch_todays_total_message_count_returns_count_for_today(notify_db, diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index a4fd20117..a147267f4 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -619,28 +619,24 @@ def test_get_jobs(client, notify_db, notify_db_session, sample_template): assert len(resp_json['data']) == 5 -def test_get_jobs_with_limit_days(client, notify_db, notify_db_session, sample_template): - create_job( - notify_db, - notify_db_session, - service=sample_template.service, - template=sample_template, - ) - create_job( - notify_db, - notify_db_session, - service=sample_template.service, - template=sample_template, - created_at=datetime.now() - timedelta(days=7)) +def test_get_jobs_with_limit_days(admin_request, notify_db, notify_db_session, sample_template): + for time in [ + 'Sunday 1st July 2018 22:59', + 'Sunday 2nd July 2018 23:00', # beginning of monday morning + 'Monday 3rd July 2018 12:00' + ]: + with freeze_time(time): + create_job( + notify_db, + notify_db_session, + service=sample_template.service, + template=sample_template, + ) - service_id = sample_template.service.id + with freeze_time('Monday 9th July 2018 12:00'): + resp_json = admin_request.get('job.get_jobs_by_service', service_id=sample_template.service_id, limit_days=7) - path = '/service/{}/job'.format(service_id) - auth_header = create_authorization_header() - response = client.get(path, headers=[auth_header], query_string={'limit_days': 5}) - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert len(resp_json['data']) == 1 + assert len(resp_json['data']) == 2 def test_get_jobs_should_return_statistics(client, notify_db, notify_db_session, notify_api, sample_service): diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index abf464773..2aed2374b 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -33,7 +33,7 @@ def set_up_get_all_from_hash(mock_redis, side_effect): @pytest.mark.parametrize('query_string', [ {}, {'limit_days': 0}, - {'limit_days': 8}, + {'limit_days': 9}, {'limit_days': 3.5}, {'limit_days': 'blurk'}, ]) @@ -190,6 +190,7 @@ def test_get_template_statistics_for_service_by_day_gets_stats_for_correct_days( {sample_template.id: 1}, {sample_template.id: 1}, {sample_template.id: 1}, + {sample_template.id: 1}, None, None, ]) @@ -203,25 +204,26 @@ def test_get_template_statistics_for_service_by_day_gets_stats_for_correct_days( json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', service_id=sample_template.service_id, - limit_days=7 + limit_days=8 ) assert len(json_resp['data']) == 1 - assert json_resp['data'][0]['count'] == 10 + assert json_resp['data'][0]['count'] == 11 assert json_resp['data'][0]['template_id'] == str(sample_template.id) - assert mock_redis.get_all_from_hash.call_count == 7 + assert mock_redis.get_all_from_hash.call_count == 8 - assert '2018-03-22' in mock_redis.get_all_from_hash.mock_calls[0][1][0] - assert '2018-03-23' in mock_redis.get_all_from_hash.mock_calls[1][1][0] - assert '2018-03-24' in mock_redis.get_all_from_hash.mock_calls[2][1][0] - assert '2018-03-25' in mock_redis.get_all_from_hash.mock_calls[3][1][0] - assert '2018-03-26' in mock_redis.get_all_from_hash.mock_calls[4][1][0] - assert '2018-03-27' in mock_redis.get_all_from_hash.mock_calls[5][1][0] - assert '2018-03-28' in mock_redis.get_all_from_hash.mock_calls[6][1][0] + assert '2018-03-21' in mock_redis.get_all_from_hash.mock_calls[0][1][0] # last wednesday + assert '2018-03-22' in mock_redis.get_all_from_hash.mock_calls[1][1][0] + assert '2018-03-23' in mock_redis.get_all_from_hash.mock_calls[2][1][0] + assert '2018-03-24' in mock_redis.get_all_from_hash.mock_calls[3][1][0] + assert '2018-03-25' in mock_redis.get_all_from_hash.mock_calls[4][1][0] + assert '2018-03-26' in mock_redis.get_all_from_hash.mock_calls[5][1][0] + assert '2018-03-27' in mock_redis.get_all_from_hash.mock_calls[6][1][0] + assert '2018-03-28' in mock_redis.get_all_from_hash.mock_calls[7][1][0] # current day (wednesday) mock_dao.mock_calls == [ - call(ANY, day=datetime(2018, 3, 23)), + call(ANY, day=datetime(2018, 3, 22)), call(ANY, day=datetime(2018, 3, 27)), call(ANY, day=datetime(2018, 3, 28)) ] @@ -237,11 +239,11 @@ def test_get_template_statistics_for_service_by_day_returns_empty_list_if_no_tem json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', service_id=sample_service.id, - limit_days=7 + limit_days=8 ) assert len(json_resp['data']) == 0 - assert mock_redis.get_all_from_hash.call_count == 7 + assert mock_redis.get_all_from_hash.call_count == 8 # make sure we don't try and set any empty hashes in redis assert mock_redis.set_hash_and_expire.call_count == 0 From d2d267e5d8911f12c25672b8b54db003fee477bb Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 10 Jul 2018 14:39:51 +0100 Subject: [PATCH 2/2] rename `limit_days` to `whole_days` on template statistics endpoint and return data for one more day. we're not really limiting to 7 days - we're returning 7 entire days, plus whatever time has elapsed since midnight today. I felt it would be best to rename the variable to `whole_days` to imply that it's not "limit this data set to seven days", it's "give me at least seven days". the endpoint is backwards compatible so we can rename the variable on the front-end later --- app/template_statistics/rest.py | 19 +++++----- tests/app/template_statistics/test_rest.py | 42 +++++++++++++++------- 2 files changed, 40 insertions(+), 21 deletions(-) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index d5ea53c23..1c0f3b27d 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -29,17 +29,18 @@ register_errors(template_statistics) @template_statistics.route('') def get_template_statistics_for_service_by_day(service_id): + whole_days = request.args.get('whole_days', request.args.get('limit_days', '')) try: - limit_days = int(request.args.get('limit_days', '')) + whole_days = int(whole_days) except ValueError: - error = '{} is not an integer'.format(request.args.get('limit_days')) - message = {'limit_days': [error]} + error = '{} is not an integer'.format(whole_days) + message = {'whole_days': [error]} raise InvalidRequest(message, status_code=400) - if limit_days < 1 or limit_days > 7: - raise InvalidRequest({'limit_days': ['limit_days must be between 1 and 7']}, status_code=400) + if whole_days < 0 or whole_days > 7: + raise InvalidRequest({'whole_days': ['whole_days must be between 0 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, whole_days)) @template_statistics.route('/') @@ -54,10 +55,12 @@ 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, whole_days): template_stats_by_id = Counter() - for day in last_n_days(limit_days): + # 0 whole_days = last 1 days (ie since midnight today) = today. + # 7 whole days = last 8 days (ie since midnight this day last week) = a week and a bit + for day in last_n_days(whole_days + 1): # "{SERVICE_ID}-template-usage-{YYYY-MM-DD}" key = cache_key_for_service_template_usage_per_day(service_id, day) stats = redis_store.get_all_from_hash(key) diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index 2aed2374b..b9559e134 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -32,10 +32,10 @@ def set_up_get_all_from_hash(mock_redis, side_effect): @pytest.mark.parametrize('query_string', [ {}, - {'limit_days': 0}, - {'limit_days': 9}, - {'limit_days': 3.5}, - {'limit_days': 'blurk'}, + {'whole_days': -1}, + {'whole_days': 8}, + {'whole_days': 3.5}, + {'whole_days': 'blurk'}, ]) def test_get_template_statistics_for_service_by_day_with_bad_arg_returns_400(admin_request, query_string): json_resp = admin_request.get( @@ -45,14 +45,14 @@ def test_get_template_statistics_for_service_by_day_with_bad_arg_returns_400(adm _expected_status=400 ) assert json_resp['result'] == 'error' - assert 'limit_days' in json_resp['message'] + assert 'whole_days' in json_resp['message'] def test_get_template_statistics_for_service_by_day_returns_template_info(admin_request, mocker, sample_notification): json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', service_id=sample_notification.service_id, - limit_days=1 + whole_days=1 ) assert len(json_resp['data']) == 1 @@ -64,6 +64,22 @@ def test_get_template_statistics_for_service_by_day_returns_template_info(admin_ assert json_resp['data'][0]['is_precompiled_letter'] is False +@pytest.mark.parametrize('var_name', ['limit_days', 'whole_days']) +def test_get_template_statistics_for_service_by_day_accepts_old_query_string( + admin_request, + mocker, + sample_notification, + var_name +): + json_resp = admin_request.get( + 'template_statistics.get_template_statistics_for_service_by_day', + service_id=sample_notification.service_id, + **{var_name: 1} + ) + + assert len(json_resp['data']) == 1 + + @freeze_time('2018-01-01 12:00:00') def test_get_template_statistics_for_service_by_day_gets_out_of_redis_if_available( admin_request, @@ -78,7 +94,7 @@ def test_get_template_statistics_for_service_by_day_gets_out_of_redis_if_availab json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', service_id=sample_template.service_id, - limit_days=1 + whole_days=0 ) assert len(json_resp['data']) == 1 @@ -112,7 +128,7 @@ def test_get_template_statistics_for_service_by_day_goes_to_db_if_not_in_redis( json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', service_id=sample_template.service_id, - limit_days=2 + whole_days=1 ) assert len(json_resp['data']) == 1 @@ -161,7 +177,7 @@ def test_get_template_statistics_for_service_by_day_combines_templates_correctly json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', service_id=sample_service.id, - limit_days=3 + whole_days=2 ) assert len(json_resp['data']) == 2 @@ -185,14 +201,14 @@ def test_get_template_statistics_for_service_by_day_gets_stats_for_correct_days( # first time it is called redis returns data, second time returns none set_up_get_all_from_hash(mock_redis, [ - {sample_template.id: 1}, + {sample_template.id: 1}, # last weds None, {sample_template.id: 1}, {sample_template.id: 1}, {sample_template.id: 1}, {sample_template.id: 1}, None, - None, + None, # current day ]) mock_dao = mocker.patch( 'app.template_statistics.rest.dao_get_template_usage', @@ -204,7 +220,7 @@ def test_get_template_statistics_for_service_by_day_gets_stats_for_correct_days( json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', service_id=sample_template.service_id, - limit_days=8 + whole_days=7 ) assert len(json_resp['data']) == 1 @@ -239,7 +255,7 @@ def test_get_template_statistics_for_service_by_day_returns_empty_list_if_no_tem json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', service_id=sample_service.id, - limit_days=8 + whole_days=7 ) assert len(json_resp['data']) == 0