Cache template in redis

This commit changes the code in post notification endpoint to handle a
serialised template (ie a `dict`) rather than a database object.

This is the first step towards being able to cache the template and not
hit the database on every request.

There should be no functional changes here, it’s just refactoring.

There are some changes to the tests where the signature of functions
has changed.

Importing of the template schema has to be done at a function level,
otherwise Marshmallow gets weird.

app/__init__.py

@@ -11,6 +11,7 @@ from gds_metrics import GDSMetrics
 from time import monotonic
 from notifications_utils.clients.zendesk.zendesk_client import ZendeskClient
 from notifications_utils.clients.statsd.statsd_client import StatsdClient
+from notifications_utils.clients.redis import RequestCache
 from notifications_utils.clients.redis.redis_client import RedisClient
 from notifications_utils.clients.encryption.encryption_client import Encryption
 from notifications_utils import logging, request_helper
@@ -55,6 +56,7 @@ encryption = Encryption()
 zendesk_client = ZendeskClient()
 statsd_client = StatsdClient()
 redis_store = RedisClient()
+request_cache = RequestCache(redis_store)
 performance_platform_client = PerformancePlatformClient()
 document_download_client = DocumentDownloadClient()
 metrics = GDSMetrics()

app/notifications/process_letter_notifications.py

@@ -5,14 +5,22 @@ from app.models import LETTER_TYPE
 from app.notifications.process_notifications import persist_notification
 
 
