From 6e6d471cda63e94966b7c96d0c7861b61f245886 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 19 Jan 2017 12:05:28 +0000 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20strip=20HTML=20when=20saving=20?= =?UTF-8?q?templates?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Right now we strip HTML from templates at the point of saving them. This also converts stuff like ampersands to their entity form (eg &) and this is what we save in the database. This is a bad idea when you’re sending a text message or a letter, in which an HTML entity makes no sense. But we still need to encode HTML in the body of HTML emails. The right place to do this is when rendering the templates. The code to do this is now in utils. So this commit: - pull in this new utils code - removes the old - adds some integration tests to make sure that everything is working as expected (more thorough unit tests are happening in utils) --- app/template/rest.py | 7 ------- requirements.txt | 3 +-- tests/app/conftest.py | 17 +++++++++++++++++ tests/app/delivery/test_send_to_providers.py | 15 ++++++++------- tests/app/template/test_rest.py | 6 ++++-- 5 files changed, 30 insertions(+), 18 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index cfa1643e2..17d48d797 100644 --- a/app/template/rest.py +++ b/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] diff --git a/requirements.txt b/requirements.txt index 067ddb3dc..262f8162f 100644 --- a/requirements.txt +++ b/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 diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 14db86218..9780ec38d 100644 --- a/tests/app/conftest.py +++ b/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 some HTML & 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 some HTML", + subject_line="((name)) some HTML") + + @pytest.fixture(scope='function') def sample_api_key(notify_db, notify_db_session, diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 133a3bf94..97071c210 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/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 some HTML & 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" ', 'jo.smith@example.com', - 'Jo', - body='Hello Jo\nThis is an email from GOV.\u200bUK', + 'Jo some HTML', + body='Hello Jo\nThis is an email from GOV.\u200bUK with some HTML', html_body=ANY, reply_to_address=None ) assert 'content' 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 ' + ) 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