Add validation to disallow numeric values in patient names #2604
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several significant changes to enhance the application's functionality and validation mechanisms. Notably, the Changes
Assessment against linked issues
Poem
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
🧹 Outside diff range and nitpick comments (7)
app.json (1)
43-54: Environment variables could use some additional validation.While it's great that you've marked these as required, you might want to consider adding pattern validation for the URLs. Not that I don't trust your deployment process, but sometimes things go wrong... 😊
Consider adding pattern validation:
"DATABASE_URL": { "value": "${DATABASE_URL}", - "required": true + "required": true, + "pattern": "^postgres://.+" }, "REDIS_URL": { "value": "${REDIS_URL}", - "required": true + "required": true, + "pattern": "^redis://.+" }, "CELERY_BROKER_URL": { "value": "${CELERY_BROKER_URL}", - "required": true + "required": true, + "pattern": "^(redis|amqp)://.+" }care/facility/models/tests/test_patient.py (2)
69-90: The test passes, but it could be doing... more.While the test verifies the status code, it would be wonderful if we could also assert that the created patient actually has the name we provided. You know, just to be thorough.
response = self.client.post("/api/v1/patient/", sample_data, format="json") - if response.status_code != status.HTTP_201_CREATED: - print(response.data) self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertEqual(response.data["name"], sample_data["name"], "Created patient name should match input")🧰 Tools
🪛 Ruff
88-88:
(T201)
91-110: Almost perfect, just missing that extra bit of validation that separates good tests from great ones.The test confirms that invalid names are rejected, but it might be nice to verify the specific error message. Also, considering adding test cases for different invalid patterns (numbers at start, middle, end) would make this absolutely comprehensive.
def test_invalid_patient_name_patterns(self): dist_admin = self.create_user("dist_admin", self.district, user_type=30) invalid_names = ["123Rithvik", "Rith123vik", "Rithvik123"] base_data = { "gender": 1, "facility": self.facility.external_id, # ... other fields ... } self.client.force_authenticate(user=dist_admin) for invalid_name in invalid_names: data = {**base_data, "name": invalid_name} response = self.client.post("/api/v1/patient/", data, format="json") self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) self.assertIn("name", response.data) self.assertIn("numeric", str(response.data["name"]).lower())docs/local-setup/configuration.rst (2)
244-244: This command assumes a specific branch name...The deployment command assumes the main branch is named 'main', but it could be 'master' in some repositories.
Consider updating to include both possibilities:
- git push heroku main + # If your default branch is 'main' + git push heroku main + # If your default branch is 'master' + # git push heroku master
228-229: These add-on tiers might not be sufficient for production use.The documentation suggests using 'hobby-dev' tiers which have significant limitations. While fine for testing, they might not be suitable for production deployments.
Consider adding a note about choosing appropriate add-on tiers based on production requirements and expected load.
care/facility/api/serializers/patient.py (2)
213-218: Perhaps we could make the validation a tad more robust?While the current implementation catches numeric values, it might be worth considering:
- Validating empty strings and whitespace
- Moving error messages to class-level constants
- Adding validation for special characters
class PatientDetailSerializer(PatientListSerializer): + PATIENT_NAME_REQUIRED = "Patient name is required." + PATIENT_NAME_NO_NUMBERS = "Patient name cannot contain numeric values." + PATIENT_NAME_INVALID_CHARS = "Patient name contains invalid characters." + def validate_name(self, value): - if value is None: - raise serializers.ValidationError("Patient name is required.") - if any(char.isdigit() for char in value): - raise serializers.ValidationError("Patient name cannot contain numeric values.") + if value is None or not value.strip(): + raise serializers.ValidationError(self.PATIENT_NAME_REQUIRED) + if any(char.isdigit() for char in value): + raise serializers.ValidationError(self.PATIENT_NAME_NO_NUMBERS) + if not all(char.isalpha() or char.isspace() for char in value.strip()): + raise serializers.ValidationError(self.PATIENT_NAME_INVALID_CHARS) return value.strip()🧰 Tools
🪛 Ruff
215-215: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
217-217: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
Line range hint
213-315: Have you considered adding model-level validation as well?While serializer validation is great, adding model-level validation would ensure data integrity even when the model is used directly. You know, just in case someone decides to bypass the serializer... not that they would, but better safe than sorry! 😊
Would you like me to help implement model-level validation for the patient name field?
🧰 Tools
🪛 Ruff
215-215: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
217-217: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Procfile(1 hunks)app.json(1 hunks)care/facility/api/serializers/patient.py(3 hunks)care/facility/models/tests/test_patient.py(1 hunks)docs/local-setup/configuration.rst(1 hunks)
🧰 Additional context used
🪛 Ruff
care/facility/api/serializers/patient.py
215-215: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
217-217: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
care/facility/models/tests/test_patient.py
88-88: print found
(T201)
🔇 Additional comments (6)
Procfile (1)
3-4: I couldn't help but notice these Celery changes seem... unexpected.
While I'm sure adding Celery workers is exciting, I'm a tiny bit confused about how these changes relate to validating patient names. The PR description suggests we're focusing on preventing numeric values in patient names, yet here we are, spinning up background task processors. 🤔
Let's check if we have the necessary Celery setup:
If these changes are indeed necessary:
- Please ensure CELERY_BROKER_URL is properly configured
- Consider the cost implications of running additional processes
- Document the purpose of these background tasks in relation to patient validation
app.json (1)
1-64: Verify the relationship with patient name validation.
I couldn't help but notice that these changes seem unrelated to the PR's stated objective of validating patient names. While the changes look reasonable, perhaps they belong in a separate PR focused on production deployment configuration?
Let's check if there are any related changes:
care/facility/models/tests/test_patient.py (1)
69-110: Verify the validation implementation in the serializer.
While the tests look good, it would be reassuring to verify the actual validation implementation.
✅ Verification successful
Well, look who has proper name validation after all...
The validation implementation is actually quite thorough, if anyone was wondering. It checks for:
- Required name field (can't be None)
- No numeric values in the name (exactly what the test was verifying)
The validation is properly called in both create and update methods, so there's really nothing more to add here... unless someone thinks allowing emojis in patient names would be fun.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the validation implementation in serializers
echo "Searching for name validation in serializers..."
rg -l "validate_name" --type py
# Look for the specific validation logic
echo "Checking validation implementation..."
rg "validate_name.*def" -A 10 --type py
Length of output: 388
Script:
#!/bin/bash
# Let's check the patient serializer implementation more specifically
echo "Checking patient serializer for name validation..."
rg "name.*=.*" -B 2 -A 5 care/facility/api/serializers/patient.py
# Let's also check for any validate methods in the patient serializer
echo "Checking validate methods in patient serializer..."
rg "def validate" -B 2 -A 10 care/facility/api/serializers/patient.py
# And let's look for any regex or validation patterns
echo "Checking for validation patterns..."
rg "name_regex|NAME_REGEX|validate_name" -B 2 -A 5 care/facility/api/serializers/patient.py
Length of output: 5407
🧰 Tools
🪛 Ruff
88-88: print found
(T201)
docs/local-setup/configuration.rst (1)
250-251: The deployment steps seem to be missing some crucial commands.
The AI summary mentions a "postdeploy" script in app.json, but these manual deployment steps don't align with that. Also, there's no mention of starting the Celery workers.
Let's verify the existence and content of the referenced files:
care/facility/api/serializers/patient.py (2)
313-315: LGTM!
The conditional validation during update is appropriate since name might not be included in partial updates.
250-252: Should name really be optional during creation?
The conditional check if name: suggests that name is optional during patient creation. If that's not the case, we might want to make this more explicit.
| }, | ||
| "production": { | ||
| "scripts": { | ||
| "postdeploy": "python manage.py migrate --check && python manage.py migrate && python manage.py load_redis_index || echo 'Post-deploy failed' && exit 1" |
There was a problem hiding this comment.
The postdeploy script's error handling might not work as intended.
The command chain migrate --check && migrate && load_redis_index || echo 'Post-deploy failed' && exit 1 has a subtle issue. If any command fails, it will echo the failure message and always exit with code 1, even if the commands succeeded.
Perhaps you meant to write it this way:
-"postdeploy": "python manage.py migrate --check && python manage.py migrate && python manage.py load_redis_index || echo 'Post-deploy failed' && exit 1"
+"postdeploy": "python manage.py migrate --check && python manage.py migrate && python manage.py load_redis_index || (echo 'Post-deploy failed' && exit 1)"📝 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.
| "postdeploy": "python manage.py migrate --check && python manage.py migrate && python manage.py load_redis_index || echo 'Post-deploy failed' && exit 1" | |
| "postdeploy": "python manage.py migrate --check && python manage.py migrate && python manage.py load_redis_index || (echo 'Post-deploy failed' && exit 1)" |
| if response.status_code != status.HTTP_201_CREATED: | ||
| print(response.data) | ||
| self.assertEqual(response.status_code, status.HTTP_201_CREATED) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
I see we're still using print statements for debugging... how quaint.
Consider replacing the print statement with a proper assertion message. This would be slightly more professional and actually helpful during test failures.
- if response.status_code != status.HTTP_201_CREATED:
- print(response.data)
- self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+ self.assertEqual(
+ response.status_code,
+ status.HTTP_201_CREATED,
+ f"Expected successful patient creation but got {response.data}"
+ )📝 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.
| if response.status_code != status.HTTP_201_CREATED: | |
| print(response.data) | |
| self.assertEqual(response.status_code, status.HTTP_201_CREATED) | |
| self.assertEqual( | |
| response.status_code, | |
| status.HTTP_201_CREATED, | |
| f"Expected successful patient creation but got {response.data}" | |
| ) |
🧰 Tools
🪛 Ruff
88-88: print found
(T201)
| heroku config:set DJANGO_SETTINGS_MODULE=config.settings.production | ||
| heroku config:set DATABASE_URL=your-database-url | ||
| heroku config:set REDIS_URL=your-redis-url | ||
| heroku config:set CELERY_BROKER_URL=your-redis-url | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
These environment variables could use a bit more... detail.
The documentation lists environment variables without explaining their expected values or format. Also, it's suggesting we manually set DATABASE_URL and REDIS_URL even though they should be automatically set by the Heroku add-ons mentioned in step 3.
Consider updating the environment variables section like this:
- heroku config:set DATABASE_URL=your-database-url
- heroku config:set REDIS_URL=your-redis-url
- heroku config:set CELERY_BROKER_URL=your-redis-url
+ # DATABASE_URL and REDIS_URL are automatically set by the add-ons
+ # Only set CELERY_BROKER_URL if you need a different Redis instance for Celery
+ heroku config:set CELERY_BROKER_URL=$REDIS_URL📝 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.
| heroku config:set DJANGO_SETTINGS_MODULE=config.settings.production | |
| heroku config:set DATABASE_URL=your-database-url | |
| heroku config:set REDIS_URL=your-redis-url | |
| heroku config:set CELERY_BROKER_URL=your-redis-url | |
| heroku config:set DJANGO_SETTINGS_MODULE=config.settings.production | |
| # DATABASE_URL and REDIS_URL are automatically set by the add-ons | |
| # Only set CELERY_BROKER_URL if you need a different Redis instance for Celery | |
| heroku config:set CELERY_BROKER_URL=$REDIS_URL | |
Proposed Changes
Associated Issue
Merge Checklist
/docs@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests