diff --git a/app/__init__.py b/app/__init__.py index 6697fb7c4..db4a51d31 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -131,6 +131,7 @@ def create_app(): application.add_template_filter(format_notification_status_as_time) application.add_template_filter(format_notification_status_as_field_status) application.add_template_filter(format_notification_status_as_url) + application.add_template_filter(formatted_list) application.after_request(useful_headers_after_request) application.after_request(save_service_after_request) @@ -345,6 +346,33 @@ def format_notification_status_as_url(status): }.get(status) +def formatted_list( + items, + conjunction='and', + before_each='‘', + after_each='’', + separator=', ', + prefix='', + prefix_plural='' +): + if prefix: + prefix += ' ' + if prefix_plural: + prefix_plural += ' ' + + items = list(items) + if len(items) == 1: + return '{prefix}{before_each}{items[0]}{after_each}'.format(**locals()) + elif items: + formatted_items = ['{}{}{}'.format(before_each, item, after_each) for item in items] + + first_items = separator.join(formatted_items[:-1]) + last_item = formatted_items[-1] + return ( + '{prefix_plural}{first_items} {conjunction} {last_item}' + ).format(**locals()) + + @login_manager.user_loader def load_user(user_id): return user_api_client.get_user(user_id) diff --git a/app/main/forms.py b/app/main/forms.py index 5b736a57b..e3a8a97a4 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1,3 +1,5 @@ +import re + import pytz from flask_login import current_user from flask_wtf import Form @@ -23,7 +25,7 @@ from wtforms import ( from wtforms.fields.html5 import EmailField, TelField from wtforms.validators import (DataRequired, Email, Length, Regexp, Optional) -from app.main.validators import (Blacklist, CsvFileValidator, ValidGovEmail, NoCommasInPlaceHolders) +from app.main.validators import (Blacklist, CsvFileValidator, ValidGovEmail, NoCommasInPlaceHolders, OnlyGSMCharacters) def get_time_value_and_label(future_time): @@ -251,7 +253,7 @@ class ConfirmPasswordForm(Form): raise ValidationError('Invalid password') -class SMSTemplateForm(Form): +class BaseTemplateForm(Form): name = StringField( u'Template name', validators=[DataRequired(message="Can’t be empty")]) @@ -274,7 +276,12 @@ class SMSTemplateForm(Form): ) -class EmailTemplateForm(SMSTemplateForm): +class SMSTemplateForm(BaseTemplateForm): + def validate_template_content(self, field): + OnlyGSMCharacters()(None, field) + + +class EmailTemplateForm(BaseTemplateForm): subject = TextAreaField( u'Subject', validators=[DataRequired(message="Can’t be empty")]) @@ -479,7 +486,6 @@ class ServiceSmsSender(Form): ) def validate_sms_sender(form, field): - import re if field.data and not re.match('^[a-zA-Z0-9\s]+$', field.data): raise ValidationError('Use letters and numbers only') diff --git a/app/main/validators.py b/app/main/validators.py index d07d7cab7..42cd29c8d 100644 --- a/app/main/validators.py +++ b/app/main/validators.py @@ -1,13 +1,16 @@ from wtforms import ValidationError from notifications_utils.template import Template +from notifications_utils.gsm import get_non_gsm_compatible_characters + +from app import formatted_list +from app.main._blacklisted_passwords import blacklisted_passwords from app.utils import ( Spreadsheet, is_gov_user ) -from ._blacklisted_passwords import blacklisted_passwords -class Blacklist(object): +class Blacklist: def __init__(self, message=None): if not message: message = 'Password is blacklisted.' @@ -18,7 +21,7 @@ class Blacklist(object): raise ValidationError(self.message) -class CsvFileValidator(object): +class CsvFileValidator: def __init__(self, message='Not a csv file'): self.message = message @@ -28,7 +31,7 @@ class CsvFileValidator(object): raise ValidationError("{} isn’t a spreadsheet that Notify can read".format(field.data.filename)) -class ValidGovEmail(object): +class ValidGovEmail: def __call__(self, form, field): from flask import url_for @@ -40,7 +43,7 @@ class ValidGovEmail(object): raise ValidationError(message) -class NoCommasInPlaceHolders(): +class NoCommasInPlaceHolders: def __init__(self, message='You can’t have commas in your fields'): self.message = message @@ -48,3 +51,15 @@ class NoCommasInPlaceHolders(): def __call__(self, form, field): if ',' in ''.join(Template({'content': field.data}).placeholders): raise ValidationError(self.message) + + +class OnlyGSMCharacters: + def __call__(self, form, field): + non_gsm_characters = sorted(list(get_non_gsm_compatible_characters(field.data))) + if non_gsm_characters: + raise ValidationError( + 'You can’t use {} in text messages. {} won’t show up properly on everyone’s phones.'.format( + formatted_list(non_gsm_characters, conjunction='or', before_each='', after_each=''), + ('It' if len(non_gsm_characters) == 1 else 'They') + ) + ) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index a578168a5..cd6699d3b 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -163,11 +163,12 @@ def add_service_template(service_id, template_type): form.process_type.data ) except HTTPError as e: - if e.status_code == 400: - if 'content' in e.message and any(['character count greater than' in x for x in e.message['content']]): - form.template_content.errors.extend(e.message['content']) - else: - raise e + if ( + e.status_code == 400 and + 'content' in e.message and + any(['character count greater than' in x for x in e.message['content']]) + ): + form.template_content.errors.extend(e.message['content']) else: raise e else: diff --git a/app/templates/components/list.html b/app/templates/components/list.html index 26eb8e6ee..ec74264ca 100644 --- a/app/templates/components/list.html +++ b/app/templates/components/list.html @@ -1,35 +1,5 @@ -{% macro formatted_list( - items, - conjunction='and', - before_each='‘', - after_each='’', - separator=', ', - prefix='', - prefix_plural='' -) %} - {% if items|length == 1 %} - {{ prefix }} {{ before_each|safe }}{{ (items|list)[0] }}{{ after_each|safe }} - {% elif items %} - {{ prefix_plural }} - {% for item in (items|list)[0:-1] -%} - {{ before_each|safe -}} - {{ item -}} - {{ after_each|safe -}} - {% if not loop.last -%} - {{ separator -}} - {% endif -%} - {% endfor %} - {{ conjunction }} - {{ before_each|safe -}} - {{ (items|list)[-1] -}} - {{ after_each|safe }} - {%- endif %} -{%- endmacro %} - - {% macro list_of_placeholders(placeholders) %} - {{ formatted_list( - placeholders, + {{ placeholders | formatted_list( before_each="((", after_each='))', separator=' ' diff --git a/app/templates/views/check.html b/app/templates/views/check.html index e0f891f7d..b8880fe30 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -4,7 +4,6 @@ {% from "components/table.html" import list_table, field, text_field, index_field, hidden_field_heading %} {% from "components/file-upload.html" import file_upload %} {% from "components/page-footer.html" import page_footer %} -{% from "components/list.html" import formatted_list %} {% from "components/message-count-label.html" import message_count_label %} {% set file_contents_header_id = 'file-preview' %} @@ -41,17 +40,15 @@
{% call banner_wrapper(type='dangerous') %}

- Your file needs to have {{ formatted_list( - recipients.recipient_column_headers, + Your file needs to have {{ recipients.recipient_column_headers | formatted_list( prefix='a column called', prefix_plural='columns called' ) }}

- Your file has {{ formatted_list( - recipients.column_headers, - prefix='one column, called', - prefix_plural='columns called' + Your file has {{ recipients.column_headers | formatted_list( + prefix='one column, called ', + prefix_plural='columns called ' ) }}.

{{ skip_to_file_contents() }} @@ -67,18 +64,16 @@ your template

- Your file has {{ formatted_list( - recipients.column_headers, - prefix='one column, called', - prefix_plural='columns called' + Your file has {{ recipients.column_headers | formatted_list( + prefix='one column, called ', + prefix_plural='columns called ' ) }}.

- It doesn’t have {{ formatted_list( - recipients.missing_column_headers, + It doesn’t have {{ recipients.column_headers | formatted_list( conjunction='or', - prefix='a column called', - prefix_plural='columns called' + prefix='a column called ', + prefix_plural='columns called ' ) }}.

{{ skip_to_file_contents() }} diff --git a/app/templates/views/styleguide.html b/app/templates/views/styleguide.html index bcb4a34bc..10e3070fa 100644 --- a/app/templates/views/styleguide.html +++ b/app/templates/views/styleguide.html @@ -8,7 +8,6 @@ {% from "components/textbox.html" import textbox %} {% from "components/file-upload.html" import file_upload %} {% from "components/api-key.html" import api_key %} -{% from "components/list.html" import formatted_list %} {% block per_page_title %} Styleguide @@ -196,19 +195,19 @@

Formatted list

- {{ formatted_list('A', prefix="one item called") }} + {{ 'A' | formatted_list(prefix="one item called") }}

- {{ formatted_list('AB', prefix_plural="two items called") }} + {{ 'AB' | formatted_list(prefix_plural="two items called") }}

- {{ formatted_list('ABC') }} + {{ 'ABC' | formatted_list }}

- {{ formatted_list('ABCD', before_each='', after_each='', conjunction='or') }} + {{ 'ABCD' | formatted_list(before_each='', after_each='', conjunction='or') }}

{% endblock %} diff --git a/requirements.txt b/requirements.txt index 22dd3c13e..5637048f6 100644 --- a/requirements.txt +++ b/requirements.txt @@ -33,4 +33,4 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@13.7.0#egg=notifications-utils==13.7.0 +git+https://github.com/alphagov/notifications-utils.git@13.8.0#egg=notifications-utils==13.8.0 diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index 9d4917507..93b007337 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -1,6 +1,6 @@ import pytest from app.main.forms import RegisterUserForm, ServiceSmsSender -from app.main.validators import ValidGovEmail, NoCommasInPlaceHolders +from app.main.validators import ValidGovEmail, NoCommasInPlaceHolders, OnlyGSMCharacters from wtforms import ValidationError from unittest.mock import Mock @@ -141,6 +141,34 @@ def test_for_commas_in_placeholders( NoCommasInPlaceHolders()(None, _gen_mock_field('Hello ((name))')) +@pytest.mark.parametrize('msg', ['The quick brown fox', 'Thé “quick” bröwn fox\u200B']) +def test_gsm_character_validation(client, msg): + OnlyGSMCharacters()(None, _gen_mock_field(msg)) + + +@pytest.mark.parametrize('data, err_msg', [ + ( + '∆ abc 📲 def 📵 ghi', + ( + 'You can’t use ∆, 📲 or 📵 in text messages. ' + 'They won’t show up properly on everyone’s phones.' + ) + ), + ( + '📵', + ( + 'You can’t use 📵 in text messages. ' + 'It won’t show up properly on everyone’s phones.' + ) + ), +]) +def test_non_gsm_character_validation(data, err_msg, client): + with pytest.raises(ValidationError) as error: + OnlyGSMCharacters()(None, _gen_mock_field(data)) + + assert str(error.value) == err_msg + + def test_sms_sender_form_validation( client, mock_get_user_by_email, diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index aa15e83f5..ab336482f 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -1,6 +1,6 @@ from itertools import repeat from datetime import datetime -from unittest.mock import Mock, patch +from unittest.mock import Mock, patch, ANY import pytest from bs4 import BeautifulSoup @@ -32,6 +32,30 @@ def test_should_show_page_for_one_template( mock_get_service_template.assert_called_with(service_one['id'], template_id) +def test_should_show_sms_template_with_downgraded_unicode_characters( + logged_in_client, + mocker, + service_one, + fake_uuid, +): + msg = 'here:\tare some “fancy quotes” and zero\u200Bwidth\u200Bspaces' + rendered_msg = 'here: are some "fancy quotes" and zerowidthspaces' + + mocker.patch( + 'app.service_api_client.get_service_template', + return_value={'data': template_json(service_one['id'], fake_uuid, type_='sms', content=msg)} + ) + + template_id = fake_uuid + response = logged_in_client.get(url_for( + '.view_template', + service_id=service_one['id'], + template_id=template_id)) + + assert response.status_code == 200 + assert rendered_msg in response.get_data(as_text=True) + + def test_should_show_page_template_with_priority_select_if_platform_admin( logged_in_platform_admin_client, platform_admin_user, @@ -69,17 +93,12 @@ def test_should_show_preview_letter_templates( view_suffix, expected_content_type, logged_in_client, - api_user_active, - mock_login, - mock_get_service, mock_get_service_email_template, - mock_get_user, mock_get_user_by_email, - mock_has_permissions, - fake_uuid, - mocker, + service_one, + fake_uuid ): - service_id, template_id = repeat(fake_uuid, 2) + service_id, template_id = service_one['id'], fake_uuid response = logged_in_client.get(url_for( '{}_{}'.format(view, view_suffix), service_id=service_id, @@ -609,3 +628,102 @@ def test_get_last_use_message_uses_most_recent_statistics(): ]) def test_get_human_readable_delta(from_time, until_time, message): assert get_human_readable_delta(from_time, until_time) == message + + +def test_can_create_email_template_with_emoji( + logged_in_client, + service_one, + mock_create_service_template +): + data = { + 'name': "new name", + 'subject': "Food incoming!", + 'template_content': "here's a burrito 🌯", + 'template_type': 'email', + 'service': service_one['id'], + 'process_type': 'normal' + } + resp = logged_in_client.post(url_for( + '.add_service_template', + service_id=service_one['id'], + template_type='email' + ), data=data) + + assert resp.status_code == 302 + + +def test_should_not_create_sms_template_with_emoji( + logged_in_client, + service_one, + mock_create_service_template +): + data = { + 'name': "new name", + 'template_content': "here are some noodles 🍜", + 'template_type': 'sms', + 'service': service_one['id'], + 'process_type': 'normal' + } + resp = logged_in_client.post(url_for( + '.add_service_template', + service_id=service_one['id'], + template_type='sms' + ), data=data) + + assert resp.status_code == 200 + assert "You can’t use 🍜 in text messages." in resp.get_data(as_text=True) + + +def test_should_not_update_sms_template_with_emoji( + logged_in_client, + service_one, + mock_get_service_template, + mock_update_service_template, + fake_uuid, +): + data = { + 'id': fake_uuid, + 'name': "new name", + 'template_content': "here's a burger 🍔", + 'service': service_one['id'], + 'template_type': 'sms', + 'process_type': 'normal' + } + resp = logged_in_client.post(url_for( + '.edit_service_template', + service_id=service_one['id'], + template_id=fake_uuid), data=data) + + assert resp.status_code == 200 + assert "You can’t use 🍔 in text messages." in resp.get_data(as_text=True) + + +def test_should_create_sms_template_without_downgrading_unicode_characters( + logged_in_client, + service_one, + mock_create_service_template +): + msg = 'here:\tare some “fancy quotes” and non\u200Bbreaking\u200Bspaces' + + data = { + 'name': "new name", + 'template_content': msg, + 'template_type': 'sms', + 'service': service_one['id'], + 'process_type': 'normal' + } + resp = logged_in_client.post(url_for( + '.add_service_template', + service_id=service_one['id'], + template_type='sms' + ), data=data) + + mock_create_service_template.assert_called_with( + ANY, # name + ANY, # type + msg, # content + ANY, # service_id + ANY, # subject + ANY # process_type + ) + assert resp.status_code == 302 diff --git a/tests/app/test_jinja_filters.py b/tests/app/test_jinja_filters.py new file mode 100644 index 000000000..2d24fc00e --- /dev/null +++ b/tests/app/test_jinja_filters.py @@ -0,0 +1,16 @@ +import pytest + +from app import formatted_list + + +@pytest.mark.parametrize('items, kwargs, expected_output', [ + ([1], {}, '‘1’'), + ([1, 2], {}, '‘1’ and ‘2’'), + ([1, 2, 3], {}, '‘1’, ‘2’ and ‘3’'), + ([1, 2, 3], {'prefix': 'foo', 'prefix_plural': 'bar'}, 'bar ‘1’, ‘2’ and ‘3’'), + ([1], {'prefix': 'foo', 'prefix_plural': 'bar'}, 'foo ‘1’'), + ([1, 2, 3], {'before_each': 'a', 'after_each': 'b'}, 'a1b, a2b and a3b'), + ([1, 2, 3], {'conjunction': 'foo'}, '‘1’, ‘2’ foo ‘3’'), +]) +def test_formatted_list(items, kwargs, expected_output): + assert formatted_list(items, **kwargs) == expected_output diff --git a/tests/conftest.py b/tests/conftest.py index 98d9a9f50..3da72832c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -386,9 +386,8 @@ def mock_get_service_letter_template(mocker): @pytest.fixture(scope='function') def mock_create_service_template(mocker, fake_uuid): - def _create(name, type_, content, service, subject=None): - template = template_json( - fake_uuid, name, type_, content, service) + def _create(name, type_, content, service, subject=None, process_type=None): + template = template_json(fake_uuid, name, type_, content, service, process_type) return {'data': template} return mocker.patch( @@ -399,8 +398,7 @@ def mock_create_service_template(mocker, fake_uuid): @pytest.fixture(scope='function') def mock_update_service_template(mocker): def _update(id_, name, type_, content, service, subject=None, process_type=None): - template = template_json( - service, id_, name, type_, content, subject) + template = template_json(service, id_, name, type_, content, subject, process_type) return {'data': template} return mocker.patch(