From ad13fa63c93cacba63a856d02dc85b85bf113d86 Mon Sep 17 00:00:00 2001 From: Srijan Tripathi Date: Sat, 15 Aug 2026 16:42:36 +0000 Subject: [PATCH 1/6] fix: prevent 500 error on partial update of multivariate options --- api/features/multivariate/serializers.py | 18 ++++++++--- .../test_unit_multivariate_views.py | 30 +++++++++++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/api/features/multivariate/serializers.py b/api/features/multivariate/serializers.py index 5b7ade60c7a8..94644e0cad31 100644 --- a/api/features/multivariate/serializers.py +++ b/api/features/multivariate/serializers.py @@ -79,14 +79,23 @@ def validate_key(self, value: str | None) -> str | None: def validate(self, attrs): # type: ignore[no-untyped-def] attrs = super().validate(attrs) + + # Safely get feature and allocation, falling back to instance for PATCH requests + feature = attrs.get("feature", getattr(self.instance, "feature", None)) + default_percentage_allocation = attrs.get( + "default_percentage_allocation", + getattr(self.instance, "default_percentage_allocation", 0) + ) + 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: @@ -97,12 +106,13 @@ def validate(self, attrs): # type: ignore[no-untyped-def] self._validate_key_is_unique(attrs) return attrs - + def _validate_key_is_unique(self, attrs: dict[str, typing.Any]) -> None: key = attrs.get("key") if key is None: return - if self._get_siblings(attrs["feature"]).filter(key=key).exists(): + feature = attrs.get("feature",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." 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..18a7ab82b67b 100644 --- a/api/tests/unit/features/multivariate/test_unit_multivariate_views.py +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_views.py @@ -1,4 +1,5 @@ import uuid +import json import pytest from common.projects.permissions import ( @@ -105,3 +106,32 @@ def test_list_mv_options__feature_in_other_project__returns_404( # Then assert response.status_code == status.HTTP_404_NOT_FOUND + +def test_partially_updating_multivariate_option_success( + admin_client: APIClient, + project: Project, + multivariate_feature: Feature, +) -> None: + # Given + mv_option = multivariate_feature.multivariate_options.first() + url = reverse( + "api-v1:projects:feature-mv-options-detail", + args=[project.id, multivariate_feature.id, mv_option.id], + ) + + new_key = "hero" + data = {"key": new_key} + + initial_allocation = mv_option.default_percentage_allocation + + # When + response = admin_client.patch( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_200_OK + + mv_option.refresh_from_db() + assert mv_option.key == new_key + assert mv_option.default_percentage_allocation == initial_allocation \ No newline at end of file From 6c97df25996444c3c092fbcf389c3e1a257be0cc Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 15 Aug 2026 16:48:51 +0000 Subject: [PATCH 2/6] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- api/features/multivariate/serializers.py | 10 +++++----- .../multivariate/test_unit_multivariate_views.py | 13 +++++++------ 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/api/features/multivariate/serializers.py b/api/features/multivariate/serializers.py index 67d42baa58d8..ec0d01009ace 100644 --- a/api/features/multivariate/serializers.py +++ b/api/features/multivariate/serializers.py @@ -82,12 +82,12 @@ def validate_key(self, value: str | None) -> str | None: def validate(self, attrs): # type: ignore[no-untyped-def] attrs = super().validate(attrs) - + # Safely get feature and allocation, falling back to instance for PATCH requests feature = attrs.get("feature", getattr(self.instance, "feature", None)) default_percentage_allocation = attrs.get( - "default_percentage_allocation", - getattr(self.instance, "default_percentage_allocation", 0) + "default_percentage_allocation", + getattr(self.instance, "default_percentage_allocation", 0), ) total_sibling_percentage_allocation = ( @@ -96,7 +96,7 @@ def validate(self, attrs): # type: ignore[no-untyped-def] )["total_percentage_allocation"] or 0 ) - + total_percentage_allocation = ( total_sibling_percentage_allocation + default_percentage_allocation ) @@ -145,7 +145,7 @@ def _validate_key_is_unique(self, attrs: dict[str, typing.Any]) -> None: key = attrs.get("key") if key is None: return - feature = attrs.get("feature",getattr(self.instance, "feature",None)) + feature = attrs.get("feature", getattr(self.instance, "feature", None)) if self._get_siblings(feature).filter(key=key).exists(): raise ValidationError( { 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 18a7ab82b67b..e211a2d8c780 100644 --- a/api/tests/unit/features/multivariate/test_unit_multivariate_views.py +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_views.py @@ -1,5 +1,5 @@ -import uuid import json +import uuid import pytest from common.projects.permissions import ( @@ -106,7 +106,8 @@ def test_list_mv_options__feature_in_other_project__returns_404( # Then assert response.status_code == status.HTTP_404_NOT_FOUND - + + def test_partially_updating_multivariate_option_success( admin_client: APIClient, project: Project, @@ -118,10 +119,10 @@ def test_partially_updating_multivariate_option_success( "api-v1:projects:feature-mv-options-detail", args=[project.id, multivariate_feature.id, mv_option.id], ) - + new_key = "hero" data = {"key": new_key} - + initial_allocation = mv_option.default_percentage_allocation # When @@ -131,7 +132,7 @@ def test_partially_updating_multivariate_option_success( # Then assert response.status_code == status.HTTP_200_OK - + mv_option.refresh_from_db() assert mv_option.key == new_key - assert mv_option.default_percentage_allocation == initial_allocation \ No newline at end of file + assert mv_option.default_percentage_allocation == initial_allocation From 461a720c5baa698f3a57970926d339123d3f0d4c Mon Sep 17 00:00:00 2001 From: Srijan Tripathi Date: Sat, 15 Aug 2026 16:56:18 +0000 Subject: [PATCH 3/6] Fix mypy Testing errors and Key Extraction --- api/features/multivariate/serializers.py | 3 ++- .../multivariate/test_unit_multivariate_views.py | 15 +++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/api/features/multivariate/serializers.py b/api/features/multivariate/serializers.py index ec0d01009ace..c044396a9914 100644 --- a/api/features/multivariate/serializers.py +++ b/api/features/multivariate/serializers.py @@ -142,9 +142,10 @@ def _validate_environment_allocations( ) def _validate_key_is_unique(self, attrs: dict[str, typing.Any]) -> None: - key = attrs.get("key") + key = attrs.get("key", getattr(self.instance, "key", None)) if key is None: return + feature = attrs.get("feature", getattr(self.instance, "feature", None)) if self._get_siblings(feature).filter(key=key).exists(): raise ValidationError( 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 e211a2d8c780..dfedfd95c9f4 100644 --- a/api/tests/unit/features/multivariate/test_unit_multivariate_views.py +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_views.py @@ -1,5 +1,5 @@ -import json import uuid +import json import pytest from common.projects.permissions import ( @@ -106,8 +106,7 @@ def test_list_mv_options__feature_in_other_project__returns_404( # Then assert response.status_code == status.HTTP_404_NOT_FOUND - - + def test_partially_updating_multivariate_option_success( admin_client: APIClient, project: Project, @@ -115,14 +114,17 @@ def test_partially_updating_multivariate_option_success( ) -> None: # Given mv_option = multivariate_feature.multivariate_options.first() + + assert mv_option is not None + url = reverse( "api-v1:projects:feature-mv-options-detail", args=[project.id, multivariate_feature.id, mv_option.id], ) - + new_key = "hero" data = {"key": new_key} - + initial_allocation = mv_option.default_percentage_allocation # When @@ -132,7 +134,8 @@ def test_partially_updating_multivariate_option_success( # Then assert response.status_code == status.HTTP_200_OK - + mv_option.refresh_from_db() assert mv_option.key == new_key assert mv_option.default_percentage_allocation == initial_allocation + From eb5081f190461c4777ab3d8c80184abef77c41ec Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 15 Aug 2026 16:56:33 +0000 Subject: [PATCH 4/6] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- api/features/multivariate/serializers.py | 2 +- .../multivariate/test_unit_multivariate_views.py | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/api/features/multivariate/serializers.py b/api/features/multivariate/serializers.py index c044396a9914..13f2560d4b09 100644 --- a/api/features/multivariate/serializers.py +++ b/api/features/multivariate/serializers.py @@ -145,7 +145,7 @@ def _validate_key_is_unique(self, attrs: dict[str, typing.Any]) -> None: key = attrs.get("key", getattr(self.instance, "key", None)) if key is None: return - + feature = attrs.get("feature", getattr(self.instance, "feature", None)) if self._get_siblings(feature).filter(key=key).exists(): raise ValidationError( 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 dfedfd95c9f4..44e034d02d6b 100644 --- a/api/tests/unit/features/multivariate/test_unit_multivariate_views.py +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_views.py @@ -1,5 +1,5 @@ -import uuid import json +import uuid import pytest from common.projects.permissions import ( @@ -106,7 +106,8 @@ def test_list_mv_options__feature_in_other_project__returns_404( # Then assert response.status_code == status.HTTP_404_NOT_FOUND - + + def test_partially_updating_multivariate_option_success( admin_client: APIClient, project: Project, @@ -114,17 +115,17 @@ def test_partially_updating_multivariate_option_success( ) -> None: # Given mv_option = multivariate_feature.multivariate_options.first() - + assert mv_option is not None - + url = reverse( "api-v1:projects:feature-mv-options-detail", args=[project.id, multivariate_feature.id, mv_option.id], ) - + new_key = "hero" data = {"key": new_key} - + initial_allocation = mv_option.default_percentage_allocation # When @@ -134,8 +135,7 @@ def test_partially_updating_multivariate_option_success( # Then assert response.status_code == status.HTTP_200_OK - + mv_option.refresh_from_db() assert mv_option.key == new_key assert mv_option.default_percentage_allocation == initial_allocation - From 34efb5b0c672abe1a5417804ff65a9af0944502c Mon Sep 17 00:00:00 2001 From: Srijan Tripathi Date: Sun, 16 Aug 2026 12:45:09 +0000 Subject: [PATCH 5/6] style: rename multivariate test to match linting conventions --- .../unit/features/multivariate/test_unit_multivariate_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 44e034d02d6b..366bcf68eee5 100644 --- a/api/tests/unit/features/multivariate/test_unit_multivariate_views.py +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_views.py @@ -108,7 +108,7 @@ def test_list_mv_options__feature_in_other_project__returns_404( assert response.status_code == status.HTTP_404_NOT_FOUND -def test_partially_updating_multivariate_option_success( +def test_partial_update_multivariate_option__valid_data__returns_200_and_updates( admin_client: APIClient, project: Project, multivariate_feature: Feature, From b3b8004b3f058af060144beae294aaf73c2e1f75 Mon Sep 17 00:00:00 2001 From: Srijan Tripathi Date: Sun, 16 Aug 2026 14:48:00 +0000 Subject: [PATCH 6/6] test: use compiled url string for mv option patch test --- .../features/multivariate/test_unit_multivariate_views.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) 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 366bcf68eee5..1c4a0ad629f9 100644 --- a/api/tests/unit/features/multivariate/test_unit_multivariate_views.py +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_views.py @@ -118,10 +118,7 @@ def test_partial_update_multivariate_option__valid_data__returns_200_and_updates assert mv_option is not None - url = reverse( - "api-v1:projects:feature-mv-options-detail", - args=[project.id, multivariate_feature.id, mv_option.id], - ) + url = f"/api/v1/projects/{project.id}/features/{multivariate_feature.id}/mv-options/{mv_option.id}/" new_key = "hero" data = {"key": new_key}