[ENG-513] Add missing fields to datapoints for template support#3677
Open
rithviknishad wants to merge 4 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds display lookup dictionaries and new mapped fields across allergy_intolerance, diagnosis, diagnostic_report, medication, service_request, and symptom context builders; introduces Queryset-based tag sub-contexts for medication prescriptions and service requests and standardizes empty-string fallbacks. ChangesEMR Context Builder Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
Greptile SummaryThis PR exposes additional data-point fields across six context builders to support template rendering:
Confidence Score: 3/5Three of the six changed files contain correctness bugs that will produce wrong or crashing behavior in production; the remaining three files look correct. The care/emr/reports/context_builder/data_points/diagnosis.py, care/emr/reports/context_builder/data_points/symptom.py, and care/emr/reports/context_builder/data_points/diagnostic_report.py need fixes before merging. Important Files Changed
Reviews (1): Last reviewed commit: "ENG-513 Add all other missing fields to ..." | Re-trigger Greptile |
![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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
care/emr/reports/context_builder/data_points/medication.py (1)
189-189: ⚡ Quick winUse an immutable backend container to satisfy Ruff RUF012.
Line 189 and Line 216 use a mutable class attribute list for
__filterset_backends__; Ruff is (predictably) unhappy here. Switch both to tuples.As per coding guidelines, "`**/*.py`: Use Ruff for linting and formatting Python code (replaces black, isort, flake8)".Proposed diff
class MedicationPrescriptionTagContextBuilder(QuerysetContextBuilder): filterset_class = TagFilter - __filterset_backends__ = [filters.DjangoFilterBackend] + __filterset_backends__ = (filters.DjangoFilterBackend,) @@ class MedicationPrescriptionContextBuilder(QuerysetContextBuilder): filterset_class = MedicationPrescriptionReportFilter - __filterset_backends__ = [filters.DjangoFilterBackend] + __filterset_backends__ = (filters.DjangoFilterBackend,)Also applies to: 216-216
🤖 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/medication.py` at line 189, The class attribute __filterset_backends__ is currently a mutable list (e.g., __filterset_backends__ = [filters.DjangoFilterBackend]) which triggers Ruff RUF012; change both occurrences to use an immutable tuple instead (e.g., __filterset_backends__ = (filters.DjangoFilterBackend,)) so the attribute is immutable—update the two spots where __filterset_backends__ is defined in medication.py 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.
Inline comments:
In `@care/emr/reports/context_builder/data_points/diagnosis.py`:
- Around line 62-66: The severity Field mapping can raise AttributeError when
Condition.severity is None because the default expression c.severity.title() is
evaluated eagerly; update the mapping lambda for severity in diagnosis.py (and
similarly in symptom.py) to guard against null/empty values like the
clinical_status / verification_status pattern: check if c.severity is truthy
first, use SEVERITY_DISPLAY.get(c.severity, c.severity.title()) when present,
otherwise return None or an appropriate empty string; reference symbols: the
severity Field, mapping lambda, SEVERITY_DISPLAY, and Condition.severity.
In `@care/emr/reports/context_builder/data_points/service_request.py`:
- Line 119: Replace the misspelled display label "Occurance" with the correct
spelling "Occurrence" in the service request data point definition; locate the
display="Occurance" occurrence in
care/emr/reports/context_builder/data_points/service_request.py (the data point
or dict where display is set to "Occurance") and update it to
display="Occurrence" so templates show the correct word.
- Around line 58-60: get_context currently returns
TagConfig.objects.filter(id__in=self.parent_context.tags) which can leak tags
across tenants; change get_context to include the tenant/facility/org
discriminator from parent_context in the filter (e.g. add
facility_id=self.parent_context.facility_id or
organization_id=self.parent_context.organization_id or use a
parent_context-provided tenant filter method) so the query becomes a filtered
lookup by both id__in=self.parent_context.tags and the same tenant scope used
elsewhere in the app (refer to get_context, TagConfig.objects.filter and
self.parent_context.tags to locate where to add the extra filter).
- Line 49: Replace the mutable list assigned to __filterset_backends__ with an
immutable tuple in both classes: change
ServiceRequestTagContextBuilder.__filterset_backends__ and
ServiceRequestBaseContextBuilder.__filterset_backends__ from
[filters.DjangoFilterBackend] to (filters.DjangoFilterBackend,) so Ruff RUF012
is satisfied; ensure the trailing comma is present to create a single-element
tuple.
In `@care/emr/reports/context_builder/data_points/symptom.py`:
- Around line 63-67: The severity Field's mapping currently calls
c.severity.title() without null-check, which can raise AttributeError if
c.severity is None; update the mapping lambda used in the severity Field to
guard against missing values (mirror the pattern used by the other status
fields) by returning a default/empty string or using c.severity.title() only
when c.severity is truthy, and otherwise falling back to the SEVERITY_DISPLAY
lookup or a safe default; modify the mapping lambda and keep references to
SEVERITY_DISPLAY and the severity Field to locate the change.
---
Nitpick comments:
In `@care/emr/reports/context_builder/data_points/medication.py`:
- Line 189: The class attribute __filterset_backends__ is currently a mutable
list (e.g., __filterset_backends__ = [filters.DjangoFilterBackend]) which
triggers Ruff RUF012; change both occurrences to use an immutable tuple instead
(e.g., __filterset_backends__ = (filters.DjangoFilterBackend,)) so the attribute
is immutable—update the two spots where __filterset_backends__ is defined in
medication.py accordingly.
🪄 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: 0d777b4d-0f1e-40f7-91bf-94a5542e3657
📒 Files selected for processing (6)
care/emr/reports/context_builder/data_points/allergy_intolerance.pycare/emr/reports/context_builder/data_points/diagnosis.pycare/emr/reports/context_builder/data_points/diagnostic_report.pycare/emr/reports/context_builder/data_points/medication.pycare/emr/reports/context_builder/data_points/service_request.pycare/emr/reports/context_builder/data_points/symptom.py
Member
|
@rithviknishad i see a lot of coderabbit comments open, please resolve them with valid reasoning |
Member
|
@nandkishorr to review |
…atapoints-for-template-support
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/filters/__init__.py`:
- Line 1: Replace the wildcard re-export in __init__.py by importing and
exporting TagFilter explicitly: change the import from .tag_filter to import
TagFilter (from .tag_filter import TagFilter) and add an explicit __all__ =
["TagFilter"] so downstream imports continue to work and Ruff F403 is resolved;
locate the file's existing wildcard import and update it to reference the
TagFilter symbol from tag_filter.
🪄 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: 586df43d-26a8-439e-9ff1-88e00085fc64
📒 Files selected for processing (6)
care/emr/reports/context_builder/data_points/encounter.pycare/emr/reports/context_builder/data_points/medication.pycare/emr/reports/context_builder/data_points/patient.pycare/emr/reports/context_builder/data_points/service_request.pycare/emr/reports/context_builder/filters/__init__.pycare/emr/reports/context_builder/filters/tag_filter.py
✅ Files skipped from review due to trivial changes (1)
- care/emr/reports/context_builder/data_points/encounter.py
🚧 Files skipped from review as they are similar to previous changes (2)
- care/emr/reports/context_builder/data_points/medication.py
- care/emr/reports/context_builder/data_points/service_request.py
| @@ -0,0 +1 @@ | |||
| from .tag_filter import * | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm no wildcard re-export remains
rg -n 'from \.tag_filter import \*' care/emr/reports/context_builder/filters/__init__.py || true
# 2) Confirm explicit export exists
rg -n 'from \.tag_filter import TagFilter|__all__\s*=\s*\["TagFilter"\]' care/emr/reports/context_builder/filters/__init__.py
# 3) Re-run Ruff on the touched package
ruff check care/emr/reports/context_builder/filters/__init__.py care/emr/reports/context_builder/filters/tag_filter.pyRepository: ohcnetwork/care
Length of output: 87
Replace the wildcard re-export in care/emr/reports/context_builder/filters/__init__.py (Ruff F403 / CI blocker)
__init__.py still has from .tag_filter import * (Ruff F403), so lint is still failing. Export TagFilter explicitly so downstream imports keep working without tripping Ruff.
Suggested fix
-from .tag_filter import *
+from .tag_filter import TagFilter
+
+__all__ = ["TagFilter"]🧰 Tools
🪛 GitHub Actions: Lint Code Base / 0_lint.txt
[error] 1-1: pre-commit hook 'ruff-check' failed (exit code 1). Ruff reported F403: 'from .tag_filter import * used; unable to detect undefined names'.
🪛 GitHub Actions: Lint Code Base / lint
[error] 1-1: Ruff (ruff-check) failed: F403 'from .tag_filter import * used; unable to detect undefined names'.
🪛 Ruff (0.15.15)
[error] 1-1: from .tag_filter import * used; unable to detect undefined names
(F403)
🤖 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/filters/__init__.py` at line 1, Replace the
wildcard re-export in __init__.py by importing and exporting TagFilter
explicitly: change the import from .tag_filter to import TagFilter (from
.tag_filter import TagFilter) and add an explicit __all__ = ["TagFilter"] so
downstream imports continue to work and Ruff F403 is resolved; locate the file's
existing wildcard import and update it to reference the TagFilter symbol from
tag_filter.
Sources: Coding guidelines, Linters/SAST tools, Pipeline failures
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3677 +/- ##
===========================================
+ Coverage 76.69% 76.72% +0.02%
===========================================
Files 479 481 +2
Lines 22992 23030 +38
Branches 2379 2379
===========================================
+ Hits 17634 17670 +36
- Misses 4804 4806 +2
Partials 554 554 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Adds the following fields to datapoints for template support:
encounter -> diagnostic_report.statusencounter -> diagnostic_report.category (ValueSet)encounter -> symptoms.severityencounter -> allergy_intolerance.categoryencounter -> allergy_intolerance.allergy_intolerance_typeencounter -> diagnosis.severityencounter -> medication_prescriptions.nameencounter -> medication_prescriptions.tagsencounter -> service_request.noteencounter -> service_request.occuranceencounter -> service_request.patient_instructionencounter -> service_request.tagsSummary by CodeRabbit
New Features
Improvements