From 9365ad624b5f298cedbe8e56c17e79c2971c1199 Mon Sep 17 00:00:00 2001 From: "opensearch-trigger-bot[bot]" <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Date: Sat, 12 Oct 2024 13:43:10 +0800 Subject: [PATCH] Add discover summary error info and button click logic enhancement (#8352) (#8568) * Add discover summary error info and button click logic enhancement * add changelog * Address comments --------- (cherry picked from commit c4049eb5b042edd7ca573aac8e462f581e54f5bf) Signed-off-by: Liyun Xiu Signed-off-by: github-actions[bot] Co-authored-by: github-actions[bot] --- changelogs/fragments/8352.yml | 2 + .../common/query_assist/index.ts | 7 +- .../common/query_assist/types.ts | 6 ++ .../components/query_assist_summary.test.tsx | 59 +++++++----- .../components/query_assist_summary.tsx | 90 ++++++++++--------- 5 files changed, 100 insertions(+), 64 deletions(-) create mode 100644 changelogs/fragments/8352.yml diff --git a/changelogs/fragments/8352.yml b/changelogs/fragments/8352.yml new file mode 100644 index 000000000000..048f8a94a22a --- /dev/null +++ b/changelogs/fragments/8352.yml @@ -0,0 +1,2 @@ +fix: +- Add discover summary error info and button click logic enhancement ([#8352](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8352)) diff --git a/src/plugins/query_enhancements/common/query_assist/index.ts b/src/plugins/query_enhancements/common/query_assist/index.ts index 7c577db88834..0541207e49f8 100644 --- a/src/plugins/query_enhancements/common/query_assist/index.ts +++ b/src/plugins/query_enhancements/common/query_assist/index.ts @@ -3,4 +3,9 @@ * SPDX-License-Identifier: Apache-2.0 */ -export { QueryAssistParameters, QueryAssistResponse, QueryAssistContextType } from './types'; +export { + QueryAssistParameters, + QueryAssistResponse, + QueryAssistContextType, + FeedbackStatus, +} from './types'; diff --git a/src/plugins/query_enhancements/common/query_assist/types.ts b/src/plugins/query_enhancements/common/query_assist/types.ts index d191befad5e7..a10da649d573 100644 --- a/src/plugins/query_enhancements/common/query_assist/types.ts +++ b/src/plugins/query_enhancements/common/query_assist/types.ts @@ -23,3 +23,9 @@ export enum QueryAssistContextType { QUERY, DATA, } + +export enum FeedbackStatus { + NONE = 'none', + THUMB_UP = 'thumbup', + THUMB_DOWN = 'thumbdown', +} 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 4ec5289a3fa0..09059494b48a 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 @@ -9,6 +9,7 @@ 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'; jest.mock('react', () => ({ ...jest.requireActual('react'), @@ -89,10 +90,6 @@ describe('query assist summary', () => { YES: true, NO: false, }; - const FEEDBACK = { - YES: true, - NO: false, - }; const renderQueryAssistSummary = (isCollapsed: boolean) => { const component = render( @@ -136,7 +133,7 @@ describe('query assist summary', () => { }; const defaultUseStateMock = () => { - mockUseState(null, LOADING.NO, FEEDBACK.NO); + mockUseState(null, LOADING.NO, FEEDBACK.NONE); }; it('should not show if collapsed is true', () => { @@ -147,14 +144,14 @@ describe('query assist summary', () => { }); it('should not show if assistant is disabled by capability', () => { - mockUseState(null, LOADING.NO, FEEDBACK.NO, false); + mockUseState(null, LOADING.NO, FEEDBACK.NONE, false); renderQueryAssistSummary(COLLAPSED.NO); const summaryPanels = screen.queryAllByTestId('queryAssist__summary'); expect(summaryPanels).toHaveLength(0); }); it('should not show if query assistant is collapsed', () => { - mockUseState(null, LOADING.NO, FEEDBACK.NO, true, COLLAPSED.YES); + mockUseState(null, LOADING.NO, FEEDBACK.NONE, true, COLLAPSED.YES); renderQueryAssistSummary(COLLAPSED.NO); const summaryPanels = screen.queryAllByTestId('queryAssist__summary'); expect(summaryPanels).toHaveLength(0); @@ -168,7 +165,7 @@ describe('query assist summary', () => { }); it('should display loading view if loading state is true', () => { - mockUseState(null, LOADING.YES, FEEDBACK.NO); + mockUseState(null, LOADING.YES, FEEDBACK.NONE); renderQueryAssistSummary(COLLAPSED.NO); expect(screen.getByTestId('queryAssist_summary_loading')).toBeInTheDocument(); expect(screen.queryAllByTestId('queryAssist_summary_result')).toHaveLength(0); @@ -176,7 +173,7 @@ describe('query assist summary', () => { }); it('should display loading view if loading state is true even with summary', () => { - mockUseState('summary', LOADING.YES, FEEDBACK.NO); + mockUseState('summary', LOADING.YES, FEEDBACK.NONE); renderQueryAssistSummary(COLLAPSED.NO); expect(screen.getByTestId('queryAssist_summary_loading')).toBeInTheDocument(); expect(screen.queryAllByTestId('queryAssist_summary_result')).toHaveLength(0); @@ -192,7 +189,7 @@ describe('query assist summary', () => { }); it('should display summary result', () => { - mockUseState('summary', LOADING.NO, FEEDBACK.NO); + mockUseState('summary', LOADING.NO, FEEDBACK.NONE); renderQueryAssistSummary(COLLAPSED.NO); expect(screen.getByTestId('queryAssist_summary_result')).toBeInTheDocument(); expect(screen.getByTestId('queryAssist_summary_result')).toHaveTextContent('summary'); @@ -201,12 +198,12 @@ describe('query assist summary', () => { }); it('should report metric for thumbup click', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.NO); + mockUseState('summary', LOADING.NO, FEEDBACK.NONE); renderQueryAssistSummary(COLLAPSED.NO); expect(screen.getByTestId('queryAssist_summary_result')).toBeInTheDocument(); await screen.getByTestId('queryAssist_summary_buttons_thumbup'); fireEvent.click(screen.getByTestId('queryAssist_summary_buttons_thumbup')); - expect(setFeedback).toHaveBeenCalledWith(true); + expect(setFeedback).toHaveBeenCalledWith(FEEDBACK.THUMB_UP); expect(reportUiStatsMock).toHaveBeenCalledWith( 'query-assist', 'click', @@ -215,12 +212,12 @@ describe('query assist summary', () => { }); it('should report metric for thumbdown click', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.NO); + mockUseState('summary', LOADING.NO, FEEDBACK.NONE); renderQueryAssistSummary(COLLAPSED.NO); expect(screen.getByTestId('queryAssist_summary_result')).toBeInTheDocument(); await screen.getByTestId('queryAssist_summary_buttons_thumbdown'); fireEvent.click(screen.getByTestId('queryAssist_summary_buttons_thumbdown')); - expect(setFeedback).toHaveBeenCalledWith(true); + expect(setFeedback).toHaveBeenCalledWith(FEEDBACK.THUMB_DOWN); expect(reportUiStatsMock).toHaveBeenCalledWith( 'query-assist', 'click', @@ -228,8 +225,24 @@ describe('query assist summary', () => { ); }); + 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'); + expect(screen.queryByTestId('queryAssist_summary_buttons_thumbup')).not.toBeInTheDocument(); + }); + it('should not fetch summary if data is empty', async () => { - mockUseState(null, LOADING.NO, FEEDBACK.NO); + mockUseState(null, LOADING.NO, FEEDBACK.NONE); renderQueryAssistSummary(COLLAPSED.NO); question$.next(question); query$.next({ query: PPL, language: 'PPL' }); @@ -238,7 +251,7 @@ describe('query assist summary', () => { }); it('should fetch summary with expected payload and response', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.NO); + mockUseState('summary', LOADING.NO, FEEDBACK.NONE); const RESPONSE_TEXT = 'response'; httpMock.post.mockResolvedValue(RESPONSE_TEXT); renderQueryAssistSummary(COLLAPSED.NO); @@ -258,27 +271,27 @@ describe('query assist summary', () => { dataSourceId: undefined, }, }); - expect(setSummary).toHaveBeenNthCalledWith(1, null); + 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.NO); + 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(1); + expect(setSummary).toBeCalledTimes(2); expect(setLoading).toHaveBeenNthCalledWith(1, true); expect(setLoading).toHaveBeenNthCalledWith(2, false); }); it('should not update queryResults if subscription changed not in order', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.NO); + mockUseState('summary', LOADING.NO, FEEDBACK.NONE); const RESPONSE_TEXT = 'response'; httpMock.post.mockResolvedValue(RESPONSE_TEXT); renderQueryAssistSummary(COLLAPSED.NO); @@ -290,7 +303,7 @@ describe('query assist summary', () => { }); it('should update queryResults if subscriptions changed in order', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.NO); + mockUseState('summary', LOADING.NO, FEEDBACK.NONE); const RESPONSE_TEXT = 'response'; httpMock.post.mockResolvedValue(RESPONSE_TEXT); renderQueryAssistSummary(COLLAPSED.NO); @@ -308,7 +321,7 @@ describe('query assist summary', () => { }); it('should reset feedback state if re-fetch summary', async () => { - mockUseState('summary', LOADING.NO, FEEDBACK.YES); + mockUseState('summary', LOADING.NO, FEEDBACK.THUMB_UP); const RESPONSE_TEXT = 'response'; httpMock.post.mockResolvedValue(RESPONSE_TEXT); renderQueryAssistSummary(COLLAPSED.NO); @@ -316,6 +329,6 @@ describe('query assist summary', () => { query$.next({ query: PPL, language: 'PPL' }); data$.next(dataFrame as IDataFrame); await sleep(WAIT_TIME); - expect(setFeedback).toHaveBeenCalledWith(FEEDBACK.NO); + expect(setFeedback).toHaveBeenCalledWith(FEEDBACK.NONE); }); }); 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 4606a975713c..9a20c98fcbbd 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 @@ -29,6 +29,7 @@ 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'; export interface QueryContext { question: string; @@ -65,9 +66,9 @@ export const convertResult = (body: IDataFrame) => { export const QueryAssistSummary: React.FC = (props) => { const { query, search } = props.data; - const [summary, setSummary] = useState(null); // store fetched data + const [summary, setSummary] = useState(''); // store fetched data const [loading, setLoading] = useState(false); // track loading state - const [feedback, setFeedback] = useState(false); + const [feedback, setFeedback] = useState(FeedbackStatus.NONE); const [isEnabledByCapability, setIsEnabledByCapability] = useState(false); const selectedDataset = useRef(query.queryString.getQuery()?.dataset); const { question$, isQueryAssistCollapsed } = useQueryAssist(); @@ -76,6 +77,9 @@ export const QueryAssistSummary: React.FC = (props) => defaultMessage: 'Thank you for the feedback. Try again by adjusting your question so that I have the opportunity to better assist you.', }); + const errorPrompt = i18n.translate('queryEnhancements.queryAssist.summary.errorPrompt', { + defaultMessage: 'I am unable to respond to this query. Try another question.', + }); const sampleSize = 10; @@ -117,8 +121,8 @@ export const QueryAssistSummary: React.FC = (props) => async (queryContext: QueryContext) => { if (isEmpty(queryContext?.queryResults)) return; setLoading(true); - setSummary(null); - setFeedback(false); + setSummary(''); + setFeedback(FeedbackStatus.NONE); const SUCCESS_METRIC = 'fetch_summary_success'; try { const actualSampleSize = Math.min(sampleSize, queryContext?.queryResults?.length); @@ -140,11 +144,12 @@ export const QueryAssistSummary: React.FC = (props) => reportCountMetric(SUCCESS_METRIC, 1); } catch (error) { reportCountMetric(SUCCESS_METRIC, 0); + setSummary(errorPrompt); } finally { setLoading(false); } }, - [props.http, reportCountMetric] + [props.http, reportCountMetric, errorPrompt] ); useEffect(() => { @@ -203,9 +208,10 @@ export const QueryAssistSummary: React.FC = (props) => const onFeedback = useCallback( (satisfied: boolean) => { - if (feedback) return; - setFeedback(true); - reportMetric(satisfied ? 'thumbup' : 'thumbdown'); + if (feedback !== FeedbackStatus.NONE) return; + const feedbackStatus = satisfied ? FeedbackStatus.THUMB_UP : FeedbackStatus.THUMB_DOWN; + setFeedback(feedbackStatus); + reportMetric(feedbackStatus); }, [feedback, reportMetric] ); @@ -247,38 +253,42 @@ export const QueryAssistSummary: React.FC = (props) => })} /> - - onFeedback(true)} - data-test-subj="queryAssist_summary_buttons_thumbup" - /> - - - onFeedback(false)} - data-test-subj="queryAssist_summary_buttons_thumbdown" - /> - + {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" + /> + + )}