Fix delete method in viewsets #2723
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a systematic renaming of deletion-related methods across multiple viewsets in the EMR API. The primary change involves replacing "delete" with "destroy" in method and mixin names, such as Changes
Poem
Finishing Touches
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
care/emr/api/viewsets/base.py (1)
177-188: LGTM! Consider adding docstrings.The rename from
EMRDeleteMixintoEMRDestroyMixinaligns perfectly with Django's conventions. Though, it might be nice to add some docstrings explaining the purpose of each method, you know, for those who come after us.class EMRDestroyMixin: + """Mixin class for handling object destruction in EMR viewsets. + + This mixin provides standard destroy functionality following Django conventions. + """ + def authorize_destroy(self, instance): + """Authorize the destruction of an instance. Override this method to implement custom authorization.""" pass def perform_destroy(self, instance): + """Perform soft deletion by setting deleted flag.""" instance.deleted = True instance.save(update_fields=["deleted"]) def destroy(self, request, *args, **kwargs): + """Handle the destruction of an instance with proper authorization."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
care/emr/api/viewsets/base.py(2 hunks)care/emr/api/viewsets/encounter_authz_base.py(1 hunks)care/emr/api/viewsets/facility.py(1 hunks)care/emr/api/viewsets/facility_organization.py(2 hunks)care/emr/api/viewsets/organization.py(2 hunks)care/emr/api/viewsets/patient.py(1 hunks)care/emr/api/viewsets/questionnaire.py(1 hunks)care/emr/api/viewsets/resource_request.py(2 hunks)care/emr/api/viewsets/scheduling/availability_exceptions.py(1 hunks)care/emr/api/viewsets/scheduling/schedule.py(6 hunks)care/emr/api/viewsets/user.py(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- care/emr/api/viewsets/patient.py
- care/emr/api/viewsets/encounter_authz_base.py
- care/emr/api/viewsets/facility.py
- care/emr/api/viewsets/organization.py
- care/emr/api/viewsets/user.py
- care/emr/api/viewsets/facility_organization.py
- care/emr/api/viewsets/scheduling/schedule.py
🔇 Additional comments (5)
care/emr/api/viewsets/base.py (1)
274-274: LGTM! Consistent inheritance update.The update to use
EMRDestroyMixinmaintains consistency with the renamed mixin.care/emr/api/viewsets/scheduling/availability_exceptions.py (1)
38-39: LGTM! Method rename maintains consistency.The rename from
authorize_deletetoauthorize_destroyaligns with the base class changes while preserving the existing authorization logic.care/emr/api/viewsets/resource_request.py (1)
77-77: LGTM! Mixin update maintains consistency.The update to use
EMRDestroyMixinaligns with the base class changes.care/emr/api/viewsets/questionnaire.py (2)
92-94: LGTM! Method rename maintains authorization logic.The rename from
authorize_deletetoauthorize_destroyaligns with the base class changes while preserving the superuser authorization check.
Line range hint
177-188: Verify complete migration to destroy terminology.Let's ensure we haven't missed any instances of the old terminology.
✅ Verification successful
Looks like someone did their homework with the destroy terminology migration 🎯
The specific methods mentioned in the review have been successfully renamed, and the remaining "delete" occurrences are just Django's built-in methods and migrations that should stay as they are. How... refreshingly thorough.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of delete-related method names echo "Checking for remaining authorize_delete methods..." rg "def authorize_delete" echo "Checking for remaining EMRDeleteMixin references..." rg "EMRDeleteMixin" echo "Checking for remaining perform_delete methods..." rg "def perform_delete"Length of output: 404
Script:
#!/bin/bash # More comprehensive check for delete-related patterns echo "Checking for delete in method names (case insensitive)..." rg -i "def.*delete" echo "Checking for delete in class names..." rg -i "class.*delete" echo "Checking method declarations with AST..." ast-grep --pattern 'def $_(self, $$$) { $$$ delete $$$ }' echo "Checking for any remaining delete references in viewsets..." fd viewset -t f -x rg -i "delete" {} echo "Checking for delete in class methods..." ast-grep --pattern 'class $_ { $$$ def $_($$$) { $$$ delete $$$ } $$$ }'Length of output: 19971
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2723 +/- ##
========================================
Coverage 45.97% 45.97%
========================================
Files 313 313
Lines 17684 17684
Branches 1702 1702
========================================
Hits 8130 8130
Misses 9494 9494
Partials 60 60 ☔ View full report in Codecov by Sentry. |
Proposed Changes
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
Refactor
authorize_deletemethods toauthorize_destroyacross multiple viewsetsTechnical Note