Merge pull request #2795 from alphagov/extract-placeholders-from-all-parts-of-letter-template

Look in all parts of a letter template to find placeholders

tests/app/celery/test_tasks.py

@@ -9,7 +9,11 @@ from freezegun import freeze_time
 from requests import RequestException
 from sqlalchemy.exc import SQLAlchemyError
 from celery.exceptions import Retry
-from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate
+from notifications_utils.template import (
+    LetterPrintTemplate,
+    PlainTextEmailTemplate,
+    SMSMessageTemplate,
+)
 from notifications_utils.columns import Row
 
 from app import (
@@ -26,11 +30,12 @@ from app.celery.tasks import (
     save_letter,
     process_incomplete_job,
     process_incomplete_jobs,
-    get_template_class,
     s3,
     send_inbound_sms_to_service,
     process_returned_letters_list,
-    save_api_email)
+    save_api_email,
+    get_recipient_csv_and_template_and_sender_id,
+)
 from app.config import QueueNames
 from app.dao import jobs_dao, service_email_reply_to_dao, service_sms_sender_dao
 from app.models import (
@@ -1298,13 +1303,81 @@ def test_should_cancel_job_if_service_is_inactive(sample_service,
     tasks.process_row.assert_not_called()
 
 
-@pytest.mark.parametrize('template_type, expected_class', [
-    (SMS_TYPE, SMSMessageTemplate),
-    (EMAIL_TYPE, WithSubjectTemplate),
-    (LETTER_TYPE, WithSubjectTemplate),
-])
-def test_get_template_class(template_type, expected_class):
-    assert get_template_class(template_type) == expected_class
+def test_get_email_template_instance(mocker, sample_email_template, sample_job):
+    mocker.patch(
+        'app.celery.tasks.s3.get_job_and_metadata_from_s3',
+        return_value=('', {}),
+    )
+    sample_job.template_id = sample_email_template.id
+    (
+        recipient_csv,
+        template,
+        _sender_id,
+    ) = get_recipient_csv_and_template_and_sender_id(sample_job)
+
+    assert isinstance(template, PlainTextEmailTemplate)
+    assert recipient_csv.placeholders == [
+        'email address'
+    ]
+
+
+def test_get_sms_template_instance(mocker, sample_template, sample_job):
+    mocker.patch(
+        'app.celery.tasks.s3.get_job_and_metadata_from_s3',
+        return_value=('', {}),
+    )
+    sample_job.template = sample_template
+    (
+        recipient_csv,
+        template,
+        _sender_id,
+    ) = get_recipient_csv_and_template_and_sender_id(sample_job)
+
+    assert isinstance(template, SMSMessageTemplate)
+    assert recipient_csv.placeholders == [
+        'phone number'
+    ]
+
+
+def test_get_letter_template_instance(mocker, sample_job):
+    mocker.patch(
+        'app.celery.tasks.s3.get_job_and_metadata_from_s3',
+        return_value=('', {}),
+    )
+    sample_contact_block = create_letter_contact(
+        service=sample_job.service,
+        contact_block='((reference number))'
+    )
+    sample_template = create_template(
+        service=sample_job.service,
+        template_type=LETTER_TYPE,
+        reply_to=sample_contact_block.id,
+    )
+    sample_job.template_id = sample_template.id
+
+    (
+        recipient_csv,
+        template,
+        _sender_id,
+    ) = get_recipient_csv_and_template_and_sender_id(sample_job)
+
+    assert isinstance(template, LetterPrintTemplate)
+    assert template.contact_block == (
+        '((reference number))'
+    )
+    assert template.placeholders == {
+        'reference number'
+    }
+    assert recipient_csv.placeholders == [
+        'reference number',
+        'address line 1',
+        'address line 2',
+        'address line 3',
+        'address line 4',
+        'address line 5',
+        'address line 6',
+        'postcode',
+    ]
 
 
 def test_send_inbound_sms_to_service_post_https_request_to_service(notify_api, sample_service):

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"
-    letter_contact = create_letter_contact(service=service, contact_block=service_address, is_default=True)
-    template = create_template(service=service, template_type='letter', reply_to=letter_contact.id)
+    create_letter_contact(service=service, contact_block='ignored', is_default=True)
+    template = create_template(service=service, template_type='letter')
     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)
-    auth_header = create_authorization_header(service_id=sample_service_custom_letter_contact_block.id)
+    template = create_template(sample_service, **create_template_args)
+    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)