Update allergy_intolerance update spec and added tests #2778
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant refinements to the Allergy Intolerance management system, focusing on enhancing authorization, data validation, and specification models. The changes primarily target the Changes
Sequence DiagramsequenceDiagram
participant User
participant AllergyIntoleranceViewSet
participant EncounterModel
participant AuthorizationService
User->>AllergyIntoleranceViewSet: Request Update
AllergyIntoleranceViewSet->>EncounterModel: Retrieve Encounter
EncounterModel-->>AllergyIntoleranceViewSet: Encounter Details
AllergyIntoleranceViewSet->>AuthorizationService: Check Permissions
AuthorizationService-->>AllergyIntoleranceViewSet: Permission Result
alt Permission Granted
AllergyIntoleranceViewSet->>AllergyIntoleranceViewSet: Update Record
else Permission Denied
AllergyIntoleranceViewSet-->>User: 403 Forbidden
end
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
care/emr/api/viewsets/allergy_intolerance.py (2)
69-77: Granular update authorization introduced.
Theauthorize_updatemethod now properly checks if the user can update the relatedEncounter. Might be worth verifying that mismatched patient IDs are also disallowed at this level, if that’s not enforced elsewhere.
78-80: Selective field preservation.
Retaining only theencounterfield inclean_update_datais sensible, but verify there's no unintentional data loss if you plan to allow partial updates to other fields in the future.care/emr/resources/allergy_intolerance/spec.py (2)
76-82: Renaming and reorganizing write spec fields.
RenamingAllergyIntoleranceSpectoAllergyIntoleranceWriteSpecis a logical step. The structure is more coherent, though watch out for any missing fields if future expansions are planned.
104-107: Settingencounterandpatienton deserialization.
The approach directly pulls the encounter from the DB, reducing guesswork. Just confirm that no additional data validation is needed for partial updates.care/emr/tests/test_allergy_intolerance_api.py (4)
22-46: Comprehensive setup.
Creating test data and patchingvalidate_valuesethelps keep tests isolated. This is well done, though you might eventually consider a fixture-based approach for even cleaner code organization.
102-122: Test: list with valid permissions.
Properly verifying a200response forcan_view_clinical_data. It's thorough, though verifying the response body might catch data regressions.
156-174: Test: list single encounter with permissions.
Filtering by?encounter=is validated. Possibly check if only that encounter’s results appear in the response.
446-469: Test: retrieve with permissions.
Checks a successful read scenario. Possibly reaffirm the entire payload is correct in future expansions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/api/viewsets/allergy_intolerance.py(4 hunks)care/emr/resources/allergy_intolerance/spec.py(3 hunks)care/emr/tests/test_allergy_intolerance_api.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (36)
care/emr/api/viewsets/allergy_intolerance.py (4)
18-18: Good import addition.
ImportingEncounteris necessary for the new update logic.
23-24: New spec imports look consistent.
Renaming these specs aligns well with the updated naming convention. Nice job maintaining clarity here.
36-36: Accurate schema assignment for create.
UsingAllergyIntoleranceWriteSpecensures the correct data structure is validated upon creation.
48-50: Consistent model references.
Assigningpydantic_update_modeltoAllergyIntoleranceUpdateSpecclarifies how partial updates differ from writes. Just make sure no references to the old classes remain in other parts of the code.✅ Verification successful
Everything looks perfectly aligned with the new spec classes
I've thoroughly checked, and there are no lingering references to any old spec classes anywhere in the codebase. The transition appears to be complete, not that anyone was worried.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checking references to old spec classes rg -A 3 "AllergyIntoleranceSpec"Length of output: 1487
Script:
#!/bin/bash # Broader search for any allergy intolerance related classes rg -i "allergy.*intolerance.*spec|allergyintolerance.*spec"Length of output: 1828
care/emr/resources/allergy_intolerance/spec.py (6)
55-55: NewAllergyIntoleranceUpdateSpecclass recognized.
This separation clarifies the distinction between creating vs updating. Looks good.
60-61: Non-mandatory note field.
Allowingnoteto be optional nicely accommodates situations with no comments. No issues here.
63-69: Encounter validator helps prevent invalid references.
Raising aValueErrorensures immediate feedback if the provided encounter doesn’t exist—this is a good move.
72-72: Deserialization step for encounter.
This ensures the model object is correctly linked to the encounter. Just double-check concurrency handling if multiple processes might update the same record simultaneously.
87-87: Default onset object is fine.
Providing a default dictionary foronsetcan help avoid KeyError checks. No concerns here.
134-135: Readingencounterexternal ID on serialization.
Includingencounterin the output fosters clarity. Looks neat.care/emr/tests/test_allergy_intolerance_api.py (26)
47-49: Graceful teardown.
Stopping the patcher ensures no side effects leak into other tests. Minimal but necessary cleanup.
50-59: Detail URL helper.
A neat utility to clarify test code. Approved for readability.
60-80: Creation helper.
baker.makeusage is flexible and keeps test logic DRY. The random status choices are a tiny bit unpredictable but fine for coverage.
81-100: Factory for request data.
Generating valid POST bodies ensures consistency across tests. Good approach.
123-141: Test: list with completed encounter.
Ensuring403when the encounter is completed is correct. A prime example of negative testing done right.
142-155: Test: list with no permissions.
Good job confirming unauthorized access fails with403. Straightforward coverage.
175-194: Test: single encounter with completed status.
Returns403for a completed record. Preserves data integrity. Clear test scenario.
195-209: Test: list single encounter without permissions.
Again, neat negative test ensuring no leaks of info. Consistency is good.
211-230: Test: create without permissions.
Enforces403as expected. Nicely ensures non-privileged users can’t create data.
231-264: Test: create with different facility organization.
Smartly checks mismatch scenarios. The code does a thorough job rejecting unauthorized facility access.
265-305: Test: creation with organization user.
Verifies a user from a certain org can indeed create data for a patient. The test is quite robust, ensuring correct cross-organization checks.
306-332: Test: create with permissions on open encounter.
200indicates success. Checking the returned fields for correctness is a nice detail.
333-357: Test: create with permissions on completed encounter.
Proves the user is blocked with403. This matches real-world workflows where completed encounters shouldn’t be altered.
358-385: Test: create with missing facility association.
This scenario ensures the user can’t create entries in unassociated facilities. Good, strict enforcement.
386-413: Test: create with mismatched patient ID.
Returning403is correct. The test ensures no accidental cross-patient data manipulations.
414-444: Test: create with invalid encounter ID.
400error is the correct response, verifying the new validator checks. Good clarity in the returned error message.
470-495: Test: retrieve single encounter with permissions.
Ensures a parameter-based retrieval also respects the user’s access. Nicely consistent with the rest of the logic.
496-515: Test: retrieve single encounter without permissions.
403is correct. Continues the pattern of robust permission checks.
516-533: Test: retrieve without relevant permissions.
Even if the user can write, they can’t view. Rightfully returns403.
535-566: Test: update with all permissions.
can_write_encounter,can_write_patient, andcan_view_clinical_dataproduce a200. Confirming updated fields is a good practice.
567-602: Test: update single encounter with permissions.
Again, returning200confirms success. The additional param?encounter=helps replicate real usage scenarios.
603-631: Test: update single encounter lackingcan_view_clinical_data.
403properly enforces read-level permission. This consistency helps avoid data leaks.
632-660: Test: update without relevant permissions.
Solely havingcan_write_patientisn’t enough to read or update data. Good negative scenario coverage.
661-691: Test: update closed encounter.
Completed encounters are immutable.403is consistent with the domain policy.
692-737: Test: changing encounter ID with proper permissions.
Confirms the user can reassign an allergy intolerance to another active encounter. This is a strong multi-encounter use case test.
738-778: Test: changing encounter ID without new encounter permission.
Denies the update with403. Crisp conclusion that roles and ownership are strictly enforced.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2778 +/- ##
===========================================
- Coverage 56.69% 56.69% -0.01%
===========================================
Files 208 208
Lines 9547 9569 +22
Branches 939 942 +3
===========================================
+ Hits 5413 5425 +12
- Misses 4118 4128 +10
Partials 16 16 ☔ View full report in Codecov by Sentry. |
Proposed Changes
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
New Features
Bug Fixes
Tests