Merge pull request #796 from alphagov/dont-strip-html-on-save

Don’t strip HTML when saving templates

app/template/rest.py

@@ -4,7 +4,6 @@ from flask import (
     request,
     current_app
 )
-import bleach
 
 from app.dao.templates_dao import (
     dao_update_template,
@@ -41,7 +40,6 @@ def create_template(service_id):
     fetched_service = dao_fetch_service_by_id(service_id=service_id)
     new_template = template_schema.load(request.get_json()).data
     new_template.service = fetched_service
-    new_template.content = _strip_html(new_template.content)
     over_limit = _content_count_greater_than_limit(new_template.content, new_template.template_type)
     if over_limit:
         char_count_limit = current_app.config.get('SMS_CHAR_COUNT_LIMIT')
@@ -60,7 +58,6 @@ def update_template(service_id, template_id):
     current_data = dict(template_schema.dump(fetched_template).data.items())
     updated_template = dict(template_schema.dump(fetched_template).data.items())
     updated_template.update(request.get_json())
-    updated_template['content'] = _strip_html(updated_template['content'])
     # Check if there is a change to make.
     if _template_has_not_changed(current_data, updated_template):
         return jsonify(data=updated_template), 200
@@ -137,10 +134,6 @@ def get_template_versions(service_id, template_id):
     return jsonify(data=data)
 
 
-def _strip_html(content):
-    return bleach.clean(content, tags=[], strip=True)
-
-
 def _template_has_not_changed(current_data, updated_template):
     return all(
         current_data[key] == updated_template[key]

requirements.txt

@@ -1,5 +1,4 @@
 apispec==0.14.0
-bleach==1.4.3
 Flask==0.10.1
 Flask-Script==2.0.5
 Flask-Migrate==1.3.1
@@ -24,6 +23,6 @@ gunicorn==19.6.0
 # pin to minor version 3.1.x
 notifications-python-client>=3.1,<3.2
 
-git+https://github.com/alphagov/notifications-utils.git@13.1.0#egg=notifications-utils==13.1.0
+git+https://github.com/alphagov/notifications-utils.git@13.2.0#egg=notifications-utils==13.2.0
 
 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3

tests/app/conftest.py

@@ -188,6 +188,14 @@ def sample_template_with_placeholders(notify_db, notify_db_session):
     return sample_template(notify_db, notify_db_session, content="Hello (( Name))\nYour thing is due soon")
 
 
+@pytest.fixture(scope='function')
+def sample_sms_template_with_html(notify_db, notify_db_session):
+    # deliberate space and title case in placeholder
+    return sample_template(notify_db, notify_db_session, content=(
+        "Hello (( Name))\nHere is <em>some HTML</em> & entities"
+    ))
+
+
 @pytest.fixture(scope='function')
 def sample_email_template(
         notify_db,
@@ -224,6 +232,15 @@ def sample_email_template_with_placeholders(notify_db, notify_db_session):
         subject_line="((name))")
 
 
+@pytest.fixture(scope='function')
+def sample_email_template_with_html(notify_db, notify_db_session):
+    return sample_email_template(
+        notify_db,
+        notify_db_session,
+        content="Hello ((name))\nThis is an email from GOV.UK with <em>some HTML</em>",
+        subject_line="((name)) <em>some HTML</em>")
+
+
 @pytest.fixture(scope='function')
 def sample_api_key(notify_db,
                    notify_db_session,

tests/app/delivery/test_send_to_providers.py

@@ -50,10 +50,10 @@ def test_should_return_highest_priority_active_provider(notify_db, notify_db_ses
 def test_should_send_personalised_template_to_correct_sms_provider_and_persist(
     notify_db,
     notify_db_session,
-    sample_template_with_placeholders,
+    sample_sms_template_with_html,
     mocker
 ):
-    db_notification = sample_notification(notify_db, notify_db_session, template=sample_template_with_placeholders,
+    db_notification = sample_notification(notify_db, notify_db_session, template=sample_sms_template_with_html,
                                           to_field="+447234123123", personalisation={"name": "Jo"},
                                           status='created')
 
@@ -66,7 +66,7 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist(
 
     mmg_client.send_sms.assert_called_once_with(
         to=format_phone_number(validate_phone_number("+447234123123")),
-        content="Sample service: Hello Jo\nYour thing is due soon",
+        content="Sample service: Hello Jo\nHere is <em>some HTML</em> & entities",
         reference=str(db_notification.id),
         sender=None
     )
@@ -82,12 +82,12 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist(
 def test_should_send_personalised_template_to_correct_email_provider_and_persist(
     notify_db,
     notify_db_session,
-    sample_email_template_with_placeholders,
+    sample_email_template_with_html,
     mocker
 ):
     db_notification = sample_notification(
         notify_db=notify_db, notify_db_session=notify_db_session,
-        template=sample_email_template_with_placeholders,
+        template=sample_email_template_with_html,
         to_field="jo.smith@example.com",
         personalisation={'name': 'Jo'}
     )
@@ -102,12 +102,13 @@ def test_should_send_personalised_template_to_correct_email_provider_and_persist
     app.aws_ses_client.send_email.assert_called_once_with(
         '"Sample service" <sample.service@test.notify.com>',
         'jo.smith@example.com',
-        'Jo',
-        body='Hello Jo\nThis is an email from GOV.\u200bUK',
+        'Jo <em>some HTML</em>',
+        body='Hello Jo\nThis is an email from GOV.\u200bUK with <em>some HTML</em>',
         html_body=ANY,
         reply_to_address=None
     )
     assert '<!DOCTYPE html' in app.aws_ses_client.send_email.call_args[1]['html_body']
+    assert '&lt;em&gt;some HTML&lt;/em&gt;' in app.aws_ses_client.send_email.call_args[1]['html_body']
 
     notification = Notification.query.filter_by(id=db_notification.id).one()
     assert notification.status == 'sending'

tests/app/template/test_rest.py

@@ -37,7 +37,7 @@ def test_should_create_a_new_template_for_a_service(
     json_resp = json.loads(response.get_data(as_text=True))
     assert json_resp['data']['name'] == 'my template'
     assert json_resp['data']['template_type'] == template_type
-    assert json_resp['data']['content'] == 'template content'
+    assert json_resp['data']['content'] == 'template <b>content</b>'
     assert json_resp['data']['service'] == str(sample_service.id)
     assert json_resp['data']['id']
     assert json_resp['data']['version'] == 1
@@ -153,7 +153,9 @@ def test_update_should_update_a_template(client, sample_user, sample_template):
 
     assert update_response.status_code == 200
     update_json_resp = json.loads(update_response.get_data(as_text=True))
-    assert update_json_resp['data']['content'] == 'my template has new content alert("foo")'
+    assert update_json_resp['data']['content'] == (
+        'my template has new content <script type="text/javascript">alert("foo")</script>'
+    )
     assert update_json_resp['data']['name'] == sample_template.name
     assert update_json_resp['data']['template_type'] == sample_template.template_type
     assert update_json_resp['data']['version'] == 2