From 41fa1586353374d611f7b7e7690b03c13b830880 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 13 Feb 2017 13:11:29 +0000 Subject: [PATCH 1/7] error when users put non-GSM chars in a sms template additionally, this moves the formatted_list jinja macro into a python function, so that it can be called from the form validator --- app/__init__.py | 23 +++++++++++++++++++++ app/main/forms.py | 5 +++-- app/main/validators.py | 21 ++++++++++++++----- app/templates/components/list.html | 32 +---------------------------- app/templates/views/check.html | 13 ++++-------- app/templates/views/styleguide.html | 9 ++++---- 6 files changed, 51 insertions(+), 52 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 6697fb7c4..1c95a7f75 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,28 @@ def format_notification_status_as_url(status): }.get(status) +def formatted_list( + items, + conjunction='and', + before_each='‘', + after_each='’', + separator=', ', + prefix='', + 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..87bfec066 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -23,7 +23,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): @@ -260,7 +260,8 @@ class SMSTemplateForm(Form): u'Message', validators=[ DataRequired(message="Can’t be empty"), - NoCommasInPlaceHolders() + NoCommasInPlaceHolders(), + OnlyGSMCharacters() ] ) process_type = RadioField( diff --git a/app/main/validators.py b/app/main/validators.py index d07d7cab7..096f83c51 100644 --- a/app/main/validators.py +++ b/app/main/validators.py @@ -1,13 +1,15 @@ from wtforms import ValidationError from notifications_utils.template import Template +from notifications_utils.gsm import get_non_gsm_characters + +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 +20,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 +30,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 +42,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 +50,12 @@ 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 = get_non_gsm_characters(field.data) + if non_gsm_characters: + raise ValidationError('The following characters are not allowed in text messages: {}'.format( + ', '.join(non_gsm_characters) + )) 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..ef0788923 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,15 +40,13 @@
{% 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, + Your file has {{ recipients.column_headers | formatted_list( prefix='one column, called', prefix_plural='columns called' ) }}. @@ -67,15 +64,13 @@ your template

- Your file has {{ formatted_list( - recipients.column_headers, + 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' 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 %} From c25fff903206455060bde7527b38f726a9bfde94 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 13 Feb 2017 15:04:12 +0000 Subject: [PATCH 2/7] add tests for formatted_list --- app/__init__.py | 23 ++++++++++++++--------- app/templates/views/check.html | 12 ++++++------ tests/app/test_jinja_filters.py | 16 ++++++++++++++++ 3 files changed, 36 insertions(+), 15 deletions(-) create mode 100644 tests/app/test_jinja_filters.py diff --git a/app/__init__.py b/app/__init__.py index 1c95a7f75..db4a51d31 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -347,24 +347,29 @@ def format_notification_status_as_url(status): def formatted_list( - items, - conjunction='and', - before_each='‘', - after_each='’', - separator=', ', - prefix='', - prefix_plural='' + 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()) + 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}' + '{prefix_plural}{first_items} {conjunction} {last_item}' ).format(**locals()) diff --git a/app/templates/views/check.html b/app/templates/views/check.html index ef0788923..b8880fe30 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -47,8 +47,8 @@

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

{{ skip_to_file_contents() }} @@ -65,15 +65,15 @@

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

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/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 From 73a965a3c67ec65ffe38b11344b78d534c03ebce Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 14 Feb 2017 17:06:32 +0000 Subject: [PATCH 3/7] allow downgradeable unicode characters in SMS templates --- app/main/validators.py | 13 ++++++++----- tests/app/main/test_validators.py | 17 ++++++++++++++++- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/app/main/validators.py b/app/main/validators.py index 096f83c51..6b1b60374 100644 --- a/app/main/validators.py +++ b/app/main/validators.py @@ -1,7 +1,8 @@ from wtforms import ValidationError from notifications_utils.template import Template -from notifications_utils.gsm import get_non_gsm_characters +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, @@ -54,8 +55,10 @@ class NoCommasInPlaceHolders: class OnlyGSMCharacters: def __call__(self, form, field): - non_gsm_characters = get_non_gsm_characters(field.data) + non_gsm_characters = sorted(list(get_non_gsm_compatible_characters(field.data))) if non_gsm_characters: - raise ValidationError('The following characters are not allowed in text messages: {}'.format( - ', '.join(non_gsm_characters) - )) + raise ValidationError( + 'You can’t use {} in text messages. They won’t show up properly on everyone’s phones.'.format( + formatted_list(non_gsm_characters, conjunction='or') + ) + ) diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index 9d4917507..432d73aac 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,21 @@ 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)) + + +def test_non_gsm_character_validation(client): + with pytest.raises(ValidationError) as error: + OnlyGSMCharacters()(None, _gen_mock_field('∆ abc 📲 def 📵 ghi')) + + assert str(error.value) == ( + 'You can’t use ‘∆’, ‘📲’ or ‘📵’ in text messages. ' + 'They won’t show up properly on everyone’s phones.' + ) + + def test_sms_sender_form_validation( client, mock_get_user_by_email, From 6f8568b9041a6f6ab760cf9450ea72d753082795 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 15 Feb 2017 13:49:15 +0000 Subject: [PATCH 4/7] add tests for gsm handling in save/edit template --- requirements.txt | 2 +- tests/app/main/views/test_templates.py | 114 +++++++++++++++++++++++-- tests/conftest.py | 8 +- 3 files changed, 109 insertions(+), 15 deletions(-) diff --git a/requirements.txt b/requirements.txt index ee6fae6fd..d54b7e9af 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.5.0#egg=notifications-utils==13.5.0 +git+https://github.com/alphagov/notifications-utils.git@13.6.0#egg=notifications-utils==13.6.0 diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index aa15e83f5..6ace9466e 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,29 @@ 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_without_downgrading_unicode_characters( + logged_in_client, + mocker, + service_one, + fake_uuid, +): + msg = 'here:\tare some “fancy quotes” and non\u200Bbreaking\u200Bspaces' + + 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 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 +92,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 +627,81 @@ 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_should_not_create_sms_template_with_emoji( + logged_in_client, + service_one, + mock_get_service_template, + 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/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( From 9046ec3bbc1765bf20e74411f0374f3236d9d70d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 15 Feb 2017 15:06:47 +0000 Subject: [PATCH 5/7] ensure emails still accept emoji --- app/main/forms.py | 15 ++++++++++----- app/main/views/templates.py | 11 ++++++----- tests/app/main/views/test_templates.py | 23 ++++++++++++++++++++++- 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 87bfec066..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 @@ -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")]) @@ -260,8 +262,7 @@ class SMSTemplateForm(Form): u'Message', validators=[ DataRequired(message="Can’t be empty"), - NoCommasInPlaceHolders(), - OnlyGSMCharacters() + NoCommasInPlaceHolders() ] ) process_type = RadioField( @@ -275,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")]) @@ -480,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/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/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 6ace9466e..69c5eaf3b 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -629,10 +629,31 @@ 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_get_service_template, mock_create_service_template ): data = { From f550699daffc318d7747e96da9da9a74d150dd5b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 15 Feb 2017 16:21:14 +0000 Subject: [PATCH 6/7] fix non-gsm error message Use `it`/`they` depending on how many different characters you've used Also don't wrap the message with quotes, as it looks confusing and potentialy implies that you can't use apostrophes --- app/main/validators.py | 5 +++-- tests/app/main/test_validators.py | 25 +++++++++++++++++++------ tests/app/main/views/test_templates.py | 4 ++-- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/app/main/validators.py b/app/main/validators.py index 6b1b60374..42cd29c8d 100644 --- a/app/main/validators.py +++ b/app/main/validators.py @@ -58,7 +58,8 @@ class OnlyGSMCharacters: 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. They won’t show up properly on everyone’s phones.'.format( - formatted_list(non_gsm_characters, conjunction='or') + '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/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index 432d73aac..93b007337 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -146,14 +146,27 @@ def test_gsm_character_validation(client, msg): OnlyGSMCharacters()(None, _gen_mock_field(msg)) -def test_non_gsm_character_validation(client): +@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('∆ abc 📲 def 📵 ghi')) + OnlyGSMCharacters()(None, _gen_mock_field(data)) - assert str(error.value) == ( - 'You can’t use ‘∆’, ‘📲’ or ‘📵’ in text messages. ' - 'They won’t show up properly on everyone’s phones.' - ) + assert str(error.value) == err_msg def test_sms_sender_form_validation( diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 69c5eaf3b..29b9173ee 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -670,7 +670,7 @@ def test_should_not_create_sms_template_with_emoji( ), data=data) assert resp.status_code == 200 - assert "You can’t use ‘🍜’ in text messages." in resp.get_data(as_text=True) + assert "You can’t use 🍜 in text messages." in resp.get_data(as_text=True) def test_should_not_update_sms_template_with_emoji( @@ -694,7 +694,7 @@ def test_should_not_update_sms_template_with_emoji( 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) + 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( From efd976e32e22dfeaefbe7dd7e5e6eeff6c3e2b46 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 20 Feb 2017 12:03:16 +0000 Subject: [PATCH 7/7] downgrade non-gsm chars in SMS previews bump utils to 13.8.0 we still save the content as the user intended, and they'll still see that content in the text field if they go to edit the template, but the SMS previews will appear as they will on a user's phone --- requirements.txt | 2 +- tests/app/main/views/test_templates.py | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) 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/views/test_templates.py b/tests/app/main/views/test_templates.py index 29b9173ee..ab336482f 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -32,13 +32,14 @@ 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_without_downgrading_unicode_characters( +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 non\u200Bbreaking\u200Bspaces' + 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', @@ -52,7 +53,7 @@ def test_should_show_sms_template_without_downgrading_unicode_characters( template_id=template_id)) assert response.status_code == 200 - assert msg in response.get_data(as_text=True) + assert rendered_msg in response.get_data(as_text=True) def test_should_show_page_template_with_priority_select_if_platform_admin(