Override Dob Or Age for Patients #2792
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
care/emr/resources/patient/spec.py (1)
77-77: Consider timezone edge cases in year calculation.Using
timezone.now()for year calculation could lead to incorrect results near year boundaries depending on the timezone. For instance, it might be December 31st in one timezone but January 1st in another.- obj.year_of_birth = timezone.now().date().year - self.age + # Use the organization's timezone or a reference timezone for consistency + reference_date = timezone.localtime(timezone.now(), obj.geo_organization.timezone).date() + obj.year_of_birth = reference_date.year - self.age
📜 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 (1)
- GitHub Check: test / test
🔇 Additional comments (2)
care/emr/resources/patient/spec.py (2)
74-78: 🛠️ Refactor suggestionConsider data loss implications when overriding date_of_birth.
When age is provided, setting
date_of_birthto None might lead to loss of precise birth date information that could be important for medical records. Perhaps we should store both values?Also, there's no validation for the age value. It might be worth adding some reasonable bounds, unless you enjoy having 200-year-old patients in your system. 😊
if self.age: + if not 0 <= self.age <= 150: + raise ValueError("Age must be between 0 and 150 years") # override dob if user chooses to update age obj.date_of_birth = None obj.year_of_birth = timezone.now().date().year - self.age
74-79: Verify the impact on existing features.The removal of the
is_updatecheck changes the behavior for update operations. Please ensure this doesn't affect any existing features that might depend on the previous behavior.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2792 +/- ##
===========================================
- Coverage 56.16% 56.14% -0.02%
===========================================
Files 215 215
Lines 10191 10205 +14
Branches 1030 1031 +1
===========================================
+ Hits 5724 5730 +6
- Misses 4451 4459 +8
Partials 16 16 ☔ 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_patient_api.py (3)
142-161: Perhaps consider separating the permission combinations into individual test cases?While the setup is comprehensive, it might be more maintainable to test each permission combination separately. Also, it seems you forgot to verify the calculated age from the initial date_of_birth.
def test_update_patient_age_and_date_of_birth(self): # ... existing setup ... self.assertEqual(response.data["date_of_birth"], "1993-01-10") self.assertEqual(response.data["year_of_birth"], 1993) + current_year = datetime.datetime.now(datetime.UTC).year + expected_age = current_year - 1993 + self.assertEqual(response.data["age"], expected_age)
172-177: The test seems to be missing some rather obvious assertions.While you're verifying the date_of_birth and year_of_birth, you might want to also verify the calculated age and ensure the age field is properly handled in the response.
self.assertEqual(response.data["date_of_birth"], "1992-01-10") self.assertEqual(response.data["year_of_birth"], 1992) +current_year = datetime.datetime.now(datetime.UTC).year +expected_age = current_year - 1992 +self.assertEqual(response.data["age"], expected_age) +self.assertNotIn("age", patient_data) # Verify age was removed from request
142-177: The test coverage seems a bit... minimal.Consider adding test cases for:
- Edge cases (e.g., age=0, age=120)
- Invalid inputs (e.g., future date_of_birth, negative age)
- Concurrent updates of both age and date_of_birth
Would you like me to generate the additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/tests/test_patient_api.py(2 hunks)
🔇 Additional comments (1)
care/emr/tests/test_patient_api.py (1)
162-171: Time-dependent test that might fail around New Year's Eve... just saying.The test calculates
year_of_birthusing the current year, which could lead to flaky tests around midnight on New Year's Eve. Also, it seems you're not verifying if the age field in the response matches the input age.patient_data["age"] = 33 response = self.client.put(reverse_url, patient_data, format="json") self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.data["date_of_birth"], None) +self.assertEqual(response.data["age"], 33) +# Consider freezing time or mocking datetime.now() to avoid time-dependent failures self.assertEqual( response.data["year_of_birth"], datetime.datetime.now(datetime.UTC).year - 33, )
Proposed Changes
Associated Issue
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