test: add coverage for created_by filter and tag config org clearing - #3713
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds API test coverage for filtering questionnaire responses by creator and for clearing omitted organization-scoping fields during tag configuration updates. ChangesQuestionnaire response filtering
Tag configuration update semantics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
care/emr/tests/test_questionnaire_response_api.py (1)
477-495: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert exact result counts for tighter tests.
While asserting for specific inclusions and exclusions works fine, it leaves the door open for unexpected records (like the pre-existing response created by
self.userinsetUp) if the filter misbehaves in weird ways. Checking the exact length of the returned array is slightly more robust, though I'm sure you already knew that.♻️ Proposed refactor for stricter assertions
# Filter by user_a response = self.client.get( self.get_url(), {"created_by": str(user_a.external_id)} ) self.assertEqual(response.status_code, 200) results = response.json()["results"] - result_ids = [res["id"] for res in results] - self.assertIn(response_a["id"], result_ids) - self.assertNotIn(response_b["id"], result_ids) + self.assertEqual(len(results), 1) + self.assertEqual(results[0]["id"], response_a["id"]) # Filter by user_b response = self.client.get( self.get_url(), {"created_by": str(user_b.external_id)} ) self.assertEqual(response.status_code, 200) results = response.json()["results"] - result_ids = [res["id"] for res in results] - self.assertIn(response_b["id"], result_ids) - self.assertNotIn(response_a["id"], result_ids) + self.assertEqual(len(results), 1) + self.assertEqual(results[0]["id"], response_b["id"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@care/emr/tests/test_questionnaire_response_api.py` around lines 477 - 495, Update the test method containing the user_a and user_b filter requests to assert the exact result count for each response before checking included and excluded IDs. Ensure each filtered result set contains only the expected single record, while preserving the existing membership assertions.care/emr/tests/test_tag_config_api.py (1)
438-444: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider preserving the
facilityin the update payload.You omitted the
facilityfield in the PUT payload. This likely clears it as well, converting the tag to a global one. While the test still happens to pass for your specific assertion, passingfacility=self.facility.external_idwould actually isolate the test to just thefacility_organizationfield. I suppose this is fine if you're not overly concerned with test precision.💡 Proposed change
response = self.client.put( self.get_detail_url(tag_config.external_id), self.generate_tag_config_data( resource=TagResource.encounter.value, + facility=self.facility.external_id, ), format="json", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@care/emr/tests/test_tag_config_api.py` around lines 438 - 444, Update the PUT payload in the test using generate_tag_config_data to include facility=self.facility.external_id, preserving the existing facility while changing only the facility_organization-related value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@care/emr/tests/test_questionnaire_response_api.py`:
- Around line 477-495: Update the test method containing the user_a and user_b
filter requests to assert the exact result count for each response before
checking included and excluded IDs. Ensure each filtered result set contains
only the expected single record, while preserving the existing membership
assertions.
In `@care/emr/tests/test_tag_config_api.py`:
- Around line 438-444: Update the PUT payload in the test using
generate_tag_config_data to include facility=self.facility.external_id,
preserving the existing facility while changing only the
facility_organization-related value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf934a54-4437-4d9d-b25e-83f69678f5c7
📒 Files selected for processing (2)
care/emr/tests/test_questionnaire_response_api.pycare/emr/tests/test_tag_config_api.py
There was a problem hiding this comment.
Pull request overview
Adds missing backend test coverage for two previously merged behaviors: filtering questionnaire responses by creator (created_by) and clearing organization / facility_organization when omitted in tag config PUT updates.
Changes:
- Add tests validating
created_byUUID filter on questionnaire response listing. - Add tests validating that omitting
organizationclears it toNoneon tag config updates. - Add tests validating that omitting
facility_organizationclears it toNoneon tag config updates.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| care/emr/tests/test_tag_config_api.py | Adds tests for clearing organization and facility_organization when omitted in PUT updates. |
| care/emr/tests/test_questionnaire_response_api.py | Adds a test for created_by filter behavior on questionnaire response list endpoint. |
Greptile SummaryThis PR adds test coverage for two features that shipped without tests: the
Confidence Score: 5/5Test-only change adding coverage for two previously untested features; no production code is modified. Both new test methods are logically sound: the questionnaire filter test correctly isolates per-user submissions and asserts bidirectional filter results, and the tag config tests properly use baker.make + a real PUT + refresh_from_db to verify the clearing behavior. Superuser bypass of role checks is confirmed in the authorization layer, so the list call in the filter test is valid without explicit role attachment. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "test: incorporate code review feedback o..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@vigneshhari |
Proposed Changes
without tests.
created_byfilter on questionnaire responses (ENG-558, #3695):A
created_byUUID filter was added to the questionnaire response listendpoint but no tests were written for it. The new tests verify that
filtering by a user's
external_idreturns only that user's responsesand excludes responses created by other users.
Clearing
organization/facility_organizationon tag config update(ENG-580, #3690):
The tag config update spec (
TagConfigUpdateSpec.perform_extra_deserialization)was updated to explicitly set
organizationandfacility_organizationto
Nonewhen those fields are omitted from a PUT request — previouslythey would be left unchanged. The new tests verify that omitting either
field in an update actually clears the existing value to
Nonein thedatabase.
Merge Checklist
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit