Add abatement and chronic_condition to conditions #2771
Conversation
📝 WalkthroughWalkthroughThis pull request introduces an Changes
Sequence DiagramsequenceDiagram
participant Model as Condition Model
participant Migration as Database Migration
participant Spec as Condition Specification
Migration->>Model: Add abatement JSONField
Migration->>Model: Update category field
Spec->>Model: Define abatement specification
Model-->>Spec: Validate abatement details
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 (
|
![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 (4)
care/emr/migrations/0010_condition_abatement.py (2)
6-12: Consider adding a batch size for large datasets.The category update looks fine, but for larger datasets, you might want to consider using
.iterator()and processing in batches. Not that I'm suggesting you have performance issues, but it's always good to plan ahead.def fix_encounter_diagnosis_category_case(apps, schema_editor): Condition = apps.get_model("emr", "Condition") - Condition.objects.filter(category="encounter-diagnosis").update(category="encounter_diagnosis") + batch_size = 1000 + queryset = Condition.objects.filter(category="encounter-diagnosis") + for batch in queryset.iterator(chunk_size=batch_size): + batch.category = "encounter_diagnosis" + batch.save()
17-29: Maintain consistent quote style.I noticed you're using single quotes. The static analysis tool suggests using double quotes to maintain consistency. Not a big deal, but consistency is nice.
dependencies = [ - ('emr', '0009_medicationrequest_authored_on'), + ("emr", "0009_medicationrequest_authored_on"), ] operations = [ migrations.AddField( - model_name='condition', - name='abatement', + model_name="condition", + name="abatement", field=models.JSONField(default=dict), ),🧰 Tools
🪛 Ruff (0.8.2)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/emr/resources/condition/spec.py (2)
53-58: Consider adding field validation for age and datetime.The
ConditionAbatementSpeclooks good, following the same pattern asConditionOnSetSpec. However, you might want to add some validation to ensureabatement_ageis non-negative andabatement_datetimeis not in the future. Just a thought.from pydantic import validator from datetime import datetime class ConditionAbatementSpec(EMRResource): # ... existing fields ... @validator("abatement_age") def validate_age(cls, v): if v is not None and v < 0: raise ValueError("Age cannot be negative") return v @validator("abatement_datetime") def validate_datetime(cls, v): if v is not None and v > datetime.now(): raise ValueError("Abatement datetime cannot be in the future") return v
73-73: Ensure consistent default value usage across specs.I notice you're using
{}in some places anddictin others for theabatementdefault. While both work, it might be better to stick to one style:# ConditionSpec abatement: ConditionAbatementSpec = {} # Line 73 # ConditionSpecRead abatement: ConditionAbatementSpec = dict # Line 114 # ConditionSpecUpdate abatement: ConditionAbatementSpec = {} # Line 136Also applies to: 114-114, 136-136
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/migrations/0010_condition_abatement.py(1 hunks)care/emr/models/condition.py(1 hunks)care/emr/resources/condition/spec.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/migrations/0010_condition_abatement.py
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
care/emr/models/condition.py (1)
16-16: Consider documenting the abatement field's purpose and structure.I see we're adding another JSONField with
default=dict. While I'm sure you know what you're doing, it might be helpful for others (and your future self) to document the expected structure and purpose of this field. Also, remember that usingdefault=dictcan lead to shared state issues if not handled properly.Let's check if there are any other JSONFields with similar patterns in the codebase:
✅ Verification successful
The
abatementfield follows established codebase patterns.Looking at the extensive use of
JSONField(default=dict)across the codebase, particularly in medical record models, this implementation is perfectly consistent with the project's conventions. The field name is actually quite self-documenting for those familiar with medical records, though I suppose a little documentation never hurt anyone... if you're into that sort of thing.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for JSONField usage patterns rg "JSONField\(default=dict" -A 2 -B 2Length of output: 51389
care/emr/resources/condition/spec.py (1)
36-37: Verify the impact of category enum changes.The addition of
chronic_conditionand modification ofencounter_diagnosismight affect existing code. I assume you've thoroughly tested this, but it wouldn't hurt to verify.✅ Verification successful
Everything seems perfectly fine with the category changes
The changes are properly handled through migration file
0010_condition_abatement.py. Thechronic_conditionis a new addition that doesn't affect existing code, and theencounter_diagnosisformat change is managed through proper data migration. I suppose we can all rest easy now.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of the modified category values echo "Searching for old category value usage..." rg "encounter-diagnosis" --type py echo "Searching for chronic_condition references..." rg "chronic_condition" --type pyLength of output: 638
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2771 +/- ##
===========================================
+ Coverage 64.72% 64.74% +0.02%
===========================================
Files 252 252
Lines 12733 12743 +10
Branches 1119 1119
===========================================
+ Hits 8241 8251 +10
Misses 4384 4384
Partials 108 108 ☔ View full report in Codecov by Sentry. |
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
New Features
Bug Fixes
Refactor