adding model caching for UserSpec and other frequently used specs #3461
Conversation
📝 WalkthroughWalkthroughReplaces many explicit spec.serialize(...).to_json() calls with cache-backed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
# Conflicts: # care/emr/api/viewsets/encounter.py # care/emr/resources/facility/spec.py # care/emr/resources/organization/spec.py
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
care/emr/resources/meta_artifact/spec.py (1)
39-45: Missing defaults oncreated_byandupdated_bymay cause validation errors.Unlike the other specs in this PR (e.g.,
NoteThreadReadSpec,ValueSetReadSpec,QuestionnaireReadSpec), these fields lack default values. Theserialize_audit_usershelper only sets these keys when the IDs are non-null, which could cause Pydantic validation errors for records wherecreated_by_idorupdated_by_idhappens to be null.Suggested fix: add defaults for consistency
- created_by: UserSpec - updated_by: UserSpec + created_by: UserSpec = {} + updated_by: UserSpec = {}care/emr/resources/inventory/inventory_item/spec.py (1)
32-33: Incorrect docstring.The docstring says "Supply delivery read specification" but this class is
InventoryItemReadSpec. Someone was perhaps a bit too enthusiastic with copy-paste.📝 Suggested fix
class InventoryItemReadSpec(BaseInventoryItemSpec): - """Supply delivery read specification""" + """Inventory item read specification"""care/emr/models/location.py (1)
40-57: Handle potentialNonereturn frommodel_from_cache.If the parent
FacilityLocationrecord is deleted (but the FK reference still exists due toon_delete=models.SET_NULL),model_from_cachewill returnNonewith the defaultquiet=True. Line 51 then attemptstemp_data["cache_expiry"], which would raise aTypeError.While this edge case may be rare, it's worth adding a defensive check.
Suggested fix
self.parent.get_parent_json() temp_data = model_from_cache(FacilityLocationListSpec, id=self.parent.id) + if temp_data is None: + return {} temp_data["cache_expiry"] = str( timezone.now() + timedelta(days=self.cache_expiry_days) )
🤖 Fix all issues with AI agents
In `@care/emr/api/viewsets/patient.py`:
- Around line 281-284: The list comprehension building data passes the
ForeignKey object patient_user.user into model_from_cache which expects an id;
change the call inside the comprehension to pass patient_user.user_id instead
(e.g., model_from_cache(UserSpec, id=patient_user.user_id)) so model_from_cache
receives an int/str/None; update the comprehension that assigns data and keep
using UserSpec and patient_users as the referenced symbols.
In `@care/emr/resources/consent/spec.py`:
- Around line 119-124: The loop over obj.verification_details mutates each
verification dict in-place by assigning verification["verified_by"] =
model_from_cache(...); instead, avoid side effects by creating a new list of
verification dicts (e.g., iterate and for each verification create a shallow
copy or new dict), replace the "verified_by" value in the copy using
model_from_cache(UserSpec, external_id=...), and then assign that new list to
mapping["verification_details"] so obj.verification_details and any shared
cached data remain unchanged.
In `@care/emr/resources/device/history_spec.py`:
- Around line 42-44: history.get("updated_by") may be None which causes
model_from_cache(UserSpec, id=user) to raise ValueError before the `or {}` runs;
guard the call by checking / short-circuiting when user is falsy (e.g., if not
user: history["updated_by"] = {}) and only call model_from_cache(UserSpec,
id=user) when user is present, updating the assignment around the history
variable used in edit_history.append to avoid the exception.
In `@care/emr/resources/medication/dispense/dispense_order.py`:
- Around line 64-66: The mapping currently assigns mapping["location"] =
model_from_cache(FacilityLocationListSpec, id=obj.location.id) without guarding
for obj.location or None returns; change this to first check if obj.location is
truthy, then call model_from_cache and only assign mapping["location"] if the
call returns a non-None value — i.e., wrap the call in an if obj.location: block
and then set mapping["location"] = result only when result is not None,
referencing obj.location, mapping["location"], model_from_cache, and
FacilityLocationListSpec.
In `@care/emr/resources/observation/spec.py`:
- Around line 133-137: The code is loading the related User via
obj.data_entered_by when only the FK is needed, causing an extra query; change
the check to use obj.data_entered_by_id and pass that id to model_from_cache
instead of accessing the related object. Update the block around
cls.serialize_audit_users so it uses obj.data_entered_by_id in the conditional
and calls model_from_cache(UserSpec, id=obj.data_entered_by_id) to avoid loading
the User instance.
🧹 Nitpick comments (13)
care/emr/api/viewsets/scheduling/booking.py (1)
276-283: Consider filtering out potentialNonevalues from the response.Per the
model_from_cachesignature (returnsdict[str, Any] | None), if a user cannot be found (andquiet=Trueby default), the list comprehension will includeNoneentries. While the queryset filters for non-deleted users, it might be worth ensuring the API response doesn't inadvertently containNonevalues.🔧 Suggested defensive filter
return Response( { "users": [ - model_from_cache(UserSpec, id=user_resource.user_id) + user for user_resource in user_resources + if (user := model_from_cache(UserSpec, id=user_resource.user_id)) + is not None ] } )Alternatively, if a missing user indicates a data integrity violation that should be surfaced immediately, you could pass
quiet=Falseto raise an exception instead.care/emr/resources/questionnaire_response/spec.py (1)
48-49: Consider usingNoneas default instead ofdicttype.The defaults
created_by: UserSpec = dictandupdated_by: UserSpec = dictassign thedicttype as the default value, not a dict instance. This works becauseperform_extra_serializationpopulates these fields before they're needed, but it's an unusual pattern that could confuse future maintainers. PerhapsNonewithUserSpec | Nonewould express intent more clearly—unless, of course, this is just how things are done around here.♻️ Suggested improvement
- created_by: UserSpec = dict - updated_by: UserSpec = dict + created_by: UserSpec | None = None + updated_by: UserSpec | None = Nonecare/emr/resources/scheduling/resource/spec.py (1)
8-15: Consider applying caching consistently across all resource types.Lines 10 and 14 now use
model_from_cachefor practitioners and locations, but line 12 still uses the oldserialize(...).to_json()pattern forHealthcareServiceReadSpec. IfHealthcareServiceReadSpecisn't cacheable yet, that's perfectly fine—just thought you might want to keep a note of it for future consistency.care/emr/resources/activity_definition/spec.py (1)
148-154: Redundant existence check beforemodel_from_cache.The explicit query on lines 150-152 is now redundant since
model_from_cachereturnsNonefor missing objects (whenquiet=True, which is the default). Currently, this could trigger two database queries per location on a cache miss: one for the existence check and one insidemodel_from_cache.Consider simplifying by relying on the cache function's behavior:
Suggested simplification
locations = [] for location in obj.locations: - location_obj = FacilityLocation.objects.filter(id=location).first() - if not location_obj: + cached_location = model_from_cache(FacilityLocationListSpec, id=location) + if not cached_location: continue - locations.append(model_from_cache(FacilityLocationListSpec, id=location)) + locations.append(cached_location) mapping["locations"] = locationscare/emr/resources/inventory/supply_request/request_order.py (1)
102-104: Minor type annotation inconsistency.The
supplierfield is typed asOrganizationReadSpec | None, butmodel_from_cachereturnsdict | None. While Pydantic'smodel_constructhandles this gracefully, you might want to align the type annotation with the other fields for consistency.💡 Suggested alignment
- supplier: OrganizationReadSpec | None = None + supplier: dict | None = Nonecare/emr/resources/tag/config_spec.py (1)
172-172: Consider moving import to module level.The
model_from_cachefunction is already available fromcare.emr.resources.base(same module as the existing imports at line 14). Moving this import to the module level would be cleaner, unless there's a specific reason for keeping it inline that I'm not seeing.💡 Suggested change
At line 14, update the import:
-from care.emr.resources.base import EMRResource, cacheable, model_string +from care.emr.resources.base import EMRResource, cacheable, model_from_cache, model_stringThen remove line 172.
care/emr/resources/user/spec.py (1)
192-194: Variable shadowing:objreused in list comprehension.The loop variable
objshadows the method parameterobj: User. While the code works correctly (you want the Organization's id here), this can be confusing for future readers.Suggested rename for clarity
mapping["organizations"] = [ - model_from_cache(OrganizationReadSpec, id=obj.id) for obj in organizations + model_from_cache(OrganizationReadSpec, id=org.id) for org in organizations ]care/emr/resources/file_upload/spec.py (1)
115-116: Inconsistent pattern: accessingobj.updated_by.idinstead ofobj.updated_by_id.The condition checks
obj.updated_bywhich triggers a database query, and then accesses.idon the result. Meanwhile, lines 95-98 correctly useobj.created_by_idandobj.archived_by_iddirectly. For consistency and to avoid the extra query, consider usingobj.updated_by_idhere as well.♻️ Suggested improvement
- if obj.updated_by: - mapping["updated_by"] = model_from_cache(UserSpec, id=obj.updated_by.id) + if obj.updated_by_id: + mapping["updated_by"] = model_from_cache(UserSpec, id=obj.updated_by_id)care/emr/resources/patient/spec.py (1)
267-270: Consider usingobj.geo_organization_idto avoid an extra query.The current code accesses
obj.geo_organization.id, which may trigger a database query ifgeo_organizationisn't already loaded. Usingobj.geo_organization_idwould directly access the foreign key value without the extra fetch.♻️ Suggested improvement
- if obj.geo_organization: - mapping["geo_organization"] = model_from_cache( - OrganizationReadSpec, id=obj.geo_organization.id - ) + if obj.geo_organization_id: + mapping["geo_organization"] = model_from_cache( + OrganizationReadSpec, id=obj.geo_organization_id + )care/emr/resources/facility/spec.py (2)
111-116: Same FK access pattern issue as elsewhere.Using
obj.geo_organization.id(line 115) instead ofobj.geo_organization_idmay cause an unnecessary database hit. Since you're already checking for truthiness, switching to the_idfield would be more efficient.♻️ Suggested improvement
cls.serialize_audit_users(mapping, obj) mapping["facility_type"] = REVERSE_FACILITY_TYPES[obj.facility_type] - if obj.geo_organization: + if obj.geo_organization_id: mapping["geo_organization"] = model_from_cache( - OrganizationReadSpec, id=obj.geo_organization.id + OrganizationReadSpec, id=obj.geo_organization_id )
215-218: Same pattern here in FacilityMinimalReadSpec.For consistency with the suggested change above, this should also use
obj.geo_organization_id.♻️ Suggested improvement
- if obj.geo_organization: + if obj.geo_organization_id: mapping["geo_organization"] = model_from_cache( - OrganizationReadSpec, id=obj.geo_organization.id + OrganizationReadSpec, id=obj.geo_organization_id )care/emr/resources/inventory/supply_delivery/delivery_order.py (1)
80-90: Same FK access pattern applies here.For consistency with the recommended changes across this PR, consider using the
_idfields directly (obj.origin_id,obj.destination_id,obj.supplier_id) instead of accessing through the related object. This avoids potentially unnecessary database queries.♻️ Suggested improvement
- if obj.origin: - mapping["origin"] = model_from_cache( - FacilityLocationListSpec, id=obj.origin.id - ) - mapping["destination"] = model_from_cache( - FacilityLocationListSpec, id=obj.destination.id - ) - if obj.supplier: - mapping["supplier"] = model_from_cache( - OrganizationReadSpec, id=obj.supplier.id - ) + if obj.origin_id: + mapping["origin"] = model_from_cache( + FacilityLocationListSpec, id=obj.origin_id + ) + mapping["destination"] = model_from_cache( + FacilityLocationListSpec, id=obj.destination_id + ) + if obj.supplier_id: + mapping["supplier"] = model_from_cache( + OrganizationReadSpec, id=obj.supplier_id + )care/emr/resources/resource_request/spec.py (1)
150-151: Consider usingassigned_to_idfor consistency and efficiency.The
serialize_audit_usershelper (which you're using just below) accessesobj.created_by_iddirectly to avoid fetching the related object. Here,obj.assigned_totriggers a database fetch before extracting.id. It would be just lovely if this followed the same pattern.♻️ Suggested change
- if obj.assigned_to: - mapping["assigned_to"] = model_from_cache(UserSpec, id=obj.assigned_to.id) + if obj.assigned_to_id: + mapping["assigned_to"] = model_from_cache(UserSpec, id=obj.assigned_to_id)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
care/emr/api/viewsets/encounter.pycare/emr/api/viewsets/location.pycare/emr/api/viewsets/patient.pycare/emr/api/viewsets/questionnaire.pycare/emr/api/viewsets/scheduling/booking.pycare/emr/models/location.pycare/emr/resources/activity_definition/spec.pycare/emr/resources/allergy_intolerance/spec.pycare/emr/resources/condition/spec.pycare/emr/resources/consent/spec.pycare/emr/resources/device/history_spec.pycare/emr/resources/device/spec.pycare/emr/resources/encounter/spec.pycare/emr/resources/facility/spec.pycare/emr/resources/facility_organization/spec.pycare/emr/resources/file_upload/spec.pycare/emr/resources/healthcare_service/spec.pycare/emr/resources/inventory/inventory_item/spec.pycare/emr/resources/inventory/supply_delivery/delivery_order.pycare/emr/resources/inventory/supply_request/request_order.pycare/emr/resources/location/spec.pycare/emr/resources/medication/administration/spec.pycare/emr/resources/medication/dispense/dispense_order.pycare/emr/resources/medication/dispense/spec.pycare/emr/resources/meta_artifact/spec.pycare/emr/resources/notes/notes_spec.pycare/emr/resources/notes/thread_spec.pycare/emr/resources/observation/spec.pycare/emr/resources/organization/spec.pycare/emr/resources/patient/spec.pycare/emr/resources/payment_reconciliation/spec.pycare/emr/resources/questionnaire/spec.pycare/emr/resources/questionnaire_response/spec.pycare/emr/resources/resource_request/spec.pycare/emr/resources/scheduling/resource/spec.pycare/emr/resources/service_request/spec.pycare/emr/resources/tag/config_spec.pycare/emr/resources/user/spec.pycare/emr/resources/valueset/spec.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/resources/inventory/inventory_item/spec.pycare/emr/resources/service_request/spec.pycare/emr/resources/condition/spec.pycare/emr/resources/notes/thread_spec.pycare/emr/resources/medication/administration/spec.pycare/emr/resources/questionnaire/spec.pycare/emr/resources/encounter/spec.pycare/emr/api/viewsets/encounter.pycare/emr/resources/valueset/spec.pycare/emr/resources/inventory/supply_delivery/delivery_order.pycare/emr/resources/patient/spec.pycare/emr/resources/inventory/supply_request/request_order.pycare/emr/models/location.pycare/emr/api/viewsets/patient.pycare/emr/resources/scheduling/resource/spec.pycare/emr/resources/location/spec.pycare/emr/resources/allergy_intolerance/spec.pycare/emr/resources/consent/spec.pycare/emr/resources/healthcare_service/spec.pycare/emr/resources/tag/config_spec.pycare/emr/resources/notes/notes_spec.pycare/emr/resources/observation/spec.pycare/emr/resources/payment_reconciliation/spec.pycare/emr/resources/questionnaire_response/spec.pycare/emr/api/viewsets/questionnaire.pycare/emr/resources/user/spec.pycare/emr/resources/file_upload/spec.pycare/emr/api/viewsets/scheduling/booking.pycare/emr/resources/resource_request/spec.pycare/emr/resources/meta_artifact/spec.pycare/emr/resources/medication/dispense/spec.pycare/emr/resources/facility_organization/spec.pycare/emr/resources/organization/spec.pycare/emr/resources/facility/spec.pycare/emr/api/viewsets/location.pycare/emr/resources/activity_definition/spec.pycare/emr/resources/device/spec.pycare/emr/resources/medication/dispense/dispense_order.pycare/emr/resources/device/history_spec.py
**/{models,views,management/commands}/*.py
📄 CodeRabbit inference engine (.cursorrules)
Leverage Django’s ORM for database interactions; avoid raw SQL queries unless necessary for performance.
Files:
care/emr/models/location.py
🧠 Learnings (6)
📚 Learning: 2024-11-28T06:16:31.373Z
Learnt from: DraKen0009
Repo: ohcnetwork/care PR: 2620
File: care/facility/models/facility.py:306-311
Timestamp: 2024-11-28T06:16:31.373Z
Learning: In the 'care' project, moving imports of models like `Asset`, `AssetLocation`, `FacilityDefaultAssetLocation`, and `PatientSample` to the module level in `care/facility/models/facility.py` causes circular imports. Therefore, it's acceptable to keep these imports inside the `delete` method of the `Facility` class.
Applied to files:
care/emr/resources/inventory/inventory_item/spec.pycare/emr/resources/inventory/supply_delivery/delivery_order.pycare/emr/resources/inventory/supply_request/request_order.pycare/emr/models/location.pycare/emr/resources/location/spec.pycare/emr/resources/healthcare_service/spec.pycare/emr/resources/payment_reconciliation/spec.pycare/emr/resources/resource_request/spec.pycare/emr/resources/medication/dispense/spec.pycare/emr/resources/facility/spec.pycare/emr/api/viewsets/location.pycare/emr/resources/activity_definition/spec.pycare/emr/resources/device/spec.pycare/emr/resources/medication/dispense/dispense_order.py
📚 Learning: 2025-07-15T13:39:29.540Z
Learnt from: NikhilA8606
Repo: ohcnetwork/care PR: 3148
File: care/emr/api/viewsets/consent.py:22-24
Timestamp: 2025-07-15T13:39:29.540Z
Learning: In the care codebase, BaseModel (in care/utils/models/base.py) contains an external_id field which is inherited by all EMR models including Encounter. This allows filtering using field paths like `encounter__external_id` in Django filters.
Applied to files:
care/emr/resources/encounter/spec.pycare/emr/api/viewsets/encounter.pycare/emr/resources/location/spec.pycare/emr/resources/consent/spec.pycare/emr/resources/observation/spec.pycare/emr/resources/device/spec.py
📚 Learning: 2025-07-08T13:27:05.363Z
Learnt from: NikhilA8606
Repo: ohcnetwork/care PR: 3124
File: care/emr/resources/medication/administration/spec.py:163-164
Timestamp: 2025-07-08T13:27:05.363Z
Learning: In the care codebase, all EMR models inherit from EMRBaseModel which inherits from BaseModel (in care/utils/models/base.py). BaseModel provides common fields including created_date and modified_date with auto_now_add=True and auto_now=True respectively. These fields are available on all EMR models through inheritance.
Applied to files:
care/emr/api/viewsets/encounter.pycare/emr/api/viewsets/patient.pycare/emr/resources/consent/spec.pycare/emr/resources/observation/spec.pycare/emr/resources/file_upload/spec.pycare/emr/resources/device/spec.pycare/emr/resources/device/history_spec.py
📚 Learning: 2025-10-08T08:08:44.661Z
Learnt from: rithviknishad
Repo: ohcnetwork/care PR: 3302
File: care/emr/resources/scheduling/slot/spec.py:146-151
Timestamp: 2025-10-08T08:08:44.661Z
Learning: In care/emr/resources/scheduling/slot/spec.py and related token booking code, TokenBooking instances always have a patient associated. The codebase prefers a fail-fast approach where missing patients should cause errors rather than being handled gracefully with optional/nullable fields.
Applied to files:
care/emr/api/viewsets/patient.pycare/emr/api/viewsets/scheduling/booking.py
📚 Learning: 2025-10-08T08:07:56.493Z
Learnt from: rithviknishad
Repo: ohcnetwork/care PR: 3302
File: care/emr/resources/scheduling/slot/spec.py:156-161
Timestamp: 2025-10-08T08:07:56.493Z
Learning: In `care/emr/resources/scheduling/slot/spec.py`, token bookings must always have an associated patient. The maintainers prefer fail-fast behavior (allowing exceptions) over graceful degradation when a token booking lacks a patient, as it indicates a data integrity violation that should be caught immediately.
Applied to files:
care/emr/api/viewsets/patient.pycare/emr/api/viewsets/scheduling/booking.py
📚 Learning: 2025-03-19T14:53:14.299Z
Learnt from: DraKen0009
Repo: ohcnetwork/care PR: 2931
File: care/emr/migrations/0023_allergyintolerance_allergy_intolerance_type.py:16-16
Timestamp: 2025-03-19T14:53:14.299Z
Learning: For the AllergyIntolerance model, field validations are handled by Pydantic specs (AllergyIntoleranceWriteSpec) using enum types rather than in the Django model definition. The field `allergy_intolerance_type` is validated through the AllergyIntoleranceTypeOptions enum which contains "allergy" and "intolerance" options.
Applied to files:
care/emr/resources/allergy_intolerance/spec.py
🧬 Code graph analysis (37)
care/emr/resources/inventory/inventory_item/spec.py (2)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)
care/emr/resources/service_request/spec.py (2)
care/emr/resources/base.py (1)
model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)
care/emr/resources/condition/spec.py (1)
care/emr/resources/base.py (1)
serialize_audit_users(162-168)
care/emr/resources/notes/thread_spec.py (1)
care/emr/resources/base.py (1)
serialize_audit_users(162-168)
care/emr/resources/medication/administration/spec.py (1)
care/emr/resources/base.py (1)
serialize_audit_users(162-168)
care/emr/resources/questionnaire/spec.py (1)
care/emr/resources/base.py (1)
serialize_audit_users(162-168)
care/emr/api/viewsets/encounter.py (2)
care/emr/resources/base.py (1)
model_from_cache(232-275)care/emr/resources/facility_organization/spec.py (1)
FacilityOrganizationReadSpec(75-89)
care/emr/resources/valueset/spec.py (1)
care/emr/resources/base.py (1)
serialize_audit_users(162-168)
care/emr/resources/inventory/supply_delivery/delivery_order.py (3)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)care/emr/resources/organization/spec.py (1)
OrganizationReadSpec(54-63)
care/emr/resources/patient/spec.py (3)
care/emr/resources/base.py (3)
model_from_cache(232-275)perform_extra_serialization(40-41)serialize_audit_users(162-168)care/emr/resources/organization/spec.py (2)
OrganizationReadSpec(54-63)perform_extra_serialization(61-63)care/emr/resources/user/spec.py (4)
perform_extra_serialization(135-138)perform_extra_serialization(148-158)perform_extra_serialization(178-213)perform_extra_serialization(224-226)
care/emr/resources/inventory/supply_request/request_order.py (3)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/organization/spec.py (1)
OrganizationReadSpec(54-63)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)
care/emr/models/location.py (2)
care/emr/resources/base.py (1)
model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)
care/emr/api/viewsets/patient.py (2)
care/emr/resources/base.py (1)
model_from_cache(232-275)care/emr/resources/user/spec.py (1)
UserSpec(124-138)
care/emr/resources/scheduling/resource/spec.py (2)
care/emr/resources/base.py (1)
model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)
care/emr/resources/location/spec.py (1)
care/emr/resources/base.py (4)
EMRResource(13-168)cacheable(279-305)model_from_cache(232-275)serialize_audit_users(162-168)
care/emr/resources/allergy_intolerance/spec.py (1)
care/emr/resources/base.py (1)
serialize_audit_users(162-168)
care/emr/resources/consent/spec.py (2)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/user/spec.py (1)
UserSpec(124-138)
care/emr/resources/healthcare_service/spec.py (3)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)care/emr/resources/facility_organization/spec.py (1)
FacilityOrganizationReadSpec(75-89)
care/emr/resources/tag/config_spec.py (3)
care/emr/resources/base.py (1)
model_from_cache(232-275)care/emr/resources/facility_organization/spec.py (1)
FacilityOrganizationReadSpec(75-89)care/emr/resources/organization/spec.py (1)
OrganizationReadSpec(54-63)
care/emr/resources/notes/notes_spec.py (1)
care/emr/resources/base.py (1)
serialize_audit_users(162-168)
care/emr/resources/observation/spec.py (2)
care/emr/resources/base.py (3)
EMRResource(13-168)model_from_cache(232-275)serialize_audit_users(162-168)care/emr/resources/user/spec.py (1)
UserSpec(124-138)
care/emr/resources/payment_reconciliation/spec.py (2)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)
care/emr/resources/questionnaire_response/spec.py (1)
care/emr/resources/base.py (1)
serialize_audit_users(162-168)
care/emr/api/viewsets/questionnaire.py (2)
care/emr/resources/base.py (1)
model_from_cache(232-275)care/emr/resources/organization/spec.py (1)
OrganizationReadSpec(54-63)
care/emr/resources/file_upload/spec.py (2)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/user/spec.py (1)
UserSpec(124-138)
care/emr/api/viewsets/scheduling/booking.py (2)
care/emr/resources/base.py (1)
model_from_cache(232-275)care/emr/resources/user/spec.py (1)
UserSpec(124-138)
care/emr/resources/resource_request/spec.py (2)
care/emr/resources/base.py (3)
EMRResource(13-168)model_from_cache(232-275)serialize_audit_users(162-168)care/emr/resources/user/spec.py (1)
UserSpec(124-138)
care/emr/resources/meta_artifact/spec.py (1)
care/emr/resources/base.py (1)
serialize_audit_users(162-168)
care/emr/resources/medication/dispense/spec.py (2)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)
care/emr/resources/facility_organization/spec.py (1)
care/emr/resources/base.py (3)
EMRResource(13-168)cacheable(279-305)serialize_audit_users(162-168)
care/emr/resources/organization/spec.py (1)
care/emr/resources/base.py (3)
EMRResource(13-168)cacheable(279-305)serialize_audit_users(162-168)
care/emr/resources/facility/spec.py (2)
care/emr/resources/base.py (4)
EMRResource(13-168)cacheable(279-305)model_from_cache(232-275)serialize_audit_users(162-168)care/emr/resources/organization/spec.py (1)
OrganizationReadSpec(54-63)
care/emr/api/viewsets/location.py (2)
care/emr/resources/base.py (1)
model_from_cache(232-275)care/emr/resources/facility_organization/spec.py (1)
FacilityOrganizationReadSpec(75-89)
care/emr/resources/activity_definition/spec.py (2)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)
care/emr/resources/device/spec.py (3)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)care/emr/resources/facility_organization/spec.py (1)
FacilityOrganizationReadSpec(75-89)
care/emr/resources/medication/dispense/dispense_order.py (2)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)
care/emr/resources/device/history_spec.py (2)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/user/spec.py (1)
UserSpec(124-138)
🔇 Additional comments (56)
care/emr/api/viewsets/scheduling/booking.py (1)
30-30: LGTM on the import.The import aligns with the PR objective of using cached model lookups.
care/emr/resources/questionnaire_response/spec.py (2)
64-64: LGTM on the refactor to use centralized audit serialization.This change correctly delegates to the shared
serialize_audit_users()helper, which aligns with the PR's goal of consolidating audit-user serialization logic.
9-9: TheUserSpecimport is actually in use.It's referenced as a type hint for the
created_byandupdated_byattributes (lines 48-49), so keeping it around makes sense.Likely an incorrect or invalid review comment.
care/emr/resources/scheduling/resource/spec.py (1)
14-14:@cacheabledecorator is present onFacilityLocationListSpec— no issues here.The decorator exists, so
model_from_cachewill work as intended.care/emr/resources/allergy_intolerance/spec.py (1)
136-136: LGTM!Clean refactor to the centralized audit user serialization helper. The field defaults (
created_by: dict = {},updated_by: dict = {}) at lines 126-127 ensure safe handling when IDs are null.care/emr/resources/notes/thread_spec.py (1)
47-47: LGTM!Consistent with the centralized audit serialization pattern. Field defaults at lines 39-40 handle the null case appropriately.
care/emr/resources/valueset/spec.py (1)
69-69: LGTM!Centralized audit serialization applied correctly. Field defaults at lines 63-64 ensure graceful handling of null audit user IDs.
care/emr/resources/questionnaire/spec.py (1)
293-293: LGTM!Audit serialization properly centralized. Defaults are in place at lines 282-283, so this should work seamlessly.
care/emr/resources/condition/spec.py (1)
128-134: LGTM!Nicely consolidated audit field serialization using the centralized helper. Clean and consistent with the broader refactoring approach.
care/emr/resources/notes/notes_spec.py (1)
48-51: LGTM!The refactor to
serialize_audit_userskeeps things consistent across the codebase.care/emr/resources/observation/spec.py (1)
7-7: LGTM!Import addition required for the caching functionality.
care/emr/resources/service_request/spec.py (1)
169-173: LGTM!The switch to
model_from_cachealigns with the caching strategy. The batch query ensures iteration over valid locations, and the cache will help on subsequent requests. Just something to keep in mind: if the cache is cold, eachmodel_from_cachecall may hit the database again—but that's the nature of caching warm-up.care/emr/resources/activity_definition/spec.py (1)
15-15: LGTM!Import addition required for the caching functionality.
care/emr/resources/medication/administration/spec.py (1)
194-194: LGTM!The refactor to use
cls.serialize_audit_users(mapping, obj)properly centralizes audit field serialization, which is consistent with the pattern being applied across other specs in this PR.care/emr/resources/inventory/inventory_item/spec.py (1)
43-45: LGTM!The change to use
model_from_cachefor location serialization is consistent with the broader caching strategy in this PR.care/emr/resources/inventory/supply_request/request_order.py (1)
108-121: LGTM!The serialization logic correctly applies the
model_from_cachepattern forsupplier,origin, anddestination. The conditional checks for optional fields are properly preserved.care/emr/resources/tag/config_spec.py (1)
180-187: LGTM!The cache-backed serialization for
facility_organizationandorganizationis correctly implemented with appropriate conditional checks for optional fields.care/emr/resources/medication/dispense/spec.py (1)
215-217: Type annotation doesn't matchmodel_from_cachereturn type, but practically safe.The
locationfield isdict(required), whilemodel_from_cachereturnsdict | None. Sinceobj.locationis a valid foreign key reference that must exist, this won't fail at runtime—though Python's type checker might complain.FacilityLocationListSpecis correctly decorated with@cacheable, so the call is properly validated at runtime.care/emr/resources/encounter/spec.py (2)
167-170: LGTM — null guard is in place.The
if obj.current_locationcheck at line 167 ensuresmodel_from_cacheis only called when a location exists. The change correctly aligns with the caching pattern used elsewhere in this PR.
161-165: The@cacheabledecorator is already present onFacilityOrganizationReadSpec.That said,
model_from_cachecan still returnNonewhen an object isn't found (withquiet=Trueas default). This means theorganizationslist could containNonevalues if an organization is deleted between fetchingEncounterOrganizationrecords and the cache lookup—which might not be what you want.care/emr/resources/payment_reconciliation/spec.py (1)
148-152: LGTM — properly guarded cache lookup.The null check at line 148 ensures
model_from_cacheis only invoked whenobj.locationexists. The change is consistent with the broader caching pattern introduced in this PR.care/emr/resources/location/spec.py (3)
115-133: Good addition of@cacheabledecorator.The
FacilityLocationListSpecis now properly marked as cacheable, which enables themodel_from_cachelookups used throughout this PR. The implementation looks correct.One thing to note:
perform_extra_serializationcallsobj.get_parent_json()(line 128), which itself now usesmodel_from_cache(FacilityLocationListSpec, ...)internally. This creates a recursive caching pattern where serializing a location will cache its parent chain. Just something to be aware of for debugging cache-related issues.
140-142: LGTM — cleaner audit serialization.Using the shared
serialize_audit_usershelper is more maintainable than manually handlingcreated_byandupdated_byfields.
190-195: LGTM — location is a required FK.Based on the model definition in
care/emr/models/location.py(line 153),locationis a non-nullableForeignKey, so the direct access toobj.location.idis safe here.care/emr/resources/organization/spec.py (2)
53-64: LGTM!The
@cacheabledecorator is correctly applied, and the new fields (level_cache,system_generated,has_children,parent) appropriately expand the spec for cache-backed retrieval.
69-74: Nice centralization of audit user serialization.Using
serialize_audit_usersinstead of inline serialization is a good pattern that keeps things DRY across the codebase.care/emr/api/viewsets/questionnaire.py (2)
191-194: LGTM!The switch to
model_from_cachefor organization serialization is clean and consistent with the caching strategy.OrganizationReadSpecis properly decorated with@cacheableas verified in the organization spec file.
250-255: Consistent caching pattern applied.Same pattern as
get_organizations— well done keeping it uniform.care/emr/resources/user/spec.py (1)
154-158: LGTM!Cache-backed retrieval for
geo_organizationis correctly implemented.care/emr/resources/healthcare_service/spec.py (2)
81-84: Caching pattern is correctly applied.
FacilityOrganizationReadSpecproperly has the@cacheabledecorator, so themodel_from_cache()usage here is solid.
74-79: No issues here. BothFacilityLocationListSpecandFacilityOrganizationReadSpecare properly decorated with@cacheable, so themodel_from_cachecalls will work as intended.Likely an incorrect or invalid review comment.
care/emr/resources/file_upload/spec.py (2)
9-9: LGTM!Import addition aligns with the caching pattern being adopted across the codebase.
95-98: LGTM!Nice use of
_idfields to avoid unnecessary database queries when fetching cached user data. The conditional checks ensure no cache lookup is attempted for null values.care/emr/resources/patient/spec.py (2)
271-271: LGTM!Good use of the centralized
serialize_audit_usershelper - keeps things DRY and consistent across specs.
263-264: Remove the suggestion to move imports to module level.The local imports of
model_from_cacheandOrganizationReadSpecat lines 263-264 are intentional. Whilefacility/spec.pyimports these at module level,patient/spec.pydeliberately keeps them local—likely to avoid circular dependencies. This is a recognized pattern in this codebase, where local imports inside functions are acceptable when module-level imports would create circular references.Likely an incorrect or invalid review comment.
care/emr/resources/facility/spec.py (1)
10-10: LGTM!Clean import at module level - this is the pattern other files should follow.
care/emr/api/viewsets/patient.py (2)
17-17: LGTM!Import at module level, as it should be.
302-302: LGTM!Correct usage here -
useris already fetched and.idgives the proper primary key value.care/emr/resources/inventory/supply_delivery/delivery_order.py (1)
10-10: LGTM!Module-level import is correct.
care/emr/resources/facility_organization/spec.py (3)
6-6: LGTM on the import addition.The
cacheabledecorator import aligns with the caching strategy being introduced.
74-83: LGTM on the@cacheabledecorator and new fields.The
@cacheabledecorator is correctly applied, enabling cache invalidation viapost_savesignals. The new fields (created_by,updated_by,system_generated,level_cache,has_children) are well-typed and have sensible defaults.
85-89: Nice consolidation of audit user serialization. The delegation tocls.serialize_audit_users(mapping, obj)removes duplication and, as it turns out, does usemodel_from_cachefor consistent cache-backed user serialization, so that concern has been handily addressed.care/emr/api/viewsets/location.py (3)
19-19: LGTM on the import.The
model_from_cacheimport is necessary for the cache-backed serialization changes below.
183-189: LGTM on switching to cache-backed serialization.The change from
FacilityOrganizationReadSpec.serialize(...).to_json()tomodel_from_cache(FacilityOrganizationReadSpec, id=...)is consistent with the PR's caching strategy. Sincemodel_from_cachealready returns adict, the response structure remains compatible.
223-225: LGTM on the organizations_add response.Consistent with the other cache-backed changes. The response now benefits from caching.
care/emr/api/viewsets/encounter.py (3)
34-34: LGTM on the import.Adding
model_from_cachefor the cache-backed organization serialization.
259-265: LGTM on the organizations endpoint change.Mirrors the pattern used in
location.py. The cache-backed approach is consistent across both viewsets, which is... thoughtful.
292-294: LGTM on the organizations_add response.Consistent cache-backed serialization for the response.
care/emr/resources/device/spec.py (4)
8-8: LGTM on the import.Required for the cache-backed serialization changes below.
112-115: LGTM on the managing_organization caching.
FacilityOrganizationReadSpecis confirmed cacheable from the changes in this PR.
130-137: Same verification needed forFacilityLocationListSpec.This usage also depends on
FacilityLocationListSpecbeing cacheable. The verification script above should confirm this.
99-106:FacilityLocationListSpecis properly decorated with@cacheable.The decorator is in place in
care/emr/resources/location/spec.py, so themodel_from_cachecall is safe from runtime errors. No action needed.care/emr/resources/resource_request/spec.py (4)
9-9: LGTM on the import.The
model_from_cacheimport aligns with the caching refactor objective.
153-153: LGTM on centralized audit serialization.Using
serialize_audit_usersfor the audit fields is consistent with the refactoring goal and the spec properly declares bothcreated_byandupdated_byfields.
150-153: No action needed.UserSpecis already decorated with@cacheable(use_base_manager=True)at line 123 of the same file.Likely an incorrect or invalid review comment.
167-173: The spec'supdated_byexclusion is intentional, not an oversight.
ResourceRequestCommentinherits bothcreated_byandupdated_byfields fromEMRBaseModel, soserialize_audit_userswon't raise an error when it checks for both. However, sinceResourceRequestCommentListSpeconly declarescreated_by(line 168), the serializer will setupdated_byin the mapping dict, and Pydantic will silently ignore it. If this is intentional — that is, you want only audit creation data exposed in list responses — it's fine as-is, but consider adding a comment to clarify the design choice.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@care/emr/api/viewsets/questionnaire.py`:
- Around line 190-194: The list comprehension building organizations_serialized
can include None because model_from_cache(OrganizationReadSpec, id=org_id)
returns None when not found; filter out None values (or skip cache misses) so
the response does not contain null entries. Update the comprehension that
iterates over questionnaire_organizations to only include non-None results from
model_from_cache (or add a conditional guard) and ensure
organizations_serialized contains only valid OrganizationReadSpec instances
before returning or serializing.
- Around line 250-255: The list comprehension building organizations_serialized
can include None entries from model_from_cache; update the comprehension that
calls model_from_cache(OrganizationReadSpec, id=org_id) for each org_id from
QuestionnaireOrganization.objects.filter(questionnaire=questionnaire).values_list("organization_id",
flat=True) to filter out None results (e.g., by adding a conditional in the
comprehension or wrapping with a filter) so organizations_serialized contains
only non-None OrganizationReadSpec instances.
In `@care/emr/resources/scheduling/resource/spec.py`:
- Line 14: serialize_resource can return None via model_from_cache(…,
quiet=True), but TokenBookingMinimumReadSpec and TokenRetrieveSpec declare their
resource field as dict (or dict = {}) and assign mapping["resource"] = None when
lookups fail; update the type to allow None (dict | None or Optional[dict]) on
the resource field in both specs (TokenBookingMinimumReadSpec,
TokenRetrieveSpec) and/or add an explicit None-check in their
perform_extra_serialization methods to convert a missing resource to an
acceptable value (e.g., None or an empty dict) so the assigned
mapping["resource"] matches the field type.
In `@care/emr/resources/user/spec.py`:
- Around line 196-199: mapping["organizations"] is being populated by calling
model_from_cache(OrganizationReadSpec, id=org_id) for each org_id but
model_from_cache can return None; update the construction to exclude None
results (e.g., only include items where model_from_cache(...) is not None) so
mapping["organizations"] contains only valid OrganizationReadSpec instances;
locate the list comprehension that builds mapping["organizations"] and add a
conditional filter to skip None values or post-filter the list.
🧹 Nitpick comments (1)
care/emr/resources/scheduling/resource/spec.py (1)
8-15: Uneven application of caching in serialization branches.Lines 10 and 14 now use
model_from_cache(), while line 12 still usesHealthcareServiceReadSpec.serialize(obj.healthcare_service).to_json(). All three Spec classes follow the same pattern withperform_extra_serialization()methods, so one approach should work across all three—unless there's something special aboutHealthcareServiceReadSpecthat requires direct serialization.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
care/emr/api/viewsets/questionnaire.pycare/emr/resources/file_upload/spec.pycare/emr/resources/resource_request/spec.pycare/emr/resources/scheduling/resource/spec.pycare/emr/resources/service_request/spec.pycare/emr/resources/user/spec.py
🚧 Files skipped from review as they are similar to previous changes (2)
- care/emr/resources/file_upload/spec.py
- care/emr/resources/resource_request/spec.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/api/viewsets/questionnaire.pycare/emr/resources/scheduling/resource/spec.pycare/emr/resources/service_request/spec.pycare/emr/resources/user/spec.py
🧠 Learnings (2)
📚 Learning: 2024-11-28T06:16:31.373Z
Learnt from: DraKen0009
Repo: ohcnetwork/care PR: 2620
File: care/facility/models/facility.py:306-311
Timestamp: 2024-11-28T06:16:31.373Z
Learning: In the 'care' project, moving imports of models like `Asset`, `AssetLocation`, `FacilityDefaultAssetLocation`, and `PatientSample` to the module level in `care/facility/models/facility.py` causes circular imports. Therefore, it's acceptable to keep these imports inside the `delete` method of the `Facility` class.
Applied to files:
care/emr/resources/scheduling/resource/spec.py
📚 Learning: 2025-07-15T13:39:29.540Z
Learnt from: NikhilA8606
Repo: ohcnetwork/care PR: 3148
File: care/emr/api/viewsets/consent.py:22-24
Timestamp: 2025-07-15T13:39:29.540Z
Learning: In the care codebase, BaseModel (in care/utils/models/base.py) contains an external_id field which is inherited by all EMR models including Encounter. This allows filtering using field paths like `encounter__external_id` in Django filters.
Applied to files:
care/emr/resources/service_request/spec.py
🧬 Code graph analysis (4)
care/emr/api/viewsets/questionnaire.py (3)
care/emr/resources/base.py (1)
model_from_cache(232-275)care/emr/resources/organization/spec.py (1)
OrganizationReadSpec(54-63)care/emr/models/questionnaire.py (1)
QuestionnaireOrganization(134-153)
care/emr/resources/scheduling/resource/spec.py (2)
care/emr/resources/base.py (1)
model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)
care/emr/resources/service_request/spec.py (2)
care/emr/resources/base.py (1)
model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)
care/emr/resources/user/spec.py (3)
care/emr/resources/base.py (1)
model_from_cache(232-275)care/emr/resources/organization/spec.py (1)
OrganizationReadSpec(54-63)care/emr/models/organization.py (2)
Organization(106-107)OrganizationUser(110-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
🔇 Additional comments (5)
care/emr/api/viewsets/questionnaire.py (1)
19-21: LGTM on the import.The import for
model_from_cachealongsideOrganizationReadSpecis appropriate for the caching pattern being introduced.care/emr/resources/user/spec.py (3)
11-11: LGTM on the import update.Adding
model_from_cacheto the existing imports fromcare.emr.resources.baseis clean and follows the existing import pattern.
154-158: Trade-off acknowledged - caching benefits outweigh minor inefficiency.Since
obj.geo_organizationis already loaded at this point, callingmodel_from_cachewith just the ID means potentially re-fetching what we already have. That said, the cache population for subsequent requests and codebase consistency make this a reasonable trade-off. Just something to keep in mind.
184-199: The superuser vs non-superuser organization retrieval logic looks correct.The refactoring to fetch IDs first via
values_listand then usemodel_from_cacheis a sensible approach that aligns with the caching pattern being introduced across the codebase. The conditional logic is preserved correctly.care/emr/resources/service_request/spec.py (1)
168-172: ID mismatch claim is incorrect; however,Noneentries and N+1 concern remain valid.
ServiceRequest.locationsstoresArrayField(IntegerField)— plain DB PKs, not UUIDs — somodel_from_cache(..., id=location_id)at line 170 is actually the right call. The type hintlist[UUID4]inServiceRequestWriteSpecis misleading but unrelated.That said, two real issues remain:
- If a location isn't found in cache or DB,
model_from_cachereturnsNone(with defaultquiet=True), which gets appended directly, leavingNoneentries in the list (breaking the type contractlist[dict]).- On cold cache, this becomes one DB query per location instead of a single
id__inbulk fetch.Corrected fix (use the right logic + skip missing items)
- locations = [] - for location_id in obj.locations: - locations.append(model_from_cache(FacilityLocationListSpec, id=location_id)) + locations: list[dict] = [] + for location_id in obj.locations: + location = model_from_cache( + FacilityLocationListSpec, id=location_id + ) + if location is not None: + locations.append(location)Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@care/emr/resources/device/history_spec.py`:
- Around line 41-47: The loop over obj.edit_history may assign None when
model_from_cache(UserSpec, id=user) misses; update the code in the block that
handles history entries (the for loop iterating obj.edit_history and setting
history["updated_by"]) to capture the return value of model_from_cache and, if
it is None, replace it with an empty object (e.g., {}) so updated_by always has
an object shape; keep using UserSpec and preserve the existing edit_history
accumulation (edit_history.append(history)).
♻️ Duplicate comments (1)
care/emr/resources/medication/dispense/dispense_order.py (1)
54-67: Don’t assign a possibly-Nonecache lookup into a requireddictfield (and don’t silently omitlocation).
model_from_cache(...)returnsdict | None. Right nowlocationis typed as a requireddict, but this code can still end up settingmapping["location"] = None(or leaving it unset), which is a leaky contract for downstream consumers and may produce oddmodel_constructstates.If
locationis truly required, make the failure loud; if it’s optional, type it as optional and setNoneintentionally.Proposed fix (required location → fail loudly)
class MedicationDispenseOrderReadSpec(BaseMedicationDispenseOrderSpec): patient: dict location: dict @@ def perform_extra_serialization(cls, mapping, obj): mapping["id"] = obj.external_id mapping["patient"] = PatientListSpec.serialize(obj.patient).to_json() - if obj.location_id: - mapping["location"] = model_from_cache( - FacilityLocationListSpec, id=obj.location_id - ) + if obj.location_id: + mapping["location"] = model_from_cache( + FacilityLocationListSpec, id=obj.location_id, quiet=False + )Alternative (optional location → make it explicit)
class MedicationDispenseOrderReadSpec(BaseMedicationDispenseOrderSpec): patient: dict - location: dict + location: dict | None = None @@ if obj.location_id: - mapping["location"] = model_from_cache( - FacilityLocationListSpec, id=obj.location_id - ) + mapping["location"] = model_from_cache( + FacilityLocationListSpec, id=obj.location_id + )Verification script (confirm nullability + expectations):
#!/bin/bash set -euo pipefail # 1) Is DispenseOrder.location nullable / blank? rg -n "class DispenseOrder\b" -n care/emr/models/medication_dispense.py rg -n "location\s*=\s*models\.(ForeignKey|OneToOneField)\(" -n care/emr/models/medication_dispense.py -n -C2 # 2) Any callers/tests expecting location always present in read payload? rg -nP --type py "\bMedicationDispenseOrderReadSpec\b|\bdispense order\b" -C2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
care/emr/api/viewsets/patient.pycare/emr/resources/device/history_spec.pycare/emr/resources/medication/dispense/dispense_order.pycare/emr/resources/observation/spec.py
🚧 Files skipped from review as they are similar to previous changes (1)
- care/emr/api/viewsets/patient.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/resources/medication/dispense/dispense_order.pycare/emr/resources/observation/spec.pycare/emr/resources/device/history_spec.py
🧠 Learnings (3)
📚 Learning: 2024-11-28T06:16:31.373Z
Learnt from: DraKen0009
Repo: ohcnetwork/care PR: 2620
File: care/facility/models/facility.py:306-311
Timestamp: 2024-11-28T06:16:31.373Z
Learning: In the 'care' project, moving imports of models like `Asset`, `AssetLocation`, `FacilityDefaultAssetLocation`, and `PatientSample` to the module level in `care/facility/models/facility.py` causes circular imports. Therefore, it's acceptable to keep these imports inside the `delete` method of the `Facility` class.
Applied to files:
care/emr/resources/medication/dispense/dispense_order.pycare/emr/resources/device/history_spec.py
📚 Learning: 2025-07-08T13:27:05.363Z
Learnt from: NikhilA8606
Repo: ohcnetwork/care PR: 3124
File: care/emr/resources/medication/administration/spec.py:163-164
Timestamp: 2025-07-08T13:27:05.363Z
Learning: In the care codebase, all EMR models inherit from EMRBaseModel which inherits from BaseModel (in care/utils/models/base.py). BaseModel provides common fields including created_date and modified_date with auto_now_add=True and auto_now=True respectively. These fields are available on all EMR models through inheritance.
Applied to files:
care/emr/resources/observation/spec.pycare/emr/resources/device/history_spec.py
📚 Learning: 2025-07-15T13:39:29.540Z
Learnt from: NikhilA8606
Repo: ohcnetwork/care PR: 3148
File: care/emr/api/viewsets/consent.py:22-24
Timestamp: 2025-07-15T13:39:29.540Z
Learning: In the care codebase, BaseModel (in care/utils/models/base.py) contains an external_id field which is inherited by all EMR models including Encounter. This allows filtering using field paths like `encounter__external_id` in Django filters.
Applied to files:
care/emr/resources/observation/spec.py
🧬 Code graph analysis (3)
care/emr/resources/medication/dispense/dispense_order.py (2)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/location/spec.py (1)
FacilityLocationListSpec(116-132)
care/emr/resources/observation/spec.py (2)
care/emr/resources/base.py (3)
EMRResource(13-168)model_from_cache(232-275)serialize_audit_users(162-168)care/emr/resources/user/spec.py (1)
UserSpec(124-138)
care/emr/resources/device/history_spec.py (2)
care/emr/resources/base.py (2)
EMRResource(13-168)model_from_cache(232-275)care/emr/resources/user/spec.py (1)
UserSpec(124-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
🔇 Additional comments (4)
care/emr/resources/device/history_spec.py (1)
6-6: Import change looks fine.Pulling
model_from_cachein alongsideEMRResourceis consistent with the direction of the PR.care/emr/resources/medication/dispense/dispense_order.py (1)
9-9: Importingmodel_from_cacheis fine (and at least it’s actually used).care/emr/resources/observation/spec.py (2)
6-8: Import change looks fine; just keep it consistent.Pulling in
model_from_cachehere is reasonable given the new serialization path; nothing exciting, but it does the job.
133-137: The soft-deleted user concern is already mitigated—UserSpec uses@cacheable(use_base_manager=True).The potential issue you raised about
model_from_cache()silently filtering deleted users is valid in principle:CustomUserManagerfiltersdeleted=Falsein itsget_queryset(), and without the base-manager bypass, cache lookups would miss soft-deleted users. However,UserSpecat line 123 is already decorated with@cacheable(use_base_manager=True), which sets__cacheable_use_base_manager__and ensuresmodel_from_cache()uses the unfiltered_base_manager. No changes needed.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3461 +/- ##
===========================================
+ Coverage 74.67% 74.74% +0.06%
===========================================
Files 471 471
Lines 21507 21478 -29
Branches 2225 2198 -27
===========================================
- Hits 16061 16053 -8
+ Misses 4955 4945 -10
+ Partials 491 480 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# Conflicts: # care/emr/resources/medication/dispense/dispense_order.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@care/emr/resources/user/spec.py`:
- Around line 193-197: The organization_ids list can contain duplicates when a
user has multiple OrganizationUser rows for the same org; update the query that
builds organization_ids (the OrganizationUser.objects.filter(user=obj) call) to
deduplicate results by applying distinct() at the ORM level (e.g., call
.distinct() before .values_list(...)) or by converting to a set after fetching,
so organization_ids contains unique organization IDs only.
♻️ Duplicate comments (1)
care/emr/resources/user/spec.py (1)
198-201: Filter outNonefrom cache lookups.
model_from_cachecan returnNone, and the list comprehension will happily include it.🔧 Suggested fix
- mapping["organizations"] = [ - model_from_cache(OrganizationReadSpec, id=org_id) - for org_id in organization_ids - ] + mapping["organizations"] = [ + org + for org_id in organization_ids + if (org := model_from_cache(OrganizationReadSpec, id=org_id)) is not None + ]
🧹 Nitpick comments (2)
care/emr/api/viewsets/patient.py (1)
288-291: Consider filtering out potentialNonevalues from the result list.The
model_from_cachefunction returnsNonewhen the object isn't found (withquiet=Truedefault). If aPatientUserrecord exists but its associatedUserhas been deleted or is otherwise missing, this list comprehension would includeNonevalues in the response, which might not be the intended behavior for clients.♻️ Suggested improvement
data = [ - model_from_cache(UserSpec, id=patient_user.user_id) + user_data for patient_user in patient_users + if (user_data := model_from_cache(UserSpec, id=patient_user.user_id)) is not None ]care/emr/api/viewsets/scheduling/booking.py (1)
278-285: SameNonefiltering consideration as in the patient viewset.While the queryset already filters for
user__deleted=False(which helpfully reduces the risk), there's still a theoretical window where a user could be deleted between query execution and cache lookup. For consistency with defensive practices, you might want to filter outNonevalues here as well.♻️ Suggested improvement
return Response( { "users": [ - model_from_cache(UserSpec, id=user_resource.user_id) + user_data for user_resource in user_resources + if (user_data := model_from_cache(UserSpec, id=user_resource.user_id)) is not None ] } )
| organization_ids = list( | ||
| OrganizationUser.objects.filter(user=obj).values_list( | ||
| "organization_id", flat=True | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Deduplicate org IDs to avoid repeated organizations.
If a user has multiple OrganizationUser rows for the same org, the response will now include duplicates (the old id__in path implicitly deduped).
🔧 Proposed fix
- organization_ids = list(
- OrganizationUser.objects.filter(user=obj).values_list(
- "organization_id", flat=True
- )
- )
+ organization_ids = list(
+ OrganizationUser.objects.filter(user=obj)
+ .values_list("organization_id", flat=True)
+ .distinct()
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| organization_ids = list( | |
| OrganizationUser.objects.filter(user=obj).values_list( | |
| "organization_id", flat=True | |
| ) | |
| ) | |
| organization_ids = list( | |
| OrganizationUser.objects.filter(user=obj) | |
| .values_list("organization_id", flat=True) | |
| .distinct() | |
| ) |
🤖 Prompt for AI Agents
In `@care/emr/resources/user/spec.py` around lines 193 - 197, The organization_ids
list can contain duplicates when a user has multiple OrganizationUser rows for
the same org; update the query that builds organization_ids (the
OrganizationUser.objects.filter(user=obj) call) to deduplicate results by
applying distinct() at the ORM level (e.g., call .distinct() before
.values_list(...)) or by converting to a set after fetching, so organization_ids
contains unique organization IDs only.
# Conflicts: # care/emr/api/viewsets/questionnaire.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@care/emr/resources/medication/dispense/dispense_order.py`:
- Around line 63-64: Replace the redundant query
FacilityLocation.objects.get(id=obj.location_id) with the relation accessor
obj.location and serialize via
FacilityLocationListSpec.serialize(obj.location).to_json(); also ensure the
viewset that returns these DispenseOrder objects uses select_related("location")
in its get_queryset() to avoid N+1 queries so both implicit access and
query-time optimization are addressed (also update the second occurrence around
the other mapping lines 78–79 similarly).
In `@care/emr/resources/service_request/spec.py`:
- Around line 169-173: Replace the per-id queries with a single bulk fetch:
query FacilityLocation.objects.filter(id__in=obj.locations) once, build a dict
mapping id -> FacilityLocation instance, then iterate the original obj.locations
list to preserve order and append
FacilityLocationListSpec.serialize(location).to_json() only when the id exists
in the dict; update the code around the locations list creation (the loop over
obj.locations and the FacilityLocation.objects.filter call) to use this
approach.
🧹 Nitpick comments (2)
care/emr/resources/inventory/supply_request/request_order.py (1)
114-121: Prefer using existing FK objects to avoid extra queries + possibleDoesNotExist.
Right now you always hit the DB and can 500 on a dangling FK. Using the related objects keeps it cheap and safer if the relation is already loaded (or even missing).♻️ Proposed tweak
- if obj.origin_id: - origin = FacilityLocation.objects.get(id=obj.origin_id) - mapping["origin"] = FacilityLocationListSpec.serialize(origin).to_json() - if obj.destination_id: - destination = FacilityLocation.objects.get(id=obj.destination_id) - mapping["destination"] = FacilityLocationListSpec.serialize( - destination - ).to_json() + if obj.origin_id and obj.origin: + mapping["origin"] = FacilityLocationListSpec.serialize(obj.origin).to_json() + if obj.destination_id and obj.destination: + mapping["destination"] = FacilityLocationListSpec.serialize( + obj.destination + ).to_json()care/emr/resources/healthcare_service/spec.py (1)
76-82: Overly broad exception handling silently swallows errors.The
except Exception: passcatches and ignores all exceptions, including unexpected ones that might indicate actual bugs or data corruption. If a location ID exists inobj.locationsbut can't be fetched, it would be helpful to at least know about it.Consider catching only
FacilityLocation.DoesNotExistor adding minimal logging so debugging doesn't become a treasure hunt later.♻️ Suggested improvement
try: location_obj = FacilityLocation.objects.get(id=location) locations.append( FacilityLocationListSpec.serialize(location_obj).to_json() ) - except Exception: # noqa S110 + except FacilityLocation.DoesNotExist: # noqa S110 pass
| location = FacilityLocation.objects.get(id=obj.location_id) | ||
| mapping["location"] = FacilityLocationListSpec.serialize(location).to_json() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the file around lines 63-64 to understand context
head -n 90 care/emr/resources/medication/dispense/dispense_order.py | tail -n 35Repository: ohcnetwork/care
Length of output: 1364
🏁 Script executed:
# Search for select_related("location") usage in the codebase
rg -n --type py 'select_related\([^)]*location' care/Repository: ohcnetwork/care
Length of output: 134
🏁 Script executed:
# Look for the DispenseOrder model and related queryset definitions
rg -n --type py 'class.*DispenseOrder|select_related|prefetch_related' care/emr/resources/medication/dispense/ -A 3Repository: ohcnetwork/care
Length of output: 2547
🏁 Script executed:
# Check if there's a related_name or ForeignKey definition for location in the model
find care -name '*.py' -type f | xargs grep -l 'class.*DispenseOrder\|class.*Dispense' | head -5Repository: ohcnetwork/care
Length of output: 296
🏁 Script executed:
# Look for the model definition that contains location_id
rg -n --type py 'location_id|location.*ForeignKey' care/emr/ -B 2 -A 2 | head -50Repository: ohcnetwork/care
Length of output: 4242
🏁 Script executed:
# Look for the DispenseOrder model definition to understand the location relationship
rg -n --type py 'class DispenseOrder' care/emr/models/ -A 20Repository: ohcnetwork/care
Length of output: 1587
🏁 Script executed:
# Check the viewset that uses these specs to see if select_related is used
rg -n --type py 'MedicationDispenseOrderReadSpec|MedicationDispenseOrderRetrieveSpec' care/emr/api/viewsets/ -B 5 -A 10Repository: ohcnetwork/care
Length of output: 3226
🏁 Script executed:
# Check if obj.location is accessible (check if it's a ForeignKey relationship)
rg -n --type py 'location.*=.*ForeignKey\|location.*=.*models\.ForeignKey' care/emr/models/medication_dispense.py -B 1 -A 1Repository: ohcnetwork/care
Length of output: 41
🏁 Script executed:
# Examine the full DispenseOrder model file to understand the relationships
cat care/emr/models/medication_dispense.py | head -100Repository: ohcnetwork/care
Length of output: 2560
🏁 Script executed:
# Check if the viewset's get_queryset uses select_related or prefetch_related
rg -n --type py 'get_queryset|select_related|prefetch_related' care/emr/api/viewsets/inventory/dispense_order.py -A 10 -B 2Repository: ohcnetwork/care
Length of output: 805
🏁 Script executed:
# Also check the base EMRBaseViewSet class to see if it implements queryset optimization
find care -name 'base.py' -path '*/viewsets/*' -type f | xargs grep -l 'EMRBaseViewSet' | head -1 | xargs cat | head -150Repository: ohcnetwork/care
Length of output: 5982
🏁 Script executed:
# Get the full get_queryset method from the dispense_order viewset
sed -n '104,130p' care/emr/api/viewsets/inventory/dispense_order.pyRepository: ohcnetwork/care
Length of output: 1278
🏁 Script executed:
# Check if EMRBaseViewSet has a get_queryset that adds select_related
rg -n --type py 'class EMRBaseViewSet' care/emr/api/viewsets/ -A 50 | grep -E 'get_queryset|select_related|prefetch_related' -A 5 -B 5Repository: ohcnetwork/care
Length of output: 1506
🏁 Script executed:
# Look for any parent class or mixin that might add select_related to DispenseOrder
rg -n --type py 'DispenseOrder' care/emr/api/viewsets/inventory/dispense_order.py -B 5 -A 30 | head -60Repository: ohcnetwork/care
Length of output: 2399
Use the relation accessor instead of a redundant query.
The current code ignores Django's relation cache entirely, forcing a fresh query per row even though location is a ForeignKey on the model. Using obj.location is more idiomatic and respects the relation pattern.
That said, the underlying N+1 problem persists unless the viewset's queryset also adds select_related("location") at query time—otherwise you're still hitting the database, just implicitly via the relation accessor. Consider optimizing both the queryset and the serialization:
🛠️ Suggested changes
In care/emr/api/viewsets/inventory/dispense_order.py, add to get_queryset():
def get_queryset(self):
facility = self.get_facility_obj()
- queryset = super().get_queryset().filter(facility=facility)
+ queryset = super().get_queryset().select_related("location").filter(facility=facility)In care/emr/resources/medication/dispense/dispense_order.py, lines 63–64 and 78–79:
- location = FacilityLocation.objects.get(id=obj.location_id)
- mapping["location"] = FacilityLocationListSpec.serialize(location).to_json()
+ mapping["location"] = FacilityLocationListSpec.serialize(obj.location).to_json()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| location = FacilityLocation.objects.get(id=obj.location_id) | |
| mapping["location"] = FacilityLocationListSpec.serialize(location).to_json() | |
| mapping["location"] = FacilityLocationListSpec.serialize(obj.location).to_json() |
🤖 Prompt for AI Agents
In `@care/emr/resources/medication/dispense/dispense_order.py` around lines 63 -
64, Replace the redundant query FacilityLocation.objects.get(id=obj.location_id)
with the relation accessor obj.location and serialize via
FacilityLocationListSpec.serialize(obj.location).to_json(); also ensure the
viewset that returns these DispenseOrder objects uses select_related("location")
in its get_queryset() to avoid N+1 queries so both implicit access and
query-time optimization are addressed (also update the second occurrence around
the other mapping lines 78–79 similarly).
| locations = [] | ||
| for location in FacilityLocation.objects.filter(id__in=obj.locations): | ||
| locations.append(FacilityLocationListSpec.serialize(location).to_json()) | ||
| for location_id in obj.locations: | ||
| location = FacilityLocation.objects.filter(id=location_id).first() | ||
| if location: | ||
| locations.append(FacilityLocationListSpec.serialize(location).to_json()) |
There was a problem hiding this comment.
Avoid N+1 queries when loading locations.
Right now each location_id triggers its own query, which is… not great on a hot read path. Please switch to a single bulk fetch and preserve order.
🔧 Suggested fix (bulk fetch + order preservation)
- locations = []
- for location_id in obj.locations:
- location = FacilityLocation.objects.filter(id=location_id).first()
- if location:
- locations.append(FacilityLocationListSpec.serialize(location).to_json())
+ locations = []
+ if obj.locations:
+ location_map = {
+ loc.id: loc
+ for loc in FacilityLocation.objects.filter(id__in=obj.locations)
+ }
+ for location_id in obj.locations:
+ location = location_map.get(location_id)
+ if location:
+ locations.append(
+ FacilityLocationListSpec.serialize(location).to_json()
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| locations = [] | |
| for location in FacilityLocation.objects.filter(id__in=obj.locations): | |
| locations.append(FacilityLocationListSpec.serialize(location).to_json()) | |
| for location_id in obj.locations: | |
| location = FacilityLocation.objects.filter(id=location_id).first() | |
| if location: | |
| locations.append(FacilityLocationListSpec.serialize(location).to_json()) | |
| locations = [] | |
| if obj.locations: | |
| location_map = { | |
| loc.id: loc | |
| for loc in FacilityLocation.objects.filter(id__in=obj.locations) | |
| } | |
| for location_id in obj.locations: | |
| location = location_map.get(location_id) | |
| if location: | |
| locations.append( | |
| FacilityLocationListSpec.serialize(location).to_json() | |
| ) |
🤖 Prompt for AI Agents
In `@care/emr/resources/service_request/spec.py` around lines 169 - 173, Replace
the per-id queries with a single bulk fetch: query
FacilityLocation.objects.filter(id__in=obj.locations) once, build a dict mapping
id -> FacilityLocation instance, then iterate the original obj.locations list to
preserve order and append FacilityLocationListSpec.serialize(location).to_json()
only when the id exists in the dict; update the code around the locations list
creation (the loop over obj.locations and the FacilityLocation.objects.filter
call) to use this approach.
Proposed Changes
Added caching for -
Associated Issue
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Performance Improvements
Refactor
API / Schema
✏️ Tip: You can customize this high-level summary in your review settings.