diff --git a/frontend/src/lib/utils/eventUsageLogic.ts b/frontend/src/lib/utils/eventUsageLogic.ts index d84c8d04222b0..8aef510145114 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 05386e9877552..990cd28f313a6 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,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 (
@@ -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/data-warehouse/external/DataWarehouseTables.tsx b/frontend/src/scenes/data-warehouse/external/DataWarehouseTables.tsx index 3a55b86ebd00a..0c41c02b7dc6d 100644 --- 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 index 10203705b096a..e0cf982908dbe 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 64170fc92eed1..3cdf1caa82590 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 9a993110cd2bf..921af221adbb2 100644 --- 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, + }, + }), ], ]) }) diff --git a/frontend/src/scenes/insights/insightLogic.tsx b/frontend/src/scenes/insights/insightLogic.tsx index 00e98686e1939..73016f93038ba 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.findMounted(insightProps)?.values.query || null, + ], 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, }, { diff --git a/frontend/src/scenes/insights/insightSceneLogic.tsx b/frontend/src/scenes/insights/insightSceneLogic.tsx index 8ecaf9f4de564..e6620042648e1 100644 --- a/frontend/src/scenes/insights/insightSceneLogic.tsx +++ b/frontend/src/scenes/insights/insightSceneLogic.tsx @@ -190,11 +190,11 @@ export const insightSceneLogic = kea([ })), urlToAction(({ actions, values }) => ({ '/data-warehouse/*': (_, __, { q }) => { - actions.setSceneState(String('new') as InsightShortId, ItemMode.Edit, undefined) + actions.setSceneState(String('new-dataWarehouse') as InsightShortId, ItemMode.Edit, undefined) values.insightDataLogicRef?.logic.actions.setQuery(examples.DataWarehouse) values.insightLogicRef?.logic.actions.setInsight( { - ...createEmptyInsight('new', false), + ...createEmptyInsight('new-dataWarehouse', false), ...(q ? { query: JSON.parse(q) } : {}), }, { @@ -204,7 +204,7 @@ export const insightSceneLogic = kea([ ) }, '/data-warehouse/view/:id': (_, __, { q }) => { - actions.setSceneState(String('new') as InsightShortId, ItemMode.Edit, undefined) + actions.setSceneState(String('new-dataWarehouse') as InsightShortId, ItemMode.Edit, undefined) values.insightDataLogicRef?.logic.actions.setQuery({ kind: NodeKind.DataVisualizationNode, source: JSON.parse(q),