From 3a62946ecd2ab2982d479481f96bf1df487442f8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 30 Oct 2018 14:24:50 +0000 Subject: [PATCH 1/5] Let people send one off letters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We didn’t used to allow this because it wasn’t really possible with the old DVLA set up and we didn’t think there’s a need. We think it’s possible now because, even though it’s cumbersome, it’s better than the manual process. --- app/main/views/send.py | 75 ++++++++++++++------ app/navigation.py | 4 ++ app/templates/views/templates/_template.html | 7 +- tests/app/main/views/test_send.py | 44 +++++++++--- 4 files changed, 90 insertions(+), 40 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 5992b4ac3..aec7e44e1 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -317,16 +317,13 @@ def send_test(service_id, template_id): def get_notification_check_endpoint(service_id, template): - if template.template_type == 'letter': - return make_and_upload_csv_file(service_id, template) - else: - return redirect(url_for( - 'main.check_notification', - service_id=service_id, - template_id=template.id, - # at check phase we should move to help stage 2 ("the template pulls in the data you provide") - help='2' if 'help' in request.args else None - )) + return redirect(url_for( + 'main.check_notification', + service_id=service_id, + template_id=template.id, + # at check phase we should move to help stage 2 ("the template pulls in the data you provide") + help='2' if 'help' in request.args else None + )) @main.route( @@ -669,6 +666,26 @@ def check_messages_preview(service_id, template_id, upload_id, filetype, row_ind return TemplatePreview.from_utils_template(template, filetype, page=page) +@main.route( + "/services///check.", + methods=['GET'], +) +@login_required +@user_has_permissions('send_messages') +def check_notification_preview(service_id, template_id, filetype): + if filetype == 'pdf': + page = None + elif filetype == 'png': + page = request.args.get('page', 1) + else: + abort(404) + + template = _check_notification( + service_id, template_id, + )['template'] + return TemplatePreview.from_utils_template(template, filetype, page=page) + + @main.route("/services//start-job/", methods=['POST']) @login_required @user_has_permissions('send_messages', restrict_admin_usage=True) @@ -762,8 +779,6 @@ def all_placeholders_in_session(placeholders): def get_send_test_page_title(template_type, help_argument, entering_recipient, name=None): if help_argument: return 'Example text message' - if template_type == 'letter': - return 'Print a test letter' if entering_recipient: return 'Send ‘{}’'.format(name) return 'Personalise this message' @@ -807,7 +822,10 @@ def get_back_link(service_id, template, step_index): @login_required @user_has_permissions('send_messages', restrict_admin_usage=True) def check_notification(service_id, template_id): - return _check_notification(service_id, template_id) + return render_template( + 'views/notifications/check.html', + **_check_notification(service_id, template_id), + ) def _check_notification(service_id, template_id, exception=None): @@ -823,25 +841,33 @@ def _check_notification(service_id, template_id, exception=None): current_service, show_recipient=True, email_reply_to=email_reply_to, - sms_sender=sms_sender + sms_sender=sms_sender, + letter_preview_url=url_for( + '.check_notification_preview', + service_id=service_id, + template_id=template_id, + filetype='png', + ), + page_count=get_page_count_for_letter(db_template), ) back_link = get_back_link(service_id, template, len(fields_to_fill_in(template))) if ( - not session.get('recipient') or - not all_placeholders_in_session(template.placeholders) + ( + not session.get('recipient') + and db_template['template_type'] != 'letter' + ) + or not all_placeholders_in_session(template.placeholders) ): - return redirect(back_link) + raise RequestRedirect(back_link) template.values = get_recipient_and_placeholders_from_session(template.template_type) - return render_template( - 'views/notifications/check.html', + return dict( template=template, back_link=back_link, help=get_help_argument(), - - **(get_template_error_dict(exception) if exception else {}) + **(get_template_error_dict(exception) if exception else {}), ) @@ -880,7 +906,7 @@ def send_notification(service_id, template_id): noti = notification_api_client.send_notification( service_id, template_id=template_id, - recipient=session['recipient'], + recipient=session['recipient'] or session['placeholders']['address line 1'], personalisation=session['placeholders'], sender_id=session['sender_id'] if 'sender_id' in session else None ) @@ -889,7 +915,10 @@ def send_notification(service_id, template_id): current_service.id, exception.message )) - return _check_notification(service_id, template_id, exception) + return render_template( + 'views/notifications/check.html', + **_check_notification(service_id, template_id, exception), + ) session.pop('placeholders') session.pop('recipient') diff --git a/app/navigation.py b/app/navigation.py index 148302750..cf893c5ca 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -125,6 +125,7 @@ class HeaderNavigation(Navigation): 'check_messages', 'check_messages_preview', 'check_notification', + 'check_notification_preview', 'choose_account', 'choose_service', 'choose_template', @@ -395,6 +396,7 @@ class MainNavigation(Navigation): 'check_and_resend_text_code', 'check_and_resend_verification_code', 'check_messages_preview', + 'check_notification_preview', 'choose_account', 'choose_service', 'confirm_edit_organisation_name', @@ -567,6 +569,7 @@ class CaseworkNavigation(Navigation): 'check_messages', 'check_messages_preview', 'check_notification', + 'check_notification_preview', 'choose_account', 'choose_service', 'choose_template_to_copy', @@ -801,6 +804,7 @@ class OrgNavigation(Navigation): 'check_messages', 'check_messages_preview', 'check_notification', + 'check_notification_preview', 'choose_account', 'choose_service', 'choose_template', diff --git a/app/templates/views/templates/_template.html b/app/templates/views/templates/_template.html index b6c19510d..333d35c1c 100644 --- a/app/templates/views/templates/_template.html +++ b/app/templates/views/templates/_template.html @@ -16,14 +16,9 @@
{% if template.template_type == 'letter' %} {% if current_user.has_permissions('send_messages', restrict_admin_usage=True) %} -
- - Upload a list of addresses - -
- Print a test letter + Send
{% endif %} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 7f570d3cb..3e8d321a8 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1074,13 +1074,13 @@ def test_send_one_off_does_not_send_without_the_correct_permissions( ( mock_get_service_letter_template, partial(url_for, 'main.send_test'), - 'Print a test letter', + 'Send ‘Two week reminder’', False, ), ( mock_get_service_letter_template, partial(url_for, 'main.send_one_off'), - 'Print a test letter', + 'Send ‘Two week reminder’', False, ), ]) @@ -1595,10 +1595,9 @@ def test_send_test_letter_redirects_to_right_url( assert response.status_code == 302 assert response.location.startswith(url_for( - 'main.check_messages', + 'main.check_notification', service_id=SERVICE_ONE_ID, template_id=fake_uuid, - upload_id=fake_uuid, _external=True, )) @@ -2875,27 +2874,50 @@ def test_check_notification_shows_help( ) +@pytest.mark.parametrize('template, recipient, placeholders, expected_personalisation', ( + ( + mock_get_service_template, + '07700900001', + {'a': 'b'}, + {'a': 'b'}, + ), + ( + mock_get_service_email_template, + 'test@example.com', + {}, + {}, + ), + ( + mock_get_service_letter_template, + 'foo', + {}, + {}, + ), +)) def test_send_notification_submits_data( client_request, - service_one, fake_uuid, mock_send_notification, + template, + recipient, + placeholders, + expected_personalisation, ): with client_request.session_transaction() as session: - session['recipient'] = '07700900001' - session['placeholders'] = {'a': 'b'} + session['recipient'] = recipient + session['placeholders'] = placeholders client_request.post( 'main.send_notification', - service_id=service_one['id'], + service_id=SERVICE_ONE_ID, template_id=fake_uuid ) mock_send_notification.assert_called_once_with( - service_one['id'], + SERVICE_ONE_ID, template_id=fake_uuid, - recipient='07700900001', - personalisation={'a': 'b'}, + recipient=recipient, + personalisation=expected_personalisation, sender_id=None ) From a79dfd1d6e80766064c48739e9846fa7fb88cb74 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 30 Oct 2018 14:27:09 +0000 Subject: [PATCH 2/5] Always show download link for PDF This feature is useful for people to try the letter thing out and see how it feels, maybe print one on their own printer before incurring cost. --- app/main/views/send.py | 1 - app/templates/views/notifications/check.html | 7 +- tests/app/main/views/test_send.py | 75 +++++++++++++++++--- 3 files changed, 70 insertions(+), 13 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index aec7e44e1..7f5ab7fba 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -592,7 +592,6 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ trying_to_send_letters_in_trial_mode=all(( current_service.trial_mode, template.template_type == 'letter', - not request.args.get('from_test'), )), required_recipient_columns=OrderedSet(recipients.recipient_column_headers) - optional_address_columns, preview_row=preview_row, diff --git a/app/templates/views/notifications/check.html b/app/templates/views/notifications/check.html index dec854f0b..99b287757 100644 --- a/app/templates/views/notifications/check.html +++ b/app/templates/views/notifications/check.html @@ -50,13 +50,12 @@ )}}" class='page-footer'> {% if not error %} - {% if template.template_type != 'letter' or not request.args.from_test %} - {% else %} - Download as a printable PDF - {% endif %} {% endif %} Back + {% if template.template_type == 'letter' %} + Download as a printable PDF + {% endif %}
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 3e8d321a8..beeafabc5 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1964,7 +1964,7 @@ def test_test_message_can_only_be_sent_now( def test_letter_can_only_be_sent_now( client_request, mocker, - service_one, + mock_get_live_service, mock_get_service_letter_template, mock_get_users_by_service, mock_get_service_statistics, @@ -1978,10 +1978,9 @@ def test_letter_can_only_be_sent_now( content = client_request.get( 'main.check_messages', - service_id=service_one['id'], + service_id=SERVICE_ONE_ID, upload_id=fake_uuid, template_id=fake_uuid, - from_test=True ) assert 'name="scheduled_for"' not in content @@ -2657,7 +2656,11 @@ def test_check_messages_column_error_doesnt_show_optional_columns( ) -def test_generate_test_letter_doesnt_block_in_trial_mode( +@pytest.mark.parametrize('extra_args', ( + {}, + {'from_test': True}, +)) +def test_letters_from_csv_files_dont_have_download_link( client_request, mocker, mock_get_service, @@ -2668,6 +2671,7 @@ def test_generate_test_letter_doesnt_block_in_trial_mode( mock_get_service_statistics, mock_get_job_doesnt_exist, mock_s3_set_metadata, + extra_args, ): mocker.patch('app.main.views.send.s3download', return_value=""" @@ -2692,14 +2696,69 @@ def test_generate_test_letter_doesnt_block_in_trial_mode( service_id=SERVICE_ONE_ID, template_id=fake_uuid, upload_id=fake_uuid, - from_test=True, + _test_page_title=False, + **extra_args + ) + + assert normalize_spaces( + page.select_one('.banner-dangerous').text + ) == normalize_spaces( + 'You can’t send this letter ' + 'In trial mode you can only preview how your letters will look ' + 'Skip to file contents' + ) + + assert len(page.select('.letter img')) == 5 + assert not page.select('a[download]') + + +@pytest.mark.parametrize('service_mock', ( + mock_get_service, + mock_get_live_service, +)) +def test_one_off_letters_have_download_link( + client_request, + mocker, + api_user_active, + mock_get_service_letter_template, + mock_has_permissions, + fake_uuid, + mock_get_users_by_service, + mock_get_service_statistics, + service_mock, +): + + service_mock(mocker, api_user_active) + + mocker.patch( + 'app.main.views.send.get_page_count_for_letter', + return_value=5, + ) + + with client_request.session_transaction() as session: + session['recipient'] = None + session['placeholders'] = { + 'address_line_1': 'First Last', + 'address_line_2': '123 Street', + 'postcode': 'SW1 1AA', + } + + page = client_request.get( + 'main.check_notification', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, _test_page_title=False, ) - assert not page.select('.banner-dangerous') - assert len(page.select('.letter img')) == 5 - assert page.select_one('a.button').text == 'Download as a printable PDF' + + assert page.select_one('a[download]')['href'] == url_for( + 'main.check_notification_preview', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + filetype='pdf', + ) + assert page.select_one('a[download]').text == 'Download as a printable PDF' def test_check_messages_shows_over_max_row_error( From 1675e6c8d214d4138ae5e7e9cc23948f1dc15029 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 30 Oct 2018 16:32:10 +0000 Subject: [PATCH 3/5] Show error banner for one off letter in trial mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In trial mode you can’t send letters. But it’s still useful to be able to build up a letter to see how it work. Best place to put this error is before someone tries to send a letter for real. --- .../trying-to-send-letters-in-trial-mode.html | 8 +++ app/templates/views/check/column-errors.html | 17 +++--- app/templates/views/notifications/check.html | 13 ++++- tests/app/main/views/test_send.py | 54 ++++++++++++++++++- 4 files changed, 81 insertions(+), 11 deletions(-) create mode 100644 app/templates/partials/check/trying-to-send-letters-in-trial-mode.html diff --git a/app/templates/partials/check/trying-to-send-letters-in-trial-mode.html b/app/templates/partials/check/trying-to-send-letters-in-trial-mode.html new file mode 100644 index 000000000..df585a345 --- /dev/null +++ b/app/templates/partials/check/trying-to-send-letters-in-trial-mode.html @@ -0,0 +1,8 @@ +

+ You can’t send + {{ 'this letter' if count_of_recipients == 1 else 'these letters' }} +

+

+ In trial mode you + can only preview how your letters will look +

diff --git a/app/templates/views/check/column-errors.html b/app/templates/views/check/column-errors.html index 75e2101c7..e91bacc67 100644 --- a/app/templates/views/check/column-errors.html +++ b/app/templates/views/check/column-errors.html @@ -109,14 +109,15 @@ {% elif trying_to_send_letters_in_trial_mode %} -

- You can’t send - {{ 'this letter' if count_of_recipients == 1 else 'these letters' }} -

-

- In trial mode you - can only preview how your letters will look -

+
+ {% with + count_of_recipients=count_of_recipients + %} + {% call banner_wrapper(type='dangerous') %} + {% include "partials/check/trying-to-send-letters-in-trial-mode.html" %} + {% endcall %} + {% endwith %} +
{% elif recipients.more_rows_than_can_send %} diff --git a/app/templates/views/notifications/check.html b/app/templates/views/notifications/check.html index 99b287757..b0a060c7a 100644 --- a/app/templates/views/notifications/check.html +++ b/app/templates/views/notifications/check.html @@ -7,7 +7,18 @@ {% endblock %} {% block maincolumn_content %} - {% if error == 'not-allowed-to-send-to' %} + {% if template.template_type == 'letter' and current_service.trial_mode %} + {% set error = 'trial-mode-letters' %} +
+ {% with + count_of_recipients=1 + %} + {% call banner_wrapper(type='dangerous') %} + {% include "partials/check/trying-to-send-letters-in-trial-mode.html" %} + {% endcall %} + {% endwith %} +
+ {% elif error == 'not-allowed-to-send-to' %}
{% call banner_wrapper(type='dangerous') %} {% with diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index beeafabc5..7a6e2f502 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1976,14 +1976,19 @@ def test_letter_can_only_be_sent_now( mocker.patch('app.main.views.send.set_metadata_on_csv_upload') mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1) - content = client_request.get( + page = client_request.get( 'main.check_messages', service_id=SERVICE_ONE_ID, upload_id=fake_uuid, template_id=fake_uuid, ) - assert 'name="scheduled_for"' not in content + assert 'name="scheduled_for"' not in page + assert normalize_spaces( + page.select_one('[type=submit]').text + ) == ( + 'Send 1 letter' + ) @pytest.mark.parametrize('when', [ @@ -2761,6 +2766,51 @@ def test_one_off_letters_have_download_link( assert page.select_one('a[download]').text == 'Download as a printable PDF' +def test_send_one_off_letter_errors_in_trial_mode( + client_request, + mocker, + mock_get_service, + mock_get_service_letter_template, + mock_has_permissions, + fake_uuid, + mock_get_users_by_service, + mock_get_service_statistics, + mock_get_job_doesnt_exist, + mock_s3_set_metadata, +): + + mocker.patch( + 'app.main.views.send.get_page_count_for_letter', + return_value=5, + ) + + with client_request.session_transaction() as session: + session['recipient'] = None + session['placeholders'] = { + 'address_line_1': 'First Last', + 'address_line_2': '123 Street', + 'postcode': 'SW1 1AA', + } + + page = client_request.get( + 'main.check_notification', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _test_page_title=False, + ) + + assert normalize_spaces(page.select('.banner-dangerous')) == normalize_spaces( + 'You can’t send this letter ' + 'In trial mode you can only preview how your letters will look' + ) + + assert len(page.select('.letter img')) == 5 + + assert not page.select('[type=submit]') + assert page.select_one('.page-footer-back-link').text == 'Back' + assert page.select_one('a[download]').text == 'Download as a printable PDF' + + def test_check_messages_shows_over_max_row_error( logged_in_client, api_user_active, From d6b785d3fa4c1836e835333353e0d74c7b3c6a40 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 31 Oct 2018 14:08:37 +0000 Subject: [PATCH 4/5] =?UTF-8?q?Style=20=E2=80=98Download=20PDF=E2=80=99=20?= =?UTF-8?q?link?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/assets/stylesheets/components/page-footer.scss | 7 +++++++ app/templates/views/notifications/check.html | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/components/page-footer.scss b/app/assets/stylesheets/components/page-footer.scss index 2d668b5a5..d126068c5 100644 --- a/app/assets/stylesheets/components/page-footer.scss +++ b/app/assets/stylesheets/components/page-footer.scss @@ -1,5 +1,6 @@ .page-footer { + position: relative; margin-bottom: 30px; &-back-link { @@ -50,6 +51,12 @@ margin-top: $gutter; } + &-right-aligned-link { + position: absolute; + right: 0; + top: 10px; // align baseline with buttons + } + .button, .button-destructive { margin-right: 10px; diff --git a/app/templates/views/notifications/check.html b/app/templates/views/notifications/check.html index b0a060c7a..c904f1788 100644 --- a/app/templates/views/notifications/check.html +++ b/app/templates/views/notifications/check.html @@ -65,7 +65,7 @@ {% endif %} Back {% if template.template_type == 'letter' %} - Download as a printable PDF + Download as a printable PDF {% endif %}
From 82005144616ff050acf32741855539c4f146b2f2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 1 Nov 2018 14:45:12 +0000 Subject: [PATCH 5/5] Fix double error border --- app/templates/views/check/column-errors.html | 4 +--- app/templates/views/notifications/check.html | 12 ++++++------ 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/app/templates/views/check/column-errors.html b/app/templates/views/check/column-errors.html index e91bacc67..94c81a559 100644 --- a/app/templates/views/check/column-errors.html +++ b/app/templates/views/check/column-errors.html @@ -113,9 +113,7 @@ {% with count_of_recipients=count_of_recipients %} - {% call banner_wrapper(type='dangerous') %} - {% include "partials/check/trying-to-send-letters-in-trial-mode.html" %} - {% endcall %} + {% include "partials/check/trying-to-send-letters-in-trial-mode.html" %} {% endwith %} diff --git a/app/templates/views/notifications/check.html b/app/templates/views/notifications/check.html index c904f1788..15beea30f 100644 --- a/app/templates/views/notifications/check.html +++ b/app/templates/views/notifications/check.html @@ -10,13 +10,13 @@ {% if template.template_type == 'letter' and current_service.trial_mode %} {% set error = 'trial-mode-letters' %}
- {% with - count_of_recipients=1 - %} - {% call banner_wrapper(type='dangerous') %} + {% call banner_wrapper(type='dangerous') %} + {% with + count_of_recipients=1 + %} {% include "partials/check/trying-to-send-letters-in-trial-mode.html" %} - {% endcall %} - {% endwith %} + {% endwith %} + {% endcall %}
{% elif error == 'not-allowed-to-send-to' %}