Use same template to validate and send notification

To be absolutely sure that we can send a message we should also validate
it using the same template class that we use to render it.

app/models.py

@@ -983,9 +983,14 @@ class TemplateBase(db.Model):
         if self.template_type == LETTER_TYPE:
             return LetterPrintTemplate(
                 self.__dict__,
-                contact_block=self.service.get_default_letter_contact(),
+                contact_block=self.get_reply_to_text(),
             )
 
+    def _as_utils_template_with_personalisation(self, values):
+        template = self._as_utils_template()
+        template.values = values
+        return template
+
     def serialize(self):
         serialized = {
             "id": str(self.id),

app/notifications/process_notifications.py

@@ -32,11 +32,10 @@ from app.dao.notifications_dao import (
 )
 
 from app.v2.errors import BadRequestError
-from app.utils import get_template_instance
 
 
 def create_content_for_notification(template, personalisation):
-    template_object = get_template_instance(template.__dict__, personalisation)
+    template_object = template._as_utils_template_with_personalisation(personalisation)
     check_placeholders(template_object)
 
     return template_object

app/notifications/rest.py

@@ -4,6 +4,7 @@ from flask import (
     request,
     current_app
 )
+from notifications_utils.template import WithSubjectTemplate
 
 from app import api_user, authenticated_service
 from app.config import QueueNames
@@ -35,7 +36,7 @@ from app.schemas import (
     notifications_filter_schema
 )
 from app.service.utils import service_allowed_to_send_to
-from app.utils import pagination_links, get_template_instance, get_public_notify_type_text
+from app.utils import pagination_links, get_public_notify_type_text
 
 from notifications_utils import SMS_CHAR_COUNT_LIMIT
 from notifications_utils.recipients import get_international_phone_info
@@ -149,13 +150,15 @@ def send_notification(notification_type):
 
 def get_notification_return_data(notification_id, notification, template):
     output = {
-        'body': str(template),
         'template_version': notification['template_version'],
         'notification': {'id': notification_id}
     }
 
-    if template.template_type == 'email':
+    if template.template_type == SMS_TYPE:
-        output.update({'subject': template.subject})
+        output['body'] = str(template)
+    else:
+        output['body'] = WithSubjectTemplate.__str__(template)
+        output['subject'] = template.subject
 
     return output
 
@@ -187,7 +190,7 @@ def _service_allowed_to_send_to(notification, service):
 
 
 def create_template_object_for_notification(template, personalisation):
-    template_object = get_template_instance(template.__dict__, personalisation)
+    template_object = template._as_utils_template_with_personalisation(personalisation)
 
     if template_object.missing_data:
         message = 'Missing personalisation: {}'.format(", ".join(template_object.missing_data))

app/template/rest.py

@@ -10,7 +10,7 @@ from flask import (
     request)
 from notifications_utils import SMS_CHAR_COUNT_LIMIT
 from notifications_utils.pdf import extract_page_from_pdf
-from notifications_utils.template import SMSMessageTemplate
+from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate
 from requests import post as requests_post
 from sqlalchemy.orm.exc import NoResultFound
 
@@ -38,7 +38,7 @@ from app.notifications.validators import service_has_permission, check_reply_to
 from app.schema_validation import validate
 from app.schemas import (template_schema, template_history_schema)
 from app.template.template_schemas import post_create_template_schema
-from app.utils import get_template_instance, get_public_notify_type_text
+from app.utils import get_public_notify_type_text
 
 template_blueprint = Blueprint('template', __name__, url_prefix='/service/<uuid:service_id>/template')
 
@@ -166,7 +166,9 @@ def get_template_by_id_and_service_id(service_id, template_id):
 def preview_template_by_id_and_service_id(service_id, template_id):
     fetched_template = dao_get_template_by_id_and_service_id(template_id=template_id, service_id=service_id)
     data = template_schema.dump(fetched_template).data
-    template_object = get_template_instance(data, values=request.args.to_dict())
+    template_object = fetched_template._as_utils_template_with_personalisation(
+        request.args.to_dict()
+    )
 
     if template_object.missing_data:
         raise InvalidRequest(
@@ -175,7 +177,11 @@ def preview_template_by_id_and_service_id(service_id, template_id):
             ]}, status_code=400
         )
 
-    data['subject'], data['content'] = template_object.subject, str(template_object)
+    data['subject'] = template_object.subject
+    if template_object.template_type == SMS_TYPE:
+        data['content'] = str(template_object)
+    else:
+        data['content'] = WithSubjectTemplate.__str__(template_object)
 
     return jsonify(data)
 

