care/facility/models/patient_investigation.py (3)

42-47: I couldn't help but notice some... interesting... style choices

While the validation logic is perfect (really, it is), there are a couple of things that caught my eye:

• That lonely empty line at 45 seems a bit out of place
• The return statement before super().delete() is unnecessary since delete methods typically return None

Here's a slightly cleaner version:

 def delete(self, *args):
     if InvestigationValue.objects.filter(investigation=self).exists():
         error = f"Cannot delete PatientInvestigation {self} because they are referenced as `investigation` in InvestigationValue records."
-
         raise ValidationError(error)
-    return super().delete(*args)
+    super().delete(*args)

---

58-62: Another return statement and a rather cryptic error message

The validation logic is solid, but:

• Using external_id in the error message might not be very user-friendly
• There's that unnecessary return statement again

Consider this more user-friendly version:

 def delete(self, *args):
     if InvestigationValue.objects.filter(session=self).exists():
-        error = f"Cannot delete InvestigationSession {self.external_id} because they are referenced as `session` in InvestigationValue records."
+        error = f"Cannot delete Investigation Session #{self.id} because it has associated investigation values."
         raise ValidationError(error)
-    return super().delete(*args)
+    super().delete(*args)

---

Line range hint 19-62: Perhaps we could DRY this up a bit?

I couldn't help but notice that all three models implement very similar delete validation logic. Consider creating a mixin or moving this to the BaseModel:

class DeleteProtectionMixin:
    def delete(self, *args):
        protected_relations = getattr(self, 'protected_relations', {})
        for relation_name, error_template in protected_relations.items():
            if getattr(self, relation_name).exists():
                raise ValidationError(error_template.format(self=self))
        super().delete(*args)

This would reduce duplication and standardize the validation across models.

care/facility/models/bed.py (3)

62-64: The error message could be more... helpful.

The current error message could be enhanced to provide more actionable information to the user.

-           error = f"Cannot delete Bed {self} because they are referenced as `bed` in ConsultationBed records."
+           error = f"Cannot delete Bed '{self}' as it is currently assigned to active consultations. Please end or reassign these consultations first."

---

62-65: Inconsistent handling of related models detected.

While AssetBed records are soft-deleted, ConsultationBed records prevent deletion entirely. This inconsistency might be... interesting to explain to users.

Consider one of these approaches:

1. Soft-delete both AssetBed and ConsultationBed records:

    def delete(self, *args, **kwargs) -> None:
-       if ConsultationBed.objects.filter(bed=self).exists():
-           error = f"Cannot delete Bed {self} because they are referenced as `bed` in ConsultationBed records."
-           raise ValidationError(error)
        AssetBed.objects.filter(bed=self).update(deleted=True)
+       ConsultationBed.objects.filter(bed=self).update(deleted=True)
        super().delete(*args, **kwargs)

2. Prevent deletion if either type of relationship exists:

    def delete(self, *args, **kwargs) -> None:
+       if AssetBed.objects.filter(bed=self).exists():
+           raise ValidationError(f"Cannot delete Bed '{self}' as it has associated assets.")
        if ConsultationBed.objects.filter(bed=self).exists():
            error = f"Cannot delete Bed {self} because they are referenced as `bed` in ConsultationBed records."
            raise ValidationError(error)
-       AssetBed.objects.filter(bed=self).update(deleted=True)
        super().delete(*args, **kwargs)

---

Line range hint 107-116: Missing deletion handling in ConsultationBedAsset.

I couldn't help but notice that ConsultationBedAsset might need similar deletion handling as its siblings.

Consider adding a delete method:

def delete(self, *args, **kwargs) -> None:
    with transaction.atomic():
        # Add any necessary validation or related model updates
        super().delete(*args, **kwargs)

care/facility/models/asset.py (1)

Line range hint 1-324: We might want to reconsider the use of PROTECT across M2M relationships

I notice that most M2M relationships in this file use on_delete=models.PROTECT. While this is generally a safe default, it creates friction with our goal of implementing cascade deletes. Perhaps we should:

1. Document the rationale for using PROTECT
2. Consider if CASCADE or SET_NULL might be more appropriate for some relationships
3. Establish consistent guidelines for when to use each on_delete strategy

Would you like me to help draft architectural guidelines for managing model relationships in the context of soft deletes?

care/facility/models/facility.py (3)

313-315: Enhance the validation error message

The error message could be more helpful by including the count of references and suggesting what action to take.

-            error = f"Cannot delete Facility {self} because they are referenced as `facility` in FacilityDefaultAssetLocation records."
+            reference_count = FacilityDefaultAssetLocation.objects.filter(facility=self).count()
+            error = (
+                f"Cannot delete Facility '{self}' because it is referenced in {reference_count} "
+                f"FacilityDefaultAssetLocation record(s). Please remove these references first."
+            )

