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

feat(prop-defs): add hidden option to property definitions #29085

Merged
merged 20 commits into from
Mar 1, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 26 additions & 7 deletions ee/api/ee_event_definition.py
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
from typing import cast

import posthoganalytics
from django.utils import timezone
from loginas.utils import is_impersonated_session
from rest_framework import serializers

from ee.models.event_definition import EnterpriseEventDefinition
from posthog.api.shared import UserBasicSerializer
from posthog.api.tagged_item import TaggedItemSerializerMixin
from posthog.event_usage import groups
from posthog.models import User
from posthog.models.activity_logging.activity_log import (
Detail,
dict_changes_between,
log_activity,
Detail,
)

from loginas.utils import is_impersonated_session
from typing import cast
import posthoganalytics
from posthog.event_usage import groups
from posthog.models import User


class EnterpriseEventDefinitionSerializer(TaggedItemSerializerMixin, serializers.ModelSerializer):
updated_by = UserBasicSerializer(read_only=True)
Expand Down Expand Up @@ -44,6 +44,7 @@ class Meta:
"verified",
"verified_at",
"verified_by",
"hidden",
# Action fields
"is_action",
"action_id",
Expand All @@ -69,9 +70,27 @@ class Meta:
"created_by",
]

def validate(self, data):
validated_data = super().validate(data)

if "hidden" in validated_data and "verified" in validated_data:
if validated_data["hidden"] and validated_data["verified"]:
raise serializers.ValidationError("An event cannot be both hidden and verified")

return validated_data

def update(self, event_definition: EnterpriseEventDefinition, validated_data):
validated_data["updated_by"] = self.context["request"].user

# If setting hidden=True, ensure verified becomes false
if validated_data.get("hidden", False):
validated_data["verified"] = False
validated_data["verified_by"] = None
validated_data["verified_at"] = None
# If setting verified=True, ensure hidden becomes false
elif validated_data.get("verified", False):
validated_data["hidden"] = False

if "verified" in validated_data:
if validated_data["verified"] and not event_definition.verified:
# Verify event only if previously unverified
Expand Down
35 changes: 31 additions & 4 deletions ee/api/ee_property_definition.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
from rest_framework import serializers
from django.utils import timezone
from loginas.utils import is_impersonated_session
from rest_framework import serializers

from ee.models.property_definition import EnterprisePropertyDefinition
from posthog.api.shared import UserBasicSerializer
from posthog.api.tagged_item import TaggedItemSerializerMixin
from posthog.models import PropertyDefinition
from posthog.models.activity_logging.activity_log import (
Detail,
dict_changes_between,
log_activity,
Detail,
)
from loginas.utils import is_impersonated_session


class EnterprisePropertyDefinitionSerializer(TaggedItemSerializerMixin, serializers.ModelSerializer):
Expand All @@ -31,6 +32,7 @@ class Meta:
"verified",
"verified_at",
"verified_by",
"hidden",
)
read_only_fields = [
"id",
Expand All @@ -41,7 +43,32 @@ class Meta:
"verified_by",
]

def update(self, property_definition: EnterprisePropertyDefinition, validated_data):
def validate(self, data):
validated_data = super().validate(data)

if "hidden" in validated_data and "verified" in validated_data:
if validated_data["hidden"] and validated_data["verified"]:
raise serializers.ValidationError("A property cannot be both hidden and verified")

# If setting hidden=True, ensure verified becomes false
if validated_data.get("hidden", False):
validated_data["verified"] = False
# If setting verified=True, ensure hidden becomes false
elif validated_data.get("verified", False):
validated_data["hidden"] = False

return validated_data

def update(self, property_definition: EnterprisePropertyDefinition, validated_data: dict):
# If setting hidden=True, ensure verified becomes false
if validated_data.get("hidden", False):
validated_data["verified"] = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this logic duplicated here? validate should be called before update so if it was all in validate then should be covered?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know 🙈

