Merge pull request #65 from NHSDigital/dtoss-9144-link-radio-errors-to-first-choice

MatMoore · web-flow · commit 5c5afc0665d8 · 2025-05-27T10:14:18.000+01:00
Correctly link errors to the matching form field
diff --git a/manage_breast_screening/config/system_test_setup.py b/manage_breast_screening/config/system_test_setup.py
@@ -2,7 +2,7 @@
 
 import pytest
 from django.contrib.staticfiles.testing import StaticLiveServerTestCase
-from playwright.sync_api import sync_playwright, expect
+from playwright.sync_api import expect, sync_playwright
 
 
 @pytest.mark.system
@@ -27,11 +27,28 @@ def setUp(self):
     def tearDown(self):
         self.page.close()
 
-    def expect_validation_error(self, id: str, fieldset_legend: str, error_text: str):
+    def expect_validation_error(
+        self,
+        error_text: str,
+        fieldset_legend: str,
+        field_label: str,
+        field_name: str | None = "",
+    ):
         summary_box = self.page.locator(".nhsuk-error-summary")
-        error_link = summary_box.locator(f"a[href='#{id}']")
-        expect(error_link).to_have_text(error_text)
+        expect(summary_box).to_contain_text(error_text)
 
-        fieldset = self.page.locator('fieldset').filter(has_text=fieldset_legend)
+        error_link = summary_box.get_by_text(error_text)
+        error_link.click()
+
+        fieldset = self.page.locator("fieldset").filter(has_text=fieldset_legend)
         error_span = fieldset.locator("span").filter(has_text=error_text)
-        expect(error_span).to_have_id(id)
+        expect(error_span).to_contain_text(error_text)
+
+        if field_name:
+            field = fieldset.get_by_label(field_label).and_(
+                fieldset.locator(f"[name='{field_name}']")
+            )
+        else:
+            field = fieldset.get_by_label(field_label)
+
+        expect(field).to_be_focused()
diff --git a/manage_breast_screening/record_a_mammogram/forms.py b/manage_breast_screening/record_a_mammogram/forms.py
@@ -68,6 +68,12 @@ def __init__(self, *args, **kwargs):
         for field_name, _ in self.STOPPED_REASON_CHOICES:
             self.fields[f"{field_name}_details"] = forms.CharField(required=False)
 
