Merge pull request #1215 from alphagov/fix-address-column-handling

Fix address column handling

app/main/views/send.py

@@ -77,7 +77,6 @@ def get_example_letter_address(key):
     return {
         'address line 1': 'A. Name',
         'address line 2': '123 Example Street',
-        'address line 3': 'Example town',
         'postcode': 'XM4 5HQ'
     }.get(key, '')
 

app/main/views/templates.py

@@ -287,13 +287,17 @@ def edit_service_template(service_id, template_id):
         }, current_service)
         template_change = get_template(template, current_service).compare_to(new_template)
         if template_change.has_different_placeholders and not request.form.get('confirm'):
+            example_column_headings = (
+                first_column_headings[new_template.template_type] +
+                list(new_template.placeholders)
+            )
             return render_template(
                 'views/templates/breaking-change.html',
                 template_change=template_change,
                 new_template=new_template,
-                column_headings=list(ascii_uppercase[:len(new_template.placeholders) + 1]),
+                column_headings=list(ascii_uppercase[:len(example_column_headings)]),
                 example_rows=[
-                    first_column_headings[new_template.template_type] + list(new_template.placeholders),
+                    example_column_headings,
                     get_example_csv_rows(new_template),
                     get_example_csv_rows(new_template)
                 ],

app/templates/views/templates/breaking-change.html

@@ -41,8 +41,8 @@
 
   <p>
     When you send messages using this template you’ll need
-    {{ new_template.placeholders|length + 1 }}
+    {{ column_headings|length }}
-    column{{ 's' if new_template.placeholders|length > 0 else '' }} of data:
+    column{{ 's' if column_headings|length > 1 else '' }} of data:
   </p>
 
   <div class="spreadsheet">

requirements.txt

@@ -31,4 +31,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@14.0.0#egg=notifications-utils==14.0.0
+git+https://github.com/alphagov/notifications-utils.git@15.0.1#egg=notifications-utils==15.0.1

tests/app/main/views/test_templates.py

@@ -8,6 +8,7 @@ from flask import url_for
 from freezegun import freeze_time
 from notifications_python_client.errors import HTTPError
 from tests.conftest import service_one as create_sample_service
+from tests.conftest import mock_get_service_email_template, mock_get_service_letter_template
 from tests import validate_route_permission, template_json, single_notification_json
 from tests.app.test_utils import normalize_spaces
 
@@ -313,18 +314,43 @@ def test_should_403_when_create_template_with_process_type_of_priority_for_non_p
     mock_update_service_template.called == 0
 
 
+@pytest.mark.parametrize('template_mock, expected_paragraphs', [
+    (
+        mock_get_service_email_template,
+        [
+            'You removed ((date))',
+            'You added ((name))',
+            'When you send messages using this template you’ll need 3 columns of data:',
+        ]
+    ),
+    (
+        mock_get_service_letter_template,
+        [
+            'You removed ((date))',
+            'You added ((name))',
+            'When you send messages using this template you’ll need 9 columns of data:',
+        ]
+    ),
+])
 def test_should_show_interstitial_when_making_breaking_change(
     logged_in_client,
     api_user_active,
     mock_login,
-    mock_get_service_email_template,
     mock_update_service_template,
     mock_get_user,
     mock_get_service,
     mock_get_user_by_email,
     mock_has_permissions,
     fake_uuid,
+    mocker,
+    template_mock,
+    expected_paragraphs,
 ):
+    template_mock(
+        mocker,
+        subject="Your ((thing)) is due soon",
+        content="Your vehicle tax expires on ((date))",
+    )
     service_id = fake_uuid
     template_id = fake_uuid
     response = logged_in_client.post(
@@ -346,11 +372,7 @@ def test_should_show_interstitial_when_making_breaking_change(
     assert page.find('a', {'class': 'page-footer-back-link'})['href'] == url_for(".edit_service_template",
                                                                                  service_id=service_id,
                                                                                  template_id=template_id)
-    for index, p in enumerate([
+    for index, p in enumerate(expected_paragraphs):
-        'You removed ((date))',
-        'You added ((name))',
-        'When you send messages using this template you’ll need 3 columns of data:',
-    ]):
         assert normalize_spaces(page.select('main p')[index].text) == p
 
     for key, value in {

tests/conftest.py

@@ -336,15 +336,15 @@ def mock_get_service_template_with_placeholders(mocker):
 
 
 @pytest.fixture(scope='function')
-def mock_get_service_email_template(mocker):
+def mock_get_service_email_template(mocker, content=None, subject=None):
     def _get(service_id, template_id, version=None):
         template = template_json(
             service_id,
             template_id,
             "Two week reminder",
             "email",
-            "Your vehicle tax expires on ((date))",
+            content or "Your vehicle tax expires on ((date))",
-            "Your ((thing)) is due soon"
+            subject or "Your ((thing)) is due soon",
         )
         return {'data': template}
 
@@ -354,30 +354,24 @@ def mock_get_service_email_template(mocker):
 
 @pytest.fixture(scope='function')
 def mock_get_service_email_template_without_placeholders(mocker):
-    def _create(service_id, template_id):
+    return mock_get_service_email_template(
-        template = template_json(
+        mocker,
-            service_id,
+        content="Your vehicle tax expires soon",
-            template_id,
+        subject="Your thing is due soon",
-            "Two week reminder",
+    )
-            "email",
-            "Your vehicle tax expires soon",
-            "Your thing is due soon"
-        )
-        return {'data': template}
-
-    return mocker.patch(
-        'app.service_api_client.get_service_template', side_effect=_create)
 
 
 @pytest.fixture(scope='function')
-def mock_get_service_letter_template(mocker):
+def mock_get_service_letter_template(mocker, content=None, subject=None):
     def _create(service_id, template_id):
         template = template_json(
             service_id,
             template_id,
             "Two week reminder",
             "letter",
-            "Template <em>content</em> with & entity", "Subject")
+            content or "Template <em>content</em> with & entity",
+            subject or "Subject",
+        )
         return {'data': template}
 
     return mocker.patch(