[ENG-378] Add support for the missing encounter related fields in Repor…#3662
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEncounter context now includes priority mapping, tags, hospitalization fields, external identifiers, and per-facility patient identifier selection; patient context no longer exposes facility-specific identifiers. IdentifiersContextBuilder.config drops its preview_value parameter. ChangesEncounter and Patient Context Restructuring
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs:
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR extends the encounter report builder by adding hospitalization detail fields, an encounter priority lookup, and moves facility-identifier resolution from the patient context to the encounter context (scoped to the encounter's facility).
Confidence Score: 3/5The encounter report will crash at runtime for any encounter where facility identifiers are absent for the current facility, and the primary new features are never exposed in the report builder. The unguarded dict access in PatientFacilityIdentifiersContextBuilder will raise KeyError in production for encounters without facility-specific identifiers. EncounterHospitalizationContextBuilder and ENCOUNTER_PRIORITY_DISPLAY are both fully defined but never attached to a Field, so the hospitalization and priority fields the PR aimed to add are silently absent. care/emr/reports/context_builder/data_points/encounter.py needs attention: the duplicate dict, the missing Field registrations for hospitalization and priority, and the unsafe key access all require fixes before merging. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
care/emr/reports/context_builder/data_points/encounter.py (1)
306-379:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose hospitalization fields in
EncounterReportContext(currently not wired).
EncounterHospitalizationContextBuilderis defined, but noFieldreferences it here, so those new hospitalization datapoints are not actually available in encounter reports.Proposed fix
class EncounterReportContext(BaseEncounterReportContext): @@ + hospitalization = Field( + display="Hospitalization", + target_context=EncounterHospitalizationContextBuilder, + preview_value="", + description="Hospitalization details associated with the encounter", + ) + facility_identifiers = Field( display="Facility Identifiers", target_context=PatientFacilityIdentifiersContextBuilder, preview_value="", description="Facility identifiers associated with the encounter", )🤖 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 306 - 379, Add a Field on the EncounterReportContext named e.g. hospitalization that targets EncounterHospitalizationContextBuilder so the hospitalization datapoints are exposed; specifically, in the EncounterReportContext class (where other Fields like patient, facility, diagnoses are declared) add a Field with target_context=EncounterHospitalizationContextBuilder, a meaningful display like "Hospitalization Details", a preview_value (e.g. ""), and a description such as "Hospitalization details for the encounter" so the EncounterHospitalizationContextBuilder is wired into the encounter report context.
🧹 Nitpick comments (1)
care/emr/reports/context_builder/data_points/encounter.py (1)
58-65: ⚡ Quick winRemove the duplicate
ENCOUNTER_CLASS_DISPLAYdeclaration.This repeats the constant defined above and creates two sources of truth in the same module. Keeping just one avoids quiet drift later.
Proposed cleanup
-ENCOUNTER_CLASS_DISPLAY = { - "imp": "Inpatient", - "amb": "Outpatient", - "obsenc": "Observation", - "emer": "Emergency", - "vr": "Virtual", - "hh": "Home Health", -}As per coding guidelines: "Prioritize readability and maintainability."
🤖 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 58 - 65, There are two identical declarations of the constant ENCOUNTER_CLASS_DISPLAY in this module; remove the duplicate declaration so only a single ENCOUNTER_CLASS_DISPLAY exists (keep the original definition and delete the later one), and run a quick grep/IDE search for ENCOUNTER_CLASS_DISPLAY to ensure no code relies on the removed duplicate and that all references resolve to the single remaining constant.
🤖 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 299-303: PatientFacilityIdentifiersContextBuilder.get_context
currently indexes parent_context.facility_identifiers with
str(parent_context.facility.id) which can raise KeyError for encounters lacking
that facility entry; change the lookup to use a safe getter (e.g.,
parent_context.facility_identifiers.get(key, {})) so it returns an empty
identifier set instead of raising. Specifically, in
PatientFacilityIdentifiersContextBuilder.get_context replace the direct indexing
of parent_context.facility_identifiers[str(self.parent_context.facility.id)]
with a .get call (using the same str(self.parent_context.facility.id) key) and
return a suitable empty structure (empty dict or the expected empty collection)
so callers of get_context still receive the correct type.
- Around line 257-297: get_context() can return None causing the Field mapping
lambdas (re_admission, admit_source, discharge_disposition, diet_preference) to
call .get on None; make the context null-safe by returning an empty dict when
hospitalization is missing (i.e., change
EncounterHospitalizationContextBuilder.get_context to return
self.parent_context.hospitalization or {}), so existing mapping lambdas can
continue using h.get(...) without raising AttributeError.
---
Outside diff comments:
In `@care/emr/reports/context_builder/data_points/encounter.py`:
- Around line 306-379: Add a Field on the EncounterReportContext named e.g.
hospitalization that targets EncounterHospitalizationContextBuilder so the
hospitalization datapoints are exposed; specifically, in the
EncounterReportContext class (where other Fields like patient, facility,
diagnoses are declared) add a Field with
target_context=EncounterHospitalizationContextBuilder, a meaningful display like
"Hospitalization Details", a preview_value (e.g. ""), and a description such as
"Hospitalization details for the encounter" so the
EncounterHospitalizationContextBuilder is wired into the encounter report
context.
---
Nitpick comments:
In `@care/emr/reports/context_builder/data_points/encounter.py`:
- Around line 58-65: There are two identical declarations of the constant
ENCOUNTER_CLASS_DISPLAY in this module; remove the duplicate declaration so only
a single ENCOUNTER_CLASS_DISPLAY exists (keep the original definition and delete
the later one), and run a quick grep/IDE search for ENCOUNTER_CLASS_DISPLAY to
ensure no code relies on the removed duplicate and that all references resolve
to the single remaining constant.
🪄 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: 0bf9ad2e-ba9d-4f4c-bfbd-b3b2cb69fcf6
📒 Files selected for processing (2)
care/emr/reports/context_builder/data_points/encounter.pycare/emr/reports/context_builder/data_points/patient.py
💤 Files with no reviewable changes (1)
- care/emr/reports/context_builder/data_points/patient.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3662 +/- ##
===========================================
+ Coverage 75.83% 75.84% +0.01%
===========================================
Files 479 479
Lines 23014 23034 +20
Branches 2378 2378
===========================================
+ Hits 17452 17470 +18
- Misses 4988 4990 +2
Partials 574 574 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
care/emr/reports/context_builder/data_points/encounter.py (1)
177-179:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStill needs null-safety fix for hospitalization context.
As previously discussed,
get_context()may returnNonewhenhospitalizationis not set on the encounter, which will cause all field mappings (lines 183, 190, 200, 210) to raiseAttributeErrorwhen attempting to call.get(...)onNone.🛡️ Proposed fix (as suggested in previous review)
class EncounterHospitalizationContextBuilder(SingleObjectContextBuilder): def get_context(self): - return self.parent_context.hospitalization + return self.parent_context.hospitalization or {}🤖 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 177 - 179, EncounterHospitalizationContextBuilder.get_context currently returns self.parent_context.hospitalization which can be None and later causes AttributeError when downstream field mappings call .get(...); change get_context to return a safe empty mapping when hospitalization is unset (e.g., return self.parent_context.hospitalization or {}) so all consumers expecting a dict can call .get without crashing.
🤖 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.
Duplicate comments:
In `@care/emr/reports/context_builder/data_points/encounter.py`:
- Around line 177-179: EncounterHospitalizationContextBuilder.get_context
currently returns self.parent_context.hospitalization which can be None and
later causes AttributeError when downstream field mappings call .get(...);
change get_context to return a safe empty mapping when hospitalization is unset
(e.g., return self.parent_context.hospitalization or {}) so all consumers
expecting a dict can call .get without crashing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d6c49374-72cd-4b80-b46e-6a7e6bcd1a95
📒 Files selected for processing (1)
care/emr/reports/context_builder/data_points/encounter.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
care/emr/reports/context_builder/data_points/encounter.py (1)
185-224:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe null-safety concern from the previous review remains unaddressed.
As discussed earlier,
get_context()can returnNonewhenhospitalizationis absent, causingAttributeErrorwhen the field mappings attempt to call.get(...)onNone. There_admissionfield at line 191 is particularly vulnerable since it has no guard at all.The cleanest fix is still to make
get_context()null-safe by returning an empty dict whenhospitalizationis missing, which would protect all four field mappings in one place.🛡️ Suggested null-safe implementation
def get_context(self): - return self.parent_context.hospitalization + return self.parent_context.hospitalization or {}🤖 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 185 - 224, The get_context method in EncounterHospitalizationContextBuilder can return None causing AttributeError in field mappings (re_admission, admit_source, discharge_disposition, diet_preference); modify get_context to return an empty dict when self.parent_context.hospitalization is falsy (e.g., None) so all mapping lambdas can safely call .get(...) without additional guards.
🤖 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.
Duplicate comments:
In `@care/emr/reports/context_builder/data_points/encounter.py`:
- Around line 185-224: The get_context method in
EncounterHospitalizationContextBuilder can return None causing AttributeError in
field mappings (re_admission, admit_source, discharge_disposition,
diet_preference); modify get_context to return an empty dict when
self.parent_context.hospitalization is falsy (e.g., None) so all mapping lambdas
can safely call .get(...) without additional guards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4c4da2b4-8932-4cd4-b563-256508e219f1
📒 Files selected for processing (1)
care/emr/reports/context_builder/data_points/encounter.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
care/emr/reports/context_builder/data_points/encounter.py (2)
107-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNull-safe facility tag lookup is still missing.
This still assumes
facility_tagsis always a dict. With nullable JSON data, this will crash in production a bit too eagerly.Suggested fix
class EncounterPatientFacilityTagContextBuilder(PatientTagContextBuilder): @@ def get_context(self): + facility_tags = self.parent_context.patient.facility_tags or {} return TagConfig.objects.filter( - id__in=self.parent_context.patient.facility_tags.get( - str(self.parent_context.facility.id), [] - ) + id__in=facility_tags.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 107 - 112, The get_context method assumes parent_context.patient.facility_tags is a dict and will crash if it's null; make the lookup null-safe by coercing facility_tags to a dict (e.g., use a fallback {} or [] before calling .get) and then pass the resulting list to TagConfig.objects.filter(id__in=...), referencing the get_context method, parent_context.patient.facility_tags and parent_context.facility.id to locate the change and ensure id__in receives an iterable even when facility_tags is None or not a dict.
324-327:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
facility_identifiersagainstNonebefore.get.Same pattern here: nullable JSON means
.get(...)can raiseAttributeErrorfor older/partial records.Suggested fix
class PatientFacilityIdentifiersContextBuilder(IdentifiersContextBuilder): def get_context(self): - return self.parent_context.patient.facility_identifiers.get( - str(self.parent_context.facility.id), [] - ) + facility_identifiers = self.parent_context.patient.facility_identifiers or {} + return facility_identifiers.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 324 - 327, The get_context method calls self.parent_context.patient.facility_identifiers.get(...) which can raise AttributeError if facility_identifiers is None; change get_context to first guard facility_identifiers (e.g. assign fid = self.parent_context.patient.facility_identifiers or {} / check if truthy) and then call fid.get(str(self.parent_context.facility.id), []) so it returns [] for missing/nullable JSON instead of erroring; update the method around get_context and references to facility_identifiers accordingly.
🤖 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.
Outside diff comments:
In `@care/emr/reports/context_builder/data_points/encounter.py`:
- Around line 107-112: The get_context method assumes
parent_context.patient.facility_tags is a dict and will crash if it's null; make
the lookup null-safe by coercing facility_tags to a dict (e.g., use a fallback
{} or [] before calling .get) and then pass the resulting list to
TagConfig.objects.filter(id__in=...), referencing the get_context method,
parent_context.patient.facility_tags and parent_context.facility.id to locate
the change and ensure id__in receives an iterable even when facility_tags is
None or not a dict.
- Around line 324-327: The get_context method calls
self.parent_context.patient.facility_identifiers.get(...) which can raise
AttributeError if facility_identifiers is None; change get_context to first
guard facility_identifiers (e.g. assign fid =
self.parent_context.patient.facility_identifiers or {} / check if truthy) and
then call fid.get(str(self.parent_context.facility.id), []) so it returns [] for
missing/nullable JSON instead of erroring; update the method around get_context
and references to facility_identifiers accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e27ffbdc-7e99-4152-8602-2a1aa230e371
📒 Files selected for processing (1)
care/emr/reports/context_builder/data_points/encounter.py
vigneshhari
deleted the
ENG-378-add-support-for-the-missing-encounter-related-fields-in-report-builder
branch
1 participant
Associated Issue
ENG-378
Summary by CodeRabbit
New Features
Changes