From 7260561ad57e9bb37c1ab1d6834afbb43d02ea47 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 1 Dec 2016 17:20:05 +0000 Subject: [PATCH 1/4] Makes the app use the redis and statsd clients from utils --- app/__init__.py | 4 +- app/clients/redis/__init__.py | 5 -- app/clients/redis/redis_client.py | 40 ----------- app/clients/statsd/__init__.py | 0 app/clients/statsd/statsd_client.py | 31 --------- app/notifications/process_notifications.py | 2 +- app/notifications/validators.py | 2 +- tests/app/clients/test_redis_client.py | 81 ---------------------- 8 files changed, 4 insertions(+), 161 deletions(-) delete mode 100644 app/clients/redis/__init__.py delete mode 100644 app/clients/redis/redis_client.py delete mode 100644 app/clients/statsd/__init__.py delete mode 100644 app/clients/statsd/statsd_client.py delete mode 100644 tests/app/clients/test_redis_client.py diff --git a/app/__init__.py b/app/__init__.py index 8b22ec77d..4184e80d2 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -6,17 +6,17 @@ from flask import request, url_for, g, jsonify from flask.ext.sqlalchemy import SQLAlchemy from flask_marshmallow import Marshmallow from monotonic import monotonic +from notifications_utils.clients.statsd.statsd_client import StatsdClient +from notifications_utils.clients.redis.redis_client import RedisClient from notifications_utils import logging, request_id from werkzeug.local import LocalProxy from app.celery.celery import NotifyCelery from app.clients import Clients from app.clients.email.aws_ses import AwsSesClient -from app.clients.redis.redis_client import RedisClient from app.clients.sms.firetext import FiretextClient from app.clients.sms.loadtesting import LoadtestingClient from app.clients.sms.mmg import MMGClient -from app.clients.statsd.statsd_client import StatsdClient from app.encryption import Encryption diff --git a/app/clients/redis/__init__.py b/app/clients/redis/__init__.py deleted file mode 100644 index dbffb0dbd..000000000 --- a/app/clients/redis/__init__.py +++ /dev/null @@ -1,5 +0,0 @@ -from datetime import datetime - - -def daily_limit_cache_key(service_id): - return "{}-{}-{}".format(str(service_id), datetime.utcnow().strftime("%Y-%m-%d"), "count") diff --git a/app/clients/redis/redis_client.py b/app/clients/redis/redis_client.py deleted file mode 100644 index 18581ff30..000000000 --- a/app/clients/redis/redis_client.py +++ /dev/null @@ -1,40 +0,0 @@ -from flask.ext.redis import FlaskRedis -from flask import current_app - - -class RedisClient: - redis_store = FlaskRedis() - active = False - - def init_app(self, app): - self.active = app.config.get('REDIS_ENABLED') - if self.active: - self.redis_store.init_app(app) - - def set(self, key, value, ex=None, px=None, nx=False, xx=False, raise_exception=False): - if self.active: - try: - self.redis_store.set(key, value, ex, px, nx, xx) - except Exception as e: - current_app.logger.exception(e) - if raise_exception: - raise e - - def incr(self, key, raise_exception=False): - if self.active: - try: - return self.redis_store.incr(key) - except Exception as e: - current_app.logger.exception(e) - if raise_exception: - raise e - - def get(self, key, raise_exception=False): - if self.active: - try: - return self.redis_store.get(key) - except Exception as e: - current_app.logger.exception(e) - if raise_exception: - raise e - return None diff --git a/app/clients/statsd/__init__.py b/app/clients/statsd/__init__.py deleted file mode 100644 index e69de29bb..000000000 diff --git a/app/clients/statsd/statsd_client.py b/app/clients/statsd/statsd_client.py deleted file mode 100644 index 4c5daf32f..000000000 --- a/app/clients/statsd/statsd_client.py +++ /dev/null @@ -1,31 +0,0 @@ -from statsd import StatsClient - - -class StatsdClient(StatsClient): - def init_app(self, app, *args, **kwargs): - self.active = app.config.get('STATSD_ENABLED') - self.namespace = app.config.get('NOTIFY_ENVIRONMENT') + ".notifications.api." - - if self.active: - StatsClient.__init__( - self, - app.config.get('STATSD_HOST'), - app.config.get('STATSD_PORT'), - prefix=app.config.get('STATSD_PREFIX') - ) - - def format_stat_name(self, stat): - return self.namespace + stat - - def incr(self, stat, count=1, rate=1): - if self.active: - super(StatsClient, self).incr(self.format_stat_name(stat), count, rate) - - def timing(self, stat, delta, rate=1): - if self.active: - super(StatsClient, self).timing(self.format_stat_name(stat), delta, rate) - - def timing_with_dates(self, stat, start, end, rate=1): - if self.active: - delta = (start - end).total_seconds() - super(StatsClient, self).timing(self.format_stat_name(stat), delta, rate) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 72b079011..17feaa523 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -6,7 +6,7 @@ from notifications_utils.template import Template from app import redis_store from app.celery import provider_tasks -from app.clients import redis +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.notifications.validators import check_sms_content_char_count diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 2be1e54f5..63a46d6ff 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -5,7 +5,7 @@ from app.models import KEY_TYPE_TEST, KEY_TYPE_TEAM from app.service.utils import service_allowed_to_send_to from app.v2.errors import TooManyRequestsError, BadRequestError from app import redis_store -from app.clients import redis +from notifications_utils.clients import redis def check_service_message_limit(key_type, service): diff --git a/tests/app/clients/test_redis_client.py b/tests/app/clients/test_redis_client.py deleted file mode 100644 index 0634aa3b4..000000000 --- a/tests/app/clients/test_redis_client.py +++ /dev/null @@ -1,81 +0,0 @@ -import pytest -from unittest.mock import Mock - -from app.clients.redis.redis_client import RedisClient -from app.clients.redis import daily_limit_cache_key -from freezegun import freeze_time - - -@pytest.fixture(scope='function') -def enabled_redis_client(notify_api, mocker): - notify_api.config['REDIS_ENABLED'] = True - return build_redis_client(notify_api, mocker) - - -@pytest.fixture(scope='function') -def disabled_redis_client(notify_api, mocker): - notify_api.config['REDIS_ENABLED'] = False - return build_redis_client(notify_api, mocker) - - -def build_redis_client(notify_api, mocker): - redis_client = RedisClient() - redis_client.init_app(notify_api) - mocker.patch.object(redis_client.redis_store, 'get', return_value=100) - mocker.patch.object(redis_client.redis_store, 'set') - return redis_client - - -def test_should_not_raise_exception_if_raise_set_to_false(notify_api): - notify_api.config['REDIS_ENABLED'] = True - redis_client = RedisClient() - redis_client.init_app(notify_api) - redis_client.redis_store.get = Mock(side_effect=Exception()) - redis_client.redis_store.set = Mock(side_effect=Exception()) - redis_client.redis_store.incr = Mock(side_effect=Exception()) - assert redis_client.get('test') is None - assert redis_client.set('test', 'test') is None - assert redis_client.incr('test') is None - - -def test_should_raise_exception_if_raise_set_to_true(notify_api): - notify_api.config['REDIS_ENABLED'] = True - redis_client = RedisClient() - redis_client.init_app(notify_api) - redis_client.redis_store.get = Mock(side_effect=Exception('get failed')) - redis_client.redis_store.set = Mock(side_effect=Exception('set failed')) - redis_client.redis_store.incr = Mock(side_effect=Exception('inc failed')) - with pytest.raises(Exception) as e: - redis_client.get('test', raise_exception=True) - assert str(e.value) == 'get failed' - with pytest.raises(Exception) as e: - redis_client.set('test', 'test', raise_exception=True) - assert str(e.value) == 'set failed' - with pytest.raises(Exception) as e: - redis_client.incr('test', raise_exception=True) - assert str(e.value) == 'inc failed' - - -def test_should_not_call_set_if_not_enabled(disabled_redis_client): - assert not disabled_redis_client.set('key', 'value') - disabled_redis_client.redis_store.set.assert_not_called() - - -def test_should_call_set_if_enabled(enabled_redis_client): - enabled_redis_client.set('key', 'value') - enabled_redis_client.redis_store.set.assert_called_with('key', 'value', None, None, False, False) - - -def test_should_not_call_get_if_not_enabled(disabled_redis_client): - disabled_redis_client.set('key', 'value') - disabled_redis_client.redis_store.get.assert_not_called() - - -def test_should_call_get_if_enabled(enabled_redis_client): - assert enabled_redis_client.get('key') == 100 - enabled_redis_client.redis_store.get.assert_called_with('key') - - -def test_should_build_cache_key_service_and_action(sample_service): - with freeze_time("2016-01-01 12:00:00.000000"): - assert daily_limit_cache_key(sample_service.id) == '{}-2016-01-01-count'.format(sample_service.id) From 9fb378021c6e3e80cb365b39732a3fae9748f488 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 1 Dec 2016 17:24:23 +0000 Subject: [PATCH 2/4] Bumped version to use latest utils - note this is not yet published so will break..... - sorts out an issue with service id logging --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index bf185138e..32da14065 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,6 +22,6 @@ Flask-Redis==0.1.0 git+https://github.com/alphagov/notifications-python-client.git@3.0.0#egg=notifications-python-client==3.0.0 -git+https://github.com/alphagov/notifications-utils.git@10.3.1#egg=notifications-utils==10.3.1 +git+https://github.com/alphagov/notifications-utils.git@11.0.1#egg=notifications-utils==11.0.1 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From d449475dd53273582d898e966e789a42dcaa834c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Dec 2016 10:26:03 +0000 Subject: [PATCH 3/4] Incorporate breaking utils changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `.replaced…` methods on instances of `Template` were removed in https://github.com/alphagov/notifications-utils/pull/84 --- app/delivery/send_to_providers.py | 13 +++++++------ app/notifications/process_notifications.py | 2 +- app/notifications/rest.py | 7 ++++--- app/schemas.py | 5 +++-- app/template/rest.py | 6 +++++- app/v2/notifications/post_notifications.py | 6 +++--- .../app/notifications/test_process_notification.py | 4 ++-- 7 files changed, 25 insertions(+), 18 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 8c7c021a8..d273ec28a 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -1,10 +1,11 @@ from datetime import datetime from flask import current_app +from notifications_utils.field import Field from notifications_utils.recipients import ( validate_and_format_phone_number ) -from notifications_utils.template import Template, get_sms_fragment_count +from notifications_utils.template import Template from notifications_utils.renderers import HTMLEmail, PlainTextEmail, SMSMessage from app import clients, statsd_client, create_uuid @@ -35,11 +36,11 @@ def send_sms_to_provider(notification): else: provider.send_sms( to=validate_and_format_phone_number(notification.to), - content=template.replaced, + content=template.rendered, reference=str(notification.id), sender=service.sms_sender ) - notification.billable_units = get_sms_fragment_count(template.replaced_content_count) + notification.billable_units = template.sms_fragment_count notification.sent_at = datetime.utcnow() notification.sent_by = provider.get_name() @@ -83,9 +84,9 @@ def send_email_to_provider(notification): reference = provider.send_email( from_address, notification.to, - plain_text_email.replaced_subject, - body=plain_text_email.replaced, - html_body=html_email.replaced, + str(Field(plain_text_email.subject, notification.personalisation)), + body=plain_text_email.rendered, + html_body=html_email.rendered, reply_to_address=service.reply_to_email_address, ) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 17feaa523..549b9d595 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -22,7 +22,7 @@ def create_content_for_notification(template, personalisation): check_placeholders(template_object) if template_object.template_type == SMS_TYPE: - check_sms_content_char_count(template_object.replaced_content_count) + check_sms_content_char_count(template_object.content_count) return template_object diff --git a/app/notifications/rest.py b/app/notifications/rest.py index fe3210e4d..75ba2aca9 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -7,6 +7,7 @@ from flask import ( current_app, json ) +from notifications_utils.field import Field from notifications_utils.renderers import PassThrough from notifications_utils.template import Template @@ -254,13 +255,13 @@ def send_notification(notification_type): def get_notification_return_data(notification_id, notification, template): output = { - 'body': template.replaced, + 'body': template.rendered, 'template_version': notification['template_version'], 'notification': {'id': notification_id} } if template.template_type == 'email': - output.update({'subject': template.replaced_subject}) + output.update({'subject': str(Field(template.subject, template.values))}) return output @@ -304,7 +305,7 @@ def create_template_object_for_notification(template, personalisation): if ( template_object.template_type == SMS_TYPE and - template_object.replaced_content_count > current_app.config.get('SMS_CHAR_COUNT_LIMIT') + template_object.content_count > current_app.config.get('SMS_CHAR_COUNT_LIMIT') ): char_count = current_app.config.get('SMS_CHAR_COUNT_LIMIT') message = 'Content has a character count greater than the limit of {}'.format(char_count) diff --git a/app/schemas.py b/app/schemas.py index 0c913427e..185519303 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -15,6 +15,7 @@ from marshmallow import ( ) from marshmallow_sqlalchemy import field_for +from notifications_utils.field import Field from notifications_utils.recipients import ( validate_email_address, InvalidEmailError, @@ -395,10 +396,10 @@ class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): in_data['personalisation'], renderer=PassThrough() ) - in_data['body'] = template.replaced + in_data['body'] = template.rendered template_type = in_data['template']['template_type'] if template_type == 'email': - in_data['subject'] = template.replaced_subject + in_data['subject'] = str(Field(template.subject, in_data['personalisation'])) in_data['content_char_count'] = None else: in_data['content_char_count'] = len(in_data['body']) diff --git a/app/template/rest.py b/app/template/rest.py index c1d00cac0..05654f87f 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -13,6 +13,7 @@ from app.dao.templates_dao import ( dao_get_all_templates_for_service, dao_get_template_versions ) +from notifications_utils.field import Field from notifications_utils.template import Template from notifications_utils.renderers import PassThrough from app.dao.services_dao import dao_fetch_service_by_id @@ -108,7 +109,10 @@ def preview_template_by_id_and_service_id(service_id, template_id): ]}, status_code=400 ) - data['subject'], data['content'] = template_object.replaced_subject, template_object.replaced + data['subject'], data['content'] = ( + str(Field(template_object.subject, template_object.values)), + template_object.rendered + ) return jsonify(data) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 4f63c299d..9e15e7a3d 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -42,7 +42,7 @@ def post_sms_notification(): send_notification_to_queue(notification, service.research_mode) resp = create_post_sms_response_from_notification(notification, - template_with_content.replaced, + template_with_content.rendered, service.sms_sender, request.url_root) return jsonify(resp), 201 @@ -70,7 +70,7 @@ def post_email_notification(): send_notification_to_queue(notification, service.research_mode) resp = create_post_email_response_from_notification(notification=notification, - content=template_with_content.replaced, + content=template_with_content.rendered, subject=template_with_content.subject, email_from=service.email_from, url_root=request.url_root) @@ -89,5 +89,5 @@ def __validate_template(form, service, notification_type): check_template_is_for_notification_type(notification_type, template.template_type) check_template_is_active(template) template_with_content = create_content_for_notification(template, form.get('personalisation', {})) - check_sms_content_char_count(template_with_content.replaced_content_count) + check_sms_content_char_count(template_with_content.content_count) return template, template_with_content diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 79db94c96..9a12fdf57 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -17,14 +17,14 @@ from tests.app.conftest import sample_notification, sample_template, sample_emai def test_create_content_for_notification_passes(sample_email_template): template = Template.query.get(sample_email_template.id) content = create_content_for_notification(template, None) - assert content.replaced == template.content + assert content.rendered == template.content def test_create_content_for_notification_with_placeholders_passes(sample_template_with_placeholders): template = Template.query.get(sample_template_with_placeholders.id) content = create_content_for_notification(template, {'name': 'Bobby'}) assert content.content == template.content - assert 'Bobby' in content.replaced + assert 'Bobby' in content.rendered def test_create_content_for_notification_fails_with_missing_personalisation(sample_template_with_placeholders): From 3342ca2993a5cc04b2435b6310badf5a5f50e85c Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 2 Dec 2016 16:56:23 +0000 Subject: [PATCH 4/4] Bumped version --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 32da14065..5d7b0e0d7 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,6 +22,6 @@ Flask-Redis==0.1.0 git+https://github.com/alphagov/notifications-python-client.git@3.0.0#egg=notifications-python-client==3.0.0 -git+https://github.com/alphagov/notifications-utils.git@11.0.1#egg=notifications-utils==11.0.1 +git+https://github.com/alphagov/notifications-utils.git@11.0.2#egg=notifications-utils==11.0.2 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3