[Eng 387] feat : add encounter minimal spec to medication dispense#3663
Conversation
|
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)
📝 WalkthroughWalkthroughThe medication dispense retrieval spec now includes a serialized encounter object in its response. The change adds an import for ChangesMedication Dispense Encounter Serialization
🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 makes two independent changes: it adds an
Confidence Score: 4/5Safe to merge; the diagnostic report authorization refactor is logically correct and well-tested, and the spec change is a small additive field following established patterns. Both changes are low-risk. The authorization fix correctly swaps ValidationError for PermissionDenied and the new test suite validates those 403 paths. The medication dispense spec change is a straightforward field addition consistent with how sibling specs handle nested objects. The only gap is the absence of tests for the spec change itself. care/emr/resources/medication/dispense/spec.py — the new encounter field in MedicationDispenseRetrieveSpec has no test coverage. Important Files Changed
Reviews (1): Last reviewed commit: "feat:added encounter base spec" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "system": "http://terminology.hl7.org/CodeSystem/v2-0074", | ||
| }, | ||
| "service_request": kwargs.setdefault( | ||
| "service_request", self.service_request |
There was a problem hiding this comment.
secrets.choice is the cryptographically-secure variant designed for security-sensitive selections (tokens, passwords, etc.). For generating random test data, random.choice is the conventional choice and avoids any unintended coupling to the secrets module.
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!
| class MedicationDispenseRetrieveSpec(MedicationDispenseReadSpec): | ||
| pass | ||
| encounter: dict | ||
|
|
||
| @classmethod | ||
| def perform_extra_serialization(cls, mapping, obj): | ||
| super().perform_extra_serialization(mapping, obj) | ||
| mapping["encounter"] = EncounterSpecBase.serialize(obj.encounter).to_json() |
There was a problem hiding this comment.
The primary purpose of this PR is adding the encounter field to MedicationDispenseRetrieveSpec, but the added test file (test_diagnostic_report_api.py) contains no tests for the medication dispense retrieve endpoint or for the presence/correctness of the encounter key in its response. If a future refactor breaks the serialization (e.g., obj.encounter is null, or EncounterSpecBase.serialize changes shape), there is no test to catch it.
nandkishorr
force-pushed
the
ENG-387-add-encounter-minimal-spec-to-medication-dispense
branch
from
8d1da0a to
89c9140
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
care/emr/api/viewsets/diagnostic_report.py (1)
148-158: 💤 Low valueRedundant
get_patient_obj()call and filter.Line 117 already calls
self.get_patient_obj()and filters the queryset by that patient. Here at line 149, you're calling it again (extra DB query), and the filter at line 158 is also redundant since the queryset is already patient-scoped.You could just return
querysetdirectly after the authorization check, or cache the patient from line 117.♻️ Suggested simplification
# Authorize with Patient - patient = self.get_patient_obj() if not AuthorizationController.call( "can_view_clinical_data", self.request.user, - patient, + self.get_patient_obj(), ): raise PermissionDenied( "You do not have permission to view clinical data for this patient" ) - return queryset.filter(patient=patient) + return queryset # Already filtered by patient at line 117🤖 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/api/viewsets/diagnostic_report.py` around lines 148 - 158, The code redundantly calls self.get_patient_obj() and re-filters the queryset after it was already obtained and filtered earlier; update the view to avoid the extra DB call by either reusing the already-cached patient object from the earlier call (store it in a local variable like patient_from_scope and use that for the AuthorizationController.call check) or simply perform the permission check with AuthorizationController.call("can_view_clinical_data", self.request.user, patient) and then return queryset directly instead of returning queryset.filter(patient=patient); adjust the block around get_patient_obj(), AuthorizationController.call, and the return to remove the duplicate get_patient_obj() and redundant queryset.filter(patient=patient).care/emr/tests/test_diagnostic_report_api.py (1)
1-1: 💤 Low valueUsing
secrets.choicefor non-security test data is unconventional.The
secretsmodule is intended for cryptographically secure random values. For test fixtures where reproducibility can be helpful,random.choicewould be the standard choice. Though I'm sure you had your reasons.♻️ Suggested fix
-from secrets import choice +from random import choice🤖 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/tests/test_diagnostic_report_api.py` at line 1, The test imports secrets.choice which is meant for cryptographic use; replace that import with random.choice and update any uses of choice in test_diagnostic_report_api.py (e.g., where choice(...) is called) to use the random module so tests use non-crypto RNG; optionally seed random.seed(...) near test setup for reproducible fixtures if needed.
🤖 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.
Nitpick comments:
In `@care/emr/api/viewsets/diagnostic_report.py`:
- Around line 148-158: The code redundantly calls self.get_patient_obj() and
re-filters the queryset after it was already obtained and filtered earlier;
update the view to avoid the extra DB call by either reusing the already-cached
patient object from the earlier call (store it in a local variable like
patient_from_scope and use that for the AuthorizationController.call check) or
simply perform the permission check with
AuthorizationController.call("can_view_clinical_data", self.request.user,
patient) and then return queryset directly instead of returning
queryset.filter(patient=patient); adjust the block around get_patient_obj(),
AuthorizationController.call, and the return to remove the duplicate
get_patient_obj() and redundant queryset.filter(patient=patient).
In `@care/emr/tests/test_diagnostic_report_api.py`:
- Line 1: The test imports secrets.choice which is meant for cryptographic use;
replace that import with random.choice and update any uses of choice in
test_diagnostic_report_api.py (e.g., where choice(...) is called) to use the
random module so tests use non-crypto RNG; optionally seed random.seed(...) near
test setup for reproducible fixtures if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8890c190-ea2d-4dc5-82bf-16b4fc791d8e
📒 Files selected for processing (3)
care/emr/api/viewsets/diagnostic_report.pycare/emr/resources/medication/dispense/spec.pycare/emr/tests/test_diagnostic_report_api.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3663 +/- ##
===========================================
- Coverage 75.85% 75.85% -0.01%
===========================================
Files 479 479
Lines 23034 23040 +6
Branches 2378 2378
===========================================
+ Hits 17472 17476 +4
- Misses 4989 4991 +2
Partials 573 573 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vigneshhari
deleted the
ENG-387-add-encounter-minimal-spec-to-medication-dispense
branch
2 participants
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
Release Notes