From e34d981dda9acbde9e5fd39bc5423c8e604cc694 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 6 Apr 2017 10:22:09 +0100 Subject: [PATCH] Fix no. of column headers on breaking change page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The breaking change page wasn’t properly accounting for the fact that letter recipients span multiple columns – it was assuming they’d only take up one column like they do for email and SMS. This commit fixes: - the number of column headers (A, B, C, …) to be correct - the count of columns (you will need X columns in your file) to be correct It then parameterises the test to look at a case where a recipient is in one column (email) and multiple columns (letter). --- app/main/views/templates.py | 8 +++-- .../views/templates/breaking-change.html | 4 +-- requirements.txt | 2 +- tests/app/main/views/test_templates.py | 34 +++++++++++++++---- tests/conftest.py | 30 +++++++--------- 5 files changed, 49 insertions(+), 29 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 5b0e4e7e3..cb6831304 100644 --- a/app/main/views/templates.py +++ b/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) ], diff --git a/app/templates/views/templates/breaking-change.html b/app/templates/views/templates/breaking-change.html index ef82f7263..8bd486584 100644 --- a/app/templates/views/templates/breaking-change.html +++ b/app/templates/views/templates/breaking-change.html @@ -41,8 +41,8 @@

When you send messages using this template you’ll need - {{ new_template.placeholders|length + 1 }} - column{{ 's' if new_template.placeholders|length > 0 else '' }} of data: + {{ column_headings|length }} + column{{ 's' if column_headings|length > 1 else '' }} of data:

diff --git a/requirements.txt b/requirements.txt index f186ab97a..a1642c343 100644 --- a/requirements.txt +++ b/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 diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 71ea2fb8a..d4777468f 100644 --- a/tests/app/main/views/test_templates.py +++ b/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([ - 'You removed ((date))', - 'You added ((name))', - 'When you send messages using this template you’ll need 3 columns of data:', - ]): + for index, p in enumerate(expected_paragraphs): assert normalize_spaces(page.select('main p')[index].text) == p for key, value in { diff --git a/tests/conftest.py b/tests/conftest.py index b622768e2..7805b6568 100644 --- a/tests/conftest.py +++ b/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))", - "Your ((thing)) is due soon" + content or "Your vehicle tax expires on ((date))", + 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): - template = template_json( - service_id, - template_id, - "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) + return mock_get_service_email_template( + mocker, + content="Your vehicle tax expires soon", + subject="Your thing is due soon", + ) @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 content with & entity", "Subject") + content or "Template content with & entity", + subject or "Subject", + ) return {'data': template} return mocker.patch(