From 37e2fcce09cde68ce828d88f1a3b747a41565847 Mon Sep 17 00:00:00 2001 From: Sandy Spicer Date: Wed, 9 Oct 2024 11:06:50 -0700 Subject: [PATCH] fix: funnel first_time_for_user logic change (#25448) --- .../InsightQuery/utils/filtersToQueryNode.ts | 5 +- frontend/src/queries/schema.json | 7 ++ frontend/src/queries/schema.ts | 9 ++- .../ActionFilterRow/ActionFilterRow.tsx | 65 +++++++++------- frontend/src/scenes/trends/mathsLogic.tsx | 55 ++++++++++++- frontend/src/types.ts | 6 ++ .../hogql_queries/insights/funnels/base.py | 8 +- .../insights/funnels/test/test_funnel.py | 61 +++++++++++++++ posthog/schema.py | 78 +++++++++++++++++-- 9 files changed, 250 insertions(+), 44 deletions(-) diff --git a/frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.ts b/frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.ts index aebeaf774f973..742320e8197eb 100644 --- a/frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.ts +++ b/frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.ts @@ -51,6 +51,7 @@ import { DataWarehouseFilter, FilterType, FunnelExclusionLegacy, + FunnelMathType, FunnelsFilterType, GroupMathType, HogQLMathType, @@ -84,7 +85,7 @@ const actorsOnlyMathTypes = [ HogQLMathType.HogQL, ] -const funnelsMathTypes = [BaseMathType.FirstTimeForUser] +const funnelsMathTypes = [FunnelMathType.FirstTimeForUser, FunnelMathType.FirstTimeForUserWithFilters] type FilterTypeActionsAndEvents = { events?: ActionFilter[] @@ -118,7 +119,7 @@ export const legacyEntityToNode = ( } if (mathAvailability !== MathAvailability.None) { - // only trends and stickiness insights support math. + // only trends, funnels, and stickiness insights support math. // transition to then default math for stickiness, when an unsupported math type is encountered. if (mathAvailability === MathAvailability.ActorsOnly && !actorsOnlyMathTypes.includes(entity.math as any)) { shared = { diff --git a/frontend/src/queries/schema.json b/frontend/src/queries/schema.json index d821f6e576336..9f35a775afd6b 100644 --- a/frontend/src/queries/schema.json +++ b/frontend/src/queries/schema.json @@ -5373,6 +5373,10 @@ "enum": ["horizontal", "vertical"], "type": "string" }, + "FunnelMathType": { + "enum": ["total", "first_time_for_user", "first_time_for_user_with_filters"], + "type": "string" + }, "FunnelPathType": { "enum": ["funnel_path_before_step", "funnel_path_between_steps", "funnel_path_after_step"], "type": "string" @@ -7132,6 +7136,9 @@ { "$ref": "#/definitions/BaseMathType" }, + { + "$ref": "#/definitions/FunnelMathType" + }, { "$ref": "#/definitions/PropertyMathType" }, diff --git a/frontend/src/queries/schema.ts b/frontend/src/queries/schema.ts index c38d4d9d8ac3f..9bbffba136dab 100644 --- a/frontend/src/queries/schema.ts +++ b/frontend/src/queries/schema.ts @@ -17,6 +17,7 @@ import { FeaturePropertyFilter, FilterLogicalOperator, FilterType, + FunnelMathType, FunnelsFilterType, GroupMathType, GroupPropertyFilter, @@ -460,7 +461,13 @@ export interface HogQLAutocomplete extends DataNode { endPosition: integer } -export type MathType = BaseMathType | PropertyMathType | CountPerActorMathType | GroupMathType | HogQLMathType +export type MathType = + | BaseMathType + | FunnelMathType + | PropertyMathType + | CountPerActorMathType + | GroupMathType + | HogQLMathType export interface EntityNode extends Node { name?: string diff --git a/frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx b/frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx index 0ce194ebca5c0..6340b62226ad7 100644 --- a/frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx +++ b/frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx @@ -6,7 +6,6 @@ import { CSS } from '@dnd-kit/utilities' import { IconCopy, IconEllipsis, IconFilter, IconPencil, IconTrash, IconWarning } from '@posthog/icons' import { LemonBadge, - LemonCheckbox, LemonDivider, LemonMenu, LemonSelect, @@ -21,8 +20,7 @@ import { PropertyKeyInfo } from 'lib/components/PropertyKeyInfo' import { SeriesGlyph, SeriesLetter } from 'lib/components/SeriesGlyph' import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types' import { TaxonomicPopover, TaxonomicStringPopover } from 'lib/components/TaxonomicPopover/TaxonomicPopover' -import { IconWithCount } from 'lib/lemon-ui/icons' -import { SortableDragIcon } from 'lib/lemon-ui/icons' +import { IconWithCount, SortableDragIcon } from 'lib/lemon-ui/icons' import { LemonButton } from 'lib/lemon-ui/LemonButton' import { LemonDropdown } from 'lib/lemon-ui/LemonDropdown' import { Tooltip } from 'lib/lemon-ui/Tooltip' @@ -529,20 +527,15 @@ export function ActionFilterRow({ { label: () => ( <> - { - onMathSelect( - index, - checked - ? BaseMathType.FirstTimeForUser - : undefined - ) - }} - data-attr={`math-first-time-for-user-${index}`} - label="Count by first time for user" - fullWidth + @@ -570,7 +563,7 @@ export function ActionFilterRow({ @@ -649,8 +642,13 @@ function useMathSelectorOptions({ const isStickiness = query && isInsightVizNode(query) && isStickinessQuery(query.source) - const { needsUpgradeForGroups, canStartUsingGroups, staticMathDefinitions, staticActorsOnlyMathDefinitions } = - useValues(mathsLogic) + const { + needsUpgradeForGroups, + canStartUsingGroups, + staticMathDefinitions, + funnelMathDefinitions, + staticActorsOnlyMathDefinitions, + } = useValues(mathsLogic) const [propertyMathTypeShown, setPropertyMathTypeShown] = useState( isPropertyValueMath(math) ? math : PropertyMathType.Average @@ -659,9 +657,14 @@ function useMathSelectorOptions({ isCountPerActorMath(math) ? math : CountPerActorMathType.Average ) - const options: LemonSelectOption[] = Object.entries( - mathAvailability != MathAvailability.ActorsOnly ? staticMathDefinitions : staticActorsOnlyMathDefinitions - ) + let definitions = staticMathDefinitions + if (mathAvailability === MathAvailability.FunnelsOnly) { + definitions = funnelMathDefinitions + } else if (mathAvailability === MathAvailability.ActorsOnly) { + definitions = staticActorsOnlyMathDefinitions + } + + const options: LemonSelectOption[] = Object.entries(definitions) .filter(([key]) => { if (isStickiness) { // Remove WAU and MAU from stickiness insights @@ -691,7 +694,7 @@ function useMathSelectorOptions({ } }) - if (mathAvailability !== MathAvailability.ActorsOnly) { + if (mathAvailability !== MathAvailability.ActorsOnly && mathAvailability !== MathAvailability.FunnelsOnly) { options.splice(1, 0, { value: countPerActorMathTypeShown, label: `Count per user ${COUNT_PER_ACTOR_MATH_DEFINITIONS[countPerActorMathTypeShown].shortName}`, @@ -749,12 +752,14 @@ function useMathSelectorOptions({ }) } - options.push({ - value: HogQLMathType.HogQL, - label: 'HogQL expression', - tooltip: 'Aggregate events by custom SQL expression.', - 'data-attr': `math-node-hogql-expression-${index}`, - }) + if (mathAvailability !== MathAvailability.FunnelsOnly) { + options.push({ + value: HogQLMathType.HogQL, + label: 'HogQL expression', + tooltip: 'Aggregate events by custom SQL expression.', + 'data-attr': `math-node-hogql-expression-${index}`, + }) + } return [ { diff --git a/frontend/src/scenes/trends/mathsLogic.tsx b/frontend/src/scenes/trends/mathsLogic.tsx index 1e5c93663bcd9..04756fd135dd9 100644 --- a/frontend/src/scenes/trends/mathsLogic.tsx +++ b/frontend/src/scenes/trends/mathsLogic.tsx @@ -4,7 +4,7 @@ import { groupsAccessLogic } from 'lib/introductions/groupsAccessLogic' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' import { groupsModel } from '~/models/groupsModel' -import { BaseMathType, CountPerActorMathType, HogQLMathType, PropertyMathType } from '~/types' +import { BaseMathType, CountPerActorMathType, FunnelMathType, HogQLMathType, PropertyMathType } from '~/types' import type { mathsLogicType } from './mathsLogicType' @@ -25,6 +25,50 @@ export interface MathDefinition { category: MathCategory } +export const FUNNEL_MATH_DEFINITIONS: Record = { + [FunnelMathType.AnyMatch]: { + name: 'Any events match', + shortName: 'any event', + description: <>Any event of this type that matches the filter will count towards the funnel, + category: MathCategory.EventCount, + }, + [FunnelMathType.FirstTimeForUser]: { + name: 'First event for user', + shortName: 'first event', + description: ( + <> + Only the first time the user performed this event will count towards the funnel, and only if it matches + the event filters. +
+
+ + Example: If the we are looking for pageview events to posthog.com/about, but the user's first + pageview was on posthog.com, it will not match, even if they went to posthog.com/about afterwards. + + + ), + category: MathCategory.EventCount, + }, + [FunnelMathType.FirstTimeForUserWithFilters]: { + name: 'First matching event for user', + shortName: 'first matching event', + description: ( + <> + The first time the user performed this event that matches the event filters will count towards the + funnel. +
+
+ + Example: If the we are looking for pageview events to posthog.com/about, and the user's first + pageview was on posthog.com but then they navigated to posthog.com/about, it will match the pageview + event from posthog.com/about + + + ), + category: MathCategory.EventCount, + }, +} + export const BASE_MATH_DEFINITIONS: Record = { [BaseMathType.TotalCount]: { name: 'Total count', @@ -320,6 +364,15 @@ export const mathsLogic = kea([ return filterMathTypesUnderFeatureFlags(allMathDefinitions, featureFlags) }, ], + funnelMathDefinitions: [ + (s) => [s.featureFlags], + (featureFlags): Record => { + const funnelMathDefinitions: Record = { + ...FUNNEL_MATH_DEFINITIONS, + } + return filterMathTypesUnderFeatureFlags(funnelMathDefinitions, featureFlags) + }, + ], // Static means the options do not have nested selectors (like math function) staticMathDefinitions: [ (s) => [s.groupsMathDefinitions, s.needsUpgradeForGroups, s.featureFlags], diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 4840eb2df96ac..33cacd4590382 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -3514,6 +3514,12 @@ export interface InstanceSetting { is_secret: boolean } +export enum FunnelMathType { + AnyMatch = 'total', + FirstTimeForUser = 'first_time_for_user', + FirstTimeForUserWithFilters = 'first_time_for_user_with_filters', +} + export enum BaseMathType { TotalCount = 'total', UniqueUsers = 'dau', diff --git a/posthog/hogql_queries/insights/funnels/base.py b/posthog/hogql_queries/insights/funnels/base.py index 8066ada3c642f..cf6836a4dd168 100644 --- a/posthog/hogql_queries/insights/funnels/base.py +++ b/posthog/hogql_queries/insights/funnels/base.py @@ -25,7 +25,6 @@ from posthog.queries.util import correct_result_for_sampling from posthog.schema import ( ActionsNode, - BaseMathType, BreakdownAttributionType, BreakdownType, DataWarehouseNode, @@ -34,6 +33,7 @@ FunnelTimeToConvertResults, FunnelVizType, FunnelExclusionEventsNode, + FunnelMathType, ) from posthog.types import EntityNode, ExclusionEntityNode @@ -697,10 +697,14 @@ def _build_step_query( filter_expr = property_to_expr(entity.properties, self.context.team) filters.append(filter_expr) - if entity.math == BaseMathType.FIRST_TIME_FOR_USER: + if entity.math == FunnelMathType.FIRST_TIME_FOR_USER: subquery = FirstTimeForUserAggregationQuery(self.context, filter_expr, event_expr).to_query() first_time_filter = parse_expr("e.uuid IN {subquery}", placeholders={"subquery": subquery}) return ast.And(exprs=[*filters, first_time_filter]) + elif entity.math == FunnelMathType.FIRST_TIME_FOR_USER_WITH_FILTERS: + subquery = FirstTimeForUserAggregationQuery(self.context, ast.Constant(value=1), filter_expr).to_query() + first_time_filter = parse_expr("e.uuid IN {subquery}", placeholders={"subquery": subquery}) + return ast.And(exprs=[*filters, first_time_filter]) elif len(filters) > 1: return ast.And(exprs=filters) return filters[0] diff --git a/posthog/hogql_queries/insights/funnels/test/test_funnel.py b/posthog/hogql_queries/insights/funnels/test/test_funnel.py index c5dee72089d1d..4b3b2e1aa2360 100644 --- a/posthog/hogql_queries/insights/funnels/test/test_funnel.py +++ b/posthog/hogql_queries/insights/funnels/test/test_funnel.py @@ -47,6 +47,7 @@ InsightDateRange, PersonsOnEventsMode, PropertyOperator, + FunnelMathType, ) from posthog.test.base import ( APIBaseTest, @@ -4189,6 +4190,66 @@ def test_first_time_for_user_funnel_multiple_ids(self): self.assertEqual(results[0]["count"], 3) self.assertEqual(results[1]["count"], 1) + def test_first_time_for_user_funnel_person_properties(self): + _create_person(distinct_ids=["user_1"], team=self.team, properties={"email": "test@test.com"}) + _create_person( + distinct_ids=["user_2"], + properties={"email": "bonjonjovi@gmail.com"}, + team=self.team, + ) + + _create_event( + team=self.team, + event="event1", + distinct_id="user_1", + timestamp="2024-03-20T13:00:00Z", + ) + _create_event( + team=self.team, + event="event1", + distinct_id="user_1", + properties={"property": "woah"}, + timestamp="2024-03-21T13:00:00Z", + ) + _create_event( + team=self.team, + event="event1", + distinct_id="user_2", + timestamp="2024-03-22T14:00:00Z", + ) + _create_event( + team=self.team, + event="event2", + distinct_id="user_1", + timestamp="2024-03-23T13:00:00Z", + ) + + query = FunnelsQuery( + series=[ + EventsNode( + event="event1", + math=FunnelMathType.FIRST_TIME_FOR_USER_WITH_FILTERS, + properties=[ + EventPropertyFilter(key="property", value="woah", operator=PropertyOperator.EXACT), + ], + ), + EventsNode(event="event2"), + ], + dateRange=InsightDateRange( + date_from="2024-03-20", + date_to="2024-03-24", + ), + ) + results = FunnelsQueryRunner(query=query, team=self.team).calculate().results + + self.assertEqual(results[0]["count"], 1) + self.assertEqual(results[1]["count"], 1) + + query.series[0].math = FunnelMathType.FIRST_TIME_FOR_USER + results = FunnelsQueryRunner(query=query, team=self.team).calculate().results + # classic and udf funnels handle no events differently + assert len(results) == 0 or results[0]["count"] == 0 + def test_funnel_personless_events_are_supported(self): user_id = uuid.uuid4() _create_event( diff --git a/posthog/schema.py b/posthog/schema.py index 941359e136d09..d78dc0d8ac2f8 100644 --- a/posthog/schema.py +++ b/posthog/schema.py @@ -581,6 +581,12 @@ class FunnelLayout(StrEnum): VERTICAL = "vertical" +class FunnelMathType(StrEnum): + TOTAL = "total" + FIRST_TIME_FOR_USER = "first_time_for_user" + FIRST_TIME_FOR_USER_WITH_FILTERS = "first_time_for_user_with_filters" + + class FunnelPathType(StrEnum): FUNNEL_PATH_BEFORE_STEP = "funnel_path_before_step" FUNNEL_PATH_BETWEEN_STEPS = "funnel_path_between_steps" @@ -4190,7 +4196,14 @@ class DataWarehouseNode(BaseModel): id_field: str kind: Literal["DataWarehouseNode"] = "DataWarehouseNode" math: Optional[ - Union[BaseMathType, PropertyMathType, CountPerActorMathType, Literal["unique_group"], Literal["hogql"]] + Union[ + BaseMathType, + FunnelMathType, + PropertyMathType, + CountPerActorMathType, + Literal["unique_group"], + Literal["hogql"], + ] ] = None math_group_type_index: Optional[MathGroupTypeIndex] = None math_hogql: Optional[str] = None @@ -4273,7 +4286,14 @@ class EntityNode(BaseModel): ) kind: NodeKind math: Optional[ - Union[BaseMathType, PropertyMathType, CountPerActorMathType, Literal["unique_group"], Literal["hogql"]] + Union[ + BaseMathType, + FunnelMathType, + PropertyMathType, + CountPerActorMathType, + Literal["unique_group"], + Literal["hogql"], + ] ] = None math_group_type_index: Optional[MathGroupTypeIndex] = None math_hogql: Optional[str] = None @@ -4344,7 +4364,14 @@ class EventsNode(BaseModel): kind: Literal["EventsNode"] = "EventsNode" limit: Optional[int] = None math: Optional[ - Union[BaseMathType, PropertyMathType, CountPerActorMathType, Literal["unique_group"], Literal["hogql"]] + Union[ + BaseMathType, + FunnelMathType, + PropertyMathType, + CountPerActorMathType, + Literal["unique_group"], + Literal["hogql"], + ] ] = None math_group_type_index: Optional[MathGroupTypeIndex] = None math_hogql: Optional[str] = None @@ -4405,7 +4432,14 @@ class FunnelExclusionActionsNode(BaseModel): id: int kind: Literal["ActionsNode"] = "ActionsNode" math: Optional[ - Union[BaseMathType, PropertyMathType, CountPerActorMathType, Literal["unique_group"], Literal["hogql"]] + Union[ + BaseMathType, + FunnelMathType, + PropertyMathType, + CountPerActorMathType, + Literal["unique_group"], + Literal["hogql"], + ] ] = None math_group_type_index: Optional[MathGroupTypeIndex] = None math_hogql: Optional[str] = None @@ -4466,7 +4500,14 @@ class FunnelExclusionEventsNode(BaseModel): kind: Literal["EventsNode"] = "EventsNode" limit: Optional[int] = None math: Optional[ - Union[BaseMathType, PropertyMathType, CountPerActorMathType, Literal["unique_group"], Literal["hogql"]] + Union[ + BaseMathType, + FunnelMathType, + PropertyMathType, + CountPerActorMathType, + Literal["unique_group"], + Literal["hogql"], + ] ] = None math_group_type_index: Optional[MathGroupTypeIndex] = None math_hogql: Optional[str] = None @@ -4738,7 +4779,14 @@ class AIActionsNode(BaseModel): ] = None kind: Literal["EventsNode"] = "EventsNode" math: Optional[ - Union[BaseMathType, PropertyMathType, CountPerActorMathType, Literal["unique_group"], Literal["hogql"]] + Union[ + BaseMathType, + FunnelMathType, + PropertyMathType, + CountPerActorMathType, + Literal["unique_group"], + Literal["hogql"], + ] ] = None math_group_type_index: Optional[MathGroupTypeIndex] = None math_property: Optional[str] = None @@ -4779,7 +4827,14 @@ class AIEventsNode(BaseModel): ] = None kind: Literal["EventsNode"] = "EventsNode" math: Optional[ - Union[BaseMathType, PropertyMathType, CountPerActorMathType, Literal["unique_group"], Literal["hogql"]] + Union[ + BaseMathType, + FunnelMathType, + PropertyMathType, + CountPerActorMathType, + Literal["unique_group"], + Literal["hogql"], + ] ] = None math_group_type_index: Optional[MathGroupTypeIndex] = None math_property: Optional[str] = None @@ -4830,7 +4885,14 @@ class ActionsNode(BaseModel): id: int kind: Literal["ActionsNode"] = "ActionsNode" math: Optional[ - Union[BaseMathType, PropertyMathType, CountPerActorMathType, Literal["unique_group"], Literal["hogql"]] + Union[ + BaseMathType, + FunnelMathType, + PropertyMathType, + CountPerActorMathType, + Literal["unique_group"], + Literal["hogql"], + ] ] = None math_group_type_index: Optional[MathGroupTypeIndex] = None math_hogql: Optional[str] = None