refactor(insights): simplify "save insight"

PostHog · Jul 29, 2024 · 708b959 · 708b959
1 parent 7288fb8
commit 708b959
Show file tree

Hide file tree

Showing 7 changed files with 48 additions and 90 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,7 +40,7 @@ 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',
             cachedInsight: null,
@@ -53,7 +52,6 @@ export function DataWarehouseExternalScene(): JSX.Element {
             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/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
@@ -26,7 +26,6 @@ import {
     PropertyOperator,
 } from '~/types'
 
-import { insightDataLogic } from './insightDataLogic'
 import { createEmptyInsight, insightLogic } from './insightLogic'
 
 const API_FILTERS: Partial<FilterType> = {
@@ -575,11 +574,8 @@ describe('insightLogic', () => {
         })
         logic.mount()
 
-        const dataLogic = insightDataLogic(logic.values.insightProps)
-        dataLogic.mount()
-
         await expectLogic(logic, () => {
-            logic.actions.saveAsConfirmation('New Insight (copy)', dataLogic.values.query)
+            logic.actions.saveAsConfirmation('New Insight (copy)')
         })
             .toDispatchActions(['setInsight'])
             .toDispatchActions(savedInsightsLogic, ['loadInsights'])
@@ -737,24 +733,24 @@ describe('insightLogic', () => {
             jest.spyOn(api, 'create')
 
             await expectLogic(logic, () => {
-                logic.actions.setInsight(
-                    { filters: {}, query: { kind: NodeKind.DataTableNode } as DataTableNode },
-                    { overrideFilter: true }
-                )
-                logic.actions.saveInsight()
+                // logic.actions.setInsight(
+                //     { filters: {}, query: { kind: NodeKind.DataTableNode } as DataTableNode },
+                //     { overrideFilter: true }
+                // )
+                logic.actions.saveInsight({ kind: NodeKind.DataTableNode } as DataTableNode)
             })
 
             const mockCreateCalls = (api.create as jest.Mock).mock.calls
             expect(mockCreateCalls).toEqual([
                 [
                     `api/projects/${MOCK_TEAM_ID}/insights/`,
-                    {
+                    expect.objectContaining({
                         derived_name: 'DataTableNode query',
                         query: {
                             kind: 'DataTableNode',
                         },
                         saved: true,
-                    },
+                    }),
                 ],
             ])
         })

diff --git a/frontend/src/scenes/insights/insightLogic.tsx b/frontend/src/scenes/insights/insightLogic.tsx
@@ -24,9 +24,8 @@ import { dashboardsModel } from '~/models/dashboardsModel'
 import { groupsModel } from '~/models/groupsModel'
 import { insightsModel } from '~/models/insightsModel'
 import { tagsModel } from '~/models/tagsModel'
-import { getInsightFilterOrQueryForPersistance } from '~/queries/nodes/InsightQuery/utils/queryNodeToFilter'
 import { getQueryBasedInsightModel } from '~/queries/nodes/InsightViz/utils'
-import { InsightVizNode, Node } from '~/queries/schema'
+import { Node } from '~/queries/schema'
 import {
     FilterType,
     InsightLogicProps,
@@ -38,9 +37,10 @@ import {
 } from '~/types'
 
 import { teamLogic } from '../teamLogic'
+import { insightDataLogic } from './insightDataLogic'
 import type { insightLogicType } from './insightLogicType'
 import { getInsightId } from './utils'
-import { insightsApi } from './utils/api'
+import { insightsApi, InsightsApiOptions } from './utils/api'
 
 export const UNSAVED_INSIGHT_MIN_REFRESH_INTERVAL_MINUTES = 3
 
@@ -89,10 +89,10 @@ export const insightLogic = kea<insightLogicType>([
             insight,
             options,
         }),
-        saveAs: (query: Node, redirectToViewMode?: boolean) => ({ query, redirectToViewMode }),
-        saveAsConfirmation: (name: string, query: Node, redirectToViewMode?: boolean) => ({
+        saveAs: (redirectToViewMode?: boolean) => ({ redirectToViewMode }),
+        saveAsConfirmation: (name: string, redirectToViewMode?: boolean) => ({
             name,
-            query,
+
             redirectToViewMode,
         }),
         cancelChanges: true,
@@ -294,6 +294,10 @@ export const insightLogic = kea<insightLogicType>([
         ],
     })),
     selectors({
+        query: [
+            (s) => [s.insightProps],
+            (insightProps): Node | null => insightDataLogic.find(insightProps).values.query,
+        ],
         queryBasedInsightSaving: [
             (s) => [s.featureFlags],
             (featureFlags) => !!featureFlags[FEATURE_FLAGS.QUERY_BASED_INSIGHTS_SAVING],
@@ -314,9 +318,9 @@ export const insightLogic = kea<insightLogicType>([
             ({ pathname }) => /^.*\/experiments\/\d+$/.test(pathname),
         ],
         derivedName: [
-            (s) => [s.queryBasedInsight, s.aggregationLabel, s.cohortsById, s.mathDefinitions],
-            (insight, aggregationLabel, cohortsById, mathDefinitions) =>
-                summarizeInsight(insight.query, {
+            (s) => [s.query, s.aggregationLabel, s.cohortsById, s.mathDefinitions],
+            (query, aggregationLabel, cohortsById, mathDefinitions) =>
+                summarizeInsight(query, {
                     aggregationLabel,
                     cohortsById,
                     mathDefinitions,
@@ -344,7 +348,7 @@ export const insightLogic = kea<insightLogicType>([
                 )
             },
         ],
-        showPersonsModal: [() => [(_, p) => p.query], (query?: InsightVizNode) => !query || !query.hidePersonsModal],
+        showPersonsModal: [() => [(s) => s.query], (query) => !query || !query.hidePersonsModal],
     }),
     listeners(({ actions, values }) => ({
         saveInsight: async ({ redirectToViewMode }) => {
@@ -354,32 +358,28 @@ export const insightLogic = kea<insightLogicType>([
             const { name, description, favorited, deleted, dashboards, tags } = values.legacyInsight
 
             let savedInsight: InsightModel
-            const { filters, query } = getInsightFilterOrQueryForPersistance(
-                values.queryBasedInsight,
-                values.queryBasedInsightSaving
-            )
 
             try {
                 // We don't want to send ALL the insight properties back to the API, so only grabbing fields that might have changed
-                const insightRequest: Partial<InsightModel> = {
+                const insightRequest: Partial<QueryBasedInsightModel> = {
                     name,
                     derived_name: values.derivedName,
                     description,
                     favorited,
-                    filters,
-                    query,
+                    query: values.query,
                     deleted,
                     saved: true,
                     dashboards,
                     tags,
                 }
 
+                const options: InsightsApiOptions<false> = {
+                    writeAsQuery: values.queryBasedInsightSaving,
+                    readAsQuery: false,
+                }
                 savedInsight = insightNumericId
-                    ? await api.update(
-                          `api/projects/${teamLogic.values.currentTeamId}/insights/${insightNumericId}`,
-                          insightRequest
-                      )
-                    : await api.create(`api/projects/${teamLogic.values.currentTeamId}/insights/`, insightRequest)
+                    ? await insightsApi.update(insightNumericId, insightRequest, options)
+                    : await insightsApi.create(insightRequest, options)
                 savedInsightsLogic.findMounted()?.actions.loadInsights() // Load insights afresh
                 actions.saveInsightSuccess()
             } catch (e) {
@@ -389,9 +389,9 @@ export const insightLogic = kea<insightLogicType>([
 
             // the backend can't return the result for a query based insight,
             // and so we shouldn't copy the result from `values.insight` as it might be stale
-            const result = savedInsight.result || (query ? values.legacyInsight.result : null)
+            const result = savedInsight.result || (values.query ? values.legacyInsight.result : null)
             actions.setInsight({ ...savedInsight, result: result }, { fromPersistentApi: true, overrideFilter: true })
-            eventUsageLogic.actions.reportInsightSaved(filters || {}, insightNumericId === undefined)
+            eventUsageLogic.actions.reportInsightSaved(values.query, insightNumericId === undefined)
             lemonToast.success(`Insight saved${dashboards?.length === 1 ? ' & added to dashboard' : ''}`, {
                 button: {
                     label: 'View Insights list',
@@ -427,7 +427,7 @@ export const insightLogic = kea<insightLogicType>([
                 router.actions.push(urls.insightEdit(savedInsight.short_id))
             }
         },
-        saveAs: async ({ query, redirectToViewMode }) => {
+        saveAs: async ({ redirectToViewMode }) => {
             LemonDialog.openForm({
                 title: 'Save as new insight',
                 initialValues: {
@@ -444,14 +444,14 @@ export const insightLogic = kea<insightLogicType>([
                 errors: {
                     name: (name) => (!name ? 'You must enter a name' : undefined),
                 },
-                onSubmit: async ({ name }) => actions.saveAsConfirmation(name, query, redirectToViewMode),
+                onSubmit: async ({ name }) => actions.saveAsConfirmation(name, redirectToViewMode),
             })
         },
-        saveAsConfirmation: async ({ name, query, redirectToViewMode }) => {
+        saveAsConfirmation: async ({ name, redirectToViewMode }) => {
             const insight = await insightsApi.create(
                 {
                     name,
-                    query,
+                    query: values.query,
                     saved: true,
                 },
                 {