Skip to content

Commit cf16f3d

Browse files
authored
fix: Webhook payloads do not include multivariate values (#6666)
1 parent 0a2464b commit cf16f3d

File tree

6 files changed

+227
-26
lines changed

6 files changed

+227
-26
lines changed

api/audit/signals.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
from audit.serializers import AuditLogListSerializer
1111
from audit.services import get_audited_instance_from_audit_log_record
1212
from features.models import FeatureState, FeatureStateValue
13+
from features.multivariate.models import MultivariateFeatureStateValue
1314
from features.signals import feature_state_change_went_live
1415
from integrations.common.models import IntegrationsModel
1516
from integrations.datadog.datadog import DataDogWrapper
@@ -214,9 +215,9 @@ def send_audit_log_event_to_slack(sender, instance, **kwargs): # type: ignore[n
214215
def send_feature_flag_went_live_signal(sender, instance, **kwargs): # type: ignore[no-untyped-def]
215216
audited_instance = get_audited_instance_from_audit_log_record(instance)
216217

217-
# Handle both FeatureState and FeatureStateValue audit logs
218-
# FeatureStateValue changes also have related_object_type=FEATURE_STATE
219-
if isinstance(audited_instance, FeatureStateValue):
218+
# Handle FeatureState, FeatureStateValue, and MultivariateFeatureStateValue audit logs
219+
# All these types have related_object_type=FEATURE_STATE
220+
if isinstance(audited_instance, (FeatureStateValue, MultivariateFeatureStateValue)):
220221
feature_state = audited_instance.feature_state
221222
elif isinstance(audited_instance, FeatureState):
222223
feature_state = audited_instance

api/environments/models.py

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -638,6 +638,8 @@ def generate_webhook_feature_state_data(
638638
identity_id: int | str | None = None,
639639
identity_identifier: str | None = None,
640640
feature_segment: FeatureSegment | None = None,
641+
multivariate_feature_state_values: list[MultivariateFeatureStateValue]
642+
| None = None,
641643
) -> dict: # type: ignore[type-arg]
642644
if (identity_id or identity_identifier) and not (
643645
identity_id and identity_identifier
@@ -647,8 +649,20 @@ def generate_webhook_feature_state_data(
647649
if (identity_id and identity_identifier) and feature_segment:
648650
raise ValueError("Cannot provide identity information and feature segment")
649651

652+
mv_values_data = [
653+
{
654+
"id": mv.id,
655+
"multivariate_feature_option": {
656+
"id": mv.multivariate_feature_option_id,
657+
"value": mv.multivariate_feature_option.value,
658+
},
659+
"percentage_allocation": mv.percentage_allocation,
660+
}
661+
for mv in (multivariate_feature_state_values or [])
662+
]
663+
650664
# TODO: refactor to use a serializer / schema
651-
data = {
665+
data: dict[str, typing.Any] = {
652666
"feature": {
653667
"id": feature.id,
654668
"created_date": feature.created_date.strftime("%Y-%m-%dT%H:%M:%S.%fZ"),
@@ -671,6 +685,7 @@ def generate_webhook_feature_state_data(
671685
"feature_segment": None,
672686
"enabled": enabled,
673687
"feature_state_value": value,
688+
"multivariate_feature_state_values": mv_values_data,
674689
}
675690
if feature_segment:
676691
data["feature_segment"] = {

api/features/tasks.py

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
import logging
2+
from typing import Any
23

34
from task_processor.decorators import (
45
register_task_handler,
56
)
67

78
from environments.models import Webhook
89
from features.models import Feature, FeatureState
10+
from features.multivariate.models import MultivariateFeatureStateValue
911
from webhooks.constants import WEBHOOK_DATETIME_FORMAT
1012
from webhooks.tasks import (
1113
call_environment_webhooks,
@@ -41,7 +43,7 @@ def trigger_feature_state_change_webhooks( # type: ignore[no-untyped-def]
4143
new_state = (
4244
None
4345
if event_type == WebhookEventType.FLAG_DELETED
44-
else _get_feature_state_webhook_data(instance) # type: ignore[no-untyped-call]
46+
else _get_feature_state_webhook_data(instance)
4547
)
4648
data = {"new_state": new_state, "changed_by": changed_by, "timestamp": timestamp}
4749
previous_state = _get_previous_state(instance, history_instance, event_type)
@@ -68,33 +70,56 @@ def _get_previous_state(
6870
event_type: WebhookEventType,
6971
) -> dict: # type: ignore[type-arg]
7072
if event_type == WebhookEventType.FLAG_DELETED:
71-
return _get_feature_state_webhook_data(instance) # type: ignore[no-untyped-call,no-any-return]
73+
return _get_feature_state_webhook_data(instance)
7274
if history_instance and history_instance.prev_record:
73-
return _get_feature_state_webhook_data( # type: ignore[no-untyped-call,no-any-return]
75+
return _get_feature_state_webhook_data(
7476
history_instance.prev_record.instance, previous=True
7577
)
7678
return None # type: ignore[return-value]
7779

7880

79-
def _get_feature_state_webhook_data(feature_state, previous=False): # type: ignore[no-untyped-def]
80-
# TODO: fix circular imports and use serializers instead.
81-
feature_state_value = (
82-
feature_state.get_feature_state_value()
83-
if not previous
84-
else feature_state.previous_feature_state_value
85-
)
81+
def _get_feature_state_webhook_data(
82+
feature_state: FeatureState,
83+
previous: bool = False,
84+
) -> dict[str, Any]:
85+
if previous:
86+
value = feature_state.previous_feature_state_value
87+
mv_values = _get_previous_multivariate_values(feature_state)
88+
else:
89+
value = feature_state.get_feature_state_value()
90+
mv_values = list(
91+
feature_state.multivariate_feature_state_values.select_related(
92+
"multivariate_feature_option"
93+
).all()
94+
)
8695

96+
assert feature_state.environment is not None
8797
return Webhook.generate_webhook_feature_state_data(
8898
feature_state.feature,
8999
environment=feature_state.environment,
90100
enabled=feature_state.enabled,
91-
value=feature_state_value,
101+
value=value,
92102
identity_id=feature_state.identity_id,
93103
identity_identifier=getattr(feature_state.identity, "identifier", None),
94104
feature_segment=feature_state.feature_segment,
105+
multivariate_feature_state_values=mv_values,
95106
)
96107

97108

109+
def _get_previous_multivariate_values(
110+
feature_state: FeatureState,
111+
) -> list[MultivariateFeatureStateValue]:
112+
"""Get previous multivariate values from history."""
113+
mv_values: list[MultivariateFeatureStateValue] = []
114+
for mv in MultivariateFeatureStateValue.objects.filter(
115+
feature_state_id=feature_state.id
116+
).select_related("multivariate_feature_option"):
117+
history = mv.history.first()
118+
if history and history.prev_record:
119+
mv_values.append(history.prev_record.instance)
120+
return mv_values
121+
122+
98123
@register_task_handler()
99124
def delete_feature(feature_id: int) -> None:
100125
Feature.objects.get(pk=feature_id).delete()

api/tests/integration/features/featurestate/test_webhooks.py

Lines changed: 141 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,54 @@
11
import json
22

3+
import pytest
4+
import responses
35
from django.urls import reverse
6+
from pytest_lazyfixture import lazy_fixture # type: ignore[import-untyped]
47
from pytest_mock import MockerFixture
58
from rest_framework import status
69
from rest_framework.test import APIClient
710

11+
WEBHOOK_URL = "https://example.com/webhook"
812

13+
14+
@pytest.fixture
15+
def environment_webhook(
16+
admin_client: APIClient,
17+
environment: int,
18+
environment_api_key: str,
19+
) -> str:
20+
"""Create an environment webhook via API and return its URL."""
21+
url = reverse(
22+
"api-v1:environments:environment-webhooks-list",
23+
args=[environment_api_key],
24+
)
25+
response = admin_client.post(url, data={"url": WEBHOOK_URL, "enabled": True})
26+
assert response.status_code == status.HTTP_201_CREATED
27+
return WEBHOOK_URL
28+
29+
30+
@pytest.fixture
31+
def organisation_webhook(
32+
admin_client: APIClient,
33+
organisation: int,
34+
) -> str:
35+
"""Create an organisation webhook via API and return its URL."""
36+
url = reverse(
37+
"api-v1:organisations:organisation-webhooks-list",
38+
args=[organisation],
39+
)
40+
response = admin_client.post(url, data={"url": WEBHOOK_URL, "enabled": True})
41+
assert response.status_code == status.HTTP_201_CREATED
42+
return WEBHOOK_URL
43+
44+
45+
@responses.activate
946
def test_update_segment_override__webhook_payload_has_correct_previous_and_new_values(
1047
admin_client: APIClient,
1148
environment: int,
1249
feature: int,
1350
segment: int,
14-
mocker: MockerFixture,
51+
environment_webhook: str,
1552
) -> None:
1653
"""
1754
Test for issue #6050: Webhook payload shows incorrect previous_state values
@@ -26,6 +63,8 @@ def test_update_segment_override__webhook_payload_has_correct_previous_and_new_v
2663
old_value = 0
2764
new_value = 1
2865

66+
responses.add(responses.POST, environment_webhook, status=200)
67+
2968
# First create a feature_segment
3069
feature_segment_url = reverse("api-v1:features:feature-segment-list")
3170
feature_segment_data = {
@@ -56,11 +95,8 @@ def test_update_segment_override__webhook_payload_has_correct_previous_and_new_v
5695
assert create_response.status_code == status.HTTP_201_CREATED
5796
segment_override_id = create_response.json()["id"]
5897

59-
# Mock call_environment_webhooks to capture the actual payload
60-
mock_call_environment_webhooks = mocker.patch(
61-
"features.tasks.call_environment_webhooks"
62-
)
63-
mocker.patch("features.tasks.call_organisation_webhooks")
98+
# Clear responses from create operation
99+
responses.calls.reset()
64100

65101
# When - update the segment override via API
66102
url = reverse("api-v1:features:featurestates-detail", args=[segment_override_id])
@@ -78,12 +114,107 @@ def test_update_segment_override__webhook_payload_has_correct_previous_and_new_v
78114
# Then
79115
assert response.status_code == status.HTTP_200_OK
80116

81-
# Verify webhook was called
82-
mock_call_environment_webhooks.delay.assert_called_once()
83-
webhook_args = mock_call_environment_webhooks.delay.call_args.kwargs["args"]
84-
webhook_payload = webhook_args[1] # (environment_id, data, event_type)
117+
# Verify webhook was called with correct payload
118+
assert len(responses.calls) == 1
119+
webhook_payload = json.loads(responses.calls[0].request.body)["data"] # type: ignore[union-attr]
85120

86121
# Verify the payload has correct values
87122
assert webhook_payload["new_state"]["feature_segment"] is not None
88123
assert webhook_payload["new_state"]["feature_state_value"] == new_value
89124
assert webhook_payload["previous_state"]["feature_state_value"] == old_value
125+
126+
127+
@pytest.mark.parametrize(
128+
"webhook",
129+
[lazy_fixture("environment_webhook"), lazy_fixture("organisation_webhook")],
130+
)
131+
@responses.activate
132+
def test_update_multivariate_percentage__webhook_payload_includes_multivariate_values(
133+
admin_client: APIClient,
134+
environment: int,
135+
feature: int,
136+
mv_option_50_percent: int,
137+
mv_option_value: str,
138+
webhook: str,
139+
mocker: MockerFixture,
140+
) -> None:
141+
"""
142+
Test for issue #6190: Webhook payloads do not include multivariate values.
143+
144+
When updating the percentage allocation of a multivariate option,
145+
the webhook payload should include the multivariate_feature_state_values
146+
with their percentage allocations.
147+
"""
148+
# Given
149+
# Get the feature state for this environment
150+
feature_states_url = reverse("api-v1:features:featurestates-list")
151+
feature_states_response = admin_client.get(
152+
f"{feature_states_url}?environment={environment}&feature={feature}"
153+
)
154+
assert feature_states_response.status_code == status.HTTP_200_OK
155+
feature_state = feature_states_response.json()["results"][0]
156+
feature_state_id = feature_state["id"]
157+
158+
# Get current multivariate feature state values
159+
assert len(feature_state["multivariate_feature_state_values"]) == 1
160+
mv_fs_value = feature_state["multivariate_feature_state_values"][0]
161+
old_percentage = mv_fs_value["percentage_allocation"]
162+
new_percentage = 75
163+
164+
responses.add(responses.POST, webhook, status=200)
165+
166+
# When
167+
# update only the multivariate percentage allocation
168+
url = reverse("api-v1:features:featurestates-detail", args=[feature_state_id])
169+
data = {
170+
"id": feature_state_id,
171+
"enabled": feature_state["enabled"],
172+
"feature_state_value": feature_state["feature_state_value"],
173+
"environment": environment,
174+
"feature": feature,
175+
"multivariate_feature_state_values": [
176+
{
177+
"id": mv_fs_value["id"],
178+
"multivariate_feature_option": mv_fs_value[
179+
"multivariate_feature_option"
180+
],
181+
"percentage_allocation": new_percentage,
182+
}
183+
],
184+
}
185+
admin_client.put(url, data=json.dumps(data), content_type="application/json")
186+
187+
# Then
188+
# `FLAG_UPDATED`` webhook was called
189+
# (should be the last sent event)
190+
last_call = responses.calls[-1]
191+
assert not isinstance(last_call, list)
192+
webhook_payload = json.loads(last_call.request.body)
193+
assert webhook_payload["event_type"] == "FLAG_UPDATED"
194+
195+
# the payload includes multivariate values
196+
event_data = webhook_payload["data"]
197+
198+
assert "multivariate_feature_state_values" in event_data["new_state"]
199+
assert "multivariate_feature_state_values" in event_data["previous_state"]
200+
201+
assert event_data["new_state"]["multivariate_feature_state_values"] == [
202+
{
203+
"id": mocker.ANY,
204+
"multivariate_feature_option": {
205+
"id": mv_option_50_percent,
206+
"value": mv_option_value,
207+
},
208+
"percentage_allocation": new_percentage,
209+
},
210+
]
211+
assert event_data["previous_state"]["multivariate_feature_state_values"] == [
212+
{
213+
"id": mocker.ANY,
214+
"multivariate_feature_option": {
215+
"id": mv_option_50_percent,
216+
"value": mv_option_value,
217+
},
218+
"percentage_allocation": old_percentage,
219+
},
220+
]

api/webhooks/webhooks.py

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@
1818
from core.signing import sign_payload
1919
from environments.models import Environment, Webhook
2020
from features.models import Feature
21+
from features.multivariate.models import (
22+
MultivariateFeatureOption,
23+
MultivariateFeatureStateValue,
24+
)
2125
from organisations.models import OrganisationWebhook
2226
from projects.models import ( # type: ignore[attr-defined]
2327
Organisation,
@@ -328,6 +332,19 @@ def generate_environment_sample_webhook_data() -> dict[str, Any]:
328332
),
329333
)
330334

335+
mv_option = MultivariateFeatureOption(
336+
id=1,
337+
feature=feature,
338+
default_percentage_allocation=50,
339+
type="unicode",
340+
string_value="variant_a",
341+
)
342+
mv_state_value = MultivariateFeatureStateValue(
343+
id=1,
344+
multivariate_feature_option=mv_option,
345+
percentage_allocation=50,
346+
)
347+
331348
data = {
332349
"changed_by": "[email protected]",
333350
"timestamp": "2021-06-18T07:50:26.595298Z",
@@ -338,6 +355,7 @@ def generate_environment_sample_webhook_data() -> dict[str, Any]:
338355
value="feature_state_value",
339356
identity_id=1,
340357
identity_identifier="test_identity",
358+
multivariate_feature_state_values=[mv_state_value],
341359
),
342360
"previous_state": Webhook.generate_webhook_feature_state_data(
343361
feature=feature,
@@ -346,6 +364,7 @@ def generate_environment_sample_webhook_data() -> dict[str, Any]:
346364
value="old_feature_state_value",
347365
identity_id=1,
348366
identity_identifier="test_identity",
367+
multivariate_feature_state_values=[mv_state_value],
349368
),
350369
}
351370

0 commit comments

Comments
 (0)