diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 953b678c2..b80f9d26b 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -81,10 +81,12 @@ def dao_get_template_usage(service_id, *, limit_days=None, day=None): Template.name, Template.template_type, Template.is_precompiled_letter, - notifications_aggregate_query.c.count - ).join( + func.coalesce(notifications_aggregate_query.c.count, 0).label('count') + ).outerjoin( notifications_aggregate_query, notifications_aggregate_query.c.template_id == Template.id + ).filter( + Template.service_id == service_id ).order_by(Template.name) return query.all() diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 7e9297cff..f992fe90d 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -1,7 +1,8 @@ from flask import ( Blueprint, jsonify, - request + request, + current_app ) from app import redis_store @@ -61,15 +62,28 @@ 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): + print('\n') # "{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) - if not stats: + if stats: + stats = { + k.decode('utf-8'): int(v) for k, v in stats.items() + } + else: # key didn't exist (or redis was down) - lets populate from DB. stats = { str(row.id): row.count for row in dao_get_template_usage(service_id, day=day) } - + # 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) + if stats: + redis_store.set_hash_and_expire( + key, + stats, + current_app.config['EXPIRE_CACHE_EIGHT_DAYS'] + ) template_stats_by_id += Counter(stats) # attach count from stats to name/type/etc from database @@ -83,4 +97,7 @@ def get_template_statistics_for_last_n_days(service_id, limit_days): 'is_precompiled_letter': template.is_precompiled_letter } for template in template_details + # we don't want to return templates with no count to the front-end, + # but they're returned from the DB and might be put in redis like that (if there was no data that day) + if template_stats_by_id[str(template.id)] != 0 ] 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 8f44f4996..e66d50955 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 @@ -114,23 +114,28 @@ def test_template_usage_should_filter_by_service(notify_db_session): template_1 = create_template(service_1) template_2 = create_template(service_2) # noqa - template_3 = create_template(service_3) + template_3a = create_template(service_3) + template_3b = create_template(service_3) # noqa # two for service_1, one for service_3 create_notification(template_1) create_notification(template_1) - create_notification(template_3) + create_notification(template_3a) res1 = dao_get_template_usage(service_1.id, limit_days=1) res2 = dao_get_template_usage(service_2.id, limit_days=1) res3 = dao_get_template_usage(service_3.id, limit_days=1) assert len(res1) == 1 - assert len(res2) == 0 - assert len(res3) == 1 assert res1[0].count == 2 + + assert len(res2) == 1 + assert res2[0].count == 0 + + assert len(res3) == 2 assert res3[0].count == 1 + assert res3[1].count == 0 def test_template_usage_should_by_able_to_get_zero_count_from_notifications_history_if_no_rows(sample_service): diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index cfedd8e9e..0f1a652a9 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -3,6 +3,7 @@ from datetime import datetime from unittest.mock import Mock, call, ANY import pytest +from flask import current_app from freezegun import freeze_time from tests.app.db import ( @@ -11,8 +12,24 @@ from tests.app.db import ( ) +def set_up_get_all_from_hash(mock_redis, side_effect): + """ + redis returns binary strings for both keys and values - so given a list of side effects (return values), + make sure + """ + assert type(side_effect) == list + side_effects = [] + for ret_val in side_effect: + if ret_val is None: + side_effects.append(None) + else: + side_effects += [{str(k).encode('utf-8'): str(v).encode('utf-8') for k, v in ret_val.items()}] + + mock_redis.get_all_from_hash.side_effect = side_effects + # get_template_statistics_for_service_by_day + @pytest.mark.parametrize('query_string', [ {}, {'limit_days': 0}, @@ -54,9 +71,9 @@ def test_get_template_statistics_for_service_by_day_gets_out_of_redis_if_availab sample_template ): mock_redis = mocker.patch('app.template_statistics.rest.redis_store') - mock_redis.get_all_from_hash.return_value = { - str(sample_template.id): 3 - } + set_up_get_all_from_hash(mock_redis, [ + {sample_template.id: 3} + ]) json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', @@ -68,7 +85,7 @@ def test_get_template_statistics_for_service_by_day_gets_out_of_redis_if_availab assert json_resp['data'][0]['count'] == 3 assert json_resp['data'][0]['template_id'] == str(sample_template.id) mock_redis.get_all_from_hash.assert_called_once_with( - "service-{}-template-usage-{}".format(sample_template.service_id, '2018-01-01') + 'service-{}-template-usage-{}'.format(sample_template.service_id, '2018-01-01') ) @@ -81,10 +98,10 @@ def test_get_template_statistics_for_service_by_day_goes_to_db_if_not_in_redis( mock_redis = mocker.patch('app.template_statistics.rest.redis_store') # first time it is called redis returns data, second time returns none - mock_redis.get_all_from_hash.side_effect = [ - {str(sample_template.id): 2}, + set_up_get_all_from_hash(mock_redis, [ + {sample_template.id: 2}, None - ] + ]) mock_dao = mocker.patch( 'app.template_statistics.rest.dao_get_template_usage', return_value=[ @@ -102,18 +119,20 @@ def test_get_template_statistics_for_service_by_day_goes_to_db_if_not_in_redis( assert json_resp['data'][0]['count'] == 5 assert json_resp['data'][0]['template_id'] == str(sample_template.id) # first redis call - assert mock_redis.mock_calls == [ - call.get_all_from_hash( - "service-{}-template-usage-{}".format(sample_template.service_id, '2018-01-01') - ), - call.get_all_from_hash( - "service-{}-template-usage-{}".format(sample_template.service_id, '2018-01-02') - ) + assert mock_redis.get_all_from_hash.mock_calls == [ + call('service-{}-template-usage-{}'.format(sample_template.service_id, '2018-01-01')), + call('service-{}-template-usage-{}'.format(sample_template.service_id, '2018-01-02')) ] # dao only called for 2nd, since redis returned values for first call mock_dao.assert_called_once_with( str(sample_template.service_id), day=datetime(2018, 1, 2) ) + mock_redis.set_hash_and_expire.assert_called_once_with( + 'service-{}-template-usage-{}'.format(sample_template.service_id, '2018-01-02'), + # sets the data that the dao returned + {str(sample_template.id): 3}, + current_app.config['EXPIRE_CACHE_EIGHT_DAYS'] + ) def test_get_template_statistics_for_service_by_day_combines_templates_correctly( @@ -127,11 +146,11 @@ def test_get_template_statistics_for_service_by_day_combines_templates_correctly mock_redis = mocker.patch('app.template_statistics.rest.redis_store') # first time it is called redis returns data, second time returns none - mock_redis.get_all_from_hash.side_effect = [ - {str(t1.id): 2}, + set_up_get_all_from_hash(mock_redis, [ + {t1.id: 2}, None, - {str(t1.id): 1, str(t2.id): 4}, - ] + {t1.id: 1, t2.id: 4}, + ]) mock_dao = mocker.patch( 'app.template_statistics.rest.dao_get_template_usage', return_value=[ @@ -165,15 +184,15 @@ def test_get_template_statistics_for_service_by_day_gets_stats_for_correct_days( mock_redis = mocker.patch('app.template_statistics.rest.redis_store') # first time it is called redis returns data, second time returns none - mock_redis.get_all_from_hash.side_effect = [ - {str(sample_template.id): 1}, + set_up_get_all_from_hash(mock_redis, [ + {sample_template.id: 1}, None, - {str(sample_template.id): 1}, - {str(sample_template.id): 1}, - {str(sample_template.id): 1}, + {sample_template.id: 1}, + {sample_template.id: 1}, + {sample_template.id: 1}, None, None, - ] + ]) mock_dao = mocker.patch( 'app.template_statistics.rest.dao_get_template_usage', return_value=[ @@ -213,6 +232,8 @@ def test_get_template_statistics_for_service_by_day_returns_empty_list_if_no_tem mocker, sample_service ): + mock_redis = mocker.patch('app.template_statistics.rest.redis_store') + json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', service_id=sample_service.id, @@ -220,6 +241,9 @@ def test_get_template_statistics_for_service_by_day_returns_empty_list_if_no_tem ) assert len(json_resp['data']) == 0 + assert mock_redis.get_all_from_hash.call_count == 7 + # make sure we don't try and set any empty hashes in redis + assert mock_redis.set_hash_and_expire.call_count == 0 # get_template_statistics_for_template