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

fix(insights): polishing of multiple breakdowns in Trends #23649

Merged
merged 30 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
1eb129f
fix: reset a breakdown filter when BoldNumber is selected
skoob13 Jul 10, 2024
75bb13c
fix: reset map view in multiple breakdowns
skoob13 Jul 10, 2024
c8f7151
fix: InsightCard series letter for multiple breakdowns
skoob13 Jul 10, 2024
f43affc
fix: hide smoothing with breakdowns
skoob13 Jul 10, 2024
620d737
fix: show info popup for sessions
skoob13 Jul 10, 2024
36360f9
feat: serialize multiple breakdowns in activity
skoob13 Jul 11, 2024
8ff6bf0
Update UI snapshots for `chromium` (2)
github-actions[bot] Jul 11, 2024
a665772
fix: multiple breakdowns formatting
skoob13 Jul 11, 2024
13b3146
Merge branch 'fix/multiple-breakdowns-polishing' of github.com:PostHo…
skoob13 Jul 11, 2024
526e3db
Update UI snapshots for `chromium` (2)
github-actions[bot] Jul 11, 2024
40c44b9
fix: pr comments
skoob13 Jul 12, 2024
3ac0fcc
Merge branch 'fix/multiple-breakdowns-polishing' of github.com:PostHo…
skoob13 Jul 12, 2024
7a6dc15
Merge branch 'master' of github.com:PostHog/posthog into fix/multiple…
skoob13 Jul 23, 2024
8c22f56
fix: compatibility with legacy breakdowns
skoob13 Jul 23, 2024
905aca5
fix: descriptive comment
skoob13 Jul 23, 2024
27b96ac
test: fix
skoob13 Jul 23, 2024
261abc4
Update UI snapshots for `chromium` (1)
github-actions[bot] Jul 23, 2024
159d100
Update UI snapshots for `chromium` (1)
github-actions[bot] Jul 23, 2024
2142366
Update UI snapshots for `chromium` (1)
github-actions[bot] Jul 23, 2024
cae9f78
Update UI snapshots for `chromium` (1)
github-actions[bot] Jul 23, 2024
10cd49c
Update UI snapshots for `chromium` (1)
github-actions[bot] Jul 23, 2024
3dfa316
Merge branch 'master' of github.com:PostHog/posthog into fix/multiple…
skoob13 Jul 23, 2024
65cd895
Update UI snapshots for `chromium` (1)
github-actions[bot] Jul 23, 2024
95a4b5d
Update UI snapshots for `chromium` (2)
github-actions[bot] Jul 23, 2024
8201cea
Update UI snapshots for `chromium` (1)
github-actions[bot] Jul 23, 2024
3a90078
Update UI snapshots for `chromium` (1)
github-actions[bot] Jul 23, 2024
190ba5c
Merge branch 'master' of github.com:PostHog/posthog into fix/multiple…
skoob13 Jul 24, 2024
00f2712
Update UI snapshots for `chromium` (1)
github-actions[bot] Jul 24, 2024
30d8819
Merge branch 'master' of github.com:PostHog/posthog into fix/multiple…
skoob13 Jul 24, 2024
b75188f
Merge branch 'fix/multiple-breakdowns-polishing' of github.com:PostHo…
skoob13 Jul 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
thmsobrmlr marked this conversation as resolved.
Show resolved Hide resolved
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -84,81 +84,105 @@ describe('the activity log logic', () => {
})

