fix(surveys): Validate user targeting, and allow crud of targeting fl…

…ag (#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

@@ -386,19 +386,17 @@ export function SurveyForm({ id }: { id: string }): JSX.Element {
                                         <div className="mt-2">
                                             <FeatureFlagReleaseConditions excludeTitle={true} />
                                         </div>
-                                        {id === 'new' && (
-                                            <LemonButton
-                                                type="secondary"
-                                                status="danger"
-                                                className="w-max"
-                                                onClick={() => {
-                                                    setSurveyValue('targeting_flag_filters', undefined)
-                                                    setSurveyValue('targeting_flag', null)
-                                                }}
-                                            >
-                                                Remove all user properties
-                                            </LemonButton>
-                                        )}
+                                        <LemonButton
+                                            type="secondary"
+                                            status="danger"
+                                            className="w-max"
+                                            onClick={() => {
+                                                setSurveyValue('targeting_flag_filters', undefined)
+                                                setSurveyValue('targeting_flag', null)
+                                            }}
+                                        >
+                                            Remove all user properties
+                                        </LemonButton>
                                     </>
                                 )}
                             </BindLogic>

frontend/src/scenes/surveys/surveyLogic.tsx

@@ -346,7 +346,7 @@ export const surveyLogic = kea<surveyLogicType>([
         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
             },
         ],
     }),

frontend/src/types.ts

@@ -2074,7 +2074,7 @@ export interface Survey {
     linked_flag: FeatureFlagBasicType | null
     targeting_flag: FeatureFlagBasicType | null
     targeting_flag_filters: Pick<FeatureFlagFilters, 'groups'> | 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',
 }

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):

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)