From 383553b9646cadeef7d12f632ac469a42296ef46 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 13 Feb 2017 17:13:39 +0000 Subject: [PATCH 01/10] Remove notify app folder and installed python packages before we deploy the app --- appspec.yml | 4 ++++ scripts/aws_clear_instance.sh | 14 ++++++++++++++ 2 files changed, 18 insertions(+) create mode 100755 scripts/aws_clear_instance.sh diff --git a/appspec.yml b/appspec.yml index a6455d55c..87a0f944a 100644 --- a/appspec.yml +++ b/appspec.yml @@ -5,6 +5,10 @@ files: - destination: /home/notify-app/notifications-api source: / hooks: + BeforeInstall: + - location: scripts/aws_clear_instance.sh + runas: root + timeout: 1000 AfterInstall: - location: scripts/aws_install_dependencies.sh runas: root diff --git a/scripts/aws_clear_instance.sh b/scripts/aws_clear_instance.sh new file mode 100755 index 000000000..b62397b06 --- /dev/null +++ b/scripts/aws_clear_instance.sh @@ -0,0 +1,14 @@ +#!/bin/bash + +echo "Removing application and dependencies" + +if [ -d "/home/notify-app/notifications-api" ]; then + # Remove and re-create the directory + rm -rf /home/notify-app/notifications-api + mkdir -vp /home/notify-app/notifications-api + # Remove installed py3 packages + pip3 freeze | xargs pip3 uninstall -y +else + echo "Directory does not exist, something went wrong!" +fi + From 458adefcb8e1f2af84363f1b68aebed735dcbb33 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 13 Feb 2017 18:47:29 +0000 Subject: [PATCH 02/10] Added a redis cache for the template usage stats. Cache expires every 10 minutes, but will help with the every 2 second query, especially when a job is running. There is some clean up and qa to do for this yet --- app/__init__.py | 6 + app/config.py | 2 + app/dao/templates_dao.py | 39 +- app/notifications/process_notifications.py | 3 +- app/template_statistics/rest.py | 33 +- tests/app/dao/test_templates_dao.py | 54 ++- .../rest/test_send_notification.py | 57 ++- .../test_process_notification.py | 8 +- tests/app/template_statistics/test_rest.py | 367 +++++++++--------- 9 files changed, 351 insertions(+), 218 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 2f834193b..70f3ae919 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -65,6 +65,8 @@ def create_app(app_name=None): aws_ses_client.init_app(application.config['AWS_REGION'], statsd_client=statsd_client) notify_celery.init_app(application) encryption.init_app(application) + print(os.environ['REDIS_URL']) + print(application.config['REDIS_ENABLED']) redis_store.init_app(application) performance_platform_client.init_app(application) clients.init_app(sms_clients=[firetext_client, mmg_client, loadtest_client], email_clients=[aws_ses_client]) @@ -176,3 +178,7 @@ def process_user_agent(user_agent_string): return "non-notify-user-agent" else: return "unknown" + + +def cache_key_for_service_template_counter(service_id, limit_days=7): + return "{}-template-counter-limit-{}-days".format(service_id, limit_days) diff --git a/app/config.py b/app/config.py index f5354dc3d..172422d07 100644 --- a/app/config.py +++ b/app/config.py @@ -49,6 +49,7 @@ 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 # Performance platform PERFORMANCE_PLATFORM_ENABLED = os.getenv('PERFORMANCE_PLATFORM_ENABLED') == '1' @@ -185,6 +186,7 @@ class Development(Config): Queue('research-mode', Exchange('default'), routing_key='research-mode') ] API_HOST_NAME = "http://localhost:6011" + REDIS_ENABLED = True class Test(Config): diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index f7f4c5e4d..064bc3605 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -1,6 +1,7 @@ import uuid -from sqlalchemy import (asc, desc) +import sqlalchemy +from sqlalchemy import (desc, cast, String, text) from app import db from app.models import (Template, TemplateHistory) @@ -56,3 +57,39 @@ def dao_get_template_versions(service_id, template_id): ).order_by( desc(TemplateHistory.version) ).all() + + +# def dao_get_templates_by_for_cache(cache): +# if not cache or len(cache) == 0: +# return [] + +# # First create a subquery that is a union select of the cache values +# # Then join templates to the subquery +# cache_queries = [ +# db.session.query(sqlalchemy.sql.expression.bindparam("template_id" + str(i), +# template_id).label('template_id'), +# sqlalchemy.sql.expression.bindparam("count" + str(i), count).label('count')) +# for i, (template_id, count) in enumerate(cache)] +# cache_subq = cache_queries[0].union(*cache_queries[1:]).subquery() +# query = db.session.query(Template.id.label('template_id'), +# Template.template_type, +# Template.name, +# cache_subq.c.count.label('count') +# ).join(cache_subq, +# cast(Template.id, String) == cast(cache_subq.c.template_id, String) +# ).order_by(Template.name) +# +# return query.all() + + +def dao_get_templates_by_for_cache(cache): + if not cache or len(cache) == 0: + return [] + txt = "( " + " Union all ".join( + "select '{}'::text as template_id, {} as count".format(x.decode(), + y.decode()) for x, y in cache) + " ) as cache" + txt = "Select t.id as template_id, t.template_type, t.name, cache.count from templates t, " + \ + txt + " where t.id::text = cache.template_id order by t.name" + stmt = text(txt) + + return db.session.execute(stmt).fetchall() diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 1b66bd36d..a4a743f3b 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -2,7 +2,7 @@ from datetime import datetime from flask import current_app -from app import redis_store +from app import redis_store, cache_key_for_service_template_counter from app.celery import provider_tasks from notifications_utils.clients import redis from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id @@ -63,6 +63,7 @@ def persist_notification(template_id, if not simulated: dao_create_notification(notification) redis_store.incr(redis.daily_limit_cache_key(service.id)) + redis_store.increment_hash_value(cache_key_for_service_template_counter(service.id), template_id) current_app.logger.info( "{} {} created at {}".format(notification.notification_type, notification.id, notification.created_at) ) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 87335c439..33875ed94 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -1,14 +1,16 @@ from flask import ( Blueprint, jsonify, - request -) + request, + current_app) +from app import redis_store from app.dao.notifications_dao import ( dao_get_template_usage, dao_get_last_template_usage) +from app.dao.templates_dao import dao_get_templates_by_for_cache -from app.schemas import notifications_filter_schema, NotificationWithTemplateSchema, notification_with_template_schema +from app.schemas import notification_with_template_schema template_statistics = Blueprint('template-statistics', __name__, @@ -30,7 +32,15 @@ def get_template_statistics_for_service_by_day(service_id): raise InvalidRequest(message, status_code=400) else: limit_days = None - stats = dao_get_template_usage(service_id, limit_days=limit_days) + + if limit_days == 7: + stats = get_template_statistics_for_7_days(limit_days, service_id) + print(stats) + # [(UUID('c2a331f8-e0b9-43de-9dd2-88300511a1d7'), 'Create with priority', 'sms', 1)] + + else: + stats = dao_get_template_usage(service_id, limit_days=limit_days) + print(stats) def serialize(data): return { @@ -52,3 +62,18 @@ def get_template_statistics_for_template_id(service_id, template_id): raise InvalidRequest(errors, status_code=404) data = notification_with_template_schema.dump(notification).data return jsonify(data=data) + + +def get_template_statistics_for_7_days(limit_days, service_id): + cache_key = "{}-template-counter-limit-7-days".format(service_id) + template_stats_by_id = redis_store.get_all_from_hash(cache_key) + if not template_stats_by_id: + print("populate cache") + stats = dao_get_template_usage(service_id, limit_days=limit_days) + cache_values = dict([(x.template_id, x.count) for x in stats]) + redis_store.set_hash_and_expire(cache_key, cache_values, current_app.config.get('EXPIRE_CACHE_IN_SECONDS', 600)) + current_app.logger.info('use redis-client: {}'.format(cache_key)) + else: + print("template_stats_by_id: {}".format(template_stats_by_id)) + stats = dao_get_templates_by_for_cache(template_stats_by_id.items()) + return stats diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 2670254c2..a34cb336c 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -4,7 +4,8 @@ from app.dao.templates_dao import ( dao_get_template_by_id_and_service_id, dao_get_all_templates_for_service, dao_update_template, - dao_get_template_versions) + dao_get_template_versions, + dao_get_templates_by_for_cache) from tests.app.conftest import sample_template as create_sample_template from app.models import Template, TemplateHistory import pytest @@ -265,3 +266,54 @@ def test_get_template_versions(sample_template): from app.schemas import template_history_schema v = template_history_schema.load(versions, many=True) assert len(v) == 2 + + +def test_get_templates_by_ids_successful(notify_db, notify_db_session): + template_1 = create_sample_template( + notify_db, + notify_db_session, + template_name='Sample Template 1', + template_type="sms", + content="Template content" + ) + template_2 = create_sample_template( + notify_db, + notify_db_session, + template_name='Sample Template 2', + template_type="sms", + content="Template content" + ) + create_sample_template( + notify_db, + notify_db_session, + template_name='Sample Template 3', + template_type="email", + content="Template content" + ) + sample_cache_dict = {str.encode(str(template_1.id)): str.encode('2'), + str.encode(str(template_2.id)): str.encode('3')} + cache = [[k, v] for k, v in sample_cache_dict.items()] + templates = dao_get_templates_by_for_cache(cache) + assert len(templates) == 2 + assert [(template_1.id, template_1.template_type, template_1.name, 2), + (template_2.id, template_2.template_type, template_2.name, 3)] == templates + + +def test_get_templates_by_ids_successful_for_one_cache_item(notify_db, notify_db_session): + template_1 = create_sample_template( + notify_db, + notify_db_session, + template_name='Sample Template 1', + template_type="sms", + content="Template content" + ) + sample_cache_dict = {str.encode(str(template_1.id)): str.encode('2')} + cache = [[k, v] for k, v in sample_cache_dict.items()] + templates = dao_get_templates_by_for_cache(cache) + assert len(templates) == 1 + assert [(template_1.id, template_1.template_type, template_1.name, 2)] == templates + + +def test_get_templates_by_ids_returns_empty_list(): + assert dao_get_templates_by_for_cache({}) == [] + assert dao_get_templates_by_for_cache(None) == [] diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index baf655b9a..f5efa1cea 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -670,42 +670,41 @@ def test_should_persist_notification(notify_api, sample_template, @pytest.mark.parametrize('template_type', ['sms', 'email']) def test_should_delete_notification_and_return_error_if_sqs_fails( - notify_api, + client, sample_email_template, sample_template, fake_uuid, mocker, template_type): - with notify_api.test_request_context(), notify_api.test_client() as client: - mocked = mocker.patch( - 'app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type), - side_effect=Exception("failed to talk to SQS") - ) - m1 = mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) - template = sample_template if template_type == 'sms' else sample_email_template - to = sample_template.service.created_by.mobile_number if template_type == 'sms' \ - else sample_email_template.service.created_by.email_address - data = { - 'to': to, - 'template': template.id - } - api_key = ApiKey( - service=template.service, - name='team_key', - created_by=template.created_by, - key_type=KEY_TYPE_TEAM) - save_model_api_key(api_key) - auth_header = create_jwt_token(secret=api_key.unsigned_secret, client_id=str(api_key.service_id)) + mocked = mocker.patch( + 'app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type), + side_effect=Exception("failed to talk to SQS") + ) + mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) + template = sample_template if template_type == 'sms' else sample_email_template + to = sample_template.service.created_by.mobile_number if template_type == 'sms' \ + else sample_email_template.service.created_by.email_address + data = { + 'to': to, + 'template': template.id + } + api_key = ApiKey( + service=template.service, + name='team_key', + created_by=template.created_by, + key_type=KEY_TYPE_TEAM) + save_model_api_key(api_key) + auth_header = create_jwt_token(secret=api_key.unsigned_secret, client_id=str(api_key.service_id)) - response = client.post( - path='/notifications/{}'.format(template_type), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + response = client.post( + path='/notifications/{}'.format(template_type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - mocked.assert_called_once_with([fake_uuid], queue='send-{}'.format(template_type)) - assert response.status_code == 500 - assert not notifications_dao.get_notification_by_id(fake_uuid) - assert not NotificationHistory.query.get(fake_uuid) + mocked.assert_called_once_with([fake_uuid], queue='send-{}'.format(template_type)) + assert response.status_code == 500 + assert not notifications_dao.get_notification_by_id(fake_uuid) + assert not NotificationHistory.query.get(fake_uuid) @pytest.mark.parametrize('to_email', [ diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 6679a7f2f..ebb5f2c50 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -7,6 +7,7 @@ from sqlalchemy.exc import SQLAlchemyError from freezegun import freeze_time from collections import namedtuple +from app import cache_key_for_service_template_counter from app.models import Template, Notification, NotificationHistory from app.notifications import SendNotificationToQueueError from app.notifications.process_notifications import (create_content_for_notification, @@ -114,7 +115,8 @@ def test_exception_thown_by_redis_store_get_should_not_be_fatal(sample_template, def test_cache_is_not_incremented_on_failure_to_persist_notification(sample_api_key, mocker): - mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.incr') + mocked_redis = mocker.patch('app.redis_store.incr') + mocked_redis_hash = mocker.patch('app.redis_store.increment_hash_value') with pytest.raises(SQLAlchemyError): persist_notification(template_id=None, template_version=None, @@ -125,6 +127,7 @@ def test_cache_is_not_incremented_on_failure_to_persist_notification(sample_api_ api_key_id=sample_api_key.id, key_type=sample_api_key.key_type) mocked_redis.assert_not_called() + mocked_redis_hash.assert_not_called() @freeze_time("2016-01-01 11:09:00.061258") @@ -132,6 +135,7 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.incr') + mocked_redis_hash = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value') n_id = uuid.uuid4() created_at = datetime.datetime(2016, 11, 11, 16, 8, 18) persist_notification(template_id=sample_job.template.id, @@ -154,6 +158,8 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) assert persisted_notification.job_row_number == 10 assert persisted_notification.created_at == created_at mocked_redis.assert_called_once_with(str(sample_job.service_id) + "-2016-01-01-count") + mocked_redis_hash.assert_called_once_with(cache_key_for_service_template_counter(sample_job.service_id), + sample_job.template.id) assert persisted_notification.client_reference == "ref from client" assert persisted_notification.reference is None diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index 8a9ad4ae3..a5c9bd0a4 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -1,222 +1,227 @@ from datetime import datetime, timedelta import json +import pytest from freezegun import freeze_time from tests import create_authorization_header -from tests.app.conftest import sample_template as create_sample_template, sample_template, sample_notification, \ - sample_email_template +from tests.app.conftest import (sample_template as create_sample_template, sample_notification, sample_email_template) -def test_get_all_template_statistics_with_bad_arg_returns_400(notify_api, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() +def test_get_all_template_statistics_with_bad_arg_returns_400(client, sample_service): + auth_header = create_authorization_header() - response = client.get( - '/service/{}/template-statistics'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - query_string={'limit_days': 'blurk'} - ) + response = client.get( + '/service/{}/template-statistics'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 'blurk'} + ) - assert response.status_code == 400 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['result'] == 'error' - assert json_resp['message'] == {'limit_days': ['blurk is not an integer']} + assert response.status_code == 400 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['result'] == 'error' + assert json_resp['message'] == {'limit_days': ['blurk is not an integer']} @freeze_time('2016-08-18') -def test_get_template_statistics_for_service(notify_db, notify_db_session, notify_api, sample_service): - sms = sample_template(notify_db, notify_db_session, service=sample_service) - email = sample_email_template(notify_db, notify_db_session, service=sample_service) - today = datetime.now() - sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) - sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) - sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) - sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) +def test_get_template_statistics_for_service(notify_db, notify_db_session, client, mocker): + email, sms = set_up_notifications(notify_db, notify_db_session) - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() + mocked_redis = mocker.patch('app.redis_store.get_all_from_hash') - response = client.get( - '/service/{}/template-statistics'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header] - ) + auth_header = create_authorization_header() - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 2 - assert json_resp['data'][0]['count'] == 2 - assert json_resp['data'][0]['template_id'] == str(email.id) - assert json_resp['data'][0]['template_name'] == email.name - assert json_resp['data'][0]['template_type'] == email.template_type - assert json_resp['data'][1]['count'] == 2 - assert json_resp['data'][1]['template_id'] == str(sms.id) - assert json_resp['data'][1]['template_name'] == sms.name - assert json_resp['data'][1]['template_type'] == sms.template_type + response = client.get( + '/service/{}/template-statistics'.format(email.service_id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 3 + assert json_resp['data'][0]['template_id'] == str(email.id) + assert json_resp['data'][0]['template_name'] == email.name + assert json_resp['data'][0]['template_type'] == email.template_type + assert json_resp['data'][1]['count'] == 3 + assert json_resp['data'][1]['template_id'] == str(sms.id) + assert json_resp['data'][1]['template_name'] == sms.name + assert json_resp['data'][1]['template_type'] == sms.template_type + + mocked_redis.assert_not_called() @freeze_time('2016-08-18') -def test_get_template_statistics_for_service_limited_by_day(notify_db, notify_db_session, notify_api, sample_service): - sms = sample_template(notify_db, notify_db_session, service=sample_service) - email = sample_email_template(notify_db, notify_db_session, service=sample_service) +def test_get_template_statistics_for_service_limited_1_day(notify_db, notify_db_session, client, + mocker): + email, sms = set_up_notifications(notify_db, notify_db_session) + mock_redis = mocker.patch('app.redis_store.get_all_from_hash') + + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics'.format(email.service_id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 1} + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True))['data'] + assert len(json_resp) == 2 + + assert json_resp[0]['count'] == 1 + assert json_resp[0]['template_id'] == str(email.id) + assert json_resp[0]['template_name'] == email.name + assert json_resp[0]['template_type'] == email.template_type + assert json_resp[1]['count'] == 1 + assert json_resp[1]['template_id'] == str(sms.id) + assert json_resp[1]['template_name'] == sms.name + assert json_resp[1]['template_type'] == sms.template_type + + mock_redis.assert_not_called() + + +@pytest.mark.parametrize("cache_values", [False, True]) +@freeze_time('2016-08-18') +def test_get_template_statistics_for_service_limit_7_days(notify_db, notify_db_session, client, + mocker, + cache_values): + email, sms = set_up_notifications(notify_db, notify_db_session) + mock_cache_values = {str.encode(str(sms.id)): str.encode('2'), + str.encode(str(email.id)): str.encode('2')} if cache_values else None + mocked_redis_get = mocker.patch('app.redis_store.get_all_from_hash', return_value=mock_cache_values) + mocked_redis_set = mocker.patch('app.redis_store.set_hash_and_expire') + + auth_header = create_authorization_header() + response_for_a_week = client.get( + '/service/{}/template-statistics'.format(email.service_id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 7} + ) + + assert response_for_a_week.status_code == 200 + json_resp = json.loads(response_for_a_week.get_data(as_text=True)) + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 2 + assert json_resp['data'][0]['template_name'] == 'Email Template Name' + assert json_resp['data'][1]['count'] == 2 + assert json_resp['data'][1]['template_name'] == 'Template Name' + + mocked_redis_get.assert_called_once_with("{}-template-counter-limit-7-days".format(email.service_id)) + if cache_values: + mocked_redis_set.assert_not_called() + else: + mocked_redis_set.assert_called_once_with("{}-template-counter-limit-7-days".format(email.service_id), + {sms.id: 2, email.id: 2}, 600) + + +@freeze_time('2016-08-18') +def test_get_template_statistics_for_service_limit_30_days(notify_db, notify_db_session, client, + mocker): + email, sms = set_up_notifications(notify_db, notify_db_session) + mock_redis = mocker.patch('app.redis_store.get_all_from_hash') + + auth_header = create_authorization_header() + + response_for_a_month = client.get( + '/service/{}/template-statistics'.format(email.service_id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 30} + ) + + assert response_for_a_month.status_code == 200 + json_resp = json.loads(response_for_a_month.get_data(as_text=True)) + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 3 + assert json_resp['data'][0]['template_name'] == 'Email Template Name' + assert json_resp['data'][1]['count'] == 3 + assert json_resp['data'][1]['template_name'] == 'Template Name' + + mock_redis.assert_not_called() + + +@freeze_time('2016-08-18') +def test_get_template_statistics_for_service_no_limit(notify_db, notify_db_session, client, + mocker): + email, sms = set_up_notifications(notify_db, notify_db_session) + mock_redis = mocker.patch('app.redis_store.get_all_from_hash') + auth_header = create_authorization_header() + response_for_all = client.get( + '/service/{}/template-statistics'.format(email.service_id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + assert response_for_all.status_code == 200 + json_resp = json.loads(response_for_all.get_data(as_text=True)) + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['count'] == 3 + assert json_resp['data'][0]['template_name'] == 'Email Template Name' + assert json_resp['data'][1]['count'] == 3 + assert json_resp['data'][1]['template_name'] == 'Template Name' + + mock_redis.assert_not_called() + + +def set_up_notifications(notify_db, notify_db_session): + sms = create_sample_template(notify_db, notify_db_session) + email = sample_email_template(notify_db, notify_db_session) today = datetime.now() a_week_ago = datetime.now() - timedelta(days=7) a_month_ago = datetime.now() - timedelta(days=30) - sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=sms) - sample_notification(notify_db, notify_db_session, created_at=today, service=sample_service, template=email) - sample_notification(notify_db, notify_db_session, created_at=a_week_ago, service=sample_service, template=sms) - sample_notification(notify_db, notify_db_session, created_at=a_week_ago, service=sample_service, template=email) - sample_notification(notify_db, notify_db_session, created_at=a_month_ago, service=sample_service, template=sms) - sample_notification(notify_db, notify_db_session, created_at=a_month_ago, service=sample_service, template=email) - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - - response = client.get( - '/service/{}/template-statistics'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - query_string={'limit_days': 1} - ) - - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 2 - assert json_resp['data'][0]['count'] == 1 - assert json_resp['data'][0]['template_id'] == str(email.id) - assert json_resp['data'][0]['template_name'] == email.name - assert json_resp['data'][0]['template_type'] == email.template_type - assert json_resp['data'][1]['count'] == 1 - assert json_resp['data'][1]['template_id'] == str(sms.id) - assert json_resp['data'][1]['template_name'] == sms.name - assert json_resp['data'][1]['template_type'] == sms.template_type - - response_for_a_week = client.get( - '/service/{}/template-statistics'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - query_string={'limit_days': 7} - ) - - assert response.status_code == 200 - json_resp = json.loads(response_for_a_week.get_data(as_text=True)) - assert len(json_resp['data']) == 2 - assert json_resp['data'][0]['count'] == 2 - assert json_resp['data'][0]['template_name'] == 'Email Template Name' - assert json_resp['data'][1]['count'] == 2 - assert json_resp['data'][1]['template_name'] == 'Template Name' - - response_for_a_month = client.get( - '/service/{}/template-statistics'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - query_string={'limit_days': 30} - ) - - assert response_for_a_month.status_code == 200 - json_resp = json.loads(response_for_a_month.get_data(as_text=True)) - assert len(json_resp['data']) == 2 - assert json_resp['data'][0]['count'] == 3 - assert json_resp['data'][0]['template_name'] == 'Email Template Name' - assert json_resp['data'][1]['count'] == 3 - assert json_resp['data'][1]['template_name'] == 'Template Name' - - response_for_all = client.get( - '/service/{}/template-statistics'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header] - ) - - assert response_for_all.status_code == 200 - json_resp = json.loads(response_for_all.get_data(as_text=True)) - assert len(json_resp['data']) == 2 - assert json_resp['data'][0]['count'] == 3 - assert json_resp['data'][0]['template_name'] == 'Email Template Name' - assert json_resp['data'][1]['count'] == 3 - assert json_resp['data'][1]['template_name'] == 'Template Name' + sample_notification(notify_db, notify_db_session, created_at=today, template=sms) + sample_notification(notify_db, notify_db_session, created_at=today, template=email) + sample_notification(notify_db, notify_db_session, created_at=a_week_ago, template=sms) + sample_notification(notify_db, notify_db_session, created_at=a_week_ago, template=email) + sample_notification(notify_db, notify_db_session, created_at=a_month_ago, template=sms) + sample_notification(notify_db, notify_db_session, created_at=a_month_ago, template=email) + return email, sms @freeze_time('2016-08-18') -def test_returns_empty_list_if_no_templates_used(notify_api, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() +def test_returns_empty_list_if_no_templates_used(client, sample_service): + auth_header = create_authorization_header() - response = client.get( - '/service/{}/template-statistics'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header] - ) + response = client.get( + '/service/{}/template-statistics'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header] + ) - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 0 + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 0 def test_get_template_statistics_by_id_returns_last_notification( notify_db, notify_db_session, - notify_api, - sample_service): + client): + sample_notification(notify_db, notify_db_session) + sample_notification(notify_db, notify_db_session) + notification_3 = sample_notification(notify_db, notify_db_session) - template = create_sample_template( - notify_db, - notify_db_session, - template_name='Sample Template 1', - service=sample_service + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/{}'.format(notification_3.service_id, notification_3.template_id), + headers=[('Content-Type', 'application/json'), auth_header], ) - notification_1 = sample_notification( - notify_db, - notify_db_session, - service=sample_service, - template=template) - notification_2 = sample_notification( - notify_db, - notify_db_session, - service=sample_service, - template=template) - notification_3 = sample_notification( - notify_db, - notify_db_session, - service=sample_service, - template=template) - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - - response = client.get( - '/service/{}/template-statistics/{}'.format(sample_service.id, template.id), - headers=[('Content-Type', 'application/json'), auth_header], - ) - - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True))['data'] - assert json_resp['id'] == str(notification_3.id) + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True))['data'] + assert json_resp['id'] == str(notification_3.id) def test_get_template_statistics_for_template_returns_empty_if_no_statistics( - notify_db, - notify_db_session, - notify_api, - sample_service + client, + sample_template, ): - template = create_sample_template( - notify_db, - notify_db_session, - template_name='Sample Template 1', - service=sample_service + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/{}'.format(sample_template.service_id, sample_template.id), + headers=[('Content-Type', 'application/json'), auth_header], ) - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - - response = client.get( - '/service/{}/template-statistics/{}'.format(sample_service.id, template.id), - headers=[('Content-Type', 'application/json'), auth_header], - ) - - assert response.status_code == 404 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['result'] == 'error' - assert json_resp['message']['template_id'] == ['No template found for id {}'.format(template.id)] + assert response.status_code == 404 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['result'] == 'error' + assert json_resp['message']['template_id'] == ['No template found for id {}'.format(sample_template.id)] From 2346634fec061c6770d44730b07ea8c79956ab22 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 14 Feb 2017 09:54:37 +0000 Subject: [PATCH 03/10] Some code clean up, removed print statements. --- app/dao/templates_dao.py | 23 ----------------------- app/template_statistics/rest.py | 6 ------ 2 files changed, 29 deletions(-) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index 064bc3605..c69602b12 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -59,29 +59,6 @@ def dao_get_template_versions(service_id, template_id): ).all() -# def dao_get_templates_by_for_cache(cache): -# if not cache or len(cache) == 0: -# return [] - -# # First create a subquery that is a union select of the cache values -# # Then join templates to the subquery -# cache_queries = [ -# db.session.query(sqlalchemy.sql.expression.bindparam("template_id" + str(i), -# template_id).label('template_id'), -# sqlalchemy.sql.expression.bindparam("count" + str(i), count).label('count')) -# for i, (template_id, count) in enumerate(cache)] -# cache_subq = cache_queries[0].union(*cache_queries[1:]).subquery() -# query = db.session.query(Template.id.label('template_id'), -# Template.template_type, -# Template.name, -# cache_subq.c.count.label('count') -# ).join(cache_subq, -# cast(Template.id, String) == cast(cache_subq.c.template_id, String) -# ).order_by(Template.name) -# -# return query.all() - - def dao_get_templates_by_for_cache(cache): if not cache or len(cache) == 0: return [] diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 33875ed94..fecc7fc7c 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -35,12 +35,8 @@ def get_template_statistics_for_service_by_day(service_id): if limit_days == 7: stats = get_template_statistics_for_7_days(limit_days, service_id) - print(stats) - # [(UUID('c2a331f8-e0b9-43de-9dd2-88300511a1d7'), 'Create with priority', 'sms', 1)] - else: stats = dao_get_template_usage(service_id, limit_days=limit_days) - print(stats) def serialize(data): return { @@ -68,12 +64,10 @@ def get_template_statistics_for_7_days(limit_days, service_id): cache_key = "{}-template-counter-limit-7-days".format(service_id) template_stats_by_id = redis_store.get_all_from_hash(cache_key) if not template_stats_by_id: - print("populate cache") stats = dao_get_template_usage(service_id, limit_days=limit_days) cache_values = dict([(x.template_id, x.count) for x in stats]) redis_store.set_hash_and_expire(cache_key, cache_values, current_app.config.get('EXPIRE_CACHE_IN_SECONDS', 600)) current_app.logger.info('use redis-client: {}'.format(cache_key)) else: - print("template_stats_by_id: {}".format(template_stats_by_id)) stats = dao_get_templates_by_for_cache(template_stats_by_id.items()) return stats From 681f52b691875d844aa88aa475b82bdeab2e18a3 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 14 Feb 2017 09:59:33 +0000 Subject: [PATCH 04/10] Missed some print statements and unused imports. --- app/__init__.py | 2 -- app/config.py | 1 - app/dao/templates_dao.py | 3 +-- app/template_statistics/rest.py | 1 - 4 files changed, 1 insertion(+), 6 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 70f3ae919..64888c870 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -65,8 +65,6 @@ def create_app(app_name=None): aws_ses_client.init_app(application.config['AWS_REGION'], statsd_client=statsd_client) notify_celery.init_app(application) encryption.init_app(application) - print(os.environ['REDIS_URL']) - print(application.config['REDIS_ENABLED']) redis_store.init_app(application) performance_platform_client.init_app(application) clients.init_app(sms_clients=[firetext_client, mmg_client, loadtest_client], email_clients=[aws_ses_client]) diff --git a/app/config.py b/app/config.py index 172422d07..45762a33a 100644 --- a/app/config.py +++ b/app/config.py @@ -186,7 +186,6 @@ class Development(Config): Queue('research-mode', Exchange('default'), routing_key='research-mode') ] API_HOST_NAME = "http://localhost:6011" - REDIS_ENABLED = True class Test(Config): diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index c69602b12..db0c29e19 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -1,7 +1,6 @@ import uuid -import sqlalchemy -from sqlalchemy import (desc, cast, String, text) +from sqlalchemy import (desc, text) from app import db from app.models import (Template, TemplateHistory) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index fecc7fc7c..4ed1f0d89 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -67,7 +67,6 @@ def get_template_statistics_for_7_days(limit_days, service_id): stats = dao_get_template_usage(service_id, limit_days=limit_days) cache_values = dict([(x.template_id, x.count) for x in stats]) redis_store.set_hash_and_expire(cache_key, cache_values, current_app.config.get('EXPIRE_CACHE_IN_SECONDS', 600)) - current_app.logger.info('use redis-client: {}'.format(cache_key)) else: stats = dao_get_templates_by_for_cache(template_stats_by_id.items()) return stats From 1c6cfb9bc834b5c809c35dc4ff59a5621ac6c612 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 14 Feb 2017 11:05:23 +0000 Subject: [PATCH 05/10] Got the SqlAlchemy query to work --- app/dao/templates_dao.py | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index db0c29e19..c98dc3e70 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -1,6 +1,7 @@ import uuid -from sqlalchemy import (desc, text) +import sqlalchemy +from sqlalchemy import desc from app import db from app.models import (Template, TemplateHistory) @@ -61,11 +62,21 @@ def dao_get_template_versions(service_id, template_id): def dao_get_templates_by_for_cache(cache): if not cache or len(cache) == 0: return [] - txt = "( " + " Union all ".join( - "select '{}'::text as template_id, {} as count".format(x.decode(), - y.decode()) for x, y in cache) + " ) as cache" - txt = "Select t.id as template_id, t.template_type, t.name, cache.count from templates t, " + \ - txt + " where t.id::text = cache.template_id order by t.name" - stmt = text(txt) - return db.session.execute(stmt).fetchall() + # First create a subquery that is a union select of the cache values + # Then join templates to the subquery + cache_queries = [ + db.session.query(sqlalchemy.sql.expression.bindparam("template_id" + str(i), + uuid.UUID(template_id.decode())).label('template_id'), + sqlalchemy.sql.expression.bindparam("count" + str(i), int(count.decode())).label('count')) + for i, (template_id, count) in enumerate(cache)] + cache_subq = cache_queries[0].union(*cache_queries[1:]).subquery() + query = db.session.query(Template.id.label('template_id'), + Template.template_type, + Template.name, + cache_subq.c.count.label('count') + ).join(cache_subq, + Template.id == cache_subq.c.template_id + ).order_by(Template.name) + + return query.all() From 5ac3643d9d7da24f6c9ae87bbe43b9eaad44e934 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 14 Feb 2017 11:36:38 +0000 Subject: [PATCH 06/10] Cancel uninstalling all py3 packages --- scripts/aws_clear_instance.sh | 4 ---- 1 file changed, 4 deletions(-) diff --git a/scripts/aws_clear_instance.sh b/scripts/aws_clear_instance.sh index b62397b06..645d07318 100755 --- a/scripts/aws_clear_instance.sh +++ b/scripts/aws_clear_instance.sh @@ -6,9 +6,5 @@ if [ -d "/home/notify-app/notifications-api" ]; then # Remove and re-create the directory rm -rf /home/notify-app/notifications-api mkdir -vp /home/notify-app/notifications-api - # Remove installed py3 packages - pip3 freeze | xargs pip3 uninstall -y -else - echo "Directory does not exist, something went wrong!" fi From 2e4b09154ed599c90774391398aa0176c96f33d1 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 14 Feb 2017 11:37:47 +0000 Subject: [PATCH 07/10] Remove verbose logging of directory creation --- scripts/aws_clear_instance.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/aws_clear_instance.sh b/scripts/aws_clear_instance.sh index 645d07318..9cdc14568 100755 --- a/scripts/aws_clear_instance.sh +++ b/scripts/aws_clear_instance.sh @@ -5,6 +5,6 @@ echo "Removing application and dependencies" if [ -d "/home/notify-app/notifications-api" ]; then # Remove and re-create the directory rm -rf /home/notify-app/notifications-api - mkdir -vp /home/notify-app/notifications-api + mkdir -p /home/notify-app/notifications-api fi From 53b7ad096168c752a8ab27177fee69bf6459a3d6 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 14 Feb 2017 14:22:52 +0000 Subject: [PATCH 08/10] Moved the cache key to the utils module. Renamed the dao method. --- app/__init__.py | 4 ---- app/dao/templates_dao.py | 10 +++++----- app/notifications/process_notifications.py | 4 ++-- app/template_statistics/rest.py | 7 ++++--- app/utils.py | 4 ++++ tests/app/dao/test_templates_dao.py | 10 +++++----- .../app/notifications/test_process_notification.py | 13 +++++++------ 7 files changed, 27 insertions(+), 25 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 64888c870..2f834193b 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -176,7 +176,3 @@ def process_user_agent(user_agent_string): return "non-notify-user-agent" else: return "unknown" - - -def cache_key_for_service_template_counter(service_id, limit_days=7): - return "{}-template-counter-limit-{}-days".format(service_id, limit_days) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index c98dc3e70..567e763f3 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -1,7 +1,7 @@ import uuid -import sqlalchemy from sqlalchemy import desc +from sqlalchemy.sql.expression import bindparam from app import db from app.models import (Template, TemplateHistory) @@ -59,16 +59,16 @@ def dao_get_template_versions(service_id, template_id): ).all() -def dao_get_templates_by_for_cache(cache): +def dao_get_templates_for_cache(cache): if not cache or len(cache) == 0: return [] # First create a subquery that is a union select of the cache values # Then join templates to the subquery cache_queries = [ - db.session.query(sqlalchemy.sql.expression.bindparam("template_id" + str(i), - uuid.UUID(template_id.decode())).label('template_id'), - sqlalchemy.sql.expression.bindparam("count" + str(i), int(count.decode())).label('count')) + db.session.query(bindparam("template_id" + str(i), + uuid.UUID(template_id.decode())).label('template_id'), + bindparam("count" + str(i), int(count.decode())).label('count')) for i, (template_id, count) in enumerate(cache)] cache_subq = cache_queries[0].union(*cache_queries[1:]).subquery() query = db.session.query(Template.id.label('template_id'), diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index a4a743f3b..caad56c28 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -2,13 +2,13 @@ from datetime import datetime from flask import current_app -from app import redis_store, cache_key_for_service_template_counter +from app import redis_store from app.celery import provider_tasks from notifications_utils.clients import redis from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE from app.v2.errors import BadRequestError, SendNotificationToQueueError -from app.utils import get_template_instance +from app.utils import get_template_instance, cache_key_for_service_template_counter def create_content_for_notification(template, personalisation): diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 4ed1f0d89..16cd17672 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -8,9 +8,10 @@ from app import redis_store from app.dao.notifications_dao import ( dao_get_template_usage, dao_get_last_template_usage) -from app.dao.templates_dao import dao_get_templates_by_for_cache +from app.dao.templates_dao import dao_get_templates_for_cache from app.schemas import notification_with_template_schema +from app.utils import cache_key_for_service_template_counter template_statistics = Blueprint('template-statistics', __name__, @@ -61,12 +62,12 @@ def get_template_statistics_for_template_id(service_id, template_id): def get_template_statistics_for_7_days(limit_days, service_id): - cache_key = "{}-template-counter-limit-7-days".format(service_id) + cache_key = cache_key_for_service_template_counter(service_id) template_stats_by_id = redis_store.get_all_from_hash(cache_key) if not template_stats_by_id: stats = dao_get_template_usage(service_id, limit_days=limit_days) cache_values = dict([(x.template_id, x.count) for x in stats]) redis_store.set_hash_and_expire(cache_key, cache_values, current_app.config.get('EXPIRE_CACHE_IN_SECONDS', 600)) else: - stats = dao_get_templates_by_for_cache(template_stats_by_id.items()) + stats = dao_get_templates_for_cache(template_stats_by_id.items()) return stats diff --git a/app/utils.py b/app/utils.py index 522d008f5..15d38e42d 100644 --- a/app/utils.py +++ b/app/utils.py @@ -63,3 +63,7 @@ def get_london_month_from_utc_column(column): "month", func.timezone("Europe/London", func.timezone("UTC", column)) ) + + +def cache_key_for_service_template_counter(service_id, limit_days=7): + return "{}-template-counter-limit-{}-days".format(service_id, limit_days) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index a34cb336c..182f91928 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -5,7 +5,7 @@ from app.dao.templates_dao import ( dao_get_all_templates_for_service, dao_update_template, dao_get_template_versions, - dao_get_templates_by_for_cache) + dao_get_templates_for_cache) from tests.app.conftest import sample_template as create_sample_template from app.models import Template, TemplateHistory import pytest @@ -293,7 +293,7 @@ def test_get_templates_by_ids_successful(notify_db, notify_db_session): sample_cache_dict = {str.encode(str(template_1.id)): str.encode('2'), str.encode(str(template_2.id)): str.encode('3')} cache = [[k, v] for k, v in sample_cache_dict.items()] - templates = dao_get_templates_by_for_cache(cache) + templates = dao_get_templates_for_cache(cache) assert len(templates) == 2 assert [(template_1.id, template_1.template_type, template_1.name, 2), (template_2.id, template_2.template_type, template_2.name, 3)] == templates @@ -309,11 +309,11 @@ def test_get_templates_by_ids_successful_for_one_cache_item(notify_db, notify_db ) sample_cache_dict = {str.encode(str(template_1.id)): str.encode('2')} cache = [[k, v] for k, v in sample_cache_dict.items()] - templates = dao_get_templates_by_for_cache(cache) + templates = dao_get_templates_for_cache(cache) assert len(templates) == 1 assert [(template_1.id, template_1.template_type, template_1.name, 2)] == templates def test_get_templates_by_ids_returns_empty_list(): - assert dao_get_templates_by_for_cache({}) == [] - assert dao_get_templates_by_for_cache(None) == [] + assert dao_get_templates_for_cache({}) == [] + assert dao_get_templates_for_cache(None) == [] diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index ebb5f2c50..ceccc6976 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -7,13 +7,13 @@ from sqlalchemy.exc import SQLAlchemyError from freezegun import freeze_time from collections import namedtuple -from app import cache_key_for_service_template_counter from app.models import Template, Notification, NotificationHistory from app.notifications import SendNotificationToQueueError from app.notifications.process_notifications import (create_content_for_notification, persist_notification, send_notification_to_queue, simulated_recipient) +from app.utils import cache_key_for_service_template_counter from app.v2.errors import BadRequestError @@ -116,7 +116,7 @@ def test_exception_thown_by_redis_store_get_should_not_be_fatal(sample_template, def test_cache_is_not_incremented_on_failure_to_persist_notification(sample_api_key, mocker): mocked_redis = mocker.patch('app.redis_store.incr') - mocked_redis_hash = mocker.patch('app.redis_store.increment_hash_value') + mock_service_template_cache = mocker.patch('app.redis_store.increment_hash_value') with pytest.raises(SQLAlchemyError): persist_notification(template_id=None, template_version=None, @@ -127,7 +127,7 @@ def test_cache_is_not_incremented_on_failure_to_persist_notification(sample_api_ api_key_id=sample_api_key.id, key_type=sample_api_key.key_type) mocked_redis.assert_not_called() - mocked_redis_hash.assert_not_called() + mock_service_template_cache.assert_not_called() @freeze_time("2016-01-01 11:09:00.061258") @@ -135,7 +135,8 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.incr') - mocked_redis_hash = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value') + mock_service_template_cache = mocker.patch( + 'app.notifications.process_notifications.redis_store.increment_hash_value') n_id = uuid.uuid4() created_at = datetime.datetime(2016, 11, 11, 16, 8, 18) persist_notification(template_id=sample_job.template.id, @@ -158,8 +159,8 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) assert persisted_notification.job_row_number == 10 assert persisted_notification.created_at == created_at mocked_redis.assert_called_once_with(str(sample_job.service_id) + "-2016-01-01-count") - mocked_redis_hash.assert_called_once_with(cache_key_for_service_template_counter(sample_job.service_id), - sample_job.template.id) + mock_service_template_cache.assert_called_once_with(cache_key_for_service_template_counter(sample_job.service_id), + sample_job.template.id) assert persisted_notification.client_reference == "ref from client" assert persisted_notification.reference is None From 74e29708f95bf9ac76fc1aff3edbd310cefaac1d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 15 Feb 2017 11:49:19 +0000 Subject: [PATCH 09/10] Fix bug where the increment calls set count to 1 if the cache does not exist. --- app/notifications/process_notifications.py | 6 ++-- .../test_process_notification.py | 35 +++++++++++++++---- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index caad56c28..91ada9110 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -62,8 +62,10 @@ def persist_notification(template_id, ) if not simulated: dao_create_notification(notification) - redis_store.incr(redis.daily_limit_cache_key(service.id)) - redis_store.increment_hash_value(cache_key_for_service_template_counter(service.id), template_id) + if redis_store.get(redis.daily_limit_cache_key(service.id)): + 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) current_app.logger.info( "{} {} created at {}".format(notification.notification_type, notification.id, notification.created_at) ) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index ceccc6976..022aa9f06 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -45,7 +45,7 @@ def test_create_content_for_notification_fails_with_additional_personalisation(s @freeze_time("2016-01-01 11:09:00.061258") def test_persist_notification_creates_and_save_to_db(sample_template, sample_api_key, sample_job, mocker): - mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.incr') + mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.get') assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 @@ -115,8 +115,8 @@ def test_exception_thown_by_redis_store_get_should_not_be_fatal(sample_template, def test_cache_is_not_incremented_on_failure_to_persist_notification(sample_api_key, mocker): - mocked_redis = mocker.patch('app.redis_store.incr') - mock_service_template_cache = mocker.patch('app.redis_store.increment_hash_value') + mocked_redis = mocker.patch('app.redis_store.get') + mock_service_template_cache = mocker.patch('app.redis_store.get_all_from_hash') with pytest.raises(SQLAlchemyError): persist_notification(template_id=None, template_version=None, @@ -134,9 +134,9 @@ def test_cache_is_not_incremented_on_failure_to_persist_notification(sample_api_ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker): assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 - mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.incr') + mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.get') mock_service_template_cache = mocker.patch( - 'app.notifications.process_notifications.redis_store.increment_hash_value') + 'app.notifications.process_notifications.redis_store.get_all_from_hash') n_id = uuid.uuid4() created_at = datetime.datetime(2016, 11, 11, 16, 8, 18) persist_notification(template_id=sample_job.template.id, @@ -159,12 +159,33 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) assert persisted_notification.job_row_number == 10 assert persisted_notification.created_at == created_at mocked_redis.assert_called_once_with(str(sample_job.service_id) + "-2016-01-01-count") - mock_service_template_cache.assert_called_once_with(cache_key_for_service_template_counter(sample_job.service_id), - sample_job.template.id) + mock_service_template_cache.assert_called_once_with(cache_key_for_service_template_counter(sample_job.service_id)) assert persisted_notification.client_reference == "ref from client" assert persisted_notification.reference is None +@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') + + persist_notification(sample_template.id, sample_template.version, '+447111111111', + sample_template.service, {}, 'sms', sample_api_key.id, + sample_api_key.key_type, reference="ref") + mock_incr.assert_not_called() + mock_incr_hash_value.assert_not_called() + + 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(sample_template.id, sample_template.version, '+447111111122', + sample_template.service, {}, 'sms', sample_api_key.id, + 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) + + @pytest.mark.parametrize('research_mode, requested_queue, expected_queue, notification_type, key_type', [(True, None, 'research-mode', 'sms', 'normal'), (True, None, 'research-mode', 'email', 'normal'), From 07fc71cc4c02cd2d2f5ca21fcb938e504503a949 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 15 Feb 2017 11:59:31 +0000 Subject: [PATCH 10/10] Fix codestyle --- tests/app/notifications/test_process_notification.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 022aa9f06..89557ff94 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -170,8 +170,8 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa mock_incr_hash_value = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value') persist_notification(sample_template.id, sample_template.version, '+447111111111', - sample_template.service, {}, 'sms', sample_api_key.id, - sample_api_key.key_type, reference="ref") + sample_template.service, {}, 'sms', sample_api_key.id, + sample_api_key.key_type, reference="ref") mock_incr.assert_not_called() mock_incr_hash_value.assert_not_called() @@ -179,8 +179,8 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value={sample_template.id, 1}) persist_notification(sample_template.id, sample_template.version, '+447111111122', - sample_template.service, {}, 'sms', sample_api_key.id, - sample_api_key.key_type, reference="ref2") + sample_template.service, {}, 'sms', sample_api_key.id, + 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)