[OSDEV-1467] Releases. Block API create facility endpoint if "disable_list_uploading" switch is enabled in Admin panel#426
Conversation
React App | Jest test suite - Code coverage reportTotal: 27.8%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request updates the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@Innavin369 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/django/api/views/facility/facilities_view_set.py (1)
581-583: Consider moving the maintenance message to a constants file.The maintenance message could be reused across different endpoints. Consider moving it to a constants file for better maintainability and consistency.
Example implementation:
# api/constants.py +MAINTENANCE_MESSAGES = { + 'UPLOADING_DISABLED': ('Open Supply Hub is undergoing maintenance and ' + 'not accepting new data at the moment. Please ' + 'try again in a few minutes.') +} # facilities_view_set.py +from api.constants import MAINTENANCE_MESSAGES - block_message = ('Open Supply Hub is undergoing maintenance and ' - 'not accepting new data at the moment. Please ' - 'try again in a few minutes.') + block_message = MAINTENANCE_MESSAGES['UPLOADING_DISABLED']doc/release/RELEASE-NOTES.md (1)
Line range hint
1-24: Release 1.26.0 sections need completion.Several critical sections are incomplete:
- Release date is set to future (December 14, 2024)
- Database migrations section contains placeholder text
- Code/API changes section contains placeholder text
- Architecture/Environment changes section contains placeholder text
- Release instructions section contains placeholder text
Please update these sections with actual implementation details before release.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
doc/release/RELEASE-NOTES.md(1 hunks)src/django/api/views/facility/facilities_view_set.py(2 hunks)
🔇 Additional comments (3)
src/django/api/views/facility/facilities_view_set.py (1)
580-584: LGTM! Implementation aligns with requirements.
The code correctly implements the requirement to block the facility creation endpoint when the 'disable_list_uploading' switch is enabled. The implementation:
- Uses appropriate error handling with
ServiceUnavailableException - Returns a clear user-friendly message
- Checks the feature flag early to prevent unnecessary processing
Let's verify the error handling implementation:
✅ Verification successful
Implementation is correct and consistent across the codebase
The verification confirms that:
ServiceUnavailableExceptionis properly defined insrc/django/api/exceptions.pywith correct HTTP 503 status code- The
disable_list_uploadingfeature flag is properly defined via a migration and managed through waffle switches - The same error handling pattern is consistently used in both
facilities_view_set.pyandfacility_list_view_set.pyto block uploads during maintenance - The implementation is tested as evidenced by the test file
test_facility_list_create.py
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ServiceUnavailableException is properly imported and used
# Check if ServiceUnavailableException is defined in api/exceptions.py
rg -A 5 "class ServiceUnavailableException" api/exceptions.py
# Verify no other endpoints are using this exception for the same purpose
rg "disable_list_uploading.*ServiceUnavailableException" --type py
Length of output: 190
Script:
#!/bin/bash
# Let's try searching in the src directory structure
# Check for ServiceUnavailableException definition
rg -A 5 "class ServiceUnavailableException" src/
# Check imports of ServiceUnavailableException
rg "from.*import.*ServiceUnavailableException" src/
# Check all usages of ServiceUnavailableException
rg "ServiceUnavailableException" src/
Length of output: 1313
Script:
#!/bin/bash
# Let's check the context of disable_list_uploading usage in facility_list_view_set.py
rg -B 2 -A 2 "disable_list_uploading" src/django/api/views/facility/facility_list_view_set.py
# Check if the switch is defined in a central location
rg "disable_list_uploading" src/ --type py
Length of output: 1696
doc/release/RELEASE-NOTES.md (2)
Line range hint 25-27: Provide implementation details for the waiter replacement.
The bugfix section mentions replacing boto3 waiter with a custom one but doesn't provide:
- The reason why boto3 waiter couldn't wait more than half an hour
- The implementation details of the custom waiter
- The impact of this change on existing functionality
53-53: Verify the API endpoint disabling implementation.
The release notes mention disabling the endpoint POST /api/facilities/?public=true&create=true during release process, but don't provide details about:
- The specific error message returned with 503 status code
- The conditions under which the endpoint is disabled
- The impact on existing API consumers
✅ Verification successful
Let me analyze the verification results:
- Found the implementation of disabling endpoint
POST /api/facilities/?public=true&create=truein multiple files:
-
src/django/api/views/facility/facilities_view_set.py- Checks thedisable_list_uploadingswitch and returns 503 with message "Open Supply Hub is undergoing maintenance and not accepting new data at the moment. Please try again in a few minutes." -
src/django/api/migrations/0161_create_bdisable_list_uploading.py- Creates thedisable_list_uploadingswitch in Django admin panel
- The implementation includes:
- Proper error message with 503 status code
- Switch in Django admin panel to control the feature
- Migration to create the switch
- Tests verifying the functionality
The endpoint disabling implementation is properly done with all necessary components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of endpoint disabling functionality
# Test: Search for the endpoint implementation and error handling
rg -A 10 "disable_list_uploading"
# Test: Search for the error message implementation
rg -A 5 "503"
Length of output: 142863
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 56.14%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.91%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Django App | Unittest test suite - Code coverage reportTotal: 79.14%Your code coverage diff: -0.02% ▾ ✅ All code changes are covered |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
doc/release/RELEASE-NOTES.md (1)
Line range hint
1124-1131: Release instructions need to be more specificThe release instructions should include:
- The exact order of operations
- Any required system state checks between steps
- Potential rollback procedures
Consider updating the release instructions to:
### Release instructions: - apply migrations up to 0124_itroduce_raw_json - execute command fill_raw_json - apply migrations up to 0129_delete_facility_index - apply command index_facilities_new +### Release instructions: +1. Pre-deployment checks: + - Ensure database backup is available + - Verify system is in a stable state + +2. Migration steps (in order): + a. Apply migrations up to 0124_itroduce_raw_json + b. Execute command: fill_raw_json + c. Verify raw_json data population completed successfully + d. Apply remaining migrations up to 0129_delete_facility_index + e. Execute command: index_facilities_new + +3. Post-deployment verification: + - Verify all migrations completed successfully + - Check system logs for any errors + - Verify API endpoints are responding correctly + +4. Rollback procedure (if needed): + - Restore from pre-deployment backup + - Revert code to previous version
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
doc/release/RELEASE-NOTES.md(1 hunks)
🔇 Additional comments (2)
doc/release/RELEASE-NOTES.md (2)
Line range hint 1-24: LGTM! Clear and well-structured release notes header
The header section follows best practices by:
- Clearly stating adherence to Semantic Versioning
- Including a reference to the template file
- Providing proper formatting for version numbers
58-58: Verify the implementation of the API endpoint disabling functionality
The note indicates a critical change where the POST /api/facilities/ endpoint will be disabled during release process, returning a 503 error. This needs careful verification to ensure proper error handling and minimal disruption to users.
✅ Verification successful
Implementation of API endpoint disabling functionality is properly implemented and verified
The verification shows a complete and robust implementation:
- A waffle switch
disable_list_uploadingis properly implemented:- Migration file
0161_create_bdisable_list_uploading.pycreates the switch - Switch is used in both
FacilityViewSetandFacilityListViewSet - Proper error handling with
ServiceUnavailableExceptionreturning 503 status code - Consistent error message across endpoints
- Unit tests verify the functionality
- Frontend components handle the switch state
- Management command exists to control the switch
- Migration file
The implementation matches what's described in the release notes and includes all necessary components for a reliable feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of the API endpoint disabling functionality
# Test: Check if the disable_list_uploading switch exists in the waffle_switch table
echo "Checking for disable_list_uploading switch..."
rg -A 5 "disable_list_uploading"
# Test: Check the error handling implementation
echo "Checking error handling implementation..."
rg -A 10 "ServiceUnavailableException"
Length of output: 11160



OSDEV-1467 Implemented disabling endpoint
POST /api/facilities/during the release process. It is raising an error message with status code 503.