---

316-321: Consider using the default manager instead of _base_manager

Using _base_manager bypasses the default manager and its filters. Unless there's a specific reason to use it (which should be documented), consider using the default manager.

         Asset.objects.filter(
-            current_location_id__in=AssetLocation._base_manager.filter(  # noqa: SLF001
+            current_location_id__in=AssetLocation.objects.filter(
                 facility_id=self.id
             ).values_list("id", flat=True)
         ).update(deleted=True)

---

322-325: Add error handling and document the deletion order

The code would benefit from explicit error handling and documentation about the order of operations.

+        # Order of operations:
+        # 1. Delete facility users (hard delete as it's a through table)
+        # 2. Clear testing facility references in patient samples (soft delete not needed)
+        try:
             FacilityUser.objects.filter(facility=self).delete()
             PatientSample.objects.filter(testing_facility=self).update(
                 testing_facility=None
             )
+        except Exception as e:
+            raise ValidationError(f"Error while deleting facility relationships: {str(e)}")
         return super().delete(*args)

care/facility/models/patient.py (2)

478-484: Add a docstring to explain the override behavior

The delete method would benefit from a docstring explaining its purpose and the validation it performs.

 def delete(self, *args):
+    """
+    Override delete to prevent deletion of patients referenced in PatientSample records.
+    
+    Raises:
+        ValidationError: If the patient is referenced in any PatientSample records.
+    """
     from care.facility.models.patient_sample import PatientSample

---

481-483: Improve the error message with specific record details

The error message could be more helpful by including the count and IDs of the blocking records.

-        if PatientSample.objects.filter(patient=self).exists():
-            error = f"Cannot delete PatientRegistration {self} because they are referenced as `patient` in PatientSample records."
+        blocking_samples = PatientSample.objects.filter(patient=self)
+        if blocking_samples.exists():
+            sample_ids = list(blocking_samples.values_list('id', flat=True))
+            error = (
+                f"Cannot delete PatientRegistration {self} because they are referenced "
+                f"in {len(sample_ids)} PatientSample records (IDs: {', '.join(map(str, sample_ids))}). "
+                "Please delete these samples first."
+            )

care/utils/tests/test_utils.py (3)

744-744: Consider using a more descriptive default name.

Using now() directly as the name might make test outputs less readable. Perhaps something like f"Investigation Group {now()}" would be more helpful for debugging?

-        data = {"name": now()}
+        data = {"name": f"Investigation Group {now()}"}

---

754-760: Add parameter validation for asset location creation methods.

While the implementation is clean, it might be good to add some basic validation to ensure the location belongs to the correct facility for FacilityDefaultAssetLocation and has appropriate permissions for UserDefaultAssetLocation.

Here's a suggested implementation for create_facility_default_asset_location:

     @classmethod
     def create_facility_default_asset_location(
         cls, facility: Facility, location: AssetLocation
     ) -> FacilityDefaultAssetLocation:
+        if location.facility_id != facility.id:
+            raise ValueError("Location must belong to the specified facility")
         data = {"facility": facility, "location": location}
         return FacilityDefaultAssetLocation.objects.create(**data)

Also applies to: 761-767

---

730-741: LGTM, but could be more concise.

The implementation is correct, though it could be slightly more concise by directly passing the kwargs to create.

     @classmethod
     def create_facility_user(
         cls, facility: Facility, user: User, created_by: User, **kwargs
     ) -> FacilityUser:
-        data = {
-            "facility": facility,
-            "created_by": created_by,
-            "user": user,
-        }
-        data.update(**kwargs)
-        return FacilityUser.objects.create(**data)
+        return FacilityUser.objects.create(
+            facility=facility,
+            created_by=created_by,
+            user=user,
+            **kwargs
+        )

care/users/models.py (3)

411-413: Typically, imports should be at the top of the file

While importing inside methods is possible, placing the imports at the top of the file is generally better practice for readability and maintainability. Unless there's a compelling reason, such as avoiding circular imports, moving these imports might be a good idea.

---

415-421: Refactor duplicate validation checks into a helper method

Noticing that the validation checks are quite similar, perhaps extracting them into a helper function might reduce duplication and improve code clarity.

---

416-416: Ensure consistency in error messages

It might be helpful to use {self.username} consistently in the error messages to avoid any confusion. In one place, you're using {self.username}, and in another, {self}. Keeping it uniform enhances readability.

Also applies to: 420-420

care/facility/tests/test_m2m_soft_delete_for_related_fields.py (8)

72-78: You might find assertRaisesMessage more straightforward here

Using assertRaisesMessage simplifies exception assertions by combining the exception type and message check into a single context manager.

Apply this diff to refactor your test:

 def test_facility_user_delete_when_related_created_by_is_deleted_for_existing_facility_user(self):
     # case 1 when facility user exists for created_by user
     self.assertTrue(FacilityUser.objects.filter(created_by=self.user2).exists())

-    with self.assertRaises(ValidationError) as context:
-        self.user2.delete()
-
-    self.assertIn(
-        f"Cannot delete User {self.user2} because they are referenced as `created_by` in FacilityUser records.",
-        context.exception,
-    )
+    with self.assertRaisesMessage(
+        ValidationError,
+        f"Cannot delete User {self.user2} because they are referenced as `created_by` in FacilityUser records."
+    ):
+        self.user2.delete()

---

128-134: Consider simplifying exception checks with assertRaisesMessage

Refactoring to assertRaisesMessage can make your test assertions more concise and readable.

Here's the suggested change:

 def test_delete_patient_investigation_with_investigation_value(self):
     # Create an InvestigationValue referencing the PatientInvestigation
     InvestigationValue.objects.create(
         investigation=self.investigation,
         group=self.investigation_group,
         consultation=self.consultation,
         session=self.investigation_session,
     )

     # Attempt to delete the investigation
-    with self.assertRaises(ValidationError) as context:
-        self.investigation.delete()
-
-    self.assertIn(
-        f"Cannot delete PatientInvestigation {self.investigation!s} because they are referenced as `investigation` in InvestigationValue records.",
-        context.exception,
-    )
+    with self.assertRaisesMessage(
+        ValidationError,
+        f"Cannot delete PatientInvestigation {self.investigation!s} because they are referenced as `investigation` in InvestigationValue records."
+    ):
+        self.investigation.delete()

---

165-172: Streamline your test with assertRaisesMessage

Using assertRaisesMessage can make exception assertion more direct and less verbose.

Consider this change:

 def test_delete_investigation_session_with_investigation_value(self):
     # Create an InvestigationValue referencing the InvestigationSession
     InvestigationValue.objects.create(
         investigation=self.investigation,
         group=self.investigation_group,
         consultation=self.consultation,
         session=self.investigation_session,
     )

     # Attempt to delete the session
-    with self.assertRaises(ValidationError) as context:
-        self.investigation_session.delete()
-
-    self.assertIn(
-        f"Cannot delete InvestigationSession {self.investigation_session.external_id} because they are referenced as `session` in InvestigationValue records.",
-        str(context.exception),
-    )
+    with self.assertRaisesMessage(
+        ValidationError,
+        f"Cannot delete InvestigationSession {self.investigation_session.external_id} because they are referenced as `session` in InvestigationValue records."
+    ):
+        self.investigation_session.delete()

---

303-309: Using assertRaisesMessage might be beneficial here

This change can make your exception assertions cleaner and more maintainable.

Here's the suggested modification:

 def test_delete_patient_registration_with_related_sample(self):
     # Ensure the sample is linked to the patient
     self.assertTrue(PatientSample.objects.filter(patient=self.patient).exists())

     # Attempt to delete the patient
-    with self.assertRaises(ValidationError) as context:
-        self.patient.delete()
-
-    self.assertIn(
-        f"Cannot delete PatientRegistration {self.patient} because they are referenced as `patient` in PatientSample records.",
-        str(context.exception),
-    )
+    with self.assertRaisesMessage(
+        ValidationError,
+        f"Cannot delete PatientRegistration {self.patient} because they are referenced as `patient` in PatientSample records."
+    ):
+        self.patient.delete()

---

338-344: Simplify exception assertion with assertRaisesMessage

This adjustment can enhance the clarity of your test.

Implement this change:

 def test_delete_patient_consultation_with_related_sample(self):
     # Ensure the sample is linked to the consultation
     self.assertTrue(
         PatientSample.objects.filter(consultation=self.consultation).exists()
     )

     # Attempt to delete the consultation
-    with self.assertRaises(ValidationError) as context:
-        self.consultation.delete()
-
-    self.assertIn(
-        f"Cannot delete PatientConsultation {self.consultation} because they are referenced as `consultation` in PatientSample records.",
-        str(context.exception),
-    )
+    with self.assertRaisesMessage(
+        ValidationError,
+        f"Cannot delete PatientConsultation {self.consultation} because they are referenced as `consultation` in PatientSample records."
+    ):
+        self.consultation.delete()

---

468-475: Switch to assertRaisesMessage for concise exception testing

This refactoring can make your test assertions more straightforward.

Here's the recommended change:

 def test_delete_user_with_related_user_default_asset_location(self):
     # Ensure the user is linked to UserDefaultAssetLocation
     self.assertTrue(
         UserDefaultAssetLocation.objects.filter(user=self.user).exists()
     )

     # Attempt to delete the user
-    with self.assertRaises(ValidationError) as context:
-        self.user.delete()
-
-    # Assert that the exception is raised
-    self.assertIn(
-        f"Cannot delete User {self.user} because they are referenced as `user` in UserDefaultAssetLocation records.",
-        str(context.exception),
-    )
+    with self.assertRaisesMessage(
+        ValidationError,
+        f"Cannot delete User {self.user} because they are referenced as `user` in UserDefaultAssetLocation records."
+    ):
+        self.user.delete()

---

627-633: Refactor exception handling using assertRaisesMessage

This will make your test code cleaner and more precise.

Consider applying this change:

 def test_delete_location_with_related_facility_default_asset_location(self):
     # Ensure the location is linked to FacilityDefaultAssetLocation
     self.assertTrue(
         FacilityDefaultAssetLocation.objects.filter(location=self.location).exists()
     )

     # Attempt to delete the location
-    with self.assertRaises(ValidationError) as context:
-        self.location.delete()
-
-    # Assert that the correct exception is raised
-    self.assertIn(
-        f"Cannot delete AssetLocation {self.location} because they are referenced as `location` in FacilityDefaultAssetLocation records.",
-        str(context.exception),
-    )
+    with self.assertRaisesMessage(
+        ValidationError,
+        f"Cannot delete AssetLocation {self.location} because they are referenced as `location` in FacilityDefaultAssetLocation records."
+    ):
+        self.location.delete()

---

741-748: Using assertRaisesMessage could enhance your test

This approach simplifies exception assertions and improves readability.

Here's how you can refactor:

 def test_delete_consultation_with_related_consultation_bed(self):
     # Ensure the consultation is linked to ConsultationBed
     self.assertTrue(
         ConsultationBed.objects.filter(consultation=self.consultation).exists()
     )

     # Attempt to delete the consultation
-    with self.assertRaises(ValidationError) as context:
-        self.consultation.delete()
-
-    # Assert that the correct exception is raised
-    self.assertIn(
-        f"Cannot delete PatientConsultation {self.consultation} because they are referenced as `consultation` in ConsultationBed records.",
-        str(context.exception),
-    )
+    with self.assertRaisesMessage(
+        ValidationError,
+        f"Cannot delete PatientConsultation {self.consultation} because they are referenced as `consultation` in ConsultationBed records."
+    ):
+        self.consultation.delete()

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Check for active consultations that might be affected

# Search for ConsultationBed usage patterns
ast-grep --pattern 'class ConsultationBed {
  $$$
  consultation = models.ForeignKey($$$)
  $$$
}'

