feat(insights): Warn about WAU/MAU in total value trends (#21067)

* feat(insights): Warn about WAU/MAU in total value trends * Update trends.cy.ts * Reword warning * Reword warning more * Update frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx Co-authored-by: Thomas Obermüller <[email protected]> * Update UI snapshots for `chromium` (2) * Update query snapshots * Update UI snapshots for `chromium` (2) * Update UI snapshots for `chromium` (2) * Update query snapshots * Update UI snapshots for `chromium` (2) --------- Co-authored-by: Thomas Obermüller <[email protected]> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
PostHog · Mar 26, 2024 · 93f47ad · 93f47ad
1 parent fad9f5e
commit 93f47ad
Show file tree

Hide file tree

Showing 9 changed files with 109 additions and 47 deletions.
diff --git a/cypress/e2e/trends.cy.ts b/cypress/e2e/trends.cy.ts
@@ -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')
@@ -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) => {
@@ -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')
     })
@@ -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')
 
@@ -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')
     })
 
@@ -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 recommend using "Unique users" here instead').should('exist')
+    })
 })
diff --git a/frontend/src/lib/constants.tsx b/frontend/src/lib/constants.tsx
@@ -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. */

diff --git a/frontend/src/lib/lemon-ui/LemonSelect/LemonSelect.tsx b/frontend/src/lib/lemon-ui/LemonSelect/LemonSelect.tsx
@@ -154,6 +154,7 @@ export function LemonSelect<T extends string | number | boolean | null>({
                           }
                         : null
                 }
+                tooltip={activeLeaf?.tooltip}
                 {...buttonProps}
             >
                 <span className="flex flex-1">

diff --git a/frontend/src/queries/schema.json b/frontend/src/queries/schema.json
@@ -501,14 +501,14 @@
         "ChartDisplayType": {
             "enum": [
                 "ActionsLineGraph",
-                "ActionsLineGraphCumulative",
+                "ActionsBar",
                 "ActionsAreaGraph",
-                "ActionsTable",
+                "ActionsLineGraphCumulative",
+                "BoldNumber",
                 "ActionsPie",
-                "ActionsBar",
                 "ActionsBarValue",
-                "WorldMap",
-                "BoldNumber"
+                "ActionsTable",
+                "WorldMap"
             ],
             "type": "string"
         },

diff --git a/frontend/src/queries/schema.ts b/frontend/src/queries/schema.ts
@@ -4,6 +4,7 @@ import {
     Breakdown,
     BreakdownKeyType,
     BreakdownType,
+    ChartDisplayCategory,
     ChartDisplayType,
     CountPerActorMathType,
     EventPropertyFilter,
@@ -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

diff --git a/frontend/src/scenes/insights/filters/ActionFilter/ActionFilter.tsx b/frontend/src/scenes/insights/filters/ActionFilter/ActionFilter.tsx
@@ -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'
@@ -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,

diff --git a/frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx b/frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx
@@ -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'
@@ -39,6 +39,7 @@ import {
     ActionFilter,
     ActionFilter as ActionFilterType,
     BaseMathType,
+    ChartDisplayCategory,
     CountPerActorMathType,
     EntityType,
     EntityTypes,
@@ -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({
@@ -142,6 +144,7 @@ export function ActionFilterRow({
     disabled = false,
     readOnly = false,
     renderRow,
+    trendsDisplayCategory,
 }: ActionFilterRowProps): JSX.Element {
     const { entityFilterVisible } = useValues(logic)
     const {
@@ -377,6 +380,7 @@ export function ActionFilterRow({
                                         disabled={readOnly}
                                         style={{ maxWidth: '100%', width: 'initial' }}
                                         mathAvailability={mathAvailability}
+                                        trendsDisplayCategory={trendsDisplayCategory}
                                     />
                                     {mathDefinitions[math || BaseMathType.TotalCount]?.category ===
                                         MathCategory.PropertyValue && (
@@ -514,6 +518,7 @@ interface MathSelectorProps {
     disabled?: boolean
     disabledReason?: string
     onMathSelect: (index: number, value: any) => any
+    trendsDisplayCategory: ChartDisplayCategory | null
     style?: React.CSSProperties
 }
 
@@ -525,11 +530,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
@@ -550,19 +558,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>
+                            In total value insights, it's usually not clear what date range "{definition.name}" refers
+                            to. For full clarity, we recommend using "Unique users" here 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, {
@@ -580,7 +602,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()}

diff --git a/frontend/src/types.ts b/frontend/src/types.ts
@@ -1838,14 +1838,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 {
+    TimeSeries = 'TimeSeries',
+    CumulativeTimeSeries = 'CumulativeTimeSeries',
+    TotalValue = 'TotalValue',
 }
 
 export type BreakdownType = 'cohort' | 'person' | 'event' | 'group' | 'session' | 'hogql' | 'data_warehouse'

diff --git a/posthog/schema.py b/posthog/schema.py
@@ -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):
-Original file line number
+Diff line change
@@ Expand Up @@
                               }
                             : null
                     }
+                    tooltip={activeLeaf?.tooltip}
                     {...buttonProps}
                 >
                     <span className="flex flex-1">
@@ Expand Down @@