Adding tests for questionnaire and few minor changes #2745
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the questionnaire API's permission handling and validation logic. Changes span across multiple files, focusing on refining permission checks in the Changes
Possibly related PRs
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: 0
🧹 Nitpick comments (5)
care/emr/tests/test_questionnaire_api.py (1)
224-249: Consider testing all question types for comprehensive validationWhile the
test_individual_invalid_submissionsmethod covers several question types, it omits tests for'string','text', and'structured'. Including these types would ensure that all question variants are properly validated, preventing potential issues with untested types.care/emr/resources/questionnaire/utils.py (4)
30-40: Would it kill you to add some type hints?The function looks good, but it could benefit from:
- Type hints for parameters and return value
- Validation that answer_option is actually a list
-def get_valid_choices(question): +def get_valid_choices(question: dict) -> list[str]: """ Extracts valid choices from a choice question dictionary. + + Args: + question: The question dictionary containing answer options + Returns: + List of valid choice values + Raises: + ValueError: If no answer options are found """ answer_options = question.get("answer_option", []) - if not answer_options: + if not isinstance(answer_options, list) or not answer_options: error = f"No 'answer_option' found in question with id {question.get('id')}." raise ValueError(error)
66-68: Perhaps we could make the date formats a bit more... informative?The switch to explicit format strings is good, but consider:
- Moving format strings to constants
- Including the expected format in error messages
+DATE_FORMAT = "%Y-%m-%d" +DATETIME_FORMAT = "%Y-%m-%dT%H:%M:%S" +TIME_FORMAT = "%H:%M:%S" - datetime.strptime(value.value, "%Y-%m-%d").date() # noqa DTZ007 + datetime.strptime(value.value, DATE_FORMAT).date() # noqa DTZ007 - datetime.strptime(value.value, "%Y-%m-%dT%H:%M:%S") # noqa DTZ007 + datetime.strptime(value.value, DATETIME_FORMAT) # noqa DTZ007And in the error handling:
- errors.append(f"Invalid {value_type}") + errors.append(f"Invalid {value_type}. Expected format: {DATE_FORMAT if value_type == QuestionType.date.value else DATETIME_FORMAT}")
74-77: The URL validation is a bit... minimalist, isn't it?While checking scheme and netloc is a good start, consider:
- More descriptive error messages
- Additional validations for common URL issues
elif value_type == QuestionType.url.value: parsed = urlparse(value.value) if not all([parsed.scheme, parsed.netloc]): - errors.append(f"Invalid {value_type}") + missing = [] + if not parsed.scheme: + missing.append("scheme (e.g., http, https)") + if not parsed.netloc: + missing.append("network location (e.g., example.com)") + errors.append(f"Invalid URL: missing {' and '.join(missing)}") + elif parsed.scheme not in ['http', 'https']: + errors.append(f"Invalid URL: scheme must be http or https")
42-42: That noqa PLR0912 is practically begging for a refactor...The
validate_datafunction has grown quite complex. Consider splitting it into smaller, focused validation functions for each type:
- validate_numeric_value
- validate_date_value
- validate_choice_value
- validate_url_value
This would improve maintainability and make it easier to add new validations in the future. Would you like me to provide an example of how this could be structured?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
care/emr/api/viewsets/questionnaire.py(2 hunks)care/emr/resources/questionnaire/utils.py(3 hunks)care/emr/tests/test_questionnaire_api.py(1 hunks)care/utils/tests/base.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (3)
care/emr/tests/test_questionnaire_api.py (1)
620-653: Verify that submittingNonevalues activates the questionnaire as intendedIn
test_active_questionnaire_modification_prevented, the test submits a response withNonevalues to activate the questionnaire. It would be wise to confirm that this approach effectively activates the questionnaire without causing validation errors, ensuring the test accurately reflects the desired behavior.care/utils/tests/base.py (1)
16-19: Addition ofcreate_super_usermethod enhances test capabilitiesThe
create_super_usermethod is correctly implemented, allowing for efficient creation of superuser accounts in tests. This addition will facilitate testing scenarios that require elevated privileges.care/emr/api/viewsets/questionnaire.py (1)
Line range hint
83-88: Confirm intention behind update permission changesThe
permissions_controllermethod no longer includes"update"in the actions requiringcan_write_questionnairepermission. Instead, updates are now restricted to superusers via theauthorize_updatemethod. Just to be sure, please verify that this change aligns with the desired permission model and that regular users with write permissions are no longer expected to update questionnaires.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2745 +/- ##
===========================================
+ Coverage 63.09% 64.71% +1.61%
===========================================
Files 252 252
Lines 12718 12732 +14
Branches 1114 1119 +5
===========================================
+ Hits 8025 8240 +215
+ Misses 4626 4384 -242
- Partials 67 108 +41 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
care/emr/tests/test_symptom_api.py (2)
348-373: I suppose the docstring could be a bit more descriptive...While the test implementation is solid, the docstring could better explain what constitutes a "different patient" and why this validation is important for data integrity.
Consider expanding the docstring:
- Users with `can_write_encounter` on a encounter with different patient => (HTTP 400). + Tests that creating a symptom fails with HTTP 400 when the encounter's patient + doesn't match the patient specified in the URL, even with valid permissions. + This prevents accidentally associating symptoms with the wrong patient.
374-398: Oh, there seems to be a tiny discrepancy in the docstring...The docstring mentions "incomplete encounter" but the test is actually about an invalid (non-existent) encounter ID. These are quite different scenarios, wouldn't you agree?
Here's a more accurate docstring:
- Users with `can_write_encounter` on a incomplete encounter => (HTTP 400). + Tests that creating a symptom fails with HTTP 400 when using a non-existent + encounter ID, even with valid permissions. This prevents creating symptoms + for invalid encounters.care/emr/tests/test_diagnosis_api.py (1)
348-373: I couldn't help but notice the striking similarity with the symptom tests...The test implementation is identical to
test_create_symptoms_with_permissions_with_mismatched_patient_idintest_symptom_api.py. Perhaps we could reduce this duplication?Consider creating a base test class with shared test methods:
class BaseConditionTestCase(CareAPITestBase): def test_create_condition_with_mismatched_patient_id(self): """Template method for testing patient ID mismatch""" permissions = [EncounterPermissions.can_write_encounter.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.create_patient(), facility=self.facility, organization=self.organization, status=None, ) condition_data = self.generate_data_for_condition(encounter) response = self.client.post(self.base_url, condition_data, format="json") response_data = response.json() self.assertEqual(response.status_code, 400) self.assertIn("errors", response_data) error = response_data["errors"][0] self.assertEqual(error["type"], "validation_error") self.assertIn( "Patient external ID mismatch with encounter's patient", error["msg"] ) class TestSymptomViewSet(BaseConditionTestCase): condition_type = "symptom" # Override setup methods as needed class TestDiagnosisViewSet(BaseConditionTestCase): condition_type = "diagnosis" # Override setup methods as needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/tests/test_diagnosis_api.py(2 hunks)care/emr/tests/test_symptom_api.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
care/emr/tests/test_diagnosis_api.py (1)
374-398: Ah, we meet again, duplicate code and inaccurate docstring...The same issues appear here as in the symptom tests:
- The test is identical to
test_create_symptom_with_permissions_with_invalid_encounter_id- The docstring mentions "incomplete encounter" instead of "invalid encounter"
This could be addressed by:
- Moving the test to the suggested base class in the previous comment
- Updating the docstring as suggested in the symptom test review
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
care/emr/resources/patient/spec.py (1)
83-84: Inconsistent age-related fields between specs.
PatientCreateSpecuses bothageanddate_of_birth, whilePatientListSpecnow usesdate_of_birthandyear_of_birth. This inconsistency might lead to confusion. Perhaps we should standardize the approach across all specs?Consider:
- Using only
date_of_birthconsistently across specs- Computing age and year_of_birth as properties when needed
- Documenting the rationale for having different fields in different specs if this is intentional
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/resources/patient/spec.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
| year_of_birth: datetime.date | None = None | ||
|
|
There was a problem hiding this comment.
Type hint for year_of_birth seems... interesting.
The year_of_birth is defined as datetime.date | None, but based on PatientCreateSpec.perform_extra_deserialization, it's actually stored as an integer year value. Perhaps we should align the type hints with reality?
Apply this diff to fix the type hint:
- year_of_birth: datetime.date | None = None
+ year_of_birth: int | None = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| year_of_birth: datetime.date | None = None | |
| year_of_birth: int | None = None | |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
care/emr/migrations/0009_medicationrequest_authored_on.py (1)
9-9: I see we're mixing quote styles...The static analysis tool suggests using double quotes consistently. Not that it matters much, but consistency is... nice.
- ('emr', '0008_medicationrequest_medication'), + ("emr", "0008_medicationrequest_medication"), - model_name='medicationrequest', - name='authored_on', + model_name="medicationrequest", + name="authored_on",Also applies to: 14-15
🧰 Tools
🪛 Ruff (0.8.2)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/migrations/0009_medicationrequest_authored_on.py(1 hunks)care/emr/models/medication_request.py(1 hunks)care/emr/resources/medication/request/spec.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/migrations/0009_medicationrequest_authored_on.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (1)
care/emr/models/medication_request.py (1)
19-19: Would it be too much trouble to add a docstring explaining the field's purpose?The
authored_onfield seems quite permissive withnull=True, blank=True, default=None. I'm sure there's a good reason for allowing empty values, but it might be worth documenting why this timestamp can be empty, especially since it tracks when the medication request was authored.Let's check if there are any existing medication requests that might be affected:
✅ Verification successful
Documentation would be nice, but the permissive field is actually fine
Looking at the codebase, all fields in the
MedicationRequestmodel follow this same permissive pattern, which is quite typical for EMR systems following FHIR-like structures where data completeness isn't guaranteed at creation time. Though I still think a tiny docstring wouldn't hurt... you know, for posterity? 😊🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any existing medication requests # that might need the authored_on field populated rg -l "MedicationRequest\.objects\.create" | xargs -I {} rg -A 5 "MedicationRequest\.objects\.create" {}Length of output: 110
Script:
#!/bin/bash # Search for MedicationRequest usage patterns echo "=== Searching for MedicationRequest class usage ===" rg "class MedicationRequest" -A 5 echo -e "\n=== Searching for MedicationRequest imports ===" rg "from.*MedicationRequest.*import" echo -e "\n=== Searching for medication request in test files ===" fd -e py test | xargs rg -l "MedicationRequest" echo -e "\n=== Checking model relationships ===" rg "ForeignKey.*MedicationRequest"Length of output: 5537
| @@ -221,6 +221,7 @@ class BaseMedicationRequestSpec(MedicationRequestResource): | |||
| encounter: UUID4 | |||
|
|
|||
| dosage_instruction: list[DosageInstruction] = Field() | |||
| authored_on: datetime | |||
There was a problem hiding this comment.
Oh, interesting choice of field definition...
The authored_on field in the spec (datetime) doesn't quite match the model's permissiveness (null=True, blank=True, default=None). This could lead to some... let's say, interesting validation scenarios. Perhaps we should align these?
Consider updating the spec to match the model:
- authored_on: datetime
+ authored_on: datetime | None = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| authored_on: datetime | |
| authored_on: datetime | None = None |
| "date": "invalid-date", | ||
| "dateTime": "01-16-2025T10:30:00", | ||
| "time": "25:61:00", | ||
| "choice": "INVALID_CHOICE", |
There was a problem hiding this comment.
lets add some randomness here to ensure that this is always an invalid choice
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
New Features
authored_onfield to medication requests for tracking submission timestamps.Bug Fixes
Tests