Merge pull request #2873 from alphagov/serialise-template-immediately

Serialise template as soon as possible

app/notifications/process_letter_notifications.py

@@ -6,7 +6,14 @@ 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, updated_at=None
+    letter_data,
+    template,
+    service,
+    api_key,
+    status,
+    reply_to_text=None,
+    billable_units=None,
+    updated_at=None,
 ):
     notification = persist_notification(
         template_id=template.id,
@@ -14,7 +21,7 @@ def create_letter_notification(
         template_postage=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
@@ -44,7 +49,34 @@ REDIS_GET_AND_INCR_DAILY_LIMIT_DURATION_SECONDS = Histogram(
 
 
 def create_content_for_notification(template, personalisation):
-    template_object = template._as_utils_template_with_personalisation(personalisation)
+    if template.template_type == EMAIL_TYPE:
+        template_object = PlainTextEmailTemplate(
+            {
+                'content': template.content,
+                'subject': template.subject,
+                'template_type': template.template_type,
+            },
+            personalisation,
+        )
+    if template.template_type == SMS_TYPE:
+        template_object = SMSMessageTemplate(
+            {
+                'content': template.content,
+                'template_type': template.template_type,
+            },
+            personalisation,
+        )
+    if template.template_type == LETTER_TYPE:
+        template_object = LetterPrintTemplate(
+            {
+                'content': template.content,
+                'subject': template.subject,
+                'template_type': template.template_type,
+            },
+            personalisation,
+            contact_block=template.reply_to_text,
+        )
+
     check_placeholders(template_object)
 
     return template_object

app/notifications/rest.py

@@ -128,7 +128,7 @@ 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.reply_to_text
                                               )
     if not simulated:
         queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None

app/notifications/validators.py

@@ -8,7 +8,7 @@ from notifications_utils.recipients import (
 )
 from notifications_utils.clients.redis import rate_limit_cache_key, daily_limit_cache_key
 
-from app.dao import services_dao, templates_dao
+from app.dao import services_dao
 from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id
 from app.models import (
     INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE,
@@ -21,6 +21,7 @@ from app.notifications.process_notifications import create_content_for_notificat
 from app.utils import get_public_notify_type_text
 from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id
 from app.dao.service_letter_contact_dao import dao_get_letter_contact_by_id
+from app.serialised_models import SerialisedTemplate
 
 from gds_metrics.metrics import Histogram
 
@@ -156,11 +157,9 @@ def check_notification_content_is_not_empty(template_with_content):
 
 
 def validate_template(template_id, personalisation, service, notification_type):
+
     try:
-        template = templates_dao.dao_get_template_by_id_and_service_id(
+        template = SerialisedTemplate.from_id_and_service_id(template_id, service.id)
-            template_id=template_id,
-            service_id=service.id
-        )
     except NoResultFound:
         message = 'Template not found'
         raise BadRequestError(message=message,

app/schemas.py

@@ -343,7 +343,9 @@ class BaseTemplateSchema(BaseSchema):
 
 class TemplateSchema(BaseTemplateSchema):
 
-    created_by = field_for(models.Template, 'created_by', required=True)
+    created_by_id = field_for(
+        models.Template, 'created_by_id', dump_to='created_by', dump_only=True
+    )
     process_type = field_for(models.Template, 'process_type')
     redact_personalisation = fields.Method("redact")
 
@@ -357,6 +359,9 @@ class TemplateSchema(BaseTemplateSchema):
             if not subject or subject.strip() == '':
                 raise ValidationError('Invalid template subject', 'subject')
 
+    class Meta(BaseTemplateSchema.Meta):
+        exclude = BaseTemplateSchema.Meta.exclude + ('created_by',)
+
 
 class TemplateHistorySchema(BaseSchema):
 

app/serialised_models.py

@@ -0,0 +1,57 @@
+from abc import ABC, abstractmethod
+
+from app.dao import templates_dao
+
+
+class SerialisedModel(ABC):
+
+    """
+    A SerialisedModel takes a dictionary, typically created by
+    serialising a database object. It then takes the value of specified
+    keys from the dictionary and adds them to itself as properties, so
+    that it can be interacted with like a normal database model object,
+    but with no risk that it will actually go back to the database.
+    """
+
+    @property
+    @abstractmethod
+    def ALLOWED_PROPERTIES(self):
+        pass
+
+    def __init__(self, _dict):
+        for property in self.ALLOWED_PROPERTIES:
+            setattr(self, property, _dict[property])
+
+    def __dir__(self):
+        return super().__dir__() + list(sorted(self.ALLOWED_PROPERTIES))
+
+
+class SerialisedTemplate(SerialisedModel):
+    ALLOWED_PROPERTIES = {
+        'archived',
+        'content',
+        'id',
+        'postage',
+        'process_type',
+        'reply_to_text',
+        'subject',
+        'template_type',
+        'version',
+    }
+
+    @classmethod
+    def from_id_and_service_id(cls, template_id, service_id):
+        return cls(cls.get_dict(template_id, service_id))
+
+    @staticmethod
+    def get_dict(template_id, service_id):
+        from app.schemas import template_schema
+
+        fetched_template = templates_dao.dao_get_template_by_id_and_service_id(
+            template_id=template_id,
+            service_id=service_id
+        )
+
+        template_dict = template_schema.dump(fetched_template).data
+
+        return template_dict

app/v2/notifications/post_notifications.py

@@ -98,14 +98,13 @@ 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,
+        service=authenticated_service,
         template=template,
         template_with_content=None,  # not required for precompiled
-        reply_to_text=reply_to,
+        reply_to_text='',  # not required for precompiled
         precompiled=True
     )
 
@@ -147,6 +146,7 @@ def post_notification(notification_type):
         notification = process_letter_notification(
             letter_data=form,
             api_key=api_user,
+            service=authenticated_service,
             template=template,
             template_with_content=template_with_content,
             reply_to_text=reply_to
@@ -156,7 +156,8 @@ def post_notification(notification_type):
             form=form,
             notification_type=notification_type,
             api_key=api_user,
-            template=template_with_content,
+            template=template,
+            template_with_content=template_with_content,
             template_process_type=template.process_type,
             service=authenticated_service,
             reply_to_text=reply_to
@@ -166,7 +167,15 @@ def post_notification(notification_type):
 
 
 def process_sms_or_email_notification(
-    *, form, notification_type, api_key, template, template_process_type, service, reply_to_text=None
+    *,
+    form,
+    notification_type,
+    api_key,
+    template,
+    template_with_content,
+    template_process_type,
+    service,
+    reply_to_text=None,
 ):
     notification_id = uuid.uuid4()
     form_send_to = form['email_address'] if notification_type == EMAIL_TYPE else form['phone_number']
@@ -186,19 +195,19 @@ def process_sms_or_email_notification(
     )
     if document_download_count:
         # We changed personalisation which means we need to update the content
-        template.values = personalisation
+        template_with_content.values = personalisation
     key_type = api_key.key_type
     service_in_research_mode = service.research_mode
     resp = create_response_for_post_notification(
         notification_id=notification_id,
         client_reference=form.get('reference', None),
         template_id=template.id,
-        template_version=template._template['version'],
+        template_version=template.version,
         service_id=service.id,
         notification_type=notification_type,
         reply_to=reply_to_text,
         scheduled_for=form.get("scheduled_for", None),
-        template_with_content=template)
+        template_with_content=template_with_content)
 
     if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE') and api_key.key_type == KEY_TYPE_NORMAL \
        and notification_type == EMAIL_TYPE:
@@ -229,7 +238,7 @@ def process_sms_or_email_notification(
     persist_notification(
         notification_id=notification_id,
         template_id=template.id,
-        template_version=template._template['version'],
+        template_version=template.version,
         recipient=form_send_to,
         service=service,
         personalisation=personalisation,
@@ -276,7 +285,7 @@ def save_email_to_queue(
     data = {
         "id": notification_id,
         "template_id": str(template.id),
-        "template_version": template._template['version'],
+        "template_version": template.version,
         "to": form['email_address'],
         "service_id": str(service_id),
         "personalisation": personalisation,
@@ -328,7 +337,7 @@ def process_document_uploads(personalisation_data, service, simulated=False):
 
 
 def process_letter_notification(
-    *, letter_data, api_key, template, template_with_content, reply_to_text, precompiled=False
+    *, letter_data, api_key, service, template, template_with_content, 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)
@@ -339,6 +348,7 @@ def process_letter_notification(
     if precompiled:
         return process_precompiled_letter_notifications(letter_data=letter_data,
                                                         api_key=api_key,
+                                                        service=service,
                                                         template=template,
                                                         reply_to_text=reply_to_text)
 
@@ -360,6 +370,7 @@ def process_letter_notification(
     queue = QueueNames.CREATE_LETTERS_PDF if not test_key else QueueNames.RESEARCH_MODE
 
     notification = create_letter_notification(letter_data=letter_data,
+                                              service=service,
                                               template=template,
                                               api_key=api_key,
                                               status=status,
@@ -414,7 +425,7 @@ def validate_address(api_key, letter_data):
         )
 
 
-def process_precompiled_letter_notifications(*, letter_data, api_key, template, reply_to_text):
+def process_precompiled_letter_notifications(*, letter_data, api_key, service, template, reply_to_text):
     try:
         status = NOTIFICATION_PENDING_VIRUS_CHECK
         letter_content = base64.b64decode(letter_data['content'])
@@ -422,6 +433,7 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, template,
         raise BadRequestError(message='Cannot decode letter content (invalid base64 encoding)', status_code=400)
 
     notification = create_letter_notification(letter_data=letter_data,
+                                              service=service,
                                               template=template,
                                               api_key=api_key,
                                               status=status,
@@ -460,7 +472,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.reply_to_text
 
     elif notification_type == SMS_TYPE:
         service_sms_sender_id = form.get("sms_sender_id", None)
@@ -470,10 +482,10 @@ 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.reply_to_text
 
     elif notification_type == LETTER_TYPE:
-        reply_to = template.get_reply_to_text()
+        reply_to = template.reply_to_text
 
     return reply_to
 

tests/app/notifications/test_process_letter_notifications.py

@@ -2,6 +2,7 @@ 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.serialised_models import SerialisedTemplate
 
 
 def test_create_letter_notification_creates_notification(sample_letter_template, sample_api_key):
@@ -13,7 +14,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 = SerialisedTemplate.from_id_and_service_id(
+        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 +49,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 = SerialisedTemplate.from_id_and_service_id(
+        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 +73,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 = SerialisedTemplate.from_id_and_service_id(
-                                              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

@@ -11,7 +11,6 @@ from app.models import (
     Notification,
     NotificationHistory,
     ScheduledNotification,
-    Template,
     LETTER_TYPE
 )
 from app.notifications.process_notifications import (
@@ -21,32 +20,41 @@ from app.notifications.process_notifications import (
     send_notification_to_queue,
     simulated_recipient
 )
+from app.serialised_models import SerialisedTemplate
 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, create_api_key
 
 
 def test_create_content_for_notification_passes(sample_email_template):
-    template = Template.query.get(sample_email_template.id)
+    template = SerialisedTemplate.from_id_and_service_id(
+        sample_email_template.id, sample_email_template.service_id
+    )
     content = create_content_for_notification(template, 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)
+    template = SerialisedTemplate.from_id_and_service_id(
+        sample_template_with_placeholders.id, sample_template_with_placeholders.service_id
+    )
     content = create_content_for_notification(template, {'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 = SerialisedTemplate.from_id_and_service_id(
+        sample_template_with_placeholders.id, sample_template_with_placeholders.service_id
+    )
     with pytest.raises(BadRequestError):
         create_content_for_notification(template, None)
 
 
 def test_create_content_for_notification_allows_additional_personalisation(sample_template_with_placeholders):
-    template = Template.query.get(sample_template_with_placeholders.id)
+    template = SerialisedTemplate.from_id_and_service_id(
+        sample_template_with_placeholders.id, sample_template_with_placeholders.service_id
+    )
     create_content_for_notification(template, {'name': 'Bobby', 'Additional placeholder': 'Data'})
 
 

tests/app/notifications/test_validators.py

@@ -23,6 +23,7 @@ from app.notifications.validators import (
     validate_and_format_recipient,
     validate_template,
 )
+from app.serialised_models import SerialisedTemplate
 from app.utils import get_template_instance
 
 from app.v2.errors import (
@@ -314,7 +315,7 @@ 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 = SerialisedTemplate.from_id_and_service_id(
         template_id=template_id,
         service_id=sample_service.id
     )
@@ -330,7 +331,7 @@ 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 = SerialisedTemplate.from_id_and_service_id(
         template_id=template_id,
         service_id=sample_service.id
     )
@@ -356,7 +357,7 @@ def test_validate_template_calls_all_validators(mocker, fake_uuid, sample_servic
     )
     mock_check_not_empty = mocker.patch('app.notifications.validators.check_notification_content_is_not_empty')
     mock_check_message_is_too_long = mocker.patch('app.notifications.validators.check_content_char_count')
-    validate_template(template.id, {}, sample_service, "email")
+    template, template_with_content = 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)