Merge pull request #1359 from alphagov/fix-inbound-page

Fix 500 on two-way conversation page

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

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,

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;

tests/app/main/views/test_conversation.py

@@ -7,85 +7,96 @@ from flask import (
 from notifications_python_client.errors import HTTPError
 from tests.conftest import (
     SERVICE_ONE_ID,
+    normalize_spaces,
+    mock_get_notifications,
+    mock_get_inbound_sms,
 )
-from tests.conftest import normalize_spaces
 from freezegun import freeze_time
 from unittest import mock
 from app.main.views.conversation import get_user_number
 
 
-@mock.patch(
+def test_get_user_phone_number_when_only_inbound_exists(mocker):
-    'app.main.views.conversation.service_api_client.get_inbound_sms_by_id',
+
-    return_value={
+    mock_get_inbound_sms = mocker.patch(
-        'user_number': '4407900900123'
+        '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,
+    mock_get_notification = mocker.patch(
-)
+        'app.main.views.conversation.notification_api_client.get_notification',
-def test_get_user_phone_number_when_only_inbound_exists(
+        side_effect=HTTPError,
-    mock_get_notification,
+    )
-    mock_get_inbound_sms,
-):
     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(
+def test_get_user_phone_number_when_only_outbound_exists(mocker):
-    'app.main.views.conversation.service_api_client.get_inbound_sms_by_id',
+    mock_get_inbound_sms = mocker.patch(
-    side_effect=HTTPError,
+        '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',
+    mock_get_notification = mocker.patch(
-    return_value={
+        'app.main.views.conversation.notification_api_client.get_notification',
-        'to': '15550000000'
+        return_value={
-    }
+            'to': '15550000000'
-)
+        }
-def test_get_user_phone_number_when_only_outbound_exists(
+    )
-    mock_get_notification,
-    mock_get_inbound_sms,
-):
     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(
+def test_get_user_phone_number_raises_if_both_API_requests_fail(mocker):
-    'app.main.views.conversation.service_api_client.get_inbound_sms_by_id',
+    mock_get_inbound_sms = mocker.patch(
-    side_effect=HTTPError,
+        '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',
+    mock_get_notification = mocker.patch(
-    side_effect=HTTPError,
+        '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,
-):
     with pytest.raises(HTTPError):
         get_user_number('service', 'notification')
     mock_get_inbound_sms.assert_called_once_with('service', 'notification')
     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,
+    client_request,
-    fake_uuid,
+    mocker,
+    api_user_active,
     mock_get_notification,
-    mock_get_inbound_sms,
+    fake_uuid,
-    mock_get_notifications,
+    outbound_redacted,
+    expected_outbound_content,
 ):
 
-    response = logged_in_client.get(url_for(
+    mock_get_notifications(
+        mocker,
+        api_user_active,
+        template_content='Hello ((name))',
+        personalisation={'name': 'Jo'},
+        redact_personalisation=outbound_redacted,
+    )
+
+    mock_get_inbound_sms(
+        mocker
+    )
+
+    page = client_request.get(
         'main.conversation',
         service_id=SERVICE_ONE_ID,
         notification_id=fake_uuid,
-    ))
+        _test_page_title=False,
-    assert response.status_code == 200
+    )
-    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
 
     messages = page.select('.sms-message-wrapper')
     statuses = page.select('.sms-message-status')
@@ -127,23 +138,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',
         ),
     ]):