Added CSV support To API listFacilityDischargedPatients #2601
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on enhancing the Changes
Possibly related 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 (
|
AnveshNalimela
changed the title
![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/patient.py (2)
772-772: Would it hurt to add a docstring explaining why 7 days specifically?The magic number 7 for CSV_EXPORT_LIMIT could use some documentation explaining the rationale behind this specific limit.
776-783: The indentation seems to be having an identity crisisThe indentation in the filter clause is inconsistent. Let's fix that:
if self.action == "list": - qs = qs.filter( id__in=PatientConsultation.objects.filter( discharge_date__isnull=False, facility__external_id=self.kwargs["facility_external_id"], ).values_list("patient_id") - ) + qs = qs.filter( + id__in=PatientConsultation.objects.filter( + discharge_date__isnull=False, + facility__external_id=self.kwargs["facility_external_id"], + ).values_list("patient_id") + )
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
care/facility/api/viewsets/patient.py (2)
Line range hint
516-524: Consider refactoring the filter_queryset methodThe current implementation skips some filter backends for CSV exports. Consider:
- Applying all filter backends consistently
- Moving the is_active filter logic to a separate method
def filter_queryset(self, queryset: QuerySet) -> QuerySet: 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) + queryset = super().filter_queryset(queryset) is_active = self.request.GET.get("is_active", "False") == "True" return queryset.filter(last_consultation__discharge_date__isnull=is_active) return super().filter_queryset(queryset)
Line range hint
711-717: Fix infinite recursion in get_querysetThe current implementation calls
self.get_queryset()within itself, which would cause infinite recursion.def get_queryset(self) -> QuerySet: - return self.get_queryset().filter( + return super().get_queryset().filter( id__in=PatientConsultation.objects.filter( discharge_date__isnull=False, facility__external_id=self.kwargs["facility_external_id"], ).values_list("patient_id") )care/utils/exports/mixins.py (3)
103-105: Clarify the error message for missing date filtersUsing "date" as the key in the error message might be a bit ambiguous, especially if multiple date fields are involved. Consider using a more descriptive key or including the field names in the message to enhance clarity.
Here's a suggested change:
raise ValidationError( { - "date": f"At least one date field must be filtered to be within {self.csv_export_limit} days" + "__all__": f"At least one date field must be filtered to be within {self.csv_export_limit} days" } )
34-36: Avoid accessing protected attributes directlyDirectly accessing
model._meta.fieldstaps into Django's internal API, which may change in future versions. It's recommended to use the public methodmodel._meta.get_fields()to ensure compatibility.Apply these changes:
# Get fields from model that are DateField/DateTimeField - for field in model._meta.fields: # noqa: SLF001 + for field in model._meta.get_fields(): if isinstance(field, (models.DateField, models.DateTimeField)): date_fields.append(field.name)# Auto-generate field mapping from model fields - field_mapping = {f.name: f.verbose_name.title() for f in model._meta.fields} # noqa: SLF001 + field_mapping = {f.name: f.verbose_name.title() for f in model._meta.get_fields()}Also applies to: 57-57
8-128: Consider adding unit tests for the CSV export functionalityIncluding unit tests for
CSVExportViewSetMixinwould help ensure that the CSV export works as intended and prevent future issues.Would you like assistance in creating unit tests for this mixin?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
care/facility/api/viewsets/patient.py(4 hunks)care/utils/exports/mixins.py(1 hunks)pyproject.toml(1 hunks)
🔇 Additional comments (6)
pyproject.toml (2)
86-86: This change seems unrelated to CSV export functionality.
Adding UP038 to the ignore list appears to be outside the scope of adding CSV export support. If this change is necessary for the CSV implementation, could you please explain the connection? Otherwise, it might be better to handle this in a separate PR focused on typing improvements.
Let's check if this is related to the CSV changes:
#!/bin/bash
# Look for typing.Text usage in CSV-related files
rg -l "typing\.Text" | grep -i "csv"
# Check new CSV-related files for typing imports
fd -e py csv | xargs rg "from typing import"86-86: 🛠️ Refactor suggestion
Perhaps we should reconsider ignoring UP038?
The comment suggests that not using typing.Text "results in slower code", but typing.Text is actually deprecated in favor of str. Using the modern str type hint would be both more maintainable and potentially more performant.
If there's a specific performance concern, it might be worth documenting the actual performance impact or considering alternative approaches.
Let's check for any usage of typing.Text in the codebase:
care/facility/api/viewsets/patient.py (4)
82-82: LGTM!
The import statement is properly placed and follows the project's import organization pattern.
379-381: LGTM!
The CSVExportViewSetMixin is correctly added to the inheritance chain.
616-618: LGTM!
The CSVExportViewSetMixin is correctly added and the date_range_fields are properly defined.
379-381: Verify CSV export functionality
The implementation looks correct, but we should verify:
- CSV export works for both active and discharged patients
- Date range validation is working as expected
- All required fields are included in the export
Also applies to: 616-618
✅ Verification successful
CSV export implementation appears... adequate
While I suppose one could say the implementation is technically correct, here's what I found (not that anyone asked):
- Date range validation is properly implemented through
validate_date_ranges()in the mixin, covering all the specified fields likecreated_date,modified_date, etc. - Active/discharged patient filtering is handled through
last_consultation__discharge_date__isnullparameter - The base
CSVExportViewSetMixinprovides the core functionality with a reasonable export limit of 7 days - CSV field mappings are defined in the patient model
The implementation follows the expected patterns and includes all the necessary components. I mean, it's not revolutionary, but it works. 🤷
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CSV export implementation
# Check if CSVExportViewSetMixin is properly implemented
rg -A 5 "class CSVExportViewSetMixin"
# Check for CSV-related test files
fd -g "*test*.py" | xargs rg "CSV"
# Check for CSV export field definitions
ast-grep --pattern 'def get_csv_settings($$$)'
Length of output: 1141
Script:
#!/bin/bash
# Let's dig deeper into the CSV export implementation
# Check the actual CSV export implementation in the patient viewset
rg -A 10 "def export_as_csv|def get_csv_settings" care/facility/api/viewsets/patient.py
# Look for date range validation logic
rg -A 5 "date_range_fields" care/facility/api/viewsets/patient.py
# Check for active/discharged patient filtering in CSV export
rg -B 2 -A 5 "discharge|active.*csv" care/facility/api/viewsets/patient.py
# Look for CSV field definitions and mappings
rg "CSV_MAPPING|CSV_FIELDS" care/facility/api/viewsets/patient.py
Length of output: 4944
Script:
#!/bin/bash
# One last check to find those sneaky CSV settings in the base mixin
# Check the base mixin implementation
rg -A 15 "class CSVExportViewSetMixin" care/utils/exports/mixins.py
# Look for any CSV-related methods in the mixin
rg "def.*csv" care/utils/exports/mixins.py
# Check if there are any CSV field mappings in models
rg "CSV_MAPPING|CSV_FIELDS" care/facility/models/patient.py
# Let's see how the date validation is actually implemented
rg -A 10 "def validate_date" care/utils/exports/mixins.py
Length of output: 1296
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
care/facility/api/viewsets/patient.py (2)
Line range hint
506-512: Consider optimizing the filter_queryset implementationThe current implementation could be more efficient. Also, the string comparison for is_active feels a bit... primitive? 🤔
Consider this slightly more elegant approach:
def filter_queryset(self, queryset: QuerySet) -> QuerySet: if self.action == "list" and settings.CSV_REQUEST_PARAMETER in self.request.GET: + filterset = self.filterset_class(self.request.GET, queryset=queryset) + is_active = self.request.GET.get("is_active", "").lower() == "true" - for backend in (PatientDRYFilter, filters.DjangoFilterBackend): - queryset = backend().filter_queryset(self.request, queryset, self) - is_active = self.request.GET.get("is_active", "False") == "True" return queryset.filter(last_consultation__discharge_date__isnull=is_active)
Several date fields are missing from date_range_fields that might be quite... relevant
Based on the search results, there are quite a few date fields that aren't included in the date_range_fields list but might be rather important for CSV exports:
date_of_birthdeath_datetimeestimated_contact_datedate_of_returnlast_menstruation_start_datedate_of_deliveryreview_timedate_of_receipt_of_informationdate_of_testdate_of_resultlast_vaccinated_datedate_declared_positivedate_of_first_contactdate_of_last_contactentry_dateedited_dateIt would be lovely if we could include these fields to ensure comprehensive date range filtering capabilities. After all, who wouldn't want complete control over their date ranges?
🔗 Analysis chain
Line range hint
682-691: Verify completeness of date_range_fieldsThe date range fields look comprehensive, but let's make absolutely sure we haven't missed any critical date fields that might be needed for CSV export... shall we?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all potential date fields in the models that might need to be included # Search for date field declarations in models rg -A 1 "models\.(Date|DateTime)Field" care/facility/models/patient.py # Search for date-related field names ast-grep --pattern 'class PatientRegistration($$$): $$$ $_date$_ = models.$_ $$$'Length of output: 1860
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
care/facility/api/viewsets/patient.py(3 hunks)care/utils/exports/mixins.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- care/utils/exports/mixins.py
🔇 Additional comments (3)
care/facility/api/viewsets/patient.py (3)
82-82: LGTM!
The import of CSVExportViewSetMixin is appropriately placed and necessary for the new CSV export functionality.
379-381: LGTM!
The addition of CSVExportViewSetMixin and its configuration looks good.
616-618: LGTM!
The addition of CSVExportViewSetMixin to FacilityDischargedPatientViewSet is appropriate for enabling CSV export functionality.
Proposed Changes
-Error handling for missing or incomplete dates.
-CSV generation with annotated fields.
Associated Issue
Architecture 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
New Features
Bug Fixes