added pending testcases for pr 3532#3570
Merged
vigneshhari merged 3 commits intodevelopfrom Mar 13, 2026
Merged
Conversation
Contributor
|
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)
📝 WalkthroughWalkthroughAdds extensive, test-only changes across EMR test modules: new permission and identifier-configuration tests, expanded patient search/management and tag tests, prescription permission scenarios, and a superuser resource-request listing; several tests add model imports and helper utilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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: 4
🧹 Nitpick comments (1)
care/emr/tests/test_medication_request_prescription_api.py (1)
155-183: The new “with permissions” tests still only exercise the superuser path.Because
setUpalready authenticatesself.superuser, these cases duplicate the existing retrieve/update happy paths instead of covering a granted-permission path for a regular user. If the goal here is permission coverage, attach the required role toself.userand run these through that account instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_medication_request_prescription_api.py` around lines 155 - 183, The tests named test_retrieve_prescription_with_permissions and test_update_prescription_with_permissions are accidentally using the already-authenticated superuser from setUp instead of a regular user with granted permissions; update each test to: assign the needed role/permission to self.user (e.g., add the role to self.user.groups or create the specific permission and add it to self.user.user_permissions), call self.client.force_authenticate(user=self.user) before the request, and remove or override any superuser authentication for those cases so the request flows through the granted-permission path rather than the superuser path.
🤖 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_encounter_api.py`:
- Around line 596-627: Update the two tests to actually assert the behaviors
their names describe: in test_set_facility_identifier_delete_by_null_value call
the "encounter-set-facility-idenitifier" endpoint with data where "value" is
None (or omitted/JSON null) for the created identifier_config and then assert
the identifier was removed (e.g., response status 200 and subsequent GET on the
encounter shows no value for that identifier); in
test_set_facility_identifier_with_value_and_set_default after posting with
"set_default": True assert the identifier_config/default store was updated
(fetch the identifier_config or re-fetch the encounter via the same
"encounter-set-facility-idenitifier" GET/lookup and assert the default value
equals the expected f'ID-{patient_count}'), and add a follow-up request that
clears the explicit value to verify the default is applied. Ensure you reference
the identifier_config created by _create_identifier_config and the reverse name
"encounter-set-facility-idenitifier" when locating the code to change.
In `@care/emr/tests/test_medication_request_prescription_api.py`:
- Around line 222-230: The test test_summary_as_superuser currently allows both
200 and 403, so it doesn't assert the intended contract; update the test to
assert a single expected outcome: either (A) require a 200 by authenticating as
a user with the pharmacist permission (create or reuse a fixture/user with the
exact pharmacist role/permission and call the request as that user) and assert
response.status_code == status.HTTP_200_OK, or (B) if the intended contract is
that superusers must be forbidden, change the assertion to response.status_code
== status.HTTP_403_FORBIDDEN; use the test helper methods (e.g., any existing
user creation helpers) to create/authenticate the pharmacist-permission user and
keep the test name or rename it to reflect the actual actor (e.g.,
test_summary_as_pharmacist) so the behavior is unambiguous.
In `@care/emr/tests/test_patient_api.py`:
- Around line 307-314: The test_search_by_phone_number currently only checks
that results exist which could pass if the filter is ignored; modify the test to
ensure the created patient is actually returned: have
_create_patient_with_phone(phone) return the created patient (or retrieve the
created patient's id/phone after creation), then after the POST to
reverse("patient-search") assert that response.data["results"] contains an entry
with that patient's id or phone (e.g., assert any(r["id"] == created.id or
r.get("phone_number") == phone for r in response.data["results"])). Keep the
existing status and partial assertions and use the phone and
response.data["results"] symbols shown.
- Around line 400-663: The tests such as test_add_user_to_patient,
test_delete_user_from_patient, test_add_duplicate_user_to_patient,
test_get_users_for_patient, test_update_identifier,
test_update_identifier_auto_maintained,
test_set_instance_tags/test_remove_instance_tags and
test_set_facility_tags/test_remove_facility_tags currently only assert HTTP
status codes; update each to re-fetch the resource (either call the patient GET
endpoint or query the DB models like Patient, PatientIdentifier, TagConfig or
the patient-user relation) after the POST and assert the side-effect: that the
added user exists in the patient’s user list, that a deleted user is removed,
that duplicate-add returns 400 and no duplicate relation was persisted, that the
identifier value is stored or rejected (for auto_maintained configs), and that
tags (instance and facility) are actually attached/removed from the patient; use
the existing URL helpers (reverse("patient-get-users"),
reverse("patient-update-identifier"), reverse("patient-get-<or-detail>") or
model queries) and the created objects (role, tag, config, facility) to validate
persisted state rather than only status codes.
---
Nitpick comments:
In `@care/emr/tests/test_medication_request_prescription_api.py`:
- Around line 155-183: The tests named
test_retrieve_prescription_with_permissions and
test_update_prescription_with_permissions are accidentally using the
already-authenticated superuser from setUp instead of a regular user with
granted permissions; update each test to: assign the needed role/permission to
self.user (e.g., add the role to self.user.groups or create the specific
permission and add it to self.user.user_permissions), call
self.client.force_authenticate(user=self.user) before the request, and remove or
override any superuser authentication for those cases so the request flows
through the granted-permission path rather than the superuser path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c0177c9e-0cd2-41e2-b807-0f9690622d2d
📒 Files selected for processing (4)
care/emr/tests/test_encounter_api.pycare/emr/tests/test_medication_request_prescription_api.pycare/emr/tests/test_patient_api.pycare/emr/tests/test_resource_request_api.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3570 +/- ##
===========================================
+ Coverage 76.22% 77.06% +0.83%
===========================================
Files 474 474
Lines 22271 22271
Branches 2325 2325
===========================================
+ Hits 16976 17163 +187
+ Misses 4766 4552 -214
- Partials 529 556 +27 ☔ 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
♻️ Duplicate comments (2)
care/emr/tests/test_encounter_api.py (2)
596-615:⚠️ Potential issue | 🟠 MajorTest name doesn't match behavior - deletion via null value is not tested.
The test is named
test_set_facility_identifier_delete_by_null_valuebut it only creates an identifier with a value. To actually test the "delete by null value" behavior, you'd need to first create the identifier, then send another request with a null/empty value to delete it, and verify the identifier was removed. Currently, this test just duplicates whattest_set_facility_identifier_with_permissionswould do (if that test had proper assertions).Suggested fix to actually test deletion
def test_set_facility_identifier_delete_by_null_value(self): self.client.force_authenticate(user=self.superuser) identifier_config = self._create_identifier_config(self.facility) url = reverse( "encounter-set-facility-idenitifier", kwargs={"external_id": self.encounter.external_id}, ) + # First, create the identifier data = { "identifier": str(identifier_config.external_id), "value": "TEST-VALUE", } response = self.client.post(url, data, format="json") self.assertEqual(response.status_code, 200) self.assertTrue( PatientIdentifier.objects.filter( patient=self.patient, config=identifier_config, value="TEST-VALUE", ).exists() ) + # Now, delete by sending null value + delete_data = { + "identifier": str(identifier_config.external_id), + "value": None, + } + response = self.client.post(url, delete_data, format="json") + self.assertEqual(response.status_code, 200) + self.assertFalse( + PatientIdentifier.objects.filter( + patient=self.patient, + config=identifier_config, + ).exists() + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_encounter_api.py` around lines 596 - 615, Rename or update the test body for test_set_facility_identifier_delete_by_null_value so it actually asserts deletion: use self._create_identifier_config(self.facility) and create the identifier first by POSTing to the "encounter-set-facility-idenitifier" endpoint with value "TEST-VALUE" and assert PatientIdentifier.exists for patient/self.patient and config/identifier_config; then send a second POST to the same endpoint with identifier set to str(identifier_config.external_id) and value set to None (or an empty string if API expects that) and assert the response indicates success and that PatientIdentifier.objects.filter(patient=self.patient, config=identifier_config).exists() is False (or count==0) to verify the record was removed.
617-636:⚠️ Potential issue | 🟡 MinorTest doesn't actually exercise the "set_default" behavior.
The test name includes "set_default" and the config has a
default_value, but the request data doesn't include anyset_defaultflag, and there's no verification that the default mechanism works. The test just sets an explicit value and verifies it was stored, which is fine but doesn't match what the name implies. If you want to test the default behavior, you'd need to either: (1) not provide a value and verify the default is applied, or (2) sendset_default=Trueand verify the config was updated accordingly.At least the explicit value assertion is now present, which is an improvement. But the test name might be misleading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_encounter_api.py` around lines 617 - 636, The test test_set_facility_identifier_with_value_and_set_default is misleading because it never exercises the default behavior: either change the test name to reflect it only sets an explicit value, or modify the test to exercise the default path — for the latter, call the same endpoint (reverse "encounter-set-facility-idenitifier") without a "value" in the POST body and assert a PatientIdentifier is created with value equal to identifier_config.default_value, or include "set_default": True in the POST data and assert the identifier_config (or PatientIdentifier) was updated to use the default; ensure you still assert response.status_code == 200 and query PatientIdentifier.objects.get(patient=self.patient, config=identifier_config) to verify the applied value or updated config.
🧹 Nitpick comments (5)
care/emr/tests/test_medication_request_prescription_api.py (3)
207-216: Helper method is duplicated fromTestMedicationRequestPrescriptionViewSet.The
_create_prescription_objmethod is identical to the one in the other test class (lines 54-63). Consider extracting this to a shared mixin or base class if you want to keep things DRY. Not a big deal for test code, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_medication_request_prescription_api.py` around lines 207 - 216, The helper method _create_prescription_obj is duplicated between this test and TestMedicationRequestPrescriptionViewSet; extract it into a shared TestPrescriptionMixin (or a common base test class) that defines _create_prescription_obj and import/inherit that mixin in both test classes so they call the single implementation; ensure the mixin uses the same parameters (encounter, patient, MedicationRequestPrescriptionStatus, superuser) and returns baker.make(MedicationRequestPrescription, **data) so callers in both test modules keep existing usage unchanged.
171-183: This test is also redundant withtest_update_prescription.The existing
test_update_prescription(lines 91-103) already covers updating as a superuser. Having two tests that do essentially the same thing doesn't add coverage. Perhaps one of them could be removed, or the naming could indicate a meaningful difference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_medication_request_prescription_api.py` around lines 171 - 183, Remove or consolidate the redundant test function test_update_prescription_with_permissions since test_update_prescription already covers updating a prescription as a superuser; either delete test_update_prescription_with_permissions or replace it with a distinct scenario (e.g., asserting permission-denied behavior for a non-superuser) and update its name accordingly so the two tests no longer duplicate coverage (refer to test_update_prescription and test_update_prescription_with_permissions to locate the code).
156-162: This test appears redundant withtest_retrieve_prescription.The existing
test_retrieve_prescription(lines 84-89) already tests retrieval as a superuser and asserts the same 200 status and ID match. This new test just adds a name assertion. You might consider either removing the duplicate or consolidating them. It's not wrong, just... duplicative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_medication_request_prescription_api.py` around lines 156 - 162, The test test_retrieve_prescription_with_permissions duplicates behavior already covered by test_retrieve_prescription; either remove the redundant test or consolidate its extra assertion into the existing test_retrieve_prescription: delete test_retrieve_prescription_with_permissions (which calls _create_prescription_obj and _get_detail_url) or move its self.assertEqual(response.data["name"], "Test Prescription") assertion into test_retrieve_prescription so the same retrieval covers status, id and name in one test, keeping helper calls (_create_prescription_obj, _get_detail_url) unchanged.care/emr/tests/test_patient_api.py (2)
492-502: These tests are rather minimal.Both tests only verify that the endpoints return 200, without checking any response content. Since no appointments or tokens are created beforehand, they're essentially just smoke tests. If the goal is to verify the endpoints work at all, that's fine, but they don't test much behavior.
🤖 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 492 - 502, The tests test_get_appointments and test_get_tokens are only asserting HTTP 200 and should validate response content; update each test to create corresponding fixtures via _setup_patient_with_write_permission (or create appointment/token objects tied to the returned patient_id) before calling reverse("patient-get-appointments", ...) and reverse("patient-get-tokens", ...), then GET the endpoints and assert the JSON body contains the expected items (e.g., count/IDs and field values) and proper serialization keys rather than only status codes; ensure you reference the same patient_id from _setup_patient_with_write_permission and clean up or use test factories so the assertions are deterministic.
423-437: Consider verifying no duplicate was persisted.The test asserts 400 for a duplicate add attempt, which is fine, but you might also want to confirm that only one
PatientUserrecord exists for this user-patient pair. Just a thought.Optional enhancement
response = self.client.post( url, {"user": str(other_user.external_id), "role": str(role.external_id)}, format="json", ) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual( + PatientUser.objects.filter( + user=other_user, patient__external_id=patient_id + ).count(), + 1, + )🤖 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 423 - 437, In test_add_duplicate_user_to_patient add an assertion that only one PatientUser record was persisted for the given patient/user pair: after the second POST (which already asserts a 400) query PatientUser (use PatientUser.objects.filter with patient or patient__external_id and user/ user_id or other_user) and assert count() == 1; ensure the test imports PatientUser or references the correct model and uses the same patient_id/other_user used in the POSTs.
🤖 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_encounter_api.py`:
- Around line 543-564: The test test_set_facility_identifier_with_permissions
only asserts a 200 status but not the side effect; after posting to the
"encounter-set-facility-idenitifier" URL (using self.encounter.external_id and
the identifier_config created by _create_identifier_config), query the
PatientIdentifier model for a record matching identifier_config (external_id or
id), value "TEST-123" and the patient/encounter association (e.g.,
self.encounter.patient or self.encounter) and assert it exists and its fields
match (identifier config and value); update the test to include this database
assertion to verify the resource was actually created.
---
Duplicate comments:
In `@care/emr/tests/test_encounter_api.py`:
- Around line 596-615: Rename or update the test body for
test_set_facility_identifier_delete_by_null_value so it actually asserts
deletion: use self._create_identifier_config(self.facility) and create the
identifier first by POSTing to the "encounter-set-facility-idenitifier" endpoint
with value "TEST-VALUE" and assert PatientIdentifier.exists for
patient/self.patient and config/identifier_config; then send a second POST to
the same endpoint with identifier set to str(identifier_config.external_id) and
value set to None (or an empty string if API expects that) and assert the
response indicates success and that
PatientIdentifier.objects.filter(patient=self.patient,
config=identifier_config).exists() is False (or count==0) to verify the record
was removed.
- Around line 617-636: The test
test_set_facility_identifier_with_value_and_set_default is misleading because it
never exercises the default behavior: either change the test name to reflect it
only sets an explicit value, or modify the test to exercise the default path —
for the latter, call the same endpoint (reverse
"encounter-set-facility-idenitifier") without a "value" in the POST body and
assert a PatientIdentifier is created with value equal to
identifier_config.default_value, or include "set_default": True in the POST data
and assert the identifier_config (or PatientIdentifier) was updated to use the
default; ensure you still assert response.status_code == 200 and query
PatientIdentifier.objects.get(patient=self.patient, config=identifier_config) to
verify the applied value or updated config.
---
Nitpick comments:
In `@care/emr/tests/test_medication_request_prescription_api.py`:
- Around line 207-216: The helper method _create_prescription_obj is duplicated
between this test and TestMedicationRequestPrescriptionViewSet; extract it into
a shared TestPrescriptionMixin (or a common base test class) that defines
_create_prescription_obj and import/inherit that mixin in both test classes so
they call the single implementation; ensure the mixin uses the same parameters
(encounter, patient, MedicationRequestPrescriptionStatus, superuser) and returns
baker.make(MedicationRequestPrescription, **data) so callers in both test
modules keep existing usage unchanged.
- Around line 171-183: Remove or consolidate the redundant test function
test_update_prescription_with_permissions since test_update_prescription already
covers updating a prescription as a superuser; either delete
test_update_prescription_with_permissions or replace it with a distinct scenario
(e.g., asserting permission-denied behavior for a non-superuser) and update its
name accordingly so the two tests no longer duplicate coverage (refer to
test_update_prescription and test_update_prescription_with_permissions to locate
the code).
- Around line 156-162: The test test_retrieve_prescription_with_permissions
duplicates behavior already covered by test_retrieve_prescription; either remove
the redundant test or consolidate its extra assertion into the existing
test_retrieve_prescription: delete test_retrieve_prescription_with_permissions
(which calls _create_prescription_obj and _get_detail_url) or move its
self.assertEqual(response.data["name"], "Test Prescription") assertion into
test_retrieve_prescription so the same retrieval covers status, id and name in
one test, keeping helper calls (_create_prescription_obj, _get_detail_url)
unchanged.
In `@care/emr/tests/test_patient_api.py`:
- Around line 492-502: The tests test_get_appointments and test_get_tokens are
only asserting HTTP 200 and should validate response content; update each test
to create corresponding fixtures via _setup_patient_with_write_permission (or
create appointment/token objects tied to the returned patient_id) before calling
reverse("patient-get-appointments", ...) and reverse("patient-get-tokens", ...),
then GET the endpoints and assert the JSON body contains the expected items
(e.g., count/IDs and field values) and proper serialization keys rather than
only status codes; ensure you reference the same patient_id from
_setup_patient_with_write_permission and clean up or use test factories so the
assertions are deterministic.
- Around line 423-437: In test_add_duplicate_user_to_patient add an assertion
that only one PatientUser record was persisted for the given patient/user pair:
after the second POST (which already asserts a 400) query PatientUser (use
PatientUser.objects.filter with patient or patient__external_id and user/
user_id or other_user) and assert count() == 1; ensure the test imports
PatientUser or references the correct model and uses the same
patient_id/other_user used in the POSTs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: af46590c-de67-42a6-a6da-143a05f7f658
📒 Files selected for processing (3)
care/emr/tests/test_encounter_api.pycare/emr/tests/test_medication_request_prescription_api.pycare/emr/tests/test_patient_api.py
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
2 participants
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
Only 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