code review changes for breast augmentation

Author: swebberuk

manage_breast_screening/data/east_tester_one_week_ago_clinic_data.yml

@@ -135,6 +135,7 @@ clinic:
                 - OTHER_AUGMENTATION
             - left_breast_procedures:
                 - NO_PROCEDURES
+            - implants_have_been_removed: False
     - id: 848c9ecf-94af-45f7-b072-29ba1f82d24b
       duration_in_minutes: 8
       starts_at_time: 09:32

manage_breast_screening/mammograms/forms/breast_augmentation_history_form.py

@@ -2,6 +2,7 @@
 
 from django import forms
 from django.db.models import TextChoices
+from django.forms import Form
 from django.forms.widgets import Textarea
 
 from manage_breast_screening.core.services.auditor import Auditor
@@ -12,7 +13,6 @@
 from manage_breast_screening.nhsuk_forms.fields.choice_fields import (
     MultipleChoiceField,
 )
-from manage_breast_screening.nhsuk_forms.forms import FormWithConditionalFields
 from manage_breast_screening.participants.models.breast_augmentation_history_item import (
     BreastAugmentationHistoryItem,
 )
@@ -22,7 +22,7 @@ class RemovalStatusChoices(TextChoices):
     HAS_BEEN_REMOVED = "HAS_BEEN_REMOVED", "Implants have been removed"
 
 
-class BreastAugmentationHistoryForm(FormWithConditionalFields):
+class BreastAugmentationHistoryForm(Form):
     right_breast_procedures = MultipleChoiceField(
         label="Right breast",
         label_classes="nhsuk-fieldset__legend--s",
@@ -90,24 +90,20 @@ def __init__(self, *args, participant, **kwargs):
             min_value=min_year,
             max_value=max_year,
             error_messages={
-                "required": "Enter the year the implant was removed",
                 "min_value": year_outside_range_error_message,
                 "max_value": year_outside_range_error_message,
                 "invalid": year_invalid_format_error_message,
             },
         )
 
-        self.given_field_value(
-            "removal_status", RemovalStatusChoices.HAS_BEEN_REMOVED
-        ).require_field("removal_year")
-
     def model_values(self):
         return dict(
             left_breast_procedures=self.cleaned_data.get("left_breast_procedures", []),
             right_breast_procedures=self.cleaned_data.get(
                 "right_breast_procedures", []
             ),
-            removal_year=self.cleaned_data.get("removal_year", ""),
+            implants_have_been_removed=_have_implants_been_removed(self.cleaned_data),
+            removal_year=self.cleaned_data.get("removal_year"),
             procedure_year=self.cleaned_data.get("procedure_year", ""),
             additional_details=self.cleaned_data.get("additional_details", ""),
         )
@@ -129,6 +125,11 @@ def clean(self):
         cleaned_data = super().clean()
         procedure_year = cleaned_data.get("procedure_year")
         removal_year = cleaned_data.get("removal_year")
+
+        if removal_year and not _have_implants_been_removed(cleaned_data):
+            removal_year = None
+            cleaned_data["removal_year"] = removal_year
+
         if procedure_year and removal_year and procedure_year > removal_year:
             self.add_error(
                 "removal_year",
@@ -139,3 +140,7 @@ def clean(self):
             )
 
         return cleaned_data
+
+
+def _have_implants_been_removed(data):
+    return RemovalStatusChoices.HAS_BEEN_REMOVED in data.get("removal_status", [])

manage_breast_screening/mammograms/tests/forms/test_breast_augmentation_history_form.py

@@ -156,7 +156,6 @@ def test_removal_year_invalid_format(self, clinical_user):
         assert form.errors == {
             "removal_year": [
                 "Enter year as a number.",
-                "Enter the year the implant was removed",
             ]
         }
 
@@ -244,7 +243,6 @@ def test_removal_year_outside_range(self, clinical_user, removal_year):
         assert form.errors == {
             "removal_year": [
                 year_outside_range_error_message,
-                "Enter the year the implant was removed",
             ]
         }
 
@@ -288,34 +286,6 @@ def test_removal_year_before_procedure_year(
             "removal_year": ["Year removed cannot be before year of procedure"]
         }
 
-    def test_has_been_removed_without_removal_date(self, clinical_user):
-        appointment = AppointmentFactory()
-        request = RequestFactory().get("/test-form")
-        request.user = clinical_user
-
-        form = BreastAugmentationHistoryForm(
-            QueryDict(
-                urlencode(
-                    {
-                        "right_breast_procedures": [
-                            BreastAugmentationHistoryItem.Procedure.BREAST_IMPLANTS
-                        ],
-                        "left_breast_procedures": [
-                            BreastAugmentationHistoryItem.Procedure.BREAST_IMPLANTS
-                        ],
-                        "removal_status": RemovalStatusChoices.HAS_BEEN_REMOVED,
-                    },
-                    doseq=True,
-                ),
-            ),
-            participant=appointment.participant,
-        )
-
-        assert not form.is_valid()
-        assert form.errors == {
-            "removal_year": ["Enter the year the implant was removed"]
-        }
-
     @pytest.mark.parametrize(
         "data",
         [
@@ -387,6 +357,15 @@ def test_has_been_removed_without_removal_date(self, clinical_user):
                 "removal_status": RemovalStatusChoices.HAS_BEEN_REMOVED,
                 "removal_year": 2015,
             },
+            {
+                "right_breast_procedures": [
+                    BreastAugmentationHistoryItem.Procedure.BREAST_IMPLANTS
+                ],
+                "left_breast_procedures": [
+                    BreastAugmentationHistoryItem.Procedure.BREAST_IMPLANTS
+                ],
+                "removal_status": RemovalStatusChoices.HAS_BEEN_REMOVED,
+            },
         ],
     )
     def test_success(self, clinical_user, data):
