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

@@ -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 key_type == KEY_TYPE_TEST:
-
-        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)
@@ -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
+        branding.logo
-    ) if service.email_branding.logo else None
+    ) if branding.logo else None
 
     return {
-        'govuk_banner': service.email_branding.brand_type == BRANDING_BOTH,
+        'govuk_banner': branding.brand_type == BRANDING_BOTH,
-        'brand_banner': service.email_branding.brand_type == BRANDING_ORG_BANNER,
+        'brand_banner': branding.brand_type == BRANDING_ORG_BANNER,
-        'brand_colour': service.email_branding.colour,
+        'brand_colour': branding.colour,
         'brand_logo': logo_url,
-        'brand_text': service.email_branding.text,
+        'brand_text': branding.text,
-        'brand_name': service.email_branding.name,
+        'brand_name': branding.name,
     }
 
 

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

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 <em>some HTML</em> & 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(
+def test_send_sms_to_provider_should_use_normalised_to(
-        sample_email_template,
+        mocker, client, sample_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')
-
+    notification = create_notification(template=sample_template,
-    send_to_providers.send_sms_to_provider(sample_notification)
+                                       to_field='+447700900855',
-
+                                       normalised_to='447700900855')
-    assert send_mock.call_args[1]['to'] == '447123123123'
+    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):
+def test_send_email_to_provider_should_user_normalised_to(
-    sample_email_notification.to = 'test@example.com\t'
+        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
+def test_send_sms_to_provider_should_return_template_if_found_in_redis(
-    send_mock.assert_called_once_with(
+        mocker, client, sample_template
-        ANY,
+):
-        # to_addresses
+    from app.schemas import service_schema, template_schema
-        'test@example.com',
+    service_dict = service_schema.dump(sample_template.service).data
-        ANY,
+    template_dict = template_schema.dump(sample_template).data
-        body=ANY,
+
-        html_body=ANY,
+    mocker.patch(
-        reply_to_address=ANY,
+        '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,
+                             }