Skip to content

Commit

Permalink
revert: "revert: "fix(surveys): Validate user targeting, and allow cr…
Browse files Browse the repository at this point in the history
…ud of targeting flag"" (#17496)
  • Loading branch information
neilkakkar authored Sep 18, 2023
1 parent beda1d3 commit f4bb9ee
Show file tree
Hide file tree
Showing 6 changed files with 377 additions and 28 deletions.
26 changes: 13 additions & 13 deletions frontend/src/scenes/surveys/Survey.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,7 @@ export function SurveyForm({ id }: { id: string }): JSX.Element {
className="w-max"
onClick={() => {
setSurveyValue('targeting_flag_filters', { groups: [] })
setSurveyValue('remove_targeting_flag', false)
}}
>
Add user targeting
Expand All @@ -386,19 +387,18 @@ 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', null)
setSurveyValue('targeting_flag', null)
setSurveyValue('remove_targeting_flag', true)
}}
>
Remove all user properties
</LemonButton>
</>
)}
</BindLogic>
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/scenes/surveys/surveyLogic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
],
}),
Expand Down
6 changes: 3 additions & 3 deletions frontend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2074,14 +2074,15 @@ 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
created_by: UserBasicType | null
start_date: string | null
end_date: string | null
archived: boolean
remove_targeting_flag?: boolean
}

export enum SurveyType {
Expand Down Expand Up @@ -2113,7 +2114,7 @@ interface SurveyQuestionBase {
}

export interface BasicSurveyQuestion extends SurveyQuestionBase {
type: SurveyQuestionType.Open | SurveyQuestionType.NPS
type: SurveyQuestionType.Open
}

export interface LinkSurveyQuestion extends SurveyQuestionBase {
Expand All @@ -2140,7 +2141,6 @@ export enum SurveyQuestionType {
Open = 'open',
MultipleChoice = 'multiple_choice',
SingleChoice = 'single_choice',
NPS = 'nps',
Rating = 'rating',
Link = 'link',
}
Expand Down
51 changes: 44 additions & 7 deletions posthog/api/survey.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ 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)
remove_targeting_flag = serializers.BooleanField(required=False, write_only=True, allow_null=True)

class Meta:
model = Survey
Expand All @@ -70,6 +71,7 @@ class Meta:
"targeting_flag_id",
"targeting_flag",
"targeting_flag_filters",
"remove_targeting_flag",
"questions",
"conditions",
"appearance",
Expand All @@ -82,7 +84,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)
Expand All @@ -91,15 +93,41 @@ 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):
if "remove_targeting_flag" in validated_data:
validated_data.pop("remove_targeting_flag")

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"]
)
Expand All @@ -110,13 +138,21 @@ def create(self, validated_data):
return super().create(validated_data)

def update(self, instance: Survey, validated_data):

if validated_data.get("remove_targeting_flag"):
if instance.targeting_flag:
instance.targeting_flag.delete()
validated_data["targeting_flag_id"] = None
validated_data.pop("remove_targeting_flag")

# 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"):
new_filters = validated_data["targeting_flag_filters"]
if instance.targeting_flag:
existing_targeting_flag = instance.targeting_flag
serialized_data_filters = {
**existing_targeting_flag.filters,
**validated_data["targeting_flag_filters"],
**new_filters,
}
existing_flag_serializer = FeatureFlagSerializer(
existing_targeting_flag,
Expand All @@ -127,9 +163,10 @@ def update(self, instance: Survey, validated_data):
existing_flag_serializer.is_valid(raise_exception=True)
existing_flag_serializer.save()
else:
new_flag = self._create_new_targeting_flag(instance.name, validated_data["targeting_flag_filters"])
new_flag = self._create_new_targeting_flag(instance.name, new_filters)
validated_data["targeting_flag_id"] = new_flag.id
validated_data.pop("targeting_flag_filters")

return super().update(instance, validated_data)

def _create_new_targeting_flag(self, name, filters):
Expand Down
4 changes: 2 additions & 2 deletions posthog/api/test/test_feature_flag.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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)

Expand Down
Loading

0 comments on commit f4bb9ee

Please sign in to comment.