it('can handle change of insight query', async () => {
const logic = await insightTestSetup('test insight', 'updated', [
{
type: ActivityScope.INSIGHT,
action: 'changed',
field: 'query',
after: {
kind: 'TrendsQuery',
properties: {
type: 'AND',
values: [
{
type: 'OR',
values: [
{
type: 'event',
key: '$current_url',
operator: 'exact',
value: ['https://hedgebox.net/files/'],
},
{
type: 'event',
key: '$geoip_country_code',
operator: 'exact',
value: ['US', 'AU'],
},
],
},
],
},
filterTestAccounts: false,
interval: 'day',
dateRange: {
date_from: '-7d',
},
series: [
const insightMock = {
type: ActivityScope.INSIGHT,
action: 'changed',
field: 'query',
after: {
kind: 'TrendsQuery',
properties: {
type: 'AND',
values: [
{
kind: 'EventsNode',
name: '$pageview',
custom_name: 'Views',
event: '$pageview',
properties: [
type: 'OR',
values: [
{
type: 'event',
key: '$browser',
key: '$current_url',
operator: 'exact',
value: 'Chrome',
value: ['https://hedgebox.net/files/'],
},
{
type: 'cohort',
key: 'id',
value: 2,
type: 'event',
key: '$geoip_country_code',
operator: 'exact',
value: ['US', 'AU'],
},
],
limit: 100,
},
],
trendsFilter: {
display: 'ActionsAreaGraph',
},
breakdownFilter: {
breakdown: '$geoip_country_code',
breakdown_type: 'event',
},
filterTestAccounts: false,
interval: 'day',
dateRange: {
date_from: '-7d',
},
series: [
{
kind: 'EventsNode',
name: '$pageview',
custom_name: 'Views',
event: '$pageview',
properties: [
{
type: 'event',
key: '$browser',
operator: 'exact',
value: 'Chrome',
},
{
type: 'cohort',
key: 'id',
value: 2,
},
],
limit: 100,
},
],
trendsFilter: {
display: 'ActionsAreaGraph',
},
breakdownFilter: {
breakdown: '$geoip_country_code',
breakdown_type: 'event',
},
},
])
const actual = logic.values.humanizedActivity
}

const renderedDescription = render(<>{actual[0].description}</>).container
let logic = await insightTestSetup('test insight', 'updated', [insightMock as any])
let actual = logic.values.humanizedActivity

let renderedDescription = render(<>{actual[0].description}</>).container
expect(renderedDescription).toHaveTextContent('peter changed query definition on test insight')

const renderedExtendedDescription = render(<>{actual[0].extendedDescription}</>).container
let renderedExtendedDescription = render(<>{actual[0].extendedDescription}</>).container
expect(renderedExtendedDescription).toHaveTextContent(
"Query summaryAShowing \"Views\"Pageviewcounted by total countwhere event'sBrowser= equals Chromeand person belongs to cohortID 2FiltersEvent'sCurrent URL= equals https://hedgebox.net/files/or event'sCountry Code= equals US or AUBreakdown byCountry Code"
)

insightMock.after.breakdownFilter = {
breakdowns: [
{
value: '$geoip_country_code',
type: 'event',
},
{
value: '$session_duration',
type: 'session',
},
],
} as any

logic = await insightTestSetup('test insight', 'updated', [insightMock as any])
actual = logic.values.humanizedActivity

renderedDescription = render(<>{actual[0].description}</>).container
expect(renderedDescription).toHaveTextContent('peter changed query definition on test insight')

renderedExtendedDescription = render(<>{actual[0].extendedDescription}</>).container
expect(renderedExtendedDescription).toHaveTextContent(
"Query summaryAShowing \"Views\"Pageviewcounted by total countwhere event'sBrowser= equals Chromeand person belongs to cohortID 2FiltersEvent'sCurrent URL= equals https://hedgebox.net/files/or event'sCountry Code= equals US or AUBreakdown byCountry CodeSession duration"
)
})

it('can handle change of filters on a retention graph', async () => {
Expand Down
30 changes: 14 additions & 16 deletions frontend/src/lib/components/Cards/InsightCard/InsightDetails.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
isLifecycleQuery,
isPathsQuery,
isTrendsQuery,
isValidBreakdown,
} from '~/queries/utils'
import {
AnyPropertyFilter,
Expand Down Expand Up @@ -155,11 +156,7 @@ function SeriesDisplay({
const { mathDefinitions } = useValues(mathsLogic)
const filter = query.series[seriesIndex]

const hasBreakdown =
isInsightQueryWithBreakdown(query) &&
query.breakdownFilter != null &&
query.breakdownFilter.breakdown_type != null &&
query.breakdownFilter.breakdown != null
const hasBreakdown = isInsightQueryWithBreakdown(query) && isValidBreakdown(query.breakdownFilter)

const mathDefinition = mathDefinitions[
isLifecycleQuery(query)
Expand Down Expand Up @@ -338,25 +335,26 @@ export function LEGACY_FilterBasedBreakdownSummary({ filters }: { filters: Parti
}

export function BreakdownSummary({ query }: { query: InsightQueryNode }): JSX.Element | null {
if (
!isInsightQueryWithBreakdown(query) ||
!query.breakdownFilter ||
query.breakdownFilter.breakdown_type == null ||
query.breakdownFilter.breakdown == null
) {
if (!isInsightQueryWithBreakdown(query) || !isValidBreakdown(query.breakdownFilter)) {
return null
}

const { breakdown_type, breakdown } = query.breakdownFilter
const breakdownArray = Array.isArray(breakdown) ? breakdown : [breakdown]
const { breakdown_type, breakdown, breakdowns } = query.breakdownFilter

return (
<>
<h5>Breakdown by</h5>
<section className="InsightDetails__breakdown">
{breakdownArray.map((b) => (
<BreakdownTag key={b} breakdown={b} breakdownType={breakdown_type} />
))}
{Array.isArray(breakdowns)
? breakdowns.map((b) => (
skoob13 marked this conversation as resolved.
Show resolved Hide resolved
<BreakdownTag key={b.type + b.value} breakdown={b.value} breakdownType={b.type} />
))
: breakdown &&
(Array.isArray(breakdown)
? breakdown
: [breakdown].map((b) => (
<BreakdownTag key={b} breakdown={b} breakdownType={breakdown_type} />
)))}
</section>
</>
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { PathStepPicker } from 'scenes/insights/views/Paths/PathStepPicker'
import { trendsDataLogic } from 'scenes/trends/trendsDataLogic'
import { useDebouncedCallback } from 'use-debounce'

import { isValidBreakdown } from '~/queries/utils'
import { ChartDisplayType } from '~/types'

export function InsightDisplayConfig(): JSX.Element {
Expand Down Expand Up @@ -62,7 +63,7 @@ export function InsightDisplayConfig(): JSX.Element {
isLifecycle ||
((isTrends || isStickiness) && !(display && NON_TIME_SERIES_DISPLAY_TYPES.includes(display)))
const showSmoothing =
isTrends && !breakdownFilter?.breakdown_type && (!display || display === ChartDisplayType.ActionsLineGraph)
isTrends && !isValidBreakdown(breakdownFilter) && (!display || display === ChartDisplayType.ActionsLineGraph)

const { showValuesOnSeries, mightContainFractionalNumbers } = useValues(trendsDataLogic(insightProps))

Expand Down
11 changes: 11 additions & 0 deletions frontend/src/queries/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -447,3 +447,14 @@ export function hogql(strings: TemplateStringsArray, ...values: any[]): string {
return strings.reduce((acc, str, i) => acc + str + (i < strings.length - 1 ? formatHogQlValue(values[i]) : ''), '')
}
hogql.identifier = hogQlIdentifier

/**
* Get the breakdown from a query
*/
skoob13 marked this conversation as resolved.
Show resolved Hide resolved
export function isValidBreakdown(breakdownFilter?: BreakdownFilter | null): breakdownFilter is BreakdownFilter {
return !!(
breakdownFilter &&
((breakdownFilter.breakdown && breakdownFilter.breakdown_type) ||
(breakdownFilter.breakdowns && breakdownFilter.breakdowns.length > 0))
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { expectLogic } from 'kea-test-utils'
import { TaxonomicFilterGroup, TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types'

import { initKeaTests } from '~/test/init'
import { InsightLogicProps } from '~/types'
import { ChartDisplayType, InsightLogicProps } from '~/types'

import { taxonomicBreakdownFilterLogic } from './taxonomicBreakdownFilterLogic'

Expand Down Expand Up @@ -131,6 +131,32 @@ describe('taxonomicBreakdownFilterLogic', () => {
breakdown_group_type_index: 0,
})
})

it('resets the map view when adding a next breakdown', async () => {
logic = taxonomicBreakdownFilterLogic({
insightProps,
breakdownFilter: {
breakdown: '$geoip_country_code',
breakdown_type: 'person',
},
isTrends: true,
display: ChartDisplayType.WorldMap,
updateBreakdownFilter,
updateDisplay,
})
logic.mount()
const changedBreakdown = 'c'
const group: TaxonomicFilterGroup = taxonomicGroupFor(TaxonomicFilterGroupType.EventProperties, undefined)

await expectLogic(logic, () => {
logic.actions.addBreakdown(changedBreakdown, group)
}).toFinishListeners()

expect(updateBreakdownFilter).toHaveBeenCalledWith({
breakdown_type: 'event',
breakdown: 'c',
})
})
})

describe('multiple breakdowns', () => {
Expand Down Expand Up @@ -343,5 +369,34 @@ describe('taxonomicBreakdownFilterLogic', () => {

expect(updateBreakdownFilter).not.toHaveBeenCalled()
})

it('resets the map view when adding a next breakdown', async () => {
logic = taxonomicBreakdownFilterLogic({
insightProps,
breakdownFilter: {
breakdowns: [{ value: '$geoip_country_code', type: 'person' }],
},
isTrends: true,
display: ChartDisplayType.WorldMap,
updateBreakdownFilter,
updateDisplay,
})
mockFeatureFlag(logic)
logic.mount()
const changedBreakdown = 'c'
const group: TaxonomicFilterGroup = taxonomicGroupFor(TaxonomicFilterGroupType.EventProperties, undefined)

await expectLogic(logic, () => {
logic.actions.addBreakdown(changedBreakdown, group)
}).toFinishListeners()

expect(updateBreakdownFilter).toHaveBeenCalledWith({
breakdowns: [
{ value: '$geoip_country_code', type: 'person' },
{ value: 'c', type: 'event' },
],
})
expect(updateDisplay).toHaveBeenCalledWith(undefined)
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -234,11 +234,22 @@ export const taxonomicBreakdownFilterLogic = kea<taxonomicBreakdownFilterLogicTy
normalize_url: isNormalizeable ? true : undefined,
}

const updatedBreakdowns = values.breakdownFilter.breakdowns
? [...values.breakdownFilter.breakdowns, newBreakdown]
: [newBreakdown]

props.updateBreakdownFilter({
breakdowns: values.breakdownFilter.breakdowns
? [...values.breakdownFilter.breakdowns, newBreakdown]
: [newBreakdown],
breakdowns: updatedBreakdowns,
})

// Make sure we are no longer in map view after removing the Country Code breakdown
if (
skoob13 marked this conversation as resolved.
Show resolved Hide resolved
props.isTrends &&
props.display === ChartDisplayType.WorldMap &&
(updatedBreakdowns.length !== 1 || updatedBreakdowns[0].value !== 'geoip_country_code')
) {
props.updateDisplay?.(undefined)
}
} else {
props.updateBreakdownFilter({
breakdowns: undefined,
Expand Down
9 changes: 8 additions & 1 deletion frontend/src/scenes/insights/insightVizDataLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,9 @@ export const insightVizDataLogic = kea<insightVizDataLogicType>([
isUsingSessionAnalysis: [
(s) => [s.series, s.breakdownFilter, s.properties],
(series, breakdownFilter, properties) => {
const using_session_breakdown = breakdownFilter?.breakdown_type === 'session'
const using_session_breakdown =
breakdownFilter?.breakdown_type === 'session' ||
breakdownFilter?.breakdowns?.find((breakdown) => breakdown.type === 'session')
const using_session_math = series?.some((entity) => entity.math === 'unique_session')
const using_session_property_math = series?.some((entity) => {
// Should be made more generic is we ever add more session properties
Expand Down Expand Up @@ -598,6 +600,11 @@ const handleQuerySourceUpdateSideEffects = (
mergedUpdate['properties'] = []
}

// Remove breakdown filter if display type is BoldNumber because it is not supported
if (kind === NodeKind.TrendsQuery && maybeChangedDisplay === ChartDisplayType.BoldNumber) {
mergedUpdate['breakdownFilter'] = null
}

// Don't allow minutes on anything other than Trends
if (
currentState.kind == NodeKind.TrendsQuery &&
Expand Down
Loading
Loading