Alt phone number and qualificationspec #2767
Conversation
📝 WalkthroughWalkthroughThis pull request introduces significant changes across multiple files in the care/emr and care/users modules. The modifications primarily focus on enhancing user specifications, introducing a new Changes
Sequence DiagramsequenceDiagram
participant User
participant UserModel
participant QualificationSpec
participant PeriodSpec
User->>UserModel: Create/Update User
UserModel->>QualificationSpec: Validate Qualification
QualificationSpec->>PeriodSpec: Validate Period
PeriodSpec-->>QualificationSpec: Return Validated Period
QualificationSpec-->>UserModel: Return Validated Qualification
UserModel-->>User: Save User Data
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: 2
🔭 Outside diff range comments (1)
care/emr/resources/encounter/spec.py (1)
Line range hint
33-34: Avoid using mutable default values for Pydantic fieldsAssigning mutable default values like
{}to Pydantic model fields can lead to unexpected behavior because the same object is shared across all instances. Instead, usePeriodSpec()andHospitalizationSpec()to create new instances with default values.Apply this diff to fix the issue:
-period: PeriodSpec = {} -hospitalization: HospitalizationSpec | None = {} +period: PeriodSpec = PeriodSpec() +hospitalization: HospitalizationSpec | None = HospitalizationSpec()
🧹 Nitpick comments (4)
care/users/migrations/0022_migrate_users_qualification.py (1)
8-31: Replace single quotes with double quotes for consistencyThe static analysis tool suggests that double quotes are preferred over single quotes in this codebase. Updating the quotes will enhance consistency with the project's style guidelines.
Apply this diff to replace single quotes with double quotes:
- ('users', '0021_rename_gender_user_old_gender_user_geo_organization_and_more'), + ("users", "0021_rename_gender_user_old_gender_user_geo_organization_and_more"),Please update the remaining strings in the migration file similarly.
🧰 Tools
🪛 Ruff (0.8.2)
8-8: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
8-8: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
13-13: 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)
18-18: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: 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)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/emr/resources/user/spec.py (2)
32-35: Consider adding validation for the identifier field.While the structure follows FHIR Practitioner.qualification, the identifier field could benefit from some constraints.
class QualificationSpec(EMRResource): - identifier: str + identifier: str = Field(min_length=1, max_length=50) code: Coding period: PeriodSpec
36-37: Remove unnecessary empty lines.There are two consecutive empty lines after the class definition.
care/users/models.py (1)
308-324: Consider adding FHIR validation middleware.Since we're aligning with HL7 FHIR standards, it might be worth adding middleware to validate the qualification structure against the FHIR schema. This would ensure:
- Consistent data structure
- Compatibility with external FHIR systems
- Easier future migrations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
care/emr/resources/common/period.py(1 hunks)care/emr/resources/encounter/spec.py(1 hunks)care/emr/resources/user/spec.py(3 hunks)care/users/migrations/0022_migrate_users_qualification.py(1 hunks)care/users/models.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
care/users/migrations/0022_migrate_users_qualification.py
8-8: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
8-8: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
13-13: 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)
18-18: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: 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)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: 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 (3)
care/emr/resources/encounter/spec.py (1)
10-10: ImportingPeriodSpecfrom the common module looks goodImporting
PeriodSpecfromcare.emr.resources.common.periodpromotes reusability and maintains a cleaner codebase.care/emr/resources/common/period.py (1)
6-14:PeriodSpecclass implementation is well-structuredThe
PeriodSpecclass is correctly defined with appropriate validation to ensure that the start date is not after the end date.care/users/models.py (1)
308-315: Verify migration strategy for qualification data.Setting
blank=Falseon both new and old qualification fields might cause issues during migration. Consider the following:
- Existing records might not have qualification data
- The migration path from TextField to JSONField needs validation
✅ Verification successful
Actually, the migration strategy looks quite well thought out
The concerns about
blank=Falseare unwarranted because:
- The new
qualificationfield hasdefault=list, which prevents any blank value issues- The
qualification_oldfield allowsnull=True, maintaining compatibility with existing data from the originaldoctor_qualificationfield🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing records with empty qualifications echo "Checking for records with empty qualifications..." rg -l "qualification\s*=\s*models\.TextField" | xargs -I {} rg "blank\s*=\s*True" {}Length of output: 199
Script:
#!/bin/bash # Search for any qualification field definition historically echo "Searching for qualification field definitions..." rg "qualification.*=.*models\." -A 3 echo -e "\nChecking for migration files..." fd ".*migrations.*\.py$" care/users/ echo -e "\nLooking for qualification field usage..." rg "qualification.*=" care/users/Length of output: 979
| alt_phone_number: str = Field(max_length=14) | ||
| qualification: list[QualificationSpec] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add phone number format validation.
The alt_phone_number field could benefit from the same validation as phone_number.
- alt_phone_number: str = Field(max_length=14)
+ alt_phone_number: str = Field(max_length=14, pattern=r'^\+?1?\d{9,15}$')📝 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.
| alt_phone_number: str = Field(max_length=14) | |
| qualification: list[QualificationSpec] | |
| alt_phone_number: str = Field(max_length=14, pattern=r'^\+?1?\d{9,15}$') | |
| qualification: list[QualificationSpec] |
| doctor_experience_commenced_on_old = models.DateField( | ||
| default=None, | ||
| blank=False, | ||
| null=True, | ||
| ) | ||
| doctor_medical_council_registration = models.CharField( | ||
| doctor_medical_council_registration_old = models.CharField( | ||
| max_length=255, | ||
| default=None, | ||
| blank=False, |
There was a problem hiding this comment.
💡 Codebase verification
Your migration seems to be having a lovely identity crisis
The migration file shows incomplete implementation that could lead to data loss:
- Fields are renamed with _old suffix ✓
- New JSONField is added ✓
- But... where's the data migration logic? 🤔
- And the serializers are still living in the past, validating old fields
We should:
- Add data migration logic to transform existing doctor experience data into the new JSON structure
- Update serializer validation to use the new JSON structure
- Document the new JSON schema for qualification data
- Add a rollback strategy in case of migration issues
🔗 Analysis chain
Clarify the migration path for doctor experience data.
The fields are marked with _old suffix, but I don't see corresponding new fields. Are these being moved to the qualification JSONField? If so, we should:
- Document the new structure
- Ensure the migration script handles the data transformation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for migration files handling these fields
echo "Checking for related migrations..."
fd -e py -e yaml . | xargs rg "doctor_experience_commenced_on|doctor_medical_council_registration"
Length of output: 2611
Script:
#!/bin/bash
# Look for qualification field and related migrations
echo "Searching for qualification field..."
rg -A 5 "qualification.*JSONField|qualification.*Field.*JSON" care/users/models.py
echo -e "\nChecking migration files for qualification data structure..."
rg -A 10 "qualification.*=.*{" care/users/migrations/
Length of output: 612
Script:
#!/bin/bash
# Check the content of the migration file
echo "Examining qualification migration content..."
cat care/users/migrations/0022_migrate_users_qualification.py
echo -e "\nLooking for any documentation..."
fd README.md | xargs rg -i "qualification|doctor.*experience"
Length of output: 1283
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
New Features
QualificationSpecclassImprovements
Database Changes
The release introduces more comprehensive user profile management with improved data flexibility and structure.