From 42fc2b31b9955586021e228589a966abe0ea5e3d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 11 Dec 2024 17:16:07 +0000 Subject: [PATCH 1/8] Check for duplicate id attributes in HTML MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s invalid HTML to have multiple elements with the same `id` attribute. Plus it can cause trouble for Javascripts, and make anchor linking unpredictable. --- tests/conftest.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index 17c7cfe311..b5595597a7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3016,6 +3016,8 @@ def get_url( if _test_for_script_csp_nonce: ClientRequest.test_for_script_csp_nonce(page) + ClientRequest._test_for_duplicate_ids(page) + return page @staticmethod @@ -3168,6 +3170,14 @@ def test_for_script_csp_nonce(page): else: assert nonce == fake_nonce + @staticmethod + def _test_for_duplicate_ids(page): + ids = [element["id"] for element in page.select("*[id]")] + for id in ids: + assert ids.count(id) == 1, f"Duplicate id `{id}` found on these elements:\n " + ", ".join( + f"<{element.name}>" for element in page.select(f"*[id='{id}']") + ) + return ClientRequest From 8f841bad689617ff5cf3e31f1d643452598162ae Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 11 Dec 2024 17:19:55 +0000 Subject: [PATCH 2/8] Delete redundant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This wasn’t doing anything styling-wise or functionality-wise, and it had the same ID as its parent element --- app/templates/views/unsubscribe-request-report.html | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/templates/views/unsubscribe-request-report.html b/app/templates/views/unsubscribe-request-report.html index bf89fcf205..8db2235167 100644 --- a/app/templates/views/unsubscribe-request-report.html +++ b/app/templates/views/unsubscribe-request-report.html @@ -56,9 +56,7 @@ {% endcall %}
{% if report.is_a_batched_report %} - This - report will be deleted {{ report.will_be_archived_at|format_delta_days(numeric_prefix="in ")}}. - + This report will be deleted {{ report.will_be_archived_at|format_delta_days(numeric_prefix="in ")}}. {% else %}

Once downloaded, reports are available for 7 days. From 871f063302ace37e4800caf5de6db781a09ffc8e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 11 Dec 2024 17:25:48 +0000 Subject: [PATCH 3/8] Fix wrongly duplicated id on accessibility page --- app/templates/views/accessibility_statement.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/accessibility_statement.html b/app/templates/views/accessibility_statement.html index 422259bc10..f812726c9f 100644 --- a/app/templates/views/accessibility_statement.html +++ b/app/templates/views/accessibility_statement.html @@ -16,7 +16,7 @@

Accessibility statement

"Next review due": "29 January 2025" } ) }} - {# Last non-functional changes: 4 June 2024 #} + {# Last non-functional changes: 13 December 2024 #}

This accessibility statement applies to the www.notifications.service.gov.uk domain. It does not apply to the GOV.UK Notify API documentation subdomain. @@ -82,7 +82,7 @@

Technical information abou GDS is committed to making its websites accessible, in accordance with the Public Sector Bodies (Websites and Mobile Applications) (No. 2) Accessibility Regulations 2018.

-

Compliance status

+

Compliance status

We have tested the Notify website against the Web Content Accessibility Guidelines (WCAG) 2.2 AA standard. From 298d87e7cc6b773b31837ba8562c9a0552e45968 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 11 Dec 2024 17:30:56 +0000 Subject: [PATCH 4/8] Remove ID for pill selected item on usage page I think we used to have this as a target for some ARIA attributes, but since moved way from using those here. --- app/templates/components/pill.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/components/pill.html b/app/templates/components/pill.html index 6fe12678a4..fe80483d02 100644 --- a/app/templates/components/pill.html +++ b/app/templates/components/pill.html @@ -11,7 +11,7 @@ {% for label, option, link, count in items %}

  • {% if current_value == option %} - + {% else %} {% endif %} From d5689687b378593c54c737302e6da64b4826f8c1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 13 Dec 2024 14:07:27 +0000 Subject: [PATCH 5/8] Fix email/letter branding hidden inputs Because we are rendering the same form multiple times on one page we need to make sure each time the hidden field is rendered it gets a different ID. --- app/main/forms.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 80fee56615..988a74669a 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2042,15 +2042,30 @@ def validate(self, *args, **kwargs): return rv +class DuplicatableHiddenField(HiddenField): + """ + An instance of HiddenField which can be reused in multiple forms on the same + page without being given the same ID. + """ + + def __init__(self, *args, **kwargs): + self._call_index = 0 + super().__init__(*args, **kwargs) + + def __call__(self, **kwargs): + self._call_index += 1 + return super().__call__(id=f"{self.name}_{self._call_index}", **kwargs) + + class AdminChangeOrganisationDefaultEmailBrandingForm(StripWhitespaceForm): - email_branding_id = HiddenField( + email_branding_id = DuplicatableHiddenField( "Email branding id", validators=[DataRequired()], ) class AdminChangeOrganisationDefaultLetterBrandingForm(StripWhitespaceForm): - letter_branding_id = HiddenField( + letter_branding_id = DuplicatableHiddenField( "Letter branding id", validators=[DataRequired()], ) From e78ea74513925f81f9a7d3e47a4e685c2716e9b3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 13 Dec 2024 15:26:28 +0000 Subject: [PATCH 6/8] Remove IDs from table rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Where the table rows don’t need IDs, let’s not show them --- app/templates/components/table.html | 4 ++-- app/templates/views/dashboard/_jobs.html | 3 ++- app/templates/views/providers/provider.html | 3 ++- app/templates/views/providers/providers.html | 3 ++- app/templates/views/uploads/contact-list/contact-list.html | 3 ++- 5 files changed, 10 insertions(+), 6 deletions(-) diff --git a/app/templates/components/table.html b/app/templates/components/table.html index d31f40265a..f22abfa549 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -28,12 +28,12 @@ {%- endmacro %} -{% macro list_table(items, caption='', empty_message='', field_headings=[], field_headings_visible=True, caption_visible=True, equal_length=False) -%} +{% macro list_table(items, caption='', empty_message='', field_headings=[], field_headings_visible=True, caption_visible=True, equal_length=False, give_rows_ids=True) -%} {% set parent_caller = caller %} {% call mapping_table(caption, field_headings, field_headings_visible, caption_visible, equal_length) %} {% for item in items %} - {% call row(item.id) %} + {% call row(item.id if give_rows_ids else None) %} {{ parent_caller(item, loop.index + 1) }} {% endcall %} {% endfor %} diff --git a/app/templates/views/dashboard/_jobs.html b/app/templates/views/dashboard/_jobs.html index 9878e0201e..f4bea1c450 100644 --- a/app/templates/views/dashboard/_jobs.html +++ b/app/templates/views/dashboard/_jobs.html @@ -13,7 +13,8 @@ 'File', 'Status' ], - field_headings_visible=False + field_headings_visible=False, + give_rows_ids=False, ) %} {% call row_heading() %}
    diff --git a/app/templates/views/providers/provider.html b/app/templates/views/providers/provider.html index 67c9c78c0e..55fba94d3e 100644 --- a/app/templates/views/providers/provider.html +++ b/app/templates/views/providers/provider.html @@ -28,7 +28,8 @@ caption_visible=False, empty_message='No history for this provider', field_headings=['Version', 'Last Updated', 'Updated By', 'Priority', 'Active'], - field_headings_visible=True + field_headings_visible=True, + give_rows_ids=False, ) %} {{ text_field(item.version) }} diff --git a/app/templates/views/providers/providers.html b/app/templates/views/providers/providers.html index 066cb3cc61..43e96ee897 100644 --- a/app/templates/views/providers/providers.html +++ b/app/templates/views/providers/providers.html @@ -19,7 +19,8 @@

    SMS

    caption_visible=False, empty_message='No domestic sms providers', field_headings=['Provider', 'Priority', '% monthly traffic', 'Active', 'Last Updated', 'Updated By'], - field_headings_visible=True + field_headings_visible=True, + give_rows_ids=False, ) %} {{ link_field(item.display_name, url_for('main.view_provider', provider_id=item.id)) }} diff --git a/app/templates/views/uploads/contact-list/contact-list.html b/app/templates/views/uploads/contact-list/contact-list.html index 4757ec04d3..cffb178334 100644 --- a/app/templates/views/uploads/contact-list/contact-list.html +++ b/app/templates/views/uploads/contact-list/contact-list.html @@ -36,7 +36,8 @@ 'Template', 'Status' ], - field_headings_visible=False + field_headings_visible=False, + give_rows_ids=False, ) %} {% call row_heading() %}
    From 8d91eb0f778782cb2a073833e0f9dd12ad63c650 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 13 Dec 2024 14:30:01 +0000 Subject: [PATCH 7/8] Make ids for inbound SMS unique in fixture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’re not relying on these IDs being fixed elsewhere in the tests, so let’s make them random. --- tests/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/__init__.py b/tests/__init__.py index 22dae456ea..ce54fb6c49 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -415,7 +415,7 @@ def inbound_sms_json(): "notify_number": "07900000002", "content": f"message-{index + 1}", "created_at": (datetime.utcnow() - timedelta(minutes=60 * hours_ago, seconds=index)).isoformat(), - "id": sample_uuid(), + "id": str(uuid.uuid4()), } for index, hours_ago, phone_number in ( (0, 1, "447900900000"), From 125ed671775578f67d48e636c52c9a6a44f577a5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 13 Dec 2024 14:27:51 +0000 Subject: [PATCH 8/8] Make notification IDs unique Functional tests still need these on the page: https://github.com/alphagov/notifications-functional-tests/blob/c9549ded6ab64ea61ba812a41cd7153541ceb784/tests/pages/pages.py#L917 This gives us a way of having the IDs in test fixtures still be deterministic (up to single digits in base 16), but also unique --- tests/__init__.py | 10 +++++++++- tests/app/main/views/test_jobs.py | 4 ++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/__init__.py b/tests/__init__.py index ce54fb6c49..f4320bb88e 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -1,5 +1,6 @@ import uuid from datetime import UTC, datetime, timedelta +from itertools import repeat from unittest.mock import patch from urllib.parse import parse_qs, urlparse @@ -552,10 +553,17 @@ def notification_json( # noqa: C901 if job: job_payload = {"id": job["id"], "original_file_name": job["original_file_name"]} + def _get_notification_id(index): + if index < 16: + return "{}{}{}{}{}{}{}{}-{}{}{}{}-{}{}{}{}-{}{}{}{}-{}{}{}{}{}{}{}{}{}{}{}{}".format( + *repeat(hex(index)[2:], 32) + ) + return str(uuid.uuid4()) + data = { "notifications": [ { - "id": sample_uuid(), + "id": _get_notification_id(i), "to": to, "template": template, "job": job_payload, diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 118f776208..017f11b53d 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -7,7 +7,7 @@ from freezegun import freeze_time from app.main.views.jobs import get_time_left -from tests import NotifyBeautifulSoup, job_json, notification_json, sample_uuid, user_json +from tests import NotifyBeautifulSoup, job_json, notification_json, user_json from tests.conftest import ( SERVICE_ONE_ID, create_active_caseworking_user, @@ -117,7 +117,7 @@ def test_should_show_page_for_one_job( assert page.select_one("tbody tr a")["href"] == url_for( "main.view_notification", service_id=SERVICE_ONE_ID, - notification_id=sample_uuid(), + notification_id="00000000-0000-0000-0000-000000000000", from_job=fake_uuid, )