diff --git a/api/features/multivariate/serializers.py b/api/features/multivariate/serializers.py index 6c04cb5ac68b..399d48ece8e6 100644 --- a/api/features/multivariate/serializers.py +++ b/api/features/multivariate/serializers.py @@ -55,17 +55,54 @@ def get_control_value(self, obj: dict[str, typing.Any]) -> str | int | bool | No class MultivariateFeatureOptionSerializer(NestedMultivariateFeatureOptionSerializer): class Meta(NestedMultivariateFeatureOptionSerializer.Meta): fields = NestedMultivariateFeatureOptionSerializer.Meta.fields + ("feature",) # type: ignore[assignment] + extra_kwargs = { + "feature": {"required": False}, + } def validate(self, attrs): # type: ignore[no-untyped-def] attrs = super().validate(attrs) + + # For partial updates or nested routes, use existing instance or URL value as fallback + feature = attrs.get("feature") + if feature is None and self.instance is not None: + feature = self.instance.feature + + if feature is None: + view = self.context.get("view") + feature_pk = getattr(view, "kwargs", {}).get("feature_pk") if view else None + project_pk = getattr(view, "kwargs", {}).get("project_pk") if view else None + if feature_pk is not None: + qs = Feature.objects.filter(pk=feature_pk) + if project_pk is not None: + qs = qs.filter(project__id=project_pk) + feature = qs.first() + + if feature is None: + raise serializers.ValidationError({"feature": "Feature is required"}) + + # Ensure feature is always present in validated data (e.g. for creates via nested routes) + attrs["feature"] = feature + + # Handle legacy records where default_percentage_allocation may be NULL in the database. + if self.instance and self.instance.default_percentage_allocation is not None: + instance_default_allocation = self.instance.default_percentage_allocation + else: + instance_default_allocation = 100 + + default_allocation = attrs.get( + "default_percentage_allocation", + instance_default_allocation, + ) + attrs["default_percentage_allocation"] = default_allocation + total_sibling_percentage_allocation = ( - self._get_siblings(attrs["feature"]).aggregate( + self._get_siblings(feature).aggregate( total_percentage_allocation=Sum("default_percentage_allocation") )["total_percentage_allocation"] or 0 ) total_percentage_allocation = ( - total_sibling_percentage_allocation + attrs["default_percentage_allocation"] + total_sibling_percentage_allocation + default_allocation ) if total_percentage_allocation > 100: diff --git a/api/tests/integration/features/multivariate/test_integration_multivariate.py b/api/tests/integration/features/multivariate/test_integration_multivariate.py index 49effb4c8ff9..864370233658 100644 --- a/api/tests/integration/features/multivariate/test_integration_multivariate.py +++ b/api/tests/integration/features/multivariate/test_integration_multivariate.py @@ -7,6 +7,8 @@ from rest_framework.test import APIClient from features.models import Feature +from features.multivariate.models import MultivariateFeatureOption +from features.multivariate.serializers import MultivariateFeatureOptionSerializer from organisations.models import Organisation from projects.models import Project from users.models import FFAdminUser @@ -40,6 +42,181 @@ def test_can_create_mv_option(client, project, mv_option_50_percent, feature): assert set(data.items()).issubset(set(response.json().items())) +@pytest.mark.parametrize( + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], +) +def test_create_mv_option__without_feature_in_payload__uses_feature_from_url( + client: APIClient, + project: int, + feature: int, +) -> None: + # Given + url = reverse( + "api-v1:projects:feature-mv-options-list", + args=[project, feature], + ) + data = { + "type": "unicode", + "string_value": "bigger", + "default_percentage_allocation": 50, + } + # When + response = client.post( + url, + data=json.dumps(data), + content_type="application/json", + ) + # Then + assert response.status_code == status.HTTP_201_CREATED + body = response.json() + assert body["id"] + assert body["feature"] == feature + assert body["string_value"] == "bigger" + assert body["default_percentage_allocation"] == 50 + + +@pytest.mark.parametrize( + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], +) +def test_create_mv_option__without_default_percentage_allocation__uses_default( + client: APIClient, + project: int, + feature: int, +) -> None: + # Given + url = reverse( + "api-v1:projects:feature-mv-options-list", + args=[project, feature], + ) + data = { + "type": "unicode", + "feature": feature, + "string_value": "test_value", + } + + # When + response = client.post( + url, + data=json.dumps(data), + content_type="application/json", + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + assert response.json()["id"] + assert response.json()["default_percentage_allocation"] == 100 + + +@pytest.mark.parametrize( + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], +) +def test_create_mv_option__without_default_percentage_allocation_with_existing_sibling_and_total_gt_100__returns_400( # noqa: E501 + client: APIClient, + project: int, + feature: int, + mv_option_50_percent: int, +) -> None: + # Given + url = reverse( + "api-v1:projects:feature-mv-options-list", + args=[project, feature], + ) + data = { + "type": "unicode", + "feature": feature, + "string_value": "another_value", + } + + # When + response = client.post( + url, + data=json.dumps(data), + content_type="application/json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json()["default_percentage_allocation"] == [ + "Invalid percentage allocation" + ] + + +@pytest.mark.parametrize( + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], +) +def test_partial_update_mv_option__without_feature_and_allocation__uses_existing_values( + client: APIClient, + project: int, + feature: int, + mv_option_50_percent: int, +) -> None: + # Given + url = reverse( + "api-v1:projects:feature-mv-options-detail", + args=[project, feature, mv_option_50_percent], + ) + data = { + "string_value": "updated_value", + } + + # When + response = client.patch( + url, + data=json.dumps(data), + content_type="application/json", + ) + + # Then + assert response.status_code == status.HTTP_200_OK + assert response.json()["string_value"] == "updated_value" + assert response.json()["default_percentage_allocation"] == 50 + assert response.json()["feature"] == feature + + +@pytest.mark.parametrize( + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], +) +def test_partial_update_mv_option__handles_legacy_null_default_percentage_allocation( # noqa: E501 + client: APIClient, + project: int, + feature: int, + mv_option_50_percent: int, +) -> None: + # Given + # Simulate a legacy instance in memory where default_percentage_allocation is NULL. + mv_option = MultivariateFeatureOption.objects.get(id=mv_option_50_percent) + mv_option.default_percentage_allocation = None + + serializer = MultivariateFeatureOptionSerializer( + instance=mv_option, + data={"string_value": "updated_legacy_value"}, + partial=True, + context={ + "view": type( + "View", + (), + { + "kwargs": { + "project_pk": project, + "feature_pk": feature, + }, + }, + )(), + }, + ) + assert serializer.is_valid(), serializer.errors + instance = serializer.save() + + assert instance.string_value == "updated_legacy_value" + assert instance.default_percentage_allocation == 100 + assert instance.feature_id == feature + + @pytest.mark.parametrize( "client, feature_id", [ @@ -183,6 +360,56 @@ def test_can_update_default_percentage_allocation( # type: ignore[no-untyped-de assert set(data.items()).issubset(set(response.json().items())) +@pytest.mark.parametrize( + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], +) +def test_partial_update_default_percentage_allocation__sibling_total_gt_100__returns_400( # noqa: E501 + project: int, + mv_option_50_percent: int, + client: APIClient, + feature: int, +) -> None: + # First let's create another mv_option with 30 percent allocation + url = reverse( + "api-v1:projects:feature-mv-options-list", + args=[project, feature], + ) + data = { + "type": "unicode", + "feature": feature, + "string_value": "bigger", + "default_percentage_allocation": 30, + } + response = client.post( + url, + data=json.dumps(data), + content_type="application/json", + ) + assert response.status_code == status.HTTP_201_CREATED + mv_option_30_percent = response.json()["id"] + + # Next, let's partially update the 30 percent mv option to 51 percent + url = reverse( + "api-v1:projects:feature-mv-options-detail", + args=[project, feature, mv_option_30_percent], + ) + data = { + "default_percentage_allocation": 51, + } + # When + response = client.patch( + url, + data=json.dumps(data), + content_type="application/json", + ) + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json()["default_percentage_allocation"] == [ + "Invalid percentage allocation" + ] + + @pytest.mark.parametrize( "client", [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")],