Add benign lump form #739

malcolmbaig · 2025-11-18T15:29:56Z

Description

Add the form for adding a benign lump history item.

See commit messages for summary of changes.

Validation errors — no data submitted

Validation errors — no location details

Jira link

https://nhsd-jira.digital.nhs.uk/browse/DTOSS-11451

Review notes

~~The YearField isn't customizable in terms of the range it checks for. Does it need to be at this stage?~~ < no longer the case after the changes introduced following this comment Add benign lump form #739 (comment)

Review checklist

Check database queries are correctly scoped to current_provider

swebberuk · 2025-11-20T10:51:13Z

Do we need to add a test_benign_lump_history_item_views.py?

I found we'd done something similar for sympton views:
https://github.com/NHSDigital/dtos-manage-breast-screening/blob/main/manage_breast_screening/mammograms/tests/views/test_symptom_views.py

So I did the same for the implanted medical devices changes I made:
https://github.com/NHSDigital/dtos-manage-breast-screening/blob/main/manage_breast_screening/mammograms/tests/views/test_implanted_medical_device_history_views.py

Maybe we feel that behaviour is already sufficiently covered by the playwright tests in test_benign_lump_history.py?

MatMoore · 2025-11-20T11:14:51Z

manage_breast_screening/nhsuk_forms/fields/integer_field.py

+        **kwargs,
+    ):
+        if min_value is None:
+            min_value = date.today().year - 80


Just noticed a possible problem here - fields can be created when the form is declared, rather than when the form is instantiated, so the field's validation could get stale if the app doesn't restart. Maybe less of an issue here, but definitely in the SplitDateField.

Is it possible to make min_value/max_value a callable?

Yes, good point. I've attempted to address this in 6cc2573. WDYT?

looks good to me 👍🏻

MatMoore · 2025-11-20T11:17:36Z

manage_breast_screening/mammograms/views/benign_lump_history_item_views.py

+        messages.add_message(
+            self.request,
+            messages.SUCCESS,
+            "Added benign lump history",


I think the pattern elsewhere is "Noun added" rather than "Added noun" - not sure which we prefer but they should be consistent

manage_breast_screening/mammograms/forms/benign_lump_history_item_form.py

MatMoore

I think there might be some visually hidden text missing - see inline comment

edwardhorsford · 2025-11-20T12:13:41Z

We'll want a specific error for the conditional text inputs for treatment location.

malcolmbaig · 2025-11-21T11:05:26Z

Do we need to add a test_benign_lump_history_item_views.py?

Maybe we feel that behaviour is already sufficiently covered by the playwright tests in test_benign_lump_history.py?

@swebberuk exactly this — I tend to add view tests if there's some complex behaviour (or permutations of some kind) that aren't adequately covered by the system test. In this case, I think the system test is sufficient.

swebberuk · 2025-11-21T15:18:36Z

manage_breast_screening/nhsuk_forms/fields/integer_field.py

+                    "max_value", "Year must be less than %(max_value)s"
+                ),
+                code="max_value",
+                params={"max_value": max_value},


If using the message "Year must be less than %(max_value)s" then should this be: params={"max_value": max_value + 1},

It was suggested on Slack that the error message could be "Year of procedure cannot be in the future" - so don't know if we need to make it configurable? To avoid blocking merging this PR, maybe we can review the error message as part of a separate ticket?

Here is a link to a related comment from a different PR: #742 (comment)

Maybe we can raise another ticket that looks at both the YearField and the SplitDateField? Both will need some tweaking to the error messages I think

I think a ticket to revisit these would be a good idea, including potentially making the error message configurable. For this PR, I'll adjust these to "Year must be (value) or later" and "Year must be (value) or earlier".

swebberuk · 2025-11-21T15:19:32Z

manage_breast_screening/nhsuk_forms/fields/integer_field.py

+                    "min_value", "Year must be greater than %(min_value)s"
+                ),
+                code="min_value",
+                params={"min_value": min_value},


Similar question as for max value validation, should this be: params={"min_value": min_value - 1},

manage_breast_screening/mammograms/forms/benign_lump_history_item_form.py

manage_breast_screening/nhsuk_forms/fields/integer_field.py

malcolmbaig · 2025-11-24T10:53:07Z

We'll want a specific error for the conditional text inputs for treatment location.

Updated in 6ba8a1c and screenshot replaced in PR description

github-actions · 2025-11-24T11:00:31Z

The review app at this URL has been deleted:
https://pr-739.manage-breast-screening.non-live.screening.nhs.uk

swebberuk · 2025-11-24T11:28:23Z

manage_breast_screening/nhsuk_forms/tests/fields/test_integer_field.py

+        bounds["max"] = 2002
+        form = TestForm(data={"field": 2003})
+        assert not form.is_valid()
+        assert form.errors["field"] == ["Year must be less than 2002"]


Need to change to Year must be 2002 or earlier and line 56 to Year must be 2000 or later.

Introduce the initial template, form object and routing for creating benign lump history items on the medical info screen.

- Introduce YearField as a specific type of IntegerField that validates for a year between 80 years ago and today. - Use this in the benign lump history item form.

Matching the choice in the prototype to make this heading aria-hidden=true.

