From 88ec750728d1991ac69f089a117c359c38d7f802 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Jamr=C3=B3z?= Date: Wed, 6 Apr 2022 13:49:25 +0200 Subject: [PATCH] Query History: Load history when QueryHistoryContainer is opened (#46457) * Load Rich History when the container is opened * Store rich history for each pane separately * Do not update currently opened query history when an item is added It's impossible to figure out if the item should be added or not, because filters are applied in the backend. We don't want to replicate that filtering logic in frontend. One way to make it work could be by refreshing both panes. * Test starring and deleting query history items when both panes are open * Remove e2e dependency on ExploreId * Fix unit test * Assert exact queries * Simplify test * Fix e2e tests * Fix toolbar a11y * Reload the history after an item is added * Fix unit test * Remove references to Explore from generic PageToolbar component * Update test name * Fix test assertion * Add issue item to TODO * Improve test assertion * Simplify test setup --- .betterer.results | 3 - .../src/components/PageLayout/PageToolbar.tsx | 20 ++- public/app/core/utils/richHistory.test.ts | 135 ++++++------------ public/app/core/utils/richHistory.ts | 42 ++---- .../app/features/explore/ExploreToolbar.tsx | 1 + .../app/features/explore/QueryRows.test.tsx | 2 +- .../RichHistory/RichHistoryContainer.test.tsx | 30 ++-- .../RichHistory/RichHistoryContainer.tsx | 22 ++- public/app/features/explore/Wrapper.test.tsx | 7 +- public/app/features/explore/Wrapper.tsx | 5 - .../features/explore/spec/helper/assert.ts | 33 ++++- .../explore/spec/helper/interactions.ts | 36 +++-- .../features/explore/spec/helper/setup.tsx | 18 ++- .../explore/spec/queryHistory.test.tsx | 67 ++++++++- .../app/features/explore/state/explorePane.ts | 11 +- public/app/features/explore/state/helpers.ts | 1 + public/app/features/explore/state/history.ts | 84 +++++++++-- public/app/features/explore/state/main.ts | 13 +- public/app/features/explore/state/query.ts | 37 ++--- public/app/features/explore/state/utils.ts | 1 + public/app/types/explore.ts | 9 +- 21 files changed, 342 insertions(+), 235 deletions(-) diff --git a/.betterer.results b/.betterer.results index a354be90586..03900ca860c 100644 --- a/.betterer.results +++ b/.betterer.results @@ -236,9 +236,6 @@ exports[`no enzyme tests`] = { "public/app/features/explore/RichHistory/RichHistoryCard.test.tsx:689438177": [ [1, 19, 13, "RegExp match", "2409514259"] ], - "public/app/features/explore/RichHistory/RichHistoryContainer.test.tsx:396471778": [ - [1, 17, 13, "RegExp match", "2409514259"] - ], "public/app/features/explore/RichHistory/RichHistoryQueriesTab.test.tsx:3436519226": [ [1, 17, 13, "RegExp match", "2409514259"] ], diff --git a/packages/grafana-ui/src/components/PageLayout/PageToolbar.tsx b/packages/grafana-ui/src/components/PageLayout/PageToolbar.tsx index b13d341c15a..07c02c3bc65 100644 --- a/packages/grafana-ui/src/components/PageLayout/PageToolbar.tsx +++ b/packages/grafana-ui/src/components/PageLayout/PageToolbar.tsx @@ -21,11 +21,25 @@ export interface Props { children?: ReactNode; className?: string; isFullscreen?: boolean; + 'aria-label'?: string; } /** @alpha */ export const PageToolbar: FC = React.memo( - ({ title, parent, pageIcon, onGoBack, children, titleHref, parentHref, leftItems, isFullscreen, className }) => { + ({ + title, + parent, + pageIcon, + onGoBack, + children, + titleHref, + parentHref, + leftItems, + isFullscreen, + className, + /** main nav-container aria-label **/ + 'aria-label': ariaLabel, + }) => { const styles = useStyles2(getStyles); /** @@ -44,7 +58,7 @@ export const PageToolbar: FC = React.memo( ); return ( -
+
+ ); } ); diff --git a/public/app/core/utils/richHistory.test.ts b/public/app/core/utils/richHistory.test.ts index e89d3860e15..cd6b12a58ac 100644 --- a/public/app/core/utils/richHistory.test.ts +++ b/public/app/core/utils/richHistory.test.ts @@ -12,7 +12,8 @@ import { } from './richHistory'; import store from 'app/core/store'; import { dateTime, DataQuery } from '@grafana/data'; -import RichHistoryStorage, { RichHistoryServiceError, RichHistoryStorageWarning } from '../history/RichHistoryStorage'; +import RichHistoryStorage, { RichHistoryStorageWarning } from '../history/RichHistoryStorage'; +import { RichHistoryQuery } from '../../types'; const richHistoryStorageMock: RichHistoryStorage = {} as RichHistoryStorage; @@ -22,20 +23,27 @@ jest.mock('../history/richHistoryStorageProvider', () => { }; }); +interface MockQuery extends DataQuery { + expr: string; + maxLines?: number | null; +} + +const storedHistory: Array> = [ + { + id: '1', + createdAt: 1, + comment: '', + datasourceUid: 'datasource uid', + datasourceName: 'datasource history name', + queries: [ + { expr: 'query1', maxLines: null, refId: '1' }, + { expr: 'query2', refId: '2' }, + ], + starred: true, + }, +]; + const mock: any = { - storedHistory: [ - { - id: '1', - createdAt: 1, - comment: '', - datasourceName: 'datasource history name', - queries: [ - { expr: 'query1', maxLines: null, refId: '1' }, - { expr: 'query2', refId: '2' }, - ], - starred: true, - }, - ], testComment: '', testDatasourceUid: 'datasourceUid', testDatasourceName: 'datasourceName', @@ -83,23 +91,10 @@ describe('richHistory', () => { deleteAllFromRichHistory(); expect(store.exists(key)).toBeFalsy(); }); - const expectedResult = [ - { - comment: mock.testComment, - datasourceUid: mock.testDatasourceUid, - datasourceName: mock.testDatasourceName, - queries: mock.testQueries, - starred: mock.testStarred, - createdAt: 2, - id: 'GENERATED ID', - }, - mock.storedHistory[0], - ]; it('should append query to query history', async () => { Date.now = jest.fn(() => 2); - const { richHistory: newHistory } = await addToRichHistory( - mock.storedHistory, + const { limitExceeded, richHistoryStorageFull } = await addToRichHistory( mock.testDatasourceUid, mock.testDatasourceName, mock.testQueries, @@ -108,23 +103,8 @@ describe('richHistory', () => { true, true ); - expect(newHistory).toEqual(expectedResult); - }); - - it('should add query history to storage', async () => { - Date.now = jest.fn(() => 2); - - const { richHistory } = await addToRichHistory( - mock.storedHistory, - mock.testDatasourceUid, - mock.testDatasourceName, - mock.testQueries, - mock.testStarred, - mock.testComment, - true, - true - ); - expect(richHistory).toMatchObject(expectedResult); + expect(limitExceeded).toBeFalsy(); + expect(richHistoryStorageFull).toBeFalsy(); expect(richHistoryStorageMock.addToRichHistory).toBeCalledWith({ datasourceUid: mock.testDatasourceUid, datasourceName: mock.testDatasourceName, @@ -134,27 +114,7 @@ describe('richHistory', () => { }); }); - it('should not append duplicated query to query history', async () => { - Date.now = jest.fn(() => 2); - - const duplicatedEntryError = new Error(); - duplicatedEntryError.name = RichHistoryServiceError.DuplicatedEntry; - richHistoryStorageMock.addToRichHistory = jest.fn().mockRejectedValue(duplicatedEntryError); - - 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, - mock.testComment, - true, - true - ); - expect(newHistory).toEqual([mock.storedHistory[0]]); - }); - - it('it should append new items even when the limit is exceeded', async () => { + it('it should return a flag indicating that the limit has been exceed', async () => { Date.now = jest.fn(() => 2); richHistoryStorageMock.addToRichHistory = jest.fn((query) => { @@ -167,8 +127,7 @@ describe('richHistory', () => { }); }); - const { richHistory, limitExceeded } = await addToRichHistory( - mock.storedHistory, + const { richHistoryStorageFull, limitExceeded } = await addToRichHistory( mock.testDatasourceUid, mock.testDatasourceName, mock.testQueries, @@ -177,29 +136,29 @@ describe('richHistory', () => { true, true ); - expect(richHistory).toEqual(expectedResult); + expect(richHistoryStorageFull).toBeFalsy(); expect(limitExceeded).toBeTruthy(); }); }); describe('updateStarredInRichHistory', () => { it('should update starred in query in history', async () => { - const updatedStarred = await updateStarredInRichHistory(mock.storedHistory, '1', !mock.starred); - expect(updatedStarred[0].starred).toEqual(!mock.starred); + const updatedStarred = await updateStarredInRichHistory('1', !mock.starred); + expect(updatedStarred!.starred).toEqual(!mock.starred); }); }); describe('updateCommentInRichHistory', () => { it('should update comment in query in history', async () => { - const updatedComment = await updateCommentInRichHistory(mock.storedHistory, '1', 'new comment'); - expect(updatedComment[0].comment).toEqual('new comment'); + const updatedComment = await updateCommentInRichHistory('1', 'new comment'); + expect(updatedComment!.comment).toEqual('new comment'); }); }); describe('deleteQueryInRichHistory', () => { it('should delete query in query in history', async () => { - const deletedHistory = await deleteQueryInRichHistory(mock.storedHistory, '1'); - expect(deletedHistory).toEqual([]); + const deletedHistoryId = await deleteQueryInRichHistory('1'); + expect(deletedHistoryId).toEqual('1'); }); }); @@ -220,7 +179,7 @@ describe('richHistory', () => { describe('filterQueries', () => { it('should filter out queries based on data source filter', () => { const filteredQueries = filterAndSortQueries( - mock.storedHistory, + storedHistory, SortOrder.Ascending, ['not provided data source'], '' @@ -228,25 +187,15 @@ describe('richHistory', () => { expect(filteredQueries).toHaveLength(0); }); it('should keep queries based on data source filter', () => { - const filteredQueries = filterAndSortQueries( - mock.storedHistory, - SortOrder.Ascending, - ['datasource history name'], - '' - ); + const filteredQueries = filterAndSortQueries(storedHistory, SortOrder.Ascending, ['datasource history name'], ''); expect(filteredQueries).toHaveLength(1); }); it('should filter out all queries based on search filter', () => { - const filteredQueries = filterAndSortQueries( - mock.storedHistory, - SortOrder.Ascending, - [], - 'i do not exist in query' - ); + const filteredQueries = filterAndSortQueries(storedHistory, SortOrder.Ascending, [], 'i do not exist in query'); expect(filteredQueries).toHaveLength(0); }); it('should include queries based on search filter', () => { - const filteredQueries = filterAndSortQueries(mock.storedHistory, SortOrder.Ascending, [], 'query1'); + const filteredQueries = filterAndSortQueries(storedHistory, SortOrder.Ascending, [], 'query1'); expect(filteredQueries).toHaveLength(1); }); }); @@ -254,13 +203,13 @@ 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].createdAt = 1 + -1 * dateTime().utcOffset() * 60 * 1000; - const heading = createQueryHeading(mock.storedHistory[0], SortOrder.Ascending); + storedHistory[0].createdAt = 1 + -1 * dateTime().utcOffset() * 60 * 1000; + const heading = createQueryHeading(storedHistory[0], SortOrder.Ascending); expect(heading).toEqual('January 1'); }); it('should correctly create heading for queries when sort order is datasourceAZ ', () => { - const heading = createQueryHeading(mock.storedHistory[0], SortOrder.DatasourceAZ); - expect(heading).toEqual(mock.storedHistory[0].datasourceName); + const heading = createQueryHeading(storedHistory[0], SortOrder.DatasourceAZ); + expect(heading).toEqual(storedHistory[0].datasourceName); }); }); }); diff --git a/public/app/core/utils/richHistory.ts b/public/app/core/utils/richHistory.ts index 567fbd97005..feebda044e3 100644 --- a/public/app/core/utils/richHistory.ts +++ b/public/app/core/utils/richHistory.ts @@ -33,7 +33,6 @@ export { SortOrder }; */ export async function addToRichHistory( - richHistory: RichHistoryQuery[], datasourceUid: string, datasourceName: string | null, queries: DataQuery[], @@ -41,7 +40,7 @@ export async function addToRichHistory( comment: string | null, showQuotaExceededError: boolean, showLimitExceededWarning: boolean -): Promise<{ richHistory: RichHistoryQuery[]; richHistoryStorageFull?: boolean; limitExceeded?: boolean }> { +): Promise<{ richHistoryStorageFull?: boolean; limitExceeded?: boolean }> { /* Save only queries, that are not falsy (e.g. empty object, null, ...) */ const newQueriesToSave: DataQuery[] = queries && queries.filter((query) => notEmptyQuery(query)); @@ -49,7 +48,6 @@ export async function addToRichHistory( let richHistoryStorageFull = false; let limitExceeded = false; let warning: RichHistoryStorageWarningDetails | undefined; - let newRichHistory: RichHistoryQuery; try { const result = await getRichHistoryStorage().addToRichHistory({ @@ -60,7 +58,6 @@ export async function addToRichHistory( comment: comment ?? '', }); warning = result.warning; - newRichHistory = result.richHistoryQuery; } catch (error) { if (error.name === RichHistoryServiceError.StorageFull) { richHistoryStorageFull = true; @@ -69,7 +66,7 @@ export async function addToRichHistory( dispatch(notifyApp(createErrorNotification('Rich History update failed', error.message))); } // Saving failed. Do not add new entry. - return { richHistory, richHistoryStorageFull, limitExceeded }; + return { richHistoryStorageFull, limitExceeded }; } // Limit exceeded but new entry was added. Notify that old entries have been removed. @@ -78,12 +75,11 @@ export async function addToRichHistory( showLimitExceededWarning && dispatch(notifyApp(createWarningNotification(warning.message))); } - // Saving successful - add new entry. - return { richHistory: [newRichHistory, ...richHistory], richHistoryStorageFull, limitExceeded }; + return { richHistoryStorageFull, limitExceeded }; } - // Nothing to save - return { richHistory }; + // Nothing to change + return {}; } export async function getRichHistory(): Promise { @@ -94,41 +90,31 @@ export async function deleteAllFromRichHistory(): Promise { return getRichHistoryStorage().deleteAll(); } -export async function updateStarredInRichHistory(richHistory: RichHistoryQuery[], id: string, starred: boolean) { +export async function updateStarredInRichHistory(id: string, starred: boolean) { try { - const updatedQuery = await getRichHistoryStorage().updateStarred(id, starred); - return richHistory.map((query) => (query.id === id ? updatedQuery : query)); + return await getRichHistoryStorage().updateStarred(id, starred); } catch (error) { dispatch(notifyApp(createErrorNotification('Saving rich history failed', error.message))); - return richHistory; + return undefined; } } -export async function updateCommentInRichHistory( - richHistory: RichHistoryQuery[], - id: string, - newComment: string | undefined -) { +export async function updateCommentInRichHistory(id: string, newComment: string | undefined) { try { - const updatedQuery = await getRichHistoryStorage().updateComment(id, newComment); - return richHistory.map((query) => (query.id === id ? updatedQuery : query)); + return await getRichHistoryStorage().updateComment(id, newComment); } catch (error) { dispatch(notifyApp(createErrorNotification('Saving rich history failed', error.message))); - return richHistory; + return undefined; } } -export async function deleteQueryInRichHistory( - richHistory: RichHistoryQuery[], - id: string -): Promise { - const updatedHistory = richHistory.filter((query) => query.id !== id); +export async function deleteQueryInRichHistory(id: string) { try { await getRichHistoryStorage().deleteRichHistory(id); - return updatedHistory; + return id; } catch (error) { dispatch(notifyApp(createErrorNotification('Saving rich history failed', error.message))); - return richHistory; + return undefined; } } diff --git a/public/app/features/explore/ExploreToolbar.tsx b/public/app/features/explore/ExploreToolbar.tsx index 29ca0cf5df7..f38824de47f 100644 --- a/public/app/features/explore/ExploreToolbar.tsx +++ b/public/app/features/explore/ExploreToolbar.tsx @@ -81,6 +81,7 @@ class UnConnectedExploreToolbar extends PureComponent { return (
) => { richHistory: [], firstTab: Tabs.RichHistory, deleteRichHistory: jest.fn(), + loadRichHistory: jest.fn(), onClose: jest.fn(), }; Object.assign(props, propOverrides); - const wrapper = mount(); - return wrapper; + return render(); }; describe('RichHistoryContainer', () => { - it('should render reseizable component', () => { - const wrapper = setup(); - expect(wrapper.find(Resizable)).toHaveLength(1); - }); it('should render component with correct width', () => { - const wrapper = setup(); - expect(wrapper.getDOMNode().getAttribute('style')).toContain('width: 531.5px'); + const { container } = setup(); + expect(container.firstElementChild!.getAttribute('style')).toContain('width: 531.5px'); }); it('should render component with correct height', () => { - const wrapper = setup(); - expect(wrapper.getDOMNode().getAttribute('style')).toContain('height: 400px'); + const { container } = setup(); + expect(container.firstElementChild!.getAttribute('style')).toContain('height: 400px'); + }); + it('should re-request rich history every time the component is mounted', () => { + const loadRichHistory = jest.fn(); + const { unmount } = setup({ loadRichHistory }); + expect(loadRichHistory).toBeCalledTimes(1); + + unmount(); + expect(loadRichHistory).toBeCalledTimes(1); + + setup({ loadRichHistory }); + expect(loadRichHistory).toBeCalledTimes(2); }); }); diff --git a/public/app/features/explore/RichHistory/RichHistoryContainer.tsx b/public/app/features/explore/RichHistory/RichHistoryContainer.tsx index c68162f9834..aa13771e438 100644 --- a/public/app/features/explore/RichHistory/RichHistoryContainer.tsx +++ b/public/app/features/explore/RichHistory/RichHistoryContainer.tsx @@ -1,5 +1,5 @@ // Libraries -import React, { useState } from 'react'; +import React, { useEffect, useState } from 'react'; import { connect, ConnectedProps } from 'react-redux'; // Services & Utils @@ -14,7 +14,7 @@ import { ExploreId } from 'app/types/explore'; import { RichHistory, Tabs } from './RichHistory'; //Actions -import { deleteRichHistory } from '../state/history'; +import { deleteRichHistory, loadRichHistory } from '../state/history'; import { ExploreDrawer } from '../ExploreDrawer'; function mapStateToProps(state: StoreState, { exploreId }: { exploreId: ExploreId }) { @@ -25,7 +25,7 @@ function mapStateToProps(state: StoreState, { exploreId }: { exploreId: ExploreI const firstTab = store.getBool(RICH_HISTORY_SETTING_KEYS.starredTabAsFirstTab, false) ? Tabs.Starred : Tabs.RichHistory; - const { richHistory } = explore; + const { richHistory } = item; return { richHistory, firstTab, @@ -34,6 +34,7 @@ function mapStateToProps(state: StoreState, { exploreId }: { exploreId: ExploreI } const mapDispatchToProps = { + loadRichHistory, deleteRichHistory, }; @@ -49,7 +50,20 @@ export type Props = ConnectedProps & OwnProps; export function RichHistoryContainer(props: Props) { const [height, setHeight] = useState(400); - const { richHistory, width, firstTab, activeDatasourceInstance, exploreId, deleteRichHistory, onClose } = props; + const { + richHistory, + width, + firstTab, + activeDatasourceInstance, + exploreId, + deleteRichHistory, + loadRichHistory, + onClose, + } = props; + + useEffect(() => { + loadRichHistory(exploreId); + }, [loadRichHistory, exploreId]); return ( { range: { from: 'now-1h', to: 'now' }, }), }; - const { datasources, store } = setupExplore({ urlParams }); + const { datasources } = setupExplore({ urlParams }); (datasources.loki.query as Mock).mockReturnValueOnce(makeLogsQueryResponse()); // Make sure we render the logs panel @@ -84,11 +84,6 @@ describe('Wrapper', () => { ...urlParams, }); - expect(store.getState().explore.richHistory[0]).toMatchObject({ - datasourceName: 'loki', - queries: [{ expr: '{ label="value"}', refId: 'A' }], - }); - // We called the data source query method once expect(datasources.loki.query).toBeCalledTimes(1); expect((datasources.loki.query as Mock).mock.calls[0][0]).toMatchObject({ diff --git a/public/app/features/explore/Wrapper.tsx b/public/app/features/explore/Wrapper.tsx index 4a6d46a0458..4f2d5439ea4 100644 --- a/public/app/features/explore/Wrapper.tsx +++ b/public/app/features/explore/Wrapper.tsx @@ -3,7 +3,6 @@ import { connect, ConnectedProps } from 'react-redux'; import { ExploreId, ExploreQueryParams } from 'app/types/explore'; import { ErrorBoundaryAlert } from '@grafana/ui'; import { lastSavedUrl, resetExploreAction, richHistoryUpdatedAction } from './state/main'; -import { getRichHistory } from '../../core/utils/richHistory'; import { ExplorePaneContainer } from './ExplorePaneContainer'; import { GrafanaRouteComponentProps } from 'app/core/navigation/types'; import { Branding } from '../../core/components/Branding/Branding'; @@ -52,10 +51,6 @@ class WrapperUnconnected extends PureComponent { if (searchParams.from || searchParams.to) { locationService.partial({ from: undefined, to: undefined }, true); } - - getRichHistory().then((richHistory) => { - this.props.richHistoryUpdatedAction({ richHistory }); - }); } componentDidUpdate(prevProps: Props) { diff --git a/public/app/features/explore/spec/helper/assert.ts b/public/app/features/explore/spec/helper/assert.ts index e0ec61f94f1..353278aa21a 100644 --- a/public/app/features/explore/spec/helper/assert.ts +++ b/public/app/features/explore/spec/helper/assert.ts @@ -1,7 +1,32 @@ -import { screen } from '@testing-library/react'; +import { waitFor } from '@testing-library/react'; +import { ExploreId } from '../../../../types'; +import { withinExplore } from './setup'; -export const assertQueryHistoryExists = (query: string) => { - expect(screen.getByText('1 queries')).toBeInTheDocument(); - const queryItem = screen.getByLabelText('Query text'); +export const assertQueryHistoryExists = (query: string, exploreId: ExploreId = ExploreId.left) => { + const selector = withinExplore(exploreId); + + expect(selector.getByText('1 queries')).toBeInTheDocument(); + const queryItem = selector.getByLabelText('Query text'); expect(queryItem).toHaveTextContent(query); }; + +export const assertQueryHistory = async (expectedQueryTexts: string[], exploreId: ExploreId = ExploreId.left) => { + const selector = withinExplore(exploreId); + await waitFor(() => { + expect(selector.getByText(`${expectedQueryTexts.length} queries`)).toBeInTheDocument(); + const queryTexts = selector.getAllByLabelText('Query text'); + expectedQueryTexts.forEach((expectedQueryText, queryIndex) => { + expect(queryTexts[queryIndex]).toHaveTextContent(expectedQueryText); + }); + }); +}; + +export const assertQueryHistoryIsStarred = async (expectedStars: boolean[], exploreId: ExploreId = ExploreId.left) => { + const selector = withinExplore(exploreId); + const starButtons = selector.getAllByRole('button', { name: /Star query|Unstar query/ }); + await waitFor(() => + expectedStars.forEach((starred, queryIndex) => { + expect(starButtons[queryIndex]).toHaveAccessibleName(starred ? 'Unstar query' : 'Star query'); + }) + ); +}; diff --git a/public/app/features/explore/spec/helper/interactions.ts b/public/app/features/explore/spec/helper/interactions.ts index 50675cac69c..df8f51fb0bc 100644 --- a/public/app/features/explore/spec/helper/interactions.ts +++ b/public/app/features/explore/spec/helper/interactions.ts @@ -1,6 +1,8 @@ import { selectors } from '@grafana/e2e-selectors'; import userEvent from '@testing-library/user-event'; -import { fireEvent, screen } from '@testing-library/react'; +import { fireEvent, screen, within } from '@testing-library/react'; +import { ExploreId } from '../../../../types'; +import { withinExplore } from './setup'; export const changeDatasource = async (name: string) => { const datasourcePicker = (await screen.findByLabelText(selectors.components.DataSourcePicker.container)).children[0]; @@ -9,20 +11,38 @@ export const changeDatasource = async (name: string) => { fireEvent.click(option); }; -export const inputQuery = (query: string) => { - const input = screen.getByRole('textbox', { name: 'query' }); +export const inputQuery = (query: string, exploreId: ExploreId = ExploreId.left) => { + const input = withinExplore(exploreId).getByRole('textbox', { name: 'query' }); + userEvent.clear(input); userEvent.type(input, query); }; -export const runQuery = () => { - const button = screen.getByRole('button', { name: /run query/i }); +export const runQuery = (exploreId: ExploreId = ExploreId.left) => { + const explore = withinExplore(exploreId); + const toolbar = within(explore.getByLabelText('Explore toolbar')); + const button = toolbar.getByRole('button', { name: /run query/i }); userEvent.click(button); }; -export const openQueryHistory = async () => { - const button = screen.getByRole('button', { name: 'Rich history button' }); +export const openQueryHistory = async (exploreId: ExploreId = ExploreId.left) => { + const selector = withinExplore(exploreId); + const button = selector.getByRole('button', { name: 'Rich history button' }); userEvent.click(button); expect( - await screen.findByText('The history is local to your browser and is not shared with others.') + await selector.findByText('The history is local to your browser and is not shared with others.') ).toBeInTheDocument(); }; + +export const starQueryHistory = (queryIndex: number, exploreId: ExploreId = ExploreId.left) => { + invokeAction(queryIndex, 'Star query', exploreId); +}; + +export const deleteQueryHistory = (queryIndex: number, exploreId: ExploreId = ExploreId.left) => { + invokeAction(queryIndex, 'Delete query', exploreId); +}; + +const invokeAction = (queryIndex: number, actionAccessibleName: string, exploreId: ExploreId) => { + const selector = withinExplore(exploreId); + const buttons = selector.getAllByRole('button', { name: actionAccessibleName }); + userEvent.click(buttons[queryIndex]); +}; diff --git a/public/app/features/explore/spec/helper/setup.tsx b/public/app/features/explore/spec/helper/setup.tsx index 81e292cceac..2d3b243c8a2 100644 --- a/public/app/features/explore/spec/helper/setup.tsx +++ b/public/app/features/explore/spec/helper/setup.tsx @@ -1,5 +1,6 @@ import React from 'react'; import { render, screen } from '@testing-library/react'; +import { within } from '@testing-library/dom'; import { EnhancedStore } from '@reduxjs/toolkit'; import { Provider } from 'react-redux'; import { Route, Router } from 'react-router-dom'; @@ -16,6 +17,7 @@ import { initialUserState } from '../../../profile/state/reducers'; import { LokiDatasource } from '../../../../plugins/datasource/loki/datasource'; import { LokiQuery } from '../../../../plugins/datasource/loki/types'; +import { ExploreId } from '../../../../types'; type DatasourceSetup = { settings: DataSourceInstanceSettings; api: DataSourceApi }; @@ -31,6 +33,7 @@ export function setupExplore(options?: SetupOptions): { datasources: { [name: string]: DataSourceApi }; store: EnhancedStore; unmount: () => void; + container: HTMLElement; } { // Clear this up otherwise it persists data source selection // TODO: probably add test for that too @@ -87,7 +90,7 @@ export function setupExplore(options?: SetupOptions): { const route = { component: Wrapper }; - const { unmount } = render( + const { unmount, container } = render( } /> @@ -95,7 +98,7 @@ export function setupExplore(options?: SetupOptions): { ); - return { datasources: fromPairs(dsSettings.map((d) => [d.api.name, d.api])), store, unmount }; + return { datasources: fromPairs(dsSettings.map((d) => [d.api.name, d.api])), store, unmount, container }; } function makeDatasourceSetup({ name = 'loki', id = 1 }: { name?: string; id?: number } = {}): DatasourceSetup { @@ -137,16 +140,21 @@ function makeDatasourceSetup({ name = 'loki', id = 1 }: { name?: string; id?: nu name: name, uid: name, query: jest.fn(), - getRef: jest.fn(), + getRef: jest.fn().mockReturnValue(name), meta, } as any, }; } -export const waitForExplore = async () => { - await screen.findAllByText(/Editor/i); +export const waitForExplore = async (exploreId: ExploreId = ExploreId.left) => { + return await withinExplore(exploreId).findByText(/Editor/i); }; export const tearDown = () => { window.localStorage.clear(); }; + +export const withinExplore = (exploreId: ExploreId) => { + const container = screen.getAllByTestId('data-testid Explore'); + return within(container[exploreId === ExploreId.left ? 0 : 1]); +}; diff --git a/public/app/features/explore/spec/queryHistory.test.tsx b/public/app/features/explore/spec/queryHistory.test.tsx index 2451de00a3a..87e9f8a2bd4 100644 --- a/public/app/features/explore/spec/queryHistory.test.tsx +++ b/public/app/features/explore/spec/queryHistory.test.tsx @@ -1,8 +1,11 @@ import React from 'react'; +import { serializeStateToUrlParam } from '@grafana/data'; import { setupExplore, tearDown, waitForExplore } from './helper/setup'; -import { inputQuery, openQueryHistory, runQuery } from './helper/interactions'; -import { assertQueryHistoryExists } from './helper/assert'; +import { deleteQueryHistory, inputQuery, openQueryHistory, runQuery, starQueryHistory } from './helper/interactions'; +import { assertQueryHistory, assertQueryHistoryExists, assertQueryHistoryIsStarred } from './helper/assert'; import { makeLogsQueryResponse } from './helper/query'; +import { ExploreId } from '../../../types'; +import { silenceConsoleOutput } from '../../../../test/core/utils/silenceConsoleOutput'; jest.mock('react-virtualized-auto-sizer', () => { return { @@ -17,6 +20,8 @@ describe('Explore: Query History', () => { const USER_INPUT = 'my query'; const RAW_QUERY = `{"expr":"${USER_INPUT}"}`; + silenceConsoleOutput(); + afterEach(() => { tearDown(); }); @@ -33,7 +38,7 @@ describe('Explore: Query History', () => { await openQueryHistory(); // the query that was run is in query history - assertQueryHistoryExists(RAW_QUERY); + await assertQueryHistoryExists(RAW_QUERY); // when Explore is opened again unmount(); @@ -42,6 +47,60 @@ describe('Explore: Query History', () => { // previously added query is in query history await openQueryHistory(); - assertQueryHistoryExists(RAW_QUERY); + await assertQueryHistoryExists(RAW_QUERY); + }); + + it('adds recently added query if the query history panel is already open', async () => { + const urlParams = { + left: serializeStateToUrlParam({ + datasource: 'loki', + queries: [{ refId: 'A', expr: 'query #1' }], + range: { from: 'now-1h', to: 'now' }, + }), + }; + + const { datasources } = setupExplore({ urlParams }); + (datasources.loki.query as jest.Mock).mockReturnValueOnce(makeLogsQueryResponse()); + await waitForExplore(); + await openQueryHistory(); + + inputQuery('query #2'); + runQuery(); + await assertQueryHistory(['{"expr":"query #2"}', '{"expr":"query #1"}']); + }); + + it('updates the state in both Explore panes', async () => { + const urlParams = { + left: serializeStateToUrlParam({ + datasource: 'loki', + queries: [{ refId: 'A', expr: 'query #1' }], + range: { from: 'now-1h', to: 'now' }, + }), + right: serializeStateToUrlParam({ + datasource: 'loki', + queries: [{ refId: 'A', expr: 'query #2' }], + range: { from: 'now-1h', to: 'now' }, + }), + }; + + const { datasources } = setupExplore({ urlParams }); + (datasources.loki.query as jest.Mock).mockReturnValue(makeLogsQueryResponse()); + await waitForExplore(); + await waitForExplore(ExploreId.right); + + // queries in history + await openQueryHistory(ExploreId.left); + await assertQueryHistory(['{"expr":"query #2"}', '{"expr":"query #1"}'], ExploreId.left); + await openQueryHistory(ExploreId.right); + await assertQueryHistory(['{"expr":"query #2"}', '{"expr":"query #1"}'], ExploreId.right); + + // star one one query + starQueryHistory(1, ExploreId.left); + await assertQueryHistoryIsStarred([false, true], ExploreId.left); + await assertQueryHistoryIsStarred([false, true], ExploreId.right); + + deleteQueryHistory(0, ExploreId.left); + await assertQueryHistory(['{"expr":"query #1"}'], ExploreId.left); + await assertQueryHistory(['{"expr":"query #1"}'], ExploreId.right); }); }); diff --git a/public/app/features/explore/state/explorePane.ts b/public/app/features/explore/state/explorePane.ts index a40de3799cb..38b07eb3f5b 100644 --- a/public/app/features/explore/state/explorePane.ts +++ b/public/app/features/explore/state/explorePane.ts @@ -36,7 +36,6 @@ import { import { ThunkResult } from 'app/types'; import { getFiscalYearStartMonth, getTimeZone } from 'app/features/profile/state/selectors'; import { getDataSourceSrv } from '@grafana/runtime'; -import { getRichHistory } from '../../../core/utils/richHistory'; import { richHistoryUpdatedAction, stateSave } from './main'; import { keybindingSrv } from 'app/core/services/keybindingSrv'; @@ -181,9 +180,6 @@ export function initializeExplore( // user to go back to previous url. dispatch(runQueries(exploreId, { replaceUrl: true })); } - - const richHistory = await getRichHistory(); - dispatch(richHistoryUpdatedAction({ richHistory })); }; } @@ -259,6 +255,13 @@ export const paneReducer = (state: ExploreItemState = makeExplorePaneState(), ac state = timeReducer(state, action); state = historyReducer(state, action); + if (richHistoryUpdatedAction.match(action)) { + return { + ...state, + richHistory: action.payload.richHistory, + }; + } + if (changeSizeAction.match(action)) { const containerWidth = action.payload.width; return { ...state, containerWidth }; diff --git a/public/app/features/explore/state/helpers.ts b/public/app/features/explore/state/helpers.ts index c4c0fc84224..5e744449262 100644 --- a/public/app/features/explore/state/helpers.ts +++ b/public/app/features/explore/state/helpers.ts @@ -38,6 +38,7 @@ export const createDefaultInitialState = () => { value: 0, }, cache: [], + richHistory: [], }, }, }; diff --git a/public/app/features/explore/state/history.ts b/public/app/features/explore/state/history.ts index 1f680ef367e..a44329ba995 100644 --- a/public/app/features/explore/state/history.ts +++ b/public/app/features/explore/state/history.ts @@ -1,12 +1,14 @@ import { + addToRichHistory, deleteAllFromRichHistory, deleteQueryInRichHistory, + getRichHistory, updateCommentInRichHistory, updateStarredInRichHistory, } from 'app/core/utils/richHistory'; -import { ExploreId, ExploreItemState, ThunkResult } from 'app/types'; -import { richHistoryUpdatedAction } from './main'; -import { HistoryItem } from '@grafana/data'; +import { ExploreId, ExploreItemState, ExploreState, RichHistoryQuery, ThunkResult } from 'app/types'; +import { richHistoryLimitExceededAction, richHistoryStorageFullAction, richHistoryUpdatedAction } from './main'; +import { DataQuery, HistoryItem } from '@grafana/data'; import { AnyAction, createAction } from '@reduxjs/toolkit'; // @@ -23,31 +25,89 @@ export const historyUpdatedAction = createAction('explore // Action creators // +type SyncHistoryUpdatesOptions = { + updatedQuery?: RichHistoryQuery; + deletedId?: string; +}; + +/** + * Updates current state in both Explore panes after changing or deleting a query history item + */ +const updateRichHistoryState = ({ updatedQuery, deletedId }: SyncHistoryUpdatesOptions): ThunkResult => { + return async (dispatch, getState) => { + forEachExplorePane(getState().explore, (item, exploreId) => { + const newRichHistory = item.richHistory + // update + .map((query) => (query.id === updatedQuery?.id ? updatedQuery : query)) + // or remove + .filter((query) => query.id !== deletedId); + dispatch(richHistoryUpdatedAction({ richHistory: newRichHistory, exploreId })); + }); + }; +}; + +const forEachExplorePane = (state: ExploreState, callback: (item: ExploreItemState, exploreId: ExploreId) => void) => { + callback(state.left, ExploreId.left); + state.right && callback(state.right, ExploreId.right); +}; + +export const addHistoryItem = ( + datasourceUid: string, + datasourceName: string, + queries: DataQuery[] +): ThunkResult => { + return async (dispatch, getState) => { + const { richHistoryStorageFull, limitExceeded } = await addToRichHistory( + datasourceUid, + datasourceName, + queries, + false, + '', + !getState().explore.richHistoryStorageFull, + !getState().explore.richHistoryLimitExceededWarningShown + ); + if (richHistoryStorageFull) { + dispatch(richHistoryStorageFullAction()); + } + if (limitExceeded) { + dispatch(richHistoryLimitExceededAction()); + } + }; +}; + export const starHistoryItem = (id: string, starred: boolean): ThunkResult => { return async (dispatch, getState) => { - const nextRichHistory = await updateStarredInRichHistory(getState().explore.richHistory, id, starred); - dispatch(richHistoryUpdatedAction({ richHistory: nextRichHistory })); + const updatedQuery = await updateStarredInRichHistory(id, starred); + dispatch(updateRichHistoryState({ updatedQuery })); }; }; 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 })); + return async (dispatch) => { + const updatedQuery = await updateCommentInRichHistory(id, comment); + dispatch(updateRichHistoryState({ updatedQuery })); }; }; export const deleteHistoryItem = (id: string): ThunkResult => { - return async (dispatch, getState) => { - const nextRichHistory = await deleteQueryInRichHistory(getState().explore.richHistory, id); - dispatch(richHistoryUpdatedAction({ richHistory: nextRichHistory })); + return async (dispatch) => { + const deletedId = await deleteQueryInRichHistory(id); + dispatch(updateRichHistoryState({ deletedId })); }; }; export const deleteRichHistory = (): ThunkResult => { return async (dispatch) => { await deleteAllFromRichHistory(); - dispatch(richHistoryUpdatedAction({ richHistory: [] })); + dispatch(richHistoryUpdatedAction({ richHistory: [], exploreId: ExploreId.left })); + dispatch(richHistoryUpdatedAction({ richHistory: [], exploreId: ExploreId.right })); + }; +}; + +export const loadRichHistory = (exploreId: ExploreId): ThunkResult => { + return async (dispatch) => { + const richHistory = await getRichHistory(); + dispatch(richHistoryUpdatedAction({ richHistory, exploreId })); }; }; diff --git a/public/app/features/explore/state/main.ts b/public/app/features/explore/state/main.ts index 5d2bb1f10c9..249d7cda74c 100644 --- a/public/app/features/explore/state/main.ts +++ b/public/app/features/explore/state/main.ts @@ -2,7 +2,7 @@ import { AnyAction } from 'redux'; import { DataSourceSrv, getDataSourceSrv, locationService } from '@grafana/runtime'; import { ExploreUrlState, serializeStateToUrlParam, SplitOpen, UrlQueryMap } from '@grafana/data'; import { GetExploreUrlArguments, stopQueryState } from 'app/core/utils/explore'; -import { ExploreId, ExploreItemState, ExploreState } from 'app/types/explore'; +import { ExploreId, ExploreItemState, ExploreState, RichHistoryQuery } from 'app/types/explore'; import { paneReducer } from './explorePane'; import { createAction } from '@reduxjs/toolkit'; import { getUrlStateFromPaneState, makeExplorePaneState } from './utils'; @@ -19,7 +19,8 @@ export interface SyncTimesPayload { } export const syncTimesAction = createAction('explore/syncTimes'); -export const richHistoryUpdatedAction = createAction('explore/richHistoryUpdated'); +export const richHistoryUpdatedAction = + createAction<{ richHistory: RichHistoryQuery[]; exploreId: ExploreId }>('explore/richHistoryUpdated'); export const richHistoryStorageFullAction = createAction('explore/richHistoryStorageFullAction'); export const richHistoryLimitExceededAction = createAction('explore/richHistoryLimitExceededAction'); @@ -157,7 +158,6 @@ export const initialExploreState: ExploreState = { syncedTimes: false, left: initialExploreItemState, right: undefined, - richHistory: [], richHistoryStorageFull: false, richHistoryLimitExceededWarningShown: false, }; @@ -207,13 +207,6 @@ export const exploreReducer = (state = initialExploreState, action: AnyAction): return { ...state, syncedTimes: action.payload.syncedTimes }; } - if (richHistoryUpdatedAction.match(action)) { - return { - ...state, - richHistory: action.payload.richHistory, - }; - } - if (richHistoryStorageFullAction.match(action)) { return { ...state, diff --git a/public/app/features/explore/state/query.ts b/public/app/features/explore/state/query.ts index 1dc93d3f76f..10216ecdb3c 100644 --- a/public/app/features/explore/state/query.ts +++ b/public/app/features/explore/state/query.ts @@ -27,7 +27,6 @@ import { stopQueryState, updateHistory, } from 'app/core/utils/explore'; -import { addToRichHistory } from 'app/core/utils/richHistory'; import { ExploreItemState, ExplorePanelData, ThunkDispatch, ThunkResult } from 'app/types'; import { ExploreId, ExploreState, QueryOptions } from 'app/types/explore'; import { getTimeZone } from 'app/features/profile/state/selectors'; @@ -36,15 +35,10 @@ import { notifyApp } from '../../../core/actions'; import { runRequest } from '../../query/state/runRequest'; import { decorateData } from '../utils/decorators'; import { createErrorNotification } from '../../../core/copy/appNotification'; -import { - richHistoryStorageFullAction, - richHistoryLimitExceededAction, - richHistoryUpdatedAction, - stateSave, -} from './main'; +import { stateSave } from './main'; import { AnyAction, createAction, PayloadAction } from '@reduxjs/toolkit'; import { updateTime } from './time'; -import { historyUpdatedAction } from './history'; +import { addHistoryItem, historyUpdatedAction, loadRichHistory } from './history'; import { createCacheKey, getResultsFromCache } from './utils'; import deepEqual from 'fast-deep-equal'; @@ -320,29 +314,14 @@ async function handleHistory( ) { const datasourceId = datasource.meta.id; const nextHistory = updateHistory(history, datasourceId, queries); - const { - richHistory: nextRichHistory, - richHistoryStorageFull, - limitExceeded, - } = await addToRichHistory( - state.richHistory || [], - datasource.uid, - datasource.name, - queries, - false, - '', - !state.richHistoryStorageFull, - !state.richHistoryLimitExceededWarningShown - ); dispatch(historyUpdatedAction({ exploreId, history: nextHistory })); - dispatch(richHistoryUpdatedAction({ richHistory: nextRichHistory })); - if (richHistoryStorageFull) { - dispatch(richHistoryStorageFullAction()); - } - if (limitExceeded) { - dispatch(richHistoryLimitExceededAction()); - } + dispatch(addHistoryItem(datasource.uid, datasource.name, queries)); + + // Because filtering happens in the backend we cannot add a new entry without checking if it matches currently + // used filters. Instead, we refresh the query history list. + // TODO: run only if Query History list is opened (#47252) + dispatch(loadRichHistory(exploreId)); } /** diff --git a/public/app/features/explore/state/utils.ts b/public/app/features/explore/state/utils.ts index a75c193d2c7..563c86326eb 100644 --- a/public/app/features/explore/state/utils.ts +++ b/public/app/features/explore/state/utils.ts @@ -61,6 +61,7 @@ export const makeExplorePaneState = (): ExploreItemState => ({ logsResult: null, eventBridge: null as unknown as EventBusExtended, cache: [], + richHistory: [], logsVolumeDataProvider: undefined, logsVolumeData: undefined, graphStyle: loadGraphStyle(), diff --git a/public/app/types/explore.ts b/public/app/types/explore.ts index 52bbef90223..626b894070b 100644 --- a/public/app/types/explore.ts +++ b/public/app/types/explore.ts @@ -42,10 +42,6 @@ export interface ExploreState { * Explore state of the right area in split view. */ right?: ExploreItemState; - /** - * History of all queries - */ - richHistory: RichHistoryQuery[]; /** * True if local storage quota was exceeded when a rich history item was added. This is to prevent showing @@ -153,6 +149,11 @@ export interface ExploreItemState { showTrace?: boolean; showNodeGraph?: boolean; + /** + * History of all queries + */ + richHistory: RichHistoryQuery[]; + /** * We are using caching to store query responses of queries run from logs navigation. * In logs navigation, we do pagination and we don't want our users to unnecessarily run the same queries that they've run just moments before.