Repository: PostHog/posthog
Pull request: Dashboard item: Only pass object id when deleting dashboard item
## Changes

*Please describe.*  
- Dashboard item deletes were passing an entire payload back to api which caused stringifying issues if the dashboard item was a path based viz. Example: can't delete the paths item from [this dashboard](https://app.posthog.com/dashboard/16413)
- This reduces the parameters passed and only passes the id that's needed for the deletion
*If this affects the frontend, include screenshots.*  

## How did you test this code?

*Please describe.*


Problem statement:
Dashboard Items: Path dashboard items can't be deleted
## Bug description

*Please describe.*  
If you add a path viz to a dashboard and try deleting, it won't delete

## Expected behavior
Item should be deletable


## How to reproduce

1. Add a path viz to dashboards
2. Try deleting

## Environment

- [ ] PostHog Cloud
- [ ] self-hosted PostHog (ClickHouse-based), version/commit: _please provide_
- [ ] self-hosted PostHog (Postgres-based, legacy), version/commit: _please provide_

## Additional context



#### *Thank you* for your bug report – we love squashing them!

Hints from issue discussion:

Commit message: add tests that validate loadResults exists on insight logics for dashboard item refresh

Patch to review:
diff --git a/frontend/src/scenes/dashboard/DashboardItem.tsx b/frontend/src/scenes/dashboard/DashboardItem.tsx
index e87081ab16fa1..ac8ff4cfafad3 100644
--- a/frontend/src/scenes/dashboard/DashboardItem.tsx
+++ b/frontend/src/scenes/dashboard/DashboardItem.tsx
@@ -565,7 +565,9 @@ export function DashboardItem({
                                                     data-attr={'dashboard-item-' + index + '-dropdown-delete'}
                                                     onClick={() =>
                                                         deleteWithUndo({
-                                                            object: item,
+                                                            object: {
+                                                                id: item.id,
+                                                            },
                                                             endpoint: 'insight',
                                                             callback: loadDashboardItems,
                                                         })
diff --git a/frontend/src/scenes/funnels/funnelLogic.test.ts b/frontend/src/scenes/funnels/funnelLogic.test.ts
index bae66f38005f6..2e1d63d477a3a 100644
--- a/frontend/src/scenes/funnels/funnelLogic.test.ts
+++ b/frontend/src/scenes/funnels/funnelLogic.test.ts
@@ -227,4 +227,18 @@ describe('funnelLogic', () => {
                 })
         })
     })
+
+    describe('dashboard item interface', () => {
+        initKeaTestLogic({
+            logic: funnelLogic,
+            props: { dashboardItemId: undefined },
+            onLogic: (l) => (logic = l),
+        })
+
+        it('can load directly', async () => {
+            await expectLogic(logic, () => {
+                logic.actions.loadResults()
+            }).toDispatchActions(['loadResults'])
+        })
+    })
 })
diff --git a/frontend/src/scenes/paths/pathsLogic.test.ts b/frontend/src/scenes/paths/pathsLogic.test.ts
index 1cca3622ca4da..3339594de129d 100644
--- a/frontend/src/scenes/paths/pathsLogic.test.ts
+++ b/frontend/src/scenes/paths/pathsLogic.test.ts
@@ -11,7 +11,9 @@ describe('pathsLogic', () => {
 
     mockAPI(async (url) => {
         const { pathname } = url
-        if (['api/insight/paths/'].includes(pathname)) {
+        if (pathname === 'api/insight') {
+            return { results: [] }
+        } else if (['api/insight/paths/'].includes(pathname)) {
             return { result: ['result from api'] }
         }
         return defaultAPIMocks(url)
@@ -66,4 +68,18 @@ describe('pathsLogic', () => {
                 })
         })
     })
+
+    describe('dashboard item interface', () => {
+        initKeaTestLogic({
+            logic: pathsLogic,
+            props: { dashboardItemId: undefined },
+            onLogic: (l) => (logic = l),
+        })
+
+        it('can load directly', async () => {
+            await expectLogic(logic, () => {
+                logic.actions.loadResults()
+            }).toDispatchActions(['loadResults'])
+        })
+    })
 })
diff --git a/frontend/src/scenes/paths/pathsLogic.ts b/frontend/src/scenes/paths/pathsLogic.ts
index 1f9e9fd58a49c..468ed92801511 100644
--- a/frontend/src/scenes/paths/pathsLogic.ts
+++ b/frontend/src/scenes/paths/pathsLogic.ts
@@ -40,7 +40,7 @@ export const pathsLogic = kea<pathsLogicType<PathNode>>({
 
     connect: (props: InsightLogicProps) => ({
         values: [insightLogic(props), ['filters as filter', 'insight', 'insightLoading']],
-        actions: [insightLogic(props), ['loadResultsSuccess']],
+        actions: [insightLogic(props), ['loadResultsSuccess', 'loadResults']],
     }),
 
     actions: {
diff --git a/frontend/src/scenes/retention/retentionTableLogic.test.ts b/frontend/src/scenes/retention/retentionTableLogic.test.ts
index dfa9635b9fb12..2996b7a8b6fee 100644
--- a/frontend/src/scenes/retention/retentionTableLogic.test.ts
+++ b/frontend/src/scenes/retention/retentionTableLogic.test.ts
@@ -64,4 +64,18 @@ describe('retentionTableLogic', () => {
                 })
         })
     })
