diff --git a/ee/api/ee_event_definition.py b/ee/api/ee_event_definition.py index 2c36958e505c7..d90c079726e8d 100644 --- a/ee/api/ee_event_definition.py +++ b/ee/api/ee_event_definition.py @@ -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) @@ -44,6 +44,7 @@ class Meta: "verified", "verified_at", "verified_by", + "hidden", # Action fields "is_action", "action_id", @@ -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 diff --git a/ee/api/ee_property_definition.py b/ee/api/ee_property_definition.py index 308e7461942ca..4478cb68178f9 100644 --- a/ee/api/ee_property_definition.py +++ b/ee/api/ee_property_definition.py @@ -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): @@ -31,6 +32,7 @@ class Meta: "verified", "verified_at", "verified_by", + "hidden", ) read_only_fields = [ "id", @@ -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 + 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": diff --git a/ee/api/test/test_event_definition.py b/ee/api/test/test_event_definition.py index 2aa87e63e2e65..972cd90db7756 100644 --- a/ee/api/test/test_event_definition.py +++ b/ee/api/test/test_event_definition.py @@ -1,5 +1,5 @@ from datetime import datetime -from typing import cast, Optional, Any +from typing import Any, Optional, cast import dateutil.parser from django.utils import timezone @@ -7,14 +7,14 @@ 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") @@ -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) diff --git a/ee/api/test/test_property_definition.py b/ee/api/test/test_property_definition.py index effa43a9f4b18..42c55b5ca3d49 100644 --- a/ee/api/test/test_property_definition.py +++ b/ee/api/test/test_property_definition.py @@ -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 @@ -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) @@ -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) ) @@ -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") @@ -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) diff --git a/ee/migrations/0022_enterpriseeventdefinition_hidden_and_more.py b/ee/migrations/0022_enterpriseeventdefinition_hidden_and_more.py new file mode 100644 index 0000000000000..202fe413f963c --- /dev/null +++ b/ee/migrations/0022_enterpriseeventdefinition_hidden_and_more.py @@ -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), + ), + ] diff --git a/ee/migrations/max_migration.txt b/ee/migrations/max_migration.txt index 0dd473a0b9c5e..9cb4a4808bc3a 100644 --- a/ee/migrations/max_migration.txt +++ b/ee/migrations/max_migration.txt @@ -1 +1 @@ -0021_conversation_status +0022_enterpriseeventdefinition_hidden_and_more diff --git a/ee/models/event_definition.py b/ee/models/event_definition.py index fc172c4ac3c8f..aa4ec79acd9bf 100644 --- a/ee/models/event_definition.py +++ b/ee/models/event_definition.py @@ -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) diff --git a/ee/models/property_definition.py b/ee/models/property_definition.py index 3354afacb4184..b339cc98e6ebb 100644 --- a/ee/models/property_definition.py +++ b/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) # 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) diff --git a/frontend/src/lib/components/DefinitionPopover/DefinitionPopoverContents.tsx b/frontend/src/lib/components/DefinitionPopover/DefinitionPopoverContents.tsx index a9df100402cb5..6b8a82ba3820c 100644 --- a/frontend/src/lib/components/DefinitionPopover/DefinitionPopoverContents.tsx +++ b/frontend/src/lib/components/DefinitionPopover/DefinitionPopoverContents.tsx @@ -1,6 +1,6 @@ import { hide } from '@floating-ui/react' -import { IconInfo } from '@posthog/icons' -import { LemonButton, LemonDivider, LemonSelect, LemonSwitch } from '@posthog/lemon-ui' +import { IconBadge, IconEye, IconHide } from '@posthog/icons' +import { LemonButton, LemonDivider, LemonSegmentedButton, LemonSelect, LemonTag } from '@posthog/lemon-ui' import { useActions, useValues } from 'kea' import { ActionPopoverInfo } from 'lib/components/DefinitionPopover/ActionPopoverInfo' import { CohortPopoverInfo } from 'lib/components/DefinitionPopover/CohortPopoverInfo' @@ -23,48 +23,86 @@ import { CORE_FILTER_DEFINITIONS_BY_GROUP, isCoreFilter } from 'lib/taxonomy' import { Fragment, useEffect, useMemo } from 'react' import { DataWarehouseTableForInsight } from 'scenes/data-warehouse/types' -import { ActionType, CohortType, EventDefinition, PropertyDefinition } from '~/types' +import { + ActionType, + CohortType, + EventDefinition, + PropertyDefinition, + PropertyDefinitionVerificationStatus, +} from '~/types' import { HogQLDropdown } from '../HogQLDropdown/HogQLDropdown' import { taxonomicFilterLogic } from '../TaxonomicFilter/taxonomicFilterLogic' import { TZLabel } from '../TZLabel' -export function VerifiedDefinitionCheckbox({ +export function PropertyStatusControl({ verified, - isProperty, + hidden, + showHiddenOption, + allowVerification, onChange, compact = false, + isProperty, }: { verified: boolean - isProperty: boolean - onChange: (nextVerified: boolean) => void + hidden: boolean + showHiddenOption: boolean + allowVerification: boolean + onChange: (status: { verified: boolean; hidden: boolean }) => void compact?: boolean + isProperty: boolean }): JSX.Element { - const copy = isProperty - ? 'Verifying a property is a signal to collaborators that this property should be used in favor of similar properties.' - : 'Verified events are prioritized in filters and other selection components. Verifying an event is a signal to collaborators that this event should be used in favor of similar events.' + const definitionType = isProperty ? 'property' : 'event' + const copy = { + verified: `Prioritize this ${definitionType} in filters and other selection components to signal to collaborators that this ${definitionType} should be used in favor of similar ${definitionType}s.`, + visible: `${ + definitionType.charAt(0).toUpperCase() + definitionType.slice(1) + } is available for use but has not been verified by the team.`, + hidden: `Hide this ${definitionType} from filters and other selection components by default. Use this for deprecated or irrelevant ${definitionType}s.`, + } + + const verifiedDisabledCorePropCopy = `Core PostHog ${definitionType}s are inherently treated as if verified, but they can still be hidden.` + + const currentStatus: PropertyDefinitionVerificationStatus = hidden ? 'hidden' : verified ? 'verified' : 'visible' return ( <> - {!compact &&

