diff --git a/api/features/multivariate/serializers.py b/api/features/multivariate/serializers.py index 5b7ade60c7a8..7142aa8f83e8 100644 --- a/api/features/multivariate/serializers.py +++ b/api/features/multivariate/serializers.py @@ -77,16 +77,23 @@ def validate_key(self, value: str | None) -> str | None: raise serializers.ValidationError(RESERVED_VARIANT_KEY_MESSAGE) return value - def validate(self, attrs): # type: ignore[no-untyped-def] +def validate(self, attrs): # type: ignore[no-untyped-def] attrs = super().validate(attrs) + + feature = attrs.get("feature") or getattr(self.instance, "feature", None) + default_percentage_allocation = attrs.get( + "default_percentage_allocation", + getattr(self.instance, "default_percentage_allocation", 0) if self.instance else 100 + ) + 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_percentage_allocation ) if total_percentage_allocation > 100: @@ -99,16 +106,19 @@ def validate(self, attrs): # type: ignore[no-untyped-def] return attrs def _validate_key_is_unique(self, attrs: dict[str, typing.Any]) -> None: + key = attrs.get("key") or getattr(self.instance, "key", None) key = attrs.get("key") if key is None: return - if self._get_siblings(attrs["feature"]).filter(key=key).exists(): + + feature = attrs.get("feature") or getattr(self.instance, "feature", None) + + if self._get_siblings(feature).filter(key=key).exists(): raise ValidationError( { "key": "Multivariate option with this key already exists for the feature." } ) - def _get_siblings(self, feature: Feature): # type: ignore[no-untyped-def] siblings = feature.multivariate_options.all() if self.instance: diff --git a/api/tests/unit/features/multivariate/test_unit_multivariate_views.py b/api/tests/unit/features/multivariate/test_unit_multivariate_views.py index 3fb923d257b3..42609d657421 100644 --- a/api/tests/unit/features/multivariate/test_unit_multivariate_views.py +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_views.py @@ -105,3 +105,25 @@ def test_list_mv_options__feature_in_other_project__returns_404( # Then assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_multivariate_feature_option_create_omitted_allocation_fails_if_siblings_exist( + feature, +): + # Given an existing option taking up 50% + MultivariateFeatureOption.objects.create( + feature=feature, + default_percentage_allocation=50, + type="unicode", + string_value="control", + ) + + # When we try to create a new one without specifying an allocation + # (Serializer defaults to 100, so 50 + 100 = 150 > 100) + serializer = MultivariateFeatureOptionSerializer( + data={"feature": feature.id, "type": "unicode", "string_value": "variant"} + ) + + # Then it should fail validation + assert serializer.is_valid() is False + assert "default_percentage_allocation" in serializer.errors