From 3aca64958b775217179936052e9151aab58b42d1 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Tue, 29 Sep 2020 11:10:27 +0200 Subject: [PATCH 1/4] increase default number of results to show all apps --- .../plugins/global_search/common/constants.ts | 7 ++ .../public/services/search_service.ts | 2 +- .../server/services/search_service.ts | 2 +- .../public/components/search_bar.tsx | 82 ++++++++++++------- .../providers/saved_objects/provider.test.ts | 12 ++- .../providers/saved_objects/provider.ts | 4 + 6 files changed, 74 insertions(+), 35 deletions(-) create mode 100644 x-pack/plugins/global_search/common/constants.ts diff --git a/x-pack/plugins/global_search/common/constants.ts b/x-pack/plugins/global_search/common/constants.ts new file mode 100644 index 0000000000000..423cf5f8be5a8 --- /dev/null +++ b/x-pack/plugins/global_search/common/constants.ts @@ -0,0 +1,7 @@ +/* + * 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 defaultMaxProviderResults = 40; diff --git a/x-pack/plugins/global_search/public/services/search_service.ts b/x-pack/plugins/global_search/public/services/search_service.ts index 68970b75ad975..62b347d925868 100644 --- a/x-pack/plugins/global_search/public/services/search_service.ts +++ b/x-pack/plugins/global_search/public/services/search_service.ts @@ -12,6 +12,7 @@ import { HttpStart } from 'src/core/public'; import { GlobalSearchProviderResult, GlobalSearchBatchedResults } from '../../common/types'; import { GlobalSearchFindError } from '../../common/errors'; import { takeInArray } from '../../common/operators'; +import { defaultMaxProviderResults } from '../../common/constants'; import { processProviderResult } from '../../common/process_result'; import { ILicenseChecker } from '../../common/license_checker'; import { GlobalSearchResultProvider } from '../types'; @@ -79,7 +80,6 @@ interface StartDeps { licenseChecker: ILicenseChecker; } -const defaultMaxProviderResults = 20; const mapToUndefined = () => undefined; /** @internal */ diff --git a/x-pack/plugins/global_search/server/services/search_service.ts b/x-pack/plugins/global_search/server/services/search_service.ts index d79f3781c6bec..1897a24196cf1 100644 --- a/x-pack/plugins/global_search/server/services/search_service.ts +++ b/x-pack/plugins/global_search/server/services/search_service.ts @@ -11,6 +11,7 @@ import { KibanaRequest, CoreStart, IBasePath } from 'src/core/server'; import { GlobalSearchProviderResult, GlobalSearchBatchedResults } from '../../common/types'; import { GlobalSearchFindError } from '../../common/errors'; import { takeInArray } from '../../common/operators'; +import { defaultMaxProviderResults } from '../../common/constants'; import { ILicenseChecker } from '../../common/license_checker'; import { processProviderResult } from '../../common/process_result'; @@ -80,7 +81,6 @@ interface StartDeps { licenseChecker: ILicenseChecker; } -const defaultMaxProviderResults = 20; const mapToUndefined = () => undefined; /** @internal */ diff --git a/x-pack/plugins/global_search_bar/public/components/search_bar.tsx b/x-pack/plugins/global_search_bar/public/components/search_bar.tsx index 54066cee414d8..547904ebd8666 100644 --- a/x-pack/plugins/global_search_bar/public/components/search_bar.tsx +++ b/x-pack/plugins/global_search_bar/public/components/search_bar.tsx @@ -17,8 +17,9 @@ import { } from '@elastic/eui'; import { i18n } from '@kbn/i18n'; import { FormattedMessage } from '@kbn/i18n/react'; +import { Subscription } from 'rxjs'; import { ApplicationStart } from 'kibana/public'; -import React, { useCallback, useState } from 'react'; +import React, { useCallback, useState, useRef } from 'react'; import useDebounce from 'react-use/lib/useDebounce'; import useEvent from 'react-use/lib/useEvent'; import useMountedState from 'react-use/lib/useMountedState'; @@ -42,48 +43,73 @@ const clearField = (field: HTMLInputElement) => { const cleanMeta = (str: string) => (str.charAt(0).toUpperCase() + str.slice(1)).replace(/-/g, ' '); const blurEvent = new FocusEvent('blur'); +const sortByScore = (a: GlobalSearchResult, b: GlobalSearchResult): number => { + if (a.score < b.score) return 1; + if (a.score > b.score) return -1; + return 0; +}; + +const sortByTitle = (a: GlobalSearchResult, b: GlobalSearchResult): number => { + const titleA = a.title.toUpperCase(); // ignore upper and lowercase + const titleB = b.title.toUpperCase(); // ignore upper and lowercase + if (titleA < titleB) return -1; + if (titleA > titleB) return 1; + return 0; +}; + +const resultToOption = (result: GlobalSearchResult): EuiSelectableTemplateSitewideOption => { + const { id, title, url, icon, type, meta } = result; + const option: EuiSelectableTemplateSitewideOption = { + key: id, + label: title, + url, + }; + + if (icon) { + option.icon = { type: icon }; + } + + if (type === 'application') { + option.meta = [{ text: meta?.categoryLabel as string }]; + } else { + option.meta = [{ text: cleanMeta(type) }]; + } + + return option; +}; + export function SearchBar({ globalSearch, navigateToUrl }: Props) { const isMounted = useMountedState(); const [searchValue, setSearchValue] = useState(''); const [searchRef, setSearchRef] = useState(null); + const searchSubscription = useRef(null); const [options, _setOptions] = useState([] as EuiSelectableTemplateSitewideOption[]); const isMac = navigator.platform.toLowerCase().indexOf('mac') >= 0; const setOptions = useCallback( (_options: GlobalSearchResult[]) => { - if (!isMounted()) return; - - _setOptions([ - ..._options.map(({ id, title, url, icon, type, meta }) => { - const option: EuiSelectableTemplateSitewideOption = { - key: id, - label: title, - url, - }; - - if (icon) option.icon = { type: icon }; - - if (type === 'application') option.meta = [{ text: meta?.categoryLabel as string }]; - else option.meta = [{ text: cleanMeta(type) }]; + if (!isMounted()) { + return; + } - return option; - }), - ]); + _setOptions(_options.map(resultToOption)); }, [isMounted, _setOptions] ); useDebounce( () => { + // cancel pending search if not completed yet + if (searchSubscription.current) { + searchSubscription.current.unsubscribe(); + searchSubscription.current = null; + } + let arr: GlobalSearchResult[] = []; - globalSearch(searchValue, {}).subscribe({ + searchSubscription.current = globalSearch(searchValue, {}).subscribe({ next: ({ results }) => { if (searchValue.length > 0) { - arr = [...results, ...arr].sort((a, b) => { - if (a.score < b.score) return 1; - if (a.score > b.score) return -1; - return 0; - }); + arr = [...results, ...arr].sort(sortByScore); setOptions(arr); return; } @@ -91,13 +117,7 @@ export function SearchBar({ globalSearch, navigateToUrl }: Props) { // if searchbar is empty, filter to only applications and sort alphabetically results = results.filter(({ type }: GlobalSearchResult) => type === 'application'); - arr = [...results, ...arr].sort((a, b) => { - const titleA = a.title.toUpperCase(); // ignore upper and lowercase - const titleB = b.title.toUpperCase(); // ignore upper and lowercase - if (titleA < titleB) return -1; - if (titleA > titleB) return 1; - return 0; - }); + arr = [...results, ...arr].sort(sortByTitle); setOptions(arr); }, diff --git a/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.test.ts b/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.test.ts index 352191658ed0d..1914357f4824a 100644 --- a/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.test.ts +++ b/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.test.ts @@ -5,6 +5,7 @@ */ import { EMPTY } from 'rxjs'; +import { toArray } from 'rxjs/operators'; import { TestScheduler } from 'rxjs/testing'; import { SavedObjectsFindResponse, @@ -114,8 +115,8 @@ describe('savedObjectsResultProvider', () => { expect(provider.id).toBe('savedObjects'); }); - it('calls `savedObjectClient.find` with the correct parameters', () => { - provider.find('term', defaultOption, context); + it('calls `savedObjectClient.find` with the correct parameters', async () => { + await provider.find('term', defaultOption, context).toPromise(); expect(context.core.savedObjects.client.find).toHaveBeenCalledTimes(1); expect(context.core.savedObjects.client.find).toHaveBeenCalledWith({ @@ -128,6 +129,13 @@ describe('savedObjectsResultProvider', () => { }); }); + it('does not call `savedObjectClient.find` if `term` is empty', async () => { + const results = await provider.find('', defaultOption, context).pipe(toArray()).toPromise(); + + expect(context.core.savedObjects.client.find).not.toHaveBeenCalled(); + expect(results).toEqual([]); + }); + it('converts the saved objects to results', async () => { context.core.savedObjects.client.find.mockResolvedValue( createFindResponse([ diff --git a/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.ts b/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.ts index 1c79380fe17fd..fee1a7b6fa1a8 100644 --- a/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.ts +++ b/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.ts @@ -13,6 +13,10 @@ export const createSavedObjectsResultProvider = (): GlobalSearchResultProvider = return { id: 'savedObjects', find: (term, { aborted$, maxResults, preference }, { core }) => { + if (!term) { + return from([]); + } + const { capabilities, savedObjects: { client, typeRegistry }, From c3f55e81a82aa41abbf26633a24125e5b4825888 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Tue, 29 Sep 2020 16:31:33 +0200 Subject: [PATCH 2/4] fix circuit breaker --- .../server/providers/saved_objects/provider.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.ts b/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.ts index fee1a7b6fa1a8..3861858a53626 100644 --- a/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.ts +++ b/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { from, combineLatest } from 'rxjs'; +import { from, combineLatest, of } from 'rxjs'; import { map, takeUntil, first } from 'rxjs/operators'; import { GlobalSearchResultProvider } from '../../../../global_search/server'; import { mapToResults } from './map_object_to_result'; @@ -14,7 +14,7 @@ export const createSavedObjectsResultProvider = (): GlobalSearchResultProvider = id: 'savedObjects', find: (term, { aborted$, maxResults, preference }, { core }) => { if (!term) { - return from([]); + return of([]); } const { From b8810e078ce30d8e0664645e02f3ece0cfbfec0d Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Tue, 29 Sep 2020 17:24:22 +0200 Subject: [PATCH 3/4] fix ut --- .../server/providers/saved_objects/provider.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.test.ts b/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.test.ts index 1914357f4824a..b556e2785b4b4 100644 --- a/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.test.ts +++ b/x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.test.ts @@ -133,7 +133,7 @@ describe('savedObjectsResultProvider', () => { const results = await provider.find('', defaultOption, context).pipe(toArray()).toPromise(); expect(context.core.savedObjects.client.find).not.toHaveBeenCalled(); - expect(results).toEqual([]); + expect(results).toEqual([[]]); }); it('converts the saved objects to results', async () => { From 70fb8944054fa56d874700c4639cf8436cd81880 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Mon, 5 Oct 2020 09:31:52 +0200 Subject: [PATCH 4/4] add unit test --- .../public/components/search_bar.test.tsx | 132 +++++++++++++----- 1 file changed, 96 insertions(+), 36 deletions(-) diff --git a/x-pack/plugins/global_search_bar/public/components/search_bar.test.tsx b/x-pack/plugins/global_search_bar/public/components/search_bar.test.tsx index 6fad3335c5efc..3c86c4e70e346 100644 --- a/x-pack/plugins/global_search_bar/public/components/search_bar.test.tsx +++ b/x-pack/plugins/global_search_bar/public/components/search_bar.test.tsx @@ -5,17 +5,19 @@ */ import React from 'react'; -import { wait } from '@testing-library/react'; -import { of } from 'rxjs'; +import { waitFor, act } from '@testing-library/react'; +import { ReactWrapper } from 'enzyme'; +import { of, BehaviorSubject } from 'rxjs'; +import { filter, map } from 'rxjs/operators'; import { mountWithIntl } from 'test_utils/enzyme_helpers'; -import { httpServiceMock, uiSettingsServiceMock } from '../../../../../src/core/public/mocks'; -import { - GlobalSearchBatchedResults, - GlobalSearchPluginStart, - GlobalSearchResult, -} from '../../../global_search/public'; +import { applicationServiceMock } from '../../../../../src/core/public/mocks'; +import { GlobalSearchBatchedResults, GlobalSearchResult } from '../../../global_search/public'; import { globalSearchPluginMock } from '../../../global_search/public/mocks'; -import { SearchBar } from '../components/search_bar'; +import { SearchBar } from './search_bar'; + +jest.mock('@elastic/eui/lib/services/accessibility/html_id_generator', () => ({ + htmlIdGenerator: () => () => 'mockId', +})); type Result = { id: string; type: string } | string; @@ -38,30 +40,46 @@ const createBatch = (...results: Result[]): GlobalSearchBatchedResults => ({ results: results.map(createResult), }); -jest.mock('@elastic/eui/lib/services/accessibility/html_id_generator', () => ({ - htmlIdGenerator: () => () => 'mockId', -})); - const getSelectableProps: any = (component: any) => component.find('EuiSelectable').props(); const getSearchProps: any = (component: any) => component.find('EuiFieldSearch').props(); describe('SearchBar', () => { - let searchService: GlobalSearchPluginStart; - let findSpy: jest.SpyInstance; - const http = httpServiceMock.createSetupContract({ basePath: '/test' }); - const basePathUrl = http.basePath.prepend('/plugins/globalSearchBar/assets/'); - const uiSettings = uiSettingsServiceMock.createStartContract(); - const darkMode = uiSettings.get('theme:darkMode'); + let searchService: ReturnType; + let applications: ReturnType; + const basePathUrl = '/plugins/globalSearchBar/assets/'; + const darkMode = false; + + let component: ReactWrapper; beforeEach(() => { + applications = applicationServiceMock.createStartContract(); searchService = globalSearchPluginMock.createStartContract(); - findSpy = jest.spyOn(searchService, 'find'); jest.useFakeTimers(); }); + const triggerFocus = () => { + component.find('input[data-test-subj="header-search"]').simulate('focus'); + }; + + const update = () => { + act(() => { + jest.runAllTimers(); + }); + component.update(); + }; + + const simulateTypeChar = async (text: string) => { + await waitFor(() => + getSearchProps(component).onKeyUpCapture({ currentTarget: { value: text } }) + ); + }; + + const getDisplayedOptionsLabel = () => { + return getSelectableProps(component).options.map((option: any) => option.label); + }; + it('correctly filters and sorts results', async () => { - const navigate = jest.fn(); - findSpy + searchService.find .mockReturnValueOnce( of( createBatch('Discover', 'Canvas'), @@ -70,35 +88,37 @@ describe('SearchBar', () => { ) .mockReturnValueOnce(of(createBatch('Discover', { id: 'My Dashboard', type: 'test' }))); - const component = mountWithIntl( + component = mountWithIntl( ); - expect(findSpy).toHaveBeenCalledTimes(0); - component.find('input[data-test-subj="header-search"]').simulate('focus'); - jest.runAllTimers(); - component.update(); - expect(findSpy).toHaveBeenCalledTimes(1); - expect(findSpy).toHaveBeenCalledWith('', {}); + expect(searchService.find).toHaveBeenCalledTimes(0); + + triggerFocus(); + update(); + + expect(searchService.find).toHaveBeenCalledTimes(1); + expect(searchService.find).toHaveBeenCalledWith('', {}); expect(getSelectableProps(component).options).toMatchSnapshot(); - await wait(() => getSearchProps(component).onKeyUpCapture({ currentTarget: { value: 'd' } })); - jest.runAllTimers(); - component.update(); + + await simulateTypeChar('d'); + update(); + expect(getSelectableProps(component).options).toMatchSnapshot(); - expect(findSpy).toHaveBeenCalledTimes(2); - expect(findSpy).toHaveBeenCalledWith('d', {}); + expect(searchService.find).toHaveBeenCalledTimes(2); + expect(searchService.find).toHaveBeenCalledWith('d', {}); }); it('supports keyboard shortcuts', () => { mountWithIntl( @@ -113,4 +133,44 @@ describe('SearchBar', () => { expect(document.activeElement).toMatchSnapshot(); }); + + it('only display results from the last search', async () => { + const firstSearchTrigger = new BehaviorSubject(false); + const firstSearch = firstSearchTrigger.pipe( + filter((event) => event), + map(() => { + return createBatch('Discover', 'Canvas'); + }) + ); + const secondSearch = of(createBatch('Visualize', 'Map')); + + searchService.find.mockReturnValueOnce(firstSearch).mockReturnValueOnce(secondSearch); + + component = mountWithIntl( + + ); + + triggerFocus(); + update(); + + expect(searchService.find).toHaveBeenCalledTimes(1); + + await simulateTypeChar('d'); + update(); + + expect(getDisplayedOptionsLabel().length).toBe(2); + expect(getDisplayedOptionsLabel()).toEqual(expect.arrayContaining(['Visualize', 'Map'])); + + firstSearchTrigger.next(true); + + update(); + + expect(getDisplayedOptionsLabel().length).toBe(2); + expect(getDisplayedOptionsLabel()).toEqual(expect.arrayContaining(['Visualize', 'Map'])); + }); });