From d115a7eb7bb96d46146292e2a3c4f40de5a41c5e Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Wed, 10 Apr 2024 16:16:08 +0200 Subject: [PATCH 01/18] Remove find insight from mounted logic We rather load fresh to be save --- frontend/src/scenes/insights/insightLogic.ts | 14 +-------- frontend/src/scenes/insights/utils.tsx | 33 -------------------- 2 files changed, 1 insertion(+), 46 deletions(-) diff --git a/frontend/src/scenes/insights/insightLogic.ts b/frontend/src/scenes/insights/insightLogic.ts index cd6b4b6ac88e5..81deaccace4a1 100644 --- a/frontend/src/scenes/insights/insightLogic.ts +++ b/frontend/src/scenes/insights/insightLogic.ts @@ -52,7 +52,7 @@ import { import { teamLogic } from '../teamLogic' import { toLocalFilters } from './filters/ActionFilter/entityFilterLogic' import type { insightLogicType } from './insightLogicType' -import { extractObjectDiffKeys, findInsightFromMountedLogic, getInsightId } from './utils' +import { extractObjectDiffKeys, getInsightId } from './utils' const IS_TEST_MODE = process.env.NODE_ENV === 'test' export const UNSAVED_INSIGHT_MIN_REFRESH_INTERVAL_MINUTES = 3 @@ -710,18 +710,6 @@ export const insightLogic = kea([ return } - const insight = findInsightFromMountedLogic( - props.dashboardItemId as string | InsightShortId, - props.dashboardId - ) - if (insight) { - actions.setInsight(insight, { overrideFilter: true, fromPersistentApi: true }) - if (insight?.result) { - actions.reportInsightViewed(insight, insight.filters || {}) - } - return - } - if (!props.doNotLoad) { actions.loadInsight(props.dashboardItemId as InsightShortId) } diff --git a/frontend/src/scenes/insights/utils.tsx b/frontend/src/scenes/insights/utils.tsx index 212eff05f6c75..2ba3b5dca932e 100644 --- a/frontend/src/scenes/insights/utils.tsx +++ b/frontend/src/scenes/insights/utils.tsx @@ -4,11 +4,8 @@ import { CORE_FILTER_DEFINITIONS_BY_GROUP } from 'lib/taxonomy' import { ensureStringIsNotBlank, humanFriendlyNumber, objectsEqual } from 'lib/utils' import { getCurrentTeamId } from 'lib/utils/getAppContext' import { ReactNode } from 'react' -import { dashboardLogic } from 'scenes/dashboard/dashboardLogic' -import { savedInsightsLogic } from 'scenes/saved-insights/savedInsightsLogic' import { urls } from 'scenes/urls' -import { dashboardsModel } from '~/models/dashboardsModel' import { FormatPropertyValueForDisplayFunction } from '~/models/propertyDefinitionsModel' import { examples } from '~/queries/examples' import { ActionsNode, BreakdownFilter, DataWarehouseNode, EventsNode, PathsFilter } from '~/queries/schema' @@ -23,7 +20,6 @@ import { EntityFilter, EntityTypes, EventType, - InsightModel, InsightShortId, InsightType, PathType, @@ -126,35 +122,6 @@ export function extractObjectDiffKeys( return changedKeys } -export function findInsightFromMountedLogic( - insightShortId: InsightShortId | string, - dashboardId: number | undefined -): Partial | null { - if (dashboardId) { - const insightOnDashboard = dashboardLogic - .findMounted({ id: dashboardId }) - ?.values.insightTiles?.find((tile) => tile.insight?.short_id === insightShortId)?.insight - if (insightOnDashboard) { - return insightOnDashboard - } else { - const dashboards = dashboardsModel.findMounted()?.values.rawDashboards - let foundOnModel: Partial | undefined - for (const dashModelId of Object.keys(dashboards || {})) { - foundOnModel = dashboardLogic - .findMounted({ id: parseInt(dashModelId) }) - ?.values.insightTiles?.find((tile) => tile.insight?.short_id === insightShortId)?.insight - } - return foundOnModel || null - } - } else { - return ( - savedInsightsLogic - .findMounted() - ?.values.insights?.results?.find((item) => item.short_id === insightShortId) || null - ) - } -} - export async function getInsightId(shortId: InsightShortId): Promise { const insightId = insightLogic.findMounted({ dashboardItemId: shortId })?.values?.insight?.id From 8ecebbb6efab7bd449e15d05cf44510a329415cb Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Wed, 10 Apr 2024 17:28:59 +0200 Subject: [PATCH 02/18] Do not set insights anymore --- .../components/Cards/TextCard/TextCard.tsx | 2 +- .../src/scenes/dashboard/dashboardLogic.tsx | 37 ------------------- 2 files changed, 1 insertion(+), 38 deletions(-) diff --git a/frontend/src/lib/components/Cards/TextCard/TextCard.tsx b/frontend/src/lib/components/Cards/TextCard/TextCard.tsx index f0a09c95cfb29..c9b359d82d9b4 100644 --- a/frontend/src/lib/components/Cards/TextCard/TextCard.tsx +++ b/frontend/src/lib/components/Cards/TextCard/TextCard.tsx @@ -133,7 +133,7 @@ export function TextCardInternal( fullWidth data-attr="remove-text-tile-from-dashboard" > - Remove from dashboard + Delete )} diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index c21233b7b9a52..ffe78f3b27d92 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -29,7 +29,6 @@ import { clearDOMTextSelection, isUserLoggedIn, shouldCancelQuery, toParams, uui import { DashboardEventSource, eventUsageLogic } from 'lib/utils/eventUsageLogic' import { Layout, Layouts } from 'react-grid-layout' import { calculateLayouts } from 'scenes/dashboard/tileLayouts' -import { insightLogic } from 'scenes/insights/insightLogic' import { Scene } from 'scenes/sceneTypes' import { urls } from 'scenes/urls' import { userLogic } from 'scenes/userLogic' @@ -102,29 +101,6 @@ const layoutsByTile = (layouts: Layouts): Record([ path(['scenes', 'dashboard', 'dashboardLogic']), connect(() => ({ @@ -908,7 +884,6 @@ export const dashboardLogic = kea([ const refreshedInsightResponse: Response = await api.getResponse(apiUrl, methodOptions) const refreshedInsight: InsightModel = await getJSONOrNull(refreshedInsightResponse) breakpoint() - updateExistingInsightState({ cachedInsight: insight, dashboardId, refreshedInsight }) dashboardsModel.actions.updateDashboardInsight( refreshedInsight, [], @@ -1046,7 +1021,6 @@ export const dashboardLogic = kea([ allLoaded = false } else { const tilesWithNoResults = values.tiles?.filter((t) => !!t.insight && !t.insight.result) || [] - const tilesWithResults = values.tiles?.filter((t) => !!t.insight && t.insight.result) || [] if (tilesWithNoResults.length) { actions.refreshAllDashboardItems({ @@ -1057,17 +1031,6 @@ export const dashboardLogic = kea([ }) allLoaded = false } - - for (const tile of tilesWithResults) { - if (tile.insight) { - updateExistingInsightState({ - cachedInsight: tile.insight, - dashboardId: dashboard.id, - refreshedInsight: tile.insight, - }) - dashboardsModel.actions.updateDashboardInsight(tile.insight) - } - } } const payload: TimeToSeeDataPayload = { From 3a6aca5ccf9a81d4058dce66530f8abd247ddc47 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Wed, 10 Apr 2024 17:43:40 +0200 Subject: [PATCH 03/18] Bring some tests --- cypress/e2e/dashboard.cy.ts | 23 +++++++++++++++++++++++ cypress/productAnalytics/index.ts | 8 ++++++++ 2 files changed, 31 insertions(+) diff --git a/cypress/e2e/dashboard.cy.ts b/cypress/e2e/dashboard.cy.ts index 959aa5118fb80..af02e540aae7d 100644 --- a/cypress/e2e/dashboard.cy.ts +++ b/cypress/e2e/dashboard.cy.ts @@ -31,6 +31,29 @@ describe('Dashboard', () => { insight.addInsightToDashboard(dashboardName, { visitAfterAdding: true }) cy.get('.CardMeta h4').should('have.text', insightName) + + dashboard.addPropertyFilter() + cy.get('main').contains('There are no matching events for this query').should('not.exist') + + cy.clickNavMenu('dashboards') + const dashboardNameB = randomString('Dashboard with insight B') + dashboards.createAndGoToEmptyDashboard(dashboardNameB) + + insight.visitInsight(insightName) + insight.addInsightToDashboard(dashboardNameB, { visitAfterAdding: true }) + + dashboard.addPropertyFilter('Browser', 'Hogbrowser') + + // Go back and forth to make sure the filters are correctly applied + for (let i = 0; i < 3; i++) { + cy.clickNavMenu('dashboards') + dashboards.visitDashboard(dashboardName) + cy.get('main').contains('There are no matching events for this query').should('not.exist') + + cy.clickNavMenu('dashboards') + dashboards.visitDashboard(dashboardNameB) + cy.get('main').contains('There are no matching events for this query').should('exist') + } }) it('Adding new insight to dashboard does not clear filters', () => { diff --git a/cypress/productAnalytics/index.ts b/cypress/productAnalytics/index.ts index 584972cc63968..ad5070c9f96bc 100644 --- a/cypress/productAnalytics/index.ts +++ b/cypress/productAnalytics/index.ts @@ -169,6 +169,14 @@ export const dashboard = { cy.get('[data-attr=insight-save-button]').contains('Save & add to dashboard').click() cy.wait('@postInsight') }, + addPropertyFilter(type: string = 'Browser', value: string = 'Chrome'): void { + cy.get('.PropertyFilterButton').should('have.length', 0) + cy.get('[data-attr="property-filter-0"]').click() + cy.get('[data-attr="taxonomic-filter-searchfield"]').click().type('Browser').wait(1000) + cy.get('[data-attr="prop-filter-event_properties-0"]').click({ force: true }) + cy.get('.ant-select-selector').type(value) + cy.get('.ant-select-item-option-content').click({ force: true }) + }, addAnyFilter(): void { cy.get('.PropertyFilterButton').should('have.length', 0) cy.get('[data-attr="property-filter-0"]').click() From 36e1a3ac661a9ca9bc43f9e270a76977f5505a54 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 11 Apr 2024 11:49:18 +0200 Subject: [PATCH 04/18] Adjust Cypress tests --- cypress/e2e/dashboard.cy.ts | 15 ++++++++++----- cypress/productAnalytics/index.ts | 6 +++--- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/cypress/e2e/dashboard.cy.ts b/cypress/e2e/dashboard.cy.ts index af02e540aae7d..c187a5e40cfcb 100644 --- a/cypress/e2e/dashboard.cy.ts +++ b/cypress/e2e/dashboard.cy.ts @@ -20,7 +20,7 @@ describe('Dashboard', () => { }) it('Adding new insight to dashboard works', () => { - const dashboardName = randomString('Dashboard with insight A') + const dashboardName = randomString('Dashboard with matching filter') const insightName = randomString('insight to add to dashboard') // create and visit a dashboard to get it into turbomode cache @@ -36,22 +36,27 @@ describe('Dashboard', () => { cy.get('main').contains('There are no matching events for this query').should('not.exist') cy.clickNavMenu('dashboards') - const dashboardNameB = randomString('Dashboard with insight B') - dashboards.createAndGoToEmptyDashboard(dashboardNameB) + const dashboardNonMatching = randomString('Dashboard with non-matching filter') + dashboards.createAndGoToEmptyDashboard(dashboardNonMatching) insight.visitInsight(insightName) - insight.addInsightToDashboard(dashboardNameB, { visitAfterAdding: true }) + insight.addInsightToDashboard(dashboardNonMatching, { visitAfterAdding: true }) dashboard.addPropertyFilter('Browser', 'Hogbrowser') + cy.get('main').contains('There are no matching events for this query').should('exist') // Go back and forth to make sure the filters are correctly applied for (let i = 0; i < 3; i++) { cy.clickNavMenu('dashboards') dashboards.visitDashboard(dashboardName) + cy.get('.CardMeta h4').should('have.text', insightName) + cy.get('h4').contains('Refreshing').should('not.exist') cy.get('main').contains('There are no matching events for this query').should('not.exist') cy.clickNavMenu('dashboards') - dashboards.visitDashboard(dashboardNameB) + dashboards.visitDashboard(dashboardNonMatching) + cy.get('.CardMeta h4').should('have.text', insightName) + cy.get('h4').contains('Refreshing').should('not.exist') cy.get('main').contains('There are no matching events for this query').should('exist') } }) diff --git a/cypress/productAnalytics/index.ts b/cypress/productAnalytics/index.ts index ad5070c9f96bc..dfdb2f6502ac5 100644 --- a/cypress/productAnalytics/index.ts +++ b/cypress/productAnalytics/index.ts @@ -67,7 +67,7 @@ export const insight = { }, visitInsight: (insightName: string): void => { cy.clickNavMenu('savedinsights') - cy.contains('.row-name > .Link', insightName).click() + cy.contains('.Link', insightName).click() }, create: (insightName: string, insightType: string = 'TRENDS'): void => { cy.clickNavMenu('savedinsights') @@ -174,8 +174,8 @@ export const dashboard = { cy.get('[data-attr="property-filter-0"]').click() cy.get('[data-attr="taxonomic-filter-searchfield"]').click().type('Browser').wait(1000) cy.get('[data-attr="prop-filter-event_properties-0"]').click({ force: true }) - cy.get('.ant-select-selector').type(value) - cy.get('.ant-select-item-option-content').click({ force: true }) + cy.get('.LemonInput').type(value) + cy.contains('.LemonButton__content', value).click({ force: true }) }, addAnyFilter(): void { cy.get('.PropertyFilterButton').should('have.length', 0) From e2dbdb8c3a0f181f069c60905e50572f3a071ecd Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 11 Apr 2024 12:36:01 +0200 Subject: [PATCH 05/18] Skip unit test --- frontend/src/scenes/insights/insightLogic.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/scenes/insights/insightLogic.test.ts b/frontend/src/scenes/insights/insightLogic.test.ts index 6bf04a1aedd95..01ffdc59a0561 100644 --- a/frontend/src/scenes/insights/insightLogic.test.ts +++ b/frontend/src/scenes/insights/insightLogic.test.ts @@ -629,7 +629,7 @@ describe('insightLogic', () => { }), }) - it('loads from the dashboardLogic when in dashboard context', async () => { + it.skip('loads from the dashboardLogic when in dashboard context', async () => { // 1. the dashboard is mounted const dashLogic = dashboardLogic({ id: 33 }) dashLogic.mount() From 4d0b40c4ccda57424864bb8cf6695b9bb91db042 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 11 Apr 2024 12:47:57 +0200 Subject: [PATCH 06/18] Skip unit test --- frontend/src/scenes/insights/insightLogic.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/scenes/insights/insightLogic.test.ts b/frontend/src/scenes/insights/insightLogic.test.ts index 01ffdc59a0561..c2854c1d4a2cf 100644 --- a/frontend/src/scenes/insights/insightLogic.test.ts +++ b/frontend/src/scenes/insights/insightLogic.test.ts @@ -660,7 +660,7 @@ describe('insightLogic', () => { await verifyItLoadsFromTheAPI(logic) }) - it('loads from the savedInsightLogic when not in a dashboard context', async () => { + it.skip('loads from the savedInsightLogic when not in a dashboard context', async () => { // 1. open saved insights router.actions.push(urls.savedInsights(), {}, {}) savedInsightsLogic.mount() From 30c82b3e59ddd59bd6e74eb76c925c35b6fe1fd4 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Tue, 16 Apr 2024 10:41:05 +0200 Subject: [PATCH 07/18] Try less loading --- frontend/src/scenes/insights/insightLogic.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/scenes/insights/insightLogic.ts b/frontend/src/scenes/insights/insightLogic.ts index 81deaccace4a1..9bd3b0da8d656 100644 --- a/frontend/src/scenes/insights/insightLogic.ts +++ b/frontend/src/scenes/insights/insightLogic.ts @@ -711,7 +711,7 @@ export const insightLogic = kea([ } if (!props.doNotLoad) { - actions.loadInsight(props.dashboardItemId as InsightShortId) + //actions.loadInsight(props.dashboardItemId as InsightShortId) } }, })), From 90826a0ac6ac5c9617346d6052257d177286d875 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Wed, 24 Apr 2024 20:41:48 +0200 Subject: [PATCH 08/18] Adjust dataNodeLogic --- .../queries/nodes/DataNode/dataNodeLogic.ts | 23 +++++++++++-------- frontend/src/scenes/insights/insightLogic.ts | 2 +- .../scenes/trends/viz/ActionsLineGraph.tsx | 12 ++++++---- 3 files changed, 21 insertions(+), 16 deletions(-) diff --git a/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts b/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts index 56d11af1665b5..7e8266712ad71 100644 --- a/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts +++ b/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts @@ -113,15 +113,16 @@ export const dataNodeLogic = kea([ if (oldProps.query && props.query.kind !== oldProps.query.kind) { actions.clearResponse() } - if (!queryEqual(props.query, oldProps.query)) { - if ( - !props.cachedResults || - (isInsightQueryNode(props.query) && !props.cachedResults['result'] && !props.cachedResults['results']) - ) { - actions.loadData() - } else { - actions.setResponse(props.cachedResults) - } + if ( + !queryEqual(props.query, oldProps.query) && + (!props.cachedResults || + (isInsightQueryNode(props.query) && !props.cachedResults['result'] && !props.cachedResults['results'])) + ) { + actions.loadData() + } + if (props.cachedResults) { + // Use cached results if available, otherwise this logic will load the data again + actions.setResponse(props.cachedResults) } }), actions({ @@ -624,7 +625,9 @@ export const dataNodeLogic = kea([ }, })), afterMount(({ actions, props }) => { - if (Object.keys(props.query || {}).length > 0) { + if (Object.keys(props.query || {}).length > 0 && !props.key.includes('dashboard')) { + // Attention: When on dashboard we don't want to load data on mount + // as it will have be loaded by some other logic actions.loadData() } diff --git a/frontend/src/scenes/insights/insightLogic.ts b/frontend/src/scenes/insights/insightLogic.ts index 9bd3b0da8d656..81deaccace4a1 100644 --- a/frontend/src/scenes/insights/insightLogic.ts +++ b/frontend/src/scenes/insights/insightLogic.ts @@ -711,7 +711,7 @@ export const insightLogic = kea([ } if (!props.doNotLoad) { - //actions.loadInsight(props.dashboardItemId as InsightShortId) + actions.loadInsight(props.dashboardItemId as InsightShortId) } }, })), diff --git a/frontend/src/scenes/trends/viz/ActionsLineGraph.tsx b/frontend/src/scenes/trends/viz/ActionsLineGraph.tsx index bcee7d21580a8..268e2054ce4cf 100644 --- a/frontend/src/scenes/trends/viz/ActionsLineGraph.tsx +++ b/frontend/src/scenes/trends/viz/ActionsLineGraph.tsx @@ -75,9 +75,13 @@ export function ActionsLineGraph({ isInsightVizNode(query) && isTrendsQuery(query.source) - return indexedResults && - indexedResults[0]?.data && - indexedResults.filter((result) => result.count !== 0).length > 0 ? ( + if ( + !(indexedResults && indexedResults[0]?.data && indexedResults.filter((result) => result.count !== 0).length > 0) + ) { + return + } + + return ( - ) : ( - ) } From af8028a008c68de1fb27a128e9c0815080d178a3 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Wed, 24 Apr 2024 20:57:46 +0200 Subject: [PATCH 09/18] Make insightLogic not load things on mount when cached --- frontend/src/scenes/insights/insightLogic.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/scenes/insights/insightLogic.ts b/frontend/src/scenes/insights/insightLogic.ts index 81deaccace4a1..45afd10e49c3c 100644 --- a/frontend/src/scenes/insights/insightLogic.ts +++ b/frontend/src/scenes/insights/insightLogic.ts @@ -710,7 +710,7 @@ export const insightLogic = kea([ return } - if (!props.doNotLoad) { + if (!props.doNotLoad && !props.cachedInsight) { actions.loadInsight(props.dashboardItemId as InsightShortId) } }, From e3be07e1c19b128821654f7ea99927b81059a6b2 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Wed, 24 Apr 2024 21:15:04 +0200 Subject: [PATCH 10/18] Make sure refresh happens on filter change --- frontend/src/scenes/dashboard/dashboardLogic.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index ffe78f3b27d92..3ed9cc3d202aa 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -861,7 +861,9 @@ export const dashboardLogic = kea([ let refreshesFinished = 0 let totalResponseBytes = 0 - const hardRefreshWithoutCache = ['refresh', 'load_missing'].includes(action) + const hardRefreshWithoutCache = ['refresh', 'load_missing', 'refresh_insights_on_filters_updated'].includes( + action + ) // array of functions that reload each item const fetchItemFunctions = insights.map((insight) => async () => { From 389e26d36489da78f20530f59a855a0ff9393be9 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Wed, 24 Apr 2024 21:46:47 +0200 Subject: [PATCH 11/18] Make dataNodeLogic never load for dashboard with cached results --- frontend/src/queries/nodes/DataNode/dataNodeLogic.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts b/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts index 7e8266712ad71..104ea71294314 100644 --- a/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts +++ b/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts @@ -114,13 +114,13 @@ export const dataNodeLogic = kea([ actions.clearResponse() } if ( + !(props.cachedResults && props.key.includes('dashboard')) && // Don't load data on dashboard if cached results are available !queryEqual(props.query, oldProps.query) && (!props.cachedResults || (isInsightQueryNode(props.query) && !props.cachedResults['result'] && !props.cachedResults['results'])) ) { actions.loadData() - } - if (props.cachedResults) { + } else if (props.cachedResults) { // Use cached results if available, otherwise this logic will load the data again actions.setResponse(props.cachedResults) } From c95623e757a7fbab2d55af33c0e35c89eee4512d Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 25 Apr 2024 14:27:33 +0200 Subject: [PATCH 12/18] Fix usage of `updateTileOnDashboards` Fixes remaining unwanted propagation of dashboard filter changes to other dashboards. --- frontend/src/scenes/dashboard/dashboardLogic.tsx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index 3ed9cc3d202aa..088b1adc73b34 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -340,10 +340,11 @@ export const dashboardLogic = kea([ const newTiles = state.tiles.slice() if (tileIndex >= 0) { - if (insight.dashboards?.includes(props.id)) { - newTiles[tileIndex] = { ...newTiles[tileIndex], insight: insight } - if (updateTileOnDashboards?.includes(props.id)) { - newTiles[tileIndex].last_refresh = insight.last_refresh + if (insight.dashboards?.includes(props.id) && updateTileOnDashboards?.includes(props.id)) { + newTiles[tileIndex] = { + ...newTiles[tileIndex], + insight: insight, + last_refresh: insight.last_refresh, } } else { newTiles.splice(tileIndex, 1) From e017744c32f7683272ff53b8155bd861aed0007b Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 25 Apr 2024 16:50:38 +0200 Subject: [PATCH 13/18] Fix "Remove from dashboard" buttons being out of date --- .../AddToDashboard/AddToDashboardModal.tsx | 12 ++++--- .../addToDashboardModalLogic.ts | 36 ++++++++++++------- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/frontend/src/lib/components/AddToDashboard/AddToDashboardModal.tsx b/frontend/src/lib/components/AddToDashboard/AddToDashboardModal.tsx index c7ebea301a5f9..c924d8b9d59b6 100644 --- a/frontend/src/lib/components/AddToDashboard/AddToDashboardModal.tsx +++ b/frontend/src/lib/components/AddToDashboard/AddToDashboardModal.tsx @@ -128,7 +128,10 @@ export function AddToDashboardModal({ return ( { + closeModal() + setSearchQuery('') + }} isOpen={isOpen} title="Add to dashboard" footer={ @@ -139,7 +142,7 @@ export function AddToDashboardModal({ onClick={addNewDashboard} disabledReason={ !canEditInsight - ? 'You do not have permission to add this Insight to dashboards' + ? 'You do not have permission to add this insight to dashboards' : undefined } > @@ -162,9 +165,8 @@ export function AddToDashboardModal({ onChange={(newValue) => setSearchQuery(newValue)} />
- This insight is referenced on{' '} - {insight.dashboard_tiles?.length}{' '} - {pluralize(insight.dashboard_tiles?.length || 0, 'dashboard', 'dashboards', false)} + This insight is referenced on {currentDashboards.length}{' '} + {pluralize(currentDashboards.length, 'dashboard', 'dashboards', false)}
{/* eslint-disable-next-line react/forbid-dom-props */}
diff --git a/frontend/src/lib/components/AddToDashboard/addToDashboardModalLogic.ts b/frontend/src/lib/components/AddToDashboard/addToDashboardModalLogic.ts index b120dcc51c752..d2f1687611fd4 100644 --- a/frontend/src/lib/components/AddToDashboard/addToDashboardModalLogic.ts +++ b/frontend/src/lib/components/AddToDashboard/addToDashboardModalLogic.ts @@ -30,16 +30,28 @@ export const addToDashboardModalLogic = kea([ return insight.short_id }), path(['lib', 'components', 'AddToDashboard', 'saveToDashboardModalLogic']), - connect((props: AddToDashboardModalLogicProps) => ({ - actions: [ - insightLogic({ dashboardItemId: props.insight.short_id, cachedInsight: props.insight }), - ['updateInsight', 'updateInsightSuccess', 'updateInsightFailure'], - eventUsageLogic, - ['reportSavedInsightToDashboard', 'reportRemovedInsightFromDashboard', 'reportCreatedDashboardFromModal'], - newDashboardLogic, - ['showNewDashboardModal'], - ], - })), + connect((props: AddToDashboardModalLogicProps) => { + const builtInsightLogic = insightLogic({ + dashboardItemId: props.insight.short_id, + dashboardId: props.fromDashboard, + cachedInsight: props.insight, + }) + return { + values: [builtInsightLogic, ['insight']], + actions: [ + builtInsightLogic, + ['updateInsight', 'updateInsightSuccess', 'updateInsightFailure'], + eventUsageLogic, + [ + 'reportSavedInsightToDashboard', + 'reportRemovedInsightFromDashboard', + 'reportCreatedDashboardFromModal', + ], + newDashboardLogic, + ['showNewDashboardModal'], + ], + } + }), actions({ addNewDashboard: true, setSearchQuery: (query: string) => ({ query }), @@ -79,8 +91,8 @@ export const addToDashboardModalLogic = kea([ : nameSortedDashboards, ], currentDashboards: [ - (s, p) => [s.filteredDashboards, p.insight], - (filteredDashboards, insight: InsightModel): DashboardBasicType[] => + (s) => [s.filteredDashboards, s.insight], + (filteredDashboards, insight): DashboardBasicType[] => filteredDashboards.filter((d) => insight.dashboard_tiles?.map((dt) => dt.dashboard_id)?.includes(d.id)), ], availableDashboards: [ From dba304ddfdf3da6086ec69d263219f0cbcf6aaa5 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 25 Apr 2024 16:54:01 +0200 Subject: [PATCH 14/18] Add one more E2E test against a frustrating edge case I first made this test on master, making sure it fails. Passes on this branch! --- cypress/e2e/dashboard.cy.ts | 45 +++++++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/cypress/e2e/dashboard.cy.ts b/cypress/e2e/dashboard.cy.ts index c187a5e40cfcb..5961f1e3d9960 100644 --- a/cypress/e2e/dashboard.cy.ts +++ b/cypress/e2e/dashboard.cy.ts @@ -1,5 +1,6 @@ import { randomString } from '../support/random' import { insight, dashboards, dashboard } from '../productAnalytics' +import { urls } from 'scenes/urls' describe('Dashboard', () => { beforeEach(() => { @@ -23,7 +24,7 @@ describe('Dashboard', () => { const dashboardName = randomString('Dashboard with matching filter') const insightName = randomString('insight to add to dashboard') - // create and visit a dashboard to get it into turbomode cache + // Create and visit a dashboard to get it into turbo mode cache dashboards.createAndGoToEmptyDashboard(dashboardName) insight.create(insightName) @@ -61,12 +62,52 @@ describe('Dashboard', () => { } }) + it.only('Dashboard filter updates are correctly isolated for one insight on multiple dashboards', () => { + const dashboardAName = randomString('Dashboard with insight A') + const dashboardBName = randomString('Dashboard with insight B') + const insightName = randomString('insight to add to dashboard') + + // Create and visit two dashboards to get them into turbo mode cache + dashboards.createAndGoToEmptyDashboard(dashboardAName) + cy.clickNavMenu('dashboards') + dashboards.createAndGoToEmptyDashboard(dashboardBName) + + insight.create(insightName) + + // Add that one insight to both dashboards + insight.addInsightToDashboard(dashboardAName, { visitAfterAdding: false }) + cy.get('[aria-label="close"]').click() + insight.addInsightToDashboard(dashboardBName, { visitAfterAdding: false }) + cy.get('[aria-label="close"]').click() + + // Let's get dashboard A mounted + cy.clickNavMenu('dashboards') + dashboards.visitDashboard(dashboardAName) + cy.get('[data-attr=date-filter]').contains('No date range override') + cy.get('.InsightCard h5').should('have.length', 1).contains('Last 7 days') + // Now let's see dashboard B + cy.clickNavMenu('dashboards') + dashboards.visitDashboard(dashboardBName) + cy.get('[data-attr=date-filter]').contains('No date range override') + cy.get('.InsightCard h5').should('have.length', 1).contains('Last 7 days') + // Override the time range on dashboard B + cy.get('[data-attr=date-filter]').contains('No date range override').click() + cy.get('div').contains('Yesterday').should('exist').click() + cy.get('[data-attr=date-filter]').contains('Yesterday') + cy.get('.InsightCard h5').should('have.length', 1).contains('Yesterday') + // Cool, now back to A and make sure the insight is still using the original range there, not the one from B + cy.clickNavMenu('dashboards') + dashboards.visitDashboard(dashboardAName) + cy.get('[data-attr=date-filter]').contains('No date range override') + cy.get('.InsightCard h5').should('have.length', 1).contains('Last 7 days') // This must not be "Yesterday"! + }) + it('Adding new insight to dashboard does not clear filters', () => { const dashboardName = randomString('to add an insight to') const firstInsight = randomString('insight to add to dashboard') const secondInsight = randomString('another insight to add to dashboard') - // create and visit a dashboard to get it into turbomode cache + // Create and visit a dashboard to get it into turbo mode cache dashboards.createAndGoToEmptyDashboard(dashboardName) dashboard.addInsightToEmptyDashboard(firstInsight) From 6851d4e85fe7eec07a5c4ec9adc1774fcf2fad8c Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 25 Apr 2024 16:55:25 +0200 Subject: [PATCH 15/18] De-only-fy --- cypress/e2e/dashboard.cy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cypress/e2e/dashboard.cy.ts b/cypress/e2e/dashboard.cy.ts index 5961f1e3d9960..294104fdc91a9 100644 --- a/cypress/e2e/dashboard.cy.ts +++ b/cypress/e2e/dashboard.cy.ts @@ -62,7 +62,7 @@ describe('Dashboard', () => { } }) - it.only('Dashboard filter updates are correctly isolated for one insight on multiple dashboards', () => { + it('Dashboard filter updates are correctly isolated for one insight on multiple dashboards', () => { const dashboardAName = randomString('Dashboard with insight A') const dashboardBName = randomString('Dashboard with insight B') const insightName = randomString('insight to add to dashboard') From 9a751c821e5d5ab404914a5b792071b55c982b4b Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Fri, 26 Apr 2024 11:52:08 +0200 Subject: [PATCH 16/18] Adjust and remove test --- .../src/scenes/insights/insightLogic.test.ts | 37 +------------------ 1 file changed, 2 insertions(+), 35 deletions(-) diff --git a/frontend/src/scenes/insights/insightLogic.test.ts b/frontend/src/scenes/insights/insightLogic.test.ts index c2854c1d4a2cf..f274314245e7a 100644 --- a/frontend/src/scenes/insights/insightLogic.test.ts +++ b/frontend/src/scenes/insights/insightLogic.test.ts @@ -609,17 +609,6 @@ describe('insightLogic', () => { }) describe('takes data from other logics if available', () => { - const verifyItLoadsFromALogic = async ( - logicUnderTest: ReturnType, - partialExpectedInsight: Partial - ): Promise => - expectLogic(logicUnderTest) - .toDispatchActions(['setInsight']) - .toNotHaveDispatchedActions(['setFilters', 'loadResults', 'loadInsight', 'updateInsight']) - .toMatchValues({ - insight: partial(partialExpectedInsight), - }) - const verifyItLoadsFromTheAPI = async (logicUnderTest: ReturnType): Promise => expectLogic(logicUnderTest) .toDispatchActions(['loadInsight']) @@ -629,7 +618,7 @@ describe('insightLogic', () => { }), }) - it.skip('loads from the dashboardLogic when in dashboard context', async () => { + it('loads from the api when coming from dashboard context', async () => { // 1. the dashboard is mounted const dashLogic = dashboardLogic({ id: 33 }) dashLogic.mount() @@ -639,12 +628,7 @@ describe('insightLogic', () => { logic = insightLogic({ dashboardItemId: Insight42, dashboardId: 33 }) logic.mount() - // 3. verify it didn't make any API calls - await verifyItLoadsFromALogic(logic, { - id: 42, - result: 'result!', - filters: { insight: InsightType.TRENDS, interval: 'month' }, - }) + await verifyItLoadsFromTheAPI(logic) }) it('does not load from the dashboardLogic when not in that dashboard context', async () => { @@ -660,23 +644,6 @@ describe('insightLogic', () => { await verifyItLoadsFromTheAPI(logic) }) - it.skip('loads from the savedInsightLogic when not in a dashboard context', async () => { - // 1. open saved insights - router.actions.push(urls.savedInsights(), {}, {}) - savedInsightsLogic.mount() - - // 2. the insights are loaded - await expectLogic(savedInsightsLogic).toDispatchActions(['loadInsights', 'loadInsightsSuccess']) - - // 3. mount the insight - logic = insightLogic({ dashboardItemId: Insight42 }) - logic.mount() - - await verifyItLoadsFromALogic(logic, { - short_id: '42' as InsightShortId, - }) - }) - it('does not load from the savedInsightLogic when in a dashboard context', async () => { // 1. open saved insights router.actions.push(urls.savedInsights(), {}, {}) From 16e98551edeb4ae6428048dff29416a36a173a07 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Fri, 26 Apr 2024 15:01:55 +0200 Subject: [PATCH 17/18] Fix conflicting insight IDs in Storybook mocks --- .../fixtures/api/projects/team_id/insights/stickiness.json | 2 +- .../api/projects/team_id/insights/trendsBarBreakdown.json | 2 +- .../projects/team_id/insights/trendsCumulativeBreakdown.json | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/src/mocks/fixtures/api/projects/team_id/insights/stickiness.json b/frontend/src/mocks/fixtures/api/projects/team_id/insights/stickiness.json index f79e4db7a1545..db147c1c744e6 100644 --- a/frontend/src/mocks/fixtures/api/projects/team_id/insights/stickiness.json +++ b/frontend/src/mocks/fixtures/api/projects/team_id/insights/stickiness.json @@ -1,6 +1,6 @@ { "id": 51, - "short_id": "jEAIVJnI", + "short_id": "xEAIVJnI", "name": "", "derived_name": "User stickiness based on Pageview", "filters": { diff --git a/frontend/src/mocks/fixtures/api/projects/team_id/insights/trendsBarBreakdown.json b/frontend/src/mocks/fixtures/api/projects/team_id/insights/trendsBarBreakdown.json index 4a99df0739a55..d6c12efe46b7c 100644 --- a/frontend/src/mocks/fixtures/api/projects/team_id/insights/trendsBarBreakdown.json +++ b/frontend/src/mocks/fixtures/api/projects/team_id/insights/trendsBarBreakdown.json @@ -1,6 +1,6 @@ { "id": 51, - "short_id": "jEAIVJnK", + "short_id": "xEAIVJnK", "name": "", "derived_name": "Pageview count by event's Library Version", "filters": { diff --git a/frontend/src/mocks/fixtures/api/projects/team_id/insights/trendsCumulativeBreakdown.json b/frontend/src/mocks/fixtures/api/projects/team_id/insights/trendsCumulativeBreakdown.json index 5ab85ea0ea2e4..b22148cb81fc4 100644 --- a/frontend/src/mocks/fixtures/api/projects/team_id/insights/trendsCumulativeBreakdown.json +++ b/frontend/src/mocks/fixtures/api/projects/team_id/insights/trendsCumulativeBreakdown.json @@ -1,6 +1,6 @@ { "id": 51, - "short_id": "jEAIVJnM", + "short_id": "xEAIVJnM", "name": "", "derived_name": "Pageview count by event's Browser Version", "filters": { From 614068b491b83b8704c6c103bfce61aff88cb1b4 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Fri, 26 Apr 2024 17:00:27 +0200 Subject: [PATCH 18/18] Remove `fromDashboard` from `addToDashboardModalLogic` --- .../src/lib/components/AddToDashboard/AddToDashboardModal.tsx | 2 -- .../lib/components/AddToDashboard/addToDashboardModalLogic.ts | 2 -- 2 files changed, 4 deletions(-) diff --git a/frontend/src/lib/components/AddToDashboard/AddToDashboardModal.tsx b/frontend/src/lib/components/AddToDashboard/AddToDashboardModal.tsx index c924d8b9d59b6..e69da14b09f95 100644 --- a/frontend/src/lib/components/AddToDashboard/AddToDashboardModal.tsx +++ b/frontend/src/lib/components/AddToDashboard/AddToDashboardModal.tsx @@ -42,7 +42,6 @@ const DashboardRelationRow = ({ }: DashboardRelationRowProps): JSX.Element => { const logic = addToDashboardModalLogic({ insight: insight, - fromDashboard: insight.dashboards?.[0] || undefined, }) const { addToDashboard, removeFromDashboard } = useActions(logic) const { dashboardWithActiveAPICall } = useValues(logic) @@ -104,7 +103,6 @@ export function AddToDashboardModal({ }: SaveToDashboardModalProps): JSX.Element { const logic = addToDashboardModalLogic({ insight: insight, - fromDashboard: insight.dashboards?.[0] || undefined, }) const { searchQuery, currentDashboards, orderedDashboards, scrollIndex } = useValues(logic) diff --git a/frontend/src/lib/components/AddToDashboard/addToDashboardModalLogic.ts b/frontend/src/lib/components/AddToDashboard/addToDashboardModalLogic.ts index d2f1687611fd4..7d3d5164d81ea 100644 --- a/frontend/src/lib/components/AddToDashboard/addToDashboardModalLogic.ts +++ b/frontend/src/lib/components/AddToDashboard/addToDashboardModalLogic.ts @@ -14,7 +14,6 @@ import type { addToDashboardModalLogicType } from './addToDashboardModalLogicTyp export interface AddToDashboardModalLogicProps { insight: Partial - fromDashboard?: number } // Helping kea-typegen navigate the exported default class for Fuse @@ -33,7 +32,6 @@ export const addToDashboardModalLogic = kea([ connect((props: AddToDashboardModalLogicProps) => { const builtInsightLogic = insightLogic({ dashboardItemId: props.insight.short_id, - dashboardId: props.fromDashboard, cachedInsight: props.insight, }) return {