Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a permission and authorization method to prevent clearing a patient's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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 (2)
care/emr/api/viewsets/patient.py (1)
158-166: Consider caching the result ofself.get_object()to avoid duplicate database queries.The call to
self.get_object()on line 160 fetches the patient from the database. If this method is called elsewhere in the update flow (or ifsuper().perform_update()also fetches it), you're making redundant queries. You might want to store the result and reuse it.That said, the permission logic itself looks correct—nicely preventing unauthorized reversal of deceased status.
♻️ Proposed optimization
def perform_update(self, instance): identifiers = instance._identifiers # noqa: SLF001 + existing_obj = self.get_object() with transaction.atomic(): if ( instance.deceased_datetime is None - and self.get_object().deceased_datetime + and existing_obj.deceased_datetime and not AuthorizationController.call( "can_unmark_deceased_patient", self.request.user ) ): raise PermissionDenied(detail="Cannot mark deceased patient alive")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/api/viewsets/patient.py` around lines 158 - 166, Store the result of self.get_object() in a local variable and reuse it instead of calling it twice; e.g., in the viewset update/perform_update flow, call existing = self.get_object() once, then compare instance.deceased_datetime to existing.deceased_datetime and call AuthorizationController.call("can_unmark_deceased_patient", self.request.user) as before, raising PermissionDenied(...) if unauthorized—this eliminates the duplicate database fetch and keeps the same logic in perform_update/get_object/AuthorizationController.call.care/emr/tests/test_patient_api.py (1)
209-270: Good test coverage for the primary use cases.These tests correctly validate the permission boundary for unmarking deceased patients. The superuser test confirms authorized access, and the non-superuser test confirms the 403 response. Nicely done.
One thing you might want to add: a test verifying that a partial update (one that doesn't include
deceased_datetimeat all) doesn't accidentally clear the deceased status. Given the implementation inspec.py, this edge case could expose unexpected behavior.💡 Suggested additional test case
def test_update_patient_without_deceased_datetime_preserves_status(self): """Test that updating a patient without providing deceased_datetime doesn't clear it""" geo_organization = self.create_organization(org_type="govt") superuser = self.create_super_user() role = self.create_role_with_permissions( permissions=[ PatientPermissions.can_create_patient.name, PatientPermissions.can_write_patient.name, PatientPermissions.can_list_patients.name, ] ) self.attach_role_organization_user(geo_organization, superuser, role) self.client.force_authenticate(user=superuser) # Create patient with deceased_datetime deceased_time = care_now() - datetime.timedelta(days=2) patient_data = self.generate_patient_data( geo_organization=geo_organization.external_id, deceased_datetime=deceased_time, ) response = self.client.post(self.base_url, patient_data, format="json") self.assertEqual(response.status_code, status.HTTP_200_OK) patient_id = response.data["id"] # Update patient WITHOUT including deceased_datetime update_url = reverse("patient-detail", kwargs={"external_id": patient_id}) del patient_data["deceased_datetime"] response = self.client.put(update_url, patient_data, format="json") self.assertEqual(response.status_code, status.HTTP_200_OK) # This SHOULD still have the original deceased_datetime self.assertIsNotNone(response.data["deceased_datetime"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_patient_api.py` around lines 209 - 270, Add a test named test_update_patient_without_deceased_datetime_preserves_status that follows the existing pattern: use create_organization/create_super_user/create_role_with_permissions/attach_role_organization_user and client.force_authenticate, create a patient via POST using generate_patient_data with deceased_datetime set, then perform a PUT to reverse("patient-detail", kwargs={"external_id": patient_id}) after removing the "deceased_datetime" key from the payload (del patient_data["deceased_datetime"]) and assert status 200 and that response.data["deceased_datetime"] is still not None to ensure partial updates don't clear the field.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
care/emr/api/viewsets/patient.pycare/emr/resources/patient/spec.pycare/emr/tests/test_patient_api.pycare/security/authorization/patient.py
🤖 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/resources/patient/spec.py`:
- Around line 190-191: The current code unconditionally clears
obj.deceased_datetime when self.deceased_datetime is None; change the logic to
only modify obj.deceased_datetime if the field was explicitly provided in the
input. For example, detect explicit presence using the Pydantic model's
__fields_set__ (e.g., if "deceased_datetime" in self.__fields_set__: set
obj.deceased_datetime = self.deceased_datetime), or implement a sentinel/default
marker for omitted fields and only assign when the field is present; update the
block that references self.deceased_datetime and obj.deceased_datetime
accordingly.
---
Nitpick comments:
In `@care/emr/api/viewsets/patient.py`:
- Around line 158-166: Store the result of self.get_object() in a local variable
and reuse it instead of calling it twice; e.g., in the viewset
update/perform_update flow, call existing = self.get_object() once, then compare
instance.deceased_datetime to existing.deceased_datetime and call
AuthorizationController.call("can_unmark_deceased_patient", self.request.user)
as before, raising PermissionDenied(...) if unauthorized—this eliminates the
duplicate database fetch and keeps the same logic in
perform_update/get_object/AuthorizationController.call.
In `@care/emr/tests/test_patient_api.py`:
- Around line 209-270: Add a test named
test_update_patient_without_deceased_datetime_preserves_status that follows the
existing pattern: use
create_organization/create_super_user/create_role_with_permissions/attach_role_organization_user
and client.force_authenticate, create a patient via POST using
generate_patient_data with deceased_datetime set, then perform a PUT to
reverse("patient-detail", kwargs={"external_id": patient_id}) after removing the
"deceased_datetime" key from the payload (del patient_data["deceased_datetime"])
and assert status 200 and that response.data["deceased_datetime"] is still not
None to ensure partial updates don't clear the field.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3542 +/- ##
===========================================
+ Coverage 75.32% 75.35% +0.02%
===========================================
Files 473 473
Lines 22066 22072 +6
Branches 2305 2307 +2
===========================================
+ Hits 16622 16633 +11
+ Misses 4926 4922 -4
+ Partials 518 517 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
care/emr/tests/test_patient_api.py (1)
308-317:⚠️ Potential issue | 🔴 Critical
geo_organizationis referenced before assignment, causing test failure.The initial block in
test_invalid_age_and_death_dateusesgeo_organizationbefore it is defined (F821), so the test crashes before reaching the intended validation checks.🛠️ Minimal fix
def test_invalid_age_and_death_date(self): - patient_data = self.generate_patient_data( - geo_organization=geo_organization.external_id - ) - response = self.client.post(self.base_url, patient_data, format="json") - self.assertEqual(response.status_code, status.HTTP_200_OK) - patient_id = response.data["id"] - delete_url = reverse("patient-detail", kwargs={"external_id": patient_id}) - response = self.client.delete(delete_url) - self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) user = self.create_user() geo_organization = self.create_organization(org_type="govt")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_patient_api.py` around lines 308 - 317, test_invalid_age_and_death_date references geo_organization before it’s defined; fix by creating or assigning geo_organization before calling generate_patient_data: e.g. set geo_organization = self.create_geo_organization() or obtain it from the test setup/fixture, then call patient_data = self.generate_patient_data(geo_organization=geo_organization.external_id); keep the rest of the test (base_url, reverse("patient-detail"), delete) unchanged.
🧹 Nitpick comments (1)
care/emr/tests/test_patient_api.py (1)
291-307: Test name no longer matches what it asserts.
test_delete_patient_as_non_superusernow verifies aPUTunmark-deceased permission path, not delete authorization. Please rename it to match intent, or restore delete assertions in a separate test.As per coding guidelines
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance). Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_patient_api.py` around lines 291 - 307, The test currently named test_delete_patient_as_non_superuser is asserting that a non-superuser cannot unset deceased_datetime via a PUT, so rename the test to reflect its intent (e.g., test_unmark_deceased_as_non_superuser) or separate into two tests (one for delete authorization and one for unmarking deceased) and update any references; ensure the new test name uses lowercase_with_underscores and is descriptive, and keep the existing assertions around the PUT to verify HTTP_403_FORBIDDEN on attempting to set deceased_datetime to None in the patient-detail update flow.
🤖 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/tests/test_patient_api.py`:
- Around line 241-244: The test test_non_superuser_cannot_unmark_deceased is a
stub that leaves unused locals and should either be implemented or removed; to
fix it, implement the test: use create_organization and create_user to set up a
non-superuser, create a patient record with deceased_datetime set, attempt to
unmark the patient (e.g., call the API endpoint or PatientViewSet method used
elsewhere in tests) as the non-superuser, assert the response is forbidden (HTTP
403) and verify the patient's deceased_datetime remains unchanged; if this
behavior is already covered by another test, simply delete the stub to remove
the unused-variable lint error.
---
Outside diff comments:
In `@care/emr/tests/test_patient_api.py`:
- Around line 308-317: test_invalid_age_and_death_date references
geo_organization before it’s defined; fix by creating or assigning
geo_organization before calling generate_patient_data: e.g. set geo_organization
= self.create_geo_organization() or obtain it from the test setup/fixture, then
call patient_data =
self.generate_patient_data(geo_organization=geo_organization.external_id); keep
the rest of the test (base_url, reverse("patient-detail"), delete) unchanged.
---
Nitpick comments:
In `@care/emr/tests/test_patient_api.py`:
- Around line 291-307: The test currently named
test_delete_patient_as_non_superuser is asserting that a non-superuser cannot
unset deceased_datetime via a PUT, so rename the test to reflect its intent
(e.g., test_unmark_deceased_as_non_superuser) or separate into two tests (one
for delete authorization and one for unmarking deceased) and update any
references; ensure the new test name uses lowercase_with_underscores and is
descriptive, and keep the existing assertions around the PUT to verify
HTTP_403_FORBIDDEN on attempting to set deceased_datetime to None in the
patient-detail update flow.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tests/test_patient_api.py`:
- Around line 241-270: After asserting the 403 in
test_non_superuser_cannot_unmark_deceased, perform a GET on the patient-detail
(reuse update_url or reverse with external_id=patient_id) to fetch the current
record and assert the GET returns 200 and that
response.data["deceased_datetime"] is still set (either assertIsNotNone or
compare to the original deceased_time). This ensures the failed PUT did not
clear the deceased_datetime.
- Around line 209-239: Add a follow-up assertion in
test_non_superuser_cannot_unmark_deceased to ensure the patient's
deceased_datetime was not changed after the 403 response: after the existing
self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) call, retrieve
the patient state (either via a GET to reverse("patient-detail",
kwargs={"external_id": patient_id}) or by reloading the patient record from the
DB using the patient_id) and assert that the returned data/model's
deceased_datetime is still not None; this ensures the test validates that no
partial update occurred.
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.
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
Tests