diff --git a/app/commands.py b/app/commands.py index 41f58c4dd..b14e5680d 100644 --- a/app/commands.py +++ b/app/commands.py @@ -1,4 +1,3 @@ -import sys import functools import uuid from datetime import datetime, timedelta @@ -9,10 +8,9 @@ import flask from click_datetime import Datetime as click_dt from flask import current_app, json from sqlalchemy.orm.exc import NoResultFound -from sqlalchemy import func from notifications_utils.statsd_decorators import statsd -from app import db, DATETIME_FORMAT, encryption, redis_store +from app import db, DATETIME_FORMAT, encryption 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 @@ -34,11 +32,7 @@ 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, Notification from app.performance_platform.processing_time import send_processing_time_for_start_and_end -from app.utils import ( - cache_key_for_service_template_usage_per_day, - get_london_midnight_in_utc, - get_midnight_for_day_before, -) +from app.utils import get_london_midnight_in_utc, get_midnight_for_day_before @click.group(name='command', help='Additional commands') @@ -430,50 +424,6 @@ def migrate_data_to_ft_billing(start_date, end_date): current_app.logger.info('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. - """ - if not current_app.config['REDIS_ENABLED']: - current_app.logger.error('Cannot populate redis template usage - redis not enabled') - sys.exit(1) - - # the day variable is set by click to be midnight of that day - start_time = get_london_midnight_in_utc(day) - end_time = get_london_midnight_in_utc(day + timedelta(days=1)) - - 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, - Notification.created_at >= start_time, - Notification.created_at < end_time - ).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'], - raise_exception=True - ) - - @notify_command(name='rebuild-ft-billing-for-day') @click.option('-s', '--service_id', required=False, type=click.UUID) @click.option('-d', '--day', help="The date to recalculate, as YYYY-MM-DD", required=True, diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 8fc2f15f6..c71fb1631 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -9,7 +9,7 @@ from notifications_utils.recipients import ( validate_and_format_phone_number, format_email_address ) -from notifications_utils.timezones import convert_bst_to_utc, convert_utc_to_bst +from notifications_utils.timezones import convert_bst_to_utc from app import redis_store from app.celery import provider_tasks @@ -35,11 +35,7 @@ from app.dao.notifications_dao import ( from app.dao.templates_dao import dao_get_template_by_id from app.v2.errors import BadRequestError -from app.utils import ( - cache_key_for_service_template_counter, - cache_key_for_service_template_usage_per_day, - get_template_instance, -) +from app.utils import cache_key_for_service_template_counter, get_template_instance def create_content_for_notification(template, personalisation): @@ -127,23 +123,12 @@ def persist_notification( 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/utils.py b/app/utils.py index b00a53bda..25bbab968 100644 --- a/app/utils.py +++ b/app/utils.py @@ -72,13 +72,6 @@ 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): - """ - You should pass a BST datetime into this function - """ - 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, UPLOAD_DOCUMENT, PRECOMPILED_LETTER) notify_type_text = notify_type diff --git a/tests/app/commands/test_populate_redis.py b/tests/app/commands/test_populate_redis.py deleted file mode 100644 index 25001642a..000000000 --- a/tests/app/commands/test_populate_redis.py +++ /dev/null @@ -1,73 +0,0 @@ -from datetime import datetime - -from freezegun import freeze_time -import pytest - -from app.commands import populate_redis_template_usage - -from tests.conftest import set_config -from tests.app.db import create_notification, create_template, create_service - - -def test_populate_redis_template_usage_does_nothing_if_redis_disabled(mocker, notify_api, sample_service): - mock_redis = mocker.patch('app.commands.redis_store') - with set_config(notify_api, 'REDIS_ENABLED', False): - with pytest.raises(SystemExit) as exit_signal: - populate_redis_template_usage.callback.__wrapped__(sample_service.id, datetime.utcnow()) - - assert mock_redis.mock_calls == [] - # sys.exit with nonzero exit code - assert exit_signal.value.code != 0 - - -def test_populate_redis_template_usage_does_nothing_if_no_data(mocker, notify_api, sample_service): - mock_redis = mocker.patch('app.commands.redis_store') - with set_config(notify_api, 'REDIS_ENABLED', True): - populate_redis_template_usage.callback.__wrapped__(sample_service.id, datetime.utcnow()) - - assert mock_redis.mock_calls == [] - - -@freeze_time('2017-06-12') -def test_populate_redis_template_usage_only_populates_for_today(mocker, notify_api, sample_template): - mock_redis = mocker.patch('app.commands.redis_store') - # created at in utc - create_notification(sample_template, created_at=datetime(2017, 6, 9, 23, 0, 0)) - create_notification(sample_template, created_at=datetime(2017, 6, 9, 23, 0, 0)) - create_notification(sample_template, created_at=datetime(2017, 6, 10, 0, 0, 0)) - create_notification(sample_template, created_at=datetime(2017, 6, 10, 23, 0, 0)) # actually on 11th BST - - with set_config(notify_api, 'REDIS_ENABLED', True): - populate_redis_template_usage.callback.__wrapped__(sample_template.service_id, datetime(2017, 6, 10)) - - mock_redis.set_hash_and_expire.assert_called_once_with( - 'service-{}-template-usage-2017-06-10'.format(sample_template.service_id), - {str(sample_template.id): 3}, - notify_api.config['EXPIRE_CACHE_EIGHT_DAYS'], - raise_exception=True - ) - - -@freeze_time('2017-06-12') -def test_populate_redis_template_usage_only_populates_for_given_service(mocker, notify_api, notify_db_session): - mock_redis = mocker.patch('app.commands.redis_store') - # created at in utc - s1 = create_service(service_name='a') - s2 = create_service(service_name='b') - t1 = create_template(s1) - t2 = create_template(s2) - - create_notification(t1, created_at=datetime(2017, 6, 10)) - create_notification(t1, created_at=datetime(2017, 6, 10)) - - create_notification(t2, created_at=datetime(2017, 6, 10)) - - with set_config(notify_api, 'REDIS_ENABLED', True): - populate_redis_template_usage.callback.__wrapped__(s1.id, datetime(2017, 6, 10)) - - mock_redis.set_hash_and_expire.assert_called_once_with( - 'service-{}-template-usage-2017-06-10'.format(s1.id), - {str(t1.id): 2}, - notify_api.config['EXPIRE_CACHE_EIGHT_DAYS'], - raise_exception=True - ) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 192644bde..5d02d7bee 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -213,7 +213,6 @@ 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_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) @@ -229,16 +228,11 @@ def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(sa reference="ref" ) mock_incr.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}) @@ -255,10 +249,6 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa reference="ref2") mock_incr.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count", ) - 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(( @@ -516,44 +506,6 @@ def test_persist_letter_notification_finds_correct_postage( assert persisted_notification.postage == expected_postage -@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'] - ) - - def test_persist_notification_with_billable_units_stores_correct_info( sample_template, ):