fix medication statement and medication request #2777
Conversation
📝 WalkthroughWalkthroughThe pull request introduces updates to medication-related specifications across multiple files in the care/emr directory. The changes primarily focus on enhancing the structure of medication statement and request specifications by reintroducing the Changes
Sequence DiagramsequenceDiagram
participant Client
participant MedicationStatementViewSet
participant MedicationStatementUpdateSpec
participant Database
Client->>MedicationStatementViewSet: Request Update
MedicationStatementViewSet->>MedicationStatementUpdateSpec: Validate Update
MedicationStatementUpdateSpec->>Database: Validate Encounter
MedicationStatementUpdateSpec->>Database: Validate Medication
Database-->>MedicationStatementUpdateSpec: Validation Result
MedicationStatementUpdateSpec-->>MedicationStatementViewSet: Validated Data
MedicationStatementViewSet-->>Client: Update Response
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
care/emr/resources/medication/statement/spec.py (1)
82-91: Thoughtful addition of MedicationStatementUpdateSpec.Implementing a dedicated update spec is a clean way to validate medication statements during updates. You might eventually unify these patterns across your medication resources to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/api/viewsets/medication_statement.py(2 hunks)care/emr/resources/medication/request/spec.py(2 hunks)care/emr/resources/medication/statement/spec.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
care/emr/api/viewsets/medication_statement.py (2)
12-12: Good import addition.Adding
MedicationStatementUpdateSpecimport aligns well with your update flow.
27-27: Verify usage of the newly assigned update model.Setting
pydantic_update_model = MedicationStatementUpdateSpecis a strong improvement. It might be worth running a quick check to ensure all update calls are in sync with this new spec.✅ Verification successful
Everything looks perfectly fine here, but you probably knew that already
The
MedicationStatementUpdateSpecis properly configured in the viewset and follows the standard EMR pattern withEMRModelViewSet. The update operations will automatically use this spec for validation, just as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references to the update model within medication statement code. rg 'MedicationStatementUpdateSpec' -A 5Length of output: 1579
Script:
#!/bin/bash # Search for update operations and PUT/PATCH endpoints ast-grep --pattern 'def update($$$) { $$$ }' rg -B 2 -A 2 'PUT|PATCH' care/emr/api/viewsets/medication_statement.pyLength of output: 119
Script:
#!/bin/bash # Check the class hierarchy and router configurations ast-grep --pattern 'class MedicationStatement$$$ViewSet($$$) { $$$ }' rg -B 2 'class MedicationStatement.*ViewSet'Length of output: 292
Script:
#!/bin/bash # Get the complete class definition with context rg -A 10 'class MedicationStatementViewSet' care/emr/api/viewsets/medication_statement.pyLength of output: 621
care/emr/resources/medication/request/spec.py (2)
231-231: Reintroducing encounter.Re-introducing
encounter: UUID4inMedicationRequestSpecis consistent with the specifications you've outlined. Small caution: watch out for potential null or missing encounters in certain scenarios.
257-257: Unified inheritance approach for updates.The shift to
class MedicationRequestUpdateSpec(BaseMedicationRequestSpec):is a tidy choice, helping to keep request specs consistent. A quick integration test might be beneficial to confirm no subtle breaks.✅ Verification successful
Oh look, a perfectly reasonable inheritance change
The inheritance structure is consistent with other specs in the file, and the update spec maintains its focused responsibility of handling status updates. I mean, it would have been concerning if it didn't, but thankfully it does.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for usage of MedicationRequestUpdateSpec rg 'MedicationRequestUpdateSpec' -A 5Length of output: 1543
Script:
#!/bin/bash # Check the base class implementation and other similar specs ast-grep --pattern 'class BaseMedicationRequestSpec { $$$ }' # Look for test files rg -g '*test*.py' 'MedicationRequestUpdateSpec|BaseMedicationRequestSpec' -A 3Length of output: 145
Script:
#!/bin/bash # Check for base class definition and show context rg "class BaseMedicationRequestSpec" -A 10 # Show the entire spec file to understand the structure cat care/emr/resources/medication/request/spec.pyLength of output: 8339
care/emr/resources/medication/statement/spec.py (1)
55-56: Encounter field reintroduction.Adding
encounter: UUID4here ensures the statement is properly linked to its encounter. Although this does duplicate logic from your medication request specs, it's still acceptable.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2777 +/- ##
===========================================
+ Coverage 64.90% 65.01% +0.10%
===========================================
Files 252 252
Lines 12761 12767 +6
Branches 1121 1121
===========================================
+ Hits 8283 8300 +17
+ Misses 4369 4355 -14
- Partials 109 112 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
care/emr/tests/test_medication_statement.py (3)
11-43: Consider usingsetUpTestDatafor performance gains.
WhilesetUpis acceptable, switching tosetUpTestDatacan speed up repeated test runs by creating shared objects once—assuming test isolation is preserved. But if you fancy more runs, that's okay too.
101-123: Use HTTP 201 for creation responses.
A small best practice: resource creation typically returns HTTP 201. If the API is indeed returning 200, that’s acceptable but slightly unconventional. Just ensure consistency across endpoints to avoid confusion.
124-158: Consider including partial updates (HTTP PATCH).
Your coverage of full updates is thorough. Having a test for partial updates would be beneficial, especially if your view supports PATCH. By the look of things here, one can only guess if partial updates are relevant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/tests/test_medication_statement.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
care/emr/tests/test_medication_statement.py (5)
1-2: Good usage ofpatchfor mocking.
The imports forpatchfromunittest.mockare perfectly fine here and help isolate external dependencies in tests.
6-8: Commendable permission checks import.
ImportingEncounterPermissionsandPatientPermissionskeeps the tests consistent with the security model.
44-52: Helper URL method is well-structured.
The_get_medication_statement_urlhelper keeps the code DRY and easy to follow. Kudos.
53-80: Data factories are well-handled.
Your approach to generatingMedicationStatementdata withcreate_medication_statementandget_medication_statement_datais neat and ensures test clarity.
81-100: Nice distinction between permission/no-permission in list tests.
Good job covering both success (can_view_clinical_data) and forbidden scenarios, ensuring robust test coverage for the read API.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests