[OSDEV-1453] Use the 'detail' keyword instead of 'message' in response objects#431
Conversation
📝 WalkthroughWalkthroughThe pull request primarily updates error handling across various serializers, validators, and views in the Django API. The changes involve replacing the error message key from Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 9
🧹 Outside diff range and nitpick comments (17)
src/django/api/serializers/v1/opensearch_common_validators/source_validator.py (1)
26-29: LGTM! Consider minor message format enhancementThe change to 'detail' is correct and the error message is informative.
Consider this minor enhancement for better readability:
- "detail": ( - f"Invalid source(s) {invalid_sources}. " - "Allowed values are 'SLC' or 'API'." - ) + "detail": ( + f"Invalid source(s): {invalid_sources}. " + "Allowed values are: 'SLC', 'API'." + )src/django/api/serializers/v1/opensearch_common_validators/coordinates_validator.py (1)
20-21: Simplify validation logic by combining nested if statementsThe nested if statements can be combined to improve readability without changing the functionality.
Here's the suggested change:
- if lat is not None: - if not (-90 <= lat <= 90): + if lat is not None and not (-90 <= lat <= 90): errors.append({ "field": "coordinates", "detail": "Latitude must be between -90 and 90 degrees." }) - if lng is not None: - if not (-180 <= lng <= 180): + if lng is not None and not (-180 <= lng <= 180): errors.append({ "field": "coordinates", "detail": "Longitude must be between -180 and 180 degrees." })Also applies to: 27-28
🧰 Tools
🪛 Ruff (0.8.0)
20-21: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
src/django/api/serializers/v1/opensearch_common_validators/moderation_id_validator.py (1)
42-44: LGTM with a minor suggestion for improved clarity.The format change to 'detail' is correct, but we could make the error message more specific about accepted input types.
Consider this minor enhancement for better clarity:
- "detail": ( - "moderation_id must be a valid UUID or a list of UUIDs." - ) + "detail": ( + "moderation_id must be either a string containing a valid UUID " + "or a list of strings containing valid UUIDs." + )src/django/api/serializers/v1/opensearch_common_validators/percent_of_female_workers_validator.py (1)
28-28: Consider enhancing the error message clarityWhile the key change from 'message' to 'detail' is correct, the error message could be more specific.
Consider this more descriptive message:
- "detail": "Percentages must be between 0 and 100." + "detail": "Percentages must be greater than 0 and less than or equal to 100."src/django/api/serializers/v1/moderation_event_update_serializer.py (1)
Line range hint
1-71: Consider centralizing validation error message structureTo ensure consistent error response structure across the application, consider:
- Creating a utility function or base class that standardizes the error response format
- Defining error message templates in a central location
- Using an enum or constants for common error messages
This would make it easier to maintain the new 'detail' based error format and ensure consistency across all serializers.
src/django/api/services/opensearch/search.py (1)
Line range hint
65-71: Consider preserving error context in exceptions.While the error handling is consistent, we're losing valuable debugging information by not including the original error details in the raised exception.
Consider this improvement:
except OpenSearchException as e: logger.error(f"An error occurred while searching in \ index '{index_name}': {e}") - raise OpenSearchServiceException() + raise OpenSearchServiceException(f"OpenSearch error: {str(e)}") except Exception as e: logger.error(f"An unexpected error occurred: {e}") - raise OpenSearchServiceException() + raise OpenSearchServiceException(f"Unexpected error: {str(e)}")This change would:
- Preserve the original error context for better debugging
- Help distinguish between OpenSearch-specific and general errors
- Maintain the standardized 'detail' field while providing more information
src/django/api/serializers/v1/moderation_events_serializer.py (1)
85-88: Consider documenting the error response structure.Since this is an API-facing change in error response format, it would be helpful to add a docstring or comment describing the new error response structure for API consumers.
Example addition:
if errors: # [OSDEV-1441] Pass error msg to the Rollbar here + # Returns a standardized error response: + # { + # "detail": str, # Generic error message + # "errors": List[str] # Specific validation errors + # } raise ValidationError({ "detail": COMMON_ERROR_DETAIL, "errors": errors })src/django/api/views/v1/utils.py (1)
46-64: Consider standardizing error message formattingWhile the changes to use 'detail' are correct, there's inconsistency in error message formatting:
- Some messages are explicitly titled (using
.title())- Some messages might come through without consistent casing
Consider applying this improvement:
- error_response['detail'] = detail_errors[0].title() + error_response['detail'] = str(detail_errors[0]).strip().capitalize()This ensures consistent capitalization without affecting words mid-sentence.
src/django/api/views/v1/moderation_events.py (2)
92-94: Consider refactoring duplicated UUID validation logic.The UUID validation and error handling logic is duplicated between the
retrieveandpatchmethods. Consider extracting this into a shared helper method.class ModerationEvents(ViewSet): + def _validate_moderation_id(self, pk): + if not ModerationIdValidator.is_valid_uuid(pk): + return handle_path_error( + field="moderation_id", + detail="Invalid UUID format.", + status_code=status.HTTP_400_BAD_REQUEST + ) + return NoneThen use it in both methods:
def retrieve(self, _, pk=None): error = self._validate_moderation_id(pk) if error: return error # ... rest of the method def patch(self, request, pk=None): error = self._validate_moderation_id(pk) if error: return error # ... rest of the method
Line range hint
117-121: Standardize error response structure.The current error response structure differs from other endpoints by including both 'detail' and 'error' fields. Consider standardizing it to match the pattern used elsewhere.
return Response( { - "detail": 'The request body contains ' - 'invalid or missing fields.', - "error": [serializer.errors] + "detail": serializer.errors }, status=status.HTTP_400_BAD_REQUEST )This change would:
- Maintain consistency with other error responses
- Directly expose the serializer's validation messages
- Follow Django REST framework's standard error format
src/django/api/tests/test_v1_utils.py (2)
109-122: LGTM! Consider enhancing test coverage.The error response structure follows a good pattern with a top-level 'detail' message and field-specific errors. The test correctly validates both aspects.
Consider adding test cases for:
- Multiple simultaneous field validation errors
- Different types of validation errors (e.g., required fields, format validation)
133-139: Consider improving error handling test coverage.While the test correctly validates the new 'detail' structure, it could be more comprehensive.
Consider:
- Testing different types of ValueError scenarios
- Validating the complete error response structure
- Making the error message more specific than "There was a problem processing your request"
def test_handle_value_error_with_specific_message(self): specific_error = ValueError("Invalid date format") response = handle_value_error(specific_error) self.assertEqual(response.status_code, 400) self.assertEqual(response.data['detail'], "The request query is invalid.") self.assertEqual(response.data['errors'][0]['field'], "general") self.assertEqual(response.data['errors'][0]['detail'], "Invalid date format")src/react/src/components/Dashboard/DashboardModerationQueueListTable.jsx (1)
45-45: LGTM! Consider improving error handling.While the change from
error.messagetoerror.detailaligns with the standardization effort, consider enhancing the error handling:
- Add user feedback when the operation fails
- Consider using a proper error tracking service instead of just console.error
Example enhancement:
} catch (error) { - console.error( - `Failed to open contribution record: ${error.detail}`, - ); + console.error(`Failed to open contribution record:`, error); + // Add toast/snackbar notification for user feedback + props.onError?.(error.detail || 'Failed to open contribution record'); }src/tests/v1/test_moderation_events.py (4)
209-211: Consider consistent capitalization in error messagesWhile the change from
messagetodetailis correct, consider making the error message capitalization more consistent. Currently it mixes different styles: 'Date_Gte' and 'Date_Lt'.Consider updating to:
- "The 'Date_Gte' Must Be Less Than Or Equal To 'Date_Lt'." + "The 'date_gte' must be less than or equal to 'date_lt'."
238-238: Consider clarifying the error messageWhile the change from
messagetodetailis correct, the error message could be more helpful by mentioning the expected format.Consider updating to:
- self.assertEqual(error['detail'], "'Usa' Is Not A Valid Alpha-2 Country Code.") + self.assertEqual(error['detail'], "'Usa' is not a valid ISO 3166-1 alpha-2 country code (e.g., 'US', 'GB').")
265-265: Consider enhancing the UUID validation messageWhile the change from
messagetodetailis correct, the error message could be more helpful by explaining the expected UUID format.Consider updating to:
- self.assertEqual(error['detail'], "Invalid Uuid(S): 123!.") + self.assertEqual(error['detail'], "Invalid UUID format: '123!'. Expected format: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx")
Line range hint
151-265: Consider standardizing error message formatWhile all changes from
messagetodetailare correct, consider establishing and following consistent conventions for error messages across all endpoints:
- Consistent capitalization (either sentence case or lower case)
- Consistent punctuation
- Including examples in validation errors where applicable
- Using consistent terminology (e.g., "invalid" vs "not valid")
This would improve the API's overall user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (21)
src/django/api/serializers/v1/moderation_event_update_serializer.py(2 hunks)src/django/api/serializers/v1/moderation_events_serializer.py(2 hunks)src/django/api/serializers/v1/opensearch_common_validators/coordinates_validator.py(1 hunks)src/django/api/serializers/v1/opensearch_common_validators/countries_validator.py(1 hunks)src/django/api/serializers/v1/opensearch_common_validators/date_range_validator.py(1 hunks)src/django/api/serializers/v1/opensearch_common_validators/moderation_id_validator.py(1 hunks)src/django/api/serializers/v1/opensearch_common_validators/number_of_workers_validator.py(2 hunks)src/django/api/serializers/v1/opensearch_common_validators/percent_of_female_workers_validator.py(2 hunks)src/django/api/serializers/v1/opensearch_common_validators/request_type_validator.py(1 hunks)src/django/api/serializers/v1/opensearch_common_validators/size_validator.py(1 hunks)src/django/api/serializers/v1/opensearch_common_validators/source_validator.py(1 hunks)src/django/api/serializers/v1/opensearch_common_validators/status_validator.py(1 hunks)src/django/api/serializers/v1/production_locations_serializer.py(2 hunks)src/django/api/services/opensearch/search.py(1 hunks)src/django/api/tests/test_facility_claim_view_set.py(2 hunks)src/django/api/tests/test_production_locations_view_set.py(1 hunks)src/django/api/tests/test_v1_utils.py(2 hunks)src/django/api/views/v1/moderation_events.py(4 hunks)src/django/api/views/v1/utils.py(5 hunks)src/react/src/components/Dashboard/DashboardModerationQueueListTable.jsx(1 hunks)src/tests/v1/test_moderation_events.py(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/django/api/serializers/v1/production_locations_serializer.py
🧰 Additional context used
📓 Learnings (5)
src/django/api/serializers/v1/moderation_events_serializer.py (1)
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/moderation_events.py:41-43
Timestamp: 2024-11-12T11:42:07.768Z
Learning: In the `ModerationEvents` API endpoint, parameter validation is handled in `src/django/api/serializers/v1/moderation_events_serializer.py`.
src/django/api/serializers/v1/opensearch_common_validators/date_range_validator.py (3)
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/serializers/v1/opensearch_common_validators/date_range_validator.py:9-11
Timestamp: 2024-11-12T11:15:04.794Z
Learning: In `src/django/api/serializers/v1/opensearch_common_validators/date_range_validator.py`, date format validation is not required within the `DateRangeValidator` class, as date validation is handled elsewhere in the codebase.
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/serializers/v1/opensearch_common_validators/date_range_validator.py:12-19
Timestamp: 2024-11-12T11:14:17.011Z
Learning: In the codebase, when performing validation in validators like `DateRangeValidator`, errors should be collected into an `errors` list for later processing, rather than throwing exceptions directly.
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py:61-79
Timestamp: 2024-11-14T11:06:37.772Z
Learning: In the `OpenSearchQueryBuilder` class located at `src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py`, date validation is performed at a higher level in the application. Therefore, it's unnecessary to add additional date validation within the `__build_date_range` method.
src/django/api/serializers/v1/opensearch_common_validators/moderation_id_validator.py (1)
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/serializers/v1/opensearch_common_validators/moderation_id_validator.py:20-25
Timestamp: 2024-11-13T07:21:28.686Z
Learning: In the `ModerationIdValidator` class located at `src/django/api/serializers/v1/opensearch_common_validators/moderation_id_validator.py`, it's not necessary to add input size validation for the `moderation_id` list. OpenSearch already imposes a page size limit, which mitigates potential DoS attacks from large inputs.
src/django/api/serializers/v1/opensearch_common_validators/request_type_validator.py (1)
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/serializers/v1/opensearch_common_validators/request_type_validator.py:10-0
Timestamp: 2024-11-12T11:12:25.832Z
Learning: In `src/django/api/serializers/v1/opensearch_common_validators/request_type_validator.py`, the `request_type` field is optional. The validator should return without errors if `request_type` is missing.
src/django/api/serializers/v1/opensearch_common_validators/status_validator.py (1)
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/serializers/v1/opensearch_common_validators/status_validator.py:21-24
Timestamp: 2024-11-13T07:20:15.181Z
Learning: In the `ModerationEvent` model, validation for the `status` field—including checks for `None` or empty string values—is performed at the model level. Therefore, additional validation in the `StatusValidator` class in `status_validator.py` is unnecessary.
🪛 Ruff (0.8.0)
src/django/api/serializers/v1/opensearch_common_validators/coordinates_validator.py
20-21: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
27-28: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🔇 Additional comments (30)
src/django/api/serializers/v1/opensearch_common_validators/date_range_validator.py (1)
Line range hint 16-20: LGTM! The change aligns with the standardization of error responses.
The update from 'message' to 'detail' in the error structure maintains the existing validation logic while conforming to the new API response standard. The error message remains clear and descriptive.
src/django/api/serializers/v1/opensearch_common_validators/request_type_validator.py (1)
19-21: LGTM! Change aligns with PR objectives.
The update from 'message' to 'detail' in the error response maintains the existing validation logic while successfully standardizing the error response format.
src/django/api/serializers/v1/opensearch_common_validators/size_validator.py (1)
20-24: LGTM! Changes align with standardization effort.
The replacement of "message" with "detail" in error responses is consistent with the PR objectives and maintains clear error messaging.
Let's verify consistency across other validators:
Also applies to: 28-28
✅ Verification successful
Standardization of error responses is complete and consistent
The search results confirm that all validators in the opensearch_common_validators directory are consistently using "detail" for error messages, with no instances of the old "message" key remaining. The changes in size_validator.py are part of a complete and consistent standardization across all validator files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any validators still use "message" instead of "detail"
# Expected: No occurrences of "message" in error responses
# Search for any remaining "message" keys in validator files
rg '"message":' src/django/api/serializers/v1/opensearch_common_validators/
Length of output: 77
Script:
#!/bin/bash
# Let's try a more comprehensive search for both "message" and "detail" in validator files
# This will help us verify the consistency of error response keys
# First, let's see all Python files in the validators directory
fd '\.py$' src/django/api/serializers/v1/opensearch_common_validators/
# Then search for any error response patterns with both "message" and "detail"
rg -A 1 'raise ValidationError\(' src/django/api/serializers/v1/opensearch_common_validators/
# Also search for dictionary definitions that might contain these keys
rg -A 1 '(message|detail)["'\'']:\s*' src/django/api/serializers/v1/opensearch_common_validators/
Length of output: 6477
src/django/api/serializers/v1/opensearch_common_validators/source_validator.py (1)
19-19: LGTM! Error key change aligns with standardization effort
The change from 'message' to 'detail' maintains the error information while following the new standardized format.
src/django/api/serializers/v1/opensearch_common_validators/status_validator.py (1)
Line range hint 11-30: Consider removing redundant validation
Based on previous feedback, the status field validation is already handled at the model level in ModerationEvent. This validator might be introducing redundant validation.
Let's verify the model-level validation:
If the model-level validation is confirmed, consider removing this validator to avoid duplication and potential inconsistencies in validation logic.
src/django/api/serializers/v1/opensearch_common_validators/number_of_workers_validator.py (3)
Line range hint 17-21: LGTM! Error message key change aligns with standardization effort.
The change from 'message' to 'detail' follows the PR objective while maintaining clear error messaging for invalid number_of_workers parameters.
Line range hint 27-31: LGTM! Consistent use of 'detail' key for error messaging.
The error message key change maintains consistency with the new standard while preserving the clear validation logic for min/max value comparison.
Line range hint 17-31: Verify consistent error key usage across all validators.
Let's ensure all validator classes have been updated to use 'detail' instead of 'message'.
✅ Verification successful
All validators consistently use 'detail' key for error messages
The verification results show that all validator files in the opensearch_common_validators directory consistently use the 'detail' key for error messages, and there are no instances of the 'message' key being used. This confirms that the error key usage is uniform across all validators.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of 'message' in validator error responses
# and verify consistent usage of 'detail' across all validators
# Search for any remaining 'message' keys in validator error responses
echo "Checking for remaining 'message' keys in validators..."
rg '"message"' src/django/api/serializers/v1/opensearch_common_validators/
# Verify 'detail' key usage in validators
echo "Verifying 'detail' key usage in validators..."
rg '"detail"' src/django/api/serializers/v1/opensearch_common_validators/
Length of output: 3075
src/django/api/serializers/v1/opensearch_common_validators/coordinates_validator.py (3)
16-18: LGTM! Error message key change is consistent with requirements
The change from "message" to "detail" aligns with the PR objectives while maintaining clear error messaging.
23-25: LGTM! Latitude validation error format is correct
The error message key change maintains proper validation messaging while conforming to the new standard.
30-33: LGTM! Longitude validation error format is correct
The error message key change and multi-line formatting maintain proper validation messaging while conforming to the new standard.
src/django/api/serializers/v1/opensearch_common_validators/moderation_id_validator.py (3)
31-31: LGTM! Error format update is consistent with PR objectives.
The change from 'message' to 'detail' maintains clear error reporting while aligning with the standardization effort.
37-37: LGTM! Error format is consistent with the new standard.
The error message maintains clarity while adopting the 'detail' key format.
Line range hint 16-46: Implementation correctly follows established patterns.
The validator appropriately focuses on UUID validation without implementing unnecessary input size validation, as OpenSearch's page size limit already provides protection against large inputs (as noted in previous feedback).
src/django/api/serializers/v1/opensearch_common_validators/percent_of_female_workers_validator.py (3)
19-19: LGTM: Error message key updated correctly
The change from 'message' to 'detail' aligns with the PR objectives while maintaining clear error messaging.
33-36: LGTM: Error message key and format are appropriate
The change from 'message' to 'detail' is correct, and the multi-line error message format maintains good readability.
Line range hint 19-36: Verify consistency of error response format
Let's ensure this change is consistent with other validators in the codebase.
✅ Verification successful
Error response format is consistent across validators
The verification shows that all validators in the opensearch_common_validators directory consistently use the "detail" field for error messages, with no instances of the older "message" format remaining. The changes in percent_of_female_workers_validator.py align with the standardized error response format used throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of "message" in error responses
rg '"message":' --type py src/django/api/serializers/v1/opensearch_common_validators/
# Search for new "detail" format to confirm consistency
rg '"detail":' --type py src/django/api/serializers/v1/opensearch_common_validators/
Length of output: 2865
src/django/api/serializers/v1/moderation_event_update_serializer.py (2)
42-42: Verify consistency with DRF's built-in validation errors
The change from 'message' to 'detail' aligns with the PR objectives. However, let's ensure this custom error structure matches DRF's built-in validation errors for required fields.
✅ Verification successful
The change from 'message' to 'detail' is consistent with DRF's error response format
The search results show that both formats ('message' and 'detail') are used across the codebase, but there's a clear pattern of using 'detail' in newer code and DRF-specific responses. The change aligns with Django REST Framework's standard error response format and the PR's objective to standardize error responses.
Key evidence:
- Multiple serializers and views use the 'detail' field for error messages (e.g.,
production_locations_serializer.py,moderation_events_serializer.py) - DRF-specific query parameter serializers consistently use 'detail' (e.g.,
facility_query_params_serializer.py,merge_query_params_serializer.py) - The change maintains consistency with other validation error responses in the moderation event serializers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar required field validation patterns in the codebase
rg -A 2 "raise ValidationError.*field.*required" --type py
Length of output: 60
Script:
#!/bin/bash
# Let's try a broader search for ValidationError patterns in DRF serializers
rg -B 2 -A 2 "ValidationError\(" --type py
# Also search for required field validation patterns
rg -B 2 -A 2 "required.*message|detail.*required" --type py
# Search for similar serializer validation patterns
ast-grep --pattern 'raise ValidationError({"$_": $_})'
Length of output: 33258
56-59: LGTM! Consider enhancing error message with available choices
The change from 'message' to 'detail' is correct. However, we could make the error message more helpful by dynamically listing the available choices from ModerationEvent.Status.
Consider this enhancement:
"detail": (
- "Moderation status must be one of "
- "PENDING, APPROVED or REJECTED."
+ f"Moderation status must be one of: "
+ f"{', '.join(sorted([ModerationEvent.Status.PENDING, "
+ f"ModerationEvent.Status.APPROVED, "
+ f"ModerationEvent.Status.REJECTED]))}"
)✅ Verification successful
Let me check the status validation patterns in more detail.
Let me check the specific implementation of the status validator to compare the error message patterns.
Let me try a different approach to find the status validation implementation.
Based on the verification results, I can now provide the final response:
The error message pattern is consistent with other validators, but dynamic choices would be better
Looking at the codebase, particularly the StatusValidator and other validators, we see consistent use of the "field"/"detail" pattern for error messages. However, the StatusValidator in the OpenSearch validators shows a more dynamic approach by using VALID_STATUSES as a class constant and referencing invalid values in error messages.
- The change from 'message' to 'detail' aligns with the codebase's validation patterns
- The suggestion to use dynamic status values would indeed improve the error message, making it consistent with other validators' approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar validation patterns in other validators
rg -A 3 "class.*Validator" --type py
Length of output: 7110
Script:
#!/bin/bash
# Check for status validation patterns and error messages
rg -B 2 -A 4 "status.*not.*in" --type py
rg -B 2 -A 4 "VALID_STATUSES" --type py
Length of output: 6917
Script:
#!/bin/bash
# Check the implementation of status validation in the moderation event serializer
ast-grep --pattern 'def __validate_status' -A 20
Length of output: 50
Script:
#!/bin/bash
# Search for status validation in moderation event files
rg -B 2 -A 10 "validate.*status" src/django/api/serializers/v1/moderation_event_update_serializer.py
Length of output: 891
src/django/api/serializers/v1/moderation_events_serializer.py (1)
26-26: LGTM! Changes align with standardization effort.
The replacement of 'message' with 'detail' in both the import and error response structure is consistent with the PR objectives to standardize error handling across the API.
Also applies to: 86-86
src/django/api/views/v1/utils.py (3)
Line range hint 76-84: LGTM! Error structure properly standardized
The error response structure has been correctly updated to use 'detail' while maintaining a clear and helpful error message format.
113-120: LGTM! Path error handling properly standardized
The path error handler has been correctly updated to use 'detail' consistently, with a good separation between the generic error message and specific field details.
11-11: Verify constant references across the codebase
The constant rename from COMMON_ERROR_MESSAGE to COMMON_ERROR_DETAIL aligns with the PR objectives.
Let's verify all references have been updated:
✅ Verification successful
Constant rename has been properly implemented across the codebase
The verification shows that:
- No references to the old constant
COMMON_ERROR_MESSAGEexist in the codebase - The new constant
COMMON_ERROR_DETAILis consistently used across multiple files:- Imported and used in
production_locations_serializer.py - Imported and used in
moderation_events_serializer.py - Used multiple times within
utils.py
- Imported and used in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old constant name
# and verify the new constant is used consistently
# Search for any remaining references to old constant
echo "Checking for old constant references..."
rg "COMMON_ERROR_MESSAGE"
# Search for new constant usage to verify consistent implementation
echo "Checking new constant usage..."
rg "COMMON_ERROR_DETAIL"
Length of output: 915
src/django/api/views/v1/moderation_events.py (3)
67-69: LGTM! Error handling follows best practices.
The error response structure is well-formatted and uses the standardized 'detail' field with an appropriate HTTP status code.
101-103: LGTM! Appropriate error handling for not found case.
The error response correctly uses the 'detail' field and an appropriate HTTP 404 status code for the not found scenario.
Line range hint 1-124: Verify complete migration from 'message' to 'detail'.
Let's ensure all occurrences of 'message' have been replaced with 'detail' in the codebase.
✅ Verification successful
Error response format is correctly standardized
The code consistently uses 'detail' instead of 'message' for error responses. The error handling utilities in utils.py enforce a standardized format where:
- All responses use 'detail' for the main error message
- Specific field errors are structured under 'errors' array with 'field' and 'detail' properties
- No instances of 'message' found in error responses in the moderation events related files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining 'message' keys in error responses
# Search for 'message' in response construction
rg -g '*.py' '"message":\s*[f"]' src/
# Search for Response objects with 'message'
ast-grep --pattern 'Response({
$$$
"message": $_,
$$$
})'
Length of output: 1652
Script:
#!/bin/bash
# Let's check for error responses in the moderation_events.py file and related files
# to ensure consistency in using 'detail' instead of 'message'
# Check the specific file and its error handling
rg -g '*.py' '"detail":\s*[f"]|"error":\s*[f"]' src/django/api/views/v1/moderation_events.py
# Check error handling utilities that might be used
rg -g '*.py' 'handle_path_error|handle_errors_decorator' src/django/api/views/v1/utils.py
# Check related serializers
rg -g '*.py' '"detail":\s*[f"]|"error":\s*[f"]' src/django/api/serializers/v1/moderation_events_serializer.py src/django/api/serializers/v1/moderation_event_update_serializer.py
Length of output: 577
Script:
#!/bin/bash
# Let's examine the handle_path_error utility to see how it structures error responses
cat src/django/api/views/v1/utils.py
Length of output: 4162
src/django/api/tests/test_v1_utils.py (1)
Line range hint 1-147: Verify consistent usage of 'detail' across the codebase.
Let's ensure the migration from 'message' to 'detail' is complete across all files.
✅ Verification successful
Error handling is consistent with 'detail' key usage across the codebase
The verification shows that the codebase consistently uses the 'detail' key for error responses. The only instances of 'message' found are in test files (test_row_sector_serializer.py and test_row_facility_type_serializer.py) which appear to be testing specific validation scenarios. The actual error handling implementation across the codebase uses 'detail' key in:
- API responses (
api/middleware.py,api/facility_history.py) - Error handlers (
api/views/v1/utils.py) - Validation error responses (
api/serializers/v1/production_locations_serializer.py,api/serializers/v1/moderation_events_serializer.py)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of 'message' in error responses
echo "Checking for remaining 'message' keys in response objects..."
rg -g '*.py' "'message':\s*['\"]" src/
echo "Checking for response object definitions..."
ast-grep --pattern "Response($_, {'message': $_})"
echo "Checking serializer error messages..."
rg -g '*.py' "raise.*serializers\.ValidationError.*message" src/
Length of output: 1279
Script:
#!/bin/bash
# Let's check more thoroughly for error response patterns
# Check for 'detail' key usage in responses
echo "Checking 'detail' key usage in responses..."
rg -g '*.py' "'detail':\s*['\"]" src/
# Check for Response objects with error messages
echo "Checking Response objects with error handling..."
rg -g '*.py' "Response\(.*status=(?:400|404|500)" -A 2 src/
# Check ValidationError usage
echo "Checking ValidationError patterns..."
rg -g '*.py' "ValidationError\(" -A 2 src/
# Check error handling in test files
echo "Checking error assertions in tests..."
rg -g '*test*.py' "assert.*(?:detail|message).*response" src/
Length of output: 21069
src/react/src/components/Dashboard/DashboardModerationQueueListTable.jsx (1)
44-47: Verify makeContributionRecordLink compatibility.
Since we're changing from message to detail in error responses, we should verify that the makeContributionRecordLink utility function properly handles errors with the new format.
src/django/api/tests/test_facility_claim_view_set.py (1)
Line range hint 191-209: Verify consistent updates across related files
Let's ensure that all related files have been updated to use 'detail' instead of 'message'.
src/tests/v1/test_moderation_events.py (1)
151-151: LGTM: Error assertion updated correctly
The change from message to detail aligns with the PR objective of standardizing error responses.
src/django/api/serializers/v1/opensearch_common_validators/countries_validator.py
Show resolved
Hide resolved
src/django/api/serializers/v1/opensearch_common_validators/source_validator.py
Show resolved
Hide resolved
src/django/api/serializers/v1/opensearch_common_validators/status_validator.py
Show resolved
Hide resolved
Innavin369
requested review from
VadimKovalenkoSNF,
mazursasha1990,
roman-stolar and
vladsha-dev
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
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
🧹 Outside diff range and nitpick comments (2)
doc/release/RELEASE-NOTES.md (2)
Line range hint
12-24: Enhance release instructions formatConsider standardizing the release instructions format across all releases. For example:
### Release instructions: - * *Provide release instructions here.* + 1. Pre-deployment steps: + * Backup database + * Stop affected services + 2. Deployment steps: + * Apply migrations up to [migration_number] + * Run specific commands: [list_commands] + 3. Post-deployment verification: + * Verify database state + * Check service health
Line range hint
1-1000: Standardize release instructions across versionsThe release instructions vary significantly between releases. Some releases have detailed step-by-step instructions while others are minimal. Consider maintaining a consistent level of detail across all releases to ensure reliable deployments.
For example:
- Release 1.6.0 has specific migration steps
- Release 1.7.0 mentions "apply migrations" without specifying which ones
- Some releases lack post-deployment verification steps
Consider implementing a release checklist template that all releases must follow, including:
- Required pre-deployment steps
- Specific migrations to apply
- Commands to run
- Post-deployment verification steps
- Rollback procedures
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
doc/release/RELEASE-NOTES.md(1 hunks)src/django/api/serializers/v1/moderation_event_update_serializer.py(2 hunks)src/django/api/serializers/v1/moderation_events_serializer.py(2 hunks)src/django/api/serializers/v1/production_locations_serializer.py(2 hunks)src/django/api/services/opensearch/search.py(1 hunks)src/django/api/tests/test_v1_utils.py(2 hunks)src/django/api/views/v1/utils.py(5 hunks)src/tests/v1/test_moderation_events.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/django/api/serializers/v1/moderation_event_update_serializer.py
- src/django/api/serializers/v1/moderation_events_serializer.py
- src/django/api/serializers/v1/production_locations_serializer.py
- src/django/api/services/opensearch/search.py
- src/django/api/tests/test_v1_utils.py
- src/django/api/views/v1/utils.py
- src/tests/v1/test_moderation_events.py
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
React App | Jest test suite - Code coverage reportTotal: 27.8%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
…e objects (#431) [OSDEV-1453](https://opensupplyhub.atlassian.net/browse/OSDEV-1453) Use the detail keyword instead of message in response objects for endpoints in V1 specs. <img width="400" alt="Screenshot 2024-11-28 at 11 43 58" src="https://github.com/user-attachments/assets/9de4a0ab-9f69-4cb5-aa1b-f2f613007406"> [OSDEV-1453]: https://opensupplyhub.atlassian.net/browse/OSDEV-1453?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ <img width="400" alt="Screenshot 2024-11-28 at 13 49 44" src="https://github.com/user-attachments/assets/bdffc771-523b-443c-b148-d7a02e7c5f37"> --------- Co-authored-by: Inessa Druzhkova <[email protected]> Co-authored-by: Vadim Kovalenko <[email protected]>
3 participants
OSDEV-1453 Use the detail keyword instead of message in response objects for endpoints in V1 specs.