fix(webhooks): accept 2xx status codes in webhook test endpoint#7992
fix(webhooks): accept 2xx status codes in webhook test endpoint#7992Marnie0415 wants to merge 6 commits into
Conversation
|
@Marnie0415 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~5 minutes Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/webhooks/views.py (1)
50-58: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign the webhook error payload with the existing test assertions.
detailandbodynow differ fromapi/tests/unit/webhooks/test_unit_webhooks.py, so this change will fail the current webhook test unless the response text and assertions are updated together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f9f7ee49-c4fd-41aa-8ca0-d1ad7707e07b
📒 Files selected for processing (1)
api/webhooks/views.py
The webhook test endpoint previously treated any non-200 response as a failure. This is incorrect — HTTP 201 (Created), 202 (Accepted), and 204 (No Content) are all valid webhook responses. Change the condition from \status_code != 200\ to \status_code >= 400\ so that any 2xx response is considered successful. Also update the error message to include the actual status code instead of telling users to return a 200 OK.
Marnie0415
force-pushed
the
fix/webhook-test-status-codes
branch
from
8888e4c to
c595f84
Compare
|
Note: the existing test in \ est_send_test_webhook__various_error_status_codes__returns_correct_response\ has been updated to match the new error message and body format. The test now also validates that the actual status code from the external webhook is included in the response. |
|
Updated test assertions in test_unit_webhooks.py to match the new error messages. |
Address Zaimwa9's feedback on PR Flagsmith#7992: - Use not response.ok instead of status_code >= 400 to correctly treat 3xx redirects as failures - Add parametrised test for 201/202/204 status codes as success cases
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d9465c78-8465-4f7e-be16-6ca6da81080c
📒 Files selected for processing (2)
api/tests/unit/webhooks/test_unit_webhooks.pyapi/webhooks/views.py
The tests were mocking requests.post but the actual code calls requests.post in webhooks.webhooks module. Change mock path to webhooks.webhooks.requests.post to match the actual import path.
test.webhook.com resolves to a private IP in Docker, triggering the NoSSRFURLField validation. Use https://example.com instead.
The NoSSRFURLField resolves hostnames to check for private IPs. In Docker, test URLs resolve to private IPs. Mock socket.getaddrinfo to return a public IP so the SSRF check passes.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
@Zaimwa9 Thanks for the review! Addressed both suggestions: *1. Changed >= 400\ to 2. Added parametrised test for 201/202/204 Test Results (all 24 tests passing):\ ================= 24 passed in 50.62s ================== The new parametrised test verifies that 201, 202, and 204 are all treated as successful responses with Also addressed @coderabbitai suggestion to set \mock_response.ok = external_api_response_status < 400\ explicitly. |
|
Tip For best results, initiate chat on the files or code changes.
ᕦ(ò_óˇ)ᕤ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7992 +/- ##
==========================================
- Coverage 98.63% 98.48% -0.15%
==========================================
Files 1496 1496
Lines 59072 59098 +26
==========================================
- Hits 58266 58205 -61
- Misses 806 893 +87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
MagicMock().ok returns a truthy MagicMock, not a real boolean. Set mock_response.ok explicitly based on status code so that 'not response.ok' works correctly in the view.
Marnie0415
force-pushed
the
fix/webhook-test-status-codes
branch
from
811bbb5 to
a9f1da2
Compare
Response to @Zaimwa9 reviewSuggestion 1: Use not response.ok instead of >= 400File: api/webhooks/views.py line 50
Suggestion 2: Add parametrised test for 201/204File: api/tests/unit/webhooks/test_unit_webhooks.py @pytest.mark.parametrize("external_api_response_status", [201, 202, 204]) Test Results: 24/24 PASSEDtest_send_test_webhook__200_response_from_webhook__returns_correct_response PASSED ============================= 24 passed in 52.11s ============================== Ready for re-review. |
|
@Zaimwa9 All suggestions addressed, 24/24 tests passed. Ready for re-review. |
2 participants
Changes
The webhook test endpoint (
POST /api/v1/webhooks/test/) previously treated any non-200 response as a failure. This is incorrect - HTTP 201 (Created), 202 (Accepted), and 204 (No Content) are all valid and common webhook responses.Before: Users testing a webhook that returns 201 or 204 would see
Webhook returned invalid status - Please check the webhook endpoint to validate it returns a 200 OK.even though their webhook is working correctly.After: Any 2xx response (200-299) is treated as successful. Only 4xx/5xx responses are reported as failures, with the actual status code included in the error message.
How did you test this code?
!= 200->>= 400)test_unit_webhooks.pyto match the new error messages