+
+    describe('dashboard item interface', () => {
+        initKeaTestLogic({
+            logic: retentionTableLogic,
+            props: { dashboardItemId: undefined },
+            onLogic: (l) => (logic = l),
+        })
+
+        it('can load directly', async () => {
+            await expectLogic(logic, () => {
+                logic.actions.loadResults()
+            }).toDispatchActions(['loadResults'])
+        })
+    })
 })
diff --git a/frontend/src/scenes/retention/retentionTableLogic.ts b/frontend/src/scenes/retention/retentionTableLogic.ts
index a37352ee39745..e1a49463ca8e4 100644
--- a/frontend/src/scenes/retention/retentionTableLogic.ts
+++ b/frontend/src/scenes/retention/retentionTableLogic.ts
@@ -35,7 +35,7 @@ export const retentionTableLogic = kea<retentionTableLogicType>({
     key: keyForInsightLogicProps(DEFAULT_RETENTION_LOGIC_KEY),
     connect: (props: InsightLogicProps) => ({
         values: [insightLogic(props), ['filters', 'insight', 'insightLoading'], actionsModel, ['actions']],
-        actions: [insightLogic(props), ['loadResultsSuccess']],
+        actions: [insightLogic(props), ['loadResultsSuccess', 'loadResults']],
     }),
     actions: () => ({
         setFilters: (filters: Partial<FilterType>) => ({ filters }),
diff --git a/frontend/src/scenes/trends/trendsLogic.test.ts b/frontend/src/scenes/trends/trendsLogic.test.ts
index 6a28b223ba9bd..e1a2878e036d0 100644
--- a/frontend/src/scenes/trends/trendsLogic.test.ts
+++ b/frontend/src/scenes/trends/trendsLogic.test.ts
@@ -110,4 +110,18 @@ describe('trendsLogic', () => {
                 })
         })
     })
+
+    describe('dashboard item interface', () => {
+        initKeaTestLogic({
+            logic: trendsLogic,
+            props: { dashboardItemId: undefined },
+            onLogic: (l) => (logic = l),
+        })
+
+        it('can load directly', async () => {
+            await expectLogic(logic, () => {
+                logic.actions.loadResults()
+            }).toDispatchActions(['loadResults'])
+        })
+    })
 })
diff --git a/frontend/src/scenes/trends/trendsLogic.ts b/frontend/src/scenes/trends/trendsLogic.ts
index b166534d4ec8a..953fb6d7b332e 100644
--- a/frontend/src/scenes/trends/trendsLogic.ts
+++ b/frontend/src/scenes/trends/trendsLogic.ts
@@ -13,7 +13,7 @@ export const trendsLogic = kea<trendsLogicType>({
 
     connect: (props: InsightLogicProps) => ({
         values: [insightLogic(props), ['filters', 'insight', 'insightLoading']],
-        actions: [insightLogic(props), ['loadResultsSuccess']],
+        actions: [insightLogic(props), ['loadResultsSuccess', 'loadResults']],
     }),
 
     actions: () => ({


Code review comments:
diff_hunk: @@ -565,7 +565,9 @@ export function DashboardItem({
                                                     data-attr={'dashboard-item-' + index + '-dropdown-delete'}
                                                     onClick={() =>
                                                         deleteWithUndo({
-                                                            object: item,
+                                                            object: {
+                                                                id: item.id,
+                                                            },
path: frontend/src/scenes/dashboard/DashboardItem.tsx
text: @user1:
I think this "undo" function requires the full item to do the undoing. 

@user1:
actually no, just the id and the name...

@author:
oops thanks

diff_hunk: @@ -227,4 +227,18 @@ describe('funnelLogic', () => {
                 })
         })
     })
+
+    describe('dashboard item interface', () => {
+        initKeaTestLogic({
+            logic: funnelLogic,
+            props: { dashboardItemId: undefined },
+            onLogic: (l) => (logic = l),
+        })
+
+        it('can load directly', async () => {
+            await expectLogic(logic, () => {
+                logic.actions.loadResults()
+            }).toDispatchActions(['loadResults'])
+        })
+    })
path: frontend/src/scenes/funnels/funnelLogic.test.ts
text: @user1:
I don't think these tests add value. This just checks that calling loadResults calls loadResults, which doesn't check for anything other than its existence... Whether `loadResults` is a function we can call is already checked by typescript, so 🤷...


@author:
I agree but I wanted to put something in to prevent regression because typescript didn't catch this 

@user1:
Was this something related to deleting dashboard items?

@author:
yeah, I saw you have a new PR fix for it so I can remove these changes

@user1:
Yup, in addition we shouldn't actually be calling `loadResults` on any of the funnel/trends/paths/retentionTable logics anymore. They all proxy the action up to `insightsLogic` which does the API calls.