From 3bd62c2aeff626bc7fb04ab7b19e638f22821514 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 18 Jul 2018 09:48:19 +0100 Subject: [PATCH] Offer link to upload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ‘Upload recipients’ and ‘Send to one recipient’ have always been slightly clunky phrases. Now that basic view jumps straight into the ‘Send to one recipient’ flow there’s no way for users to get to the ‘Upload recipients’ flow. By adding a link to it from the ‘Send to one recipient’ flow it’s possible for users of basic view to access it. But we don’t want to introduce too much inconsistency between basic view and admin view because users will be migrating from one to another. They might also be talking to their manager, who wouldn’t be able to tell them where to click if they were looking at two completely different interfaces. This also means that we can keep the left-hand navigation in basic view nice and simple with the two options (‘Templates’ and ‘Sent messages’), rather than trying to introduce something like ‘Send one message’ and ‘Send lots of messages’ later on. --- app/main/views/send.py | 4 + .../components/message-count-label.html | 29 +++++ app/templates/views/send-test.html | 10 +- tests/app/main/views/test_send.py | 122 +++++++++++++++--- tests/conftest.py | 2 +- 5 files changed, 149 insertions(+), 18 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 785b277a5..5bb7de3f2 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -459,6 +459,10 @@ def send_test_step(service_id, template_id, step_index): optional_placeholder=optional_placeholder, back_link=back_link, help=get_help_argument(), + link_to_upload=( + request.endpoint == 'main.send_one_off_step' and + step_index == 0 + ), ) diff --git a/app/templates/components/message-count-label.html b/app/templates/components/message-count-label.html index 771473341..14b997665 100644 --- a/app/templates/components/message-count-label.html +++ b/app/templates/components/message-count-label.html @@ -26,3 +26,32 @@ {%- endif -%} {%- endif %} {{ suffix }} {%- endmacro %} + +{% macro recipient_count_label(count, template_type) -%} + {% if template_type == None %} + {%- if count == 1 -%} + recipient + {%- else -%} + recipients + {%- endif -%} + {% endif %} + {%- if template_type == 'sms' -%} + {%- if count == 1 -%} + phone number + {%- else -%} + phone numbers + {%- endif -%} + {%- elif template_type == 'email' -%} + {%- if count == 1 -%} + email address + {%- else -%} + email addresses + {%- endif -%} + {%- elif template_type == 'letter' -%} + {%- if count == 1 -%} + address + {%- else -%} + addresses + {%- endif -%} + {%- endif %} +{%- endmacro %} diff --git a/app/templates/views/send-test.html b/app/templates/views/send-test.html index 17721a508..1691c8244 100644 --- a/app/templates/views/send-test.html +++ b/app/templates/views/send-test.html @@ -1,8 +1,7 @@ {% extends "withnav_template.html" %} {% from "components/page-footer.html" import page_footer %} -{% from "components/file-upload.html" import file_upload %} +{% from "components/message-count-label.html" import recipient_count_label %} {% from "components/textbox.html" import textbox %} -{% from "components/table.html" import list_table, field, text_field, index_field, index_field_heading %} {% block service_page_title %} {{ page_title }} @@ -29,6 +28,13 @@ {% endif %} + {% if link_to_upload %} +

+ + Upload a list of {{ recipient_count_label(999, template.template_type) }} + +

+ {% endif %} {{ page_footer('Continue', back_link=back_link) }} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 72ef0af98..4b2cfe3cb 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1191,6 +1191,101 @@ def test_skip_link_will_not_show_on_sms_one_off_if_service_has_no_mobile_number( assert not skip_links +@pytest.mark.parametrize('user, link_index', ( + (active_user_with_permissions, 1), + (active_caseworking_user, 0), +)) +def test_send_one_off_offers_link_to_upload( + client_request, + fake_uuid, + mock_get_service_template, + mock_has_jobs, + user, + link_index, +): + client_request.login(user(fake_uuid)) + + page = client_request.get( + 'main.send_one_off', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _follow_redirects=True, + ) + + link = page.select('main a')[link_index] + + assert link.text.strip() == 'Upload a list of phone numbers' + assert link['href'] == url_for( + 'main.send_messages', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + ) + + +@pytest.mark.parametrize('user', ( + active_user_with_permissions, + active_caseworking_user, +)) +def test_link_to_upload_not_offered_in_tour( + client_request, + fake_uuid, + mock_get_service_template, + user, +): + client_request.login(user(fake_uuid)) + + page = client_request.get( + 'main.send_test', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + help=1, + _follow_redirects=True, + ) + + # We’re in the tour… + assert page.select('.banner-tour') + # …but first link on the page is ‘Back’, so not preceeded by ‘Upload’ + assert page.select_one('main a').text == 'Back' + + +@pytest.mark.parametrize('user', ( + active_user_with_permissions, + active_caseworking_user, +)) +@pytest.mark.parametrize('endpoint, step_index', ( + ('main.send_one_off_step', 1), + ('main.send_test_step', 0), +)) +def test_link_to_upload_not_offered_when_entering_personalisation( + client_request, + fake_uuid, + mock_get_service_template_with_placeholders, + mock_has_jobs, + user, + endpoint, + step_index, +): + client_request.login(user(fake_uuid)) + + with client_request.session_transaction() as session: + session['recipient'] = '07900900900' + session['placeholders'] = {'phone number': '07900900900'} + + page = client_request.get( + endpoint, + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + step_index=step_index, + ) + + # We’re entering personalisation + assert page.select_one('input[type=text]')['name'] == 'placeholder_value' + assert page.select_one('label[for=placeholder_value]').text.strip() == 'name' + # …but first link on the page is ‘Back’, so not preceeded by ‘Upload’ + assert page.select_one('main a').text == 'Back' + assert 'Upload' not in page.select_one('main').text + + @pytest.mark.parametrize('user', ( active_user_with_permissions, active_caseworking_user, @@ -1208,9 +1303,8 @@ def test_skip_link_will_not_show_on_sms_one_off_if_service_has_no_mobile_number( ), ]) def test_send_test_redirects_to_end_if_step_out_of_bounds( - logged_in_client, + client_request, mock_has_no_jobs, - service_one, fake_uuid, endpoint, placeholders, @@ -1218,26 +1312,24 @@ def test_send_test_redirects_to_end_if_step_out_of_bounds( mocker, user, ): - mocker.patch('app.user_api_client.get_user', return_value=user(fake_uuid)) + client_request.login(user(fake_uuid)) - with logged_in_client.session_transaction() as session: + with client_request.session_transaction() as session: session['placeholders'] = placeholders - response = logged_in_client.get(url_for( + client_request.get( endpoint, - service_id=service_one['id'], + service_id=SERVICE_ONE_ID, template_id=fake_uuid, step_index=999, - )) - - assert response.status_code == 302 - expected_url = url_for( - expected_redirect, - service_id=service_one['id'], - template_id=fake_uuid, - _external=True, + _expected_status=302, + _expected_redirect=url_for( + expected_redirect, + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _external=True, + ) ) - assert response.location == expected_url @pytest.mark.parametrize('user', ( diff --git a/tests/conftest.py b/tests/conftest.py index aafed172e..39310e16a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2642,7 +2642,7 @@ def client_request( url_for(endpoint, **(endpoint_kwargs or {})), follow_redirects=_follow_redirects, ) - assert resp.status_code == _expected_status + assert resp.status_code == _expected_status, resp.location if _expected_redirect: assert resp.location == _expected_redirect page = BeautifulSoup(resp.data.decode('utf-8'), 'html.parser')