refact:added patient age in patient retrive spec#3546
refact:added patient age in patient retrive spec#3546nandkishorr wants to merge 8 commits intodevelopfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a public Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
nandkishorr
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
care/emr/resources/patient/spec.py (1)
140-145:⚠️ Potential issue | 🟠 MajorInconsistent age handling can silently drop newborn age updates.
Line 140 uses a truthy check (
if self.age:), while Line 186 correctly usesis not None. Soage=0is ignored on create but handled on update. Subtle, but definitely not fun to debug later.Suggested fix
- if self.age: + if self.age is not None: # override dob if user chooses to update age obj.date_of_birth = None obj.year_of_birth = timezone.now().date().year - self.ageAlso applies to: 186-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/resources/patient/spec.py` around lines 140 - 145, The code uses a truthy check on self.age (if self.age) which drops legitimate zero values (newborn age); change the condition to explicit "is not None" wherever age is checked (e.g., replace "if self.age:" with "if self.age is not None:" around the block that sets obj.date_of_birth = None and obj.year_of_birth = timezone.now().date().year - self.age) and make the same change in the other occurrence handling update (the block currently at the later age handling) so age=0 is accepted consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@care/emr/resources/patient/spec.py`:
- Around line 140-145: The code uses a truthy check on self.age (if self.age)
which drops legitimate zero values (newborn age); change the condition to
explicit "is not None" wherever age is checked (e.g., replace "if self.age:"
with "if self.age is not None:" around the block that sets obj.date_of_birth =
None and obj.year_of_birth = timezone.now().date().year - self.age) and make the
same change in the other occurrence handling update (the block currently at the
later age handling) so age=0 is accepted consistently.
| obj.date_of_birth = None | ||
| obj.year_of_birth = timezone.now().year - self.age | ||
| elif self.date_of_birth: | ||
| obj.date_of_birth = self.date_of_birth |
There was a problem hiding this comment.
while creating and updating , on de serialization the date_of_birth is converted to str (in-memory) So on serialize the get_age() will crash since using data from the same in-memory obj !
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
care/utils/tests/base.py (1)
10-12: Avoid pulling shared test helpers from a management command module.This works, but importing from
care.emr.management.commands.load_fixturescouples test utilities to command-layer code a bit more than needed. Please move this helper to a neutral utility module and import from there.As per coding guidelines,
**/*.py: 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/utils/tests/base.py` around lines 10 - 12, The test is importing generate_unique_indian_phone_number from a management command; extract that helper into a neutral utility module (e.g., a new function in a tests or utils module) and update imports: move the implementation of generate_unique_indian_phone_number out of the management command and into the new module, change care/utils/tests/base.py to import generate_unique_indian_phone_number from the new utility module, and update the original management command (load_fixtures) to import the helper from the new location so both tests and command use the shared, neutral utility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@care/utils/tests/base.py`:
- Line 100: Replace the non-deterministic default in the test factory call
kwargs.setdefault("date_of_birth", self.fake.date_of_birth()) with a stable
deterministic date (e.g., a fixed datetime.date like 1990-01-01) so age-based
assertions don't drift; keep using kwargs.setdefault so individual tests can
still override date_of_birth when needed and update any docstring/comment to
note the fixed default.
---
Nitpick comments:
In `@care/utils/tests/base.py`:
- Around line 10-12: The test is importing generate_unique_indian_phone_number
from a management command; extract that helper into a neutral utility module
(e.g., a new function in a tests or utils module) and update imports: move the
implementation of generate_unique_indian_phone_number out of the management
command and into the new module, change care/utils/tests/base.py to import
generate_unique_indian_phone_number from the new utility module, and update the
original management command (load_fixtures) to import the helper from the new
location so both tests and command use the shared, neutral utility.
| kwargs.setdefault("address", self.fake.address()) | ||
| kwargs.setdefault("pincode", self.fake.random_int(min=100000, max=999999)) | ||
| kwargs.setdefault("blood_group", secrets.choice(list(BloodGroupChoices)).value) | ||
| kwargs.setdefault("date_of_birth", self.fake.date_of_birth()) |
There was a problem hiding this comment.
Use a deterministic date_of_birth default to reduce flaky age-based tests.
Line 100 currently randomizes DOB, which can make age assertions drift over time. A fixed default is more reliable, and specific tests can still override it.
Proposed change
+from datetime import date
import secrets
import sys
from secrets import choice
@@
- kwargs.setdefault("date_of_birth", self.fake.date_of_birth())
+ kwargs.setdefault("date_of_birth", date(1990, 1, 1))As per coding guidelines, **/tests/**/*.py: Use Django’s built-in tools for testing (unittest and pytest-django) to ensure code quality and reliability.
📝 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.
| kwargs.setdefault("date_of_birth", self.fake.date_of_birth()) | |
| from datetime import date | |
| import secrets | |
| import sys | |
| from secrets import choice | |
| kwargs.setdefault("date_of_birth", date(1990, 1, 1)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/utils/tests/base.py` at line 100, Replace the non-deterministic default
in the test factory call kwargs.setdefault("date_of_birth",
self.fake.date_of_birth()) with a stable deterministic date (e.g., a fixed
datetime.date like 1990-01-01) so age-based assertions don't drift; keep using
kwargs.setdefault so individual tests can still override date_of_birth when
needed and update any docstring/comment to note the fixed default.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3546 +/- ##
===========================================
- Coverage 76.22% 75.20% -1.03%
===========================================
Files 474 475 +1
Lines 22270 22687 +417
Branches 2325 2364 +39
===========================================
+ Hits 16976 17061 +85
- Misses 4765 5096 +331
- Partials 529 530 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nandkishorr
changed the title
2 participants
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
Improvements
Tests