diff --git a/app/main/views/send.py b/app/main/views/send.py index c6d720be1..c3415c78b 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -15,8 +15,9 @@ from flask import ( ) from flask_login import current_user from notifications_python_client.errors import HTTPError -from notifications_utils import SMS_CHAR_COUNT_LIMIT +from notifications_utils import LETTER_MAX_PAGE_COUNT, SMS_CHAR_COUNT_LIMIT from notifications_utils.columns import Columns +from notifications_utils.pdf import is_letter_too_long from notifications_utils.recipients import ( RecipientCSV, first_column_headings, @@ -522,6 +523,7 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ email_reply_to = get_email_reply_to_address_from_session() elif db_template['template_type'] == 'sms': sms_sender = get_sms_sender_from_session() + template = get_template( db_template, current_service, @@ -567,6 +569,8 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ elif preview_row > 2: abort(404) + page_count = get_page_count_for_letter(db_template, template.values) + return dict( recipients=recipients, template=template, @@ -589,7 +593,10 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ preview_row=preview_row, sent_previously=job_api_client.has_sent_previously( service_id, template.id, db_template['version'], request.args.get('original_file_name', '') - ) + ), + letter_too_long=is_letter_too_long(page_count), + letter_max_pages=LETTER_MAX_PAGE_COUNT, + page_count=page_count ) @@ -601,12 +608,12 @@ def check_messages(service_id, template_id, upload_id, row_index=2): data = _check_messages(service_id, template_id, upload_id, row_index) if ( - data['recipients'].too_many_rows or - not data['count_of_recipients'] or - not data['recipients'].has_recipient_columns or - data['recipients'].duplicate_recipient_column_headers or - data['recipients'].missing_column_headers or - data['sent_previously'] + data['recipients'].too_many_rows + or not data['count_of_recipients'] + or not data['recipients'].has_recipient_columns + or data['recipients'].duplicate_recipient_column_headers + or data['recipients'].missing_column_headers + or data['sent_previously'] ): return render_template('views/check/column-errors.html', **data) @@ -614,8 +621,8 @@ def check_messages(service_id, template_id, upload_id, row_index=2): return render_template('views/check/row-errors.html', **data) if ( - data['errors'] or - data['trying_to_send_letters_in_trial_mode'] + data['errors'] + or data['trying_to_send_letters_in_trial_mode'] ): return render_template('views/check/column-errors.html', **data) @@ -877,10 +884,14 @@ def _check_notification(service_id, template_id, exception=None): raise PermanentRedirect(back_link) template.values = get_recipient_and_placeholders_from_session(template.template_type) + page_count = get_page_count_for_letter(db_template, template.values) return dict( template=template, back_link=back_link, help=get_help_argument(), + letter_too_long=is_letter_too_long(page_count), + letter_max_pages=LETTER_MAX_PAGE_COUNT, + page_count=page_count, **(get_template_error_dict(exception) if exception else {}), ) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 7a5923e13..7e8fc6019 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -6,7 +6,9 @@ from flask import abort, flash, redirect, render_template, request, url_for from flask_login import current_user from markupsafe import Markup from notifications_python_client.errors import HTTPError +from notifications_utils import LETTER_MAX_PAGE_COUNT from notifications_utils.formatters import nl2br +from notifications_utils.pdf import is_letter_too_long from notifications_utils.recipients import first_column_headings from app import ( @@ -58,6 +60,8 @@ def view_template(service_id, template_id): '.send_one_off', service_id=service_id, template_id=template_id )) + page_count = get_page_count_for_letter(template) + return render_template( 'views/templates/template.html', template=get_template( @@ -74,6 +78,9 @@ def view_template(service_id, template_id): ), template_postage=template["postage"], user_has_template_permission=user_has_template_permission, + letter_too_long=is_letter_too_long(page_count), + letter_max_pages=LETTER_MAX_PAGE_COUNT, + page_count=page_count ) diff --git a/app/templates/components/banner.html b/app/templates/components/banner.html index 3b30cf1cb..1a272fa55 100644 --- a/app/templates/components/banner.html +++ b/app/templates/components/banner.html @@ -1,12 +1,15 @@ {% from "components/form.html" import form_wrapper %} -{% macro banner(body, type=None, with_tick=False, delete_button=None, subhead=None, context=None, action=None) %} +{% macro banner(body, type=None, with_tick=False, delete_button=None, subhead=None, context=None, action=None, id=None) %}
{% if subhead -%}

{{ subhead }}

