From 8c8c8b63281d78117a3d99f166b4c093cc5fa631 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Apr 2020 12:50:22 +0100 Subject: [PATCH 1/2] Look in all parts of a letter template to find placeholders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Text messages have placeholders in their body. Emails have them in their subject line too. Letters have them in their body, subject line and contact block. We were only looking in the the body and subject when processing a job, therefore the thing assembling the letter was not looking in all the CSV columns it needed to, because it hadn’t been told about any placeholders in the contact block. Fixing this means always making sure we use the correct `Template` instance for the type of template we’re dealing with. Which we were already doing in a different part of the codebase. So it makes sense to reuse that. Turns out we fixed the same bug for email subjects over 3 years ago: https://github.com/alphagov/notifications-api/commit/3ed97151ee827ca8e304d82eac862a81a7d90466 --- app/celery/tasks.py | 16 +----- app/commands.py | 5 +- app/models.py | 10 ++-- tests/app/celery/test_tasks.py | 93 ++++++++++++++++++++++++++++++---- 4 files changed, 89 insertions(+), 35 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 02291d728..ce7f906e7 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -8,10 +8,6 @@ from notifications_utils.recipients import ( RecipientCSV ) from notifications_utils.statsd_decorators import statsd -from notifications_utils.template import ( - SMSMessageTemplate, - WithSubjectTemplate, -) from notifications_utils.timezones import convert_utc_to_bst from requests import ( HTTPError, @@ -128,9 +124,8 @@ def job_complete(job, resumed=False, start=None): def get_recipient_csv_and_template_and_sender_id(job): db_template = dao_get_template_by_id(job.template_id, job.template_version) + template = db_template._as_utils_template() - TemplateClass = get_template_class(db_template.template_type) - template = TemplateClass(db_template.__dict__) contents, meta_data = s3.get_job_and_metadata_from_s3(service_id=str(job.service_id), job_id=str(job.id)) recipient_csv = RecipientCSV(file_data=contents, template_type=template.template_type, @@ -454,15 +449,6 @@ def handle_exception(task, notification, notification_id, exc): current_app.logger.error('Max retry failed' + retry_msg) -def get_template_class(template_type): - if template_type == SMS_TYPE: - return SMSMessageTemplate - elif template_type in (EMAIL_TYPE, LETTER_TYPE): - # since we don't need rendering capabilities (we only need to extract placeholders) both email and letter can - # use the same base template - return WithSubjectTemplate - - @notify_celery.task(bind=True, name='update-letter-notifications-statuses') @statsd(namespace="tasks") def update_letter_notifications_statuses(self, filename): diff --git a/app/commands.py b/app/commands.py index 7b877f2c1..a28be6fa7 100644 --- a/app/commands.py +++ b/app/commands.py @@ -17,7 +17,7 @@ from notifications_utils.statsd_decorators import statsd from app import db, DATETIME_FORMAT, encryption from app.aws import s3 -from app.celery.tasks import record_daily_sorted_counts, get_template_class, process_row +from app.celery.tasks import record_daily_sorted_counts, process_row from app.celery.nightly_tasks import send_total_sent_notifications_to_performance_platform from app.celery.service_callback_tasks import send_delivery_status_to_service from app.celery.letters_pdf_tasks import create_letters_pdf @@ -889,8 +889,7 @@ def process_row_from_job(job_id, job_row_number): job = dao_get_job_by_id(job_id) db_template = dao_get_template_by_id(job.template_id, job.template_version) - TemplateClass = get_template_class(db_template.template_type) - template = TemplateClass(db_template.__dict__) + template = db_template._as_utils_template() for row in RecipientCSV( s3.get_job_from_s3(str(job.service_id), str(job.id)), diff --git a/app/models.py b/app/models.py index 64beaca52..2bed6639d 100644 --- a/app/models.py +++ b/app/models.py @@ -977,16 +977,12 @@ class TemplateBase(db.Model): def _as_utils_template(self): if self.template_type == EMAIL_TYPE: - return PlainTextEmailTemplate( - {'content': self.content, 'subject': self.subject} - ) + return PlainTextEmailTemplate(self.__dict__) if self.template_type == SMS_TYPE: - return SMSMessageTemplate( - {'content': self.content} - ) + return SMSMessageTemplate(self.__dict__) if self.template_type == LETTER_TYPE: return LetterPrintTemplate( - {'content': self.content, 'subject': self.subject}, + self.__dict__, contact_block=self.service.get_default_letter_contact(), ) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 3175f7f1e..d1a60cf75 100644 --- a/tests/app/celery/test_tasks.py +++ b/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): From 025ac3ea8934b24d988d2dc0a530ed7ced11d027 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Apr 2020 14:25:43 +0100 Subject: [PATCH 2/2] 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 | 7 +- app/notifications/process_notifications.py | 3 +- app/notifications/rest.py | 13 ++-- app/template/rest.py | 14 ++-- app/v2/notifications/post_notifications.py | 9 ++- app/v2/template/post_template.py | 6 +- .../test_process_notification.py | 2 +- .../test_post_letter_notifications.py | 64 +++++++++++++++++-- tests/app/v2/template/test_get_template.py | 64 ++++++++++++------- 9 files changed, 136 insertions(+), 46 deletions(-) diff --git a/app/models.py b/app/models.py index 2bed6639d..ad3262be8 100644 --- a/app/models.py +++ b/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), diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 651fad687..9c698e693 100644 --- a/app/notifications/process_notifications.py +++ b/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 diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 088f8479c..4d20a609f 100644 --- a/app/notifications/rest.py +++ b/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': - output.update({'subject': template.subject}) + if template.template_type == SMS_TYPE: + 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)) diff --git a/app/template/rest.py b/app/template/rest.py index 0db1e4786..aeae18922 100644 --- a/app/template/rest.py +++ b/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//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) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index f2454e727..c0c586dd1 100644 --- a/app/v2/notifications/post_notifications.py +++ b/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 ) diff --git a/app/v2/template/post_template.py b/app/v2/template/post_template.py index 23cc983a9..fd67460fe 100644 --- a/app/v2/template/post_template.py +++ b/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.__dict__, values=data.get('personalisation')) + template_object = template._as_utils_template_with_personalisation( + data.get('personalisation') + ) check_placeholders(template_object) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 629f040fa..9a1b42d71 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/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): diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 305506d9f..fc80ecef7 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/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): diff --git a/tests/app/v2/template/test_get_template.py b/tests/app/v2/template/test_get_template.py index dadf82983..a6628cb49 100644 --- a/tests/app/v2/template/test_get_template.py +++ b/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)