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 c4049eb) Signed-off-by: Liyun Xiu <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-project · Oct 12, 2024 · 9365ad6 · 9365ad6
1 parent 4000357
commit 9365ad6
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 64 deletions.
diff --git a/changelogs/fragments/8352.yml 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
@@ -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
@@ -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
@@ -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,15 +165,15 @@ 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);
     expect(screen.queryAllByTestId('queryAssist_summary_empty_text')).toHaveLength(0);
   });
 
   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,21 +212,37 @@ 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',
       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');
+    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,14 +321,14 @@ 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);
     question$.next(question);
     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
@@ -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<QueryAssistSummaryProps> = (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<QueryAssistSummaryProps> = (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<QueryAssistSummaryProps> = (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<QueryAssistSummaryProps> = (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<QueryAssistSummaryProps> = (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<QueryAssistSummaryProps> = (props) =>
                     })}
                   />
                 </EuiFlexItem>
-                <EuiFlexItem grow={false}>
-                  <EuiSmallButtonIcon
-                    aria-label="feedback thumbs up"
-                    color="text"
-                    iconType="thumbsUp"
-                    title={
-                      !feedback
-                        ? i18n.translate('queryEnhancements.queryAssist.summary.goodResponse', {
-                            defaultMessage: `Good response`,
-                          })
-                        : afterFeedbackTip
-                    }
-                    onClick={() => onFeedback(true)}
-                    data-test-subj="queryAssist_summary_buttons_thumbup"
-                  />
-                </EuiFlexItem>
-                <EuiFlexItem grow={false}>
-                  <EuiSmallButtonIcon
-                    aria-label="feedback thumbs down"
-                    color="text"
-                    title={
-                      !feedback
-                        ? i18n.translate('queryEnhancements.queryAssist.summary.badResponse', {
-                            defaultMessage: `Bad response`,
-                          })
-                        : afterFeedbackTip
-                    }
-                    iconType="thumbsDown"
-                    onClick={() => onFeedback(false)}
-                    data-test-subj="queryAssist_summary_buttons_thumbdown"
-                  />
-                </EuiFlexItem>
+                {feedback !== FeedbackStatus.THUMB_DOWN && (
+                  <EuiFlexItem grow={false}>
+                    <EuiSmallButtonIcon
+                      aria-label="feedback thumbs up"
+                      color={feedback === FeedbackStatus.THUMB_UP ? 'subdued' : 'text'}
+                      iconType="thumbsUp"
+                      title={
+                        !feedback
+                          ? i18n.translate('queryEnhancements.queryAssist.summary.goodResponse', {
+                              defaultMessage: `Good response`,
+                            })
+                          : afterFeedbackTip
+                      }
+                      onClick={() => onFeedback(true)}
+                      data-test-subj="queryAssist_summary_buttons_thumbup"
+                    />
+                  </EuiFlexItem>
+                )}
+                {feedback !== FeedbackStatus.THUMB_UP && (
+                  <EuiFlexItem grow={false}>
+                    <EuiSmallButtonIcon
+                      aria-label="feedback thumbs down"
+                      color={feedback === FeedbackStatus.THUMB_DOWN ? 'subdued' : 'text'}
+                      title={
+                        !feedback
+                          ? i18n.translate('queryEnhancements.queryAssist.summary.badResponse', {
+                              defaultMessage: `Bad response`,
+                            })
+                          : afterFeedbackTip
+                      }
+                      iconType="thumbsDown"
+                      onClick={() => onFeedback(false)}
+                      data-test-subj="queryAssist_summary_buttons_thumbdown"
+                    />
+                  </EuiFlexItem>
+                )}
                 <EuiSpacer size="m" style={{ borderLeft: '1px solid #D3DAE6', height: '20px' }} />
                 <EuiFlexItem grow={false}>
                   <EuiCopy textToCopy={summary ?? ''}>
Original file line number	Diff line number	Diff line change
		@@ -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))