@@ -407,5 +386,8 @@ def test_success(self, clinical_user, data):
         assert obj.left_breast_procedures == data.get("left_breast_procedures")
         assert obj.right_breast_procedures == data.get("right_breast_procedures")
         assert obj.procedure_year == data.get("procedure_year", None)
+        assert obj.implants_have_been_removed == (
+            RemovalStatusChoices.HAS_BEEN_REMOVED in data.get("removal_status", [])
+        )
         assert obj.removal_year == data.get("removal_year", None)
         assert obj.additional_details == data.get("additional_details", "")

manage_breast_screening/participants/migrations/0043_alter_breastaugmentationhistoryitem_implants_have_been_removed_and_more.py

@@ -0,0 +1,40 @@
+# Generated by Django 5.2.8 on 2025-11-19 17:29
+
+import django.contrib.postgres.fields
+import manage_breast_screening.nhsuk_forms.validators
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('participants', '0042_alter_implantedmedicaldevicehistoryitem_device'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='breastaugmentationhistoryitem',
+            name='implants_have_been_removed',
+            field=models.BooleanField(),
+        ),
+        migrations.AlterField(
+            model_name='breastaugmentationhistoryitem',
+            name='left_breast_procedures',
+            field=django.contrib.postgres.fields.ArrayField(base_field=models.CharField(choices=[('BREAST_IMPLANTS', 'Breast implants (silicone or saline)'), ('OTHER_AUGMENTATION', 'Other augmentation'), ('NO_PROCEDURES', 'No procedures')]), default=list, size=None, validators=[manage_breast_screening.nhsuk_forms.validators.ExcludesOtherOptionsValidator('NO_PROCEDURES', 'No procedures')]),
+        ),
+        migrations.AlterField(
+            model_name='breastaugmentationhistoryitem',
+            name='procedure_year',
+            field=models.IntegerField(blank=True, null=True),
+        ),
+        migrations.AlterField(
+            model_name='breastaugmentationhistoryitem',
+            name='removal_year',
+            field=models.IntegerField(blank=True, null=True),
+        ),
+        migrations.AlterField(
+            model_name='breastaugmentationhistoryitem',
+            name='right_breast_procedures',
+            field=django.contrib.postgres.fields.ArrayField(base_field=models.CharField(choices=[('BREAST_IMPLANTS', 'Breast implants (silicone or saline)'), ('OTHER_AUGMENTATION', 'Other augmentation'), ('NO_PROCEDURES', 'No procedures')]), default=list, size=None, validators=[manage_breast_screening.nhsuk_forms.validators.ExcludesOtherOptionsValidator('NO_PROCEDURES', 'No procedures')]),
+        ),
+    ]

manage_breast_screening/participants/migrations/max_migration.txt

@@ -1 +1 @@
-0042_alter_implantedmedicaldevicehistoryitem_device
+0043_alter_breastaugmentationhistoryitem_implants_have_been_removed_and_more

manage_breast_screening/participants/models/breast_augmentation_history_item.py

@@ -2,6 +2,7 @@
 from django.db import models
 
 from manage_breast_screening.core.models import BaseModel
+from manage_breast_screening.nhsuk_forms.validators import ExcludesOtherOptionsValidator
 from manage_breast_screening.participants.models.appointment import Appointment
 
 
@@ -20,18 +21,25 @@ class Procedure(models.TextChoices):
         on_delete=models.PROTECT,
         related_name="breast_augmentation_history_items",
     )
-    # TODO should we be setting validators here to use ExcludesOtherOptionsValidator?
     right_breast_procedures = ArrayField(
-        base_field=models.CharField(choices=Procedure), default=list
+        base_field=models.CharField(choices=Procedure),
+        default=list,
+        validators=[
+            ExcludesOtherOptionsValidator(
+                Procedure.NO_PROCEDURES.value, Procedure.NO_PROCEDURES.label
+            )
+        ],
     )
-    # TODO should we be setting validators here to use ExcludesOtherOptionsValidator?
     left_breast_procedures = ArrayField(
-        base_field=models.CharField(choices=Procedure), default=list
+        base_field=models.CharField(choices=Procedure),
+        default=list,
+        validators=[
+            ExcludesOtherOptionsValidator(
+                Procedure.NO_PROCEDURES.value, Procedure.NO_PROCEDURES.label
+            )
+        ],
     )
-    # TODO should procedure_year be blank=True as well as null=True?
-    procedure_year = models.IntegerField(null=True)
-    # TODO should we remove implants_have_been_removed? could instead rely on removal_year being set or not?
-    implants_have_been_removed = models.BooleanField(default=False)
-    # TODO should removal_year be blank=True as well as null=True?
-    removal_year = models.IntegerField(null=True)
+    procedure_year = models.IntegerField(null=True, blank=True)
+    implants_have_been_removed = models.BooleanField()
+    removal_year = models.IntegerField(null=True, blank=True)
     additional_details = models.TextField(null=False, blank=True, default="")

manage_breast_screening/participants/tests/factories.py

@@ -195,6 +195,7 @@ class Meta:
     surgery_reason = MastectomyOrLumpectomyHistoryItem.SurgeryReason.OTHER_REASON
     additional_details = ""
 
+
 class CystHistoryItemFactory(DjangoModelFactory):
     class Meta:
         model = models.CystHistoryItem
@@ -222,6 +223,7 @@ class Meta:
     left_breast_procedures = [
         models.BreastAugmentationHistoryItem.Procedure.NO_PROCEDURES
     ]
+    implants_have_been_removed = False
 
 
 class OtherProcedureHistoryItemFactory(DjangoModelFactory):