From 57f8d68449577228a6f32cd317c4693cf5282824 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Mon, 24 Jun 2024 14:36:34 +0100 Subject: [PATCH] feat(versioning): add logic to create version in single endpoint (#3991) --- api/conftest.py | 7 + api/features/feature_segments/serializers.py | 18 +- api/features/versioning/serializers.py | 173 ++++++++ api/features/versioning/views.py | 3 + .../versioning/test_unit_versioning_views.py | 374 +++++++++++++++++- 5 files changed, 566 insertions(+), 9 deletions(-) diff --git a/api/conftest.py b/api/conftest.py index 7a6f1726564a..c468b9c2cd04 100644 --- a/api/conftest.py +++ b/api/conftest.py @@ -468,6 +468,13 @@ def multivariate_feature(project): return feature +@pytest.fixture() +def multivariate_options( + multivariate_feature: Feature, +) -> list[MultivariateFeatureOption]: + return list(multivariate_feature.multivariate_options.all()) + + @pytest.fixture() def identity_matching_segment(project, trait): segment = Segment.objects.create(name="Matching segment", project=project) diff --git a/api/features/feature_segments/serializers.py b/api/features/feature_segments/serializers.py index 519fe62a8c68..ff0b58ced4a8 100644 --- a/api/features/feature_segments/serializers.py +++ b/api/features/feature_segments/serializers.py @@ -1,3 +1,5 @@ +import typing + from rest_framework import serializers from rest_framework.exceptions import PermissionDenied @@ -37,10 +39,24 @@ def validate(self, data): class CreateSegmentOverrideFeatureSegmentSerializer(serializers.ModelSerializer): + # Since the `priority` field on the FeatureSegment model is set to editable=False + # (to adhere to the django-ordered-model functionality), we redefine the priority + # field here, and use it manually in the save method. + priority = serializers.IntegerField(min_value=0, required=False) + class Meta: model = FeatureSegment fields = ("id", "segment", "priority", "uuid") - read_only_fields = ("priority",) + + def save(self, **kwargs: typing.Any) -> FeatureSegment: + priority: int | None = self.initial_data.pop("priority", None) + + feature_segment: FeatureSegment = super().save(**kwargs) + + if priority: + feature_segment.to(priority) + + return feature_segment class FeatureSegmentQuerySerializer(serializers.Serializer): diff --git a/api/features/versioning/serializers.py b/api/features/versioning/serializers.py index 1e5666005f32..9774961b4274 100644 --- a/api/features/versioning/serializers.py +++ b/api/features/versioning/serializers.py @@ -1,3 +1,5 @@ +import typing + from rest_framework import serializers from api_keys.user import APIKeyUser @@ -88,6 +90,177 @@ def get_previous_version_uuid( return str(previous_version.uuid) +class EnvironmentFeatureVersionCreateSerializer(EnvironmentFeatureVersionSerializer): + feature_states_to_create = EnvironmentFeatureVersionFeatureStateSerializer( + many=True, + allow_null=True, + required=False, + help_text=( + "Array of feature states that will be created in the new version. " + "Note: these can only include segment overrides." + ), + write_only=True, + ) + feature_states_to_update = EnvironmentFeatureVersionFeatureStateSerializer( + many=True, + allow_null=True, + required=False, + help_text="Array of feature states to update in the new version.", + write_only=True, + ) + segment_ids_to_delete_overrides = serializers.ListSerializer( + child=serializers.IntegerField(), + required=False, + allow_null=True, + help_text="List of segment ids for which the segment overrides will be removed in the new version.", + write_only=True, + ) + publish_immediately = serializers.BooleanField( + required=False, + default=False, + help_text="Boolean to confirm whether the new version should be publish immediately or not.", + write_only=True, + ) + + class Meta(EnvironmentFeatureVersionSerializer.Meta): + fields = EnvironmentFeatureVersionSerializer.Meta.fields + ( + "feature_states_to_create", + "feature_states_to_update", + "segment_ids_to_delete_overrides", + "publish_immediately", + ) + non_model_fields = ( + "feature_states_to_create", + "feature_states_to_update", + "segment_ids_to_delete_overrides", + "publish_immediately", + ) + + def create( + self, validated_data: dict[str, typing.Any] + ) -> EnvironmentFeatureVersion: + # Note that we use self.initial_data below for handling the feature states + # since we want the raw data (rather than the serialized ORM objects) to pass + # into the serializers in the separate private methods used for modifying the + # FeatureState objects. As such, we just want to blindly remove the non-model + # attribute keys from the validated before calling super to create the version. + for field_name in self.Meta.non_model_fields: + validated_data.pop(field_name, None) + + version = super().create(validated_data) + + for feature_state_to_create in self.initial_data.get( + "feature_states_to_create", [] + ): + self._create_feature_state( + {**feature_state_to_create, "environment": version.environment_id}, + version, + ) + + for feature_state_to_update in self.initial_data.get( + "feature_states_to_update", [] + ): + self._update_feature_state(feature_state_to_update, version) + + self._delete_feature_states( + self.initial_data.get("segment_ids_to_delete_overrides", []), version + ) + + if self.validated_data.get("publish_immediately", False): + request = self.context["request"] + version.publish( + published_by=( + request.user if isinstance(request.user, FFAdminUser) else None + ) + ) + + return version + + def _create_feature_state( + self, feature_state: dict, version: EnvironmentFeatureVersion + ) -> None: + if not self._is_segment_override(feature_state): + raise serializers.ValidationError( + { + "feature_states_to_create": "Cannot create FeatureState objects that are not segment overrides." + } + ) + + segment_id = feature_state["feature_segment"]["segment"] + if version.feature_states.filter( + feature_segment__segment_id=segment_id + ).exists(): + raise serializers.ValidationError( + { + "feature_states_to_create": "Segment override already exists for Segment %d" + % segment_id + } + ) + + save_kwargs = { + "feature": version.feature, + "environment": version.environment, + "environment_feature_version": version, + } + fs_serializer = EnvironmentFeatureVersionFeatureStateSerializer( + data=feature_state, + context=save_kwargs, + ) + fs_serializer.is_valid(raise_exception=True) + fs_serializer.save(**save_kwargs) + + def _update_feature_state( + self, feature_state: dict[str, typing.Any], version: EnvironmentFeatureVersion + ) -> None: + if self._is_segment_override(feature_state): + instance = version.feature_states.get( + feature_segment__segment_id=feature_state["feature_segment"]["segment"] + ) + # Patch the id of the feature segment onto the feature state data so that + # the serializer knows to update rather than try and create a new one. + feature_state["feature_segment"]["id"] = instance.feature_segment_id + else: + instance = version.feature_states.get(feature_segment__isnull=True) + + # TODO: can this be simplified at all? + for existing_mvfsv in instance.multivariate_feature_state_values.all(): + updated_mvfsv_dicts = feature_state.get( + "multivariate_feature_state_values", [] + ) + updated_mvfsv_dict = next( + filter( + lambda d: d["multivariate_feature_option"] + == existing_mvfsv.multivariate_feature_option_id, + updated_mvfsv_dicts, + ), + None, + ) + if updated_mvfsv_dict: + updated_mvfsv_dict["id"] = existing_mvfsv.id + + fs_serializer = EnvironmentFeatureVersionFeatureStateSerializer( + instance=instance, + data=feature_state, + context={ + "feature": version.feature, + "environment": version.environment, + "environment_feature_version": version, + }, + ) + fs_serializer.is_valid(raise_exception=True) + fs_serializer.save( + environment_feature_version=version, environment=version.environment + ) + + def _delete_feature_states( + self, segment_ids: list[int], version: EnvironmentFeatureVersion + ) -> None: + version.feature_segments.filter(segment_id__in=segment_ids).delete() + + def _is_segment_override(self, feature_state: dict) -> bool: + return feature_state.get("feature_segment") is not None + + class EnvironmentFeatureVersionPublishSerializer(serializers.Serializer): live_from = serializers.DateTimeField(required=False) diff --git a/api/features/versioning/views.py b/api/features/versioning/views.py index 14fa4929bbcc..f4d1417e466f 100644 --- a/api/features/versioning/views.py +++ b/api/features/versioning/views.py @@ -29,6 +29,7 @@ EnvironmentFeatureVersionRetrievePermissions, ) from features.versioning.serializers import ( + EnvironmentFeatureVersionCreateSerializer, EnvironmentFeatureVersionFeatureStateSerializer, EnvironmentFeatureVersionPublishSerializer, EnvironmentFeatureVersionQuerySerializer, @@ -66,6 +67,8 @@ def get_serializer_class(self): return EnvironmentFeatureVersionPublishSerializer case "retrieve": return EnvironmentFeatureVersionRetrieveSerializer + case "create": + return EnvironmentFeatureVersionCreateSerializer case _: return EnvironmentFeatureVersionSerializer diff --git a/api/tests/unit/features/versioning/test_unit_versioning_views.py b/api/tests/unit/features/versioning/test_unit_versioning_views.py index 3a68ea9fd5a3..09e42cfc2552 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_views.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_views.py @@ -3,6 +3,7 @@ from datetime import datetime, timedelta import pytest +from core.constants import STRING from django.urls import reverse from django.utils import timezone from freezegun import freeze_time @@ -14,8 +15,12 @@ from audit.models import AuditLog from audit.related_object_type import RelatedObjectType from environments.models import Environment -from environments.permissions.constants import VIEW_ENVIRONMENT +from environments.permissions.constants import ( + UPDATE_FEATURE_STATE, + VIEW_ENVIRONMENT, +) from features.models import Feature, FeatureSegment, FeatureState +from features.multivariate.models import MultivariateFeatureOption from features.versioning.models import EnvironmentFeatureVersion from projects.permissions import VIEW_PROJECT from segments.models import Segment @@ -74,25 +79,30 @@ def test_get_versions_for_a_feature_and_environment( def test_create_new_feature_version( - admin_user: FFAdminUser, - admin_client: APIClient, + staff_user: FFAdminUser, + staff_client: APIClient, environment_v2_versioning: Environment, feature: Feature, + with_environment_permissions: WithEnvironmentPermissionsCallable, + with_project_permissions: WithProjectPermissionsCallable, ) -> None: # Given + with_environment_permissions([VIEW_ENVIRONMENT, UPDATE_FEATURE_STATE]) + with_project_permissions([VIEW_PROJECT]) + url = reverse( "api-v1:versioning:environment-feature-versions-list", args=[environment_v2_versioning.id, feature.id], ) # When - response = admin_client.post(url) + response = staff_client.post(url) # Then assert response.status_code == status.HTTP_201_CREATED response_json = response.json() - assert response_json["created_by"] == admin_user.id + assert response_json["created_by"] == staff_user.id assert response_json["uuid"] @@ -511,7 +521,6 @@ def test_update_environment_feature_version_feature_state( def test_cannot_update_feature_state_in_published_environment_feature_version( admin_client: APIClient, - admin_user: FFAdminUser, environment_v2_versioning: Environment, feature: Feature, ) -> None: @@ -556,7 +565,6 @@ def test_cannot_update_feature_state_in_published_environment_feature_version( def test_delete_environment_feature_version_feature_state( admin_client: APIClient, - admin_user: FFAdminUser, environment_v2_versioning: Environment, segment: Segment, feature: Feature, @@ -647,7 +655,6 @@ def test_cannot_delete_feature_state_in_published_environment_feature_version( def test_cannot_delete_environment_default_feature_state_for_unpublished_environment_feature_version( admin_client: APIClient, - admin_user: FFAdminUser, environment_v2_versioning: Environment, feature: Feature, ) -> None: @@ -766,3 +773,354 @@ def test_disable_v2_versioning_returns_bad_request_if_not_using_v2_versioning( # Then assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_create_new_version_with_changes_in_single_request( + feature: Feature, + segment: Segment, + segment_featurestate: FeatureState, + admin_client_new: APIClient, + environment_v2_versioning: Environment, +) -> None: + # Given + additional_segment_1 = Segment.objects.create( + name="additional-segment-1", project=feature.project + ) + additional_segment_2 = Segment.objects.create( + name="additional-segment-2", project=feature.project + ) + + url = reverse( + "api-v1:versioning:environment-feature-versions-list", + args=[environment_v2_versioning.id, feature.id], + ) + + data = { + "publish_immediately": True, + "feature_states_to_update": [ + { + "feature_segment": None, + "enabled": True, + "feature_state_value": {"type": "unicode", "string_value": "updated!"}, + } + ], + "feature_states_to_create": [ + { + "feature_segment": { + "segment": additional_segment_1.id, + "priority": 2, + }, + "enabled": True, + "feature_state_value": { + "type": "unicode", + "string_value": "segment-override-1", + }, + }, + { + "feature_segment": { + "segment": additional_segment_2.id, + "priority": 1, + }, + "enabled": True, + "feature_state_value": { + "type": "unicode", + "string_value": "segment-override-2", + }, + }, + ], + "segment_ids_to_delete_overrides": [segment.id], + } + + # When + response = admin_client_new.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + + new_version_uuid = response.json()["uuid"] + new_version = EnvironmentFeatureVersion.objects.get(uuid=new_version_uuid) + + assert new_version.feature_states.count() == 3 + + new_version_environment_fs = new_version.feature_states.filter( + feature_segment__isnull=True + ).get() + assert new_version_environment_fs.get_feature_state_value() == "updated!" + assert new_version_environment_fs.enabled is True + + new_version_segment_fs_1 = new_version.feature_states.filter( + feature_segment__segment=additional_segment_1 + ).get() + assert new_version_segment_fs_1.get_feature_state_value() == "segment-override-1" + assert new_version_segment_fs_1.enabled is True + assert new_version_segment_fs_1.feature_segment.priority == 2 + + new_version_segment_fs_2 = new_version.feature_states.filter( + feature_segment__segment=additional_segment_2 + ).get() + assert new_version_segment_fs_2.get_feature_state_value() == "segment-override-2" + assert new_version_segment_fs_2.enabled is True + assert new_version_segment_fs_2.feature_segment.priority == 1 + + assert not new_version.feature_states.filter( + feature_segment__segment=segment + ).exists() + + assert new_version.published is True + assert new_version.is_live is True + + +def test_update_and_create_segment_override_in_single_request( + feature: Feature, + segment: Segment, + segment_featurestate: FeatureState, + admin_client_new: APIClient, + environment_v2_versioning: Environment, +) -> None: + # Given + additional_segment = Segment.objects.create( + name="additional-segment", project=feature.project + ) + + url = reverse( + "api-v1:versioning:environment-feature-versions-list", + args=[environment_v2_versioning.id, feature.id], + ) + + data = { + "publish_immediately": True, + "feature_states_to_update": [ + { + "feature_segment": {"segment": segment.id, "priority": 2}, + "enabled": True, + "feature_state_value": { + "type": "unicode", + "string_value": "updated-segment-override", + }, + } + ], + "feature_states_to_create": [ + { + "feature_segment": { + "segment": additional_segment.id, + "priority": 1, + }, + "enabled": True, + "feature_state_value": { + "type": "unicode", + "string_value": "additional-segment-override", + }, + }, + ], + } + + # When + response = admin_client_new.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + + new_version_uuid = response.json()["uuid"] + new_version = EnvironmentFeatureVersion.objects.get(uuid=new_version_uuid) + + assert new_version.feature_states.count() == 3 + + updated_segment_override = new_version.feature_states.filter( + feature_segment__segment=segment + ).get() + assert ( + updated_segment_override.get_feature_state_value() == "updated-segment-override" + ) + assert updated_segment_override.enabled is True + assert updated_segment_override.feature_segment.priority == 2 + + new_segment_override = new_version.feature_states.filter( + feature_segment__segment=additional_segment + ).get() + assert ( + new_segment_override.get_feature_state_value() == "additional-segment-override" + ) + assert new_segment_override.enabled is True + assert new_segment_override.feature_segment.priority == 1 + + assert new_version.published is True + assert new_version.is_live is True + + +def test_create_environment_default_when_creating_new_version_fails( + environment_v2_versioning: Environment, + feature: Feature, + admin_client_new: APIClient, +) -> None: + # Given + data = { + "feature_states_to_create": [ + { + "feature_segment": None, + "enabled": True, + "feature_state_value": { + "type": "unicode", + "string_value": "some new value", + }, + } + ] + } + + url = reverse( + "api-v1:versioning:environment-feature-versions-list", + args=[environment_v2_versioning.id, feature.id], + ) + + # When + response = admin_client_new.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + + assert response.json() == { + "feature_states_to_create": "Cannot create FeatureState objects that are not segment overrides." + } + + +def test_create_segment_override_for_existing_override_when_creating_new_version_fails( + feature: Feature, + admin_client_new: APIClient, + segment: Segment, + segment_featurestate: FeatureState, + environment_v2_versioning: Environment, +) -> None: + # Given + data = { + "feature_states_to_create": [ + { + "feature_segment": {"segment": segment.id}, + "enabled": True, + "feature_state_value": { + "type": "unicode", + "string_value": "some new value", + }, + } + ] + } + + url = reverse( + "api-v1:versioning:environment-feature-versions-list", + args=[environment_v2_versioning.id, feature.id], + ) + + # When + response = admin_client_new.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json() == { + "feature_states_to_create": "Segment override already exists for Segment %d" + % segment.id + } + + +def test_create_new_version_for_multivariate_feature( + multivariate_feature: Feature, + multivariate_options: list[MultivariateFeatureOption], + environment_v2_versioning: Environment, + staff_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, + with_project_permissions: WithProjectPermissionsCallable, +) -> None: + # Given + with_environment_permissions( + [VIEW_ENVIRONMENT, UPDATE_FEATURE_STATE], environment_v2_versioning.id + ) + with_project_permissions([VIEW_PROJECT]) + + create_version_url = reverse( + "api-v1:versioning:environment-feature-versions-list", + args=[environment_v2_versioning.id, multivariate_feature.id], + ) + + data = { + "feature_states_to_update": [ + { + "feature_segment": None, + "enabled": True, + "feature_state_value": { + "type": STRING, + "string_value": multivariate_feature.initial_value, + }, + "multivariate_feature_state_values": [ + { + "multivariate_feature_option": mvfo.id, + "percentage_allocation": 10, + } + for mvfo in multivariate_options + ], + } + ] + } + + # When + response = staff_client.post( + create_version_url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + + version_uuid = response.json()["uuid"] + new_version = EnvironmentFeatureVersion.objects.get(uuid=version_uuid) + + assert all( + [ + mvfsv.percentage_allocation == 10 + for mvfsv in new_version.feature_states.get( + feature=multivariate_feature + ).multivariate_feature_state_values.all() + ] + ) + + +def test_create_new_version_delete_segment_override_updates_overrides_immediately( + feature: Feature, + segment: Segment, + feature_segment: FeatureSegment, + segment_featurestate: FeatureState, + environment_v2_versioning: Environment, + admin_client: APIClient, +) -> None: + # Given + create_version_url = reverse( + "api-v1:versioning:environment-feature-versions-list", + args=[environment_v2_versioning.id, feature.id], + ) + + data = { + "segment_ids_to_delete_overrides": [segment.id], + "publish_immediately": True, + } + + # When + response = admin_client.post( + create_version_url, + data=json.dumps(data), + content_type="application/json", + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + + get_feature_segments_url = "%s?environment=%d&feature=%d" % ( + reverse("api-v1:features:feature-segment-list"), + environment_v2_versioning.id, + feature.id, + ) + get_feature_segments_response = admin_client.get(get_feature_segments_url) + assert get_feature_segments_response.status_code == status.HTTP_200_OK + assert get_feature_segments_response.json()["count"] == 0