From e7605ad974d45b3398d43846d9ca5da3047f7c0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Jamr=C3=B3z?= Date: Thu, 10 Feb 2022 13:59:43 +0100 Subject: [PATCH] Query History: Split data and view models (#44922) * Remove unused properties * Fix unit tests * Fix unit tests * Split data models * Simplify updating items in rich history * Update tests * Fix starring an item and add a unit test * Move the converter to a separate file and add unit tests * Convert a private function to an inline function * Add more docs and clean up the code * Update public/app/core/history/localStorageConverter.ts Co-authored-by: Giordano Ricci * Update public/app/core/utils/richHistory.test.ts Co-authored-by: Giordano Ricci * Use template literals over explicit casting * Split updateRichHistory to three separate functions Co-authored-by: Giordano Ricci --- .betterer.results | 2 +- .../history/RichHistoryLocalStorage.test.ts | 101 +++++++++++----- .../core/history/RichHistoryLocalStorage.ts | 109 +++++++++++------- public/app/core/history/RichHistoryStorage.ts | 15 ++- .../history/localStorageConverter.test.ts | 60 ++++++++++ .../app/core/history/localStorageConverter.ts | 41 +++++++ .../history/richHistoryLocalStorageUtils.ts | 8 +- public/app/core/utils/richHistory.test.ts | 54 ++++++--- public/app/core/utils/richHistory.ts | 70 ++++------- .../RichHistory/RichHistoryCard.test.tsx | 48 +++++--- .../explore/RichHistory/RichHistoryCard.tsx | 36 ++++-- .../RichHistory/RichHistoryQueriesTab.tsx | 2 +- .../RichHistory/RichHistoryStarredTab.tsx | 2 +- public/app/features/explore/state/history.ts | 28 +++-- public/app/features/explore/state/query.ts | 1 + public/app/types/explore.ts | 8 +- 16 files changed, 401 insertions(+), 184 deletions(-) create mode 100644 public/app/core/history/localStorageConverter.test.ts create mode 100644 public/app/core/history/localStorageConverter.ts diff --git a/.betterer.results b/.betterer.results index 7379bf002b7..b1e8976a9d1 100644 --- a/.betterer.results +++ b/.betterer.results @@ -263,7 +263,7 @@ exports[`no enzyme tests`] = { "public/app/features/explore/RichHistory/RichHistory.test.tsx:409631018": [ [1, 17, 13, "RegExp match", "2409514259"] ], - "public/app/features/explore/RichHistory/RichHistoryCard.test.tsx:357697997": [ + "public/app/features/explore/RichHistory/RichHistoryCard.test.tsx:689438177": [ [1, 19, 13, "RegExp match", "2409514259"] ], "public/app/features/explore/RichHistory/RichHistoryContainer.test.tsx:396471778": [ diff --git a/public/app/core/history/RichHistoryLocalStorage.test.ts b/public/app/core/history/RichHistoryLocalStorage.test.ts index b4fb8823a2a..a7b18c317da 100644 --- a/public/app/core/history/RichHistoryLocalStorage.test.ts +++ b/public/app/core/history/RichHistoryLocalStorage.test.ts @@ -4,23 +4,47 @@ import { RichHistoryQuery } from '../../types'; import { DataQuery } from '@grafana/data'; import { afterEach, beforeEach } from '../../../test/lib/common'; import { RichHistoryStorageWarning } from './RichHistoryStorage'; +import { backendSrv } from '../services/backend_srv'; const key = 'grafana.explore.richHistory'; -const mockItem: RichHistoryQuery = { - ts: 2, +jest.mock('@grafana/runtime', () => ({ + ...(jest.requireActual('@grafana/runtime') as unknown as object), + getBackendSrv: () => backendSrv, + getDataSourceSrv: () => { + return { + getList: () => { + return [ + { uid: 'dev-test-uid', name: 'dev-test' }, + { uid: 'dev-test-2-uid', name: 'dev-test-2' }, + ]; + }, + }; + }, +})); + +interface MockQuery extends DataQuery { + query: string; +} + +const mockItem: RichHistoryQuery = { + id: '2', + createdAt: 2, starred: true, + datasourceUid: 'dev-test-uid', datasourceName: 'dev-test', comment: 'test', - queries: [{ refId: 'ref', query: 'query-test' } as DataQuery], + queries: [{ refId: 'ref', query: 'query-test' }], }; -const mockItem2: RichHistoryQuery = { - ts: 3, +const mockItem2: RichHistoryQuery = { + id: '3', + createdAt: 3, starred: true, + datasourceUid: 'dev-test-2-uid', datasourceName: 'dev-test-2', comment: 'test-2', - queries: [{ refId: 'ref-2', query: 'query-2' } as DataQuery], + queries: [{ refId: 'ref-2', query: 'query-2' }], }; describe('RichHistoryLocalStorage', () => { @@ -45,7 +69,7 @@ describe('RichHistoryLocalStorage', () => { it('should save query history to localStorage', async () => { await storage.addToRichHistory(mockItem); expect(store.exists(key)).toBeTruthy(); - expect(store.getObject(key)).toMatchObject([mockItem]); + expect(await storage.getRichHistory()).toMatchObject([mockItem]); }); it('should not save duplicated query to localStorage', async () => { @@ -54,24 +78,25 @@ describe('RichHistoryLocalStorage', () => { await expect(async () => { await storage.addToRichHistory(mockItem2); }).rejects.toThrow('Entry already exists'); - expect(store.getObject(key)).toMatchObject([mockItem2, mockItem]); + expect(await storage.getRichHistory()).toMatchObject([mockItem2, mockItem]); }); it('should update starred in localStorage', async () => { await storage.addToRichHistory(mockItem); - await storage.updateStarred(mockItem.ts, false); - expect(store.getObject(key)[0].starred).toEqual(false); + await storage.updateStarred(mockItem.id, false); + expect((await storage.getRichHistory())[0].starred).toEqual(false); }); it('should update comment in localStorage', async () => { await storage.addToRichHistory(mockItem); - await storage.updateComment(mockItem.ts, 'new comment'); - expect(store.getObject(key)[0].comment).toEqual('new comment'); + await storage.updateComment(mockItem.id, 'new comment'); + expect((await storage.getRichHistory())[0].comment).toEqual('new comment'); }); it('should delete query in localStorage', async () => { await storage.addToRichHistory(mockItem); - await storage.deleteRichHistory(mockItem.ts); + await storage.deleteRichHistory(mockItem.id); + expect(await storage.getRichHistory()).toEqual([]); expect(store.getObject(key)).toEqual([]); }); }); @@ -92,9 +117,9 @@ describe('RichHistoryLocalStorage', () => { expect(richHistory).toMatchObject([ mockItem, - { starred: true, ts: 0, queries: [] }, - { starred: true, ts: now, queries: [] }, - { starred: false, ts: now, queries: [] }, + { starred: true, createdAt: 0, queries: [] }, + { starred: true, createdAt: now, queries: [] }, + { starred: false, createdAt: now, queries: [] }, ]); }); @@ -119,7 +144,7 @@ describe('RichHistoryLocalStorage', () => { expect(history.filter((h) => !h.starred)).toHaveLength(notStarredItemsInHistory); store.setObject(key, history); - const warning = await storage.addToRichHistory(mockItem); + const { warning } = await storage.addToRichHistory(mockItem); expect(warning).toMatchObject({ type: RichHistoryStorageWarning.LimitExceeded, }); @@ -143,13 +168,22 @@ describe('RichHistoryLocalStorage', () => { describe('should load from localStorage data in old formats', () => { it('should load when queries are strings', async () => { - const oldHistoryItem = { - ...mockItem, - queries: ['test query 1', 'test query 2', 'test query 3'], - }; - store.setObject(key, [oldHistoryItem]); + store.setObject(key, [ + { + ts: 2, + starred: true, + datasourceName: 'dev-test', + comment: 'test', + queries: ['test query 1', 'test query 2', 'test query 3'], + }, + ]); const expectedHistoryItem = { - ...mockItem, + id: '2', + createdAt: 2, + starred: true, + datasourceUid: 'dev-test-uid', + datasourceName: 'dev-test', + comment: 'test', queries: [ { expr: 'test query 1', @@ -171,13 +205,22 @@ describe('RichHistoryLocalStorage', () => { }); it('should load when queries are json-encoded strings', async () => { - const oldHistoryItem = { - ...mockItem, - queries: ['{"refId":"A","key":"key1","metrics":[]}', '{"refId":"B","key":"key2","metrics":[]}'], - }; - store.setObject(key, [oldHistoryItem]); + store.setObject(key, [ + { + ts: 2, + starred: true, + datasourceName: 'dev-test', + comment: 'test', + queries: ['{"refId":"A","key":"key1","metrics":[]}', '{"refId":"B","key":"key2","metrics":[]}'], + }, + ]); const expectedHistoryItem = { - ...mockItem, + id: '2', + createdAt: 2, + starred: true, + datasourceUid: 'dev-test-uid', + datasourceName: 'dev-test', + comment: 'test', queries: [ { refId: 'A', diff --git a/public/app/core/history/RichHistoryLocalStorage.ts b/public/app/core/history/RichHistoryLocalStorage.ts index d0b1c0b4ccd..b4d05c0c931 100644 --- a/public/app/core/history/RichHistoryLocalStorage.ts +++ b/public/app/core/history/RichHistoryLocalStorage.ts @@ -2,12 +2,22 @@ import RichHistoryStorage, { RichHistoryServiceError, RichHistoryStorageWarning import { RichHistoryQuery } from '../../types'; import store from '../store'; import { DataQuery } from '@grafana/data'; -import { isEqual, omit } from 'lodash'; +import { find, isEqual, omit } from 'lodash'; import { createRetentionPeriodBoundary, RICH_HISTORY_SETTING_KEYS } from './richHistoryLocalStorageUtils'; +import { fromDTO, toDTO } from './localStorageConverter'; export const RICH_HISTORY_KEY = 'grafana.explore.richHistory'; export const MAX_HISTORY_ITEMS = 10000; +export type RichHistoryLocalStorageDTO = { + // works as an unique identifier + ts: number; + datasourceName: string; + starred: boolean; + comment: string; + queries: DataQuery[]; +}; + /** * Local storage implementation for Rich History. It keeps all entries in browser's local storage. */ @@ -16,21 +26,27 @@ export default class RichHistoryLocalStorage implements RichHistoryStorage { * Return all history entries, perform migration and clean up entries not matching retention policy. */ async getRichHistory() { - const richHistory: RichHistoryQuery[] = store.getObject(RICH_HISTORY_KEY, []); - const transformedRichHistory = migrateRichHistory(richHistory); - return transformedRichHistory; + return getRichHistoryDTOs().map(fromDTO); } - async addToRichHistory(richHistoryQuery: RichHistoryQuery) { - const richHistory = cleanUp(await this.getRichHistory()); + async addToRichHistory(newRichHistoryQuery: Omit) { + const ts = Date.now(); + const richHistoryQuery = { + id: ts.toString(), + createdAt: ts, + ...newRichHistoryQuery, + }; + + const newRichHistoryQueryDTO = toDTO(richHistoryQuery); + const currentRichHistoryDTOs = cleanUp(getRichHistoryDTOs()); /* Compare queries of a new query and last saved queries. If they are the same, (except selected properties, * which can be different) don't save it in rich history. */ - const newQueriesToCompare = richHistoryQuery.queries.map((q) => omit(q, ['key', 'refId'])); + const newQueriesToCompare = newRichHistoryQueryDTO.queries.map((q) => omit(q, ['key', 'refId'])); const lastQueriesToCompare = - richHistory.length > 0 && - richHistory[0].queries.map((q) => { + currentRichHistoryDTOs.length > 0 && + currentRichHistoryDTOs[0].queries.map((q) => { return omit(q, ['key', 'refId']); }); @@ -40,9 +56,9 @@ export default class RichHistoryLocalStorage implements RichHistoryStorage { throw error; } - const { queriesToKeep, limitExceeded } = checkLimits(richHistory); + const { queriesToKeep, limitExceeded } = checkLimits(currentRichHistoryDTOs); - const updatedHistory: RichHistoryQuery[] = [richHistoryQuery, ...queriesToKeep]; + const updatedHistory: RichHistoryLocalStorageDTO[] = [newRichHistoryQueryDTO, ...queriesToKeep]; try { store.setObject(RICH_HISTORY_KEY, updatedHistory); @@ -56,52 +72,59 @@ export default class RichHistoryLocalStorage implements RichHistoryStorage { if (limitExceeded) { return { - type: RichHistoryStorageWarning.LimitExceeded, - message: `Query history reached the limit of ${MAX_HISTORY_ITEMS}. Old, not-starred items have been removed.`, + warning: { + type: RichHistoryStorageWarning.LimitExceeded, + message: `Query history reached the limit of ${MAX_HISTORY_ITEMS}. Old, not-starred items have been removed.`, + }, + richHistoryQuery, }; } - return undefined; + return { richHistoryQuery }; } async deleteAll() { store.delete(RICH_HISTORY_KEY); } - async deleteRichHistory(id: number) { - const richHistory: RichHistoryQuery[] = store.getObject(RICH_HISTORY_KEY, []); - const updatedHistory = richHistory.filter((query) => query.ts !== id); + async deleteRichHistory(id: string) { + const ts = parseInt(id, 10); + const richHistory: RichHistoryLocalStorageDTO[] = store.getObject(RICH_HISTORY_KEY, []); + const updatedHistory = richHistory.filter((query) => query.ts !== ts); store.setObject(RICH_HISTORY_KEY, updatedHistory); } - async updateStarred(id: number, starred: boolean) { - const richHistory: RichHistoryQuery[] = store.getObject(RICH_HISTORY_KEY, []); - const updatedHistory = richHistory.map((query) => { - if (query.ts === id) { - query.starred = starred; - } - return query; - }); + async updateStarred(id: string, starred: boolean) { + return updateRichHistory(id, (richHistoryDTO) => (richHistoryDTO.starred = starred)); + } - store.setObject(RICH_HISTORY_KEY, updatedHistory); + async updateComment(id: string, comment: string) { + return updateRichHistory(id, (richHistoryDTO) => (richHistoryDTO.comment = comment)); } +} - async updateComment(id: number, comment: string) { - const richHistory: RichHistoryQuery[] = store.getObject(RICH_HISTORY_KEY, []); - const updatedHistory = richHistory.map((query) => { - if (query.ts === id) { - query.comment = comment; - } - return query; - }); - store.setObject(RICH_HISTORY_KEY, updatedHistory); +function updateRichHistory( + id: string, + updateCallback: (richHistoryDTO: RichHistoryLocalStorageDTO) => void +): RichHistoryQuery { + const ts = parseInt(id, 10); + const richHistoryDTOs: RichHistoryLocalStorageDTO[] = store.getObject(RICH_HISTORY_KEY, []); + const richHistoryDTO = find(richHistoryDTOs, { ts }); + + if (!richHistoryDTO) { + throw new Error('Rich history item not found.'); } + + updateCallback(richHistoryDTO); + + store.setObject(RICH_HISTORY_KEY, richHistoryDTOs); + return fromDTO(richHistoryDTO); } /** * Removes entries that do not match retention policy criteria. */ -function cleanUp(richHistory: RichHistoryQuery[]): RichHistoryQuery[] { +function cleanUp(richHistory: RichHistoryLocalStorageDTO[]): RichHistoryLocalStorageDTO[] { const retentionPeriod: number = store.getObject(RICH_HISTORY_SETTING_KEYS.retentionPeriod, 7); const retentionPeriodLastTs = createRetentionPeriodBoundary(retentionPeriod, false); @@ -115,7 +138,10 @@ function cleanUp(richHistory: RichHistoryQuery[]): RichHistoryQuery[] { * Ensures the entry can be added. Throws an error if current limit has been hit. * Returns queries that should be saved back giving space for one extra query. */ -function checkLimits(queriesToKeep: RichHistoryQuery[]): { queriesToKeep: RichHistoryQuery[]; limitExceeded: boolean } { +function checkLimits(queriesToKeep: RichHistoryLocalStorageDTO[]): { + queriesToKeep: RichHistoryLocalStorageDTO[]; + limitExceeded: boolean; +} { // remove oldest non-starred items to give space for the recent query let limitExceeded = false; let current = queriesToKeep.length - 1; @@ -130,7 +156,12 @@ function checkLimits(queriesToKeep: RichHistoryQuery[]): { queriesToKeep: RichHi return { queriesToKeep, limitExceeded }; } -function migrateRichHistory(richHistory: RichHistoryQuery[]) { +function getRichHistoryDTOs(): RichHistoryLocalStorageDTO[] { + const richHistory: RichHistoryLocalStorageDTO[] = store.getObject(RICH_HISTORY_KEY, []); + return migrateRichHistory(richHistory); +} + +function migrateRichHistory(richHistory: RichHistoryLocalStorageDTO[]): RichHistoryLocalStorageDTO[] { const transformedRichHistory = richHistory.map((query) => { const transformedQueries: DataQuery[] = query.queries.map((q, index) => createDataQuery(query, q, index)); return { ...query, queries: transformedQueries }; @@ -139,7 +170,7 @@ function migrateRichHistory(richHistory: RichHistoryQuery[]) { return transformedRichHistory; } -function createDataQuery(query: RichHistoryQuery, individualQuery: DataQuery | string, index: number) { +function createDataQuery(query: RichHistoryLocalStorageDTO, individualQuery: DataQuery | string, index: number) { const letters = 'ABCDEFGHIJKLMNOPQRSTUVXYZ'; if (typeof individualQuery === 'object') { // the current format diff --git a/public/app/core/history/RichHistoryStorage.ts b/public/app/core/history/RichHistoryStorage.ts index 4254be0eee4..4242e13658f 100644 --- a/public/app/core/history/RichHistoryStorage.ts +++ b/public/app/core/history/RichHistoryStorage.ts @@ -32,9 +32,16 @@ export type RichHistoryStorageWarningDetails = { */ export default interface RichHistoryStorage { getRichHistory(): Promise; - addToRichHistory(richHistoryQuery: RichHistoryQuery): Promise; + + /** + * Creates new RichHistoryQuery, returns object with unique id and created date + */ + addToRichHistory( + newRichHistoryQuery: Omit + ): Promise<{ warning?: RichHistoryStorageWarningDetails; richHistoryQuery: RichHistoryQuery }>; + deleteAll(): Promise; - deleteRichHistory(id: number): Promise; - updateStarred(id: number, starred: boolean): Promise; - updateComment(id: number, comment: string | undefined): Promise; + deleteRichHistory(id: string): Promise; + updateStarred(id: string, starred: boolean): Promise; + updateComment(id: string, comment: string | undefined): Promise; } diff --git a/public/app/core/history/localStorageConverter.test.ts b/public/app/core/history/localStorageConverter.test.ts new file mode 100644 index 00000000000..5a18d27747e --- /dev/null +++ b/public/app/core/history/localStorageConverter.test.ts @@ -0,0 +1,60 @@ +import { backendSrv } from '../services/backend_srv'; +import { fromDTO, toDTO } from './localStorageConverter'; +import { RichHistoryQuery } from '../../types'; +import { RichHistoryLocalStorageDTO } from './RichHistoryLocalStorage'; + +jest.mock('@grafana/runtime', () => ({ + ...(jest.requireActual('@grafana/runtime') as unknown as object), + getBackendSrv: () => backendSrv, + getDataSourceSrv: () => { + return { + getList: () => { + return [{ uid: 'uid', name: 'dev-test' }]; + }, + }; + }, +})); + +const validRichHistory: RichHistoryQuery = { + comment: 'comment', + createdAt: 1, + datasourceName: 'dev-test', + datasourceUid: 'uid', + id: '1', + queries: [{ refId: 'A' }], + starred: true, +}; + +const validDTO: RichHistoryLocalStorageDTO = { + comment: 'comment', + datasourceName: 'dev-test', + queries: [{ refId: 'A' }], + starred: true, + ts: 1, +}; + +describe('LocalStorage converted', () => { + it('converts RichHistoryQuery to local storage DTO', () => { + expect(toDTO(validRichHistory)).toMatchObject(validDTO); + }); + + it('throws an error when data source for RichHistory does not exist to avoid saving invalid items', () => { + const invalidRichHistory = { ...validRichHistory, datasourceUid: 'invalid' }; + expect(() => { + toDTO(invalidRichHistory); + }).toThrow(); + }); + + it('converts DTO to RichHistoryQuery', () => { + expect(fromDTO(validDTO)).toMatchObject(validRichHistory); + }); + + it('uses empty uid when datasource does not exist for a DTO to fail gracefully for queries from removed datasources', () => { + const invalidDto = { ...validDTO, datasourceName: 'removed' }; + expect(fromDTO(invalidDto)).toMatchObject({ + ...validRichHistory, + datasourceName: 'removed', + datasourceUid: '', + }); + }); +}); diff --git a/public/app/core/history/localStorageConverter.ts b/public/app/core/history/localStorageConverter.ts new file mode 100644 index 00000000000..e8569969449 --- /dev/null +++ b/public/app/core/history/localStorageConverter.ts @@ -0,0 +1,41 @@ +import { find } from 'lodash'; +import { DataSourceInstanceSettings } from '@grafana/data'; +import { getDataSourceSrv } from '@grafana/runtime'; +import { RichHistoryLocalStorageDTO } from './RichHistoryLocalStorage'; +import { RichHistoryQuery } from '../../types'; + +export const fromDTO = (dto: RichHistoryLocalStorageDTO): RichHistoryQuery => { + const datasource = find( + getDataSourceSrv().getList(), + (settings: DataSourceInstanceSettings) => settings.name === dto.datasourceName + ); + + return { + id: dto.ts.toString(), + createdAt: dto.ts, + datasourceName: dto.datasourceName, + datasourceUid: datasource?.uid || '', // will be show on the list as coming from a removed data source + starred: dto.starred, + comment: dto.comment, + queries: dto.queries, + }; +}; + +export const toDTO = (richHistoryQuery: RichHistoryQuery): RichHistoryLocalStorageDTO => { + const datasource = find( + getDataSourceSrv().getList(), + (settings: DataSourceInstanceSettings) => settings.uid === richHistoryQuery.datasourceUid + ); + + if (!datasource) { + throw new Error('Datasource not found.'); + } + + return { + ts: richHistoryQuery.createdAt, + datasourceName: richHistoryQuery.datasourceName, + starred: richHistoryQuery.starred, + comment: richHistoryQuery.comment, + queries: richHistoryQuery.queries, + }; +}; diff --git a/public/app/core/history/richHistoryLocalStorageUtils.ts b/public/app/core/history/richHistoryLocalStorageUtils.ts index a7c9a1de442..67b644f496c 100644 --- a/public/app/core/history/richHistoryLocalStorageUtils.ts +++ b/public/app/core/history/richHistoryLocalStorageUtils.ts @@ -23,7 +23,7 @@ export const createRetentionPeriodBoundary = (days: number, isLastTs: boolean) = export function filterQueriesByTime(queries: RichHistoryQuery[], timeFilter: [number, number]) { const filter1 = createRetentionPeriodBoundary(timeFilter[0], true); // probably the vars should have a different name const filter2 = createRetentionPeriodBoundary(timeFilter[1], false); - return queries.filter((q) => q.ts < filter1 && q.ts > filter2); + return queries.filter((q) => q.createdAt < filter1 && q.createdAt > filter2); } export function filterQueriesByDataSource(queries: RichHistoryQuery[], listOfDatasourceFilters: string[]) { @@ -53,10 +53,12 @@ export const sortQueries = (array: RichHistoryQuery[], sortOrder: SortOrder) => let sortFunc; if (sortOrder === SortOrder.Ascending) { - sortFunc = (a: RichHistoryQuery, b: RichHistoryQuery) => (a.ts < b.ts ? -1 : a.ts > b.ts ? 1 : 0); + sortFunc = (a: RichHistoryQuery, b: RichHistoryQuery) => + a.createdAt < b.createdAt ? -1 : a.createdAt > b.createdAt ? 1 : 0; } if (sortOrder === SortOrder.Descending) { - sortFunc = (a: RichHistoryQuery, b: RichHistoryQuery) => (a.ts < b.ts ? 1 : a.ts > b.ts ? -1 : 0); + sortFunc = (a: RichHistoryQuery, b: RichHistoryQuery) => + a.createdAt < b.createdAt ? 1 : a.createdAt > b.createdAt ? -1 : 0; } if (sortOrder === SortOrder.DatasourceZA) { diff --git a/public/app/core/utils/richHistory.test.ts b/public/app/core/utils/richHistory.test.ts index eb12d22f46c..e89d3860e15 100644 --- a/public/app/core/utils/richHistory.test.ts +++ b/public/app/core/utils/richHistory.test.ts @@ -25,6 +25,8 @@ jest.mock('../history/richHistoryStorageProvider', () => { const mock: any = { storedHistory: [ { + id: '1', + createdAt: 1, comment: '', datasourceName: 'datasource history name', queries: [ @@ -32,11 +34,10 @@ const mock: any = { { expr: 'query2', refId: '2' }, ], starred: true, - ts: 1, }, ], testComment: '', - testDatasourceId: 'datasourceId', + testDatasourceUid: 'datasourceUid', testDatasourceName: 'datasourceName', testQueries: [ { expr: 'query3', refId: 'B' }, @@ -53,12 +54,24 @@ describe('richHistory', () => { jest.useFakeTimers('modern'); jest.setSystemTime(new Date(1970, 0, 1)); - richHistoryStorageMock.addToRichHistory = jest.fn().mockResolvedValue(undefined); + richHistoryStorageMock.addToRichHistory = jest.fn((r) => { + return Promise.resolve({ richHistoryQuery: { ...r, id: 'GENERATED ID', createdAt: Date.now() } }); + }); richHistoryStorageMock.deleteAll = jest.fn().mockResolvedValue({}); richHistoryStorageMock.deleteRichHistory = jest.fn().mockResolvedValue({}); richHistoryStorageMock.getRichHistory = jest.fn().mockResolvedValue({}); - richHistoryStorageMock.updateComment = jest.fn().mockResolvedValue({}); - richHistoryStorageMock.updateStarred = jest.fn().mockResolvedValue({}); + richHistoryStorageMock.updateComment = jest.fn((id, comment) => { + return { + ...mock, + comment, + }; + }); + richHistoryStorageMock.updateStarred = jest.fn((id, starred) => { + return { + ...mock, + starred, + }; + }); }); afterEach(() => { @@ -73,10 +86,12 @@ describe('richHistory', () => { const expectedResult = [ { comment: mock.testComment, + datasourceUid: mock.testDatasourceUid, datasourceName: mock.testDatasourceName, queries: mock.testQueries, starred: mock.testStarred, - ts: 2, + createdAt: 2, + id: 'GENERATED ID', }, mock.storedHistory[0], ]; @@ -85,6 +100,7 @@ describe('richHistory', () => { Date.now = jest.fn(() => 2); const { richHistory: newHistory } = await addToRichHistory( mock.storedHistory, + mock.testDatasourceUid, mock.testDatasourceName, mock.testQueries, mock.testStarred, @@ -100,6 +116,7 @@ describe('richHistory', () => { const { richHistory } = await addToRichHistory( mock.storedHistory, + mock.testDatasourceUid, mock.testDatasourceName, mock.testQueries, mock.testStarred, @@ -109,11 +126,11 @@ describe('richHistory', () => { ); expect(richHistory).toMatchObject(expectedResult); expect(richHistoryStorageMock.addToRichHistory).toBeCalledWith({ + datasourceUid: mock.testDatasourceUid, datasourceName: mock.testDatasourceName, starred: mock.testStarred, comment: mock.testComment, queries: mock.testQueries, - ts: 2, }); }); @@ -126,6 +143,7 @@ describe('richHistory', () => { const { richHistory: newHistory } = await addToRichHistory( mock.storedHistory, + mock.storedHistory[0].datasourceUid, mock.storedHistory[0].datasourceName, [{ expr: 'query1', maxLines: null, refId: 'A' } as DataQuery, { expr: 'query2', refId: 'B' } as DataQuery], mock.testStarred, @@ -139,13 +157,19 @@ describe('richHistory', () => { it('it should append new items even when the limit is exceeded', async () => { Date.now = jest.fn(() => 2); - richHistoryStorageMock.addToRichHistory = jest.fn().mockReturnValue({ - type: RichHistoryStorageWarning.LimitExceeded, - message: 'Limit exceeded', + richHistoryStorageMock.addToRichHistory = jest.fn((query) => { + return Promise.resolve({ + richHistoryQuery: { ...query, id: 'GENERATED ID', createdAt: Date.now() }, + warning: { + type: RichHistoryStorageWarning.LimitExceeded, + message: 'Limit exceeded', + }, + }); }); const { richHistory, limitExceeded } = await addToRichHistory( mock.storedHistory, + mock.testDatasourceUid, mock.testDatasourceName, mock.testQueries, mock.testStarred, @@ -160,21 +184,21 @@ describe('richHistory', () => { describe('updateStarredInRichHistory', () => { it('should update starred in query in history', async () => { - const updatedStarred = await updateStarredInRichHistory(mock.storedHistory, 1); - expect(updatedStarred[0].starred).toEqual(false); + const updatedStarred = await updateStarredInRichHistory(mock.storedHistory, '1', !mock.starred); + expect(updatedStarred[0].starred).toEqual(!mock.starred); }); }); describe('updateCommentInRichHistory', () => { it('should update comment in query in history', async () => { - const updatedComment = await updateCommentInRichHistory(mock.storedHistory, 1, 'new comment'); + const updatedComment = await updateCommentInRichHistory(mock.storedHistory, '1', 'new comment'); expect(updatedComment[0].comment).toEqual('new comment'); }); }); describe('deleteQueryInRichHistory', () => { it('should delete query in query in history', async () => { - const deletedHistory = await deleteQueryInRichHistory(mock.storedHistory, 1); + const deletedHistory = await deleteQueryInRichHistory(mock.storedHistory, '1'); expect(deletedHistory).toEqual([]); }); }); @@ -230,7 +254,7 @@ describe('richHistory', () => { describe('createQueryHeading', () => { it('should correctly create heading for queries when sort order is ascending ', () => { // Have to offset the timezone of a 1 microsecond epoch, and then reverse the changes - mock.storedHistory[0].ts = 1 + -1 * dateTime().utcOffset() * 60 * 1000; + mock.storedHistory[0].createdAt = 1 + -1 * dateTime().utcOffset() * 60 * 1000; const heading = createQueryHeading(mock.storedHistory[0], SortOrder.Ascending); expect(heading).toEqual('January 1'); }); diff --git a/public/app/core/utils/richHistory.ts b/public/app/core/utils/richHistory.ts index c2757d28c94..567fbd97005 100644 --- a/public/app/core/utils/richHistory.ts +++ b/public/app/core/utils/richHistory.ts @@ -34,6 +34,7 @@ export { SortOrder }; export async function addToRichHistory( richHistory: RichHistoryQuery[], + datasourceUid: string, datasourceName: string | null, queries: DataQuery[], starred: boolean, @@ -41,25 +42,25 @@ export async function addToRichHistory( showQuotaExceededError: boolean, showLimitExceededWarning: boolean ): Promise<{ richHistory: RichHistoryQuery[]; richHistoryStorageFull?: boolean; limitExceeded?: boolean }> { - const ts = Date.now(); /* Save only queries, that are not falsy (e.g. empty object, null, ...) */ const newQueriesToSave: DataQuery[] = queries && queries.filter((query) => notEmptyQuery(query)); if (newQueriesToSave.length > 0) { - const newRichHistory: RichHistoryQuery = { - queries: newQueriesToSave, - ts, - datasourceName: datasourceName ?? '', - starred, - comment: comment ?? '', - }; - let richHistoryStorageFull = false; let limitExceeded = false; let warning: RichHistoryStorageWarningDetails | undefined; + let newRichHistory: RichHistoryQuery; try { - warning = await getRichHistoryStorage().addToRichHistory(newRichHistory); + const result = await getRichHistoryStorage().addToRichHistory({ + datasourceUid: datasourceUid, + datasourceName: datasourceName ?? '', + queries: newQueriesToSave, + starred, + comment: comment ?? '', + }); + warning = result.warning; + newRichHistory = result.richHistoryQuery; } catch (error) { if (error.name === RichHistoryServiceError.StorageFull) { richHistoryStorageFull = true; @@ -93,26 +94,10 @@ export async function deleteAllFromRichHistory(): Promise { return getRichHistoryStorage().deleteAll(); } -export async function updateStarredInRichHistory(richHistory: RichHistoryQuery[], ts: number) { - let updatedQuery: RichHistoryQuery | undefined; - - const updatedHistory = richHistory.map((query) => { - /* Timestamps are currently unique - we can use them to identify specific queries */ - if (query.ts === ts) { - const isStarred = query.starred; - updatedQuery = Object.assign({}, query, { starred: !isStarred }); - return updatedQuery; - } - return query; - }); - - if (!updatedQuery) { - return richHistory; - } - +export async function updateStarredInRichHistory(richHistory: RichHistoryQuery[], id: string, starred: boolean) { try { - await getRichHistoryStorage().updateStarred(ts, updatedQuery.starred); - return updatedHistory; + const updatedQuery = await getRichHistoryStorage().updateStarred(id, starred); + return richHistory.map((query) => (query.id === id ? updatedQuery : query)); } catch (error) { dispatch(notifyApp(createErrorNotification('Saving rich history failed', error.message))); return richHistory; @@ -121,25 +106,12 @@ export async function updateStarredInRichHistory(richHistory: RichHistoryQuery[] export async function updateCommentInRichHistory( richHistory: RichHistoryQuery[], - ts: number, + id: string, newComment: string | undefined ) { - let updatedQuery: RichHistoryQuery | undefined; - const updatedHistory = richHistory.map((query) => { - if (query.ts === ts) { - updatedQuery = Object.assign({}, query, { comment: newComment }); - return updatedQuery; - } - return query; - }); - - if (!updatedQuery) { - return richHistory; - } - try { - await getRichHistoryStorage().updateComment(ts, newComment); - return updatedHistory; + const updatedQuery = await getRichHistoryStorage().updateComment(id, newComment); + return richHistory.map((query) => (query.id === id ? updatedQuery : query)); } catch (error) { dispatch(notifyApp(createErrorNotification('Saving rich history failed', error.message))); return richHistory; @@ -148,11 +120,11 @@ export async function updateCommentInRichHistory( export async function deleteQueryInRichHistory( richHistory: RichHistoryQuery[], - ts: number + id: string ): Promise { - const updatedHistory = richHistory.filter((query) => query.ts !== ts); + const updatedHistory = richHistory.filter((query) => query.id !== id); try { - await getRichHistoryStorage().deleteRichHistory(ts); + await getRichHistoryStorage().deleteRichHistory(id); return updatedHistory; } catch (error) { dispatch(notifyApp(createErrorNotification('Saving rich history failed', error.message))); @@ -234,7 +206,7 @@ export function createQueryHeading(query: RichHistoryQuery, sortOrder: SortOrder if (sortOrder === SortOrder.DatasourceAZ || sortOrder === SortOrder.DatasourceZA) { heading = query.datasourceName; } else { - heading = createDateStringFromTs(query.ts); + heading = createDateStringFromTs(query.createdAt); } return heading; } diff --git a/public/app/features/explore/RichHistory/RichHistoryCard.test.tsx b/public/app/features/explore/RichHistory/RichHistoryCard.test.tsx index 80210bca91c..004d5983c62 100644 --- a/public/app/features/explore/RichHistory/RichHistoryCard.test.tsx +++ b/public/app/features/explore/RichHistory/RichHistoryCard.test.tsx @@ -1,26 +1,36 @@ import React from 'react'; import { shallow } from 'enzyme'; import { RichHistoryCard, Props } from './RichHistoryCard'; -import { ExploreId } from '../../../types/explore'; +import { ExploreId, RichHistoryQuery } from '../../../types/explore'; import { DataSourceApi, DataQuery } from '@grafana/data'; -const setup = (propOverrides?: Partial) => { - const props: Props = { +const starRichHistoryMock = jest.fn(); + +interface MockQuery extends DataQuery { + query: string; +} + +const setup = (propOverrides?: Partial>) => { + const props: Props = { query: { - ts: 1, + id: '1', + createdAt: 1, + datasourceUid: 'Test datasource uid', datasourceName: 'Test datasource', starred: false, comment: '', queries: [ - { expr: 'query1', refId: 'A' } as DataQuery, - { expr: 'query2', refId: 'B' } as DataQuery, - { expr: 'query3', refId: 'C' } as DataQuery, + { query: 'query1', refId: 'A' }, + { query: 'query2', refId: 'B' }, + { query: 'query3', refId: 'C' }, ], }, dsImg: '/app/img', isRemoved: false, changeDatasource: jest.fn(), - updateRichHistory: jest.fn(), + starHistoryItem: starRichHistoryMock, + commentHistoryItem: jest.fn(), + deleteHistoryItem: jest.fn(), setQueries: jest.fn(), exploreId: ExploreId.left, datasourceInstance: { name: 'Datasource' } as DataSourceApi, @@ -32,8 +42,10 @@ const setup = (propOverrides?: Partial) => { return wrapper; }; -const starredQueryWithComment = { - ts: 1, +const starredQueryWithComment: RichHistoryQuery = { + id: '1', + createdAt: 1, + datasourceUid: 'Test datasource uid', datasourceName: 'Test datasource', starred: true, comment: 'test comment', @@ -48,9 +60,9 @@ describe('RichHistoryCard', () => { it('should render all queries', () => { const wrapper = setup(); expect(wrapper.find({ 'aria-label': 'Query text' })).toHaveLength(3); - expect(wrapper.find({ 'aria-label': 'Query text' }).at(0).text()).toEqual('{"expr":"query1"}'); - expect(wrapper.find({ 'aria-label': 'Query text' }).at(1).text()).toEqual('{"expr":"query2"}'); - expect(wrapper.find({ 'aria-label': 'Query text' }).at(2).text()).toEqual('{"expr":"query3"}'); + expect(wrapper.find({ 'aria-label': 'Query text' }).at(0).text()).toEqual('{"query":"query1"}'); + expect(wrapper.find({ 'aria-label': 'Query text' }).at(1).text()).toEqual('{"query":"query2"}'); + expect(wrapper.find({ 'aria-label': 'Query text' }).at(2).text()).toEqual('{"query":"query3"}'); }); it('should render data source icon', () => { const wrapper = setup(); @@ -120,11 +132,17 @@ describe('RichHistoryCard', () => { describe('starring', () => { it('should have title "Star query", if not starred', () => { const wrapper = setup(); - expect(wrapper.find({ title: 'Star query' })).toHaveLength(1); + const starButton = wrapper.find({ title: 'Star query' }); + expect(starButton).toHaveLength(1); + starButton.simulate('click'); + expect(starRichHistoryMock).toBeCalledWith(starredQueryWithComment.id, true); }); it('should have title "Unstar query", if not starred', () => { const wrapper = setup({ query: starredQueryWithComment }); - expect(wrapper.find({ title: 'Unstar query' })).toHaveLength(1); + const starButton = wrapper.find({ title: 'Unstar query' }); + expect(starButton).toHaveLength(1); + starButton.simulate('click'); + expect(starRichHistoryMock).toBeCalledWith(starredQueryWithComment.id, false); }); }); }); diff --git a/public/app/features/explore/RichHistory/RichHistoryCard.tsx b/public/app/features/explore/RichHistory/RichHistoryCard.tsx index f6ae378ca20..61120641229 100644 --- a/public/app/features/explore/RichHistory/RichHistoryCard.tsx +++ b/public/app/features/explore/RichHistory/RichHistoryCard.tsx @@ -3,7 +3,7 @@ import { connect, ConnectedProps } from 'react-redux'; import { css, cx } from '@emotion/css'; import { stylesFactory, useTheme, TextArea, Button, IconButton } from '@grafana/ui'; import { getDataSourceSrv } from '@grafana/runtime'; -import { GrafanaTheme, DataSourceApi } from '@grafana/data'; +import { GrafanaTheme, DataSourceApi, DataQuery } from '@grafana/data'; import { RichHistoryQuery, ExploreId } from 'app/types/explore'; import { createUrlFromRichHistory, createQueryText } from 'app/core/utils/richHistory'; import { createAndCopyShortLink } from 'app/core/utils/shortLinks'; @@ -14,7 +14,7 @@ import { notifyApp } from 'app/core/actions'; import { createSuccessNotification } from 'app/core/copy/appNotification'; import { StoreState } from 'app/types'; -import { updateRichHistory } from '../state/history'; +import { starHistoryItem, commentHistoryItem, deleteHistoryItem } from '../state/history'; import { changeDatasource } from '../state/datasource'; import { setQueries } from '../state/query'; import { ShowConfirmModalEvent } from '../../../types/events'; @@ -30,19 +30,21 @@ function mapStateToProps(state: StoreState, { exploreId }: { exploreId: ExploreI const mapDispatchToProps = { changeDatasource, - updateRichHistory, + deleteHistoryItem, + commentHistoryItem, + starHistoryItem, setQueries, }; const connector = connect(mapStateToProps, mapDispatchToProps); -interface OwnProps { - query: RichHistoryQuery; +interface OwnProps { + query: RichHistoryQuery; dsImg: string; isRemoved: boolean; } -export type Props = ConnectedProps & OwnProps; +export type Props = ConnectedProps & OwnProps; const getStyles = stylesFactory((theme: GrafanaTheme, isRemoved: boolean) => { /* Hard-coded value so all buttons and icons on right side of card are aligned */ @@ -143,8 +145,18 @@ const getStyles = stylesFactory((theme: GrafanaTheme, isRemoved: boolean) => { }); export function RichHistoryCard(props: Props) { - const { query, dsImg, isRemoved, updateRichHistory, changeDatasource, exploreId, datasourceInstance, setQueries } = - props; + const { + query, + dsImg, + isRemoved, + commentHistoryItem, + starHistoryItem, + deleteHistoryItem, + changeDatasource, + exploreId, + datasourceInstance, + setQueries, + } = props; const [activeUpdateComment, setActiveUpdateComment] = useState(false); const [comment, setComment] = useState(query.comment); const [queryDsInstance, setQueryDsInstance] = useState(undefined); @@ -192,25 +204,25 @@ export function RichHistoryCard(props: Props) { yesText: 'Delete', icon: 'trash-alt', onConfirm: () => { - updateRichHistory(query.ts, 'delete'); + deleteHistoryItem(query.id); dispatch(notifyApp(createSuccessNotification('Query deleted'))); }, }) ); } else { - updateRichHistory(query.ts, 'delete'); + deleteHistoryItem(query.id); dispatch(notifyApp(createSuccessNotification('Query deleted'))); } }; const onStarrQuery = () => { - updateRichHistory(query.ts, 'starred'); + starHistoryItem(query.id, !query.starred); }; const toggleActiveUpdateComment = () => setActiveUpdateComment(!activeUpdateComment); const onUpdateComment = () => { - updateRichHistory(query.ts, 'comment', comment); + commentHistoryItem(query.id, comment); setActiveUpdateComment(false); }; diff --git a/public/app/features/explore/RichHistory/RichHistoryQueriesTab.tsx b/public/app/features/explore/RichHistory/RichHistoryQueriesTab.tsx index 68708b05445..44fd55e42c3 100644 --- a/public/app/features/explore/RichHistory/RichHistoryQueriesTab.tsx +++ b/public/app/features/explore/RichHistory/RichHistoryQueriesTab.tsx @@ -240,7 +240,7 @@ export function RichHistoryQueriesTab(props: Props) { return ( ('explore // Action creators // -export const updateRichHistory = (ts: number, property: string, updatedProperty?: string): ThunkResult => { +export const starHistoryItem = (id: string, starred: boolean): ThunkResult => { return async (dispatch, getState) => { - // Side-effect: Saving rich history in localstorage - let nextRichHistory; - if (property === 'starred') { - nextRichHistory = await updateStarredInRichHistory(getState().explore.richHistory, ts); - } - if (property === 'comment') { - nextRichHistory = await updateCommentInRichHistory(getState().explore.richHistory, ts, updatedProperty); - } - if (property === 'delete') { - nextRichHistory = await deleteQueryInRichHistory(getState().explore.richHistory, ts); - } + const nextRichHistory = await updateStarredInRichHistory(getState().explore.richHistory, id, starred); + dispatch(richHistoryUpdatedAction({ richHistory: nextRichHistory })); + }; +}; + +export const commentHistoryItem = (id: string, comment?: string): ThunkResult => { + return async (dispatch, getState) => { + const nextRichHistory = await updateCommentInRichHistory(getState().explore.richHistory, id, comment); + dispatch(richHistoryUpdatedAction({ richHistory: nextRichHistory })); + }; +}; + +export const deleteHistoryItem = (id: string): ThunkResult => { + return async (dispatch, getState) => { + const nextRichHistory = await deleteQueryInRichHistory(getState().explore.richHistory, id); dispatch(richHistoryUpdatedAction({ richHistory: nextRichHistory })); }; }; diff --git a/public/app/features/explore/state/query.ts b/public/app/features/explore/state/query.ts index 372a9304319..9825a6a1dfc 100644 --- a/public/app/features/explore/state/query.ts +++ b/public/app/features/explore/state/query.ts @@ -326,6 +326,7 @@ async function handleHistory( limitExceeded, } = await addToRichHistory( state.richHistory || [], + datasource.uid, datasource.name, queries, false, diff --git a/public/app/types/explore.ts b/public/app/types/explore.ts index 66952a2ec9a..9bc5a2a5c3d 100644 --- a/public/app/types/explore.ts +++ b/public/app/types/explore.ts @@ -195,12 +195,14 @@ export interface QueryTransaction { scanning?: boolean; } -export type RichHistoryQuery = { - ts: number; +export type RichHistoryQuery = { + id: string; + createdAt: number; + datasourceUid: string; datasourceName: string; starred: boolean; comment: string; - queries: DataQuery[]; + queries: T[]; }; export interface ExplorePanelData extends PanelData {