From cb83a16c74c6be36d1587b5eda17558e83cd9401 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 29 Oct 2024 11:30:28 +0000 Subject: [PATCH 1/5] feat(experiments): Add saved metrics --- .../views/experiment_saved_metrics.py | 63 +++++++++ ee/clickhouse/views/experiments.py | 68 ++++++++- .../test/test_experiment_saved_metrics.py | 131 ++++++++++++++++++ latest_migrations.manifest | 2 +- posthog/api/__init__.py | 4 + ...metric_experimenttosavedmetric_and_more.py | 57 ++++++++ posthog/models/experiment.py | 27 ++++ 7 files changed, 350 insertions(+), 2 deletions(-) create mode 100644 ee/clickhouse/views/experiment_saved_metrics.py create mode 100644 ee/clickhouse/views/test/test_experiment_saved_metrics.py create mode 100644 posthog/migrations/0503_experimentsavedmetric_experimenttosavedmetric_and_more.py diff --git a/ee/clickhouse/views/experiment_saved_metrics.py b/ee/clickhouse/views/experiment_saved_metrics.py new file mode 100644 index 0000000000000..0e8d40c0e9243 --- /dev/null +++ b/ee/clickhouse/views/experiment_saved_metrics.py @@ -0,0 +1,63 @@ +from rest_framework import serializers, viewsets +from rest_framework.exceptions import ValidationError + + +from posthog.api.routing import TeamAndOrgViewSetMixin +from posthog.api.shared import UserBasicSerializer +from posthog.models.experiment import ExperimentSavedMetric, ExperimentToSavedMetric + + +class ExperimentToSavedMetricSerializer(serializers.ModelSerializer): + class Meta: + model = ExperimentToSavedMetric + fields = [ + "id", + "experiment", + "saved_metric", + "metadata", + "created_at", + ] + read_only_fields = [ + "id", + "created_at", + ] + + +class ExperimentSavedMetricSerializer(serializers.ModelSerializer): + created_by = UserBasicSerializer(read_only=True) + + class Meta: + model = ExperimentSavedMetric + fields = [ + "id", + "name", + "description", + "query", + "created_by", + "created_at", + "updated_at", + ] + read_only_fields = [ + "id", + "created_by", + "created_at", + "updated_at", + ] + + def validate_query(self, value): + if not value: + raise ValidationError("Query is required to create a saved metric") + return value + + def create(self, validated_data): + request = self.context["request"] + validated_data["created_by"] = request.user + validated_data["team_id"] = self.context["team_id"] + return super().create(validated_data) + + +class ExperimentSavedMetricViewSet(TeamAndOrgViewSetMixin, viewsets.ModelViewSet): + scope_object = "experiment" + queryset = ExperimentSavedMetric.objects.prefetch_related("created_by").all() + serializer_class = ExperimentSavedMetricSerializer + ordering = "-created_at" diff --git a/ee/clickhouse/views/experiments.py b/ee/clickhouse/views/experiments.py index 6df24dc012cea..a430329276de0 100644 --- a/ee/clickhouse/views/experiments.py +++ b/ee/clickhouse/views/experiments.py @@ -20,6 +20,7 @@ ) from ee.clickhouse.queries.experiments.utils import requires_flag_warning from ee.clickhouse.views.experiment_holdouts import ExperimentHoldoutSerializer +from ee.clickhouse.views.experiment_saved_metrics import ExperimentToSavedMetricSerializer from posthog.api.cohort import CohortSerializer from posthog.api.feature_flag import FeatureFlagSerializer, MinimalFeatureFlagSerializer from posthog.api.routing import TeamAndOrgViewSetMixin @@ -28,7 +29,7 @@ from posthog.caching.insight_cache import update_cached_state from posthog.clickhouse.query_tagging import tag_queries from posthog.constants import INSIGHT_TRENDS -from posthog.models.experiment import Experiment, ExperimentHoldout +from posthog.models.experiment import Experiment, ExperimentHoldout, ExperimentSavedMetric from posthog.models.filters.filter import Filter from posthog.utils import generate_cache_key, get_safe_cache @@ -163,6 +164,8 @@ class ExperimentSerializer(serializers.ModelSerializer): holdout_id = serializers.PrimaryKeyRelatedField( queryset=ExperimentHoldout.objects.all(), source="holdout", required=False, allow_null=True ) + saved_metrics = ExperimentToSavedMetricSerializer(many=True, source="experimenttosavedmetric_set", read_only=True) + saved_metrics_ids = serializers.ListField(child=serializers.JSONField(), write_only=True, required=False) class Meta: model = Experiment @@ -179,6 +182,8 @@ class Meta: "exposure_cohort", "parameters", "secondary_metrics", + "saved_metrics", + "saved_metrics_ids", "filters", "archived", "created_by", @@ -193,8 +198,34 @@ class Meta: "feature_flag", "exposure_cohort", "holdout", + "saved_metrics", ] + def validate_saved_metrics_ids(self, value): + # check value is valid json list with id and optionally metadata param + if not isinstance(value, list): + raise ValidationError("Saved metrics must be a list") + + for saved_metric in value: + if not isinstance(saved_metric, dict): + raise ValidationError("Saved metric must be an object") + if "id" not in saved_metric: + raise ValidationError("Saved metric must have an id") + if "metadata" in saved_metric and not isinstance(saved_metric["metadata"], dict): + raise ValidationError("Metadata must be an object") + + # metadata is optional, but if it exists, should have type key + # TODO: extend with other metadata keys when known + if "metadata" in saved_metric and "type" not in saved_metric["metadata"]: + raise ValidationError("Metadata must have a type key") + + # check if all saved metrics exist + saved_metrics = ExperimentSavedMetric.objects.filter(id__in=[saved_metric["id"] for saved_metric in value]) + if saved_metrics.count() != len(value): + raise ValidationError("Saved metric does not exist") + + return value + def validate_parameters(self, value): if not value: return value @@ -213,6 +244,8 @@ def create(self, validated_data: dict, *args: Any, **kwargs: Any) -> Experiment: if not validated_data.get("filters"): raise ValidationError("Filters are required to create an Experiment") + saved_metrics_data = validated_data.pop("saved_metrics_ids", []) + variants = [] aggregation_group_type_index = None if validated_data["parameters"]: @@ -263,9 +296,42 @@ def create(self, validated_data: dict, *args: Any, **kwargs: Any) -> Experiment: experiment = Experiment.objects.create( team_id=self.context["team_id"], feature_flag=feature_flag, **validated_data ) + + if saved_metrics_data: + for saved_metric_data in saved_metrics_data: + saved_metric_serializer = ExperimentToSavedMetricSerializer( + data={ + "experiment": experiment.id, + "saved_metric": saved_metric_data["id"], + "metadata": saved_metric_data.get("metadata"), + }, + context=self.context, + ) + saved_metric_serializer.is_valid(raise_exception=True) + saved_metric_serializer.save() + # TODO: Going the above route means we can still sometimes fail when validation fails? + # But this shouldn't really happen, if it does its a bug in our validation logic (validate_saved_metrics_ids) return experiment def update(self, instance: Experiment, validated_data: dict, *args: Any, **kwargs: Any) -> Experiment: + update_saved_metrics = "saved_metrics" in validated_data + saved_metrics_data = validated_data.pop("saved_metrics_ids", []) or [] + + # We replace all saved metrics on update to avoid issues with partial updates + if update_saved_metrics: + instance.experimenttosavedmetric_set.all().delete() + for saved_metric_data in saved_metrics_data: + saved_metric_serializer = ExperimentToSavedMetricSerializer( + data={ + "experiment": instance.id, + "saved_metric": saved_metric_data["id"], + "metadata": saved_metric_data.get("metadata"), + }, + context=self.context, + ) + saved_metric_serializer.is_valid(raise_exception=True) + saved_metric_serializer.save() + has_start_date = validated_data.get("start_date") is not None feature_flag = instance.feature_flag diff --git a/ee/clickhouse/views/test/test_experiment_saved_metrics.py b/ee/clickhouse/views/test/test_experiment_saved_metrics.py new file mode 100644 index 0000000000000..79750d75745d3 --- /dev/null +++ b/ee/clickhouse/views/test/test_experiment_saved_metrics.py @@ -0,0 +1,131 @@ +from rest_framework import status + +from ee.api.test.base import APILicensedTest +from posthog.models.experiment import Experiment, ExperimentToSavedMetric + + +class TestExperimentSavedMetricsCRUD(APILicensedTest): + def test_can_list_experiment_saved_metrics(self): + response = self.client.get(f"/api/projects/{self.team.id}/experiment_saved_metrics/") + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_create_update_experiment_saved_metrics(self) -> None: + response = self.client.post( + f"/api/projects/{self.team.id}/experiment_saved_metrics/", + data={ + "name": "Test Experiment saved metric", + "description": "Test description", + "query": {"events": [{"id": "$pageview"}]}, + }, + format="json", + ) + + saved_metric_id = response.json()["id"] + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertEqual(response.json()["name"], "Test Experiment saved metric") + self.assertEqual(response.json()["description"], "Test description") + self.assertEqual( + response.json()["query"], + {"events": [{"id": "$pageview"}]}, + ) + self.assertEqual(response.json()["created_by"]["id"], self.user.pk) + + # Generate experiment to have saved metric + ff_key = "a-b-tests" + response = self.client.post( + f"/api/projects/{self.team.id}/experiments/", + { + "name": "Test Experiment", + "description": "", + "start_date": "2021-12-01T10:23", + "end_date": None, + "feature_flag_key": ff_key, + "parameters": None, + "filters": { + "events": [ + {"order": 0, "id": "$pageview"}, + {"order": 1, "id": "$pageleave"}, + ], + "properties": [], + }, + "saved_metrics_ids": [{"id": saved_metric_id, "metadata": {"type": "secondary"}}], + }, + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + + exp_id = response.json()["id"] + + self.assertEqual(response.json()["name"], "Test Experiment") + self.assertEqual(response.json()["feature_flag_key"], ff_key) + + self.assertEqual(Experiment.objects.get(pk=exp_id).saved_metrics.count(), 1) + experiment_to_saved_metric = Experiment.objects.get(pk=exp_id).experimenttosavedmetric_set.first() + self.assertEqual(experiment_to_saved_metric.metadata, {"type": "secondary"}) + saved_metric = Experiment.objects.get(pk=exp_id).saved_metrics.first() + self.assertEqual(saved_metric.id, saved_metric_id) + self.assertEqual(saved_metric.query, {"events": [{"id": "$pageview"}]}) + + # Now try updating saved metric + response = self.client.patch( + f"/api/projects/{self.team.id}/experiment_saved_metrics/{saved_metric_id}", + { + "name": "Test Experiment saved metric 2", + "description": "Test description 2", + "query": {"events": [{"id": "$pageleave"}]}, + }, + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.json()["name"], "Test Experiment saved metric 2") + self.assertEqual( + response.json()["query"], + {"events": [{"id": "$pageleave"}]}, + ) + + # make sure experiment in question was updated as well + self.assertEqual(Experiment.objects.get(pk=exp_id).saved_metrics.count(), 1) + saved_metric = Experiment.objects.get(pk=exp_id).saved_metrics.first() + self.assertEqual(saved_metric.id, saved_metric_id) + self.assertEqual( + saved_metric.query, + { + "events": [ + {"id": "$pageleave"}, + ] + }, + ) + self.assertEqual(saved_metric.name, "Test Experiment saved metric 2") + self.assertEqual(saved_metric.description, "Test description 2") + + # now delete saved metric + response = self.client.delete(f"/api/projects/{self.team.id}/experiment_saved_metrics/{saved_metric_id}") + self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + + # make sure experiment in question was updated as well + self.assertEqual(Experiment.objects.get(pk=exp_id).saved_metrics.count(), 0) + self.assertEqual(ExperimentToSavedMetric.objects.filter(experiment_id=exp_id).count(), 0) + + def test_invalid_create(self): + response = self.client.post( + f"/api/projects/{self.team.id}/experiment_saved_metrics/", + data={ + "name": None, # invalid + "query": {"events": [{"id": "$pageview"}]}, + }, + format="json", + ) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.json()["detail"], "This field may not be null.") + + response = self.client.post( + f"/api/projects/{self.team.id}/experiment_saved_metrics/", + data={ + "name": "xyz", + "query": {}, + }, + format="json", + ) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.json()["detail"], "Query is required to create a saved metric") diff --git a/latest_migrations.manifest b/latest_migrations.manifest index 90046f4109b84..cc7b583fa386e 100644 --- a/latest_migrations.manifest +++ b/latest_migrations.manifest @@ -5,7 +5,7 @@ contenttypes: 0002_remove_content_type_name ee: 0016_rolemembership_organization_member otp_static: 0002_throttling otp_totp: 0002_auto_20190420_0723 -posthog: 0502_team_session_recording_url_blocklist_config +posthog: 0503_experimentsavedmetric_experimenttosavedmetric_and_more sessions: 0001_initial social_django: 0010_uid_db_index two_factor: 0007_auto_20201201_1019 diff --git a/posthog/api/__init__.py b/posthog/api/__init__.py index 06c97742c3a27..ad341edcfa5b0 100644 --- a/posthog/api/__init__.py +++ b/posthog/api/__init__.py @@ -412,6 +412,7 @@ def register_grandfathered_environment_nested_viewset( if EE_AVAILABLE: from ee.clickhouse.views.experiments import EnterpriseExperimentsViewSet from ee.clickhouse.views.experiment_holdouts import ExperimentHoldoutViewSet + from ee.clickhouse.views.experiment_saved_metrics import ExperimentSavedMetricViewSet from ee.clickhouse.views.groups import GroupsTypesViewSet, GroupsViewSet from ee.clickhouse.views.insights import EnterpriseInsightsViewSet from ee.clickhouse.views.person import EnterprisePersonViewSet, LegacyEnterprisePersonViewSet @@ -420,6 +421,9 @@ def register_grandfathered_environment_nested_viewset( projects_router.register( r"experiment_holdouts", ExperimentHoldoutViewSet, "project_experiment_holdouts", ["project_id"] ) + projects_router.register( + r"experiment_saved_metrics", ExperimentSavedMetricViewSet, "project_experiment_saved_metrics", ["project_id"] + ) register_grandfathered_environment_nested_viewset(r"groups", GroupsViewSet, "environment_groups", ["team_id"]) projects_router.register(r"groups_types", GroupsTypesViewSet, "project_groups_types", ["project_id"]) environment_insights_router, legacy_project_insights_router = register_grandfathered_environment_nested_viewset( diff --git a/posthog/migrations/0503_experimentsavedmetric_experimenttosavedmetric_and_more.py b/posthog/migrations/0503_experimentsavedmetric_experimenttosavedmetric_and_more.py new file mode 100644 index 0000000000000..8c8e18739eeb9 --- /dev/null +++ b/posthog/migrations/0503_experimentsavedmetric_experimenttosavedmetric_and_more.py @@ -0,0 +1,57 @@ +# Generated by Django 4.2.15 on 2024-10-29 10:54 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import django.utils.timezone + + +class Migration(migrations.Migration): + dependencies = [ + ("posthog", "0502_team_session_recording_url_blocklist_config"), + ] + + operations = [ + migrations.CreateModel( + name="ExperimentSavedMetric", + fields=[ + ("id", models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), + ("name", models.CharField(max_length=400)), + ("description", models.CharField(blank=True, max_length=400, null=True)), + ("query", models.JSONField()), + ("created_at", models.DateTimeField(default=django.utils.timezone.now)), + ("updated_at", models.DateTimeField(auto_now=True)), + ( + "created_by", + models.ForeignKey( + null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL + ), + ), + ("team", models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="posthog.team")), + ], + ), + migrations.CreateModel( + name="ExperimentToSavedMetric", + fields=[ + ("id", models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), + ("metadata", models.JSONField(default=dict)), + ("created_at", models.DateTimeField(default=django.utils.timezone.now)), + ("updated_at", models.DateTimeField(auto_now=True)), + ("experiment", models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="posthog.experiment")), + ( + "saved_metric", + models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="posthog.experimentsavedmetric"), + ), + ], + ), + migrations.AddField( + model_name="experiment", + name="saved_metrics", + field=models.ManyToManyField( + blank=True, + related_name="experiments", + through="posthog.ExperimentToSavedMetric", + to="posthog.experimentsavedmetric", + ), + ), + ] diff --git a/posthog/models/experiment.py b/posthog/models/experiment.py index 2266982282892..8f25480182887 100644 --- a/posthog/models/experiment.py +++ b/posthog/models/experiment.py @@ -41,6 +41,9 @@ class ExperimentType(models.TextChoices): variants = models.JSONField(default=dict, null=True, blank=True) metrics = models.JSONField(default=list, null=True, blank=True) + saved_metrics = models.ManyToManyField( + "ExperimentSavedMetric", blank=True, related_name="experiments", through="ExperimentToSavedMetric" + ) def get_feature_flag_key(self): return self.feature_flag.key @@ -62,3 +65,27 @@ class ExperimentHoldout(models.Model): created_by = models.ForeignKey("User", on_delete=models.SET_NULL, null=True) created_at = models.DateTimeField(default=timezone.now) updated_at = models.DateTimeField(auto_now=True) + + +class ExperimentSavedMetric(models.Model): + name = models.CharField(max_length=400) + description = models.CharField(max_length=400, null=True, blank=True) + team = models.ForeignKey("Team", on_delete=models.CASCADE) + + query = models.JSONField() + + created_by = models.ForeignKey("User", on_delete=models.SET_NULL, null=True) + created_at = models.DateTimeField(default=timezone.now) + updated_at = models.DateTimeField(auto_now=True) + + +class ExperimentToSavedMetric(models.Model): + experiment = models.ForeignKey("Experiment", on_delete=models.CASCADE) + saved_metric = models.ForeignKey("ExperimentSavedMetric", on_delete=models.CASCADE) + + # Metadata for the saved metric at the time of the experiment creation + # has stuff like whether this metric is primary, and any other information + # we need for the metric, other than the query. + metadata = models.JSONField(default=dict) + created_at = models.DateTimeField(default=timezone.now) + updated_at = models.DateTimeField(auto_now=True) From 3a254fdb532773ba15c07ef2b281bd8093bb32be Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 29 Oct 2024 11:47:14 +0000 Subject: [PATCH 2/5] Update query snapshots --- posthog/api/test/__snapshots__/test_api_docs.ambr | 1 + 1 file changed, 1 insertion(+) diff --git a/posthog/api/test/__snapshots__/test_api_docs.ambr b/posthog/api/test/__snapshots__/test_api_docs.ambr index a5f9b394809ae..9fb5a304c8102 100644 --- a/posthog/api/test/__snapshots__/test_api_docs.ambr +++ b/posthog/api/test/__snapshots__/test_api_docs.ambr @@ -84,6 +84,7 @@ "/home/runner/work/posthog/posthog/posthog/api/event_definition.py: Error [EventDefinitionViewSet]: exception raised while getting serializer. Hint: Is get_serializer_class() returning None or is get_queryset() not working without a request? Ignoring the view for now. (Exception: 'AnonymousUser' object has no attribute 'organization')", '/home/runner/work/posthog/posthog/posthog/api/event_definition.py: Warning [EventDefinitionViewSet]: could not derive type of path parameter "project_id" because model "posthog.models.event_definition.EventDefinition" contained no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".', '/home/runner/work/posthog/posthog/ee/clickhouse/views/experiment_holdouts.py: Warning [ExperimentHoldoutViewSet]: could not derive type of path parameter "project_id" because model "posthog.models.experiment.ExperimentHoldout" contained no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".', + '/home/runner/work/posthog/posthog/ee/clickhouse/views/experiment_saved_metrics.py: Warning [ExperimentSavedMetricViewSet]: could not derive type of path parameter "project_id" because model "posthog.models.experiment.ExperimentSavedMetric" contained no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".', '/home/runner/work/posthog/posthog/ee/clickhouse/views/experiments.py: Warning [EnterpriseExperimentsViewSet]: could not derive type of path parameter "project_id" because model "posthog.models.experiment.Experiment" contained no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".', '/home/runner/work/posthog/posthog/posthog/api/feature_flag.py: Warning [FeatureFlagViewSet]: could not derive type of path parameter "project_id" because model "posthog.models.feature_flag.feature_flag.FeatureFlag" contained no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".', '/home/runner/work/posthog/posthog/ee/api/feature_flag_role_access.py: Warning [FeatureFlagRoleAccessViewSet]: could not derive type of path parameter "project_id" because model "ee.models.feature_flag_role_access.FeatureFlagRoleAccess" contained no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".', From 34d09f664b8d87a3e69807d420fc0fff0b5305a4 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Wed, 30 Oct 2024 12:16:50 +0000 Subject: [PATCH 3/5] all sorted --- ee/clickhouse/views/experiments.py | 11 +- .../views/test/test_clickhouse_experiments.py | 254 +++++++++++++++++- posthog/models/experiment.py | 5 +- 3 files changed, 264 insertions(+), 6 deletions(-) diff --git a/ee/clickhouse/views/experiments.py b/ee/clickhouse/views/experiments.py index a430329276de0..324716dce6a63 100644 --- a/ee/clickhouse/views/experiments.py +++ b/ee/clickhouse/views/experiments.py @@ -165,7 +165,9 @@ class ExperimentSerializer(serializers.ModelSerializer): queryset=ExperimentHoldout.objects.all(), source="holdout", required=False, allow_null=True ) saved_metrics = ExperimentToSavedMetricSerializer(many=True, source="experimenttosavedmetric_set", read_only=True) - saved_metrics_ids = serializers.ListField(child=serializers.JSONField(), write_only=True, required=False) + saved_metrics_ids = serializers.ListField( + child=serializers.JSONField(), write_only=True, required=False, allow_null=True + ) class Meta: model = Experiment @@ -202,6 +204,9 @@ class Meta: ] def validate_saved_metrics_ids(self, value): + if value is None: + return value + # check value is valid json list with id and optionally metadata param if not isinstance(value, list): raise ValidationError("Saved metrics must be a list") @@ -314,7 +319,7 @@ def create(self, validated_data: dict, *args: Any, **kwargs: Any) -> Experiment: return experiment def update(self, instance: Experiment, validated_data: dict, *args: Any, **kwargs: Any) -> Experiment: - update_saved_metrics = "saved_metrics" in validated_data + update_saved_metrics = "saved_metrics_ids" in validated_data saved_metrics_data = validated_data.pop("saved_metrics_ids", []) or [] # We replace all saved metrics on update to avoid issues with partial updates @@ -436,7 +441,7 @@ def update(self, instance: Experiment, validated_data: dict, *args: Any, **kwarg class EnterpriseExperimentsViewSet(TeamAndOrgViewSetMixin, viewsets.ModelViewSet): scope_object = "experiment" serializer_class = ExperimentSerializer - queryset = Experiment.objects.prefetch_related("feature_flag", "created_by", "holdout").all() + queryset = Experiment.objects.prefetch_related("feature_flag", "created_by", "holdout", "saved_metrics").all() ordering = "-created_at" # ****************************************** diff --git a/ee/clickhouse/views/test/test_clickhouse_experiments.py b/ee/clickhouse/views/test/test_clickhouse_experiments.py index 7377c76eaef5a..e804af4a36d85 100644 --- a/ee/clickhouse/views/test/test_clickhouse_experiments.py +++ b/ee/clickhouse/views/test/test_clickhouse_experiments.py @@ -55,7 +55,7 @@ def test_getting_experiments_is_not_nplus1(self) -> None: format="json", ).json() - with self.assertNumQueries(FuzzyInt(8, 9)): + with self.assertNumQueries(FuzzyInt(9, 10)): response = self.client.get(f"/api/projects/{self.team.id}/experiments") self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -72,7 +72,7 @@ def test_getting_experiments_is_not_nplus1(self) -> None: format="json", ).json() - with self.assertNumQueries(FuzzyInt(8, 9)): + with self.assertNumQueries(FuzzyInt(9, 10)): response = self.client.get(f"/api/projects/{self.team.id}/experiments") self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -309,6 +309,256 @@ def test_transferring_holdout_to_another_group(self): [{"properties": [], "rollout_percentage": 5, "variant": f"holdout-{holdout_2_id}"}], ) + def test_saved_metrics(self): + response = self.client.post( + f"/api/projects/{self.team.id}/experiment_saved_metrics/", + { + "name": "Test Experiment saved metric", + "description": "Test description", + "query": {"events": [{"id": "$pageview"}]}, + }, + ) + + saved_metric_id = response.json()["id"] + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertEqual(response.json()["name"], "Test Experiment saved metric") + self.assertEqual(response.json()["description"], "Test description") + self.assertEqual( + response.json()["query"], + { + "events": [{"id": "$pageview"}], + }, + ) + self.assertEqual(response.json()["created_by"]["id"], self.user.pk) + + # Generate experiment to have saved metric + ff_key = "a-b-tests" + response = self.client.post( + f"/api/projects/{self.team.id}/experiments/", + { + "name": "Test Experiment", + "description": "", + "start_date": "2021-12-01T10:23", + "end_date": None, + "feature_flag_key": ff_key, + "parameters": None, + "filters": { + "events": [ + {"order": 0, "id": "$pageview"}, + {"order": 1, "id": "$pageleave"}, + ], + "properties": [], + }, + "saved_metrics_ids": [{"id": saved_metric_id, "metadata": {"type": "secondary"}}], + }, + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + + exp_id = response.json()["id"] + + self.assertEqual(response.json()["name"], "Test Experiment") + self.assertEqual(response.json()["feature_flag_key"], ff_key) + + self.assertEqual(Experiment.objects.get(pk=exp_id).saved_metrics.count(), 1) + experiment_to_saved_metric = Experiment.objects.get(pk=exp_id).experimenttosavedmetric_set.first() + self.assertEqual(experiment_to_saved_metric.metadata, {"type": "secondary"}) + saved_metric = Experiment.objects.get(pk=exp_id).saved_metrics.first() + self.assertEqual(saved_metric.id, saved_metric_id) + self.assertEqual(saved_metric.query, {"events": [{"id": "$pageview"}]}) + + # Now try updating experiment with new saved metric + response = self.client.post( + f"/api/projects/{self.team.id}/experiment_saved_metrics/", + { + "name": "Test Experiment saved metric 2", + "description": "Test description 2", + "query": { + "events": [{"id": "$pageleave"}], + }, + }, + ) + + saved_metric_2_id = response.json()["id"] + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertEqual(response.json()["name"], "Test Experiment saved metric 2") + + response = self.client.patch( + f"/api/projects/{self.team.id}/experiments/{exp_id}", + { + "saved_metrics_ids": [ + {"id": saved_metric_id, "metadata": {"type": "secondary"}}, + {"id": saved_metric_2_id, "metadata": {"type": "tertiary"}}, + ] + }, + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + + self.assertEqual(Experiment.objects.get(pk=exp_id).saved_metrics.count(), 2) + experiment_to_saved_metric = Experiment.objects.get(pk=exp_id).experimenttosavedmetric_set.all() + self.assertEqual(experiment_to_saved_metric[0].metadata, {"type": "secondary"}) + self.assertEqual(experiment_to_saved_metric[1].metadata, {"type": "tertiary"}) + saved_metric = Experiment.objects.get(pk=exp_id).saved_metrics.all() + self.assertEqual(sorted([saved_metric[0].id, saved_metric[1].id]), [saved_metric_id, saved_metric_2_id]) + + response = self.client.patch( + f"/api/projects/{self.team.id}/experiments/{exp_id}", + {"saved_metrics_ids": []}, + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(Experiment.objects.get(pk=exp_id).saved_metrics.count(), 0) + + response = self.client.patch( + f"/api/projects/{self.team.id}/experiments/{exp_id}", + { + "saved_metrics_ids": [ + {"id": saved_metric_id, "metadata": {"type": "secondary"}}, + ] + }, + ) + + response = self.client.patch( + f"/api/projects/{self.team.id}/experiments/{exp_id}", + {"saved_metrics_ids": None}, + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(Experiment.objects.get(pk=exp_id).saved_metrics.count(), 0) + + response = self.client.patch( + f"/api/projects/{self.team.id}/experiments/{exp_id}", + { + "saved_metrics_ids": [ + {"id": saved_metric_id, "metadata": {"type": "secondary"}}, + ] + }, + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(Experiment.objects.get(pk=exp_id).saved_metrics.count(), 1) + + # not updating saved metrics shouldn't change anything + response = self.client.patch( + f"/api/projects/{self.team.id}/experiments/{exp_id}", + { + "name": "Test Experiment 2", + }, + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(Experiment.objects.get(pk=exp_id).saved_metrics.count(), 1) + + # now delete saved metric + response = self.client.delete(f"/api/projects/{self.team.id}/experiment_saved_metrics/{saved_metric_id}") + self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + + # make sure experiment in question was updated as well + self.assertEqual(Experiment.objects.get(pk=exp_id).saved_metrics.count(), 0) + + def test_validate_saved_metrics_payload(self): + response = self.client.post( + f"/api/projects/{self.team.id}/experiment_saved_metrics/", + { + "name": "Test Experiment saved metric", + "description": "Test description", + "query": {"events": [{"id": "$pageview"}]}, + }, + ) + + saved_metric_id = response.json()["id"] + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + + # Generate experiment to have saved metric + ff_key = "a-b-tests" + exp_data = { + "name": "Test Experiment", + "description": "", + "start_date": "2021-12-01T10:23", + "end_date": None, + "feature_flag_key": ff_key, + "parameters": None, + "filters": { + "events": [ + {"order": 0, "id": "$pageview"}, + {"order": 1, "id": "$pageleave"}, + ], + "properties": [], + }, + } + response = self.client.post( + f"/api/projects/{self.team.id}/experiments/", + { + **exp_data, + "saved_metrics_ids": [{"id": saved_metric_id, "metadata": {"xxx": "secondary"}}], + }, + ) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.json()["type"], "validation_error") + self.assertEqual( + response.json()["detail"], + "Metadata must have a type key", + ) + + response = self.client.post( + f"/api/projects/{self.team.id}/experiments/", + { + **exp_data, + "saved_metrics_ids": [{"saved_metric": saved_metric_id}], + }, + ) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.json()["type"], "validation_error") + self.assertEqual(response.json()["detail"], "Saved metric must have an id") + + response = self.client.post( + f"/api/projects/{self.team.id}/experiments/", + { + **exp_data, + "saved_metrics_ids": [{"id": 12345678}], + }, + ) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.json()["type"], "validation_error") + self.assertEqual(response.json()["detail"], "Saved metric does not exist") + + response = self.client.post( + f"/api/projects/{self.team.id}/experiments/", + { + **exp_data, + "saved_metrics_ids": {"id": saved_metric_id}, + }, + ) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.json()["type"], "validation_error") + self.assertEqual(response.json()["detail"], 'Expected a list of items but got type "dict".') + + response = self.client.post( + f"/api/projects/{self.team.id}/experiments/", + { + **exp_data, + "saved_metrics_ids": [[saved_metric_id]], + }, + ) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.json()["type"], "validation_error") + self.assertEqual(response.json()["detail"], "Saved metric must be an object") + + response = self.client.post( + f"/api/projects/{self.team.id}/experiments/", + { + **exp_data, + "saved_metrics_ids": [{"id": saved_metric_id, "metadata": "secondary"}], + }, + ) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.json()["type"], "validation_error") + self.assertEqual(response.json()["detail"], "Metadata must be an object") + def test_adding_behavioral_cohort_filter_to_experiment_fails(self): cohort = Cohort.objects.create( team=self.team, diff --git a/posthog/models/experiment.py b/posthog/models/experiment.py index 8f25480182887..5581e197a9be8 100644 --- a/posthog/models/experiment.py +++ b/posthog/models/experiment.py @@ -41,7 +41,7 @@ class ExperimentType(models.TextChoices): variants = models.JSONField(default=dict, null=True, blank=True) metrics = models.JSONField(default=list, null=True, blank=True) - saved_metrics = models.ManyToManyField( + saved_metrics: models.ManyToManyField = models.ManyToManyField( "ExperimentSavedMetric", blank=True, related_name="experiments", through="ExperimentToSavedMetric" ) @@ -89,3 +89,6 @@ class ExperimentToSavedMetric(models.Model): metadata = models.JSONField(default=dict) created_at = models.DateTimeField(default=timezone.now) updated_at = models.DateTimeField(auto_now=True) + + def __str__(self): + return f"{self.experiment.name} - {self.saved_metric.name} - {self.metadata}" From 3ed1e20c6b2e4cdc86e1f14486384f7befb69539 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Wed, 30 Oct 2024 12:34:35 +0000 Subject: [PATCH 4/5] fix test --- ee/clickhouse/views/experiments.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/clickhouse/views/experiments.py b/ee/clickhouse/views/experiments.py index de1e350cf617a..f043e98e8071c 100644 --- a/ee/clickhouse/views/experiments.py +++ b/ee/clickhouse/views/experiments.py @@ -442,7 +442,9 @@ def update(self, instance: Experiment, validated_data: dict, *args: Any, **kwarg class EnterpriseExperimentsViewSet(TeamAndOrgViewSetMixin, viewsets.ModelViewSet): scope_object = "experiment" serializer_class = ExperimentSerializer - queryset = Experiment.objects.prefetch_related("feature_flag", "created_by", "holdout", "saved_metrics").all() + queryset = Experiment.objects.prefetch_related( + "feature_flag", "created_by", "holdout", "experimenttosavedmetric_set", "saved_metrics" + ).all() ordering = "-created_at" # ****************************************** From e034df0dc449971111f736a66e2f632b176645b1 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 30 Oct 2024 12:34:48 +0000 Subject: [PATCH 5/5] Update query snapshots --- posthog/api/test/__snapshots__/test_api_docs.ambr | 1 + 1 file changed, 1 insertion(+) diff --git a/posthog/api/test/__snapshots__/test_api_docs.ambr b/posthog/api/test/__snapshots__/test_api_docs.ambr index 044ee43f1983e..1558efeef9470 100644 --- a/posthog/api/test/__snapshots__/test_api_docs.ambr +++ b/posthog/api/test/__snapshots__/test_api_docs.ambr @@ -11,6 +11,7 @@ '/home/runner/work/posthog/posthog/ee/api/subscription.py: Warning [SubscriptionViewSet > SubscriptionSerializer]: unable to resolve type hint for function "summary". Consider using a type hint or @extend_schema_field. Defaulting to string.', '/home/runner/work/posthog/posthog/ee/api/subscription.py: Warning [SubscriptionViewSet]: could not derive type of path parameter "project_id" because model "posthog.models.subscription.Subscription" contained no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".', '/home/runner/work/posthog/posthog/ee/clickhouse/views/experiment_holdouts.py: Warning [ExperimentHoldoutViewSet]: could not derive type of path parameter "project_id" because model "posthog.models.experiment.ExperimentHoldout" contained no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".', + '/home/runner/work/posthog/posthog/ee/clickhouse/views/experiment_saved_metrics.py: Warning [ExperimentSavedMetricViewSet]: could not derive type of path parameter "project_id" because model "posthog.models.experiment.ExperimentSavedMetric" contained no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".', '/home/runner/work/posthog/posthog/ee/clickhouse/views/experiments.py: Warning [EnterpriseExperimentsViewSet]: could not derive type of path parameter "project_id" because model "posthog.models.experiment.Experiment" contained no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".', '/home/runner/work/posthog/posthog/ee/clickhouse/views/groups.py: Warning [GroupsTypesViewSet]: could not derive type of path parameter "project_id" because model "posthog.models.group_type_mapping.GroupTypeMapping" contained no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".', '/home/runner/work/posthog/posthog/ee/clickhouse/views/groups.py: Warning [GroupsViewSet]: could not derive type of path parameter "project_id" because model "posthog.models.group.group.Group" contained no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".',