Skip to content

fix(webhooks): address Zaimwa9 feedback - use response.ok and add 2xx tests #7996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Marnie0415 wants to merge 2 commits into from
+48 −5
Closed

fix(webhooks): address Zaimwa9 feedback - use response.ok and add 2xx tests #7996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c595f84
fix(webhooks): accept 2xx status codes in webhook test endpoint
Marnie0415 Jul 11, 2026
9630524
fix(webhooks): use response.ok instead of status_code >= 400 and add …
Marnie0415 Jul 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 45 additions & 2 deletions api/tests/unit/webhooks/test_unit_webhooks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,49 @@ def test_send_test_webhook__200_response_from_webhook__returns_correct_response(
assert response_json["detail"] == "Webhook test successful"


@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"


Comment on lines +395 to +437

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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 -S

Repository: 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.py

Repository: 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)

@pytest.mark.parametrize(
"external_api_response_status, external_api_error_text, expected_final_status",
[
Expand Down Expand Up @@ -434,10 +477,10 @@ def test_send_test_webhook__various_error_status_codes__returns_correct_response
mock_post.assert_called_once()
response_json = response.json()
assert response_json["status"] == external_api_response_status
assert response_json["detail"] == "Webhook returned invalid status"
assert response_json["detail"] == "Webhook returned error status"
assert (
response_json["body"]
== "Please check the webhook endpoint to validate it returns a 200 OK."
== f"Webhook returned HTTP {external_api_response_status}."
)


Expand Down
6 changes: 3 additions & 3 deletions api/webhooks/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,11 @@ def test(self, request: Request) -> Response:
else WebhookType.ENVIRONMENT
)
response = send_test_request_to_webhook(webhook_url, secret, webhook_type)
if response.status_code != 200:
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}.",
Comment on lines +50 to +54

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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)
PY

Repository: 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 -n

Repository: 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 -n

Repository: Flagsmith/flagsmith

Length of output: 9376

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '300,340p' api/webhooks/webhooks.py | cat -n

Repository: Flagsmith/flagsmith

Length of output: 1822

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '315,335p' api/webhooks/webhooks.py | cat -n

Repository: 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

"status": response.status_code,
},
status=status.HTTP_400_BAD_REQUEST,
Expand Down
Loading