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(insights): Warn about WAU/MAU in total value trends #21067

Merged
merged 13 commits into from
Mar 26, 2024
Merged
34 changes: 23 additions & 11 deletions cypress/e2e/trends.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('Trends', () => {
cy.get('[data-attr=trend-element-subject-1]').click()
cy.get('[data-attr=taxonomic-tab-actions]').click()
cy.get('[data-attr=taxonomic-filter-searchfield]').click().type('home')
cy.contains('Hogflix homepage view').click({ force: true })
cy.contains('Hogflix homepage view').click()

// then
cy.get('[data-attr=trend-line-graph]').should('exist')
Expand Down Expand Up @@ -66,15 +66,15 @@ describe('Trends', () => {
it('Apply specific filter on default pageview event', () => {
cy.get('[data-attr=trend-element-subject-0]').click()
cy.get('[data-attr=taxonomic-filter-searchfield]').click().type('Pageview')
cy.get('.taxonomic-infinite-list').find('.taxonomic-list-row').contains('Pageview').click({ force: true })
cy.get('.taxonomic-infinite-list').find('.taxonomic-list-row').contains('Pageview').click()
cy.get('[data-attr=trend-element-subject-0]').should('have.text', 'Pageview')

// Apply a property filter
cy.get('[data-attr=show-prop-filter-0]').click()
cy.get('[data-attr=property-select-toggle-0]').click()
cy.get('[data-attr=prop-filter-event_properties-1]').click({ force: true })
cy.get('[data-attr=prop-filter-event_properties-1]').click()

cy.get('[data-attr=prop-val]').click({ force: true })
cy.get('[data-attr=prop-val]').click()
// cypress is odd and even though when a human clicks this the right dropdown opens
// in the test that doesn't happen
cy.get('body').then(($body) => {
Expand All @@ -88,22 +88,22 @@ describe('Trends', () => {
it('Apply 1 overall filter', () => {
cy.get('[data-attr=trend-element-subject-0]').click()
cy.get('[data-attr=taxonomic-filter-searchfield]').click().type('Pageview')
cy.get('.taxonomic-infinite-list').find('.taxonomic-list-row').contains('Pageview').click({ force: true })
cy.get('.taxonomic-infinite-list').find('.taxonomic-list-row').contains('Pageview').click()
cy.get('[data-attr=trend-element-subject-0]').should('have.text', 'Pageview')

cy.get('[data-attr$=add-filter-group]').click()
cy.get('[data-attr=property-select-toggle-0]').click()
cy.get('[data-attr=taxonomic-filter-searchfield]').click()
cy.get('[data-attr=prop-filter-event_properties-1]').click({ force: true })
cy.get('[data-attr=prop-val]').click({ force: true })
cy.get('[data-attr=prop-filter-event_properties-1]').click()
cy.get('[data-attr=prop-val]').click()
// cypress is odd and even though when a human clicks this the right dropdown opens
// in the test that doesn't happen
cy.get('body').then(($body) => {
if ($body.find('[data-attr=prop-val-0]').length === 0) {
cy.get('[data-attr=taxonomic-value-select]').click()
}
})
cy.get('[data-attr=prop-val-0]').click({ force: true })
cy.get('[data-attr=prop-val-0]').click()

cy.get('[data-attr=trend-line-graph]', { timeout: 8000 }).should('exist')
})
Expand All @@ -117,14 +117,14 @@ describe('Trends', () => {

it('Apply pie filter', () => {
cy.get('[data-attr=chart-filter]').click()
cy.get('.Popover').find('.LemonButton').contains('Pie').click({ force: true })
cy.get('.Popover').find('.LemonButton').contains('Pie').click()

cy.get('[data-attr=trend-pie-graph]').should('exist')
})

it('Apply table filter', () => {
cy.get('[data-attr=chart-filter]').click()
cy.get('.Popover').find('.LemonButton').contains('Table').click({ force: true })
cy.get('.Popover').find('.LemonButton').contains('Table').click()

cy.get('[data-attr=insights-table-graph]').should('exist')

Expand All @@ -144,7 +144,7 @@ describe('Trends', () => {

it('Apply property breakdown', () => {
cy.get('[data-attr=add-breakdown-button]').click()
cy.get('[data-attr=prop-filter-event_properties-1]').click({ force: true })
cy.get('[data-attr=prop-filter-event_properties-1]').click()
cy.get('[data-attr=trend-line-graph]').should('exist')
})

Expand All @@ -154,4 +154,16 @@ describe('Trends', () => {
cy.contains('All Users*').click()
cy.get('[data-attr=trend-line-graph]').should('exist')
})

it('Show warning on MAU math in total value insight', () => {
cy.get('[data-attr=chart-filter]').click()
cy.get('.Popover').find('.LemonButton').contains('Pie').click()
cy.get('[data-attr=trend-pie-graph]').should('exist') // Make sure the pie chart is loaded before proceeding

cy.get('[data-attr=math-selector-0]').click()
cy.get('[data-attr=math-monthly_active-0] .LemonIcon').should('exist') // This should be the warning icon

cy.get('[data-attr=math-monthly_active-0]').trigger('mouseenter') // Activate warning tooltip
cy.get('.Tooltip').contains('We advise against').should('exist')
})
})
26 changes: 17 additions & 9 deletions frontend/src/lib/constants.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,23 @@
import { LemonSelectOptions } from '@posthog/lemon-ui'

import { ChartDisplayType, Region, SSOProvider } from '../types'
import { ChartDisplayCategory, ChartDisplayType, Region, SSOProvider } from '../types'

// Sync with backend DISPLAY_TYPES_TO_CATEGORIES
export const DISPLAY_TYPES_TO_CATEGORIES: Record<ChartDisplayType, ChartDisplayCategory> = {
[ChartDisplayType.ActionsLineGraph]: ChartDisplayCategory.TimeSeries,
[ChartDisplayType.ActionsBar]: ChartDisplayCategory.TimeSeries,
[ChartDisplayType.ActionsAreaGraph]: ChartDisplayCategory.TimeSeries,
[ChartDisplayType.ActionsLineGraphCumulative]: ChartDisplayCategory.CumulativeTimeSeries,
[ChartDisplayType.BoldNumber]: ChartDisplayCategory.TotalValue,
[ChartDisplayType.ActionsPie]: ChartDisplayCategory.TotalValue,
[ChartDisplayType.ActionsBarValue]: ChartDisplayCategory.TotalValue,
[ChartDisplayType.ActionsTable]: ChartDisplayCategory.TotalValue,
[ChartDisplayType.WorldMap]: ChartDisplayCategory.TotalValue,
}
export const NON_TIME_SERIES_DISPLAY_TYPES = Object.entries(DISPLAY_TYPES_TO_CATEGORIES)
.filter(([, category]) => category === ChartDisplayCategory.TotalValue)
.map(([displayType]) => displayType as ChartDisplayType)

/** Display types which don't allow grouping by unit of time. Sync with backend NON_TIME_SERIES_DISPLAY_TYPES. */
export const NON_TIME_SERIES_DISPLAY_TYPES = [
ChartDisplayType.ActionsTable,
ChartDisplayType.ActionsPie,
ChartDisplayType.ActionsBarValue,
ChartDisplayType.WorldMap,
ChartDisplayType.BoldNumber,
]
/** Display types for which `breakdown` is hidden and ignored. Sync with backend NON_BREAKDOWN_DISPLAY_TYPES. */
export const NON_BREAKDOWN_DISPLAY_TYPES = [ChartDisplayType.BoldNumber]
/** Display types which only work with a single series. */
Expand Down
1 change: 1 addition & 0 deletions frontend/src/lib/lemon-ui/LemonSelect/LemonSelect.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ export function LemonSelect<T extends string | number | boolean | null>({
}
: null
}
tooltip={activeLeaf?.tooltip}
{...buttonProps}
>
<span className="flex flex-1">
Expand Down
10 changes: 5 additions & 5 deletions frontend/src/queries/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -501,14 +501,14 @@
"ChartDisplayType": {
"enum": [
"ActionsLineGraph",
"ActionsLineGraphCumulative",
"ActionsBar",
"ActionsAreaGraph",
"ActionsTable",
"ActionsLineGraphCumulative",
"BoldNumber",
"ActionsPie",
"ActionsBar",
"ActionsBarValue",
"WorldMap",
"BoldNumber"
"ActionsTable",
"WorldMap"
],
"type": "string"
},
Expand Down
3 changes: 3 additions & 0 deletions frontend/src/queries/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
Breakdown,
BreakdownKeyType,
BreakdownType,
ChartDisplayCategory,
ChartDisplayType,
CountPerActorMathType,
EventPropertyFilter,
Expand All @@ -26,6 +27,8 @@ import {
TrendsFilterType,
} from '~/types'

export { ChartDisplayCategory }

// Type alias for number to be reflected as integer in json-schema.
/** @asType integer */
type integer = number
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,22 @@ import { IconPlusSmall } from '@posthog/icons'
import clsx from 'clsx'
import { BindLogic, useActions, useValues } from 'kea'
import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types'
import { DISPLAY_TYPES_TO_CATEGORIES as DISPLAY_TYPES_TO_CATEGORY } from 'lib/constants'
import { LemonButton, LemonButtonProps } from 'lib/lemon-ui/LemonButton'
import { verticalSortableListCollisionDetection } from 'lib/sortable'
import { eventUsageLogic } from 'lib/utils/eventUsageLogic'
import React, { useEffect } from 'react'
import { RenameModal } from 'scenes/insights/filters/ActionFilter/RenameModal'
import { isTrendsFilter } from 'scenes/insights/sharedUtils'

import { ActionFilter as ActionFilterType, FilterType, FunnelExclusionLegacy, InsightType, Optional } from '~/types'
import {
ActionFilter as ActionFilterType,
ChartDisplayType,
FilterType,
FunnelExclusionLegacy,
InsightType,
Optional,
} from '~/types'

import { teamLogic } from '../../../teamLogic'
import { ActionFilterRow, MathAvailability } from './ActionFilterRow/ActionFilterRow'
Expand Down Expand Up @@ -147,6 +156,9 @@ export const ActionFilter = React.forwardRef<HTMLDivElement, ActionFilterProps>(
mathAvailability,
customRowSuffix,
hasBreakdown: !!filters.breakdown,
trendsDisplayCategory: isTrendsFilter(filters)
? DISPLAY_TYPES_TO_CATEGORY[filters.display || ChartDisplayType.ActionsLineGraph]
: null,
actionsTaxonomicGroupTypes,
propertiesTaxonomicGroupTypes,
propertyFiltersPopover,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import './ActionFilterRow.scss'
import { DraggableSyntheticListeners } from '@dnd-kit/core'
import { useSortable } from '@dnd-kit/sortable'
import { CSS } from '@dnd-kit/utilities'
import { IconCopy, IconFilter, IconPencil, IconTrash } from '@posthog/icons'
import { IconCopy, IconFilter, IconPencil, IconTrash, IconWarning } from '@posthog/icons'
import { LemonSelect, LemonSelectOption, LemonSelectOptions } from '@posthog/lemon-ui'
import { BuiltLogic, useActions, useValues } from 'kea'
import { EntityFilterInfo } from 'lib/components/EntityFilterInfo'
Expand Down Expand Up @@ -39,6 +39,7 @@ import {
ActionFilter,
ActionFilter as ActionFilterType,
BaseMathType,
ChartDisplayCategory,
CountPerActorMathType,
EntityType,
EntityTypes,
Expand Down Expand Up @@ -115,6 +116,7 @@ export interface ActionFilterRowProps {
renameRowButton,
deleteButton,
}: Record<string, JSX.Element | string | undefined>) => JSX.Element // build your own row given these components
trendsDisplayCategory: ChartDisplayCategory | null
}

export function ActionFilterRow({
Expand Down Expand Up @@ -142,6 +144,7 @@ export function ActionFilterRow({
disabled = false,
readOnly = false,
renderRow,
trendsDisplayCategory,
}: ActionFilterRowProps): JSX.Element {
const { entityFilterVisible } = useValues(logic)
const {
Expand Down Expand Up @@ -376,6 +379,7 @@ export function ActionFilterRow({
disabled={readOnly}
style={{ maxWidth: '100%', width: 'initial' }}
mathAvailability={mathAvailability}
trendsDisplayCategory={trendsDisplayCategory}
/>
{mathDefinitions[math || BaseMathType.TotalCount]?.category ===
MathCategory.PropertyValue && (
Expand Down Expand Up @@ -513,6 +517,7 @@ interface MathSelectorProps {
disabled?: boolean
disabledReason?: string
onMathSelect: (index: number, value: any) => any
trendsDisplayCategory: ChartDisplayCategory | null
style?: React.CSSProperties
}

Expand All @@ -524,11 +529,14 @@ function isCountPerActorMath(math: string | undefined): math is CountPerActorMat
return !!math && math in COUNT_PER_ACTOR_MATH_DEFINITIONS
}

const TRAILING_MATH_TYPES = new Set<string>([BaseMathType.WeeklyActiveUsers, BaseMathType.MonthlyActiveUsers])

function useMathSelectorOptions({
math,
index,
mathAvailability,
onMathSelect,
trendsDisplayCategory,
}: MathSelectorProps): LemonSelectOptions<string> {
const mountedInsightDataLogic = insightDataLogic.findMounted()
const query = mountedInsightDataLogic?.values?.query
Expand All @@ -549,19 +557,33 @@ function useMathSelectorOptions({
mathAvailability != MathAvailability.ActorsOnly ? staticMathDefinitions : staticActorsOnlyMathDefinitions
)
.filter(([key]) => {
if (!isStickiness) {
return true
if (isStickiness) {
// Remove WAU and MAU from stickiness insights
return !TRAILING_MATH_TYPES.has(key)
}
return true
})
.map(([key, definition]) => {
const shouldWarnAboutTrailingMath =
TRAILING_MATH_TYPES.has(key) && trendsDisplayCategory === ChartDisplayCategory.TotalValue
return {
value: key,
icon: shouldWarnAboutTrailingMath ? <IconWarning /> : undefined,
label: definition.name,
tooltip: !shouldWarnAboutTrailingMath ? (
definition.description
) : (
<>
<p>{definition.description}</p>
<i>
We advise against using "{definition.name}" in total value insights, as this combination has
unclear mechanics. Use "Unique users" instead
</i>
</>
),
'data-attr': `math-${key}-${index}`,
}

// Remove WAU and MAU from stickiness insights
return key !== BaseMathType.WeeklyActiveUsers && key !== BaseMathType.MonthlyActiveUsers
})
.map(([key, definition]) => ({
value: key,
label: definition.name,
tooltip: definition.description,
'data-attr': `math-${key}-${index}`,
}))

if (mathAvailability !== MathAvailability.ActorsOnly) {
options.splice(1, 0, {
Expand All @@ -579,7 +601,6 @@ function useMathSelectorOptions({
options={Object.entries(COUNT_PER_ACTOR_MATH_DEFINITIONS).map(([key, definition]) => ({
value: key,
label: definition.shortName,
tooltip: definition.description,
'data-attr': `math-${key}-${index}`,
}))}
onClick={(e) => e.stopPropagation()}
Expand Down
13 changes: 9 additions & 4 deletions frontend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1836,14 +1836,19 @@ export interface DatedAnnotationType extends Omit<AnnotationType, 'date_marker'>

export enum ChartDisplayType {
ActionsLineGraph = 'ActionsLineGraph',
ActionsLineGraphCumulative = 'ActionsLineGraphCumulative',
ActionsBar = 'ActionsBar',
ActionsAreaGraph = 'ActionsAreaGraph',
ActionsTable = 'ActionsTable',
ActionsLineGraphCumulative = 'ActionsLineGraphCumulative',
BoldNumber = 'BoldNumber',
ActionsPie = 'ActionsPie',
ActionsBar = 'ActionsBar',
ActionsBarValue = 'ActionsBarValue',
ActionsTable = 'ActionsTable',
WorldMap = 'WorldMap',
BoldNumber = 'BoldNumber',
}
export enum ChartDisplayCategory {
Copy link
Member Author

@Twixes Twixes Mar 21, 2024

Choose a reason for hiding this comment

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

Added ChartDisplayCategory as an abstraction over the specific display types

I also wanted to use it in the backend, the way we reuse ChartDisplayType thanks to schema.{ts,json,py}, but unfortunately ts-json-schema-generator doesn't support re-exporting, so we can't do that (unless we start using ChartDisplayCategory in a type defined in schema.ts itself). I put out a fix for the library (super interesting opportunity to learn more about TS syntax internals!), but in the meantime it's fine for this to be frontend-only

Copy link
Member Author

Choose a reason for hiding this comment

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

ChartDisplayCategory would specifically be super useful for caching

TimeSeries = 'TimeSeries',
CumulativeTimeSeries = 'CumulativeTimeSeries',
TotalValue = 'TotalValue',
}

export type BreakdownType = 'cohort' | 'person' | 'event' | 'group' | 'session' | 'hogql' | 'data_warehouse'
Expand Down
8 changes: 4 additions & 4 deletions posthog/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,14 @@ class ChartAxis(BaseModel):

class ChartDisplayType(str, Enum):
ActionsLineGraph = "ActionsLineGraph"
ActionsLineGraphCumulative = "ActionsLineGraphCumulative"
ActionsBar = "ActionsBar"
ActionsAreaGraph = "ActionsAreaGraph"
ActionsTable = "ActionsTable"
ActionsLineGraphCumulative = "ActionsLineGraphCumulative"
BoldNumber = "BoldNumber"
ActionsPie = "ActionsPie"
ActionsBar = "ActionsBar"
ActionsBarValue = "ActionsBarValue"
ActionsTable = "ActionsTable"
WorldMap = "WorldMap"
BoldNumber = "BoldNumber"


class CohortPropertyFilter(BaseModel):
Expand Down
Loading