From 45c82dc6413be82ccee858d144c60fa77c76d56c Mon Sep 17 00:00:00 2001 From: Li Yi Yu Date: Mon, 18 Sep 2023 07:02:55 -0400 Subject: [PATCH] revert: "fix(surveys): Validate user targeting, and allow crud of targeting flag" (#17493) --- 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, 26 insertions(+), 285 deletions(-) diff --git a/frontend/src/scenes/surveys/Survey.tsx b/frontend/src/scenes/surveys/Survey.tsx index ec13e3cc733b2..d59ed4b674e69 100644 --- a/frontend/src/scenes/surveys/Survey.tsx +++ b/frontend/src/scenes/surveys/Survey.tsx @@ -386,17 +386,19 @@ export function SurveyForm({ id }: { id: string }): JSX.Element {
- { - setSurveyValue('targeting_flag_filters', undefined) - setSurveyValue('targeting_flag', null) - }} - > - Remove all user properties - + {id === 'new' && ( + { + 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 a09148203b5ef..9c4fe305a8b79 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.targeting_flag_filters + return !!survey.targeting_flag || !!(survey.id === 'new' && survey.targeting_flag_filters) }, ], }), diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 21b8924699319..de8b30192d4a6 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; seenSurveyWaitPeriodInDays?: number } | null + conditions: { url: string; selector: string; is_headless?: boolean } | 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 + type: SurveyQuestionType.Open | SurveyQuestionType.NPS } export interface LinkSurveyQuestion extends SurveyQuestionBase { @@ -2140,6 +2140,7 @@ 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 53f7fe2b491d7..0e6135508edbe 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, allow_null=True) + targeting_flag_filters = serializers.JSONField(required=False, write_only=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") + linked_flag_id = data.get("linked_flag_id", None) if linked_flag_id: try: FeatureFlag.objects.get(pk=linked_flag_id) @@ -91,38 +91,15 @@ def validate(self, data): if ( self.context["request"].method == "POST" - and Survey.objects.filter(name=data.get("name"), team_id=self.context["team_id"]).exists() + and Survey.objects.filter(name=data.get("name", None), 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"): + if validated_data.get("targeting_flag_filters", None): targeting_feature_flag = self._create_new_targeting_flag( validated_data["name"], validated_data["targeting_flag_filters"] ) @@ -134,7 +111,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"): + if validated_data.get("targeting_flag_filters", None): if instance.targeting_flag: existing_targeting_flag = instance.targeting_flag serialized_data_filters = { @@ -153,10 +130,6 @@ 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 b243b46200764..b0d6f73c87ebb 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(11, 12)): + with self.assertNumQueries(FuzzyInt(10, 11)): 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(11, 12)): + with self.assertNumQueries(FuzzyInt(10, 11)): 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 4f3ac5d9b27b9..f393e5cec4379 100644 --- a/posthog/api/test/test_survey.py +++ b/posthog/api/test/test_survey.py @@ -219,206 +219,14 @@ 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": 20, "properties": []}]}, + "targeting_flag_filters": {"groups": [{"variant": None, "rollout_percentage": None, "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": 20}] + "groups": [{"variant": None, "properties": [], "rollout_percentage": None}] } - 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) @@ -508,49 +316,6 @@ 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):