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
📝 WalkthroughWalkthroughA new test module for the FormSubmission viewset is introduced, providing comprehensive coverage of list, create, retrieve, update, and delete endpoints, including permission validation, filter enforcement, and completed encounter restrictions across HTTP methods. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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.
🧹 Nitpick comments (3)
care/emr/tests/test_form_submission_api.py (3)
64-72: Consolidate duplicated permission helper setup.Both helpers repeat the same role-attachment workflow; a single parameterized helper would reduce copy/paste drift (which we all enjoy maintaining until we don’t).
Proposed refactor
+ def _grant_submit_permission(self, permission_name): + permissions = [permission_name] + role = self.create_role_with_permissions(permissions) + self.attach_role_facility_organization_user(self.organization, self.user, role) + def _grant_patient_submit_permission(self): - permissions = [PatientPermissions.can_submit_patient_questionnaire.name] - role = self.create_role_with_permissions(permissions) - self.attach_role_facility_organization_user(self.organization, self.user, role) + self._grant_submit_permission( + PatientPermissions.can_submit_patient_questionnaire.name + ) def _grant_encounter_submit_permission(self): - permissions = [EncounterPermissions.can_submit_encounter_questionnaire.name] - role = self.create_role_with_permissions(permissions) - self.attach_role_facility_organization_user(self.organization, self.user, role) + self._grant_submit_permission( + EncounterPermissions.can_submit_encounter_questionnaire.name + )As per coding guidelines, "Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_form_submission_api.py` around lines 64 - 72, The two helpers _grant_patient_submit_permission and _grant_encounter_submit_permission duplicate the same role creation and attachment logic; replace them with a single parameterized helper (e.g., _grant_submit_permission or _grant_permission) that accepts the permission name or enum value, calls create_role_with_permissions([permission]) and attach_role_facility_organization_user(self.organization, self.user, role), then update callers to use the new helper; remove the two duplicated methods to keep tests DRY and PEP8-compliant.
227-236: Use the created response ID instead of “latest row” selection.
order_by("-id").first()is a bit looser than needed here; selecting by returnedidmakes the assertion deterministic and easier to reason about.Proposed refactor
response = self.client.post(self.base_url, data, format="json") self.assertEqual(response.status_code, 200) - submission = FormSubmission.objects.order_by("-id").first() + submission = FormSubmission.objects.get(external_id=response.json()["id"]) self.assertEqual(submission.patient, self.encounter.patient)As per coding guidelines, "Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_form_submission_api.py` around lines 227 - 236, The test test_create_with_encounter_sets_patient_from_encounter uses a loose lookup FormSubmission.objects.order_by("-id").first() to assert the created submission’s patient; instead, extract the created submission id from the POST response (e.g. response.data or response.json() id field returned by self.client.post) and query FormSubmission.objects.get(pk=that_id) to make the assertion deterministic and clear, replacing the order_by("-id").first() lookup with a get by id.
191-198: Rename “invalid UUID” tests to “nonexistent UUID” for precision.These cases use valid UUIDs that do not exist in DB. Renaming keeps intent crisp and avoids future confusion when malformed-UUID tests are added.
As per coding guidelines, "Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables)."
Also applies to: 257-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_form_submission_api.py` around lines 191 - 198, Rename the misleading "invalid_uuid" test functions to "nonexistent_uuid" to reflect that they use well-formed UUIDs not present in the DB: change test_list_with_invalid_patient_uuid -> test_list_with_nonexistent_patient_uuid and test_list_with_invalid_encounter_uuid -> test_list_with_nonexistent_encounter_uuid (and apply the same renaming pattern to the similar tests referenced at lines 257-269). Update any local references/usages of those function names (e.g., in test discovery or comments) so the new names are used consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@care/emr/tests/test_form_submission_api.py`:
- Around line 64-72: The two helpers _grant_patient_submit_permission and
_grant_encounter_submit_permission duplicate the same role creation and
attachment logic; replace them with a single parameterized helper (e.g.,
_grant_submit_permission or _grant_permission) that accepts the permission name
or enum value, calls create_role_with_permissions([permission]) and
attach_role_facility_organization_user(self.organization, self.user, role), then
update callers to use the new helper; remove the two duplicated methods to keep
tests DRY and PEP8-compliant.
- Around line 227-236: The test
test_create_with_encounter_sets_patient_from_encounter uses a loose lookup
FormSubmission.objects.order_by("-id").first() to assert the created
submission’s patient; instead, extract the created submission id from the POST
response (e.g. response.data or response.json() id field returned by
self.client.post) and query FormSubmission.objects.get(pk=that_id) to make the
assertion deterministic and clear, replacing the order_by("-id").first() lookup
with a get by id.
- Around line 191-198: Rename the misleading "invalid_uuid" test functions to
"nonexistent_uuid" to reflect that they use well-formed UUIDs not present in the
DB: change test_list_with_invalid_patient_uuid ->
test_list_with_nonexistent_patient_uuid and
test_list_with_invalid_encounter_uuid ->
test_list_with_nonexistent_encounter_uuid (and apply the same renaming pattern
to the similar tests referenced at lines 257-269). Update any local
references/usages of those function names (e.g., in test discovery or comments)
so the new names are used consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c2a1cf02-4521-411d-b784-86f4be2399ae
📒 Files selected for processing (1)
care/emr/tests/test_form_submission_api.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3622 +/- ##
===========================================
+ Coverage 77.20% 77.37% +0.16%
===========================================
Files 474 474
Lines 22421 22421
Branches 2348 2348
===========================================
+ Hits 17310 17348 +38
+ Misses 4531 4492 -39
- Partials 580 581 +1 ☔ 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.
Proposed Changes
Associated Issue
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