From e3670de6c408551b89c3c6e183972976cb5946d2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 10 Jan 2020 17:00:42 +0000 Subject: [PATCH 1/9] Remove the title from the short errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This undoes some of the temporary work we did previously in order to ship the new ‘address is empty’ error message. --- app/templates/views/notifications/notification.html | 2 +- tests/app/main/views/test_notifications.py | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index f9ffe85ef..12663f3fc 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -44,7 +44,7 @@

{% elif notification_status == 'validation-failed' %}

- Validation failed – {{ message.title | safe }}. {{ message.detail | safe }} + Validation failed. {{ message.detail | safe }}

{% elif notification_status == 'technical-failure' %}

diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 9e1c1bcdf..69e1da316 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -331,9 +331,10 @@ def test_notification_page_shows_validation_failed_precompiled_letter( ) error_message = page.find('p', class_='notification-status-cancelled').text - assert normalize_spaces(error_message) == \ - "Validation failed – Your content is outside the printable area. " \ - "You need to edit page 1.Files must meet our letter specification." + assert normalize_spaces(error_message) == ( + 'Validation failed. You need to edit page 1.' + 'Files must meet our letter specification.' + ) assert not page.select('p.notification-status') From 72abd89fe0b4616f6b2fb37380863c75cb293ca5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 10 Jan 2020 17:04:50 +0000 Subject: [PATCH 2/9] Fix indentation and trailing commas This will make the diffs introducing substative changes easier to read. Consistent indenting and always having trailing commas on lists and dictionaries makes for smaller diffs. --- app/utils.py | 34 +++++++++++++++++++++++----------- tests/app/test_utils.py | 6 +++--- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/app/utils.py b/app/utils.py index af2537a05..bacce22b8 100644 --- a/app/utils.py +++ b/app/utils.py @@ -569,32 +569,44 @@ def get_letter_printing_statement(status, created_at): LETTER_VALIDATION_MESSAGES = { 'letter-not-a4-portrait-oriented': { 'title': 'Your letter is not A4 portrait size', - 'detail': 'You need to change the size or orientation of {invalid_pages}.
' - 'Files must meet our letter specification.' + 'detail': ( + 'You need to change the size or orientation of {invalid_pages}.
' + 'Files must meet our letter specification.' + ), }, 'content-outside-printable-area': { 'title': 'Your content is outside the printable area', - 'detail': 'You need to edit {invalid_pages}.
' - 'Files must meet our letter specification.' + 'detail': ( + 'You need to edit {invalid_pages}.
' + 'Files must meet our letter specification.' + ), }, 'letter-too-long': { 'title': 'Your letter is too long', - 'detail': 'Letters must be 10 pages or less.
Your letter is {page_count} pages long.' + 'detail': ( + 'Letters must be 10 pages or less.
' + 'Your letter is {page_count} pages long.' + ), }, 'no-encoded-string': { 'title': 'Sanitise failed - No encoded string' }, 'unable-to-read-the-file': { 'title': 'There’s a problem with your file', - 'detail': 'Notify cannot read this PDF.
Save a new copy of your file and try again.' + 'detail': ( + 'Notify cannot read this PDF.' + '
Save a new copy of your file and try again.' + ), }, 'address-is-empty': { 'title': 'The address block is empty', - 'detail': 'You need to add a recipient address.
' - 'Files must meet our letter specification.' + 'detail': ( + 'You need to add a recipient address.
' + 'Files must meet our letter specification.' + ), } } diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 8a62252d4..9c555cd5f 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -426,9 +426,9 @@ def test_get_letter_validation_error_for_unknown_error(): 'Letters must be 10 pages or less.
Your letter is 13 pages long.') ]) def test_get_letter_validation_error_for_known_errors( - error_message, - expected_title, - expected_content, + error_message, + expected_title, + expected_content, ): error = get_letter_validation_error(error_message, invalid_pages=[2], page_count=13) From a186d0eeff136666e5e2b035e6c33813d1ec4ce8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 10 Jan 2020 17:13:08 +0000 Subject: [PATCH 3/9] =?UTF-8?q?Don=E2=80=99t=20repeat=20the=20letter=20spe?= =?UTF-8?q?c=20URL=20in=20the=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We change this URL fairly frequently because we bump the version number. Let’s make it easier to change by only defining it once. --- app/main/views/index.py | 7 ++++++- app/main/views/uploads.py | 2 ++ app/templates/views/features/letters.html | 2 +- app/templates/views/uploads/choose-file.html | 2 +- app/utils.py | 15 +++++++++------ 5 files changed, 19 insertions(+), 9 deletions(-) diff --git a/app/main/views/index.py b/app/main/views/index.py index 8eb1b6584..7f21ac340 100644 --- a/app/main/views/index.py +++ b/app/main/views/index.py @@ -17,7 +17,11 @@ from app.main import main from app.main.forms import FieldWithNoneOption, SearchByNameForm from app.main.views.feedback import QUESTION_TICKET_TYPE from app.main.views.sub_navigation_dictionaries import features_nav, pricing_nav -from app.utils import get_logo_cdn_domain, user_is_logged_in +from app.utils import ( + LETTER_SPECIFICATION_URL, + get_logo_cdn_domain, + user_is_logged_in, +) @main.route('/') @@ -269,6 +273,7 @@ def features_sms(): def features_letters(): return render_template( 'views/features/letters.html', + letter_specification_url=LETTER_SPECIFICATION_URL, navigation_links=features_nav() ) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index fcbd64ef0..a403e76cc 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -33,6 +33,7 @@ from app.s3_client.s3_letter_upload_client import ( ) from app.template_previews import TemplatePreview, sanitise_letter from app.utils import ( + LETTER_SPECIFICATION_URL, generate_next_dict, generate_previous_dict, get_letter_validation_error, @@ -65,6 +66,7 @@ def uploads(service_id): prev_page=prev_page, next_page=next_page, scheduled_jobs='', + letter_specification_url=LETTER_SPECIFICATION_URL, ) diff --git a/app/templates/views/features/letters.html b/app/templates/views/features/letters.html index 6c5a40c2b..12bd5112f 100644 --- a/app/templates/views/features/letters.html +++ b/app/templates/views/features/letters.html @@ -33,7 +33,7 @@

Upload your own letters

You can create reusable letter templates in Notify, or upload and send your own letters with the Notify API.

-

Use the letter specification document to help you set up your letter, save it as a PDF, then upload it to Notify.

+

Use the letter specification document to help you set up your letter, save it as a PDF, then upload it to Notify.

Read our API documentation for more information.

Pricing

diff --git a/app/templates/views/uploads/choose-file.html b/app/templates/views/uploads/choose-file.html index 855134988..0593157e8 100644 --- a/app/templates/views/uploads/choose-file.html +++ b/app/templates/views/uploads/choose-file.html @@ -33,7 +33,7 @@ )}}

