add tests and validation for filterset #2588
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve significant updates to 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 (
|
![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 (6)
care/facility/api/viewsets/patient.py (4)
110-115: Consider adding custom error messages for phone number validation.The mobile validator is correctly applied to both phone number fields, but custom error messages would improve the user experience.
phone_number = filters.CharFilter( - field_name="phone_number", validators=[mobile_validator] + field_name="phone_number", + validators=[mobile_validator], + error_messages={'invalid': 'Please enter a valid phone number in E.164 format (e.g., +917795937091)'} )
124-130: Consider adding an upper limit for age validation.While preventing negative ages is good, an upper limit would prevent unrealistic values.
-age = filters.NumberFilter(field_name="age", validators=[MinValueValidator(0)]) +age = filters.NumberFilter( + field_name="age", + validators=[MinValueValidator(0), MaxValueValidator(150)] +)
Line range hint
401-451: Consider adding database indexes for frequently queried fields.The queryset uses multiple joins and complex annotations. Adding indexes would improve query performance.
Consider adding indexes to these fields in your models:
PatientRegistration.external_idPatientRegistration.facilityPatientRegistration.last_consultationPatientRegistration.modified_dateExample model changes:
class PatientRegistration(models.Model): class Meta: indexes = [ models.Index(fields=['external_id']), models.Index(fields=['facility']), models.Index(fields=['last_consultation']), models.Index(fields=['modified_date']), ]
Line range hint
532-583: Consider adding transaction management and rate limiting to the transfer endpoint.The patient transfer endpoint handles multiple operations but lacks transaction management. Also, rate limiting would prevent abuse.
@action(detail=True, methods=["POST"]) def transfer(self, request, *args, **kwargs): + from django.db import transaction + from rest_framework.throttling import UserRateThrottle + + @transaction.atomic def _transfer_patient(): patient = PatientRegistration.objects.get(external_id=kwargs["external_id"]) facility = Facility.objects.get(external_id=request.data["facility"]) # ... rest of the transfer logic ... return Response(data=response_serializer.data, status=status.HTTP_200_OK) + + return _transfer_patient()care/facility/tests/test_patient_api.py (2)
732-837: Consider extracting test data and assertions into helper methods... if you're into that sort of thing.While the test coverage is comprehensive, the code could be more maintainable. The repetitive pattern of setting up test data, making requests, and asserting responses could be simplified.
Consider this slightly more elegant approach:
+ def assert_length_validation(self, param_name, invalid_value, max_length): + res = self.client.get(self.get_base_url() + f"?{param_name}={invalid_value}") + self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn( + f"Ensure this value has at most {max_length} characters (it has {len(invalid_value)}).", + res.json()[param_name], + ) + def assert_numeric_validation(self, param_name, invalid_value, min_val, max_val=None): + res = self.client.get(self.get_base_url() + f"?{param_name}={invalid_value}") + self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) + if invalid_value < min_val: + self.assertIn( + f"Ensure this value is greater than or equal to {min_val}.", + res.json()[param_name], + ) + elif max_val and invalid_value > max_val: + self.assertIn( + f"Ensure this value is less than or equal to {max_val}.", + res.json()[param_name], + ) def test_filter_by_invalid_params(self): self.client.force_authenticate(user=self.user) - # name length > 200 words - invalid_name_param = "a" * 201 - res = self.client.get(self.get_base_url() + f"?name={invalid_name_param}") - self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) - self.assertIn( - "Ensure this value has at most 200 characters (it has 201).", - res.json()["name"], - ) + self.assert_length_validation("name", "a" * 201, 200) + self.assert_length_validation("srf_id", "a" * 201, 200) + self.assert_length_validation("district_name", "a" * 256, 255) + self.assert_length_validation("local_body_name", "a" * 256, 255) + self.assert_length_validation("state_name", "a" * 256, 255) + self.assert_numeric_validation("age", -2, 0) + self.assert_numeric_validation("age_min", -2, 0) + self.assert_numeric_validation("age_max", -2, 0) + self.assert_numeric_validation("number_of_doses", -2, 0, 3)
838-885: Oh look, more opportunities for DRY code... how exciting.The test has good coverage of invalid phone number formats, but there's some repetition that's making me slightly uncomfortable.
Here's a more maintainable approach:
+ INVALID_PHONE_NUMBERS = [ + "9876543210", + "+9123456789", + # ... rest of the numbers ... + ] + def assert_phone_validation(self, param_name, phone_number): + encoded_phone_number = quote(phone_number) + res = self.client.get( + self.get_base_url() + f"?{param_name}={encoded_phone_number}" + ) + self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn( + f"Invalid phone number. Must be one of the following types: mobile. Received: {phone_number}", + res.json()[param_name], + ) def test_invalid_phone_params(self): self.client.force_authenticate(user=self.user) - invalid_phone_numbers = [ - "9876543210", - "+9123456789", - # ... rest of the numbers ... - ] - for phone_number in invalid_phone_numbers: - encoded_phone_number = quote(phone_number) - res = self.client.get( - self.get_base_url() + f"?phone_number={encoded_phone_number}" - ) - self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) - self.assertIn( - f"Invalid phone number. Must be one of the following types: mobile. Received: {phone_number}", - res.json()["phone_number"], - ) + for phone_number in self.INVALID_PHONE_NUMBERS: + self.assert_phone_validation("phone_number", phone_number) + self.assert_phone_validation("emergency_phone_number", phone_number) - for emergency_phone_number in invalid_phone_numbers: - # ... duplicate code for emergency_phone_number ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
care/facility/api/viewsets/patient.py(6 hunks)care/facility/tests/test_patient_api.py(2 hunks)
🔇 Additional comments (1)
care/facility/api/viewsets/patient.py (1)
245-248: LGTM! Proper validation for number_of_doses.
The validators correctly restrict the number of doses between 0 and 3.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
care/facility/tests/test_patient_api.py (2)
732-837: Good test coverage, but could be more DRY.The test cases thoroughly validate the input constraints. However, there's quite a bit of repetition in the test structure that could be simplified.
Consider using parameterized tests to reduce code duplication. Here's a suggestion:
- def test_filter_by_invalid_params(self): - self.client.force_authenticate(user=self.user) - # name length > 200 words - invalid_name_param = "a" * 201 - res = self.client.get(self.get_base_url() + f"?name={invalid_name_param}") - self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) - self.assertIn( - "Ensure this value has at most 200 characters (it has 201).", - res.json()["name"], - ) - # ... more test cases ... + @pytest.mark.parametrize( + "param,value,field,expected_message", + [ + ("name", "a" * 201, "name", "Ensure this value has at most 200 characters (it has 201)."), + ("gender", 4, "gender", "Select a valid choice. 4 is not one of the available choices."), + ("age", -2, "age", "Ensure this value is greater than or equal to 0."), + # ... more test cases ... + ], + ) + def test_filter_by_invalid_params(self, param, value, field, expected_message): + self.client.force_authenticate(user=self.user) + res = self.client.get(self.get_base_url() + f"?{param}={value}") + self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn(expected_message, res.json()[field])
838-887: Thorough invalid phone number testing, but missing valid cases.While the test covers an impressive array of invalid phone formats, it would be slightly more complete with test cases for valid phone numbers.
Consider adding:
- Comments explaining why each phone number format is invalid
- Test cases for valid phone numbers
def test_invalid_phone_params(self): self.client.force_authenticate(user=self.user) # invalid phone number (non Indian or non International) invalid_phone_numbers = [ - "9876543210", - "+9123456789", + "9876543210", # Missing country code + "+9123456789", # Invalid country code # ... rest of the numbers ... ] + # Add test cases for valid numbers + valid_phone_numbers = [ + "+919876543210", # Valid Indian mobile + "+447911123456", # Valid UK mobile + ] + + # Test valid numbers + for phone_number in valid_phone_numbers: + encoded_phone_number = quote(phone_number) + res = self.client.get( + self.get_base_url() + f"?phone_number={encoded_phone_number}" + ) + self.assertEqual(res.status_code, status.HTTP_200_OK)
|
test cases depends on #2591 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
care/facility/api/viewsets/patient.py (3)
126-132: Consider adding an upper bound for age validation.While preventing negative ages is good, consider adding an upper bound (e.g., 150 years) to prevent unrealistic values.
- age = filters.NumberFilter(field_name="age", validators=[MinValueValidator(0)]) + age = filters.NumberFilter(field_name="age", validators=[MinValueValidator(0), MaxValueValidator(150)]) - age_min = filters.NumberFilter( - field_name="age", lookup_expr="gte", validators=[MinValueValidator(0)] - ) + age_min = filters.NumberFilter( + field_name="age", lookup_expr="gte", validators=[MinValueValidator(0), MaxValueValidator(150)] + ) - age_max = filters.NumberFilter( - field_name="age", lookup_expr="lte", validators=[MinValueValidator(0)] - ) + age_max = filters.NumberFilter( + field_name="age", lookup_expr="lte", validators=[MinValueValidator(0), MaxValueValidator(150)] + )
245-250: Consider future-proofing the number_of_doses validator.The current implementation limits the number of doses to 3, which might be restrictive if booster shots or additional doses become common in the future.
- number_of_doses = filters.NumberFilter( - field_name="number_of_doses", - validators=[MinValueValidator(0), MaxValueValidator(3)], - ) + number_of_doses = filters.NumberFilter( + field_name="number_of_doses", + validators=[MinValueValidator(0), MaxValueValidator(10)], # Allow for future booster doses + )
Line range hint
110-250: Well-structured implementation of field validations.The validation implementation follows a consistent pattern and integrates well with Django's filter framework. Consider documenting these validation rules in the API documentation to help API consumers understand the constraints.
care/facility/tests/test_patient_api.py (2)
732-848: Consider parameterizing the test cases to reduce code duplication.The test method thoroughly validates input parameters, but there's quite a bit of repetition in the test structure. You might want to consider using parameterized tests.
Here's a suggested refactor:
- def test_filter_by_invalid_params(self): + @pytest.mark.parametrize( + "param_name,invalid_value,expected_error", + [ + ("name", "a" * 201, "Ensure this value has at most 200 characters (it has 201)."), + ("gender", 4, "Select a valid choice. 4 is not one of the available choices."), + ("age", -2, "Ensure this value is greater than or equal to 0."), + ("age_min", -2, "Ensure this value is greater than or equal to 0."), + ("age_max", -2, "Ensure this value is greater than or equal to 0."), + ("number_of_doses", -2, "Ensure this value is greater than or equal to 0."), + ("number_of_doses", 4, "Ensure this value is less than or equal to 3."), + ("srf_id", "a" * 201, "Ensure this value has at most 200 characters (it has 201)."), + ("district_name", "a" * 256, "Ensure this value has at most 255 characters (it has 256)."), + ("local_body_name", "a" * 256, "Ensure this value has at most 255 characters (it has 256)."), + ("state_name", "a" * 256, "Ensure this value has at most 255 characters (it has 256)."), + ("patient_no", "A" * 101, "Ensure this value has at most 100 characters (it has 101)."), + ] + ) + def test_filter_by_invalid_params(self, param_name, invalid_value, expected_error): self.client.force_authenticate(user=self.user) - # name length > 200 words... + res = self.client.get(self.get_base_url() + f"?{param_name}={invalid_value}") + self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn(expected_error, res.json()[param_name])
849-898: Add positive test cases for phone number validation.While the test thoroughly covers invalid phone numbers, it would be helpful to also verify that valid phone numbers are accepted.
Add these test cases:
# Test valid phone numbers valid_phone_numbers = [ "+919876543210", # Valid Indian mobile "+447123456789", # Valid UK mobile "+12025550179", # Valid US mobile ] for phone_number in valid_phone_numbers: encoded_phone_number = quote(phone_number) res = self.client.get( self.get_base_url() + f"?phone_number={encoded_phone_number}" ) self.assertEqual(res.status_code, status.HTTP_200_OK) res = self.client.get( self.get_base_url() + f"?emergency_phone_number={encoded_phone_number}" ) self.assertEqual(res.status_code, status.HTTP_200_OK)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
care/facility/api/viewsets/patient.py(6 hunks)care/facility/tests/test_patient_api.py(2 hunks)
🔇 Additional comments (4)
care/facility/api/viewsets/patient.py (4)
6-6: LGTM: Import changes are appropriate.
The added imports for validators and gender choices support the new validation requirements.
Also applies to: 82-82, 86-86
117-119: LGTM: Name length constraint and gender validation added.
The changes properly implement:
- Name field length restriction to 200 characters
- Gender field validation using GENDER_CHOICES
Also applies to: 125-125
177-177: LGTM: Location field length constraints added.
Appropriate max_length constraints (255 characters) added to:
- district_name
- local_body_name
- state_name
Also applies to: 181-181, 184-186
110-115: Verify mobile number validation implementation.
The mobile number validation has been added correctly. However, since the test cases depend on issue #2591 (mobile number validation bug), we should verify the implementation.
| "Select a valid choice. 4 is not one of the available choices.", | ||
| res.json()["gender"], | ||
| ) | ||
|
|
||
| # invalid value for age, age max , age min filter (i.e <0) | ||
| invalid_age = -2 | ||
| res = self.client.get(self.get_base_url() + f"?age={invalid_age}") | ||
| self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn( | ||
| "Ensure this value is greater than or equal to 0.", res.json()["age"] | ||
| ) | ||
|
|
||
| invalid_min_age = -2 | ||
| res = self.client.get(self.get_base_url() + f"?age_min={invalid_min_age}") | ||
| self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn( | ||
| "Ensure this value is greater than or equal to 0.", res.json()["age_min"] | ||
| ) | ||
|
|
||
| invalid_max_age = -2 | ||
| res = self.client.get(self.get_base_url() + f"?age_max={invalid_max_age}") | ||
| self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn( | ||
| "Ensure this value is greater than or equal to 0.", res.json()["age_max"] | ||
| ) | ||
|
|
||
| # invalid number_of_doses param >3 or <0 | ||
| invalid_number_of_doses = -2 | ||
| res = self.client.get( | ||
| self.get_base_url() + f"?number_of_doses={invalid_number_of_doses}" | ||
| ) | ||
| self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn( | ||
| "Ensure this value is greater than or equal to 0.", | ||
| res.json()["number_of_doses"], | ||
| ) | ||
|
|
||
| invalid_number_of_doses = 4 | ||
| res = self.client.get( | ||
| self.get_base_url() + f"?number_of_doses={invalid_number_of_doses}" | ||
| ) | ||
| self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn( | ||
| "Ensure this value is less than or equal to 3.", | ||
| res.json()["number_of_doses"], | ||
| ) | ||
|
|
||
| # invalid srf id length > 200 words | ||
| invalid_srf_param = "a" * 201 | ||
| res = self.client.get(self.get_base_url() + f"?srf_id={invalid_srf_param}") | ||
| self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn( | ||
| "Ensure this value has at most 200 characters (it has 201).", | ||
| res.json()["srf_id"], | ||
| ) | ||
|
|
||
| # invalid district_name length > 255 words | ||
| invalid_district_name_param = "a" * 256 | ||
| res = self.client.get( | ||
| self.get_base_url() + f"?district_name={invalid_district_name_param}" | ||
| ) | ||
| self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn( | ||
| "Ensure this value has at most 255 characters (it has 256).", | ||
| res.json()["district_name"], | ||
| ) | ||
|
|
||
| # invalid local_body_name length > 255 words | ||
| invalid_local_body_name_param = "a" * 256 | ||
| res = self.client.get( | ||
| self.get_base_url() + f"?local_body_name={invalid_local_body_name_param}" | ||
| ) | ||
| self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn( | ||
| "Ensure this value has at most 255 characters (it has 256).", | ||
| res.json()["local_body_name"], | ||
| ) | ||
|
|
||
| # invalid state_name length > 255 words | ||
| invalid_state_name_param = "a" * 256 | ||
| res = self.client.get( | ||
| self.get_base_url() + f"?state_name={invalid_state_name_param}" | ||
| ) | ||
| self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn( | ||
| "Ensure this value has at most 255 characters (it has 256).", | ||
| res.json()["state_name"], | ||
| ) | ||
|
|
||
| # invalid patient no value > 100 | ||
| invalid_patient_no_param = "A" * 101 | ||
| res = self.client.get( | ||
| self.get_base_url() + f"?patient_no={invalid_patient_no_param}" | ||
| ) | ||
| self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn( | ||
| "Ensure this value has at most 100 characters (it has 101).", | ||
| res.json()["patient_no"], | ||
| ) | ||
|
|
There was a problem hiding this comment.
Add validation test for covin_id field.
The PR objectives mention validating the covin_id field with a maximum length of 15 characters, but this test is missing.
Add the following test case:
# Test invalid covin_id length > 15 characters
invalid_covin_id = "A" * 16
res = self.client.get(self.get_base_url() + f"?covin_id={invalid_covin_id}")
self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST)
self.assertIn(
"Ensure this value has at most 15 characters (it has 16).",
res.json()["covin_id"],
)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
care/facility/tests/test_patient_api.py (2)
732-848: Consider splitting this method into smaller, focused test methods.While the test coverage is comprehensive, having all these validations in a single method makes it harder to maintain and understand. Consider splitting into separate methods like:
test_invalid_name_length()test_invalid_gender_choice()test_invalid_age_values()- etc.
This would make the tests more focused and easier to maintain... if you care about that sort of thing.
849-860: Add test cases for edge cases in covin_id validation.The test only verifies the upper bound (>15 chars). Consider adding test cases for:
- Empty string
- Exactly 15 characters
- Special characters/spaces
I mean, if you want to be thorough about it...
| def test_invalid_phone_params(self): | ||
| self.client.force_authenticate(user=self.user) | ||
|
|
||
| # invalid phone number (non Indian or non International) | ||
| invalid_phone_numbers = [ | ||
| "9876543210", | ||
| "+9123456789", | ||
| "+915123456789", | ||
| "+9191234", | ||
| "+91765432abcd", | ||
| "00441234567890", | ||
| "+12345", | ||
| "+911234567890", | ||
| "+151234", | ||
| "+44-123-4567-890", | ||
| "+1-800-555-1212", | ||
| "+91 98765 43210", | ||
| "+91987654321000", | ||
| "+44 1234 567890", | ||
| "+123-456-7890", | ||
| "1234567890", | ||
| "+91-9876543210", | ||
| "+123456", | ||
| "+000000000000", | ||
| "+9876543210123456", | ||
| ] | ||
|
|
||
| for phone_number in invalid_phone_numbers: | ||
| encoded_phone_number = quote(phone_number) | ||
| res = self.client.get( | ||
| self.get_base_url() + f"?phone_number={encoded_phone_number}" | ||
| ) | ||
| self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn( | ||
| f"Invalid phone number. Must be one of the following types: mobile. Received: {phone_number}", | ||
| res.json()["phone_number"], | ||
| ) | ||
|
|
||
| for emergency_phone_number in invalid_phone_numbers: | ||
| encoded_emergency_phone_number = quote(emergency_phone_number) | ||
| res = self.client.get( | ||
| self.get_base_url() | ||
| + f"?emergency_phone_number={encoded_emergency_phone_number}" | ||
| ) | ||
| self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn( | ||
| f"Invalid phone number. Must be one of the following types: mobile. Received: {emergency_phone_number}", | ||
| res.json()["emergency_phone_number"], | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in phone number validation tests.
The same validation logic is repeated for both phone_number and emergency_phone_number. Consider extracting a helper method:
def _test_invalid_phone_field(self, field_name):
invalid_phone_numbers = [
"9876543210",
"+9123456789",
# ... rest of the numbers
]
for phone_number in invalid_phone_numbers:
encoded_number = quote(phone_number)
res = self.client.get(
self.get_base_url() + f"?{field_name}={encoded_number}"
)
self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST)
self.assertIn(
f"Invalid phone number. Must be one of the following types: mobile. Received: {phone_number}",
res.json()[field_name],
)But you know, it's your code, do what you want...
Removing the phone number validations from backend since we have a check for it in frontend |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
care/facility/api/viewsets/patient.py (1)
112-114: Character length validations look good, but...The character length validations align with requirements:
- name: 200 chars
- patient_no: 100 chars
- srf_id: 200 chars
- location fields: 255 chars
- covin_id: 15 chars
However, it might be good to consider extracting these magic numbers into constants for better maintainability.
Consider adding constants like:
MAX_NAME_LENGTH = 200 MAX_LOCATION_LENGTH = 255 MAX_COVIN_ID_LENGTH = 15Also applies to: 116-119, 154-154, 172-181, 240-240
care/facility/tests/test_patient_api.py (1)
731-847: LGTM, but could be more DRY!The test cases thoroughly validate all the required parameters mentioned in the PR objectives. However, there's quite a bit of repetition in the test structure.
Consider extracting a helper method to reduce duplication:
def _assert_invalid_param(self, param_name, invalid_value, expected_error): res = self.client.get(self.get_base_url() + f"?{param_name}={invalid_value}") self.assertEqual(res.status_code, status.HTTP_400_BAD_REQUEST) self.assertIn(expected_error, res.json()[param_name])But you know, it's your code, do what you want... 🤷
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
care/facility/api/viewsets/patient.py(6 hunks)care/facility/tests/test_patient_api.py(1 hunks)
🔇 Additional comments (5)
care/facility/api/viewsets/patient.py (4)
6-6: LGTM: Import additions are appropriate.
The added imports for validators and gender choices support the new validation requirements.
Also applies to: 82-82
120-120: Smart move using GENDER_CHOICES.
Replacing NumberFilter with ChoiceFilter using GENDER_CHOICES improves data validation and type safety.
121-127: Age validation is properly implemented.
The MinValueValidator(0) ensures non-negative values for all age-related fields as required.
242-245: Vaccination doses validation is spot on.
The number_of_doses field correctly implements the required range validation (0-3).
care/facility/tests/test_patient_api.py (1)
848-859: Implementation matches the requested test case.
The test validates covin_id length as specified in the PR objectives.
This addresses the previous review comment requesting validation for the covin_id field.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
care/facility/api/viewsets/patient.py (1)
172-181: Consider standardizing length limits across location fields.While the 255-character limit for location names is reasonable, I notice that other location-related fields (like
srf_id) use different limits. Perhaps we could standardize these?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
care/facility/api/viewsets/patient.py(6 hunks)care/facility/tests/test_patient_api.py(1 hunks)
🔇 Additional comments (4)
care/facility/api/viewsets/patient.py (2)
112-126: LGTM! The validation constraints look reasonable.
The added validators for name, patient number, gender, and age fields provide appropriate constraints:
- Name and patient number have sensible length limits
- Age fields correctly prevent negative values
- Gender field properly restricts to valid choices
240-245: Consider future-proofing the number_of_doses validator.
While the current implementation is correct, the hard-coded maximum of 3 doses might need adjustment as vaccination protocols evolve. Perhaps this could be moved to a configuration setting?
Let's check if this limit is used elsewhere in the codebase:
care/facility/tests/test_patient_api.py (2)
731-847: LGTM! Comprehensive test coverage for input validation.
The test cases thoroughly validate all the filter parameters with appropriate boundary conditions and error messages. The structure is consistent and well-organized.
848-859: This test case was already requested in a previous review.
I see you've implemented the covin_id validation test as suggested in the previous review. While I appreciate your attention to detail, you could have mentioned that you were addressing the previous review comment... but it's fine, I guess.
|
@Jacobjeevan the issue ohcnetwork/care_fe#9010 should be fixed with #2591 , we will need better validations instead of regex matching that we are currently using in BE and FE |
|
@vigneshhari can you review this? |
Proposed Changes
Added Validations in the Updated
PatientFilterSetnamemax_length=200namefield to a maximum of 200 characters.age,age_min, andage_maxvalidators=[MinValueValidator(0)]srf_idmax_length=200srf_idfield to a maximum of 200 characters.district_name,local_body_name, andstate_namemax_length=255covin_idmax_length=15covin_idfield to a maximum of 15 characters.number_of_dosesvalidators=[MinValueValidator(0), MaxValueValidator(3)]Associated Issue
Merge Checklist
Only 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
Summary by CodeRabbit
New Features
Bug Fixes
Tests