Add requester to MedicationRequest and basic authzn. tests
#2773
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
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 (
|
rithviknishad
changed the title
requester to MedicationRequest and testsrequester to MedicationRequest and basic authzn. tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
care/emr/api/viewsets/medication_request.py (1)
49-60: Align handling of 'requester' and 'encounter' objects in both methodsIn
authorize_create, you're fetchingrequesterandencounterusingget_object_or_404, whereas inauthorize_update, you're usingmodel_instance.requesterandmodel_instance.encounterdirectly. For consistency and clarity, consider using the same approach in both methods.care/emr/resources/medication/request/spec.py (1)
251-254: Consider adding error handling for requester lookupWhile
get_object_or_404handles the case of a non-existent user, you might want to add more specific error handling for cases where:
- The requester exists but doesn't have the necessary permissions
- The requester is inactive or disabled
if self.requester: - obj.requester = get_object_or_404(User, external_id=self.requester) + try: + user = get_object_or_404(User, external_id=self.requester) + if not user.is_active: + raise ValueError("Requester account is inactive") + obj.requester = user + except Exception as e: + raise ValueError(f"Invalid requester: {str(e)}")care/emr/tests/test_medication_request.py (4)
10-39: Add class-level docstringWhile the test methods are well documented, adding a class-level docstring would make it even clearer what this test suite covers.
class TestMedicationRequestApi(CareAPITestBase): + """ + Test suite for MedicationRequest API endpoints. + + Tests cover: + - List operations with various permission scenarios + - Create operations with requester validation + - Update operations with permission checks + """ def setUp(self):
43-83: Consider adding input validation to helper methodsThe helper methods look good, but could be more robust with input validation.
def _get_medication_request_url(self, medication_request_id): """Helper to get the detail URL for a specific medication request.""" + if not medication_request_id: + raise ValueError("medication_request_id cannot be None or empty") return reverse(
84-169: Consider adding edge case testsWhile the permission tests are comprehensive, consider adding tests for:
- Attempting to create a request with a non-existent requester
- Creating a request with an inactive requester
- Edge cases around permission changes after request creation
Example test case:
def test_create_medication_request_with_inactive_requester(self): """ Attempt to create a medication request with an inactive requester should fail (HTTP 400). """ requester = self.create_user() requester.is_active = False requester.save() permissions = [ PatientPermissions.can_view_clinical_data.name, EncounterPermissions.can_write_encounter.name, ] role = self.create_role_with_permissions(permissions) self.attach_role_facility_organization_user(self.organization, self.user, role) data = self.get_medication_request_data(requester=requester.external_id) response = self.client.post(self.base_url, data, format="json") self.assertEqual(response.status_code, 400)
170-248: Standardize error messages across testsThe error messages in assertions could be more consistent. For example:
- Line 158: "Requester does not have permission to update encounter"
- Line 246: Same message, but in a different context
Consider creating constants for these messages to ensure consistency.
+ # At the top of the test class + ERROR_MSG_NO_PERMISSION = "Requester does not have permission to update encounter" + def test_update_medication_request_without_permission_for_requester(self): # ... self.assertContains( response, - "Requester does not have permission to update encounter", + self.ERROR_MSG_NO_PERMISSION, status_code=403, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
care/emr/api/viewsets/medication_request.py(3 hunks)care/emr/migrations/0011_medicationrequest_requester.py(1 hunks)care/emr/models/medication_request.py(1 hunks)care/emr/resources/medication/request/spec.py(5 hunks)care/emr/tests/test_medication_request.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/migrations/0011_medicationrequest_requester.py
11-11: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
11-11: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
18-18: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/emr/api/viewsets/medication_request.py
63-68: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🪛 GitHub Actions: Lint Code Base
care/emr/api/viewsets/medication_request.py
[error] 63-68: SIM102: Use a single if statement instead of nested if statements. Combine if statements using and
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
care/emr/models/medication_request.py (1)
21-23: Addition of 'requester' field looks goodThe new
requesterfield is appropriately defined with the correct options and references.care/emr/resources/medication/request/spec.py (2)
4-4: LGTM!The new imports are appropriate for handling the requester field functionality.
Also applies to: 25-25
202-202: Verify if requester should be optionalThe requester field is defined as optional (
None). While this might be intentional, it would be good to verify if all medication requests should require a requester for better accountability.Also applies to: 227-227
✅ Verification successful
The requester's optionality appears to be... intentional
The model explicitly defines the requester field with
null=True, blank=True, and usesSET_NULLon deletion, which strongly suggests this was a conscious design decision to handle various scenarios like system-generated requests or historical data migration. But you probably knew that already.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any existing medication requests without a requester # to understand current usage patterns rg -l "class MedicationRequest\(" | while read -r file; do echo "=== $file ===" rg "requester.*=.*models\." -A 5 "$file" doneLength of output: 351
| if model_instance.requester: | ||
| if not AuthorizationController.call( | ||
| "can_update_encounter_obj", | ||
| model_instance.requester, | ||
| model_instance.encounter, | ||
| ): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Combine nested if statements for better readability
You can simplify the nested if statements by combining them into a single conditional using and.
Apply this diff to refactor the code:
- if model_instance.requester:
- if not AuthorizationController.call(
+ if model_instance.requester and not AuthorizationController.call(
"can_update_encounter_obj",
model_instance.requester,
model_instance.encounter,
):
raise PermissionDenied(
"Requester does not have permission to update encounter"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if model_instance.requester: | |
| if not AuthorizationController.call( | |
| "can_update_encounter_obj", | |
| model_instance.requester, | |
| model_instance.encounter, | |
| ): | |
| if model_instance.requester and not AuthorizationController.call( | |
| "can_update_encounter_obj", | |
| model_instance.requester, | |
| model_instance.encounter, | |
| ): |
🧰 Tools
🪛 Ruff (0.8.2)
63-68: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🪛 GitHub Actions: Lint Code Base
[error] 63-68: SIM102: Use a single if statement instead of nested if statements. Combine if statements using and
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
care/emr/tests/test_medication_request.py (2)
55-70: Consider using factory patterns for test data creationThe
create_medication_requestmethod could benefit from using a factory pattern to make test data creation more maintainable. Not that the current implementation isn't fine, but you know...- def create_medication_request(self, **kwargs): - data = { - "patient": self.patient, - "encounter": self.encounter, - "status": "active", - "intent": "order", - "category": "inpatient", - "priority": "routine", - "do_not_perform": False, - "medication": self.valid_code, - "dosage_instruction": [], - "authored_on": datetime.now(UTC), - } - data.update(kwargs) - return baker.make("emr.MedicationRequest", **data) + class MedicationRequestFactory: + @staticmethod + def build(patient, encounter, **kwargs): + defaults = { + "patient": patient, + "encounter": encounter, + "status": "active", + "intent": "order", + "category": "inpatient", + "priority": "routine", + "do_not_perform": False, + "medication": self.valid_code, + "dosage_instruction": [], + "authored_on": datetime.now(UTC), + } + defaults.update(kwargs) + return baker.make("emr.MedicationRequest", **defaults)
158-162: Consider using more specific error message assertionsWhile asserting the error message is good, using a more specific assertion method might be more... appropriate? Consider:
- self.assertContains( - response, - "Requester does not have permission to update encounter", - status_code=403, - ) + self.assertEqual(response.status_code, 403) + self.assertEqual( + response.json()["detail"], + "Requester does not have permission to update encounter" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/tests/test_medication_request.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (3)
care/emr/tests/test_medication_request.py (3)
86-105: Add test cases for pagination and filteringThe list operation tests could use a bit more coverage. I mean, they're good, but wouldn't it be even better if we added tests for:
- Pagination behavior
- Filtering by status
- Filtering by date range
Would you like me to help generate these additional test cases?
172-250: Add test cases for partial updatesThe update tests are quite thorough, but they seem to be missing PATCH endpoint tests. I'm sure it was just an oversight, but we should probably add tests for:
- Partial updates using PATCH
- Validation of individual field updates
Would you like me to help generate these test cases?
1-250: Consider adding tests for additional error scenariosWhile the test coverage is... adequate, we might want to add tests for:
- Network timeout scenarios
- Database constraint violations
- Concurrent update conflicts
Would you like me to help generate these additional test cases?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2773 +/- ##
===========================================
+ Coverage 64.74% 64.90% +0.15%
===========================================
Files 252 252
Lines 12743 12761 +18
Branches 1119 1121 +2
===========================================
+ Hits 8251 8283 +32
+ Misses 4384 4369 -15
- Partials 108 109 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
care/emr/tests/test_medication_request.py (1)
86-105: Consider adding edge cases for list permissions.While the current tests cover basic scenarios, you might want to add tests for:
- Partially completed encounters
- Multiple encounters with mixed permissions
- Archived/deleted encounters
I mean, if you want to be thorough... 🤷
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/api/viewsets/medication_request.py(3 hunks)care/emr/resources/medication/request/spec.py(5 hunks)care/emr/tests/test_medication_request.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- care/emr/resources/medication/request/spec.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
care/emr/api/viewsets/medication_request.py (1)
49-59: Consider enhancing the error message clarity.The error message could be more specific by including the encounter ID.
- "Requester does not have permission to update encounter" + f"Requester does not have permission to update encounter {encounter.external_id}"And since we're here, you might want to combine those nested if statements... just saying. 😏
care/emr/tests/test_medication_request.py (3)
13-84: Well-structured test setup!The test fixtures and helper methods are comprehensive and follow best practices.
172-230: Excellent update permission tests!The tests properly verify that the requester field cannot be updated and cover all necessary permission scenarios.
106-171: Add test for non-existent requester.The tests cover valid scenarios, but what happens when the requester doesn't exist?
vigneshhari
deleted the
rithviknisahd/feat/medication-request-requester
branch
Proposed Changes
requestertoMedicationRequestand testsSummary by CodeRabbit
New Features
requesterfield to medication requests.Tests
Bug Fixes