Skip to content

Commit

Permalink
fix(flags): Dont locally match cohorts when id property is provided (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
neilkakkar authored Nov 22, 2023
1 parent f1427f5 commit 266c3ff
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 0 deletions.
4 changes: 4 additions & 0 deletions posthog/models/feature_flag/flag_matching.py
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,10 @@ def can_compute_locally(
self.cache.group_type_index_to_name[group_type_index], {}
)
for property in properties:
# can't locally compute if property is a cohort
# need to atleast fetch the cohort
if property.type == "cohort":
return False
if property.key not in target_properties:
return False
if property.operator == "is_not_set":
Expand Down
74 changes: 74 additions & 0 deletions posthog/test/test_feature_flag.py
Original file line number Diff line number Diff line change
Expand Up @@ -3458,6 +3458,80 @@ def test_cohort_filters_with_override_properties(self):
FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 0),
)

def test_cohort_filters_with_override_id_property(self):
cohort1 = Cohort.objects.create(
team=self.team,
groups=[
{
"properties": [
{
"key": "email",
"type": "person",
"value": "@posthog.com",
"negation": False,
"operator": "icontains",
}
]
}
],
name="cohort1",
)

feature_flag1: FeatureFlag = self.create_feature_flag(
key="x1",
filters={"groups": [{"properties": [{"key": "id", "value": cohort1.pk, "type": "cohort"}]}]},
)
Person.objects.create(
team=self.team,
distinct_ids=["example_id"],
properties={"email": "[email protected]"},
)

with self.assertNumQueries(5):
self.assertEqual(
FeatureFlagMatcher(
[feature_flag1],
"example_id",
property_value_overrides={},
).get_match(feature_flag1),
FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 0),
)

with self.assertNumQueries(4):
# no local computation because cohort lookup is required
# no postgres person query required here to get the person, because email is sufficient
# property id override shouldn't confuse the matcher
self.assertEqual(
FeatureFlagMatcher(
[feature_flag1],
"example_id",
property_value_overrides={"id": "example_id", "email": "bzz"},
).get_match(feature_flag1),
FeatureFlagMatch(False, None, FeatureFlagMatchReason.NO_CONDITION_MATCH, 0),
)

with self.assertNumQueries(4):
# no postgres query required here to get the person
self.assertEqual(
FeatureFlagMatcher(
[feature_flag1],
"example_id",
property_value_overrides={"id": "second_id", "email": "[email protected]"},
).get_match(feature_flag1),
FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 0),
)

with self.assertNumQueries(5):
# postgres query required here to get the person
self.assertEqual(
FeatureFlagMatcher(
[feature_flag1],
"example_id",
property_value_overrides={"id": "second_id"},
).get_match(feature_flag1),
FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 0),
)

@pytest.mark.skip("This case is not supported yet")
def test_complex_cohort_filter_with_override_properties(self):
# TODO: Currently we don't support this case for persons who haven't been ingested yet
Expand Down

0 comments on commit 266c3ff

Please sign in to comment.