adding test for diagnosis viewset #2729
Conversation
📝 WalkthroughWalkthroughThis pull request introduces enhancements to testing documentation and functionality. The Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
care/emr/tests/test_symptom_api.py (2)
167-170: I see you're keeping us on our toes with that indentation.The pipeline caught this one. Mind fixing the indentation to match the rest of the file?
🧰 Tools
🪛 GitHub Actions: Lint Code Base
[warning] 167-170: Code formatting issue: Incorrect indentation fixed by ruff-format
248-284: The test is great, but the name could be more... specific.Consider renaming to better reflect both aspects being tested:
test_create_symptom_with_organisation_user_can_view_but_not_create_across_organizationscare/emr/tests/test_diagnosis_api.py (2)
98-102: Oh, those pesky plural forms!There's a typo in the test names: "diagnosiss" should be "diagnoses". This appears in multiple test methods.
- def test_list_diagnosiss_with_permissions(self): + def test_list_diagnoses_with_permissions(self):
1-621: Impressive test coverage, though we could make it a bit DRYer.Consider extracting common test setup patterns into helper methods. For example, the permission setup and encounter creation appear frequently:
def setup_user_with_permissions(self, permissions): role = self.create_role_with_permissions(permissions) self.attach_role_facility_organization_user(self.organization, self.user, role) return role def create_test_encounter(self, status=None): return self.create_encounter( patient=self.patient, facility=self.facility, organization=self.organization, status=status, )🧰 Tools
🪛 Ruff (0.8.2)
424-424:
(T201)
621-621: No newline at end of file
Add trailing newline
(W292)
🪛 GitHub Actions: Lint Code Base
[error] 424-424: T201:
[error] 618-618: Missing newline at end of file
[warning] 167-170: Code formatting issue: Incorrect indentation fixed by ruff-format
CONTRIBUTING.md (1)
125-128: The new test commands documentation looks good, but...Consider adding a concrete example to make it even clearer:
# To run a specific test file, class, or method: make test path=<path_to_test> +# Example: +# make test path=care/emr/tests/test_diagnosis_api.py:TestDiagnosisViewSet.test_create_diagnosis_with_permissions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CONTRIBUTING.md(1 hunks)Makefile(1 hunks)care/emr/tests/test_diagnosis_api.py(1 hunks)care/emr/tests/test_symptom_api.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/tests/test_diagnosis_api.py
424-424: print found
(T201)
621-621: No newline at end of file
Add trailing newline
(W292)
care/emr/tests/test_symptom_api.py
620-620: No newline at end of file
Add trailing newline
(W292)
🪛 GitHub Actions: Lint Code Base
care/emr/tests/test_diagnosis_api.py
[error] 424-424: T201: print statement found in test file. Remove print statement.
[error] 618-618: Missing newline at end of file
[warning] 167-170: Code formatting issue: Incorrect indentation fixed by ruff-format
care/emr/tests/test_symptom_api.py
[error] 617-617: Missing newline at end of file
[warning] 167-170: Code formatting issue: Incorrect indentation fixed by ruff-format
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
care/emr/tests/test_symptom_api.py (1)
223-235: Nice work on the cross-organization permission test!Good coverage of the security boundary between organizations. The test setup is clear and the assertions verify the expected behavior.
Makefile (1)
56-56: Nice enhancement to the test command!The addition of the
$(path)variable makes the test command more flexible. This will be particularly useful for running specific test cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
care/emr/tests/test_diagnosis_api.py (4)
21-44: Consider adding docstring to the test class.While the test methods have docstrings, the test class itself could use a docstring explaining its purpose and the overall testing strategy. I mean, it's not like we're mind readers or anything.
class TestDiagnosisViewSet(CareAPITestBase): + """ + Test suite for DiagnosisViewSet API endpoints. + + Tests CRUD operations with various permission combinations and encounter statuses. + """ def setUp(self):
58-76: Consider using more descriptive parameter names.The
**kwargsparameter increate_diagnosisis a bit... mysterious. It might be helpful to list the expected kwargs in the docstring, you know, for those of us who appreciate documentation.def create_diagnosis(self, encounter, patient, **kwargs): + """ + Create a diagnosis for testing. + + Args: + encounter: The encounter associated with the diagnosis + patient: The patient associated with the diagnosis + **kwargs: Additional fields for Condition model (e.g., notes, onset_date) + """
419-424: Consider using constants for severity values.Instead of hardcoding the string "mild", consider using the
SeverityChoicesenum that's already imported. Just a thought, you know, for consistency's sake.- diagnosis_data_updated["severity"] = "mild" + diagnosis_data_updated["severity"] = SeverityChoices.mild.value
1-620: Consider adding edge case tests.While the test coverage is good, there are some edge cases that might be worth testing:
- Creating/updating a diagnosis with invalid code format
- Testing with malformed encounter IDs
- Testing concurrent updates to the same diagnosis
Not that I'm saying the current tests aren't good enough, but you know, there's always room for improvement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/tests/test_diagnosis_api.py(1 hunks)care/emr/tests/test_symptom_api.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- care/emr/tests/test_symptom_api.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
care/emr/tests/test_diagnosis_api.py (2)
1-18: LGTM! Clean and well-organized imports.The imports are logically grouped and follow a clear pattern: standard library, Django, third-party, and local imports.
538-620: LGTM! Comprehensive delete test cases.The delete tests cover all necessary permission combinations and edge cases. Nice work on being thorough with the test coverage!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2729 +/- ##
===========================================
+ Coverage 46.72% 46.74% +0.02%
===========================================
Files 313 313
Lines 17697 17697
Branches 1704 1704
===========================================
+ Hits 8269 8273 +4
+ Misses 9359 9355 -4
Partials 69 69 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
care/emr/tests/test_diagnosis_api.py (5)
1-1: Consider usingrandom.choiceinstead ofsecrets.choiceWhile
secrets.choiceprovides cryptographically secure random selections, it's a bit... excessive for generating test data.random.choicewould be more appropriate here, unless you're testing something specifically related to security.-from secrets import choice +from random import choice
48-56: Add type hints to improve code clarityThe helper methods would benefit from type hints. It would make the code more maintainable and self-documenting, especially for methods like
_get_diagnosis_urlandcreate_diagnosis.- def _get_diagnosis_url(self, diagnosis_id): + def _get_diagnosis_url(self, diagnosis_id: str) -> str: - def create_diagnosis(self, encounter, patient, **kwargs): + def create_diagnosis(self, encounter: Encounter, patient: Patient, **kwargs) -> Condition: - def generate_data_for_diagnosis(self, encounter, **kwargs): + def generate_data_for_diagnosis(self, encounter: Encounter, **kwargs) -> dict:Also applies to: 58-76, 78-95
97-203: Add pagination test for list endpointWhile the permission tests are quite thorough (almost suspiciously so), there's no test for pagination behavior. Consider adding a test case that verifies the endpoint correctly handles large result sets.
Here's a suggested test case:
def test_list_diagnosis_pagination(self): """ Test that the list endpoint correctly paginates results """ permissions = [PatientPermissions.can_view_clinical_data.name] role = self.create_role_with_permissions(permissions) self.attach_role_facility_organization_user(self.organization, self.user, role) encounter = self.create_encounter( patient=self.patient, facility=self.facility, organization=self.organization, ) # Create more diagnoses than the default page size for _ in range(11): # Assuming default page size is 10 self.create_diagnosis(encounter=encounter, patient=self.patient) response = self.client.get(self.base_url) self.assertEqual(response.status_code, 200) self.assertTrue('next' in response.json()) self.assertEqual(len(response.json()['results']), 10)
420-559: Add test for partial updates using PATCHThe update tests are quite extensive (maybe someone really likes testing permissions?), but they're missing tests for partial updates using PATCH.
Here's a suggested test case:
def test_partial_update_diagnosis(self): """ Test that PATCH requests correctly update only specified fields """ permissions = [ EncounterPermissions.can_write_encounter.name, PatientPermissions.can_view_clinical_data.name, ] role = self.create_role_with_permissions(permissions) self.attach_role_facility_organization_user(self.organization, self.user, role) encounter = self.create_encounter( patient=self.patient, facility=self.facility, organization=self.organization, ) diagnosis = self.create_diagnosis(encounter=encounter, patient=self.patient) url = self._get_diagnosis_url(diagnosis.external_id) patch_data = {"severity": "mild"} response = self.client.patch(url, patch_data, format="json") self.assertEqual(response.status_code, 200) self.assertEqual(response.json()["severity"], "mild")
560-642: Add test for cascade deletion behaviorThe delete tests cover permissions thoroughly (perhaps a bit too thoroughly?), but they're missing a test to verify cascade deletion behavior with related objects.
Here's a suggested test case:
def test_delete_diagnosis_cascade_behavior(self): """ Test that deleting a diagnosis properly handles related objects """ permissions = [ EncounterPermissions.can_write_encounter.name, PatientPermissions.can_view_clinical_data.name, ] role = self.create_role_with_permissions(permissions) self.attach_role_facility_organization_user(self.organization, self.user, role) encounter = self.create_encounter( patient=self.patient, facility=self.facility, organization=self.organization, ) diagnosis = self.create_diagnosis(encounter=encounter, patient=self.patient) # Create related objects here if any url = self._get_diagnosis_url(diagnosis.external_id) response = self.client.delete(url) self.assertEqual(response.status_code, 204) # Verify related objects are handled correctly
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/tests/test_diagnosis_api.py(1 hunks)care/emr/tests/test_symptom_api.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- care/emr/tests/test_symptom_api.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
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
Release Notes
Documentation
Tests