-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GS] add application result provider #68488
Changes from all commits
be094b9
4757227
4b109b6
5fbb1e9
1a52005
0454d35
3ef8b1d
04d3803
183c860
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
{ | ||
"id": "globalSearchProviders", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless anyone is against it, I was planning to create a single There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM. Once #68811 is merged, we should group this plugin with the global_search plugin in the same directory. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was thinking about this more and maybe this should live in the same plugin. Where do we expect the search UI code to go? Since it needs to be under the Elastic License, it must be in a plugin. I see two options:
cc @myasonik There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤷♂️ I'm a terrible person to ask in this case. I've got little context on the tradeoffs of this... Either options seems fine. I'd maybe err towards separate plugins in case we ever want to experiment with different search strategies, it might be easier to swap between two separate plugins rather than handling it all in one. But, really, I'm just guessing at a bunch of this so other opinions encouraged. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I was thinking of something like I added a new plugin for the GS providers basically only because I could (tm) and it was maybe a slightly better isolation. But I have no strong feeling toward this. I can move the providers into the For the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's just go with separate plugins for everything and then group them in a single directory once #68811 merges. |
||
"version": "8.0.0", | ||
"kibanaVersion": "kibana", | ||
"server": false, | ||
"ui": true, | ||
"requiredPlugins": ["globalSearch"], | ||
"optionalPlugins": [], | ||
"configPath": ["xpack", "global_search_providers"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/* | ||
* 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 { PluginInitializer } from 'src/core/public'; | ||
import { GlobalSearchProvidersPlugin, GlobalSearchProvidersPluginSetupDeps } from './plugin'; | ||
|
||
export const plugin: PluginInitializer<{}, {}, GlobalSearchProvidersPluginSetupDeps, {}> = () => | ||
new GlobalSearchProvidersPlugin(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/* | ||
* 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 { coreMock } from '../../../../src/core/public/mocks'; | ||
import { globalSearchPluginMock } from '../../global_search/public/mocks'; | ||
import { GlobalSearchProvidersPlugin } from './plugin'; | ||
|
||
describe('GlobalSearchProvidersPlugin', () => { | ||
let plugin: GlobalSearchProvidersPlugin; | ||
let globalSearchSetup: ReturnType<typeof globalSearchPluginMock.createSetupContract>; | ||
|
||
beforeEach(() => { | ||
globalSearchSetup = globalSearchPluginMock.createSetupContract(); | ||
plugin = new GlobalSearchProvidersPlugin(); | ||
}); | ||
|
||
describe('#setup', () => { | ||
it('registers the `application` result provider', () => { | ||
const coreSetup = coreMock.createSetup(); | ||
plugin.setup(coreSetup, { globalSearch: globalSearchSetup }); | ||
|
||
expect(globalSearchSetup.registerResultProvider).toHaveBeenCalledTimes(1); | ||
expect(globalSearchSetup.registerResultProvider).toHaveBeenCalledWith( | ||
expect.objectContaining({ | ||
id: 'application', | ||
}) | ||
); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/* | ||
* 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 { CoreSetup, Plugin } from 'src/core/public'; | ||
import { GlobalSearchPluginSetup } from '../../global_search/public'; | ||
import { createApplicationResultProvider } from './providers'; | ||
|
||
export interface GlobalSearchProvidersPluginSetupDeps { | ||
globalSearch: GlobalSearchPluginSetup; | ||
} | ||
|
||
export class GlobalSearchProvidersPlugin | ||
implements Plugin<{}, {}, GlobalSearchProvidersPluginSetupDeps, {}> { | ||
setup( | ||
{ getStartServices }: CoreSetup<{}, {}>, | ||
{ globalSearch }: GlobalSearchProvidersPluginSetupDeps | ||
) { | ||
const applicationPromise = getStartServices().then(([core]) => core.application); | ||
globalSearch.registerResultProvider(createApplicationResultProvider(applicationPromise)); | ||
return {}; | ||
} | ||
|
||
start() { | ||
return {}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
/* | ||
* 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. | ||
*/ | ||
|
||
export const getAppResultsMock = jest.fn(); | ||
jest.doMock('./get_app_results', () => ({ | ||
getAppResults: getAppResultsMock, | ||
})); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,204 @@ | ||
/* | ||
* 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 { getAppResultsMock } from './application.test.mocks'; | ||
|
||
import { of, EMPTY } from 'rxjs'; | ||
import { TestScheduler } from 'rxjs/testing'; | ||
import { ApplicationStart, AppNavLinkStatus, AppStatus, PublicAppInfo } from 'src/core/public'; | ||
import { | ||
GlobalSearchProviderFindOptions, | ||
GlobalSearchProviderResult, | ||
} from '../../../global_search/public'; | ||
import { applicationServiceMock } from 'src/core/public/mocks'; | ||
import { createApplicationResultProvider } from './application'; | ||
|
||
const getTestScheduler = () => | ||
new TestScheduler((actual, expected) => { | ||
expect(actual).toEqual(expected); | ||
}); | ||
|
||
const createApp = (props: Partial<PublicAppInfo> = {}): PublicAppInfo => ({ | ||
id: 'app1', | ||
title: 'App 1', | ||
appRoute: '/app/app1', | ||
legacy: false, | ||
status: AppStatus.accessible, | ||
navLinkStatus: AppNavLinkStatus.visible, | ||
chromeless: false, | ||
...props, | ||
}); | ||
|
||
const createResult = (props: Partial<GlobalSearchProviderResult>): GlobalSearchProviderResult => ({ | ||
id: 'id', | ||
title: 'title', | ||
type: 'application', | ||
url: '/app/id', | ||
score: 100, | ||
...props, | ||
}); | ||
|
||
const createAppMap = (apps: PublicAppInfo[]): Map<string, PublicAppInfo> => { | ||
return new Map(apps.map((app) => [app.id, app])); | ||
}; | ||
|
||
const expectApp = (id: string) => expect.objectContaining({ id }); | ||
const expectResult = expectApp; | ||
|
||
describe('applicationResultProvider', () => { | ||
let application: ReturnType<typeof applicationServiceMock.createStartContract>; | ||
|
||
const defaultOption: GlobalSearchProviderFindOptions = { | ||
preference: 'pref', | ||
maxResults: 20, | ||
aborted$: EMPTY, | ||
}; | ||
|
||
beforeEach(() => { | ||
application = applicationServiceMock.createStartContract(); | ||
getAppResultsMock.mockReturnValue([]); | ||
}); | ||
|
||
it('has the correct id', () => { | ||
const provider = createApplicationResultProvider(Promise.resolve(application)); | ||
expect(provider.id).toBe('application'); | ||
}); | ||
|
||
it('calls `getAppResults` with the term and the list of apps', async () => { | ||
application.applications$ = of( | ||
createAppMap([ | ||
createApp({ id: 'app1', title: 'App 1' }), | ||
createApp({ id: 'app2', title: 'App 2' }), | ||
createApp({ id: 'app3', title: 'App 3' }), | ||
]) | ||
); | ||
const provider = createApplicationResultProvider(Promise.resolve(application)); | ||
|
||
await provider.find('term', defaultOption).toPromise(); | ||
|
||
expect(getAppResultsMock).toHaveBeenCalledTimes(1); | ||
expect(getAppResultsMock).toHaveBeenCalledWith('term', [ | ||
expectApp('app1'), | ||
expectApp('app2'), | ||
expectApp('app3'), | ||
]); | ||
}); | ||
|
||
it('ignores inaccessible apps', async () => { | ||
application.applications$ = of( | ||
createAppMap([ | ||
createApp({ id: 'app1', title: 'App 1' }), | ||
createApp({ id: 'disabled', title: 'disabled', status: AppStatus.inaccessible }), | ||
]) | ||
); | ||
const provider = createApplicationResultProvider(Promise.resolve(application)); | ||
await provider.find('term', defaultOption).toPromise(); | ||
|
||
expect(getAppResultsMock).toHaveBeenCalledWith('term', [expectApp('app1')]); | ||
}); | ||
|
||
it('ignores chromeless apps', async () => { | ||
application.applications$ = of( | ||
createAppMap([ | ||
createApp({ id: 'app1', title: 'App 1' }), | ||
createApp({ id: 'chromeless', title: 'chromeless', chromeless: true }), | ||
]) | ||
); | ||
|
||
const provider = createApplicationResultProvider(Promise.resolve(application)); | ||
await provider.find('term', defaultOption).toPromise(); | ||
|
||
expect(getAppResultsMock).toHaveBeenCalledWith('term', [expectApp('app1')]); | ||
}); | ||
|
||
it('sorts the results returned by `getAppResults`', async () => { | ||
getAppResultsMock.mockReturnValue([ | ||
createResult({ id: 'r60', score: 60 }), | ||
createResult({ id: 'r100', score: 100 }), | ||
createResult({ id: 'r50', score: 50 }), | ||
createResult({ id: 'r75', score: 75 }), | ||
]); | ||
|
||
const provider = createApplicationResultProvider(Promise.resolve(application)); | ||
const results = await provider.find('term', defaultOption).toPromise(); | ||
|
||
expect(results).toEqual([ | ||
expectResult('r100'), | ||
expectResult('r75'), | ||
expectResult('r60'), | ||
expectResult('r50'), | ||
]); | ||
}); | ||
|
||
it('only returns the highest `maxResults` results', async () => { | ||
getAppResultsMock.mockReturnValue([ | ||
createResult({ id: 'r60', score: 60 }), | ||
createResult({ id: 'r100', score: 100 }), | ||
createResult({ id: 'r50', score: 50 }), | ||
createResult({ id: 'r75', score: 75 }), | ||
]); | ||
|
||
const provider = createApplicationResultProvider(Promise.resolve(application)); | ||
|
||
const options = { | ||
...defaultOption, | ||
maxResults: 2, | ||
}; | ||
const results = await provider.find('term', options).toPromise(); | ||
|
||
expect(results).toEqual([expectResult('r100'), expectResult('r75')]); | ||
}); | ||
|
||
it('only emits once, even if `application$` emits multiple times', () => { | ||
getTestScheduler().run(({ hot, expectObservable }) => { | ||
const appMap = createAppMap([createApp({ id: 'app1', title: 'App 1' })]); | ||
|
||
application.applications$ = hot('--a---b', { a: appMap, b: appMap }); | ||
|
||
// test scheduler doesnt play well with promises. need to workaround by passing | ||
// an observable instead. Behavior with promise is asserted in previous tests of the suite | ||
const applicationPromise = (hot('a', { a: application }) as unknown) as Promise< | ||
ApplicationStart | ||
>; | ||
|
||
const provider = createApplicationResultProvider(applicationPromise); | ||
|
||
const options = { | ||
...defaultOption, | ||
aborted$: hot<undefined>('|'), | ||
}; | ||
|
||
const resultObs = provider.find('term', options); | ||
|
||
expectObservable(resultObs).toBe('--(a|)', { a: [] }); | ||
}); | ||
}); | ||
|
||
it('only emits results until `aborted$` emits', () => { | ||
getTestScheduler().run(({ hot, expectObservable }) => { | ||
const appMap = createAppMap([createApp({ id: 'app1', title: 'App 1' })]); | ||
|
||
application.applications$ = hot('---a', { a: appMap, b: appMap }); | ||
|
||
// test scheduler doesnt play well with promises. need to workaround by passing | ||
// an observable instead. Behavior with promise is asserted in previous tests of the suite | ||
const applicationPromise = (hot('a', { a: application }) as unknown) as Promise< | ||
ApplicationStart | ||
>; | ||
|
||
const provider = createApplicationResultProvider(applicationPromise); | ||
|
||
const options = { | ||
...defaultOption, | ||
aborted$: hot<undefined>('-(a|)', { a: undefined }), | ||
}; | ||
|
||
const resultObs = provider.find('term', options); | ||
|
||
expectObservable(resultObs).toBe('-|'); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ease consumers usage by transforming optional fields into non-optional as we are injecting default values during registration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor thing, but you could use the
Required
type, but this may be simpler to read.