Merge pull request #3134 from alphagov/improve-sender-task

Improve sender task

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
@@ -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,11 +27,12 @@ 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)
-
+    service_id = service.id
     if not service.active:
         technical_failure(notification=notification)
         return
@@ -43,23 +40,25 @@ 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.from_id_and_service_id(template_id=notification.template_id,
+                                                                  service_id=service_id,
+                                                                  version=notification.template_version).__dict__
 
         template = SMSMessageTemplate(
-            template_model.__dict__,
+            template_dict,
             values=notification.personalisation,
             prefix=service.name,
             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,28 +72,31 @@ 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)
 
 
 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)
         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,
@@ -107,6 +109,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 +117,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)

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
@@ -103,7 +104,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 +117,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 <em>some HTML</em> & entities",
         reference=str(db_notification.id),
         sender=current_app.config['FROM_NUMBER']
@@ -136,6 +139,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 +198,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 +217,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 +636,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 +646,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,
@@ -726,49 +733,102 @@ 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')
 
-    send_to_providers.send_email_to_provider(sample_email_notification)
+    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)
 
-    # to_addresses
+
-    send_mock.assert_called_once_with(
+def test_send_sms_to_provider_should_return_template_if_found_in_redis(
-        ANY,
+        mocker, client, sample_template
-        # to_addresses
+):
-        'test@example.com',
+    from app.schemas import service_schema, template_schema
-        ANY,
+    service_dict = service_schema.dump(sample_template.service).data
-        body=ANY,
+    template_dict = template_schema.dump(sample_template).data
-        html_body=ANY,
+
-        reply_to_address=ANY,
+    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)