[Eng-441] feat:ability to delete resource category#3703
Conversation
📝 WalkthroughWalkthroughAdds soft-delete protection to ChangesResourceCategory delete protection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds soft-delete support for
Confidence Score: 5/5Safe to merge — the deletion logic is correctly guarded, soft-delete is wrapped in a transaction, and the parent has_children flag is properly refreshed using the filtered default manager. The core deletion path is well-implemented: validate_destroy uses the app's soft-delete-aware ORM manager so only active children and resources block deletion, perform_destroy wraps both the soft-delete and the parent flag update in a single atomic transaction, and authorize_destroy correctly delegates to the existing write-permission check. The PROTECT migration covers all three models that reference ResourceCategory. No data-loss or incorrect-state scenarios were identified in the changed code. care/emr/api/viewsets/resource_category.py — EMRDestroyMixin placement after EMRBaseViewSet (flagged in a previous review cycle) Important Files Changed
Reviews (3): Last reviewed commit: "Merge branch 'ENG-441-ability-to-delete-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
care/emr/tests/test_resource_category_api.py (1)
271-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name doesn't match its assertion.
The test is named
test_list_resource_categories_with_parent_filter_no_resultsbut it asserts exactly 1 result (the root category). A more accurate name would reflect that an empty?parent=filters to root-level categories only.- def test_list_resource_categories_with_parent_filter_no_results(self): + def test_list_resource_categories_with_empty_parent_filter_returns_root_categories(self):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@care/emr/tests/test_resource_category_api.py` around lines 271 - 292, The test name in test_list_resource_categories_with_parent_filter_no_results does not match what it asserts, since it expects the root category to be returned rather than no results. Rename this test to reflect that an empty parent filter returns root-level resource categories, and keep the assertions aligned with that behavior in test_list_resource_categories_with_parent_filter_no_results and the related get_url query usage.care/emr/api/viewsets/resource_category.py (1)
115-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove validation logic into
validate_destroyfor consistency.This class already separates validation (
validate_data) from side effects (perform_create/perform_update). The destroy path has an analogousvalidate_destroyhook (default no-op inEMRDestroyMixin) that isn't used — all checks are bundled intoperform_destroyinstead, which departs from that pattern.♻️ Suggested restructuring
- def perform_destroy(self, instance): - if instance.children.exists(): - raise ValidationError("Cannot delete a resource category with children") - - resource_type = instance.resource_type - if ( - ( - resource_type - == ResourceCategoryResourceTypeOptions.activity_definition.value - and ActivityDefinition.objects.filter(category=instance).exists() - ) - or ( - resource_type - == ResourceCategoryResourceTypeOptions.charge_item_definition.value - and ChargeItemDefinition.objects.filter(category=instance).exists() - ) - or ( - resource_type - == ResourceCategoryResourceTypeOptions.product_knowledge.value - and ProductKnowledge.objects.filter(category=instance).exists() - ) - ): - raise ValidationError( - "Cannot delete a resource category associated with a resource" - ) - super().perform_destroy(instance) + def validate_destroy(self, instance): + if instance.children.exists(): + raise ValidationError("Cannot delete a resource category with children") + + resource_model_map = { + ResourceCategoryResourceTypeOptions.activity_definition.value: ActivityDefinition, + ResourceCategoryResourceTypeOptions.charge_item_definition.value: ChargeItemDefinition, + ResourceCategoryResourceTypeOptions.product_knowledge.value: ProductKnowledge, + } + model = resource_model_map.get(instance.resource_type) + if model and model.objects.filter(category=instance).exists(): + raise ValidationError( + "Cannot delete a resource category associated with a resource" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@care/emr/api/viewsets/resource_category.py` around lines 115 - 140, Move the destroy-time checks out of perform_destroy and into validate_destroy for consistency with validate_data and the create/update flow. Keep the existing child/resource association checks in ResourceCategory’s destroy path, but have validate_destroy raise the ValidationError(s) before any deletion happens, and let perform_destroy only call super().perform_destroy(instance). Use the existing validate_destroy hook from EMRDestroyMixin and the ResourceCategoryResourceTypeOptions / ActivityDefinition / ChargeItemDefinition / ProductKnowledge checks to locate the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@care/emr/api/viewsets/resource_category.py`:
- Around line 115-140: Move the destroy-time checks out of perform_destroy and
into validate_destroy for consistency with validate_data and the create/update
flow. Keep the existing child/resource association checks in ResourceCategory’s
destroy path, but have validate_destroy raise the ValidationError(s) before any
deletion happens, and let perform_destroy only call
super().perform_destroy(instance). Use the existing validate_destroy hook from
EMRDestroyMixin and the ResourceCategoryResourceTypeOptions / ActivityDefinition
/ ChargeItemDefinition / ProductKnowledge checks to locate the logic.
In `@care/emr/tests/test_resource_category_api.py`:
- Around line 271-292: The test name in
test_list_resource_categories_with_parent_filter_no_results does not match what
it asserts, since it expects the root category to be returned rather than no
results. Rename this test to reflect that an empty parent filter returns
root-level resource categories, and keep the assertions aligned with that
behavior in test_list_resource_categories_with_parent_filter_no_results and the
related get_url query usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3ca07cd0-3d69-4e59-aa66-c068867fcd73
📒 Files selected for processing (6)
care/emr/api/viewsets/resource_category.pycare/emr/migrations/0080_alter_activitydefinition_category_and_more.pycare/emr/models/activity_definition.pycare/emr/models/charge_item_definition.pycare/emr/models/product_knowledge.pycare/emr/tests/test_resource_category_api.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3703 +/- ##
===========================================
+ Coverage 79.66% 79.67% +0.01%
===========================================
Files 479 479
Lines 23010 23032 +22
Branches 2379 2382 +3
===========================================
+ Hits 18330 18350 +20
- Misses 4080 4082 +2
Partials 600 600 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
care/emr/api/viewsets/resource_category.py (2)
116-139: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAuthorize before destroy validation.
EMRDestroyMixin.destroy()runsvalidate_destroy()beforeauthorize_destroy(), so a user with list access but no write access can learn whether a category has children or linked resources from the 400 response. Swap the order; the denial path doesn’t need extra gossip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@care/emr/api/viewsets/resource_category.py` around lines 116 - 139, In EMRDestroyMixin.destroy, the current call order reveals validation details before permission checks; update the destroy flow so authorize_destroy runs before validate_destroy, preventing users without write access from learning whether a ResourceCategory has children or linked resources. Use the existing destroy/authorize_destroy/validate_destroy path in the ResourceCategory viewset to swap the checks without changing the validation logic itself.
116-154: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftSerialize the destroy path in
care/emr/api/viewsets/resource_category.py:116-154
validate_destroy()andperform_destroy()are split, so a child category or linkedActivityDefinition/ChargeItemDefinition/ProductKnowledgecan be created after theexists()checks pass and beforedeleted=Truecommits. Since this is a soft-delete, thePROTECTFKs never get a chance to stop that late write.parent.has_childrenis recomputed without lockingparent, so concurrent sibling deletes can leave the flag stale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@care/emr/api/viewsets/resource_category.py` around lines 116 - 154, The destroy flow in validate_destroy and perform_destroy is vulnerable to race conditions, so move the existence checks and the soft-delete update into one serialized transaction. Use locking on the ResourceCategory row (and parent when updating has_children) so child categories or linked ActivityDefinition, ChargeItemDefinition, or ProductKnowledge records cannot be created between the checks and the deleted=True save. Also lock the parent before recomputing parent.has_children in perform_destroy to keep the flag consistent under concurrent deletes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@care/emr/api/viewsets/resource_category.py`:
- Around line 116-139: In EMRDestroyMixin.destroy, the current call order
reveals validation details before permission checks; update the destroy flow so
authorize_destroy runs before validate_destroy, preventing users without write
access from learning whether a ResourceCategory has children or linked
resources. Use the existing destroy/authorize_destroy/validate_destroy path in
the ResourceCategory viewset to swap the checks without changing the validation
logic itself.
- Around line 116-154: The destroy flow in validate_destroy and perform_destroy
is vulnerable to race conditions, so move the existence checks and the
soft-delete update into one serialized transaction. Use locking on the
ResourceCategory row (and parent when updating has_children) so child categories
or linked ActivityDefinition, ChargeItemDefinition, or ProductKnowledge records
cannot be created between the checks and the deleted=True save. Also lock the
parent before recomputing parent.has_children in perform_destroy to keep the
flag consistent under concurrent deletes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bfb4fd41-1d8f-472e-837f-364bed2bba99
📒 Files selected for processing (1)
care/emr/api/viewsets/resource_category.py
1 participant
Proposed Changes
Associated Issue
Merge Checklist
/docsOnly 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