Fixing Manually cascade delete M2M models when related model is deleted #2620
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a series of modifications across various models in the Django application to enhance the deletion process. A new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (26)
care/facility/models/patient_investigation.py (3)
42-47: I couldn't help but notice some... interesting... style choicesWhile 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
returnstatement beforesuper().delete()is unnecessary since delete methods typically return NoneHere'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 messageThe validation logic is solid, but:
- Using
external_idin the error message might not be very user-friendly- There's that unnecessary
returnstatement againConsider 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
AssetBedrecords are soft-deleted,ConsultationBedrecords prevent deletion entirely. This inconsistency might be... interesting to explain to users.Consider one of these approaches:
- Soft-delete both
AssetBedandConsultationBedrecords: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)
- 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
ConsultationBedAssetmight 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 relationshipsI 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:
- Document the rationale for using PROTECT
- Consider if CASCADE or SET_NULL might be more appropriate for some relationships
- 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 messageThe 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_managerUsing
_base_managerbypasses 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 orderThe 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 behaviorThe
deletemethod 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 detailsThe 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
FacilityDefaultAssetLocationand has appropriate permissions forUserDefaultAssetLocation.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 fileWhile 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 methodNoticing 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 messagesIt 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 findassertRaisesMessagemore straightforward hereUsing
assertRaisesMessagesimplifies 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 withassertRaisesMessageRefactoring to
assertRaisesMessagecan 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 withassertRaisesMessageUsing
assertRaisesMessagecan 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: UsingassertRaisesMessagemight be beneficial hereThis 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 withassertRaisesMessageThis 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 toassertRaisesMessagefor concise exception testingThis 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 usingassertRaisesMessageThis 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: UsingassertRaisesMessagecould enhance your testThis 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()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
care/facility/models/asset.py(2 hunks)care/facility/models/bed.py(1 hunks)care/facility/models/facility.py(2 hunks)care/facility/models/patient.py(2 hunks)care/facility/models/patient_consultation.py(2 hunks)care/facility/models/patient_investigation.py(4 hunks)care/facility/tests/test_m2m_soft_delete_for_related_fields.py(1 hunks)care/users/models.py(2 hunks)care/utils/tests/test_utils.py(2 hunks)
🔇 Additional comments (8)
care/facility/models/patient_investigation.py (2)
3-3: Finally, a proper exception import!
The ValidationError import is correctly placed and necessary for the new delete methods.
19-23: Oh, I see we're living dangerously with potential race conditions
While the validation logic is correct, there's a small window between checking for references and actual deletion where new references could be created. Consider using transactions or database constraints for truly bulletproof protection.
Let's check if there are any existing database constraints:
care/facility/models/bed.py (1)
62-65: Verify the impact on active consultations.
Let's ensure we're not affecting any active consultations with this change.
✅ Verification successful
The validation looks perfectly fine, actually
The current validation is already handling this quite elegantly. It prevents bed deletion when there are any ConsultationBed records referencing it, regardless of their status. This is exactly what we want because:
- Active consultations (end_date__isnull=True) are protected
- Historical consultation records (end_date__isnull=False) are also preserved
- The validation message is clear and informative
The code is already doing what it should to maintain data integrity. I suppose we could make it more complex, but why fix what isn't broken? 🙂
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/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"
Length of output: 823
Script:
#!/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 = $$$
$$$
}'
Length of output: 683
Script:
#!/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
Length of output: 2510
care/facility/models/asset.py (2)
5-5: LGTM! Though you could have grouped it with the other Django imports...
The addition of ValidationError import is necessary for the new delete method implementation.
58-65:
This implementation seems to be doing the opposite of what was intended... 🤔
The PR objectives clearly state that we need to implement cascade delete for M2M models, but this implementation is preventing deletion instead. There are several concerns:
- The current implementation raises ValidationError, preventing deletion entirely
- It doesn't implement the soft delete cascade mentioned in the PR objectives
- The related models use
on_delete=models.PROTECTwhich conflicts with the cascade delete goal
Consider this alternative implementation that aligns with the PR objectives:
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)Let's verify if there are any other M2M models referencing AssetLocation that we need to handle:
care/facility/models/facility.py (1)
305-325: Verify all M2M relationships are handled
The implementation looks good, but let's verify we haven't missed any M2M relationships that might need similar handling.
✅ Verification successful
All M2M relationships are properly handled in the delete method
Looking at the codebase analysis, the current implementation already handles all relevant M2M relationships that could impact facility deletion:
usersM2M throughFacilityUser: ✓ Handled withFacilityUser.objects.filter(facility=self).delete()assetsM2M throughAssetLocation: ✓ Handled with soft deletePatientSamplerelationship: ✓ Handled by setting to None
The other M2M relationships found in the codebase are either:
- Not directly related to Facility (e.g., User-Skill, ConsultationBed-Asset)
- Already protected by the
FacilityDefaultAssetLocationcheck - Have ON DELETE behavior defined in their models that safely handles cascading
Though I must say, it's quite thorough how you've implemented the deletion checks... almost too thorough 😏
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/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
Length of output: 134331
care/users/models.py (1)
409-427: Deletion logic updates appear sound
The updated delete method appropriately handles related records and ensures data integrity. Wrapping the method with @transaction.atomic effectively makes the operations atomic, which is good practice.
care/facility/tests/test_m2m_soft_delete_for_related_fields.py (1)
1-822: Tests are comprehensive and align with PR objectives
Your test cases thoroughly cover the scenarios outlined in the PR objectives, ensuring that deletion behaviors are correctly validated across models.
| 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) | ||
|
|
There was a problem hiding this comment.
Shouldn't we actually delete this ? ie call the super delete method ?
There was a problem hiding this comment.
My bad, didnt see the old bits !
| @@ -54,6 +55,15 @@ class RoomType(enum.Enum): | |||
| null=True, blank=True, default=None, max_length=200 | |||
| ) | |||
|
|
|||
| 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." | |||
There was a problem hiding this comment.
It might make more sense to create a static errors and make it translatable as well, also keep the errors very human readable like
| AssetLocation.objects.filter(facility_id=self.id).update(deleted=True) | ||
| Asset.objects.filter( | ||
| current_location_id__in=AssetLocation._base_manager.filter( # noqa: SLF001 | ||
| facility_id=self.id | ||
| ).values_list("id", flat=True) | ||
| ).update(deleted=True) | ||
| FacilityUser.objects.filter(facility=self).delete() |
There was a problem hiding this comment.
Shifting might also be affected
|
|
||
| FacilityUser.objects.filter(user=self).delete() | ||
| PatientSample.objects.filter(created_by=self).update(created_by=None) | ||
| PatientSample.objects.filter(last_edited_by=self).update(last_edited_by=None) |
There was a problem hiding this comment.
WE can keep these, the whole reason we dont delete users is so that we can show these when the audit comes up.
There was a problem hiding this comment.
PatientSample.objects.filter(created_by=self).update(created_by=None)
PatientSample.objects.filter(last_edited_by=self).update(last_edited_by=None)
You mean that we don't need to set user null here?
@vigneshhari
Proposed Changes
Associated Issue
Merge Checklist
Only PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Release Notes
New Features
AssetLocation,Bed,Facility,PatientRegistration,PatientConsultation, and investigation-related records.Bug Fixes
Tests