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(flags): Add distinct id and group key matching #19902

Merged
merged 6 commits into from
Jan 22, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@
type === TaxonomicFilterGroupType.EventProperties ||
type === TaxonomicFilterGroupType.EventFeatureFlags ||
type === TaxonomicFilterGroupType.PersonProperties ||
type === TaxonomicFilterGroupType.GroupsPrefix
type === TaxonomicFilterGroupType.GroupsPrefix ||
type === TaxonomicFilterGroupType.Metadata
) {
data = getKeyMapping(value, 'event')
} else if (type === TaxonomicFilterGroupType.Elements) {
Expand Down Expand Up @@ -197,7 +198,7 @@

function HorizontalLine({ children, ...props }: DividerProps): JSX.Element {
return (
<Divider className="definition-popover-divider" {...props}>

Check warning on line 201 in frontend/src/lib/components/DefinitionPopover/DefinitionPopover.tsx

View workflow job for this annotation

GitHub Actions / Code quality checks

<Divider> is forbidden, use <LemonDivider> instead
{children}
</Divider>
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ export const definitionPopoverLogic = kea<definitionPopoverLogicType>([
TaxonomicFilterGroupType.EventProperties,
TaxonomicFilterGroupType.EventFeatureFlags,
TaxonomicFilterGroupType.NumericalEventProperties,
TaxonomicFilterGroupType.Metadata,
].includes(type) || type.startsWith(TaxonomicFilterGroupType.GroupsPrefix),
],
isCohort: [(s) => [s.type], (type) => type === TaxonomicFilterGroupType.Cohorts],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import './PropertyFilters.scss'

import { BindLogic, useActions, useValues } from 'kea'
import { TaxonomicPropertyFilter } from 'lib/components/PropertyFilters/components/TaxonomicPropertyFilter'
import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types'
import { TaxonomicFilterGroupType, TaxonomicFilterProps } from 'lib/components/TaxonomicFilter/types'
import React, { useEffect } from 'react'
import { LogicalRowDivider } from 'scenes/cohorts/CohortFilters/CohortCriteriaRowBuilder'

Expand All @@ -20,6 +20,7 @@ interface PropertyFiltersProps {
showConditionBadge?: boolean
disablePopover?: boolean
taxonomicGroupTypes?: TaxonomicFilterGroupType[]
taxonomicFilterOptionsFromProp?: TaxonomicFilterProps['optionsFromProp']
metadataSource?: AnyDataNode
showNestedArrow?: boolean
eventNames?: string[]
Expand All @@ -41,6 +42,7 @@ export function PropertyFilters({
showConditionBadge = false,
disablePopover = false, // use bare PropertyFilter without popover
taxonomicGroupTypes,
taxonomicFilterOptionsFromProp,
metadataSource,
showNestedArrow = false,
eventNames = [],
Expand Down Expand Up @@ -109,6 +111,7 @@ export function PropertyFilters({
placement: pageKey === 'insight-filters' ? 'bottomLeft' : undefined,
}}
propertyAllowList={propertyAllowList}
taxonomicFilterOptionsFromProp={taxonomicFilterOptionsFromProp}
/>
)}
errorMessage={errorMessages && errorMessages[index]}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export function TaxonomicPropertyFilter({
hasRowOperator,
metadataSource,
propertyAllowList,
taxonomicFilterOptionsFromProp,
}: PropertyFilterInternalProps): JSX.Element {
const pageKey = useMemo(() => pageKeyInput || `filter-${uniqueMemoizedIndex++}`, [pageKeyInput])
const groupTypes = taxonomicGroupTypes || [
Expand All @@ -56,9 +57,10 @@ export function TaxonomicPropertyFilter({
]
const taxonomicOnChange: (group: TaxonomicFilterGroup, value: TaxonomicFilterValue, item: any) => void = (
taxonomicGroup,
value
value,
item
) => {
selectItem(taxonomicGroup, value)
selectItem(taxonomicGroup, value, item?.propertyFilterType)
if (
taxonomicGroup.type === TaxonomicFilterGroupType.Cohorts ||
taxonomicGroup.type === TaxonomicFilterGroupType.HogQLExpression
Expand Down Expand Up @@ -108,6 +110,7 @@ export function TaxonomicPropertyFilter({
metadataSource={metadataSource}
eventNames={eventNames}
propertyAllowList={propertyAllowList}
optionsFromProp={taxonomicFilterOptionsFromProp}
/>
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
import { taxonomicFilterLogic } from 'lib/components/TaxonomicFilter/taxonomicFilterLogic'
import {
TaxonomicFilterGroup,
TaxonomicFilterGroupType,
TaxonomicFilterLogicProps,
TaxonomicFilterValue,
} from 'lib/components/TaxonomicFilter/types'
Expand Down Expand Up @@ -50,9 +51,14 @@ export const taxonomicPropertyFilterLogic = kea<taxonomicPropertyFilterLogicType
],
})),
actions({
selectItem: (taxonomicGroup: TaxonomicFilterGroup, propertyKey?: TaxonomicFilterValue) => ({
selectItem: (
taxonomicGroup: TaxonomicFilterGroup,
propertyKey?: TaxonomicFilterValue,
itemPropertyFilterType?: PropertyFilterType
) => ({
taxonomicGroup,
propertyKey,
itemPropertyFilterType,
}),
openDropdown: true,
closeDropdown: true,
Expand Down Expand Up @@ -87,8 +93,8 @@ export const taxonomicPropertyFilterLogic = kea<taxonomicPropertyFilterLogicType
],
}),
listeners(({ actions, values, props }) => ({
selectItem: ({ taxonomicGroup, propertyKey }) => {
const propertyType = taxonomicFilterTypeToPropertyFilterType(taxonomicGroup.type)
selectItem: ({ taxonomicGroup, propertyKey, itemPropertyFilterType }) => {
const propertyType = itemPropertyFilterType ?? taxonomicFilterTypeToPropertyFilterType(taxonomicGroup.type)
if (propertyKey && propertyType) {
if (propertyType === PropertyFilterType.Cohort) {
const cohortProperty: CohortPropertyFilter = {
Expand All @@ -106,9 +112,7 @@ export const taxonomicPropertyFilterLogic = kea<taxonomicPropertyFilterLogicType
props.propertyFilterLogic.actions.setFilter(props.filterIndex, hogQLProperty)
} else {
const apiType =
propertyFilterTypeToPropertyDefinitionType(
taxonomicFilterTypeToPropertyFilterType(taxonomicGroup.type)
) ?? PropertyDefinitionType.Event
propertyFilterTypeToPropertyDefinitionType(propertyType) ?? PropertyDefinitionType.Event

const propertyValueType = values.describeProperty(
propertyKey,
Expand All @@ -129,9 +133,12 @@ export const taxonomicPropertyFilterLogic = kea<taxonomicPropertyFilterLogicType
property_value_type_to_default_operator_override[propertyValueType ?? ''] ||
PropertyOperator.Exact

const isGroupNameFilter = taxonomicGroup.type.startsWith(TaxonomicFilterGroupType.GroupNamesPrefix)
// :TRICKY: When we have a GroupNamesPrefix taxonomic filter, selecting the group name
// is the equivalent of selecting a property value
const property: AnyPropertyFilter = {
key: propertyKey.toString(),
value: null,
key: isGroupNameFilter ? '$group_key' : propertyKey.toString(),
value: isGroupNameFilter ? propertyKey.toString() : null,
Comment on lines +137 to +141
Copy link
Collaborator

@mariusandra mariusandra Jan 22, 2024

Choose a reason for hiding this comment

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

This override is the last thing that raises eyebrows... Happy to let is pass though, the impact is very minimal.

The bigger UX sadness here, which can also be for later, is that once you select an organization, the filter says "Group key", not "Organization key".

2024-01-19 13 27 07

The title there can probably be fixed/overridden easily, but up to you if you want to...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Want to keep it is group_key for now, given it's in context of matching org(s), and this is the actual field when, say, calling $groupIdentify(). Will revisit though if this raises confusion

operator,
type: propertyType as AnyPropertyFilter['type'] as any, // bad | pipe chain :(
group_type_index: taxonomicGroup.groupTypeIndex,
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/lib/components/PropertyFilters/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { SelectGradientOverflowProps } from 'lib/components/SelectGradientOverfl
import {
TaxonomicFilterGroup,
TaxonomicFilterGroupType,
TaxonomicFilterProps,
TaxonomicFilterValue,
} from 'lib/components/TaxonomicFilter/types'

Expand Down Expand Up @@ -39,6 +40,7 @@ export interface PropertyFilterInternalProps {
onComplete: () => void
disablePopover: boolean
taxonomicGroupTypes?: TaxonomicFilterGroupType[]
taxonomicFilterOptionsFromProp?: TaxonomicFilterProps['optionsFromProp']
eventNames?: string[]
propertyGroupType?: FilterLogicalOperator | null
orFiltering?: boolean
Expand Down
5 changes: 4 additions & 1 deletion frontend/src/lib/components/PropertyFilters/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,10 @@ export function taxonomicFilterTypeToPropertyFilterType(
if (filterType === TaxonomicFilterGroupType.CohortsWithAllUsers) {
return PropertyFilterType.Cohort
}
if (filterType?.startsWith(TaxonomicFilterGroupType.GroupsPrefix)) {
if (
filterType?.startsWith(TaxonomicFilterGroupType.GroupsPrefix) ||
filterType?.startsWith(TaxonomicFilterGroupType.GroupNamesPrefix)
) {
return PropertyFilterType.Group
}

Expand Down
4 changes: 3 additions & 1 deletion frontend/src/lib/components/TaxonomicFilter/InfiniteList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ const renderItemContents = ({
listGroupType === TaxonomicFilterGroupType.PersonProperties ||
listGroupType === TaxonomicFilterGroupType.Events ||
listGroupType === TaxonomicFilterGroupType.CustomEvents ||
listGroupType === TaxonomicFilterGroupType.Metadata ||
listGroupType.startsWith(TaxonomicFilterGroupType.GroupsPrefix) ? (
<>
<div className={clsx('taxonomic-list-row-contents', isStale && 'text-muted')}>
Expand Down Expand Up @@ -136,7 +137,7 @@ const selectedItemHasPopover = (
group?: TaxonomicFilterGroup
): boolean => {
return (
// NB: also update "renderItemPopover" above
// NB: also update "renderItemContents" above
!!item &&
!!group?.getValue?.(item) &&
!!listGroupType &&
Expand All @@ -151,6 +152,7 @@ const selectedItemHasPopover = (
TaxonomicFilterGroupType.PersonProperties,
TaxonomicFilterGroupType.Cohorts,
TaxonomicFilterGroupType.CohortsWithAllUsers,
TaxonomicFilterGroupType.Metadata,
].includes(listGroupType) ||
listGroupType.startsWith(TaxonomicFilterGroupType.GroupsPrefix))
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ export const infiniteListLogic = kea<infiniteListLogicType>([
[TaxonomicFilterGroupType.Events]: 'event',
[TaxonomicFilterGroupType.EventProperties]: 'event',
[TaxonomicFilterGroupType.PersonProperties]: 'event',
[TaxonomicFilterGroupType.Metadata]: 'event',
[TaxonomicFilterGroupType.Elements]: 'element',
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,16 @@ export const taxonomicFilterLogic = kea<taxonomicFilterLogicType>([
getValue: (option: SimpleOption) => option.name,
getPopoverHeader: () => 'Autocapture Element',
},
{
name: 'Metadata',
searchPlaceholder: 'metadata',
type: TaxonomicFilterGroupType.Metadata,
// populate options using `optionsFromProp` depending on context in which
// this taxonomic group type is used
getName: (option: SimpleOption) => option.name,
getValue: (option: SimpleOption) => option.name,
...propertyTaxonomicGroupProps(true),
},
{
name: 'Event properties',
searchPlaceholder: 'event properties',
Expand Down
14 changes: 12 additions & 2 deletions frontend/src/lib/components/TaxonomicFilter/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,18 @@ import Fuse from 'fuse.js'
import { LogicWrapper } from 'kea'

import { AnyDataNode } from '~/queries/schema'
import { ActionType, CohortType, EventDefinition, PersonProperty, PropertyDefinition } from '~/types'
import {
ActionType,
CohortType,
EventDefinition,
PersonProperty,
PropertyDefinition,
PropertyFilterType,
} from '~/types'

export interface SimpleOption {
name: string
propertyFilterType?: PropertyFilterType
}

export interface TaxonomicFilterProps {
Expand All @@ -23,7 +31,7 @@ export interface TaxonomicFilterProps {
selectFirstItem?: boolean
/** use to filter results in a group by name, currently only working for EventProperties */
excludedProperties?: { [key in TaxonomicFilterGroupType]?: TaxonomicFilterValue[] }
propertyAllowList?: { [key in TaxonomicFilterGroupType]?: string[] } // only return properties in this list, currently only working for EventProperties
propertyAllowList?: { [key in TaxonomicFilterGroupType]?: string[] } // only return properties in this list, currently only working for EventProperties and PersonProperties
metadataSource?: AnyDataNode
}

Expand Down Expand Up @@ -66,6 +74,8 @@ export interface TaxonomicFilterGroup {
}

export enum TaxonomicFilterGroupType {
// Person and event metadata that isn't present in properties
Metadata = 'metadata',
Actions = 'actions',
Cohorts = 'cohorts',
CohortsWithAllUsers = 'cohorts_with_all',
Expand Down
4 changes: 4 additions & 0 deletions frontend/src/lib/constants.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ export const INSTANTLY_AVAILABLE_PROPERTIES = [
'$geoip_continent_code',
'$geoip_postal_code',
'$geoip_time_zone',
// Person and group identifiers
'$group_key',
'distinct_id',
]

// Event constants
Expand Down Expand Up @@ -196,6 +199,7 @@ export const FEATURE_FLAGS = {
REDIRECT_WEB_PRODUCT_ANALYTICS_ONBOARDING: 'redirect-web-product-analytics-onboarding', // owner: @biancayang
RECRUIT_ANDROID_MOBILE_BETA_TESTERS: 'recruit-android-mobile-beta-testers', // owner: #team-replay
SIDEPANEL_STATUS: 'sidepanel-status', // owner: @benjackwhite
NEW_FEATURE_FLAG_OPERATORS: 'new-feature-flag-operators', // owner: @neilkakkar
AI_SESSION_SUMMARY: 'ai-session-summary', // owner: #team-replay
} as const
export type FeatureFlagKey = (typeof FEATURE_FLAGS)[keyof typeof FEATURE_FLAGS]
Expand Down
5 changes: 5 additions & 0 deletions frontend/src/lib/taxonomy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,11 @@ export const KEY_MAPPING: KeyMappingInterface = {
examples: ['16ff262c4301e5-0aa346c03894bc-39667c0e-1aeaa0-16ff262c431767'],
system: true,
},
distinct_id: {
label: 'Distinct ID',
description: 'The current distinct ID of the user',
examples: ['16ff262c4301e5-0aa346c03894bc-39667c0e-1aeaa0-16ff262c431767'],
},
$event_type: {
label: 'Event Type',
description:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export function FeatureFlagReleaseConditions({
computeBlastRadiusPercentage,
affectedUsers,
totalUsers,
featureFlagTaxonomicOptions,
} = useValues(logic)
const {
setAggregationGroupTypeIndex,
Expand Down Expand Up @@ -202,6 +203,7 @@ export function FeatureFlagReleaseConditions({
addText="Add condition"
onChange={(properties) => updateConditionSet(index, undefined, properties)}
taxonomicGroupTypes={taxonomicGroupTypes}
taxonomicFilterOptionsFromProp={featureFlagTaxonomicOptions}
hasRowOperator={false}
sendAllKeyUpdates
errorMessages={
Expand Down
45 changes: 40 additions & 5 deletions frontend/src/scenes/feature-flags/featureFlagLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@ import { loaders } from 'kea-loaders'
import { router, urlToAction } from 'kea-router'
import api from 'lib/api'
import { convertPropertyGroupToProperties } from 'lib/components/PropertyFilters/utils'
import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types'
import { TaxonomicFilterGroupType, TaxonomicFilterProps } from 'lib/components/TaxonomicFilter/types'
import { FEATURE_FLAGS } from 'lib/constants'
import { dayjs } from 'lib/dayjs'
import { lemonToast } from 'lib/lemon-ui/LemonToast/LemonToast'
import { featureFlagLogic as enabledFeaturesLogic } from 'lib/logic/featureFlagLogic'
import { sum, toParams } from 'lib/utils'
import { deleteWithUndo } from 'lib/utils/deleteWithUndo'
import { eventUsageLogic } from 'lib/utils/eventUsageLogic'
Expand Down Expand Up @@ -189,6 +191,8 @@ export const featureFlagLogic = kea<featureFlagLogicType>([
['dashboards'],
organizationLogic,
['currentOrganization'],
enabledFeaturesLogic,
['featureFlags as enabledFeatures'],
],
actions: [
newDashboardLogic({ featureFlagId: typeof props.id === 'number' ? props.id : undefined }),
Expand Down Expand Up @@ -927,17 +931,33 @@ export const featureFlagLogic = kea<featureFlagLogicType>([
},
],
taxonomicGroupTypes: [
(s) => [s.featureFlag, s.groupsTaxonomicTypes],
(featureFlag, groupsTaxonomicTypes): TaxonomicFilterGroupType[] => {
(s) => [s.featureFlag, s.groupsTaxonomicTypes, s.enabledFeatures],
(featureFlag, groupsTaxonomicTypes, enabledFeatures): TaxonomicFilterGroupType[] => {
const baseGroupTypes = []
const additionalGroupTypes = []
const newFlagOperatorsEnabled = enabledFeatures[FEATURE_FLAGS.NEW_FEATURE_FLAG_OPERATORS]
if (
featureFlag &&
featureFlag.filters.aggregation_group_type_index != null &&
groupsTaxonomicTypes.length > 0
) {
return [groupsTaxonomicTypes[featureFlag.filters.aggregation_group_type_index]]
baseGroupTypes.push(groupsTaxonomicTypes[featureFlag.filters.aggregation_group_type_index])

if (newFlagOperatorsEnabled) {
additionalGroupTypes.push(
`${TaxonomicFilterGroupType.GroupNamesPrefix}_${featureFlag.filters.aggregation_group_type_index}` as unknown as TaxonomicFilterGroupType
)
}
} else {
baseGroupTypes.push(TaxonomicFilterGroupType.PersonProperties)
baseGroupTypes.push(TaxonomicFilterGroupType.Cohorts)

if (newFlagOperatorsEnabled) {
additionalGroupTypes.push(TaxonomicFilterGroupType.Metadata)
}
}

return [TaxonomicFilterGroupType.PersonProperties, TaxonomicFilterGroupType.Cohorts]
return [...baseGroupTypes, ...additionalGroupTypes]
},
],
breadcrumbs: [
Expand All @@ -951,6 +971,21 @@ export const featureFlagLogic = kea<featureFlagLogicType>([
{ key: [Scene.FeatureFlag, featureFlag.id || 'unknown'], name: featureFlag.key || 'Unnamed' },
],
],
featureFlagTaxonomicOptions: [
(s) => [s.featureFlag],
(featureFlag) => {
if (featureFlag && featureFlag.filters.aggregation_group_type_index != null) {
return {}
}

const taxonomicOptions: TaxonomicFilterProps['optionsFromProp'] = {
[TaxonomicFilterGroupType.Metadata]: [
{ name: 'distinct_id', propertyFilterType: PropertyFilterType.Person },
],
}
return taxonomicOptions
},
],
propertySelectErrors: [
(s) => [s.featureFlag],
(featureFlag) => {
Expand Down
Loading