diff --git a/app/commands.py b/app/commands.py index a0a00ca32..c76f563f3 100644 --- a/app/commands.py +++ b/app/commands.py @@ -8,8 +8,10 @@ import flask from click_datetime import Datetime as click_dt from flask import current_app from sqlalchemy.orm.exc import NoResultFound +from sqlalchemy import func +from notifications_utils.statsd_decorators import statsd -from app import db, DATETIME_FORMAT, encryption +from app import db, DATETIME_FORMAT, encryption, redis_store from app.celery.scheduled_tasks import send_total_sent_notifications_to_performance_platform from app.celery.service_callback_tasks import send_delivery_status_to_service from app.celery.letters_pdf_tasks import create_letters_pdf @@ -28,8 +30,11 @@ from app.dao.services_dao import ( from app.dao.users_dao import (delete_model_user, delete_user_verify_codes) from app.models import PROVIDERS, User, SMS_TYPE, EMAIL_TYPE, Notification from app.performance_platform.processing_time import (send_processing_time_for_start_and_end) -from app.utils import get_midnight_for_day_before, get_london_midnight_in_utc -from notifications_utils.statsd_decorators import statsd +from app.utils import ( + cache_key_for_service_template_usage_per_day, + get_london_midnight_in_utc, + get_midnight_for_day_before, +) @click.group(name='command', help='Additional commands') @@ -489,3 +494,37 @@ def migrate_data_to_ft_billing(start_date, end_date): total_updated += result.rowcount print('Total inserted/updated records = {}'.format(total_updated)) + + +@notify_command() +@click.option('-s', '--service_id', required=True, type=click.UUID) +@click.option('-d', '--day', required=True, type=click_dt(format='%Y-%m-%d')) +def populate_redis_template_usage(service_id, day): + """ + Recalculate and replace the stats in redis for a day. + To be used if redis data is lost for some reason. + """ + assert current_app.config['REDIS_ENABLED'] + usage = { + str(row.template_id): row.count + for row in db.session.query( + Notification.template_id, + func.count().label('count') + ).filter( + Notification.service_id == service_id, + ).group_by( + Notification.template_id + ) + } + current_app.logger.info('Populating usage dict for service {} day {}: {}'.format( + service_id, + day, + usage.items()) + ) + if usage: + key = cache_key_for_service_template_usage_per_day(service_id, day) + redis_store.set_hash_and_expire( + key, + usage, + current_app.config['EXPIRE_CACHE_EIGHT_DAYS'] + ) diff --git a/app/config.py b/app/config.py index 315845a5d..b9c6b9d9b 100644 --- a/app/config.py +++ b/app/config.py @@ -95,7 +95,8 @@ class Config(object): # URL of redis instance REDIS_URL = os.getenv('REDIS_URL') REDIS_ENABLED = os.getenv('REDIS_ENABLED') == '1' - EXPIRE_CACHE_IN_SECONDS = 600 + EXPIRE_CACHE_TEN_MINUTES = 600 + EXPIRE_CACHE_EIGHT_DAYS = 8 * 24 * 60 * 60 # Performance platform PERFORMANCE_PLATFORM_ENABLED = False diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 41378f8da..100790f83 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -29,7 +29,13 @@ from app.dao.notifications_dao import ( ) from app.v2.errors import BadRequestError -from app.utils import get_template_instance, cache_key_for_service_template_counter, convert_bst_to_utc +from app.utils import ( + cache_key_for_service_template_counter, + cache_key_for_service_template_usage_per_day, + convert_bst_to_utc, + convert_utc_to_bst, + get_template_instance, +) def create_content_for_notification(template, personalisation): @@ -108,12 +114,24 @@ def persist_notification( redis_store.incr(redis.daily_limit_cache_key(service.id)) if redis_store.get_all_from_hash(cache_key_for_service_template_counter(service.id)): redis_store.increment_hash_value(cache_key_for_service_template_counter(service.id), template_id) + + increment_template_usage_cache(service.id, template_id, notification_created_at) + current_app.logger.info( "{} {} created at {}".format(notification_type, notification_id, notification_created_at) ) return notification +def increment_template_usage_cache(service_id, template_id, created_at): + key = cache_key_for_service_template_usage_per_day(service_id, convert_utc_to_bst(created_at)) + redis_store.increment_hash_value(key, template_id) + # set key to expire in eight days - we don't know if we've just created the key or not, so must assume that we + # have and reset the expiry. Eight days is longer than any notification is in the notifications table, so we'll + # always capture the full week's numbers + redis_store.expire(key, current_app.config['EXPIRE_CACHE_EIGHT_DAYS']) + + def send_notification_to_queue(notification, research_mode, queue=None): if research_mode or notification.key_type == KEY_TYPE_TEST: queue = QueueNames.RESEARCH_MODE diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 950b5ecd6..71667860e 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -79,7 +79,22 @@ def get_template_statistics_for_7_days(limit_days, service_id): if cache_values: redis_store.set_hash_and_expire(cache_key, cache_values, - current_app.config.get('EXPIRE_CACHE_IN_SECONDS', 600)) + current_app.config['EXPIRE_CACHE_TEN_MINUTES']) else: stats = dao_get_templates_for_cache(template_stats_by_id.items()) return stats + + # TODO: can only switch to this code when redis has been populated (either through time passing or a manual step) + # from collections import Counter + # from notifications_utils.redis_client import RedisException + # template_stats_by_id = Counter() + # for day in last_7_days: + # # "-template-usage-{YYYY-MM-DD}" + # key = cache_key_for_service_templates_used_per_day(service_id, limit_days) + # try: + # template_stats_by_id += Counter(redis_store.get_all_from_hash(key, raise_exception=True)) + # except RedisException: + # # TODO: ???? + # + # # TODO: streamline db query and avoid weird unions if possible. + # return dao_get_templates_for_cache(template_stats_by_id.items()) diff --git a/app/utils.py b/app/utils.py index b1367cc7d..1646639c6 100644 --- a/app/utils.py +++ b/app/utils.py @@ -79,6 +79,10 @@ def cache_key_for_service_template_counter(service_id, limit_days=7): return "{}-template-counter-limit-{}-days".format(service_id, limit_days) +def cache_key_for_service_template_usage_per_day(service_id, datetime): + return "service-{}-template-usage-{}".format(service_id, datetime.date().isoformat()) + + def get_public_notify_type_text(notify_type, plural=False): from app.models import SMS_TYPE notify_type_text = notify_type diff --git a/requirements.txt b/requirements.txt index 5e85a803a..5f7c747aa 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,6 +23,6 @@ notifications-python-client==4.8.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@25.2.3#egg=notifications-utils==25.2.3 +git+https://github.com/alphagov/notifications-utils.git@25.3.0#egg=notifications-utils==25.3.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 342a17987..cc3df6226 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -1,11 +1,13 @@ import datetime import uuid -import pytest +from unittest.mock import call +import pytest from boto3.exceptions import Boto3Error from sqlalchemy.exc import SQLAlchemyError from freezegun import freeze_time from collections import namedtuple +from flask import current_app from app.models import ( Notification, @@ -205,9 +207,11 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) @freeze_time("2016-01-01 11:09:00.061258") -def test_persist_notification_increments_cache_if_key_exists(sample_template, sample_api_key, mocker): +def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(sample_template, sample_api_key, mocker): mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr') mock_incr_hash_value = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value') + mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=None) + mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value=None) persist_notification( template_id=sample_template.id, @@ -221,11 +225,20 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa reference="ref" ) mock_incr.assert_not_called() - mock_incr_hash_value.assert_not_called() + mock_incr_hash_value.assert_called_once_with( + "service-{}-template-usage-2016-01-01".format(sample_template.service_id), + sample_template.id + ) + +@freeze_time("2016-01-01 11:09:00.061258") +def test_persist_notification_increments_cache_if_key_exists(sample_template, sample_api_key, mocker): + mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr') + mock_incr_hash_value = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value') mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1) mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value={sample_template.id, 1}) + persist_notification( template_id=sample_template.id, template_version=sample_template.version, @@ -236,9 +249,12 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa api_key_id=sample_api_key.id, key_type=sample_api_key.key_type, reference="ref2") + mock_incr.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count", ) - mock_incr_hash_value.assert_called_once_with(cache_key_for_service_template_counter(sample_template.service_id), - sample_template.id) + assert mock_incr_hash_value.mock_calls == [ + call("{}-template-counter-limit-7-days".format(sample_template.service_id), sample_template.id), + call("service-{}-template-usage-2016-01-01".format(sample_template.service_id), sample_template.id), + ] @pytest.mark.parametrize('research_mode, requested_queue, expected_queue, notification_type, key_type', @@ -446,3 +462,41 @@ def test_persist_email_notification_stores_normalised_email( assert persisted_notification.to == recipient assert persisted_notification.normalised_to == expected_recipient_normalised + + +@pytest.mark.parametrize('utc_time, day_in_key', [ + ('2016-01-01 23:00:00', '2016-01-01'), + ('2016-06-01 22:59:00', '2016-06-01'), + ('2016-06-01 23:00:00', '2016-06-02'), +]) +def test_persist_notification_increments_and_expires_redis_template_usage( + utc_time, + day_in_key, + sample_template, + sample_api_key, + mocker +): + mock_incr_hash_value = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value') + mock_expire = mocker.patch('app.notifications.process_notifications.redis_store.expire') + mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=None) + mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value=None) + + with freeze_time(utc_time): + persist_notification( + template_id=sample_template.id, + template_version=sample_template.version, + recipient='+447111111122', + service=sample_template.service, + personalisation={}, + notification_type='sms', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + ) + mock_incr_hash_value.assert_called_once_with( + 'service-{}-template-usage-{}'.format(str(sample_template.service_id), day_in_key), + sample_template.id + ) + mock_expire.assert_called_once_with( + 'service-{}-template-usage-{}'.format(str(sample_template.service_id), day_in_key), + current_app.config['EXPIRE_CACHE_EIGHT_DAYS'] + )