Address review feedback (elastic#14)

* Fix Prettier linting issues * Escape App Search API endpoint URLs - per PR feedback - querystring should automatically encodeURIComponent / escape query param strings * Update server plugin.ts to use getStartServices() rather than storing local references from start() - Per feedback: https://github.com/elastic/kibana/blob/master/src/core/CONVENTIONS.md#applications - Note: savedObjects.registerType needs to be outside of getStartServices, or an error is thrown - Side update to registerTelemetryUsageCollector to simplify args - Update/fix tests to account for changes
scottybollinger · Jul 6, 2020 · 28d6258 · 28d6258
1 parent 17a5fea
commit 28d6258
Show file tree

Hide file tree

Showing 16 changed files with 63 additions and 78 deletions.
diff --git a/x-pack/plugins/enterprise_search/public/applications/__mocks__/shallow_with_i18n.mock.tsx b/x-pack/plugins/enterprise_search/public/applications/__mocks__/shallow_with_i18n.mock.tsx
@@ -20,7 +20,7 @@ const { intl } = intlProvider.getChildContext();
  *
  * const wrapper = shallowWithIntl(<Component />);
  */
-export const shallowWithIntl = children => {
+export const shallowWithIntl = (children) => {
   return shallow(<I18nProvider>{children}</I18nProvider>, {
     context: { intl },
     childContextTypes: { intl },

diff --git a/...prise_search/public/applications/app_search/components/empty_states/empty_states.test.tsx b/...prise_search/public/applications/app_search/components/empty_states/empty_states.test.tsx
@@ -42,10 +42,7 @@ describe('NoUserState', () => {
     getUserName.mockImplementationOnce(() => 'dolores-abernathy');
     const wrapper = shallowWithIntl(<NoUserState />);
     const prompt = wrapper.find(EuiEmptyPrompt).dive();
-    const description1 = prompt
-      .find(FormattedMessage)
-      .at(1)
-      .dive();
+    const description1 = prompt.find(FormattedMessage).at(1).dive();
 
     expect(description1.find(EuiCode).prop('children')).toContain('dolores-abernathy');
   });

diff --git a/...search/public/applications/app_search/components/engine_overview/engine_overview.test.tsx b/...search/public/applications/app_search/components/engine_overview/engine_overview.test.tsx
@@ -105,11 +105,7 @@ describe('EngineOverview', () => {
     });
 
     describe('pagination', () => {
-      const getTablePagination = () =>
-        wrapper
-          .find(EngineTable)
-          .first()
-          .prop('pagination');
+      const getTablePagination = () => wrapper.find(EngineTable).first().prop('pagination');
 
       it('passes down page data from the API', () => {
         const pagination = getTablePagination();

diff --git a/...se_search/public/applications/app_search/components/engine_overview/engine_table.test.tsx b/...se_search/public/applications/app_search/components/engine_overview/engine_table.test.tsx
@@ -51,7 +51,7 @@ describe('EngineTable', () => {
   it('contains engine links which send telemetry', () => {
     const engineLinks = wrapper.find(EuiLink);
 
-    engineLinks.forEach(link => {
+    engineLinks.forEach((link) => {
       expect(link.prop('href')).toEqual('http://localhost:3002/as/engines/test-engine');
       link.simulate('click');
 

diff --git a/...erprise_search/public/applications/app_search/components/engine_overview/engine_table.tsx b/...erprise_search/public/applications/app_search/components/engine_overview/engine_table.tsx
@@ -38,7 +38,7 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
   pagination: { totalEngines, pageIndex = 0, onPaginate },
 }) => {
   const { enterpriseSearchUrl, http } = useContext(KibanaContext) as IKibanaContext;
-  const engineLinkProps = name => ({
+  const engineLinkProps = (name) => ({
     href: `${enterpriseSearchUrl}/as/engines/${name}`,
     target: '_blank',
     onClick: () =>
@@ -56,7 +56,7 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
       name: i18n.translate('xpack.enterpriseSearch.appSearch.enginesOverview.table.column.name', {
         defaultMessage: 'Name',
       }),
-      render: name => <EuiLink {...engineLinkProps(name)}>{name}</EuiLink>,
+      render: (name) => <EuiLink {...engineLinkProps(name)}>{name}</EuiLink>,
       width: '30%',
       truncateText: true,
       mobileOptions: {
@@ -75,7 +75,7 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
         }
       ),
       dataType: 'string',
-      render: dateString => (
+      render: (dateString) => (
         // e.g., January 1, 1970
         <FormattedDate value={new Date(dateString)} year="numeric" month="long" day="numeric" />
       ),
@@ -89,7 +89,7 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
         }
       ),
       dataType: 'number',
-      render: number => <FormattedNumber value={number} />,
+      render: (number) => <FormattedNumber value={number} />,
       truncateText: true,
     },
     {
@@ -101,7 +101,7 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
         }
       ),
       dataType: 'number',
-      render: number => <FormattedNumber value={number} />,
+      render: (number) => <FormattedNumber value={number} />,
       truncateText: true,
     },
     {
@@ -113,7 +113,7 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
         }
       ),
       dataType: 'string',
-      render: name => (
+      render: (name) => (
         <EuiLink {...engineLinkProps(name)}>
           <FormattedMessage
             id="xpack.enterpriseSearch.appSearch.enginesOverview.table.action.manage"

diff --git a/...s/enterprise_search/public/applications/shared/kibana_breadcrumbs/generate_breadcrumbs.ts b/...s/enterprise_search/public/applications/shared/kibana_breadcrumbs/generate_breadcrumbs.ts
@@ -25,7 +25,7 @@ export const generateBreadcrumb = ({ text, path, history }: IGenerateBreadcrumbP
 
   if (path && history) {
     breadcrumb.href = history.createHref({ pathname: path });
-    breadcrumb.onClick = event => {
+    breadcrumb.onClick = (event) => {
       if (letBrowserHandleEvent(event)) return;
       event.preventDefault();
       history.push(path);

diff --git a/.../enterprise_search/public/applications/shared/kibana_breadcrumbs/set_breadcrumbs.test.tsx b/.../enterprise_search/public/applications/shared/kibana_breadcrumbs/set_breadcrumbs.test.tsx
@@ -23,7 +23,7 @@ describe('SetAppSearchBreadcrumbs', () => {
     jest.clearAllMocks();
   });
 
-  const mountSetAppSearchBreadcrumbs = props => {
+  const mountSetAppSearchBreadcrumbs = (props) => {
     return mountWithKibanaContext(<SetAppSearchBreadcrumbs {...props} />, {
       http: {},
       enterpriseSearchUrl: 'http://localhost:3002',

diff --git a/...ck/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.tsx b/...ck/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.tsx
@@ -25,7 +25,7 @@ interface IEuiReactRouterProps {
 export const EuiReactRouterLink: React.FC<IEuiReactRouterProps> = ({ to, isButton, ...rest }) => {
   const history = useHistory();
 
-  const onClick = event => {
+  const onClick = (event) => {
     if (letBrowserHandleEvent(event)) return;
 
     // Prevent regular link behavior, which causes a browser refresh.
@@ -42,6 +42,6 @@ export const EuiReactRouterLink: React.FC<IEuiReactRouterProps> = ({ to, isButto
   return isButton ? <EuiButton {...props} /> : <EuiLink {...props} />;
 };
 
-export const EuiReactRouterButton: React.FC<IEuiReactRouterProps> = props => (
+export const EuiReactRouterButton: React.FC<IEuiReactRouterProps> = (props) => (
   <EuiReactRouterLink {...props} isButton />
 );
diff --git a/...ins/enterprise_search/public/applications/shared/react_router_helpers/link_events.test.ts b/...ins/enterprise_search/public/applications/shared/react_router_helpers/link_events.test.ts
@@ -95,7 +95,7 @@ describe('letBrowserHandleEvent', () => {
   });
 });
 
-const targetValue = value => {
+const targetValue = (value) => {
   return {
     getAttribute: () => value,
   };

diff --git a/.../plugins/enterprise_search/public/applications/shared/react_router_helpers/link_events.ts b/.../plugins/enterprise_search/public/applications/shared/react_router_helpers/link_events.ts
@@ -13,18 +13,18 @@ import { SyntheticEvent } from 'react';
 
 type THandleEvent = (event: SyntheticEvent) => boolean;
 
-export const letBrowserHandleEvent: THandleEvent = event =>
+export const letBrowserHandleEvent: THandleEvent = (event) =>
   event.defaultPrevented ||
   isModifiedEvent(event) ||
   !isLeftClickEvent(event) ||
   isTargetBlank(event);
 
-const isModifiedEvent: THandleEvent = event =>
+const isModifiedEvent: THandleEvent = (event) =>
   !!(event.metaKey || event.altKey || event.ctrlKey || event.shiftKey);
 
-const isLeftClickEvent: THandleEvent = event => event.button === 0;
+const isLeftClickEvent: THandleEvent = (event) => event.button === 0;
 
-const isTargetBlank: THandleEvent = event => {
+const isTargetBlank: THandleEvent = (event) => {
   const target = event.target.getAttribute('target');
   return !!target && target !== '_self';
 };
diff --git a/x-pack/plugins/enterprise_search/server/collectors/app_search/telemetry.test.ts b/x-pack/plugins/enterprise_search/server/collectors/app_search/telemetry.test.ts
@@ -14,6 +14,10 @@ import { registerTelemetryUsageCollector, incrementUICounter } from './telemetry
 describe('App Search Telemetry Usage Collector', () => {
   const makeUsageCollectorStub = jest.fn();
   const registerStub = jest.fn();
+  const usageCollectionMock = {
+    makeUsageCollector: makeUsageCollectorStub,
+    registerCollector: registerStub,
+  };
 
   const savedObjectsRepoStub = {
     get: () => ({
@@ -29,14 +33,8 @@ describe('App Search Telemetry Usage Collector', () => {
     }),
     incrementCounter: jest.fn(),
   };
-  const dependencies = {
-    usageCollection: {
-      makeUsageCollector: makeUsageCollectorStub,
-      registerCollector: registerStub,
-    },
-    savedObjects: {
-      createInternalRepository: jest.fn(() => savedObjectsRepoStub),
-    },
+  const savedObjectsMock = {
+    createInternalRepository: jest.fn(() => savedObjectsRepoStub),
   };
 
   beforeEach(() => {
@@ -45,7 +43,7 @@ describe('App Search Telemetry Usage Collector', () => {
 
   describe('registerTelemetryUsageCollector', () => {
     it('should make and register the usage collector', () => {
-      registerTelemetryUsageCollector(dependencies);
+      registerTelemetryUsageCollector(usageCollectionMock, savedObjectsMock);
 
       expect(registerStub).toHaveBeenCalledTimes(1);
       expect(makeUsageCollectorStub).toHaveBeenCalledTimes(1);
@@ -55,7 +53,7 @@ describe('App Search Telemetry Usage Collector', () => {
 
   describe('fetchTelemetryMetrics', () => {
     it('should return existing saved objects data', async () => {
-      registerTelemetryUsageCollector(dependencies);
+      registerTelemetryUsageCollector(usageCollectionMock, savedObjectsMock);
       const savedObjectsCounts = await makeUsageCollectorStub.mock.calls[0][0].fetch();
 
       expect(savedObjectsCounts).toEqual({
@@ -76,10 +74,9 @@ describe('App Search Telemetry Usage Collector', () => {
     });
 
     it('should not error & should return a default telemetry object if no saved data exists', async () => {
-      registerTelemetryUsageCollector({
-        ...dependencies,
-        savedObjects: { createInternalRepository: () => ({}) },
-      });
+      const emptySavedObjectsMock = { createInternalRepository: () => ({}) };
+
+      registerTelemetryUsageCollector(usageCollectionMock, emptySavedObjectsMock);
       const savedObjectsCounts = await makeUsageCollectorStub.mock.calls[0][0].fetch();
 
       expect(savedObjectsCounts).toEqual({
@@ -102,10 +99,8 @@ describe('App Search Telemetry Usage Collector', () => {
 
   describe('incrementUICounter', () => {
     it('should increment the saved objects internal repository', async () => {
-      const { savedObjects } = dependencies;
-
       const response = await incrementUICounter({
-        savedObjects,
+        savedObjects: savedObjectsMock,
         uiAction: 'ui_clicked',
         metric: 'button',
       });

diff --git a/x-pack/plugins/enterprise_search/server/collectors/app_search/telemetry.ts b/x-pack/plugins/enterprise_search/server/collectors/app_search/telemetry.ts
@@ -14,15 +14,10 @@ import { AS_TELEMETRY_NAME, ITelemetrySavedObject } from '../../saved_objects/ap
  * Register the telemetry collector
  */
 
-interface Dependencies {
-  savedObjects: SavedObjectsServiceStart;
-  usageCollection: UsageCollectionSetup;
-}
-
-export const registerTelemetryUsageCollector = ({
-  usageCollection,
-  savedObjects,
-}: Dependencies) => {
+export const registerTelemetryUsageCollector = (
+  usageCollection: UsageCollectionSetup,
+  savedObjects: SavedObjectsServiceStart
+) => {
   const telemetryUsageCollector = usageCollection.makeUsageCollector({
     type: 'app_search',
     fetch: async () => fetchTelemetryMetrics(savedObjects),

diff --git a/x-pack/plugins/enterprise_search/server/plugin.ts b/x-pack/plugins/enterprise_search/server/plugin.ts
@@ -10,7 +10,6 @@ import {
   Plugin,
   PluginInitializerContext,
   CoreSetup,
-  CoreStart,
   Logger,
   SavedObjectsServiceStart,
 } from 'src/core/server';
@@ -52,26 +51,22 @@ export class EnterpriseSearchPlugin implements Plugin {
     /**
      * Bootstrap the routes, saved objects, and collector for telemetry
      */
-    registerTelemetryRoute({
-      ...dependencies,
-      getSavedObjectsService: () => {
-        if (!this.savedObjectsServiceStart) {
-          throw new Error('Saved Objects Start service not available');
-        }
-        return this.savedObjectsServiceStart;
-      },
-    });
     savedObjects.registerType(appSearchTelemetryType);
-    if (usageCollection) {
-      getStartServices().then(([{ savedObjects: savedObjectsStarted }]) => {
-        registerTelemetryUsageCollector({ usageCollection, savedObjects: savedObjectsStarted });
+
+    getStartServices().then(([coreStart]) => {
+      const savedObjectsStarted = coreStart.savedObjects as SavedObjectsServiceStart;
+
+      registerTelemetryRoute({
+        ...dependencies,
+        getSavedObjectsService: () => savedObjectsStarted,
       });
-    }
+      if (usageCollection) {
+        registerTelemetryUsageCollector(usageCollection, savedObjectsStarted);
+      }
+    });
   }
 
-  public start({ savedObjects }: CoreStart) {
-    this.savedObjectsServiceStart = savedObjects;
-  }
+  public start() {}
 
   public stop() {}
 }
diff --git a/x-pack/plugins/enterprise_search/server/routes/__mocks__/router.mock.ts b/x-pack/plugins/enterprise_search/server/routes/__mocks__/router.mock.ts
@@ -30,7 +30,7 @@ export class MockRouter {
     this.router = httpServiceMock.createRouter();
   };
 
-  public callRoute = async request => {
+  public callRoute = async (request) => {
     const [_, handler] = this.router[this.method].mock.calls[0];
 
     const context = {} as jest.Mocked<RequestHandlerContext>;
@@ -41,18 +41,18 @@ export class MockRouter {
    * Schema validation helpers
    */
 
-  public validateRoute = request => {
+  public validateRoute = (request) => {
     const [config] = this.router[this.method].mock.calls[0];
     const validate = config.validate as RouteValidatorConfig<{}, {}, {}>;
 
     validate[this.payload].validate(request[this.payload]);
   };
 
-  public shouldValidate = request => {
+  public shouldValidate = (request) => {
     expect(() => this.validateRoute(request)).not.toThrow();
   };
 
-  public shouldThrow = request => {
+  public shouldThrow = (request) => {
     expect(() => this.validateRoute(request)).toThrow();
   };
 }

diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts
@@ -46,7 +46,7 @@ describe('engine routes', () => {
     describe('when the underlying App Search API returns a 200', () => {
       beforeEach(() => {
         AppSearchAPI.shouldBeCalledWith(
-          `http://localhost:3002/as/engines/collection?type=indexed&page[current]=1&page[size]=10`,
+          `http://localhost:3002/as/engines/collection?type=indexed&page%5Bcurrent%5D=1&page%5Bsize%5D=10`,
           { headers: { Authorization: AUTH_HEADER } }
         ).andReturn({
           results: [{ name: 'engine1' }],
@@ -67,7 +67,7 @@ describe('engine routes', () => {
     describe('when the underlying App Search API redirects to /login', () => {
       beforeEach(() => {
         AppSearchAPI.shouldBeCalledWith(
-          `http://localhost:3002/as/engines/collection?type=indexed&page[current]=1&page[size]=10`,
+          `http://localhost:3002/as/engines/collection?type=indexed&page%5Bcurrent%5D=1&page%5Bsize%5D=10`,
           { headers: { Authorization: AUTH_HEADER } }
         ).andReturnRedirect();
       });
@@ -85,7 +85,7 @@ describe('engine routes', () => {
     describe('when the App Search URL is invalid', () => {
       beforeEach(() => {
         AppSearchAPI.shouldBeCalledWith(
-          `http://localhost:3002/as/engines/collection?type=indexed&page[current]=1&page[size]=10`,
+          `http://localhost:3002/as/engines/collection?type=indexed&page%5Bcurrent%5D=1&page%5Bsize%5D=10`,
           { headers: { Authorization: AUTH_HEADER } }
         ).andReturnError();
       });
@@ -104,7 +104,7 @@ describe('engine routes', () => {
     describe('when the App Search API returns invalid data', () => {
       beforeEach(() => {
         AppSearchAPI.shouldBeCalledWith(
-          `http://localhost:3002/as/engines/collection?type=indexed&page[current]=1&page[size]=10`,
+          `http://localhost:3002/as/engines/collection?type=indexed&page%5Bcurrent%5D=1&page%5Bsize%5D=10`,
           { headers: { Authorization: AUTH_HEADER } }
         ).andReturnInvalidData();
       });

diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts
@@ -5,6 +5,7 @@
  */
 
 import fetch from 'node-fetch';
+import querystring from 'querystring';
 import { schema } from '@kbn/config-schema';
 
 import { ENGINES_PAGE_SIZE } from '../../../common/constants';
@@ -25,7 +26,13 @@ export function registerEnginesRoute({ router, config, log }) {
         const appSearchUrl = config.host;
         const { type, pageIndex } = request.query;
 
-        const url = `${appSearchUrl}/as/engines/collection?type=${type}&page[current]=${pageIndex}&page[size]=${ENGINES_PAGE_SIZE}`;
+        const params = querystring.stringify({
+          type,
+          'page[current]': pageIndex,
+          'page[size]': ENGINES_PAGE_SIZE,
+        });
+        const url = `${encodeURI(appSearchUrl)}/as/engines/collection?${params}`;
+
         const enginesResponse = await fetch(url, {
           headers: { Authorization: request.headers.authorization },
         });