Delete TokenSlots when availability is deleted #2786
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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 (3)
care/emr/api/viewsets/scheduling/schedule.py (1)
183-183: LGTM! Though we could make the error handling a bit more... robust.The implementation correctly marks TokenSlots as deleted when their associated availability is destroyed. The placement within the Lock context and after the future bookings check is well thought out.
Consider wrapping the update in a try-except block to handle potential database errors:
- TokenSlot.objects.filter(availability_id=instance.id).update(deleted=True) + try: + TokenSlot.objects.filter(availability_id=instance.id).update(deleted=True) + except Exception as e: + raise ValidationError(f"Failed to delete associated token slots: {str(e)}")care/emr/tests/test_schedule_api.py (2)
746-755: Good test coverage for the new TokenSlot deletion feature.The test properly verifies both the availability and slot deletion states.
Consider adding a verification that no hard deletion occurred:
self.assertTrue(self.availability.deleted) self.assertTrue(self.slot.deleted) + self.assertTrue(TokenSlot.objects.filter(id=self.slot.id).exists())
Line range hint
762-794: The test cases look comprehensive, but...The test coverage for error cases is good, particularly for future bookings. However, it might be worth adding a test for the database error scenario if you implement the suggested try-except block.
Would you like me to generate a test case for handling database errors in the TokenSlot deletion process?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/api/viewsets/scheduling/schedule.py(1 hunks)care/emr/tests/test_schedule_api.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
care/emr/tests/test_schedule_api.py (1)
632-633: Nice refactoring of the test setup!The addition of helper methods improves code reusability and test maintainability.
Also applies to: 690-700
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2786 +/- ##
===========================================
- Coverage 56.68% 56.67% -0.01%
===========================================
Files 208 208
Lines 9571 9572 +1
Branches 942 942
===========================================
Hits 5425 5425
- Misses 4130 4131 +1
Partials 16 16 ☔ View full report in Codecov by Sentry. |
Proposed Changes
Associated Issue
Architecture 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
New Features
Tests