Testcases for questionnaire response api#3609
Open
nandkishorr wants to merge 5 commits intodevelopfrom
Open
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
📝 WalkthroughWalkthroughIntroduces a comprehensive test module for the questionnaire response API, providing a base test class with full end-to-end data provisioning and 14 test cases covering retrieval, filtering, listing, updating operations, plus permission and validation edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 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_questionnaire_response_api.py`:
- Around line 379-413: The two tests
(test_retrieve_questionnaire_response_with_questionnaire_slug_filter and
test_retrieve_questionnaire_response_with_only_unstructured_filter) are too weak
because only one QuestionnaireResponse exists in fixtures; modify the tests to
create or attach a second QuestionnaireResponse that should be excluded by the
filter (or query using a non-matching slug) and then assert the filtered results
length is 1 and that the excluded response is not present; specifically, in each
test set up a second response (e.g., another QuestionnaireResponse object with a
different questionnaire slug or with structured answers) before calling
self.client.get(self.get_url(), ...) and then assert the
response.json()["results"] contains only self.questionnaire_response["id"] and
not the second response's id so the filter behavior in the view is actually
exercised.
- Around line 348-377: Both tests
(test_retrieve_questionnaire_response_without_encounter_permission and
test_retrieve_questionnaire_response_without_patient_permission) create a
brand-new user which may lack multiple prerequisites, so change each to grant
the user all required roles/permissions/facility access except the specific
permission under test: for the encounter-permission test, create a user, attach
the same role/facility and patient-level permissions used by other successful
tests but do NOT grant encounter access, authenticate via
self.client.force_authenticate, then call
get_detail_url(self.questionnaire_response["id"]) with {"encounter":
self.encounter.external_id} and assert 403; for the patient-permission test,
give the user all required encounter and facility permissions but omit the
patient-level permission, call get_detail_url(self.questionnaire_response["id"])
and assert the same 403; apply the same pattern for the related tests at the
other block (lines 434-460) to ensure each failure isolates a single missing
permission.
- Around line 555-585: The test is hitting the authorize_update guard that
rejects "entered_in_error" before the created_date check in authorize_update (in
questionnaire_response.py), so change
test_update_questionnaire_response_status_after_time_limit to backdate the
QuestionnaireResponse while its status is still "completed" (fetch
QuestionnaireResponse via QuestionnaireResponse.objects.get(external_id=...),
decrement created_date by settings.QUESTIONNAIRE_ERRORED_TIME_LIMIT_MINUTES + 1,
save and refresh_from_db), then perform the PUT to change status to
"entered_in_error" and assert a 403 with "Questionnaire Response cannot be
edited" so the time-limit branch is exercised instead of the entered_in_error
guard.
- Around line 63-201: The tests define mutable fixtures named questions (and
responses) at class scope which creates shared state across test instances; move
their initialization into setUp() or into factory/helper functions (e.g.,
make_questions() / make_responses()) and assign to instance attributes like
self.questions/self.responses so each test gets a fresh copy; update any tests
that reference the class-level questions/responses to use the instance
attributes or call the helper to avoid RUF012 shared-mutable-state issues.
🪄 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: e3f30b04-fa49-4741-ba3c-4ccc34748023
📒 Files selected for processing (1)
care/emr/tests/test_questionnaire_response_api.py
Comment on lines
+63
to
+201
| questions = [ | ||
| { | ||
| "id": "31f02e46-7d14-4a68-9c94-de009b54fb54", | ||
| "text": "Bilateral Air Entry", | ||
| "type": "group", | ||
| "link_id": "1.1", | ||
| "required": False, | ||
| "questions": [ | ||
| { | ||
| "id": "9358475e-b6f3-4a59-a88d-4b4e66222879", | ||
| "text": "Is bilateral air entry present?", | ||
| "type": "choice", | ||
| "link_id": "1.1.1", | ||
| "required": True, | ||
| "answer_option": [{"value": "yes"}, {"value": "no"}], | ||
| }, | ||
| { | ||
| "id": "63631f8f-0880-4758-891b-8ad7ac663be8", | ||
| "text": "Note on Bilateral Air Entry", | ||
| "type": "text", | ||
| "link_id": "1.1.2", | ||
| "required": False, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| "id": "80130e9a-e15c-4181-a32e-73dc68bd8755", | ||
| "text": "Respiratory Support", | ||
| "type": "group", | ||
| "link_id": "1.2", | ||
| "required": True, | ||
| "questions": [ | ||
| { | ||
| "id": "56634d0c-5321-41ff-8f0e-a6a4525be6ff", | ||
| "text": "Select Modality", | ||
| "type": "choice", | ||
| "link_id": "1.2.1", | ||
| "required": True, | ||
| "answer_option": [ | ||
| {"value": "oxygen_support"}, | ||
| {"value": "non_invasive"}, | ||
| {"value": "invasive"}, | ||
| ], | ||
| }, | ||
| { | ||
| "id": "d9bd1fa1-38ba-48ca-afdd-b0ff0623568e", | ||
| "text": "Select Oxygen Support Device", | ||
| "type": "choice", | ||
| "link_id": "1.2.2", | ||
| "required": False, | ||
| "answer_option": [ | ||
| {"value": "nasal_prongs"}, | ||
| {"value": "simple_face_mask"}, | ||
| {"value": "nrm"}, | ||
| {"value": "hfnc"}, | ||
| ], | ||
| }, | ||
| { | ||
| "id": "0493d006-af74-42c3-bb4a-c03ee735b05d", | ||
| "text": "Oxygen Flow Rate (L/min)", | ||
| "type": "decimal", | ||
| "link_id": "1.2.3", | ||
| "required": False, | ||
| }, | ||
| { | ||
| "id": "54a458e2-4ea0-4f0e-ad1c-c7e8cf9c0a12", | ||
| "text": "Select Ventilator Mode (Non-invasive)", | ||
| "type": "choice", | ||
| "link_id": "1.2.4", | ||
| "required": False, | ||
| "answer_option": [ | ||
| {"value": "cmv"}, | ||
| {"value": "simv"}, | ||
| {"value": "cpap_psv"}, | ||
| ], | ||
| }, | ||
| ], | ||
| "styling_metadata": {"containerClasses": "grid-2-col"}, | ||
| }, | ||
| { | ||
| "id": "c0f9e113-57e5-4167-a326-f86c6b457b62", | ||
| "text": "Ventilation Parameters", | ||
| "type": "group", | ||
| "link_id": "1.3", | ||
| "required": False, | ||
| "questions": [ | ||
| { | ||
| "id": "8fdaf3ab-3fa2-475e-a605-b05654f22ec7", | ||
| "text": "PEEP (cm H2O)", | ||
| "type": "decimal", | ||
| "link_id": "1.3.1", | ||
| "required": False, | ||
| }, | ||
| { | ||
| "id": "9b922018-5468-418e-a205-e3c1f9bb6847", | ||
| "text": "PIP (cm H2O)", | ||
| "type": "decimal", | ||
| "link_id": "1.3.2", | ||
| "required": False, | ||
| }, | ||
| { | ||
| "id": "e5661139-58b2-4773-96a8-cf9ec51e0320", | ||
| "text": "MAP (cm H2O)", | ||
| "type": "decimal", | ||
| "link_id": "1.3.3", | ||
| "required": False, | ||
| }, | ||
| { | ||
| "id": "65d21913-c59a-4b4d-99ea-3411b0fcdce1", | ||
| "text": "Ventilator RR (breaths per minute)", | ||
| "type": "integer", | ||
| "link_id": "1.3.4", | ||
| "required": False, | ||
| }, | ||
| { | ||
| "id": "0e5391e3-a6b3-44ab-8f20-9050aa279680", | ||
| "text": "Pressure Support (cm H2O)", | ||
| "type": "decimal", | ||
| "link_id": "1.3.5", | ||
| "required": False, | ||
| }, | ||
| { | ||
| "id": "46c70bd9-7dd8-47d9-80d3-4de7fc4d77c4", | ||
| "text": "Tidal Volume (mL)", | ||
| "type": "integer", | ||
| "link_id": "1.3.6", | ||
| "required": False, | ||
| }, | ||
| { | ||
| "id": "286afbde-9ac1-4717-964e-17453fa0ea01", | ||
| "text": "FiO2 (%)", | ||
| "type": "decimal", | ||
| "link_id": "1.3.7", | ||
| "required": False, | ||
| }, | ||
| ], | ||
| "styling_metadata": {"containerClasses": "grid-2-col"}, | ||
| }, | ||
| ] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "test_questionnaire_response_api.py" -type fRepository: ohcnetwork/care
Length of output: 110
🏁 Script executed:
head -n 310 ./care/emr/tests/test_questionnaire_response_api.py | tail -n +50Repository: ohcnetwork/care
Length of output: 9722
🏁 Script executed:
wc -l ./care/emr/tests/test_questionnaire_response_api.pyRepository: ohcnetwork/care
Length of output: 114
🏁 Script executed:
head -n 80 ./care/emr/tests/test_questionnaire_response_api.pyRepository: ohcnetwork/care
Length of output: 3040
🏁 Script executed:
grep -n "^ questions = \|^ responses = " ./care/emr/tests/test_questionnaire_response_api.pyRepository: ohcnetwork/care
Length of output: 101
🏁 Script executed:
cd ./care/emr/tests && ruff check test_questionnaire_response_api.pyRepository: ohcnetwork/care
Length of output: 15072
Move these shared fixtures to setUp() or helper methods.
questions and responses are mutable class attributes flagged by Ruff (RUF012). Defining them at the class body level creates shared state across all test instances—any in-place mutation bleeds across the entire test suite. Initialize fresh copies in setUp() or return them from helper methods instead.
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 63-201: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/tests/test_questionnaire_response_api.py` around lines 63 - 201, The
tests define mutable fixtures named questions (and responses) at class scope
which creates shared state across test instances; move their initialization into
setUp() or into factory/helper functions (e.g., make_questions() /
make_responses()) and assign to instance attributes like
self.questions/self.responses so each test gets a fresh copy; update any tests
that reference the class-level questions/responses to use the instance
attributes or call the helper to avoid RUF012 shared-mutable-state issues.
Comment on lines
+348
to
+377
| def test_retrieve_questionnaire_response_without_encounter_permission(self): | ||
| """ | ||
| Tests that retrieving a questionnaire response without appropriate permissions results in a permission denied error. | ||
| """ | ||
| user = self.create_user() | ||
| self.client.force_authenticate(user=user) | ||
| response = self.client.get( | ||
| self.get_detail_url(self.questionnaire_response["id"]), | ||
| {"encounter": self.encounter.external_id}, | ||
| ) | ||
| self.assertEqual(response.status_code, 403) | ||
| self.assertEqual( | ||
| response.json()["detail"], | ||
| "You do not have permission to view questionnaire responses", | ||
| ) | ||
|
|
||
| def test_retrieve_questionnaire_response_without_patient_permission(self): | ||
| """ | ||
| Tests that retrieving a questionnaire response without appropriate patient permissions results in a permission denied error. | ||
| """ | ||
| user = self.create_user() | ||
| self.client.force_authenticate(user=user) | ||
| response = self.client.get( | ||
| self.get_detail_url(self.questionnaire_response["id"]) | ||
| ) | ||
| self.assertEqual(response.status_code, 403) | ||
| self.assertEqual( | ||
| response.json()["detail"], | ||
| "You do not have permission to view questionnaire responses", | ||
| ) |
Contributor
There was a problem hiding this comment.
These 403 cases don’t prove which permission failed.
Each test uses a brand-new user with no attached role or facility access, so the denial can come from several missing prerequisites at once. If the encounter-specific or patient-specific guard disappeared, these would still stay green. Give the user every other required permission and omit only the one under test.
As per coding guidelines, **/tests/**/*.py: Use Django’s built-in tools for testing (unittest and pytest-django) to ensure code quality and reliability.
Also applies to: 434-460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/tests/test_questionnaire_response_api.py` around lines 348 - 377,
Both tests (test_retrieve_questionnaire_response_without_encounter_permission
and test_retrieve_questionnaire_response_without_patient_permission) create a
brand-new user which may lack multiple prerequisites, so change each to grant
the user all required roles/permissions/facility access except the specific
permission under test: for the encounter-permission test, create a user, attach
the same role/facility and patient-level permissions used by other successful
tests but do NOT grant encounter access, authenticate via
self.client.force_authenticate, then call
get_detail_url(self.questionnaire_response["id"]) with {"encounter":
self.encounter.external_id} and assert 403; for the patient-permission test,
give the user all required encounter and facility permissions but omit the
patient-level permission, call get_detail_url(self.questionnaire_response["id"])
and assert the same 403; apply the same pattern for the related tests at the
other block (lines 434-460) to ensure each failure isolates a single missing
permission.
Comment on lines
+379
to
+413
| def test_retrieve_questionnaire_response_with_questionnaire_slug_filter(self): | ||
| """ | ||
| Tests retrieval of questionnaire responses filtered by questionnaire slug and validates the response data. | ||
| """ | ||
| self.attach_role_facility_organization_user( | ||
| self.facility_organization, | ||
| self.user, | ||
| self.role, | ||
| ) | ||
| response = self.client.get( | ||
| self.get_url(), {"questionnaire_slugs": self.questionnaire["slug"]} | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
| response_data = response.json() | ||
| self.assertEqual(len(response_data["results"]), 1) | ||
| self.assertEqual( | ||
| response_data["results"][0]["id"], self.questionnaire_response["id"] | ||
| ) | ||
|
|
||
| def test_retrieve_questionnaire_response_with_only_unstructured_filter(self): | ||
| """ | ||
| Tests retrieval of questionnaire responses filtered by only unstructured responses and validates the response data. | ||
| """ | ||
| self.attach_role_facility_organization_user( | ||
| self.facility_organization, | ||
| self.user, | ||
| self.role, | ||
| ) | ||
| response = self.client.get(self.get_url(), {"only_unstructured": "true"}) | ||
| self.assertEqual(response.status_code, 200) | ||
| response_data = response.json() | ||
| self.assertEqual(len(response_data["results"]), 1) | ||
| self.assertEqual( | ||
| response_data["results"][0]["id"], self.questionnaire_response["id"] | ||
| ) |
Contributor
There was a problem hiding this comment.
These filter tests are a bit too easy to satisfy.
There is only one questionnaire response in the fixture set, so both assertions still pass if questionnaire_slugs or only_unstructured is ignored entirely. Add a second response that should be excluded, or use a non-matching slug case, so the filter behavior is actually exercised.
As per coding guidelines, **/tests/**/*.py: Use Django’s built-in tools for testing (unittest and pytest-django) to ensure code quality and reliability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/tests/test_questionnaire_response_api.py` around lines 379 - 413,
The two tests
(test_retrieve_questionnaire_response_with_questionnaire_slug_filter and
test_retrieve_questionnaire_response_with_only_unstructured_filter) are too weak
because only one QuestionnaireResponse exists in fixtures; modify the tests to
create or attach a second QuestionnaireResponse that should be excluded by the
filter (or query using a non-matching slug) and then assert the filtered results
length is 1 and that the excluded response is not present; specifically, in each
test set up a second response (e.g., another QuestionnaireResponse object with a
different questionnaire slug or with structured answers) before calling
self.client.get(self.get_url(), ...) and then assert the
response.json()["results"] contains only self.questionnaire_response["id"] and
not the second response's id so the filter behavior in the view is actually
exercised.
Comment on lines
+555
to
+585
| def test_update_questionnaire_response_status_after_time_limit(self): | ||
| """ | ||
| Tests that updating the status of a questionnaire response after the allowed time limit results in a permission denied error. | ||
| """ | ||
| user = self.create_user() | ||
| self.client.force_authenticate(user=user) | ||
| self.attach_role_facility_organization_user( | ||
| self.facility_organization, | ||
| user, | ||
| self.role, | ||
| ) | ||
| update_url = self.get_detail_url(self.questionnaire_response["id"]) | ||
| response = self.client.put( | ||
| update_url, {"status": "entered_in_error"}, format="json" | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
| # Simulate time passing beyond the allowed limit | ||
| questionnaire_response_obj = QuestionnaireResponse.objects.get( | ||
| external_id=self.questionnaire_response["id"] | ||
| ) | ||
| questionnaire_response_obj.created_date -= timedelta( | ||
| minutes=settings.QUESTIONNAIRE_ERRORED_TIME_LIMIT_MINUTES + 1 | ||
| ) | ||
| questionnaire_response_obj.save() | ||
| # Ensure the object is refreshed from DB to avoid stale data | ||
| questionnaire_response_obj.refresh_from_db() | ||
| response = self.client.put(update_url, {"status": "completed"}, format="json") | ||
| self.assertEqual(response.status_code, 403) | ||
| self.assertEqual( | ||
| response.json()["detail"], | ||
| "Questionnaire Response cannot be edited", |
Contributor
There was a problem hiding this comment.
The time-limit test never reaches the time-limit branch.
In care/emr/api/viewsets/questionnaire_response.py:37-49, authorize_update rejects entered_in_error before it checks created_date. This test flips the response to entered_in_error first, so the final 403 is satisfied by the wrong guard. Backdate the still-completed response, then make the first update attempt and expect 403.
Suggested fix
update_url = self.get_detail_url(self.questionnaire_response["id"])
- response = self.client.put(
- update_url, {"status": "entered_in_error"}, format="json"
- )
- self.assertEqual(response.status_code, 200)
- # Simulate time passing beyond the allowed limit
questionnaire_response_obj = QuestionnaireResponse.objects.get(
external_id=self.questionnaire_response["id"]
)
questionnaire_response_obj.created_date -= timedelta(
minutes=settings.QUESTIONNAIRE_ERRORED_TIME_LIMIT_MINUTES + 1
)
- questionnaire_response_obj.save()
- # Ensure the object is refreshed from DB to avoid stale data
- questionnaire_response_obj.refresh_from_db()
- response = self.client.put(update_url, {"status": "completed"}, format="json")
+ questionnaire_response_obj.save(update_fields=["created_date"])
+
+ response = self.client.put(
+ update_url, {"status": "entered_in_error"}, format="json"
+ )
self.assertEqual(response.status_code, 403)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/tests/test_questionnaire_response_api.py` around lines 555 - 585,
The test is hitting the authorize_update guard that rejects "entered_in_error"
before the created_date check in authorize_update (in
questionnaire_response.py), so change
test_update_questionnaire_response_status_after_time_limit to backdate the
QuestionnaireResponse while its status is still "completed" (fetch
QuestionnaireResponse via QuestionnaireResponse.objects.get(external_id=...),
decrement created_date by settings.QUESTIONNAIRE_ERRORED_TIME_LIMIT_MINUTES + 1,
save and refresh_from_db), then perform the PUT to change status to
"entered_in_error" and assert a 403 with "Questionnaire Response cannot be
edited" so the time-limit branch is exercised instead of the entered_in_error
guard.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3609 +/- ##
===========================================
+ Coverage 77.20% 77.32% +0.12%
===========================================
Files 474 474
Lines 22421 22421
Branches 2348 2348
===========================================
+ Hits 17310 17338 +28
+ Misses 4531 4501 -30
- Partials 580 582 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
ohcnetwork/roadmap#252
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
Tests