From 93f47ade2923b0d7f1c097cb836749a71ac9aaac Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Tue, 26 Mar 2024 12:28:50 +0100 Subject: [PATCH] feat(insights): Warn about WAU/MAU in total value trends (#21067) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> --- cypress/e2e/trends.cy.ts | 34 +++++++++----- frontend/src/lib/constants.tsx | 26 ++++++---- .../lib/lemon-ui/LemonSelect/LemonSelect.tsx | 1 + frontend/src/queries/schema.json | 10 ++-- frontend/src/queries/schema.ts | 3 ++ .../filters/ActionFilter/ActionFilter.tsx | 14 +++++- .../ActionFilterRow/ActionFilterRow.tsx | 47 ++++++++++++++----- frontend/src/types.ts | 13 +++-- posthog/schema.py | 8 ++-- 9 files changed, 109 insertions(+), 47 deletions(-) diff --git a/cypress/e2e/trends.cy.ts b/cypress/e2e/trends.cy.ts index a1aa9d31a5594..36809958d7c25 100644 --- 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,14 +88,14 @@ 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) => { @@ -103,7 +103,7 @@ describe('Trends', () => { 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 index c2d40d03957fe..292fcb5d957b7 100644 --- 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.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 index 49e6c6c59190c..8e06a932310ab 100644 --- 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({ } : null } + tooltip={activeLeaf?.tooltip} {...buttonProps} > diff --git a/frontend/src/queries/schema.json b/frontend/src/queries/schema.json index 400ef8d1774e3..bb6e2256a43c8 100644 --- 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 index 9bd04dd3c62a9..fc45ff6ecadcb 100644 --- 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 index b27d271f53d24..8ebb237640060 100644 --- 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( 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 index bca5a483baf48..0cb3eaeb086b3 100644 --- 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) => 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([BaseMathType.WeeklyActiveUsers, BaseMathType.MonthlyActiveUsers]) + function useMathSelectorOptions({ math, index, mathAvailability, onMathSelect, + trendsDisplayCategory, }: MathSelectorProps): LemonSelectOptions { 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 ? : undefined, + label: definition.name, + tooltip: !shouldWarnAboutTrailingMath ? ( + definition.description + ) : ( + <> +

{definition.description}

+ + 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. + + + ), + '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 index 131598f9a79d2..58aea7f43fdab 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -1838,14 +1838,19 @@ export interface DatedAnnotationType extends Omit 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 index 9d83587351683..7068c39090a2f 100644 --- 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):