From 7f5b27335a9985c24a2786b45757f4f77374ade6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Oberm=C3=BCller?= Date: Mon, 16 Oct 2023 14:57:34 +0200 Subject: [PATCH] refactor(insight): simplify insightVizDataLogic update actions (#17972) --- .../queries/nodes/InsightQuery/defaults.ts | 4 +- .../insights/insightVizDataLogic.test.ts | 239 +++++++++++++++--- .../scenes/insights/insightVizDataLogic.ts | 57 ++--- 3 files changed, 229 insertions(+), 71 deletions(-) diff --git a/frontend/src/queries/nodes/InsightQuery/defaults.ts b/frontend/src/queries/nodes/InsightQuery/defaults.ts index 8aced3682c278..69e199a55f29e 100644 --- a/frontend/src/queries/nodes/InsightQuery/defaults.ts +++ b/frontend/src/queries/nodes/InsightQuery/defaults.ts @@ -11,7 +11,7 @@ import { } from '~/queries/schema' import { BaseMathType, FunnelVizType, InsightType, PathType, RetentionPeriod } from '~/types' -const trendsQueryDefault: TrendsQuery = { +export const trendsQueryDefault: TrendsQuery = { kind: NodeKind.TrendsQuery, series: [ { @@ -24,7 +24,7 @@ const trendsQueryDefault: TrendsQuery = { trendsFilter: {}, } -const funnelsQueryDefault: FunnelsQuery = { +export const funnelsQueryDefault: FunnelsQuery = { kind: NodeKind.FunnelsQuery, series: [ { diff --git a/frontend/src/scenes/insights/insightVizDataLogic.test.ts b/frontend/src/scenes/insights/insightVizDataLogic.test.ts index fb7a947115b91..55e98f7dec9cb 100644 --- a/frontend/src/scenes/insights/insightVizDataLogic.test.ts +++ b/frontend/src/scenes/insights/insightVizDataLogic.test.ts @@ -7,13 +7,16 @@ import { insightDataLogic } from './insightDataLogic' import { useMocks } from '~/mocks/jest' import { insightVizDataLogic } from 'scenes/insights/insightVizDataLogic' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' +import { trendsQueryDefault, funnelsQueryDefault } from '~/queries/nodes/InsightQuery/defaults' +import { NodeKind } from '~/queries/schema' +import { FunnelLayout } from 'lib/constants' const Insight123 = '123' as InsightShortId describe('insightVizDataLogic', () => { - let theInsightVizDataLogic: ReturnType - let theInsightDataLogic: ReturnType - let theFeatureFlagLogic: ReturnType + let builtInsightVizDataLogic: ReturnType + let builtInsightDataLogic: ReturnType + let builtFeatureFlagLogic: ReturnType beforeEach(() => { useMocks({ @@ -23,49 +26,227 @@ describe('insightVizDataLogic', () => { }) initKeaTests() - theFeatureFlagLogic = featureFlagLogic() - theFeatureFlagLogic.mount() + builtFeatureFlagLogic = featureFlagLogic() + builtFeatureFlagLogic.mount() const props = { dashboardItemId: Insight123 } - theInsightVizDataLogic = insightVizDataLogic(props) - theInsightDataLogic = insightDataLogic(props) + builtInsightVizDataLogic = insightVizDataLogic(props) + builtInsightDataLogic = insightDataLogic(props) - theInsightDataLogic.mount() - theInsightVizDataLogic.mount() + builtInsightDataLogic.mount() + builtInsightVizDataLogic.mount() }) - describe('manages query source state', () => { - it('updateQuerySource updates the query source', () => { - expectLogic(theInsightDataLogic, () => { - theInsightVizDataLogic.actions.updateQuerySource({ filterTestAccounts: true }) + describe('updateQuerySource', () => { + it('updates the query source', () => { + // with default query + expectLogic(builtInsightDataLogic, () => { + builtInsightVizDataLogic.actions.updateQuerySource({ filterTestAccounts: true }) }).toMatchValues({ - query: expect.objectContaining({ - source: expect.objectContaining({ + query: { + kind: NodeKind.InsightVizNode, + source: { + ...trendsQueryDefault, filterTestAccounts: true, - }), - }), + }, + }, }) - expect(theInsightVizDataLogic.values.querySource).toMatchObject({ filterTestAccounts: true }) + expect(builtInsightVizDataLogic.values.querySource).toMatchObject({ filterTestAccounts: true }) + + // merges with existing changes + expectLogic(builtInsightDataLogic, () => { + builtInsightVizDataLogic.actions.updateQuerySource({ samplingFactor: 0.1 }) + }).toMatchValues({ + query: { + kind: NodeKind.InsightVizNode, + source: { + ...trendsQueryDefault, + filterTestAccounts: true, + samplingFactor: 0.1, + }, + }, + }) + + expect(builtInsightVizDataLogic.values.querySource).toEqual({ + ...trendsQueryDefault, + filterTestAccounts: true, + samplingFactor: 0.1, + }) + }) + }) + + describe('updateDateRange', () => { + it('updates the date range', () => { + // when dateRange is empty + expectLogic(builtInsightDataLogic, () => { + builtInsightVizDataLogic.actions.updateDateRange({ + date_from: '-7d', + date_to: null, + }) + }).toMatchValues({ + query: { + kind: NodeKind.InsightVizNode, + source: { + ...trendsQueryDefault, + dateRange: { + date_from: '-7d', + date_to: null, + }, + }, + }, + }) + + expect(builtInsightVizDataLogic.values.dateRange).toEqual({ + date_from: '-7d', + date_to: null, + }) + + // merges with existing dateRange + expectLogic(builtInsightDataLogic, () => { + builtInsightVizDataLogic.actions.updateDateRange({ + date_to: '-3d', + }) + }).toMatchValues({ + query: { + kind: NodeKind.InsightVizNode, + source: { + ...trendsQueryDefault, + dateRange: { + date_from: '-7d', + date_to: '-3d', + }, + }, + }, + }) + + expect(builtInsightVizDataLogic.values.dateRange).toEqual({ + date_from: '-7d', + date_to: '-3d', + }) }) }) - describe('manages insight filter state', () => { - it('updateInsightFilter updates the insight filter', () => { - expectLogic(theInsightDataLogic, () => { - theInsightVizDataLogic.actions.updateInsightFilter({ display: ChartDisplayType.ActionsAreaGraph }) + describe('updateBreakdown', () => { + it('updates the breakdown', () => { + // when breakdown is empty + expectLogic(builtInsightDataLogic, () => { + builtInsightVizDataLogic.actions.updateBreakdown({ + breakdown_type: 'event', + breakdown: '$current_url', + }) }).toMatchValues({ - query: expect.objectContaining({ - source: expect.objectContaining({ - trendsFilter: expect.objectContaining({ + query: { + kind: NodeKind.InsightVizNode, + source: { + ...trendsQueryDefault, + breakdown: { + breakdown_type: 'event', + breakdown: '$current_url', + }, + }, + }, + }) + + expect(builtInsightVizDataLogic.values.breakdown).toEqual({ + breakdown_type: 'event', + breakdown: '$current_url', + }) + + // merges with existing breakdown + expectLogic(builtInsightDataLogic, () => { + builtInsightVizDataLogic.actions.updateBreakdown({ + breakdown: '$browser', + }) + }).toMatchValues({ + query: { + kind: NodeKind.InsightVizNode, + source: { + ...trendsQueryDefault, + breakdown: { + breakdown_type: 'event', + breakdown: '$browser', + }, + }, + }, + }) + + expect(builtInsightVizDataLogic.values.breakdown).toEqual({ + breakdown_type: 'event', + breakdown: '$browser', + }) + }) + }) + + describe('updateInsightFilter', () => { + it('updates the insight filter', () => { + // when insight filter is empty + expectLogic(builtInsightDataLogic, () => { + builtInsightVizDataLogic.actions.updateInsightFilter({ display: ChartDisplayType.ActionsAreaGraph }) + }).toMatchValues({ + query: { + kind: NodeKind.InsightVizNode, + source: { + ...trendsQueryDefault, + trendsFilter: { display: 'ActionsAreaGraph', - }), - }), - }), + }, + }, + }, }) - expect(theInsightVizDataLogic.values.insightFilter).toMatchObject({ display: 'ActionsAreaGraph' }) + expect(builtInsightVizDataLogic.values.insightFilter).toEqual({ display: 'ActionsAreaGraph' }) + + // merges with existing insight filter + expectLogic(builtInsightDataLogic, () => { + builtInsightVizDataLogic.actions.updateInsightFilter({ + show_values_on_series: true, + }) + }).toMatchValues({ + query: { + kind: NodeKind.InsightVizNode, + source: { + ...trendsQueryDefault, + trendsFilter: { + display: 'ActionsAreaGraph', + show_values_on_series: true, + }, + }, + }, + }) + + expect(builtInsightVizDataLogic.values.insightFilter).toEqual({ + display: 'ActionsAreaGraph', + show_values_on_series: true, + }) + }) + + it('updates the insight filter for other insight query kinds', () => { + builtInsightVizDataLogic.actions.updateQuerySource(funnelsQueryDefault) + + expectLogic(builtInsightDataLogic, () => { + builtInsightVizDataLogic.actions.updateInsightFilter({ + layout: FunnelLayout.horizontal, + }) + }).toMatchValues({ + query: { + kind: NodeKind.InsightVizNode, + source: { + ...funnelsQueryDefault, + funnelsFilter: { + ...funnelsQueryDefault.funnelsFilter, + layout: FunnelLayout.horizontal, + }, + trendsFilter: {}, // we currently don't remove insight filters of previous query kinds + }, + }, + }) + + expect(builtInsightVizDataLogic.values.insightFilter).toMatchObject({ + ...funnelsQueryDefault.funnelsFilter, + layout: FunnelLayout.horizontal, + }) }) }) }) diff --git a/frontend/src/scenes/insights/insightVizDataLogic.ts b/frontend/src/scenes/insights/insightVizDataLogic.ts index 9d451f8d12f4d..df725a1896e78 100644 --- a/frontend/src/scenes/insights/insightVizDataLogic.ts +++ b/frontend/src/scenes/insights/insightVizDataLogic.ts @@ -95,6 +95,11 @@ export const insightVizDataLogic = kea([ (s) => [s.query], (query) => (isNodeWithSource(query) && isInsightQueryNode(query.source) ? query.source : null), ], + localQuerySource: [ + (s) => [s.querySource, s.filterTestAccountsDefault], + (querySource, filterTestAccountsDefault) => + querySource ? querySource : queryFromKind(NodeKind.TrendsQuery, filterTestAccountsDefault).source, + ], isTrends: [(s) => [s.querySource], (q) => isTrendsQuery(q)], isFunnels: [(s) => [s.querySource], (q) => isFunnelsQuery(q)], @@ -191,53 +196,25 @@ export const insightVizDataLogic = kea([ listeners(({ actions, values, props }) => ({ updateDateRange: ({ dateRange }) => { - const localQuerySource = values.querySource - ? values.querySource - : queryFromKind(NodeKind.TrendsQuery, values.filterTestAccountsDefault).source - if (isInsightQueryNode(localQuerySource)) { - const newQuerySource = { ...localQuerySource, dateRange } - actions.updateQuerySource(newQuerySource) - } + actions.updateQuerySource({ dateRange: { ...values.dateRange, ...dateRange } }) }, updateBreakdown: ({ breakdown }) => { - const localQuerySource = values.querySource - ? values.querySource - : queryFromKind(NodeKind.TrendsQuery, values.filterTestAccountsDefault).source - if (isInsightQueryNode(localQuerySource)) { - const newQuerySource = { - ...localQuerySource, - breakdown: { ...(localQuerySource as TrendsQuery).breakdown, ...breakdown }, - } - actions.updateQuerySource(newQuerySource) - } + actions.updateQuerySource({ breakdown: { ...values.breakdown, ...breakdown } } as Partial) + }, + updateInsightFilter: ({ insightFilter }) => { + const filterProperty = filterPropertyForQuery(values.localQuerySource) + actions.updateQuerySource({ + [filterProperty]: { ...values.localQuerySource[filterProperty], ...insightFilter }, + }) }, updateDisplay: ({ display }) => { actions.updateInsightFilter({ display }) }, - updateInsightFilter: ({ insightFilter }) => { - const localQuerySource = values.querySource - ? values.querySource - : queryFromKind(NodeKind.TrendsQuery, values.filterTestAccountsDefault).source - if (isInsightQueryNode(localQuerySource)) { - const filterProperty = filterPropertyForQuery(localQuerySource) - const newQuerySource = { ...localQuerySource } - newQuerySource[filterProperty] = { - ...localQuerySource[filterProperty], - ...insightFilter, - } - actions.updateQuerySource(newQuerySource) - } - }, updateQuerySource: ({ querySource }) => { - const localQuery = values.query - ? values.query - : queryFromKind(NodeKind.TrendsQuery, values.filterTestAccountsDefault) - if (localQuery && isInsightVizNode(localQuery)) { - actions.setQuery({ - ...localQuery, - source: { ...(localQuery as InsightVizNode).source, ...querySource }, - } as Node) - } + actions.setQuery({ + ...values.query, + source: { ...values.querySource, ...querySource }, + } as Node) }, setQuery: ({ query }) => { if (isInsightVizNode(query)) {