From 943741edc9b2e11be2215cbca3fca757243c2209 Mon Sep 17 00:00:00 2001 From: Yulong Ruan Date: Wed, 23 Oct 2024 23:15:22 +0800 Subject: [PATCH] feat: update discover summary UX It will only auto-trigger the summary generating the first time a ppl query was generated. The following summary generation requires user to click a "Generate summary" button. Signed-off-by: Yulong Ruan --- .../dashboard_top_nav.test.tsx.snap | 54 ++ src/plugins/data/public/search/mocks.ts | 9 + .../components/query_assist_bar.test.tsx | 7 +- .../components/query_assist_bar.tsx | 4 +- .../components/query_assist_summary.test.tsx | 589 ++++++++++-------- .../components/query_assist_summary.tsx | 361 ++++++----- .../query_assist/hooks/use_query_assist.ts | 11 +- .../query_assist/utils/create_extension.tsx | 54 +- 8 files changed, 655 insertions(+), 434 deletions(-) diff --git a/src/plugins/dashboard/public/application/components/dashboard_top_nav/__snapshots__/dashboard_top_nav.test.tsx.snap b/src/plugins/dashboard/public/application/components/dashboard_top_nav/__snapshots__/dashboard_top_nav.test.tsx.snap index b09ffb33bb6e..303fea836b50 100644 --- a/src/plugins/dashboard/public/application/components/dashboard_top_nav/__snapshots__/dashboard_top_nav.test.tsx.snap +++ b/src/plugins/dashboard/public/application/components/dashboard_top_nav/__snapshots__/dashboard_top_nav.test.tsx.snap @@ -410,6 +410,15 @@ exports[`Dashboard top nav render in embed mode 1`] = ` }, "df": Object { "clear": [MockFunction], + "df$": BehaviorSubject { + "_isScalar": false, + "_value": undefined, + "closed": false, + "hasError": false, + "isStopped": false, + "observers": Array [], + "thrownError": null, + }, "get": [MockFunction], "set": [MockFunction], }, @@ -1489,6 +1498,15 @@ exports[`Dashboard top nav render in embed mode, and force hide filter bar 1`] = }, "df": Object { "clear": [MockFunction], + "df$": BehaviorSubject { + "_isScalar": false, + "_value": undefined, + "closed": false, + "hasError": false, + "isStopped": false, + "observers": Array [], + "thrownError": null, + }, "get": [MockFunction], "set": [MockFunction], }, @@ -2568,6 +2586,15 @@ exports[`Dashboard top nav render in embed mode, components can be forced show b }, "df": Object { "clear": [MockFunction], + "df$": BehaviorSubject { + "_isScalar": false, + "_value": undefined, + "closed": false, + "hasError": false, + "isStopped": false, + "observers": Array [], + "thrownError": null, + }, "get": [MockFunction], "set": [MockFunction], }, @@ -3647,6 +3674,15 @@ exports[`Dashboard top nav render in full screen mode with appended URL param bu }, "df": Object { "clear": [MockFunction], + "df$": BehaviorSubject { + "_isScalar": false, + "_value": undefined, + "closed": false, + "hasError": false, + "isStopped": false, + "observers": Array [], + "thrownError": null, + }, "get": [MockFunction], "set": [MockFunction], }, @@ -4726,6 +4762,15 @@ exports[`Dashboard top nav render in full screen mode, no componenets should be }, "df": Object { "clear": [MockFunction], + "df$": BehaviorSubject { + "_isScalar": false, + "_value": undefined, + "closed": false, + "hasError": false, + "isStopped": false, + "observers": Array [], + "thrownError": null, + }, "get": [MockFunction], "set": [MockFunction], }, @@ -5805,6 +5850,15 @@ exports[`Dashboard top nav render with all components 1`] = ` }, "df": Object { "clear": [MockFunction], + "df$": BehaviorSubject { + "_isScalar": false, + "_value": undefined, + "closed": false, + "hasError": false, + "isStopped": false, + "observers": Array [], + "thrownError": null, + }, "get": [MockFunction], "set": [MockFunction], }, diff --git a/src/plugins/data/public/search/mocks.ts b/src/plugins/data/public/search/mocks.ts index 826d0ed69fab..d72d984817e0 100644 --- a/src/plugins/data/public/search/mocks.ts +++ b/src/plugins/data/public/search/mocks.ts @@ -28,6 +28,8 @@ * under the License. */ +import { BehaviorSubject } from 'rxjs'; +import { IDataFrame } from '../../common'; import { searchAggsSetupMock, searchAggsStartMock } from './aggs/mocks'; import { searchSourceMock } from './search_source/mocks'; import { ISearchSetup, ISearchStart } from './types'; @@ -37,6 +39,12 @@ function createSetupContract(): jest.Mocked { aggs: searchAggsSetupMock(), __enhance: jest.fn(), getDefaultSearchInterceptor: jest.fn(), + df: { + get: jest.fn().mockReturnValue({}), + set: jest.fn().mockReturnValue({}), + clear: jest.fn(), + df$: new BehaviorSubject(undefined), + }, }; } @@ -52,6 +60,7 @@ function createStartContract(): jest.Mocked { get: jest.fn().mockReturnValue({}), set: jest.fn().mockReturnValue({}), clear: jest.fn(), + df$: new BehaviorSubject(undefined), }, }; } diff --git a/src/plugins/query_enhancements/public/query_assist/components/query_assist_bar.test.tsx b/src/plugins/query_enhancements/public/query_assist/components/query_assist_bar.test.tsx index 367f44c6426b..6c8e74852ff4 100644 --- a/src/plugins/query_enhancements/public/query_assist/components/query_assist_bar.test.tsx +++ b/src/plugins/query_enhancements/public/query_assist/components/query_assist_bar.test.tsx @@ -28,7 +28,7 @@ jest.mock('../hooks', () => ({ useGenerateQuery: jest.fn().mockReturnValue({ generateQuery: jest.fn(), loading: false }), useQueryAssist: jest .fn() - .mockReturnValue({ updateQuestion: jest.fn(), isQueryAssistCollapsed: false }), + .mockReturnValue({ updateQueryState: jest.fn(), isQueryAssistCollapsed: false }), })); jest.mock('./query_assist_input', () => ({ @@ -91,7 +91,10 @@ describe('QueryAssistBar', () => { }); it('renders null if question assist is collapsed', () => { - useQueryAssist.mockReturnValueOnce({ updateQuestion: jest.fn(), isQueryAssistCollapsed: true }); + useQueryAssist.mockReturnValueOnce({ + updateQueryState: jest.fn(), + isQueryAssistCollapsed: true, + }); const { component } = renderQueryAssistBar({ dependencies: { ...dependencies, isCollapsed: false }, }); diff --git a/src/plugins/query_enhancements/public/query_assist/components/query_assist_bar.tsx b/src/plugins/query_enhancements/public/query_assist/components/query_assist_bar.tsx index 5b3662fbff2d..fb8c4a99ed7e 100644 --- a/src/plugins/query_enhancements/public/query_assist/components/query_assist_bar.tsx +++ b/src/plugins/query_enhancements/public/query_assist/components/query_assist_bar.tsx @@ -43,7 +43,7 @@ export const QueryAssistBar: React.FC = (props) => { ); const selectedIndex = selectedDataset?.title; const previousQuestionRef = useRef(); - const { updateQuestion, isQueryAssistCollapsed } = useQueryAssist(); + const { isQueryAssistCollapsed, updateQueryState } = useQueryAssist(); useEffect(() => { const subscription = queryString.getUpdates$().subscribe((query) => { @@ -66,7 +66,6 @@ export const QueryAssistBar: React.FC = (props) => { setAgentError(undefined); previousQuestionRef.current = inputRef.current.value; persistedLog.add(inputRef.current.value); - updateQuestion(inputRef.current.value); const params: QueryAssistParameters = { question: inputRef.current.value, index: selectedIndex, @@ -88,6 +87,7 @@ export const QueryAssistBar: React.FC = (props) => { language: params.language, dataset: selectedDataset, }); + updateQueryState({ question: previousQuestionRef.current, answer: response.query }); if (response.timeRange) services.data.query.timefilter.timefilter.setTime(response.timeRange); setCallOutType('query_generated'); } diff --git a/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.test.tsx b/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.test.tsx index 09059494b48a..c6c54556cdc2 100644 --- a/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.test.tsx +++ b/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.test.tsx @@ -2,333 +2,414 @@ * Copyright OpenSearch Contributors * SPDX-License-Identifier: Apache-2.0 */ -import { fireEvent, render, screen } from '@testing-library/react'; -import React, { ComponentProps } from 'react'; -import { coreMock } from '../../../../../core/public/mocks'; +import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import React from 'react'; +import { dataPluginMock } from 'src/plugins/data/public/mocks'; +import { CoreSetup } from 'opensearch-dashboards/public'; +import { DataPublicPluginSetup, QueryEditorExtensionDependencies } from 'src/plugins/data/public'; import { BehaviorSubject } from 'rxjs'; -import { QueryAssistSummary, convertResult } from './query_assist_summary'; -import { useQueryAssist } from '../hooks'; -import { IDataFrame, Query } from '../../../../data/common'; -import { FeedbackStatus as FEEDBACK } from '../../../common/query_assist'; +import { usageCollectionPluginMock } from 'src/plugins/usage_collection/public/mocks'; -jest.mock('react', () => ({ - ...jest.requireActual('react'), - useState: jest.fn((value) => [value, () => null]), - useRef: jest.fn(() => ({ current: null })), -})); +import { convertResult, QueryAssistSummary } from './query_assist_summary'; +import { QueryAssistContext, QueryAssistContextValue } from '../hooks'; +import { DATA_FRAME_TYPES, IDataFrame } from '../../../../data/common'; +import { coreMock } from '../../../../../core/public/mocks'; +import { UsageCollectionSetup } from 'src/plugins/usage_collection/public'; -jest.mock('../hooks', () => ({ - useQueryAssist: jest.fn(), -})); +jest.useFakeTimers(); describe('query assist summary', () => { - const PPL = 'ppl'; - const question = 'Are there any errors in my logs?'; - const dataFrame = { - fields: [{ name: 'name', values: ['value'] }], - size: 1, - }; - const emptyDataFrame = { - fields: [], - size: 0, + const defaultCoreSetupMock = coreMock.createSetup(); + const defaultDataSetupMock = dataPluginMock.createSetupContract(); + const defaultUsageCollectionSetupMock = usageCollectionPluginMock.createSetupContract(); + const defaultDependenciesMock = { + language: 'PPL', + onSelectLanguage: jest.fn(), + // query editor is not collapsed + isCollapsed: false, + setIsCollapsed: jest.fn(), }; - const coreSetupMock = coreMock.createSetup({}); - const httpMock = coreSetupMock.http; - const data$ = new BehaviorSubject(undefined); - const question$ = new BehaviorSubject(''); - const query$ = new BehaviorSubject(undefined); - const reportUiStatsMock = jest.fn(); - const setSummary = jest.fn(); - const setLoading = jest.fn(); - const setFeedback = jest.fn(); - const setIsAssistantEnabledByCapability = jest.fn(); - const getQuery = jest.fn(); - const dataMock = { - query: { - queryString: { - getUpdates$: () => query$, - getQuery, - }, - }, - search: { - df: { - df$: data$, - }, - }, + const defaultCoreStartMock = coreMock.createStart(); + defaultCoreStartMock.application.capabilities = { + ...defaultCoreStartMock.application.capabilities, + // assistant is turned on + assistant: { enabled: true }, }; + defaultCoreSetupMock.getStartServices.mockResolvedValue([defaultCoreStartMock, {}, {}]); - afterEach(() => { - data$.next(undefined); - question$.next(''); - query$.next(undefined); - jest.clearAllMocks(); - }); - - const usageCollectionMock = { - reportUiStats: reportUiStatsMock, - METRIC_TYPE: { - CLICK: 'click', - }, - }; - const props: ComponentProps = { - data: dataMock, - http: httpMock, - usageCollection: usageCollectionMock, - dependencies: { - isCollapsed: false, - isSummaryCollapsed: false, - }, - core: coreSetupMock, - }; - - const LOADING = { - YES: true, - NO: false, - }; - const COLLAPSED = { - YES: true, - NO: false, - }; - - const renderQueryAssistSummary = (isCollapsed: boolean) => { + const renderQueryAssistSummary = ( + props: Partial, + deps?: { + coreSetup?: CoreSetup; + dataSetup?: DataPublicPluginSetup; + dependencies?: QueryEditorExtensionDependencies; + usageCollection?: UsageCollectionSetup; + } + ) => { + const mocks = { + coreSetup: defaultCoreSetupMock, + dataSetup: defaultDataSetupMock, + dependencies: defaultDependenciesMock, + usageCollection: defaultUsageCollectionSetupMock, + ...deps, + }; + const defaults: QueryAssistContextValue = { + // query assist is not collapsed + isQueryAssistCollapsed: false, + updateIsQueryAssistCollapsed: jest.fn(), + queryState: { question: '', answer: '' }, + updateQueryState: jest.fn(), + }; const component = render( -
+ -
+ ); - return component; + return (rerenderProps: Partial) => { + component.rerender( + + + + ); + }; }; - const sleep = (ms) => { - return new Promise((resolve) => setTimeout(resolve, ms)); - }; - const WAIT_TIME = 100; - - const mockUseState = ( - summary, - loading, - feedback, - isAssistantEnabledByCapability = true, - isQueryAssistCollapsed = COLLAPSED.NO - ) => { - React.useState.mockImplementationOnce(() => [summary, setSummary]); - React.useState.mockImplementationOnce(() => [loading, setLoading]); - React.useState.mockImplementationOnce(() => [feedback, setFeedback]); - React.useState.mockImplementationOnce(() => [ - isAssistantEnabledByCapability, - setIsAssistantEnabledByCapability, - ]); - useQueryAssist.mockImplementationOnce(() => ({ - question: 'question', - question$, - isQueryAssistCollapsed, - })); - }; + const setupInitScreen = async () => { + let rerender: (props: Partial) => any = jest.fn(); + const ppl = 'source=test | stats COUNT() as count'; + const question = 'test question'; + const summary = 'mock summary response'; + const dataFrame: IDataFrame = { + type: DATA_FRAME_TYPES.DEFAULT, + name: '73823750-90ec-11ef-8789-dd56e6283d4c', + size: 3, + fields: [ + { + name: 'count', + type: 'integer', + values: [1483, 79, 48], + }, + { + name: 'response', + type: 'string', + values: ['200', '404', '503'], + }, + ], + }; + const coreSetup = coreMock.createSetup(); + // mock fetchSummary so the http request takes 1s to response + coreSetup.http.post.mockImplementation( + () => new Promise((resolve) => setTimeout(() => resolve(summary), 1000)) + ); + // use defaultCoreSetupMock make to make sure assist capability is turned on + coreSetup.getStartServices.mockResolvedValue([defaultCoreStartMock, {}, {}]); + + const dataSetup = dataPluginMock.createSetupContract(); + dataSetup.search.df.df$ = new BehaviorSubject(undefined); + dataSetup.query.queryString.getQuery = jest + .fn() + .mockReturnValue({ query: ppl, language: 'PPL' }); + + const usageCollection = usageCollectionPluginMock.createSetupContract(); - const defaultUseStateMock = () => { - mockUseState(null, LOADING.NO, FEEDBACK.NONE); + await act(async () => { + rerender = renderQueryAssistSummary( + { + // ppl is generated + queryState: { question, answer: ppl }, + }, + { dataSetup, coreSetup, usageCollection } + ); + }); + return { + usageCollection, + coreSetup, + rerender, + dataSetup, + ppl, + dataFrame, + question, + summary, + }; }; - it('should not show if collapsed is true', () => { - defaultUseStateMock(); - renderQueryAssistSummary(COLLAPSED.YES); + afterEach(() => { + jest.clearAllTimers(); + }); + + it('should show summary component', async () => { + await act(async () => { + renderQueryAssistSummary({}); + }); const summaryPanels = screen.queryAllByTestId('queryAssist__summary'); - expect(summaryPanels).toHaveLength(0); + expect(summaryPanels).toHaveLength(1); }); - it('should not show if assistant is disabled by capability', () => { - mockUseState(null, LOADING.NO, FEEDBACK.NONE, false); - renderQueryAssistSummary(COLLAPSED.NO); + it('should not show if collapsed is true', async () => { + await act(async () => { + renderQueryAssistSummary( + {}, + { dependencies: { ...defaultDependenciesMock, isCollapsed: true } } + ); + }); const summaryPanels = screen.queryAllByTestId('queryAssist__summary'); expect(summaryPanels).toHaveLength(0); }); - it('should not show if query assistant is collapsed', () => { - mockUseState(null, LOADING.NO, FEEDBACK.NONE, true, COLLAPSED.YES); - renderQueryAssistSummary(COLLAPSED.NO); + it('should not show if assistant is disabled by capability', async () => { + const coreSetupAssistDisabledMock = coreMock.createSetup(); + const coreStartAssistDisabledMock = coreMock.createStart(); + coreStartAssistDisabledMock.application.capabilities = { + ...coreStartAssistDisabledMock.application.capabilities, + // assistant is turned off + assistant: { enabled: false }, + }; + coreSetupAssistDisabledMock.getStartServices.mockResolvedValue([ + coreStartAssistDisabledMock, + {}, + {}, + ]); + await act(async () => { + renderQueryAssistSummary({}, { coreSetup: coreSetupAssistDisabledMock }); + }); const summaryPanels = screen.queryAllByTestId('queryAssist__summary'); expect(summaryPanels).toHaveLength(0); }); - it('should show if collapsed is false', () => { - defaultUseStateMock(); - renderQueryAssistSummary(COLLAPSED.NO); + it('should not show if query assistant is collapsed', async () => { + await act(async () => { + renderQueryAssistSummary({ isQueryAssistCollapsed: true }); + }); const summaryPanels = screen.queryAllByTestId('queryAssist__summary'); - expect(summaryPanels).toHaveLength(1); + expect(summaryPanels).toHaveLength(0); }); - it('should display loading view if loading state is true', () => { - mockUseState(null, LOADING.YES, FEEDBACK.NONE); - renderQueryAssistSummary(COLLAPSED.NO); - expect(screen.getByTestId('queryAssist_summary_loading')).toBeInTheDocument(); - expect(screen.queryAllByTestId('queryAssist_summary_result')).toHaveLength(0); - expect(screen.queryAllByTestId('queryAssist_summary_empty_text')).toHaveLength(0); - }); + it('should load the summary automatically only the first time ppl query was generated', async () => { + const { coreSetup, dataSetup, rerender, dataFrame, summary } = await setupInitScreen(); + await act(async () => { + // ppl query returned results + dataSetup.search.df.df$.next(dataFrame); + }); - it('should display loading view if loading state is true even with summary', () => { - mockUseState('summary', LOADING.YES, FEEDBACK.NONE); - renderQueryAssistSummary(COLLAPSED.NO); - expect(screen.getByTestId('queryAssist_summary_loading')).toBeInTheDocument(); - expect(screen.queryAllByTestId('queryAssist_summary_result')).toHaveLength(0); - expect(screen.queryAllByTestId('queryAssist_summary_empty_text')).toHaveLength(0); - }); + await waitFor(() => { + // the generate summary request is sent only once + expect(coreSetup.http.post).toHaveBeenCalledTimes(1); + expect(screen.getByTestId('queryAssist_summary_loading')).toBeInTheDocument(); + }); - it('should display initial view if loading state is false and no summary', () => { - defaultUseStateMock(); - renderQueryAssistSummary(COLLAPSED.NO); - expect(screen.getByTestId('queryAssist_summary_empty_text')).toBeInTheDocument(); - expect(screen.queryAllByTestId('queryAssist_summary_result')).toHaveLength(0); - expect(screen.queryAllByTestId('queryAssist_summary_loading')).toHaveLength(0); + // after 1s, the fetch summary request respond + jest.advanceTimersByTime(1000); + await waitFor(() => { + // the summary result is displayed + expect(screen.getByText(summary)).toBeInTheDocument(); + }); + + // ppl query is generated another time + const anotherPPL = 'source=another_test | stats COUNT() as count'; + dataSetup.query.queryString.getQuery = jest + .fn() + .mockReturnValue({ query: anotherPPL, language: 'PPL' }); + + await act(async () => { + rerender({ queryState: { question: 'another question', answer: anotherPPL } }); + // ppl query returned results + dataSetup.search.df.df$.next({ + type: DATA_FRAME_TYPES.DEFAULT, + name: '73823750-90ec-11ef-8789-dd56e6283d4c', + size: 2, + fields: [ + { + name: 'count', + type: 'integer', + values: [1483, 79], + }, + { + name: 'response', + type: 'string', + values: ['200', '404'], + }, + ], + }); + }); + await waitFor(() => { + // The generate summary request should not be called the second time because the second time + // ppl query generated should not auto-trigger the summarization + expect(coreSetup.http.post).toHaveBeenCalledTimes(1); + expect(screen.getByTestId('queryAssist_summary_click_to_generate')).toBeInTheDocument(); + }); + + fireEvent.click(screen.getByTestId('queryAssist_summary_buttons_generate')); + await waitFor(() => { + // Clicking "generate summary" should trigger the request summary call + expect(coreSetup.http.post).toHaveBeenCalledTimes(2); + expect(screen.getByTestId('queryAssist_summary_loading')).toBeInTheDocument(); + }); + // after 1s, the fetch summary request respond + jest.advanceTimersByTime(1000); + await waitFor(() => { + // the summary result is displayed + expect(screen.getByText(summary)).toBeInTheDocument(); + }); }); - it('should display summary result', () => { - mockUseState('summary', LOADING.NO, FEEDBACK.NONE); - renderQueryAssistSummary(COLLAPSED.NO); - expect(screen.getByTestId('queryAssist_summary_result')).toBeInTheDocument(); - expect(screen.getByTestId('queryAssist_summary_result')).toHaveTextContent('summary'); - expect(screen.queryAllByTestId('queryAssist_summary_empty_text')).toHaveLength(0); - expect(screen.queryAllByTestId('queryAssist_summary_loading')).toHaveLength(0); + it('should display initial view', async () => { + await act(async () => { + renderQueryAssistSummary({}); + }); + expect(screen.getByTestId('queryAssist_summary_empty_text')).toBeInTheDocument(); + expect(screen.getByTestId('queryAssist_summary_buttons_generate')).toBeDisabled(); }); it('should report metric for thumbup click', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.NONE); - renderQueryAssistSummary(COLLAPSED.NO); - expect(screen.getByTestId('queryAssist_summary_result')).toBeInTheDocument(); - await screen.getByTestId('queryAssist_summary_buttons_thumbup'); + const { usageCollection, dataSetup, dataFrame, summary } = await setupInitScreen(); + await act(async () => { + // ppl query returned results + dataSetup.search.df.df$.next(dataFrame); + }); + // after 1s, the fetch summary request respond + jest.advanceTimersByTime(1000); + await waitFor(() => { + // the summary result is displayed + expect(screen.getByText(summary)).toBeInTheDocument(); + }); + fireEvent.click(screen.getByTestId('queryAssist_summary_buttons_thumbup')); - expect(setFeedback).toHaveBeenCalledWith(FEEDBACK.THUMB_UP); - expect(reportUiStatsMock).toHaveBeenCalledWith( + expect(usageCollection.reportUiStats).toHaveBeenCalledWith( 'query-assist', 'click', expect.stringMatching(/^thumbup/) ); + // After clicking thumb up, only thumb up button will be visible + expect(screen.queryByTestId('queryAssist_summary_buttons_thumbup')).toBeInTheDocument(); + expect(screen.queryByTestId('queryAssist_summary_buttons_thumbdown')).not.toBeInTheDocument(); }); it('should report metric for thumbdown click', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.NONE); - renderQueryAssistSummary(COLLAPSED.NO); - expect(screen.getByTestId('queryAssist_summary_result')).toBeInTheDocument(); - await screen.getByTestId('queryAssist_summary_buttons_thumbdown'); + const { usageCollection, dataSetup, dataFrame, summary } = await setupInitScreen(); + await act(async () => { + // ppl query returned results + dataSetup.search.df.df$.next(dataFrame); + }); + // after 1s, the fetch summary request respond + jest.advanceTimersByTime(1000); + await waitFor(() => { + // the summary result is displayed + expect(screen.getByText(summary)).toBeInTheDocument(); + }); fireEvent.click(screen.getByTestId('queryAssist_summary_buttons_thumbdown')); - expect(setFeedback).toHaveBeenCalledWith(FEEDBACK.THUMB_DOWN); - expect(reportUiStatsMock).toHaveBeenCalledWith( + expect(usageCollection.reportUiStats).toHaveBeenCalledWith( 'query-assist', 'click', expect.stringMatching(/^thumbdown/) ); - }); - - it('should hide thumbdown button if thumbup button is clicked', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.THUMB_UP); - renderQueryAssistSummary(COLLAPSED.NO); - expect(screen.getByTestId('queryAssist_summary_result')).toBeInTheDocument(); - await screen.getByTestId('queryAssist_summary_buttons_thumbup'); - expect(screen.queryByTestId('queryAssist_summary_buttons_thumbdown')).not.toBeInTheDocument(); - }); - - it('should hide thumbup button if thumbdown button is clicked', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.THUMB_DOWN); - renderQueryAssistSummary(COLLAPSED.NO); - expect(screen.getByTestId('queryAssist_summary_result')).toBeInTheDocument(); - await screen.getByTestId('queryAssist_summary_buttons_thumbdown'); + // After clicking thumb down, only thumb down button will be visible + expect(screen.queryByTestId('queryAssist_summary_buttons_thumbdown')).toBeInTheDocument(); expect(screen.queryByTestId('queryAssist_summary_buttons_thumbup')).not.toBeInTheDocument(); }); it('should not fetch summary if data is empty', async () => { - mockUseState(null, LOADING.NO, FEEDBACK.NONE); - renderQueryAssistSummary(COLLAPSED.NO); - question$.next(question); - query$.next({ query: PPL, language: 'PPL' }); - data$.next(emptyDataFrame); - expect(httpMock.post).toBeCalledTimes(0); + const firstPPL = 'source=test | stats COUNT() as count'; + const dataSetup = dataPluginMock.createSetupContract(); + dataSetup.search.df.df$ = new BehaviorSubject(undefined); + dataSetup.query.queryString.getQuery = jest + .fn() + .mockReturnValue({ query: firstPPL, language: 'PPL' }); + + await act(async () => { + renderQueryAssistSummary( + { + queryState: { question: 'test question', answer: firstPPL }, + }, + { dataSetup } + ); + // ppl query returned empty results + dataSetup.search.df.df$.next({ + type: DATA_FRAME_TYPES.DEFAULT, + name: '73823750-90ec-11ef-8789-dd56e6283d4c', + size: 0, + fields: [], + }); + }); + expect(screen.getByTestId('queryAssist_summary_empty_text')).toBeInTheDocument(); + expect(defaultCoreSetupMock.http.post).not.toHaveBeenCalled(); }); - it('should fetch summary with expected payload and response', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.NONE); - const RESPONSE_TEXT = 'response'; - httpMock.post.mockResolvedValue(RESPONSE_TEXT); - renderQueryAssistSummary(COLLAPSED.NO); - question$.next(question); - query$.next({ query: PPL, language: 'PPL' }); - data$.next(dataFrame as IDataFrame); - await sleep(WAIT_TIME); - expect(httpMock.post).toBeCalledWith('/api/assistant/data2summary', { + it('should fetch summary with expected payload', async () => { + const { coreSetup, question, ppl, dataFrame, dataSetup } = await setupInitScreen(); + await act(async () => { + // ppl query returned results + dataSetup.search.df.df$.next(dataFrame); + }); + expect(coreSetup.http.post).toBeCalledWith('/api/assistant/data2summary', { body: JSON.stringify({ sample_data: `'${JSON.stringify(convertResult(dataFrame))}'`, - sample_count: 1, - total_count: 1, + sample_count: 3, + total_count: 3, question, - ppl: PPL, + ppl, }), query: { dataSourceId: undefined, }, }); - expect(setSummary).toHaveBeenNthCalledWith(1, ''); - expect(setSummary).toHaveBeenNthCalledWith(2, RESPONSE_TEXT); - expect(setLoading).toHaveBeenNthCalledWith(1, true); - expect(setLoading).toHaveBeenNthCalledWith(2, false); }); it('should handle fetch summary error', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.NONE); - httpMock.post.mockRejectedValueOnce({}); - renderQueryAssistSummary(COLLAPSED.NO); - question$.next(question); - query$.next({ query: PPL, language: 'PPL' }); - data$.next(dataFrame as IDataFrame); - await sleep(WAIT_TIME); - expect(setSummary).toBeCalledTimes(2); - expect(setLoading).toHaveBeenNthCalledWith(1, true); - expect(setLoading).toHaveBeenNthCalledWith(2, false); - }); + const { coreSetup, dataFrame, dataSetup } = await setupInitScreen(); + coreSetup.http.post.mockRejectedValueOnce({}); - it('should not update queryResults if subscription changed not in order', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.NONE); - const RESPONSE_TEXT = 'response'; - httpMock.post.mockResolvedValue(RESPONSE_TEXT); - renderQueryAssistSummary(COLLAPSED.NO); - data$.next(dataFrame as IDataFrame); - question$.next(question); - query$.next({ query: PPL, language: 'PPL' }); - await sleep(WAIT_TIME); - expect(httpMock.post).toHaveBeenCalledTimes(0); - }); + await act(async () => { + // ppl query returned results + dataSetup.search.df.df$.next(dataFrame); + }); - it('should update queryResults if subscriptions changed in order', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.NONE); - const RESPONSE_TEXT = 'response'; - httpMock.post.mockResolvedValue(RESPONSE_TEXT); - renderQueryAssistSummary(COLLAPSED.NO); - question$.next(question); - query$.next({ query: PPL, language: 'PPL' }); - data$.next(dataFrame as IDataFrame); - await sleep(WAIT_TIME); - expect(httpMock.post).toHaveBeenCalledTimes(1); - data$.next(undefined); - question$.next(question); - query$.next({ query: PPL, language: 'PPL' }); - data$.next(dataFrame as IDataFrame); - await sleep(WAIT_TIME); - expect(httpMock.post).toHaveBeenCalledTimes(2); + await waitFor(() => { + expect( + screen.queryByText('I am unable to respond to this query. Try another question.') + ).toBeInTheDocument(); + }); }); it('should reset feedback state if re-fetch summary', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.THUMB_UP); - const RESPONSE_TEXT = 'response'; - httpMock.post.mockResolvedValue(RESPONSE_TEXT); - renderQueryAssistSummary(COLLAPSED.NO); - question$.next(question); - query$.next({ query: PPL, language: 'PPL' }); - data$.next(dataFrame as IDataFrame); - await sleep(WAIT_TIME); - expect(setFeedback).toHaveBeenCalledWith(FEEDBACK.NONE); + const { dataSetup, dataFrame, summary } = await setupInitScreen(); + await act(async () => { + // ppl query returned results + dataSetup.search.df.df$.next(dataFrame); + }); + // after 1s, the fetch summary request respond + jest.advanceTimersByTime(1000); + await waitFor(() => { + // the summary result is displayed + expect(screen.getByText(summary)).toBeInTheDocument(); + }); + + fireEvent.click(screen.getByTestId('queryAssist_summary_buttons_thumbdown')); + // After clicking thumb down, only thumb down button will be visible + expect(screen.queryByTestId('queryAssist_summary_buttons_thumbdown')).toBeInTheDocument(); + expect(screen.queryByTestId('queryAssist_summary_buttons_thumbup')).not.toBeInTheDocument(); + + // Click generate summary + fireEvent.click(screen.getByTestId('queryAssist_summary_buttons_generate')); + // after 1s, the fetch summary request respond + jest.advanceTimersByTime(1000); + await waitFor(() => { + // the summary result is displayed + expect(screen.getByText(summary)).toBeInTheDocument(); + }); + // both thumb up/down should be visible + expect(screen.queryByTestId('queryAssist_summary_buttons_thumbdown')).toBeInTheDocument(); + expect(screen.queryByTestId('queryAssist_summary_buttons_thumbup')).toBeInTheDocument(); }); }); diff --git a/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx b/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx index 9a20c98fcbbd..f2241afb0246 100644 --- a/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx +++ b/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx @@ -12,6 +12,7 @@ import { EuiSmallButtonIcon, EuiSpacer, EuiCopy, + EuiSmallButtonEmpty, } from '@elastic/eui'; import React, { useEffect, useState, useCallback, useRef } from 'react'; @@ -19,14 +20,12 @@ import { i18n } from '@osd/i18n'; import { IDataFrame } from 'src/plugins/data/common'; import { v4 as uuidv4 } from 'uuid'; import { isEmpty } from 'lodash'; -import { merge, of } from 'rxjs'; -import { filter, distinctUntilChanged, mergeMap } from 'rxjs/operators'; +import { filter, distinctUntilChanged } from 'rxjs/operators'; import { HttpSetup } from 'opensearch-dashboards/public'; -import { useQueryAssist } from '../hooks'; +import { QueryAssistState, useQueryAssist } from '../hooks'; import { DataPublicPluginSetup, QueryEditorExtensionDependencies } from '../../../../data/public'; import { UsageCollectionSetup } from '../../../../usage_collection/public'; import { CoreSetup } from '../../../../../core/public'; -import { QueryAssistContextType } from '../../../common/query_assist'; import sparkleHollowSvg from '../../assets/sparkle_hollow.svg'; import sparkleSolidSvg from '../../assets/sparkle_solid.svg'; import { FeedbackStatus } from '../../../common/query_assist'; @@ -71,7 +70,14 @@ export const QueryAssistSummary: React.FC = (props) => const [feedback, setFeedback] = useState(FeedbackStatus.NONE); const [isEnabledByCapability, setIsEnabledByCapability] = useState(false); const selectedDataset = useRef(query.queryString.getQuery()?.dataset); - const { question$, isQueryAssistCollapsed } = useQueryAssist(); + const { isQueryAssistCollapsed, queryState } = useQueryAssist(); + + const [results, setResults] = useState([]); + // the question and answer used last time to generate summary + const lastUsedQueryStateRef = useRef(); + // the current question and generated answer + const currentQueryStateRef = useRef(queryState); + const METRIC_APP = `query-assist`; const afterFeedbackTip = i18n.translate('queryEnhancements.queryAssist.summary.afterFeedback', { defaultMessage: @@ -83,18 +89,23 @@ export const QueryAssistSummary: React.FC = (props) => const sampleSize = 10; - const reportMetric = useCallback( - (metric: string) => { - if (props.usageCollection) { - props.usageCollection.reportUiStats( - METRIC_APP, - props.usageCollection.METRIC_TYPE.CLICK, - metric + '-' + uuidv4() - ); - } - }, - [props.usageCollection, METRIC_APP] - ); + const queryChanged = + queryState.answer && queryState.answer !== props.data.query.queryString.getQuery().query; + // It can generate summary when + // 1. it has the current generated query(answer) + // 2. the generated query didn't been changed by user + // 3. there are search results + const canGenerateSummary = Boolean(results.length) && Boolean(queryState.answer) && !queryChanged; + // Generate summary can be auto triggered only when first time generating the query + const shouldAutoTrigger = !lastUsedQueryStateRef.current; + // Display a message in the panel to indicate that clicking + // "Generate summary" button is needed to generate the summary + const manualTriggerVisible = + !shouldAutoTrigger && + canGenerateSummary && + lastUsedQueryStateRef.current?.answer !== queryState.answer; + // The visibility of panel action buttons: thumbs up/down and copy to clipboard buttons + const actionButtonVisible = summary && !loading && !queryChanged && !manualTriggerVisible; const reportCountMetric = useCallback( (metric: string, count: number) => { @@ -110,13 +121,6 @@ export const QueryAssistSummary: React.FC = (props) => [props.usageCollection, METRIC_APP] ); - useEffect(() => { - const subscription = query.queryString.getUpdates$().subscribe((_query) => { - selectedDataset.current = _query?.dataset; - }); - return () => subscription.unsubscribe(); - }, [query.queryString]); - const fetchSummary = useCallback( async (queryContext: QueryContext) => { if (isEmpty(queryContext?.queryResults)) return; @@ -141,6 +145,10 @@ export const QueryAssistSummary: React.FC = (props) => }, }); setSummary(response); + lastUsedQueryStateRef.current = { + question: queryContext.question, + answer: queryContext.query, + }; reportCountMetric(SUCCESS_METRIC, 1); } catch (error) { reportCountMetric(SUCCESS_METRIC, 0); @@ -153,51 +161,51 @@ export const QueryAssistSummary: React.FC = (props) => ); useEffect(() => { - let dataStack: Array = []; - const subscription = merge( - question$.pipe( - filter((value) => !isEmpty(value)), - mergeMap((value) => of({ type: QueryAssistContextType.QUESTION as const, data: value })) - ), - query.queryString.getUpdates$().pipe( - filter((value) => !isEmpty(value)), - mergeMap((value) => of({ type: QueryAssistContextType.QUERY as const, data: value })) - ), - search.df?.df$?.pipe( - distinctUntilChanged(), - filter((value) => !isEmpty(value) && !isEmpty(value?.fields)), - mergeMap((value) => of({ type: QueryAssistContextType.DATA as const, data: value })) - ) - ).subscribe((value) => { - // to ensure we only trigger summary when user hits the query assist button with natural language input - switch (value.type) { - case QueryAssistContextType.QUESTION: - dataStack = [value.data]; - break; - case QueryAssistContextType.QUERY: - if (dataStack.length === 1) { - dataStack.push(value.data.query as string); - } - break; - case QueryAssistContextType.DATA: - if (dataStack.length === 2) { - dataStack.push(value.data); - fetchSummary({ - question: dataStack[0] as string, - query: dataStack[1] as string, - queryResults: convertResult(dataStack[2] as IDataFrame), - }); - dataStack = []; - } - break; - default: - break; + currentQueryStateRef.current = queryState; + }, [queryState]); + + useEffect(() => { + const { question, answer } = currentQueryStateRef.current; + if (shouldAutoTrigger && canGenerateSummary) { + fetchSummary({ question, query: answer, queryResults: results }); + } + }, [results, canGenerateSummary, fetchSummary, shouldAutoTrigger]); + + const reportMetric = useCallback( + (metric: string) => { + if (props.usageCollection) { + props.usageCollection.reportUiStats( + METRIC_APP, + props.usageCollection.METRIC_TYPE.CLICK, + metric + '-' + uuidv4() + ); } + }, + [props.usageCollection, METRIC_APP] + ); + + useEffect(() => { + const subscription = query.queryString.getUpdates$().subscribe((_query) => { + selectedDataset.current = _query?.dataset; }); + return () => subscription.unsubscribe(); + }, [query.queryString]); + + useEffect(() => { + const subscription = search.df.df$ + .pipe( + distinctUntilChanged(), + filter((value) => !isEmpty(value) && !isEmpty(value?.fields)) + ) + .subscribe((df) => { + if (df) { + setResults(convertResult(df)); + } + }); return () => { subscription.unsubscribe(); }; - }, [question$, query.queryString, search.df?.df$, fetchSummary]); + }, [search.df.df$]); useEffect(() => { props.core.getStartServices().then(([coreStart, depsStart]) => { @@ -216,8 +224,57 @@ export const QueryAssistSummary: React.FC = (props) => [feedback, reportMetric] ); - if (props.dependencies.isCollapsed || isQueryAssistCollapsed || !isEnabledByCapability) + const getPanelMessage = useCallback(() => { + if (loading) { + return ( + + {i18n.translate('queryEnhancements.queryAssist.summary.generating', { + defaultMessage: 'Generating response...', + })} + + ); + } + if (queryChanged) { + return ( + + {i18n.translate('queryEnhancements.queryAssist.summary.generating', { + defaultMessage: 'Summary unavailable for custom queries', + })} + + ); + } + + if (manualTriggerVisible) { + return ( + + {i18n.translate('queryEnhancements.queryAssist.summary.clickToGenerate', { + defaultMessage: 'Select the "Generate summary" button to generate summaries', + })} + + ); + } + + if (summary) { + return ( + + {summary} + + ); + } + + return ( + + {i18n.translate('queryEnhancements.queryAssist.summary.placeholder', { + defaultMessage: 'Ask a question to generate summary', + })} + + ); + }, [loading, queryChanged, summary, manualTriggerVisible]); + + if (props.dependencies.isCollapsed || isQueryAssistCollapsed || !isEnabledByCapability) { return null; + } + const isDarkMode = props.core.uiSettings.get('theme:darkMode'); return ( = (props) => - {summary && !loading && ( - - - - - - {feedback !== FeedbackStatus.THUMB_DOWN && ( + + + {actionButtonVisible && ( + <> - onFeedback(true)} - data-test-subj="queryAssist_summary_buttons_thumbup" + - )} - {feedback !== FeedbackStatus.THUMB_UP && ( - - onFeedback(false)} - data-test-subj="queryAssist_summary_buttons_thumbdown" - /> - - )} - - - - {(copy) => ( + {feedback !== FeedbackStatus.THUMB_DOWN && ( + + onFeedback(true)} + data-test-subj="queryAssist_summary_buttons_thumbup" + /> + + )} + {feedback !== FeedbackStatus.THUMB_UP && ( + onFeedback(false)} + data-test-subj="queryAssist_summary_buttons_thumbdown" /> - )} - - - - - )} + + )} + + + + {(copy) => ( + + )} + + + + )} + + + + fetchSummary({ + question: queryState.question, + query: queryState.answer, + queryResults: results, + }) + } + data-test-subj="queryAssist_summary_buttons_generate" + > + {i18n.translate('queryEnhancements.queryAssist.summary.generateSummary', { + defaultMessage: 'Generate summary', + })} + + + + - - {!summary && !loading && ( - - {i18n.translate('queryEnhancements.queryAssist.summary.placeholder', { - defaultMessage: `Ask a question to generate summary.`, - })} - - )} - {loading && ( - - {i18n.translate('queryEnhancements.queryAssist.summary.generating', { - defaultMessage: `Generating response...`, - })} - - )} - {summary && !loading && ( - - {summary} - - )} - + {getPanelMessage()} ); }; diff --git a/src/plugins/query_enhancements/public/query_assist/hooks/use_query_assist.ts b/src/plugins/query_enhancements/public/query_assist/hooks/use_query_assist.ts index f4cedeb201e5..eb3dde7ccdc7 100644 --- a/src/plugins/query_enhancements/public/query_assist/hooks/use_query_assist.ts +++ b/src/plugins/query_enhancements/public/query_assist/hooks/use_query_assist.ts @@ -3,14 +3,17 @@ * SPDX-License-Identifier: Apache-2.0 */ import React from 'react'; -import { BehaviorSubject } from 'rxjs'; -export interface QueryAssistContextValue { +export interface QueryAssistState { question: string; - question$: BehaviorSubject; - updateQuestion: (question: string) => void; + answer: string; +} + +export interface QueryAssistContextValue { isQueryAssistCollapsed: boolean; updateIsQueryAssistCollapsed: (isCollapsed: boolean) => void; + queryState: QueryAssistState; + updateQueryState: (state: QueryAssistState) => void; } export const QueryAssistContext = React.createContext( {} as QueryAssistContextValue diff --git a/src/plugins/query_enhancements/public/query_assist/utils/create_extension.tsx b/src/plugins/query_enhancements/public/query_assist/utils/create_extension.tsx index 6cfb81d90186..3e2bc1ccd05b 100644 --- a/src/plugins/query_enhancements/public/query_assist/utils/create_extension.tsx +++ b/src/plugins/query_enhancements/public/query_assist/utils/create_extension.tsx @@ -5,7 +5,7 @@ import { i18n } from '@osd/i18n'; import { HttpSetup } from 'opensearch-dashboards/public'; -import React, { useEffect, useState } from 'react'; +import React, { useCallback, useEffect, useState } from 'react'; import { BehaviorSubject } from 'rxjs'; import { distinctUntilChanged, map, startWith, switchMap } from 'rxjs/operators'; import { DATA_STRUCTURE_META_TYPES, DEFAULT_DATA } from '../../../../data/common'; @@ -24,7 +24,7 @@ import { QueryAssistButton, } from '../components'; import { UsageCollectionSetup } from '../../../../usage_collection/public'; -import { QueryAssistContext } from '../hooks/use_query_assist'; +import { QueryAssistContext, QueryAssistState } from '../hooks/use_query_assist'; import { CoreSetup } from '../../../../../core/public'; const [getAvailableLanguagesForDataSource, clearCache] = (() => { @@ -93,7 +93,7 @@ export const createQueryAssistExtension = ( ): QueryEditorExtensionConfig => { const http: HttpSetup = core.http; const isQueryAssistCollapsed$ = new BehaviorSubject(false); - const question$ = new BehaviorSubject(''); + const assistQueryState$ = new BehaviorSubject({ question: '', answer: '' }); return { id: 'query-assist', order: 1000, @@ -121,7 +121,7 @@ export const createQueryAssistExtension = ( http={http} data={data} isQueryAssistCollapsed$={isQueryAssistCollapsed$} - question$={question$} + queryState$={assistQueryState$} > {config.summary.enabled && ( @@ -139,7 +139,14 @@ export const createQueryAssistExtension = ( getBanner: (dependencies) => { // advertise query assist if user is not on a supported language. return ( - + conf.language)} @@ -153,6 +160,7 @@ export const createQueryAssistExtension = ( dependencies={dependencies} http={http} data={data} + queryState$={assistQueryState$} isQueryAssistCollapsed$={isQueryAssistCollapsed$} > @@ -167,37 +175,40 @@ interface QueryAssistWrapperProps { http: HttpSetup; data: DataPublicPluginSetup; invert?: boolean; - isQueryAssistCollapsed$?: BehaviorSubject; - question$?: BehaviorSubject; + isQueryAssistCollapsed$: BehaviorSubject; + queryState$: BehaviorSubject; } const QueryAssistWrapper: React.FC = (props) => { const [visible, setVisible] = useState(false); - const [question, setQuestion] = useState(''); const [isQueryAssistCollapsed, setIsQueryAssistCollapsed] = useState(true); - const updateQuestion = (newQuestion: string) => { - props.question$?.next(newQuestion); - }; - const question$ = props.question$; + // The current successfully generated query + const [queryState, setQueryState] = useState(props.queryState$.value); const updateIsQueryAssistCollapsed = (isCollapsed: boolean) => { - props.isQueryAssistCollapsed$?.next(isCollapsed); + props.isQueryAssistCollapsed$.next(isCollapsed); }; + const updateQueryState = useCallback( + (newState: QueryAssistState) => { + props.queryState$.next(newState); + }, + [props.queryState$] + ); + useEffect(() => { - const subscription = props.isQueryAssistCollapsed$?.subscribe((isCollapsed) => { + const subscription = props.isQueryAssistCollapsed$.subscribe((isCollapsed) => { setIsQueryAssistCollapsed(isCollapsed); }); - const questionSubscription = props.question$?.subscribe((newQuestion) => { - setQuestion(newQuestion); + const queryStateSubscription = props.queryState$.subscribe((newState) => { + setQueryState(newState); }); return () => { - questionSubscription?.unsubscribe(); subscription?.unsubscribe(); + queryStateSubscription.unsubscribe(); }; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [props.isQueryAssistCollapsed$, props.queryState$]); useEffect(() => { let mounted = true; @@ -218,11 +229,10 @@ const QueryAssistWrapper: React.FC = (props) => { <> {props.children}