From 137d40c72de379487d2431d2227683012c4dadb5 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Fri, 15 Sep 2023 18:51:11 +0100 Subject: [PATCH] fix(surveys): Validate user targeting, and allow crud of targeting flag (#17474) * fix(surveys): Validate user targeting, and allow crud of targeting flag * add test for name validation * fix flakey test from a previous PR --- frontend/src/scenes/surveys/Survey.tsx | 24 +- frontend/src/scenes/surveys/surveyLogic.tsx | 2 +- frontend/src/types.ts | 5 +- posthog/api/survey.py | 37 ++- posthog/api/test/test_feature_flag.py | 4 +- posthog/api/test/test_survey.py | 239 +++++++++++++++++++- 6 files changed, 285 insertions(+), 26 deletions(-) diff --git a/frontend/src/scenes/surveys/Survey.tsx b/frontend/src/scenes/surveys/Survey.tsx index d59ed4b674e69..ec13e3cc733b2 100644 --- a/frontend/src/scenes/surveys/Survey.tsx +++ b/frontend/src/scenes/surveys/Survey.tsx @@ -386,19 +386,17 @@ export function SurveyForm({ id }: { id: string }): JSX.Element {
- {id === 'new' && ( - { - setSurveyValue('targeting_flag_filters', undefined) - setSurveyValue('targeting_flag', null) - }} - > - Remove all user properties - - )} + { + setSurveyValue('targeting_flag_filters', undefined) + setSurveyValue('targeting_flag', null) + }} + > + Remove all user properties + )} diff --git a/frontend/src/scenes/surveys/surveyLogic.tsx b/frontend/src/scenes/surveys/surveyLogic.tsx index 9c4fe305a8b79..a09148203b5ef 100644 --- a/frontend/src/scenes/surveys/surveyLogic.tsx +++ b/frontend/src/scenes/surveys/surveyLogic.tsx @@ -346,7 +346,7 @@ export const surveyLogic = kea([ hasTargetingFlag: [ (s) => [s.survey], (survey): boolean => { - return !!survey.targeting_flag || !!(survey.id === 'new' && survey.targeting_flag_filters) + return !!survey.targeting_flag || !!survey.targeting_flag_filters }, ], }), diff --git a/frontend/src/types.ts b/frontend/src/types.ts index de8b30192d4a6..21b8924699319 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -2074,7 +2074,7 @@ export interface Survey { linked_flag: FeatureFlagBasicType | null targeting_flag: FeatureFlagBasicType | null targeting_flag_filters: Pick | undefined - conditions: { url: string; selector: string; is_headless?: boolean } | null + conditions: { url: string; selector: string; is_headless?: boolean; seenSurveyWaitPeriodInDays?: number } | null appearance: SurveyAppearance questions: (BasicSurveyQuestion | LinkSurveyQuestion | RatingSurveyQuestion | MultipleSurveyQuestion)[] created_at: string @@ -2113,7 +2113,7 @@ interface SurveyQuestionBase { } export interface BasicSurveyQuestion extends SurveyQuestionBase { - type: SurveyQuestionType.Open | SurveyQuestionType.NPS + type: SurveyQuestionType.Open } export interface LinkSurveyQuestion extends SurveyQuestionBase { @@ -2140,7 +2140,6 @@ export enum SurveyQuestionType { Open = 'open', MultipleChoice = 'multiple_choice', SingleChoice = 'single_choice', - NPS = 'nps', Rating = 'rating', Link = 'link', } diff --git a/posthog/api/survey.py b/posthog/api/survey.py index 0e6135508edbe..53f7fe2b491d7 100644 --- a/posthog/api/survey.py +++ b/posthog/api/survey.py @@ -56,7 +56,7 @@ class Meta: class SurveySerializerCreateUpdateOnly(SurveySerializer): linked_flag_id = serializers.IntegerField(required=False, write_only=True, allow_null=True) targeting_flag_id = serializers.IntegerField(required=False, write_only=True) - targeting_flag_filters = serializers.JSONField(required=False, write_only=True) + targeting_flag_filters = serializers.JSONField(required=False, write_only=True, allow_null=True) class Meta: model = Survey @@ -82,7 +82,7 @@ class Meta: read_only_fields = ["id", "linked_flag", "targeting_flag", "created_at"] def validate(self, data): - linked_flag_id = data.get("linked_flag_id", None) + linked_flag_id = data.get("linked_flag_id") if linked_flag_id: try: FeatureFlag.objects.get(pk=linked_flag_id) @@ -91,15 +91,38 @@ def validate(self, data): if ( self.context["request"].method == "POST" - and Survey.objects.filter(name=data.get("name", None), team_id=self.context["team_id"]).exists() + and Survey.objects.filter(name=data.get("name"), team_id=self.context["team_id"]).exists() ): raise serializers.ValidationError("There is already a survey with this name.", code="unique") + existing_survey: Survey | None = self.instance + + if ( + existing_survey + and existing_survey.name != data.get("name") + and Survey.objects.filter(name=data.get("name"), team_id=self.context["team_id"]) + .exclude(id=existing_survey.id) + .exists() + ): + raise serializers.ValidationError("There is already another survey with this name.", code="unique") + + if data.get("targeting_flag_filters"): + groups = (data.get("targeting_flag_filters") or {}).get("groups") or [] + full_rollout = any( + group.get("rollout_percentage") in [100, None] and len(group.get("properties", [])) == 0 + for group in groups + ) + + if full_rollout: + raise serializers.ValidationError( + "Invalid operation: User targeting rolls out to everyone. If you want to roll out to everyone, delete this targeting", + code="invalid", + ) return data def create(self, validated_data): validated_data["team_id"] = self.context["team_id"] - if validated_data.get("targeting_flag_filters", None): + if validated_data.get("targeting_flag_filters"): targeting_feature_flag = self._create_new_targeting_flag( validated_data["name"], validated_data["targeting_flag_filters"] ) @@ -111,7 +134,7 @@ def create(self, validated_data): def update(self, instance: Survey, validated_data): # if the target flag filters come back with data, update the targeting feature flag if there is one, otherwise create a new one - if validated_data.get("targeting_flag_filters", None): + if validated_data.get("targeting_flag_filters"): if instance.targeting_flag: existing_targeting_flag = instance.targeting_flag serialized_data_filters = { @@ -130,6 +153,10 @@ def update(self, instance: Survey, validated_data): new_flag = self._create_new_targeting_flag(instance.name, validated_data["targeting_flag_filters"]) validated_data["targeting_flag_id"] = new_flag.id validated_data.pop("targeting_flag_filters") + else: + if instance.targeting_flag: + instance.targeting_flag.delete() + validated_data["targeting_flag_id"] = None return super().update(instance, validated_data) def _create_new_targeting_flag(self, name, filters): diff --git a/posthog/api/test/test_feature_flag.py b/posthog/api/test/test_feature_flag.py index b0d6f73c87ebb..b243b46200764 100644 --- a/posthog/api/test/test_feature_flag.py +++ b/posthog/api/test/test_feature_flag.py @@ -961,7 +961,7 @@ def test_getting_flags_is_not_nplus1(self) -> None: format="json", ).json() - with self.assertNumQueries(FuzzyInt(10, 11)): + with self.assertNumQueries(FuzzyInt(11, 12)): response = self.client.get(f"/api/projects/{self.team.id}/feature_flags") self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -972,7 +972,7 @@ def test_getting_flags_is_not_nplus1(self) -> None: format="json", ).json() - with self.assertNumQueries(FuzzyInt(10, 11)): + with self.assertNumQueries(FuzzyInt(11, 12)): response = self.client.get(f"/api/projects/{self.team.id}/feature_flags") self.assertEqual(response.status_code, status.HTTP_200_OK) diff --git a/posthog/api/test/test_survey.py b/posthog/api/test/test_survey.py index f393e5cec4379..4f3ac5d9b27b9 100644 --- a/posthog/api/test/test_survey.py +++ b/posthog/api/test/test_survey.py @@ -219,14 +219,206 @@ def test_updating_survey_with_targeting_creates_or_updates_targeting_flag(self): updated_survey_updates_targeting_flag = self.client.patch( f"/api/projects/{self.team.id}/surveys/{survey_with_targeting['id']}/", data={ - "targeting_flag_filters": {"groups": [{"variant": None, "rollout_percentage": None, "properties": []}]}, + "targeting_flag_filters": {"groups": [{"variant": None, "rollout_percentage": 20, "properties": []}]}, }, ) assert updated_survey_updates_targeting_flag.status_code == status.HTTP_200_OK assert FeatureFlag.objects.filter(id=survey_with_targeting["targeting_flag"]["id"]).get().filters == { - "groups": [{"variant": None, "properties": [], "rollout_percentage": None}] + "groups": [{"variant": None, "properties": [], "rollout_percentage": 20}] } + def test_updating_survey_to_remove_targeting_deletes_targeting_flag(self): + survey_with_targeting = self.client.post( + f"/api/projects/{self.team.id}/surveys/", + data={ + "name": "survey with targeting", + "type": "popover", + "targeting_flag_filters": { + "groups": [ + { + "variant": None, + "rollout_percentage": None, + "properties": [ + {"key": "billing_plan", "value": ["cloud"], "operator": "exact", "type": "person"} + ], + } + ] + }, + "conditions": {"url": "https://app.posthog.com/notebooks"}, + }, + format="json", + ).json() + + flagId = survey_with_targeting["targeting_flag"]["id"] + assert FeatureFlag.objects.filter(id=flagId).exists() + + updated_survey_deletes_targeting_flag = self.client.patch( + f"/api/projects/{self.team.id}/surveys/{survey_with_targeting['id']}/", + data={ + "name": "other", + # "targeting_flag_filters": None, # delete these + }, + ) + + assert updated_survey_deletes_targeting_flag.status_code == status.HTTP_200_OK + assert updated_survey_deletes_targeting_flag.json()["name"] == "other" + assert updated_survey_deletes_targeting_flag.json()["targeting_flag"] is None + + with self.assertRaises(FeatureFlag.DoesNotExist): + FeatureFlag.objects.get(id=flagId) + + def test_updating_survey_to_send_none_targeting_deletes_targeting_flag(self): + survey_with_targeting = self.client.post( + f"/api/projects/{self.team.id}/surveys/", + data={ + "name": "survey with targeting", + "type": "popover", + "targeting_flag_filters": { + "groups": [ + { + "variant": None, + "rollout_percentage": None, + "properties": [ + {"key": "billing_plan", "value": ["cloud"], "operator": "exact", "type": "person"} + ], + } + ] + }, + "conditions": {"url": "https://app.posthog.com/notebooks"}, + }, + format="json", + ).json() + + flagId = survey_with_targeting["targeting_flag"]["id"] + assert FeatureFlag.objects.filter(id=flagId).exists() + + updated_survey_deletes_targeting_flag = self.client.patch( + f"/api/projects/{self.team.id}/surveys/{survey_with_targeting['id']}/", + data={ + "targeting_flag_filters": None, # delete these + }, + ) + + assert updated_survey_deletes_targeting_flag.status_code == status.HTTP_200_OK + assert updated_survey_deletes_targeting_flag.json()["name"] == "survey with targeting" + assert updated_survey_deletes_targeting_flag.json()["targeting_flag"] is None + + with self.assertRaises(FeatureFlag.DoesNotExist): + FeatureFlag.objects.get(id=flagId) + + def test_survey_targeting_flag_validation(self): + survey_with_targeting = self.client.post( + f"/api/projects/{self.team.id}/surveys/", + data={ + "name": "survey with targeting", + "type": "popover", + "targeting_flag_filters": { + "groups": [ + { + "variant": None, + "rollout_percentage": None, + "properties": [ + {"key": "billing_plan", "value": ["cloud"], "operator": "exact", "type": "person"} + ], + } + ] + }, + "conditions": {"url": "https://app.posthog.com/notebooks"}, + }, + format="json", + ).json() + + flagId = survey_with_targeting["targeting_flag"]["id"] + assert FeatureFlag.objects.filter(id=flagId).exists() + + updated_survey_deletes_targeting_flag = self.client.patch( + f"/api/projects/{self.team.id}/surveys/{survey_with_targeting['id']}/", + data={ + "targeting_flag_filters": { + "groups": [ + { + "variant": None, + "rollout_percentage": None, + "properties": [], + } + ] + }, + }, + ) + + invalid_detail = "Invalid operation: User targeting rolls out to everyone. If you want to roll out to everyone, delete this targeting" + + assert updated_survey_deletes_targeting_flag.status_code == status.HTTP_400_BAD_REQUEST + assert updated_survey_deletes_targeting_flag.json()["detail"] == invalid_detail + + updated_survey_deletes_targeting_flag = self.client.patch( + f"/api/projects/{self.team.id}/surveys/{survey_with_targeting['id']}/", + data={ + "targeting_flag_filters": { + "groups": [ + { + "variant": None, + "rollout_percentage": 100, + "properties": [{"key": "value"}], + }, + { + "variant": None, + "rollout_percentage": None, + "properties": [], + }, + ] + }, + }, + ) + + assert updated_survey_deletes_targeting_flag.status_code == status.HTTP_400_BAD_REQUEST + assert updated_survey_deletes_targeting_flag.json()["detail"] == invalid_detail + + updated_survey_deletes_targeting_flag = self.client.patch( + f"/api/projects/{self.team.id}/surveys/{survey_with_targeting['id']}/", + data={ + "targeting_flag_filters": { + "groups": [ + { + "variant": None, + "rollout_percentage": 100, + "properties": [{"key": "value"}], + }, + { + "variant": None, + "rollout_percentage": 100, + "properties": [], + }, + ] + }, + }, + ) + + assert updated_survey_deletes_targeting_flag.status_code == status.HTTP_400_BAD_REQUEST + assert updated_survey_deletes_targeting_flag.json()["detail"] == invalid_detail + + updated_survey_deletes_targeting_flag = self.client.patch( + f"/api/projects/{self.team.id}/surveys/{survey_with_targeting['id']}/", + data={ + "targeting_flag_filters": { + "groups": [ + { + "variant": None, + "rollout_percentage": 100, + "properties": [{"key": "value", "type": "person", "value": "bleh"}], + }, + { + "variant": None, + "rollout_percentage": 30, + "properties": [], + }, + ] + }, + }, + ) + + assert updated_survey_deletes_targeting_flag.status_code == status.HTTP_200_OK + def test_deleting_survey_does_not_delete_linked_flag(self): linked_flag = FeatureFlag.objects.create(team=self.team, key="early-access", created_by=self.user) @@ -316,6 +508,49 @@ def test_can_list_surveys(self): ], } + def test_updating_survey_name_validates(self): + survey_with_targeting = self.client.post( + f"/api/projects/{self.team.id}/surveys/", + data={ + "name": "survey with targeting", + "type": "popover", + "targeting_flag_filters": { + "groups": [ + { + "variant": None, + "rollout_percentage": None, + "properties": [ + {"key": "billing_plan", "value": ["cloud"], "operator": "exact", "type": "person"} + ], + } + ] + }, + "conditions": {"url": "https://app.posthog.com/notebooks"}, + }, + format="json", + ).json() + + self.client.post( + f"/api/projects/{self.team.id}/surveys/", + data={ + "name": "survey without targeting", + "type": "popover", + }, + format="json", + ).json() + + updated_survey_deletes_targeting_flag = self.client.patch( + f"/api/projects/{self.team.id}/surveys/{survey_with_targeting['id']}/", + data={ + "name": "survey without targeting", + }, + ) + + assert updated_survey_deletes_targeting_flag.status_code == status.HTTP_400_BAD_REQUEST + assert ( + updated_survey_deletes_targeting_flag.json()["detail"] == "There is already another survey with this name." + ) + class TestSurveysAPIList(BaseTest, QueryMatchingTest): def setUp(self):