You can upload a single letter as a PDF.

-

Your file must meet our letter specification.

+

Your file must meet our letter specification.

To help you set up your letter you can download a Word document template.

diff --git a/app/utils.py b/app/utils.py index bacce22b8..171734519 100644 --- a/app/utils.py +++ b/app/utils.py @@ -566,21 +566,25 @@ def get_letter_printing_statement(status, created_at): return 'Printed on {} at 5:30pm'.format(printed_date) +LETTER_SPECIFICATION_URL = ( + 'https://docs.notifications.service.gov.uk' + '/documentation/images/notify-pdf-letter-spec-v2.4.pdf' +) + + LETTER_VALIDATION_MESSAGES = { 'letter-not-a4-portrait-oriented': { 'title': 'Your letter is not A4 portrait size', 'detail': ( 'You need to change the size or orientation of {invalid_pages}.
' - 'Files must meet our letter specification.' + f'Files must meet our letter specification.' ), }, 'content-outside-printable-area': { 'title': 'Your content is outside the printable area', 'detail': ( 'You need to edit {invalid_pages}.
' - 'Files must meet our letter specification.' + f'Files must meet our letter specification.' ), }, 'letter-too-long': { @@ -604,8 +608,7 @@ LETTER_VALIDATION_MESSAGES = { 'title': 'The address block is empty', 'detail': ( 'You need to add a recipient address.
' - 'Files must meet our letter specification.' + f'Files must meet our letter specification.' ), } } From 540945539b3bd96c89a227b6f0f58f16c5272e0e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 10 Jan 2020 17:29:58 +0000 Subject: [PATCH 4/9] Add some summaries of letter validation errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We show letter validation errors in two places: 1. In response to a user uploading a PDF Here we use the error banner pattern because the problem is as a direct consequence of a user’s action, and is blocking them from continuing. 2. Once a PDF provided through the API has been validated We use a less prominent pattern of red text with no border because the message is reporting on something that’s already happened, and which wasn’t a direct consequence of the user clicking something Because the context and patterns used are different we need slightly different content in each of these situations. Previously we tried to reuse the same content to make the code cleaner and less repetitive. But ultimately a clear interface trumps clear code. --- .../views/notifications/notification.html | 2 +- app/utils.py | 30 +++++++++- tests/app/main/views/test_notifications.py | 2 +- tests/app/test_utils.py | 55 +++++++++++++++---- 4 files changed, 75 insertions(+), 14 deletions(-) diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 12663f3fc..f422414ea 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -44,7 +44,7 @@

{% elif notification_status == 'validation-failed' %}

- Validation failed. {{ message.detail | safe }} + {{ message.summary | safe }}

{% elif notification_status == 'technical-failure' %}

diff --git a/app/utils.py b/app/utils.py index 171734519..5c89b0c85 100644 --- a/app/utils.py +++ b/app/utils.py @@ -579,6 +579,10 @@ LETTER_VALIDATION_MESSAGES = { 'You need to change the size or orientation of {invalid_pages}.
' f'Files must meet our letter specification.' ), + 'summary': ( + 'Validation failed because {invalid_pages} {invalid_pages_are_or_is} not A4 portrait size.
' + f'Files must meet our letter specification.' + ), }, 'content-outside-printable-area': { 'title': 'Your content is outside the printable area', @@ -586,6 +590,10 @@ LETTER_VALIDATION_MESSAGES = { 'You need to edit {invalid_pages}.
' f'Files must meet our letter specification.' ), + 'summary': ( + 'Validation failed because content is outside the printable area on {invalid_pages}.
' + f'Files must meet our letter specification.' + ), }, 'letter-too-long': { 'title': 'Your letter is too long', @@ -593,6 +601,10 @@ LETTER_VALIDATION_MESSAGES = { 'Letters must be 10 pages or less.
' 'Your letter is {page_count} pages long.' ), + 'summary': ( + 'Validation failed because this letter is {page_count} pages long.
' + 'Letters must be 10 pages or less.' + ), }, 'no-encoded-string': { 'title': 'Sanitise failed - No encoded string' @@ -603,6 +615,10 @@ LETTER_VALIDATION_MESSAGES = { 'Notify cannot read this PDF.' '
Save a new copy of your file and try again.' ), + 'summary': ( + 'Letters must be 10 pages or less.
' + 'This letter is {page_count} pages long.' + ), }, 'address-is-empty': { 'title': 'The address block is empty', @@ -610,6 +626,10 @@ LETTER_VALIDATION_MESSAGES = { 'You need to add a recipient address.
' f'Files must meet our letter specification.' ), + 'summary': ( + 'Validation failed because the address block is empty.
' + f'Files must meet our letter specification.' + ), } } @@ -618,6 +638,8 @@ def get_letter_validation_error(validation_message, invalid_pages=None, page_cou if validation_message not in LETTER_VALIDATION_MESSAGES: return {'title': 'Validation failed'} + invalid_pages_are_or_is = 'is' if len(invalid_pages) == 1 else 'are' + invalid_pages = unescaped_formatted_list( invalid_pages or [], before_each='', @@ -630,8 +652,14 @@ def get_letter_validation_error(validation_message, invalid_pages=None, page_cou 'title': LETTER_VALIDATION_MESSAGES[validation_message]['title'], 'detail': LETTER_VALIDATION_MESSAGES[validation_message]['detail'].format( invalid_pages=invalid_pages, + invalid_pages_are_or_is=invalid_pages_are_or_is, page_count=page_count, - ) + ), + 'summary': LETTER_VALIDATION_MESSAGES[validation_message]['summary'].format( + invalid_pages=invalid_pages, + invalid_pages_are_or_is=invalid_pages_are_or_is, + page_count=page_count, + ), } diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 69e1da316..012ccaa1a 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -332,7 +332,7 @@ def test_notification_page_shows_validation_failed_precompiled_letter( error_message = page.find('p', class_='notification-status-cancelled').text assert normalize_spaces(error_message) == ( - 'Validation failed. You need to edit page 1.' + 'Validation failed because content is outside the printable area on page 1.' 'Files must meet our letter specification.' ) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 9c555cd5f..f0004b774 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -413,24 +413,57 @@ def test_get_letter_validation_error_for_unknown_error(): } -@pytest.mark.parametrize('error_message, expected_title, expected_content', [ - ('letter-not-a4-portrait-oriented', 'Your letter is not A4 portrait size', - 'You need to change the size or orientation of page 2.
Files must meet our ' - 'letter specification.'), - ('content-outside-printable-area', 'Your content is outside the printable area', - 'You need to edit page 2.
Files must meet our ' - 'letter specification.'), - ('letter-too-long', 'Your letter is too long', - 'Letters must be 10 pages or less.
Your letter is 13 pages long.') +@pytest.mark.parametrize('error_message, expected_title, expected_content, expected_summary', [ + ( + 'letter-not-a4-portrait-oriented', + 'Your letter is not A4 portrait size', + ( + 'You need to change the size or orientation of page 2.
Files must meet our ' + 'letter specification.' + ), + ( + 'Validation failed because page 2 is not A4 portrait size.
' + 'Files must meet our ' + 'letter specification.' + ), + ), + ( + 'content-outside-printable-area', + 'Your content is outside the printable area', + ( + 'You need to edit page 2.
Files must meet our ' + 'letter specification.' + ), + ( + 'Validation failed because content is outside the printable area ' + 'on page 2.
Files must meet our ' + 'letter specification.' + ), + ), + ( + 'letter-too-long', + 'Your letter is too long', + ( + 'Letters must be 10 pages or less.
Your letter is 13 pages long.' + ), + ( + 'Validation failed because this letter is 13 pages long.
' + 'Letters must be 10 pages or less.' + ), + ), ]) def test_get_letter_validation_error_for_known_errors( error_message, expected_title, expected_content, + expected_summary, ): error = get_letter_validation_error(error_message, invalid_pages=[2], page_count=13) assert error['title'] == expected_title assert expected_content in error['detail'] + assert error['summary'] == expected_summary From b57e4a0d0de2ef8344283c47986e74b2ed278ec8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 10 Jan 2020 17:40:39 +0000 Subject: [PATCH 5/9] Test URLs separately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s hard to read the tests when they have HTML bundled up with content. So this commit: - introduces BeautifulSoup to parse the HTML - asserts separately on the text and any links found in the HTML --- tests/app/test_utils.py | 45 +++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index f0004b774..635f4c2ca 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -4,6 +4,7 @@ from io import StringIO from pathlib import Path import pytest +from bs4 import BeautifulSoup from freezegun import freeze_time from app import format_datetime_relative @@ -418,40 +419,36 @@ def test_get_letter_validation_error_for_unknown_error(): 'letter-not-a4-portrait-oriented', 'Your letter is not A4 portrait size', ( - 'You need to change the size or orientation of page 2.
Files must meet our ' - 'letter specification.' + 'You need to change the size or orientation of page 2. ' + 'Files must meet our letter specification.' ), ( - 'Validation failed because page 2 is not A4 portrait size.
' - 'Files must meet our ' - 'letter specification.' + 'Validation failed because page 2 is not A4 portrait size.' + 'Files must meet our letter specification.' ), ), ( 'content-outside-printable-area', 'Your content is outside the printable area', ( - 'You need to edit page 2.
Files must meet our ' - 'letter specification.' + 'You need to edit page 2.' + 'Files must meet our letter specification.' ), ( 'Validation failed because content is outside the printable area ' - 'on page 2.
Files must meet our ' - 'letter specification.' + 'on page 2.' + 'Files must meet our letter specification.' ), ), ( 'letter-too-long', 'Your letter is too long', ( - 'Letters must be 10 pages or less.
Your letter is 13 pages long.' + 'Letters must be 10 pages or less. ' + 'Your letter is 13 pages long.' ), ( - 'Validation failed because this letter is 13 pages long.
' + 'Validation failed because this letter is 13 pages long.' 'Letters must be 10 pages or less.' ), ), @@ -462,8 +459,22 @@ def test_get_letter_validation_error_for_known_errors( expected_content, expected_summary, ): + expected_letter_spec_url = ( + 'https://docs.notifications.service.gov.uk/' + 'documentation/images/notify-pdf-letter-spec-v2.4.pdf' + ) error = get_letter_validation_error(error_message, invalid_pages=[2], page_count=13) + detail = BeautifulSoup(error['detail'], 'html.parser') + summary = BeautifulSoup(error['summary'], 'html.parser') assert error['title'] == expected_title - assert expected_content in error['detail'] - assert error['summary'] == expected_summary + + assert detail.text == expected_content + if detail.select_one('a'): + assert detail.select_one('a')['href'] == expected_letter_spec_url + assert detail.select_one('a')['target'] == '_blank' + + assert summary.text == expected_summary + if summary.select_one('a'): + assert summary.select_one('a')['href'] == expected_letter_spec_url + assert summary.select_one('a')['target'] == '_blank' From 3762daad84a3908172781e29f60a68b7fdd7d6dc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 15 Jan 2020 10:56:14 +0000 Subject: [PATCH 6/9] Add a redirect for the letter specification This way we have a URL we can give people that always points to the latest version of the spec. And it makes our code more Flask-idiomatic to be using `url_for` to be generating a URL, rather than passing around a constant. --- app/main/views/index.py | 15 +++++++++------ app/main/views/uploads.py | 2 -- app/navigation.py | 4 ++++ app/templates/views/features/letters.html | 2 +- app/templates/views/uploads/choose-file.html | 2 +- app/utils.py | 20 ++++++++------------ tests/app/main/views/test_index.py | 18 ++++++++++++++++++ tests/app/test_utils.py | 10 ++++------ 8 files changed, 45 insertions(+), 28 deletions(-) diff --git a/app/main/views/index.py b/app/main/views/index.py index 7f21ac340..a910963e6 100644 --- a/app/main/views/index.py +++ b/app/main/views/index.py @@ -17,11 +17,7 @@ from app.main import main from app.main.forms import FieldWithNoneOption, SearchByNameForm from app.main.views.feedback import QUESTION_TICKET_TYPE from app.main.views.sub_navigation_dictionaries import features_nav, pricing_nav -from app.utils import ( - LETTER_SPECIFICATION_URL, - get_logo_cdn_domain, - user_is_logged_in, -) +from app.utils import get_logo_cdn_domain, user_is_logged_in @main.route('/') @@ -273,7 +269,6 @@ def features_sms(): def features_letters(): return render_template( 'views/features/letters.html', - letter_specification_url=LETTER_SPECIFICATION_URL, navigation_links=features_nav() ) @@ -348,3 +343,11 @@ def old_page_redirects(): 'main.old_integration_testing': 'main.integration_testing', } return redirect(url_for(redirects[request.endpoint]), code=301) + + +@main.route('/docs/notify-pdf-letter-spec-latest.pdf') +def letter_spec(): + return redirect( + 'https://docs.notifications.service.gov.uk' + '/documentation/images/notify-pdf-letter-spec-v2.4.pdf' + ) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index a403e76cc..fcbd64ef0 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -33,7 +33,6 @@ from app.s3_client.s3_letter_upload_client import ( ) from app.template_previews import TemplatePreview, sanitise_letter from app.utils import ( - LETTER_SPECIFICATION_URL, generate_next_dict, generate_previous_dict, get_letter_validation_error, @@ -66,7 +65,6 @@ def uploads(service_id): prev_page=prev_page, next_page=next_page, scheduled_jobs='', - letter_specification_url=LETTER_SPECIFICATION_URL, ) diff --git a/app/navigation.py b/app/navigation.py index 354483c7d..8cbd0ecab 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -208,6 +208,7 @@ class HeaderNavigation(Navigation): 'invite_org_user', 'invite_user', 'no_cookie.letter_branding_preview_image', + 'letter_spec', 'letter_template', 'link_service_to_organisation', 'manage_org_users', @@ -533,6 +534,7 @@ class MainNavigation(Navigation): 'no_cookie.letter_branding_preview_image', 'live_services', 'live_services_csv', + 'letter_spec', 'letter_template', 'message_status', 'manage_org_users', @@ -763,6 +765,7 @@ class CaseworkNavigation(Navigation): 'invite_user', 'no_cookie.letter_branding_preview_image', 'letter_branding', + 'letter_spec', 'letter_template', 'link_service_to_organisation', 'live_services', @@ -1049,6 +1052,7 @@ class OrgNavigation(Navigation): 'invite_user', 'letter_branding', 'no_cookie.letter_branding_preview_image', + 'letter_spec', 'letter_template', 'link_service_to_organisation', 'live_services', diff --git a/app/templates/views/features/letters.html b/app/templates/views/features/letters.html index 12bd5112f..1b498a05b 100644 --- a/app/templates/views/features/letters.html +++ b/app/templates/views/features/letters.html @@ -33,7 +33,7 @@

Upload your own letters

You can create reusable letter templates in Notify, or upload and send your own letters with the Notify API.

-

Use the letter specification document to help you set up your letter, save it as a PDF, then upload it to Notify.

+

Use the letter specification document to help you set up your letter, save it as a PDF, then upload it to Notify.

Read our API documentation for more information.

Pricing

diff --git a/app/templates/views/uploads/choose-file.html b/app/templates/views/uploads/choose-file.html index 0593157e8..70adde281 100644 --- a/app/templates/views/uploads/choose-file.html +++ b/app/templates/views/uploads/choose-file.html @@ -33,7 +33,7 @@ )}}

You can upload a single letter as a PDF.

-

Your file must meet our letter specification.

+

Your file must meet our letter specification.

To help you set up your letter you can download a Word document template.

diff --git a/app/utils.py b/app/utils.py index 5c89b0c85..56d711117 100644 --- a/app/utils.py +++ b/app/utils.py @@ -566,33 +566,27 @@ def get_letter_printing_statement(status, created_at): return 'Printed on {} at 5:30pm'.format(printed_date) -LETTER_SPECIFICATION_URL = ( - 'https://docs.notifications.service.gov.uk' - '/documentation/images/notify-pdf-letter-spec-v2.4.pdf' -) - - LETTER_VALIDATION_MESSAGES = { 'letter-not-a4-portrait-oriented': { 'title': 'Your letter is not A4 portrait size', 'detail': ( 'You need to change the size or orientation of {invalid_pages}.
' - f'Files must meet our letter specification.' + 'Files must meet our letter specification.' ), 'summary': ( 'Validation failed because {invalid_pages} {invalid_pages_are_or_is} not A4 portrait size.
' - f'Files must meet our letter specification.' + 'Files must meet our letter specification.' ), }, 'content-outside-printable-area': { 'title': 'Your content is outside the printable area', 'detail': ( 'You need to edit {invalid_pages}.
' - f'Files must meet our letter specification.' + 'Files must meet our letter specification.' ), 'summary': ( 'Validation failed because content is outside the printable area on {invalid_pages}.
' - f'Files must meet our letter specification.' + 'Files must meet our letter specification.' ), }, 'letter-too-long': { @@ -624,11 +618,11 @@ LETTER_VALIDATION_MESSAGES = { 'title': 'The address block is empty', 'detail': ( 'You need to add a recipient address.
' - f'Files must meet our letter specification.' + 'Files must meet our letter specification.' ), 'summary': ( 'Validation failed because the address block is empty.
' - f'Files must meet our letter specification.' + 'Files must meet our letter specification.' ), } } @@ -654,11 +648,13 @@ def get_letter_validation_error(validation_message, invalid_pages=None, page_cou invalid_pages=invalid_pages, invalid_pages_are_or_is=invalid_pages_are_or_is, page_count=page_count, + letter_spec=url_for('.letter_spec'), ), 'summary': LETTER_VALIDATION_MESSAGES[validation_message]['summary'].format( invalid_pages=invalid_pages, invalid_pages_are_or_is=invalid_pages_are_or_is, page_count=page_count, + letter_spec=url_for('.letter_spec'), ), } diff --git a/tests/app/main/views/test_index.py b/tests/app/main/views/test_index.py index 2b5232178..191820595 100644 --- a/tests/app/main/views/test_index.py +++ b/tests/app/main/views/test_index.py @@ -254,3 +254,21 @@ def test_letter_template_preview_headers( ) assert response.headers.get('X-Frame-Options') == 'SAMEORIGIN' + + +def test_letter_spec_redirect(client_request): + expected_url = ( + 'https://docs.notifications.service.gov.uk' + '/documentation/images/notify-pdf-letter-spec-v2.4.pdf' + ) + client_request.get( + 'main.letter_spec', + _expected_status=302, + _expected_redirect=expected_url, + ) + client_request.logout() + client_request.get( + 'main.letter_spec', + _expected_status=302, + _expected_redirect=expected_url, + ) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 635f4c2ca..9cb005208 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -5,6 +5,7 @@ from pathlib import Path import pytest from bs4 import BeautifulSoup +from flask import url_for from freezegun import freeze_time from app import format_datetime_relative @@ -454,15 +455,12 @@ def test_get_letter_validation_error_for_unknown_error(): ), ]) def test_get_letter_validation_error_for_known_errors( + client_request, error_message, expected_title, expected_content, expected_summary, ): - expected_letter_spec_url = ( - 'https://docs.notifications.service.gov.uk/' - 'documentation/images/notify-pdf-letter-spec-v2.4.pdf' - ) error = get_letter_validation_error(error_message, invalid_pages=[2], page_count=13) detail = BeautifulSoup(error['detail'], 'html.parser') summary = BeautifulSoup(error['summary'], 'html.parser') @@ -471,10 +469,10 @@ def test_get_letter_validation_error_for_known_errors( assert detail.text == expected_content if detail.select_one('a'): - assert detail.select_one('a')['href'] == expected_letter_spec_url + assert detail.select_one('a')['href'] == url_for('.letter_spec') assert detail.select_one('a')['target'] == '_blank' assert summary.text == expected_summary if summary.select_one('a'): - assert summary.select_one('a')['href'] == expected_letter_spec_url + assert summary.select_one('a')['href'] == url_for('.letter_spec') assert summary.select_one('a')['target'] == '_blank' From bc7deebcc7f8530c7061f9d7a56a3dd3789addce Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 20 Jan 2020 15:46:03 +0000 Subject: [PATCH 7/9] Split test in two for readability --- tests/app/main/views/test_index.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/app/main/views/test_index.py b/tests/app/main/views/test_index.py index 191820595..38c12b34d 100644 --- a/tests/app/main/views/test_index.py +++ b/tests/app/main/views/test_index.py @@ -257,18 +257,23 @@ def test_letter_template_preview_headers( def test_letter_spec_redirect(client_request): - expected_url = ( - 'https://docs.notifications.service.gov.uk' - '/documentation/images/notify-pdf-letter-spec-v2.4.pdf' - ) client_request.get( 'main.letter_spec', _expected_status=302, - _expected_redirect=expected_url, + _expected_redirect=( + 'https://docs.notifications.service.gov.uk' + '/documentation/images/notify-pdf-letter-spec-v2.4.pdf' + ), ) + + +def test_letter_spec_redirect_with_non_logged_in_user(client_request): client_request.logout() client_request.get( 'main.letter_spec', _expected_status=302, - _expected_redirect=expected_url, + _expected_redirect=( + 'https://docs.notifications.service.gov.uk' + '/documentation/images/notify-pdf-letter-spec-v2.4.pdf' + ), ) From 1fc0f5854132e1f5f35e233a9220bc35b4fec36e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 20 Jan 2020 15:50:16 +0000 Subject: [PATCH 8/9] Add test for plural form of error message --- tests/app/test_utils.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 9cb005208..ee177e6b3 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -415,9 +415,10 @@ def test_get_letter_validation_error_for_unknown_error(): } -@pytest.mark.parametrize('error_message, expected_title, expected_content, expected_summary', [ +@pytest.mark.parametrize('error_message, invalid_pages, expected_title, expected_content, expected_summary', [ ( 'letter-not-a4-portrait-oriented', + [2], 'Your letter is not A4 portrait size', ( 'You need to change the size or orientation of page 2. ' @@ -428,8 +429,22 @@ def test_get_letter_validation_error_for_unknown_error(): 'Files must meet our letter specification.' ), ), + ( + 'letter-not-a4-portrait-oriented', + [2, 3, 4], + 'Your letter is not A4 portrait size', + ( + 'You need to change the size or orientation of pages 2, 3 and 4. ' + 'Files must meet our letter specification.' + ), + ( + 'Validation failed because pages 2, 3 and 4 are not A4 portrait size.' + 'Files must meet our letter specification.' + ), + ), ( 'content-outside-printable-area', + [2], 'Your content is outside the printable area', ( 'You need to edit page 2.' @@ -443,6 +458,7 @@ def test_get_letter_validation_error_for_unknown_error(): ), ( 'letter-too-long', + [2], 'Your letter is too long', ( 'Letters must be 10 pages or less. ' @@ -457,11 +473,12 @@ def test_get_letter_validation_error_for_unknown_error(): def test_get_letter_validation_error_for_known_errors( client_request, error_message, + invalid_pages, expected_title, expected_content, expected_summary, ): - error = get_letter_validation_error(error_message, invalid_pages=[2], page_count=13) + error = get_letter_validation_error(error_message, invalid_pages=invalid_pages, page_count=13) detail = BeautifulSoup(error['detail'], 'html.parser') summary = BeautifulSoup(error['summary'], 'html.parser') From 34f209a08ba5e8b2c394d86bc538fc9d936a0648 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 20 Jan 2020 15:54:07 +0000 Subject: [PATCH 9/9] Fix mixed-up error messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The too many pages error was being returned when the file couldn’t be read. This commit corrects the error message, and adds a test to make sure this case is covered. --- app/utils.py | 4 ++-- tests/app/test_utils.py | 13 +++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/app/utils.py b/app/utils.py index 56d711117..3f8a3b599 100644 --- a/app/utils.py +++ b/app/utils.py @@ -610,8 +610,8 @@ LETTER_VALIDATION_MESSAGES = { '
Save a new copy of your file and try again.' ), 'summary': ( - 'Letters must be 10 pages or less.
' - 'This letter is {page_count} pages long.' + 'Validation failed because Notify cannot read this PDF.
' + 'Save a new copy of your file and try again.' ), }, 'address-is-empty': { diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index ee177e6b3..a8dc0755b 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -469,6 +469,19 @@ def test_get_letter_validation_error_for_unknown_error(): 'Letters must be 10 pages or less.' ), ), + ( + 'unable-to-read-the-file', + [2], + 'There’s a problem with your file', + ( + 'Notify cannot read this PDF.' + 'Save a new copy of your file and try again.' + ), + ( + 'Validation failed because Notify cannot read this PDF.' + 'Save a new copy of your file and try again.' + ), + ), ]) def test_get_letter_validation_error_for_known_errors( client_request,