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

raquelmsmith · 2025-02-21T21:11:56Z

Problem

PostHog ingests a lot of properties, and sometimes it can be confusing for posthog users of certain orgs to know which properties they should be using. For example, do they use the "Country" person property, or the "Latest country" person property? They might not even realize that "Latest country" is an option if they just scroll and see "Country" - even if that is the preferred prop that their org wants them to use.

We have a way to mark properties as "Verified" - ie someone at the org says "this is a good one, use it." But we have no way of saying "this is not a good one, don't use it."

Changes

Adds a "hidden" field to the property definition model. Allows someone to select between Verified / Visible / Hidden for any property - except "core" PostHog properties can't be "Verified" (why? because that's how it originally was so I didn't change it 🤷‍♀️ - they can still be hidden though)

NB: I decided to just add a new field for "hidden" and deal with the conflict of that vs verified in the business logic (eg we don't want to end up with a prop that is both verified and hidden). I could have made a new field like "visibility -> hidden / visible / verified" and migrated it over but it seemed like there was limited benefit to that and I wasn't sure if there was a future where we might want some combination of hidden + verified. So.... I went with the logic instead.

If a property is hidden it still shows up in the Data Management area, but not in the filter property selector thing.

Bunch o screenshots:

apparently the prop dev view page didn't say if it was verified so.... added that:

Neither did the popover, how is someone supposed to know if it's verified / preferred if it doesn't show up?!? 😆

Popover edit (ignore that it says "standard" - i've changed to "visible"):

No screenshots for API changes and validation but it should all be there..

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

N/A - cloud only, teams plan only!

How did you test this code?

Tests, 👀

raquelmsmith · 2025-02-21T21:12:47Z

Random review request from you @Twixes because I feel like you've touched all these things in the past, and you were a suggested reviewer 😏

greptile-apps

PR Summary

Based on the provided files, I'll summarize the key changes in this PR:

This PR adds a hidden property status feature to property definitions in PostHog, enabling better property management. Here are the key points:

Added hidden boolean field to EnterprisePropertyDefinition model with default=False, allowing properties to be marked as hidden from property selectors
Introduced new PropertyStatusControl component replacing VerifiedDefinitionCheckbox, offering three states: Verified, Standard (visible), or Hidden
Added validation ensuring properties cannot be both hidden and verified simultaneously - hiding a verified property unverifies it and vice versa
Implemented exclude_hidden parameter in API endpoints to filter out hidden properties when enterprise taxonomy feature is enabled
Added visual indicators in UI (definition view, popovers) to clearly show property verification status with appropriate icons and tooltips

The changes help organizations better manage their properties by allowing them to hide deprecated or unwanted properties while keeping them accessible in the Data Management area.

_{13 file(s) reviewed, 12 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-02-21T21:13:20Z

ee/migrations/0022_enterprisepropertydefinition_hidden.py

+        migrations.AddField(
+            model_name="enterprisepropertydefinition",
+            name="hidden",
+            field=models.BooleanField(blank=True, default=False, null=True),


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

greptile-apps · 2025-02-21T21:13:52Z

ee/models/property_definition.py

@@ -19,6 +19,7 @@ class EnterprisePropertyDefinition(PropertyDefinition):
        blank=True,
        related_name="property_verifying_user",
    )
+    hidden = models.BooleanField(blank=True, null=True, default=False)


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)

This is a good suggestion, same as verified above

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?

frontend/src/lib/components/DefinitionPopover/DefinitionPopoverContents.tsx

frontend/src/scenes/data-management/definition/DefinitionEdit.tsx

frontend/src/scenes/data-management/definition/DefinitionView.tsx

frontend/src/scenes/data-management/definition/definitionLogic.ts

greptile-apps · 2025-02-21T21:15:52Z

frontend/src/types.ts


+export type PropertyDefinitionVerificationStatus = 'verified' | 'hidden' | 'standard'


style: Consider using an enum instead of a union type for better type safety and IDE support

posthog/api/test/test_property_definition.py

posthog/taxonomy/property_definition_api.py

github-actions · 2025-02-25T04:58:23Z

Size Change: +1.63 kB (+0.02%)

Total Size: 9.72 MB

Filename	Size	Change
`frontend/dist/toolbar.js`	9.72 MB	+1.63 kB (+0.02%)

_{compressed-size-action}

zlwaterfield · 2025-02-25T14:26:22Z

ee/models/property_definition.py

@@ -19,6 +19,7 @@ class EnterprisePropertyDefinition(PropertyDefinition):
        blank=True,
        related_name="property_verifying_user",
    )
+    hidden = models.BooleanField(blank=True, null=True, default=False)


This is a good suggestion, same as verified above

zlwaterfield · 2025-02-25T14:27:24Z

ee/models/property_definition.py

@@ -19,6 +19,7 @@ class EnterprisePropertyDefinition(PropertyDefinition):
        blank=True,
        related_name="property_verifying_user",
    )


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)

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?

