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.

Sign up for GitHub

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(web-analytics): Create group property filters correctly in hogql property_to_expr #19981

Merged
merged 3 commits into from
Jan 26, 2024
Merged

fix(web-analytics): Create group property filters correctly in hogql property_to_expr #19981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e0cd051
fx(web-analytics): Create group property filters correctly
robbie-c Jan 26, 2024
43c77e4
Fix error message in test
robbie-c Jan 26, 2024
eb48e57
Add a test which failed but is fixed by this PR
robbie-c Jan 26, 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
2 changes: 2 additions & 0 deletions posthog/hogql/property.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ def property_to_expr(
type=property.type,
key=property.key,
operator=property.operator,
group_type_index=property.group_type_index,
value=v,
),
team,
Expand Down Expand Up @@ -263,6 +264,7 @@ def property_to_expr(
type=property.type,
key=property.key,
operator=property.operator,
group_type_index=property.group_type_index,
value=v,
),
team,
Expand Down
6 changes: 5 additions & 1 deletion posthog/hogql/test/test_property.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,16 @@ def test_property_to_expr_group(self):
{"type": "group", "group_type_index": 0, "key": "a", "value": "b", "operator": "is_not_set"}
),
)
self.assertEqual(
self._property_to_expr(Property(type="group", group_type_index=0, key="a", value=["b", "c"])),
self._parse_expr("group_0.properties.a = 'b' OR group_0.properties.a = 'c'"),
)

with self.assertRaises(Exception) as e:
self._property_to_expr({"type": "group", "key": "a", "value": "b"})
self.assertEqual(
str(e.exception),
"Missing required key group_type_index for property type group with name a",
"Missing required attr group_type_index for property type group with key a",
)

def test_property_to_expr_event(self):
Expand Down
12 changes: 6 additions & 6 deletions posthog/models/property/property.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,14 +253,14 @@ def __init__(
if self.type not in VALIDATE_PROP_TYPES.keys():
raise ValueError(f"Invalid property type: {self.type}")

for key in VALIDATE_PROP_TYPES[self.type]:
if getattr(self, key, None) is None:
raise ValueError(f"Missing required key {key} for property type {self.type} with name {self.key}")
for attr in VALIDATE_PROP_TYPES[self.type]:
if getattr(self, attr, None) is None:
raise ValueError(f"Missing required attr {attr} for property type {self.type} with key {self.key}")

if self.type == "behavioral":
for key in VALIDATE_BEHAVIORAL_PROP_TYPES[cast(BehavioralPropertyType, self.value)]:
if getattr(self, key, None) is None:
raise ValueError(f"Missing required key {key} for property type {self.type}::{self.value}")
for attr in VALIDATE_BEHAVIORAL_PROP_TYPES[cast(BehavioralPropertyType, self.value)]:
if getattr(self, attr, None) is None:
raise ValueError(f"Missing required attr {attr} for property type {self.type}::{self.value}")

def __repr__(self):
params_repr = ", ".join(f"{key}={repr(value)}" for key, value in self.to_dict().items())
Expand Down
Loading