From dd686bd7a801159a50f3a85e9f821d00442fde8a Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 16 Feb 2021 14:24:40 +0000 Subject: [PATCH 1/2] Add caching and remove extra call to database Add caching by using the SeriralisedTemplate and SerialisedService objects Removed extra call to the database to fetch the notification after the commit by saving the created_at and key_type to a local variable. After the update to the notification to mark it as sending the db.session is committed. Any reference to the the Notification data model after that will require a query to fetch the object again because it is considered "dirty" or out of date. Added name, sms_prefix and email branding to SerialisedService. Refactor the get_html_options to work with the SerialisedService object. Removed the need to validate and format the to field by using `normalised_to`, since when persisting the notification the `normalised_to` field has already had this done. Removed the validate and format for reply_to_text for email reply_to, this has been done when the email address has been added via the frontend, no need to validate this address every time a services sends an email. --- app/delivery/send_to_providers.py | 62 ++++--- app/serialised_models.py | 3 + tests/app/delivery/test_send_to_providers.py | 186 ++++++++++++++----- 3 files changed, 176 insertions(+), 75 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 71a8c8d02..b5390cc1b 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -3,13 +3,10 @@ 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 +from app.dao.email_branding_dao import dao_get_email_branding_by_id from app.dao.notifications_dao import ( dao_update_notification ) @@ -18,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, @@ -31,10 +27,11 @@ from app.models import ( NOTIFICATION_SENDING, NOTIFICATION_STATUS_TYPES_COMPLETED ) +from app.serialised_models import SerialisedTemplate, SerialisedService def send_sms_to_provider(notification): - service = notification.service + service = SerialisedService.from_id(notification.service_id) if not service.active: technical_failure(notification=notification) @@ -43,7 +40,9 @@ 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_model = SerialisedTemplate.from_id_and_service_id( + template_id=notification.template_id, service_id=service.id, version=notification.template_version + ) template = SMSMessageTemplate( template_model.__dict__, @@ -51,7 +50,8 @@ def send_sms_to_provider(notification): prefix=service.name, show_prefix=service.prefix_sms, ) - + created_at = notification.created_at + key_type = notification.key_type 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) @@ -59,7 +59,7 @@ def send_sms_to_provider(notification): 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,10 +73,10 @@ 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: + if 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) @@ -87,14 +87,17 @@ def send_sms_to_provider(notification): def send_email_to_provider(notification): - service = notification.service + service = SerialisedService.from_id(notification.service_id) + if not service.active: technical_failure(notification=notification) return 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.from_id_and_service_id( + template_id=notification.template_id, service_id=service.id, version=notification.template_version + ).__dict__ html_email = HTMLEmailTemplate( template_dict, @@ -106,7 +109,8 @@ def send_email_to_provider(notification): template_dict, values=notification.personalisation ) - + created_at = notification.created_at + key_type = notification.key_type if service.research_mode or notification.key_type == KEY_TYPE_TEST: notification.reference = str(create_uuid()) update_notification_to_sending(notification, provider) @@ -115,22 +119,19 @@ def send_email_to_provider(notification): 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() - created_at).total_seconds() - delta_seconds = (datetime.utcnow() - notification.created_at).total_seconds() - - if notification.key_type == KEY_TYPE_TEST: + if 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) @@ -190,25 +191,28 @@ def get_logo_url(base_url, logo_file): def get_html_email_options(service): - if service.email_branding is None: return { 'govuk_banner': True, 'brand_banner': False, } + if isinstance(service.email_branding, str): + branding = dao_get_email_branding_by_id(service.email_branding) + else: + branding = service.email_branding logo_url = get_logo_url( current_app.config['ADMIN_BASE_URL'], - service.email_branding.logo - ) if service.email_branding.logo else None + branding.logo + ) if branding.logo else None return { - 'govuk_banner': service.email_branding.brand_type == BRANDING_BOTH, - 'brand_banner': service.email_branding.brand_type == BRANDING_ORG_BANNER, - 'brand_colour': service.email_branding.colour, + 'govuk_banner': branding.brand_type == BRANDING_BOTH, + 'brand_banner': branding.brand_type == BRANDING_ORG_BANNER, + 'brand_colour': branding.colour, 'brand_logo': logo_url, - 'brand_text': service.email_branding.text, - 'brand_name': service.email_branding.name, + 'brand_text': branding.text, + 'brand_name': branding.name, } 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 4ddfa0563..64ba6ea14 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 @@ -13,6 +14,7 @@ from app import notification_provider_clients, mmg_client, firetext_client from app.dao import notifications_dao from app.dao.provider_details_dao import get_provider_details_by_identifier from app.delivery import send_to_providers +from app.delivery.send_to_providers import get_html_email_options, get_logo_url from app.exceptions import NotificationTechnicalFailureException from app.models import ( Notification, @@ -24,13 +26,14 @@ from app.models import ( BRANDING_BOTH, BRANDING_ORG_BANNER ) +from app.serialised_models import SerialisedService from tests.app.db import ( create_service, create_template, create_notification, create_reply_to_email, create_service_sms_sender, - create_service_with_defined_sms_sender + create_service_with_defined_sms_sender, create_email_branding ) @@ -105,7 +108,9 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( db_notification = create_notification(template=sample_sms_template_with_html, to_field="+447234123123", personalisation={"name": "Jo"}, status='created', - reply_to_text=sample_sms_template_with_html.service.get_default_sms_sender()) + reply_to_text=sample_sms_template_with_html.service.get_default_sms_sender(), + normalised_to="447234123123" + ) mocker.patch('app.mmg_client.send_sms') @@ -114,7 +119,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="447234123123", content="Sample service: Hello Jo\nHere is some HTML & entities", reference=str(db_notification.id), sender=current_app.config['FROM_NUMBER'] @@ -136,7 +141,8 @@ 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", - personalisation={'name': 'Jo'} + personalisation={'name': 'Jo'}, + normalised_to="jo.smith@example.com", ) mocker.patch('app.aws_ses_client.send_email', return_value='reference') @@ -194,7 +200,8 @@ 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', - reply_to_text=sample_template.service.get_default_sms_sender()) + reply_to_text=sample_template.service.get_default_sms_sender(), + normalised_to='447234123123') mocker.patch('app.mmg_client.send_sms') @@ -634,7 +641,8 @@ def test_should_send_sms_to_international_providers( personalisation={"name": "Jo"}, status='created', international=False, - reply_to_text=sample_template.service.get_default_sms_sender() + reply_to_text=sample_template.service.get_default_sms_sender(), + normalised_to="447234123999" ) notification_international = create_notification( @@ -643,7 +651,8 @@ def test_should_send_sms_to_international_providers( personalisation={"name": "Jo"}, status='created', international=True, - reply_to_text=sample_template.service.get_default_sms_sender() + reply_to_text=sample_template.service.get_default_sms_sender(), + normalised_to='601117224412' ) send_to_providers.send_sms_to_provider( notification_uk @@ -726,49 +735,134 @@ def test_send_email_to_provider_uses_reply_to_from_notification( ) -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' +def test_send_sms_to_provider_should_use_normalised_to( + mocker, client, sample_template +): 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' + 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_format_email_address(sample_email_notification, mocker): - sample_email_notification.to = 'test@example.com\t' +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') + print("************* start") + send_to_providers.send_email_to_provider(notification) + print("End ******************") + send_mock.assert_called_once_with(ANY, + notification.normalised_to, + ANY, + body=ANY, + html_body=ANY, + reply_to_address=notification.reply_to_text) - 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, +def test_send_sms_to_provider_should_return_template_if_found_in_redis( + mocker, client, sample_template +): + 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 + + 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' + ) + + 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) + 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( + mocker, client, sample_email_template +): + 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 + + 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' + ) + 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) + 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) + + +def test_get_html_email_options_return_email_branding_from_serialised_service( + sample_service +): + branding = create_email_branding() + sample_service.email_branding = branding + service = SerialisedService.from_id(sample_service.id) + email_options = get_html_email_options(service) + assert email_options is not None + assert email_options == {'govuk_banner': branding.brand_type == BRANDING_BOTH, + 'brand_banner': branding.brand_type == BRANDING_ORG_BANNER, + 'brand_colour': branding.colour, + 'brand_logo': get_logo_url(current_app.config['ADMIN_BASE_URL'], branding.logo), + 'brand_text': branding.text, + 'brand_name': branding.name, + } + + +def test_get_html_email_options_add_email_branding_from_service(sample_service): + branding = create_email_branding() + sample_service.email_branding = branding + email_options = get_html_email_options(sample_service) + assert email_options is not None + assert email_options == {'govuk_banner': branding.brand_type == BRANDING_BOTH, + 'brand_banner': branding.brand_type == BRANDING_ORG_BANNER, + 'brand_colour': branding.colour, + 'brand_logo': get_logo_url(current_app.config['ADMIN_BASE_URL'], branding.logo), + 'brand_text': branding.text, + 'brand_name': branding.name, + } From b464894325338cd3d58fcbe298add68470ac7d5c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 18 Feb 2021 12:54:22 +0000 Subject: [PATCH 2/2] update to check for instance of SerialisedService --- app/delivery/send_to_providers.py | 2 +- tests/app/delivery/test_send_to_providers.py | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index b5390cc1b..1900fc250 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -196,7 +196,7 @@ def get_html_email_options(service): 'govuk_banner': True, 'brand_banner': False, } - if isinstance(service.email_branding, str): + if isinstance(service, SerialisedService): branding = dao_get_email_branding_by_id(service.email_branding) else: branding = service.email_branding diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 64ba6ea14..1cad72af3 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -756,9 +756,8 @@ def test_send_email_to_provider_should_user_normalised_to( notification = create_notification(template=sample_email_template, to_field='TEST@example.com', normalised_to='test@example.com') - print("************* start") + send_to_providers.send_email_to_provider(notification) - print("End ******************") send_mock.assert_called_once_with(ANY, notification.normalised_to, ANY,