zlwaterfield · 2025-02-25T14:29:52Z

ee/api/ee_property_definition.py

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


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

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...

zlwaterfield · 2025-02-25T14:35:39Z

posthog/taxonomy/property_definition_api.py

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


I don't fully understand why there are two view sets for updating prop defs, but maybe we should update the logic here to be the same, where you also clear the verified at and verified by.

Twixes

Very vaguely involved with definitions, but I'm always in favor of great UX, and I think we're pretty close here! However, some things to smooth out before we merge

frontend/src/scenes/data-management/definition/DefinitionView.tsx

frontend/src/scenes/data-management/definition/definitionLogic.ts

Twixes · 2025-02-25T21:58:41Z

posthog/taxonomy/property_definition_api.py

+    exclude_hidden = serializers.BooleanField(
+        help_text="Whether to exclude properties marked as hidden",
+        required=False,
+        default=False,
+    )


It seems it should be the inverse, hidden properties should be hidden unless you opt into including them (we only want to include in the property definitions scene)

I kind of disagree - I feel like it's better to include everything by default in the endpoint and use filters to narrow it down, not broaden the scope.

Hidden can make you think it shouldn't be automatically included because of the field name, but in reality the hiding is a UI thing, it's not like it's archived (if it was an archive or delete option I'd agree with you that it should be excluded by default).

frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.tsx

frontend/src/scenes/data-management/definition/DefinitionView.tsx

frontend/src/lib/components/DefinitionPopover/DefinitionPopoverContents.tsx

into feat/hide-prop-defs

raquelmsmith added 5 commits February 20, 2025 21:22

add hidden field to prop defs

7e0849a

allow hiding of core props and show in popover

0d5f776

add exclude_hidden to api serializer

96e3063

Merge branch 'master' into feat/hide-prop-defs

b002853

add migration files

4c1a568

raquelmsmith requested review from zlwaterfield and Twixes February 21, 2025 21:12

greptile-apps bot reviewed Feb 21, 2025

View reviewed changes

raquelmsmith added 3 commits February 21, 2025 13:21

standard -> visible

08271a1

address PR feedback

8051106

fix tests

b0182a0

zlwaterfield reviewed Feb 25, 2025

View reviewed changes

Merge branch 'master' into feat/hide-prop-defs

df7d08a

Twixes requested changes Feb 25, 2025

View reviewed changes

raquelmsmith added 5 commits February 25, 2025 19:11

pr feedback

47fdd9e

hide item from infinite list if marked hidden

63268e3

use verified icon

2f46373

show verified info in the infinite list directly

12f860c

posthog props are inherently verified

eecb4cd

raquelmsmith requested a review from Twixes February 26, 2025 04:39

Merge branch 'feat/hide-prop-defs' of https://github.com/PostHog/posthog

916cc42

into feat/hide-prop-defs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

raquelmsmith commented Feb 21, 2025 •

edited

Loading

raquelmsmith commented Feb 21, 2025

greptile-apps bot left a comment

greptile-apps bot Feb 21, 2025

greptile-apps bot Feb 21, 2025

zlwaterfield Feb 25, 2025

raquelmsmith Feb 25, 2025

greptile-apps bot Feb 21, 2025

github-actions bot commented Feb 25, 2025 •

edited

Loading

zlwaterfield Feb 25, 2025

zlwaterfield Feb 25, 2025

raquelmsmith Feb 25, 2025

zlwaterfield Feb 25, 2025

raquelmsmith Feb 25, 2025

zlwaterfield Feb 25, 2025

Twixes left a comment

Twixes Feb 25, 2025 •

edited

Loading

raquelmsmith Feb 26, 2025

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


		export type PropertyDefinitionVerificationStatus = 'verified' \| 'hidden' \| 'standard'

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

Are you sure you want to change the base?

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

Conversation

raquelmsmith commented Feb 21, 2025 • edited Loading

Problem

Changes

Does this work well for both Cloud and self-hosted?

How did you test this code?

raquelmsmith commented Feb 21, 2025

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Feb 21, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot Feb 21, 2025

Choose a reason for hiding this comment

github-actions bot commented Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Twixes left a comment

Choose a reason for hiding this comment

Twixes Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raquelmsmith commented Feb 21, 2025 •

edited

Loading

github-actions bot commented Feb 25, 2025 •

edited

Loading

Twixes Feb 25, 2025 •

edited

Loading