Used UserAssignedSerializer instead of UserBaseMinimumSerializer for assigned_to_object in patient details #2624
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the patient serializers in the facility API, specifically updating the user serializer references for the Changes
Assessment against linked issues
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
🧹 Outside diff range and nitpick comments (3)
care/users/api/serializers/user.py (3)
391-391: Consider adding phone number validationWhile adding the emergency contact number is a good step, it might be nice to ensure it follows a consistent format. You know, just to avoid those awkward moments when someone enters "call-me-maybe" as their emergency contact.
Consider adding a validator to ensure proper phone number format:
from django.core.validators import RegexValidator phone_regex = RegexValidator( regex=r'^\+?1?\d{9,15}$', message="Phone number must be entered in the format: '+999999999'. Up to 15 digits allowed." ) class UserBaseMinimumSerializer(serializers.ModelSerializer): alt_phone_number = serializers.CharField(validators=[phone_regex], allow_blank=True)
Line range hint
408-432: Documentation would be lovely hereSince this serializer is used for assigned users and now includes emergency contact information, it might be helpful to add a docstring explaining the purpose and usage of this field. You know, for those future developers who might wonder why we're collecting this information.
Add a docstring to clarify the purpose:
class UserAssignedSerializer(serializers.ModelSerializer): """ Serializer for assigned users, typically volunteers. Includes emergency contact information (alt_phone_number) for safety protocols when volunteers are assigned to patients. """
391-391: Tests would make this PR perfectWhile the implementation looks good, adding some test cases would really complete this PR. You know, just to make sure everything works as expected.
Would you like me to help generate test cases for:
- Phone number validation
- Serializer field presence
- API response format with the new field?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
care/users/api/serializers/user.py(1 hunks)
🔇 Additional comments (1)
care/users/api/serializers/user.py (1)
Line range hint 433-485: Did we forget something?
I couldn't help but notice that alt_phone_number isn't included in the UserListSerializer fields. If this information is needed on the patient details page, we might want to add it here too... unless there's a specific reason not to?
Let's check if this serializer is used in patient details views:
|
@vigneshhari @sainak Can either of you take a look? Thanks |
There was a problem hiding this comment.
LGTM as it solves the issue, but not sure if this is a security issue since UserBaseMinumumSerializer is used pretty much for all places where just the user's minimum information is required without revealing full details of the user itself.
cc: @sainak @vigneshhari
Rishith25
changed the title
Rishith25
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
care/facility/api/serializers/patient.py (1)
43-43: Confirm the need for both imports.It seems
UserBaseMinimumSerializeris still imported even though references have shifted towardUserAssignedSerializer. If there are no remaining usages ofUserBaseMinimumSerializer, consider removing this import.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/facility/api/serializers/patient.py(3 hunks)
🔇 Additional comments (2)
care/facility/api/serializers/patient.py (2)
89-89: Verify presence of the emergency contact field.
The PR objective mentions introducing an emergency number in UserBaseMinimumSerializer, but here you've switched to UserAssignedSerializer. Double-check that UserAssignedSerializer also contains the newly added emergency contact field to align with the PR's stated goal.
170-170: Double-check consistent inclusion of the emergency contact field.
Similarly, confirm that UserAssignedSerializer in the PatientDetailSerializer includes the new emergency contact field. Otherwise, the patient details page might not show the data as intended.
Rishith25
changed the title
|
@vigneshhari I have used UserAssignedSerializer instead of UserBaseMinimumSerializer. |
Proposed Changes
Architecture 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