feat:added service request datapoint#3592
Open
nandkishorr wants to merge 4 commits intodevelopfrom
Open
Conversation
nandkishorr
changed the title
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a ServiceRequest context builder and exposes service requests on EncounterReportContext via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 adds a Confidence Score: 2/5Not safe to merge — the class name mismatch causes an ImportError that breaks all encounter reports at module load time. The PR introduces a single, easily-fixed P0 bug (wrong class name) that prevents the entire encounter reports module from loading. The fix is a one-line rename, but until that is addressed the feature is completely non-functional. care/emr/reports/context_builder/data_points/service_request.py — class must be renamed to Important Files Changed
Reviews (1): Last reviewed commit: "feat:added service request datapoints" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+86
to
+93
| priority = Field( | ||
| display="Priority", | ||
| preview_value="Routine", | ||
| mapping=lambda sr: PRIORITY_CHOICE.get(sr.priority, sr.priority.title()) | ||
| if sr.priority | ||
| else "", | ||
| description="Priority level of the service request", | ||
| ) |
There was a problem hiding this comment.
note field not exposed
The ServiceRequest model has a note = models.TextField(null=True, blank=True) field, but it is not included in the context builder. Comparable builders (e.g. SymptomsContextBuilder) expose their note field. Consider adding it for completeness:
note = Field(
display="Note",
preview_value="",
description="Additional notes about the service request",
)Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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: 1
🧹 Nitpick comments (1)
care/emr/reports/context_builder/data_points/service_request.py (1)
54-54: Convert mutable list to tuple for configuration immutability.Line 54 uses a mutable list for
__filterset_backends__, which works fine—no mutations are happening anywhere in the codebase, but using a tuple is more idiomatic for static configuration and prevents accidental modification later.Suggested fix
- __filterset_backends__ = [filters.DjangoFilterBackend] + __filterset_backends__ = (filters.DjangoFilterBackend,)Note: This pattern appears across multiple context builder files (diagnosis.py, symptom.py, medication.py, etc.); consider applying the same change consistently throughout for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/reports/context_builder/data_points/service_request.py` at line 54, The configuration attribute __filterset_backends__ is defined as a mutable list; change it to an immutable tuple containing filters.DjangoFilterBackend (i.e., replace the list literal with a tuple literal) to prevent accidental mutation and follow idiomatic static config; do the same replacement for the same attribute in the related context builder modules (diagnosis.py, symptom.py, medication.py, etc.) to keep consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@care/emr/reports/context_builder/data_points/encounter.py`:
- Around line 29-31: The import and usages reference
ServiceRequestDataPointBuilder which doesn't exist; update the import to bring
in ServiceRequestBaseContextBuilder (from
care.emr.reports.context_builder.data_points.service_request) and then rename
all usages/references of ServiceRequestDataPointBuilder to
ServiceRequestBaseContextBuilder (e.g., in the encounter context builder where
it is referenced around the previous ServiceRequestDataPointBuilder mentions) so
the module can import and run without runtime failure.
---
Nitpick comments:
In `@care/emr/reports/context_builder/data_points/service_request.py`:
- Line 54: The configuration attribute __filterset_backends__ is defined as a
mutable list; change it to an immutable tuple containing
filters.DjangoFilterBackend (i.e., replace the list literal with a tuple
literal) to prevent accidental mutation and follow idiomatic static config; do
the same replacement for the same attribute in the related context builder
modules (diagnosis.py, symptom.py, medication.py, etc.) to keep consistency.
🪄 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: 021179a2-d008-42e3-adcc-523f09de7b7f
📒 Files selected for processing (2)
care/emr/reports/context_builder/data_points/encounter.pycare/emr/reports/context_builder/data_points/service_request.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
care/emr/reports/context_builder/data_points/encounter.py (1)
216-221:⚠️ Potential issue | 🔴 CriticalThe import says one thing, the usage says another.
Line 30 imports
ServiceRequestBaseContextBuilder, but line 218 referencesServiceRequestDataPointBuilderwhich... doesn't exist. The pipeline is kindly letting you know about this as well. This will fail at import time.🐛 Proposed fix
service_requests = Field( display="Service Requests", - target_context=ServiceRequestDataPointBuilder, + target_context=ServiceRequestBaseContextBuilder, preview_value="", description="Service requests associated with the encounter", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/reports/context_builder/data_points/encounter.py` around lines 216 - 221, The Field declaration uses an undefined symbol ServiceRequestDataPointBuilder while the file imports ServiceRequestBaseContextBuilder; fix by making the symbols consistent: either change the Field's target_context to ServiceRequestBaseContextBuilder or import/define ServiceRequestDataPointBuilder (so the name used in the service_requests Field matches an actual exported class), and ensure any downstream usages expect the chosen class name.
🧹 Nitpick comments (1)
care/emr/reports/context_builder/data_points/service_request.py (1)
54-54: Consider using a tuple instead of a list for class attribute.Ruff flags this as a mutable default value (RUF012). While it probably won't cause issues in practice since this is configuration, using a tuple would be more defensive and consistent with immutable configuration patterns.
🔧 Suggested fix
- __filterset_backends__ = [filters.DjangoFilterBackend] + __filterset_backends__ = (filters.DjangoFilterBackend,)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/reports/context_builder/data_points/service_request.py` at line 54, The class attribute __filterset_backends__ is defined as a mutable list which Ruff flags (RUF012); change it to an immutable tuple by replacing the list literal with a tuple literal (e.g., (__filterset_backends__ = (filters.DjangoFilterBackend,),) referencing the __filterset_backends__ attribute in service_request.py) so configuration is immutable and the linter warning is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@care/emr/reports/context_builder/data_points/encounter.py`:
- Around line 216-221: The Field declaration uses an undefined symbol
ServiceRequestDataPointBuilder while the file imports
ServiceRequestBaseContextBuilder; fix by making the symbols consistent: either
change the Field's target_context to ServiceRequestBaseContextBuilder or
import/define ServiceRequestDataPointBuilder (so the name used in the
service_requests Field matches an actual exported class), and ensure any
downstream usages expect the chosen class name.
---
Nitpick comments:
In `@care/emr/reports/context_builder/data_points/service_request.py`:
- Line 54: The class attribute __filterset_backends__ is defined as a mutable
list which Ruff flags (RUF012); change it to an immutable tuple by replacing the
list literal with a tuple literal (e.g., (__filterset_backends__ =
(filters.DjangoFilterBackend,),) referencing the __filterset_backends__
attribute in service_request.py) so configuration is immutable and the linter
warning is resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 75e961d4-0195-4e3b-b86c-b03ad201b46e
📒 Files selected for processing (2)
care/emr/reports/context_builder/data_points/encounter.pycare/emr/reports/context_builder/data_points/service_request.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3592 +/- ##
===========================================
+ Coverage 77.20% 77.22% +0.02%
===========================================
Files 474 475 +1
Lines 22421 22448 +27
Branches 2348 2348
===========================================
+ Hits 17310 17336 +26
- Misses 4531 4532 +1
Partials 580 580 ☔ View full report in Codecov by Sentry. 🚀 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
1 participant
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.
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