refactor(insight): simplify insightVizDataLogic update actions (#17972)

PostHog · Oct 16, 2023 · 7f5b273 · 7f5b273
1 parent 208451b
commit 7f5b273
Show file tree

Hide file tree

Showing 3 changed files with 229 additions and 71 deletions.
diff --git 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
@@ -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<typeof insightVizDataLogic.build>
-    let theInsightDataLogic: ReturnType<typeof insightDataLogic.build>
-    let theFeatureFlagLogic: ReturnType<typeof featureFlagLogic.build>
+    let builtInsightVizDataLogic: ReturnType<typeof insightVizDataLogic.build>
+    let builtInsightDataLogic: ReturnType<typeof insightDataLogic.build>
+    let builtFeatureFlagLogic: ReturnType<typeof featureFlagLogic.build>
 
     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
@@ -95,6 +95,11 @@ export const insightVizDataLogic = kea<insightVizDataLogicType>([
             (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<insightVizDataLogicType>([
 
     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<TrendsQuery>)
+        },
+        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)) {