From 74d1145c998836361437f8772355790389634dfd Mon Sep 17 00:00:00 2001 From: Constance Date: Mon, 1 Jun 2020 12:48:45 -0700 Subject: [PATCH] Address review feedback (#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 --- .../__mocks__/shallow_with_i18n.mock.tsx | 2 +- .../empty_states/empty_states.test.tsx | 5 +--- .../engine_overview/engine_overview.test.tsx | 6 +--- .../engine_overview/engine_table.test.tsx | 2 +- .../engine_overview/engine_table.tsx | 12 ++++---- .../generate_breadcrumbs.ts | 2 +- .../set_breadcrumbs.test.tsx | 2 +- .../shared/react_router_helpers/eui_link.tsx | 4 +-- .../react_router_helpers/link_events.test.ts | 2 +- .../react_router_helpers/link_events.ts | 8 ++--- .../collectors/app_search/telemetry.test.ts | 29 ++++++++----------- .../server/collectors/app_search/telemetry.ts | 13 +++------ .../enterprise_search/server/plugin.ts | 29 ++++++++----------- .../server/routes/__mocks__/router.mock.ts | 8 ++--- .../server/routes/app_search/engines.test.ts | 8 ++--- .../server/routes/app_search/engines.ts | 9 +++++- 16 files changed, 63 insertions(+), 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 index f37d02d251c92..7815bb71fa50e 100644 --- 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(); */ -export const shallowWithIntl = children => { +export const shallowWithIntl = (children) => { return shallow({children}, { context: { intl }, childContextTypes: { intl }, diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/empty_states/empty_states.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/empty_states/empty_states.test.tsx index 5419029a37b16..bb37229998ed2 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/empty_states/empty_states.test.tsx +++ b/x-pack/plugins/enterprise_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(); 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/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.test.tsx index e7223b1c6b002..a9670163e76b8 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.test.tsx +++ b/x-pack/plugins/enterprise_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/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_table.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_table.test.tsx index f7591fb6d1dee..1665726251bd6 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_table.test.tsx +++ b/x-pack/plugins/enterprise_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/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_table.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_table.tsx index 60d24c2d13864..8a6033d9e1d90 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_table.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_table.tsx @@ -38,7 +38,7 @@ export const EngineTable: ReactFC = ({ 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 = ({ name: i18n.translate('xpack.appSearch.enginesOverview.table.column.name', { defaultMessage: 'Name', }), - render: name => {name}, + render: (name) => {name}, width: '30%', truncateText: true, mobileOptions: { @@ -72,7 +72,7 @@ export const EngineTable: ReactFC = ({ defaultMessage: 'Created At', }), dataType: 'string', - render: dateString => ( + render: (dateString) => ( // e.g., January 1, 1970 ), @@ -83,7 +83,7 @@ export const EngineTable: ReactFC = ({ defaultMessage: 'Document Count', }), dataType: 'number', - render: number => , + render: (number) => , truncateText: true, }, { @@ -92,7 +92,7 @@ export const EngineTable: ReactFC = ({ defaultMessage: 'Field Count', }), dataType: 'number', - render: number => , + render: (number) => , truncateText: true, }, { @@ -101,7 +101,7 @@ export const EngineTable: ReactFC = ({ defaultMessage: 'Actions', }), dataType: 'string', - render: name => ( + render: (name) => ( { + breadcrumb.onClick = (event) => { if (letBrowserHandleEvent(event)) return; event.preventDefault(); history.push(path); diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/kibana_breadcrumbs/set_breadcrumbs.test.tsx b/x-pack/plugins/enterprise_search/public/applications/shared/kibana_breadcrumbs/set_breadcrumbs.test.tsx index 38aae0e499c92..aeaa38a5ad38f 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/kibana_breadcrumbs/set_breadcrumbs.test.tsx +++ b/x-pack/plugins/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(, { http: {}, enterpriseSearchUrl: 'http://localhost:3002', diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.tsx b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.tsx index b56535d984e5c..3c410584cc49d 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.tsx @@ -25,7 +25,7 @@ interface IEuiReactRouterProps { export const EuiReactRouterLink: React.FC = ({ 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 = ({ to, isButto return isButton ? : ; }; -export const EuiReactRouterButton: React.FC = props => ( +export const EuiReactRouterButton: React.FC = (props) => ( ); diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/link_events.test.ts b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/link_events.test.ts index 49ab5ed920e36..0845e5562776b 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/link_events.test.ts +++ b/x-pack/plugins/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/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/link_events.ts b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/link_events.ts index bb87ecaf6877b..67e987623c2c1 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/link_events.ts +++ b/x-pack/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 index b4922a822ae23..4be2c220024bc 100644 --- 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 index 72f6fc2201be8..12b5a165bf1ac 100644 --- 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 index f93fab18ab90e..077d900a8d8cf 100644 --- 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 index 3f1c310daac02..1cec5da055140 100644 --- 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; @@ -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 index e78d389ca8818..722ad0d9269d3 100644 --- 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 index 3a474dc58e4dd..ebe2252b24eef 100644 --- 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 }, });