@Twixes how are the multiple API endpoints used - there is one for ee and one for non-ee. My ee tests didn't pass without the ee api changes so I just copied them over...

validated_data["verified_by"] = None
validated_data["verified_at"] = None
# If setting verified=True, ensure hidden becomes false
elif validated_data.get("verified", False):
validated_data["hidden"] = False

validated_data["updated_by"] = self.context["request"].user
if "property_type" in validated_data:
if validated_data["property_type"] == "Numeric":
Expand Down
51 changes: 46 additions & 5 deletions ee/api/test/test_event_definition.py
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
from datetime import datetime
from typing import cast, Optional, Any
from typing import Any, Optional, cast

import dateutil.parser
from django.utils import timezone
from freezegun import freeze_time
from rest_framework import status

from ee.models.event_definition import EnterpriseEventDefinition
from ee.models.license import License, LicenseManager
from posthog.api.test.test_event_definition import capture_event, EventData
from ee.models.license import AvailableFeature, License, LicenseManager
from posthog.api.test.test_event_definition import EventData, capture_event
from posthog.api.test.test_organization import create_organization
from posthog.api.test.test_team import create_team
from posthog.api.test.test_user import create_user
from posthog.models import Tag, ActivityLog, Team, User
from posthog.models import ActivityLog, Tag, Team, User
from posthog.models.event_definition import EventDefinition
from posthog.test.base import APIBaseTest
from posthog.api.test.test_organization import create_organization


@freeze_time("2020-01-02")
Expand Down Expand Up @@ -368,6 +368,47 @@ def test_event_definition_no_duplicate_tags(self):

self.assertListEqual(sorted(response.json()["tags"]), ["a", "b"])

def test_exclude_hidden_events(self):
super(LicenseManager, cast(LicenseManager, License.objects)).create(
plan="enterprise", valid_until=timezone.datetime(2500, 1, 19, 3, 14, 7)
)
# Create some events with hidden flag
EnterpriseEventDefinition.objects.create(team=self.demo_team, name="visible_event")
EnterpriseEventDefinition.objects.create(team=self.demo_team, name="hidden_event1", hidden=True)
EnterpriseEventDefinition.objects.create(team=self.demo_team, name="hidden_event2", hidden=True)

# Test without enterprise taxonomy - hidden events should still be shown even with exclude_hidden=true
self.organization.available_features = []
self.organization.save()

response = self.client.get(f"/api/projects/{self.demo_team.pk}/event_definitions/?exclude_hidden=true")
assert response.status_code == status.HTTP_200_OK
event_names = {p["name"] for p in response.json()["results"]}
assert "visible_event" in event_names
assert "hidden_event1" in event_names
assert "hidden_event2" in event_names

# Test with enterprise taxonomy enabled - hidden events should be excluded when exclude_hidden=true
self.demo_team.organization.available_product_features = [
{"key": AvailableFeature.INGESTION_TAXONOMY, "name": "ingestion-taxonomy"}
]
self.demo_team.organization.save()

response = self.client.get(f"/api/projects/{self.demo_team.pk}/event_definitions/?exclude_hidden=true")
assert response.status_code == status.HTTP_200_OK
event_names = {p["name"] for p in response.json()["results"]}
assert "visible_event" in event_names
assert "hidden_event1" not in event_names
assert "hidden_event2" not in event_names

# Test with exclude_hidden=false (should be same as not setting it)
response = self.client.get(f"/api/projects/{self.demo_team.pk}/event_definitions/?exclude_hidden=false")
assert response.status_code == status.HTTP_200_OK
event_names = {p["name"] for p in response.json()["results"]}
assert "visible_event" in event_names
assert "hidden_event1" in event_names
assert "hidden_event2" in event_names

