Skip to content

Commit

Permalink
feat(trends): added persons modal to all trends graph types (#19758)
Browse files Browse the repository at this point in the history
* Added persons modal to all trends graph types

* Persons modal returns the correct figures for trends

* mypy sync

* Fixed breakdown value limit being a float

* Fixed persons modal showuing the breakdown_other internal value instead of the string other

* Use util method
  • Loading branch information
Gilbert09 authored Jan 23, 2024
1 parent fab7fbe commit a93d86f
Show file tree
Hide file tree
Showing 10 changed files with 145 additions and 24 deletions.
2 changes: 1 addition & 1 deletion frontend/src/scenes/insights/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ export const BREAKDOWN_OTHER_NUMERIC_LABEL = 9007199254740991 // pow(2, 53) - 1
export const BREAKDOWN_NULL_STRING_LABEL = '$$_posthog_breakdown_null_$$'
export const BREAKDOWN_NULL_NUMERIC_LABEL = 9007199254740990 // pow(2, 53) - 2

export function isOtherBreakdown(breakdown_value: string | number | null | undefined): boolean {
export function isOtherBreakdown(breakdown_value: string | number | null | undefined | ReactNode): boolean {
return breakdown_value === BREAKDOWN_OTHER_STRING_LABEL || breakdown_value === BREAKDOWN_OTHER_NUMERIC_LABEL
}

Expand Down
44 changes: 40 additions & 4 deletions frontend/src/scenes/insights/views/BoldNumber/BoldNumber.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import { LemonRow, Link } from '@posthog/lemon-ui'
import clsx from 'clsx'
import { useValues } from 'kea'
import { PropertyKeyInfo } from 'lib/components/PropertyKeyInfo'
import { FEATURE_FLAGS } from 'lib/constants'
import { IconFlare, IconTrendingDown, IconTrendingFlat, IconTrendingUp } from 'lib/lemon-ui/icons'
import { featureFlagLogic } from 'lib/logic/featureFlagLogic'
import { percentage } from 'lib/utils'
import { useLayoutEffect, useRef, useState } from 'react'
import { useEffect } from 'react'
Expand All @@ -16,6 +18,8 @@ import { insightVizDataLogic } from 'scenes/insights/insightVizDataLogic'
import { openPersonsModal } from 'scenes/trends/persons-modal/PersonsModal'

import { groupsModel } from '~/models/groupsModel'
import { NodeKind } from '~/queries/schema'
import { isInsightVizNode, isTrendsQuery } from '~/queries/utils'
import { ChartParams, TrendResult } from '~/types'

import { insightLogic } from '../../insightLogic'
Expand Down Expand Up @@ -82,14 +86,22 @@ function useBoldNumberTooltip({

export function BoldNumber({ showPersonsModal = true }: ChartParams): JSX.Element {
const { insightProps } = useValues(insightLogic)
const { insightData, trendsFilter } = useValues(insightVizDataLogic(insightProps))
const { insightData, trendsFilter, isTrends, query } = useValues(insightVizDataLogic(insightProps))
const { featureFlags } = useValues(featureFlagLogic)

const [isTooltipShown, setIsTooltipShown] = useState(false)
const valueRef = useBoldNumberTooltip({ showPersonsModal, isTooltipShown })

const showComparison = !!trendsFilter?.compare && insightData?.result?.length > 1
const resultSeries = insightData?.result?.[0] as TrendResult | undefined

const isTrendsQueryWithFeatureFlagOn =
featureFlags[FEATURE_FLAGS.HOGQL_INSIGHTS_TRENDS] &&
isTrends &&
query &&
isInsightVizNode(query) &&
isTrendsQuery(query.source)

return resultSeries ? (
<div className="BoldNumber">
<div
Expand All @@ -98,7 +110,15 @@ export function BoldNumber({ showPersonsModal = true }: ChartParams): JSX.Elemen
// != is intentional to catch undefined too
showPersonsModal && resultSeries.aggregated_value != null
? () => {
if (resultSeries.persons?.url) {
if (isTrendsQueryWithFeatureFlagOn) {
openPersonsModal({
title: resultSeries.label,
query: {
kind: NodeKind.InsightActorsQuery,
source: query.source,
},
})
} else if (resultSeries.persons?.url) {
openPersonsModal({
url: resultSeries.persons?.url,
title: <PropertyKeyInfo value={resultSeries.label} disablePopover />,
Expand All @@ -124,7 +144,8 @@ export function BoldNumber({ showPersonsModal = true }: ChartParams): JSX.Elemen

function BoldNumberComparison({ showPersonsModal }: Pick<ChartParams, 'showPersonsModal'>): JSX.Element | null {
const { insightProps } = useValues(insightLogic)
const { insightData } = useValues(insightVizDataLogic(insightProps))
const { insightData, isTrends, query } = useValues(insightVizDataLogic(insightProps))
const { featureFlags } = useValues(featureFlagLogic)

if (!insightData?.result) {
return null
Expand All @@ -149,6 +170,13 @@ function BoldNumberComparison({ showPersonsModal }: Pick<ChartParams, 'showPerso
? `Down ${percentage(-percentageDiff)} from`
: 'No change from'

const isTrendsQueryWithFeatureFlagOn =
featureFlags[FEATURE_FLAGS.HOGQL_INSIGHTS_TRENDS] &&
isTrends &&
query &&
isInsightVizNode(query) &&
isTrendsQuery(query.source)

return (
<LemonRow
icon={
Expand All @@ -175,7 +203,15 @@ function BoldNumberComparison({ showPersonsModal }: Pick<ChartParams, 'showPerso
) : (
<Link
onClick={() => {
if (previousPeriodSeries.persons?.url) {
if (isTrendsQueryWithFeatureFlagOn) {
openPersonsModal({
title: previousPeriodSeries.label,
query: {
kind: NodeKind.InsightActorsQuery,
source: query.source,
},
})
} else if (previousPeriodSeries.persons?.url) {
openPersonsModal({
url: previousPeriodSeries.persons?.url,
title: <PropertyKeyInfo value={previousPeriodSeries.label} disablePopover />,
Expand Down
17 changes: 15 additions & 2 deletions frontend/src/scenes/trends/persons-modal/PersonsModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ import { ProfilePicture } from 'lib/lemon-ui/ProfilePicture'
import { Spinner } from 'lib/lemon-ui/Spinner/Spinner'
import { Tooltip } from 'lib/lemon-ui/Tooltip'
import { capitalizeFirstLetter, isGroupType, midEllipsis, pluralize } from 'lib/utils'
import { useState } from 'react'
import { useCallback, useState } from 'react'
import { createRoot } from 'react-dom/client'
import { isOtherBreakdown } from 'scenes/insights/utils'
import { GroupActorDisplay, groupDisplayId } from 'scenes/persons/GroupActorDisplay'
import { asDisplay } from 'scenes/persons/person-utils'
import { PersonDisplay } from 'scenes/persons/PersonDisplay'
Expand Down Expand Up @@ -85,6 +86,18 @@ export function PersonsModal({

const totalActorsCount = missingActorsCount + actors.length

const getTitle = useCallback(() => {
if (typeof title === 'function') {
return title(capitalizeFirstLetter(actorLabel.plural))
}

if (isOtherBreakdown(title)) {
return 'Other'
}

return title
}, [title, actorLabel.plural])

return (
<>
<LemonModal
Expand All @@ -97,7 +110,7 @@ export function PersonsModal({
inline={inline}
>
<LemonModal.Header>
<h3>{typeof title === 'function' ? title(capitalizeFirstLetter(actorLabel.plural)) : title}</h3>
<h3>{getTitle()}</h3>
</LemonModal.Header>
<div className="px-6 py-2">
{actorsResponse && !!missingActorsCount && (
Expand Down
24 changes: 23 additions & 1 deletion frontend/src/scenes/trends/viz/ActionsHorizontalBar.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
import { useValues } from 'kea'
import { getSeriesColor } from 'lib/colors'
import { PropertyKeyInfo } from 'lib/components/PropertyKeyInfo'
import { FEATURE_FLAGS } from 'lib/constants'
import { featureFlagLogic } from 'lib/logic/featureFlagLogic'
import { useEffect, useState } from 'react'
import { insightLogic } from 'scenes/insights/insightLogic'
import { insightVizDataLogic } from 'scenes/insights/insightVizDataLogic'
import { formatBreakdownLabel, isNullBreakdown, isOtherBreakdown } from 'scenes/insights/utils'

import { cohortsModel } from '~/models/cohortsModel'
import { propertyDefinitionsModel } from '~/models/propertyDefinitionsModel'
import { NodeKind } from '~/queries/schema'
import { isInsightVizNode, isTrendsQuery } from '~/queries/utils'
import { ChartParams, GraphType } from '~/types'

import { InsightEmptyState } from '../../insights/EmptyStates'
Expand All @@ -25,6 +30,8 @@ export function ActionsHorizontalBar({ showPersonsModal = true }: ChartParams):
const { formatPropertyValueForDisplay } = useValues(propertyDefinitionsModel)

const { insightProps, hiddenLegendKeys } = useValues(insightLogic)
const { featureFlags } = useValues(featureFlagLogic)
const { isTrends, query } = useValues(insightVizDataLogic(insightProps))
const { indexedResults, labelGroupType, trendsFilter, formula, showValueOnSeries } = useValues(
trendsDataLogic(insightProps)
)
Expand Down Expand Up @@ -69,6 +76,13 @@ export function ActionsHorizontalBar({ showPersonsModal = true }: ChartParams):
}
}, [indexedResults])

const isTrendsQueryWithFeatureFlagOn =
featureFlags[FEATURE_FLAGS.HOGQL_INSIGHTS_TRENDS] &&
isTrends &&
query &&
isInsightVizNode(query) &&
isTrendsQuery(query.source)

return data && total > 0 ? (
<LineGraph
data-attr="trend-bar-value-graph"
Expand All @@ -95,7 +109,15 @@ export function ActionsHorizontalBar({ showPersonsModal = true }: ChartParams):
const urls = urlsForDatasets(crossDataset, index, cohorts, formatPropertyValueForDisplay)
const selectedUrl = urls[index]?.value

if (selectedUrl) {
if (isTrendsQueryWithFeatureFlagOn) {
openPersonsModal({
title: label || '',
query: {
kind: NodeKind.InsightActorsQuery,
source: query.source,
},
})
} else if (selectedUrl) {
openPersonsModal({
urlsIndex: index,
urls,
Expand Down
25 changes: 24 additions & 1 deletion frontend/src/scenes/trends/viz/ActionsPie.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,19 @@ import { useValues } from 'kea'
import { getSeriesColor } from 'lib/colors'
import { InsightLegend } from 'lib/components/InsightLegend/InsightLegend'
import { PropertyKeyInfo } from 'lib/components/PropertyKeyInfo'
import { FEATURE_FLAGS } from 'lib/constants'
import { featureFlagLogic } from 'lib/logic/featureFlagLogic'
import { useEffect, useState } from 'react'
import { formatAggregationAxisValue } from 'scenes/insights/aggregationAxisFormat'
import { insightLogic } from 'scenes/insights/insightLogic'
import { insightVizDataLogic } from 'scenes/insights/insightVizDataLogic'
import { formatBreakdownLabel } from 'scenes/insights/utils'
import { PieChart } from 'scenes/insights/views/LineGraph/PieChart'

import { cohortsModel } from '~/models/cohortsModel'
import { propertyDefinitionsModel } from '~/models/propertyDefinitionsModel'
import { NodeKind } from '~/queries/schema'
import { isInsightVizNode, isTrendsQuery } from '~/queries/utils'
import { ChartDisplayType, ChartParams, GraphDataset, GraphType } from '~/types'

import { urlsForDatasets } from '../persons-modal/persons-modal-utils'
Expand All @@ -29,6 +34,7 @@ export function ActionsPie({

const { cohorts } = useValues(cohortsModel)
const { formatPropertyValueForDisplay } = useValues(propertyDefinitionsModel)
const { featureFlags } = useValues(featureFlagLogic)

const { insightProps, hiddenLegendKeys } = useValues(insightLogic)
const {
Expand All @@ -43,10 +49,19 @@ export function ActionsPie({
pieChartVizOptions,
} = useValues(trendsDataLogic(insightProps))

const { isTrends, query } = useValues(insightVizDataLogic(insightProps))

const renderingMetadata = context?.chartRenderingMetadata?.[ChartDisplayType.ActionsPie]

const showAggregation = !pieChartVizOptions?.hideAggregation

const isTrendsQueryWithFeatureFlagOn =
featureFlags[FEATURE_FLAGS.HOGQL_INSIGHTS_TRENDS] &&
isTrends &&
query &&
isInsightVizNode(query) &&
isTrendsQuery(query.source)

function updateData(): void {
const _data = [...indexedResults].sort((a, b) => b.aggregated_value - a.aggregated_value)
const days = _data.length > 0 ? _data[0].days : []
Expand Down Expand Up @@ -95,7 +110,15 @@ export function ActionsPie({
const urls = urlsForDatasets(crossDataset, index, cohorts, formatPropertyValueForDisplay)
const selectedUrl = urls[index]?.value

if (selectedUrl) {
if (isTrendsQueryWithFeatureFlagOn) {
openPersonsModal({
title: label || '',
query: {
kind: NodeKind.InsightActorsQuery,
source: query.source,
},
})
} else if (selectedUrl) {
openPersonsModal({
urls,
urlsIndex: index,
Expand Down
5 changes: 5 additions & 0 deletions mypy-baseline.txt
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,11 @@ posthog/hogql/metadata.py:0: error: Argument "metadata_source" to "translate_hog
posthog/hogql/metadata.py:0: error: Incompatible types in assignment (expression has type "Expr", variable has type "SelectQuery | SelectUnionQuery") [assignment]
posthog/queries/breakdown_props.py:0: error: Argument 1 to "translate_hogql" has incompatible type "str | int"; expected "str" [arg-type]
posthog/hogql_queries/insights/trends/query_builder.py:0: error: Incompatible types in assignment (expression has type "SelectUnionQuery", variable has type "SelectQuery") [assignment]
posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "extend" [union-attr]
posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr]
posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr]
posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr]
posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr]
posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr]
posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr]
posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def to_query(self) -> ast.SelectQuery | ast.SelectUnionQuery:
return paths_runner.to_actors_query()
elif isinstance(self.source_runner, TrendsQueryRunner):
trends_runner = cast(TrendsQueryRunner, self.source_runner)
return trends_runner.to_actors_query()
return trends_runner.to_actors_query(self.query.day)
elif isinstance(self.source_runner, RetentionQueryRunner):
retention_runner = cast(RetentionQueryRunner, self.source_runner)
return retention_runner.to_actors_query(interval=self.query.interval)
Expand Down
2 changes: 1 addition & 1 deletion posthog/hogql_queries/insights/trends/breakdown_values.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def get_breakdown_values(self) -> List[str | int]:
if self.chart_display_type == ChartDisplayType.WorldMap:
breakdown_limit = BREAKDOWN_VALUES_LIMIT_FOR_COUNTRIES
else:
breakdown_limit = self.breakdown_limit or BREAKDOWN_VALUES_LIMIT
breakdown_limit = int(self.breakdown_limit) if self.breakdown_limit is not None else BREAKDOWN_VALUES_LIMIT

inner_events_query = parse_select(
"""
Expand Down
34 changes: 24 additions & 10 deletions posthog/hogql_queries/insights/trends/query_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ class TrendsQueryBuilder:
series: EventsNode | ActionsNode
timings: HogQLTimings
modifiers: HogQLQueryModifiers
person_query_time_frame: Optional[str | int]
is_person_query: bool

def __init__(
self,
Expand All @@ -32,15 +34,21 @@ def __init__(
series: EventsNode | ActionsNode,
timings: HogQLTimings,
modifiers: HogQLQueryModifiers,
person_query_time_frame: Optional[str | int] = None,
):
self.query = trends_query
self.team = team
self.query_date_range = query_date_range
self.series = series
self.timings = timings
self.modifiers = modifiers
self.person_query_time_frame = person_query_time_frame
self.is_person_query = person_query_time_frame is not None

def build_query(self) -> ast.SelectQuery | ast.SelectUnionQuery:
if self.is_person_query:
return self._get_events_subquery(True)

if self._trends_display.should_aggregate_values():
events_query = self._get_events_subquery(False)
else:
Expand All @@ -54,15 +62,6 @@ def build_query(self) -> ast.SelectQuery | ast.SelectUnionQuery:

return full_query

def build_persons_query(self) -> ast.SelectQuery:
event_query = self._get_events_subquery(True)

event_query.select = [ast.Alias(alias="person_id", expr=ast.Field(chain=["e", "person", "id"]))]
event_query.distinct = True
event_query.group_by = None

return event_query

def _get_date_subqueries(self, ignore_breakdowns: bool = False) -> List[ast.SelectQuery]:
if not self._breakdown.enabled or ignore_breakdowns:
return [
Expand Down Expand Up @@ -166,6 +165,12 @@ def _get_events_subquery(self, no_modifications: Optional[bool]) -> ast.SelectQu
default_query.select.append(day_start)
default_query.group_by.append(ast.Field(chain=["day_start"]))

# TODO: Move this logic into the below branches when working on adding breakdown support for the person modal
if self.is_person_query:
default_query.select = [ast.Alias(alias="person_id", expr=ast.Field(chain=["e", "person", "id"]))]
default_query.distinct = True
default_query.group_by = None

# No breakdowns and no complex series aggregation
if (
not self._breakdown.enabled
Expand Down Expand Up @@ -356,7 +361,16 @@ def _events_filter(self, ignore_breakdowns: bool = False) -> ast.Expr:
filters: List[ast.Expr] = []

# Dates
if not self._aggregation_operation.requires_query_orchestration():
if self.is_person_query:
to_start_of_time_frame = f"toStartOf{self.query_date_range.interval_name.capitalize()}"
filters.append(
ast.CompareOperation(
left=ast.Call(name=to_start_of_time_frame, args=[ast.Field(chain=["timestamp"])]),
op=ast.CompareOperationOp.Eq,
right=ast.Call(name="toDateTime", args=[ast.Constant(value=self.person_query_time_frame)]),
)
)
elif not self._aggregation_operation.requires_query_orchestration():
filters.extend(
[
parse_expr(
Expand Down
Loading

0 comments on commit a93d86f

Please sign in to comment.