[OSDEV-1778] Fix the validation for number_of_workers field in POST, PATCH production locations API#532
Conversation
Innavin369
requested review from
VadimKovalenkoSNF,
mazursasha1990,
roman-stolar and
vladsha-dev
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the validation logic in the production locations API regarding the "number of workers" field. A new release notes entry documents that the minimum value must be less than or equal to the maximum. In the serializer, a new Changes
Possibly related PRs
Suggested reviewers
✨ 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 (
|
![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
🧹 Nitpick comments (1)
doc/release/RELEASE-NOTES.md (1)
79-79: Fix typo in release note entry.There's a small typo in the release note - it says "PATH" instead of "PATCH" when referring to the production locations API.
-* [OSDEV-1778](https://opensupplyhub.atlassian.net/browse/OSDEV-1778) - Fixed the validation for number of workers field in POST, PATH production locations API. The min field must be less than or equal to the max field. +* [OSDEV-1778](https://opensupplyhub.atlassian.net/browse/OSDEV-1778) - Fixed the validation for number of workers field in POST, PATCH production locations API. The min field must be less than or equal to the max field.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
doc/release/RELEASE-NOTES.md(1 hunks)src/django/api/serializers/v1/number_of_workers_serializer.py(1 hunks)
🔇 Additional comments (3)
src/django/api/serializers/v1/number_of_workers_serializer.py (3)
9-11: LGTM! Added improved validation error message for integer requirement.The added error message provides better clarity to API users about the expected data type.
17-19: LGTM! Added improved validation error message for integer requirement.The added error message provides better clarity to API users about the expected data type.
22-36: LGTM! Added validation to ensure min ≤ max.This implementation correctly validates that the minimum value is less than or equal to the maximum value, addressing the issue described in OSDEV-1778. The validation only happens when both values are present, which is appropriately handled.
The error message is clear and descriptive, helping API consumers understand the validation rule.
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
React App | Jest test suite - Code coverage reportTotal: 31.61%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 56.14%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/django/api/serializers/v1/number_of_workers_serializer.py (1)
34-71: Consider simplifying validation approach.While the custom validation works, it creates redundancy with Django REST Framework's built-in validation. The serializer already defines both fields as required IntegerFields with min_value constraints (lines 5-12).
Instead of completely reimplementing validation:
- Let DRF handle the type checking and required validation
- Keep only the min-max relationship validation in the validate method
- def to_internal_value(self, data): - min = data.get('min') - max = data.get('max') - errors = [] - - if not min: - errors.append({ - "field": "min", - "detail": - "The min field is required!" - }) - - if not max: - errors.append({ - "field": "max", - "detail": - "The max field is required!" - }) - - if min and not isinstance(data.get('min'), int): - errors.append({ - "field": "min", - "detail": - "The min field must be an integer." - }) - - if max and not isinstance(data.get('max'), int): - errors.append({ - "field": "max", - "detail": - "The max field must be an integer." - }) - - if len(errors) > 0: - raise serializers.ValidationError({"field": {self.field_name}, - "errors": errors}) - - return super().to_internal_value(data)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
doc/release/RELEASE-NOTES.md(1 hunks)src/django/api/serializers/v1/number_of_workers_serializer.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- doc/release/RELEASE-NOTES.md
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-django-code-quality
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (3)
src/django/api/serializers/v1/number_of_workers_serializer.py (3)
14-28: Good addition of min/max relationship validation.The added validate method properly checks that min is less than or equal to max when both values are present, which aligns with the PR objectives.
53-58: Integer validation may be too strict.Using
isinstance(data.get('min'), int)might reject valid integer values that are submitted as strings (common in HTTP APIs). Django REST Framework's IntegerField would handle this conversion automatically.
71-71: Consider error handling for super().to_internal_value.After performing custom validation, calling
super().to_internal_value(data)might raise additional validation errors with different formats. Consider handling these or documenting the expected behavior.
vladsha-dev
left a comment
vladsha-dev
left a comment
There was a problem hiding this comment.
@Innavin369
Could you please also check that the SLC form works as expected after the changes you made?
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
doc/release/RELEASE-NOTES.md(1 hunks)src/django/api/serializers/v1/number_of_workers_serializer.py(1 hunks)src/django/api/serializers/v1/production_location_schema_serializer.py(0 hunks)
💤 Files with no reviewable changes (1)
- src/django/api/serializers/v1/production_location_schema_serializer.py
🚧 Files skipped from review as they are similar to previous changes (1)
- doc/release/RELEASE-NOTES.md
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-fe-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-django-code-quality
🔇 Additional comments (3)
src/django/api/serializers/v1/number_of_workers_serializer.py (3)
14-28: LGTM: The validation logic for min and max relationship is correctly implemented.The new validate method properly ensures that the minimum value must be less than or equal to the maximum value, which aligns with the PR objective.
30-37:Details
❓ Verification inconclusive
Consider adding validation for negative numbers or zero in min/max fields.
The API should validate that min and max values are positive integers. While the field definitions on lines 5-12 specify min_value=1, the manual validation should align with these constraints.
I notice that your IntegerField definitions include min_value=1, but your validation logic doesn't explicitly check for this. It's worth verifying if Django REST framework's validation will handle this automatically or if additional validation should be added:
🏁 Script executed:
#!/bin/bash # Check how validation is typically handled in other serializers grep -r "min_value" --include="*.py" --include="*.md" src/django/ | grep -v '__pycache__' | head -20Length of output: 2109
Ensure Consistent Positive Integer Validation for
min/maxFields
While the serializer’s field definitions enforcemin_value=1at the top of the file (as confirmed by the grep results), the customto_internal_value()method does not explicitly validate thatminandmaxare positive. In other parts of the codebase, similar serializers rely on these built-in validations, so you might consider one of the following approaches:
Rely on DRF’s Field Validation:
If you’re confident that the DRF field-level validations (i.e.,min_value=1) will always be executed, you could leave the manual validation out ofto_internal_value(). This helps avoid duplication and keeps the code consolidated.Add Explicit Manual Checks:
If there’s any risk of bypassing the standard validation (or if you prefer a clearer error message at this stage), add explicit checks before further processing. For example:if min is not None and min < 1: errors.append("The 'min' value must be a positive integer.") if max is not None and max < 1: errors.append("The 'max' value must be a positive integer.")Please review and decide whether to rely solely on the field definitions or to introduce these additional manual checks to ensure consistency.
67-70:Details
❓ Verification inconclusive
Ensure field_name attribute is properly defined.
The code uses
self.field_namein the error structure. Make sure this attribute is defined somewhere in the class or inherited from a parent class, as it's not defined within the code shown.
🏁 Script executed:
#!/bin/bash # Check if field_name is defined in the serializer base class or elsewhere grep -r "field_name" --include="*.py" src/django/api/serializers/ | grep -v 'from\|import'Length of output: 3092
Action: Verify the Definition of
self.field_nameThe code at lines 67–70 in
src/django/api/serializers/v1/number_of_workers_serializer.pyraises aValidationErrorusingself.field_name, but there’s no visible definition of this attribute in the snippet. Similar serializers (e.g. instring_or_list_field.py) also useself.field_name, suggesting that it may be initialized in a common base class or elsewhere. Please confirm that:
self.field_nameis either initialized in this serializer’s constructor or inherited from a parent class.- The attribute’s definition is consistent across serializers to prevent potential runtime issues.
vladsha-dev
left a comment
vladsha-dev
left a comment
There was a problem hiding this comment.
- Left comment.
- Please check the title of the PR. I see PATH there instead of PATCH
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/django/api/serializers/v1/number_of_workers_serializer.py (2)
5-10: Consider addingrequired=Trueto the field definitions for clarity.The validation logic in
to_internal_valuechecks if these fields are present, effectively treating them as required. For better code clarity and self-documentation, it would be good to add therequired=Trueparameter to both field definitions, which aligns with the comment from @vladsha-dev in the previous review.- min = serializers.IntegerField( - min_value=1, - ) - max = serializers.IntegerField( - min_value=1, - ) + min = serializers.IntegerField( + min_value=1, + required=True, + ) + max = serializers.IntegerField( + min_value=1, + required=True, + )
30-31: Fix spacing in error message for better consistency.There's an extra space in the error message between "a" and "dictionary".
- "Invalid data. Expected a dictionary(object).") + "Invalid data. Expected a dictionary (object).")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/django/api/serializers/v1/number_of_workers_serializer.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-fe-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-countries-cov
🔇 Additional comments (3)
src/django/api/serializers/v1/number_of_workers_serializer.py (3)
12-26: LGTM! The validation logic is clear and matches the requirements.This implementation correctly validates that the minimum value is less than or equal to the maximum value, which aligns with the PR objective. The error message is clear and helpful for API users.
28-69: The validation implementation looks solid with proper error handling.The
to_internal_valuemethod now correctly validates input data format, presence of required fields, and type checking with appropriate error messages. The changes properly handle edge cases like when values are zero, as addressed in the previous review comments.
66-67:Details
❓ Verification inconclusive
Verify that
self.field_namewill always be available in this context.A previous review comment mentioned an issue with
self.field_name. Make sure this attribute is always available when this serializer is used. If this serializer is used directly without being nested in another serializer that sets the field name, this could potentially cause an error.
🏁 Script executed:
#!/bin/bash # Check how NumberOfWorkersSerializer is used in the codebase echo "Checking uses of NumberOfWorkersSerializer:" grep -r "NumberOfWorkersSerializer" --include="*.py" src/django/ echo -e "\nChecking if field_name is set somewhere in the serializer hierarchy:" grep -r "field_name" --include="*.py" src/django/api/serializers/Length of output: 3846
Double-check that
self.field_nameis reliably set on NumberOfWorkersSerializer instances.
- Our investigation shows that this serializer is used as a nested serializer (e.g., in production_location_schema_serializer.py), which likely binds
field_nameautomatically. However, there’s no explicit assignment or default value forself.field_namewithin NumberOfWorkersSerializer itself.- Please verify that all usages of this serializer rely on the parent serializer to call
bind()(or otherwise set this attribute) so that a direct (standalone) usage won’t result in an AttributeError.- If there’s any chance the serializer could be instantiated without being nested, consider adding an explicit initialization or fallback to ensure
self.field_nameis always available.
4 participants
OSDEV-1778 Fix the validation for
number_of_workersfield in POST, PATCH production locations API.