def test_event_type_event(self):
super(LicenseManager, cast(LicenseManager, License.objects)).create(
plan="enterprise", valid_until=timezone.datetime(2500, 1, 19, 3, 14, 7)
Expand Down
122 changes: 117 additions & 5 deletions ee/api/test/test_property_definition.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
from typing import cast, Optional
from freezegun import freeze_time
from typing import Optional, cast

import pytest
from django.db.utils import IntegrityError
from django.utils import timezone
from freezegun import freeze_time
from rest_framework import status

from ee.models.license import License, LicenseManager
from ee.models.property_definition import EnterprisePropertyDefinition
from posthog.models import EventProperty, Tag, ActivityLog
from posthog.constants import AvailableFeature
from posthog.models import ActivityLog, EventProperty, Tag
from posthog.models.property_definition import PropertyDefinition
from posthog.test.base import APIBaseTest

Expand Down Expand Up @@ -378,6 +380,72 @@ def test_verify_then_unverify(self):
assert response.json()["verified_by"] is None
assert response.json()["verified_at"] is None

@freeze_time("2021-08-25T22:09:14.252Z")
def test_hidden_property_behavior(self):
super(LicenseManager, cast(LicenseManager, License.objects)).create(
plan="enterprise", valid_until=timezone.datetime(2500, 1, 19, 3, 14, 7)
)
property = EnterprisePropertyDefinition.objects.create(team=self.team, name="hidden test property")
response = self.client.get(f"/api/projects/@current/property_definitions/{property.id}")
self.assertEqual(response.status_code, status.HTTP_200_OK)

assert response.json()["hidden"] is False
assert response.json()["verified"] is False

# Hide the property
self.client.patch(
f"/api/projects/@current/property_definitions/{property.id}",
{"hidden": True},
)
response = self.client.get(f"/api/projects/@current/property_definitions/{property.id}")
self.assertEqual(response.status_code, status.HTTP_200_OK)

assert response.json()["hidden"] is True
assert response.json()["verified"] is False # Hiding should ensure verified is False

# Try to verify and hide a property at the same time (should fail)
response = self.client.patch(
f"/api/projects/@current/property_definitions/{property.id}",
{"verified": True, "hidden": True},
)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
self.assertIn("cannot be both hidden and verified", response.json()["detail"].lower())

# Unhide the property
self.client.patch(
f"/api/projects/@current/property_definitions/{property.id}",
{"hidden": False},
)
response = self.client.get(f"/api/projects/@current/property_definitions/{property.id}")
self.assertEqual(response.status_code, status.HTTP_200_OK)

assert response.json()["hidden"] is False
assert response.json()["verified"] is False

@freeze_time("2021-08-25T22:09:14.252Z")
def test_marking_hidden_removes_verified_status(self):
super(LicenseManager, cast(LicenseManager, License.objects)).create(
plan="enterprise", valid_until=timezone.datetime(2500, 1, 19, 3, 14, 7)
)
property = EnterprisePropertyDefinition.objects.create(
team=self.team, name="verified test property", verified=True
)
response = self.client.get(f"/api/projects/@current/property_definitions/{property.id}")
self.assertEqual(response.status_code, status.HTTP_200_OK)

assert response.json()["hidden"] is False
assert response.json()["verified"] is True

response = self.client.patch(
f"/api/projects/@current/property_definitions/{property.id}",
{"hidden": True},
)
self.assertEqual(response.status_code, status.HTTP_200_OK)
property.refresh_from_db()
assert property.hidden is True
assert property.verified is False

@freeze_time("2021-08-25T22:09:14.252Z")
def test_verify_then_verify_again_no_change(self):
super(LicenseManager, cast(LicenseManager, License.objects)).create(
plan="enterprise", valid_until=timezone.datetime(2500, 1, 19, 3, 14, 7)
Expand Down Expand Up @@ -445,7 +513,7 @@ def test_cannot_update_verified_meta_properties_directly(self):
assert response.json()["verified_by"] is None
assert response.json()["verified_at"] is None

def test_list_property_definitions(self):
def test_list_property_definitions_verified_ordering(self):
super(LicenseManager, cast(LicenseManager, License.objects)).create(
plan="enterprise", valid_until=timezone.datetime(2500, 1, 19, 3, 14, 7)
)
Expand Down Expand Up @@ -494,7 +562,6 @@ def test_list_property_definitions(self):
]

# We should prefer properties that have been seen on an event if that is available

EventProperty.objects.get_or_create(team=self.team, event="$pageview", property="3_when_verified")
EventProperty.objects.get_or_create(team=self.team, event="$pageview", property="4_when_verified")

Expand All @@ -508,3 +575,48 @@ def test_list_property_definitions(self):
("5_when_verified", False, False),
("6_when_verified", False, False),
]