# Look for any consultation-related queries or filters
rg -l "ConsultationBed.*filter|filter.*ConsultationBed" 

#!/bin/bash
# Let's check the actual ConsultationBed model definition and its relationships
rg -A 10 "class ConsultationBed" care/facility/models/patient_consultation.py

# Check how ConsultationBed is being used in filters
rg "ConsultationBed.*filter|filter.*ConsultationBed" care/facility/api/viewsets/patient_consultation.py care/facility/api/viewsets/patient.py

# Check if there are any status or active consultation checks
rg "consultation.*status|status.*consultation" care/facility/models/patient_consultation.py

# Look for any consultation state management
ast-grep --pattern 'class ConsultationBed {
  $$$
  status = $$$
  $$$
}'

#!/bin/bash
# Let's see what these ConsultationBed filters are actually doing
rg -B 2 -A 4 "ConsultationBed.*filter|filter.*ConsultationBed" care/facility/api/viewsets/patient.py care/facility/api/viewsets/patient_consultation.py

# And check the actual validation in the bed model
rg -B 2 -A 2 "Cannot delete Bed" care/facility/models/bed.py

# Look for any end_date related logic
rg "end_date" care/facility/models/patient_consultation.py

 def delete(self, *args):
-    if UserDefaultAssetLocation.objects.filter(location=self).exists():
-        error = f"Cannot delete AssetLocation {self} because they are referenced as `location` in UserDefaultAssetLocation records."
-        raise ValidationError(error)
-    if FacilityDefaultAssetLocation.objects.filter(location=self).exists():
-        error = f"Cannot delete AssetLocation {self} because they are referenced as `location` in FacilityDefaultAssetLocation records."
-        raise ValidationError(error)
+    # Implement soft delete cascade for M2M models
+    UserDefaultAssetLocation.objects.filter(location=self).update(deleted=True)
+    FacilityDefaultAssetLocation.objects.filter(location=self).update(deleted=True)
     return super().delete(*args)

#!/bin/bash
# Search for ManyToManyField definitions in models
echo "Searching for ManyToManyField relationships..."
rg "ManyToManyField\(" -A 5

# Search for through relationships
echo "Searching for through relationships..."
rg "through\s*=" -A 5

# Search for related_name in model relationships
echo "Searching for related relationships..."
rg "related_name\s*=" -A 5