Skip to content

Commit

Permalink
feat(exp): Add full holdouts to response (#25814)
Browse files Browse the repository at this point in the history
  • Loading branch information
neilkakkar authored Oct 25, 2024
1 parent 7d8f252 commit 763214d
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 185 deletions.
11 changes: 9 additions & 2 deletions ee/clickhouse/views/experiments.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
ClickhouseTrendExperimentResult,
)
from ee.clickhouse.queries.experiments.utils import requires_flag_warning
from ee.clickhouse.views.experiment_holdouts import ExperimentHoldoutSerializer
from posthog.api.cohort import CohortSerializer
from posthog.api.feature_flag import FeatureFlagSerializer, MinimalFeatureFlagSerializer
from posthog.api.routing import TeamAndOrgViewSetMixin
Expand All @@ -27,7 +28,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
from posthog.models.experiment import Experiment, ExperimentHoldout
from posthog.models.filters.filter import Filter
from posthog.utils import generate_cache_key, get_safe_cache

Expand Down Expand Up @@ -156,6 +157,10 @@ class ExperimentSerializer(serializers.ModelSerializer):
feature_flag_key = serializers.CharField(source="get_feature_flag_key")
created_by = UserBasicSerializer(read_only=True)
feature_flag = MinimalFeatureFlagSerializer(read_only=True)
holdout = ExperimentHoldoutSerializer(read_only=True)
holdout_id = serializers.PrimaryKeyRelatedField(
queryset=ExperimentHoldout.objects.all(), source="holdout", required=False, allow_null=True
)

class Meta:
model = Experiment
Expand All @@ -168,6 +173,7 @@ class Meta:
"feature_flag_key",
"feature_flag",
"holdout",
"holdout_id",
"exposure_cohort",
"parameters",
"secondary_metrics",
Expand All @@ -184,6 +190,7 @@ class Meta:
"updated_at",
"feature_flag",
"exposure_cohort",
"holdout",
]

def validate_parameters(self, value):
Expand Down Expand Up @@ -361,7 +368,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").all()
queryset = Experiment.objects.prefetch_related("feature_flag", "created_by", "holdout").all()
ordering = "-created_at"

# ******************************************
Expand Down
12 changes: 6 additions & 6 deletions ee/clickhouse/views/test/test_clickhouse_experiments.py
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ def test_transferring_holdout_to_another_group(self):
],
"properties": [],
},
"holdout": holdout_id,
"holdout_id": holdout_id,
},
)

Expand Down Expand Up @@ -202,7 +202,7 @@ def test_transferring_holdout_to_another_group(self):

response = self.client.patch(
f"/api/projects/{self.team.id}/experiments/{exp_id}",
{"holdout": holdout_2_id},
{"holdout_id": holdout_2_id},
)

self.assertEqual(response.status_code, status.HTTP_200_OK)
Expand Down Expand Up @@ -262,7 +262,7 @@ def test_transferring_holdout_to_another_group(self):
# remove holdouts
response = self.client.patch(
f"/api/projects/{self.team.id}/experiments/{exp_id}",
{"holdout": None},
{"holdout_id": None},
)

self.assertEqual(response.status_code, status.HTTP_200_OK)
Expand All @@ -276,15 +276,15 @@ def test_transferring_holdout_to_another_group(self):
# try adding invalid holdout
response = self.client.patch(
f"/api/projects/{self.team.id}/experiments/{exp_id}",
{"holdout": 123456},
{"holdout_id": 123456},
)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
self.assertEqual(response.json()["detail"], 'Invalid pk "123456" - object does not exist.')

# add back holdout
response = self.client.patch(
f"/api/projects/{self.team.id}/experiments/{exp_id}",
{"holdout": holdout_2_id},
{"holdout_id": holdout_2_id},
)

# launch experiment and try updating holdouts again
Expand All @@ -297,7 +297,7 @@ def test_transferring_holdout_to_another_group(self):

response = self.client.patch(
f"/api/projects/{self.team.id}/experiments/{exp_id}",
{"holdout": holdout_id},
{"holdout_id": holdout_id},
)

self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
Expand Down
2 changes: 1 addition & 1 deletion ee/clickhouse/views/test/test_experiment_holdouts.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def test_create_update_experiment_holdouts(self) -> None:
],
"properties": [],
},
"holdout": holdout_id,
"holdout_id": holdout_id,
},
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
FROM events LEFT JOIN (
SELECT person_static_cohort.person_id AS cohort_person_id, 1 AS matched, person_static_cohort.cohort_id AS cohort_id
FROM person_static_cohort
WHERE and(equals(person_static_cohort.team_id, 420), in(person_static_cohort.cohort_id, [6]))) AS __in_cohort ON equals(__in_cohort.cohort_person_id, events.person_id)
WHERE and(equals(person_static_cohort.team_id, 420), in(person_static_cohort.cohort_id, [4]))) AS __in_cohort ON equals(__in_cohort.cohort_person_id, events.person_id)
WHERE and(equals(events.team_id, 420), 1, ifNull(equals(__in_cohort.matched, 1), 0))
LIMIT 100
SETTINGS readonly=2, max_execution_time=60, allow_experimental_object_type=1, format_csv_allow_double_quotes=0, max_ast_elements=4000000, max_expanded_ast_elements=4000000, max_bytes_before_external_group_by=0
Expand All @@ -42,7 +42,7 @@
FROM events LEFT JOIN (
SELECT person_id AS cohort_person_id, 1 AS matched, cohort_id
FROM static_cohort_people
WHERE in(cohort_id, [6])) AS __in_cohort ON equals(__in_cohort.cohort_person_id, person_id)
WHERE in(cohort_id, [4])) AS __in_cohort ON equals(__in_cohort.cohort_person_id, person_id)
WHERE and(1, equals(__in_cohort.matched, 1))
LIMIT 100
'''
Expand All @@ -55,7 +55,7 @@
FROM events LEFT JOIN (
SELECT person_static_cohort.person_id AS cohort_person_id, 1 AS matched, person_static_cohort.cohort_id AS cohort_id
FROM person_static_cohort
WHERE and(equals(person_static_cohort.team_id, 420), in(person_static_cohort.cohort_id, [7]))) AS __in_cohort ON equals(__in_cohort.cohort_person_id, events.person_id)
WHERE and(equals(person_static_cohort.team_id, 420), in(person_static_cohort.cohort_id, [5]))) AS __in_cohort ON equals(__in_cohort.cohort_person_id, events.person_id)
WHERE and(equals(events.team_id, 420), 1, ifNull(equals(__in_cohort.matched, 1), 0))
LIMIT 100
SETTINGS readonly=2, max_execution_time=60, allow_experimental_object_type=1, format_csv_allow_double_quotes=0, max_ast_elements=4000000, max_expanded_ast_elements=4000000, max_bytes_before_external_group_by=0
Expand All @@ -66,7 +66,7 @@
FROM events LEFT JOIN (
SELECT person_id AS cohort_person_id, 1 AS matched, cohort_id
FROM static_cohort_people
WHERE in(cohort_id, [7])) AS __in_cohort ON equals(__in_cohort.cohort_person_id, person_id)
WHERE in(cohort_id, [5])) AS __in_cohort ON equals(__in_cohort.cohort_person_id, person_id)
WHERE and(1, equals(__in_cohort.matched, 1))
LIMIT 100
'''
Expand Down
Loading

0 comments on commit 763214d

Please sign in to comment.