@@ -26,6 +29,6 @@
{% endmacro %} -{% macro banner_wrapper(type=None, with_tick=False, delete_button=None, subhead=None, action=None) %} - {{ banner(caller()|safe, type=type, with_tick=with_tick, delete_button=delete_button, subhead=subhead, action=action) }} +{% macro banner_wrapper(type=None, with_tick=False, delete_button=None, subhead=None, action=None, id=None) %} + {{ banner(caller()|safe, type=type, with_tick=with_tick, delete_button=delete_button, subhead=subhead, action=action, id=id) }} {% endmacro %} diff --git a/app/templates/partials/check/letter-too-long.html b/app/templates/partials/check/letter-too-long.html new file mode 100644 index 000000000..578759bc7 --- /dev/null +++ b/app/templates/partials/check/letter-too-long.html @@ -0,0 +1,8 @@ +

+ Your letter is too long +

+

+ Letters must be {{ letter_max_pages }} pages or less. +
+ Your letter is {{ page_count }} pages long. +

diff --git a/app/templates/views/check/ok.html b/app/templates/views/check/ok.html index 693208bf9..d726395a9 100644 --- a/app/templates/views/check/ok.html +++ b/app/templates/views/check/ok.html @@ -25,10 +25,15 @@ back_link=back_link ) }} + {% if letter_too_long %} + {% call banner_wrapper(type='dangerous', id='letter-too-long') %} + {% include "partials/check/letter-too-long.html" %} + {% endcall %} + {% endif %} + {{ skip_to_file_contents() }} {{ template|string }} -
@@ -39,7 +44,7 @@ wrapping_class='bottom-gutter-2-3' ) }} {% endif %} - {% if template.template_type != 'letter' or not request.args.from_test %} + {% if (template.template_type != 'letter' or not request.args.from_test) and not letter_too_long %} {% else %} Download as a PDF diff --git a/app/templates/views/notifications/check.html b/app/templates/views/notifications/check.html index bdf55ecf4..9fc2ff20a 100644 --- a/app/templates/views/notifications/check.html +++ b/app/templates/views/notifications/check.html @@ -49,6 +49,14 @@ {% include "partials/check/message-too-long.html" %} {% endcall %}
+ {% elif letter_too_long %} + {% set error = 'letter-too-long' %} + {{ govuk_back_link(back_link) }} +
+ {% call banner_wrapper(type='dangerous', id='letter-too-long') %} + {% include "partials/check/letter-too-long.html" %} + {% endcall %} +
{% else %} {{ page_header( 'Preview of ‘{}’'.format(template.name), diff --git a/app/templates/views/templates/_template.html b/app/templates/views/templates/_template.html index a6d907a23..f1112724b 100644 --- a/app/templates/views/templates/_template.html +++ b/app/templates/views/templates/_template.html @@ -1,4 +1,5 @@ {% from 'components/message-count-label.html' import message_count_label %} +{% from "components/banner.html" import banner_wrapper %}
{% if template._template.archived %} @@ -15,7 +16,12 @@
{% if template.template_type == 'letter' %} - {% if current_user.has_permissions('send_messages', restrict_admin_usage=True) %} + {% if letter_too_long %} + {% call banner_wrapper(type='dangerous', id='letter-too-long') %} + {% include "partials/check/letter-too-long.html" %} + {% endcall %} + {% endif %} + {% if current_user.has_permissions('send_messages', restrict_admin_usage=True) and not letter_too_long %}
Send diff --git a/app/utils.py b/app/utils.py index 723f29f4f..71e058fd3 100644 --- a/app/utils.py +++ b/app/utils.py @@ -43,6 +43,7 @@ FAILURE_STATUSES = ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure', 'virus-scan-failed', 'validation-failed'] REQUESTED_STATUSES = SENDING_STATUSES + DELIVERED_STATUSES + FAILURE_STATUSES + with open('{}/email_domains.txt'.format( os.path.dirname(os.path.realpath(__file__)) )) as email_domains: diff --git a/requirements-app.txt b/requirements-app.txt index a650b656e..55df8fd15 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -23,5 +23,5 @@ awscli-cwlogs>=1.4,<1.5 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default itsdangerous==0.24 # pyup: <1.0.0 -git+https://github.com/alphagov/notifications-utils.git@34.1.0#egg=notifications-utils==34.1.0 +git+https://github.com/alphagov/notifications-utils.git@35.0.0#egg=notifications-utils==35.0.0 git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.3.0-alpha#egg=govuk-frontend-jinja==0.3.0-alpha diff --git a/requirements.txt b/requirements.txt index 9f5399ffd..b05f94dc2 100644 --- a/requirements.txt +++ b/requirements.txt @@ -25,18 +25,18 @@ awscli-cwlogs>=1.4,<1.5 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default itsdangerous==0.24 # pyup: <1.0.0 -git+https://github.com/alphagov/notifications-utils.git@34.1.0#egg=notifications-utils==34.1.0 +git+https://github.com/alphagov/notifications-utils.git@35.0.0#egg=notifications-utils==35.0.0 git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.3.0-alpha#egg=govuk-frontend-jinja==0.3.0-alpha ## The following requirements were added by pip freeze: -awscli==1.16.251 +awscli==1.16.255 bleach==3.1.0 -boto3==1.6.16 -botocore==1.12.241 +boto3==1.9.221 +botocore==1.12.245 certifi==2019.9.11 chardet==3.0.4 Click==7.0 -colorama==0.3.9 +colorama==0.4.1 dnspython==1.16.0 docopt==0.6.2 docutils==0.15.2 @@ -46,7 +46,7 @@ future==0.17.1 greenlet==0.4.15 idna==2.8 jdcal==1.4.1 -Jinja2==2.10.1 +Jinja2==2.10.3 jmespath==0.9.4 lml==0.0.9 lxml==4.4.1 @@ -55,14 +55,14 @@ mistune==0.8.4 monotonic==1.5 openpyxl==2.5.14 orderedset==2.0.1 -phonenumbers==8.10.13 +phonenumbers==8.10.17 pyasn1==0.4.7 pyexcel-ezodf==0.3.4 PyJWT==1.7.1 PyPDF2==1.26.0 python-dateutil==2.8.0 python-json-logger==0.1.11 -PyYAML==4.2b1 +PyYAML==5.1.2 redis==3.3.8 requests==2.22.0 rsa==3.4.2 diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index c63c9fe29..6b5ff2921 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -2735,6 +2735,56 @@ def test_check_messages_shows_trial_mode_error_for_letters( assert page.select_one('.table-field-index a').text == '3' +@pytest.mark.parametrize('number_of_rows, expected_error_message', [ + (1, 'This letter is'), + (11, 'These letters are'), # TODO: Pluralise too many pages error message for multiple letters +]) +def test_check_messages_does_not_allow_to_send_letter_longer_than_10_pages( + client_request, + api_user_active, + mock_get_service_letter_template, + mock_has_permissions, + mock_get_users_by_service, + mock_get_service_statistics, + mock_get_job_doesnt_exist, + mock_get_jobs, + mock_s3_set_metadata, + fake_uuid, + mocker, + mock_get_live_service, + number_of_rows, + expected_error_message, +): + mocker.patch('app.main.views.send.s3download', return_value='\n'.join( + ['address_line_1,address_line_2,postcode,'] + + ['First Last, 123 Street, SW1 1AA'] * number_of_rows + )) + mocker.patch( + 'app.main.views.send.get_page_count_for_letter', + return_value=11, + ) + + with client_request.session_transaction() as session: + session['file_uploads'] = { + fake_uuid: { + 'template_id': '', + } + } + + page = client_request.get( + 'main.check_messages', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + upload_id=fake_uuid, + _test_page_title=False, + ) + + assert page.select('#letter-too-long') + + assert len(page.select('.letter img')) == 10 # if letter longer than 10 pages, only 10 first pages are displayed + assert not page.select('[type=submit]') + + def test_check_messages_shows_data_errors_before_trial_mode_errors_for_letters( mocker, client_request, @@ -3066,6 +3116,45 @@ def test_send_one_off_letter_errors_in_trial_mode( assert page.select_one('a[download]').text == 'Download as a PDF' +def test_send_one_off_letter_errors_if_letter_longer_than_10_pages( + client_request, + mocker, + mock_get_live_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=11, + ) + + 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 page.select('#letter-too-long') + assert len(page.select('.letter img')) == 10 + + assert not page.select('[type=submit]') + + def test_check_messages_shows_over_max_row_error( client_request, mock_get_users_by_service, diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index b9e0c041e..924636805 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -521,6 +521,31 @@ def test_view_non_letter_template_does_not_display_postage( assert "Postage" not in page.text +def test_view_letter_template_does_not_display_send_button_if_template_over_10_pages_long( + client_request, + service_one, + mock_get_service_templates, + mock_get_template_folders, + single_letter_contact_block, + mock_has_jobs, + active_user_with_permissions, + mocker, + fake_uuid, +): + mocker.patch('app.main.views.templates.get_page_count_for_letter', return_value=11) + client_request.login(active_user_with_permissions) + mock_get_service_letter_template(mocker, postage="second") + page = client_request.get( + 'main.view_template', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _test_page_title=False, + ) + + assert "Send" not in page.text + assert page.select('#letter-too-long') + + def test_edit_letter_template_postage_page_displays_correctly( client_request, service_one,