{copy}

} - - { - onChange(!verified) + { + const status = value as PropertyDefinitionVerificationStatus + onChange({ + verified: status === 'verified', + hidden: status === 'hidden', + }) }} - bordered - label={ - <> - Mark as verified {isProperty ? 'property' : 'event'} - {compact && ( - - - - )} - - } + options={[ + { + value: 'verified', + label: 'Verified', + tooltip: allowVerification ? copy.verified : undefined, + icon: , + disabledReason: !allowVerification ? verifiedDisabledCorePropCopy : undefined, + }, + { + value: 'visible', + label: 'Visible', + tooltip: copy.visible, + icon: , + }, + ...(showHiddenOption + ? [ + { + value: 'hidden', + label: 'Hidden', + tooltip: copy.hidden, + icon: , + }, + ] + : []), + ]} /> + {!compact &&

{copy[currentStatus]}

} ) } @@ -201,6 +239,15 @@ function DefinitionView({ group }: { group: TaxonomicFilterGroup }): JSX.Element return ( <> {sharedComponents} + {_definition.verified && ( +
+ + + Verified + + +
+ )} @@ -382,6 +429,10 @@ function DefinitionEdit(): JSX.Element { return <> } + const showHiddenOption = hasTaxonomyFeatures && 'hidden' in localDefinition + const allowVerification = + hasTaxonomyFeatures && !isCoreFilter(definition.name || '') && 'verified' in localDefinition + return ( <> @@ -421,15 +472,20 @@ function DefinitionEdit(): JSX.Element { )} - {definition && definition.name && !isCoreFilter(definition.name) && 'verified' in localDefinition && ( - { - setLocalDefinition({ verified: nextVerified }) - }} - compact - /> + {definition && definition.name && (showHiddenOption || allowVerification) && ( +
+
)}
diff --git a/frontend/src/lib/components/TaxonomicFilter/infiniteListLogic.ts b/frontend/src/lib/components/TaxonomicFilter/infiniteListLogic.ts index c2f1e3a8605a1..4e879c5438c1a 100644 --- a/frontend/src/lib/components/TaxonomicFilter/infiniteListLogic.ts +++ b/frontend/src/lib/components/TaxonomicFilter/infiniteListLogic.ts @@ -186,9 +186,13 @@ export const infiniteListLogic = kea([ // On updating item, invalidate cache apiCache = {} apiCacheTimers = {} + const popFromResults = 'hidden' in item && item.hidden + const results: TaxonomicDefinitionTypes[] = values.remoteItems.results + .map((i) => (i.name === item.name ? (popFromResults ? null : item) : i)) + .filter((i): i is TaxonomicDefinitionTypes => i !== null) return { ...values.remoteItems, - results: values.remoteItems.results.map((i) => (i.name === item.name ? item : i)), + results, } }, }, diff --git a/frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.tsx b/frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.tsx index 766f65c64058a..152023e6de7d5 100644 --- a/frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.tsx +++ b/frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.tsx @@ -212,6 +212,7 @@ export const taxonomicFilterLogic = kea([ ), endpoint: combineUrl(`api/projects/${projectId}/event_definitions`, { event_type: EventDefinitionType.Event, + exclude_hidden: true, }).url, getName: (eventDefinition: Record) => eventDefinition.name, getValue: (eventDefinition: Record) => @@ -292,6 +293,7 @@ export const taxonomicFilterLogic = kea([ properties: propertyAllowList?.[TaxonomicFilterGroupType.EventProperties] ? propertyAllowList[TaxonomicFilterGroupType.EventProperties].join(',') : undefined, + exclude_hidden: true, }).url, scopedEndpoint: eventNames.length > 0 @@ -302,6 +304,7 @@ export const taxonomicFilterLogic = kea([ properties: propertyAllowList?.[TaxonomicFilterGroupType.EventProperties] ? propertyAllowList[TaxonomicFilterGroupType.EventProperties].join(',') : undefined, + exclude_hidden: true, }).url : undefined, expandLabel: ({ count, expandedCount }: { count: number; expandedCount: number }) => @@ -366,6 +369,7 @@ export const taxonomicFilterLogic = kea([ properties: propertyAllowList?.[TaxonomicFilterGroupType.PersonProperties] ? propertyAllowList[TaxonomicFilterGroupType.PersonProperties].join(',') : undefined, + exclude_hidden: true, }).url, getName: (personProperty: PersonProperty) => personProperty.name, getValue: (personProperty: PersonProperty) => personProperty.name, @@ -424,6 +428,7 @@ export const taxonomicFilterLogic = kea([ type: TaxonomicFilterGroupType.CustomEvents, endpoint: combineUrl(`api/projects/${projectId}/event_definitions`, { event_type: EventDefinitionType.EventCustom, + exclude_hidden: true, }).url, getName: (eventDefinition: EventDefinition) => eventDefinition.name, getValue: (eventDefinition: EventDefinition) => eventDefinition.name, @@ -576,6 +581,7 @@ export const taxonomicFilterLogic = kea([ endpoint: combineUrl(`api/projects/${projectId}/property_definitions`, { type: 'group', group_type_index: type.group_type_index, + exclude_hidden: true, }).url, valuesEndpoint: (key) => `api/projects/${projectId}/groups/property_values?${toParams({ diff --git a/frontend/src/scenes/data-management/definition/DefinitionEdit.tsx b/frontend/src/scenes/data-management/definition/DefinitionEdit.tsx index 834f3a25af00d..928fc5d34f4a4 100644 --- a/frontend/src/scenes/data-management/definition/DefinitionEdit.tsx +++ b/frontend/src/scenes/data-management/definition/DefinitionEdit.tsx @@ -1,7 +1,7 @@ import { LemonSkeleton, LemonTag } from '@posthog/lemon-ui' import { useActions, useValues } from 'kea' import { Form } from 'kea-forms' -import { VerifiedDefinitionCheckbox } from 'lib/components/DefinitionPopover/DefinitionPopoverContents' +import { PropertyStatusControl } from 'lib/components/DefinitionPopover/DefinitionPopoverContents' import { NotFound } from 'lib/components/NotFound' import { ObjectTags } from 'lib/components/ObjectTags/ObjectTags' import { PageHeader } from 'lib/components/PageHeader' @@ -33,8 +33,9 @@ export function DefinitionEdit(props: DefinitionLogicProps = {}): JSX.Element { const { saveDefinition } = useActions(logic) const { tags, tagsLoading } = useValues(tagsModel) - const showVerifiedCheckbox = - hasTaxonomyFeatures && !isCoreFilter(editDefinition.name) && 'verified' in editDefinition + const allowVerification = hasTaxonomyFeatures && !isCoreFilter(editDefinition.name) && 'verified' in editDefinition + + const showHiddenOption = hasTaxonomyFeatures && 'hidden' in editDefinition if (definitionMissing) { return @@ -108,17 +109,25 @@ export function DefinitionEdit(props: DefinitionLogicProps = {}): JSX.Element {
)} - {showVerifiedCheckbox && ( + {(allowVerification || showHiddenOption) && (
- - {({ value, onChange }) => ( - { - onChange(nextVerified) - }} - /> + + {({ value: verified, onChange }) => ( + + {({ value: hidden, onChange: onHiddenChange }) => ( + )}
diff --git a/frontend/src/scenes/data-management/definition/DefinitionView.tsx b/frontend/src/scenes/data-management/definition/DefinitionView.tsx index 5e22f38e1043e..c4e17f47ef6a1 100644 --- a/frontend/src/scenes/data-management/definition/DefinitionView.tsx +++ b/frontend/src/scenes/data-management/definition/DefinitionView.tsx @@ -1,4 +1,6 @@ -import { LemonDivider, LemonTag } from '@posthog/lemon-ui' +import { IconBadge, IconEye } from '@posthog/icons' +import { IconHide } from '@posthog/icons' +import { LemonDivider, LemonTag, LemonTagType, Tooltip } from '@posthog/lemon-ui' import { useActions, useValues } from 'kea' import { EditableField } from 'lib/components/EditableField/EditableField' import { NotFound } from 'lib/components/NotFound' @@ -22,7 +24,7 @@ import { urls } from 'scenes/urls' import { defaultDataTableColumns } from '~/queries/nodes/DataTable/utils' import { Query } from '~/queries/Query/Query' import { NodeKind } from '~/queries/schema/schema-general' -import { FilterLogicalOperator, PropertyDefinition, ReplayTabs } from '~/types' +import { FilterLogicalOperator, PropertyDefinition, PropertyDefinitionVerificationStatus, ReplayTabs } from '~/types' export const scene: SceneExport = { component: DefinitionView, @@ -32,6 +34,40 @@ export const scene: SceneExport = { }), } +type StatusProps = { + tagType: LemonTagType + label: string + icon: React.ReactNode + tooltip: string +} + +const getStatusProps = (isProperty: boolean): Record => ({ + verified: { + tagType: 'success', + label: 'Verified', + icon: , + tooltip: `This ${ + isProperty ? 'property' : 'event' + } is verified and can be used in filters and other selection components.`, + }, + hidden: { + tagType: 'danger', + label: 'Hidden', + icon: , + tooltip: `This ${ + isProperty ? 'property' : 'event' + } is hidden and will not appear in filters and other selection components.`, + }, + visible: { + tagType: 'default', + label: 'Visible', + icon: , + tooltip: `This ${ + isProperty ? 'property' : 'event' + } is visible and can be used in filters and other selection components.`, + }, +}) + export function DefinitionView(props: DefinitionLogicProps = {}): JSX.Element { const logic = definitionLogic(props) const { definition, definitionLoading, definitionMissing, hasTaxonomyFeatures, singular, isEvent, isProperty } = @@ -60,6 +96,10 @@ export function DefinitionView(props: DefinitionLogicProps = {}): JSX.Element { return } + const definitionStatus = definition.verified ? 'verified' : definition.hidden ? 'hidden' : 'visible' + + const statusProps = getStatusProps(isProperty) + return ( <> {' '} - will no longer appear in selectors. Associated data will remain - in the database. + will no longer appear in selectors. Associated data will remain in the + database.

- This definition will be recreated if the {singular} is ever seen again - in the event stream. + This definition will be recreated if the ${singular} is ever seen again + in the event stream.

), @@ -199,6 +239,20 @@ export function DefinitionView(props: DefinitionLogicProps = {}): JSX.Element { )} + {definitionStatus && ( +
+
Verification status
+
+ + + {statusProps[definitionStatus].icon} + {statusProps[definitionStatus].label} + + +
+
+ )} + {isProperty && (
Property type
diff --git a/frontend/src/scenes/data-management/definition/definitionLogic.ts b/frontend/src/scenes/data-management/definition/definitionLogic.ts index 8a814720b6d74..c469ead44d6fb 100644 --- a/frontend/src/scenes/data-management/definition/definitionLogic.ts +++ b/frontend/src/scenes/data-management/definition/definitionLogic.ts @@ -19,6 +19,8 @@ import type { definitionLogicType } from './definitionLogicType' export const createNewDefinition = (isEvent: boolean): Definition => ({ id: 'new', name: `New ${isEvent ? 'Event' : 'Event property'}`, + verified: false, + hidden: false, }) export interface SetDefinitionProps { diff --git a/frontend/src/scenes/data-management/events/DefinitionHeader.tsx b/frontend/src/scenes/data-management/events/DefinitionHeader.tsx index 9ecf8b43ce2ed..074bedf7ad0ec 100644 --- a/frontend/src/scenes/data-management/events/DefinitionHeader.tsx +++ b/frontend/src/scenes/data-management/events/DefinitionHeader.tsx @@ -1,33 +1,63 @@ import { IconBadge, IconBolt, IconCursor, IconEye, IconLeave, IconList, IconLogomark } from '@posthog/icons' import { PropertyKeyInfo } from 'lib/components/PropertyKeyInfo' import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types' -import { IconSelectAll } from 'lib/lemon-ui/icons' +import { IconEyeHidden, IconSelectAll } from 'lib/lemon-ui/icons' import { LemonTableLink } from 'lib/lemon-ui/LemonTable/LemonTableLink' import { LinkProps } from 'lib/lemon-ui/Link' import { Tooltip } from 'lib/lemon-ui/Tooltip' import { CORE_FILTER_DEFINITIONS_BY_GROUP, getCoreFilterDefinition } from 'lib/taxonomy' +import React from 'react' import { EventDefinition, PropertyDefinition } from '~/types' +interface IconWithBadgeProps { + icon: JSX.Element + verified?: boolean + hidden?: boolean + tooltipTitle: string + className?: string +} + +function IconWithBadge({ icon, verified, hidden, tooltipTitle, className }: IconWithBadgeProps): JSX.Element { + const wrappedIcon = ( +
+ {React.cloneElement(icon, { className: className || icon.props.className })} + {(verified || hidden) && ( +
+ {hidden ? ( + + ) : ( + + )} +
+ )} +
+ ) + + const tooltipSuffix = hidden ? ', hidden' : verified ? ', verified' : '' + return {wrappedIcon} +} + export function getPropertyDefinitionIcon(definition: PropertyDefinition): JSX.Element { if (CORE_FILTER_DEFINITIONS_BY_GROUP.event_properties[definition.name]) { return ( - - - - ) - } - if (definition.verified) { - return ( - - - + } + tooltipTitle="PostHog event property" + className="taxonomy-icon taxonomy-icon-muted" + verified={definition.verified} + hidden={definition.hidden} + /> ) } return ( - - - + } + tooltipTitle="Event property" + className="taxonomy-icon taxonomy-icon-muted" + verified={definition.verified} + hidden={definition.hidden} + /> ) } @@ -35,46 +65,67 @@ export function getEventDefinitionIcon(definition: EventDefinition & { value?: s // Rest are events if (definition.name === '$pageview' || definition.name === '$screen') { return ( - - - + } + verified={definition.verified} + hidden={definition.hidden} + tooltipTitle="Pageview" + className="taxonomy-icon taxonomy-icon-ph taxonomy-icon-muted" + /> ) } if (definition.name === '$pageleave') { return ( - - - + } + verified={definition.verified} + hidden={definition.hidden} + tooltipTitle="PostHog event" + className="taxonomy-icon taxonomy-icon-ph taxonomy-icon-muted" + /> ) } if (definition.name === '$autocapture') { - return - } - if (definition.name && definition.verified) { return ( - - - + } + verified={definition.verified} + hidden={definition.hidden} + tooltipTitle="Autocapture event" + className="taxonomy-icon taxonomy-icon-ph taxonomy-icon-muted" + /> ) } if (definition.name && !!CORE_FILTER_DEFINITIONS_BY_GROUP.events[definition.name]) { return ( - - - + } + verified={definition.verified} + hidden={definition.hidden} + tooltipTitle="PostHog event" + className="taxonomy-icon taxonomy-icon-muted" + /> ) } if (definition.value === null) { return ( - - - + } + verified={definition.verified} + hidden={definition.hidden} + tooltipTitle="All events" + className="taxonomy-icon taxonomy-icon-built-in" + /> ) } return ( - - - + } + verified={definition.verified} + hidden={definition.hidden} + tooltipTitle="Custom event" + className="taxonomy-icon taxonomy-icon-muted" + /> ) } @@ -95,30 +146,7 @@ export function DefinitionHeader({ to={to} description={description} title={ - <> - - {definition.verified && ( - <> - - - - - )} - {!!CORE_FILTER_DEFINITIONS_BY_GROUP.events[definition.name] && ( - - - - )} - + } /> ) diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 37d59493ba52e..356a95f68d57d 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -3305,6 +3305,7 @@ export interface EventDefinition { verified_at?: string verified_by?: string is_action?: boolean + hidden?: boolean } // TODO duplicated from plugin server. Follow-up to de-duplicate @@ -3345,6 +3346,7 @@ export interface PropertyDefinition { verified?: boolean verified_at?: string verified_by?: string + hidden?: boolean } export enum PropertyDefinitionState { @@ -3354,6 +3356,7 @@ export enum PropertyDefinitionState { Error = 'error', } +export type PropertyDefinitionVerificationStatus = 'verified' | 'hidden' | 'visible' export type Definition = EventDefinition | PropertyDefinition export interface PersonProperty { diff --git a/posthog/api/event_definition.py b/posthog/api/event_definition.py index 4156d10f82793..e767d8ba2910c 100644 --- a/posthog/api/event_definition.py +++ b/posthog/api/event_definition.py @@ -1,14 +1,8 @@ from typing import Any, Literal, cast from django.db.models import Manager -from rest_framework import ( - mixins, - serializers, - viewsets, - request, - status, - response, -) +from loginas.utils import is_impersonated_session +from rest_framework import mixins, request, response, serializers, status, viewsets from posthog.api.routing import TeamAndOrgViewSetMixin from posthog.api.shared import UserBasicSerializer @@ -23,7 +17,6 @@ from posthog.models.user import User from posthog.models.utils import UUIDT from posthog.settings import EE_AVAILABLE -from loginas.utils import is_impersonated_session # If EE is enabled, we use ee.api.ee_event_definition.EnterpriseEventDefinitionSerializer @@ -55,6 +48,15 @@ class Meta: "post_to_slack", ) + 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: EventDefinition, validated_data): raise EnterpriseFeatureException() @@ -103,6 +105,10 @@ def dangerously_get_queryset(self): else: event_definition_object_manager = EventDefinition.objects + exclude_hidden = self.request.GET.get("exclude_hidden", "false").lower() == "true" + if exclude_hidden and is_enterprise: + search_query = search_query + " AND (hidden IS NULL OR hidden = false)" + sql = create_event_definitions_sql( event_type, is_enterprise=is_enterprise, diff --git a/posthog/api/test/test_property_definition.py b/posthog/api/test/test_property_definition.py index 2a568e1e843d7..c3a275e14a11f 100644 --- a/posthog/api/test/test_property_definition.py +++ b/posthog/api/test/test_property_definition.py @@ -5,12 +5,12 @@ from rest_framework import status from posthog.models import ( + ActivityLog, EventDefinition, EventProperty, Organization, PropertyDefinition, Team, - ActivityLog, ) from posthog.taxonomy.property_definition_api import PropertyDefinitionQuerySerializer from posthog.test.base import APIBaseTest, BaseTest diff --git a/posthog/taxonomy/property_definition_api.py b/posthog/taxonomy/property_definition_api.py index 401f9c86264c4..56eb233098dad 100644 --- a/posthog/taxonomy/property_definition_api.py +++ b/posthog/taxonomy/property_definition_api.py @@ -1,19 +1,19 @@ import dataclasses import json -from django.db.models.functions import Coalesce -from typing import Any, Optional, cast, Self +from typing import Any, Optional, Self, cast from django.db import connection, models +from django.db.models.functions import Coalesce from django.shortcuts import get_object_or_404 from loginas.utils import is_impersonated_session from rest_framework import mixins, request, response, serializers, status, viewsets -from posthog.api.utils import action from rest_framework.exceptions import ValidationError from rest_framework.pagination import LimitOffsetPagination from posthog.api.documentation import extend_schema from posthog.api.routing import TeamAndOrgViewSetMixin from posthog.api.tagged_item import TaggedItemSerializerMixin, TaggedItemViewSetMixin +from posthog.api.utils import action from posthog.constants import GROUP_TYPES_LIMIT, AvailableFeature from posthog.event_usage import report_user_action from posthog.exceptions import EnterpriseFeatureException @@ -78,6 +78,12 @@ class PropertyDefinitionQuerySerializer(serializers.Serializer): required=False, ) + exclude_hidden = serializers.BooleanField( + help_text="Whether to exclude properties marked as hidden", + required=False, + default=False, + ) + def validate(self, attrs): type_ = attrs.get("type", "event") @@ -259,6 +265,19 @@ def with_excluded_properties(self, excluded_properties: Optional[str], type: str }, ) + def with_hidden_filter(self, exclude_hidden: bool, use_enterprise_taxonomy: bool) -> Self: + if exclude_hidden and use_enterprise_taxonomy: + hidden_filter = " AND (hidden IS NULL OR hidden = false)" + return dataclasses.replace( + self, + excluded_properties_filter=( + self.excluded_properties_filter + hidden_filter + if self.excluded_properties_filter + else hidden_filter + ), + ) + return self + def as_sql(self, order_by_verified: bool): verified_ordering = "verified DESC NULLS LAST," if order_by_verified else "" query = f""" @@ -374,7 +393,23 @@ class Meta: "is_seen_on_filtered_events", ) - def update(self, property_definition: PropertyDefinition, 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") + + return validated_data + + 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 + # If setting verified=True, ensure hidden becomes false + elif validated_data.get("verified", False): + validated_data["hidden"] = False + changed_fields = { k: v for k, v in validated_data.items() @@ -527,6 +562,9 @@ def dangerously_get_queryset(self): query.validated_data.get("excluded_properties"), type=query.validated_data.get("type"), ) + .with_hidden_filter( + query.validated_data.get("exclude_hidden", False), use_enterprise_taxonomy=use_enterprise_taxonomy + ) ) with connection.cursor() as cursor: