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

fix: Add validation to prevent numeric values in patient names #2605

Closed
wants to merge 9 commits into from
Closed

fix: Add validation to prevent numeric values in patient names #2605

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
46c9e39
Add validation to disallow numeric values in patient names
areebahmeddd Nov 19, 2024
e0c8351
Added coderrabit suggestions
areebahmeddd Nov 19, 2024
dd57b7f
Added regex validation
areebahmeddd Nov 19, 2024
fae9153
Add validation to disallow numeric values in patient names
areebahmeddd Nov 19, 2024
dc5278c
Added coderrabit suggestions
areebahmeddd Nov 19, 2024
176a68d
Added regex validation
areebahmeddd Nov 19, 2024
0f02ac3
Merge branch 'patient-name-validation' of https://github.com/areebahm…
areebahmeddd Nov 21, 2024
9972c9d
Removed redundant validation
areebahmeddd Nov 21, 2024
e455511
Merge branch 'develop' into patient-name-validation
vigneshhari Nov 21, 2024
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
6 changes: 6 additions & 0 deletions care/facility/api/serializers/patient.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from django.db import transaction
from django.utils.timezone import now
from rest_framework import serializers
import re

from care.facility.api.serializers import TIMESTAMP_FIELDS
from care.facility.api.serializers.facility import (
Expand Down Expand Up @@ -210,6 +211,11 @@ def validate_year_of_birth(self, value):
raise serializers.ValidationError(msg)
return value

def validate_name(self, value):
if not re.match(r'^[a-zA-Z\s]+$', value):
raise serializers.ValidationError("Patient name should contain only alphabets and spaces")
return value
Comment on lines +214 to +217
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

The validation could be a bit more... accommodating.

While the current implementation prevents numeric values, it might be a bit too restrictive for real-world names. Also, the error handling could use some polish.

Consider this enhanced implementation:

+    # Error messages as class constants
+    NAME_REQUIRED_ERROR = "Patient name is required"
+    NAME_EMPTY_ERROR = "Patient name cannot be empty or contain only whitespace"
+    NAME_INVALID_ERROR = "Patient name can only contain letters, spaces, hyphens, apostrophes, and periods"
+
     def validate_name(self, value):
-        if not re.match(r'^[a-zA-Z\s]+$', value):
-            raise serializers.ValidationError("Patient name should contain only alphabets and spaces")
+        if value is None:
+            raise serializers.ValidationError(self.NAME_REQUIRED_ERROR)
+        
+        value = value.strip()
+        if not value:
+            raise serializers.ValidationError(self.NAME_EMPTY_ERROR)
+        
+        # Allow letters (including international characters), spaces, hyphens, apostrophes, and periods
+        if not re.match(r'^[\p{L}\s\-\'\.]+$', value, re.UNICODE):
+            raise serializers.ValidationError(self.NAME_INVALID_ERROR)
+
         return value

This implementation:

  1. Handles international characters (e.g., "José", "François")
  2. Allows common name punctuation (e.g., "O'Connor", "Mary-Jane", "Jr.")
  3. Properly validates None/empty values
  4. Uses class constants for error messages
  5. Strips leading/trailing whitespace
📝 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.

Suggested change
def validate_name(self, value):
if not re.match(r'^[a-zA-Z\s]+$', value):
raise serializers.ValidationError("Patient name should contain only alphabets and spaces")
return value
# Error messages as class constants
NAME_REQUIRED_ERROR = "Patient name is required"
NAME_EMPTY_ERROR = "Patient name cannot be empty or contain only whitespace"
NAME_INVALID_ERROR = "Patient name can only contain letters, spaces, hyphens, apostrophes, and periods"
def validate_name(self, value):
if value is None:
raise serializers.ValidationError(self.NAME_REQUIRED_ERROR)
value = value.strip()
if not value:
raise serializers.ValidationError(self.NAME_EMPTY_ERROR)
# Allow letters (including international characters), spaces, hyphens, apostrophes, and periods
if not re.match(r'^[\p{L}\s\-\'\.]+$', value, re.UNICODE):
raise serializers.ValidationError(self.NAME_INVALID_ERROR)
return value
🧰 Tools
🪛 Ruff

215-215: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

216-216: Exception must not use a string literal, assign to variable first

Assign to variable; remove string literal

(EM101)


def validate(self, attrs):
validated = super().validate(attrs)
if not self.partial and not (
Expand Down
41 changes: 41 additions & 0 deletions care/facility/models/tests/test_patient.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,44 @@ def test_year_of_birth_validation(self):
response = self.client.post("/api/v1/patient/", sample_data, format="json")
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
self.assertIn("year_of_birth", response.data)

def test_valid_patient_name(self):
dist_admin = self.create_user("dist_admin", self.district, user_type=30)
sample_data = {
"name": "Rithvik",
"gender": 1,
"facility": self.facility.external_id,
"blood_group": "AB+",
"date_of_birth": "2000-01-01",
"year_of_birth": 2000,
"disease_status": "NEGATIVE",
"emergency_phone_number": "+919000000666",
"is_vaccinated": "false",
"number_of_doses": 0,
"phone_number": "+919000044343",
"is_antenatal": False,
}
self.client.force_authenticate(user=dist_admin)
response = self.client.post("/api/v1/patient/", sample_data, format="json")
self.assertEqual(response.status_code, status.HTTP_201_CREATED)

def test_invalid_patient_name(self):
dist_admin = self.create_user("dist_admin", self.district, user_type=30)
sample_data = {
"name": "Rithvik123",
"gender": 1,
"facility": self.facility.external_id,
"blood_group": "AB+",
"date_of_birth": "2000-01-01",
"year_of_birth": 2000,
"disease_status": "NEGATIVE",
"emergency_phone_number": "+919000000666",
"is_vaccinated": "false",
"number_of_doses": 0,
"phone_number": "+919000044343",
"is_antenatal": False,
}
self.client.force_authenticate(user=dist_admin)
response = self.client.post("/api/v1/patient/", sample_data, format="json")
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
self.assertIn("name", response.data)
Comment on lines +89 to +108
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance invalid name test coverage.

The test could be more thorough by checking these delightful invalid scenarios:

  • Names with special characters (e.g., "@#$%")
  • Empty string
  • Only numbers
  • Very long names (if there's a length limit)

Also, it might be helpful to assert the specific error message instead of just checking if "name" exists in response.data. Something like:

-        self.assertIn("name", response.data)
+        self.assertEqual(response.data["name"][0], "Name should not contain numeric characters")

Committable suggestion skipped: line range outside the PR's diff.