[ENG-702] refact :mapped all fields under the standalone context builder#3706
[ENG-702] refact :mapped all fields under the standalone context builder#3706nandkishorr wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughReport context builders for patient, encounter, and account are refactored to remove minimum/base variants in favor of unified full context builders. ChangesReport Context Builder Consolidation
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Well, someone finally decided the "minimum" context builders had outlived their minimalism — consolidating everything into full builders now, no gradual deprecation notice needed, apparently. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR collapses three-level class hierarchies (Base + Minimum + Standalone) into a single class for each of the
Confidence Score: 5/5Safe to merge — the refactoring is purely structural with no logic changes outside consolidating the class hierarchy. All three context builders preserve their original slugs, display names, and get_context() semantics. Standalone instantiation bypasses get_context() in both old and new code, so the merged method causes no regression. Embedded instantiation correctly calls getattr(parent_context, parent_attribute) as before. The schema-extraction utility's visited.copy() logic handles the newly deeper nesting without cycles. No model changes, no migrations, no API surface changes. No files require special attention — all three changed files are self-contained context builder definitions with no downstream model or view impact. Important Files Changed
Reviews (1): Last reviewed commit: "refact:mapped all fields under the stand..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
care/emr/reports/context_builder/data_points/account.py (1)
149-161: 🚀 Performance & Scalability | 🔵 TrivialFull patient/facility context now gets built twice per account report.
With
primary_encounterpointed at the fullEncounterReportContext(which itself nests a fullpatientandfacility) and account also directly nesting fullpatient/facility, an account report now computes patient demographics, identifiers, tags, and facility details twice (once viaaccount.patient/account.facility, once viaaccount.primary_encounter.patient/.facility). Previously the nested encounter used a lighter "minimum" variant, avoiding this. Worth confirming this duplicated payload/query cost is acceptable for the intended use case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@care/emr/reports/context_builder/data_points/account.py` around lines 149 - 161, The account report now builds full patient and facility context twice because `primary_encounter` uses `EncounterReportContext`, which already nests `patient` and `facility`, while `account` also includes direct `patient` and `facility` fields. Update the `primary_encounter` field in `account.py` to point to the lighter encounter context used previously, or otherwise reduce the nested context so `PatientContextBuilder` and facility-building logic are not duplicated in the account report; verify the intended context shape against `primary_encounter`, `patient`, and `facility`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@care/emr/reports/context_builder/data_points/encounter.py`:
- Around line 229-233: `PatientFacilityIdentifiersContextBuilder.get_context`
assumes `patient.facility_identifiers` is always a dict, but it can be `None` or
otherwise falsy and will crash on `.get(...)`. Update this method to defensively
handle missing values the same way `Patient.build_facility_identifiers` does,
returning an empty list when `facility_identifiers` is unset, and only calling
`.get(...)` on a valid mapping.
---
Nitpick comments:
In `@care/emr/reports/context_builder/data_points/account.py`:
- Around line 149-161: The account report now builds full patient and facility
context twice because `primary_encounter` uses `EncounterReportContext`, which
already nests `patient` and `facility`, while `account` also includes direct
`patient` and `facility` fields. Update the `primary_encounter` field in
`account.py` to point to the lighter encounter context used previously, or
otherwise reduce the nested context so `PatientContextBuilder` and
facility-building logic are not duplicated in the account report; verify the
intended context shape against `primary_encounter`, `patient`, and `facility`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dcb3d8e4-5fc5-4ad9-b158-244a7b134618
📒 Files selected for processing (3)
care/emr/reports/context_builder/data_points/account.pycare/emr/reports/context_builder/data_points/encounter.pycare/emr/reports/context_builder/data_points/patient.py
| class PatientFacilityIdentifiersContextBuilder(IdentifiersContextBuilder): | ||
| def get_context(self): | ||
| return self.parent_context.patient.facility_identifiers.get( | ||
| str(self.parent_context.facility.id), [] | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against facility_identifiers being None.
Patient.facility_identifiers is a JSONField(default=dict, null=True, blank=True), and the model's own build_facility_identifiers defensively checks if not self.facility_identifiers before use — meaning it can be falsy/None for some rows. Calling .get(...) directly here will raise AttributeError for any such patient, crashing the whole encounter report.
🛡️ Proposed fix
class PatientFacilityIdentifiersContextBuilder(IdentifiersContextBuilder):
def get_context(self):
- return self.parent_context.patient.facility_identifiers.get(
+ return (self.parent_context.patient.facility_identifiers or {}).get(
str(self.parent_context.facility.id), []
)📝 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.
| class PatientFacilityIdentifiersContextBuilder(IdentifiersContextBuilder): | |
| def get_context(self): | |
| return self.parent_context.patient.facility_identifiers.get( | |
| str(self.parent_context.facility.id), [] | |
| ) | |
| class PatientFacilityIdentifiersContextBuilder(IdentifiersContextBuilder): | |
| def get_context(self): | |
| return (self.parent_context.patient.facility_identifiers or {}).get( | |
| str(self.parent_context.facility.id), [] | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@care/emr/reports/context_builder/data_points/encounter.py` around lines 229 -
233, `PatientFacilityIdentifiersContextBuilder.get_context` assumes
`patient.facility_identifiers` is always a dict, but it can be `None` or
otherwise falsy and will crash on `.get(...)`. Update this method to defensively
handle missing values the same way `Patient.build_facility_identifiers` does,
returning an empty list when `facility_identifiers` is unset, and only calling
`.get(...)` on a valid mapping.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3706 +/- ##
===========================================
- Coverage 79.66% 79.65% -0.01%
===========================================
Files 479 479
Lines 23010 23007 -3
Branches 2379 2379
===========================================
- Hits 18330 18326 -4
- Misses 4080 4081 +1
Partials 600 600 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Proposed Changes
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
New Features
Bug Fixes