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 9 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
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
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)
17 changes: 17 additions & 0 deletions ee/migrations/0022_enterprisepropertydefinition_hidden.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Generated by Django 4.2.18 on 2025-02-21 04:20

from django.db import migrations, models


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

operations = [
migrations.AddField(
model_name="enterprisepropertydefinition",
name="hidden",
field=models.BooleanField(blank=True, default=False, null=True),
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider making null=False since default=False is provided. Having both nullable and a default value can lead to ambiguous states.

),
]
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_enterprisepropertydefinition_hidden
1 change: 1 addition & 0 deletions ee/models/property_definition.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class EnterprisePropertyDefinition(PropertyDefinition):
blank=True,
related_name="property_verifying_user",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

hidden_at, hidden_by (following the verified patter) could be helpful, esp if we don't have audit logs on prop defs (which I'm not sure whether or not we do)

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 think I'd rather this go into the activity log tbh... and I don't feel like doing that now 😬 Is that cool with you or do you really think it should be included?

hidden = models.BooleanField(blank=True, null=True, default=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider making hidden field non-nullable since it has a default value. This would simplify logic and prevent potential null checks.

Suggested change
hidden = models.BooleanField(blank=True, null=True, default=False)
hidden = models.BooleanField(blank=True, default=False)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good suggestion, same as verified above

Copy link
Member Author

Choose a reason for hiding this comment

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

We have to allow null I think so that the existing rows don't all have to be updated, if anything I would remove the default. Thoughts?

Copy link
Contributor

@zlwaterfield zlwaterfield Feb 27, 2025

Choose a reason for hiding this comment

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

When you create a non-nullable column with a default value in postgres any existing rows in the table will be automatically filled with that default value.

Copy link
Member Author

@raquelmsmith raquelmsmith Feb 27, 2025

Choose a reason for hiding this comment

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

Yeah exactly, and I don't want to have it touch every single row to fill them in with the new default as this can lock up the table (though maybe in newer versions of postgres this is less of an issue?). So saying it can be null means all existing rows are left alone and new ones just use the default.


# 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