Merge pull request #1133 from alphagov/gsm

Don't let users add non-GSMish characters to sms templates

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)

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')
 

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')
+                )
+            )

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 (
-                if 'content' in e.message and any(['character count greater than' in x for x in e.message['content']]):
+                e.status_code == 400 and
-                    form.template_content.errors.extend(e.message['content'])
+                'content' in e.message and
-                else:
+                any(['character count greater than' in x for x in e.message['content']])
-                    raise e
+            ):
+                form.template_content.errors.extend(e.message['content'])
             else:
                 raise e
         else:

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 | formatted_list(
-    placeholders,
     before_each="<span class='placeholder'>((",
     after_each='))</span>',
     separator=' '

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 @@
     <div class="bottom-gutter">
       {% call banner_wrapper(type='dangerous') %}
         <h1 class='banner-title'>
-          Your file needs to have {{ formatted_list(
+          Your file needs to have {{ recipients.recipient_column_headers | formatted_list(
-            recipients.recipient_column_headers,
             prefix='a column called',
             prefix_plural='columns called'
           ) }}
         </h1>
         <p>
-          Your file has {{ formatted_list(
+          Your file has {{ recipients.column_headers | formatted_list(
-            recipients.column_headers,
+            prefix='one column, called ',
-            prefix='one column, called',
+            prefix_plural='columns called '
-            prefix_plural='columns called'
           ) }}.
         </p>
         {{ skip_to_file_contents() }}
@@ -67,18 +64,16 @@
           your template
         </h1>
         <p>
-          Your file has {{ formatted_list(
+          Your file has {{ recipients.column_headers | formatted_list(
-            recipients.column_headers,
+            prefix='one column, called ',
-            prefix='one column, called',
+            prefix_plural='columns called '
-            prefix_plural='columns called'
           ) }}.
         </p>
         <p>
-          It doesn’t have {{ formatted_list(
+          It doesn’t have {{ recipients.column_headers | formatted_list(
-            recipients.missing_column_headers,
             conjunction='or',
-            prefix='a column called',
+            prefix='a column called ',
-            prefix_plural='columns called'
+            prefix_plural='columns called '
           ) }}.
         </p>
         {{ skip_to_file_contents() }}

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 @@
   <h2 class="heading-large">Formatted list</h2>
 
   <p>
-    {{ formatted_list('A', prefix="one item called") }}
+    {{ 'A' | formatted_list(prefix="one item called") }}
   </p>
 
   <p>
-    {{ formatted_list('AB', prefix_plural="two items called") }}
+    {{ 'AB' | formatted_list(prefix_plural="two items called") }}
   </p>
 
   <p>
-    {{ formatted_list('ABC') }}
+    {{ 'ABC' | formatted_list }}
   </p>
 
   <p>
-    {{ formatted_list('ABCD', before_each='<strike>', after_each='</strike>', conjunction='or') }}
+    {{ 'ABCD' | formatted_list(before_each='<strike>', after_each='</strike>', conjunction='or') }}
   </p>
 
 {% endblock %}

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

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,

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,
+    service_one,
-    fake_uuid,
+    fake_uuid
-    mocker,
 ):
-    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

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

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):
+    def _create(name, type_, content, service, subject=None, process_type=None):
-        template = template_json(
+        template = template_json(fake_uuid, name, type_, content, service, process_type)
-            fake_uuid, name, type_, content, service)
         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(
+        template = template_json(service, id_, name, type_, content, subject, process_type)
-            service, id_, name, type_, content, subject)
         return {'data': template}
 
     return mocker.patch(