-def create_letter_notification(letter_data, template, api_key, status, reply_to_text=None, billable_units=None):
+def create_letter_notification(
+    letter_data,
+    template,
+    service,
+    api_key,
+    status,
+    reply_to_text=None,
+    billable_units=None,
+):
     notification = persist_notification(
         template_id=template.id,
-        template_version=template.version,
+        template_version=template._template['version'],
-        template_postage=template.postage,
+        template_postage=template._template['postage'],
         # we only accept addresses_with_underscores from the API (from CSV we also accept dashes, spaces etc)
         recipient=PostalAddress.from_personalisation(letter_data['personalisation']).normalised,
-        service=template.service,
+        service=service,
         personalisation=letter_data['personalisation'],
         notification_type=LETTER_TYPE,
         api_key_id=api_key.id,

app/notifications/process_notifications.py

@@ -9,6 +9,11 @@ from notifications_utils.recipients import (
     validate_and_format_phone_number,
     format_email_address
 )
+from notifications_utils.template import (
+    PlainTextEmailTemplate,
+    SMSMessageTemplate,
+    LetterPrintTemplate,
+)
 from notifications_utils.timezones import convert_bst_to_utc
 
 from app import redis_store
@@ -34,8 +39,18 @@ from app.dao.notifications_dao import (
 from app.v2.errors import BadRequestError
 
 
-def create_content_for_notification(template, personalisation):
+def create_content_for_notification(template_dict, personalisation):
-    template_object = template._as_utils_template_with_personalisation(personalisation)
+    if template_dict['template_type'] == EMAIL_TYPE:
+        template_object = PlainTextEmailTemplate(template_dict, personalisation)
+    if template_dict['template_type'] == SMS_TYPE:
+        template_object = SMSMessageTemplate(template_dict, personalisation)
+    if template_dict['template_type'] == LETTER_TYPE:
+        template_object = LetterPrintTemplate(
+            template_dict,
+            personalisation,
+            contact_block=template_dict['reply_to_text'],
+        )
+
     check_placeholders(template_object)
 
     return template_object

app/notifications/rest.py

@@ -100,12 +100,13 @@ def send_notification(notification_type):
 
     check_rate_limiting(authenticated_service, api_user)
 
-    template, template_with_content = validate_template(
+    template_with_content = validate_template(
         template_id=notification_form['template'],
         personalisation=notification_form.get('personalisation', {}),
         service=authenticated_service,
         notification_type=notification_type
     )
+    template_dict = template_with_content._template
 
     _service_allowed_to_send_to(notification_form, authenticated_service)
     if not service_has_permission(notification_type, authenticated_service.permissions):
@@ -118,9 +119,9 @@ def send_notification(notification_type):
         _service_can_send_internationally(authenticated_service, notification_form['to'])
     # Do not persist or send notification to the queue if it is a simulated recipient
     simulated = simulated_recipient(notification_form['to'], notification_type)
-    notification_model = persist_notification(template_id=template.id,
+    notification_model = persist_notification(template_id=template_dict['id'],
-                                              template_version=template.version,
+                                              template_version=template_dict['version'],
-                                              template_postage=template.postage,
+                                              template_postage=template_dict['postage'],
                                               recipient=request.get_json()['to'],
                                               service=authenticated_service,
                                               personalisation=notification_form.get('personalisation', None),
@@ -128,16 +129,16 @@ def send_notification(notification_type):
                                               api_key_id=api_user.id,
                                               key_type=api_user.key_type,
                                               simulated=simulated,
-                                              reply_to_text=template.get_reply_to_text()
+                                              reply_to_text=template_dict['reply_to_text']
                                               )
     if not simulated:
-        queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None
+        queue_name = QueueNames.PRIORITY if template_dict['process_type'] == PRIORITY else None
         send_notification_to_queue(notification=notification_model,
                                    research_mode=authenticated_service.research_mode,
                                    queue=queue_name)
     else:
         current_app.logger.debug("POST simulated notification for id: {}".format(notification_model.id))
-    notification_form.update({"template_version": template.version})
+    notification_form.update({"template_version": template_dict['version']})
 
     return jsonify(
         data=get_notification_return_data(

app/notifications/validators.py

@@ -16,7 +16,7 @@ from app.models import (
 )
 from app.service.utils import service_allowed_to_send_to
 from app.v2.errors import TooManyRequestsError, BadRequestError, RateLimitError
-from app import redis_store
+from app import redis_store, request_cache
 from app.notifications.process_notifications import create_content_for_notification
 from app.utils import get_public_notify_type_text
 from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id
@@ -62,7 +62,7 @@ def check_template_is_for_notification_type(notification_type, template_type):
 
 
 def check_template_is_active(template):
-    if template.archived:
+    if template['archived']:
         raise BadRequestError(fields=[{'template': 'Template has been deleted'}],
                               message="Template has been deleted")
 
@@ -138,18 +138,26 @@ def check_notification_content_is_not_empty(template_with_content):
         raise BadRequestError(message=message)
 
 
-def validate_template(template_id, personalisation, service, notification_type):
+@request_cache.set('template-{template_id}-version-None')
+def get_template_dict(template_id, service_id):
+    from app.schemas import template_schema
     try:
-        template = templates_dao.dao_get_template_by_id_and_service_id(
+        fetched_template = templates_dao.dao_get_template_by_id_and_service_id(
             template_id=template_id,
-            service_id=service.id
+            service_id=service_id
         )
     except NoResultFound:
         message = 'Template not found'
         raise BadRequestError(message=message,
                               fields=[{'template': message}])
 
-    check_template_is_for_notification_type(notification_type, template.template_type)
+    return template_schema.dump(fetched_template).data
+
+
+def validate_template(template_id, personalisation, service, notification_type):
+    template = get_template_dict(template_id, service.id)
+
+    check_template_is_for_notification_type(notification_type, template['template_type'])
     check_template_is_active(template)
 
     template_with_content = create_content_for_notification(template, personalisation)
@@ -158,7 +166,7 @@ def validate_template(template_id, personalisation, service, notification_type):
 
     check_content_char_count(template_with_content)
 
-    return template, template_with_content
+    return template_with_content
 
 
 def check_reply_to(service_id, reply_to_id, type_):

app/schemas.py

@@ -2,6 +2,7 @@ from datetime import (
     datetime,
     date,
     timedelta)
+from uuid import UUID
 from flask_marshmallow.fields import fields
 from marshmallow import (
     post_load,
@@ -334,6 +335,16 @@ class TemplateSchema(BaseTemplateSchema):
             if not subject or subject.strip() == '':
                 raise ValidationError('Invalid template subject', 'subject')
 
+    @post_dump()
+    def __post_dump(self, data):
+        for field in (
+            'service',
+            'created_by',
+            'template_redacted',
+        ):
+            if isinstance(data[field], UUID):
+                data[field] = str(data[field])
+
 
 class TemplateHistorySchema(BaseSchema):
 

app/v2/notifications/post_notifications.py

@@ -42,6 +42,7 @@ from app.notifications.process_letter_notifications import (
     create_letter_notification
 )
 from app.notifications.process_notifications import (
+    create_content_for_notification,
     persist_notification,
     persist_scheduled_notification,
     send_notification_to_queue,
@@ -76,6 +77,7 @@ from app.v2.utils import get_valid_json
 
 @v2_notification_blueprint.route('/{}'.format(LETTER_TYPE), methods=['POST'])
 def post_precompiled_letter_notification():
+    from app.schemas import template_schema
     request_json = get_valid_json()
     if 'content' not in (request_json or {}):
         return post_notification(LETTER_TYPE)
@@ -88,6 +90,9 @@ def post_precompiled_letter_notification():
     check_rate_limiting(authenticated_service, api_user)
 
     template = get_precompiled_letter_template(authenticated_service.id)
+    template = create_content_for_notification(
+        template_schema.dump(template).data, {}
+    )
 
     # For precompiled letters the to field will be set to Provided as PDF until the validation passes,
     # then the address of the letter will be set as the to field
@@ -95,13 +100,12 @@ def post_precompiled_letter_notification():
         'address_line_1': 'Provided as PDF'
     }
 
-    reply_to = get_reply_to_text(LETTER_TYPE, form, template)
-
     notification = process_letter_notification(
         letter_data=form,
         api_key=api_user,
         template=template,
-        reply_to_text=reply_to,
+        service=authenticated_service,
+        reply_to_text=template._template['reply_to_text'],
         precompiled=True
     )
 
@@ -135,20 +139,21 @@ def post_notification(notification_type):
 
     check_rate_limiting(authenticated_service, api_user)
 
-    template, template_with_content = validate_template(
+    template_with_content = validate_template(
         form['template_id'],
         form.get('personalisation', {}),
         authenticated_service,
         notification_type,
     )
 
-    reply_to = get_reply_to_text(notification_type, form, template)
+    reply_to = get_reply_to_text(notification_type, form, template_with_content)
 
     if notification_type == LETTER_TYPE:
         notification = process_letter_notification(
             letter_data=form,
             api_key=api_user,
-            template=template,
+            template=template_with_content,
+            service=authenticated_service,
             reply_to_text=reply_to
         )
     else:
@@ -156,11 +161,12 @@ def post_notification(notification_type):
             form=form,
             notification_type=notification_type,
             api_key=api_user,
-            template=template,
+            template=template_with_content,
             service=authenticated_service,
             reply_to_text=reply_to
         )
 
+        # Think this is redundant
         template_with_content.values = notification.personalisation
 
     if notification_type == SMS_TYPE:
@@ -237,7 +243,7 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ
     notification = persist_notification(
         notification_id=notification_id,
         template_id=template.id,
-        template_version=template.version,
+        template_version=template._template['version'],
         recipient=form_send_to,
         service=service,
         personalisation=personalisation,
@@ -255,7 +261,7 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ
         persist_scheduled_notification(notification.id, form["scheduled_for"])
     else:
         if not simulated:
-            queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None
+            queue_name = QueueNames.PRIORITY if template._template['process_type'] == PRIORITY else None
             send_notification_to_queue(
                 notification=notification,
                 research_mode=service.research_mode,
@@ -282,7 +288,7 @@ def save_email_to_queue(
     data = {
         "id": notification_id,
         "template_id": str(template.id),
-        "template_version": template.version,
+        "template_version": template._template['version'],
         "to": form['email_address'],
         "service_id": str(service_id),
         "personalisation": personalisation,
@@ -333,7 +339,7 @@ def process_document_uploads(personalisation_data, service, simulated=False):
     return personalisation_data, len(file_keys)
 
 
-def process_letter_notification(*, letter_data, api_key, template, reply_to_text, precompiled=False):
+def process_letter_notification(*, letter_data, api_key, template, service, reply_to_text, precompiled=False):
     if api_key.key_type == KEY_TYPE_TEAM:
         raise BadRequestError(message='Cannot send letters with a team api key', status_code=403)
 
@@ -344,6 +350,7 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text
         return process_precompiled_letter_notifications(letter_data=letter_data,
                                                         api_key=api_key,
                                                         template=template,
+                                                        service=service,
                                                         reply_to_text=reply_to_text)
 
     address = PostalAddress.from_personalisation(
@@ -378,6 +385,7 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text
 
     notification = create_letter_notification(letter_data=letter_data,
                                               template=template,
+                                              service=service,
                                               api_key=api_key,
                                               status=status,
                                               reply_to_text=reply_to_text)
@@ -399,7 +407,7 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text
     return notification
 
 
-def process_precompiled_letter_notifications(*, letter_data, api_key, template, reply_to_text):
+def process_precompiled_letter_notifications(*, letter_data, api_key, template, service, reply_to_text):
     try:
         status = NOTIFICATION_PENDING_VIRUS_CHECK
         letter_content = base64.b64decode(letter_data['content'])
@@ -408,6 +416,7 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, template,
 
     notification = create_letter_notification(letter_data=letter_data,
                                               template=template,
+                                              service=service,
                                               api_key=api_key,
                                               status=status,
                                               reply_to_text=reply_to_text)
@@ -439,7 +448,7 @@ def get_reply_to_text(notification_type, form, template):
         service_email_reply_to_id = form.get("email_reply_to_id", None)
         reply_to = check_service_email_reply_to_id(
             str(authenticated_service.id), service_email_reply_to_id, notification_type
-        ) or template.get_reply_to_text()
+        ) or template._template['reply_to_text']
 
     elif notification_type == SMS_TYPE:
         service_sms_sender_id = form.get("sms_sender_id", None)
@@ -449,9 +458,9 @@ def get_reply_to_text(notification_type, form, template):
         if sms_sender_id:
             reply_to = try_validate_and_format_phone_number(sms_sender_id)
         else:
-            reply_to = template.get_reply_to_text()
+            reply_to = template._template['reply_to_text']
 
     elif notification_type == LETTER_TYPE:
-        reply_to = template.get_reply_to_text()
+        reply_to = template._template['reply_to_text']
 
     return reply_to

tests/app/notifications/test_process_letter_notifications.py

@@ -2,6 +2,8 @@ from app.models import LETTER_TYPE
 from app.models import Notification
 from app.models import NOTIFICATION_CREATED
 from app.notifications.process_letter_notifications import create_letter_notification
+from app.notifications.process_notifications import create_content_for_notification
+from app.notifications.validators import get_template_dict
 
 
 def test_create_letter_notification_creates_notification(sample_letter_template, sample_api_key):
@@ -13,7 +15,17 @@ def test_create_letter_notification_creates_notification(sample_letter_template,
         }
     }
 
-    notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED)
+    template = create_content_for_notification(get_template_dict(
+        sample_letter_template.id, sample_letter_template.service_id
+    ), {})
+
+    notification = create_letter_notification(
+        data,
+        template,
+        sample_letter_template.service,
+        sample_api_key,
+        NOTIFICATION_CREATED,
+    )
 
     assert notification == Notification.query.one()
     assert notification.job is None
@@ -38,7 +50,17 @@ def test_create_letter_notification_sets_reference(sample_letter_template, sampl
         'reference': 'foo'
     }
 
-    notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED)
+    template = create_content_for_notification(get_template_dict(
+        sample_letter_template.id, sample_letter_template.service_id
+    ), {})
+
+    notification = create_letter_notification(
+        data,
+        template,
+        sample_letter_template.service,
+        sample_api_key,
+        NOTIFICATION_CREATED,
+    )
 
     assert notification.client_reference == 'foo'
 
@@ -52,7 +74,17 @@ def test_create_letter_notification_sets_billable_units(sample_letter_template,
         },
     }
 
-    notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED,
+    template = create_content_for_notification(get_template_dict(
-                                              billable_units=3)
+        sample_letter_template.id, sample_letter_template.service_id
+    ), {})
+
+    notification = create_letter_notification(
+        data,
+        template,
+        sample_letter_template.service,
+        sample_api_key,
+        NOTIFICATION_CREATED,
+        billable_units=3,
+    )
 
     assert notification.billable_units == 3

tests/app/notifications/test_process_notification.py

@@ -21,6 +21,7 @@ from app.notifications.process_notifications import (
     send_notification_to_queue,
     simulated_recipient
 )
+from app.notifications.validators import get_template_dict
 from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address
 from app.v2.errors import BadRequestError
 from tests.app.db import create_service, create_template
@@ -28,26 +29,30 @@ from tests.app.db import create_service, create_template
 
 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)
+    template_dict = get_template_dict(template.id, template.service_id)
+    content = create_content_for_notification(template_dict, None)
     assert str(content) == template.content + '\n'
 
 
 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'})
+    template_dict = get_template_dict(template.id, template.service_id)
+    content = create_content_for_notification(template_dict, {'name': 'Bobby'})
     assert content.content == template.content
     assert 'Bobby' in str(content)
 
 
 def test_create_content_for_notification_fails_with_missing_personalisation(sample_template_with_placeholders):
     template = Template.query.get(sample_template_with_placeholders.id)
+    template_dict = get_template_dict(template.id, template.service_id)
     with pytest.raises(BadRequestError):
-        create_content_for_notification(template, None)
+        create_content_for_notification(template_dict, None)
 
 
 def test_create_content_for_notification_allows_additional_personalisation(sample_template_with_placeholders):
     template = Template.query.get(sample_template_with_placeholders.id)
-    create_content_for_notification(template, {'name': 'Bobby', 'Additional placeholder': 'Data'})
+    template_dict = get_template_dict(template.id, template.service_id)
+    create_content_for_notification(template_dict, {'name': 'Bobby', 'Additional placeholder': 'Data'})
 
 
 @freeze_time("2016-01-01 11:09:00.061258")

tests/app/notifications/test_validators.py

@@ -19,6 +19,7 @@ from app.notifications.validators import (
     check_service_sms_sender_id,
     check_service_letter_contact_id,
     check_reply_to,
+    get_template_dict,
     service_can_send_to_recipient,
     validate_and_format_recipient,
     validate_template,
@@ -175,15 +176,17 @@ def test_check_template_is_for_notification_type_fails_when_template_type_does_n
 
 
 def test_check_template_is_active_passes(sample_template):
-    assert check_template_is_active(sample_template) is None
+    template_dict = get_template_dict(sample_template.id, sample_template.service_id)
+    assert check_template_is_active(template_dict) is None
 
 
 def test_check_template_is_active_fails(sample_template):
     sample_template.archived = True
     from app.dao.templates_dao import dao_update_template
     dao_update_template(sample_template)
+    template_dict = get_template_dict(sample_template.id, sample_template.service_id)
     with pytest.raises(BadRequestError) as e:
-        check_template_is_active(sample_template)
+        check_template_is_active(template_dict)
     assert e.value.status_code == 400
     assert e.value.message == 'Template has been deleted'
     assert e.value.fields == [{'template': 'Template has been deleted'}]
@@ -314,11 +317,11 @@ def test_check_content_char_count_passes_for_long_email_or_letter(sample_service
 
 def test_check_notification_content_is_not_empty_passes(notify_api, mocker, sample_service):
     template_id = create_template(sample_service, content="Content is not empty").id
-    template = templates_dao.dao_get_template_by_id_and_service_id(
+    template_dict = get_template_dict(
         template_id=template_id,
         service_id=sample_service.id
     )
-    template_with_content = create_content_for_notification(template, {})
+    template_with_content = create_content_for_notification(template_dict, {})
     assert check_notification_content_is_not_empty(template_with_content) is None
 
 
@@ -330,11 +333,11 @@ def test_check_notification_content_is_not_empty_fails(
     notify_api, mocker, sample_service, template_content, notification_values
 ):
     template_id = create_template(sample_service, content=template_content).id
-    template = templates_dao.dao_get_template_by_id_and_service_id(
+    template_dict = get_template_dict(
         template_id=template_id,
         service_id=sample_service.id
     )
-    template_with_content = create_content_for_notification(template, notification_values)
+    template_with_content = create_content_for_notification(template_dict, notification_values)
     with pytest.raises(BadRequestError) as e:
         check_notification_content_is_not_empty(template_with_content)
     assert e.value.status_code == 400
@@ -349,6 +352,10 @@ def test_validate_template(sample_service):
 
 def test_validate_template_calls_all_validators(mocker, fake_uuid, sample_service):
     template = create_template(sample_service, template_type="email")
+    template_dict = get_template_dict(
+        template_id=template.id,
+        service_id=sample_service.id
+    )
     mock_check_type = mocker.patch('app.notifications.validators.check_template_is_for_notification_type')
     mock_check_if_active = mocker.patch('app.notifications.validators.check_template_is_active')
     mock_create_conent = mocker.patch(
@@ -359,8 +366,8 @@ def test_validate_template_calls_all_validators(mocker, fake_uuid, sample_servic
     validate_template(template.id, {}, sample_service, "email")
 
     mock_check_type.assert_called_once_with("email", "email")
-    mock_check_if_active.assert_called_once_with(template)
+    mock_check_if_active.assert_called_once_with(template_dict)
-    mock_create_conent.assert_called_once_with(template, {})
+    mock_create_conent.assert_called_once_with(template_dict, {})
     mock_check_not_empty.assert_called_once_with("content")
     mock_check_message_is_too_long.assert_called_once_with("content")