From 64967325b2b6a601d8e9a23230ea59d432194fbc Mon Sep 17 00:00:00 2001 From: Leo <108552997+lpskdl@users.noreply.github.com> Date: Mon, 8 Aug 2022 11:13:19 +0200 Subject: [PATCH] Search: Migrated impressions to use dashboardUID (#53090) * used dashboarduid in impressions * handle scenario to convert all ids saved in storage to uids --- .betterer.results | 3 +- public/app/core/services/impression_srv.ts | 42 ++++-- public/app/core/services/search_srv.ts | 22 +-- public/app/core/specs/impression_srv.test.ts | 45 ++++++ public/app/core/specs/search_srv.test.ts | 130 +++++++++--------- .../dashboard/services/DashboardLoaderSrv.ts | 6 +- .../page/components/FolderView.test.tsx | 6 +- .../search/page/components/FolderView.tsx | 9 +- .../app/plugins/panel/dashlist/DashList.tsx | 37 +++-- public/app/types/dashboard.ts | 1 + 10 files changed, 186 insertions(+), 115 deletions(-) create mode 100644 public/app/core/specs/impression_srv.test.ts diff --git a/.betterer.results b/.betterer.results index 22dd45aebc5..c21140c7fc4 100644 --- a/.betterer.results +++ b/.betterer.results @@ -4319,8 +4319,7 @@ exports[`better eslint`] = { [0, 0, 0, "Unexpected any. Specify a different type.", "5"], [0, 0, 0, "Unexpected any. Specify a different type.", "6"], [0, 0, 0, "Unexpected any. Specify a different type.", "7"], - [0, 0, 0, "Unexpected any. Specify a different type.", "8"], - [0, 0, 0, "Unexpected any. Specify a different type.", "9"] + [0, 0, 0, "Unexpected any. Specify a different type.", "8"] ], "public/app/features/dashboard/services/DashboardSrv.ts:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"], diff --git a/public/app/core/services/impression_srv.ts b/public/app/core/services/impression_srv.ts index 114468fddc7..3dcd3bdd39c 100644 --- a/public/app/core/services/impression_srv.ts +++ b/public/app/core/services/impression_srv.ts @@ -1,14 +1,15 @@ -import { filter, isArray, isNumber } from 'lodash'; +import { filter, isArray, isNumber, isString } from 'lodash'; +import { getBackendSrv } from '@grafana/runtime'; import config from 'app/core/config'; import store from 'app/core/store'; export class ImpressionSrv { constructor() {} - addDashboardImpression(dashboardId: number) { + addDashboardImpression(dashboardUID: string) { const impressionsKey = this.impressionKey(); - let impressions = []; + let impressions: string[] = []; if (store.exists(impressionsKey)) { impressions = JSON.parse(store.get(impressionsKey)); if (!isArray(impressions)) { @@ -17,10 +18,10 @@ export class ImpressionSrv { } impressions = impressions.filter((imp) => { - return dashboardId !== imp; + return dashboardUID !== imp; }); - impressions.unshift(dashboardId); + impressions.unshift(dashboardUID); if (impressions.length > 50) { impressions.pop(); @@ -28,17 +29,32 @@ export class ImpressionSrv { store.set(impressionsKey, JSON.stringify(impressions)); } - /** Returns an array of internal (numeric) dashboard IDs */ - getDashboardOpened(): number[] { - let impressions = store.get(this.impressionKey()) || '[]'; + private async convertToUIDs() { + let impressions = this.getImpressions(); + const ids = filter(impressions, (el) => isNumber(el)); + if (!ids.length) { + return; + } - impressions = JSON.parse(impressions); + const convertedUIDs = await getBackendSrv().get(`/api/dashboards/ids/${ids.join(',')}`); + store.set(this.impressionKey(), JSON.stringify([...filter(impressions, (el) => isString(el)), ...convertedUIDs])); + } - impressions = filter(impressions, (el) => { - return isNumber(el); - }); + private getImpressions() { + const impressions = store.get(this.impressionKey()) || '[]'; - return impressions; + return JSON.parse(impressions); + } + + /** Returns an array of internal (string) dashboard UIDs */ + async getDashboardOpened(): Promise { + // TODO should be removed after UID migration + try { + await this.convertToUIDs(); + } catch (_) {} + + const result = filter(this.getImpressions(), (el) => isString(el)); + return result; } impressionKey() { diff --git a/public/app/core/services/search_srv.ts b/public/app/core/services/search_srv.ts index b6111211610..14d0d4460cb 100644 --- a/public/app/core/services/search_srv.ts +++ b/public/app/core/services/search_srv.ts @@ -31,15 +31,21 @@ export class SearchSrv { } private queryForRecentDashboards(): Promise { - const dashIds: number[] = take(impressionSrv.getDashboardOpened(), 30); - if (dashIds.length === 0) { - return Promise.resolve([]); - } + return new Promise((resolve) => { + impressionSrv.getDashboardOpened().then((uids) => { + const dashUIDs: string[] = take(uids, 30); + if (dashUIDs.length === 0) { + return resolve([]); + } - return backendSrv.search({ dashboardIds: dashIds }).then((result) => { - return dashIds - .map((orderId) => result.find((result) => result.id === orderId)) - .filter((hit) => hit && !hit.isStarred) as DashboardSearchHit[]; + backendSrv.search({ dashboardUIDs: dashUIDs }).then((result) => { + return resolve( + dashUIDs + .map((orderId) => result.find((result) => result.uid === orderId)) + .filter((hit) => hit && !hit.isStarred) as DashboardSearchHit[] + ); + }); + }); }); } diff --git a/public/app/core/specs/impression_srv.test.ts b/public/app/core/specs/impression_srv.test.ts new file mode 100644 index 00000000000..32a479c36c3 --- /dev/null +++ b/public/app/core/specs/impression_srv.test.ts @@ -0,0 +1,45 @@ +const mockBackendSrv = jest.fn(); + +import impressionSrv from '../services/impression_srv'; + +jest.mock('@grafana/runtime', () => { + const originalRuntime = jest.requireActual('@grafana/runtime'); + return { + ...originalRuntime, + getBackendSrv: mockBackendSrv, + config: { + ...originalRuntime.config, + bootData: { + ...originalRuntime.config.bootData, + user: { + ...originalRuntime.config.bootData.user, + orgId: 'testOrgId', + }, + }, + }, + }; +}); + +describe('ImpressionSrv', () => { + beforeEach(() => { + window.localStorage.removeItem(impressionSrv.impressionKey()); + }); + + describe('getDashboardOpened', () => { + it('should return list of dashboard uids', async () => { + window.localStorage.setItem(impressionSrv.impressionKey(), JSON.stringify(['five', 'four', 1, 2, 3])); + mockBackendSrv.mockImplementation(() => ({ get: jest.fn().mockResolvedValue(['one', 'two', 'three']) })); + const result1 = await impressionSrv.getDashboardOpened(); + expect(result1).toEqual(['five', 'four', 'one', 'two', 'three']); + + window.localStorage.setItem(impressionSrv.impressionKey(), JSON.stringify(['three', 'four'])); + const result2 = await impressionSrv.getDashboardOpened(); + expect(result2).toEqual(['three', 'four']); + + window.localStorage.setItem(impressionSrv.impressionKey(), JSON.stringify([1, 2, 3])); + mockBackendSrv.mockImplementation(() => ({ get: jest.fn().mockResolvedValue(['one', 'two', 'three']) })); + const result3 = await impressionSrv.getDashboardOpened(); + expect(result3).toEqual(['one', 'two', 'three']); + }); + }); +}); diff --git a/public/app/core/specs/search_srv.test.ts b/public/app/core/specs/search_srv.test.ts index 28fff1478df..b9ceda26d5e 100644 --- a/public/app/core/specs/search_srv.test.ts +++ b/public/app/core/specs/search_srv.test.ts @@ -1,6 +1,7 @@ import { contextSrv } from 'app/core/services/context_srv'; import impressionSrv from 'app/core/services/impression_srv'; import { SearchSrv } from 'app/core/services/search_srv'; +import { DashboardSearchHit } from 'app/features/search/types'; import { backendSrv } from '../services/backend_srv'; @@ -26,7 +27,7 @@ describe('SearchSrv', () => { searchSrv = new SearchSrv(); contextSrv.isSignedIn = true; - impressionSrv.getDashboardOpened = jest.fn().mockReturnValue([]); + impressionSrv.getDashboardOpened = jest.fn().mockResolvedValue([]); jest.clearAllMocks(); }); @@ -34,19 +35,17 @@ describe('SearchSrv', () => { let results: any; beforeEach(() => { - searchMock.mockImplementation( - jest - .fn() - .mockReturnValueOnce( - Promise.resolve([ - { id: 2, title: 'second but first' }, - { id: 1, title: 'first but second' }, - ]) - ) - .mockReturnValue(Promise.resolve([])) - ); + searchMock.mockImplementation((options) => { + if (options.dashboardUIDs) { + return Promise.resolve([ + { uid: 'DSNdW0gVk', title: 'second but first' }, + { uid: 'srx16xR4z', title: 'first but second' }, + ] as DashboardSearchHit[]); + } + return Promise.resolve([]); + }); - impressionSrv.getDashboardOpened = jest.fn().mockReturnValue([1, 2]); + impressionSrv.getDashboardOpened = jest.fn().mockResolvedValue(['srx16xR4z', 'DSNdW0gVk']); return searchSrv.search({ query: '' }).then((res) => { results = res; @@ -66,19 +65,19 @@ describe('SearchSrv', () => { let results: any; beforeEach(() => { - searchMock.mockImplementation( - jest - .fn() - .mockReturnValueOnce( - Promise.resolve([ - { id: 2, title: 'two' }, - { id: 1, title: 'one' }, - ]) - ) - .mockReturnValue(Promise.resolve([])) - ); + searchMock.mockImplementation((options) => { + if (options.dashboardUIDs) { + return Promise.resolve([ + { uid: 'DSNdW0gVk', title: 'two' }, + { uid: 'srx16xR4z', title: 'one' }, + ] as DashboardSearchHit[]); + } + return Promise.resolve([]); + }); - impressionSrv.getDashboardOpened = jest.fn().mockReturnValue([4, 5, 1, 2, 3]); + impressionSrv.getDashboardOpened = jest + .fn() + .mockResolvedValue(['Xrx16x4z', 'CSxdW0gYA', 'srx16xR4z', 'DSNdW0gVk', 'xSxdW0gYA']); return searchSrv.search({ query: '' }).then((res) => { results = res; @@ -87,8 +86,8 @@ describe('SearchSrv', () => { it('should return 2 dashboards', () => { expect(results[0].items.length).toBe(2); - expect(results[0].items[0].id).toBe(1); - expect(results[0].items[1].id).toBe(2); + expect(results[0].items[0].uid).toBe('srx16xR4z'); + expect(results[0].items[1].uid).toBe('DSNdW0gVk'); }); }); }); @@ -97,7 +96,12 @@ describe('SearchSrv', () => { let results: any; beforeEach(() => { - searchMock.mockImplementation(jest.fn().mockReturnValue(Promise.resolve([{ id: 1, title: 'starred' }]))); + searchMock.mockImplementation((options) => { + if (options.starred) { + return Promise.resolve([{ id: 1, title: 'starred' }] as DashboardSearchHit[]); + } + return Promise.resolve([]); + }); return searchSrv.search({ query: '' }).then((res) => { results = res; @@ -114,19 +118,17 @@ describe('SearchSrv', () => { let results: any; beforeEach(() => { - searchMock.mockImplementation( - jest - .fn() - .mockReturnValueOnce( - Promise.resolve([ - { id: 1, title: 'starred and recent', isStarred: true }, - { id: 2, title: 'recent' }, - ]) - ) - .mockReturnValue(Promise.resolve([{ id: 1, title: 'starred and recent' }])) - ); + searchMock.mockImplementation((options) => { + if (options.dashboardUIDs) { + return Promise.resolve([ + { uid: 'srx16xR4z', title: 'starred and recent', isStarred: true }, + { uid: 'DSNdW0gVk', title: 'recent' }, + ] as DashboardSearchHit[]); + } + return Promise.resolve([{ uid: 'srx16xR4z', title: 'starred and recent' }] as DashboardSearchHit[]); + }); - impressionSrv.getDashboardOpened = jest.fn().mockReturnValue([1, 2]); + impressionSrv.getDashboardOpened = jest.fn().mockResolvedValue(['srx16xR4z', 'DSNdW0gVk']); return searchSrv.search({ query: '' }).then((res) => { results = res; }); @@ -150,8 +152,8 @@ describe('SearchSrv', () => { searchMock.mockImplementation( jest .fn() - .mockReturnValueOnce(Promise.resolve([])) - .mockReturnValue( + .mockResolvedValueOnce(Promise.resolve([])) + .mockResolvedValue( Promise.resolve([ { title: 'folder1', @@ -198,24 +200,22 @@ describe('SearchSrv', () => { beforeEach(() => { searchMock.mockImplementation( - jest.fn().mockReturnValue( - Promise.resolve([ - { - id: 2, - title: 'dash with no folder', - type: 'dash-db', - }, - { - id: 3, - title: 'dash in folder1 1', - type: 'dash-db', - folderId: 1, - folderUid: 'uid', - folderTitle: 'folder1', - folderUrl: '/dashboards/f/uid/folder1', - }, - ]) - ) + jest.fn().mockResolvedValue([ + { + id: 2, + title: 'dash with no folder', + type: 'dash-db', + }, + { + id: 3, + title: 'dash in folder1 1', + type: 'dash-db', + folderId: 1, + folderUid: 'uid', + folderTitle: 'folder1', + folderUrl: '/dashboards/f/uid/folder1', + }, + ]) ); return searchSrv.search({ query: 'search' }).then((res) => { @@ -239,7 +239,7 @@ describe('SearchSrv', () => { describe('with tags', () => { beforeEach(() => { - searchMock.mockImplementation(jest.fn().mockReturnValue(Promise.resolve([]))); + searchMock.mockImplementation(jest.fn().mockResolvedValue(Promise.resolve([]))); return searchSrv.search({ tag: ['atag'] }).then(() => {}); }); @@ -251,7 +251,7 @@ describe('SearchSrv', () => { describe('with starred', () => { beforeEach(() => { - searchMock.mockImplementation(jest.fn().mockReturnValue(Promise.resolve([]))); + searchMock.mockImplementation(jest.fn().mockResolvedValue(Promise.resolve([]))); return searchSrv.search({ starred: true }).then(() => {}); }); @@ -265,7 +265,7 @@ describe('SearchSrv', () => { let getRecentDashboardsCalled = false; beforeEach(() => { - searchMock.mockImplementation(jest.fn().mockReturnValue(Promise.resolve([]))); + searchMock.mockImplementation(jest.fn().mockResolvedValue(Promise.resolve([]))); searchSrv['getRecentDashboards'] = () => { getRecentDashboardsCalled = true; @@ -284,8 +284,8 @@ describe('SearchSrv', () => { let getStarredCalled = false; beforeEach(() => { - searchMock.mockImplementation(jest.fn().mockReturnValue(Promise.resolve([]))); - impressionSrv.getDashboardOpened = jest.fn().mockReturnValue([]); + searchMock.mockImplementation(jest.fn().mockResolvedValue(Promise.resolve([]))); + impressionSrv.getDashboardOpened = jest.fn().mockResolvedValue([]); searchSrv['getStarred'] = () => { getStarredCalled = true; diff --git a/public/app/features/dashboard/services/DashboardLoaderSrv.ts b/public/app/features/dashboard/services/DashboardLoaderSrv.ts index a2f781cd177..986cec7d14d 100644 --- a/public/app/features/dashboard/services/DashboardLoaderSrv.ts +++ b/public/app/features/dashboard/services/DashboardLoaderSrv.ts @@ -9,7 +9,7 @@ import impressionSrv from 'app/core/services/impression_srv'; import kbn from 'app/core/utils/kbn'; import { getDatasourceSrv } from 'app/features/plugins/datasource_srv'; import { getGrafanaStorage } from 'app/features/storage/storage'; -import { DashboardRoutes } from 'app/types'; +import { DashboardDTO, DashboardRoutes } from 'app/types'; import { appEvents } from '../../../core/core'; @@ -69,9 +69,9 @@ export class DashboardLoaderSrv { }); } - promise.then((result: any) => { + promise.then((result: DashboardDTO) => { if (result.meta.dashboardNotFound !== true) { - impressionSrv.addDashboardImpression(result.dashboard.id); + impressionSrv.addDashboardImpression(result.dashboard.uid); } return result; diff --git a/public/app/features/search/page/components/FolderView.test.tsx b/public/app/features/search/page/components/FolderView.test.tsx index 5ced047cc31..f96139b6295 100644 --- a/public/app/features/search/page/components/FolderView.test.tsx +++ b/public/app/features/search/page/components/FolderView.test.tsx @@ -99,7 +99,7 @@ describe('FolderView', () => { }); it('does not show the recent items if no dashboards have been opened recently', async () => { - jest.spyOn(impressionSrv, 'getDashboardOpened').mockReturnValue([]); + jest.spyOn(impressionSrv, 'getDashboardOpened').mockResolvedValue([]); render( ); @@ -108,7 +108,7 @@ describe('FolderView', () => { }); it('shows the recent items if any dashboards have recently been opened', async () => { - jest.spyOn(impressionSrv, 'getDashboardOpened').mockReturnValue([12345]); + jest.spyOn(impressionSrv, 'getDashboardOpened').mockResolvedValue(['7MeksYbmk']); render( ); @@ -138,7 +138,7 @@ describe('FolderView', () => { }); it('does not show the recent items even if recent dashboards have been opened', async () => { - jest.spyOn(impressionSrv, 'getDashboardOpened').mockReturnValue([12345]); + jest.spyOn(impressionSrv, 'getDashboardOpened').mockResolvedValue(['7MeksYbmk']); render( = Promise.resolve([]); - let dashIds: number[] = []; + let dashUIDs: string[] = []; if (options.showRecentlyViewed) { - dashIds = take(impressionSrv.getDashboardOpened(), options.maxItems); - recentDashboards = getBackendSrv().search({ dashboardIds: dashIds, limit: options.maxItems }); + let uids = await impressionSrv.getDashboardOpened(); + dashUIDs = take(uids, options.maxItems); + recentDashboards = getBackendSrv().search({ dashboardUIDs: dashUIDs, limit: options.maxItems }); } let searchedDashboards: Promise = Promise.resolve([]); @@ -55,27 +56,33 @@ async function fetchDashboards(options: PanelOptions, replaceVars: InterpolateFu const [starred, searched, recent] = await Promise.all([starredDashboards, searchedDashboards, recentDashboards]); // We deliberately deal with recent dashboards first so that the order of dash IDs is preserved - let dashMap = new Map(); - for (const dashId of dashIds) { - const dash = recent.find((d) => d.id === dashId); + let dashMap = new Map(); + for (const dashUID of dashUIDs) { + const dash = recent.find((d) => d.uid === dashUID); if (dash) { - dashMap.set(dashId, { ...dash, isRecent: true }); + dashMap.set(dashUID, { ...dash, isRecent: true }); } } searched.forEach((dash) => { - if (dashMap.has(dash.id)) { - dashMap.get(dash.id)!.isSearchResult = true; + if (!dash.uid) { + return; + } + if (dashMap.has(dash.uid)) { + dashMap.get(dash.uid)!.isSearchResult = true; } else { - dashMap.set(dash.id, { ...dash, isSearchResult: true }); + dashMap.set(dash.uid, { ...dash, isSearchResult: true }); } }); starred.forEach((dash) => { - if (dashMap.has(dash.id)) { - dashMap.get(dash.id)!.isStarred = true; + if (!dash.uid) { + return; + } + if (dashMap.has(dash.uid)) { + dashMap.get(dash.uid)!.isStarred = true; } else { - dashMap.set(dash.id, { ...dash, isStarred: true }); + dashMap.set(dash.uid, { ...dash, isStarred: true }); } }); @@ -83,7 +90,7 @@ async function fetchDashboards(options: PanelOptions, replaceVars: InterpolateFu } export function DashList(props: PanelProps) { - const [dashboards, setDashboards] = useState(new Map()); + const [dashboards, setDashboards] = useState(new Map()); const dispatch = useDispatch(); useEffect(() => { fetchDashboards(props.options, props.replaceVariables).then((dashes) => { @@ -98,7 +105,7 @@ export function DashList(props: PanelProps) { const isStarred = await getDashboardSrv().starDashboard(dash.id.toString(), dash.isStarred); const updatedDashboards = new Map(dashboards); - updatedDashboards.set(dash.id, { ...dash, isStarred }); + updatedDashboards.set(dash?.uid ?? '', { ...dash, isStarred }); setDashboards(updatedDashboards); dispatch(setStarred({ id: uid ?? '', title, url, isStarred })); }; diff --git a/public/app/types/dashboard.ts b/public/app/types/dashboard.ts index 5739ec3c3ff..fd8924d5303 100644 --- a/public/app/types/dashboard.ts +++ b/public/app/types/dashboard.ts @@ -43,6 +43,7 @@ export interface DashboardMeta { annotationsPermissions?: AnnotationsPermissions; publicDashboardAccessToken?: string; publicDashboardEnabled?: boolean; + dashboardNotFound?: boolean; } export interface AnnotationActions {