mirrors/posthog
0
0
Fork 0
mirror of https://github.com/PostHog/posthog.git synced 2024-11-21 21:49:51 +01:00
Code Issues Releases Activity

fix: cohort negation (#25327)

Browse Source
This commit is contained in:
Sandy Spicer 2024-10-02 09:32:55 -07:00 committed by GitHub
parent fafeeaae1d
commit cbf1ffecff
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 52 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
ee/clickhouse/models/test/test_filters.py
View File

@ -126,6 +126,7 @@ class TestFilters(PGTestFilters):
{
"key": "id",
"value": cohort.pk,
"negation": False,
"type": "precalculated-cohort",
}
],
@ -141,6 +142,7 @@ class TestFilters(PGTestFilters):
"values": [
{
"key": "id",
"negation": False,
"value": cohort.pk,
"type": "precalculated-cohort",
}
@ -158,7 +160,7 @@ class TestFilters(PGTestFilters):
{
"properties": {
"type": "AND",
"values": [{"type": "static-cohort", "key": "id", "value": cohort.pk}],
"values": [{"type": "static-cohort", "negation": False, "key": "id", "value": cohort.pk}],
}
},
)
@ -172,7 +174,7 @@ class TestFilters(PGTestFilters):
{
"properties": {
"type": "AND",
"values": [{"type": "cohort", "key": "id", "value": cohort.pk}],
"values": [{"type": "cohort", "negation": False, "key": "id", "value": cohort.pk}],
}
},
)

3
posthog/hogql/property.py
View File

@ -482,7 +482,8 @@ def property_to_expr(
return ast.CompareOperation(
left=ast.Field(chain=["id" if scope == "person" else "person_id"]),
op=ast.CompareOperationOp.NotInCohort
if property.operator == PropertyOperator.NOT_IN.value
# Kludge: negation is outdated but still used in places
if property.negation or property.operator == PropertyOperator.NOT_IN.value
else ast.CompareOperationOp.InCohort,
right=ast.Constant(value=cohort.pk),
)

16
posthog/hogql_queries/legacy_compatibility/clean_properties.py
View File

@ -1,3 +1,6 @@
from posthog.schema import PropertyOperator
def clean_global_properties(properties: dict | list[dict] | None):
if properties is None or len(properties) == 0:
# empty properties
@ -90,8 +93,15 @@ def clean_property(property: dict):
cleaned_property["type"] = "cohort"
# fix invalid property key for cohorts
if cleaned_property.get("type") == "cohort" and cleaned_property.get("key") != "id":
cleaned_property["key"] = "id"
if cleaned_property.get("type") == "cohort":
if cleaned_property.get("key") != "id":
cleaned_property["key"] = "id"
if cleaned_property.get("operator") is None:
cleaned_property["operator"] = (
PropertyOperator.NOT_IN.value if cleaned_property.get("negation", False) else PropertyOperator.IN_.value
)
if "negation" in cleaned_property:
del cleaned_property["negation"]
# set a default operator for properties that support it, but don't have an operator set
if is_property_with_operator(cleaned_property) and cleaned_property.get("operator") is None:
@ -112,7 +122,7 @@ def clean_property(property: dict):
def is_property_with_operator(property: dict):
return property.get("type") not in ("cohort", "hogql")
return property.get("type") not in ("hogql",)
# old style dict properties e.g. {"utm_medium__icontains": "email"}

32
posthog/hogql_queries/legacy_compatibility/test/test_clean_properties.py
View File

@ -40,7 +40,7 @@ class TestCleanGlobalProperties(BaseTest):
"values": [
{
"type": "AND",
"values": [{"key": "id", "type": "cohort", "value": 636}],
"values": [{"key": "id", "type": "cohort", "operator": "in", "value": 636}],
}
],
},
@ -61,7 +61,33 @@ class TestCleanGlobalProperties(BaseTest):
"values": [
{
"type": "AND",
"values": [{"key": "id", "type": "cohort", "value": 850}],
"values": [{"key": "id", "type": "cohort", "operator": "in", "value": 850}],
}
],
},
)
def test_handles_cohort_negation(self):
properties = {
"type": "AND",
"values": [
{
"type": "AND",
"values": [{"key": "id", "type": "cohort", "value": 850, "operator": None, "negation": True}],
}
],
}
result = clean_global_properties(properties)
self.assertEqual(
result,
{
"type": "AND",
"values": [
{
"type": "AND",
"values": [{"key": "id", "type": "cohort", "operator": "not_in", "value": 850}],
}
],
},
@ -82,7 +108,7 @@ class TestCleanGlobalProperties(BaseTest):
"values": [
{
"type": "AND",
"values": [{"key": "id", "type": "cohort", "value": 850}],
"values": [{"key": "id", "type": "cohort", "operator": "in", "value": 850}],
}
],
},

5
posthog/models/filters/mixins/simplify.py
View File

@ -2,6 +2,7 @@ from typing import TYPE_CHECKING, Any, Literal, TypeVar, cast
from posthog.constants import PropertyOperatorType
from posthog.models.property import GroupTypeIndex, PropertyGroup
from posthog.schema import PropertyOperator
if TYPE_CHECKING: # Avoid circular import
from posthog.models import Property, Team
@ -112,7 +113,9 @@ class SimplifyFilterMixin:
# :TODO: Handle non-existing resource in-query instead
return PropertyGroup(type=PropertyOperatorType.AND, values=[property])
return simplified_cohort_filter_properties(cohort, team, property.negation)
return simplified_cohort_filter_properties(
cohort, team, property.negation or property.operator == PropertyOperator.NOT_IN.value
)
# PropertyOperatorType doesn't really matter here, since only one value.
return PropertyGroup(type=PropertyOperatorType.AND, values=[property])