From e51b7fcdbe234c0ea4c21c6432110508ca2b08b0 Mon Sep 17 00:00:00 2001 From: Liyun Xiu Date: Thu, 26 Sep 2024 11:51:59 +0800 Subject: [PATCH 1/3] Add discover summary error info and button click logic enhancement Signed-off-by: Liyun Xiu --- .../components/query_assist_summary.test.tsx | 64 ++++++++----- .../components/query_assist_summary.tsx | 92 +++++++++++-------- 2 files changed, 94 insertions(+), 62 deletions(-) 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..946c12ba3484 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 @@ -89,10 +89,12 @@ describe('query assist summary', () => { YES: true, NO: false, }; - const FEEDBACK = { - YES: true, - NO: false, - }; + + enum FEEDBACK { + NONE, + THUMB_UP, + THUMB_DOWN, + } const renderQueryAssistSummary = (isCollapsed: boolean) => { const component = render( @@ -136,7 +138,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 +149,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 +170,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 +178,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 +194,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 +203,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 +217,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 +230,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 +256,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 +276,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 +308,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 +326,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 +334,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 d6ad2057a77d..3099a7027a6f 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 @@ -63,11 +63,17 @@ export const convertResult = (body: IDataFrame) => { return hits; }; +enum FeedbackStatus { + NONE, + THUMB_UP, + THUMB_DOWN, +} + 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 +82,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 +126,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 +149,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,8 +213,8 @@ export const QueryAssistSummary: React.FC = (props) => const onFeedback = useCallback( (satisfied: boolean) => { - if (feedback) return; - setFeedback(true); + if (feedback !== FeedbackStatus.NONE) return; + setFeedback(satisfied ? FeedbackStatus.THUMB_UP : FeedbackStatus.THUMB_DOWN); reportMetric(satisfied ? 'thumbup' : 'thumbdown'); }, [feedback, reportMetric] @@ -246,38 +256,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" + /> + + )} From d8fa9cc692717c0c7f329d1cddc5f91eef6faa46 Mon Sep 17 00:00:00 2001 From: Liyun Xiu Date: Thu, 26 Sep 2024 12:01:45 +0800 Subject: [PATCH 2/3] add changelog Signed-off-by: Liyun Xiu --- changelogs/fragments/8352.yml | 2 ++ 1 file changed, 2 insertions(+) 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)) From 1b5b3a06556bc256495d3d817bea237acc282013 Mon Sep 17 00:00:00 2001 From: Liyun Xiu Date: Fri, 27 Sep 2024 08:19:52 +0800 Subject: [PATCH 3/3] Address comments Signed-off-by: Liyun Xiu --- .../query_enhancements/common/query_assist/index.ts | 7 ++++++- .../query_enhancements/common/query_assist/types.ts | 6 ++++++ .../components/query_assist_summary.test.tsx | 7 +------ .../query_assist/components/query_assist_summary.tsx | 12 ++++-------- 4 files changed, 17 insertions(+), 15 deletions(-) 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 946c12ba3484..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'), @@ -90,12 +91,6 @@ describe('query assist summary', () => { NO: false, }; - enum FEEDBACK { - NONE, - THUMB_UP, - THUMB_DOWN, - } - const renderQueryAssistSummary = (isCollapsed: boolean) => { const component = render(
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 3099a7027a6f..129271d696a4 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; @@ -63,12 +64,6 @@ export const convertResult = (body: IDataFrame) => { return hits; }; -enum FeedbackStatus { - NONE, - THUMB_UP, - THUMB_DOWN, -} - export const QueryAssistSummary: React.FC = (props) => { const { query, search } = props.data; const [summary, setSummary] = useState(''); // store fetched data @@ -214,8 +209,9 @@ export const QueryAssistSummary: React.FC = (props) => const onFeedback = useCallback( (satisfied: boolean) => { if (feedback !== FeedbackStatus.NONE) return; - setFeedback(satisfied ? FeedbackStatus.THUMB_UP : FeedbackStatus.THUMB_DOWN); - reportMetric(satisfied ? 'thumbup' : 'thumbdown'); + const feedbackStatus = satisfied ? FeedbackStatus.THUMB_UP : FeedbackStatus.THUMB_DOWN; + setFeedback(feedbackStatus); + reportMetric(feedbackStatus); }, [feedback, reportMetric] );