app/v2/notifications/post_notifications.py

@@ -8,6 +8,7 @@ from flask import request, jsonify, current_app, abort
 from notifications_utils.recipients import (
     format_postcode_for_printing, is_a_real_uk_postcode, try_validate_and_format_phone_number
 )
+from notifications_utils.template import WithSubjectTemplate
 
 from app import (
     api_user,
@@ -166,23 +167,25 @@ def post_notification(notification_type):
     if notification_type == SMS_TYPE:
         create_resp_partial = functools.partial(
             create_post_sms_response_from_notification,
-            from_number=reply_to
+            from_number=reply_to,
+            content=str(template_with_content),
         )
     elif notification_type == EMAIL_TYPE:
         create_resp_partial = functools.partial(
             create_post_email_response_from_notification,
             subject=template_with_content.subject,
-            email_from='{}@{}'.format(authenticated_service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN'])
+            email_from='{}@{}'.format(authenticated_service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']),
+            content=WithSubjectTemplate.__str__(template_with_content),
         )
     elif notification_type == LETTER_TYPE:
         create_resp_partial = functools.partial(
             create_post_letter_response_from_notification,
             subject=template_with_content.subject,
+            content=WithSubjectTemplate.__str__(template_with_content),
         )
 
     resp = create_resp_partial(
         notification=notification,
-        content=str(template_with_content),
         url_root=request.url_root,
         scheduled_for=scheduled_for
     )

app/v2/template/post_template.py

@@ -3,7 +3,6 @@ from flask import jsonify, request
 from app import authenticated_service
 from app.dao import templates_dao
 from app.schema_validation import validate
-from app.utils import get_template_instance
 from app.v2.errors import BadRequestError
 from app.v2.template import v2_template_blueprint
 from app.v2.template.template_schemas import (
@@ -29,8 +28,9 @@ def post_template_preview(template_id):
     template = templates_dao.dao_get_template_by_id_and_service_id(
         template_id, authenticated_service.id)
 
-    template_object = get_template_instance(
+    template_object = template._as_utils_template_with_personalisation(
-        template.__dict__, values=data.get('personalisation'))
+        data.get('personalisation')
+    )
 
     check_placeholders(template_object)
 

tests/app/notifications/test_process_notification.py

@@ -29,7 +29,7 @@ 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)
-    assert str(content) == template.content
+    assert str(content) == template.content + '\n'
 
 
 def test_create_content_for_notification_with_placeholders_passes(sample_template_with_placeholders):

tests/app/v2/notifications/test_post_letter_notifications.py

@@ -274,6 +274,41 @@ def test_post_letter_notification_returns_400_for_empty_personalisation(
     }
 
 
+def test_post_notification_returns_400_for_missing_letter_contact_block_personalisation(
+    client,
+    sample_service,
+):
+    letter_contact_block = create_letter_contact(
+        service=sample_service, contact_block='((contact block))', is_default=True
+    )
+    template = create_template(
+        service=sample_service,
+        template_type='letter',
+        reply_to=letter_contact_block.id,
+    )
+    data = {
+        'template_id': str(template.id),
+        'personalisation': {
+            'address_line_1': 'Line 1',
+            'address_line_2': 'Line 2',
+            'postcode': 'SW1A 1AA',
+        },
+    }
+
+    error_json = letter_request(
+        client,
+        data,
+        service_id=sample_service.id,
+        _expected_status=400,
+    )
+
+    assert error_json['status_code'] == 400
+    assert error_json['errors'] == [{
+        'error': 'BadRequestError',
+        'message': 'Missing personalisation: contact block'
+    }]
+
+
 def test_notification_returns_400_for_missing_template_field(
     client,
     sample_service_full_permissions
@@ -450,15 +485,14 @@ def test_post_letter_notification_is_delivered_and_has_pdf_uploaded_to_test_lett
     s3mock.assert_called_once_with(ANY, b'letter-content', precompiled=True)
 
 
-def test_post_letter_notification_persists_notification_reply_to_text(
+def test_post_letter_notification_ignores_reply_to_text_for_service(
     client, notify_db_session, mocker
 ):
     mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async')
 
     service = create_service(service_permissions=[LETTER_TYPE])
-    service_address = "12 Main Street, London"
+    create_letter_contact(service=service, contact_block='ignored', is_default=True)
-    letter_contact = create_letter_contact(service=service, contact_block=service_address, is_default=True)
+    template = create_template(service=service, template_type='letter')
-    template = create_template(service=service, template_type='letter', reply_to=letter_contact.id)
     data = {
         "template_id": template.id,
         "personalisation": {'address_line_1': 'Foo', 'address_line_2': 'Bar', 'postcode': 'BA5 5AB'}
@@ -467,7 +501,27 @@ def test_post_letter_notification_persists_notification_reply_to_text(
 
     notifications = Notification.query.all()
     assert len(notifications) == 1
-    assert notifications[0].reply_to_text == service_address
+    assert notifications[0].reply_to_text is None
+
+
+def test_post_letter_notification_persists_notification_reply_to_text_for_template(
+    client, notify_db_session, mocker
+):
+    mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async')
+
+    service = create_service(service_permissions=[LETTER_TYPE])
+    create_letter_contact(service=service, contact_block='the default', is_default=True)
+    template_letter_contact = create_letter_contact(service=service, contact_block='not the default', is_default=False)
+    template = create_template(service=service, template_type='letter', reply_to=template_letter_contact.id)
+    data = {
+        "template_id": template.id,
+        "personalisation": {'address_line_1': 'Foo', 'address_line_2': 'Bar', 'postcode': 'BA5 5AB'}
+    }
+    letter_request(client, data=data, service_id=service.id, key_type=KEY_TYPE_NORMAL)
+
+    notifications = Notification.query.all()
+    assert len(notifications) == 1
+    assert notifications[0].reply_to_text == 'not the default'
 
 
 def test_post_precompiled_letter_with_invalid_base64(client, notify_user, mocker):

tests/app/v2/template/test_get_template.py

@@ -5,7 +5,7 @@ from flask import json
 from app import DATETIME_FORMAT
 from app.models import TEMPLATE_TYPES, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE
 from tests import create_authorization_header
-from tests.app.db import create_template
+from tests.app.db import create_template, create_letter_contact
 
 valid_version_params = [None, 1]
 
@@ -79,35 +79,17 @@ def test_get_template_by_id_returns_200(
             },
         },
     ),
-    (
-        {
-            "template_type": LETTER_TYPE,
-            "subject": "((letterSubject))",
-            "content": "((letter_content))",
-        },
-        {
-            "letterSubject": {
-                "required": True,
-            },
-            "letter_content": {
-                "required": True,
-            },
-            "contact block": {
-                "required": True,
-            },
-        },
-    )
 ])
 @pytest.mark.parametrize("version", valid_version_params)
 def test_get_template_by_id_returns_placeholders(
     client,
-    sample_service_custom_letter_contact_block,
+    sample_service,
     version,
     create_template_args,
     expected_personalisation,
 ):
-    template = create_template(sample_service_custom_letter_contact_block, **create_template_args)
+    template = create_template(sample_service, **create_template_args)
-    auth_header = create_authorization_header(service_id=sample_service_custom_letter_contact_block.id)
+    auth_header = create_authorization_header(service_id=sample_service.id)
 
     version_path = '/version/{}'.format(version) if version else ''
 
@@ -118,6 +100,44 @@ def test_get_template_by_id_returns_placeholders(
     assert json_response['personalisation'] == expected_personalisation
 
 
+@pytest.mark.parametrize("version", valid_version_params)
+def test_get_letter_template_by_id_returns_placeholders(
+    client,
+    sample_service,
+    version,
+):
+    contact_block = create_letter_contact(
+        service=sample_service,
+        contact_block='((contact block))',
+    )
+    template = create_template(
+        sample_service,
+        template_type=LETTER_TYPE,
+        subject="((letterSubject))",
+        content="((letter_content))",
+        reply_to=contact_block.id,
+    )
+    auth_header = create_authorization_header(service_id=sample_service.id)
+
+    version_path = '/version/{}'.format(version) if version else ''
+
+    response = client.get(path='/v2/template/{}{}'.format(template.id, version_path),
+                          headers=[('Content-Type', 'application/json'), auth_header])
+
+    json_response = json.loads(response.get_data(as_text=True))
+    assert json_response['personalisation'] == {
+        "letterSubject": {
+            "required": True,
+        },
+        "letter_content": {
+            "required": True,
+        },
+        "contact block": {
+            "required": True,
+        },
+    }
+
+
 def test_get_template_with_non_existent_template_id_returns_404(client, fake_uuid, sample_service):
     auth_header = create_authorization_header(service_id=sample_service.id)