fix(webhooks): address Zaimwa9 feedback - use response.ok and add 2xx tests#7996
fix(webhooks): address Zaimwa9 feedback - use response.ok and add 2xx tests#7996Marnie0415 wants to merge 2 commits into
Conversation
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.
…parametrised 2xx tests - Change condition from esponse.status_code >= 400 to ot response.ok to correctly handle 3xx redirects as failures (Zaimwa9's feedback) - Add parametrised test for 201/202/204 status codes as success cases - This ensures only 2xx responses are treated as successful webhook tests
|
@Marnie0415 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe webhook test endpoint now treats all 2xx responses from external webhooks as successful, preserving the returned external status in the response JSON. Non-2xx responses now produce a revised error detail and include the external HTTP status in the body. Unit tests cover status codes 201, 202, and 204, alongside updated non-2xx assertions. Estimated code review effort: 2 (Simple) | ~10 minutes Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2102279b-8e93-4109-94a0-ff089f8281e5
📒 Files selected for processing (2)
api/tests/unit/webhooks/test_unit_webhooks.pyapi/webhooks/views.py
| @pytest.mark.parametrize( | ||
| "external_api_response_status", | ||
| [ | ||
| 201, | ||
| 202, | ||
| 204, | ||
| ], | ||
| ) | ||
| def test_send_test_webhook__various_2xx_status_codes__returns_success( | ||
| mocker: MockerFixture, | ||
| admin_client: APIClient, | ||
| external_api_response_status: int, | ||
| organisation: Organisation, | ||
| ) -> None: | ||
| # Given | ||
| webhook_url = "http://test.webhook.com" | ||
| mock_post = mocker.patch("requests.post") | ||
| mock_response = MagicMock() | ||
| mock_response.status_code = external_api_response_status | ||
| mock_response.text = "success" | ||
| mock_post.return_value = mock_response | ||
|
|
||
| url = reverse("api-v1:webhooks:webhooks-test") | ||
|
|
||
| data = { | ||
| "webhook_url": webhook_url, | ||
| "secret": "some-secret", | ||
| "scope": {"type": "organisation", "id": organisation.id}, | ||
| } | ||
|
|
||
| # When | ||
| response = admin_client.post( | ||
| url, data=json.dumps(data), content_type="application/json" | ||
| ) | ||
|
|
||
| # Then | ||
| assert response.status_code == 200 | ||
| mock_post.assert_called_once() | ||
| response_json = response.json() | ||
| assert response_json["status"] == external_api_response_status | ||
| assert response_json["detail"] == "Webhook test successful" | ||
|
|
||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test file around the referenced lines
file="api/tests/unit/webhooks/test_unit_webhooks.py"
wc -l "$file"
sed -n '360,520p' "$file"
# Find the webhook test endpoint implementation and any use of response.ok / status_code
rg -n "webhooks-test|response\.ok|status_code|requests\.post|Webhook test successful|redirect" api -SRepository: Flagsmith/flagsmith
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the webhook test view implementation
sed -n '1,120p' api/webhooks/views.py
# Probe MagicMock/request semantics relevant to response.ok
python3 - <<'PY'
from unittest.mock import MagicMock
m = MagicMock()
print("has_ok_attr_before_access:", hasattr(m, "ok"))
print("type_of_ok:", type(m.ok).__name__)
print("bool_of_ok:", bool(m.ok))
print("status_code_default:", getattr(m, "status_code", None))
m.status_code = 400
print("bool_of_ok_after_status_code_400:", bool(m.ok))
PY
# Inspect the nearby error-test assertions
sed -n '440,500p' api/tests/unit/webhooks/test_unit_webhooks.pyRepository: Flagsmith/flagsmith
Length of output: 4832
Set .ok explicitly on the mocked webhook responses. MagicMock makes .ok truthy by default, so the non-2xx cases still take the success branch and never exercise the error-response assertions. Use a real requests.Response or set .ok = False for those cases.
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 426-426: use jsonify instead of json.dumps for JSON output
Context: json.dumps(data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[warning] 409-409: Do not make http calls without encryption
Context: "http://test.webhook.com"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
| if not response.ok: | ||
| return Response( | ||
| { | ||
| "detail": "Webhook returned invalid status", | ||
| "body": "Please check the webhook endpoint to validate it returns a 200 OK.", | ||
| "detail": "Webhook returned error status", | ||
| "body": f"Webhook returned HTTP {response.status_code}.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python - <<'PY'
from requests import Response
for code in (201, 301, 400):
response = Response()
response.status_code = code
print(code, response.ok)
PYRepository: Flagsmith/flagsmith
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="api/webhooks/views.py"
wc -l "$file"
printf '\n--- outline ---\n'
ast-grep outline "$file" --view expanded || true
printf '\n--- relevant lines ---\n'
sed -n '1,120p' "$file" | cat -nRepository: Flagsmith/flagsmith
Length of output: 3526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="api/webhooks/webhooks.py"
wc -l "$file"
printf '\n--- outline ---\n'
ast-grep outline "$file" --view expanded || true
printf '\n--- relevant lines ---\n'
sed -n '1,200p' "$file" | cat -nRepository: Flagsmith/flagsmith
Length of output: 9376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,340p' api/webhooks/webhooks.py | cat -nRepository: Flagsmith/flagsmith
Length of output: 1822
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '315,335p' api/webhooks/webhooks.py | cat -nRepository: Flagsmith/flagsmith
Length of output: 1024
Use an explicit 2xx check here.
send_test_request_to_webhook() sets allow_redirects=False, so 3xx responses reach this branch. response.ok still treats them as success, which makes redirects look like a passing webhook test. Switch to 200 <= response.status_code < 300 instead.
Source: MCP tools
1 participant
Summary
This PR addresses the review feedback from @Zaimwa9 on PR #7992.
Changes Made
Use not response.ok instead of response.status_code >= 400 (views.py:50)
Added parametrised test for 201/202/204 status codes (test_unit_webhooks.py)
Why This Change
The original PR changed the condition from status_code != 200 to status_code >= 400. While this fixed the original issue (accepting 201/204), it introduced a new problem: 3xx redirect responses would be treated as successes.
Using not response.ok is the idiomatic Python requests way to check for error responses - it returns False only for 4xx and 5xx status codes.
Test Results
The tests can be run locally with: make test opts='tests/unit/webhooks/test_unit_webhooks.py -v -n0'
Or via CI (GitHub Actions will run automatically).
Related