Added alt phone number, and doctor specific fields #2766
Conversation
📝 WalkthroughWalkthroughThe pull request introduces new fields to the Changes
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
🧹 Nitpick comments (1)
care/emr/resources/user/spec.py (1)
40-40: Add constraints for qualification and registration fields.The
qualificationanddoctor_medical_council_registrationfields lack any constraints. I'm sure we wouldn't want just any random string in there, would we?- qualification: str + qualification: str = Field( + min_length=2, + max_length=100, + pattern=r'^[A-Za-z0-9\s\.,()-]+$', + description="Professional medical qualification" + ) - doctor_medical_council_registration: str + doctor_medical_council_registration: str = Field( + min_length=5, + max_length=50, + pattern=r'^[A-Z0-9-]+$', + description="Medical council registration number" + )Also applies to: 42-42
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/resources/user/spec.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
| alt_phone_number: str = Field(max_length=14) | ||
| qualification: str | ||
| doctor_experience_commenced_on: str | ||
| doctor_medical_council_registration: str |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving doctor-specific fields to a separate model or adding optional markers.
I couldn't help but notice that we're adding doctor-specific fields (qualification, doctor_experience_commenced_on, doctor_medical_council_registration) to the base class that's used by all user types (doctors, nurses, staff, and volunteers). This might not be the most... optimal approach.
Consider either:
- Moving these fields to a separate
DoctorSpecclass that inherits fromUserBaseSpec - Making these fields optional for non-doctor users
class UserBaseSpec(EMRResource):
phone_number: str = Field(max_length=14)
- alt_phone_number: str = Field(max_length=14)
- qualification: str
- doctor_experience_commenced_on: str
- doctor_medical_council_registration: str
+ alt_phone_number: str | None = Field(max_length=14, default=None)
+ qualification: str | None = Field(default=None)
+ doctor_experience_commenced_on: str | None = Field(default=None)
+ doctor_medical_council_registration: str | None = Field(default=None)Or better yet:
class DoctorSpec(UserBaseSpec):
qualification: str
experience_commenced_on: str = Field(alias="doctor_experience_commenced_on")
medical_council_registration: str = Field(alias="doctor_medical_council_registration")| @@ -36,6 +36,10 @@ class UserBaseSpec(EMRResource): | |||
| first_name: str | |||
| last_name: str | |||
| phone_number: str = Field(max_length=14) | |||
| alt_phone_number: str = Field(max_length=14) | |||
| qualification: str | |||
| doctor_experience_commenced_on: str | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add date validation for experience field.
The doctor_experience_commenced_on field is defined as a string without any date format validation. This could lead to... interesting data consistency issues.
- doctor_experience_commenced_on: str
+ doctor_experience_commenced_on: str = Field(
+ pattern=r'^\d{4}-\d{2}-\d{2}$',
+ description="Date in YYYY-MM-DD format"
+ )You might also want to add a validator to ensure the date is not in the future and not unreasonably old:
@field_validator("doctor_experience_commenced_on")
@classmethod
def validate_experience_date(cls, value):
from datetime import datetime
date = datetime.strptime(value, "%Y-%m-%d")
if date > datetime.now():
raise ValueError("Experience date cannot be in the future")
if date.year < 1950:
raise ValueError("Experience date seems unreasonably old")
return value
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