From 708b95935f9a29bac412a1a86789a7f11c7acfb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Obermu=CC=88ller?= Date: Mon, 29 Jul 2024 13:11:47 +0200 Subject: [PATCH] refactor(insights): simplify "save insight" --- frontend/src/lib/utils/eventUsageLogic.ts | 7 +-- .../external/DataWarehouseExternalScene.tsx | 6 +- .../src/scenes/insights/InsightPageHeader.tsx | 7 +-- .../src/scenes/insights/InsightSaveButton.tsx | 12 +--- .../src/scenes/insights/insightDataLogic.tsx | 30 +--------- .../src/scenes/insights/insightLogic.test.ts | 20 +++---- frontend/src/scenes/insights/insightLogic.tsx | 56 +++++++++---------- 7 files changed, 48 insertions(+), 90 deletions(-) diff --git a/frontend/src/lib/utils/eventUsageLogic.ts b/frontend/src/lib/utils/eventUsageLogic.ts index d84c8d04222b0c..8aef510145114b 100644 --- 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([ }), // insights reportInsightCreated: (insightType: InsightType | null) => ({ insightType }), - reportInsightSaved: (filters: Partial, isNewInsight: boolean) => ({ filters, isNewInsight }), + reportInsightSaved: (query: Node | null, isNewInsight: boolean) => ({ query, isNewInsight }), reportInsightViewed: ( insightModel: Partial, query: Node | null, @@ -636,10 +635,10 @@ export const eventUsageLogic = kea([ 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 index 05386e9877552b..d150b0205b7352 100644 --- 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 (
@@ -64,7 +62,7 @@ export function DataWarehouseExternalScene(): JSX.Element { 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 index 10203705b096a1..e0cf982908dbed 100644 --- 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 ) ) : ( void + saveAs: () => void saveInsight: (redirectToViewMode?: boolean) => void isSaved: boolean | undefined insightSaving: boolean @@ -44,11 +40,7 @@ export function InsightSaveButton({ )} {saveAsAvailable && ( - saveAs(query)} - data-attr="insight-save-as-new-insight" - fullWidth - > + saveAs()} data-attr="insight-save-as-new-insight" fullWidth> Save as… )} diff --git a/frontend/src/scenes/insights/insightDataLogic.tsx b/frontend/src/scenes/insights/insightDataLogic.tsx index 64170fc92eed13..3cdf1caa825909 100644 --- a/frontend/src/scenes/insights/insightDataLogic.tsx +++ b/frontend/src/scenes/insights/insightDataLogic.tsx @@ -64,7 +64,7 @@ export const insightDataLogic = kea([ ], 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([ actions({ setQuery: (query: Node | null) => ({ query }), - saveInsight: (redirectToViewMode = true) => ({ redirectToViewMode }), toggleQueryEditorPanel: true, cancelChanges: true, }), @@ -101,7 +100,7 @@ export const insightDataLogic = kea([ 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([ 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 index 9a993110cd2bf6..17301966c68316 100644 --- 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 = { @@ -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 index 00e98686e19390..6e73404b10fa9e 100644 --- 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([ 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([ ], })), 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([ ({ 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([ ) }, ], - 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([ 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 = { + const insightRequest: Partial = { name, derived_name: values.derivedName, description, favorited, - filters, - query, + query: values.query, deleted, saved: true, dashboards, tags, } + const options: InsightsApiOptions = { + 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([ // 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([ 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([ 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, }, {