refactor(insights): simplify "save insight" (#24093)

PostHog · Aug 1, 2024 · d9eb2c5 · d9eb2c5
1 parent 6499bde
commit d9eb2c5
Show file tree

Hide file tree

Showing 9 changed files with 95 additions and 104 deletions.
diff --git a/frontend/src/lib/utils/eventUsageLogic.ts b/frontend/src/lib/utils/eventUsageLogic.ts
@@ -45,7 +45,6 @@ import {
     EntityType,
     Experiment,
     FilterLogicalOperator,
-    FilterType,
     FunnelCorrelation,
     HelpType,
     InsightModel,
@@ -292,7 +291,7 @@ export const eventUsageLogic = kea<eventUsageLogicType>([
         }),
         // insights
         reportInsightCreated: (insightType: InsightType | null) => ({ insightType }),
-        reportInsightSaved: (filters: Partial<FilterType>, isNewInsight: boolean) => ({ filters, isNewInsight }),
+        reportInsightSaved: (query: Node | null, isNewInsight: boolean) => ({ query, isNewInsight }),
         reportInsightViewed: (
             insightModel: Partial<InsightModel>,
             query: Node | null,
@@ -636,10 +635,10 @@ export const eventUsageLogic = kea<eventUsageLogicType>([
             await breakpoint(500) // Debounce to avoid multiple quick "New insight" clicks being reported
             posthog.capture('insight created', { insight: insightType })
         },
-        reportInsightSaved: async ({ filters, isNewInsight }) => {
+        reportInsightSaved: async ({ query, isNewInsight }) => {
             // "insight saved" is a proxy for the new insight's results being valuable to the user
             posthog.capture('insight saved', {
-                ...filters,
+                ...sanitizeQuery(query),
                 is_new_insight: isNewInsight,
             })
         },

diff --git a/frontend/src/scenes/data-warehouse/external/DataWarehouseExternalScene.tsx b/frontend/src/scenes/data-warehouse/external/DataWarehouseExternalScene.tsx
@@ -2,7 +2,6 @@ import { LemonButton, LemonTabs, Link } from '@posthog/lemon-ui'
 import { BindLogic, useActions, useValues } from 'kea'
 import { router } from 'kea-router'
 import { PageHeader } from 'lib/components/PageHeader'
-import { insightDataLogic } from 'scenes/insights/insightDataLogic'
 import { insightLogic } from 'scenes/insights/insightLogic'
 import { insightSceneLogic } from 'scenes/insights/insightSceneLogic'
 import { SceneExport } from 'scenes/sceneTypes'
@@ -41,19 +40,18 @@ export const humanFriendlyDataWarehouseTabName = (tab: DataWarehouseTab): string
 export function DataWarehouseExternalScene(): JSX.Element {
     const { currentTab } = useValues(dataWarehouseSceneLogic)
 
-    const { insightSaving, insightProps } = useValues(
+    const { insightSaving } = useValues(
         insightLogic({
-            dashboardItemId: 'new',
+            dashboardItemId: 'new-dataWarehouse',
             cachedInsight: null,
         })
     )
     const { saveAs } = useActions(
         insightLogic({
-            dashboardItemId: 'new',
+            dashboardItemId: 'new-dataWarehouse',
             cachedInsight: null,
         })
     )
-    const { query } = useValues(insightDataLogic(insightProps))
 
     return (
         <div>
@@ -64,7 +62,7 @@ export function DataWarehouseExternalScene(): JSX.Element {
                             <LemonButton
                                 type="primary"
                                 data-attr="save-exploration"
-                                onClick={() => saveAs(query, true)}
+                                onClick={() => saveAs(true)}
                                 loading={insightSaving}
                             >
                                 Save as insight

diff --git a/frontend/src/scenes/data-warehouse/external/DataWarehouseTables.tsx b/frontend/src/scenes/data-warehouse/external/DataWarehouseTables.tsx
@@ -21,7 +21,7 @@ import { DeleteTableModal, TableData } from './TableData'
 export const DataWarehouseTables = (): JSX.Element => {
     // insightLogic
     const logic = insightLogic({
-        dashboardItemId: 'new',
+        dashboardItemId: 'new-dataWarehouse',
         cachedInsight: null,
     })
     const { insightProps } = useValues(logic)

diff --git a/frontend/src/scenes/insights/InsightPageHeader.tsx b/frontend/src/scenes/insights/InsightPageHeader.tsx
@@ -46,14 +46,14 @@ export function InsightPageHeader({ insightLogicProps }: { insightLogicProps: In
         insightSaving,
         hasDashboardItemId,
     } = useValues(insightLogic(insightLogicProps))
-    const { setInsightMetadata, saveAs } = useActions(insightLogic(insightLogicProps))
+    const { setInsightMetadata, saveAs, saveInsight } = useActions(insightLogic(insightLogicProps))
 
     // savedInsightsLogic
     const { duplicateInsight, loadInsights } = useActions(savedInsightsLogic)
 
     // insightDataLogic
-    const { queryChanged, showQueryEditor, hogQL, exportContext, query } = useValues(insightDataLogic(insightProps))
-    const { saveInsight, toggleQueryEditorPanel } = useActions(insightDataLogic(insightProps))
+    const { queryChanged, showQueryEditor, hogQL, exportContext } = useValues(insightDataLogic(insightProps))
+    const { toggleQueryEditorPanel } = useActions(insightDataLogic(insightProps))
 
     // other logics
     useMountedLogic(insightCommandLogic(insightProps))
@@ -275,7 +275,6 @@ export function InsightPageHeader({ insightLogicProps }: { insightLogicProps: In
                             )
                         ) : (
                             <InsightSaveButton
-                                query={query}
                                 saveAs={saveAs}
                                 saveInsight={saveInsight}
                                 isSaved={hasDashboardItemId}

diff --git a/frontend/src/scenes/insights/InsightSaveButton.tsx b/frontend/src/scenes/insights/InsightSaveButton.tsx
@@ -1,18 +1,14 @@
 import { LemonButton } from 'lib/lemon-ui/LemonButton'
 
-import { Node } from '~/queries/schema'
-
 export function InsightSaveButton({
-    query,
     saveAs,
     saveInsight,
     isSaved,
     insightSaving,
     insightChanged,
     addingToDashboard,
 }: {
-    query: Node
-    saveAs: (query: Node) => void
+    saveAs: () => void
     saveInsight: (redirectToViewMode?: boolean) => void
     isSaved: boolean | undefined
     insightSaving: boolean
@@ -44,11 +40,7 @@ export function InsightSaveButton({
                                 </LemonButton>
                             )}
                             {saveAsAvailable && (
-                                <LemonButton
-                                    onClick={() => saveAs(query)}
-                                    data-attr="insight-save-as-new-insight"
-                                    fullWidth
-                                >
+                                <LemonButton onClick={() => saveAs()} data-attr="insight-save-as-new-insight" fullWidth>
                                     Save as…
                                 </LemonButton>
                             )}

diff --git a/frontend/src/scenes/insights/insightDataLogic.tsx b/frontend/src/scenes/insights/insightDataLogic.tsx
@@ -64,7 +64,7 @@ export const insightDataLogic = kea<insightDataLogicType>([
         ],
         actions: [
             insightLogic,
-            ['setInsight', 'loadInsightSuccess', 'saveInsight as insightLogicSaveInsight'],
+            ['setInsight', 'loadInsightSuccess'],
             dataNodeLogic({ key: insightVizDataNodeKey(props) } as DataNodeLogicProps),
             ['loadData', 'loadDataSuccess', 'loadDataFailure', 'setResponse as setInsightData'],
         ],
@@ -73,7 +73,6 @@ export const insightDataLogic = kea<insightDataLogicType>([
 
     actions({
         setQuery: (query: Node | null) => ({ query }),
-        saveInsight: (redirectToViewMode = true) => ({ redirectToViewMode }),
         toggleQueryEditorPanel: true,
         cancelChanges: true,
     }),
@@ -101,7 +100,7 @@ export const insightDataLogic = kea<insightDataLogicType>([
 
         query: [
             (s) => [s.propsQuery, s.queryBasedInsight, s.internalQuery, s.filterTestAccountsDefault],
-            (propsQuery, insight, internalQuery, filterTestAccountsDefault) =>
+            (propsQuery, insight, internalQuery, filterTestAccountsDefault): Node | null =>
                 propsQuery ||
                 internalQuery ||
                 insight.query ||
@@ -203,31 +202,6 @@ export const insightDataLogic = kea<insightDataLogicType>([
                 actions.setQuery(query)
             }
         },
-        saveInsight: ({ redirectToViewMode }) => {
-            let filters = values.legacyInsight.filters
-            if (isInsightVizNode(values.query)) {
-                const querySource = values.query.source
-                filters = queryNodeToFilter(querySource)
-            } else if (values.isQueryBasedInsight) {
-                filters = {}
-            }
-
-            let query = undefined
-            if (values.isQueryBasedInsight) {
-                query = values.query
-            }
-
-            actions.setInsight(
-                {
-                    ...values.legacyInsight,
-                    filters: filters,
-                    query: query ?? undefined,
-                },
-                { overrideFilter: true, fromPersistentApi: false }
-            )
-
-            actions.insightLogicSaveInsight(redirectToViewMode)
-        },
         cancelChanges: () => {
             const savedFilters = values.savedInsight.filters
             const savedResult = values.savedInsight.result

diff --git a/frontend/src/scenes/insights/insightLogic.test.ts b/frontend/src/scenes/insights/insightLogic.test.ts
@@ -19,6 +19,7 @@ import {
     DashboardTile,
     DashboardType,
     FilterType,
+    InsightLogicProps,
     InsightModel,
     InsightShortId,
     InsightType,
@@ -460,12 +461,16 @@ describe('insightLogic', () => {
     })
 
     test('keeps saved name, description, tags', async () => {
-        logic = insightLogic({
+        const insightProps: InsightLogicProps = {
             dashboardItemId: Insight43,
             cachedInsight: { ...createEmptyInsight(Insight43, false), filters: API_FILTERS },
-        })
+        }
+
+        logic = insightLogic(insightProps)
         logic.mount()
 
+        insightDataLogic(insightProps).mount()
+
         const expectedPartialInsight = {
             name: '',
             description: '',
@@ -502,11 +507,15 @@ describe('insightLogic', () => {
     })
 
     test('saveInsight saves new insight and redirects to view mode', async () => {
-        logic = insightLogic({
+        const insightProps: InsightLogicProps = {
             dashboardItemId: 'new',
-        })
+        }
+
+        logic = insightLogic(insightProps)
         logic.mount()
 
+        insightDataLogic(insightProps).mount()
+
         await expectLogic(logic, () => {
             logic.actions.setFilters(cleanFilters({}))
             logic.actions.saveInsight()
@@ -515,16 +524,21 @@ describe('insightLogic', () => {
 
     test('saveInsight and updateInsight update the saved insights list', async () => {
         savedInsightsLogic.mount()
-        logic = insightLogic({
+
+        const insightProps: InsightLogicProps = {
             dashboardItemId: Insight42,
             cachedInsight: {
                 short_id: Insight42,
                 filters: { insight: InsightType.FUNNELS },
-                results: {},
+                result: {},
             },
-        })
+        }
+
+        logic = insightLogic(insightProps)
         logic.mount()
 
+        insightDataLogic(insightProps).mount()
+
         logic.actions.saveInsight()
         await expectLogic(logic).toDispatchActions([savedInsightsLogic.actionTypes.addInsight])
 
@@ -539,10 +553,14 @@ describe('insightLogic', () => {
 
         savedInsightsLogic.mount()
 
-        logic = insightLogic({
+        const insightProps: InsightLogicProps = {
             dashboardItemId: Insight43,
-        })
+        }
+        logic = insightLogic(insightProps)
         logic.mount()
+
+        insightDataLogic(insightProps).mount()
+
         logic.actions.saveInsight()
 
         await expectLogic(dashLogic).toDispatchActions(['loadDashboard'])
@@ -567,19 +585,20 @@ describe('insightLogic', () => {
         router.actions.push(url)
         savedInsightsLogic.mount()
 
-        logic = insightLogic({
+        const insightProps: InsightLogicProps = {
             dashboardItemId: Insight42,
             cachedInsight: {
                 filters: { insight: InsightType.FUNNELS },
             },
-        })
+        }
+
+        logic = insightLogic(insightProps)
         logic.mount()
 
-        const dataLogic = insightDataLogic(logic.values.insightProps)
-        dataLogic.mount()
+        insightDataLogic(insightProps).mount()
 
         await expectLogic(logic, () => {
-            logic.actions.saveAsConfirmation('New Insight (copy)', dataLogic.values.query)
+            logic.actions.saveAsConfirmation('New Insight (copy)')
         })
             .toDispatchActions(['setInsight'])
             .toDispatchActions(savedInsightsLogic, ['loadInsights'])
@@ -727,10 +746,11 @@ describe('insightLogic', () => {
 
     describe('saving query based insights', () => {
         beforeEach(async () => {
-            logic = insightLogic({
-                dashboardItemId: 'new',
-            })
+            const insightProps: InsightLogicProps = { dashboardItemId: 'new' }
+            logic = insightLogic(insightProps)
             logic.mount()
+
+            insightDataLogic(insightProps).mount()
         })
 
         it('sends query when saving', async () => {
@@ -747,14 +767,23 @@ describe('insightLogic', () => {
             const mockCreateCalls = (api.create as jest.Mock).mock.calls
             expect(mockCreateCalls).toEqual([
                 [
-                    `api/projects/${MOCK_TEAM_ID}/insights/`,
-                    {
+                    `api/projects/${MOCK_TEAM_ID}/insights`,
+                    expect.objectContaining({
                         derived_name: 'DataTableNode query',
                         query: {
                             kind: 'DataTableNode',
                         },
                         saved: true,
-                    },
+                    }),
+                    expect.objectContaining({
+                        data: {
+                            derived_name: 'DataTableNode query',
+                            query: {
+                                kind: 'DataTableNode',
+                            },
+                            saved: true,
+                        },
+                    }),
                 ],
             ])
         })