From 05c82fd73c66ad5ccda5810721f58ac1848cd8dc Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 25 May 2026 11:10:30 +0200 Subject: [PATCH 01/11] feat(experimentation): add Experiment model and CRUD endpoints --- api/audit/related_object_type.py | 1 + api/environments/urls.py | 4 + api/experimentation/constants.py | 1 + api/experimentation/experiment_urls.py | 10 + .../migrations/0004_experiment.py | 94 ++++ api/experimentation/models.py | 49 ++ api/experimentation/permissions.py | 21 +- api/experimentation/serializers.py | 81 +++ api/experimentation/services.py | 31 +- api/experimentation/views.py | 86 ++- api/tests/unit/experimentation/conftest.py | 22 +- .../experimentation/test_experiment_views.py | 532 ++++++++++++++++++ 12 files changed, 924 insertions(+), 8 deletions(-) create mode 100644 api/experimentation/experiment_urls.py create mode 100644 api/experimentation/migrations/0004_experiment.py create mode 100644 api/tests/unit/experimentation/test_experiment_views.py diff --git a/api/audit/related_object_type.py b/api/audit/related_object_type.py index d43c849e574b..853011eb38f4 100644 --- a/api/audit/related_object_type.py +++ b/api/audit/related_object_type.py @@ -13,3 +13,4 @@ class RelatedObjectType(enum.Enum): FEATURE_HEALTH = "Feature health status" RELEASE_PIPELINE = "Release pipeline" WAREHOUSE_CONNECTION = "Warehouse connection" + EXPERIMENT = "Experiment" diff --git a/api/environments/urls.py b/api/environments/urls.py index 2642af9242a5..348594936a45 100644 --- a/api/environments/urls.py +++ b/api/environments/urls.py @@ -177,4 +177,8 @@ "/warehouse-connections/", include("experimentation.urls"), ), + path( + "/experiments/", + include("experimentation.experiment_urls"), + ), ] diff --git a/api/experimentation/constants.py b/api/experimentation/constants.py index a1e79a0fca8e..94f9870f881d 100644 --- a/api/experimentation/constants.py +++ b/api/experimentation/constants.py @@ -1 +1,2 @@ WAREHOUSE_CONNECTION_FLAG = "experimentation_warehouse_connection" +EXPERIMENT_FLAG = "experimental_flags" diff --git a/api/experimentation/experiment_urls.py b/api/experimentation/experiment_urls.py new file mode 100644 index 000000000000..0022909c55dd --- /dev/null +++ b/api/experimentation/experiment_urls.py @@ -0,0 +1,10 @@ +from rest_framework.routers import DefaultRouter + +from experimentation.views import ExperimentViewSet + +app_name = "experiments" + +router = DefaultRouter() +router.register(r"", ExperimentViewSet, basename="experiments") + +urlpatterns = router.urls diff --git a/api/experimentation/migrations/0004_experiment.py b/api/experimentation/migrations/0004_experiment.py new file mode 100644 index 000000000000..89924f708255 --- /dev/null +++ b/api/experimentation/migrations/0004_experiment.py @@ -0,0 +1,94 @@ +# Generated by Django 5.2.14 on 2026-05-25 08:45 + +import django.db.models.deletion +import django_lifecycle.mixins +import uuid +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("environments", "0037_add_uuid_field"), + ("experimentation", "0003_unique_connection_per_environment"), + ("features", "0066_constrain_feature_type"), + ] + + operations = [ + migrations.CreateModel( + name="Experiment", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "deleted_at", + models.DateTimeField( + blank=True, + db_index=True, + default=None, + editable=False, + null=True, + ), + ), + ( + "uuid", + models.UUIDField(default=uuid.uuid4, editable=False, unique=True), + ), + ("name", models.CharField(max_length=255)), + ("hypothesis", models.TextField()), + ( + "status", + models.CharField( + choices=[ + ("created", "Created"), + ("running", "Running"), + ("paused", "Paused"), + ("completed", "Completed"), + ], + default="created", + max_length=50, + ), + ), + ("created_at", models.DateTimeField(auto_now_add=True)), + ("updated_at", models.DateTimeField(auto_now=True)), + ("started_at", models.DateTimeField(blank=True, null=True)), + ("ended_at", models.DateTimeField(blank=True, null=True)), + ( + "environment", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="experiments", + to="environments.environment", + ), + ), + ( + "feature", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="experiments", + to="features.feature", + ), + ), + ], + options={ + "constraints": [ + models.UniqueConstraint( + condition=models.Q( + ("deleted_at__isnull", True), + models.Q(("status", "completed"), _negated=True), + ), + fields=("feature", "environment"), + name="unique_active_experiment_per_feature_env", + ) + ], + }, + bases=(django_lifecycle.mixins.LifecycleModelMixin, models.Model), + ), + ] diff --git a/api/experimentation/models.py b/api/experimentation/models.py index fd3fbf513782..4dd2dcec0ac8 100644 --- a/api/experimentation/models.py +++ b/api/experimentation/models.py @@ -1,4 +1,5 @@ from django.db import models +from django.db.models import Q from django_lifecycle import LifecycleModelMixin # type: ignore[import-untyped] from core.models import SoftDeleteExportableModel @@ -47,3 +48,51 @@ class Meta: name="unique_active_warehouse_per_env", ), ] + + +class ExperimentStatus(models.TextChoices): + CREATED = "created", "Created" + RUNNING = "running", "Running" + PAUSED = "paused", "Paused" + COMPLETED = "completed", "Completed" + + +VALID_STATUS_TRANSITIONS: dict[str, set[str]] = { + ExperimentStatus.CREATED: {ExperimentStatus.RUNNING}, + ExperimentStatus.RUNNING: {ExperimentStatus.PAUSED, ExperimentStatus.COMPLETED}, + ExperimentStatus.PAUSED: {ExperimentStatus.RUNNING, ExperimentStatus.COMPLETED}, + ExperimentStatus.COMPLETED: set(), +} + + +class Experiment(LifecycleModelMixin, SoftDeleteExportableModel): # type: ignore[misc] + environment = models.ForeignKey( + Environment, + on_delete=models.CASCADE, + related_name="experiments", + ) + feature = models.ForeignKey( + "features.Feature", + on_delete=models.CASCADE, + related_name="experiments", + ) + name = models.CharField(max_length=255) + hypothesis = models.TextField() + status = models.CharField( + max_length=50, + choices=ExperimentStatus.choices, + default=ExperimentStatus.CREATED, + ) + created_at = models.DateTimeField(auto_now_add=True) + updated_at = models.DateTimeField(auto_now=True) + started_at = models.DateTimeField(null=True, blank=True) + ended_at = models.DateTimeField(null=True, blank=True) + + class Meta: + constraints = [ + models.UniqueConstraint( + fields=["feature", "environment"], + condition=Q(deleted_at__isnull=True) & ~Q(status="completed"), + name="unique_active_experiment_per_feature_env", + ), + ] diff --git a/api/experimentation/permissions.py b/api/experimentation/permissions.py index 83e1463d1a5c..e1fdc8035101 100644 --- a/api/experimentation/permissions.py +++ b/api/experimentation/permissions.py @@ -3,7 +3,10 @@ from rest_framework.views import APIView from environments.models import Environment -from experimentation.services import is_warehouse_feature_enabled +from experimentation.services import ( + is_experiment_feature_enabled, + is_warehouse_feature_enabled, +) from users.models import FFAdminUser @@ -21,3 +24,19 @@ def has_permission(self, request: Request, view: APIView) -> bool: user: FFAdminUser = request.user # type: ignore[assignment] return user.is_environment_admin(environment) + + +class ExperimentPermission(BasePermission): + def has_permission(self, request: Request, view: APIView) -> bool: + try: + environment = Environment.objects.get( + api_key=view.kwargs.get("environment_api_key") + ) + except Environment.DoesNotExist: + return False + + if not is_experiment_feature_enabled(environment.project.organisation): + return False + + user: FFAdminUser = request.user # type: ignore[assignment] + return user.is_environment_admin(environment) diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index bfece9cc5242..bd93b3591717 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -1,14 +1,19 @@ from typing import Any +from django.utils import timezone from rest_framework import serializers from environments.models import Environment from experimentation.models import ( + VALID_STATUS_TRANSITIONS, + Experiment, + ExperimentStatus, WarehouseConnection, WarehouseConnectionStatus, WarehouseType, ) from experimentation.types import SNOWFLAKE_DEFAULTS, SnowflakeConfig +from features.feature_types import MULTIVARIATE class WarehouseConnectionSerializer(serializers.ModelSerializer): # type: ignore[type-arg] @@ -88,3 +93,79 @@ def _validate_snowflake_config(config: dict[str, Any]) -> SnowflakeConfig: **config, # type: ignore[typeddict-item] } return merged + + +class ExperimentSerializer(serializers.ModelSerializer): # type: ignore[type-arg] + class Meta: + model = Experiment + fields = ( + "id", + "feature", + "name", + "hypothesis", + "status", + "created_at", + "updated_at", + "started_at", + "ended_at", + ) + read_only_fields = ( + "id", + "created_at", + "updated_at", + "started_at", + "ended_at", + ) + + def validate_feature(self, feature: Any) -> Any: + if feature.type != MULTIVARIATE: + raise serializers.ValidationError( + "Experiments can only be created for multivariate flags." + ) + view = self.context.get("view") + if view: + environment: Environment = view._get_environment() + if feature.project_id != environment.project_id: + raise serializers.ValidationError( + "Feature does not belong to this environment's project." + ) + return feature + + def validate_status(self, status: str) -> str: + if self.instance is None: + if status != ExperimentStatus.CREATED: + raise serializers.ValidationError( + "Status must be 'created' when creating an experiment." + ) + return status + + current_status: str = self.instance.status # type: ignore[union-attr] + if status == current_status: + return status + + valid_targets = VALID_STATUS_TRANSITIONS.get(current_status, set()) + if status not in valid_targets: + raise serializers.ValidationError( + f"Cannot transition from '{current_status}' to '{status}'." + ) + return status + + def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: + if self.instance is not None and "feature" in attrs: + raise serializers.ValidationError( + {"feature": "Cannot change the feature of an existing experiment."} + ) + return attrs + + def update( + self, + instance: Experiment, + validated_data: dict[str, Any], + ) -> Experiment: + new_status = validated_data.get("status") + if new_status == ExperimentStatus.RUNNING and instance.started_at is None: + validated_data["started_at"] = timezone.now() + elif new_status == ExperimentStatus.COMPLETED: + validated_data["ended_at"] = timezone.now() + result: Experiment = super().update(instance, validated_data) + return result diff --git a/api/experimentation/services.py b/api/experimentation/services.py index fc44b501db1e..562361b38f0e 100644 --- a/api/experimentation/services.py +++ b/api/experimentation/services.py @@ -4,11 +4,11 @@ from audit.models import AuditLog from audit.related_object_type import RelatedObjectType -from experimentation.constants import WAREHOUSE_CONNECTION_FLAG +from experimentation.constants import EXPERIMENT_FLAG, WAREHOUSE_CONNECTION_FLAG from integrations.flagsmith.client import get_openfeature_client if typing.TYPE_CHECKING: - from experimentation.models import WarehouseConnection + from experimentation.models import Experiment, WarehouseConnection from organisations.models import Organisation from users.models import FFAdminUser @@ -21,6 +21,14 @@ def is_warehouse_feature_enabled(organisation: Organisation) -> bool: ) +def is_experiment_feature_enabled(organisation: Organisation) -> bool: + return get_openfeature_client().get_boolean_value( + EXPERIMENT_FLAG, + default_value=False, + evaluation_context=organisation.openfeature_evaluation_context, + ) + + def create_warehouse_audit_log( connection: WarehouseConnection, user: FFAdminUser, @@ -38,3 +46,22 @@ def create_warehouse_audit_log( f"{connection.environment.name}" ), ) + + +def create_experiment_audit_log( + experiment: Experiment, + user: FFAdminUser, + *, + action: str, +) -> None: + AuditLog.objects.create( + environment=experiment.environment, + project=experiment.environment.project, + author=user, + related_object_id=experiment.id, + related_object_type=RelatedObjectType.EXPERIMENT.name, + log=( + f"Experiment '{experiment.name}' {action} for environment " + f"{experiment.environment.name}" + ), + ) diff --git a/api/experimentation/views.py b/api/experimentation/views.py index 5f2968c25b02..a9b902ceb1c1 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -1,3 +1,4 @@ +from django.db.models import QuerySet from rest_framework import mixins from rest_framework.permissions import IsAuthenticated from rest_framework.request import Request @@ -5,10 +6,19 @@ from rest_framework.serializers import BaseSerializer from environments.views import NestedEnvironmentViewSet -from experimentation.models import WarehouseConnection -from experimentation.permissions import WarehouseConnectionPermission -from experimentation.serializers import WarehouseConnectionSerializer -from experimentation.services import create_warehouse_audit_log +from experimentation.models import Experiment, ExperimentStatus, WarehouseConnection +from experimentation.permissions import ( + ExperimentPermission, + WarehouseConnectionPermission, +) +from experimentation.serializers import ( + ExperimentSerializer, + WarehouseConnectionSerializer, +) +from experimentation.services import ( + create_experiment_audit_log, + create_warehouse_audit_log, +) from users.models import FFAdminUser @@ -68,3 +78,71 @@ def create(self, request: Request, *args: object, **kwargs: object) -> Response: @staticmethod def _get_user(request: Request) -> FFAdminUser: return request.user # type: ignore[return-value] + + +class ExperimentViewSet( + NestedEnvironmentViewSet[Experiment], + mixins.ListModelMixin, + mixins.CreateModelMixin, + mixins.RetrieveModelMixin, + mixins.UpdateModelMixin, + mixins.DestroyModelMixin, +): + serializer_class = ExperimentSerializer + pagination_class = None + permission_classes = [IsAuthenticated, ExperimentPermission] + model_class = Experiment + lookup_field = "id" + lookup_url_kwarg = "experiment_id" + filterset_fields: list[str] = [] + + def get_queryset(self) -> "QuerySet[Experiment]": + qs = super().get_queryset() + status_filter = self.request.query_params.get("status") + if status_filter: + qs = qs.filter(status=status_filter) + return qs + + def create(self, request: Request, *args: object, **kwargs: object) -> Response: + serializer = self.get_serializer(data=request.data) + serializer.is_valid(raise_exception=True) + + feature = serializer.validated_data["feature"] + environment = self._get_environment() + if ( + Experiment.objects.filter( + feature=feature, + environment=environment, + ) + .exclude(status=ExperimentStatus.COMPLETED) + .exists() + ): + return Response( + {"detail": "An active experiment already exists for this feature."}, + status=400, + ) + + self.perform_create(serializer) + return Response(serializer.data, status=201) + + def perform_create(self, serializer: BaseSerializer[Experiment]) -> None: + experiment: Experiment = serializer.save(environment=self._get_environment()) + create_experiment_audit_log( + experiment, self._get_user(self.request), action="created" + ) + + def perform_update(self, serializer: BaseSerializer[Experiment]) -> None: + experiment: Experiment = serializer.save() + create_experiment_audit_log( + experiment, self._get_user(self.request), action="updated" + ) + + def perform_destroy(self, instance: Experiment) -> None: + create_experiment_audit_log( + instance, self._get_user(self.request), action="deleted" + ) + instance.delete() + + @staticmethod + def _get_user(request: Request) -> FFAdminUser: + return request.user # type: ignore[return-value] diff --git a/api/tests/unit/experimentation/conftest.py b/api/tests/unit/experimentation/conftest.py index 127286d3016f..f81c029df0cd 100644 --- a/api/tests/unit/experimentation/conftest.py +++ b/api/tests/unit/experimentation/conftest.py @@ -2,7 +2,13 @@ from django.urls import reverse from environments.models import Environment -from experimentation.models import WarehouseConnection, WarehouseType +from experimentation.models import ( + Experiment, + ExperimentStatus, + WarehouseConnection, + WarehouseType, +) +from features.models import Feature @pytest.fixture() @@ -21,3 +27,17 @@ def warehouse_connection_url(environment: Environment) -> str: "api-v1:environments:experimentation:warehouse-connections-list", args=[environment.api_key], ) + + +@pytest.fixture() +def experiment( + environment: Environment, + multivariate_feature: Feature, +) -> Experiment: + return Experiment.objects.create( + environment=environment, + feature=multivariate_feature, + name="Test Experiment", + hypothesis="Test hypothesis", + status=ExperimentStatus.CREATED, + ) diff --git a/api/tests/unit/experimentation/test_experiment_views.py b/api/tests/unit/experimentation/test_experiment_views.py new file mode 100644 index 000000000000..d436d535a0e6 --- /dev/null +++ b/api/tests/unit/experimentation/test_experiment_views.py @@ -0,0 +1,532 @@ +import pytest +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient + +from audit.models import AuditLog +from audit.related_object_type import RelatedObjectType +from environments.models import Environment +from experimentation.models import Experiment, ExperimentStatus +from features.models import Feature +from tests.types import EnableFeaturesFixture + +pytestmark = pytest.mark.django_db + +EXPERIMENT_FLAG = "experimental_flags" + + +def _list_url(environment: Environment) -> str: + return reverse( + "api-v1:environments:experiments:experiments-list", + args=[environment.api_key], + ) + + +def _detail_url(environment: Environment, experiment: Experiment) -> str: + return reverse( + "api-v1:environments:experiments:experiments-detail", + args=[environment.api_key, experiment.id], + ) + + +# -- Create ------------------------------------------------------------------ + + +def test_post__valid_multivariate_feature__returns_201( + admin_client: APIClient, + environment: Environment, + multivariate_feature: Feature, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client.post( + _list_url(environment), + data={ + "feature": multivariate_feature.id, + "name": "My experiment", + "hypothesis": "It will work", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + data = response.json() + assert data["feature"] == multivariate_feature.id + assert data["name"] == "My experiment" + assert data["status"] == "created" + assert data["started_at"] is None + assert data["ended_at"] is None + + +def test_post__non_multivariate_feature__returns_400( + admin_client: APIClient, + environment: Environment, + feature: Feature, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client.post( + _list_url(environment), + data={ + "feature": feature.id, + "name": "Bad experiment", + "hypothesis": "Nope", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_post__feature_from_different_project__returns_400( + admin_client: APIClient, + environment: Environment, + enable_features: EnableFeaturesFixture, + organisation_one_project_one_feature_one: Feature, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client.post( + _list_url(environment), + data={ + "feature": organisation_one_project_one_feature_one.id, + "name": "Wrong project", + "hypothesis": "Nope", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_post__active_experiment_exists__returns_400( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + multivariate_feature: Feature, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client.post( + _list_url(environment), + data={ + "feature": multivariate_feature.id, + "name": "Duplicate", + "hypothesis": "Should fail", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_post__completed_experiment_exists__returns_201( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + multivariate_feature: Feature, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + experiment.status = ExperimentStatus.COMPLETED + experiment.save() + + # When + response = admin_client.post( + _list_url(environment), + data={ + "feature": multivariate_feature.id, + "name": "New experiment", + "hypothesis": "Should work", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + + +# -- Permissions -------------------------------------------------------------- + + +def test_post__non_admin__returns_403( + staff_client: APIClient, + environment: Environment, + multivariate_feature: Feature, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = staff_client.post( + _list_url(environment), + data={ + "feature": multivariate_feature.id, + "name": "No access", + "hypothesis": "Nope", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_post__feature_flag_disabled__returns_403( + admin_client: APIClient, + environment: Environment, + multivariate_feature: Feature, +) -> None: + # When + response = admin_client.post( + _list_url(environment), + data={ + "feature": multivariate_feature.id, + "name": "No flag", + "hypothesis": "Nope", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + + +# -- List & Retrieve --------------------------------------------------------- + + +def test_get_list__with_experiments__returns_all( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client.get(_list_url(environment)) + + # Then + assert response.status_code == status.HTTP_200_OK + assert len(response.json()) == 1 + assert response.json()[0]["id"] == experiment.id + + +def test_get_list__empty__returns_200( + admin_client: APIClient, + environment: Environment, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client.get(_list_url(environment)) + + # Then + assert response.status_code == status.HTTP_200_OK + assert response.json() == [] + + +@pytest.mark.parametrize( + "filter_status, expected_count", + [ + ("created", 1), + ("running", 0), + ], +) +def test_get_list__filter_by_status__returns_filtered( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + enable_features: EnableFeaturesFixture, + filter_status: str, + expected_count: int, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client.get( + _list_url(environment), {"status": filter_status} + ) + + # Then + assert response.status_code == status.HTTP_200_OK + assert len(response.json()) == expected_count + + +def test_get_detail__exists__returns_200( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client.get(_detail_url(environment, experiment)) + + # Then + assert response.status_code == status.HTTP_200_OK + assert response.json()["id"] == experiment.id + + +# -- Update ------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "field, value", + [ + ("name", "Updated name"), + ("hypothesis", "Updated hypothesis"), + ], +) +def test_patch__update_field__returns_200( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + enable_features: EnableFeaturesFixture, + field: str, + value: str, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client.patch( + _detail_url(environment, experiment), + data={field: value}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_200_OK + assert response.json()[field] == value + + +@pytest.mark.parametrize( + "from_status, to_status", + [ + (ExperimentStatus.CREATED, ExperimentStatus.RUNNING), + (ExperimentStatus.RUNNING, ExperimentStatus.PAUSED), + (ExperimentStatus.RUNNING, ExperimentStatus.COMPLETED), + (ExperimentStatus.PAUSED, ExperimentStatus.RUNNING), + (ExperimentStatus.PAUSED, ExperimentStatus.COMPLETED), + ], +) +def test_patch__valid_status_transition__returns_200( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + enable_features: EnableFeaturesFixture, + from_status: str, + to_status: str, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + experiment.status = from_status + experiment.save() + + # When + response = admin_client.patch( + _detail_url(environment, experiment), + data={"status": to_status}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_200_OK + assert response.json()["status"] == to_status + + +@pytest.mark.parametrize( + "from_status, to_status", + [ + (ExperimentStatus.CREATED, ExperimentStatus.PAUSED), + (ExperimentStatus.CREATED, ExperimentStatus.COMPLETED), + (ExperimentStatus.COMPLETED, ExperimentStatus.RUNNING), + (ExperimentStatus.COMPLETED, ExperimentStatus.CREATED), + ], +) +def test_patch__invalid_status_transition__returns_400( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + enable_features: EnableFeaturesFixture, + from_status: str, + to_status: str, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + experiment.status = from_status + experiment.save() + + # When + response = admin_client.patch( + _detail_url(environment, experiment), + data={"status": to_status}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_patch__change_feature__returns_400( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + feature: Feature, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client.patch( + _detail_url(environment, experiment), + data={"feature": feature.id}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_patch__transition_to_running__sets_started_at( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client.patch( + _detail_url(environment, experiment), + data={"status": "running"}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_200_OK + assert response.json()["started_at"] is not None + + +def test_patch__transition_to_completed__sets_ended_at( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + experiment.status = ExperimentStatus.RUNNING + experiment.save() + + # When + response = admin_client.patch( + _detail_url(environment, experiment), + data={"status": "completed"}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_200_OK + assert response.json()["ended_at"] is not None + + +# -- Delete ------------------------------------------------------------------- + + +def test_delete__exists__returns_204_and_soft_deletes( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client.delete(_detail_url(environment, experiment)) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + assert not Experiment.objects.filter(id=experiment.id).exists() + assert ( + Experiment.objects.all_with_deleted().filter(id=experiment.id).exists() + ) + + +# -- Audit ------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "method, action_label", + [ + ("post", "created"), + ("patch", "updated"), + ("delete", "deleted"), + ], +) +def test_crud__creates_audit_log( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + multivariate_feature: Feature, + enable_features: EnableFeaturesFixture, + method: str, + action_label: str, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + if method == "post": + experiment.delete() + admin_client.post( + _list_url(environment), + data={ + "feature": multivariate_feature.id, + "name": "Audit test", + "hypothesis": "Check audit", + }, + format="json", + ) + elif method == "patch": + admin_client.patch( + _detail_url(environment, experiment), + data={"name": "Renamed"}, + format="json", + ) + else: + admin_client.delete(_detail_url(environment, experiment)) + + # Then + audit = AuditLog.objects.filter( + related_object_type=RelatedObjectType.EXPERIMENT.name + ).last() + assert audit is not None + assert action_label in audit.log From b27797d47b9969aaf1df4ceed064b25282682685 Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 25 May 2026 11:47:22 +0200 Subject: [PATCH 02/11] feat(experimentation): reworked-tests --- .../experimentation/test_experiment_views.py | 90 +++++-------------- 1 file changed, 22 insertions(+), 68 deletions(-) diff --git a/api/tests/unit/experimentation/test_experiment_views.py b/api/tests/unit/experimentation/test_experiment_views.py index d436d535a0e6..a9d74bfb4a3e 100644 --- a/api/tests/unit/experimentation/test_experiment_views.py +++ b/api/tests/unit/experimentation/test_experiment_views.py @@ -29,9 +29,6 @@ def _detail_url(environment: Environment, experiment: Experiment) -> str: ) -# -- Create ------------------------------------------------------------------ - - def test_post__valid_multivariate_feature__returns_201( admin_client: APIClient, environment: Environment, @@ -162,9 +159,6 @@ def test_post__completed_experiment_exists__returns_201( assert response.status_code == status.HTTP_201_CREATED -# -- Permissions -------------------------------------------------------------- - - def test_post__non_admin__returns_403( staff_client: APIClient, environment: Environment, @@ -194,7 +188,7 @@ def test_post__feature_flag_disabled__returns_403( environment: Environment, multivariate_feature: Feature, ) -> None: - # When + # Given / When response = admin_client.post( _list_url(environment), data={ @@ -209,9 +203,6 @@ def test_post__feature_flag_disabled__returns_403( assert response.status_code == status.HTTP_403_FORBIDDEN -# -- List & Retrieve --------------------------------------------------------- - - def test_get_list__with_experiments__returns_all( admin_client: APIClient, environment: Environment, @@ -251,6 +242,8 @@ def test_get_list__empty__returns_200( [ ("created", 1), ("running", 0), + ("paused", 0), + ("completed", 0), ], ) def test_get_list__filter_by_status__returns_filtered( @@ -265,9 +258,7 @@ def test_get_list__filter_by_status__returns_filtered( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.get( - _list_url(environment), {"status": filter_status} - ) + response = admin_client.get(_list_url(environment), {"status": filter_status}) # Then assert response.status_code == status.HTTP_200_OK @@ -291,9 +282,6 @@ def test_get_detail__exists__returns_200( assert response.json()["id"] == experiment.id -# -- Update ------------------------------------------------------------------- - - @pytest.mark.parametrize( "field, value", [ @@ -325,22 +313,28 @@ def test_patch__update_field__returns_200( @pytest.mark.parametrize( - "from_status, to_status", + "from_status, to_status, expected_status_code", [ - (ExperimentStatus.CREATED, ExperimentStatus.RUNNING), - (ExperimentStatus.RUNNING, ExperimentStatus.PAUSED), - (ExperimentStatus.RUNNING, ExperimentStatus.COMPLETED), - (ExperimentStatus.PAUSED, ExperimentStatus.RUNNING), - (ExperimentStatus.PAUSED, ExperimentStatus.COMPLETED), + (ExperimentStatus.CREATED, ExperimentStatus.RUNNING, 200), + (ExperimentStatus.RUNNING, ExperimentStatus.PAUSED, 200), + (ExperimentStatus.RUNNING, ExperimentStatus.COMPLETED, 200), + (ExperimentStatus.PAUSED, ExperimentStatus.RUNNING, 200), + (ExperimentStatus.PAUSED, ExperimentStatus.COMPLETED, 200), + (ExperimentStatus.CREATED, ExperimentStatus.PAUSED, 400), + (ExperimentStatus.CREATED, ExperimentStatus.COMPLETED, 400), + (ExperimentStatus.COMPLETED, ExperimentStatus.RUNNING, 400), + (ExperimentStatus.COMPLETED, ExperimentStatus.CREATED, 400), + (ExperimentStatus.RUNNING, ExperimentStatus.CREATED, 400), ], ) -def test_patch__valid_status_transition__returns_200( +def test_patch__status_transition__returns_expected( admin_client: APIClient, environment: Environment, experiment: Experiment, enable_features: EnableFeaturesFixture, from_status: str, to_status: str, + expected_status_code: int, ) -> None: # Given enable_features(EXPERIMENT_FLAG) @@ -355,41 +349,9 @@ def test_patch__valid_status_transition__returns_200( ) # Then - assert response.status_code == status.HTTP_200_OK - assert response.json()["status"] == to_status - - -@pytest.mark.parametrize( - "from_status, to_status", - [ - (ExperimentStatus.CREATED, ExperimentStatus.PAUSED), - (ExperimentStatus.CREATED, ExperimentStatus.COMPLETED), - (ExperimentStatus.COMPLETED, ExperimentStatus.RUNNING), - (ExperimentStatus.COMPLETED, ExperimentStatus.CREATED), - ], -) -def test_patch__invalid_status_transition__returns_400( - admin_client: APIClient, - environment: Environment, - experiment: Experiment, - enable_features: EnableFeaturesFixture, - from_status: str, - to_status: str, -) -> None: - # Given - enable_features(EXPERIMENT_FLAG) - experiment.status = from_status - experiment.save() - - # When - response = admin_client.patch( - _detail_url(environment, experiment), - data={"status": to_status}, - format="json", - ) - - # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.status_code == expected_status_code + if expected_status_code == 200: + assert response.json()["status"] == to_status def test_patch__change_feature__returns_400( @@ -457,9 +419,6 @@ def test_patch__transition_to_completed__sets_ended_at( assert response.json()["ended_at"] is not None -# -- Delete ------------------------------------------------------------------- - - def test_delete__exists__returns_204_and_soft_deletes( admin_client: APIClient, environment: Environment, @@ -475,12 +434,7 @@ def test_delete__exists__returns_204_and_soft_deletes( # Then assert response.status_code == status.HTTP_204_NO_CONTENT assert not Experiment.objects.filter(id=experiment.id).exists() - assert ( - Experiment.objects.all_with_deleted().filter(id=experiment.id).exists() - ) - - -# -- Audit ------------------------------------------------------------------- + assert Experiment.objects.all_with_deleted().filter(id=experiment.id).exists() @pytest.mark.parametrize( @@ -491,7 +445,7 @@ def test_delete__exists__returns_204_and_soft_deletes( ("delete", "deleted"), ], ) -def test_crud__creates_audit_log( +def test_crud__any_method__creates_audit_log( admin_client: APIClient, environment: Environment, experiment: Experiment, From 43b089717a2c5bbcef63a0ee2b80026fd4dccbc8 Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 25 May 2026 11:52:15 +0200 Subject: [PATCH 03/11] feat(experimentation): type-linting --- api/experimentation/migrations/0004_experiment.py | 2 +- api/tests/unit/experimentation/conftest.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/api/experimentation/migrations/0004_experiment.py b/api/experimentation/migrations/0004_experiment.py index 89924f708255..f87c9016f289 100644 --- a/api/experimentation/migrations/0004_experiment.py +++ b/api/experimentation/migrations/0004_experiment.py @@ -1,7 +1,7 @@ # Generated by Django 5.2.14 on 2026-05-25 08:45 import django.db.models.deletion -import django_lifecycle.mixins +import django_lifecycle.mixins # type: ignore[import-untyped] import uuid from django.db import migrations, models diff --git a/api/tests/unit/experimentation/conftest.py b/api/tests/unit/experimentation/conftest.py index 365384c14d47..fbc21d8f6974 100644 --- a/api/tests/unit/experimentation/conftest.py +++ b/api/tests/unit/experimentation/conftest.py @@ -42,10 +42,11 @@ def experiment( environment: Environment, multivariate_feature: Feature, ) -> Experiment: - return Experiment.objects.create( + experiment: Experiment = Experiment.objects.create( environment=environment, feature=multivariate_feature, name="Test Experiment", hypothesis="Test hypothesis", status=ExperimentStatus.CREATED, ) + return experiment From 5d55695e1d141bce4e9747001f85a389b0969afc Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 25 May 2026 12:06:14 +0200 Subject: [PATCH 04/11] feat(experimentation): changed return status when experiment exists and use hooks to set timestamps --- api/experimentation/models.py | 20 +++++++++++++++++++ api/experimentation/serializers.py | 14 ------------- api/experimentation/views.py | 2 +- .../experimentation/test_experiment_views.py | 4 ++-- 4 files changed, 23 insertions(+), 17 deletions(-) diff --git a/api/experimentation/models.py b/api/experimentation/models.py index c2c1f9712b1e..bfe74338b652 100644 --- a/api/experimentation/models.py +++ b/api/experimentation/models.py @@ -1,8 +1,10 @@ from django.db import models from django.db.models import Q +from django.utils import timezone from django_lifecycle import ( # type: ignore[import-untyped] AFTER_CREATE, AFTER_DELETE, + BEFORE_UPDATE, LifecycleModelMixin, hook, ) @@ -117,3 +119,21 @@ class Meta: name="unique_active_experiment_per_feature_env", ), ] + + @hook( + BEFORE_UPDATE, + when="status", + was=ExperimentStatus.CREATED, + is_now=ExperimentStatus.RUNNING, + ) # type: ignore[misc] + def set_started_at(self) -> None: + if not self.started_at: + self.started_at = timezone.now() + + @hook( + BEFORE_UPDATE, + when="status", + is_now=ExperimentStatus.COMPLETED, + ) # type: ignore[misc] + def set_ended_at(self) -> None: + self.ended_at = timezone.now() diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index bd93b3591717..50387e23773e 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -1,6 +1,5 @@ from typing import Any -from django.utils import timezone from rest_framework import serializers from environments.models import Environment @@ -156,16 +155,3 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: {"feature": "Cannot change the feature of an existing experiment."} ) return attrs - - def update( - self, - instance: Experiment, - validated_data: dict[str, Any], - ) -> Experiment: - new_status = validated_data.get("status") - if new_status == ExperimentStatus.RUNNING and instance.started_at is None: - validated_data["started_at"] = timezone.now() - elif new_status == ExperimentStatus.COMPLETED: - validated_data["ended_at"] = timezone.now() - result: Experiment = super().update(instance, validated_data) - return result diff --git a/api/experimentation/views.py b/api/experimentation/views.py index a9b902ceb1c1..d7a80a6ade75 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -119,7 +119,7 @@ def create(self, request: Request, *args: object, **kwargs: object) -> Response: ): return Response( {"detail": "An active experiment already exists for this feature."}, - status=400, + status=409, ) self.perform_create(serializer) diff --git a/api/tests/unit/experimentation/test_experiment_views.py b/api/tests/unit/experimentation/test_experiment_views.py index a9d74bfb4a3e..3550cde7f182 100644 --- a/api/tests/unit/experimentation/test_experiment_views.py +++ b/api/tests/unit/experimentation/test_experiment_views.py @@ -107,7 +107,7 @@ def test_post__feature_from_different_project__returns_400( assert response.status_code == status.HTTP_400_BAD_REQUEST -def test_post__active_experiment_exists__returns_400( +def test_post__active_experiment_exists__returns_409( admin_client: APIClient, environment: Environment, experiment: Experiment, @@ -129,7 +129,7 @@ def test_post__active_experiment_exists__returns_400( ) # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.status_code == status.HTTP_409_CONFLICT def test_post__completed_experiment_exists__returns_201( From 4269a8a5b41ff73c2226e06360b64231cb63920d Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 25 May 2026 12:18:13 +0200 Subject: [PATCH 05/11] feat(experimentation): added test coverage --- .../experimentation/test_experiment_views.py | 102 +++++++++++++++--- 1 file changed, 90 insertions(+), 12 deletions(-) diff --git a/api/tests/unit/experimentation/test_experiment_views.py b/api/tests/unit/experimentation/test_experiment_views.py index 3550cde7f182..3d509aeada55 100644 --- a/api/tests/unit/experimentation/test_experiment_views.py +++ b/api/tests/unit/experimentation/test_experiment_views.py @@ -1,3 +1,7 @@ +from __future__ import annotations + +from typing import TYPE_CHECKING + import pytest from django.urls import reverse from rest_framework import status @@ -7,9 +11,13 @@ from audit.related_object_type import RelatedObjectType from environments.models import Environment from experimentation.models import Experiment, ExperimentStatus +from features.feature_types import MULTIVARIATE from features.models import Feature from tests.types import EnableFeaturesFixture +if TYPE_CHECKING: + from projects.models import Project + pytestmark = pytest.mark.django_db EXPERIMENT_FLAG = "experimental_flags" @@ -87,16 +95,22 @@ def test_post__feature_from_different_project__returns_400( admin_client: APIClient, environment: Environment, enable_features: EnableFeaturesFixture, - organisation_one_project_one_feature_one: Feature, + organisation_one_project_one: "Project", ) -> None: # Given enable_features(EXPERIMENT_FLAG) + other_feature = Feature.objects.create( + project=organisation_one_project_one, + name="other_mv_feature", + type=MULTIVARIATE, + initial_value="control", + ) # When response = admin_client.post( _list_url(environment), data={ - "feature": organisation_one_project_one_feature_one.id, + "feature": other_feature.id, "name": "Wrong project", "hypothesis": "Nope", }, @@ -159,8 +173,8 @@ def test_post__completed_experiment_exists__returns_201( assert response.status_code == status.HTTP_201_CREATED -def test_post__non_admin__returns_403( - staff_client: APIClient, +def test_post__explicit_non_created_status__returns_400( + admin_client: APIClient, environment: Environment, multivariate_feature: Feature, enable_features: EnableFeaturesFixture, @@ -169,31 +183,74 @@ def test_post__non_admin__returns_403( enable_features(EXPERIMENT_FLAG) # When - response = staff_client.post( + response = admin_client.post( _list_url(environment), data={ "feature": multivariate_feature.id, - "name": "No access", + "name": "Forced status", "hypothesis": "Nope", + "status": "running", }, format="json", ) # Then - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_400_BAD_REQUEST -def test_post__feature_flag_disabled__returns_403( - admin_client: APIClient, +@pytest.mark.parametrize( + "client_fixture, enable_flag, expected_status", + [ + ("staff_client", True, status.HTTP_403_FORBIDDEN), + ("admin_client", False, status.HTTP_403_FORBIDDEN), + ], +) +def test_post__insufficient_permissions__returns_403( + request: pytest.FixtureRequest, environment: Environment, multivariate_feature: Feature, + enable_features: EnableFeaturesFixture, + client_fixture: str, + enable_flag: bool, + expected_status: int, ) -> None: - # Given / When - response = admin_client.post( + # Given + api_client: APIClient = request.getfixturevalue(client_fixture) + if enable_flag: + enable_features(EXPERIMENT_FLAG) + + # When + response = api_client.post( _list_url(environment), data={ "feature": multivariate_feature.id, - "name": "No flag", + "name": "No access", + "hypothesis": "Nope", + }, + format="json", + ) + + # Then + assert response.status_code == expected_status + + +def test_post__nonexistent_environment__returns_403( + admin_client: APIClient, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + url = reverse( + "api-v1:environments:experiments:experiments-list", + args=["bad_api_key"], + ) + + # When + response = admin_client.post( + url, + data={ + "feature": 999, + "name": "Bad env", "hypothesis": "Nope", }, format="json", @@ -354,6 +411,27 @@ def test_patch__status_transition__returns_expected( assert response.json()["status"] == to_status +def test_patch__same_status__returns_200( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client.patch( + _detail_url(environment, experiment), + data={"status": "created"}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_200_OK + assert response.json()["status"] == "created" + + def test_patch__change_feature__returns_400( admin_client: APIClient, environment: Environment, From 52fa070f94d68668c687ea2bb7464da3614c7993 Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 25 May 2026 12:35:34 +0200 Subject: [PATCH 06/11] feat(experimentation): added test coverage for existing mv features --- .../experimentation/test_experiment_views.py | 29 ++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/api/tests/unit/experimentation/test_experiment_views.py b/api/tests/unit/experimentation/test_experiment_views.py index 3d509aeada55..903cde88c57f 100644 --- a/api/tests/unit/experimentation/test_experiment_views.py +++ b/api/tests/unit/experimentation/test_experiment_views.py @@ -173,11 +173,20 @@ def test_post__completed_experiment_exists__returns_201( assert response.status_code == status.HTTP_201_CREATED -def test_post__explicit_non_created_status__returns_400( +@pytest.mark.parametrize( + "explicit_status, expected_status_code", + [ + ("created", 201), + ("running", 400), + ], +) +def test_post__explicit_status__returns_expected( admin_client: APIClient, environment: Environment, multivariate_feature: Feature, enable_features: EnableFeaturesFixture, + explicit_status: str, + expected_status_code: int, ) -> None: # Given enable_features(EXPERIMENT_FLAG) @@ -187,15 +196,15 @@ def test_post__explicit_non_created_status__returns_400( _list_url(environment), data={ "feature": multivariate_feature.id, - "name": "Forced status", - "hypothesis": "Nope", - "status": "running", + "name": "Explicit status", + "hypothesis": "Testing", + "status": explicit_status, }, format="json", ) # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.status_code == expected_status_code @pytest.mark.parametrize( @@ -436,16 +445,22 @@ def test_patch__change_feature__returns_400( admin_client: APIClient, environment: Environment, experiment: Experiment, - feature: Feature, + project: "Project", enable_features: EnableFeaturesFixture, ) -> None: # Given enable_features(EXPERIMENT_FLAG) + other_feature = Feature.objects.create( + project=project, + name="other_mv_feature", + type=MULTIVARIATE, + initial_value="control", + ) # When response = admin_client.patch( _detail_url(environment, experiment), - data={"feature": feature.id}, + data={"feature": other_feature.id}, format="json", ) From 683fc699edd7b3a49cad5663cce755c7f6b17c52 Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 25 May 2026 14:37:00 +0200 Subject: [PATCH 07/11] feat(experimentation): extracted status actions --- api/experimentation/serializers.py | 32 +-- api/experimentation/views.py | 48 +++- .../experimentation/test_experiment_views.py | 233 ++++++++---------- 3 files changed, 159 insertions(+), 154 deletions(-) diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index 50387e23773e..2380b8c33871 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -4,9 +4,7 @@ from environments.models import Environment from experimentation.models import ( - VALID_STATUS_TRANSITIONS, Experiment, - ExperimentStatus, WarehouseConnection, WarehouseConnectionStatus, WarehouseType, @@ -110,6 +108,7 @@ class Meta: ) read_only_fields = ( "id", + "status", "created_at", "updated_at", "started_at", @@ -121,33 +120,12 @@ def validate_feature(self, feature: Any) -> Any: raise serializers.ValidationError( "Experiments can only be created for multivariate flags." ) - view = self.context.get("view") - if view: - environment: Environment = view._get_environment() - if feature.project_id != environment.project_id: - raise serializers.ValidationError( - "Feature does not belong to this environment's project." - ) - return feature - - def validate_status(self, status: str) -> str: - if self.instance is None: - if status != ExperimentStatus.CREATED: - raise serializers.ValidationError( - "Status must be 'created' when creating an experiment." - ) - return status - - current_status: str = self.instance.status # type: ignore[union-attr] - if status == current_status: - return status - - valid_targets = VALID_STATUS_TRANSITIONS.get(current_status, set()) - if status not in valid_targets: + environment: Environment | None = self.context.get("environment") + if environment and feature.project_id != environment.project_id: raise serializers.ValidationError( - f"Cannot transition from '{current_status}' to '{status}'." + "Feature does not belong to this environment's project." ) - return status + return feature def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: if self.instance is not None and "feature" in attrs: diff --git a/api/experimentation/views.py b/api/experimentation/views.py index d7a80a6ade75..b7f73cc0e406 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -1,12 +1,20 @@ +from typing import Any + from django.db.models import QuerySet from rest_framework import mixins +from rest_framework.decorators import action from rest_framework.permissions import IsAuthenticated from rest_framework.request import Request from rest_framework.response import Response from rest_framework.serializers import BaseSerializer from environments.views import NestedEnvironmentViewSet -from experimentation.models import Experiment, ExperimentStatus, WarehouseConnection +from experimentation.models import ( + VALID_STATUS_TRANSITIONS, + Experiment, + ExperimentStatus, + WarehouseConnection, +) from experimentation.permissions import ( ExperimentPermission, WarehouseConnectionPermission, @@ -96,6 +104,11 @@ class ExperimentViewSet( lookup_url_kwarg = "experiment_id" filterset_fields: list[str] = [] + def get_serializer_context(self) -> dict[str, Any]: + context = super().get_serializer_context() + context["environment"] = self._get_environment() + return context + def get_queryset(self) -> "QuerySet[Experiment]": qs = super().get_queryset() status_filter = self.request.query_params.get("status") @@ -143,6 +156,39 @@ def perform_destroy(self, instance: Experiment) -> None: ) instance.delete() + @action(detail=True, methods=["post"]) + def start(self, request: Request, **kwargs: object) -> Response: + return self._transition_status(ExperimentStatus.RUNNING) + + @action(detail=True, methods=["post"]) + def pause(self, request: Request, **kwargs: object) -> Response: + return self._transition_status(ExperimentStatus.PAUSED) + + @action(detail=True, methods=["post"]) + def complete(self, request: Request, **kwargs: object) -> Response: + return self._transition_status(ExperimentStatus.COMPLETED) + + def _transition_status(self, target_status: str) -> Response: + experiment: Experiment = self.get_object() + valid_targets = VALID_STATUS_TRANSITIONS.get(experiment.status, set()) + if target_status not in valid_targets: + return Response( + { + "detail": ( + f"Cannot transition from '{experiment.status}' " + f"to '{target_status}'." + ) + }, + status=400, + ) + experiment.status = target_status + experiment.save() + create_experiment_audit_log( + experiment, self._get_user(self.request), action=target_status + ) + serializer = self.get_serializer(experiment) + return Response(serializer.data) + @staticmethod def _get_user(request: Request) -> FFAdminUser: return request.user # type: ignore[return-value] diff --git a/api/tests/unit/experimentation/test_experiment_views.py b/api/tests/unit/experimentation/test_experiment_views.py index 903cde88c57f..1454e55873b3 100644 --- a/api/tests/unit/experimentation/test_experiment_views.py +++ b/api/tests/unit/experimentation/test_experiment_views.py @@ -37,6 +37,13 @@ def _detail_url(environment: Environment, experiment: Experiment) -> str: ) +def _action_url(environment: Environment, experiment: Experiment, action: str) -> str: + return reverse( + f"api-v1:environments:experiments:experiments-{action}", + args=[environment.api_key, experiment.id], + ) + + def test_post__valid_multivariate_feature__returns_201( admin_client: APIClient, environment: Environment, @@ -95,7 +102,7 @@ def test_post__feature_from_different_project__returns_400( admin_client: APIClient, environment: Environment, enable_features: EnableFeaturesFixture, - organisation_one_project_one: "Project", + organisation_one_project_one: Project, ) -> None: # Given enable_features(EXPERIMENT_FLAG) @@ -173,40 +180,6 @@ def test_post__completed_experiment_exists__returns_201( assert response.status_code == status.HTTP_201_CREATED -@pytest.mark.parametrize( - "explicit_status, expected_status_code", - [ - ("created", 201), - ("running", 400), - ], -) -def test_post__explicit_status__returns_expected( - admin_client: APIClient, - environment: Environment, - multivariate_feature: Feature, - enable_features: EnableFeaturesFixture, - explicit_status: str, - expected_status_code: int, -) -> None: - # Given - enable_features(EXPERIMENT_FLAG) - - # When - response = admin_client.post( - _list_url(environment), - data={ - "feature": multivariate_feature.id, - "name": "Explicit status", - "hypothesis": "Testing", - "status": explicit_status, - }, - format="json", - ) - - # Then - assert response.status_code == expected_status_code - - @pytest.mark.parametrize( "client_fixture, enable_flag, expected_status", [ @@ -378,28 +351,55 @@ def test_patch__update_field__returns_200( assert response.json()[field] == value +def test_patch__change_feature__returns_400( + admin_client: APIClient, + environment: Environment, + experiment: Experiment, + project: Project, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + other_feature = Feature.objects.create( + project=project, + name="other_mv_feature", + type=MULTIVARIATE, + initial_value="control", + ) + + # When + response = admin_client.patch( + _detail_url(environment, experiment), + data={"feature": other_feature.id}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + + @pytest.mark.parametrize( - "from_status, to_status, expected_status_code", + "from_status, action_name, expected_status_code", [ - (ExperimentStatus.CREATED, ExperimentStatus.RUNNING, 200), - (ExperimentStatus.RUNNING, ExperimentStatus.PAUSED, 200), - (ExperimentStatus.RUNNING, ExperimentStatus.COMPLETED, 200), - (ExperimentStatus.PAUSED, ExperimentStatus.RUNNING, 200), - (ExperimentStatus.PAUSED, ExperimentStatus.COMPLETED, 200), - (ExperimentStatus.CREATED, ExperimentStatus.PAUSED, 400), - (ExperimentStatus.CREATED, ExperimentStatus.COMPLETED, 400), - (ExperimentStatus.COMPLETED, ExperimentStatus.RUNNING, 400), - (ExperimentStatus.COMPLETED, ExperimentStatus.CREATED, 400), - (ExperimentStatus.RUNNING, ExperimentStatus.CREATED, 400), + (ExperimentStatus.CREATED, "start", 200), + (ExperimentStatus.RUNNING, "pause", 200), + (ExperimentStatus.RUNNING, "complete", 200), + (ExperimentStatus.PAUSED, "start", 200), + (ExperimentStatus.PAUSED, "complete", 200), + (ExperimentStatus.CREATED, "pause", 400), + (ExperimentStatus.CREATED, "complete", 400), + (ExperimentStatus.COMPLETED, "start", 400), + (ExperimentStatus.COMPLETED, "pause", 400), + (ExperimentStatus.RUNNING, "start", 400), ], ) -def test_patch__status_transition__returns_expected( +def test_action__status_transition__returns_expected( admin_client: APIClient, environment: Environment, experiment: Experiment, enable_features: EnableFeaturesFixture, from_status: str, - to_status: str, + action_name: str, expected_status_code: int, ) -> None: # Given @@ -408,19 +408,13 @@ def test_patch__status_transition__returns_expected( experiment.save() # When - response = admin_client.patch( - _detail_url(environment, experiment), - data={"status": to_status}, - format="json", - ) + response = admin_client.post(_action_url(environment, experiment, action_name)) # Then assert response.status_code == expected_status_code - if expected_status_code == 200: - assert response.json()["status"] == to_status -def test_patch__same_status__returns_200( +def test_action__start__sets_started_at( admin_client: APIClient, environment: Environment, experiment: Experiment, @@ -430,45 +424,33 @@ def test_patch__same_status__returns_200( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.patch( - _detail_url(environment, experiment), - data={"status": "created"}, - format="json", - ) + response = admin_client.post(_action_url(environment, experiment, "start")) # Then assert response.status_code == status.HTTP_200_OK - assert response.json()["status"] == "created" + assert response.json()["started_at"] is not None -def test_patch__change_feature__returns_400( +def test_action__complete__sets_ended_at( admin_client: APIClient, environment: Environment, experiment: Experiment, - project: "Project", enable_features: EnableFeaturesFixture, ) -> None: # Given enable_features(EXPERIMENT_FLAG) - other_feature = Feature.objects.create( - project=project, - name="other_mv_feature", - type=MULTIVARIATE, - initial_value="control", - ) + experiment.status = ExperimentStatus.RUNNING + experiment.save() # When - response = admin_client.patch( - _detail_url(environment, experiment), - data={"feature": other_feature.id}, - format="json", - ) + response = admin_client.post(_action_url(environment, experiment, "complete")) # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.status_code == status.HTTP_200_OK + assert response.json()["ended_at"] is not None -def test_patch__transition_to_running__sets_started_at( +def test_delete__exists__returns_204_and_soft_deletes( admin_client: APIClient, environment: Environment, experiment: Experiment, @@ -478,18 +460,43 @@ def test_patch__transition_to_running__sets_started_at( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.patch( - _detail_url(environment, experiment), - data={"status": "running"}, + response = admin_client.delete(_detail_url(environment, experiment)) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + assert not Experiment.objects.filter(id=experiment.id).exists() + assert Experiment.objects.all_with_deleted().filter(id=experiment.id).exists() + + +def test_post__valid_create__creates_audit_log( + admin_client: APIClient, + environment: Environment, + multivariate_feature: Feature, + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + admin_client.post( + _list_url(environment), + data={ + "feature": multivariate_feature.id, + "name": "Audit test", + "hypothesis": "Check audit", + }, format="json", ) # Then - assert response.status_code == status.HTTP_200_OK - assert response.json()["started_at"] is not None + audit = AuditLog.objects.filter( + related_object_type=RelatedObjectType.EXPERIMENT.name + ).last() + assert audit is not None + assert "created" in audit.log -def test_patch__transition_to_completed__sets_ended_at( +def test_patch__valid_update__creates_audit_log( admin_client: APIClient, environment: Environment, experiment: Experiment, @@ -497,22 +504,23 @@ def test_patch__transition_to_completed__sets_ended_at( ) -> None: # Given enable_features(EXPERIMENT_FLAG) - experiment.status = ExperimentStatus.RUNNING - experiment.save() # When - response = admin_client.patch( + admin_client.patch( _detail_url(environment, experiment), - data={"status": "completed"}, + data={"name": "Renamed"}, format="json", ) # Then - assert response.status_code == status.HTTP_200_OK - assert response.json()["ended_at"] is not None + audit = AuditLog.objects.filter( + related_object_type=RelatedObjectType.EXPERIMENT.name + ).last() + assert audit is not None + assert "updated" in audit.log -def test_delete__exists__returns_204_and_soft_deletes( +def test_action__start__creates_audit_log( admin_client: APIClient, environment: Environment, experiment: Experiment, @@ -522,58 +530,31 @@ def test_delete__exists__returns_204_and_soft_deletes( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.delete(_detail_url(environment, experiment)) + admin_client.post(_action_url(environment, experiment, "start")) # Then - assert response.status_code == status.HTTP_204_NO_CONTENT - assert not Experiment.objects.filter(id=experiment.id).exists() - assert Experiment.objects.all_with_deleted().filter(id=experiment.id).exists() + audit = AuditLog.objects.filter( + related_object_type=RelatedObjectType.EXPERIMENT.name + ).last() + assert audit is not None + assert "running" in audit.log -@pytest.mark.parametrize( - "method, action_label", - [ - ("post", "created"), - ("patch", "updated"), - ("delete", "deleted"), - ], -) -def test_crud__any_method__creates_audit_log( +def test_delete__valid_delete__creates_audit_log( admin_client: APIClient, environment: Environment, experiment: Experiment, - multivariate_feature: Feature, enable_features: EnableFeaturesFixture, - method: str, - action_label: str, ) -> None: # Given enable_features(EXPERIMENT_FLAG) # When - if method == "post": - experiment.delete() - admin_client.post( - _list_url(environment), - data={ - "feature": multivariate_feature.id, - "name": "Audit test", - "hypothesis": "Check audit", - }, - format="json", - ) - elif method == "patch": - admin_client.patch( - _detail_url(environment, experiment), - data={"name": "Renamed"}, - format="json", - ) - else: - admin_client.delete(_detail_url(environment, experiment)) + admin_client.delete(_detail_url(environment, experiment)) # Then audit = AuditLog.objects.filter( related_object_type=RelatedObjectType.EXPERIMENT.name ).last() assert audit is not None - assert action_label in audit.log + assert "deleted" in audit.log From 3e5a9e985d5cfe83ddd4381166714f0405b3d955 Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 27 May 2026 11:08:25 +0200 Subject: [PATCH 08/11] feat(experimentation): move transition logic to service layer --- api/experimentation/models.py | 20 ----------------- api/experimentation/services.py | 39 +++++++++++++++++++++++++++++++-- api/experimentation/views.py | 22 +++++-------------- 3 files changed, 43 insertions(+), 38 deletions(-) diff --git a/api/experimentation/models.py b/api/experimentation/models.py index bfe74338b652..c2c1f9712b1e 100644 --- a/api/experimentation/models.py +++ b/api/experimentation/models.py @@ -1,10 +1,8 @@ from django.db import models from django.db.models import Q -from django.utils import timezone from django_lifecycle import ( # type: ignore[import-untyped] AFTER_CREATE, AFTER_DELETE, - BEFORE_UPDATE, LifecycleModelMixin, hook, ) @@ -119,21 +117,3 @@ class Meta: name="unique_active_experiment_per_feature_env", ), ] - - @hook( - BEFORE_UPDATE, - when="status", - was=ExperimentStatus.CREATED, - is_now=ExperimentStatus.RUNNING, - ) # type: ignore[misc] - def set_started_at(self) -> None: - if not self.started_at: - self.started_at = timezone.now() - - @hook( - BEFORE_UPDATE, - when="status", - is_now=ExperimentStatus.COMPLETED, - ) # type: ignore[misc] - def set_ended_at(self) -> None: - self.ended_at = timezone.now() diff --git a/api/experimentation/services.py b/api/experimentation/services.py index 562361b38f0e..e3bb8e91b61d 100644 --- a/api/experimentation/services.py +++ b/api/experimentation/services.py @@ -2,6 +2,8 @@ import typing +from django.utils import timezone + from audit.models import AuditLog from audit.related_object_type import RelatedObjectType from experimentation.constants import EXPERIMENT_FLAG, WAREHOUSE_CONNECTION_FLAG @@ -29,6 +31,14 @@ def is_experiment_feature_enabled(organisation: Organisation) -> bool: ) +def _resolve_audit_log_author( + user: FFAdminUser, +) -> dict[str, int | None]: + if getattr(user, "is_master_api_key_user", False): + return {"author_id": None, "master_api_key_id": user.key.id} # type: ignore[union-attr] + return {"author_id": user.pk, "master_api_key_id": None} # type: ignore[union-attr] + + def create_warehouse_audit_log( connection: WarehouseConnection, user: FFAdminUser, @@ -38,7 +48,7 @@ def create_warehouse_audit_log( AuditLog.objects.create( environment=connection.environment, project=connection.environment.project, - author=user, + **_resolve_audit_log_author(user), related_object_id=connection.id, related_object_type=RelatedObjectType.WAREHOUSE_CONNECTION.name, log=( @@ -57,7 +67,7 @@ def create_experiment_audit_log( AuditLog.objects.create( environment=experiment.environment, project=experiment.environment.project, - author=user, + **_resolve_audit_log_author(user), related_object_id=experiment.id, related_object_type=RelatedObjectType.EXPERIMENT.name, log=( @@ -65,3 +75,28 @@ def create_experiment_audit_log( f"{experiment.environment.name}" ), ) + + +def transition_experiment_status( + experiment: Experiment, + target_status: str, + user: FFAdminUser, +) -> Experiment: + from experimentation.models import VALID_STATUS_TRANSITIONS, ExperimentStatus + + valid_targets = VALID_STATUS_TRANSITIONS.get(experiment.status, set()) + if target_status not in valid_targets: + raise ValueError( + f"Cannot transition from '{experiment.status}' to '{target_status}'." + ) + + experiment.status = target_status + + if target_status == ExperimentStatus.RUNNING and not experiment.started_at: + experiment.started_at = timezone.now() + elif target_status == ExperimentStatus.COMPLETED: + experiment.ended_at = timezone.now() + + experiment.save() + create_experiment_audit_log(experiment, user, action=target_status) + return experiment diff --git a/api/experimentation/views.py b/api/experimentation/views.py index b7f73cc0e406..346c24124823 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -10,7 +10,6 @@ from environments.views import NestedEnvironmentViewSet from experimentation.models import ( - VALID_STATUS_TRANSITIONS, Experiment, ExperimentStatus, WarehouseConnection, @@ -26,6 +25,7 @@ from experimentation.services import ( create_experiment_audit_log, create_warehouse_audit_log, + transition_experiment_status, ) from users.models import FFAdminUser @@ -170,22 +170,12 @@ def complete(self, request: Request, **kwargs: object) -> Response: def _transition_status(self, target_status: str) -> Response: experiment: Experiment = self.get_object() - valid_targets = VALID_STATUS_TRANSITIONS.get(experiment.status, set()) - if target_status not in valid_targets: - return Response( - { - "detail": ( - f"Cannot transition from '{experiment.status}' " - f"to '{target_status}'." - ) - }, - status=400, + try: + experiment = transition_experiment_status( + experiment, target_status, self._get_user(self.request) ) - experiment.status = target_status - experiment.save() - create_experiment_audit_log( - experiment, self._get_user(self.request), action=target_status - ) + except ValueError as exc: + return Response({"detail": str(exc)}, status=400) serializer = self.get_serializer(experiment) return Response(serializer.data) From 39b1a5d873f162b50f8ebe049e4d3793ca5fa1c5 Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 27 May 2026 11:09:27 +0200 Subject: [PATCH 09/11] feat(experimentation): use admin_client_new in tests --- .../experimentation/test_experiment_views.py | 124 ++++++++++-------- 1 file changed, 67 insertions(+), 57 deletions(-) diff --git a/api/tests/unit/experimentation/test_experiment_views.py b/api/tests/unit/experimentation/test_experiment_views.py index 1454e55873b3..4897a7c72062 100644 --- a/api/tests/unit/experimentation/test_experiment_views.py +++ b/api/tests/unit/experimentation/test_experiment_views.py @@ -45,7 +45,7 @@ def _action_url(environment: Environment, experiment: Experiment, action: str) - def test_post__valid_multivariate_feature__returns_201( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, multivariate_feature: Feature, enable_features: EnableFeaturesFixture, @@ -54,7 +54,7 @@ def test_post__valid_multivariate_feature__returns_201( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.post( + response = admin_client_new.post( _list_url(environment), data={ "feature": multivariate_feature.id, @@ -75,7 +75,7 @@ def test_post__valid_multivariate_feature__returns_201( def test_post__non_multivariate_feature__returns_400( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, feature: Feature, enable_features: EnableFeaturesFixture, @@ -84,7 +84,7 @@ def test_post__non_multivariate_feature__returns_400( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.post( + response = admin_client_new.post( _list_url(environment), data={ "feature": feature.id, @@ -99,7 +99,7 @@ def test_post__non_multivariate_feature__returns_400( def test_post__feature_from_different_project__returns_400( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, enable_features: EnableFeaturesFixture, organisation_one_project_one: Project, @@ -114,7 +114,7 @@ def test_post__feature_from_different_project__returns_400( ) # When - response = admin_client.post( + response = admin_client_new.post( _list_url(environment), data={ "feature": other_feature.id, @@ -129,7 +129,7 @@ def test_post__feature_from_different_project__returns_400( def test_post__active_experiment_exists__returns_409( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, multivariate_feature: Feature, @@ -139,7 +139,7 @@ def test_post__active_experiment_exists__returns_409( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.post( + response = admin_client_new.post( _list_url(environment), data={ "feature": multivariate_feature.id, @@ -154,7 +154,7 @@ def test_post__active_experiment_exists__returns_409( def test_post__completed_experiment_exists__returns_201( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, multivariate_feature: Feature, @@ -166,7 +166,7 @@ def test_post__completed_experiment_exists__returns_201( experiment.save() # When - response = admin_client.post( + response = admin_client_new.post( _list_url(environment), data={ "feature": multivariate_feature.id, @@ -180,29 +180,39 @@ def test_post__completed_experiment_exists__returns_201( assert response.status_code == status.HTTP_201_CREATED -@pytest.mark.parametrize( - "client_fixture, enable_flag, expected_status", - [ - ("staff_client", True, status.HTTP_403_FORBIDDEN), - ("admin_client", False, status.HTTP_403_FORBIDDEN), - ], -) -def test_post__insufficient_permissions__returns_403( - request: pytest.FixtureRequest, +def test_post__staff_user_with_flag__returns_403( + staff_client: APIClient, environment: Environment, multivariate_feature: Feature, enable_features: EnableFeaturesFixture, - client_fixture: str, - enable_flag: bool, - expected_status: int, ) -> None: # Given - api_client: APIClient = request.getfixturevalue(client_fixture) - if enable_flag: - enable_features(EXPERIMENT_FLAG) + enable_features(EXPERIMENT_FLAG) + + # When + response = staff_client.post( + _list_url(environment), + data={ + "feature": multivariate_feature.id, + "name": "No access", + "hypothesis": "Nope", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_post__admin_without_flag__returns_403( + admin_client_new: APIClient, + environment: Environment, + multivariate_feature: Feature, +) -> None: + # Given — feature flag not enabled # When - response = api_client.post( + response = admin_client_new.post( _list_url(environment), data={ "feature": multivariate_feature.id, @@ -213,11 +223,11 @@ def test_post__insufficient_permissions__returns_403( ) # Then - assert response.status_code == expected_status + assert response.status_code == status.HTTP_403_FORBIDDEN def test_post__nonexistent_environment__returns_403( - admin_client: APIClient, + admin_client_new: APIClient, enable_features: EnableFeaturesFixture, ) -> None: # Given @@ -228,7 +238,7 @@ def test_post__nonexistent_environment__returns_403( ) # When - response = admin_client.post( + response = admin_client_new.post( url, data={ "feature": 999, @@ -243,7 +253,7 @@ def test_post__nonexistent_environment__returns_403( def test_get_list__with_experiments__returns_all( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, enable_features: EnableFeaturesFixture, @@ -252,7 +262,7 @@ def test_get_list__with_experiments__returns_all( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.get(_list_url(environment)) + response = admin_client_new.get(_list_url(environment)) # Then assert response.status_code == status.HTTP_200_OK @@ -261,7 +271,7 @@ def test_get_list__with_experiments__returns_all( def test_get_list__empty__returns_200( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, enable_features: EnableFeaturesFixture, ) -> None: @@ -269,7 +279,7 @@ def test_get_list__empty__returns_200( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.get(_list_url(environment)) + response = admin_client_new.get(_list_url(environment)) # Then assert response.status_code == status.HTTP_200_OK @@ -286,7 +296,7 @@ def test_get_list__empty__returns_200( ], ) def test_get_list__filter_by_status__returns_filtered( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, enable_features: EnableFeaturesFixture, @@ -297,7 +307,7 @@ def test_get_list__filter_by_status__returns_filtered( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.get(_list_url(environment), {"status": filter_status}) + response = admin_client_new.get(_list_url(environment), {"status": filter_status}) # Then assert response.status_code == status.HTTP_200_OK @@ -305,7 +315,7 @@ def test_get_list__filter_by_status__returns_filtered( def test_get_detail__exists__returns_200( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, enable_features: EnableFeaturesFixture, @@ -314,7 +324,7 @@ def test_get_detail__exists__returns_200( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.get(_detail_url(environment, experiment)) + response = admin_client_new.get(_detail_url(environment, experiment)) # Then assert response.status_code == status.HTTP_200_OK @@ -329,7 +339,7 @@ def test_get_detail__exists__returns_200( ], ) def test_patch__update_field__returns_200( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, enable_features: EnableFeaturesFixture, @@ -340,7 +350,7 @@ def test_patch__update_field__returns_200( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.patch( + response = admin_client_new.patch( _detail_url(environment, experiment), data={field: value}, format="json", @@ -352,7 +362,7 @@ def test_patch__update_field__returns_200( def test_patch__change_feature__returns_400( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, project: Project, @@ -368,7 +378,7 @@ def test_patch__change_feature__returns_400( ) # When - response = admin_client.patch( + response = admin_client_new.patch( _detail_url(environment, experiment), data={"feature": other_feature.id}, format="json", @@ -394,7 +404,7 @@ def test_patch__change_feature__returns_400( ], ) def test_action__status_transition__returns_expected( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, enable_features: EnableFeaturesFixture, @@ -408,14 +418,14 @@ def test_action__status_transition__returns_expected( experiment.save() # When - response = admin_client.post(_action_url(environment, experiment, action_name)) + response = admin_client_new.post(_action_url(environment, experiment, action_name)) # Then assert response.status_code == expected_status_code def test_action__start__sets_started_at( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, enable_features: EnableFeaturesFixture, @@ -424,7 +434,7 @@ def test_action__start__sets_started_at( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.post(_action_url(environment, experiment, "start")) + response = admin_client_new.post(_action_url(environment, experiment, "start")) # Then assert response.status_code == status.HTTP_200_OK @@ -432,7 +442,7 @@ def test_action__start__sets_started_at( def test_action__complete__sets_ended_at( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, enable_features: EnableFeaturesFixture, @@ -443,7 +453,7 @@ def test_action__complete__sets_ended_at( experiment.save() # When - response = admin_client.post(_action_url(environment, experiment, "complete")) + response = admin_client_new.post(_action_url(environment, experiment, "complete")) # Then assert response.status_code == status.HTTP_200_OK @@ -451,7 +461,7 @@ def test_action__complete__sets_ended_at( def test_delete__exists__returns_204_and_soft_deletes( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, enable_features: EnableFeaturesFixture, @@ -460,7 +470,7 @@ def test_delete__exists__returns_204_and_soft_deletes( enable_features(EXPERIMENT_FLAG) # When - response = admin_client.delete(_detail_url(environment, experiment)) + response = admin_client_new.delete(_detail_url(environment, experiment)) # Then assert response.status_code == status.HTTP_204_NO_CONTENT @@ -469,7 +479,7 @@ def test_delete__exists__returns_204_and_soft_deletes( def test_post__valid_create__creates_audit_log( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, multivariate_feature: Feature, enable_features: EnableFeaturesFixture, @@ -478,7 +488,7 @@ def test_post__valid_create__creates_audit_log( enable_features(EXPERIMENT_FLAG) # When - admin_client.post( + admin_client_new.post( _list_url(environment), data={ "feature": multivariate_feature.id, @@ -497,7 +507,7 @@ def test_post__valid_create__creates_audit_log( def test_patch__valid_update__creates_audit_log( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, enable_features: EnableFeaturesFixture, @@ -506,7 +516,7 @@ def test_patch__valid_update__creates_audit_log( enable_features(EXPERIMENT_FLAG) # When - admin_client.patch( + admin_client_new.patch( _detail_url(environment, experiment), data={"name": "Renamed"}, format="json", @@ -521,7 +531,7 @@ def test_patch__valid_update__creates_audit_log( def test_action__start__creates_audit_log( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, enable_features: EnableFeaturesFixture, @@ -530,7 +540,7 @@ def test_action__start__creates_audit_log( enable_features(EXPERIMENT_FLAG) # When - admin_client.post(_action_url(environment, experiment, "start")) + admin_client_new.post(_action_url(environment, experiment, "start")) # Then audit = AuditLog.objects.filter( @@ -541,7 +551,7 @@ def test_action__start__creates_audit_log( def test_delete__valid_delete__creates_audit_log( - admin_client: APIClient, + admin_client_new: APIClient, environment: Environment, experiment: Experiment, enable_features: EnableFeaturesFixture, @@ -550,7 +560,7 @@ def test_delete__valid_delete__creates_audit_log( enable_features(EXPERIMENT_FLAG) # When - admin_client.delete(_detail_url(environment, experiment)) + admin_client_new.delete(_detail_url(environment, experiment)) # Then audit = AuditLog.objects.filter( From d560403959c38ee6e3a746652192eb93cf326245 Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 27 May 2026 11:20:09 +0200 Subject: [PATCH 10/11] feat(experimentation): leaked pii and misc review feedback --- api/experimentation/views.py | 28 +++++++++++++------ .../experimentation/test_experiment_views.py | 3 +- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/api/experimentation/views.py b/api/experimentation/views.py index 346c24124823..ef5718b0a6b0 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -1,7 +1,8 @@ +import logging from typing import Any from django.db.models import QuerySet -from rest_framework import mixins +from rest_framework import mixins, status from rest_framework.decorators import action from rest_framework.permissions import IsAuthenticated from rest_framework.request import Request @@ -29,6 +30,8 @@ ) from users.models import FFAdminUser +logger = logging.getLogger(__name__) + class WarehouseConnectionViewSet( NestedEnvironmentViewSet[WarehouseConnection], @@ -77,11 +80,11 @@ def create(self, request: Request, *args: object, **kwargs: object) -> Response: { "detail": "This environment already has an active warehouse connection." }, - status=409, + status=status.HTTP_409_CONFLICT, ) self.perform_create(serializer) - return Response(serializer.data, status=201) + return Response(serializer.data, status=status.HTTP_201_CREATED) @staticmethod def _get_user(request: Request) -> FFAdminUser: @@ -102,7 +105,6 @@ class ExperimentViewSet( model_class = Experiment lookup_field = "id" lookup_url_kwarg = "experiment_id" - filterset_fields: list[str] = [] def get_serializer_context(self) -> dict[str, Any]: context = super().get_serializer_context() @@ -132,11 +134,11 @@ def create(self, request: Request, *args: object, **kwargs: object) -> Response: ): return Response( {"detail": "An active experiment already exists for this feature."}, - status=409, + status=status.HTTP_409_CONFLICT, ) self.perform_create(serializer) - return Response(serializer.data, status=201) + return Response(serializer.data, status=status.HTTP_201_CREATED) def perform_create(self, serializer: BaseSerializer[Experiment]) -> None: experiment: Experiment = serializer.save(environment=self._get_environment()) @@ -174,8 +176,18 @@ def _transition_status(self, target_status: str) -> Response: experiment = transition_experiment_status( experiment, target_status, self._get_user(self.request) ) - except ValueError as exc: - return Response({"detail": str(exc)}, status=400) + except ValueError: + logger.warning( + "Invalid experiment status transition for " + "experiment_id=%s to status=%s", + experiment.id, + target_status, + exc_info=True, + ) + return Response( + {"detail": "Unable to transition experiment status."}, + status=status.HTTP_400_BAD_REQUEST, + ) serializer = self.get_serializer(experiment) return Response(serializer.data) diff --git a/api/tests/unit/experimentation/test_experiment_views.py b/api/tests/unit/experimentation/test_experiment_views.py index 4897a7c72062..4dbc47684be7 100644 --- a/api/tests/unit/experimentation/test_experiment_views.py +++ b/api/tests/unit/experimentation/test_experiment_views.py @@ -10,6 +10,7 @@ from audit.models import AuditLog from audit.related_object_type import RelatedObjectType from environments.models import Environment +from experimentation.constants import EXPERIMENT_FLAG from experimentation.models import Experiment, ExperimentStatus from features.feature_types import MULTIVARIATE from features.models import Feature @@ -20,8 +21,6 @@ pytestmark = pytest.mark.django_db -EXPERIMENT_FLAG = "experimental_flags" - def _list_url(environment: Environment) -> str: return reverse( From 421a070b853f6b2281e54c2fc88a4a71783e5b16 Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 27 May 2026 11:31:45 +0200 Subject: [PATCH 11/11] feat(experimentation): type lint --- api/experimentation/services.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/experimentation/services.py b/api/experimentation/services.py index e3bb8e91b61d..13b5e312a59b 100644 --- a/api/experimentation/services.py +++ b/api/experimentation/services.py @@ -35,8 +35,8 @@ def _resolve_audit_log_author( user: FFAdminUser, ) -> dict[str, int | None]: if getattr(user, "is_master_api_key_user", False): - return {"author_id": None, "master_api_key_id": user.key.id} # type: ignore[union-attr] - return {"author_id": user.pk, "master_api_key_id": None} # type: ignore[union-attr] + return {"author_id": None, "master_api_key_id": user.key.id} + return {"author_id": user.pk, "master_api_key_id": None} def create_warehouse_audit_log(