Remove Summarization Tasks and Related Code #2653
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request systematically removes summarization-related components across multiple files in the Django project. This comprehensive cleanup eliminates Celery tasks, API endpoints, serializers, viewsets, configuration settings, and documentation references for various summary-related functionalities including triage, tests, facility capacity, patient, and district patient summaries. The changes suggest a deliberate streamlining of the project's summarization infrastructure, perhaps to make room for something more... efficient? Changes
Assessment against linked issues
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 1
🧹 Nitpick comments (1)
care/facility/migrations/0468_alter_facilitypatientstatshistory_unique_together_and_more.py (1)
12-41: Consider migration irreversibility implicationsJust a friendly reminder that this migration is effectively a point of no return - it completely removes several models and their data. While this aligns with our objectives, you might want to consider taking a backup... you know, just in case someone changes their mind later? 😉
🧰 Tools
🪛 Ruff (0.8.2)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
18-18: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
30-30: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
33-33: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
36-36: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
39-39: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
care/facility/api/serializers/patient.py(0 hunks)care/facility/api/viewsets/facility.py(0 hunks)care/facility/api/viewsets/patient.py(1 hunks)care/facility/api/viewsets/summary.py(0 hunks)care/facility/management/commands/summarize.py(0 hunks)care/facility/migrations/0468_alter_facilitypatientstatshistory_unique_together_and_more.py(1 hunks)care/facility/models/__init__.py(0 hunks)care/facility/models/patient.py(0 hunks)care/facility/models/summary.py(0 hunks)care/facility/tasks/__init__.py(0 hunks)care/facility/utils/summarization/district/patient_summary.py(0 hunks)care/facility/utils/summarization/facility_capacity.py(0 hunks)care/facility/utils/summarization/patient_summary.py(0 hunks)care/facility/utils/summarization/tests_summary.py(0 hunks)care/facility/utils/summarization/triage_summary.py(0 hunks)config/api_router.py(0 hunks)
💤 Files with no reviewable changes (14)
- care/facility/management/commands/summarize.py
- care/facility/utils/summarization/triage_summary.py
- care/facility/models/init.py
- care/facility/utils/summarization/facility_capacity.py
- care/facility/utils/summarization/patient_summary.py
- care/facility/utils/summarization/tests_summary.py
- care/facility/utils/summarization/district/patient_summary.py
- care/facility/api/viewsets/summary.py
- care/facility/api/serializers/patient.py
- care/facility/tasks/init.py
- care/facility/models/summary.py
- config/api_router.py
- care/facility/api/viewsets/facility.py
- care/facility/models/patient.py
✅ Files skipped from review due to trivial changes (1)
- care/facility/api/viewsets/patient.py
🧰 Additional context used
🪛 Ruff (0.8.2)
care/facility/migrations/0468_alter_facilitypatientstatshistory_unique_together_and_more.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
18-18: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
30-30: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
33-33: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
36-36: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
39-39: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🔇 Additional comments (3)
care/facility/migrations/0468_alter_facilitypatientstatshistory_unique_together_and_more.py (3)
13-16: LGTM: Constraint removal properly ordered
The unique constraint is correctly removed before touching the fields. Nice to see someone following proper migration hygiene!
🧰 Tools
🪛 Ruff (0.8.2)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
1-10: Verify this is the latest migration in sequence
The migration setup looks correct, but let's make absolutely sure there aren't any newer migrations that might conflict with these changes... wouldn't want any surprises, would we? 😊
✅ Verification successful
Well, look who's the latest migration in town 🎯
I've thoroughly checked, and migration 0468 is indeed the latest in the sequence. No sneaky dependent migrations lurking around that could cause any... unfortunate surprises. How refreshingly orderly.
- Latest migrations in sequence:
- 0466_camera_presets.py
- 0467_alter_hospitaldoctors_area.py
- 0468_alter_facilitypatientstatshistory_unique_together_and_more.py (our current one)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any migrations that might depend on this one
fd --type f ".*\.py$" care/facility/migrations/ --exec grep -l "0468_alter_facilitypatientstatshistory"
Length of output: 103
Script:
#!/bin/bash
# Let's try a different approach to check for migrations after 0468
fd "^\d{4}.*\.py$" care/facility/migrations/ | sort -n | grep -A1 "0468_alter"
# Also check the content of __init__.py to see if there are any migration references
cat care/facility/migrations/__init__.py
# And let's see what other migrations exist in the directory
fd "^\d{4}.*\.py$" care/facility/migrations/ | sort -n | tail -n 5
Length of output: 605
🧰 Tools
🪛 Ruff (0.8.2)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-28: Consider data preservation needs
I notice we're removing some rather important-looking fields here. I trust you've already thought about whether any of this data needs to be preserved for historical or audit purposes? 🤔
🧰 Tools
🪛 Ruff (0.8.2)
18-18: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/facility/migrations/0468_alter_facilitypatientstatshistory_unique_together_and_more.py
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2653 +/- ##
===========================================
+ Coverage 69.63% 70.48% +0.85%
===========================================
Files 211 201 -10
Lines 11879 11439 -440
Branches 1202 1164 -38
===========================================
- Hits 8272 8063 -209
+ Misses 3240 3014 -226
+ Partials 367 362 -5 ☔ View full report in Codecov by Sentry. |
|
@sainak Seems like you've done the remaining changes? thanks haha :) Decided best to ask in discussion after making pr |
yeah, there was some other stuff that needed to be removed, and it was easier this was |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
care/facility/api/viewsets/patient.py (1)
Line range hint
584-626: Enhance error handling in transfer methodThe transfer method could benefit from:
- Using status codes from
rest_framework.statusinstead of magic numbers- Consolidating validation logic
- Adding transaction management
+from django.db import transaction +from rest_framework import status @action(detail=True, methods=["POST"]) def transfer(self, request, *args, **kwargs): + with transaction.atomic(): patient = PatientRegistration.objects.get(external_id=kwargs["external_id"]) facility = get_object_or_404(Facility, external_id=request.data["facility"]) + validation_errors = [] if patient.is_expired: - return Response( - { - "Patient": "Patient transfer cannot be completed because the patient is expired" - }, - status=status.HTTP_406_NOT_ACCEPTABLE, - ) + validation_errors.append("Patient is expired") if patient.is_active and facility == patient.facility: - return Response( - { - "Patient": "Patient transfer cannot be completed because the patient has an active consultation in the same facility" - }, - status=status.HTTP_406_NOT_ACCEPTABLE, - ) + validation_errors.append("Patient has an active consultation in the same facility") if patient.allow_transfer is False: - return Response( - { - "Patient": "Patient transfer cannot be completed because the source facility does not permit it" - }, - status=status.HTTP_406_NOT_ACCEPTABLE, - ) + validation_errors.append("Source facility does not permit transfer") + + if validation_errors: + return Response( + {"errors": validation_errors}, + status=status.HTTP_406_NOT_ACCEPTABLE, + )
🧹 Nitpick comments (3)
care/facility/api/viewsets/patient.py (3)
26-26: Consider grouping related imports togetherThe import of
statusseems a bit lonely there betweenserializersandmixins. Perhaps it would feel more at home grouped with other REST framework imports? Just saying...
Line range hint
316-349: Simplify the diagnoses filter implementationThe
filter_by_diagnosesmethod could be more concise by extracting the verification status mapping to a class-level constant and using dict.get() for safer access.class PatientFilterSet(filters.FilterSet): + VERIFICATION_STATUS_MAP = { + "diagnoses_unconfirmed": ConditionVerificationStatus.UNCONFIRMED, + "diagnoses_provisional": ConditionVerificationStatus.PROVISIONAL, + "diagnoses_differential": ConditionVerificationStatus.DIFFERENTIAL, + "diagnoses_confirmed": ConditionVerificationStatus.CONFIRMED, + } def filter_by_diagnoses(self, queryset, name, value): if not value: return queryset filter_q = Q(last_consultation__diagnoses__diagnosis_id__in=value.split(",")) if name == "diagnoses": filter_q &= ~Q( last_consultation__diagnoses__verification_status__in=INACTIVE_CONDITION_VERIFICATION_STATUSES ) else: - verification_status = { - "diagnoses_unconfirmed": ConditionVerificationStatus.UNCONFIRMED, - "diagnoses_provisional": ConditionVerificationStatus.PROVISIONAL, - "diagnoses_differential": ConditionVerificationStatus.DIFFERENTIAL, - "diagnoses_confirmed": ConditionVerificationStatus.CONFIRMED, - }[name] + verification_status = self.VERIFICATION_STATUS_MAP.get(name) + if verification_status is None: + return queryset filter_q &= Q( last_consultation__diagnoses__verification_status=verification_status ) return queryset.filter(filter_q)
Line range hint
479-515: Add docstring to explain permission logicThe
filter_querysetmethod contains complex permission logic that could use some explanation. It would be really nice if future developers understood why certain user types have specific access levels.def filter_queryset(self, queryset: QuerySet) -> QuerySet: + """ + Filters queryset based on user permissions and CSV export requirements. + + Permission hierarchy: + - Superuser: Full access + - StateLabAdmin: Access to state facilities + - DistrictLabAdmin: Access to district facilities + - Other users: Access to allowed facilities, assigned patients + """ if self.action == "list" and settings.CSV_REQUEST_PARAMETER in self.request.GET: for backend in (PatientDRYFilter, filters.DjangoFilterBackend): queryset = backend().filter_queryset(self.request, queryset, self)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/facility/api/viewsets/patient.py(1 hunks)
🔇 Additional comments (1)
care/facility/api/viewsets/patient.py (1)
Line range hint 479-515: Optimize database queries for better performance
The queryset could be optimized to reduce database load:
- Consider using
prefetch_relatedfor reverse relationships - Cache frequently accessed querysets
- Use
values()orvalues_list()for simple lookups
Let's verify the query performance:
Proposed Changes
tasks/__init__.py.tasks/summarisation.py.config/settings/base.py.api/serializers/summary.py.api/viewsets/summary.py.config/api_router.py.Associated Issue
Merge Checklist
/docs@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Bug Fixes
Documentation
Chores