Skip to content
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

[App Search] Updated Search UI to new URL #101320

Merged
merged 5 commits into from
Jun 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { generateEncodedPath } from '../utils/encode_path_params';
export const mockEngineValues = {
engineName: 'some-engine',
engine: {} as EngineDetails,
searchKey: 'search-abc123',
};

export const mockEngineActions = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import { LogicMounter, mockHttpValues } from '../../../__mocks__';

import { nextTick } from '@kbn/test/jest';

import { ApiTokenTypes } from '../credentials/constants';

import { EngineTypes } from './types';

import { EngineLogic } from './';
Expand Down Expand Up @@ -47,6 +49,7 @@ describe('EngineLogic', () => {
hasSchemaConflicts: false,
hasUnconfirmedSchemaFields: false,
engineNotFound: false,
searchKey: '',
};

beforeEach(() => {
Expand Down Expand Up @@ -263,5 +266,57 @@ describe('EngineLogic', () => {
});
});
});

describe('searchKey', () => {
it('should select the first available search key for this engine', () => {
const engine = {
...mockEngineData,
apiTokens: [
{
key: 'private-123xyz',
name: 'privateKey',
type: ApiTokenTypes.Private,
},
{
key: 'search-123xyz',
name: 'searchKey',
type: ApiTokenTypes.Search,
},
{
key: 'search-8910abc',
name: 'searchKey2',
type: ApiTokenTypes.Search,
},
],
};
mount({ engine });

expect(EngineLogic.values).toEqual({
...DEFAULT_VALUES,
engine,
searchKey: 'search-123xyz',
});
});

it('should return an empty string if none are available', () => {
const engine = {
...mockEngineData,
apiTokens: [
{
key: 'private-123xyz',
name: 'privateKey',
type: ApiTokenTypes.Private,
},
],
};
mount({ engine });

expect(EngineLogic.values).toEqual({
...DEFAULT_VALUES,
engine,
searchKey: '',
});
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import { kea, MakeLogicType } from 'kea';

import { HttpLogic } from '../../../shared/http';
import { ApiTokenTypes } from '../credentials/constants';
import { ApiToken } from '../credentials/types';

import { EngineDetails, EngineTypes } from './types';

Expand All @@ -21,6 +23,7 @@ interface EngineValues {
hasSchemaConflicts: boolean;
hasUnconfirmedSchemaFields: boolean;
engineNotFound: boolean;
searchKey: string;
}

interface EngineActions {
Expand Down Expand Up @@ -87,6 +90,14 @@ export const EngineLogic = kea<MakeLogicType<EngineValues, EngineActions>>({
() => [selectors.engine],
(engine) => engine?.unconfirmedFields?.length > 0,
],
searchKey: [
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to POST the search key to the new search experience URL from the Search UI form. Since all keys that are valid for a particular engine are already available on engine.apiTokens from state, I added a selector to easily access the first available search token.

Since tokens that do not have access to the current engine are not returned in this object, I do not have to check their access level, as we already know it has access to the current engine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this added logic/selector. Feels elegant and makes sense.

() => [selectors.engine],
(engine: Partial<EngineDetails>) => {
const isSearchKey = (token: ApiToken) => token.type === ApiTokenTypes.Search;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could pull this function out instead of declaring it in here every time (idk if its worth writing a test for it)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going completely the opposite direction, I would have just left it as an inline fn in .find() :trollface: But it doesn't bother me either way personally

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, quite the array of opinions! I just pulled this out for readability. It was long in the tooth inline in the find.

const searchKey = (engine.apiTokens || []).find(isSearchKey);
return searchKey?.key || '';
},
],
}),
listeners: ({ actions, values }) => ({
initializeEngine: async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ import { setMockValues, setMockActions } from '../../../../__mocks__';

import React from 'react';

import { shallow } from 'enzyme';
import { shallow, ShallowWrapper } from 'enzyme';

import { EuiForm } from '@elastic/eui';

import { ActiveField } from '../types';
import { generatePreviewUrl } from '../utils';
Expand All @@ -29,6 +31,7 @@ describe('SearchUIForm', () => {
urlField: 'url',
facetFields: ['category'],
sortFields: ['size'],
dataLoading: false,
};
const actions = {
onActiveFieldChange: jest.fn(),
Expand All @@ -43,10 +46,6 @@ describe('SearchUIForm', () => {
setMockActions(actions);
});

beforeEach(() => {
jest.clearAllMocks();
});

it('renders', () => {
const wrapper = shallow(<SearchUIForm />);
expect(wrapper.find('[data-test-subj="selectTitle"]').exists()).toBe(true);
Expand All @@ -56,6 +55,7 @@ describe('SearchUIForm', () => {
});

describe('title field', () => {
beforeEach(() => jest.clearAllMocks());
const subject = () => shallow(<SearchUIForm />).find('[data-test-subj="selectTitle"]');

it('renders with its value set from state', () => {
Expand Down Expand Up @@ -84,6 +84,7 @@ describe('SearchUIForm', () => {
});

describe('url field', () => {
beforeEach(() => jest.clearAllMocks());
const subject = () => shallow(<SearchUIForm />).find('[data-test-subj="selectUrl"]');

it('renders with its value set from state', () => {
Expand Down Expand Up @@ -112,6 +113,7 @@ describe('SearchUIForm', () => {
});

describe('filters field', () => {
beforeEach(() => jest.clearAllMocks());
const subject = () => shallow(<SearchUIForm />).find('[data-test-subj="selectFilters"]');

it('renders with its value set from state', () => {
Expand Down Expand Up @@ -145,6 +147,7 @@ describe('SearchUIForm', () => {
});

describe('sorts field', () => {
beforeEach(() => jest.clearAllMocks());
const subject = () => shallow(<SearchUIForm />).find('[data-test-subj="selectSort"]');

it('renders with its value set from state', () => {
Expand Down Expand Up @@ -177,26 +180,61 @@ describe('SearchUIForm', () => {
});
});

it('includes a link to generate the preview', () => {
(generatePreviewUrl as jest.Mock).mockReturnValue('http://www.example.com?foo=bar');
describe('generate preview button', () => {
let wrapper: ShallowWrapper;

setMockValues({
...values,
urlField: 'foo',
titleField: 'bar',
facetFields: ['baz'],
sortFields: ['qux'],
beforeAll(() => {
jest.clearAllMocks();
(generatePreviewUrl as jest.Mock).mockReturnValue('http://www.example.com?foo=bar');
setMockValues({
...values,
urlField: 'foo',
titleField: 'bar',
facetFields: ['baz'],
sortFields: ['qux'],
searchKey: 'search-123abc',
});
wrapper = shallow(<SearchUIForm />);
});

it('should be a submit button', () => {
expect(wrapper.find('[data-test-subj="generateSearchUiPreview"]').prop('type')).toBe(
'submit'
);
});

it('should be wrapped in a form configured to POST to the preview screen in a new tab', () => {
const form = wrapper.find(EuiForm);
expect(generatePreviewUrl).toHaveBeenCalledWith({
urlField: 'foo',
titleField: 'bar',
facets: ['baz'],
sortFields: ['qux'],
});
expect(form.prop('action')).toBe('http://www.example.com?foo=bar');
expect(form.prop('target')).toBe('_blank');
expect(form.prop('method')).toBe('POST');
expect(form.prop('component')).toBe('form');
});

const subject = () =>
shallow(<SearchUIForm />).find('[data-test-subj="generateSearchUiPreview"]');
it('should include a searchKey in that form POST', () => {
const form = wrapper.find(EuiForm);
const hiddenInput = form.find('input[type="hidden"]');
expect(hiddenInput.prop('id')).toBe('searchKey');
expect(hiddenInput.prop('value')).toBe('search-123abc');
});
});

expect(subject().prop('href')).toBe('http://www.example.com?foo=bar');
expect(generatePreviewUrl).toHaveBeenCalledWith({
urlField: 'foo',
titleField: 'bar',
facets: ['baz'],
sortFields: ['qux'],
it('should disable everything while data is loading', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this piece of UX + test - love it! 🎉

setMockValues({
...values,
dataLoading: true,
});
const wrapper = shallow(<SearchUIForm />);
expect(wrapper.find('[data-test-subj="selectTitle"]').prop('disabled')).toBe(true);
expect(wrapper.find('[data-test-subj="selectFilters"]').prop('isDisabled')).toBe(true);
expect(wrapper.find('[data-test-subj="selectSort"]').prop('isDisabled')).toBe(true);
expect(wrapper.find('[data-test-subj="selectUrl"]').prop('disabled')).toBe(true);
expect(wrapper.find('[data-test-subj="generateSearchUiPreview"]').prop('disabled')).toBe(true);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { useValues, useActions } from 'kea';

import { EuiForm, EuiFormRow, EuiSelect, EuiComboBox, EuiButton } from '@elastic/eui';

import { EngineLogic } from '../../engine';
import {
TITLE_FIELD_LABEL,
TITLE_FIELD_HELP_TEXT,
Expand All @@ -27,7 +28,9 @@ import { ActiveField } from '../types';
import { generatePreviewUrl } from '../utils';

export const SearchUIForm: React.FC = () => {
const { searchKey } = useValues(EngineLogic);
const {
dataLoading,
validFields,
validSortFields,
validFacetFields,
Expand Down Expand Up @@ -70,9 +73,11 @@ export const SearchUIForm: React.FC = () => {
const selectedFacetOptions = formatMultiOptions(facetFields);

return (
<EuiForm>
<EuiForm component="form" action={previewHref} target="_blank" method="POST">
<input type="hidden" id="searchKey" name="searchKey" value={searchKey} />
<EuiFormRow label={TITLE_FIELD_LABEL} helpText={TITLE_FIELD_HELP_TEXT} fullWidth>
<EuiSelect
disabled={dataLoading}
options={optionFields}
value={selectedTitleOption && selectedTitleOption.value}
onChange={(e) => onTitleFieldChange(e.target.value)}
Expand All @@ -85,6 +90,7 @@ export const SearchUIForm: React.FC = () => {
</EuiFormRow>
<EuiFormRow label={FILTER_FIELD_LABEL} helpText={FILTER_FIELD_HELP_TEXT} fullWidth>
<EuiComboBox
isDisabled={dataLoading}
options={facetOptionFields}
selectedOptions={selectedFacetOptions}
onChange={(newValues) => onFacetFieldsChange(newValues.map((field) => field.value!))}
Expand All @@ -96,6 +102,7 @@ export const SearchUIForm: React.FC = () => {
</EuiFormRow>
<EuiFormRow label={SORT_FIELD_LABEL} helpText={SORT_FIELD_HELP_TEXT} fullWidth>
<EuiComboBox
isDisabled={dataLoading}
options={sortOptionFields}
selectedOptions={selectedSortOptions}
onChange={(newValues) => onSortFieldsChange(newValues.map((field) => field.value!))}
Expand All @@ -108,6 +115,7 @@ export const SearchUIForm: React.FC = () => {

<EuiFormRow label={URL_FIELD_LABEL} helpText={URL_FIELD_HELP_TEXT} fullWidth>
<EuiSelect
disabled={dataLoading}
options={optionFields}
value={selectedURLOption && selectedURLOption.value}
onChange={(e) => onUrlFieldChange(e.target.value)}
Expand All @@ -119,8 +127,8 @@ export const SearchUIForm: React.FC = () => {
/>
</EuiFormRow>
<EuiButton
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than being a link as before, this is not a form with a submit button so that we can POST to the new route.

href={previewHref}
target="_blank"
disabled={dataLoading}
type="submit"
fill
iconType="popout"
iconSide="right"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

import { i18n } from '@kbn/i18n';

import { CREDENTIALS_TITLE } from '../credentials';

export const SEARCH_UI_TITLE = i18n.translate(
'xpack.enterpriseSearch.appSearch.engine.searchUI.title',
{ defaultMessage: 'Search UI' }
Expand Down Expand Up @@ -48,3 +50,9 @@ export const GENERATE_PREVIEW_BUTTON_LABEL = i18n.translate(
'xpack.enterpriseSearch.appSearch.engine.searchUI.generatePreviewButtonLabel',
{ defaultMessage: 'Generate search experience' }
);
export const NO_SEARCH_KEY_ERROR = (engineName: string) =>
i18n.translate('xpack.enterpriseSearch.appSearch.engine.searchUI.noSearchKeyErrorMessage', {
defaultMessage:
"It looks like you don't have any Public Search Keys with access to the '{engineName}' engine. Please visit the {credentialsTitle} page to set one up.",
values: { engineName, credentialsTitle: CREDENTIALS_TITLE },
});
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { SearchUILogic } from './';
describe('SearchUILogic', () => {
const { mount } = new LogicMounter(SearchUILogic);
const { http } = mockHttpValues;
const { flashAPIErrors } = mockFlashMessageHelpers;
const { flashAPIErrors, setErrorMessage } = mockFlashMessageHelpers;

const DEFAULT_VALUES = {
dataLoading: true,
Expand All @@ -35,6 +35,7 @@ describe('SearchUILogic', () => {
beforeEach(() => {
jest.clearAllMocks();
mockEngineValues.engineName = 'engine1';
mockEngineValues.searchKey = 'search-abc123';
});

it('has expected default values', () => {
Expand Down Expand Up @@ -155,6 +156,17 @@ describe('SearchUILogic', () => {
});
});

it('will short circuit the call if there is no searchKey available for this engine', async () => {
mockEngineValues.searchKey = '';
mount();

SearchUILogic.actions.loadFieldData();

expect(setErrorMessage).toHaveBeenCalledWith(
"It looks like you don't have any Public Search Keys with access to the 'engine1' engine. Please visit the Credentials page to set one up."
);
});

it('handles errors', async () => {
http.get.mockReturnValueOnce(Promise.reject('error'));
mount();
Expand Down
Loading