[App Search] DRY helper for encoding/decoding routes that can have special characters in params (#89811)

Constance · kibanamachine · cee-chen · commit a20f1e96b296 · 2021-02-02T12:04:31.000-08:00
* Add encodePathParam helper + update components that need it

- Primarily document URLs &amp; analytics queries (which uses generateEnginePath)

* Add useDecodedParams helper + update components that need it

- Documents titles &amp; Analytics queries

* [Misc] Change popout icon to eye

- Feedback from Davey - the pages don't open in a new window, so shouldn't use the popout icon
- Not strictly related but since we're touching these links anyway, I'm shoving it in (sorry)

* Remove document detail decode test

- now handled/tested by useDecodedParams helper

* Add new generateEncodedPath helper

- Should be used in place of generatePath

* Update all instances of generatePath to generateEncodedPath

for consistency across the App Search codebase

* Fix failing tests due to extra encodeURI() done by generatePath

* Add missing branch test for analytics query titles

Co-authored-by: Kibana Machine &lt;42973632+kibanamachine@users.noreply.github.com&gt;
diff --git a/x-pack/plugins/enterprise_search/public/applications/__mocks__/react_router_history.mock.ts b/x-pack/plugins/enterprise_search/public/applications/__mocks__/react_router_history.mock.ts
@@ -24,12 +24,20 @@ export const mockLocation = {
   state: {},
 };
 
-jest.mock('react-router-dom', () => ({
-  ...(jest.requireActual('react-router-dom') as object),
-  useHistory: jest.fn(() => mockHistory),
-  useLocation: jest.fn(() => mockLocation),
-  useParams: jest.fn(() => ({})),
-}));
+jest.mock('react-router-dom', () => {
+  const originalModule = jest.requireActual('react-router-dom');
+  return {
+    ...originalModule,
+    useHistory: jest.fn(() => mockHistory),
+    useLocation: jest.fn(() => mockLocation),
+    useParams: jest.fn(() => ({})),
+    // Note: RR's generatePath() opinionatedly encodeURI()s paths (although this doesn't actually
+    // show up/affect the final browser URL). Since we already have a generateEncodedPath helper &
+    // RR is removing this behavior in history 5.0+, I'm mocking tests to remove the extra encoding
+    // for now to make reading generateEncodedPath URLs a little less of a pain
+    generatePath: jest.fn((path, params) => decodeURI(originalModule.generatePath(path, params))),
+  };
+});
 
 /**
  * For example usage, @see public/applications/shared/react_router_helpers/eui_link.test.tsx
diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/__mocks__/engine_logic.mock.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/__mocks__/engine_logic.mock.ts
@@ -4,15 +4,15 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { generatePath } from 'react-router-dom';
+import { generateEncodedPath } from '../utils/encode_path_params';
 
 export const mockEngineValues = {
   engineName: 'some-engine',
   engine: {},
 };
 
 export const mockGenerateEnginePath = jest.fn((path, pathParams = {}) =>
-  generatePath(path, { engineName: mockEngineValues.engineName, ...pathParams })
+  generateEncodedPath(path, { engineName: mockEngineValues.engineName, ...pathParams })
 );
 
 jest.mock('../components/engine', () => ({
diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/components/analytics_tables/shared_columns.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/components/analytics_tables/shared_columns.tsx
@@ -56,7 +56,7 @@ export const ACTIONS_COLUMN = {
         { defaultMessage: 'View query analytics' }
       ),
       type: 'icon',
-      icon: 'popout',
+      icon: 'eye',
       color: 'primary',
       onClick: (item: Query | RecentQuery) => {
         const { navigateToUrl } = KibanaLogic.values;
diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/views/query_detail.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/views/query_detail.test.tsx
@@ -13,14 +13,15 @@ import { shallow } from 'enzyme';
 
 import { SetAppSearchChrome as SetPageChrome } from '../../../../shared/kibana_chrome';
 
+import { AnalyticsLayout } from '../analytics_layout';
 import { AnalyticsCards, AnalyticsChart, QueryClicksTable } from '../components';
 import { QueryDetail } from './';
 
 describe('QueryDetail', () => {
   const mockBreadcrumbs = ['Engines', 'some-engine', 'Analytics'];
 
   beforeEach(() => {
-    (useParams as jest.Mock).mockReturnValueOnce({ query: 'some-query' });
+    (useParams as jest.Mock).mockReturnValue({ query: 'some-query' });
 
     setMockValues({
       totalQueriesForQuery: 100,
@@ -31,6 +32,7 @@ describe('QueryDetail', () => {
   it('renders', () => {
     const wrapper = shallow(<QueryDetail breadcrumbs={mockBreadcrumbs} />);
 
+    expect(wrapper.find(AnalyticsLayout).prop('title')).toEqual('"some-query"');
     expect(wrapper.find(SetPageChrome).prop('trail')).toEqual([
       'Engines',
       'some-engine',
@@ -43,4 +45,11 @@ describe('QueryDetail', () => {
     expect(wrapper.find(AnalyticsChart)).toHaveLength(1);
     expect(wrapper.find(QueryClicksTable)).toHaveLength(1);
   });
+
+  it('renders empty "" search titles correctly', () => {
+    (useParams as jest.Mock).mockReturnValue({ query: '""' });
+    const wrapper = shallow(<QueryDetail breadcrumbs={mockBreadcrumbs} />);
+
+    expect(wrapper.find(AnalyticsLayout).prop('title')).toEqual('""');
+  });
 });
diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/views/query_detail.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/analytics/views/query_detail.tsx
@@ -5,14 +5,14 @@
  */
 
 import React from 'react';
-import { useParams } from 'react-router-dom';
 import { useValues } from 'kea';
 
 import { i18n } from '@kbn/i18n';
 import { EuiSpacer } from '@elastic/eui';
 
 import { SetAppSearchChrome as SetPageChrome } from '../../../../shared/kibana_chrome';
 import { BreadcrumbTrail } from '../../../../shared/kibana_chrome/generate_breadcrumbs';
+import { useDecodedParams } from '../../../utils/encode_path_params';
 
 import { AnalyticsLayout } from '../analytics_layout';
 import { AnalyticsSection, QueryClicksTable } from '../components';
@@ -27,14 +27,15 @@ interface Props {
   breadcrumbs: BreadcrumbTrail;
 }
 export const QueryDetail: React.FC<Props> = ({ breadcrumbs }) => {
-  const { query } = useParams() as { query: string };
+  const { query } = useDecodedParams();
+  const queryTitle = query === '""' ? query : `"${query}"`;
 
   const { totalQueriesForQuery, queriesPerDayForQuery, startDate, topClicksForQuery } = useValues(
     AnalyticsLogic
   );
 
   return (
-    <AnalyticsLayout isQueryView title={`"${query}"`}>
+    <AnalyticsLayout isQueryView title={queryTitle}>
       <SetPageChrome trail={[...breadcrumbs, QUERY_DETAIL_TITLE, query]} />
 
       <AnalyticsCards
diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/document_detail.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/document_detail.test.tsx
@@ -104,10 +104,4 @@ describe('DocumentDetail', () => {
 
     expect(actions.deleteDocument).toHaveBeenCalledWith('1');
   });
-
-  it('correctly decodes document IDs', () => {
-    (useParams as jest.Mock).mockReturnValueOnce({ documentId: 'hello%20world%20%26%3F!' });
-    const wrapper = shallow(<DocumentDetail engineBreadcrumb={['test']} />);
-    expect(wrapper.find('h1').text()).toEqual('Document: hello world &?!');
-  });
 });
diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/document_detail.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/document_detail.tsx
@@ -23,6 +23,7 @@ import { i18n } from '@kbn/i18n';
 import { Loading } from '../../../shared/loading';
 import { SetAppSearchChrome as SetPageChrome } from '../../../shared/kibana_chrome';
 import { FlashMessages } from '../../../shared/flash_messages';
+import { useDecodedParams } from '../../utils/encode_path_params';
 import { ResultFieldValue } from '../result';
 
 import { DocumentDetailLogic } from './document_detail_logic';
@@ -43,6 +44,7 @@ export const DocumentDetail: React.FC<Props> = ({ engineBreadcrumb }) => {
   const { deleteDocument, getDocumentDetails, setFields } = useActions(DocumentDetailLogic);
 
   const { documentId } = useParams() as { documentId: string };
+  const { documentId: documentTitle } = useDecodedParams();
 
   useEffect(() => {
     getDocumentDetails(documentId);
@@ -74,13 +76,11 @@ export const DocumentDetail: React.FC<Props> = ({ engineBreadcrumb }) => {
 
   return (
     <>
-      <SetPageChrome
-        trail={[...engineBreadcrumb, DOCUMENTS_TITLE, decodeURIComponent(documentId)]}
-      />
+      <SetPageChrome trail={[...engineBreadcrumb, DOCUMENTS_TITLE, documentTitle]} />
       <EuiPageHeader>
         <EuiPageHeaderSection>
           <EuiTitle size="l">
-            <h1>{DOCUMENT_DETAIL_TITLE(decodeURIComponent(documentId))}</h1>
+            <h1>{DOCUMENT_DETAIL_TITLE(documentTitle)}</h1>
           </EuiTitle>
         </EuiPageHeaderSection>
         <EuiPageHeaderSection>
diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/utils.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/utils.ts
@@ -4,7 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { generatePath } from 'react-router-dom';
+import { generateEncodedPath } from '../../utils/encode_path_params';
 
 import { EngineLogic } from './';
 
@@ -13,5 +13,5 @@ import { EngineLogic } from './';
  */
 export const generateEnginePath = (path: string, pathParams: object = {}) => {
   const { engineName } = EngineLogic.values;
-  return generatePath(path, { engineName, ...pathParams });
+  return generateEncodedPath(path, { engineName, ...pathParams });
 };
diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engines/engines_table.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engines/engines_table.tsx
@@ -5,14 +5,14 @@
  */
 
 import React from 'react';
-import { generatePath } from 'react-router-dom';
 import { useActions } from 'kea';
 import { EuiBasicTable, EuiBasicTableColumn } from '@elastic/eui';
 import { FormattedMessage, FormattedDate, FormattedNumber } from '@kbn/i18n/react';
 import { i18n } from '@kbn/i18n';
 
 import { TelemetryLogic } from '../../../shared/telemetry';
 import { EuiLinkTo } from '../../../shared/react_router_helpers';
+import { generateEncodedPath } from '../../utils/encode_path_params';
 import { ENGINE_PATH } from '../../routes';
 
 import { ENGINES_PAGE_SIZE } from '../../../../../common/constants';
@@ -41,7 +41,7 @@ export const EnginesTable: React.FC<EnginesTableProps> = ({
   const { sendAppSearchTelemetry } = useActions(TelemetryLogic);
 
   const engineLinkProps = (engineName: string) => ({
-    to: generatePath(ENGINE_PATH, { engineName }),
+    to: generateEncodedPath(ENGINE_PATH, { engineName }),
     onClick: () =>
       sendAppSearchTelemetry({
         action: 'clicked',
diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx
@@ -5,7 +5,6 @@
  */
 
 import React, { useState, useMemo } from 'react';
-import { generatePath } from 'react-router-dom';
 import classNames from 'classnames';
 
 import './result.scss';
@@ -14,6 +13,7 @@ import { EuiPanel, EuiIcon } from '@elastic/eui';
 import { i18n } from '@kbn/i18n';
 
 import { ReactRouterHelper } from '../../../shared/react_router_helpers/eui_components';
+import { generateEncodedPath } from '../../utils/encode_path_params';
 import { ENGINE_DOCUMENT_DETAIL_PATH } from '../../routes';
 
 import { Schema } from '../../../shared/types';
@@ -52,7 +52,7 @@ export const Result: React.FC<Props> = ({
     if (schemaForTypeHighlights) return schemaForTypeHighlights[fieldName];
   };
 
-  const documentLink = generatePath(ENGINE_DOCUMENT_DETAIL_PATH, {
+  const documentLink = generateEncodedPath(ENGINE_DOCUMENT_DETAIL_PATH, {
     engineName: resultMeta.engine,
     documentId: resultMeta.id,
   });
@@ -135,7 +135,7 @@ export const Result: React.FC<Props> = ({
                 { defaultMessage: 'Visit document details' }
               )}
             >
-              <EuiIcon type="popout" />
+              <EuiIcon type="eye" />
             </a>
           </ReactRouterHelper>
         )}
diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/utils/encode_path_params/index.test.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/utils/encode_path_params/index.test.ts
@@ -0,0 +1,49 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import '../../../__mocks__/react_router_history.mock';
+import { useParams } from 'react-router-dom';
+
+import { encodePathParams, generateEncodedPath, useDecodedParams } from './';
+
+describe('encodePathParams', () => {
+  it('encodeURIComponent()s all object values', () => {
+    const params = {
+      someValue: 'hello world???',
+      anotherValue: 'test!@#$%^&*[]/|;:"<>~`',
+    };
+    expect(encodePathParams(params)).toEqual({
+      someValue: 'hello%20world%3F%3F%3F',
+      anotherValue: 'test!%40%23%24%25%5E%26*%5B%5D%2F%7C%3B%3A%22%3C%3E~%60',
+    });
+  });
+});
+
+describe('generateEncodedPath', () => {
+  it('generates a react router path with encoded path parameters', () => {
+    expect(
+      generateEncodedPath('/values/:someValue/:anotherValue/new', {
+        someValue: 'hello world???',
+        anotherValue: 'test!@#$%^&*[]/|;:"<>~`',
+      })
+    ).toEqual(
+      '/values/hello%20world%3F%3F%3F/test!%40%23%24%25%5E%26*%5B%5D%2F%7C%3B%3A%22%3C%3E~%60/new'
+    );
+  });
+});
+
+describe('useDecodedParams', () => {
+  it('decodeURIComponent()s all object values from useParams()', () => {
+    (useParams as jest.Mock).mockReturnValue({
+      someValue: 'hello%20world%3F%3F%3F',
+      anotherValue: 'test!%40%23%24%25%5E%26*%5B%5D%2F%7C%3B%3A%22%3C%3E~%60',
+    });
+    expect(useDecodedParams()).toEqual({
+      someValue: 'hello world???',
+      anotherValue: 'test!@#$%^&*[]/|;:"<>~`',
+    });
+  });
+});
diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/utils/encode_path_params/index.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/utils/encode_path_params/index.ts
@@ -0,0 +1,34 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { generatePath, useParams } from 'react-router-dom';
+
+type PathParams = Record<string, string>;
+
+export const encodePathParams = (pathParams: PathParams) => {
+  const encodedParams: PathParams = {};
+
+  Object.entries(pathParams).map(([key, value]) => {
+    encodedParams[key] = encodeURIComponent(value);
+  });
+
+  return encodedParams;
+};
+
+export const generateEncodedPath = (path: string, pathParams: PathParams) => {
+  return generatePath(path, encodePathParams(pathParams));
+};
+
+export const useDecodedParams = () => {
+  const decodedParams: PathParams = {};
+
+  const params = useParams();
+  Object.entries(params).map(([key, value]) => {
+    decodedParams[key] = decodeURIComponent(value as string);
+  });
+
+  return decodedParams;
+};
