-
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
[App Search] Create Curation view/functionality #92560
Changes from 3 commits
132b6e9
209810a
f6dc151
732ee56
3e5b31b
814152f
3f70ef8
038fc4d
0f5a530
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,3 @@ | ||
.curationQueryRow { | ||
margin-bottom: $euiSizeXS; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import { setMockActions, setMockValues } from '../../../../../__mocks__'; | ||
|
||
import React from 'react'; | ||
|
||
import { shallow } from 'enzyme'; | ||
|
||
import { CurationQuery } from './curation_query'; | ||
|
||
import { CurationQueries } from './'; | ||
|
||
describe('CurationQueries', () => { | ||
const props = { | ||
queries: ['a', 'b', 'c'], | ||
onSubmit: jest.fn(), | ||
}; | ||
const values = { | ||
queries: ['a', 'b', 'c'], | ||
hasEmptyQueries: false, | ||
hasOnlyOneQuery: false, | ||
}; | ||
const actions = { | ||
addQuery: jest.fn(), | ||
editQuery: jest.fn(), | ||
deleteQuery: jest.fn(), | ||
}; | ||
|
||
beforeEach(() => { | ||
jest.clearAllMocks(); | ||
setMockValues(values); | ||
setMockActions(actions); | ||
}); | ||
|
||
it('renders a CurationQuery row for each query', () => { | ||
const wrapper = shallow(<CurationQueries {...props} />); | ||
|
||
expect(wrapper.find(CurationQuery)).toHaveLength(3); | ||
expect(wrapper.find(CurationQuery).at(0).prop('queryValue')).toEqual('a'); | ||
expect(wrapper.find(CurationQuery).at(1).prop('queryValue')).toEqual('b'); | ||
expect(wrapper.find(CurationQuery).at(2).prop('queryValue')).toEqual('c'); | ||
}); | ||
|
||
it('calls editQuery when the CurationQuery value changes', () => { | ||
const wrapper = shallow(<CurationQueries {...props} />); | ||
wrapper.find(CurationQuery).at(0).simulate('change', 'new query value'); | ||
|
||
expect(actions.editQuery).toHaveBeenCalledWith(0, 'new query value'); | ||
}); | ||
|
||
it('calls deleteQuery when the CurationQuery calls onDelete', () => { | ||
const wrapper = shallow(<CurationQueries {...props} />); | ||
wrapper.find(CurationQuery).at(2).simulate('delete'); | ||
|
||
expect(actions.deleteQuery).toHaveBeenCalledWith(2); | ||
}); | ||
|
||
it('calls addQuery when the Add Query button is clicked', () => { | ||
const wrapper = shallow(<CurationQueries {...props} />); | ||
wrapper.find('[data-test-subj="addCurationQueryButton"]').simulate('click'); | ||
|
||
expect(actions.addQuery).toHaveBeenCalled(); | ||
}); | ||
|
||
it('calls the passed onSubmit callback when the submit button is clicked', () => { | ||
setMockValues({ ...values, queries: ['some query'] }); | ||
const wrapper = shallow(<CurationQueries {...props} />); | ||
wrapper.find('[data-test-subj="submitCurationQueriesButton"]').simulate('click'); | ||
|
||
expect(props.onSubmit).toHaveBeenCalledWith(['some query']); | ||
}); | ||
|
||
it('disables the submit button if no query fields have been filled', () => { | ||
setMockValues({ | ||
...values, | ||
queries: [''], | ||
hasOnlyOneQuery: true, | ||
hasEmptyQueries: true, | ||
}); | ||
const wrapper = shallow(<CurationQueries {...props} />); | ||
const button = wrapper.find('[data-test-subj="submitCurationQueriesButton"]'); | ||
|
||
expect(button.prop('isDisabled')).toEqual(true); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import React from 'react'; | ||
|
||
import { useValues, useActions } from 'kea'; | ||
|
||
import { EuiButton, EuiButtonEmpty, EuiSpacer } from '@elastic/eui'; | ||
import { i18n } from '@kbn/i18n'; | ||
|
||
import { Curation } from '../../types'; | ||
|
||
import { CurationQueriesLogic } from './curation_queries_logic'; | ||
import { CurationQuery } from './curation_query'; | ||
import { filterEmptyQueries } from './utils'; | ||
import './curation_queries.scss'; | ||
|
||
interface Props { | ||
queries: Curation['queries']; | ||
onSubmit(queries: Curation['queries']): void; | ||
submitButtonText?: string; | ||
} | ||
|
||
export const CurationQueries: React.FC<Props> = ({ | ||
queries: initialQueries, | ||
onSubmit, | ||
submitButtonText = i18n.translate('xpack.enterpriseSearch.actions.continue', { | ||
defaultMessage: 'Continue', | ||
}), | ||
}) => { | ||
const logic = CurationQueriesLogic({ queries: initialQueries }); | ||
const { queries, hasEmptyQueries, hasOnlyOneQuery } = useValues(logic); | ||
const { addQuery, editQuery, deleteQuery } = useActions(logic); | ||
|
||
return ( | ||
<> | ||
{queries.map((query: string, index) => ( | ||
<CurationQuery | ||
key={`query-${index}`} | ||
queryValue={query} | ||
onChange={(newValue) => editQuery(index, newValue)} | ||
onDelete={() => deleteQuery(index)} | ||
disableDelete={hasOnlyOneQuery} | ||
/> | ||
))} | ||
<EuiButtonEmpty | ||
size="s" | ||
iconType="plusInCircle" | ||
onClick={addQuery} | ||
isDisabled={hasEmptyQueries} | ||
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. Did you consider adding a test for this being disabled? I don't think it's required for code coverage, but it might be good to document this behavior via a test. 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. Great call, thanks! 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. |
||
data-test-subj="addCurationQueryButton" | ||
> | ||
{i18n.translate('xpack.enterpriseSearch.appSearch.engine.curations.addQueryButtonLabel', { | ||
defaultMessage: 'Add query', | ||
})} | ||
</EuiButtonEmpty> | ||
<EuiSpacer /> | ||
<EuiButton | ||
fill | ||
isDisabled={hasOnlyOneQuery && hasEmptyQueries} | ||
onClick={() => onSubmit(filterEmptyQueries(queries))} | ||
data-test-subj="submitCurationQueriesButton" | ||
> | ||
{submitButtonText} | ||
</EuiButton> | ||
</> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import { resetContext } from 'kea'; | ||
|
||
import { CurationQueriesLogic } from './curation_queries_logic'; | ||
|
||
describe('CurationQueriesLogic', () => { | ||
const MOCK_QUERIES = ['a', 'b', 'c']; | ||
|
||
const DEFAULT_PROPS = { queries: MOCK_QUERIES }; | ||
const DEFAULT_VALUES = { | ||
queries: MOCK_QUERIES, | ||
hasEmptyQueries: false, | ||
hasOnlyOneQuery: false, | ||
}; | ||
|
||
const mount = (props = {}) => { | ||
JasonStoltz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
CurationQueriesLogic({ ...DEFAULT_PROPS, ...props }); | ||
CurationQueriesLogic.mount(); | ||
}; | ||
|
||
beforeEach(() => { | ||
jest.clearAllMocks(); | ||
resetContext({}); | ||
}); | ||
|
||
it('has expected default values passed from props', () => { | ||
mount(); | ||
expect(CurationQueriesLogic.values).toEqual(DEFAULT_VALUES); | ||
}); | ||
|
||
describe('actions', () => { | ||
afterEach(() => { | ||
// Should not mutate the original array | ||
expect(CurationQueriesLogic.values.queries).not.toBe(MOCK_QUERIES); // Would fail if we did not clone a new array | ||
}); | ||
|
||
describe('addQuery', () => { | ||
it('appends an empty string to the queries array', () => { | ||
mount(); | ||
CurationQueriesLogic.actions.addQuery(); | ||
|
||
expect(CurationQueriesLogic.values).toEqual({ | ||
...DEFAULT_VALUES, | ||
hasEmptyQueries: true, | ||
queries: ['a', 'b', 'c', ''], | ||
}); | ||
}); | ||
}); | ||
|
||
describe('deleteQuery', () => { | ||
it('deletes the query string at the specified array index', () => { | ||
mount(); | ||
CurationQueriesLogic.actions.deleteQuery(1); | ||
|
||
expect(CurationQueriesLogic.values).toEqual({ | ||
...DEFAULT_VALUES, | ||
queries: ['a', 'c'], | ||
}); | ||
}); | ||
}); | ||
|
||
describe('editQuery', () => { | ||
it('edits the query string at the specified array index', () => { | ||
mount(); | ||
CurationQueriesLogic.actions.editQuery(2, 'z'); | ||
|
||
expect(CurationQueriesLogic.values).toEqual({ | ||
...DEFAULT_VALUES, | ||
queries: ['a', 'b', 'z'], | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('selectors', () => { | ||
describe('hasEmptyQueries', () => { | ||
it('returns true if queries has any empty strings', () => { | ||
mount({ queries: ['', '', ''] }); | ||
|
||
expect(CurationQueriesLogic.values.hasEmptyQueries).toEqual(true); | ||
}); | ||
}); | ||
|
||
describe('hasOnlyOneQuery', () => { | ||
it('returns true if queries only has one item', () => { | ||
mount({ queries: ['test'] }); | ||
|
||
expect(CurationQueriesLogic.values.hasOnlyOneQuery).toEqual(true); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import { kea, MakeLogicType } from 'kea'; | ||
|
||
interface CurationQueriesValues { | ||
queries: string[]; | ||
hasEmptyQueries: boolean; | ||
hasOnlyOneQuery: boolean; | ||
} | ||
|
||
interface CurationQueriesActions { | ||
addQuery(): void; | ||
deleteQuery(indexToDelete: number): { indexToDelete: number }; | ||
editQuery(index: number, newQueryValue: string): { index: number; newQueryValue: string }; | ||
} | ||
|
||
export const CurationQueriesLogic = kea< | ||
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. 👋 Just want to highlight this logic file as well - this actually doesn't strictly need to be a logic file and it's very lightweight, but I thought it was kinda fun/helped separate concerns to make it one. This was my original proof of concept with just basic React state: export const CurationQueries: React.FC = (props) => {
// TODO: logic file rather than state??
const [queries, setQueries] = useState(props.queries || ['']);
const addQuery = () => {
setQueries([...queries, '']);
};
const deleteQuery = (index: number) => {
const newArray = [...queries];
newArray.splice(index, 1);
setQueries(newArray);
};
const changeQuery = (index: number, newQueryValue: string) => {
const newArray = [...queries];
newArray[index] = newQueryValue;
setQueries(newArray);
};
const hasEmptyQueries = queries.indexOf('') >= 0;
const hasOnlyOneQuery = queries.length <= 1; Would be interested in hearing thoughts on what you do/don't prefer 🤷 I do think having it as logic makes it way easier to unit test. Another thing I played around with was keying this logic file with an 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 like this quite a bit constance. +1 for separation of concerns and easier unit testing. |
||
MakeLogicType<CurationQueriesValues, CurationQueriesActions> | ||
>({ | ||
path: ['enterprise_search', 'app_search', 'curation_queries_logic'], | ||
actions: () => ({ | ||
addQuery: true, | ||
deleteQuery: (indexToDelete) => ({ indexToDelete }), | ||
editQuery: (index, newQueryValue) => ({ index, newQueryValue }), | ||
}), | ||
reducers: ({ props }) => ({ | ||
queries: [ | ||
props.queries, | ||
{ | ||
addQuery: (state) => [...state, ''], | ||
deleteQuery: (state, { indexToDelete }) => { | ||
const newState = [...state]; | ||
newState.splice(indexToDelete, 1); | ||
return newState; | ||
}, | ||
editQuery: (state, { index, newQueryValue }) => { | ||
const newState = [...state]; | ||
newState[index] = newQueryValue; | ||
return newState; | ||
}, | ||
}, | ||
], | ||
}), | ||
selectors: { | ||
hasEmptyQueries: [(selectors) => [selectors.queries], (queries) => queries.indexOf('') >= 0], | ||
hasOnlyOneQuery: [(selectors) => [selectors.queries], (queries) => queries.length <= 1], | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import React from 'react'; | ||
|
||
import { shallow } from 'enzyme'; | ||
|
||
import { EuiFieldText } from '@elastic/eui'; | ||
|
||
import { CurationQuery } from './curation_query'; | ||
|
||
describe('CurationQuery', () => { | ||
const props = { | ||
queryValue: 'some query', | ||
onChange: jest.fn(), | ||
onDelete: jest.fn(), | ||
disableDelete: false, | ||
}; | ||
|
||
beforeEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
|
||
it('renders', () => { | ||
const wrapper = shallow(<CurationQuery {...props} />); | ||
|
||
expect(wrapper.find(EuiFieldText)).toHaveLength(1); | ||
expect(wrapper.find(EuiFieldText).prop('value')).toEqual('some query'); | ||
}); | ||
|
||
it('calls onChange when the input value changes', () => { | ||
const wrapper = shallow(<CurationQuery {...props} />); | ||
wrapper.find(EuiFieldText).simulate('change', { target: { value: 'new query value' } }); | ||
|
||
expect(props.onChange).toHaveBeenCalledWith('new query value'); | ||
}); | ||
|
||
it('calls onDelete when the delete button is clicked', () => { | ||
const wrapper = shallow(<CurationQuery {...props} />); | ||
wrapper.find('[data-test-subj="deleteCurationQueryButton"]').simulate('click'); | ||
|
||
expect(props.onDelete).toHaveBeenCalled(); | ||
}); | ||
|
||
it('disables the delete button if disableDelete is passed', () => { | ||
const wrapper = shallow(<CurationQuery {...props} disableDelete />); | ||
const button = wrapper.find('[data-test-subj="deleteCurationQueryButton"]'); | ||
|
||
expect(button.prop('isDisabled')).toEqual(true); | ||
}); | ||
}); |
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.
You include this in 2 different components.
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.
Doh, awesome catch
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.
3f70ef8