def test_exclude_hidden_properties(self):
super(LicenseManager, cast(LicenseManager, License.objects)).create(
plan="enterprise", valid_until=timezone.datetime(2500, 1, 19, 3, 14, 7)
)
# Create some properties with hidden flag
EnterprisePropertyDefinition.objects.create(team=self.team, name="visible_property", property_type="String")
EnterprisePropertyDefinition.objects.create(
team=self.team, name="hidden_property1", property_type="String", hidden=True
)
EnterprisePropertyDefinition.objects.create(
team=self.team, name="hidden_property2", property_type="String", hidden=True
)

# Test without enterprise taxonomy - hidden properties should still be shown even with exclude_hidden=true
self.organization.available_features = []
self.organization.save()

response = self.client.get(f"/api/projects/{self.team.pk}/property_definitions/?exclude_hidden=true")
self.assertEqual(response.status_code, status.HTTP_200_OK)
property_names = {p["name"] for p in response.json()["results"]}
self.assertIn("visible_property", property_names)
self.assertIn("hidden_property1", property_names)
self.assertIn("hidden_property2", property_names)

# Test with enterprise taxonomy enabled - hidden properties should be excluded when exclude_hidden=true
self.team.organization.available_product_features = [
{"key": AvailableFeature.INGESTION_TAXONOMY, "name": "ingestion-taxonomy"}
]
self.team.organization.save()

response = self.client.get(f"/api/projects/{self.team.pk}/property_definitions/?exclude_hidden=true")
self.assertEqual(response.status_code, status.HTTP_200_OK)
property_names = {p["name"] for p in response.json()["results"]}
self.assertIn("visible_property", property_names)
self.assertNotIn("hidden_property1", property_names)
self.assertNotIn("hidden_property2", property_names)

# Test with exclude_hidden=false (should be same as not setting it)
response = self.client.get(f"/api/projects/{self.team.pk}/property_definitions/?exclude_hidden=false")
self.assertEqual(response.status_code, status.HTTP_200_OK)
property_names = {p["name"] for p in response.json()["results"]}
self.assertIn("visible_property", property_names)
self.assertIn("hidden_property1", property_names)
self.assertIn("hidden_property2", property_names)
22 changes: 22 additions & 0 deletions ee/migrations/0022_enterpriseeventdefinition_hidden_and_more.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Generated by Django 4.2.18 on 2025-02-27 19:54

from django.db import migrations, models


class Migration(migrations.Migration):
dependencies = [
("ee", "0021_conversation_status"),
]

operations = [
migrations.AddField(
model_name="enterpriseeventdefinition",
name="hidden",
field=models.BooleanField(blank=True, default=False, null=True),
),
migrations.AddField(
model_name="enterprisepropertydefinition",
name="hidden",
field=models.BooleanField(blank=True, default=False, null=True),
),
]
2 changes: 1 addition & 1 deletion ee/migrations/max_migration.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0021_conversation_status
0022_enterpriseeventdefinition_hidden_and_more
1 change: 1 addition & 0 deletions ee/models/event_definition.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class EnterpriseEventDefinition(EventDefinition):
blank=True,
related_name="verifying_user",
)
hidden = models.BooleanField(null=True, blank=True, default=False)

# Deprecated in favour of app-wide tagging model. See EnterpriseTaggedItem
deprecated_tags: ArrayField = ArrayField(models.CharField(max_length=32), null=True, blank=True, default=list)
Expand Down
Loading
Loading