Fixes TokenSlots of schedule not getting deleted #2783
Conversation
📝 WalkthroughWalkthroughThe pull request focuses on 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
care/emr/api/viewsets/scheduling/schedule.py (2)
63-63: Consider removing unnecessary list conversion.The conversion to list using
list(availabilities.values_list("id"))is redundant and could impact performance for large datasets. The values_list already returns a queryset that can be used directly in the filter.- availability_ids = list(availabilities.values_list("id")) + availability_ids = availabilities.values_list("id", flat=True)
75-78: Consider combining filter and update into a single query.The current implementation uses two separate database calls. This could be optimized into a single update query.
- slots = TokenSlot.objects.filter( - resource=instance.resource, availability_id__in=availability_ids - ) - slots.update(deleted=True) + TokenSlot.objects.filter( + resource=instance.resource, availability_id__in=availability_ids + ).update(deleted=True)care/emr/tests/test_schedule_api.py (1)
254-263: Consider adding tests for partial deletion failures.While the current test coverage is good, it would be beneficial to add tests that verify the transaction rollback behavior when part of the deletion process fails (e.g., if slot deletion fails, the availability should not be marked as deleted).
Here's a suggested test case:
def test_delete_schedule_rolls_back_on_partial_failure(self): """Test that the transaction rolls back if any part of the deletion fails.""" permissions = [ UserSchedulePermissions.can_write_user_schedule.name, UserSchedulePermissions.can_list_user_schedule.name, ] role = self.create_role_with_permissions(permissions) self.attach_role_facility_organization_user(self.organization, self.user, role) # Mock TokenSlot.objects.filter to raise an exception with patch('care.emr.models.TokenSlot.objects.filter') as mock_filter: mock_filter.side_effect = Exception("Simulated failure") delete_url = self._get_schedule_url(self.schedule.external_id) response = self.client.delete(delete_url) self.assertEqual(response.status_code, status.HTTP_500_INTERNAL_SERVER_ERROR) # Verify nothing was deleted due to transaction rollback self.availability.refresh_from_db() self.assertFalse(self.availability.deleted)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/api/viewsets/scheduling/schedule.py(2 hunks)care/emr/tests/test_schedule_api.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
care/emr/tests/test_schedule_api.py (2)
258-262: LGTM! Nice addition of deletion verification.The test now properly verifies that both availability and slots are marked as deleted. The refresh_from_db calls ensure we're checking the latest state.
218-219: LGTM! Good refactor to reduce test complexity.Using the existing schedule fixture instead of creating new ones makes the tests more maintainable and efficient.
Also applies to: 237-238
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2783 +/- ##
===========================================
- Coverage 56.69% 56.68% -0.02%
===========================================
Files 208 208
Lines 9569 9571 +2
Branches 942 942
===========================================
Hits 5425 5425
- Misses 4128 4130 +2
Partials 16 16 ☔ View full report in Codecov by Sentry. |
vigneshhari
deleted the
rithviknishad/fix/schedule-delete-not-deleting-tokens
branch
Proposed Changes
Associated Issue
Merge Checklist
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Bug Fixes
Tests