Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added alt phone number, and doctor specific fields #2766

Closed
wants to merge 1 commit into from
Closed

Added alt phone number, and doctor specific fields #2766

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions care/emr/resources/user/spec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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

doctor_medical_council_registration: str
Comment on lines +39 to +42
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Moving these fields to a separate DoctorSpec class that inherits from UserBaseSpec
  2. 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")



class UserUpdateSpec(UserBaseSpec):
Expand Down
Loading