Remove event handlers #2593
Remove event handlers #2593
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve significant modifications across multiple serializer and viewset classes in the healthcare application. The primary focus has been on simplifying the control flow by removing 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2593 +/- ##
===========================================
+ Coverage 69.20% 69.39% +0.18%
===========================================
Files 211 209 -2
Lines 11944 11807 -137
Branches 1208 1190 -18
===========================================
- Hits 8266 8193 -73
+ Misses 3302 3241 -61
+ Partials 376 373 -3 ☔ View full report in Codecov by Sentry. |
![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
🧹 Outside diff range and nitpick comments (4)
care/facility/api/serializers/daily_round.py (1)
169-169: Oh, I see we're keeping the notification logic while removing event handlers... interesting choice.While removing the event handlers is a step in the right direction, it seems we're still mixing multiple responsibilities in this method. Perhaps we could also consider:
- Extracting notification logic to a dedicated service
- Separating validation logic from data updates
- Breaking down the rather lengthy update method into smaller, focused methods
I mean, if we're cleaning up the code, we might as well do it properly, right? 😊
Here's a potential refactor to consider:
class DailyRoundService: def handle_notifications(self, daily_round, user): NotificationGenerator( event=Notification.Event.PATIENT_CONSULTATION_UPDATE_UPDATED, caused_by=user, caused_object=daily_round, facility=daily_round.consultation.patient.facility, ).generate() def handle_consultation_update(self, daily_round, is_telemedicine): daily_round.consultation.last_updated_by_telemedicine = is_telemedicine daily_round.consultation.save(update_fields=["last_updated_by_telemedicine"]) class DailyRoundSerializer(serializers.ModelSerializer): def update(self, instance, validated_data): self._validate_discharge_status(instance) self._handle_patient_actions(instance, validated_data) service = DailyRoundService() is_telemedicine = self._is_telemedicine_update(instance) service.handle_consultation_update(instance, is_telemedicine) updated_instance = super().update(instance, validated_data) service.handle_notifications(updated_instance, self.context["request"].user) return updated_instancecare/facility/api/serializers/patient_consultation.py (3)
Line range hint
407-414: Inconsistent variable naming: use 'create_diagnoses' for clarityTo maintain consistency and improve readability, consider renaming the variable
create_diagnosistocreate_diagnosesto match the key used invalidated_data.
Line range hint
420-429: Wrap generator expression in a list for 'bulk_create'The
bulk_createmethod expects a list of model instances, but a generator expression is provided. Wrapping the generator in a list ensures proper execution.Apply this diff to fix the issue:
EncounterSymptom.objects.bulk_create( + [ EncounterSymptom( consultation=consultation, symptom=obj.get("symptom"), onset_date=obj.get("onset_date"), cure_date=obj.get("cure_date"), clinical_impression_status=obj.get("clinical_impression_status"), other_symptom=obj.get("other_symptom") or "", created_by=user, ) for obj in create_symptoms - ) + ] + )
Line range hint
459-459: Typo in error message: 'Atleast' should be 'At least'There's a minor typo in the validation error message that could be corrected for clarity.
Apply this diff:
- msg = "Atleast one diagnosis is required" + msg = "At least one diagnosis is required"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
care/facility/api/serializers/daily_round.py(1 hunks)care/facility/api/serializers/encounter_symptom.py(1 hunks)care/facility/api/serializers/patient_consultation.py(2 hunks)care/facility/api/viewsets/consultation_diagnosis.py(1 hunks)care/facility/api/viewsets/patient.py(0 hunks)care/facility/events/handler.py(0 hunks)care/facility/management/commands/load_event_types.py(0 hunks)care/utils/event_utils.py(0 hunks)
💤 Files with no reviewable changes (4)
- care/facility/api/viewsets/patient.py
- care/facility/events/handler.py
- care/facility/management/commands/load_event_types.py
- care/utils/event_utils.py
🔇 Additional comments (5)
care/facility/api/viewsets/consultation_diagnosis.py (2)
55-55: LGTM, but let's verify the event logging impact.
The simplified version looks much cleaner, I must say. Though, I can't help but wonder if we're absolutely certain that removing the event logging won't impact any audit requirements or downstream systems that might have depended on these events.
Let's check if there are any remaining references to consultation events that might expect this logging:
Line range hint 29-42: Architectural consideration regarding removed update methods.
I notice we're silently falling back to the default update behavior after removing perform_update and update. While this certainly makes the code more... streamlined 🙂, we should probably document that we've intentionally removed the old instance copying functionality, just so future maintainers don't think it was an oversight.
Let's check if there are any other places in the codebase that might have expected this copying behavior:
care/facility/api/serializers/encounter_symptom.py (2)
94-94: At least we're consistently removing transactions everywhere.
The update method follows the same pattern as create(). While I appreciate the consistency, I'm slightly concerned about potential race conditions or partial updates. But I suppose that's a problem for future us?
Let's check if there are any concurrent update scenarios we should worry about:
#!/bin/bash
# Look for potential concurrent access patterns
rg "EncounterSymptom.*update" -A 5Consider adding a comment explaining why transaction management was deemed unnecessary, just so future developers don't waste time wondering about it. Not that I'm wondering or anything...
89-89: Oh, I see we're living dangerously without transaction management now.
While removing the event handlers aligns with the PR objectives, I couldn't help but notice we've also dropped the transaction.atomic() wrapper. I'm sure you have your reasons, but perhaps we should discuss the implications?
Let's check if other serializers still use transactions:
Consider keeping the transaction management to ensure data consistency, even if we're removing the event handlers. It's just a thought... 🤷
care/facility/api/serializers/daily_round.py (1)
Line range hint 1-400: Just making sure we didn't forget any event handler references...
Let's verify that all event handler related code has been properly cleaned up from this file.
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
Release Notes
Bug Fixes
New Features
PatientConsultationSerializerandPatientNotesViewSetby removing redundant event handling, allowing for more efficient patient note creation and updates.Chores