added validation for phone number in create spec #2730
Conversation
📝 WalkthroughWalkthroughThis pull request introduces enhanced phone number validation capabilities across the project. By adding Changes
Sequence DiagramsequenceDiagram
participant User
participant PhoneValidator
participant PatientSpec
User->>PhoneValidator: Provide Phone Number
PhoneValidator->>PhoneValidator: Validate Format
alt Valid Number
PhoneValidator-->>PatientSpec: Accept Number
else Invalid Number
PhoneValidator-->>User: Raise ValueError
end
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: 3
🧹 Nitpick comments (2)
care/emr/resources/base.py (1)
159-161: Consider restricting supported regions for international numbers.The current configuration allows any region code. It might be worth considering a whitelist of supported countries to prevent potential abuse.
care/emr/resources/patient/spec.py (1)
81-83: Consider moving regex patterns to settings or constants.These hardcoded regex patterns might need updates as phone number formats evolve. It would be slightly more maintainable to move them to a configuration file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Pipfile(1 hunks)care/emr/resources/base.py(2 hunks)care/emr/resources/patient/spec.py(3 hunks)care/emr/tests/test_patient_api.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (1)
Pipfile (1)
45-46: LGTM!The added dependencies with pinned versions look good and are compatible with the project's Python version.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
care/emr/tests/test_patient_api.py (3)
13-21: Oh, using example numbers instead of truly random ones, are we? 🙂While the function works, using example numbers might not catch all edge cases. Consider using
phonenumbers.parse()with randomly generated numbers for more thorough testing.def generate_random_valid_phone_number() -> str: - regions = ["US", "IN", "GB", "DE", "FR", "JP", "AU", "CA"] + # Add more regions for better coverage + regions = ["US", "IN", "GB", "DE", "FR", "JP", "AU", "CA", "BR", "RU", "CN", "KR"] - example_number = phonenumbers.example_number_for_type( - random_region, PhoneNumberType.MOBILE - ) + # Generate truly random numbers + country_code = phonenumbers.country_code_for_region(random_region) + local_number = ''.join([str(choice(range(10))) for _ in range(10)]) + number_str = f"+{country_code}{local_number}" + try: + number = phonenumbers.parse(number_str, None) + if phonenumbers.is_valid_number(number): + return phonenumbers.format_number(number, PhoneNumberFormat.E164) + except phonenumbers.NumberParseException: + pass + return generate_random_valid_phone_number() # Recursively try again
40-55: A docstring would be nice here... just sayingThe method could use some documentation explaining the parameters and return value. Also, it might be worth documenting that either 'age' or 'date_of_birth' is required.
def generate_patient_data(self, geo_organization, **kwargs): + """Generate test data for patient creation. + + Args: + geo_organization: The geographic organization identifier + **kwargs: Optional overrides for patient data fields + (must include either 'age' or 'date_of_birth') + + Returns: + dict: A dictionary containing patient data + """
101-120: The test cases are... let's say, minimalConsider adding more edge cases for invalid phone numbers:
- Numbers with spaces or special characters (
+1 (234) 567-8900)- Valid-looking but invalid area codes
- Numbers that are valid in one region but invalid in another
- Numbers with country codes but invalid local numbers
invalid_phone_numbers = [ "12345", "abcdef", "+1234567890123456", "", + "+1 (234) 567-8900", # Formatted but invalid + "+19995551234", # Invalid area code + "+44123456789", # Valid format, invalid local number + "++1234567890", # Double plus + "+", # Just plus + "١٢٣٤٥٦٧٨٩٠", # Non-ASCII numerals ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Pipfile(1 hunks)care/emr/resources/base.py(2 hunks)care/emr/resources/patient/spec.py(2 hunks)care/emr/tests/test_patient_api.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Pipfile
- care/emr/resources/patient/spec.py
- care/emr/resources/base.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
care/emr/tests/test_patient_api.py (2)
48-49: Noticed how emergency_phone_number validation isn't being tested?Both
phone_numberandemergency_phone_numberuse the same validation, but onlyphone_numberis being tested. Consider:
- Adding test cases for emergency phone number validation
- Testing scenarios where emergency number is same as primary number
Would you like me to generate the additional test cases for emergency phone number validation?
122-139: Testing with random data without checking the response... interesting choiceWhile testing with random valid numbers is good, consider:
- Adding some deterministic test cases for reproducibility
- Verifying the phone number in the response matches what was sent
- Adding assertions for other fields in the response
valid_phone_numbers = [ + "+14155552671", # Fixed test case for US + "+919876543210", # Fixed test case for India ] + [generate_random_valid_phone_number() for _ in range(3)] for valid_number in valid_phone_numbers: patient_data = self.generate_patient_data( geo_organization=geo_organization.external_id, phone_number=valid_number ) response = self.client.post(self.base_url, patient_data, format="json") self.assertEqual(response.status_code, status.HTTP_200_OK) + response_data = response.json() + self.assertEqual(response_data["phone_number"], valid_number) + self.assertEqual(response_data["name"], patient_data["name"])✅ Verification successful
Oh, you're absolutely right about those missing assertions
The review suggestions are spot-on since the test creates a complete patient record with name, address, and other fields but only checks the status code. The phone validation is quite thorough (supporting multiple regions and types), so verifying the response data would ensure the validation actually worked as intended. Those fixed test cases would also make debugging so much... easier.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find patient model and serializer definitions ast-grep --pattern 'class Patient' ast-grep --pattern 'class PatientSerializer' # Look for phone number validation rg "phone.*number.*valid" -g '!*.pyc' # Find test helper methods rg "generate_patient_data|generate_random_valid_phone_number" -g '*.py' -A 5Length of output: 5503
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2730 +/- ##
========================================
Coverage 60.42% 60.43%
========================================
Files 252 252
Lines 12695 12698 +3
Branches 1111 1111
========================================
+ Hits 7671 7674 +3
Misses 4955 4955
Partials 69 69 ☔ View full report in Codecov by Sentry. |
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
Release Notes
New Features
Dependencies
phonenumberslitelibrary for phone number validation.pydantic-extra-typesfor extended type support.Improvements