Location, Device, Filters and More #2782
Conversation
# Conflicts: # care/emr/api/viewsets/condition.py
📝 WalkthroughWalkthroughThe pull request introduces comprehensive enhancements to the CodeRabbit EMR system, focusing on facility locations, encounters, and API documentation. Key changes include new models for facility locations, improved authorization mechanisms, schema generation for API endpoints, and refinements to existing viewsets. The modifications expand the system's capabilities for managing healthcare resources with more granular control and better documentation. It's almost like we needed these changes yesterday, but hey, better late than never, right? Changes
Sequence DiagramsequenceDiagram
participant Client
participant FacilityLocationViewSet
participant Authorization
participant Database
Client->>FacilityLocationViewSet: Create Location Request
FacilityLocationViewSet->>Authorization: Validate Permissions
Authorization-->>FacilityLocationViewSet: Permission Granted
FacilityLocationViewSet->>Database: Save Location
Database-->>FacilityLocationViewSet: Location Created
FacilityLocationViewSet-->>Client: Success Response
Poem
✨ Finishing Touches
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 (
|
…support adding swagger support
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
care/users/models.py (1)
Line range hint
138-152: Interesting choice of default phone number there... very creative! 📞While the interactive prompt is a nice addition, there are a few things that could be improved:
- The hardcoded phone number
+919696969696seems... unprofessional- The hardcoded gender value
3lacks flexibility- The error message could be more informative about the impact of proceeding without district data
Consider making these values configurable:
- extra_fields["phone_number"] = "+919696969696" - extra_fields["gender"] = 3 + extra_fields["phone_number"] = getattr(settings, "DEFAULT_SUPERUSER_PHONE", "+919696969696") + extra_fields["gender"] = getattr(settings, "DEFAULT_SUPERUSER_GENDER", 3)Also, enhance the error message:
if not district: proceed = input( - f"It looks like you haven't loaded district data. It is recommended to populate district data before you create a super user. Please run `python manage.py {data_command}`.\n Proceed anyway? [y/N]" + f"Warning: No district data found. Creating a superuser without district data may limit their ability to manage geographical hierarchies.\n" + f"To load district data, run: python manage.py {data_command}\n" + f"Proceed without district data? [y/N]: " )
🧹 Nitpick comments (19)
care/security/authorization/facility_location.py (2)
13-21: Consider null-safety.
You’re usinglocation.facility_organization_cache, so be sure location is neverNone.
55-68: Possible scaling bottleneck.
Filtering large querysets by overlapping org IDs may be slow. Consider a more direct join or caching solution when user roles are extensive.care/emr/models/location.py (5)
11-31: JSON fields caution.
These are flexible but can complicate data integrity and indexing. If frequent queries on these fields are expected, consider structured columns.
52-76: Custom uniqueness check.
While workable, it might lag behind real-time database state under heavy concurrency. Database-level constraints would be more reliable.
98-114: Parent reassignments.
Updating the parent post-creation can cause mismatches. Validate or re-sync children if the parent changes.Do you want suggestions for a robust parent-child sync mechanism?
115-117: Async cascade.
Invoking a Celery task is nice, but ensure you handle task failures—otherwise changes might only partially propagate.
119-132: Frequent cascade triggers.
Each instance save triggers a cascade task. This may be costly for large hierarchical structures. Consider batching updates or using signals more selectively.care/emr/api/viewsets/location.py (4)
42-47: Address the TODO regarding patient association before deletionSince there's a note to check if patient associations exist, consider implementing this validation to avoid inadvertent data inconsistencies.
48-55: Review error type consistency for parent location checksYou’re raising both
PermissionDeniedandValidationErrorin similar contexts. Ensuring consistent error types can improve the consuming client’s ability to handle responses uniformly.
79-81: Clarify the reuse of authorize_update in authorize_destroyWhile deferring to
authorize_updateis clever, a more descriptive name (e.g.,authorize_modification) could reduce confusion for new contributors.
176-192: Implement the TODO for encounter associationThere’s a mention that association models are yet to be built. This might be paramount to maintain referential integrity for encounters and their locations.
care/security/authorization/__init__.py (1)
10-10: Avoid wildcard imports from facility_locationExplicit imports enhance clarity, reduce the exposure of internals, and minimize potential naming collisions.
-from .facility_location import * # noqa +from .facility_location import FacilityLocationAccess # or other specific classescare/emr/models/__init__.py (1)
10-10: Replace wildcard imports from locationExplicit imports help maintain clarity and avoid unintentional overrides.
-from .location import * # noqa F403 +from .location import FacilityLocation, FacilityLocationOrganization # or necessary classescare/security/permissions/location.py (1)
15-27: Add meaningful descriptions for permissions.The permissions are well-structured, but those empty description strings are just begging to be filled with useful information that future maintainers might appreciate.
can_list_facility_locations = Permission( "Can List Facility Locations", - "", + "Allows users to view and list all facility locations in the system", PermissionContext.FACILITY, [ ADMIN_ROLE, DOCTOR_ROLE, FACILITY_ADMIN_ROLE, GEO_ADMIN, NURSE_ROLE, STAFF_ROLE, ], )care/emr/resources/location/spec.py (2)
17-24: Consider using more descriptive enum values.Those single-letter status codes (C, H, O, U, K, I) are quite... minimalist. Perhaps we could make them a bit more self-documenting?
class FacilityLocationStatusChoices(str, Enum): - C = "C" - H = "H" - O = "O" # noqa E741 - U = "U" - K = "K" - I = "I" # noqa E741 + CLOSED = "C" + HOLD = "H" + OPERATIONAL = "O" + UNDER_CONSTRUCTION = "U" + KNOWN = "K" + INACTIVE = "I"
74-82: Enhance parent validation with more specific error message.The parent validation could be a bit more helpful with its error messaging, don't you think?
@model_validator(mode="after") def validate_parent_organization(self): if ( self.parent and not FacilityLocation.objects.filter(external_id=self.parent).exists() ): - err = "Parent not found" + err = f"Parent facility with ID {self.parent} does not exist" raise ValueError(err) return selfcare/emr/api/viewsets/resource_request.py (1)
25-32: Consider adding index hints for filtered fields.The filters look great, but since we're filtering on external_id fields, it might be nice to ensure they're properly indexed.
Consider adding the following indexes to your models:
class Meta: indexes = [ models.Index(fields=['external_id']), # Add similar indexes for other frequently filtered fields ]care/emr/migrations/0010_facilitylocation_facilitylocationorganization.py (1)
32-32: Consider increasing the description field length.The description field might be a bit restrictive at 255 characters. I mean, I'm sure you know what you're doing, but longer descriptions could be useful for complex locations.
- ('description', models.CharField(max_length=255)), + ('description', models.TextField(blank=True)),🧰 Tools
🪛 Ruff (0.8.2)
32-32: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/emr/api/viewsets/base.py (1)
186-187: Consider adding validation implementation guidance.The empty
validate_destroymethod could use some documentation about expected validation logic. You probably already thought of this, but adding a docstring would be super helpful for other developers.def validate_destroy(self, instance): - pass + """ + Override this method to implement custom validation logic before destroying an instance. + + Raise ValidationError if the instance cannot be destroyed. + Example: + if instance.has_active_children: + raise ValidationError("Cannot delete instance with active children") + """ + pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Makefile(1 hunks)care/emr/api/viewsets/base.py(2 hunks)care/emr/api/viewsets/location.py(1 hunks)care/emr/api/viewsets/resource_request.py(2 hunks)care/emr/migrations/0010_facilitylocation_facilitylocationorganization.py(1 hunks)care/emr/migrations/0011_alter_facilitylocation_location_type.py(1 hunks)care/emr/models/__init__.py(1 hunks)care/emr/models/location.py(1 hunks)care/emr/models/organization.py(1 hunks)care/emr/resources/location/spec.py(1 hunks)care/security/authorization/__init__.py(1 hunks)care/security/authorization/facility_location.py(1 hunks)care/security/permissions/base.py(2 hunks)care/security/permissions/location.py(1 hunks)care/users/models.py(1 hunks)config/api_router.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/migrations/0011_alter_facilitylocation_location_type.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)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/emr/migrations/0010_facilitylocation_facilitylocationorganization.py
13-13: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
13-13: 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)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: 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)
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)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: 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)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: 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)
31-31: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
32-32: 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)
34-34: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
35-35: 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)
37-37: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
38-38: 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)
40-40: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
41-41: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
42-42: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
42-42: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
43-43: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
43-43: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
44-44: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
44-44: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
46-46: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
46-46: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
49-49: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
53-53: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
55-55: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
55-55: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
56-56: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
57-57: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
58-58: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
59-59: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
60-60: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
61-61: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
62-62: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
62-62: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
63-63: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
63-63: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
64-64: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
64-64: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
65-65: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
65-65: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
68-68: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (11)
care/security/permissions/base.py (2)
6-6: Good job importing location permissions.
This aligns with the new facility location permission structure. Just confirm that no existing permission names are duplicated.
36-36: Double-check permission collision.
Make sureFacilityLocationPermissionsdoesn’t conflict with permissions defined in other handlers.✅ Verification successful
No permission collisions detected, but you're welcome to double-check if you don't trust me 🙂
The
FacilityLocationPermissionsclass is properly implemented as an enum with unique permissions scoped to facility locations. The permissions are:
- can_list_facility_locations
- can_write_facility_locations
These don't overlap with any other permission definitions in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for repeated permission definitions across handlers rg --count-matches "Permission\(.+=[\"\']" -A 2Length of output: 155
Script:
#!/bin/bash # Find Permission classes and their content ast-grep --pattern 'class $_ (Permission) { $$$ }' # Also search for direct Permission assignments rg -l "Permission\s*=\s*['\"].*['\"]" # Find FacilityLocationPermissions class definition ast-grep --pattern 'class FacilityLocationPermissions { $$$ }'Length of output: 161
Script:
#!/bin/bash # First, find the relevant files fd -e py # Then search for permission-related content in Python files rg "class.*Permission" -t py # Look for FacilityLocationPermissions specifically rg "FacilityLocationPermissions" -t py # Search for permission string definitions rg "permission.*=.*['\"]" -t pyLength of output: 36354
care/security/authorization/facility_location.py (2)
44-54: Straightforward permission check.
The logic here matches the pattern used elsewhere, so it seems safe.
70-70: Centralized controller registration.
Registering the controller here is consistent with your current architecture. Keep an eye out for double imports if the app reloads often.care/emr/models/location.py (2)
34-50: Concurrency watch.
Concurrent calls toget_parent_jsonmay introduce stale reads forcached_parent_json. Evaluate whether you need a lock or transaction-based approach.
77-97: Handle missing root org carefully.
Skipping root organization updates can create an inconsistent cache. Log or warn if it’s not found.care/emr/api/viewsets/location.py (1)
28-36: Great use of filterset_class with DjangoFilterBackendThis approach is well-structured for handling query-based filtering.
care/emr/migrations/0011_alter_facilitylocation_location_type.py (1)
13-17: Consider data migration strategy for existing records.While the migration itself is correct, it would be lovely if we could ensure existing records are properly handled during this field type change. Consider adding a data migration step if there's existing data.
✅ Verification successful
Migration is safe, no data migration strategy needed 🎯
The field transition from CharField to JSONField is automatically handled by Django, as any existing string values will become NULL (which is allowed by the new field definition). The default dict ensures new records are properly initialized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are existing records that might be affected python << EOF from django.db import connection with connection.cursor() as cursor: cursor.execute(""" SELECT COUNT(*) FROM emr_facilitylocation WHERE location_type IS NOT NULL AND location_type != ''; """) count = cursor.fetchone()[0] print(f"Records with non-empty location_type: {count}") EOFLength of output: 127
Script:
#!/bin/bash # Check for previous migrations affecting this field fd "^[0-9]{4}.*py$" care/emr/migrations/ --exec grep -l "location_type" {} # Look for model usage patterns rg "location_type" care/emr/ -B 2 -A 2 --no-heading # Check for any test files using this field fd "test.*py$" care/ --exec grep -l "location_type" {}Length of output: 2387
🧰 Tools
🪛 Ruff (0.8.2)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/emr/api/viewsets/resource_request.py (1)
39-40: LGTM! Clean filter implementation.The filter configuration is well-structured and properly integrated.
config/api_router.py (1)
155-159: LGTM! Clean and consistent route registration.The new route follows the established pattern perfectly. Nice work!
care/emr/api/viewsets/base.py (1)
191-194: LGTM! Proper use of status codes.Nice update to use the proper status code constant instead of magic number. 👍
| ('mode', models.CharField(max_length=255)), | ||
| ('location_type', models.CharField(max_length=255)), | ||
| ('form', models.CharField(max_length=255)), | ||
| ('facility_organization_cache', django.contrib.postgres.fields.ArrayField(base_field=models.IntegerField(), default=list, size=None)), |
There was a problem hiding this comment.
Avoid using mutable defaults for JSON fields.
Using default=dict for JSON fields can lead to shared state issues. I'm sure you were going to fix this anyway, but here's a gentle reminder.
- ('facility_organization_cache', django.contrib.postgres.fields.ArrayField(base_field=models.IntegerField(), default=list, size=None)),
+ ('facility_organization_cache', django.contrib.postgres.fields.ArrayField(base_field=models.IntegerField(), default=list, size=None)),
- ('metadata', models.JSONField(default=dict)),
- ('cached_parent_json', models.JSONField(default=dict)),
+ ('metadata', models.JSONField(default=dict)),
+ ('cached_parent_json', models.JSONField(default=dict)),Also applies to: 40-40, 41-41
🧰 Tools
🪛 Ruff (0.8.2)
36-36: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/emr/migrations/0010_facilitylocation_facilitylocationorganization.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (6)
care/emr/api/viewsets/batch_request.py (1)
Line range hint
34-57: That magic number 299 is quite... magical.The error handling could use some refinement, and that status code comparison is begging to be made more explicit.
Consider these improvements:
- if response["status_code"] > 299: # noqa PLR2004 + HTTP_ERROR_THRESHOLD = 300 + if response["status_code"] >= HTTP_ERROR_THRESHOLD: errored = TrueAlso, it might be nice to:
- Add error details to the 400 response
- Use HTTP status code constants from REST framework
- Add docstring explaining the batch request behavior
care/emr/api/viewsets/observation.py (1)
Line range hint
66-85: This could be more efficient.The code makes separate database queries for each code, which might not be the most efficient approach when dealing with multiple codes.
Consider using a single query with
Qobjects:- results = [] - for code in request_params.codes: - code_queryset = queryset.filter( - main_code__code=code.code, main_code__system=code.system - )[:page_size] + from django.db.models import Q + code_filters = [Q(main_code__code=code.code, main_code__system=code.system) + for code in request_params.codes] + combined_queryset = queryset.filter(reduce(operator.or_, code_filters)) + results = [] + for code in request_params.codes: + code_results = [ + self.get_read_pydantic_model() + .serialize(obj) + .model_dump(exclude=["meta"]) + for obj in combined_queryset.filter( + main_code__code=code.code, + main_code__system=code.system + )[:page_size] + ]care/emr/api/viewsets/questionnaire_response.py (1)
Line range hint
63-67: Query parameter validation seems a bit casual.The code directly accesses query parameters without validation. I'm sure nothing could go wrong with that.
Consider adding proper validation:
+ from django.core.validators import validate_comma_separated_integer_list + if "questionnaire_slugs" in self.request.GET: questionnaire_slugs = self.request.GET.get("questionnaire_slugs").split(",") + try: + validate_comma_separated_integer_list(self.request.GET.get("questionnaire_slugs")) + except ValidationError: + raise ValidationError({"questionnaire_slugs": "Invalid format"}) queryset = queryset.filter(questionnaire__slug__in=questionnaire_slugs)care/emr/api/otp_viewsets/login.py (1)
Line range hint
21-24: Hardcoded OTP in non-SMS mode seems... interesting.Using "45612" as a default OTP in non-SMS mode might not be the most secure approach, even in development.
Consider using a more secure approach:
- if not settings.USE_SMS: - return "45612" + return "".join(secrets.choice(string.digits) for _ in range(size))care/emr/api/viewsets/scheduling/booking.py (1)
That allocated field is feeling a bit too... negative 🤨
The verification confirms there's no protection against negative slot allocations:
- The
allocatedfield inTokenSlotmodel uses a plainIntegerField- No validation before decrementing in
cancel_appointment_handlerYou might want to:
- Add a check before decrementing:
if instance.token_slot.allocated > 0- Consider using
PositiveIntegerFieldinstead ofIntegerField🔗 Analysis chain
Line range hint
9-10: Verify error handling in cancel_appointment_handler.The slot allocation decrement might cause issues if it goes below zero.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential negative slot allocations rg -A 5 "allocated.*-=.*1" .Length of output: 542
Script:
#!/bin/bash # Search for TokenSlot model definition and any related validators ast-grep --pattern 'class TokenSlot' # Also search for any validators or constraints rg -B 2 -A 5 "allocated.*=.*models\." .Length of output: 759
care/emr/api/viewsets/scheduling/schedule.py (1)
Line range hint
65-73: That lock might not be as protective as you think.The lock in
perform_destroyis released after checking for future bookings but before the actual deletion. This could lead to race conditions if bookings are created between the check and delete operations.Consider this slightly more robust implementation:
def perform_destroy(self, instance): - with Lock(f"booking:resource:{instance.resource.id}"), transaction.atomic(): + lock_key = f"booking:resource:{instance.resource.id}" + with transaction.atomic(): + with Lock(lock_key): # Check if there are any tokens allocated for this schedule in the future availabilities = instance.availability_set.all() has_future_bookings = TokenSlot.objects.filter(
🧹 Nitpick comments (23)
care/utils/tests/test_swagger_schema.py (1)
7-7: That URL looks rather... fixed in place.Using hardcoded URLs in tests can be fragile. Django's reverse() would be more maintainable.
Consider using the URL resolver:
-response = self.client.get("/swagger/") +from django.urls import reverse +response = self.client.get(reverse('schema-swagger-ui'))care/emr/api/viewsets/roles.py (1)
9-12: The schema decorator addition looks fine, I suppose.The decorator will enhance API documentation, which is... nice. Though it might have been even nicer with some class-level docstrings explaining what this viewset actually does.
Consider adding a class-level docstring to describe the purpose and usage of this viewset.
care/emr/api/otp_viewsets/patient.py (1)
Line range hint
14-24: Oh look, another viewset without documentation.While adding the schema decorator is great and all, it would be absolutely wonderful if we could also get some docstrings explaining:
- The purpose of this OTP-based patient view
- The authentication flow
- The permission requirements
- The relationship between phone numbers and patients
Consider adding comprehensive docstrings. You know, for those of us who might need to maintain this code someday.
care/utils/decorators/schema_decorator.py (1)
8-26: The base actions could use some organization.While the implementation is functional, it might be cleaner to extract these action definitions.
Consider moving the base actions to a separate constant or configuration:
BASE_SWAGGER_ACTIONS = { "create": { "request": lambda cls: cls.pydantic_model, "responses": lambda cls: {200: cls.pydantic_read_model or cls.pydantic_model}, }, # ... other actions }care/emr/api/viewsets/valueset.py (1)
Line range hint
14-18: Consider adding more validation to ExpandRequest.The display_language field could use some validation to ensure it matches a valid language code format. But I suppose that's not strictly necessary if you enjoy surprises.
class ExpandRequest(BaseModel): search: str = "" count: int = Field(10, gt=0, lt=100) - display_language: str = "en-gb" + display_language: str = Field("en-gb", pattern="^[a-z]{2}-[a-z]{2}$")care/emr/api/viewsets/medication_request.py (1)
Line range hint
51-63: Consider adding error details to PermissionDenied.The error message could be more specific about why the permission was denied. Unless keeping users in the dark about permissions is part of the security strategy.
if not AuthorizationController.call( "can_update_encounter_obj", requester, encounter ): raise PermissionDenied( - "Requester does not have permission to update encounter" + f"User {requester.external_id} does not have permission to update encounter {encounter.external_id}. Required role: encounter_updater" )care/emr/api/viewsets/scheduling/availability_exceptions.py (2)
22-22: Nice addition of the swagger schema decorator!Though I can't help but notice it's just documentation without any actual schema specifications.
Consider adding response schemas to make the documentation even more helpful, if you're not too busy:
@generate_swagger_schema_decorator @extend_schema( responses={ } )
Line range hint
45-47: Error messages could be more... informative.The error message "You do not have permission to view user schedule" is used for both writing and viewing permissions. A more specific message might help users understand what they actually can't do.
Perhaps consider:
- raise PermissionDenied("You do not have permission to view user schedule") + raise PermissionDenied("You do not have permission to modify user schedule")care/emr/api/viewsets/observation.py (1)
61-63: Swagger schema looks good, but...The
extend_schemadecorator could use a response schema. I mean, if we're doing documentation, might as well do it properly?@extend_schema( request=ObservationAnalyseRequest, + responses={200: {"type": "object", "properties": {"results": {"type": "array"}}}} )care/emr/api/otp_viewsets/login.py (1)
85-87: The schema looks nice.Though we might want to add response schemas for better API documentation. Just saying.
@extend_schema( request=OTPLoginSpec, + responses={ + 200: {"type": "object", "properties": {"access": {"type": "string"}}}, + 400: {"type": "object", "properties": {"otp": {"type": "string"}}} + } )care/emr/api/otp_viewsets/slot.py (3)
38-44: Consider enhancing error handling for authentication failures.While the authentication setup is good, it might be nice if we had more specific error messages for different authentication failure scenarios... you know, if you're into that sort of thing.
45-53: Add response schema to @extend_schema decorator.I couldn't help but notice you specified the request schema but left out the response schema. It's almost like you're trying to keep it a surprise.
Line range hint
55-67: Enhance validation error message.The current error message "Patient not allowed" is a bit... vague. Maybe we could be a tad more specific about why they're not allowed? Just a thought.
- raise ValidationError("Patient not allowed") + raise ValidationError("Patient not found with provided external ID and phone number")care/emr/api/viewsets/file_upload.py (2)
Line range hint
29-52: Consider refactoring authorization logic.The nested if-elif structure is getting a bit... cozy. Maybe we could split this into separate functions for each file type?
def _authorize_patient_file(user, patient_id, permission): patient_obj = get_object_or_404(Patient, external_id=patient_id) return AuthorizationController.call( f"can_{'view_clinical_data' if permission == 'read' else 'write_patient_obj'}", user, patient_obj ) def _authorize_encounter_file(user, encounter_id, permission): encounter_obj = get_object_or_404(Encounter, external_id=encounter_id) if permission == "read": return ( AuthorizationController.call("can_view_clinical_data", user, encounter_obj.patient) or AuthorizationController.call("can_view_encounter_obj", user, encounter_obj) ) return AuthorizationController.call("can_update_encounter_obj", user, encounter_obj)
Line range hint
128-146: Add transaction management to archive operation.Multiple field updates without transaction management? Living dangerously, I see.
+from django.db import transaction @action(detail=True, methods=["POST"]) def archive(self, request, *args, **kwargs): + with transaction.atomic(): obj = self.get_object() request_data = self.ArchiveRequestSpec(**request.data)care/emr/api/viewsets/scheduling/booking.py (1)
Line range hint
87-90: Consider adding response schema to the cancel action.I see you've added swagger schema decorators, but you might want to add response schemas too... you know, for completeness.
@action(detail=True, methods=["POST"]) +@extend_schema( + request=CancelBookingSpec, + responses={200: TokenBookingReadSpec, 403: None} +) def cancel(self, request, *args, **kwargs):care/emr/api/viewsets/patient.py (2)
87-89: Add error responses to the search schema.The schema could be more... complete with error responses.
@extend_schema( request=SearchRequestSpec, + responses={200: list[PatientPartialSpec], 400: None, 403: None} )
111-113: Consider adding validation error response to search_retrieve schema.The method can raise PermissionDenied, but it's not documented in the schema.
@extend_schema( request=SearchRetrieveRequestSpec, - responses={200: PatientRetrieveSpec} + responses={200: PatientRetrieveSpec, 403: None, 400: None} )care/emr/api/viewsets/condition.py (1)
62-62: Consider consolidating authorization logic.I see you've added swagger schemas, which is nice, but maybe we could also consider extracting the common authorization patterns into a shared mixin?
class ConditionAuthorizationMixin: def authorize_read(self): if not AuthorizationController.call( "can_view_clinical_data", self.request.user, self.get_patient_obj() ): raise PermissionDenied("Permission denied for patient data")Also applies to: 103-103, 145-145
care/emr/api/viewsets/facility_organization.py (1)
Line range hint
86-121: These permission checks could use some DRYing up.The repeated superuser checks and similar authorization patterns across methods suggest an opportunity for refactoring.
Consider extracting common authorization logic:
+ def _check_organization_permission(self, permission_name, user, organization): + if user.is_superuser: + return True + return AuthorizationController.call(permission_name, user, organization) + def authorize_destroy(self, instance): if instance.type == "root": raise PermissionDenied("Cannot delete root organization") if FacilityOrganization.objects.filter(parent=instance).exists(): raise PermissionDenied("Cannot delete organization with children") - if self.request.user.is_superuser: - return - if not AuthorizationController.call( - "can_delete_facility_organization", self.request.user, instance - ): + if not self._check_organization_permission( + "can_delete_facility_organization", self.request.user, instance + ): raise PermissionDenied( "User does not have the required permissions to update organization" )care/emr/api/viewsets/questionnaire.py (1)
Line range hint
187-207: Those schema classes seem a bit... lost.The
QuestionnaireTagsSetSchemaandQuestionnaireOrganizationUpdateSchemaclasses are defined inside the viewset. While it works, it's not exactly... elegant.Consider moving these to a dedicated schema module:
# care/emr/resources/questionnaire/schema.py from pydantic import BaseModel, UUID4 class QuestionnaireTagsSetSchema(BaseModel): tags: list[str] class QuestionnaireOrganizationUpdateSchema(BaseModel): organizations: list[UUID4]Then import and use them in the viewset. Your future self might thank you for this organization.
care/emr/api/viewsets/encounter.py (1)
Line range hint
286-291: Email validation could be more robust.While using Django's email validator is good, we could enhance the validation to be more comprehensive.
Consider adding length and domain validation:
@field_validator("email") @classmethod def validate_email(cls, value): + if len(value) > 254: # RFC 5321 + raise ValueError("Email too long") django_validate_email(value) + domain = value.split('@')[1] + if domain.lower() in ['example.com', 'test.com']: + raise ValueError("Invalid email domain") return valuecare/emr/api/viewsets/user.py (1)
103-121: Nice consolidation of profile picture handling.The method now handles both POST and DELETE operations in a clean way. Although, it might have been even nicer to add some logging to track profile picture changes.
Consider adding logging to track profile picture changes:
def profile_picture(self, request, *args, **kwargs): user = self.get_object() if not self.authorize_update({}, user): raise PermissionDenied("Permission Denied") if request.method == "POST": + logger.info(f"Uploading profile picture for user {user.username}") serializer = UserImageUploadSerializer(user, data=request.data) serializer.is_valid(raise_exception=True) serializer.save() + logger.info(f"Profile picture uploaded successfully for user {user.username}") return Response(status=200) if request.method == "DELETE": + logger.info(f"Deleting profile picture for user {user.username}") delete_cover_image(user.profile_picture_url, "avatars") user.profile_picture_url = None user.save() + logger.info(f"Profile picture deleted successfully for user {user.username}") return Response(status=204) return Response(data="Method Not Allowed", status=405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
care/emr/api/otp_viewsets/login.py(3 hunks)care/emr/api/otp_viewsets/patient.py(1 hunks)care/emr/api/otp_viewsets/slot.py(4 hunks)care/emr/api/viewsets/allergy_intolerance.py(1 hunks)care/emr/api/viewsets/base.py(3 hunks)care/emr/api/viewsets/batch_request.py(2 hunks)care/emr/api/viewsets/condition.py(4 hunks)care/emr/api/viewsets/encounter.py(6 hunks)care/emr/api/viewsets/facility.py(5 hunks)care/emr/api/viewsets/facility_organization.py(3 hunks)care/emr/api/viewsets/file_upload.py(5 hunks)care/emr/api/viewsets/medication_administration.py(2 hunks)care/emr/api/viewsets/medication_request.py(1 hunks)care/emr/api/viewsets/medication_statement.py(1 hunks)care/emr/api/viewsets/notes.py(2 hunks)care/emr/api/viewsets/observation.py(4 hunks)care/emr/api/viewsets/organization.py(4 hunks)care/emr/api/viewsets/patient.py(6 hunks)care/emr/api/viewsets/questionnaire.py(8 hunks)care/emr/api/viewsets/questionnaire_response.py(2 hunks)care/emr/api/viewsets/resource_request.py(3 hunks)care/emr/api/viewsets/roles.py(1 hunks)care/emr/api/viewsets/scheduling/availability.py(2 hunks)care/emr/api/viewsets/scheduling/availability_exceptions.py(1 hunks)care/emr/api/viewsets/scheduling/booking.py(2 hunks)care/emr/api/viewsets/scheduling/schedule.py(2 hunks)care/emr/api/viewsets/user.py(3 hunks)care/emr/api/viewsets/valueset.py(2 hunks)care/emr/migrations/0013_facilitylocation_facilitylocationorganization.py(1 hunks)care/utils/decorators/schema_decorator.py(1 hunks)care/utils/tests/test_swagger_schema.py(1 hunks)config/api_router.py(2 hunks)config/settings/base.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- care/emr/api/viewsets/notes.py
- care/emr/api/viewsets/organization.py
🚧 Files skipped from review as they are similar to previous changes (2)
- config/api_router.py
- care/emr/api/viewsets/base.py
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/migrations/0013_facilitylocation_facilitylocationorganization.py
13-13: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
13-13: 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)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: 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)
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)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: 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)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: 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)
31-31: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
32-32: 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)
34-34: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
35-35: 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)
37-37: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
38-38: 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)
40-40: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
41-41: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
42-42: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
42-42: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
43-43: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
43-43: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
44-44: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
44-44: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
46-46: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
46-46: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
49-49: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
53-53: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
55-55: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
55-55: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
56-56: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
57-57: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
58-58: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
59-59: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
60-60: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
61-61: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
62-62: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
62-62: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
63-63: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
63-63: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
64-64: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
64-64: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
65-65: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
65-65: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
68-68: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (24)
care/utils/tests/test_swagger_schema.py (1)
5-8: 🛠️ Refactor suggestionAuthentication setup seems to have slipped through the cracks.
The test might pass in development but fail in production where authentication might be required. We should probably handle that.
Let's verify if authentication is required:
Consider adding authentication setup:
class SwaggerSchemaTest(TestCase): + def setUp(self): + self.user = self.create_user() + self.client.force_authenticate(user=self.user) + def test_swagger_endpoint(self):care/emr/api/viewsets/roles.py (1)
Line range hint
1-1: That TODO comment has been sitting there for a while, hasn't it?It might be slightly more professional to either address the TODO or create a tracking issue for moving these APIs to the security section. Just saying.
Would you like me to help create a tracking issue for this task?
care/emr/api/viewsets/medication_statement.py (2)
15-15: Import added in a somewhat arbitrary position.The import could be grouped with other decorator imports... if there were any. I suppose this location works too.
22-22: LGTM! Adding Swagger schema generation.The decorator will help generate API documentation automatically. Nice of you to think about the API consumers.
care/emr/api/viewsets/medication_administration.py (1)
14-14: Oh look, another schema decorator addition.Following the same pattern as other viewsets. At least you're being consistent.
Also applies to: 24-24
care/emr/api/viewsets/valueset.py (1)
11-11: Adding schema generation to a security-sensitive viewset.Good to see proper documentation being added to a viewset with superuser-only write permissions. The schema will help make these restrictions clear to API consumers... assuming they read the docs.
Also applies to: 20-20
care/emr/api/viewsets/medication_request.py (1)
20-20: Adding schema generation to yet another viewset.The decorator will help document the complex authorization requirements. Though I wonder if anyone will actually read the generated documentation.
Also applies to: 27-27
care/emr/api/viewsets/questionnaire_response.py (1)
20-20: Another swagger schema decorator!I see we're being quite... consistent with our documentation efforts.
care/emr/api/viewsets/allergy_intolerance.py (1)
Line range hint
36-51: LGTM! Nice work on the authorization implementation.The viewset implementation is thorough with proper authorization checks and schema documentation.
care/emr/migrations/0013_facilitylocation_facilitylocationorganization.py (2)
63-64: Review CASCADE deletion behavior.Using CASCADE deletion for both location and organization might be a bit... adventurous. Have we considered what happens to historical records when either is deleted?
🧰 Tools
🪛 Ruff (0.8.2)
63-63: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
63-63: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
64-64: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
64-64: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-33: Consider increasing CharField max_lengths.Those 255-character limits on status, name, and description fields seem a bit... optimistic. I mean, I'm sure you've never seen a medical description longer than 255 characters, right?
🧰 Tools
🪛 Ruff (0.8.2)
29-29: 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)
31-31: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
32-32: 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)
care/emr/api/viewsets/file_upload.py (1)
Line range hint
62-71: LGTM! Well-structured viewset implementation.Nice work on the viewset structure with proper filtering and schema documentation.
care/emr/api/viewsets/scheduling/booking.py (1)
30-30: LGTM! Nice documentation enhancement.The addition of swagger schema decorator will improve API documentation.
Also applies to: 61-61
care/emr/api/viewsets/scheduling/schedule.py (1)
34-34: Oh look, more schema decorators!The addition of
@generate_swagger_schema_decoratorto both viewsets is a good step towards better API documentation. How thoughtful of you to remember this time.Also applies to: 134-134
care/emr/api/viewsets/facility_organization.py (1)
32-32: More swagger decorators, how... consistent.The addition of schema decorators to both viewsets maintains documentation consistency. Though it might have been nice to see some actual API documentation in the docstrings too.
Also applies to: 155-155
care/emr/api/viewsets/questionnaire.py (1)
41-41: Swagger decorators strike again!The schema decorators are properly applied. At least we're being consistent with our documentation approach.
Also applies to: 74-74
care/emr/api/viewsets/encounter.py (2)
82-82: Another day, another swagger decorator.The schema decorator addition maintains consistency with other viewsets. How... predictable.
225-225: Nice to see proper status code constants being used.The change from
status=204tostatus=status.HTTP_204_NO_CONTENTimproves code clarity. Though it's interesting this wasn't done consistently from the start.care/emr/api/viewsets/scheduling/availability.py (1)
130-130: Nice addition of Swagger schema decorator.The decorator will help generate API documentation, making it easier for other developers to understand and use the API... assuming they read the documentation.
config/settings/base.py (1)
390-390: Consider the implications of disabling schema errors and warnings.While disabling schema errors and warnings might help with deployment, it could mask potential issues that should be addressed. It would be lovely if we could keep the warnings enabled and fix the underlying issues instead.
Run the following script to check for any schema-related issues that might be hidden:
✅ Verification successful
Schema warnings can safely remain disabled
After reviewing the schema implementation, there's no evidence of underlying issues that these warnings might be hiding. The codebase uses drf-spectacular in a clean, standard way with proper schema decorators and type hints. While you're obviously welcome to enable the warnings if you'd like to see them, they're likely just noise rather than signal in this case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for schema-related issues that might be hidden. # Test: Generate schema with warnings enabled # Temporarily set DISABLE_ERRORS_AND_WARNINGS to False sed -i.bak 's/DISABLE_ERRORS_AND_WARNINGS": True/DISABLE_ERRORS_AND_WARNINGS": False/' config/settings/base.py # Run schema generation and capture warnings python manage.py spectacular --file /dev/null 2>&1 | grep -i "warning\|error" # Restore the original setting mv config/settings/base.py.bak config/settings/base.pyLength of output: 496
Script:
#!/bin/bash # Search for documentation and context about DISABLE_ERRORS_AND_WARNINGS echo "=== Searching for documentation about the setting ===" rg -i "DISABLE_ERRORS_AND_WARNINGS" -B2 -A2 echo -e "\n=== Searching for schema-related warnings in comments ===" rg -i "schema.*warning|warning.*schema" -g "!*.md" echo -e "\n=== Checking for similar settings in other config files ===" fd "settings" -e py -x cat {} | grep -i "disable.*warning\|warning.*disable" echo -e "\n=== Looking for schema generation related files ===" fd "schema" -e py -e yaml -e jsonLength of output: 1102
Script:
#!/bin/bash # Examine schema-related files and look for potential issues echo "=== Checking schema test file ===" cat care/utils/tests/test_swagger_schema.py echo -e "\n=== Checking schema decorator ===" cat care/utils/decorators/schema_decorator.py echo -e "\n=== Looking for TODOs/FIXMEs related to schema ===" rg -i "todo|fixme|xxx|hack" care/utils/assetintegration/schema.py care/utils/schema.py echo -e "\n=== Checking for validation-related code ===" rg -i "validat|error|warning" care/utils/schema.pyLength of output: 2253
care/emr/api/viewsets/resource_request.py (3)
26-33: Well-structured filter implementation.The filter class provides a clean way to filter resource requests by various facility and patient identifiers. The use of UUIDFilter with external_id is consistent with the codebase's practices.
35-35: Good enhancement of the ResourceRequestViewSet.The addition of the swagger schema decorator and filter settings improves both documentation and filtering capabilities. I'm sure the frontend team will be thrilled to discover these new filtering options.
Also applies to: 41-42
90-90: Consistent documentation improvement.The swagger schema decorator addition maintains consistency with other viewsets in the codebase.
care/emr/api/viewsets/user.py (1)
38-38: Consistent documentation improvement.The swagger schema decorator addition aligns with the documentation improvements across the codebase.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2782 +/- ##
===========================================
- Coverage 64.71% 56.16% -8.56%
===========================================
Files 252 215 -37
Lines 12732 10191 -2541
Branches 1119 1030 -89
===========================================
- Hits 8240 5724 -2516
- Misses 4384 4451 +67
+ Partials 108 16 -92 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
care/emr/models/location.py (1)
145-153:⚠️ Potential issueThe recursive implementation needs safeguards.
The recursive implementation lacks cycle detection and depth limits, which could lead to infinite loops or stack overflow.
@app.task def handle_cascade(base_location, depth=0, max_depth=10, visited=None): """ Cascade changes to a location organization to all its children with safeguards against cycles and excessive recursion. """ if visited is None: visited = set() if depth >= max_depth or base_location in visited: return visited.add(base_location) for child in FacilityLocation.objects.filter(parent_location_id=base_location): child.save(update_fields=["cached_parent_json"]) handle_cascade(child.id, depth + 1, max_depth, visited)
🧹 Nitpick comments (5)
care/emr/api/viewsets/location.py (2)
118-131: Reassure data consistency or rename "encounter_organizations" to avoid confusion.
Naming the variableencounter_organizationsmight be slightly misleading, since it stores organizations related to a location. A more descriptive name could prevent confusion for future maintainers.-encounter_organizations = FacilityLocationOrganization.objects.filter( - location=instance -).select_related("organization") +location_organizations = FacilityLocationOrganization.objects.filter( + location=instance +).select_related("organization")
215-221: Address the pending TODO items.
A fair number of unimplemented items (like concurrency checks, occupancy checks, etc.) remain. Consider resolving them or tracking them in a dedicated work item so they don't remain overlooked.care/emr/migrations/0014_encounter_current_location_facilitylocationencounter.py (1)
17-43: Use consistent quotes to match project style guidelines.
A handful of lines in this migration use single quotes instead of double quotes. Although this is a minor stylistic preference, adopting a consistent style can improve readability.-('emr', '0013_facilitylocation_facilitylocationorganization'), +("emr", "0013_facilitylocation_facilitylocationorganization"),(... apply similar changes for all lines flagged by static analysis...)
🧰 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)
20-20: 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)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: 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)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: 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)
31-31: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
32-32: 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)
34-34: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
34-34: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
35-35: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
35-35: 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)
36-36: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
37-37: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
37-37: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
40-40: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/emr/resources/location/spec.py (2)
76-84: A database query in the validator? How... interesting.The validator performs a database query on every validation. Consider caching the results or moving the validation to a pre-save hook for better performance, especially when handling bulk operations.
@classmethod def _cache_parent_locations(cls): if not hasattr(cls, '_parent_cache'): cls._parent_cache = set(FacilityLocation.objects.values_list('external_id', flat=True)) return cls._parent_cache @model_validator(mode="after") def validate_parent_organization(self): if self.parent and self.parent not in self._cache_parent_locations(): raise ValueError("Parent not found") return self
94-96: Theparentfield could use some type hinting love.The
parentfield is defined as a plaindictwithout type hints. Consider using a TypedDict or Pydantic model to ensure type safety and document the expected structure.from typing import TypedDict class ParentLocation(TypedDict): id: str name: str # ... other expected fields class FacilityLocationListSpec(FacilityLocationSpec): parent: ParentLocation mode: str has_children: bool
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
care/emr/api/viewsets/location.py(1 hunks)care/emr/migrations/0014_encounter_current_location_facilitylocationencounter.py(1 hunks)care/emr/models/encounter.py(1 hunks)care/emr/models/location.py(1 hunks)care/emr/resources/base.py(1 hunks)care/emr/resources/location/spec.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/migrations/0014_encounter_current_location_facilitylocationencounter.py
12-12: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
12-12: 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)
20-20: 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)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: 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)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: 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)
31-31: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
32-32: 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)
34-34: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
34-34: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
35-35: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
35-35: 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)
36-36: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
37-37: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
37-37: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
40-40: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
care/emr/api/viewsets/location.py (3)
54-59: Consider validating patient/location associations before destruction.
You have aTODOcomment to check if any active patient associations exist. Failing to do so might lead to dangling references or orphaned data.Would you like me to generate a script or open a new issue to ensure that any associated patient data is handled appropriately?
166-166: Reinstate the authorization call if needed.
It appears the authorization checkself.authorize_update({}, instance)was intentionally commented out. Verify whether you need this call to enforce update permissions on the location before removing organizations.
163-184:⚠️ Potential issueCritical mismatch in organizations_remove action.
You're filtering withencounter=instanceinstead oflocation=instance. This prevents correct organization removal from the location and is likely an unintentional bug.-encounter_organization = FacilityLocationOrganization.objects.filter( - encounter=instance, organization=organization -) +location_organization = FacilityLocationOrganization.objects.filter( + location=instance, organization=organization +) - if not encounter_organization.exists(): - raise ValidationError("Organization does not exist") - FacilityLocationOrganization.objects.filter( - encounter=instance, organization=organization - ).delete() + if not location_organization.exists(): + raise ValidationError("Organization does not exist") + location_organization.delete()Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
care/templates/base.html (1)
Line range hint
41-41: Someone forgot to clean up after removing Alpine.js.The template still uses Alpine.js data attributes (
x-data="{open: true}") but the library itself has been removed. This will result in non-functional navigation components.Either:
- Restore the Alpine.js library:
+ <script src="https://unpkg.com/[email protected]/dist/cdn.min.js"></script>Or:
2. Remove the Alpine.js attributes:- <nav x-data="{open: true}" class="bg-white shadow" role="navigation"> + <nav class="bg-white shadow" role="navigation">
🧹 Nitpick comments (6)
care/templates/pages/home.html (1)
9-9: Clean up redundant text classes.The paragraph element contains both
text-justifyandtext-centerclasses, which are contradictory. Also, considering thejustify-content: space-aroundstyle, text justification might not be necessary.- <p class="flex text-lg mt-1 text-gray-700 text-justify text-center" style="justify-content: space-around"> + <p class="flex text-lg mt-1 text-gray-700 text-center" style="justify-content: space-around">care/templates/base.html (2)
Line range hint
42-48: I couldn't help but notice some accessibility oversights.The navigation and logo could be more accessible to screen readers. Here are some suggestions:
- <nav class="bg-white shadow" role="navigation"> + <nav class="bg-white shadow" role="navigation" aria-label="Main"> <div class="container mx-auto p-4 flex flex-wrap items-center md:flex-no-wrap"> <div class="mr-4 md:mr-8"> - <a class="navbar-brand" href="{% url 'home' %}"> - <img class="block w-24 h-auto" src="{% static 'images/logos/black-logo.svg' %}" alt="" /> + <a class="navbar-brand" href="{% url 'home' %}" aria-label="Home"> + <img class="block w-24 h-auto" src="{% static 'images/logos/black-logo.svg' %}" alt="Open Healthcare Network Logo" /> </a> </div> </div>
Line range hint
77-77: I see we're loading reCAPTCHA on every single page.The reCAPTCHA script is loaded unconditionally, which impacts page load performance on pages that don't need it. Consider moving it to a separate block that can be included only on forms that require CAPTCHA verification.
- <script src='https://www.google.com/recaptcha/api.js'></script> + {% block recaptcha %}{% endblock recaptcha %}Then in templates that need reCAPTCHA:
{% block recaptcha %} <script src='https://www.google.com/recaptcha/api.js'></script> {% endblock %}README.md (1)
36-36: I see you're using a bare URL there...While the URL update is correct, it would be slightly more proper to use markdown link syntax instead of a bare URL. You know, for consistency and all that good stuff.
Here's a tiny improvement you might consider:
-You can find the docs at https://care-be-docs.ohc.network +You can find the docs at [https://care-be-docs.ohc.network](https://care-be-docs.ohc.network)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
36-36: Bare URL used
null(MD034, no-bare-urls)
care/contrib/sites/migrations/0003_set_site_domain_and_name.py (2)
16-19: Consider adding domain format validation.I mean, I'm sure you've triple-checked this, but maybe we could add some basic domain format validation? Just to be extra safe... if you're into that sort of thing.
Here's a slightly more robust version:
def update_site_forward(apps, schema_editor): """Set site domain and name.""" Site = apps.get_model("sites", "Site") + domain = "ohc.network" + if not all(part.isalnum() or part in '-.' for part in domain.split('.')): + raise ValueError(f"Invalid domain format: {domain}") Site.objects.update_or_create( id=settings.SITE_ID, defaults={ - "domain": "ohc.network", + "domain": domain, "name": "Care", }, )
Line range hint
1-40: Document the migration's purpose and impact.Not to be that reviewer, but it might be helpful to update the docstring to explain why we're changing the domain. You know, for those future developers who might wonder about this seemingly random domain change.
""" To understand why this file is here, please read: http://cookiecutter-django.readthedocs.io/en/latest/faq.html#why-is-there-a-django-contrib-sites-directory-in-cookiecutter-django + +This migration updates the site domain from coronasafe.in to ohc.network as part of +the organization's rebranding effort. This change affects the base domain used +throughout the application. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)care/contrib/sites/migrations/0003_set_site_domain_and_name.py(1 hunks)care/templates/base.html(1 hunks)care/templates/pages/home.html(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
36-36: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (5)
care/templates/pages/home.html (2)
10-10: Verify if the new message aligns with PR objectives.The message has been changed to a rather generic "Better Healthcare tools for everyone!" while the PR seems to focus specifically on location, device features, and filters. Perhaps we could make it more... relevant to the actual changes being introduced?
Would you like to consider a message that better reflects the new location and device management capabilities being added?
7-11: The layout structure looks good, I suppose.The flex container with column direction and centered content provides a clean, modern look. The spacing and maximum width constraints are appropriately set.
care/templates/base.html (1)
8-8: Oh, look who decided to make the title more descriptive!The title change from "Care" to "Open HealthCare Network | Care" is actually quite nice. It improves SEO and better represents the brand identity.
README.md (1)
36-36: Verify domain transition consistency.I notice you've updated the docs URL to use
ohc.network. Let's make sure we haven't missed any other URLs that need updating.✅ Verification successful
Domain transition appears impeccably complete
The codebase has been thoroughly checked for any remaining references to 'coronasafe.network', and none were found. I suppose someone did their homework rather well this time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining coronasafe.network URLs rg "coronasafe\.network" --type mdLength of output: 36
Script:
#!/bin/bash # Broader search for any remaining references to the old domain rg -i "coronasafe\.network" # Search with protocol variations rg -i "https?://[^/]*coronasafe\.network" # Search for any URLs containing 'coronasafe' rg -i "https?://[^/]*coronasafe[^/\s]*"Length of output: 115
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
36-36: Bare URL used
null(MD034, no-bare-urls)
care/contrib/sites/migrations/0003_set_site_domain_and_name.py (1)
17-17: Verify domain-dependent configurations across the codebase.While changing the domain to "ohc.network" is probably fine, it would be nice if we could ensure this won't break anything... you know, like emails, URLs, or authentication settings.
✅ Verification successful
Domain change appears safe, though I had to check thoroughly...
The migration can proceed safely as:
- No hardcoded references to the old domain exist
- All domain-dependent features use environment variables
- Password reset and email functionalities are properly configured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential hardcoded references to the old domain echo "Searching for potential references to the old domain..." rg -i "coronasafe\.in" # Check for domain-related settings in Django configuration echo "Checking Django settings files..." fd -e py -e ini -e yaml -e json settings | xargs rg -i "domain|site_id|site_name|allowed_hosts"Length of output: 18881
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
care/emr/resources/location/spec.py (1)
63-63: A TODO comment that's actually useful! How refreshing.The comment indicates a need for validation of patient associations before deletion.
Would you like me to help implement the patient association validation logic? I can generate a solution that checks for active patient associations before allowing deletion.
care/emr/api/viewsets/location.py (2)
312-318: Those parentheses could use some... organization.The condition's readability could be improved by properly grouping the expressions.
Improve the readability of the condition:
- if ( - not model_obj - or model_obj - and model_obj.end_datetime - and not instance.end_datetime - ) and base_qs.filter(end_datetime__isnull=True).exists(): + if ( + (not model_obj) or + (model_obj and model_obj.end_datetime and not instance.end_datetime) + ) and base_qs.filter(end_datetime__isnull=True).exists():🧰 Tools
🪛 Ruff (0.8.2)
314-316: Parenthesize
a and bexpressions when chainingandandortogether, to make the precedence clearParenthesize the
andsubexpression(RUF021)
209-209: A TODO comment about unbuilt association models? How... forward-thinking.The comment indicates that association models are yet to be implemented.
Would you like me to help design and implement the association models for the encounter-location relationship? I can provide a solution that follows the existing patterns in the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/api/viewsets/location.py(1 hunks)care/emr/resources/location/spec.py(1 hunks)care/templates/base.html(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/api/viewsets/location.py
314-316: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (4)
care/templates/base.html (1)
8-8: LGTM! Though I wonder why we're being so secretive...The title is now more descriptive and the robots meta tag prevents search engine indexing. Both changes look good, although I'm curious about the decision to block search engines.
Also applies to: 12-12
care/emr/resources/location/spec.py (2)
19-26: Those mysterious single-letter status codes are back again...The single-letter status codes (
C,H,O,U,K,I) lack documentation explaining their meaning. This could lead to confusion for future developers.Consider adding class-level documentation:
class FacilityLocationStatusChoices(str, Enum): + """Facility location status codes: + C - Closed + H - On Hold + O - Operational + U - Under Construction + K - Known + I - Inactive + """ C = "C" H = "H" O = "O" # noqa E741 U = "U" K = "K" I = "I" # noqa E741
Line range hint
300-303: Documentation that explains the 'why' instead of the obvious 'what'? I'm impressed!The docstring effectively explains why the method needs to be called separately for maintaining locks.
care/emr/api/viewsets/location.py (1)
178-185:⚠️ Potential issueOh look, another encounter vs. location mix-up!
The query is using
encounter=instancewhen it should be usinglocation=instance.Fix the incorrect field name in the query:
- encounter_organization = FacilityLocationOrganization.objects.filter( - encounter=instance, organization=organization - ) + location_organization = FacilityLocationOrganization.objects.filter( + location=instance, organization=organization + ) - FacilityLocationOrganization.objects.filter( - encounter=instance, organization=organization - ).delete() + FacilityLocationOrganization.objects.filter( + location=instance, organization=organization + ).delete()Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
care/emr/migrations/0015_facilitylocation_availability_status_and_more.py (1)
14-24: Consider Converting Single Quotes to Double Quotes
Static analysis tools (Ruff Q000) flag your single quotes. You may switch them to double quotes for consistency if style guidelines require it.- field=models.CharField(default='', max_length=25), + field=models.CharField(default="", max_length=25),🧰 Tools
🪛 Ruff (0.8.2)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: 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)
care/emr/resources/location/spec.py (4)
13-18: Suggest Documenting Each Status Value
Providing a quick comment or docstring for each of these statuses (e.g., “planned,” “active,” etc.) can help future readers. If that feels like too much overhead, feel free to ignore.
20-23: Recommend Clarifying Availability
Likewise, a short note on each availability type (like “unavailable” meaning location is offline?) may help new contributors.
83-99: Enhance Error Messaging
The error message"Parent not found"is quite general. Providing more context—for example, the ID that was not found—helps debug issues quicker.- err = "Parent not found" + err = f"Parent with external_id={self.parent} not found"
100-107: Default Availability
Settingobj.availability_statusto"available"unconditionally might overlook certain real-world scenarios (e.g., an inactive or under-construction location). If that’s intentional, consider documenting the assumption.care/emr/api/viewsets/location.py (2)
323-385: The validation looks almost perfect... almost.The method should also validate that there are no overlapping active encounters during the specified time period.
Add this validation before line 362:
if end_datetime: if base_qs.filter( start_datetime__lte=end_datetime, end_datetime__gte=start_datetime ).exists(): raise ValidationError("Conflict in schedule") + # Check for active encounters during this period + if base_qs.filter( + status=LocationEncounterAvailabilityStatusChoices.active.value, + start_datetime__lte=end_datetime, + end_datetime__isnull=True + ).exists(): + raise ValidationError("Active encounter exists during this period") elif base_qs.filter(start_datetime__gte=start_datetime).exists():
211-211: That TODO is looking a bit lonely.The comment suggests that association models are missing, which could affect the functionality of the
associate_encounteraction.Would you like me to help design and implement the association models? I can provide a complete implementation that aligns with the existing architecture.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
care/emr/api/viewsets/location.py(1 hunks)care/emr/migrations/0015_facilitylocation_availability_status_and_more.py(1 hunks)care/emr/models/location.py(1 hunks)care/emr/resources/location/spec.py(1 hunks)care/users/models.py(1 hunks)config/api_router.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/api_router.py
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/migrations/0015_facilitylocation_availability_status_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)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (9)
care/users/models.py (2)
133-133: Potential N+1 Query Issue
Removingselect_relatedcalls might cause multiple hits to the database whenever related fields are accessed. By relying solely on a filtered queryset, you could see performance degradation if those related fields are accessed frequently.
136-136: Possible Performance Degradation
Similarly, removingselect_relatedhere may lead to performance issues when retrieving rows that join on related tables.care/emr/resources/location/spec.py (3)
31-38: Add Documentation for Single-Letter Codes
Single-letter codes likeC,H,O,U,K,Ican be unclear. Please consider adding a docstring or comment explaining each code in plain words.
45-60: Document Two-Letter Codes
These form codes (si,bu, etc.) can confuse new devs. A simple inline comment explaining each code drastically improves readability.
146-152: Use a More Specialized Exception
Raising a plainValueErrormight not reflect the severity of a missingEncounter. If you’d like to signal a missing resource, consider a custom exception or aDoesNotExistvariant.care/emr/models/location.py (3)
74-76:⚠️ Potential issueSomeone seems to have had a naming identity crisis.
The filter uses
root_orgbut the model field is namedroot_location.Apply this fix:
- queryset = queryset.filter(root_org=root_location) + queryset = queryset.filter(root_location=root_location) - queryset = queryset.filter(root_org__isnull=True) + queryset = queryset.filter(root_location__isnull=True)Likely invalid or redundant comment.
37-54:⚠️ Potential issueThat recursive call looks... interesting.
The
get_parent_jsonmethod has some concerning issues:
- Line 45: The recursive call result is ignored, which could lead to infinite recursion
- Missing error handling for datetime parsing
- No cycle detection to prevent infinite loops in parent hierarchy
Consider this safer implementation:
def get_parent_json(self): from care.emr.resources.location.spec import FacilityLocationListSpec if self.parent_id: - if self.cached_parent_json and timezone.now() < datetime.fromisoformat( - self.cached_parent_json["cache_expiry"] - ): - return self.cached_parent_json - self.parent.get_parent_json() - temp_data = FacilityLocationListSpec.serialize(self.parent).to_json() - temp_data["cache_expiry"] = str( - timezone.now() + timedelta(days=self.cache_expiry_days) - ) - self.cached_parent_json = temp_data - super().save(update_fields=["cached_parent_json"]) - return self.cached_parent_json + try: + if self.cached_parent_json and timezone.now() < datetime.fromisoformat( + self.cached_parent_json["cache_expiry"] + ): + return self.cached_parent_json + + # Get parent's json first to ensure the entire chain is valid + parent_json = self.parent.get_parent_json() + + temp_data = FacilityLocationListSpec.serialize(self.parent).to_json() + temp_data["cache_expiry"] = str( + timezone.now() + timedelta(days=self.cache_expiry_days) + ) + self.cached_parent_json = temp_data + super().save(update_fields=["cached_parent_json"]) + return self.cached_parent_json + except (KeyError, ValueError, RecursionError) as e: + # Handle invalid cache or recursion + return {} return {}Likely invalid or redundant comment.
149-157:⚠️ Potential issueThis cascade implementation is living dangerously.
The recursive implementation lacks:
- Cycle detection to prevent infinite loops
- Depth limits to prevent stack overflow
- Uses incorrect field name
parent_location_idinstead ofparent_idConsider this safer implementation:
+MAX_DEPTH = 100 # Adjust based on your needs + @app.task -def handle_cascade(base_location): +def handle_cascade(base_location, depth=0, visited=None): """ Cascade changes to a location organization to all its children """ + if visited is None: + visited = set() + + if depth > MAX_DEPTH: + raise ValueError("Maximum recursion depth exceeded") + + if base_location in visited: + raise ValueError("Cycle detected in location hierarchy") + + visited.add(base_location) - for child in FacilityLocation.objects.filter(parent_location_id=base_location): + for child in FacilityLocation.objects.filter(parent_id=base_location): child.save(update_fields=["cached_parent_json"]) - handle_cascade(child) + handle_cascade(child, depth + 1, visited)Likely invalid or redundant comment.
care/emr/api/viewsets/location.py (1)
170-191:⚠️ Potential issueThat commented authorization check is making me nervous.
Multiple issues in this method:
- Line 173: Authorization check is commented out
- Lines 181-187: Using
encounterinstead oflocationin the filterApply these fixes:
@action(detail=True, methods=["POST"]) def organizations_remove(self, request, *args, **kwargs): instance = self.get_object() - # self.authorize_update({}, instance) + self.authorize_update({}, instance) request_data = self.FacilityLocationOrganizationManageSpec(**request.data) organization = get_object_or_404( FacilityOrganization, external_id=request_data.organization ) self.authorize_update({}, instance) self.authorize_organization(instance.facility, organization) encounter_organization = FacilityLocationOrganization.objects.filter( - encounter=instance, organization=organization + location=instance, organization=organization ) if not encounter_organization.exists(): raise ValidationError("Organization does not exist") FacilityLocationOrganization.objects.filter( - encounter=instance, organization=organization + location=instance, organization=organization ).delete()Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
care/emr/resources/location/spec.py (4)
63-68: Would it kill you to add a docstring?The base spec class lacks documentation explaining its purpose, usage, and the significance of excluded fields.
Add a docstring:
class FacilityLocationBaseSpec(EMRResource): + """Base specification for facility locations. + + Attributes: + id: Optional UUID4 identifier + + Note: + Excludes 'parent', 'facility', and 'organizations' fields from serialization + """ __model__ = FacilityLocation __exclude__ = ["parent", "facility", "organizations"]
88-99: That error message could be a tiny bit more helpful...The error message "Parent not found" doesn't provide enough context about what went wrong.
- err = "Parent not found" + err = f"Parent facility location with ID {self.parent} not found or is not an instance"
146-152: Another minimalist error message, I see...The error message in validate_encounter could be more descriptive.
- err = "Encounter not found" + err = f"Encounter with ID {self.encounter} not found in the system"
114-118: Type hints for return values are mysteriously absent.The
perform_extra_serializationmethods are missing return type hints.- def perform_extra_serialization(cls, mapping, obj): + def perform_extra_serialization(cls, mapping: dict, obj: FacilityLocation) -> None:Also applies to: 124-131, 173-176
care/security/authorization/encounter.py (1)
25-28: Perhaps we could reduce this rather obvious code duplication?The same organization list construction pattern appears in three different methods. It would be slightly more maintainable if we extracted this into a helper method.
Consider adding a private helper method:
def _get_combined_orgs(self, encounter): orgs = [*encounter.facility_organization_cache] if encounter.current_location and encounter.current_location.facility_organization_cache: orgs.extend(encounter.current_location.facility_organization_cache) return orgsAlso, it might be prudent to add a None check for
facility_organization_cachewhile we're at it. You know, just in case.Also applies to: 43-46, 60-62
care/security/authorization/patient.py (2)
28-31: Consider adding null checks and error handling.The code assumes
encounter[0]andencounter[1]will always contain valid data. While I'm sure you've thought this through, it might be worth adding some defensive programming:- encounter_set = encounter_set.union(set(encounter[0])) - # Through Location - if encounter[1]: - encounter_set = encounter_set.union(set(encounter[1])) + if encounter[0] is not None: + encounter_set = encounter_set.union(set(encounter[0])) + # Through Location + if encounter[1] is not None: + encounter_set = encounter_set.union(set(encounter[1]))
21-31: Consider optimizing the database query performance.The current implementation makes a database query and then performs multiple set operations in Python. While I'm confident the current approach works fine, we could potentially optimize this by:
- Using database-level operations to combine the organization caches
- Reducing the number of set operations
encounters = ( Encounter.objects.filter(patient=patient) .exclude(status__in=COMPLETED_CHOICES) - .values_list( - "facility_organization_cache", - "current_location__facility_organization_cache", - ) + .annotate( + combined_orgs=Coalesce( + 'facility_organization_cache', + 'current_location__facility_organization_cache', + output_field=ArrayField(IntegerField()) + ) + ) + .values_list('combined_orgs', flat=True) ) - encounter_set = set() - for encounter in encounters: - encounter_set = encounter_set.union(set(encounter[0])) - # Through Location - if encounter[1]: - encounter_set = encounter_set.union(set(encounter[1])) + encounter_set = set().union(*[set(org_ids) for org_ids in encounters if org_ids])care/emr/api/viewsets/encounter.py (1)
129-136: Oh good, someone thought about database performanceThe addition of
current_location,created_by, andupdated_bytoselect_relatedwill help prevent those pesky N+1 queries. Your database will thank you for this consideration.Consider adding an index on these frequently accessed foreign key fields if not already present:
CREATE INDEX IF NOT EXISTS idx_encounter_current_location ON encounter(current_location_id); CREATE INDEX IF NOT EXISTS idx_encounter_created_by ON encounter(created_by_id); CREATE INDEX IF NOT EXISTS idx_encounter_updated_by ON encounter(updated_by_id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
care/emr/api/viewsets/encounter.py(7 hunks)care/emr/resources/location/spec.py(1 hunks)care/security/authorization/encounter.py(5 hunks)care/security/authorization/patient.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (12)
care/emr/resources/location/spec.py (4)
1-11: LGTM! Clean and well-organized imports.The imports are logically grouped and all are being used in the codebase.
31-38: Those mysterious single-letter status codes are still undocumented...The single-letter codes (
C,H,O,U,K,I) lack documentation explaining their meaning.
45-61: Another set of undocumented codes? How... unsurprising.The
FacilityLocationFormChoicesenum uses two-letter codes without explanation.
13-29: Well-documented status choices, finally!The status enums
LocationEncounterAvailabilityStatusChoices,LocationAvailabilityStatusChoices, andStatusChoicesuse clear, self-documenting values. This is how it should be done.care/security/authorization/encounter.py (2)
1-1: LGTM! Nice addition of the Q import.The Q import is properly placed and effectively utilized for complex query construction later in the code.
25-28: Verify the security implications of expanded access.The changes broaden access by including
current_locationorganizations in permission checks. While this might be intentional, it's worth confirming that:
- This doesn't accidentally grant more access than intended
- The
current_locationrelationship is properly validated elsewhereLet's check for access validation:
Also applies to: 43-46, 60-62, 80-83
✅ Verification successful
Actually, this is a well-implemented security enhancement
The changes are intentional and properly implemented with appropriate safeguards:
- Permission checks consistently combine organization caches from both the encounter and its location
- Location references use SET_NULL to prevent dangling permissions
- Query filters properly check organization overlap for both caches
I suppose I could nitpick about the cache update ordering, but the implementation is quite thorough.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for current_location validation rg -B 5 "current_location.*save" care/emr/ # Check for any existing access restrictions on current_location rg "current_location.*restrict|restrict.*current_location" care/emr/Length of output: 115
Script:
#!/bin/bash # Check encounter model definition and current_location field ast-grep --pattern 'class Encounter: $$$ current_location = $_ $$$' # Look for security checks in authorization files rg -A 5 "current_location" care/security/ # Check facility organization cache implementation rg -A 5 "facility_organization_cache" care/Length of output: 11561
care/security/authorization/patient.py (1)
21-24: Verify the security implications of expanded role collection.The change to include
current_location__facility_organization_cachebroadens the scope of role collection. While this might be necessary for the new location features, it's probably worth double-checking that this doesn't accidentally grant more access than intended. You know, just to be safe... 😊✅ Verification successful
The change appears to be perfectly secure 🔒
The addition of
current_location__facility_organization_cachefollows the existing authorization patterns and is properly scoped to active encounters only. I mean, I suppose you could add some tests if you're feeling particularly ambitious, but the implementation itself is quite solid.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing security tests or documentation related to role collection rg -g '!*.pyc' -g '!*venv*' -A 5 "test.*find_roles_on_patient|test.*PatientAccess" # Look for any security-related comments or documentation rg -g '!*.pyc' -g '!*venv*' "security.*patient.*access|role.*collection.*security"Length of output: 169
Script:
#!/bin/bash # Get the full implementation of find_roles_on_patient ast-grep --pattern 'def find_roles_on_patient($$$)' # Look for tests with different naming patterns rg -g '*/tests/*' -g '!*.pyc' -A 5 "facility_organization_cache" # Find security-related files using facility_organization_cache rg -g '*/security/*' -g '!*.pyc' "facility_organization_cache" # Look for any access control tests rg -g '*/tests/*' -g '!*.pyc' "test.*access.*patient|test.*patient.*access"Length of output: 557
Script:
#!/bin/bash # First, find our file fd "patient.py" # Search for the implementation without path restrictions rg -g '!*.pyc' "def find_roles_on_patient" -A 10 # Look for tests without path restrictions rg -g '!*.pyc' "facility_organization_cache.*test|test.*facility_organization_cache" # Search for access control related code rg -g '!*.pyc' "PatientAccess|patient.*access.*role|role.*patient.*access"Length of output: 1530
Script:
#!/bin/bash # Get more context about the authorization system rg -g '!*.pyc' "class AuthorizationHandler|class AuthorizationController" -A 5 # Check for any role-related configurations rg -g '!*.pyc' "ROLE_.*PATIENT|PATIENT.*ROLE" -A 2Length of output: 969
care/emr/api/viewsets/encounter.py (5)
50-50: Finally, some proper API documentation!The addition of the swagger schema decorator is a welcome improvement. It's always nice when developers remember that others need to understand their APIs too.
Also applies to: 83-83
79-79: Better late than never: Location filtering addedThe addition of location filtering aligns well with the existing filter patterns. I suppose we can now properly filter encounters by location.
188-191: Documentation improvements that should have been there from the startThe addition of
@extend_schemadecorators to these methods will make the API much more discoverable. It's refreshing to see proper request/response specifications being added.Also applies to: 212-214, 304-306
233-233: Using proper status code constants instead of magic numbersNice to see the switch from
status=204tostatus=status.HTTP_204_NO_CONTENT. It's always better to use constants that explain themselves, isn't it?
Line range hint
307-345: Might want to add rate limiting to the email endpointWhile the email validation is properly implemented, it would be prudent to add rate limiting to prevent potential abuse of the email discharge summary endpoint. I'm sure you were planning to add this eventually.
Consider implementing rate limiting using Django REST framework's throttling:
from rest_framework.throttling import UserRateThrottle class EmailThrottle(UserRateThrottle): rate = '10/hour' # Adjust as needed @action(detail=True, methods=["POST"], throttle_classes=[EmailThrottle]) def email_discharge_summary(self, request, *args, **kwargs): # ... existing code ...✅ Verification successful
Rate limiting is indeed missing and should be implemented
After checking the codebase, I can confirm that there's no rate limiting implemented for any email endpoints. In fact, the only throttle-related code I found actually disables throttling (which is... interesting). The original suggestion to implement rate limiting using DRF's throttling mechanism remains valid and would help prevent potential abuse of the email discharge summary endpoint.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for throttle-related configurations and implementations echo "=== Searching for throttle configurations ===" rg -i "throttle" --type python -g "!*test*" -g "!*mock*" echo -e "\n=== Looking for similar email endpoints ===" ast-grep --pattern 'def email_$_($$$)' echo -e "\n=== Checking settings files for rate limiting ===" fd "settings.*\.py$" --exec cat {}Length of output: 486
Script:
#!/bin/bash # Search for throttle-related configurations with correct extension filter echo "=== Searching for throttle configurations ===" rg -i "throttle" -g "*.py" -g "!*test*" -g "!*mock*" -A 2 -B 2 echo -e "\n=== Looking for DRF throttling imports and configs ===" rg "from rest_framework.throttling" -g "*.py" -g "!*test*" -g "!*mock*" echo -e "\n=== Finding email-related views and endpoints ===" rg "def email_" -g "*.py" -g "!*test*" -g "!*mock*" -A 5Length of output: 2318
updated swagger schema (#2789)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
care/utils/swagger/filter.py (3)
1-7: Imports seem appropriate, but consider minimizing potential performance hits.
The imports fromdrf_spectacularanddjango.dbare fine. However, ensure thatdjango.db.modelsis used sparingly so as not to force unneeded database loads.
8-11: Class definition is concise but missing docstring.
It’s great to see a custom filter extension. A brief docstring describing the purpose, especially noting that you overrideDjangoFilterExtensionto handle GIS and advanced logic, would be grand.class CustomFilterExtension(DjangoFilterExtension): + """ + Custom filter extension to handle advanced filtering logic and GIS fields. + """ target_class = "django_filters.rest_framework.DjangoFilterBackend" priority = 1
26-46: Excellent approach for customizing schema operations, but consider adding logging for debug.
The loop throughfilterset_class.base_filtersis well-structured. For future troubleshooters, a minimal debug log or comment could help. Something like “Resolving filter: {field_name}” could speed up diagnosing oddities.result = [] with add_trace_message(filterset_class): for field_name, filter_field in filterset_class.base_filters.items(): + # optional debug log + # logger.debug(f"Resolving filter field: {field_name}") result += self.resolve_filter_field( auto_schema, model, filterset_class, field_name, filter_field ) return resultcare/utils/swagger/schema.py (4)
1-20: Consider checking import usage.While it's excellent to see thorough use of DRF Spectacular functionalities, there's a risk of clutter if some imports end up unused in the future. Just ensure they remain strictly necessary.
29-42: Request serializer logic is well-structured but keep an eye on GET.It's a good approach to handle different serializers for POST, PUT, and PATCH. For GET, returning
Noneis fine for now, but verifying if your clients need an input schema is beneficial.
43-79: Response serializer fallback might cause confusion.When Pydantic models are not defined, the code falls back to
self._get_serializer(). This is flexible, but can make debugging tricky if a view unexpectedly lacks the relevant Pydantic attributes. A small docstring reminder or logging can help.
80-138: Thorough path parameter resolution.This logic comprehensively covers typed parameters, untyped parameters, and potential model mismatches. The gentle warnings guide developers to correct their routes, which is appreciated. Just a minor concern: consider factoring out the big
if/elseinto smaller helper functions if it grows further.care/emr/api/viewsets/user.py (2)
115-116: That error message seems... interesting.The error message mentions "cover image" when we're clearly dealing with profile pictures. I mean, consistency in terminology would be nice, wouldn't it?
- return Response({"detail": "No cover image to delete"}, status=404) + return Response({"detail": "No profile picture to delete"}, status=404)
109-113: Might want to return the updated user data after upload... just saying.After successfully uploading a profile picture, it would be somewhat helpful to return the updated user data, including the new profile_picture_url.
- return Response(status=200) + return Response( + UserSerializer(user, context={"request": request}).data, + status=200 + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
care/emr/api/otp_viewsets/login.py(3 hunks)care/emr/api/otp_viewsets/slot.py(3 hunks)care/emr/api/viewsets/base.py(2 hunks)care/emr/api/viewsets/encounter.py(6 hunks)care/emr/api/viewsets/facility.py(1 hunks)care/emr/api/viewsets/file_upload.py(3 hunks)care/emr/api/viewsets/observation.py(2 hunks)care/emr/api/viewsets/patient.py(5 hunks)care/emr/api/viewsets/questionnaire.py(4 hunks)care/emr/api/viewsets/resource_request.py(2 hunks)care/emr/api/viewsets/user.py(1 hunks)care/emr/apps.py(1 hunks)care/utils/schema.py(0 hunks)care/utils/swagger/__init__.py(1 hunks)care/utils/swagger/filter.py(1 hunks)care/utils/swagger/schema.py(1 hunks)config/settings/base.py(2 hunks)
💤 Files with no reviewable changes (1)
- care/utils/schema.py
✅ Files skipped from review due to trivial changes (2)
- care/emr/apps.py
- care/utils/swagger/init.py
🚧 Files skipped from review as they are similar to previous changes (10)
- config/settings/base.py
- care/emr/api/viewsets/base.py
- care/emr/api/viewsets/resource_request.py
- care/emr/api/viewsets/observation.py
- care/emr/api/otp_viewsets/login.py
- care/emr/api/viewsets/file_upload.py
- care/emr/api/viewsets/questionnaire.py
- care/emr/api/viewsets/patient.py
- care/emr/api/otp_viewsets/slot.py
- care/emr/api/viewsets/encounter.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
care/emr/api/viewsets/facility.py (2)
115-115: Finally, a sensible consolidation of HTTP methods!Nice to see the POST and DELETE methods unified under a single action. I suppose separating them wasn't really necessary after all.
120-132: Oh look, proper error handling has arrived!The implementation now includes all the essential error handling that one might expect from a production-ready endpoint. How refreshing! Particularly appreciate:
- Validation before deletion
- Standardized error response format
- Proper resource cleanup
Though I can't help but wonder why we needed a previous review to point these out... 🤔
care/utils/swagger/filter.py (1)
12-24: Potential KeyError risk withqs.query.annotations.
Iffilter_field.field_nameis not annotated, referencingqs.query.annotations[filter_field.field_name]can raise a KeyError. Handling this gracefully could avoid runtime issues.Do you want me to generate a fallback check or exception handling code snippet?
Also, be mindful of performance concerns by calling
objects.all(). With large datasets, this might be an overkill for schema generation.care/utils/swagger/schema.py (1)
22-28: Good fallback logic for tags.The fallback to take the last path segment as a tag is elegant. Just watch out for obscure endpoint paths; they might yield tags that are too generic or not quite helpful.
care/emr/api/viewsets/user.py (1)
101-103: Nice consolidation of the endpoints... I guess.The combination of POST and DELETE methods into a single endpoint follows REST principles quite elegantly. Though I wonder if anyone will notice this thoughtful improvement.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
care/emr/models/location.py (2)
37-54:⚠️ Potential issueThat recursive call on line 45 seems to be... doing nothing?
The recursive call
self.parent.get_parent_json()is made but its result is never used. This could lead to unnecessary database queries and potential infinite recursion.Apply this fix:
- self.parent.get_parent_json() + parent_json = self.parent.get_parent_json() + if not parent_json: + return {}
149-157:⚠️ Potential issueA wild infinite recursion appears!
The
handle_cascadetask lacks cycle detection and depth limits, which could lead to infinite recursion in case of circular parent-child relationships.Also, there's an incorrect field reference:
parent_location_idshould beparent_id.- for child in FacilityLocation.objects.filter(parent_location_id=base_location): + MAX_DEPTH = 100 # Prevent infinite recursion + def _cascade_with_depth(location_id, current_depth=0): + if current_depth >= MAX_DEPTH: + return + for child in FacilityLocation.objects.filter(parent_id=location_id): + child.save(update_fields=["cached_parent_json"]) + _cascade_with_depth(child.id, current_depth + 1) + + _cascade_with_depth(base_location)care/emr/api/viewsets/location.py (1)
184-186:⚠️ Potential issueOh, look who's trying to delete from the wrong table...
The filter is using
encounterinstead oflocationin the query, which would prevent the organization from being removed correctly.- encounter=instance, organization=organization + location=instance, organization=organization
🧹 Nitpick comments (1)
care/emr/models/location.py (1)
12-17: Perhaps we could make these status fields a bit more... predictable?The
statusandoperational_statusfields would benefit from having choices or validators to prevent invalid values. Theavailability_statusfield already has choices (as indicated by the comment), but they're not enforced at the model level.- status = models.CharField(max_length=255) - operational_status = models.CharField(max_length=255) - availability_status = models.CharField( - max_length=25 - ) # Populated from FacilityLocationEncounter + status = models.CharField(max_length=255, choices=LocationStatusChoices.choices) + operational_status = models.CharField(max_length=255, choices=LocationOperationalStatusChoices.choices) + availability_status = models.CharField( + max_length=25, + choices=LocationAvailabilityStatusChoices.choices + ) # Populated from FacilityLocationEncounter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/api/viewsets/location.py(1 hunks)care/emr/models/location.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
care/emr/api/viewsets/location.py (2)
210-210: That TODO comment is looking rather lonely...The comment suggests that association models are yet to be built, but the code is already merged. This could lead to incomplete functionality.
Would you like me to help implement the association models or create an issue to track this task?
322-384: Well, well, well... look who wrote some proper validation logic!The schedule conflict validation is comprehensive and well-implemented. Good job on covering all edge cases and providing clear error messages.
Add Features
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the high-level release notes:
New Features
Improvements
Bug Fixes
Documentation
Chores