+        # Ensure that the field order matches the order we want to render in
+        details_fields = [
+            f"{field_name}_details" for field_name, _ in self.STOPPED_REASON_CHOICES
+        ]
+        self.order_fields(["stopped_reasons"] + details_fields + ["decision"])
+
     stopped_reasons = forms.MultipleChoiceField(
         choices=STOPPED_REASON_CHOICES,
         required=True,
diff --git a/manage_breast_screening/record_a_mammogram/tests/system/test_recording_a_mammogram.py b/manage_breast_screening/record_a_mammogram/tests/system/test_recording_a_mammogram.py
@@ -20,6 +20,9 @@ def before(self):
         self.appointment = AppointmentFactory(screening_episode=self.screening_episode)
 
     def test_recording_a_mammogram_without_capturing_medical_information(self):
+        """
+        I can record a mammogram without entering any relevant medical information.
+        """
         self.given_i_am_on_the_start_screening_page()
         self.then_i_should_see_the_demographic_banner()
         self.and_i_should_see_the_participant_details()
@@ -28,9 +31,30 @@ def test_recording_a_mammogram_without_capturing_medical_information(self):
         self.then_i_should_be_on_the_medical_information_page()
         self.and_i_should_be_prompted_to_ask_about_relevant_medical_information()
 
-        self.when_the_participant_shares_no_relevant_medical_information()
+        self.when_i_mark_that_the_participant_shared_no_medical_information()
         self.then_the_screen_should_show_that_it_is_awaiting_images_from_the_PACS()
 
+    def test_filling_out_forms_incorrectly(self):
+        """
+        At each step in the flow, when I fill out the forms incorrectly,
+        then I should see the errors so I can fix them.
+        """
+        self.given_i_am_on_the_start_screening_page()
+        self.when_i_submit_the_form()
+        self.then_i_am_prompted_to_answer_can_the_screening_go_ahead()
+
+        self.when_i_check_the_participants_identity_and_confirm_the_last_mammogram_date()
+        self.then_i_should_be_on_the_medical_information_page()
+
+        self.when_i_submit_the_form()
+        self.then_i_am_prompted_to_answer_has_the_participant_shared_medical_info()
+
+        self.when_i_mark_that_the_participant_shared_medical_information()
+        self.then_i_should_be_on_the_record_medical_information_page()
+
+        self.when_i_submit_the_form()
+        self.then_i_am_prompted_to_confirm_whether_imaging_can_go_ahead()
+
     def given_i_am_on_the_start_screening_page(self):
         self.page.goto(
             self.live_server_url
@@ -54,27 +78,62 @@ def when_i_check_the_participants_identity_and_confirm_the_last_mammogram_date(
         self.page.get_by_label("Yes, go to medical information").check()
         self.page.get_by_role("button", name="Continue").click()
 
+    def when_i_submit_the_form(self):
+        self.page.get_by_role("button", name="Continue").click()
+
     def then_i_should_be_on_the_medical_information_page(self):
         path = reverse(
             "record_a_mammogram:ask_for_medical_information",
             kwargs={"id": self.appointment.pk},
         )
         expect(self.page).to_have_url(re.compile(path))
 
+    def then_i_should_be_on_the_record_medical_information_page(self):
+        path = reverse(
+            "record_a_mammogram:record_medical_information",
+            kwargs={"id": self.appointment.pk},
+        )
+        expect(self.page).to_have_url(re.compile(path))
+
     def and_i_should_be_prompted_to_ask_about_relevant_medical_information(self):
         expect(
             self.page.get_by_text(
                 "Has the participant shared any relevant medical information?"
             )
         ).to_be_visible()
 
-    def when_the_participant_shares_no_relevant_medical_information(self):
+    def when_i_mark_that_the_participant_shared_no_medical_information(self):
         self.page.get_by_label("No - proceed to imaging").check()
         self.page.get_by_role("button", name="Continue").click()
 
+    def when_i_mark_that_the_participant_shared_medical_information(self):
+        self.page.get_by_label("Yes").check()
+        self.page.get_by_role("button", name="Continue").click()
+
     def then_the_screen_should_show_that_it_is_awaiting_images_from_the_PACS(self):
         path = reverse(
             "record_a_mammogram:awaiting_images",
             kwargs={"id": self.appointment.pk},
         )
         expect(self.page).to_have_url(re.compile(path))
+
+    def then_i_am_prompted_to_answer_can_the_screening_go_ahead(self):
+        self.expect_validation_error(
+            error_text="This field is required.",
+            fieldset_legend="Can the appointment go ahead?",
+            field_label="Yes, go to medical information",
+        )
+
+    def then_i_am_prompted_to_answer_has_the_participant_shared_medical_info(self):
+        self.expect_validation_error(
+            error_text="This field is required.",
+            fieldset_legend="Has the participant shared any relevant medical information?",
+            field_label="Yes",
+        )
+
+    def then_i_am_prompted_to_confirm_whether_imaging_can_go_ahead(self):
+        self.expect_validation_error(
+            error_text="This field is required.",
+            fieldset_legend="Can imaging go ahead?",
+            field_label="Yes, mark incomplete sections as ‘none’ or ‘no’",
+        )
diff --git a/manage_breast_screening/record_a_mammogram/tests/system/test_user_submits_cannot_go_ahead_form.py b/manage_breast_screening/record_a_mammogram/tests/system/test_user_submits_cannot_go_ahead_form.py
@@ -1,12 +1,17 @@
 import re
+
 import pytest
 from django.urls import reverse
 from playwright.sync_api import expect
 
+from manage_breast_screening.clinics.models import Appointment
+from manage_breast_screening.clinics.tests.factories import (
+    AppointmentFactory,
+    ScreeningEpisodeFactory,
+)
 from manage_breast_screening.config.system_test_setup import SystemTestCase
-from manage_breast_screening.clinics.tests.factories import AppointmentFactory, ScreeningEpisodeFactory
 from manage_breast_screening.participants.tests.factories import ParticipantFactory
-from manage_breast_screening.clinics.models import Appointment
+
 
 class TestUserSubmitsCannotGoAheadForm(SystemTestCase):
     @pytest.fixture(autouse=True)
@@ -31,11 +36,12 @@ def test_user_submits_cannot_go_ahead_form(self):
         self.then_i_see_the_clinics_page()
         self.and_the_appointment_is_updated()
 
-
     def given_i_am_on_the_cannot_go_ahead_form(self):
         self.page.goto(
-            self.live_server_url + reverse(
-                "record_a_mammogram:appointment_cannot_go_ahead", kwargs={"id": self.appointment.pk}
+            self.live_server_url
+            + reverse(
+                "record_a_mammogram:appointment_cannot_go_ahead",
+                kwargs={"id": self.appointment.pk},
             )
         )
 
@@ -44,14 +50,14 @@ def when_i_submit_the_form(self):
 
     def then_i_should_see_validation_errors(self):
         self.expect_validation_error(
-            id="stopped_reasons-error",
-            fieldset_legend="Why has this appointment been stopped?",
             error_text="A reason for why this appointment cannot continue must be provided",
+            fieldset_legend="Why has this appointment been stopped?",
+            field_label="Participant did not attend",
         )
         self.expect_validation_error(
-            id="decision-error",
+            error_text="Select whether the participant needs to be invited for another appointment",
             fieldset_legend="Does the appointment need to be rescheduled?",
-            error_text="Select whether the participant needs to be invited for another appointment"
+            field_label="Yes, add participant to reinvite list",
         )
 
     def when_i_select_a_reason_for_the_appointment_being_stopped(self):
@@ -62,28 +68,34 @@ def and_i_select_other_as_a_reason(self):
 
     def and_i_choose_to_add_the_participant_to_the_reinvite_list(self):
         self.page.get_by_label("Yes, add participant to reinvite list").click()
-        expect(self.page.get_by_label("Yes, add participant to reinvite list")).to_be_checked()
+        expect(
+            self.page.get_by_label("Yes, add participant to reinvite list")
+        ).to_be_checked()
 
     def then_i_see_an_error_for_other_details(self):
         self.expect_validation_error(
-            id="other_details-error",
-            fieldset_legend="Why has this appointment been stopped?",
             error_text="Explain why this appointment cannot proceed",
+            fieldset_legend="Why has this appointment been stopped?",
+            field_label="Provide details",
+            field_name="other_details",
         )
 
     def when_i_fill_in_other_details(self):
-        self.page.locator("#other_details").fill("Explain other choice")
+        self.page.locator("#id_other_details").fill("Explain other choice")
 
     def then_i_see_the_clinics_page(self):
-        expect(self.page).to_have_url(
-            re.compile(reverse("clinics:index"))
-        )
+        expect(self.page).to_have_url(re.compile(reverse("clinics:index")))
 
     def and_the_appointment_is_updated(self):
         self.appointment.refresh_from_db()
-        self.assertEqual(self.appointment.status, Appointment.Status.ATTENDED_NOT_SCREENED)
+        self.assertEqual(
+            self.appointment.status, Appointment.Status.ATTENDED_NOT_SCREENED
+        )
         self.assertEqual(self.appointment.reinvite, True)
-        self.assertEqual(self.appointment.stopped_reasons, {
-            "stopped_reasons": ["failed_identity_check", "other"],
-            "other_details": "Explain other choice"
-        })
+        self.assertEqual(
+            self.appointment.stopped_reasons,
+            {
+                "stopped_reasons": ["failed_identity_check", "other"],
+                "other_details": "Explain other choice",
+            },
+        )
diff --git a/manage_breast_screening/templates/record_a_mammogram/appointment_cannot_go_ahead.jinja b/manage_breast_screening/templates/record_a_mammogram/appointment_cannot_go_ahead.jinja
@@ -6,15 +6,17 @@
 {% block form %}
 {% macro conditionalCheckboxInput(params) %}
   {% set details_field_name = params.field_name + "_details" %}
+  {% set details_field_id = params.details_field.auto_id %}
+  {% set details_field_value = params.details_field.value() %}
   {% set input_params = {
     "label": {
       "text": "Provide details"
     },
-    "id": details_field_name,
+    "id": details_field_id,
     "name": details_field_name,
-    "value": params.details_value
+    "value": details_field_value
   } %}
-  {% if form[details_field_name].errors %}
+  {% if params.details_field.errors %}
     {% set error_params = {
       "errorMessage": {
         "text": form[details_field_name].errors | first
@@ -31,20 +33,21 @@
 {% for field_name, label in form.stopped_reasons.field.choices %}
     {% do checkboxItems.append({
       "text": label,
+      "id": form.stopped_reasons.auto_id if loop.first,
       "value": field_name,
       "checked": field_name in (form.stopped_reasons.value() or []),
       "conditional": {
         "html": conditionalCheckboxInput({
           "field_name": field_name,
-          "details_value": form[field_name + "_details"].value()
+          "details_field": form[field_name + "_details"],
         })
       }
     }) %}
 {% endfor %}
 
 {% set checkbox_params = {
-  "idPrefix": "stopped_reasons",
-  "name": "stopped_reasons",
+  "idPrefix": form.stopped_reasons.auto_id,
+  "name": form.stopped_reasons.html_name,
   "fieldset": {
     "legend": {
       "text": "Why has this appointment been stopped?",
diff --git a/manage_breast_screening/templates/wizard_step.jinja b/manage_breast_screening/templates/wizard_step.jinja
@@ -17,8 +17,11 @@
     {% if form.errors %}
       {% set ns = namespace(errors=[]) %}
 
-      {% for field, messages in form.errors | items %}
-        {% set ns.errors = ns.errors + [{"text": ",".join(messages), "href": "#" ~ field ~ "-error"}] %}
+      {% for field in form %}
+        {% set ns.errors = ns.errors + [{"text": ",".join(field.errors), "href": "#" ~ field.auto_id}] %}
+      {% endfor %}
+      {% for error_list in form.non_field_errors() %}
+        {% set ns.errors = ns.errors + [{"text": ",".join(error_list)}] %}
       {% endfor %}
 
       {{ errorSummary({
@@ -56,6 +59,7 @@
 
             {{ radios({
               "name": form.decision.html_name,
+              "idPrefix": form.decision.auto_id,
               "fieldset": {
                 "legend": {
                   "text": decision_legend,
@@ -69,6 +73,7 @@
               } if decision_hint,
               "items": [
                 {
+                  "id": form.decision.auto_id,
                   "value": form.decision.field.choices[0][0],
                   "text": form.decision[0].choice_label,
                   "checked": form.decision.value() == form.decision.field.choices[0][0]
