From 2270832873c9a44d9cc86fa40ee5d8e6ef618e4c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 11 Feb 2021 13:51:15 +0000 Subject: [PATCH 1/4] Remove validate_and_format for email and phone numbers, using normalised_to instead because that function has already happened when persisting the notificaiton. Remove 2 extra select queries after the update and commit. Once a transaction is committed SQLAlchemy will query for the db model if referenced after a commit. --- app/delivery/send_to_providers.py | 28 ++++----- tests/app/delivery/test_send_to_providers.py | 60 +++----------------- 2 files changed, 21 insertions(+), 67 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 71a8c8d02..b30e1aeb2 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -3,10 +3,7 @@ from urllib import parse from datetime import datetime, timedelta from cachetools import TTLCache, cached from flask import current_app -from notifications_utils.recipients import ( - validate_and_format_phone_number, - validate_and_format_email_address -) + from notifications_utils.template import HTMLEmailTemplate, PlainTextEmailTemplate, SMSMessageTemplate from app import notification_provider_clients, statsd_client, create_uuid @@ -35,7 +32,7 @@ from app.models import ( def send_sms_to_provider(notification): service = notification.service - + service_id = service.id if not service.active: technical_failure(notification=notification) return @@ -52,14 +49,14 @@ def send_sms_to_provider(notification): show_prefix=service.prefix_sms, ) + created_at = notification.created_at if service.research_mode or notification.key_type == KEY_TYPE_TEST: update_notification_to_sending(notification, provider) send_sms_response(provider.get_name(), str(notification.id), notification.to) - else: try: provider.send_sms( - to=validate_and_format_phone_number(notification.to, international=notification.international), + to=notification.normalised_to, content=str(template), reference=str(notification.id), sender=notification.reply_to_text @@ -73,14 +70,14 @@ def send_sms_to_provider(notification): notification.billable_units = template.fragment_count update_notification_to_sending(notification, provider) - delta_seconds = (datetime.utcnow() - notification.created_at).total_seconds() + delta_seconds = (datetime.utcnow() - created_at).total_seconds() statsd_client.timing("sms.total-time", delta_seconds) if notification.key_type == KEY_TYPE_TEST: statsd_client.timing("sms.test-key.total-time", delta_seconds) else: statsd_client.timing("sms.live-key.total-time", delta_seconds) - if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE'): + if str(service_id) in current_app.config.get('HIGH_VOLUME_SERVICE'): statsd_client.timing("sms.live-key.high-volume.total-time", delta_seconds) else: statsd_client.timing("sms.live-key.not-high-volume.total-time", delta_seconds) @@ -88,6 +85,7 @@ def send_sms_to_provider(notification): def send_email_to_provider(notification): service = notification.service + service_id = service.id if not service.active: technical_failure(notification=notification) return @@ -107,6 +105,7 @@ def send_email_to_provider(notification): values=notification.personalisation ) + created_at = notification.created_at if service.research_mode or notification.key_type == KEY_TYPE_TEST: notification.reference = str(create_uuid()) update_notification_to_sending(notification, provider) @@ -114,27 +113,24 @@ def send_email_to_provider(notification): else: from_address = '"{}" <{}@{}>'.format(service.name, service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']) - - email_reply_to = notification.reply_to_text - reference = provider.send_email( from_address, - validate_and_format_email_address(notification.to), + notification.normalised_to, plain_text_email.subject, body=str(plain_text_email), html_body=str(html_email), - reply_to_address=validate_and_format_email_address(email_reply_to) if email_reply_to else None, + reply_to_address=notification.reply_to_text, ) notification.reference = reference update_notification_to_sending(notification, provider) - delta_seconds = (datetime.utcnow() - notification.created_at).total_seconds() + delta_seconds = (datetime.utcnow() - created_at).total_seconds() if notification.key_type == KEY_TYPE_TEST: statsd_client.timing("email.test-key.total-time", delta_seconds) else: statsd_client.timing("email.live-key.total-time", delta_seconds) - if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE'): + if str(service_id) in current_app.config.get('HIGH_VOLUME_SERVICE'): statsd_client.timing("email.live-key.high-volume.total-time", delta_seconds) else: statsd_client.timing("email.live-key.not-high-volume.total-time", delta_seconds) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 4ddfa0563..8be87fd6a 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -103,7 +103,9 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( mocker ): db_notification = create_notification(template=sample_sms_template_with_html, - to_field="+447234123123", personalisation={"name": "Jo"}, + to_field="+447234123123", + normalised_to=validate_and_format_phone_number("+447234123123", True), + personalisation={"name": "Jo"}, status='created', reply_to_text=sample_sms_template_with_html.service.get_default_sms_sender()) @@ -114,7 +116,7 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( ) mmg_client.send_sms.assert_called_once_with( - to=validate_and_format_phone_number("+447234123123"), + to=db_notification.normalised_to, content="Sample service: Hello Jo\nHere is some HTML & entities", reference=str(db_notification.id), sender=current_app.config['FROM_NUMBER'] @@ -136,6 +138,7 @@ def test_should_send_personalised_template_to_correct_email_provider_and_persist db_notification = create_notification( template=sample_email_template_with_html, to_field="jo.smith@example.com", + normalised_to="jo.smith@example.com", personalisation={'name': 'Jo'} ) @@ -194,6 +197,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( sample_template, mocker): db_notification = create_notification(template=sample_template, to_field='+447234123123', status='created', + normalised_to=validate_and_format_phone_number("+447234123123"), reply_to_text=sample_template.service.get_default_sms_sender()) mocker.patch('app.mmg_client.send_sms') @@ -212,7 +216,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( ) mmg_client.send_sms.assert_called_once_with( - to=validate_and_format_phone_number("+447234123123"), + to=db_notification.normalised_to, content="Sample service: This is a template:\nwith a newline", reference=str(db_notification.id), sender=current_app.config['FROM_NUMBER'] @@ -631,6 +635,7 @@ def test_should_send_sms_to_international_providers( notification_uk = create_notification( template=sample_template, to_field="+447234123999", + normalised_to=validate_and_format_phone_number("+447234123999", True), personalisation={"name": "Jo"}, status='created', international=False, @@ -640,6 +645,7 @@ def test_should_send_sms_to_international_providers( notification_international = create_notification( template=sample_template, to_field="+6011-17224412", + normalised_to=validate_and_format_phone_number("+6011-17224412", international=True), personalisation={"name": "Jo"}, status='created', international=True, @@ -724,51 +730,3 @@ def test_send_email_to_provider_uses_reply_to_from_notification( html_body=ANY, reply_to_address="test@test.com" ) - - -def test_send_email_to_provider_should_format_reply_to_email_address( - sample_email_template, - mocker): - mocker.patch('app.aws_ses_client.send_email', return_value='reference') - - db_notification = create_notification(template=sample_email_template, reply_to_text="test@test.com\t") - - send_to_providers.send_email_to_provider( - db_notification, - ) - - app.aws_ses_client.send_email.assert_called_once_with( - ANY, - ANY, - ANY, - body=ANY, - html_body=ANY, - reply_to_address="test@test.com" - ) - - -def test_send_sms_to_provider_should_format_phone_number(sample_notification, mocker): - sample_notification.to = '+44 (7123) 123-123' - send_mock = mocker.patch('app.mmg_client.send_sms') - - send_to_providers.send_sms_to_provider(sample_notification) - - assert send_mock.call_args[1]['to'] == '447123123123' - - -def test_send_email_to_provider_should_format_email_address(sample_email_notification, mocker): - sample_email_notification.to = 'test@example.com\t' - send_mock = mocker.patch('app.aws_ses_client.send_email', return_value='reference') - - send_to_providers.send_email_to_provider(sample_email_notification) - - # to_addresses - send_mock.assert_called_once_with( - ANY, - # to_addresses - 'test@example.com', - ANY, - body=ANY, - html_body=ANY, - reply_to_address=ANY, - ) From 200f8aad812398312fc64ea7ffd67e4ac03b1906 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 11 Feb 2021 16:08:46 +0000 Subject: [PATCH 2/4] Use the cached template object. By adding SerialisedTemplate we can avoid a database call for the template. This is useful when sending many many emails/sms for the same template/version. --- app/delivery/send_to_providers.py | 11 +++-- tests/app/delivery/test_send_to_providers.py | 51 ++++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index b30e1aeb2..6fdf31d86 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -28,6 +28,7 @@ from app.models import ( NOTIFICATION_SENDING, NOTIFICATION_STATUS_TYPES_COMPLETED ) +from app.serialised_models import SerialisedTemplate def send_sms_to_provider(notification): @@ -40,10 +41,12 @@ def send_sms_to_provider(notification): if notification.status == 'created': provider = provider_to_use(SMS_TYPE, notification.international) - template_model = dao_get_template_by_id(notification.template_id, notification.template_version) + template_dict = SerialisedTemplate.get_dict(template_id=notification.template_id, + service_id=service_id, + version=notification.template_version)['data'] template = SMSMessageTemplate( - template_model.__dict__, + template_dict, values=notification.personalisation, prefix=service.name, show_prefix=service.prefix_sms, @@ -92,7 +95,9 @@ def send_email_to_provider(notification): if notification.status == 'created': provider = provider_to_use(EMAIL_TYPE) - template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__ + template_dict = SerialisedTemplate.get_dict(template_id=notification.template_id, + service_id=service_id, + version=notification.template_version)['data'] html_email = HTMLEmailTemplate( template_dict, diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 8be87fd6a..ad8e98f0c 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -1,3 +1,4 @@ +import json import uuid from collections import namedtuple from datetime import datetime, timedelta @@ -730,3 +731,53 @@ def test_send_email_to_provider_uses_reply_to_from_notification( html_body=ANY, reply_to_address="test@test.com" ) + + +def test_send_sms_to_provider_should_return_template_if_found_in_redis( + mocker, client, sample_template +): + + from app.schemas import template_schema + template_dict = template_schema.dump(sample_template).data + + notification = create_notification(template=sample_template, + to_field= '+447700900855', + normalised_to=validate_and_format_phone_number('+447700900855')) + mocker.patch( + 'app.redis_store.get', + side_effect=[ + json.dumps({'data': template_dict}).encode('utf-8'), + ], + ) + mock_get_template = mocker.patch( + 'app.dao.templates_dao.dao_get_template_by_id_and_service_id' + ) + mocker.patch('app.mmg_client.send_sms') + # mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + + send_to_providers.send_sms_to_provider(notification) + assert mock_get_template.called is False + + +def test_send_email_to_provider_should_return_template_if_found_in_redis( + mocker, client, sample_email_template +): + from app.schemas import template_schema + template_dict = template_schema.dump(sample_email_template).data + + notification = create_notification(template=sample_email_template, + to_field='test@example.com', + normalised_to='test@example.com') + mocker.patch( + 'app.redis_store.get', + side_effect=[ + json.dumps({'data': template_dict}).encode('utf-8'), + ], + ) + mock_get_template = mocker.patch( + 'app.dao.templates_dao.dao_get_template_by_id_and_service_id' + ) + mocker.patch('app.aws_ses_client.send_email', return_value='reference') + + send_to_providers.send_email_to_provider(notification) + assert mock_get_template.called is False From 61af203ad666955b8aeee468ecea1f92d981c9ac Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 11 Feb 2021 16:39:25 +0000 Subject: [PATCH 3/4] User cache for service in send_to_provider methods. This will remove a call to the db if the service exists in the cache. --- app/config.py | 1 + app/delivery/send_to_providers.py | 8 ++--- app/serialised_models.py | 3 ++ tests/app/delivery/test_send_to_providers.py | 31 +++++++++++++------- 4 files changed, 29 insertions(+), 14 deletions(-) diff --git a/app/config.py b/app/config.py index 372334cab..0e6aab7a4 100644 --- a/app/config.py +++ b/app/config.py @@ -430,6 +430,7 @@ class Development(Config): class Test(Development): + SQLALCHEMY_ECHO = False NOTIFY_EMAIL_DOMAIN = 'test.notify.com' FROM_NUMBER = 'testing' NOTIFY_ENVIRONMENT = 'test' diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 6fdf31d86..44a8fc736 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -28,11 +28,11 @@ from app.models import ( NOTIFICATION_SENDING, NOTIFICATION_STATUS_TYPES_COMPLETED ) -from app.serialised_models import SerialisedTemplate +from app.serialised_models import SerialisedTemplate, SerialisedService def send_sms_to_provider(notification): - service = notification.service + service = SerialisedService.from_id(notification.service_id) service_id = service.id if not service.active: technical_failure(notification=notification) @@ -42,7 +42,7 @@ def send_sms_to_provider(notification): provider = provider_to_use(SMS_TYPE, notification.international) template_dict = SerialisedTemplate.get_dict(template_id=notification.template_id, - service_id=service_id, + service_id=service.id, version=notification.template_version)['data'] template = SMSMessageTemplate( @@ -87,7 +87,7 @@ def send_sms_to_provider(notification): def send_email_to_provider(notification): - service = notification.service + service = SerialisedService.from_id(notification.service_id) service_id = service.id if not service.active: technical_failure(notification=notification) diff --git a/app/serialised_models.py b/app/serialised_models.py index 31870a3d5..8f9c93d69 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -75,6 +75,7 @@ class SerialisedTemplate(SerialisedModel): class SerialisedService(SerialisedModel): ALLOWED_PROPERTIES = { 'id', + 'name', 'active', 'contact_link', 'email_from', @@ -83,6 +84,8 @@ class SerialisedService(SerialisedModel): 'rate_limit', 'research_mode', 'restricted', + 'prefix_sms', + 'email_branding' } @classmethod diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index ad8e98f0c..e41eb43cb 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -736,48 +736,59 @@ def test_send_email_to_provider_uses_reply_to_from_notification( def test_send_sms_to_provider_should_return_template_if_found_in_redis( mocker, client, sample_template ): - - from app.schemas import template_schema + from app.schemas import service_schema, template_schema + service_dict = service_schema.dump(sample_template.service).data template_dict = template_schema.dump(sample_template).data - notification = create_notification(template=sample_template, - to_field= '+447700900855', - normalised_to=validate_and_format_phone_number('+447700900855')) mocker.patch( 'app.redis_store.get', side_effect=[ + json.dumps({'data': service_dict}).encode('utf-8'), json.dumps({'data': template_dict}).encode('utf-8'), ], ) mock_get_template = mocker.patch( 'app.dao.templates_dao.dao_get_template_by_id_and_service_id' ) + mock_get_service = mocker.patch( + 'app.dao.services_dao.dao_fetch_service_by_id' + ) + mocker.patch('app.mmg_client.send_sms') # mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - + notification = create_notification(template=sample_template, + to_field='+447700900855', + normalised_to=validate_and_format_phone_number('+447700900855')) send_to_providers.send_sms_to_provider(notification) assert mock_get_template.called is False + assert mock_get_service.called is False def test_send_email_to_provider_should_return_template_if_found_in_redis( mocker, client, sample_email_template ): - from app.schemas import template_schema + from app.schemas import service_schema, template_schema + service_dict = service_schema.dump(sample_email_template.service).data template_dict = template_schema.dump(sample_email_template).data - notification = create_notification(template=sample_email_template, - to_field='test@example.com', - normalised_to='test@example.com') mocker.patch( 'app.redis_store.get', side_effect=[ + json.dumps({'data': service_dict}).encode('utf-8'), json.dumps({'data': template_dict}).encode('utf-8'), ], ) mock_get_template = mocker.patch( 'app.dao.templates_dao.dao_get_template_by_id_and_service_id' ) + mock_get_service = mocker.patch( + 'app.dao.services_dao.dao_fetch_service_by_id' + ) mocker.patch('app.aws_ses_client.send_email', return_value='reference') + notification = create_notification(template=sample_email_template, + to_field='test@example.com', + normalised_to='test@example.com') send_to_providers.send_email_to_provider(notification) assert mock_get_template.called is False + assert mock_get_service.called is False From d67f9fcfd604b3b9a03cfb049c99a4fc7113270f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 15 Feb 2021 12:41:50 +0000 Subject: [PATCH 4/4] Use from_id_and_service_id method from SerialisedTemplate. Minor updates as per requested from review --- app/config.py | 1 - app/delivery/send_to_providers.py | 13 +++-- tests/app/delivery/test_send_to_providers.py | 50 ++++++++++++++++++-- 3 files changed, 51 insertions(+), 13 deletions(-) diff --git a/app/config.py b/app/config.py index 0e6aab7a4..372334cab 100644 --- a/app/config.py +++ b/app/config.py @@ -430,7 +430,6 @@ class Development(Config): class Test(Development): - SQLALCHEMY_ECHO = False NOTIFY_EMAIL_DOMAIN = 'test.notify.com' FROM_NUMBER = 'testing' NOTIFY_ENVIRONMENT = 'test' diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 44a8fc736..e72772972 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -15,7 +15,6 @@ from app.dao.provider_details_dao import ( dao_reduce_sms_provider_priority ) from app.celery.research_mode_tasks import send_sms_response, send_email_response -from app.dao.templates_dao import dao_get_template_by_id from app.exceptions import NotificationTechnicalFailureException from app.models import ( SMS_TYPE, @@ -41,9 +40,9 @@ def send_sms_to_provider(notification): if notification.status == 'created': provider = provider_to_use(SMS_TYPE, notification.international) - template_dict = SerialisedTemplate.get_dict(template_id=notification.template_id, - service_id=service.id, - version=notification.template_version)['data'] + template_dict = SerialisedTemplate.from_id_and_service_id(template_id=notification.template_id, + service_id=service_id, + version=notification.template_version).__dict__ template = SMSMessageTemplate( template_dict, @@ -95,9 +94,9 @@ def send_email_to_provider(notification): if notification.status == 'created': provider = provider_to_use(EMAIL_TYPE) - template_dict = SerialisedTemplate.get_dict(template_id=notification.template_id, - service_id=service_id, - version=notification.template_version)['data'] + template_dict = SerialisedTemplate.from_id_and_service_id(template_id=notification.template_id, + service_id=service_id, + version=notification.template_version).__dict__ html_email = HTMLEmailTemplate( template_dict, diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index e41eb43cb..ec27798b1 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -733,6 +733,37 @@ def test_send_email_to_provider_uses_reply_to_from_notification( ) +def test_send_sms_to_provider_should_use_normalised_to( + mocker, client, sample_template +): + send_mock = mocker.patch('app.mmg_client.send_sms') + notification = create_notification(template=sample_template, + to_field='+447700900855', + normalised_to='447700900855') + send_to_providers.send_sms_to_provider(notification) + send_mock.assert_called_once_with(to=notification.normalised_to, + content=ANY, + reference=str(notification.id), + sender=notification.reply_to_text) + + +def test_send_email_to_provider_should_user_normalised_to( + mocker, client, sample_email_template +): + send_mock = mocker.patch('app.aws_ses_client.send_email', return_value='reference') + notification = create_notification(template=sample_email_template, + to_field='TEST@example.com', + normalised_to='test@example.com') + + send_to_providers.send_email_to_provider(notification) + send_mock.assert_called_once_with(ANY, + notification.normalised_to, + ANY, + body=ANY, + html_body=ANY, + reply_to_address=notification.reply_to_text) + + def test_send_sms_to_provider_should_return_template_if_found_in_redis( mocker, client, sample_template ): @@ -754,14 +785,17 @@ def test_send_sms_to_provider_should_return_template_if_found_in_redis( 'app.dao.services_dao.dao_fetch_service_by_id' ) - mocker.patch('app.mmg_client.send_sms') - # mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + send_mock = mocker.patch('app.mmg_client.send_sms') notification = create_notification(template=sample_template, to_field='+447700900855', - normalised_to=validate_and_format_phone_number('+447700900855')) + normalised_to='447700900855') send_to_providers.send_sms_to_provider(notification) assert mock_get_template.called is False assert mock_get_service.called is False + send_mock.assert_called_once_with(to=notification.normalised_to, + content=ANY, + reference=str(notification.id), + sender=notification.reply_to_text) def test_send_email_to_provider_should_return_template_if_found_in_redis( @@ -784,11 +818,17 @@ def test_send_email_to_provider_should_return_template_if_found_in_redis( mock_get_service = mocker.patch( 'app.dao.services_dao.dao_fetch_service_by_id' ) - mocker.patch('app.aws_ses_client.send_email', return_value='reference') + send_mock = mocker.patch('app.aws_ses_client.send_email', return_value='reference') notification = create_notification(template=sample_email_template, - to_field='test@example.com', + to_field='TEST@example.com', normalised_to='test@example.com') send_to_providers.send_email_to_provider(notification) assert mock_get_template.called is False assert mock_get_service.called is False + send_mock.assert_called_once_with(ANY, + notification.normalised_to, + ANY, + body=ANY, + html_body=ANY, + reply_to_address=notification.reply_to_text)