feat(MA): disabling all events as a conversion goal in ma (#40667)

Co-authored-by: Javier Bahamondes <javierbahamondes@Javiers-MacBook-Pro.local>
This commit is contained in:
Javier Bahamondes
2025-10-30 16:34:54 -03:00
committed by GitHub
parent 77bfca7a77
commit d6990b5fb9
7 changed files with 316 additions and 98 deletions

View File

@@ -1,5 +1,5 @@
import { BuiltLogic, LogicWrapper, useValues } from 'kea'
import { useState } from 'react'
import { useMemo, useState } from 'react'
import { useAttachedLogic } from 'lib/logic/scenes/useAttachedLogic'
@@ -13,6 +13,10 @@ import {
import { QueryContext } from '~/queries/types'
import { marketingAnalyticsSettingsLogic } from '../../logic/marketingAnalyticsSettingsLogic'
import {
MarketingAnalyticsValidationWarningBanner,
validateConversionGoals,
} from '../MarketingAnalyticsValidationWarningBanner'
const BASE_METRICS_COUNT = 6 // Total Cost, Total Clicks, CPC, CTR, Total Impressions, Reported Conversion
@@ -44,6 +48,8 @@ export function MarketingAnalyticsOverview(props: {
const samplingRate = marketingOverviewQueryResponse?.samplingRate
const validationWarnings = useMemo(() => validateConversionGoals(conversion_goals), [conversion_goals])
// Convert results dict to array for rendering and map to OverviewItem
const overviewItems: OverviewItem[] = marketingOverviewQueryResponse?.results
? Object.entries(marketingOverviewQueryResponse.results).map(([key, item]) => ({
@@ -61,19 +67,24 @@ export function MarketingAnalyticsOverview(props: {
const numSkeletons = BASE_METRICS_COUNT + conversionGoalMetrics
return (
<OverviewGrid
items={overviewItems}
loading={responseLoading}
numSkeletons={numSkeletons}
samplingRate={samplingRate}
usedPreAggregatedTables={false}
labelFromKey={labelFromKey}
settingsLinkFromKey={() => null}
dashboardLinkFromKey={() => null}
filterEmptyItems={filterEmptyMetrics}
showBetaTags={() => false}
compact={true}
/>
<>
{validationWarnings && validationWarnings.length > 0 && (
<MarketingAnalyticsValidationWarningBanner warnings={validationWarnings} />
)}
<OverviewGrid
items={overviewItems}
loading={responseLoading}
numSkeletons={numSkeletons}
samplingRate={samplingRate}
usedPreAggregatedTables={false}
labelFromKey={labelFromKey}
settingsLinkFromKey={() => null}
dashboardLinkFromKey={() => null}
filterEmptyItems={filterEmptyMetrics}
showBetaTags={() => false}
compact={true}
/>
</>
)
}

View File

@@ -1,6 +1,7 @@
import './MarketingAnalyticsTableStyleOverride.scss'
import { BuiltLogic, LogicWrapper, useActions } from 'kea'
import { BuiltLogic, LogicWrapper, useActions, useValues } from 'kea'
import { useMemo } from 'react'
import { IconGear } from '@posthog/icons'
import { LemonButton, LemonSwitch } from '@posthog/lemon-ui'
@@ -17,8 +18,13 @@ import { webAnalyticsDataTableQueryContext } from '~/scenes/web-analytics/tiles/
import { InsightLogicProps } from '~/types'
import { marketingAnalyticsLogic } from '../../logic/marketingAnalyticsLogic'
import { marketingAnalyticsSettingsLogic } from '../../logic/marketingAnalyticsSettingsLogic'
import { marketingAnalyticsTableLogic } from '../../logic/marketingAnalyticsTableLogic'
import { MarketingAnalyticsCell } from '../../shared'
import {
MarketingAnalyticsValidationWarningBanner,
validateConversionGoals,
} from '../MarketingAnalyticsValidationWarningBanner'
import { DraftConversionGoalControls } from './DraftConversionGoalControls'
import { MarketingAnalyticsColumnConfigModal } from './MarketingAnalyticsColumnConfigModal'
@@ -35,6 +41,9 @@ export const MarketingAnalyticsTable = ({
}: MarketingAnalyticsTableProps): JSX.Element => {
const { setQuery } = useActions(marketingAnalyticsTableLogic)
const { showColumnConfigModal } = useActions(marketingAnalyticsLogic)
const { conversion_goals } = useValues(marketingAnalyticsSettingsLogic)
const validationWarnings = useMemo(() => validateConversionGoals(conversion_goals), [conversion_goals])
const handleIncludeAllConversionsChange = (checked: boolean): void => {
const sourceQuery = query.source as MarketingAnalyticsTableQuery
@@ -96,6 +105,11 @@ export const MarketingAnalyticsTable = ({
</div>
</div>
</div>
{validationWarnings && validationWarnings.length > 0 && (
<div className="pt-2">
<MarketingAnalyticsValidationWarningBanner warnings={validationWarnings} />
</div>
)}
<div className="relative marketing-analytics-table-container">
<Query
attachTo={attachTo}

View File

@@ -0,0 +1,75 @@
import { IconExternal } from '@posthog/icons'
import { Link } from '@posthog/lemon-ui'
import { LemonBanner } from '@posthog/lemon-ui'
import { urls } from 'scenes/urls'
import { NodeKind } from '~/queries/schema/schema-general'
enum ValidationWarningLink {
MarketingAnalyticsSettings = 'marketing_analytics_settings',
}
interface MarketingAnalyticsValidationWarning {
message: string
/** Link to navigate to fix the issue */
link?: ValidationWarningLink
}
export const validateConversionGoals = (goals: any[]): MarketingAnalyticsValidationWarning[] => {
const warnings: MarketingAnalyticsValidationWarning[] = []
for (const goal of goals) {
if (goal.kind === NodeKind.EventsNode) {
const event = 'event' in goal ? goal.event : null
if (event === null || event === '') {
warnings.push({
message: `Conversion goal "${goal.conversion_goal_name}" uses "All Events" which is not supported.`,
link: ValidationWarningLink.MarketingAnalyticsSettings,
} as MarketingAnalyticsValidationWarning)
}
}
}
return warnings
}
interface MarketingAnalyticsValidationWarningBannerProps {
warnings: MarketingAnalyticsValidationWarning[]
}
const getLinkUrl = (link: ValidationWarningLink): JSX.Element | null => {
switch (link) {
case ValidationWarningLink.MarketingAnalyticsSettings:
return (
<>
Check{' '}
<Link to={urls.settings('environment-marketing-analytics')} target="_blank">
marketing analytics settings
<IconExternal />
</Link>{' '}
for more details.
</>
)
default:
return null
}
}
export function MarketingAnalyticsValidationWarningBanner({
warnings,
}: MarketingAnalyticsValidationWarningBannerProps): JSX.Element | null {
if (!warnings || warnings.length === 0) {
return null
}
return (
<>
{warnings.map((warning, index) => (
<LemonBanner key={index} type="warning" className="mb-2">
{warning.message} {warning.link ? getLinkUrl(warning.link) : null}
</LemonBanner>
))}
</>
)
}

View File

@@ -1,3 +1,5 @@
import { useState } from 'react'
import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types'
import { ActionFilter as ActionFilterComponent } from 'scenes/insights/filters/ActionFilter/ActionFilter'
import { MathAvailability } from 'scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow'
@@ -29,6 +31,8 @@ interface ConversionGoalDropdownProps {
}
export function ConversionGoalDropdown({ value, onChange, typeKey }: ConversionGoalDropdownProps): JSX.Element {
const [error, setError] = useState<string | null>(null)
// Create a proper ActionFilter-compatible filter object
const currentFilter = {
events:
@@ -67,85 +71,100 @@ export function ConversionGoalDropdown({ value, onChange, typeKey }: ConversionG
}
return (
<ActionFilterComponent
bordered
allowedMathTypes={[BaseMathType.TotalCount, BaseMathType.UniqueUsers, PropertyMathType.Sum]}
filters={currentFilter}
mathAvailability={MathAvailability.All}
setFilters={({ actions, events, data_warehouse }: Partial<FilterType>): void => {
const series = actionsAndEventsToSeries(
{
actions: actions as ActionFilter[] | undefined,
events: events as ActionFilter[] | undefined,
data_warehouse: data_warehouse as DataWarehouseFilter[] | undefined,
},
true,
MathAvailability.All
)
const firstSerie = series[0] || value
const newFilter: ConversionGoalFilter = {
...value,
...firstSerie,
// Preserve the existing schema to keep UTM mappings
schema_map: {
...value.schema_map,
},
properties: firstSerie?.properties || [], // if we clear the filter we need the properties to be set to an empty array
}
// Remove the event field from ActionsNode to prevent validation errors
if (newFilter.kind === NodeKind.ActionsNode && 'event' in newFilter) {
const { event, ...rest } = newFilter as any
Object.assign(newFilter, rest)
}
// Override the schema with the schema from the data warehouse
if (data_warehouse?.[0]?.type === EntityTypes.DATA_WAREHOUSE) {
const dwNode = data_warehouse[0] as DataWarehouseFilter & Record<ConversionGoalSchema, string>
const schema = dwNode
const overrideSchema: Record<ConversionGoalSchema, string> = {
utm_campaign_name: schema[UTM_CAMPAIGN_NAME_SCHEMA_FIELD],
utm_source_name: schema[UTM_SOURCE_NAME_SCHEMA_FIELD],
timestamp_field: schema[TIMESTAMP_FIELD_SCHEMA_FIELD],
distinct_id_field: schema[DISTINCT_ID_FIELD_SCHEMA_FIELD],
}
newFilter.schema_map = overrideSchema
// Cast to DataWarehouseNode for type safety
const dwFilter = newFilter as DataWarehouseNode & ConversionGoalFilter
// Remove the event field that causes validation to fail
if ('event' in dwFilter) {
delete (dwFilter as any).event
<div>
<ActionFilterComponent
bordered
allowedMathTypes={[BaseMathType.TotalCount, BaseMathType.UniqueUsers, PropertyMathType.Sum]}
filters={currentFilter}
mathAvailability={MathAvailability.All}
setFilters={({ actions, events, data_warehouse }: Partial<FilterType>): void => {
// Check if user is trying to select "All Events"
if (events?.[0]?.id === null || events?.[0]?.id === '') {
setError('`All events` cannot be used as a conversion goal. Please select a specific event.')
return
}
dwFilter.kind = NodeKind.DataWarehouseNode
// Clear any previous errors
setError(null)
// Set all required ConversionGoalFilter3 fields
dwFilter.id = dwNode.table_name || String(dwNode.id) || ''
dwFilter.id_field = dwNode.id_field || schema[DISTINCT_ID_FIELD_SCHEMA_FIELD] || 'id'
dwFilter.distinct_id_field =
dwNode.distinct_id_field || schema[DISTINCT_ID_FIELD_SCHEMA_FIELD] || 'distinct_id'
dwFilter.table_name = dwNode.table_name || ''
dwFilter.timestamp_field =
dwNode.timestamp_field || schema[TIMESTAMP_FIELD_SCHEMA_FIELD] || 'timestamp'
dwFilter.dw_source_type = dwNode.dw_source_type
}
onChange(newFilter)
}}
typeKey={typeKey}
showSeriesIndicator={false}
entitiesLimit={1}
showNumericalPropsOnly={true}
hideRename={true}
actionsTaxonomicGroupTypes={[
TaxonomicFilterGroupType.Events,
TaxonomicFilterGroupType.Actions,
TaxonomicFilterGroupType.DataWarehouse,
]}
dataWarehousePopoverFields={conversionGoalPopoverFields}
/>
const series = actionsAndEventsToSeries(
{
actions: actions as ActionFilter[] | undefined,
events: events as ActionFilter[] | undefined,
data_warehouse: data_warehouse as DataWarehouseFilter[] | undefined,
},
true,
MathAvailability.All
)
const firstSerie = series[0] || value
const newFilter: ConversionGoalFilter = {
...value,
...firstSerie,
// Preserve the existing schema to keep UTM mappings
schema_map: {
...value.schema_map,
},
properties: firstSerie?.properties || [], // if we clear the filter we need the properties to be set to an empty array
}
// Remove the event field from ActionsNode to prevent validation errors
if (newFilter.kind === NodeKind.ActionsNode && 'event' in newFilter) {
const { event, ...rest } = newFilter as any
Object.assign(newFilter, rest)
}
// Override the schema with the schema from the data warehouse
if (data_warehouse?.[0]?.type === EntityTypes.DATA_WAREHOUSE) {
const dwNode = data_warehouse[0] as DataWarehouseFilter & Record<ConversionGoalSchema, string>
const schema = dwNode
const overrideSchema: Record<ConversionGoalSchema, string> = {
utm_campaign_name: schema[UTM_CAMPAIGN_NAME_SCHEMA_FIELD],
utm_source_name: schema[UTM_SOURCE_NAME_SCHEMA_FIELD],
timestamp_field: schema[TIMESTAMP_FIELD_SCHEMA_FIELD],
distinct_id_field: schema[DISTINCT_ID_FIELD_SCHEMA_FIELD],
}
newFilter.schema_map = overrideSchema
// Cast to DataWarehouseNode for type safety
const dwFilter = newFilter as DataWarehouseNode & ConversionGoalFilter
// Remove the event field that causes validation to fail
if ('event' in dwFilter) {
delete (dwFilter as any).event
}
dwFilter.kind = NodeKind.DataWarehouseNode
// Set all required ConversionGoalFilter3 fields
dwFilter.id = dwNode.table_name || String(dwNode.id) || ''
dwFilter.id_field = dwNode.id_field || schema[DISTINCT_ID_FIELD_SCHEMA_FIELD] || 'id'
dwFilter.distinct_id_field =
dwNode.distinct_id_field || schema[DISTINCT_ID_FIELD_SCHEMA_FIELD] || 'distinct_id'
dwFilter.table_name = dwNode.table_name || ''
dwFilter.timestamp_field =
dwNode.timestamp_field || schema[TIMESTAMP_FIELD_SCHEMA_FIELD] || 'timestamp'
dwFilter.dw_source_type = dwNode.dw_source_type
}
onChange(newFilter)
}}
typeKey={typeKey}
showSeriesIndicator={false}
entitiesLimit={1}
showNumericalPropsOnly={true}
hideRename={true}
actionsTaxonomicGroupTypes={[
TaxonomicFilterGroupType.Events,
TaxonomicFilterGroupType.Actions,
TaxonomicFilterGroupType.DataWarehouse,
]}
dataWarehousePopoverFields={conversionGoalPopoverFields}
excludedProperties={{
[TaxonomicFilterGroupType.Events]: [null],
}}
/>
{error && <div className="text-danger mt-2 text-sm">{error}</div>}
</div>
)
}

View File

@@ -1,7 +1,7 @@
import { useActions, useValues } from 'kea'
import { useState } from 'react'
import { useMemo, useState } from 'react'
import { IconCheck, IconPencil, IconTrash, IconX } from '@posthog/icons'
import { IconCheck, IconPencil, IconTrash, IconWarning, IconX } from '@posthog/icons'
import { LemonButton, LemonInput } from '@posthog/lemon-ui'
import { LemonTable } from 'lib/lemon-ui/LemonTable'
@@ -9,9 +9,13 @@ import { uuid } from 'lib/utils'
import { QUERY_TYPES_METADATA } from 'scenes/saved-insights/SavedInsights'
import { SceneSection } from '~/layout/scenes/components/SceneSection'
import { ConversionGoalFilter } from '~/queries/schema/schema-general'
import { ConversionGoalFilter, NodeKind } from '~/queries/schema/schema-general'
import { marketingAnalyticsSettingsLogic } from '../../logic/marketingAnalyticsSettingsLogic'
import {
MarketingAnalyticsValidationWarningBanner,
validateConversionGoals,
} from '../MarketingAnalyticsValidationWarningBanner'
import { ConversionGoalDropdown } from '../common/ConversionGoalDropdown'
import { defaultConversionGoalFilter } from './constants'
@@ -38,6 +42,8 @@ export function ConversionGoalsConfiguration({
const [editingGoalId, setEditingGoalId] = useState<string | null>(null)
const [editingGoal, setEditingGoal] = useState<ConversionGoalFilter | null>(null)
const validationWarnings = useMemo(() => validateConversionGoals(conversion_goals), [conversion_goals])
const handleAddConversionGoal = (): void => {
let conversionGoalName = formState.name.trim()
if (conversionGoalName === '') {
@@ -86,6 +92,10 @@ export function ConversionGoalsConfiguration({
: undefined
}
>
{validationWarnings.length > 0 && (
<MarketingAnalyticsValidationWarningBanner warnings={validationWarnings} />
)}
{/* Add New Conversion Goal Form */}
<div className="border rounded p-4 space-y-4">
<h4 className="font-medium">Add new conversion goal</h4>
@@ -137,6 +147,13 @@ export function ConversionGoalsConfiguration({
key: 'name',
title: 'Goal name',
render: (_, goal: ConversionGoalFilter) => {
// Check if this goal is invalid (All Events)
const isInvalid =
goal.kind === NodeKind.EventsNode &&
('event' in goal
? (goal as any).event === null || (goal as any).event === ''
: false)
if (editingGoalId === goal.conversion_goal_id && editingGoal) {
return (
<LemonInput
@@ -150,7 +167,14 @@ export function ConversionGoalsConfiguration({
/>
)
}
return goal.conversion_goal_name
return (
<div className={isInvalid ? 'flex items-center gap-1.5' : ''}>
<span className={isInvalid ? 'text-warning' : ''}>
{goal.conversion_goal_name}
</span>
{isInvalid && <IconWarning className="text-warning w-4 h-4 shrink-0" />}
</div>
)
},
},
{

View File

@@ -1,7 +1,7 @@
from abc import ABC, abstractmethod
from datetime import datetime
from functools import cached_property
from typing import Generic, Optional, TypeVar
from typing import Generic, Optional, TypeVar, cast
import structlog
@@ -11,6 +11,7 @@ from posthog.schema import (
ConversionGoalFilter3,
DateRange,
MarketingAnalyticsHelperForColumnNames,
NodeKind,
)
from posthog.hogql import ast
@@ -139,6 +140,28 @@ class MarketingAnalyticsBaseQueryRunner(AnalyticsQueryRunner[ResponseType], ABC,
return conversion_goals
def _filter_invalid_conversion_goals(
self, conversion_goals: list[ConversionGoalFilter1 | ConversionGoalFilter2 | ConversionGoalFilter3]
) -> list[ConversionGoalFilter1 | ConversionGoalFilter2 | ConversionGoalFilter3]:
"""
Filter out invalid conversion goals (e.g., those using "All Events").
Returns only valid conversion goals.
"""
valid_goals = []
for goal in conversion_goals:
# Skip "All Events" goals
if goal.kind == cast(str, NodeKind.EVENTS_NODE):
event_name = getattr(goal, "event", None)
if event_name is None or event_name == "":
logger.info(
"filtering_out_all_events_conversion_goal",
goal_name=getattr(goal, "conversion_goal_name", "Unknown"),
)
continue
valid_goals.append(goal)
return valid_goals
def _create_conversion_goal_processors(
self, conversion_goals: list[ConversionGoalFilter1 | ConversionGoalFilter2 | ConversionGoalFilter3]
) -> list:
@@ -289,9 +312,14 @@ class MarketingAnalyticsBaseQueryRunner(AnalyticsQueryRunner[ResponseType], ABC,
# Build the union query using the factory
union_query_string = self._factory(date_range=self.query_date_range).build_union_query(adapters)
# Get conversion goals and create processors
# Get conversion goals and filter out invalid ones
conversion_goals = self._get_team_conversion_goals()
processors = self._create_conversion_goal_processors(conversion_goals) if conversion_goals else []
valid_conversion_goals = self._filter_invalid_conversion_goals(conversion_goals)
# Create processors only for valid conversion goals
processors = (
self._create_conversion_goal_processors(valid_conversion_goals) if valid_conversion_goals else []
)
# Build the complete query with CTEs using AST
return self._build_complete_query_ast(union_query_string, processors, self.query_date_range)

View File

@@ -148,6 +148,53 @@ class TestMarketingAnalyticsTableQueryRunner(ClickhouseTestMixin, BaseTest):
assert len(goals) == 1
assert goals[0] == conversion_goal
def test_all_events_conversion_goal_filtered_out(self):
"""Test that conversion goals with 'All Events' are filtered out and warnings are returned"""
self.team.marketing_analytics_config.conversion_goals = [
{
"kind": NodeKind.EVENTS_NODE,
"event": "purchase",
"conversion_goal_id": "valid_goal",
"conversion_goal_name": "Valid Purchase Goal",
"name": "purchase",
"math": BaseMathType.TOTAL,
"schema_map": {"utm_campaign_name": "utm_campaign", "utm_source_name": "utm_source"},
},
{
"kind": NodeKind.EVENTS_NODE,
"event": "",
"conversion_goal_id": "invalid_goal",
"conversion_goal_name": "Invalid All Events Goal",
"name": "All events",
"math": BaseMathType.TOTAL,
"schema_map": {"utm_campaign_name": "utm_campaign", "utm_source_name": "utm_source"},
},
{
"kind": NodeKind.EVENTS_NODE,
"event": None,
"conversion_goal_id": "invalid_goal_null",
"conversion_goal_name": "Invalid Null Events Goal",
"name": "All events",
"math": BaseMathType.TOTAL,
"schema_map": {"utm_campaign_name": "utm_campaign", "utm_source_name": "utm_source"},
},
]
self.team.save()
runner = self._create_query_runner()
all_goals = runner._get_team_conversion_goals()
assert len(all_goals) == 3 # 1 valid + 2 invalid
valid_goals = runner._filter_invalid_conversion_goals(all_goals)
assert len(valid_goals) == 1 # Only the valid goal remains
assert valid_goals[0].conversion_goal_name == "Valid Purchase Goal"
with patch.object(MarketingAnalyticsTableQueryRunner, "_get_marketing_source_adapters") as mock_get_adapters:
mock_get_adapters.return_value = []
query = runner.to_query()
assert query is not None
def test_to_query_basic(self):
with patch.object(MarketingAnalyticsTableQueryRunner, "_get_marketing_source_adapters") as mock_get_adapters:
mock_get_adapters.return_value = []