From e3670de6c408551b89c3c6e183972976cb5946d2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 10 Jan 2020 17:00:42 +0000 Subject: [PATCH 01/44] 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 02/44] 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 03/44] =?UTF-8?q?Don=E2=80=99t=20repeat=20the=20letter=20s?= =?UTF-8?q?pec=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 04/44] 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 05/44] 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 06/44] 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 c4818eb7f228aeb1ddbc00aa99be9e59da0bdb63 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 16 Jan 2020 15:57:51 +0000 Subject: [PATCH 07/44] Rename property on ModelLists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The property doesn’t represent the whole client, but just one method on it. So this commit renames the property to better describe what it is designed to store. --- app/models/__init__.py | 4 +-- app/models/event.py | 6 ++--- app/models/job.py | 10 +++---- app/models/organisation.py | 2 +- app/models/user.py | 10 +++---- tests/__init__.py | 2 +- .../views/organisations/test_organisation.py | 6 ++--- tests/app/main/views/test_accept_invite.py | 4 +-- tests/app/main/views/test_manage_users.py | 26 +++++++++---------- tests/app/main/views/test_service_settings.py | 4 +-- tests/app/main/views/test_template_folders.py | 14 +++++----- tests/conftest.py | 14 +++++----- 12 files changed, 51 insertions(+), 51 deletions(-) diff --git a/app/models/__init__.py b/app/models/__init__.py index 91fed748f..4f8850e9d 100644 --- a/app/models/__init__.py +++ b/app/models/__init__.py @@ -55,7 +55,7 @@ class ModelList(ABC, Sequence): @property @abstractmethod - def client(self): + def client_method(self): pass @property @@ -64,7 +64,7 @@ class ModelList(ABC, Sequence): pass def __init__(self, *args): - self.items = self.client(*args) + self.items = self.client_method(*args) def __getitem__(self, index): return self.model(self.items[index]) diff --git a/app/models/event.py b/app/models/event.py index 43dfba627..de3d635cd 100644 --- a/app/models/event.py +++ b/app/models/event.py @@ -157,12 +157,12 @@ class APIKeyEvent(Event): class APIKeyEvents(ModelList): model = APIKeyEvent - client = service_api_client.get_service_api_key_history + client_method = service_api_client.get_service_api_key_history class ServiceEvents(ModelList): - client = service_api_client.get_service_service_history + client_method = service_api_client.get_service_service_history @property def model(self): @@ -187,5 +187,5 @@ class ServiceEvents(ModelList): def __init__(self, service_id): self.items = [ - event for event in self.splat(self.client(service_id)) if event.relevant + event for event in self.splat(self.client_method(service_id)) if event.relevant ] diff --git a/app/models/job.py b/app/models/job.py index 2b1eb4597..71362b2ab 100644 --- a/app/models/job.py +++ b/app/models/job.py @@ -179,28 +179,28 @@ class Job(JSONModel): class ImmediateJobs(ModelList): - client = job_api_client.get_immediate_jobs + client_method = job_api_client.get_immediate_jobs model = Job class ScheduledJobs(ImmediateJobs): - client = job_api_client.get_scheduled_jobs + client_method = job_api_client.get_scheduled_jobs class PaginatedJobs(ImmediateJobs): - client = job_api_client.get_page_of_jobs + client_method = job_api_client.get_page_of_jobs def __init__(self, service_id, page=None): try: self.current_page = int(page) except TypeError: self.current_page = 1 - response = self.client(service_id, page=self.current_page) + response = self.client_method(service_id, page=self.current_page) self.items = response['data'] self.prev_page = response.get('links', {}).get('prev', None) self.next_page = response.get('links', {}).get('next', None) class PaginatedUploads(PaginatedJobs): - client = job_api_client.get_uploads + client_method = job_api_client.get_uploads diff --git a/app/models/organisation.py b/app/models/organisation.py index 966dce9bb..327be5670 100644 --- a/app/models/organisation.py +++ b/app/models/organisation.py @@ -200,5 +200,5 @@ class Organisation(JSONModel): class Organisations(ModelList): - client = organisations_client.get_organisations + client_method = organisations_client.get_organisations model = Organisation diff --git a/app/models/user.py b/app/models/user.py index 61674ce30..7dab6c8cc 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -604,7 +604,7 @@ class AnonymousUser(AnonymousUserMixin): class Users(ModelList): - client = user_api_client.get_users_for_service + client_method = user_api_client.get_users_for_service model = User def __init__(self, service_id): @@ -618,21 +618,21 @@ class Users(ModelList): class OrganisationUsers(Users): - client = user_api_client.get_users_for_organisation + client_method = user_api_client.get_users_for_organisation class InvitedUsers(Users): - client = invite_api_client.get_invites_for_service + client_method = invite_api_client.get_invites_for_service model = InvitedUser def __init__(self, service_id): self.items = [ - user for user in self.client(service_id) + user for user in self.client_method(service_id) if user['status'] != 'accepted' ] class OrganisationInvitedUsers(InvitedUsers): - client = org_invite_api_client.get_invites_for_organisation + client_method = org_invite_api_client.get_invites_for_organisation model = InvitedOrgUser diff --git a/tests/__init__.py b/tests/__init__.py index 4348d7c3a..3a3554cb4 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -541,7 +541,7 @@ def validate_route_permission(mocker, mocker.patch('app.user_api_client.get_user', return_value=usr) mocker.patch('app.user_api_client.get_user_by_email', return_value=usr) mocker.patch('app.service_api_client.get_service', return_value={'data': service}) - mocker.patch('app.models.user.Users.client', return_value=[usr]) + mocker.patch('app.models.user.Users.client_method', return_value=[usr]) mocker.patch('app.job_api_client.has_jobs', return_value=False) with app_.test_request_context(): with app_.test_client() as client: diff --git a/tests/app/main/views/organisations/test_organisation.py b/tests/app/main/views/organisations/test_organisation.py index dad10f81b..39bd5f0cb 100644 --- a/tests/app/main/views/organisations/test_organisation.py +++ b/tests/app/main/views/organisations/test_organisation.py @@ -27,7 +27,7 @@ def test_organisation_page_shows_all_organisations( ] get_organisations = mocker.patch( - 'app.models.organisation.Organisations.client', return_value=orgs + 'app.models.organisation.Organisations.client_method', return_value=orgs ) response = platform_admin_client.get( url_for('.organisations') @@ -232,7 +232,7 @@ def test_nhs_local_can_create_own_organisations( ): mocker.patch('app.organisations_client.get_service_organisation', return_value=organisation) mocker.patch( - 'app.models.organisation.Organisations.client', + 'app.models.organisation.Organisations.client_method', return_value=[ organisation_json('t1', 'Trust 1', organisation_type='nhs_local'), organisation_json('t2', 'Trust 2', organisation_type='nhs_local'), @@ -366,7 +366,7 @@ def test_nhs_local_assigns_to_selected_organisation( ): mocker.patch('app.organisations_client.get_service_organisation', return_value=None) mocker.patch( - 'app.models.organisation.Organisations.client', + 'app.models.organisation.Organisations.client_method', return_value=[ organisation_json(ORGANISATION_ID, 'Trust 1', organisation_type='nhs_local'), ], diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 17a49328c..cbd280ffe 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -180,7 +180,7 @@ def test_existing_user_of_service_get_redirected_to_signin( ): sample_invite['email_address'] = api_user_active['email_address'] mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) - mocker.patch('app.models.user.Users.client', return_value=[api_user_active]) + mocker.patch('app.models.user.Users.client_method', return_value=[api_user_active]) response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken'), follow_redirects=True) assert response.status_code == 200 @@ -432,7 +432,7 @@ def test_accept_invite_does_not_treat_email_addresses_as_case_sensitive( # the email address of api_user_active is 'test@user.gov.uk' sample_invite['email_address'] = 'TEST@user.gov.uk' mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) - mocker.patch('app.models.user.Users.client', return_value=[api_user_active]) + mocker.patch('app.models.user.Users.client_method', return_value=[api_user_active]) client_request.get( 'main.accept_invite', diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 07f3ec1b6..c6becbafe 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -135,7 +135,7 @@ def test_should_show_overview_page( other_user['id'] = 'zzzzzzzz-zzzz-zzzz-zzzz-zzzzzzzzzzzz' mocker.patch('app.user_api_client.get_user', return_value=current_user) - mock_get_users = mocker.patch('app.models.user.Users.client', return_value=[ + mock_get_users = mocker.patch('app.models.user.Users.client_method', return_value=[ current_user, other_user, ]) @@ -166,7 +166,7 @@ def test_should_show_caseworker_on_overview_page( other_user['email_address'] = 'zzzzzzz@example.gov.uk' mocker.patch('app.user_api_client.get_user', return_value=current_user) - mocker.patch('app.models.user.Users.client', return_value=[ + mocker.patch('app.models.user.Users.client_method', return_value=[ current_user, other_user, ]) @@ -699,8 +699,8 @@ def test_invite_user( sample_invite['email_address'] = 'test@example.gov.uk' assert is_gov_user(email_address) == gov_user - mocker.patch('app.models.user.InvitedUsers.client', return_value=[sample_invite]) - mocker.patch('app.models.user.Users.client', return_value=[active_user_with_permissions]) + mocker.patch('app.models.user.InvitedUsers.client_method', return_value=[sample_invite]) + mocker.patch('app.models.user.Users.client_method', return_value=[active_user_with_permissions]) mocker.patch('app.invite_api_client.create_invite', return_value=sample_invite) page = client_request.post( 'main.invite_user', @@ -753,8 +753,8 @@ def test_invite_user_with_email_auth_service( sample_invite['email_address'] = 'test@example.gov.uk' assert is_gov_user(email_address) is gov_user - mocker.patch('app.models.user.InvitedUsers.client', return_value=[sample_invite]) - mocker.patch('app.models.user.Users.client', return_value=[active_user_with_permissions]) + mocker.patch('app.models.user.InvitedUsers.client_method', return_value=[sample_invite]) + mocker.patch('app.models.user.Users.client_method', return_value=[active_user_with_permissions]) mocker.patch('app.invite_api_client.create_invite', return_value=sample_invite) page = client_request.post( @@ -855,8 +855,8 @@ def test_manage_users_shows_invited_user( expected_text, ): sample_invite['status'] = invite_status - mocker.patch('app.models.user.InvitedUsers.client', return_value=[sample_invite]) - mocker.patch('app.models.user.Users.client', return_value=[active_user_with_permissions]) + mocker.patch('app.models.user.InvitedUsers.client_method', return_value=[sample_invite]) + mocker.patch('app.models.user.Users.client_method', return_value=[active_user_with_permissions]) page = client_request.get('main.manage_users', service_id=SERVICE_ONE_ID) assert page.h1.string.strip() == 'Team members' @@ -873,8 +873,8 @@ def test_manage_users_does_not_show_accepted_invite( invited_user_id = uuid.uuid4() sample_invite['id'] = invited_user_id sample_invite['status'] = 'accepted' - mocker.patch('app.models.user.InvitedUsers.client', return_value=[sample_invite]) - mocker.patch('app.models.user.Users.client', return_value=[active_user_with_permissions]) + mocker.patch('app.models.user.InvitedUsers.client_method', return_value=[sample_invite]) + mocker.patch('app.models.user.Users.client_method', return_value=[active_user_with_permissions]) page = client_request.get('main.manage_users', service_id=SERVICE_ONE_ID) @@ -1018,7 +1018,7 @@ def test_can_invite_user_as_platform_admin( mock_get_template_folders, mocker, ): - mocker.patch('app.models.user.Users.client', return_value=[active_user_with_permissions]) + mocker.patch('app.models.user.Users.client_method', return_value=[active_user_with_permissions]) page = client_request.get( 'main.manage_users', @@ -1252,7 +1252,7 @@ def test_confirm_edit_user_email_changes_user_email( # We want active_user_with_permissions (the current user) to update the email address for api_user_active # By default both users would have the same id, so we change the id of api_user_active api_user_active['id'] = str(uuid.uuid4()) - mocker.patch('app.models.user.Users.client', return_value=[api_user_active, active_user_with_permissions]) + mocker.patch('app.models.user.Users.client_method', return_value=[api_user_active, active_user_with_permissions]) # get_user gets called twice - first to check if current user can see the page, then to see if the team member # whose email address we're changing belongs to the service mocker.patch('app.user_api_client.get_user', @@ -1468,7 +1468,7 @@ def test_confirm_edit_user_mobile_number_changes_user_mobile_number( # By default both users would have the same id, so we change the id of api_user_active api_user_active['id'] = str(uuid.uuid4()) - mocker.patch('app.models.user.Users.client', return_value=[api_user_active, active_user_with_permissions]) + mocker.patch('app.models.user.Users.client_method', return_value=[api_user_active, active_user_with_permissions]) # get_user gets called twice - first to check if current user can see the page, then to see if the team member # whose mobile number we're changing belongs to the service mocker.patch('app.user_api_client.get_user', diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index de9e6316f..0bc576e95 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -763,7 +763,7 @@ def test_should_check_for_sending_things_right( }.get(template_type) active_user_with_permissions, mock_get_users = mocker.patch( - 'app.models.user.Users.client', + 'app.models.user.Users.client_method', return_value=( [active_user_with_permissions] * count_of_users_with_manage_service + [active_user_no_settings_permission] @@ -783,7 +783,7 @@ def test_should_check_for_sending_things_right( invite_two['permissions'] = 'view_activity' mock_get_invites = mocker.patch( - 'app.models.user.InvitedUsers.client', + 'app.models.user.InvitedUsers.client_method', return_value=( ([invite_one] * count_of_invites_with_manage_service) + [invite_two] diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 0e317fc15..5e623074b 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -480,7 +480,7 @@ def test_get_manage_folder_page( _folder('folder_two', folder_id, None, [active_user_with_permissions['id']]), ] mocker.patch( - 'app.models.user.Users.client', + 'app.models.user.Users.client_method', return_value=[active_user_with_permissions], ) page = client_request.get( @@ -514,7 +514,7 @@ def test_get_manage_folder_viewing_permissions_for_users( _folder('folder_two', folder_id, None, [active_user_with_permissions['id'], team_member_2['id']]), ] mocker.patch( - 'app.models.user.Users.client', + 'app.models.user.Users.client_method', return_value=[active_user_with_permissions, team_member, team_member_2], ) @@ -566,7 +566,7 @@ def test_get_manage_folder_viewing_permissions_for_users_not_visible_when_no_man ]}, ] mocker.patch( - 'app.models.user.Users.client', + 'app.models.user.Users.client_method', return_value=[active_user_with_permissions, team_member, team_member_2], ) @@ -600,7 +600,7 @@ def test_get_manage_folder_viewing_permissions_for_users_not_visible_for_service ]}, ] mocker.patch( - 'app.models.user.Users.client', + 'app.models.user.Users.client_method', return_value=[active_user_with_permissions], ) @@ -712,7 +712,7 @@ def test_rename_folder(client_request, active_user_with_permissions, service_one _folder('folder_two', folder_id, None, [active_user_with_permissions['id']]) ] mocker.patch( - 'app.models.user.Users.client', + 'app.models.user.Users.client_method', return_value=[active_user_with_permissions], ) @@ -745,7 +745,7 @@ def test_manage_folder_users( _folder('folder_two', folder_id, None, [active_user_with_permissions['id'], team_member['id']]) ] mocker.patch( - 'app.models.user.Users.client', + 'app.models.user.Users.client_method', return_value=[active_user_with_permissions, team_member], ) @@ -788,7 +788,7 @@ def test_manage_folder_users_doesnt_change_permissions_current_user_cannot_manag ]} ] mocker.patch( - 'app.models.user.Users.client', + 'app.models.user.Users.client_method', return_value=[active_user_with_permissions, team_member], ) diff --git a/tests/conftest.py b/tests/conftest.py index f6043b141..c9d87768f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1752,7 +1752,7 @@ def mock_get_uploads(mocker, api_user_active): } } # Why is mocking on the model needed? - return mocker.patch('app.models.job.PaginatedUploads.client', side_effect=_get_uploads) + return mocker.patch('app.models.job.PaginatedUploads.client_method', side_effect=_get_uploads) @pytest.fixture(scope='function') @@ -2013,7 +2013,7 @@ def mock_get_users_by_service(mocker): # You shouldn’t be calling the user API client directly, so it’s the # instance on the model that’s mocked here - return mocker.patch('app.models.user.Users.client', side_effect=_get_users_for_service) + return mocker.patch('app.models.user.Users.client_method', side_effect=_get_users_for_service) @pytest.fixture(scope='function') @@ -2082,7 +2082,7 @@ def mock_get_invites_for_service(mocker, service_one, sample_invite): data.append(invite) return data - return mocker.patch('app.models.user.InvitedUsers.client', side_effect=_get_invites) + return mocker.patch('app.models.user.InvitedUsers.client_method', side_effect=_get_invites) @pytest.fixture(scope='function') @@ -2101,7 +2101,7 @@ def mock_get_invites_without_manage_permission(mocker, service_one, sample_invit status='pending', )] - return mocker.patch('app.models.user.InvitedUsers.client', side_effect=_get_invites) + return mocker.patch('app.models.user.InvitedUsers.client_method', side_effect=_get_invites) @pytest.fixture(scope='function') @@ -2922,7 +2922,7 @@ def mock_get_organisations(mocker): ] mocker.patch( - 'app.models.organisation.Organisations.client', + 'app.models.organisation.Organisations.client_method', side_effect=_get_organisations, ) @@ -3037,7 +3037,7 @@ def mock_get_users_for_organisation(mocker): ] return mocker.patch( - 'app.models.user.OrganisationUsers.client', + 'app.models.user.OrganisationUsers.client_method', side_effect=_get_users_for_organisation ) @@ -3050,7 +3050,7 @@ def mock_get_invited_users_for_organisation(mocker, sample_org_invite): ] return mocker.patch( - 'app.models.user.OrganisationInvitedUsers.client', + 'app.models.user.OrganisationInvitedUsers.client_method', side_effect=_get_invited_invited_users_for_organisation ) From 67b3619229eb0ff02f927d79a6d96e0e5525c0dc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 16 Jan 2020 16:12:02 +0000 Subject: [PATCH 08/44] Remove redundant redefinition of __init__ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At some point we made the __init__ method on the base class accept `*args` as an argument, so we don’t need to define our own method here. --- app/models/user.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/models/user.py b/app/models/user.py index 7dab6c8cc..bbe89fa2b 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -607,9 +607,6 @@ class Users(ModelList): client_method = user_api_client.get_users_for_service model = User - def __init__(self, service_id): - self.items = self.client(service_id) - def get_name_from_id(self, id): for user in self: if user.id == id: From a8d6df9b040bad6005bc00a064c7668ab500f735 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 13 Dec 2019 22:07:43 +0000 Subject: [PATCH 09/44] Wrap analytics code in GOVUK interface Wraps our analytics code in a stripped down version of GOVUK.Analytics to allow us to plug in the GOVUK code for consent. --- app/assets/javascripts/analytics/analytics.js | 39 +++++++++++++++++++ app/assets/javascripts/analytics/init.js | 39 +++++++++++++++++++ app/assets/javascripts/main.js | 2 + 3 files changed, 80 insertions(+) create mode 100644 app/assets/javascripts/analytics/analytics.js create mode 100644 app/assets/javascripts/analytics/init.js diff --git a/app/assets/javascripts/analytics/analytics.js b/app/assets/javascripts/analytics/analytics.js new file mode 100644 index 000000000..580ee382a --- /dev/null +++ b/app/assets/javascripts/analytics/analytics.js @@ -0,0 +1,39 @@ +(function (window) { + "use strict"; + + window.GOVUK = window.GOVUK || {}; + + // Stripped-down wrapper for Google Analytics, based on: + // https://github.com/alphagov/static/blob/master/app/assets/javascripts/analytics_toolkit/analytics.js + const Analytics = function (config) { + window.ga('create', config.trackingId, config.cookieDomain); + + window.ga('set', 'anonymizeIp', config.anonymizeIp); + window.ga('set', 'displayFeaturesTask', config.displayFeaturesTask); + window.ga('set', 'transport', config.transport); + + }; + + Analytics.load = function () { + /* jshint ignore:start */ + (function(i, s, o, g, r, a, m){ i['GoogleAnalyticsObject'] = r; i[r] = i[r] || function () { + (i[r].q = i[r].q || []).push(arguments) }, i[r].l = 1 * new Date(); a = s.createElement(o), + m = s.getElementsByTagName(o)[0]; a.async = 1; a.src = g; m.parentNode.insertBefore(a,m) + })(window,document,'script','//www.google-analytics.com/analytics.js','ga'); + /* jshint ignore:end */ + + }; + + Analytics.prototype.trackPageview = function (path, title, options) { + + // strip UUIDs + const page = (window.location.pathname + window.location.search).replace( + /[a-f0-9]{8}-?[a-f0-9]{4}-?4[a-f0-9]{3}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12}/g, '…' + ); + window.ga('send', 'pageview', page); + + }; + + window.GOVUK.Analytics = Analytics; + +})(window); diff --git a/app/assets/javascripts/analytics/init.js b/app/assets/javascripts/analytics/init.js new file mode 100644 index 000000000..ea0d43454 --- /dev/null +++ b/app/assets/javascripts/analytics/init.js @@ -0,0 +1,39 @@ +(function (window) { + "use strict"; + + window.GOVUK = window.GOVUK || {}; + + const trackingId = 'UA-26179049-1'; + + // Disable analytics by default + window[`ga-disable-${trackingId}`] = true; + + const initAnalytics = function () { + + // guard against being called more than once + if (!('analytics' in window.GOVUK)) { + + window[`ga-disable-${trackingId}`] = false; + + // Load Google Analytics libraries + window.GOVUK.Analytics.load(); + + // Configure profiles and make interface public + // for custom dimensions, virtual pageviews and events + window.GOVUK.analytics = new GOVUK.Analytics({ + trackingId: trackingId, + cookieDomain: 'auto', + anonymizeIp: true, + displayFeaturesTask: null, + transport: 'beacon' + }); + + // Track initial pageview + window.GOVUK.analytics.trackPageview(); + + } + + }; + + window.GOVUK.initAnalytics = initAnalytics; +})(window); diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index 9c45e0508..2c3b43083 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -2,6 +2,8 @@ window.GOVUK.Frontend.initAll(); $(() => GOVUK.addCookieMessage()); +window.GOVUK.initAnalytics(); + $(() => $("time.timeago").timeago()); $(() => GOVUK.stickAtTopWhenScrolling.init()); From 0ecbff6a8be5021e0ab7860ed4ea86935d13dd0e Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Sun, 15 Dec 2019 19:36:29 +0000 Subject: [PATCH 10/44] Add consent tracking to cookie functions Taken from GOVUK components: https://github.com/alphagov/govuk_publishing_components/blob/master/app/assets/javascripts/govuk_publishing_components/lib/cookie-functions.js Also includes: - make new cookie functions handle notify domains - addition of hasConsentFor function to allow easy checking of consent for categories of cookie --- app/assets/javascripts/consent.js | 15 ++ .../javascripts/govuk/cookie-functions.js | 157 ++++++++++++++---- 2 files changed, 144 insertions(+), 28 deletions(-) create mode 100644 app/assets/javascripts/consent.js diff --git a/app/assets/javascripts/consent.js b/app/assets/javascripts/consent.js new file mode 100644 index 000000000..e5953974d --- /dev/null +++ b/app/assets/javascripts/consent.js @@ -0,0 +1,15 @@ +(function (window) { + "use strict"; + + function hasConsentFor (cookieCategory) { + const consentCookie = window.GOVUK.getConsentCookie(); + + if (consentCookie === null) { return false; } + + if (!(cookieCategory in consentCookie)) { return false; } + + return consentCookie[cookieCategory]; + } + + window.GOVUK.hasConsentFor = hasConsentFor; +})(window); diff --git a/app/assets/javascripts/govuk/cookie-functions.js b/app/assets/javascripts/govuk/cookie-functions.js index fdabe48e3..5fa15bee7 100644 --- a/app/assets/javascripts/govuk/cookie-functions.js +++ b/app/assets/javascripts/govuk/cookie-functions.js @@ -1,8 +1,17 @@ -(function () { - "use strict"; +// used by the cookie banner component - var root = this; - if(typeof root.GOVUK === 'undefined') { root.GOVUK = {}; } +(function (root) { + 'use strict'; + window.GOVUK = window.GOVUK || {}; + + var DEFAULT_COOKIE_CONSENT = { + 'analytics': false + }; + + var COOKIE_CATEGORIES = { + '_ga': 'analytics', + '_gid': 'analytics' + }; /* Cookie methods @@ -19,38 +28,129 @@ Deleting a cookie: GOVUK.cookie('hobnob', null); */ - GOVUK.cookie = function (name, value, options) { - if(typeof value !== 'undefined'){ - if(value === false || value === null) { - return GOVUK.setCookie(name, '', { days: -1 }); + window.GOVUK.cookie = function (name, value, options) { + if (typeof value !== 'undefined') { + if (value === false || value === null) { + return window.GOVUK.setCookie(name, '', { days: -1 }); } else { - return GOVUK.setCookie(name, value, options); + // Default expiry date of 30 days + if (typeof options === 'undefined') { + options = { days: 30 }; + } + return window.GOVUK.setCookie(name, value, options); } } else { - return GOVUK.getCookie(name); + return window.GOVUK.getCookie(name); } }; - GOVUK.setCookie = function (name, value, options) { - if(typeof options === 'undefined') { - options = {}; + + window.GOVUK.getConsentCookie = function () { + var consentCookie = window.GOVUK.cookie('cookies_policy'); + var consentCookieObj; + + if (consentCookie) { + try { + consentCookieObj = JSON.parse(consentCookie); + } catch (err) { + return null; + } + + if (typeof consentCookieObj !== 'object' && consentCookieObj !== null) { + consentCookieObj = JSON.parse(consentCookieObj); + } + } else { + return null; } - var cookieString = name + "=" + value + "; path=/"; - if (options.days) { - var date = new Date(); - date.setTime(date.getTime() + (options.days * 24 * 60 * 60 * 1000)); - cookieString = cookieString + "; expires=" + date.toGMTString(); - } - if (document.location.protocol == 'https:'){ - cookieString = cookieString + "; Secure"; - } - document.cookie = cookieString; + + return consentCookieObj; }; - GOVUK.getCookie = function (name) { - var nameEQ = name + "="; + + window.GOVUK.setConsentCookie = function (options) { + var cookieConsent = window.GOVUK.getConsentCookie(); + + if (!cookieConsent) { + cookieConsent = JSON.parse(JSON.stringify(DEFAULT_COOKIE_CONSENT)); + } + + for (var cookieType in options) { + cookieConsent[cookieType] = options[cookieType]; + + // Delete cookies of that type if consent being set to false + if (!options[cookieType]) { + for (var cookie in COOKIE_CATEGORIES) { + if (COOKIE_CATEGORIES[cookie] === cookieType) { + window.GOVUK.cookie(cookie, null); + + if (window.GOVUK.cookie(cookie)) { + document.cookie = cookie + '=;expires=' + new Date() + ';domain=' + window.location.hostname.replace(/^www\./, '.') + ';path=/'; + } + } + } + } + } + + window.GOVUK.setCookie('cookies_policy', JSON.stringify(cookieConsent), { days: 365 }); + }; + + window.GOVUK.checkConsentCookieCategory = function (cookieName, cookieCategory) { + var currentConsentCookie = window.GOVUK.getConsentCookie(); + + // If the consent cookie doesn't exist, but the cookie is in our known list, return true + if (!currentConsentCookie && COOKIE_CATEGORIES[cookieName]) { + return true; + } + + currentConsentCookie = window.GOVUK.getConsentCookie(); + + // Sometimes currentConsentCookie is malformed in some of the tests, so we need to handle these + try { + return currentConsentCookie[cookieCategory]; + } catch (e) { + console.error(e); + return false; + } + }; + + window.GOVUK.checkConsentCookie = function (cookieName, cookieValue) { + // If we're setting the consent cookie OR deleting a cookie, allow by default + if (cookieName === 'cookies_policy' || (cookieValue === null || cookieValue === false)) { + return true; + } + + if (COOKIE_CATEGORIES[cookieName]) { + var cookieCategory = COOKIE_CATEGORIES[cookieName]; + + return window.GOVUK.checkConsentCookieCategory(cookieName, cookieCategory); + } else { + // Deny the cookie if it is not known to us + return false; + } + }; + + window.GOVUK.setCookie = function (name, value, options) { + if (window.GOVUK.checkConsentCookie(name, value)) { + if (typeof options === 'undefined') { + options = {}; + } + var cookieString = name + '=' + value + '; path=/'; + if (options.days) { + var date = new Date(); + date.setTime(date.getTime() + (options.days * 24 * 60 * 60 * 1000)); + cookieString = cookieString + '; expires=' + date.toGMTString(); + } + if (document.location.protocol === 'https:') { + cookieString = cookieString + '; Secure'; + } + document.cookie = cookieString; + } + }; + + window.GOVUK.getCookie = function (name) { + var nameEQ = name + '='; var cookies = document.cookie.split(';'); - for(var i = 0, len = cookies.length; i < len; i++) { + for (var i = 0, len = cookies.length; i < len; i++) { var cookie = cookies[i]; - while (cookie.charAt(0) == ' ') { + while (cookie.charAt(0) === ' ') { cookie = cookie.substring(1, cookie.length); } if (cookie.indexOf(nameEQ) === 0) { @@ -59,4 +159,5 @@ } return null; }; -}).call(this); +}(window)); + From 181adc9940bb63a8a814ea85809b32ffb5c204dc Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 2 Jan 2020 14:26:57 +0000 Subject: [PATCH 11/44] On page load, call analytics based on consent --- app/assets/javascripts/main.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index 2c3b43083..354a1daea 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -2,7 +2,9 @@ window.GOVUK.Frontend.initAll(); $(() => GOVUK.addCookieMessage()); -window.GOVUK.initAnalytics(); +if (window.GOVUK.hasConsentFor('analytics')) { + window.GOVUK.initAnalytics(); +} $(() => $("time.timeago").timeago()); From 6ef77cfff36091f345b55b7619d65c1540f7bf0a Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Sun, 15 Dec 2019 19:38:49 +0000 Subject: [PATCH 12/44] Add new analytics code to frontend build --- gulpfile.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/gulpfile.js b/gulpfile.js index 257aef277..8e8893a64 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -142,6 +142,9 @@ const javascripts = () => { paths.toolkit + 'javascripts/govuk/modules.js', paths.toolkit + 'javascripts/govuk/show-hide-content.js', paths.src + 'javascripts/govuk/cookie-functions.js', + paths.src + 'javascripts/consent.js', + paths.src + 'javascripts/analytics/analytics.js', + paths.src + 'javascripts/analytics/init.js', paths.src + 'javascripts/cookieMessage.js', paths.src + 'javascripts/stick-to-window-when-scrolling.js', paths.src + 'javascripts/apiKey.js', @@ -160,7 +163,7 @@ const javascripts = () => { paths.src + 'javascripts/templateFolderForm.js', paths.src + 'javascripts/collapsibleCheckboxes.js', paths.src + 'javascripts/radioSlider.js', - paths.src + 'javascripts/main.js' + paths.src + 'javascripts/main.js', ]) .pipe(plugins.prettyerror()) .pipe(plugins.babel({ From fa7104d6c8ab6c695a309bbb0e975779cfa827d8 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 3 Jan 2020 16:03:05 +0000 Subject: [PATCH 13/44] Add new cookie banner code. Copies HTML and Sass from GOV.UK Pubishing components cookie-banner with changes to content and functionality to better suit Notify. Changes are: - adds a 'reject' button which the GOV.UK code doesn't have - adds Sass from the GOV.UK Frontend button component which the GOV.UK version used so is included here - removed click tracking from cookie banner --- app/assets/javascripts/cookieMessage.js | 96 +++++++++-- .../components/cookie-message.scss | 158 +++++++++++++++++- .../stylesheets/govuk-frontend/_all.scss | 1 + app/templates/admin_template.html | 8 +- app/templates/components/cookie-banner.html | 33 ++++ 5 files changed, 274 insertions(+), 22 deletions(-) create mode 100644 app/templates/components/cookie-banner.html diff --git a/app/assets/javascripts/cookieMessage.js b/app/assets/javascripts/cookieMessage.js index 4e17cbec7..2818a57df 100644 --- a/app/assets/javascripts/cookieMessage.js +++ b/app/assets/javascripts/cookieMessage.js @@ -1,16 +1,90 @@ -(function () { - "use strict"; +window.GOVUK = window.GOVUK || {}; +window.GOVUK.Modules = window.GOVUK.Modules || {}; - var root = this; - if(typeof root.GOVUK === 'undefined') { root.GOVUK = {}; } +(function (Modules) { + function CookieBanner () { } - GOVUK.addCookieMessage = function () { - var message = document.getElementById('global-cookie-message'), - hasCookieMessage = (message && GOVUK.cookie('seen_cookie_message') === null); + CookieBanner.prototype.start = function ($module) { + this.$module = $module[0]; + this.$module.hideCookieMessage = this.hideCookieMessage.bind(this); + this.$module.showConfirmationMessage = this.showConfirmationMessage.bind(this); + this.$module.setCookieConsent = this.setCookieConsent.bind(this); - if (hasCookieMessage) { - message.style.display = 'block'; - GOVUK.cookie('seen_cookie_message', 'yes', { days: 28 }); + this.$module.cookieBanner = document.querySelector('.notify-cookie-banner'); + this.$module.cookieBannerConfirmationMessage = this.$module.querySelector('.notify-cookie-banner__confirmation'); + + this.setupCookieMessage(); + }; + + CookieBanner.prototype.setupCookieMessage = function () { + this.$hideLink = this.$module.querySelector('button[data-hide-cookie-banner]'); + if (this.$hideLink) { + this.$hideLink.addEventListener('click', this.$module.hideCookieMessage); + } + + this.$acceptCookiesLink = this.$module.querySelector('button[data-accept-cookies=true]'); + if (this.$acceptCookiesLink) { + this.$acceptCookiesLink.addEventListener('click', () => this.$module.setCookieConsent(true)); + } + + this.$rejectCookiesLink = this.$module.querySelector('button[data-accept-cookies=false]'); + if (this.$rejectCookiesLink) { + this.$rejectCookiesLink.addEventListener('click', () => this.$module.setCookieConsent(false)); + } + + this.showCookieMessage(); + }; + + CookieBanner.prototype.showCookieMessage = function () { + // Show the cookie banner if not in the cookie settings page + if (!this.isInCookiesPage()) { + var hasCookiesPolicy = window.GOVUK.cookie('cookies_policy'); + var shouldHaveCookieMessage = (this.$module && !hasCookiesPolicy); + + if (shouldHaveCookieMessage) { + this.$module.style.display = 'block'; + } else { + this.$module.style.display = 'none'; + } + } else { + this.$module.style.display = 'none'; } }; -}).call(this); + + CookieBanner.prototype.hideCookieMessage = function (event) { + if (this.$module) { + this.$module.style.display = 'none'; + } + + if (event.target) { + event.preventDefault(); + } + }; + + CookieBanner.prototype.setCookieConsent = function (analyticsConsent) { + window.GOVUK.setConsentCookie({ 'analytics': analyticsConsent }); + + this.$module.showConfirmationMessage(analyticsConsent); + this.$module.cookieBannerConfirmationMessage.focus(); + + if (analyticsConsent) { window.GOVUK.initAnalytics(); } + }; + + CookieBanner.prototype.showConfirmationMessage = function (analyticsConsent) { + var messagePrefix = analyticsConsent ? 'You’ve accepted analytics cookies.' : 'You told us not to use analytics cookies.'; + + this.$cookieBannerMainContent = document.querySelector('.notify-cookie-banner__wrapper'); + this.$cookieBannerConfirmationMessage = document.querySelector('.notify-cookie-banner__confirmation-message'); + + this.$cookieBannerConfirmationMessage.insertAdjacentText('afterbegin', messagePrefix); + this.$cookieBannerMainContent.style.display = 'none'; + this.$module.cookieBannerConfirmationMessage.style.display = 'block'; + }; + + CookieBanner.prototype.isInCookiesPage = function () { + return window.location.pathname === '/cookies'; + }; + + Modules.CookieBanner = CookieBanner; +})(window.GOVUK.Modules); + diff --git a/app/assets/stylesheets/components/cookie-message.scss b/app/assets/stylesheets/components/cookie-message.scss index 559e07bbf..39d57e2c8 100644 --- a/app/assets/stylesheets/components/cookie-message.scss +++ b/app/assets/stylesheets/components/cookie-message.scss @@ -1,8 +1,156 @@ -.notify-cookie-message { - @include govuk-font($size: 16); - padding: govuk-spacing(3) 0; +// GOV.UK Publishing components cookie banner styles +// https://github.com/alphagov/govuk_publishing_components/blob/master/app/assets/stylesheets/govuk_publishing_components/components/_cookie-banner.scss +// sass-lint:disable mixins-before-declarations - .js-enabled & { - display: none; +.notify-cookie-banner__with-js { + display: none; +} + +.notify-cookie-banner__no-js { + display: block; +} + +.notify-cookie-banner__with-js { + display: none; +} + +.notify-cookie-banner__no-js { + display: block; +} + +.js-enabled { + .notify-cookie-banner__no-js, + .notify-cookie-banner { + display: none; // shown with JS, always on for non-JS + } + + .notify-cookie-banner__with-js { + display: block; } } + +.notify-cookie-banner__buttons { + @include govuk-clearfix; + + @include govuk-media-query($from: desktop) { + display: inline-block; + } +} + +.notify-cookie-banner__button { + display: inline-block; + width: 100%; + + @include govuk-media-query($from: mobile, $until: desktop) { + &.notify-cookie-banner__button-accept { + float: left; + width: 49%; + } + + &.notify-cookie-banner__button-reject { + .js-enabled & { + float: right; + width: 49%; + } + } + } + + @include govuk-media-query($from: desktop) { + width: auto; + } + + @include govuk-media-query($until: 455px) { + width: 100%; + } +} + +.notify-cookie-banner__button-accept { + display: none; + + .js-enabled & { + display: inline-block; + } +} + +.notify-cookie-banner__confirmation { + display: none; + position: relative; + padding: govuk-spacing(4) 0; + + @include govuk-media-query($from: desktop) { + padding: govuk-spacing(4); + } + + // This element is focused using JavaScript so that it's being read out by screen readers + // for this reason we don't want to show the default outline or emphasise it visually using `govuk-focused-text` + &:focus { + outline: none; + } +} + +.notify-cookie-banner__confirmation-message, +.notify-cookie-banner__hide-button { + display: block; + + @include govuk-media-query($from: desktop) { + display: inline-block; + } +} + +.notify-cookie-banner__confirmation-message { + margin-right: govuk-spacing(4); + + @include govuk-media-query($from: desktop) { + max-width: 90%; + } +} + +.notify-cookie-banner__hide-button { + @include govuk-font($size: 19); + outline: 0; + border: 0; + background: none; + text-decoration: underline; + padding: govuk-spacing(0); + margin-top: govuk-spacing(2); + right: govuk-spacing(3); + + @include govuk-media-query($from: desktop) { + margin-top: govuk-spacing(0); + position: absolute; + right: govuk-spacing(4); + } +} + +// GOV.UK Publishing components button styles (inherits from GOV.UK Frontend button) +// https://github.com/alphagov/govuk_publishing_components/blob/master/app/assets/stylesheets/govuk_publishing_components/components/_button.scss + +.notify-cookie-banner-button--inline { + display: block; + width: 100%; + margin-bottom: govuk-spacing(1); + vertical-align: top; + + @include govuk-media-query($from: desktop) { + display: inline-block; + width: auto; + vertical-align: baseline; + margin-right: govuk-spacing(2); + } +} + +.notify-cookie-banner-button--secondary { + padding: (govuk-spacing(2) - $govuk-border-width-form-element) govuk-spacing(2); // s1 + box-shadow: none; + + &:before { + content: none; + } +} + +// Additions + +// Override margin-bottom, inherited from using .govuk-body class +.notify-cookie-banner__confirmation-message { + margin-bottom: 0; +} diff --git a/app/assets/stylesheets/govuk-frontend/_all.scss b/app/assets/stylesheets/govuk-frontend/_all.scss index 1f013e2c6..c1609b511 100644 --- a/app/assets/stylesheets/govuk-frontend/_all.scss +++ b/app/assets/stylesheets/govuk-frontend/_all.scss @@ -23,6 +23,7 @@ $govuk-assets-path: "/static/"; @import 'components/header/_header'; @import 'components/footer/_footer'; @import 'components/back-link/_back-link'; +@import 'components/button/_button'; @import 'components/details/_details'; @import "utilities/all"; diff --git a/app/templates/admin_template.html b/app/templates/admin_template.html index 59fcc33a1..7c9c2adcc 100644 --- a/app/templates/admin_template.html +++ b/app/templates/admin_template.html @@ -1,5 +1,6 @@ {% extends "template.njk" %} {% from "components/banner.html" import banner %} +{% from "components/cookie-banner.html" import cookie_banner %} {% block headIcons %} @@ -30,12 +31,7 @@ {% block bodyStart %} {% block cookie_message %} - + {{ cookie_banner("GOV.UK Notify uses cookies which are essential for the site to work. We also use non-essential cookies to help us improve the service. Any data collected is anonymised. By continuing to use this site, you agree to our use of cookies.") }} {% endblock %} {% endblock %} diff --git a/app/templates/components/cookie-banner.html b/app/templates/components/cookie-banner.html new file mode 100644 index 000000000..5f0f722a1 --- /dev/null +++ b/app/templates/components/cookie-banner.html @@ -0,0 +1,33 @@ +{% macro cookie_banner(message, id='global-cookie-message') %} + + +{% endmacro %} From 1d864943c5bb49cc05ed040af57ac24809c613cf Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Sun, 15 Dec 2019 19:39:31 +0000 Subject: [PATCH 14/44] Move code for deleting old cookies into banner JS Removes the following cookies: - seen_cookie_message (flags if banner was already shown) - _gid (Google Analytics cookie) - _ga (Google Analytics cookie) These were set by default before so potentially still around for some users. The code for this now exists as a static method on the cookieMessage module and is called when the JS loads for the first time. --- app/assets/javascripts/cookieMessage.js | 12 ++++++++++++ app/assets/javascripts/main.js | 2 +- app/templates/admin_template.html | 8 -------- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/cookieMessage.js b/app/assets/javascripts/cookieMessage.js index 2818a57df..37174d89b 100644 --- a/app/assets/javascripts/cookieMessage.js +++ b/app/assets/javascripts/cookieMessage.js @@ -4,6 +4,18 @@ window.GOVUK.Modules = window.GOVUK.Modules || {}; (function (Modules) { function CookieBanner () { } + CookieBanner.clearOldCookies = function () { + // clear any cookies set by the previous version + var oldCookies = ['seen_cookie_message', '_ga', '_gid']; + + for (var i = 0; i < oldCookies.length; i++) { + if (window.GOVUK.cookie(oldCookies[i])) { + var cookieString = oldCookies[i] + '=;expires=' + new Date() + ';domain=' + window.location.hostname.replace(/^www\./, '.') + ';path=/'; + document.cookie = cookieString; + } + } + }; + CookieBanner.prototype.start = function ($module) { this.$module = $module[0]; this.$module.hideCookieMessage = this.hideCookieMessage.bind(this); diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index 354a1daea..44330747d 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -1,6 +1,6 @@ window.GOVUK.Frontend.initAll(); -$(() => GOVUK.addCookieMessage()); +window.GOVUK.Modules.CookieBanner.clearOldCookies(); if (window.GOVUK.hasConsentFor('analytics')) { window.GOVUK.initAnalytics(); diff --git a/app/templates/admin_template.html b/app/templates/admin_template.html index 7c9c2adcc..6c347737d 100644 --- a/app/templates/admin_template.html +++ b/app/templates/admin_template.html @@ -243,12 +243,4 @@ - {% endblock %} From 34b85cae10a10840bf1e0bc05f0ba5d26564e86f Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Mon, 23 Dec 2019 15:12:59 +0000 Subject: [PATCH 15/44] Update cookies page Includes: - new content - added option to turn analytics on/off - non-js version for the on/off switch - a banner to confirm user's choice was saved, shown when they click the save button - the cookie banner that appears on all other pages removed from this page --- app/assets/javascripts/cookieSettings.js | 84 ++++++++++++++ .../stylesheets/govuk-frontend/_all.scss | 1 + app/assets/stylesheets/main.scss | 1 + app/assets/stylesheets/views/cookies.scss | 17 +++ app/templates/views/cookies.html | 106 +++++++++++++++--- gulpfile.js | 1 + 6 files changed, 192 insertions(+), 18 deletions(-) create mode 100644 app/assets/javascripts/cookieSettings.js create mode 100644 app/assets/stylesheets/views/cookies.scss diff --git a/app/assets/javascripts/cookieSettings.js b/app/assets/javascripts/cookieSettings.js new file mode 100644 index 000000000..684b70a7e --- /dev/null +++ b/app/assets/javascripts/cookieSettings.js @@ -0,0 +1,84 @@ +window.GOVUK = window.GOVUK || {}; +window.GOVUK.Modules = window.GOVUK.Modules || {}; + +(function (Modules) { + function CookieSettings () {} + + CookieSettings.prototype.start = function ($module) { + this.$module = $module[0]; + + this.$module.submitSettingsForm = this.submitSettingsForm.bind(this); + + document.querySelector('form[data-module=cookie-settings]') + .addEventListener('submit', this.$module.submitSettingsForm); + + this.setInitialFormValues(); + }; + + CookieSettings.prototype.setInitialFormValues = function () { + var currentConsentCookie = window.GOVUK.getConsentCookie('consent'); + + if (!currentConsentCookie) { return; } + + var radioButton; + + if (currentConsentCookie.analytics) { + radioButton = document.querySelector('input[name=cookies-analytics][value=on]'); + } else { + radioButton = document.querySelector('input[name=cookies-analytics][value=off]'); + } + + radioButton.checked = true; + }; + + CookieSettings.prototype.submitSettingsForm = function (event) { + event.preventDefault(); + + var formInputs = event.target.querySelectorAll("input[name=cookies-analytics]"); + var options = {}; + + for ( var i = 0; i < formInputs.length; i++ ) { + var input = formInputs[i]; + if (input.checked) { + var value = input.value === "on" ? true : false; + + options.analytics = value; + break; + } + } + + window.GOVUK.setConsentCookie(options); + + this.showConfirmationMessage(); + + if(window.GOVUK.hasConsentFor('analytics')) { + window.GOVUK.initAnalytics(); + } + + return false; + }; + + CookieSettings.prototype.showConfirmationMessage = function () { + var confirmationMessage = document.querySelector('div[data-cookie-confirmation]'); + var previousPageLink = document.querySelector('.cookie-settings__prev-page'); + var referrer = CookieSettings.prototype.getReferrerLink(); + + document.body.scrollTop = document.documentElement.scrollTop = 0; + + if (referrer && referrer !== document.location.pathname) { + previousPageLink.href = referrer; + previousPageLink.style.display = "block"; + } else { + previousPageLink.style.display = "none"; + } + + confirmationMessage.style.display = "block"; + }; + + CookieSettings.prototype.getReferrerLink = function () { + return document.referrer ? new URL(document.referrer).pathname : false; + }; + + Modules.CookieSettings = CookieSettings; +})(window.GOVUK.Modules); + diff --git a/app/assets/stylesheets/govuk-frontend/_all.scss b/app/assets/stylesheets/govuk-frontend/_all.scss index c1609b511..9b8755367 100644 --- a/app/assets/stylesheets/govuk-frontend/_all.scss +++ b/app/assets/stylesheets/govuk-frontend/_all.scss @@ -25,6 +25,7 @@ $govuk-assets-path: "/static/"; @import 'components/back-link/_back-link'; @import 'components/button/_button'; @import 'components/details/_details'; +@import 'components/radios/_radios'; @import "utilities/all"; @import "overrides/all"; diff --git a/app/assets/stylesheets/main.scss b/app/assets/stylesheets/main.scss index b1cb870aa..26b07297d 100644 --- a/app/assets/stylesheets/main.scss +++ b/app/assets/stylesheets/main.scss @@ -82,6 +82,7 @@ $path: '/static/images/'; @import 'views/send'; @import 'views/get_started'; @import 'views/history'; +@import 'views/cookies'; // TODO: break this up @import 'app'; diff --git a/app/assets/stylesheets/views/cookies.scss b/app/assets/stylesheets/views/cookies.scss new file mode 100644 index 000000000..05974bb08 --- /dev/null +++ b/app/assets/stylesheets/views/cookies.scss @@ -0,0 +1,17 @@ +.cookie-settings__form-wrapper { + display: none; + + .js-enabled & { + display: block; + } +} + +.cookie-settings__no-js { + .js-enabled & { + display: none; + } +} + +.cookie-settings__confirmation { + display: none; +} diff --git a/app/templates/views/cookies.html b/app/templates/views/cookies.html index 0a2f1e16b..5559185b7 100644 --- a/app/templates/views/cookies.html +++ b/app/templates/views/cookies.html @@ -1,27 +1,31 @@ {% extends "withoutnav_template.html" %} +{% from "components/banner.html" import banner %} {% block per_page_title %} Cookies {% endblock %} +{% block cookie_message %}{% endblock %} + {% block maincolumn_content %}
+

Cookies

- GOV.UK Notify puts small files (known as ‘cookies’) - onto your computer. -

-

These cookies are used to remember you once you’ve logged in.

-

- Find out how to manage cookies. + Cookies are small files saved on your phone, tablet or computer when you visit a website.

+

We use cookies to make GOV.UK Notify work and collect information about how you use our service.

-

Session cookies

+

Essential cookies

- We store session cookies on your computer to help keep your information - secure while you use the service. + Essential cookies keep your information secure while you use Notify. We do not need to ask permission to use them.

@@ -37,21 +41,43 @@ notify_admin_session + + + + +
- Used to keep you logged in + Used to keep you signed in 20 hours
+ cookie_policy + + Saves your cookie consent settings + + 1 year +
-

Introductory message cookie

+

Analytics cookies (optional)

- When you first use the service, you may see a pop-up ‘welcome’ message. - Once you’ve seen the message, we store a cookie on your computer so it - knows not to show it again. + With your permission, we use Google Analytics to collect data about how you use Notify. +
+ This information helps us to improve our service.

+

+ Google is not allowed to use or share our analytics data with anyone. +

+

+ Google Analytics cookies collect and store anonymised data about: +

+
    +
  • unique users
    • +
  • informing referring sites
    • +
  • visitor and session counts
    • +
@@ -63,18 +89,62 @@ + + + + +
- seen_cookie_message + _ga - Saves a message to let us know that you have seen our cookie - message + Checks if you’ve visited GOV.UK Notify before - 1 month + 2 years +
+ _gid + + Checks if you’ve visited GOV.UK Notify before + + 24 hours
+
+

Do you want to accept analytics cookies?

+

We use Javascript to set our analytics cookies. Unfortunately Javascript is not running on your browser, so you cannot change your settings. You can try:

+
    +
  • reloading the page
    • +
  • turning on Javascript in your browser
    • +
+
+
+
+
+
+ + Do you want to accept analytics cookies? + +
+
+ + +
+
+ + +
+
+
+
+ +
+
diff --git a/gulpfile.js b/gulpfile.js index 8e8893a64..52f713f2b 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -146,6 +146,7 @@ const javascripts = () => { paths.src + 'javascripts/analytics/analytics.js', paths.src + 'javascripts/analytics/init.js', paths.src + 'javascripts/cookieMessage.js', + paths.src + 'javascripts/cookieSettings.js', paths.src + 'javascripts/stick-to-window-when-scrolling.js', paths.src + 'javascripts/apiKey.js', paths.src + 'javascripts/autofocus.js', From ca019d4a0dc12407388727a35898e43f3e54b6ab Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 20 Dec 2019 11:39:24 +0000 Subject: [PATCH 16/44] Fix typo on privacy page --- app/templates/views/privacy.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/privacy.html b/app/templates/views/privacy.html index 281e7cfb2..b4f2cf6de 100644 --- a/app/templates/views/privacy.html +++ b/app/templates/views/privacy.html @@ -61,7 +61,7 @@

We will retain your personal data for as long as you have a GOV.UK Notify account.

-

Where your data is processed and stores

+

Where your data is processed and stored

We design, build and run our systems to make sure that your data is as safe as possible at any stage, both while it’s processed and when it’s stored.

From 28140104f116a44fecb3dd9629f1d193183371e4 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 20 Dec 2019 13:51:15 +0000 Subject: [PATCH 17/44] Fix python tests broken by cookie banner --- tests/app/main/views/test_code_not_received.py | 4 ++-- tests/app/main/views/test_jobs.py | 2 +- tests/app/main/views/test_manage_users.py | 4 ++-- tests/app/main/views/test_send.py | 12 ++++++------ tests/app/main/views/test_service_settings.py | 8 ++++---- tests/app/main/views/test_template_folders.py | 12 ++++++------ tests/app/main/views/test_uploads.py | 4 ++-- tests/app/main/views/test_verify.py | 2 +- 8 files changed, 24 insertions(+), 24 deletions(-) diff --git a/tests/app/main/views/test_code_not_received.py b/tests/app/main/views/test_code_not_received.py index c3f282c21..23712125e 100644 --- a/tests/app/main/views/test_code_not_received.py +++ b/tests/app/main/views/test_code_not_received.py @@ -22,7 +22,7 @@ def test_should_render_email_verification_resend_show_email_address_and_resend_v assert page.h1.string == 'Check your email' expected = "A new confirmation email has been sent to {}".format(api_user_active['email_address']) - message = page.find_all('p')[1].text + message = page.select('main p')[0].text assert message == expected mock_send_verify_email.assert_called_with(api_user_active['id'], api_user_active['email_address']) @@ -66,7 +66,7 @@ def test_should_render_correct_resend_template_for_pending_user( assert page.h1.string == 'Check your mobile number' expected = 'Check your mobile phone number is correct and then resend the security code.' - message = page.find_all('p')[1].text + message = page.select('main p')[0].text assert message == expected assert page.find('form').input['value'] == api_user_pending['mobile_number'] diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index f8723505a..b18f2ccce 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -508,7 +508,7 @@ def test_should_show_scheduled_job( template_id='5d729fbd-239c-44ab-b498-75a985f3198f', version=1, ) - assert page.select_one('button[type=submit]').text.strip() == 'Cancel sending' + assert page.select_one('main button[type=submit]').text.strip() == 'Cancel sending' def test_should_cancel_job( diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index c6becbafe..bdaac53d0 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1046,7 +1046,7 @@ def test_edit_user_email_page( assert page.find('h1').text == "Change team member’s email address" assert page.select('p[id=user_name]')[0].text == "This will change the email address for {}.".format(user['name']) assert page.select('input[type=email]')[0].attrs["value"] == user['email_address'] - assert page.select('button[type=submit]')[0].text == "Save" + assert page.select('main button[type=submit]')[0].text == "Save" def test_edit_user_email_page_404_for_non_team_member( @@ -1367,7 +1367,7 @@ def test_edit_user_mobile_number_page( "This will change the mobile number for {}." ).format(active_user_with_permissions['name']) assert page.select('input[name=mobile_number]')[0].attrs["value"] == "0770••••762" - assert page.select('button[type=submit]')[0].text == "Save" + assert page.select('main button[type=submit]')[0].text == "Save" def test_edit_user_mobile_number_redirects_to_confirmation( diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 45e7217c7..c6ca1dedb 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1003,7 +1003,7 @@ def test_send_test_doesnt_show_file_contents( assert page.select('h1')[0].text.strip() == 'Preview of ‘Two week reminder’' assert len(page.select('table')) == 0 assert len(page.select('.banner-dangerous')) == 0 - assert page.select_one('button[type=submit]').text.strip() == 'Send 1 text message' + assert page.select_one('main button[type=submit]').text.strip() == 'Send 1 text message' @pytest.mark.parametrize('user, endpoint, template_type, content_has_placeholders, expected_recipient', [ @@ -2229,7 +2229,7 @@ def test_letter_can_only_be_sent_now( assert 'name="scheduled_for"' not in page assert normalize_spaces( - page.select_one('[type=submit]').text + page.select_one('main [type=submit]').text ) == ( 'Send 1 letter' ) @@ -2259,7 +2259,7 @@ def test_send_button_is_correctly_labelled( ) assert normalize_spaces( - page.select_one('[type=submit]').text + page.select_one('main [type=submit]').text ) == ( 'Send 1,000 text messages' ) @@ -2891,7 +2891,7 @@ def test_check_messages_does_not_allow_to_send_letter_longer_than_10_pages( assert page.find('h1', {"data-error-type": "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]') + assert not page.select('main [type=submit]') def test_check_messages_shows_data_errors_before_trial_mode_errors_for_letters( @@ -3218,7 +3218,7 @@ def test_send_one_off_letter_errors_in_trial_mode( assert len(page.select('.letter img')) == 5 - assert not page.select('[type=submit]') + assert not page.select('main [type=submit]') assert page.select_one('.govuk-back-link').text == 'Back' assert page.select_one('a[download]').text == 'Download as a PDF' @@ -3259,7 +3259,7 @@ def test_send_one_off_letter_errors_if_letter_longer_than_10_pages( assert page.find('h1', {"data-error-type": "letter-too-long"}) assert len(page.select('.letter img')) == 10 - assert not page.select('[type=submit]') + assert not page.select('main [type=submit]') def test_check_messages_shows_over_max_row_error( diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 0bc576e95..2f4cdae33 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -435,7 +435,7 @@ def test_show_restricted_service( ) assert page.find('h1').text == 'Settings' - assert page.find_all('h2')[0].text == 'Your service is in trial mode' + assert page.select('main h2')[0].text == 'Your service is in trial mode' request_to_live = page.select('main p')[1] request_to_live_link = request_to_live.select_one('a') @@ -889,7 +889,7 @@ def test_should_not_show_go_live_button_if_checklist_not_complete( page.select_one('[type=submit]').text.strip() == ('Request to go live') else: assert not page.select('form') - assert not page.select('[type=submit]') + assert not page.select('main [type=submit]') assert len(page.select('main p')) == 1 assert normalize_spaces(page.select_one('main p').text) == ( 'You must complete these steps before you can request to go live.' @@ -1192,8 +1192,8 @@ def test_non_gov_user_is_told_they_cant_go_live( assert normalize_spaces(page.select_one('main p').text) == ( 'Only team members with a government email address can request to go live.' ) - assert len(page.select('form')) == 0 - assert len(page.select('button')) == 1 + assert len(page.select('main form')) == 0 + assert len(page.select('main button')) == 0 @pytest.mark.parametrize('consent_to_research, displayed_consent', ( diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 5e623074b..f61557879 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -835,18 +835,18 @@ def test_delete_template_folder_should_request_confirmation( assert page.select_one('input[name=name]')['value'] == 'sacrifice' - assert len(page.select('form')) == 2 - assert len(page.select('button')) == 3 + assert len(page.select('main form')) == 2 + assert len(page.select('main button')) == 2 - assert 'action' not in page.select('form')[0] - assert page.select('form button')[0].text == 'Yes, delete' + assert 'action' not in page.select('main form')[0] + assert page.select('main form button')[0].text == 'Yes, delete' - assert page.select('form')[1]['action'] == url_for( + assert page.select('main form')[1]['action'] == url_for( 'main.manage_template_folder', service_id=service_one['id'], template_folder_id=folder_id, ) - assert page.select('form button')[1].text == 'Save' + assert page.select('main form button')[1].text == 'Save' def test_delete_template_folder_should_detect_non_empty_folder_on_get( diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index e0263f945..ed46f500e 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -103,7 +103,7 @@ def test_post_upload_letter_redirects_for_valid_file( assert not page.find(id='validation-error-message') assert page.find('input', {'type': 'hidden', 'name': 'file_id', 'value': fake_uuid}) - assert page.find('button', {'type': 'submit'}).text == 'Send 1 letter' + assert page.select('main button[type=submit]')[0].text == 'Send 1 letter' def test_post_upload_letter_shows_letter_preview_for_valid_file( @@ -406,7 +406,7 @@ def test_uploaded_letter_preview_does_not_show_send_button_if_service_in_trial_m 'Recipient: The Queen' ) assert not page.find('form') - assert not page.find('button', {'type': 'submit'}) + assert len(page.select('main button[type=submit]')) == 0 @pytest.mark.parametrize('invalid_pages, page_requested, overlay_expected', ( diff --git a/tests/app/main/views/test_verify.py b/tests/app/main/views/test_verify.py index 47f7c7218..f143d382a 100644 --- a/tests/app/main/views/test_verify.py +++ b/tests/app/main/views/test_verify.py @@ -22,7 +22,7 @@ def test_should_return_verify_template( page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.text == 'Check your phone' - message = page.find_all('p')[1].text + message = page.select('main p')[0].text assert message == "We’ve sent you a text message with a security code." From 9a0d52296464032fe23ca6cb3265fc15daccd066 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 31 Dec 2019 16:49:52 +0000 Subject: [PATCH 18/44] Add JS tests for analytics & cookies JS Includes: - tests for the analytics interface ported from GOVUK Frontend Toolkit - tests for the cookie banner that appears on all pages except the cookies page - tests for the cookies page JS - tests for the hasConsentFor function - adding a deleteCookie helper to remove cookies during tests - polyfill for insertAdjacentText The last one is because JSDOM doesn't support insertAdjacentText but our target browsers do. This polyfill also includes one for insertAdjacentHTML. --- tests/javascripts/analytics/analytics.test.js | 97 +++++++ tests/javascripts/analytics/init.test.js | 123 +++++++++ tests/javascripts/consent.test.js | 55 ++++ tests/javascripts/cookieMessage.test.js | 230 ++++++++++++++++ tests/javascripts/cookieSettings.test.js | 255 ++++++++++++++++++ tests/javascripts/support/helpers.js | 3 + tests/javascripts/support/helpers/cookies.js | 22 ++ tests/javascripts/support/polyfills.js | 53 ++++ tests/javascripts/support/setup.js | 3 + 9 files changed, 841 insertions(+) create mode 100644 tests/javascripts/analytics/analytics.test.js create mode 100644 tests/javascripts/analytics/init.test.js create mode 100644 tests/javascripts/consent.test.js create mode 100644 tests/javascripts/cookieMessage.test.js create mode 100644 tests/javascripts/cookieSettings.test.js create mode 100644 tests/javascripts/support/helpers/cookies.js create mode 100644 tests/javascripts/support/polyfills.js diff --git a/tests/javascripts/analytics/analytics.test.js b/tests/javascripts/analytics/analytics.test.js new file mode 100644 index 000000000..6b357a1e8 --- /dev/null +++ b/tests/javascripts/analytics/analytics.test.js @@ -0,0 +1,97 @@ +const helpers = require('../support/helpers'); + +beforeAll(() => { + + // add the script GA looks for in the document + document.body.appendChild(document.createElement('script')); + + require('../../../app/assets/javascripts/govuk/cookie-functions.js'); + require('../../../app/assets/javascripts/analytics/analytics.js'); + require('../../../app/assets/javascripts/analytics/init.js'); + +}); + +afterAll(() => { + + require('../support/teardown.js'); + +}); + +describe("Analytics", () => { + + let analytics; + + beforeEach(() => { + + window.ga = jest.fn(); + + analytics = new GOVUK.Analytics({ + trackingId: 'UA-75215134-1', + cookieDomain: 'auto', + anonymizeIp: true, + displayFeaturesTask: null, + transport: 'beacon' + }); + + }); + + afterEach(() => { + + window.ga.mockReset(); + + }); + + describe("When created", () => { + + test("It configures a tracker", () => { + + setUpArguments = window.ga.mock.calls; + + expect(setUpArguments[0]).toEqual(['create', 'UA-75215134-1', 'auto']); + expect(setUpArguments[1]).toEqual(['set', 'anonymizeIp', true]); + expect(setUpArguments[2]).toEqual(['set', 'displayFeaturesTask', null]); + expect(setUpArguments[3]).toEqual(['set', 'transport', 'beacon']); + + }); + + }); + + describe("When tracking pageviews", () => { + + test("It sends the right URL for the page if no arguments", () => { + + window.ga.mockClear(); + + jest.spyOn(window, 'location', 'get').mockImplementation(() => { + return { + 'pathname': '/privacy', + 'search': '' + }; + }); + + analytics.trackPageview(); + + expect(window.ga.mock.calls[0]).toEqual(['send', 'pageview', '/privacy']); + + }); + + test("It strips the UUIDs from URLs", () => { + + window.ga.mockClear(); + + jest.spyOn(window, 'location', 'get').mockImplementation(() => { + return { + 'pathname': '/services/6658542f-0cad-491f-bec8-ab8457700ead', + 'search': '' + }; + }); + + analytics.trackPageview(); + + expect(window.ga.mock.calls[0]).toEqual(['send', 'pageview', '/services/…']); + + }); + + }); + +}); diff --git a/tests/javascripts/analytics/init.test.js b/tests/javascripts/analytics/init.test.js new file mode 100644 index 000000000..3f3563b5c --- /dev/null +++ b/tests/javascripts/analytics/init.test.js @@ -0,0 +1,123 @@ +const helpers = require('../support/helpers'); + +beforeAll(() => { + + // add the script GA looks for in the document + document.body.appendChild(document.createElement('script')); + + require('../../../app/assets/javascripts/govuk/cookie-functions.js'); + require('../../../app/assets/javascripts/analytics/analytics.js'); + require('../../../app/assets/javascripts/analytics/init.js'); + +}); + +afterAll(() => { + + require('../support/teardown.js'); + +}); + +describe("Analytics init", () => { + + beforeAll(() => { + + window.ga = jest.fn(); + jest.spyOn(window.GOVUK.Analytics, 'load'); + + // pretend we're on the /privacy page + jest.spyOn(window, 'location', 'get').mockImplementation(() => { + return { + 'pathname': '/privacy', + 'search': '' + }; + }); + + }); + + afterEach(() => { + + window.GOVUK.Analytics.load.mockClear(); + window.ga.mockClear(); + + }); + + test("After the init.js script has been loaded, Google Analytics will be disabled", () => { + + expect(window['ga-disable-UA-26179049-1']).toBe(true); + + }); + + describe("If initAnalytics has already been called", () => { + + beforeAll(() => { + + // Fake a tracker instance + window.GOVUK.analytics = {}; + + }); + + beforeEach(() => { + + window.GOVUK.initAnalytics(); + + }); + + afterAll(() => { + + delete window.GOVUK.analytics; + + }); + + test("The Google Analytics libraries will not be loaded", () => { + + expect(window.GOVUK.Analytics.load).not.toHaveBeenCalled(); + + }); + + }); + + describe("If initAnalytics has not been called", () => { + + beforeEach(() => { + + window.GOVUK.initAnalytics(); + + }); + + afterEach(() => { + + // window.GOVUK.initAnalytics sets up a new window.GOVUK.analytics which needs clearing + delete window.GOVUK.analytics; + + }); + + test("Google Analytics will not be disabled", () => { + + expect(window['ga-disable-UA-26179049-1']).toBe(false); + + }); + + test("The Google Analytics libraries will have been loaded", () => { + + expect(window.GOVUK.Analytics.load).toHaveBeenCalled(); + + }); + + test("There will be an interface with the Google Analytics API", () => { + + expect(window.GOVUK.analytics).toBeDefined(); + + }); + + test("A pageview will be registered", () => { + + expect(window.ga.mock.calls.length).toEqual(5); + + // The first 4 calls configure the analytics tracker. All subsequent calls send data + expect(window.ga.mock.calls[4]).toEqual(['send', 'pageview', '/privacy']); + + }); + + }); + +}); diff --git a/tests/javascripts/consent.test.js b/tests/javascripts/consent.test.js new file mode 100644 index 000000000..9217bd81c --- /dev/null +++ b/tests/javascripts/consent.test.js @@ -0,0 +1,55 @@ +const helpers = require('./support/helpers'); + +beforeAll(() => { + + require('../../app/assets/javascripts/govuk/cookie-functions.js'); + require('../../app/assets/javascripts/consent.js'); + +}); + +afterAll(() => { + + require('./support/teardown.js'); + +}); + +describe("Cookie consent", () => { + + describe("hasConsentFor", () => { + + afterEach(() => { + + // remove cookie set by tests + helpers.deleteCookie('cookies_policy'); + + }); + + test("If there is no consent cookie, return false", () => { + + expect(window.GOVUK.hasConsentFor('analytics')).toBe(false); + + }); + + describe("If a consent cookie is set", () => { + + test("If the category is not saved in the cookie, return false", () => { + + window.GOVUK.setConsentCookie({ 'usage': true }); + + expect(window.GOVUK.hasConsentFor('analytics')).toBe(false); + + }); + + test("If the category is saved in the cookie, return its value", () => { + + window.GOVUK.setConsentCookie({ 'analytics': true }); + + expect(window.GOVUK.hasConsentFor('analytics')).toBe(true); + + }); + + }); + + }); + +}); diff --git a/tests/javascripts/cookieMessage.test.js b/tests/javascripts/cookieMessage.test.js new file mode 100644 index 000000000..37e5f3bdf --- /dev/null +++ b/tests/javascripts/cookieMessage.test.js @@ -0,0 +1,230 @@ +const helpers = require('./support/helpers'); + +beforeAll(() => { + + require('../../app/assets/javascripts/govuk/cookie-functions.js'); + require('../../app/assets/javascripts/analytics/analytics.js'); + require('../../app/assets/javascripts/analytics/init.js'); + require('../../app/assets/javascripts/cookieMessage.js'); + +}); + +afterAll(() => { + + require('./support/teardown.js'); + +}); + +describe("Cookie message", () => { + + let cookieMessage; + + beforeAll(() => { + + helpers.deleteCookie('cookies-policy'); + + }); + + beforeEach(() => { + + // add the script GA looks for in the document + document.body.appendChild(document.createElement('script')); + + jest.spyOn(window.GOVUK, 'initAnalytics'); + + cookieMessage = ` + `; + + document.body.innerHTML += cookieMessage; + + }); + + afterEach(() => { + + document.body.innerHTML = ''; + + // remove cookie set by tests + helpers.deleteCookie('cookies_policy'); + + // reset spies + window.GOVUK.initAnalytics.mockClear(); + + // remove analytics tracker + delete window.GOVUK.analytics; + + // reset global variable to state when init.js loaded + window['ga-disable-UA-26179049-1'] = true; + + }); + + /* + Note: If no JS, the cookie banner shows a button to take you to the cookies page for more information. + + This works through CSS, based on the presence of the `js-enabled` class on the so is not tested here. + */ + + test("If the cookies set by the old banner still exist, they can be cleared with the `clearOldCookies` method", () => { + + helpers.setCookie('seen_cookie_message', 'true', { 'days': 365 }); + helpers.setCookie('_ga', 'GA1.1.123.123', { 'days': 365 }); + helpers.setCookie('_gid', 'GA1.1.456.456', { 'days': 1 }); + + window.GOVUK.Modules.CookieBanner.clearOldCookies(); + + expect(window.GOVUK.cookie('seen_cookie_message')).toBeNull(); + expect(window.GOVUK.cookie('_ga')).toBeNull(); + expect(window.GOVUK.cookie('_gid')).toBeNull(); + + }); + + test("If user has made a choice to give their consent or not, the cookie banner should be hidden", () => { + + window.GOVUK.setConsentCookie({ 'analytics': false }); + + window.GOVUK.modules.start() + + expect(helpers.element(document.querySelector('.notify-cookie-banner')).is('hidden')).toBe(true); + + }); + + describe("If user hasn't made a choice to give their consent or not", () => { + + beforeEach(() => { + + window.GOVUK.modules.start(); + + }); + + test("The cookie banner should show", () => { + + const banner = helpers.element(document.querySelector('.notify-cookie-banner')); + + expect(banner.is('hidden')).toBe(false); + + }); + + test("No analytics should run", () => { + + expect(window.GOVUK.initAnalytics).not.toHaveBeenCalled(); + + }); + + describe("If the user clicks the button to accept analytics", () => { + + beforeEach(() => { + + const acceptButton = document.querySelector('.notify-cookie-banner__button-accept button'); + + helpers.triggerEvent(acceptButton, 'click'); + + }); + + test("the banner should confirm your choice and link to the cookies page as a way to change your mind", () => { + + confirmation = helpers.element(document.querySelector('.notify-cookie-banner__confirmation')); + + expect(confirmation.is('hidden')).toBe(false); + expect(confirmation.el.textContent.trim()).toEqual(expect.stringMatching(/^You’ve accepted analytics cookies/)); + + }); + + test("If the user clicks the 'hide' button, the banner should be hidden", () => { + + const hideButton = document.querySelector('.notify-cookie-banner__hide-button'); + const banner = helpers.element(document.querySelector('.notify-cookie-banner')); + + helpers.triggerEvent(hideButton, 'click'); + + expect(banner.is('hidden')).toBe(true); + + }); + + test("The consent cookie should be set, with analytics set to 'true'", () => { + + expect(window.GOVUK.getConsentCookie()).toEqual({ 'analytics': true }); + + }); + + test("The analytics should be set up", () => { + + expect(window.GOVUK.analytics).toBeDefined(); + + }); + + }); + + describe("If the user clicks the button to reject analytics", () => { + + beforeEach(() => { + + const rejectButton = document.querySelector('.notify-cookie-banner__button-reject button'); + + helpers.triggerEvent(rejectButton, 'click'); + + }); + + test("the banner should confirm your choice and link to the cookies page as a way to change your mind", () => { + + confirmation = helpers.element(document.querySelector('.notify-cookie-banner__confirmation')); + + expect(confirmation.is('hidden')).toBe(false); + expect(confirmation.el.textContent.trim()).toEqual(expect.stringMatching(/^You told us not to use analytics cookies/)); + + }); + + test("If the user clicks the 'hide' button, the banner should be hidden", () => { + + const hideButton = document.querySelector('.notify-cookie-banner__hide-button'); + const banner = helpers.element(document.querySelector('.notify-cookie-banner')); + + helpers.triggerEvent(hideButton, 'click'); + + expect(banner.is('hidden')).toBe(true); + + }); + + test("The consent cookie should be set, with analytics set to 'true'", () => { + + expect(window.GOVUK.getConsentCookie()).toEqual({ 'analytics': false }); + + }); + + test("The analytics should not be set up", () => { + + expect(window.GOVUK.analytics).not.toBeDefined(); + + }); + + }); + + }); + +}); diff --git a/tests/javascripts/cookieSettings.test.js b/tests/javascripts/cookieSettings.test.js new file mode 100644 index 000000000..a6c6f1344 --- /dev/null +++ b/tests/javascripts/cookieSettings.test.js @@ -0,0 +1,255 @@ +const helpers = require('./support/helpers'); + +beforeAll(() => { + + require('../../app/assets/javascripts/govuk/cookie-functions.js'); + require('../../app/assets/javascripts/consent.js'); + require('../../app/assets/javascripts/analytics/analytics.js'); + require('../../app/assets/javascripts/analytics/init.js'); + require('../../app/assets/javascripts/cookieSettings.js'); + +}); + +afterAll(() => { + + require('./support/teardown.js'); + +}); + +describe("Cookie settings", () => { + + let cookiesPageContent; + let yesRadio; + let noRadio; + let saveButton; + + beforeEach(() => { + + // add the script GA looks for in the document + document.body.appendChild(document.createElement('script')); + + window.ga = jest.fn(); + jest.spyOn(window.GOVUK, 'initAnalytics'); + + cookiesPageContent = ` + +

Cookies

+

+ Cookies are small files saved on your phone, tablet or computer when you visit a website. +

+

We use cookies to make GOV.UK Notify work and collect information about how you use our service.

+
+

Do you want to accept analytics cookies?

+

We use Javascript to set most of our cookies. Unfortunately Javascript is not running on your browser, so you cannot change your settings. You can try:

+
    +
  • reloading the page
    • +
  • turning on Javascript in your browser
    • +
+
+

Analytics cookies (optional)

+
+
+
+
+ + Do you want to accept analytics cookies? + +
+
+ + +
+
+ + +
+
+
+
+ +
+
`; + + document.body.innerHTML += cookiesPageContent; + + yesRadio = document.querySelector('#cookies-analytics-yes'); + noRadio = document.querySelector('#cookies-analytics-no'); + saveButton = document.querySelector('.govuk-button'); + + }); + + afterEach(() => { + + document.body.innerHTML = ''; + + // remove cookie set by tests + helpers.deleteCookie('cookies_policy'); + + // reset spies + window.ga.mockClear(); + window.GOVUK.initAnalytics.mockClear(); + + // remove analytics tracker + delete window.GOVUK.analytics; + + // reset global variable to state when init.js loaded + window['ga-disable-UA-26179049-1'] = true; + + }); + + /* + Note: If no JS, the cookies page contains content to explain why JS is required to set analytics cookies. + This is hidden if JS is available when the page loads. + + The message displayed to confirm any selection made is also in the page but hidden on load. + + Both of these work through CSS, based on the presence of the `js-enabled` class on the so are not tested here. + */ + + describe("When the page loads", () => { + + test("If user has not chosen to accept or reject analytics, the radios for making that choice should be set to unchecked", () => { + + window.GOVUK.modules.start(); + + expect(yesRadio.checked).toBe(false); + expect(noRadio.checked).toBe(false); + + }); + + test("If analytics are accepted, the radio for 'accept analytics' should be set to checked", () => { + + window.GOVUK.setConsentCookie({ 'analytics': true }); + + window.GOVUK.modules.start(); + + expect(yesRadio.checked).toBe(true); + expect(noRadio.checked).toBe(false); + + }); + + test("If analytics are rejected, the radio for 'reject analytics' should be set to checked", () => { + + window.GOVUK.setConsentCookie({ 'analytics': false }); + + window.GOVUK.modules.start(); + + expect(yesRadio.checked).toBe(false); + expect(noRadio.checked).toBe(true); + + }); + + }); + + describe("When the 'Save cookie settings' button is clicked", () => { + + beforeEach(() => { + + window.GOVUK.modules.start(); + + }); + + test("If no selection is made, set consent to reject analytics", () => { + + helpers.triggerEvent(saveButton, 'click'); + + expect(window.GOVUK.getConsentCookie()).toEqual({ 'analytics': false }); + + }); + + test("If a selection is made, save this as consent", () => { + + yesRadio.checked = true; + + helpers.triggerEvent(saveButton, 'click'); + + expect(window.GOVUK.getConsentCookie()).toEqual({ 'analytics': true }); + + }); + + describe("The message confirming your choice", () => { + + let confirmationMessage; + + beforeEach(() => { + + confirmationMessage = document.querySelector('.cookie-settings__confirmation'); + helpers.triggerEvent(saveButton, 'click'); + + }); + + test("Should be shown when the 'Save cookie settings' button is clicked", () => { + + expect(helpers.element(confirmationMessage).is('hidden')).toBe(false); + + }); + + test("Should include a link to the last page visited, if information on the referrer is available", () => { + + jest.spyOn(document, 'referrer', 'get').mockReturnValue('https://notifications.service.gov.uk/privacy'); + + helpers.triggerEvent(saveButton, 'click'); + + expect(confirmationMessage.querySelector('.cookie-settings__prev-page').getAttribute('href')).toEqual('/privacy'); + + }); + + }); + + describe("Analytics code", () => { + + beforeAll(() => { + + jest.spyOn(window, 'location', 'get').mockImplementation(() => { + + return { + 'pathname': '/privacy', + 'search': '' + } + + }); + + }); + + test("if user accepted analytics, the analytics code should initialise and register a pageview", () => { + + window.GOVUK.modules.start(); + + yesRadio.checked = true; + + helpers.triggerEvent(saveButton, 'click'); + + expect(window.GOVUK.initAnalytics).toHaveBeenCalled(); + + expect(window.ga).toHaveBeenCalled(); + // the first 4 calls are configuration + expect(window.ga.mock.calls[4]).toEqual(['send', 'pageview', '/privacy']); + + }); + + test("if user rejected analytics, the analytics code should not run", () => { + + window.GOVUK.modules.start(); + + noRadio.checked = true; + + helpers.triggerEvent(saveButton, 'click'); + + expect(window.GOVUK.initAnalytics).not.toHaveBeenCalled(); + + }); + + }); + + }); + +}); diff --git a/tests/javascripts/support/helpers.js b/tests/javascripts/support/helpers.js index 90c253306..9135ed245 100644 --- a/tests/javascripts/support/helpers.js +++ b/tests/javascripts/support/helpers.js @@ -1,6 +1,7 @@ const globals = require('./helpers/globals.js'); const events = require('./helpers/events.js'); const domInterfaces = require('./helpers/dom_interfaces.js'); +const cookies = require('./helpers/cookies.js'); const html = require('./helpers/html.js'); const elements = require('./helpers/elements.js'); const rendering = require('./helpers/rendering.js'); @@ -14,6 +15,8 @@ exports.moveSelectionToRadio = events.moveSelectionToRadio; exports.activateRadioWithSpace = events.activateRadioWithSpace; exports.RangeMock = domInterfaces.RangeMock; exports.SelectionMock = domInterfaces.SelectionMock; +exports.deleteCookie = cookies.deleteCookie; +exports.setCookie = cookies.setCookie; exports.getRadioGroup = html.getRadioGroup; exports.getRadios = html.getRadios; exports.templatesAndFoldersCheckboxes = html.templatesAndFoldersCheckboxes; diff --git a/tests/javascripts/support/helpers/cookies.js b/tests/javascripts/support/helpers/cookies.js new file mode 100644 index 000000000..2ef2c0ebd --- /dev/null +++ b/tests/javascripts/support/helpers/cookies.js @@ -0,0 +1,22 @@ +// Helper for deleting a cookie +function deleteCookie (cookieName) { + + document.cookie = cookieName + '=; path=/; expires=' + (new Date()); + +}; + +function setCookie (name, value, options) { + if (typeof options === 'undefined') { + options = {}; + } + var cookieString = name + '=' + value + '; path=/;domain=' + window.location.hostname; + if (options.days) { + var date = new Date(); + date.setTime(date.getTime() + (options.days * 24 * 60 * 60 * 1000)); + cookieString = cookieString + '; expires=' + date.toGMTString(); + } + document.cookie = cookieString; +}; + +exports.deleteCookie = deleteCookie; +exports.setCookie = setCookie; diff --git a/tests/javascripts/support/polyfills.js b/tests/javascripts/support/polyfills.js new file mode 100644 index 000000000..0809eb763 --- /dev/null +++ b/tests/javascripts/support/polyfills.js @@ -0,0 +1,53 @@ +// Polyfills for any parts of the DOM API available in browsers but not JSDOM + +// From: https://gist.github.com/eligrey/1276030 +HTMLElement.prototype.insertAdjacentHTML = function(position, html) { + "use strict"; + + var + ref = this + , container = ref.ownerDocument.createElementNS("http://www.w3.org/1999/xhtml", "_") + , ref_parent = ref.parentNode + , node, first_child, next_sibling + ; + + container.innerHTML = html; + + switch (position.toLowerCase()) { + case "beforebegin": + while ((node = container.firstChild)) { + ref_parent.insertBefore(node, ref); + } + break; + case "afterbegin": + first_child = ref.firstChild; + while ((node = container.lastChild)) { + first_child = ref.insertBefore(node, first_child); + } + break; + case "beforeend": + while ((node = container.firstChild)) { + ref.appendChild(node); + } + break; + case "afterend": + next_sibling = ref.nextSibling; + while ((node = container.lastChild)) { + next_sibling = ref_parent.insertBefore(node, next_sibling); + } + break; + } + +}; + +// from: https://developer.mozilla.org/en-US/docs/Web/API/Element/insertAdjacentText#Polyfill +if (!Element.prototype.insertAdjacentText) { + Element.prototype.insertAdjacentText = function(type, txt){ + this.insertAdjacentHTML( + type, + (txt+'') // convert to string + .replace(/&/g, '&') // embed ampersand symbols + .replace(/ Date: Mon, 6 Jan 2020 16:52:03 +0000 Subject: [PATCH 19/44] Improvements based on frontend feedback Paired with @aliuk2012 on the implementation and with a view to making the component generic enough to be used on digital marketplace apps as well. These changes came from that session. They include: - removal of an unused `data-accept-cookies` attribute - removal of `govuk-!-padding-top-4` class and moving of associated styles into component CSS - swapping out the `aria-label` on the parent element for an `aria-describedby` linked to the h2 to have one thing labelling the banner region - removal of unused CSS and any already provided by the govuk-button class - inclusion of @import's for styles attached to govuk-body and govuk-button classes --- .../components/cookie-message.scss | 26 +++++-------------- app/templates/components/cookie-banner.html | 18 ++++++------- 2 files changed, 16 insertions(+), 28 deletions(-) diff --git a/app/assets/stylesheets/components/cookie-message.scss b/app/assets/stylesheets/components/cookie-message.scss index 39d57e2c8..8313bb2c6 100644 --- a/app/assets/stylesheets/components/cookie-message.scss +++ b/app/assets/stylesheets/components/cookie-message.scss @@ -2,12 +2,13 @@ // https://github.com/alphagov/govuk_publishing_components/blob/master/app/assets/stylesheets/govuk_publishing_components/components/_cookie-banner.scss // sass-lint:disable mixins-before-declarations -.notify-cookie-banner__with-js { - display: none; -} +// component uses .govuk-body and .govuk-button classes from govuk-frontend +@import 'core/typography'; +@import 'components/button/_button'; -.notify-cookie-banner__no-js { - display: block; +.notify-cookie-banner__wrapper { + @include govuk-responsive-padding(4, "top"); + @include govuk-responsive-padding(4, "bottom"); } .notify-cookie-banner__with-js { @@ -125,29 +126,16 @@ // GOV.UK Publishing components button styles (inherits from GOV.UK Frontend button) // https://github.com/alphagov/govuk_publishing_components/blob/master/app/assets/stylesheets/govuk_publishing_components/components/_button.scss -.notify-cookie-banner-button--inline { - display: block; - width: 100%; +.notify-cookie-banner-button { margin-bottom: govuk-spacing(1); vertical-align: top; @include govuk-media-query($from: desktop) { - display: inline-block; - width: auto; vertical-align: baseline; margin-right: govuk-spacing(2); } } -.notify-cookie-banner-button--secondary { - padding: (govuk-spacing(2) - $govuk-border-width-form-element) govuk-spacing(2); // s1 - box-shadow: none; - - &:before { - content: none; - } -} - // Additions // Override margin-bottom, inherited from using .govuk-body class diff --git a/app/templates/components/cookie-banner.html b/app/templates/components/cookie-banner.html index 5f0f722a1..568703968 100644 --- a/app/templates/components/cookie-banner.html +++ b/app/templates/components/cookie-banner.html @@ -1,23 +1,23 @@ {% macro cookie_banner(message, id='global-cookie-message') %} -