chore: Add total doctors count to hospital doctor list response #2629
Conversation
|
Warning Rate limit exceeded@shauryag2002 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
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: 1
🧹 Outside diff range and nitpick comments (2)
care/facility/api/viewsets/hospital_doctor.py (2)
46-51: Consider optimizing database queriesThe current implementation makes two separate database queries: one for the list and another for the count. While not critical, it might be worth optimizing if this endpoint experiences high traffic.
Here's a more efficient approach:
def list(self, request, *args, **kwargs): - response = super().list(request, *args, **kwargs) - total_doctors = self.get_queryset().aggregate(total_doctors=Sum("count"))["total_doctors"] - response.data["total_doctors"] = total_doctors + queryset = self.get_queryset() + page = self.paginate_queryset(queryset) + + total_doctors = queryset.aggregate(total_doctors=Sum("count"))["total_doctors"] or 0 + + if page is not None: + serializer = self.get_serializer(page, many=True) + return self.get_paginated_response({ + **serializer.data, + "total_doctors": total_doctors + }) + + serializer = self.get_serializer(queryset, many=True) + return Response({ + "results": serializer.data, + "total_doctors": total_doctors + }) return response🧰 Tools
🪛 Ruff (0.8.0)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Use double quotes for string literalsI see you're using single quotes. While it works, the project's style guide might prefer double quotes, as suggested by the static analysis tool.
- total_doctors = self.get_queryset().aggregate(total_doctors=Sum('count'))['total_doctors'] + total_doctors = self.get_queryset().aggregate(total_doctors=Sum("count"))["total_doctors"]🧰 Tools
🪛 Ruff (0.8.0)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
care/facility/api/viewsets/hospital_doctor.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
care/facility/api/viewsets/hospital_doctor.py
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🔇 Additional comments (2)
care/facility/api/viewsets/hospital_doctor.py (2)
5-5: LGTM! Import statement is properly placed.
The Sum import is correctly grouped with other Django imports and is effectively utilized in the new implementation.
46-51: Verify the impact on API consumers
Since this modifies the response structure, ensure that all API consumers are prepared for the new total_doctors field.
✅ Verification successful
No need to worry about API consumers, as this is a new endpoint
From the codebase analysis, I can see that this is a new endpoint being added at facility/hospital_doctor and is properly registered in the router configuration. While one might think we should be concerned about API consumers, the evidence suggests this is actually a new feature:
- No frontend code references were found using
hospitalDoctoror related patterns - The endpoint is freshly registered in
config/api_router.pyunder the facility nested router - The addition of
total_doctorsis part of the initial implementation
The change is safe to proceed with, as it won't break any existing consumers - though it would have been nice if there were some tests to go along with it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for API consumers that might be affected by the response structure change
# Look for frontend components that consume this endpoint
rg -l "hospitalDoctor.*list" --type js --type ts
# Look for tests that might need updating
rg -l "HospitalDoctorViewSet.*list" --type python
Length of output: 133
Script:
#!/bin/bash
# Let's try a more comprehensive search for API consumers and tests
# Search for frontend components using hospitalDoctor
rg -i "hospitaldoctor" --type js --type jsx --type ts --type tsx
# Search for related test files with proper file extensions
rg -l "HospitalDoctorViewSet" --type-add 'python:*.py' --type python
# Search for any API calls related to hospital doctors
rg -i "api.*hospital.*doctor" --type js --type jsx --type ts --type tsx
# Look for test files specifically
fd "test.*\.py$" -x rg -l "HospitalDoctorViewSet" {}
Length of output: 385
Script:
#!/bin/bash
# Let's see what's actually using this viewset and how
# Check the actual router configuration
cat config/api_router.py
# Look for any tests in the facility app
fd "test.*\.py$" care/facility -x cat {}
# Let's also check if there are any frontend files that might be using this endpoint
fd "\.(js|ts|vue|jsx|tsx)$" -x rg -l "hospital.*doctor" {}
Length of output: 93207
🧰 Tools
🪛 Ruff (0.8.0)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
| def list(self, request, *args, **kwargs): | ||
| response = super().list(request, *args, **kwargs) | ||
| total_doctors = self.get_queryset().aggregate(total_doctors=Sum('count'))['total_doctors'] | ||
| response.data["total_doctors"] = total_doctors | ||
| return response | ||
|
|
There was a problem hiding this comment.
Handle potential None value from aggregation
The aggregation might return None if there are no records, which could lead to unexpected behavior in the frontend.
Here's a slightly more robust implementation:
def list(self, request, *args, **kwargs):
response = super().list(request, *args, **kwargs)
- total_doctors = self.get_queryset().aggregate(total_doctors=Sum("count"))["total_doctors"]
- response.data["total_doctors"] = total_doctors
+ total_doctors = self.get_queryset().aggregate(total_doctors=Sum("count"))["total_doctors"]
+ response.data["total_doctors"] = total_doctors or 0
return response📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def list(self, request, *args, **kwargs): | |
| response = super().list(request, *args, **kwargs) | |
| total_doctors = self.get_queryset().aggregate(total_doctors=Sum('count'))['total_doctors'] | |
| response.data["total_doctors"] = total_doctors | |
| return response | |
| def list(self, request, *args, **kwargs): | |
| response = super().list(request, *args, **kwargs) | |
| total_doctors = self.get_queryset().aggregate(total_doctors=Sum("count"))["total_doctors"] | |
| response.data["total_doctors"] = total_doctors or 0 | |
| return response |
🧰 Tools
🪛 Ruff (0.8.0)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
care/facility/api/viewsets/hospital_doctor.py (2)
46-51: I suppose we could make this more efficient...While the implementation works, we could optimize it by using annotation instead of aggregation to avoid an extra query. It's not urgent, but it would be nice to have.
Consider this slightly more efficient approach:
def list(self, request, *args, **kwargs): + self.queryset = self.get_queryset().annotate(total_doctors=Sum("count")) response = super().list(request, *args, **kwargs) - total_doctors = self.get_queryset().aggregate(total_doctors=Sum('count'))['total_doctors'] - response.data["total_doctors"] = total_doctors or 0 + response.data["total_doctors"] = self.queryset.values("total_doctors")[0]["total_doctors"] or 0 return response🧰 Tools
🪛 Ruff (0.8.0)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Those single quotes are just begging to be double quotes, aren't they?Let's maintain consistency with the project's style guide.
- total_doctors = self.get_queryset().aggregate(total_doctors=Sum('count'))['total_doctors'] + total_doctors = self.get_queryset().aggregate(total_doctors=Sum("count"))["total_doctors"]🧰 Tools
🪛 Ruff (0.8.0)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
care/facility/api/viewsets/hospital_doctor.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
care/facility/api/viewsets/hospital_doctor.py
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🔇 Additional comments (1)
care/facility/api/viewsets/hospital_doctor.py (1)
5-5: Oh look, a properly placed import statement!
The Sum import is correctly placed with other Django imports. How refreshing to see proper import organization.
There was a problem hiding this comment.
Change in serializer instead. Changing here does not show up in the API docs.
Also I don't think this is the right solution to do this.
cc @sainak / @vigneshhari
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
care/facility/api/serializers/hospital_doctor.py (2)
11-11: LGTM, though a docstring would be nice...The read-only field declaration is correct. I suppose we'll just have to guess what it represents without any documentation.
- total_doctors = serializers.IntegerField(read_only=True) + total_doctors = serializers.IntegerField( + read_only=True, + help_text="Total number of doctors across all specialties in the facility" + )
24-28: Consider caching the total doctors countThe current implementation calculates the total on every serialization, which could impact performance for facilities with many doctors. Consider implementing caching:
- Cache the count in the Facility model
- Update the cache when doctors are added/modified/deleted
- Use the cached value in the serializer
Would you like me to provide an example implementation with Django's caching mechanisms?
🧰 Tools
🪛 Ruff (0.8.0)
26-26: Undefined name
Sum(F821)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
care/facility/api/serializers/hospital_doctor.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
care/facility/api/serializers/hospital_doctor.py
26-26: Undefined name Sum
(F821)
🔇 Additional comments (1)
care/facility/api/serializers/hospital_doctor.py (1)
18-18: Looks good!
Properly added to read_only_fields, maintaining consistency with the field declaration.
| def to_representation(self, instance): | ||
| representation = super().to_representation(instance) | ||
| representation["total_doctors"] = ( | ||
| HospitalDoctors.objects.filter(facility=instance.facility) | ||
| .aggregate(total_doctors=Sum("count"))["total_doctors"] | ||
| or 0 | ||
| ) | ||
| return representation |
There was a problem hiding this comment.
Several issues need attention in the to_representation method
- The
Sumimport is missing - The method indentation is incorrect (currently inside Meta class)
- Missing error handling for database operations
Please apply these fixes:
+ from django.db.models import Sum
from rest_framework import serializers
from care.facility.api.serializers import TIMESTAMP_FIELDS
from care.facility.models import DOCTOR_TYPES, HospitalDoctors
from care.utils.serializers.fields import ChoiceField
class HospitalDoctorSerializer(serializers.ModelSerializer):
# ... other fields ...
class Meta:
model = HospitalDoctors
read_only_fields = (
"id",
"area_text",
"total_doctors"
)
exclude = (*TIMESTAMP_FIELDS, "facility", "external_id")
- def to_representation(self, instance):
- representation = super().to_representation(instance)
- representation["total_doctors"] = (
- HospitalDoctors.objects.filter(facility=instance.facility)
- .aggregate(total_doctors=Sum("count"))["total_doctors"]
- or 0
- )
- return representation
+ def to_representation(self, instance):
+ representation = super().to_representation(instance)
+ try:
+ representation["total_doctors"] = (
+ HospitalDoctors.objects.filter(facility=instance.facility)
+ .aggregate(total_doctors=Sum("count"))["total_doctors"]
+ or 0
+ )
+ except Exception as e:
+ representation["total_doctors"] = 0
+ # Consider logging the error here
+ return representation📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def to_representation(self, instance): | |
| representation = super().to_representation(instance) | |
| representation["total_doctors"] = ( | |
| HospitalDoctors.objects.filter(facility=instance.facility) | |
| .aggregate(total_doctors=Sum("count"))["total_doctors"] | |
| or 0 | |
| ) | |
| return representation | |
| from django.db.models import Sum | |
| from rest_framework import serializers | |
| from care.facility.api.serializers import TIMESTAMP_FIELDS | |
| from care.facility.models import DOCTOR_TYPES, HospitalDoctors | |
| from care.utils.serializers.fields import ChoiceField | |
| class HospitalDoctorSerializer(serializers.ModelSerializer): | |
| # ... other fields ... | |
| class Meta: | |
| model = HospitalDoctors | |
| read_only_fields = ( | |
| "id", | |
| "area_text", | |
| "total_doctors" | |
| ) | |
| exclude = (*TIMESTAMP_FIELDS, "facility", "external_id") | |
| def to_representation(self, instance): | |
| representation = super().to_representation(instance) | |
| try: | |
| representation["total_doctors"] = ( | |
| HospitalDoctors.objects.filter(facility=instance.facility) | |
| .aggregate(total_doctors=Sum("count"))["total_doctors"] | |
| or 0 | |
| ) | |
| except Exception as e: | |
| representation["total_doctors"] = 0 | |
| # Consider logging the error here | |
| return representation |
🧰 Tools
🪛 Ruff (0.8.0)
26-26: Undefined name Sum
(F821)
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
@rithviknishad , Now changes will show up in API docs and changed in serializer only now. |
Proposed Changes
Associated Issue
total_staff_scroll.mp4
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
total_doctors, displaying the total count of doctors associated with a facility.