fix(insights): sql insights are missing display properties (#38491)

frontend/src/queries/nodes/DataVisualization/Components/seriesBreakdownLogic.test.ts

@@ -56,13 +56,15 @@ const initialQuery: DataVisualizationNode = {
 // globalQuery represents the query object that is passed into the data
 // visualization logic and series breakdown logic it is modified by calls to
 // setQuery so we want to ensure this is updated correctly
-let globalQuery = { ...initialQuery }
+let globalQuery: DataVisualizationNode = { ...initialQuery }
 
 const dummyDataVisualizationLogicProps: DataVisualizationLogicProps = {
     key: testUniqueKey,
     query: globalQuery,
-    setQuery: (query) => {
-        globalQuery = query
+    setQuery: (setter) => {
+        globalQuery = setter(globalQuery)
+        dummyDataVisualizationLogicProps.query = globalQuery
+        dataVisualizationLogic.build(dummyDataVisualizationLogicProps)
     },
     editMode: false,
     dataNodeCollectionId: 'new-test-SQL',
@@ -112,14 +114,14 @@ describe('seriesBreakdownLogic', () => {
 
         await expectLogic(logic).toMatchValues({
             showSeriesBreakdown: false,
-            selectedSeriesBreakdownColumn: null,
+            selectedSeriesBreakdownColumn: undefined,
         })
 
         expect(globalQuery).toEqual({
             ...initialQuery,
             chartSettings: {
                 goalLines: undefined,
-                seriesBreakdownColumn: null,
+                seriesBreakdownColumn: undefined,
             },
         })
     })
@@ -136,14 +138,14 @@ describe('seriesBreakdownLogic', () => {
 
         await expectLogic(logic).toMatchValues({
             showSeriesBreakdown: false,
-            selectedSeriesBreakdownColumn: null,
+            selectedSeriesBreakdownColumn: undefined,
         })
 
         expect(globalQuery).toEqual({
             ...initialQuery,
             chartSettings: {
                 goalLines: undefined,
-                seriesBreakdownColumn: null,
+                seriesBreakdownColumn: undefined,
             },
             display: ChartDisplayType.ActionsLineGraph,
         })
@@ -164,25 +166,27 @@ describe('seriesBreakdownLogic', () => {
             chartSettings: {
                 goalLines: undefined,
                 seriesBreakdownColumn: 'test_column',
+                xAxis: undefined,
+                yAxis: [],
             },
         })
     })
 
     it('adds a series breakdown after mount if one already selected in query', async () => {
-        builtDataVizLogic.actions.setQuery({
-            ...initialQuery,
+        builtDataVizLogic.actions.setQuery((query) => ({
+            ...query,
             chartSettings: {
                 goalLines: undefined,
                 seriesBreakdownColumn: 'test_column',
             },
-        })
+        }))
 
         logic = seriesBreakdownLogic({ key: testUniqueKey })
         logic.mount()
 
         await expectLogic(logic).toMatchValues({
-            showSeriesBreakdown: true,
             selectedSeriesBreakdownColumn: 'test_column',
+            showSeriesBreakdown: true,
         })
 
         expect(globalQuery).toEqual({
@@ -203,14 +207,14 @@ describe('seriesBreakdownLogic', () => {
         logic.actions.deleteSeriesBreakdown()
         await expectLogic(logic).toMatchValues({
             showSeriesBreakdown: false,
-            selectedSeriesBreakdownColumn: null,
+            selectedSeriesBreakdownColumn: undefined,
         })
 
         expect(globalQuery).toEqual({
             ...initialQuery,
             chartSettings: {
                 goalLines: undefined,
-                seriesBreakdownColumn: null,
+                seriesBreakdownColumn: undefined,
             },
         })
     })
@@ -224,7 +228,7 @@ describe('seriesBreakdownLogic', () => {
         logic.actions.clearAxis()
         await expectLogic(logic).toMatchValues({
             showSeriesBreakdown: false,
-            selectedSeriesBreakdownColumn: null,
+            selectedSeriesBreakdownColumn: undefined,
         })
 
         expect(globalQuery).toEqual({

frontend/src/queries/nodes/DataVisualization/Components/seriesBreakdownLogic.ts

@@ -1,5 +1,4 @@
-import { actions, afterMount, connect, kea, key, path, props, reducers, selectors } from 'kea'
-import { subscriptions } from 'kea-subscriptions'
+import { actions, afterMount, connect, kea, key, listeners, path, props, selectors } from 'kea'
 
 import { AxisSeries, AxisSeriesSettings, dataVisualizationLogic } from '../dataVisualizationLogic'
 import type { seriesBreakdownLogicType } from './seriesBreakdownLogicType'
@@ -50,38 +49,25 @@ export const seriesBreakdownLogic = kea<seriesBreakdownLogicType>([
     props({ key: '' } as SeriesBreakdownLogicProps),
     connect(() => ({
         actions: [dataVisualizationLogic, ['clearAxis', 'setQuery']],
-        values: [
-            dataVisualizationLogic,
-            ['query', 'response', 'columns', 'selectedXAxis', 'selectedYAxis', 'chartSettings'],
-        ],
+        values: [dataVisualizationLogic, ['query', 'response', 'columns', 'selectedXAxis', 'selectedYAxis']],
     })),
     actions(({ values }) => ({
         addSeriesBreakdown: (columnName: string | null) => ({ columnName, response: values.response }),
         deleteSeriesBreakdown: () => ({}),
     })),
-    reducers(({ values }) => ({
-        showSeriesBreakdown: [
-            false as boolean,
-            {
-                clearAxis: () => false,
-                addSeriesBreakdown: () => true,
-                deleteSeriesBreakdown: () => false,
-            },
-        ],
-        selectedSeriesBreakdownColumn: [
-            values.query?.chartSettings?.seriesBreakdownColumn ??
-                values.chartSettings?.seriesBreakdownColumn ??
-                (null as string | null),
-            {
-                clearAxis: () => null,
-                addSeriesBreakdown: (_, { columnName }) => columnName,
-                deleteSeriesBreakdown: () => null,
-            },
-        ],
-    })),
     selectors({
+        selectedSeriesBreakdownColumn: [
+            (s) => [s.query],
+            (query): string | null | undefined => {
+                return query?.chartSettings?.seriesBreakdownColumn
+            },
+        ],
+        showSeriesBreakdown: [
+            (s) => [s.selectedSeriesBreakdownColumn],
+            (selectedSeriesBreakdownColumn): boolean => selectedSeriesBreakdownColumn !== undefined,
+        ],
         breakdownColumnValues: [
-            (state) => [state.selectedSeriesBreakdownColumn, state.response, state.columns],
+            (s) => [s.selectedSeriesBreakdownColumn, s.response, s.columns],
             (breakdownColumn, response, columns): string[] => {
                 if (!response || breakdownColumn === null) {
                     return []
@@ -99,13 +85,13 @@ export const seriesBreakdownLogic = kea<seriesBreakdownLogicType>([
             },
         ],
         seriesBreakdownData: [
-            (state) => [
-                state.selectedSeriesBreakdownColumn,
-                state.breakdownColumnValues,
-                state.selectedYAxis,
-                state.selectedXAxis,
-                state.response,
-                state.columns,
+            (s) => [
+                s.selectedSeriesBreakdownColumn,
+                s.breakdownColumnValues,
+                s.selectedYAxis,
+                s.selectedXAxis,
+                s.response,
+                s.columns,
             ],
             (
                 selectedBreakdownColumn,
@@ -235,24 +221,40 @@ export const seriesBreakdownLogic = kea<seriesBreakdownLogicType>([
             },
         ],
     }),
-    subscriptions(({ values, actions }) => ({
-        selectedSeriesBreakdownColumn: (value: string | null) => {
-            if (values.query?.chartSettings?.seriesBreakdownColumn !== value) {
-                actions.setQuery({
-                    ...values.query,
+    listeners(({ actions }) => ({
+        addSeriesBreakdown: ({ columnName }) => {
+            actions.setQuery((query) => ({
+                ...query,
+                chartSettings: {
+                    ...query.chartSettings,
+                    seriesBreakdownColumn: columnName,
+                },
+            }))
+        },
+        deleteSeriesBreakdown: () => {
+            actions.setQuery((query) => {
+                return {
+                    ...query,
                     chartSettings: {
-                        ...values.query.chartSettings,
-                        seriesBreakdownColumn: value,
+                        ...query.chartSettings,
+                        seriesBreakdownColumn: undefined,
                     },
-                })
-            }
+                }
+            })
+        },
+        clearAxis: () => {
+            actions.setQuery((query) => ({
+                ...query,
+                chartSettings: {
+                    ...query.chartSettings,
+                    seriesBreakdownColumn: undefined,
+                },
+            }))
         },
     })),
-    afterMount(({ values, actions }) => {
-        if (values.query?.chartSettings?.seriesBreakdownColumn) {
-            actions.addSeriesBreakdown(values.query.chartSettings.seriesBreakdownColumn)
-        } else if (values.chartSettings?.seriesBreakdownColumn) {
-            actions.addSeriesBreakdown(values.chartSettings.seriesBreakdownColumn)
+    afterMount(({ actions, values }) => {
+        if (values.query.chartSettings?.seriesBreakdownColumn === undefined) {
+            actions.deleteSeriesBreakdown()
         }
     }),
 ])

frontend/src/queries/nodes/DataVisualization/DataVisualization.tsx

@@ -89,7 +89,9 @@ export function DataTableVisualization({
         dataNodeCollectionId,
         loadPriority: insightProps.loadPriority,
         editMode,
-        setQuery,
+        setQuery: (setter) => {
+            setQuery(setter(query))
+        },
         cachedResults,
         variablesOverride,
     }

frontend/src/queries/nodes/DataVisualization/dataVisualizationLogic.ts

@@ -67,7 +67,7 @@ export interface DataVisualizationLogicProps {
     query: DataVisualizationNode
     editMode?: boolean
     dataNodeCollectionId: string
-    setQuery?: (node: DataVisualizationNode) => void
+    setQuery?: (setter: (node: DataVisualizationNode) => DataVisualizationNode) => void
     context?: QueryContext<DataVisualizationNode>
     cachedResults?: AnyResponseType
     insightLoading?: boolean
@@ -257,8 +257,8 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
             ['loadData'],
         ],
     })),
-    propsChanged(({ actions, props }, oldProps) => {
-        if (props.query && !objectsEqual(props.query, oldProps.query)) {
+    propsChanged(({ actions, values, props }) => {
+        if (props.query && !objectsEqual(props.query, values.query)) {
             actions._setQuery(props.query)
         }
     }),
@@ -286,7 +286,7 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
         }),
         deleteYSeries: (seriesIndex: number) => ({ seriesIndex }),
         clearAxis: true,
-        setQuery: (node: DataVisualizationNode) => ({ node }),
+        setQuery: (setter: (node: DataVisualizationNode) => DataVisualizationNode) => ({ setter }),
         updateChartSettings: (settings: ChartSettings) => ({ settings }),
         setSideBarTab: (tab: SideBarTab) => ({ tab }),
         toggleChartSettingsPanel: (open?: boolean) => ({ open }),
@@ -306,7 +306,7 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
         query: [
             props.query,
             {
-                setQuery: (_, { node }) => node,
+                setQuery: (state, { setter }) => setter(state),
                 _setQuery: (_, { node }) => node,
             },
         ],
@@ -314,11 +314,21 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
             props.query.display ?? ChartDisplayType.ActionsTable,
             {
                 setVisualizationType: (_, { visualizationType }) => visualizationType,
+                _setQuery: (_, { node }) => node.display ?? ChartDisplayType.ActionsTable,
             },
         ],
         tabularColumnSettings: [
             null as (SelectedYAxis | null)[] | null,
             {
+                _setQuery: (state, { node }) => {
+                    if (node.tableSettings?.columns) {
+                        return node.tableSettings.columns.map((column) => ({
+                            name: column.column,
+                            settings: column.settings ?? DefaultAxisSettings(),
+                        }))
+                    }
+                    return state
+                },
                 clearAxis: () => null,
                 addSeries: (state, { columnName, settings, allColumns }) => {
                     if (!state && columnName !== undefined) {
@@ -382,6 +392,7 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
         selectedXAxis: [
             null as string | null,
             {
+                _setQuery: (_, { node }) => node.chartSettings?.xAxis?.column ?? null,
                 clearAxis: () => null,
                 updateXSeries: (_, { columnName }) => columnName,
             },
@@ -389,6 +400,15 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
         selectedYAxis: [
             null as (SelectedYAxis | null)[] | null,
             {
+                _setQuery: (state, { node }) => {
+                    if (node.chartSettings?.yAxis) {
+                        return node.chartSettings.yAxis.map((axis) => ({
+                            name: axis.column,
+                            settings: axis.settings ?? DefaultAxisSettings(),
+                        }))
+                    }
+                    return state
+                },
                 clearAxis: () => null,
                 addYSeries: (state, { columnName, settings, allNumericalColumns }) => {
                     if (!state && columnName !== undefined) {
@@ -475,11 +495,9 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
         chartSettings: [
             props.query.chartSettings ?? ({} as ChartSettings),
             {
+                _setQuery: (state, { node }) => node.chartSettings ?? state,
                 updateChartSettings: (state, { settings }) => {
-                    return { ...mergeObject(state, settings) }
-                },
-                setQuery: (state, { node }) => {
-                    return { ...mergeObject(state, node.chartSettings ?? {}) }
+                    return { ...state, ...settings }
                 },
             },
         ],
@@ -505,6 +523,12 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
         conditionalFormattingRules: [
             [] as ConditionalFormattingRule[],
             {
+                _setQuery: (state, { node }) => {
+                    if (node.tableSettings?.conditionalFormatting) {
+                        return node.tableSettings.conditionalFormatting
+                    }
+                    return state
+                },
                 addConditionalFormattingRule: (state, { rule, isDarkModeOn }) => {
                     const rules = [...state]
 
@@ -540,6 +564,12 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
         conditionalFormattingRulesPanelActiveKeys: [
             [] as string[],
             {
+                _setQuery: (state, { node }) => {
+                    if (node.tableSettings?.conditionalFormatting) {
+                        return node.tableSettings.conditionalFormatting.map((n) => n.id)
+                    }
+                    return state
+                },
                 addConditionalFormattingRule: (state, { rule: { id } }) => {
                     return [...state, id]
                 },
@@ -603,7 +633,7 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
             },
         ],
         presetChartHeight: [
-            (state, props) => [props.key, state.dashboardId, state.activeSceneId],
+            (s, props) => [props.key, s.dashboardId, s.activeSceneId],
             (key, dashboardId, activeSceneId) => {
                 // Key for SQL editor based visiaulizations
                 const sqlEditorScene = activeSceneId === Scene.SQLEditor
@@ -621,7 +651,7 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
             (cachedResults: AnyResponseType | null): boolean => !!cachedResults,
         ],
         yData: [
-            (state) => [state.selectedYAxis, state.response, state.columns],
+            (s) => [s.selectedYAxis, s.response, s.columns],
             (ySeries, response, columns): AxisSeries<number>[] => {
                 if (!response || ySeries === null || ySeries.length === 0) {
                     return [EmptyYAxisSeries]
@@ -677,7 +707,7 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
             },
         ],
         xData: [
-            (state) => [state.selectedXAxis, state.response, state.columns],
+            (s) => [s.selectedXAxis, s.response, s.columns],
             (xSeries, response, columns): AxisSeries<string> | null => {
                 if (!response || xSeries === null) {
                     return {
@@ -708,7 +738,7 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
             },
         ],
         tabularData: [
-            (state) => [state.tabularColumns, state.response],
+            (s) => [s.tabularColumns, s.response],
             (tabularColumns, response): TableDataCell<any>[][] => {
                 if (!response || tabularColumns === null) {
                     return []
@@ -785,7 +815,7 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
             },
         ],
         tabularColumns: [
-            (state) => [state.tabularColumnSettings, state.response, state.columns],
+            (s) => [s.tabularColumnSettings, s.response, s.columns],
             (tabularColumnSettings, response, columns): AxisSeries<any>[] => {
                 if (!response) {
                     return []
@@ -804,14 +834,14 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
         ],
         dataVisualizationProps: [() => [(_, props) => props], (props): DataVisualizationLogicProps => props],
         isTableVisualization: [
-            (state) => [state.visualizationType],
+            (s) => [s.visualizationType],
             (visualizationType): boolean =>
                 // BoldNumber relies on yAxis formatting so it's considered a table visualization
                 visualizationType === ChartDisplayType.ActionsTable ||
                 visualizationType === ChartDisplayType.BoldNumber,
         ],
         showTableSettings: [
-            (state) => [state.visualizationType],
+            (s) => [s.visualizationType],
             (visualizationType): boolean =>
                 visualizationType === ChartDisplayType.ActionsTable ||
                 visualizationType === ChartDisplayType.BoldNumber,
@@ -819,60 +849,26 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
     }),
     listeners(({ props, actions }) => ({
         updateChartSettings: ({ settings }) => {
-            actions.setQuery({
-                ...props.query,
-                chartSettings: { ...props.query.chartSettings, ...settings },
-            })
+            actions.setQuery((query) => ({
+                ...query,
+                chartSettings: { ...query.chartSettings, ...settings },
+            }))
         },
-        setQuery: ({ node }) => {
+        setQuery: ({ setter }) => {
             if (props.setQuery) {
-                props.setQuery(node)
+                props.setQuery(setter)
             }
         },
         setVisualizationType: ({ visualizationType }) => {
-            actions.setQuery({
-                ...props.query,
+            actions.setQuery((query) => ({
+                ...query,
                 display: visualizationType,
-            })
+            }))
         },
     })),
-    propsChanged(({ props, actions, values }) => {
-        if (props.query.display && values.visualizationType !== props.query.display) {
-            actions.setVisualizationType(props.query.display)
-        }
-
-        if (props.query.chartSettings) {
-            const { xAxis, yAxis } = props.query.chartSettings
-
-            if (xAxis && xAxis.column && values.selectedXAxis !== xAxis.column) {
-                actions.updateXSeries(xAxis.column)
-            }
-
-            if (yAxis && yAxis.length && values.selectedYAxis === null) {
-                yAxis.forEach((axis) => {
-                    actions.addYSeries(axis.column, axis.settings)
-                })
-            }
-        }
-
-        if (props.query.tableSettings && values.tabularColumnSettings === null) {
-            if (props.query.tableSettings.columns) {
-                props.query.tableSettings.columns.forEach((column) => {
-                    actions.addSeries(column.column, column.settings)
-                })
-            }
-        }
-
-        if (props.query.tableSettings?.conditionalFormatting?.length && !values.conditionalFormattingRules.length) {
-            props.query.tableSettings.conditionalFormatting.forEach((rule) => {
-                actions.addConditionalFormattingRule(rule)
-            })
-            actions.setConditionalFormattingRulesPanelActiveKeys([])
-        }
-    }),
-    subscriptions(({ props, actions, values }) => ({
+    subscriptions(({ actions, values }) => ({
         columns: (value: Column[], oldValue: Column[]) => {
-            // If response is cleared, then don't update any internal values
+            // If the response is cleared, then don't update any internal values
             if (!values.response || (!(values.response as any).results && !(values.response as any).result)) {
                 return
             }
@@ -931,14 +927,14 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
                 values.selectedYAxis?.filter((n: SelectedYAxis | null): n is SelectedYAxis => Boolean(n)) ?? []
             const xColumn: ChartAxis | undefined = value !== null ? { column: value } : undefined
 
-            actions.setQuery({
-                ...props.query,
+            actions.setQuery((query) => ({
+                ...query,
                 chartSettings: {
-                    ...props.query.chartSettings,
+                    ...query.chartSettings,
                     yAxis: yColumns.map((n) => ({ column: n.name, settings: n.settings })),
                     xAxis: xColumn,
                 },
-            })
+            }))
         },
         selectedYAxis: (value: (SelectedYAxis | null)[] | null) => {
             if (values.isTableVisualization) {
@@ -949,14 +945,14 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
             const xColumn: ChartAxis | undefined =
                 values.selectedXAxis !== null ? { column: values.selectedXAxis } : undefined
 
-            actions.setQuery({
-                ...props.query,
+            actions.setQuery((query) => ({
+                ...query,
                 chartSettings: {
-                    ...props.query.chartSettings,
+                    ...query.chartSettings,
                     yAxis: yColumns.map((n) => ({ column: n.name, settings: n.settings })),
                     xAxis: xColumn,
                 },
-            })
+            }))
         },
         tabularColumnSettings: (value: (SelectedYAxis | null)[] | null) => {
             if (!values.isTableVisualization) {
@@ -965,24 +961,24 @@ export const dataVisualizationLogic = kea<dataVisualizationLogicType>([
 
             const columns = value?.filter((n: SelectedYAxis | null): n is SelectedYAxis => Boolean(n)) ?? []
 
-            actions.setQuery({
-                ...props.query,
+            actions.setQuery((query) => ({
+                ...query,
                 tableSettings: {
-                    ...props.query.tableSettings,
+                    ...query.tableSettings,
                     columns: columns.map((n) => ({ column: n.name, settings: n.settings })),
                 },
-            })
+            }))
         },
         conditionalFormattingRules: (rules: ConditionalFormattingRule[]) => {
             const saveableRules = rules.filter((n) => n.columnName && n.input && n.templateId && n.bytecode.length)
 
-            actions.setQuery({
-                ...props.query,
+            actions.setQuery((query) => ({
+                ...query,
                 tableSettings: {
-                    ...props.query.tableSettings,
+                    ...query.tableSettings,
                     conditionalFormatting: saveableRules,
                 },
-            })
+            }))
         },
     })),
 ])

frontend/src/queries/nodes/DataVisualization/displayLogic.ts

@@ -80,17 +80,17 @@ export const displayLogic = kea<displayLogicType>([
             actions.setGoalLines(chartSettings.goalLines)
         }
     }),
-    subscriptions(({ values, actions }) => ({
+    subscriptions(({ actions }) => ({
         goalLines: (value: GoalLine[]) => {
             const goalLines = value.length > 0 ? value : undefined
 
-            actions.setQuery({
-                ...values.query,
+            actions.setQuery((query) => ({
+                ...query,
                 chartSettings: {
-                    ...values.query.chartSettings,
+                    ...query.chartSettings,
                     goalLines,
                 },
-            })
+            }))
         },
     })),
 ])

frontend/src/scenes/data-warehouse/editor/EditorScene.tsx

@@ -79,7 +79,7 @@ export function EditorScene({ tabId }: { tabId?: string }): JSX.Element {
         loadPriority: undefined,
         cachedResults: undefined,
         variablesOverride: undefined,
-        setQuery: setSourceQuery,
+        setQuery: (setter) => setSourceQuery(setter(sourceQuery)),
     }
 
     const dataNodeLogicProps: DataNodeLogicProps = {