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] Create Curation view/functionality #92560

Merged
merged 9 commits into from
Feb 25, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
@@ -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';
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh, awesome catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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


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}
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great call, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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<
Copy link
Contributor Author

@cee-chen cee-chen Feb 24, 2021

Choose a reason for hiding this comment

The 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 id, but it actually made testing kind of a pain (since you have to store that keyed logic file reference in a var and can't just refer back to the plain imported CurationQueriesLogic), and I also decided I didn't really need it since the component fully unmounts/mounts between page visits/modal closures.

Copy link
Member

Choose a reason for hiding this comment

The 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);
});
});
Loading