diff --git a/api/audit/signals.py b/api/audit/signals.py index 58328df054c5..65712d164f3f 100644 --- a/api/audit/signals.py +++ b/api/audit/signals.py @@ -10,6 +10,7 @@ from audit.serializers import AuditLogListSerializer from audit.services import get_audited_instance_from_audit_log_record from features.models import FeatureState, FeatureStateValue +from features.multivariate.models import MultivariateFeatureStateValue from features.signals import feature_state_change_went_live from integrations.common.models import IntegrationsModel from integrations.datadog.datadog import DataDogWrapper @@ -214,9 +215,9 @@ def send_audit_log_event_to_slack(sender, instance, **kwargs): # type: ignore[n def send_feature_flag_went_live_signal(sender, instance, **kwargs): # type: ignore[no-untyped-def] audited_instance = get_audited_instance_from_audit_log_record(instance) - # Handle both FeatureState and FeatureStateValue audit logs - # FeatureStateValue changes also have related_object_type=FEATURE_STATE - if isinstance(audited_instance, FeatureStateValue): + # Handle FeatureState, FeatureStateValue, and MultivariateFeatureStateValue audit logs + # All these types have related_object_type=FEATURE_STATE + if isinstance(audited_instance, (FeatureStateValue, MultivariateFeatureStateValue)): feature_state = audited_instance.feature_state elif isinstance(audited_instance, FeatureState): feature_state = audited_instance diff --git a/api/environments/models.py b/api/environments/models.py index 2c3000ea2b01..d5b27ea318db 100644 --- a/api/environments/models.py +++ b/api/environments/models.py @@ -638,6 +638,8 @@ def generate_webhook_feature_state_data( identity_id: int | str | None = None, identity_identifier: str | None = None, feature_segment: FeatureSegment | None = None, + multivariate_feature_state_values: list[MultivariateFeatureStateValue] + | None = None, ) -> dict: # type: ignore[type-arg] if (identity_id or identity_identifier) and not ( identity_id and identity_identifier @@ -647,8 +649,20 @@ def generate_webhook_feature_state_data( if (identity_id and identity_identifier) and feature_segment: raise ValueError("Cannot provide identity information and feature segment") + mv_values_data = [ + { + "id": mv.id, + "multivariate_feature_option": { + "id": mv.multivariate_feature_option_id, + "value": mv.multivariate_feature_option.value, + }, + "percentage_allocation": mv.percentage_allocation, + } + for mv in (multivariate_feature_state_values or []) + ] + # TODO: refactor to use a serializer / schema - data = { + data: dict[str, typing.Any] = { "feature": { "id": feature.id, "created_date": feature.created_date.strftime("%Y-%m-%dT%H:%M:%S.%fZ"), @@ -671,6 +685,7 @@ def generate_webhook_feature_state_data( "feature_segment": None, "enabled": enabled, "feature_state_value": value, + "multivariate_feature_state_values": mv_values_data, } if feature_segment: data["feature_segment"] = { diff --git a/api/features/tasks.py b/api/features/tasks.py index 1489e5a91785..4abf7540406b 100644 --- a/api/features/tasks.py +++ b/api/features/tasks.py @@ -1,4 +1,5 @@ import logging +from typing import Any from task_processor.decorators import ( register_task_handler, @@ -6,6 +7,7 @@ from environments.models import Webhook from features.models import Feature, FeatureState +from features.multivariate.models import MultivariateFeatureStateValue from webhooks.constants import WEBHOOK_DATETIME_FORMAT from webhooks.tasks import ( call_environment_webhooks, @@ -41,7 +43,7 @@ def trigger_feature_state_change_webhooks( # type: ignore[no-untyped-def] new_state = ( None if event_type == WebhookEventType.FLAG_DELETED - else _get_feature_state_webhook_data(instance) # type: ignore[no-untyped-call] + else _get_feature_state_webhook_data(instance) ) data = {"new_state": new_state, "changed_by": changed_by, "timestamp": timestamp} previous_state = _get_previous_state(instance, history_instance, event_type) @@ -68,33 +70,56 @@ def _get_previous_state( event_type: WebhookEventType, ) -> dict: # type: ignore[type-arg] if event_type == WebhookEventType.FLAG_DELETED: - return _get_feature_state_webhook_data(instance) # type: ignore[no-untyped-call,no-any-return] + return _get_feature_state_webhook_data(instance) if history_instance and history_instance.prev_record: - return _get_feature_state_webhook_data( # type: ignore[no-untyped-call,no-any-return] + return _get_feature_state_webhook_data( history_instance.prev_record.instance, previous=True ) return None # type: ignore[return-value] -def _get_feature_state_webhook_data(feature_state, previous=False): # type: ignore[no-untyped-def] - # TODO: fix circular imports and use serializers instead. - feature_state_value = ( - feature_state.get_feature_state_value() - if not previous - else feature_state.previous_feature_state_value - ) +def _get_feature_state_webhook_data( + feature_state: FeatureState, + previous: bool = False, +) -> dict[str, Any]: + if previous: + value = feature_state.previous_feature_state_value + mv_values = _get_previous_multivariate_values(feature_state) + else: + value = feature_state.get_feature_state_value() + mv_values = list( + feature_state.multivariate_feature_state_values.select_related( + "multivariate_feature_option" + ).all() + ) + assert feature_state.environment is not None return Webhook.generate_webhook_feature_state_data( feature_state.feature, environment=feature_state.environment, enabled=feature_state.enabled, - value=feature_state_value, + value=value, identity_id=feature_state.identity_id, identity_identifier=getattr(feature_state.identity, "identifier", None), feature_segment=feature_state.feature_segment, + multivariate_feature_state_values=mv_values, ) +def _get_previous_multivariate_values( + feature_state: FeatureState, +) -> list[MultivariateFeatureStateValue]: + """Get previous multivariate values from history.""" + mv_values: list[MultivariateFeatureStateValue] = [] + for mv in MultivariateFeatureStateValue.objects.filter( + feature_state_id=feature_state.id + ).select_related("multivariate_feature_option"): + history = mv.history.first() + if history and history.prev_record: + mv_values.append(history.prev_record.instance) + return mv_values + + @register_task_handler() def delete_feature(feature_id: int) -> None: Feature.objects.get(pk=feature_id).delete() diff --git a/api/tests/integration/features/featurestate/test_webhooks.py b/api/tests/integration/features/featurestate/test_webhooks.py index 4f08c9f7f7f8..3cbd65b7d343 100644 --- a/api/tests/integration/features/featurestate/test_webhooks.py +++ b/api/tests/integration/features/featurestate/test_webhooks.py @@ -1,17 +1,54 @@ import json +import pytest +import responses from django.urls import reverse +from pytest_lazyfixture import lazy_fixture # type: ignore[import-untyped] from pytest_mock import MockerFixture from rest_framework import status from rest_framework.test import APIClient +WEBHOOK_URL = "https://example.com/webhook" + +@pytest.fixture +def environment_webhook( + admin_client: APIClient, + environment: int, + environment_api_key: str, +) -> str: + """Create an environment webhook via API and return its URL.""" + url = reverse( + "api-v1:environments:environment-webhooks-list", + args=[environment_api_key], + ) + response = admin_client.post(url, data={"url": WEBHOOK_URL, "enabled": True}) + assert response.status_code == status.HTTP_201_CREATED + return WEBHOOK_URL + + +@pytest.fixture +def organisation_webhook( + admin_client: APIClient, + organisation: int, +) -> str: + """Create an organisation webhook via API and return its URL.""" + url = reverse( + "api-v1:organisations:organisation-webhooks-list", + args=[organisation], + ) + response = admin_client.post(url, data={"url": WEBHOOK_URL, "enabled": True}) + assert response.status_code == status.HTTP_201_CREATED + return WEBHOOK_URL + + +@responses.activate def test_update_segment_override__webhook_payload_has_correct_previous_and_new_values( admin_client: APIClient, environment: int, feature: int, segment: int, - mocker: MockerFixture, + environment_webhook: str, ) -> None: """ 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 old_value = 0 new_value = 1 + responses.add(responses.POST, environment_webhook, status=200) + # First create a feature_segment feature_segment_url = reverse("api-v1:features:feature-segment-list") feature_segment_data = { @@ -56,11 +95,8 @@ def test_update_segment_override__webhook_payload_has_correct_previous_and_new_v assert create_response.status_code == status.HTTP_201_CREATED segment_override_id = create_response.json()["id"] - # Mock call_environment_webhooks to capture the actual payload - mock_call_environment_webhooks = mocker.patch( - "features.tasks.call_environment_webhooks" - ) - mocker.patch("features.tasks.call_organisation_webhooks") + # Clear responses from create operation + responses.calls.reset() # When - update the segment override via API 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 # Then assert response.status_code == status.HTTP_200_OK - # Verify webhook was called - mock_call_environment_webhooks.delay.assert_called_once() - webhook_args = mock_call_environment_webhooks.delay.call_args.kwargs["args"] - webhook_payload = webhook_args[1] # (environment_id, data, event_type) + # Verify webhook was called with correct payload + assert len(responses.calls) == 1 + webhook_payload = json.loads(responses.calls[0].request.body)["data"] # type: ignore[union-attr] # Verify the payload has correct values assert webhook_payload["new_state"]["feature_segment"] is not None assert webhook_payload["new_state"]["feature_state_value"] == new_value assert webhook_payload["previous_state"]["feature_state_value"] == old_value + + +@pytest.mark.parametrize( + "webhook", + [lazy_fixture("environment_webhook"), lazy_fixture("organisation_webhook")], +) +@responses.activate +def test_update_multivariate_percentage__webhook_payload_includes_multivariate_values( + admin_client: APIClient, + environment: int, + feature: int, + mv_option_50_percent: int, + mv_option_value: str, + webhook: str, + mocker: MockerFixture, +) -> None: + """ + Test for issue #6190: Webhook payloads do not include multivariate values. + + When updating the percentage allocation of a multivariate option, + the webhook payload should include the multivariate_feature_state_values + with their percentage allocations. + """ + # Given + # Get the feature state for this environment + feature_states_url = reverse("api-v1:features:featurestates-list") + feature_states_response = admin_client.get( + f"{feature_states_url}?environment={environment}&feature={feature}" + ) + assert feature_states_response.status_code == status.HTTP_200_OK + feature_state = feature_states_response.json()["results"][0] + feature_state_id = feature_state["id"] + + # Get current multivariate feature state values + assert len(feature_state["multivariate_feature_state_values"]) == 1 + mv_fs_value = feature_state["multivariate_feature_state_values"][0] + old_percentage = mv_fs_value["percentage_allocation"] + new_percentage = 75 + + responses.add(responses.POST, webhook, status=200) + + # When + # update only the multivariate percentage allocation + url = reverse("api-v1:features:featurestates-detail", args=[feature_state_id]) + data = { + "id": feature_state_id, + "enabled": feature_state["enabled"], + "feature_state_value": feature_state["feature_state_value"], + "environment": environment, + "feature": feature, + "multivariate_feature_state_values": [ + { + "id": mv_fs_value["id"], + "multivariate_feature_option": mv_fs_value[ + "multivariate_feature_option" + ], + "percentage_allocation": new_percentage, + } + ], + } + admin_client.put(url, data=json.dumps(data), content_type="application/json") + + # Then + # `FLAG_UPDATED`` webhook was called + # (should be the last sent event) + last_call = responses.calls[-1] + assert not isinstance(last_call, list) + webhook_payload = json.loads(last_call.request.body) + assert webhook_payload["event_type"] == "FLAG_UPDATED" + + # the payload includes multivariate values + event_data = webhook_payload["data"] + + assert "multivariate_feature_state_values" in event_data["new_state"] + assert "multivariate_feature_state_values" in event_data["previous_state"] + + assert event_data["new_state"]["multivariate_feature_state_values"] == [ + { + "id": mocker.ANY, + "multivariate_feature_option": { + "id": mv_option_50_percent, + "value": mv_option_value, + }, + "percentage_allocation": new_percentage, + }, + ] + assert event_data["previous_state"]["multivariate_feature_state_values"] == [ + { + "id": mocker.ANY, + "multivariate_feature_option": { + "id": mv_option_50_percent, + "value": mv_option_value, + }, + "percentage_allocation": old_percentage, + }, + ] diff --git a/api/webhooks/webhooks.py b/api/webhooks/webhooks.py index 338b96668898..29a1e9e071f2 100644 --- a/api/webhooks/webhooks.py +++ b/api/webhooks/webhooks.py @@ -18,6 +18,10 @@ from core.signing import sign_payload from environments.models import Environment, Webhook from features.models import Feature +from features.multivariate.models import ( + MultivariateFeatureOption, + MultivariateFeatureStateValue, +) from organisations.models import OrganisationWebhook from projects.models import ( # type: ignore[attr-defined] Organisation, @@ -328,6 +332,19 @@ def generate_environment_sample_webhook_data() -> dict[str, Any]: ), ) + mv_option = MultivariateFeatureOption( + id=1, + feature=feature, + default_percentage_allocation=50, + type="unicode", + string_value="variant_a", + ) + mv_state_value = MultivariateFeatureStateValue( + id=1, + multivariate_feature_option=mv_option, + percentage_allocation=50, + ) + data = { "changed_by": "user@domain.com", "timestamp": "2021-06-18T07:50:26.595298Z", @@ -338,6 +355,7 @@ def generate_environment_sample_webhook_data() -> dict[str, Any]: value="feature_state_value", identity_id=1, identity_identifier="test_identity", + multivariate_feature_state_values=[mv_state_value], ), "previous_state": Webhook.generate_webhook_feature_state_data( feature=feature, @@ -346,6 +364,7 @@ def generate_environment_sample_webhook_data() -> dict[str, Any]: value="old_feature_state_value", identity_id=1, identity_identifier="test_identity", + multivariate_feature_state_values=[mv_state_value], ), } diff --git a/docs/docs/administration-and-security/platform-configuration/environment-settings.md b/docs/docs/administration-and-security/platform-configuration/environment-settings.md index fbb4c271bacc..54ef93b93082 100644 --- a/docs/docs/administration-and-security/platform-configuration/environment-settings.md +++ b/docs/docs/administration-and-security/platform-configuration/environment-settings.md @@ -75,7 +75,17 @@ Flagsmith will send a `POST` request to your webhook URL with the following payl "feature_segment": null, "feature_state_value": "\nYou are using the develop environment.\n", "identity": null, - "identity_identifier": null + "identity_identifier": null, + "multivariate_feature_state_values": [ + { + "id": 1, + "multivariate_feature_option": { + "id": 10, + "value": "variant_a" + }, + "percentage_allocation": 50 + } + ] }, "previous_state": { "enabled": false,