Daily Rounds External Id in Notification Handler #2641
Conversation
📝 WalkthroughWalkthroughThe changes involve a migration script that updates the Changes
Poem
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
🧹 Outside diff range and nitpick comments (2)
care/facility/migrations/0468_notifications_dailyround_external_id.py (2)
10-10: Consider batching the notification updatesWhile the migration works, querying all notifications at once might not be the most efficient approach for large datasets.
Consider using batch processing:
- for notification in Notification.objects.all(): + batch_size = 1000 + for notification in Notification.objects.iterator(chunk_size=batch_size):
12-19: Add error logging for missing DailyRoundsThe migration silently skips notifications with missing DailyRounds. While this is safe, it might be helpful to log these cases for monitoring.
except DailyRound.DoesNotExist: - # Handle the case where the DailyRound does not exist - pass + logger.warning( + f"DailyRound {daily_round_id} not found for notification {notification.id}" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
care/facility/migrations/0468_notifications_dailyround_external_id.py(1 hunks)care/utils/notification_handler.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
care/facility/migrations/0468_notifications_dailyround_external_id.py
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🔇 Additional comments (2)
care/utils/notification_handler.py (2)
290-290: LGTM! The change aligns perfectly with the PR objectives
The modification to use external_id instead of id for daily rounds is consistent with how other objects are handled in the notification system. This change will properly support the front-end's expectation for the consultation log update page.
289-293: Verify the external_id usage in front-end routes
Just to be thorough, let's verify that the front-end correctly uses this external_id in the route pattern consultation/consultation:id/log_updates/log:id/.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2641 +/- ##
========================================
Coverage 69.63% 69.63%
========================================
Files 211 211
Lines 11879 11879
Branches 1202 1202
========================================
Hits 8272 8272
Misses 3240 3240
Partials 367 367 ☔ View full report in Codecov by Sentry. |
|
We dont need to back fill any data, you can delete the migration. Notifications are anyways deleted periodically. |
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
New Features
DailyRoundobjects by their external IDs instead of internal IDs.Bug Fixes
Notificationinstances to ensure accurate updates ofdaily_roundreferences.