From 61fd27c4f61e318dd23f6f9a49f7bd1a306e68f6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 7 Jul 2017 10:04:49 +0100 Subject: [PATCH 1/3] Fix 500 on two-way conversation page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We changed the schema used by the endpoint that searches for notifications by recipient. So the admin app was looking for the wrong thing in the JSON. This is hard to catch in tests because it relies on our fixtures matching what the API really returns. This commit fixes the code to use the correct key to lookup the template content from the JSON. This also exposed the fact that we weren’t passing in the personalisation any more (perhaps got lost in the re-reverts somehow) so users were only seeing the template in the inbound view, not the full message content. --- app/main/views/conversation.py | 10 ++++++-- tests/__init__.py | 1 - tests/app/main/views/test_conversation.py | 30 +++++++++++++++++------ 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index 5542e25de..84b6528be 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -41,14 +41,20 @@ def get_sms_thread(service_id, user_number): ), key=lambda notification: notification['created_at']): is_inbound = ('notify_number' in notification) + redact_personalisation = notification.get('template', {}).get('redact_personalisation', False) + + if redact_personalisation: + notification['personalisation'] = {} yield { 'inbound': is_inbound, 'content': SMSPreviewTemplate( { - 'content': notification.get('content') or notification['body'] + 'content': notification.get('content') or notification['template']['content'] }, - downgrade_non_gsm_characters=(not is_inbound) + notification.get('personalisation'), + downgrade_non_gsm_characters=(not is_inbound), + redact_missing_personalisation=redact_personalisation, ), 'created_at': notification['created_at'], 'status': notification.get('status'), diff --git a/tests/__init__.py b/tests/__init__.py index 33a46087d..ab20273e6 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -254,7 +254,6 @@ def notification_json( 'notifications': [{ 'id': uuid.uuid4(), 'to': to, - 'body': template['content'], 'template': template, 'job': job_payload, 'sent_at': sent_at, diff --git a/tests/app/main/views/test_conversation.py b/tests/app/main/views/test_conversation.py index 0408690d5..014e7810d 100644 --- a/tests/app/main/views/test_conversation.py +++ b/tests/app/main/views/test_conversation.py @@ -7,8 +7,9 @@ from flask import ( from notifications_python_client.errors import HTTPError from tests.conftest import ( SERVICE_ONE_ID, + mock_get_notifications, + normalize_spaces, ) -from tests.conftest import normalize_spaces from freezegun import freeze_time from unittest import mock from app.main.views.conversation import get_user_number @@ -70,15 +71,30 @@ def test_get_user_phone_number_raises_if_both_API_requests_fail( mock_get_notification.assert_called_once_with('service', 'notification') +@pytest.mark.parametrize('outbound_redacted, expected_outbound_content', [ + (True, 'Hello hidden'), + (False, 'Hello Jo'), +]) @freeze_time("2012-01-01 00:00:00") def test_view_conversation( logged_in_client, + mocker, + api_user_active, fake_uuid, mock_get_notification, mock_get_inbound_sms, - mock_get_notifications, + outbound_redacted, + expected_outbound_content, ): + mock_get_notifications( + mocker, + api_user_active, + template_content='Hello ((name))', + personalisation={'name': 'Jo'}, + redact_personalisation=outbound_redacted, + ) + response = logged_in_client.get(url_for( 'main.conversation', service_id=SERVICE_ONE_ID, @@ -127,23 +143,23 @@ def test_view_conversation( 'Failed (sent yesterday at 11:00pm)', ), ( - 'template content', + expected_outbound_content, 'yesterday at midnight', ), ( - 'template content', + expected_outbound_content, 'yesterday at midnight', ), ( - 'template content', + expected_outbound_content, 'yesterday at midnight', ), ( - 'template content', + expected_outbound_content, 'yesterday at midnight', ), ( - 'template content', + expected_outbound_content, 'yesterday at midnight', ), ]): From 91a071ce5dfad8f16b98c65158931b58b7dade19 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 7 Jul 2017 13:32:07 +0100 Subject: [PATCH 2/3] Fix import conflict --- tests/app/main/views/test_conversation.py | 91 +++++++++++------------ 1 file changed, 43 insertions(+), 48 deletions(-) diff --git a/tests/app/main/views/test_conversation.py b/tests/app/main/views/test_conversation.py index 014e7810d..605054c32 100644 --- a/tests/app/main/views/test_conversation.py +++ b/tests/app/main/views/test_conversation.py @@ -7,64 +7,57 @@ from flask import ( from notifications_python_client.errors import HTTPError from tests.conftest import ( SERVICE_ONE_ID, - mock_get_notifications, normalize_spaces, + mock_get_notifications, + mock_get_inbound_sms, ) from freezegun import freeze_time from unittest import mock from app.main.views.conversation import get_user_number -@mock.patch( - 'app.main.views.conversation.service_api_client.get_inbound_sms_by_id', - return_value={ - 'user_number': '4407900900123' - } -) -@mock.patch( - 'app.main.views.conversation.notification_api_client.get_notification', - side_effect=HTTPError, -) -def test_get_user_phone_number_when_only_inbound_exists( - mock_get_notification, - mock_get_inbound_sms, -): +def test_get_user_phone_number_when_only_inbound_exists(mocker): + + mock_get_inbound_sms = mocker.patch( + 'app.main.views.conversation.service_api_client.get_inbound_sms_by_id', + return_value={ + 'user_number': '4407900900123' + } + ) + mock_get_notification = mocker.patch( + 'app.main.views.conversation.notification_api_client.get_notification', + side_effect=HTTPError, + ) assert get_user_number('service', 'notification') == '07900 900123' mock_get_inbound_sms.assert_called_once_with('service', 'notification') assert mock_get_notification.called is False -@mock.patch( - 'app.main.views.conversation.service_api_client.get_inbound_sms_by_id', - side_effect=HTTPError, -) -@mock.patch( - 'app.main.views.conversation.notification_api_client.get_notification', - return_value={ - 'to': '15550000000' - } -) -def test_get_user_phone_number_when_only_outbound_exists( - mock_get_notification, - mock_get_inbound_sms, -): +def test_get_user_phone_number_when_only_outbound_exists(mocker): + mock_get_inbound_sms = mocker.patch( + 'app.main.views.conversation.service_api_client.get_inbound_sms_by_id', + side_effect=HTTPError, + ) + mock_get_notification = mocker.patch( + 'app.main.views.conversation.notification_api_client.get_notification', + return_value={ + 'to': '15550000000' + } + ) assert get_user_number('service', 'notification') == '+1 555-000-0000' mock_get_inbound_sms.assert_called_once_with('service', 'notification') mock_get_notification.assert_called_once_with('service', 'notification') -@mock.patch( - 'app.main.views.conversation.service_api_client.get_inbound_sms_by_id', - side_effect=HTTPError, -) -@mock.patch( - 'app.main.views.conversation.notification_api_client.get_notification', - side_effect=HTTPError, -) -def test_get_user_phone_number_raises_if_both_API_requests_fail( - mock_get_notification, - mock_get_inbound_sms, -): +def test_get_user_phone_number_raises_if_both_API_requests_fail(mocker): + mock_get_inbound_sms = mocker.patch( + 'app.main.views.conversation.service_api_client.get_inbound_sms_by_id', + side_effect=HTTPError, + ) + mock_get_notification = mocker.patch( + 'app.main.views.conversation.notification_api_client.get_notification', + side_effect=HTTPError, + ) with pytest.raises(HTTPError): get_user_number('service', 'notification') mock_get_inbound_sms.assert_called_once_with('service', 'notification') @@ -77,12 +70,11 @@ def test_get_user_phone_number_raises_if_both_API_requests_fail( ]) @freeze_time("2012-01-01 00:00:00") def test_view_conversation( - logged_in_client, + client_request, mocker, api_user_active, - fake_uuid, mock_get_notification, - mock_get_inbound_sms, + fake_uuid, outbound_redacted, expected_outbound_content, ): @@ -95,13 +87,16 @@ def test_view_conversation( redact_personalisation=outbound_redacted, ) - response = logged_in_client.get(url_for( + mock_get_inbound_sms( + mocker + ) + + page = client_request.get( 'main.conversation', service_id=SERVICE_ONE_ID, notification_id=fake_uuid, - )) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + _test_page_title=False, + ) messages = page.select('.sms-message-wrapper') statuses = page.select('.sms-message-status') From 84a291f24ff95bc1053b10ec98a6c27f965074d0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 7 Jul 2017 14:16:18 +0100 Subject: [PATCH 3/3] No idea why this is failing on Jenkins --- tests/app/main/test_asset_fingerprinter.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/app/main/test_asset_fingerprinter.py b/tests/app/main/test_asset_fingerprinter.py index acdaa6fef..c2cbc5df7 100644 --- a/tests/app/main/test_asset_fingerprinter.py +++ b/tests/app/main/test_asset_fingerprinter.py @@ -6,9 +6,9 @@ from unittest import mock from app.asset_fingerprinter import AssetFingerprinter -@mock.patch.object(AssetFingerprinter, 'get_asset_file_contents') class TestAssetFingerprint(object): - def test_url_format(self, get_file_content_mock): + def test_url_format(self, mocker): + get_file_content_mock = mocker.patch.object(AssetFingerprinter, 'get_asset_file_contents') get_file_content_mock.return_value = """ body { font-family: nta; @@ -26,7 +26,8 @@ class TestAssetFingerprint(object): '/suppliers/static/application-ie6.css?418e6f4a6cdf1142e45c072ed3e1c90a' # noqa ) - def test_building_file_path(self, get_file_content_mock): + def test_building_file_path(self, mocker): + get_file_content_mock = mocker.patch.object(AssetFingerprinter, 'get_asset_file_contents') get_file_content_mock.return_value = """ document.write('Hello world!'); """ @@ -36,7 +37,8 @@ class TestAssetFingerprint(object): 'app/static/javascripts/application.js' ) - def test_hashes_are_consistent(self, get_file_content_mock): + def test_hashes_are_consistent(self, mocker): + get_file_content_mock = mocker.patch.object(AssetFingerprinter, 'get_asset_file_contents') get_file_content_mock.return_value = """ body { font-family: nta; @@ -49,8 +51,9 @@ class TestAssetFingerprint(object): ) def test_hashes_are_different_for_different_files( - self, get_file_content_mock + self, mocker ): + get_file_content_mock = mocker.patch.object(AssetFingerprinter, 'get_asset_file_contents') asset_fingerprinter = AssetFingerprinter() get_file_content_mock.return_value = """ body { @@ -66,7 +69,8 @@ class TestAssetFingerprint(object): js_hash != css_hash ) - def test_hash_gets_cached(self, get_file_content_mock): + def test_hash_gets_cached(self, mocker): + get_file_content_mock = mocker.patch.object(AssetFingerprinter, 'get_asset_file_contents') get_file_content_mock.return_value = """ body { font-family: nta;