The current YearField validation on min and max value is implemented in a way that means values based on the current year will only be evaluated on module import. An application restart after moving from one year to the next would be needed to ensure current year is correct. This isn't a huge bug necessarily with our current use cases, but it's an inconvenient issue to leave in the codebase. This commit switches to using min_value_callable and max_value_callable to determine the year bounds. These can be overridden, but by default specify a range of 80 years to current year.

swebberuk

👍

malcolmbaig force-pushed the 11451-add-benign-lump branch 2 times, most recently from c07c407 to e4e0a15 Compare November 19, 2025 15:45

malcolmbaig changed the title ~~[wip] Add benign lump form~~ Add benign lump form Nov 19, 2025

malcolmbaig marked this pull request as ready for review November 19, 2025 16:39

malcolmbaig requested a review from a team as a code owner November 19, 2025 16:39

malcolmbaig force-pushed the 11451-add-benign-lump branch from cb048cf to 1665396 Compare November 19, 2025 16:40

malcolmbaig added the deploy label Nov 19, 2025

malcolmbaig had a problem deploying to review November 19, 2025 16:43 — with GitHub Actions Failure

MatMoore reviewed Nov 20, 2025

View reviewed changes

manage_breast_screening/mammograms/forms/benign_lump_history_item_form.py Show resolved Hide resolved

MatMoore requested changes Nov 20, 2025

View reviewed changes

MatMoore mentioned this pull request Nov 20, 2025

Breast cancer history add form #708

Merged

1 task

swebberuk reviewed Nov 21, 2025

View reviewed changes

MatMoore reviewed Nov 24, 2025

View reviewed changes

manage_breast_screening/mammograms/forms/benign_lump_history_item_form.py Show resolved Hide resolved

manage_breast_screening/nhsuk_forms/fields/integer_field.py Outdated Show resolved Hide resolved

malcolmbaig force-pushed the 11451-add-benign-lump branch from 6ba8a1c to 695bee4 Compare November 24, 2025 10:55

malcolmbaig temporarily deployed to review November 24, 2025 10:56 — with GitHub Actions Inactive

swebberuk reviewed Nov 24, 2025

View reviewed changes

malcolmbaig added 6 commits November 24, 2025 11:53

Add create benign lumps form

09dd3e4

Introduce the initial template, form object and routing for creating benign lump history items on the medical info screen.

Add YearField to encapsulate year bound validation logic

e068b7d

- Introduce YearField as a specific type of IntegerField that validates for a year between 80 years ago and today. - Use this in the benign lump history item form.

Benign lumps: Make NO_PROCEDURES an exclusive choice

e969b73

Benign lumps: Make procedures legend aria-hidden

78580bb

Matching the choice in the prototype to make this heading aria-hidden=true.

Benign lumps: Update validation error message wording

fcd0d41

Benign lumps: Add system test for creating

471a5f1

malcolmbaig added 5 commits November 24, 2025 11:53

Benign lumps: Add tests for form object

f53dc12

Benign lumps: rework success flash message warning

9a4ccbe

Benign lumps: add visually hidden text to left/right breast fields

9372a87

Benign lumps: add more detailed validation message to location details

43d26fa

malcolmbaig force-pushed the 11451-add-benign-lump branch from 695bee4 to 43d26fa Compare November 24, 2025 11:53

malcolmbaig temporarily deployed to review November 24, 2025 11:55 — with GitHub Actions Inactive

swebberuk approved these changes Nov 24, 2025

View reviewed changes

Benign lumps: update system spec

d5219d0

MatMoore approved these changes Nov 24, 2025

View reviewed changes

malcolmbaig temporarily deployed to review November 24, 2025 14:29 — with GitHub Actions Inactive

This was referenced Nov 24, 2025

Add mastectomy or lumpectomy history form #754

Merged

Make 'Year removed' optional for 'Implanted device has been removed' #761

Merged

malcolmbaig merged commit 81a64ea into main Nov 24, 2025
13 checks passed

malcolmbaig deleted the 11451-add-benign-lump branch November 24, 2025 16:19

malcolmbaig temporarily deployed to review November 24, 2025 16:19 — with GitHub Actions Inactive

Add benign lump form #739

Add benign lump form #739

Uh oh!

Conversation

malcolmbaig commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Validation errors — no data submitted

Validation errors — no location details

Jira link

Review notes

Review checklist

Uh oh!

swebberuk commented Nov 20, 2025

Uh oh!

MatMoore Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

malcolmbaig Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

MatMoore Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

MatMoore Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

malcolmbaig Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MatMoore left a comment

Choose a reason for hiding this comment

Uh oh!

edwardhorsford commented Nov 20, 2025

Uh oh!

malcolmbaig commented Nov 21, 2025

Uh oh!

swebberuk Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatMoore Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

malcolmbaig Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swebberuk Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

malcolmbaig commented Nov 24, 2025

Uh oh!

github-actions bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swebberuk Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swebberuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

malcolmbaig commented Nov 18, 2025 •

edited

Loading

swebberuk Nov 21, 2025 •

edited

Loading

malcolmbaig Nov 24, 2025 •

edited

Loading

swebberuk Nov 21, 2025 •

edited

Loading

github-actions bot commented Nov 24, 2025 •

edited

Loading

swebberuk Nov 24, 2025 •

edited

Loading