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.