Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(surveys): lt/gt operator error in survey creation #24283

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
91f7e68
Add failing test to verify targeting flag numeric property error when…
silentninja Aug 9, 2024
2839d52
Add missing type property annotation to the query used in 'check_flag…
silentninja Aug 9, 2024
ea6e96d
Merge branch 'refs/heads/master' into fix/feature-flag-math-property-…
silentninja Aug 9, 2024
20450e1
Merge branch 'master' into fix/feature-flag-math-property-error
silentninja Aug 9, 2024
8269f04
Merge branch 'PostHog:master' into fix/feature-flag-math-property-error
silentninja Aug 9, 2024
72e1fa4
Merge branch 'master' into fix/feature-flag-math-property-error
dmarticus Aug 9, 2024
cff7951
Merge branch 'master' into fix/feature-flag-math-property-error
silentninja Aug 12, 2024
0b88b2d
Merge branch 'master' into fix/feature-flag-math-property-error
dmarticus Aug 12, 2024
af25967
Merge branch 'master' into fix/feature-flag-math-property-error
dmarticus Aug 12, 2024
e8a17f4
Merge branch 'PostHog:master' into fix/feature-flag-math-property-error
silentninja Aug 13, 2024
245bd2a
Merge branch 'master' into fix/feature-flag-math-property-error
silentninja Aug 13, 2024
0b0fa2f
Merge branch 'master' into fix/feature-flag-math-property-error
dmarticus Aug 13, 2024
b99c23d
Merge branch 'master' into fix/feature-flag-math-property-error
dmarticus Aug 13, 2024
def8095
Merge branch 'master' into fix/feature-flag-math-property-error
dmarticus Aug 13, 2024
6877617
Merge branch 'master' into fix/feature-flag-math-property-error
dmarticus Aug 13, 2024
5ef75a8
Merge branch 'master' into fix/feature-flag-math-property-error
Twixes Aug 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions posthog/api/test/test_survey.py
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,34 @@ def test_survey_targeting_flag_validation(self):

assert updated_survey_deletes_targeting_flag.status_code == status.HTTP_200_OK

def test_survey_targeting_flag_numeric_validation(self):
survey_with_targeting = self.client.post(
f"/api/projects/{self.team.id}/surveys/",
data={
"name": "survey with numeric targeting",
"type": "popover",
"targeting_flag_filters": {
"groups": [
{
"variant": None,
"rollout_percentage": None,
"properties": [
{
"key": "$browser_version",
"value": "10",
"operator": "gt",
"type": "person",
}
],
}
]
},
"conditions": {"url": ""},
},
format="json",
)
assert survey_with_targeting.status_code == status.HTTP_201_CREATED

def test_updating_survey_to_send_none_linked_flag_removes_linking(self):
linked_flag = FeatureFlag.objects.create(team=self.team, key="early-access", created_by=self.user)

Expand Down
22 changes: 13 additions & 9 deletions posthog/models/feature_flag/flag_matching.py
Original file line number Diff line number Diff line change
Expand Up @@ -503,10 +503,7 @@ def condition_eval(key, condition):
# in properties_to_q, in empty_or_null_with_value_q
# These need to come in before the expr so they're available to use inside the expr.
# Same holds for the group queries below.
type_property_annotations = {
prop_key: Func(F(prop_field), function="JSONB_TYPEOF", output_field=CharField())
for prop_key, prop_field in properties_with_math_operators
}
type_property_annotations = _get_property_type_annotations(properties_with_math_operators)
person_query = person_query.annotate(
**type_property_annotations,
**{
Expand All @@ -525,10 +522,7 @@ def condition_eval(key, condition):
group_query,
group_fields,
) = group_query_per_group_type_mapping[feature_flag.aggregation_group_type_index]
type_property_annotations = {
prop_key: Func(F(prop_field), function="JSONB_TYPEOF", output_field=CharField())
for prop_key, prop_field in properties_with_math_operators
}
type_property_annotations = _get_property_type_annotations(properties_with_math_operators)
group_query = group_query.annotate(
**type_property_annotations,
**{
Expand Down Expand Up @@ -1096,15 +1090,25 @@ def check_flag_evaluation_query_is_ok(feature_flag: FeatureFlag, team_id: int) -
team_id,
property_list,
)
properties_with_math_operators = get_all_properties_with_math_operators(property_list, {}, team_id)
type_property_annotations = _get_property_type_annotations(properties_with_math_operators)
base_query = base_query.annotate(
**type_property_annotations,
**{
key: ExpressionWrapper(
expr if expr else RawSQL("true", []),
output_field=BooleanField(),
),
}
},
)
query_fields.append(key)

values = base_query.values(*query_fields)[:10]
return len(values) > 0


def _get_property_type_annotations(properties_with_math_operators):
return {
prop_key: Func(F(prop_field), function="JSONB_TYPEOF", output_field=CharField())
for prop_key, prop_field in properties_with_math_operators
}
Loading