diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index d273ec28a..55da5d933 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -1,12 +1,10 @@ from datetime import datetime from flask import current_app -from notifications_utils.field import Field from notifications_utils.recipients import ( validate_and_format_phone_number ) -from notifications_utils.template import Template -from notifications_utils.renderers import HTMLEmail, PlainTextEmail, SMSMessage +from notifications_utils.template import HTMLEmailTemplate, PlainTextEmailTemplate, SMSMessageTemplate from app import clients, statsd_client, create_uuid from app.dao.notifications_dao import dao_update_notification @@ -23,10 +21,11 @@ def send_sms_to_provider(notification): provider = provider_to_use(SMS_TYPE, notification.id) if notification.status == 'created': template_model = dao_get_template_by_id(notification.template_id, notification.template_version) - template = Template( + template = SMSMessageTemplate( template_model.__dict__, - values={} if not notification.personalisation else notification.personalisation, - renderer=SMSMessage(prefix=service.name, sender=service.sms_sender) + values=notification.personalisation, + prefix=service.name, + sender=service.sms_sender ) if service.research_mode or notification.key_type == KEY_TYPE_TEST: send_sms_response.apply_async( @@ -36,11 +35,11 @@ def send_sms_to_provider(notification): else: provider.send_sms( to=validate_and_format_phone_number(notification.to), - content=template.rendered, + content=str(template), reference=str(notification.id), sender=service.sms_sender ) - notification.billable_units = template.sms_fragment_count + notification.billable_units = template.fragment_count notification.sent_at = datetime.utcnow() notification.sent_by = provider.get_name() @@ -60,16 +59,15 @@ def send_email_to_provider(notification): if notification.status == 'created': template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__ - html_email = Template( + html_email = HTMLEmailTemplate( template_dict, values=notification.personalisation, - renderer=get_html_email_renderer(service) + **get_html_email_options(service) ) - plain_text_email = Template( + plain_text_email = PlainTextEmailTemplate( template_dict, - values=notification.personalisation, - renderer=PlainTextEmail() + values=notification.personalisation ) if service.research_mode or notification.key_type == KEY_TYPE_TEST: @@ -84,9 +82,9 @@ def send_email_to_provider(notification): reference = provider.send_email( from_address, notification.to, - str(Field(plain_text_email.subject, notification.personalisation)), - body=plain_text_email.rendered, - html_body=html_email.rendered, + plain_text_email.subject, + body=str(plain_text_email), + html_body=str(html_email), reply_to_address=service.reply_to_email_address, ) @@ -117,7 +115,7 @@ def provider_to_use(notification_type, notification_id): return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) -def get_html_email_renderer(service): +def get_html_email_options(service): govuk_banner = service.branding != BRANDING_ORG if service.organisation: logo = '{}{}{}'.format( @@ -133,4 +131,4 @@ def get_html_email_renderer(service): else: branding = {} - return HTMLEmail(govuk_banner=govuk_banner, **branding) + return dict(govuk_banner=govuk_banner, **branding) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 06c4207eb..01a6591ac 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -1,8 +1,6 @@ from datetime import datetime from flask import current_app -from notifications_utils.renderers import PassThrough -from notifications_utils.template import Template from app import redis_store from app.celery import provider_tasks @@ -11,14 +9,11 @@ from app.dao.notifications_dao import dao_create_notification, dao_delete_notifi from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE from app.notifications.validators import check_sms_content_char_count from app.v2.errors import BadRequestError, SendNotificationToQueueError +from app.utils import get_template_instance def create_content_for_notification(template, personalisation): - template_object = Template( - template.__dict__, - personalisation, - renderer=PassThrough() - ) + template_object = get_template_instance(template.__dict__, personalisation) check_placeholders(template_object) if template_object.template_type == SMS_TYPE: diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 75ba2aca9..de2fae587 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -7,9 +7,6 @@ from flask import ( current_app, json ) -from notifications_utils.field import Field -from notifications_utils.renderers import PassThrough -from notifications_utils.template import Template from app import api_user, create_uuid, statsd_client from app.clients.email.aws_ses import get_aws_responses @@ -36,7 +33,7 @@ from app.schemas import ( day_schema ) from app.service.utils import service_allowed_to_send_to -from app.utils import pagination_links +from app.utils import pagination_links, get_template_instance notifications = Blueprint('notifications', __name__) @@ -255,13 +252,13 @@ def send_notification(notification_type): def get_notification_return_data(notification_id, notification, template): output = { - 'body': template.rendered, + 'body': str(template), 'template_version': notification['template_version'], 'notification': {'id': notification_id} } if template.template_type == 'email': - output.update({'subject': str(Field(template.subject, template.values))}) + output.update({'subject': template.subject}) return output @@ -288,11 +285,8 @@ def _service_allowed_to_send_to(notification, service): def create_template_object_for_notification(template, personalisation): - template_object = Template( - template.__dict__, - personalisation, - renderer=PassThrough() - ) + template_object = get_template_instance(template.__dict__, personalisation) + if template_object.missing_data: message = 'Missing personalisation: {}'.format(", ".join(template_object.missing_data)) errors = {'template': [message]} diff --git a/app/schemas.py b/app/schemas.py index 185519303..3a220ef94 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -15,7 +15,6 @@ from marshmallow import ( ) from marshmallow_sqlalchemy import field_for -from notifications_utils.field import Field from notifications_utils.recipients import ( validate_email_address, InvalidEmailError, @@ -24,11 +23,10 @@ from notifications_utils.recipients import ( validate_and_format_phone_number ) -from notifications_utils.renderers import PassThrough - from app import ma from app import models from app.dao.permissions_dao import permission_dao +from app.utils import get_template_instance def _validate_positive_number(value, msg="Not a positive integer"): @@ -390,19 +388,13 @@ class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): @post_dump def handle_template_merge(self, in_data): in_data['template'] = in_data.pop('template_history') - from notifications_utils.template import Template - template = Template( - in_data['template'], - in_data['personalisation'], - renderer=PassThrough() - ) - in_data['body'] = template.rendered - template_type = in_data['template']['template_type'] - if template_type == 'email': - in_data['subject'] = str(Field(template.subject, in_data['personalisation'])) + template = get_template_instance(in_data['template'], in_data['personalisation']) + in_data['body'] = str(template) + if in_data['template']['template_type'] == models.EMAIL_TYPE: + in_data['subject'] = template.subject in_data['content_char_count'] = None else: - in_data['content_char_count'] = len(in_data['body']) + in_data['content_char_count'] = template.content_count in_data.pop('personalisation', None) in_data['template'].pop('content', None) diff --git a/app/template/rest.py b/app/template/rest.py index 05654f87f..0f3fabbed 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -13,9 +13,7 @@ from app.dao.templates_dao import ( dao_get_all_templates_for_service, dao_get_template_versions ) -from notifications_utils.field import Field -from notifications_utils.template import Template -from notifications_utils.renderers import PassThrough +from notifications_utils.template import SMSMessageTemplate from app.dao.services_dao import dao_fetch_service_by_id from app.models import SMS_TYPE from app.schemas import (template_schema, template_history_schema) @@ -26,13 +24,16 @@ from app.errors import ( register_errors, InvalidRequest ) +from app.utils import get_template_instance register_errors(template) def _content_count_greater_than_limit(content, template_type): - template = Template({'content': content, 'template_type': template_type}) - return template_type == SMS_TYPE and template.content_count > current_app.config.get('SMS_CHAR_COUNT_LIMIT') + if template_type != SMS_TYPE: + return False + template = SMSMessageTemplate({'content': content, 'template_type': template_type}) + return template.content_count > current_app.config.get('SMS_CHAR_COUNT_LIMIT') @template.route('', methods=['POST']) @@ -93,7 +94,8 @@ 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 = Template(data, values=request.args.to_dict(), renderer=PassThrough()) + + template_object = get_template_instance(data, values=request.args.to_dict()) if template_object.missing_data: raise InvalidRequest( @@ -109,10 +111,7 @@ def preview_template_by_id_and_service_id(service_id, template_id): ]}, status_code=400 ) - data['subject'], data['content'] = ( - str(Field(template_object.subject, template_object.values)), - template_object.rendered - ) + data['subject'], data['content'] = template_object.subject, str(template_object) return jsonify(data) diff --git a/app/utils.py b/app/utils.py index ace301040..a556ccb6e 100644 --- a/app/utils.py +++ b/app/utils.py @@ -1,4 +1,6 @@ from flask import url_for +from app.models import SMS_TYPE, EMAIL_TYPE +from notifications_utils.template import SMSMessageTemplate, PlainTextEmailTemplate def pagination_links(pagination, endpoint, **kwargs): @@ -18,3 +20,9 @@ def url_with_token(data, url, config): token = generate_token(data, config['SECRET_KEY'], config['DANGEROUS_SALT']) base_url = config['ADMIN_BASE_URL'] + url return base_url + token + + +def get_template_instance(template, values): + return { + SMS_TYPE: SMSMessageTemplate, EMAIL_TYPE: PlainTextEmailTemplate + }[template['template_type']](template, values) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 1abc610d8..bef56e31a 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -42,7 +42,7 @@ def post_sms_notification(): send_notification_to_queue(notification, service.research_mode) sms_sender = service.sms_sender if service.sms_sender else current_app.config.get('FROM_NUMBER') resp = create_post_sms_response_from_notification(notification, - template_with_content.rendered, + str(template_with_content), sms_sender, request.url_root) return jsonify(resp), 201 @@ -70,7 +70,7 @@ def post_email_notification(): send_notification_to_queue(notification, service.research_mode) resp = create_post_email_response_from_notification(notification=notification, - content=template_with_content.rendered, + content=str(template_with_content), subject=template_with_content.subject, email_from=service.email_from, url_root=request.url_root) @@ -89,5 +89,6 @@ def __validate_template(form, service, notification_type): check_template_is_for_notification_type(notification_type, template.template_type) check_template_is_active(template) template_with_content = create_content_for_notification(template, form.get('personalisation', {})) - check_sms_content_char_count(template_with_content.content_count) + if template.template_type == SMS_TYPE: + check_sms_content_char_count(template_with_content.content_count) return template, template_with_content diff --git a/requirements.txt b/requirements.txt index d36b860b9..6e5e20724 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,6 +22,6 @@ Flask-Redis==0.1.0 git+https://github.com/alphagov/notifications-python-client.git@3.0.0#egg=notifications-python-client==3.0.0 -git+https://github.com/alphagov/notifications-utils.git@11.1.1#egg=notifications-utils==11.1.1 +git+https://github.com/alphagov/notifications-utils.git@12.0.0#egg=notifications-utils==12.0.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 77db3456e..7ab9f4c95 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -42,8 +42,6 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, get_financial_year) -from notifications_utils.template import get_sms_fragment_count - from tests.app.conftest import (sample_notification, sample_template, sample_email_template, sample_service, sample_job, sample_api_key) @@ -821,22 +819,6 @@ def test_should_limit_notifications_return_by_day_limit_plus_one(notify_db, noti assert len(all_notifications) == 2 -@pytest.mark.parametrize( - "char_count, expected_sms_fragment_count", - [ - (159, 1), - (160, 1), - (161, 2), - (306, 2), - (307, 3), - (459, 3), - (460, 4), - (461, 4) - ]) -def test_sms_fragment_count(char_count, expected_sms_fragment_count): - assert get_sms_fragment_count(char_count) == expected_sms_fragment_count - - def test_creating_notification_adds_to_notification_history(sample_template): data = _notification_json(sample_template) notification = Notification(**data) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 17b16e063..dfddbb49b 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -355,11 +355,11 @@ def test_send_email_should_use_service_reply_to_email( def test_get_html_email_renderer_should_return_for_normal_service(sample_service): - renderer = send_to_providers.get_html_email_renderer(sample_service) - assert renderer.govuk_banner - assert renderer.brand_colour is None - assert renderer.brand_logo is None - assert renderer.brand_name is None + options = send_to_providers.get_html_email_options(sample_service) + assert options['govuk_banner'] + assert 'brand_colour' not in options.keys() + assert 'brand_logo' not in options.keys() + assert 'brand_name' not in options.keys() @pytest.mark.parametrize('branding_type, govuk_banner', [ @@ -373,11 +373,11 @@ def test_get_html_email_renderer_with_branding_details(branding_type, govuk_bann notify_db.session.add_all([sample_service, org]) notify_db.session.commit() - renderer = send_to_providers.get_html_email_renderer(sample_service) + options = send_to_providers.get_html_email_options(sample_service) - assert renderer.govuk_banner == govuk_banner - assert renderer.brand_colour == '000000' - assert renderer.brand_name == 'Justice League' + assert options['govuk_banner'] == govuk_banner + assert options['brand_colour'] == '#000000' + assert options['brand_name'] == 'Justice League' def test_get_html_email_renderer_prepends_logo_path(notify_db, sample_service): @@ -387,9 +387,9 @@ def test_get_html_email_renderer_prepends_logo_path(notify_db, sample_service): notify_db.session.add_all([sample_service, org]) notify_db.session.commit() - renderer = send_to_providers.get_html_email_renderer(sample_service) + renderer = send_to_providers.get_html_email_options(sample_service) - assert renderer.brand_logo == 'http://localhost:6012/static/images/email-template/crests/justice-league.png' + assert renderer['brand_logo'] == 'http://localhost:6012/static/images/email-template/crests/justice-league.png' def test_should_not_set_billable_units_if_research_mode(notify_db, sample_service, sample_notification, mocker): diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index d0b06b3af..5cc2e5e6a 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -129,7 +129,7 @@ def test_send_notification_with_placeholders_replaced(notify_api, sample_email_t queue="send-email" ) assert response.status_code == 201 - assert response_data['body'] == 'Hello Jo\nThis is an email from GOV.UK' + assert response_data['body'] == u'Hello Jo\nThis is an email from GOV.\u200BUK' assert response_data['subject'] == 'Jo' diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 7190e0fac..b43379848 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -18,14 +18,14 @@ from tests.app.conftest import sample_notification, sample_template, sample_emai 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 content.rendered == template.content + assert str(content) == template.content def test_create_content_for_notification_with_placeholders_passes(sample_template_with_placeholders): template = Template.query.get(sample_template_with_placeholders.id) content = create_content_for_notification(template, {'name': 'Bobby'}) assert content.content == template.content - assert 'Bobby' in content.rendered + assert 'Bobby' in str(content) def test_create_content_for_notification_fails_with_missing_personalisation(sample_template_with_placeholders): diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index fc49b1d6e..eff7aee43 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -711,7 +711,7 @@ def test_get_notification_by_id_returns_merged_template_content_for_email( notification = json.loads(response.get_data(as_text=True))['data']['notification'] assert response.status_code == 200 - assert notification['body'] == 'Hello world\nThis is an email from GOV.UK' + assert notification['body'] == 'Hello world\nThis is an email from GOV.\u200BUK' assert notification['subject'] == 'world' assert notification['content_char_count'] is None diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 906fd2fd9..232fdd983 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -129,8 +129,10 @@ def test_post_email_notification_returns_201(client, sample_email_template_with_ assert resp_json['id'] == str(notification.id) assert resp_json['reference'] == reference assert notification.reference is None - assert resp_json['content']['body'] == sample_email_template_with_placeholders.content.replace('((name))', 'Bob') - assert resp_json['content']['subject'] == sample_email_template_with_placeholders.subject + assert resp_json['content']['body'] == sample_email_template_with_placeholders.content\ + .replace('((name))', 'Bob').replace('GOV.UK', u'GOV.\u200bUK') + assert resp_json['content']['subject'] == sample_email_template_with_placeholders.subject\ + .replace('((name))', 'Bob') assert resp_json['content']['from_email'] == sample_email_template_with_placeholders.service.email_from assert 'v2/notifications/{}'.format(notification.id) in resp_json['uri'] assert resp_json['template']['id'] == str(sample_email_template_with_placeholders.id)