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..ad3262be8 100644 --- a/app/models.py +++ b/app/models.py @@ -977,19 +977,20 @@ 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}, - contact_block=self.service.get_default_letter_contact(), + self.__dict__, + 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/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): 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)