Merge pull request #5326 from alphagov/check-for-duplicate-ids

Check for duplicate `id` attributes in HTML

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()],
     )

app/templates/components/pill.html

@@ -11,7 +11,7 @@
       {% for label, option, link, count in items %}
         <li class="pill-item__container">
         {% if current_value == option %}
-          <a id="pill-selected-item" class="pill-item pill-item--selected govuk-link govuk-link--no-visited-state{% if not show_count %} pill-item--centered{% endif %}" href="{{ link }}" aria-current="page">
+          <a class="pill-item pill-item--selected govuk-link govuk-link--no-visited-state{% if not show_count %} pill-item--centered{% endif %}" href="{{ link }}" aria-current="page">
         {% else %}
           <a class="pill-item govuk-link govuk-link--inverse" href="{{ link }}">
         {% endif %}

app/templates/components/table.html

@@ -28,12 +28,12 @@
   </table>
 {%- 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 %}

app/templates/views/accessibility_statement.html

@@ -16,7 +16,7 @@ <h1 class="heading-large">Accessibility statement</h1>
       "Next review due": "29 January 2025"
     }
     ) }}
-    {# Last non-functional changes: 4 June 2024 #}
+    {# Last non-functional changes: 13 December 2024 #}
 
   <p class="govuk-body">
     This accessibility statement applies to the www.notifications.service.gov.uk domain. It does not apply to the <a class="govuk-link govuk-link--no-visited-state" href="https://docs.notifications.service.gov.uk">GOV.UK Notify API documentation subdomain</a>.
@@ -82,7 +82,7 @@ <h2 class="heading-medium" id="technical-information">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.
   </p>
 
-  <h2 class="heading-medium" id="non-accessible-content">Compliance status</h2>
+  <h2 class="heading-medium" id="compliance-status">Compliance status</h2>
 
   <p class="govuk-body">
     We have tested the Notify website against the Web Content Accessibility Guidelines (WCAG) 2.2 AA standard.

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() %}
       <div class="file-list">

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) }}

app/templates/views/providers/providers.html

@@ -19,7 +19,8 @@ <h2 class="heading-medium">SMS</h2>
       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)) }}

app/templates/views/unsubscribe-request-report.html

@@ -56,9 +56,7 @@
 {% endcall %}
 <div id="unsubscribe_report_availability">
     {% if report.is_a_batched_report %}
-        This <span id="unsubscribe_report_availability">
-            report will be deleted {{ report.will_be_archived_at|format_delta_days(numeric_prefix="in ")}}.
-        </span>
+        This report will be deleted {{ report.will_be_archived_at|format_delta_days(numeric_prefix="in ")}}.
     {% else %}
         <p class="govuk-body">
             Once downloaded, reports are available for 7 days.

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() %}
           <div class="file-list">

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
 
@@ -415,7 +416,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"),
@@ -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,

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,
     )
 

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