diff --git a/ee/clickhouse/models/test/test_filters.py b/ee/clickhouse/models/test/test_filters.py index 67542868e7a..96cc887df49 100644 --- a/ee/clickhouse/models/test/test_filters.py +++ b/ee/clickhouse/models/test/test_filters.py @@ -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}], } }, ) diff --git a/posthog/hogql/property.py b/posthog/hogql/property.py index ff477d98cd9..10eb991ea8f 100644 --- a/posthog/hogql/property.py +++ b/posthog/hogql/property.py @@ -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), ) diff --git a/posthog/hogql_queries/legacy_compatibility/clean_properties.py b/posthog/hogql_queries/legacy_compatibility/clean_properties.py index e77cf1ee1f9..0c2e764c2bf 100644 --- a/posthog/hogql_queries/legacy_compatibility/clean_properties.py +++ b/posthog/hogql_queries/legacy_compatibility/clean_properties.py @@ -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"} diff --git a/posthog/hogql_queries/legacy_compatibility/test/test_clean_properties.py b/posthog/hogql_queries/legacy_compatibility/test/test_clean_properties.py index e3e5fd91acd..54568ee8826 100644 --- a/posthog/hogql_queries/legacy_compatibility/test/test_clean_properties.py +++ b/posthog/hogql_queries/legacy_compatibility/test/test_clean_properties.py @@ -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}], } ], }, diff --git a/posthog/models/filters/mixins/simplify.py b/posthog/models/filters/mixins/simplify.py index 72d8d184539..b152e07113f 100644 --- a/posthog/models/filters/mixins/simplify.py +++ b/posthog/models/filters/mixins/simplify.py @@ -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])