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

[TableListView] Fix tag selection when passing initialFilter #160871

Merged
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 @@ -781,6 +781,49 @@ describe('TableListView', () => {
});
});

describe('initialFilter', () => {
const setupInitialFilter = registerTestBed<string, TableListViewTableProps>(
WithServices<TableListViewTableProps>(TableListViewTable, {
getTagList: () => [
{ id: 'id-tag-foo', name: 'foo', type: 'tag', description: '', color: '' },
],
}),
{
defaultProps: { ...requiredProps },
memoryRouter: { wrapComponent: true },
}
);

test('should filter by tag passed as in initialFilter prop', async () => {
let testBed: TestBed;

const initialFilter = 'tag:(tag-1)';
const findItems = jest.fn().mockResolvedValue({ total: 0, hits: [] });

await act(async () => {
testBed = await setupInitialFilter({
findItems,
initialFilter,
urlStateEnabled: false,
});
});

const { component, find } = testBed!;
component.update();

const getSearchBoxValue = () => find('tableListSearchBox').props().defaultValue;

const getLastCallArgsFromFindItems = () =>
findItems.mock.calls[findItems.mock.calls.length - 1];

// The search bar should be updated
const expected = initialFilter;
const [searchTerm] = getLastCallArgsFromFindItems();
expect(getSearchBoxValue()).toBe(expected);
expect(searchTerm).toBe(expected);
});
});

describe('url state', () => {
let router: Router | undefined;

Expand Down Expand Up @@ -1265,7 +1308,7 @@ describe('TableList', () => {
it('reports the page data test subject', async () => {
const setPageDataTestSubject = jest.fn();

act(() => {
await act(async () => {
setup({ setPageDataTestSubject });
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,15 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({

const isMounted = useRef(false);
const fetchIdx = useRef(0);
/**
* The "onTableSearchChange()" handler has an async behavior. We want to be able to discard
* previsous search changes and only handle the last one. For that we keep a counter of the changes.
*/
const tableSearchChangeIdx = useRef(0);
/**
* We want to build the initial query
*/
const initialQueryInitialized = useRef(false);

const {
canEditAdvancedSettings,
Expand Down Expand Up @@ -333,10 +342,7 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
showDeleteModal: false,
hasUpdatedAtMetadata: false,
selectedIds: [],
searchQuery:
initialQuery !== undefined
? { text: initialQuery, query: new Query(Ast.create([]), undefined, initialQuery) }
: { text: '', query: new Query(Ast.create([]), undefined, '') },
searchQuery: { text: '', query: new Query(Ast.create([]), undefined, '') },
pagination: {
pageIndex: 0,
totalItemCount: 0,
Expand All @@ -348,7 +354,7 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
direction: 'asc',
},
}),
[initialPageSize, initialQuery]
[initialPageSize]
);

const [state, dispatch] = useReducer(reducer, initialState);
Expand Down Expand Up @@ -614,12 +620,67 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
// ------------
// Callbacks
// ------------
const buildQueryFromText = useCallback(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This handler has been extracted from updateQueryFromURL() below

async (text: string) => {
let ast = Ast.create([]);
let termMatch = text;

if (searchQueryParser) {
// Parse possible tags in the search text
const {
references,
referencesToExclude,
searchQuery: searchTerm,
} = await searchQueryParser(text);

termMatch = searchTerm;

if (references?.length || referencesToExclude?.length) {
const allTags = getTagList();

if (references?.length) {
references.forEach(({ id: refId }) => {
const tag = allTags.find(({ id }) => id === refId);
if (tag) {
ast = ast.addOrFieldValue('tag', tag.name, true, 'eq');
}
});
}

if (referencesToExclude?.length) {
referencesToExclude.forEach(({ id: refId }) => {
const tag = allTags.find(({ id }) => id === refId);
if (tag) {
ast = ast.addOrFieldValue('tag', tag.name, false, 'eq');
}
});
}
}
}

if (termMatch.trim() !== '') {
ast = ast.addClause({ type: 'term', value: termMatch, match: 'must' });
}

return new Query(ast, undefined, text);
},
[getTagList, searchQueryParser]
);

const onTableSearchChange = useCallback(
(arg: { query: Query | null; queryText: string }) => {
const query = arg.query ?? new Query(Ast.create([]), undefined, arg.queryText);
updateQuery(query);
if (arg.query) {
updateQuery(arg.query);
} else {
const idx = tableSearchChangeIdx.current + 1;
buildQueryFromText(arg.queryText).then((query) => {
if (idx === tableSearchChangeIdx.current) {
updateQuery(query);
}
});
}
},
[updateQuery]
[updateQuery, buildQueryFromText]
);

const updateTableSortAndPagination = useCallback(
Expand Down Expand Up @@ -815,47 +876,7 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({

// Update our Query instance based on the URL "s" text
const updateQueryFromURL = async (text: string = '') => {
let ast = Ast.create([]);
let termMatch = text;

if (searchQueryParser) {
// Parse possible tags in the search text
const {
references,
referencesToExclude,
searchQuery: searchTerm,
} = await searchQueryParser(text);

termMatch = searchTerm;

if (references?.length || referencesToExclude?.length) {
const allTags = getTagList();

if (references?.length) {
references.forEach(({ id: refId }) => {
const tag = allTags.find(({ id }) => id === refId);
if (tag) {
ast = ast.addOrFieldValue('tag', tag.name, true, 'eq');
}
});
}

if (referencesToExclude?.length) {
referencesToExclude.forEach(({ id: refId }) => {
const tag = allTags.find(({ id }) => id === refId);
if (tag) {
ast = ast.addOrFieldValue('tag', tag.name, false, 'eq');
}
});
}
}
}

if (termMatch.trim() !== '') {
ast = ast.addClause({ type: 'term', value: termMatch, match: 'must' });
}

const updatedQuery = new Query(ast, undefined, text);
const updatedQuery = await buildQueryFromText(text);

dispatch({
type: 'onSearchQueryChange',
Expand Down Expand Up @@ -885,7 +906,7 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({

updateQueryFromURL(urlState.s);
updateSortFromURL(urlState.sort);
}, [urlState, searchQueryParser, getTagList, urlStateEnabled]);
}, [urlState, buildQueryFromText, urlStateEnabled]);

useEffect(() => {
isMounted.current = true;
Expand All @@ -895,6 +916,13 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
};
}, []);

useEffect(() => {
if (initialQuery && !initialQueryInitialized.current) {
initialQueryInitialized.current = true;
buildQueryFromText(initialQuery).then(updateQuery);
}
}, [initialQuery, buildQueryFromText, updateQuery]);

const PageTemplate = useMemo<typeof KibanaPageTemplate>(() => {
return withoutPageTemplateWrapper
? ((({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import { FormattedRelative, I18nProvider } from '@kbn/i18n-react';
import React, { PropsWithChildren, useCallback, useState } from 'react';
import React, { PropsWithChildren, useCallback, useMemo, useState } from 'react';

import {
type TableListViewKibanaDependencies,
Expand Down Expand Up @@ -206,6 +206,14 @@ export const DashboardListing = ({

const { getEntityName, getTableListTitle, getEntityNamePlural } = dashboardListingTableStrings;

const savedObjectsTaggingFakePlugin = useMemo(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a stable reference of the savedObjectsTagging dep.

Copy link
Contributor

Choose a reason for hiding this comment

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

UseMemo addition makes sense. We'll be introducing a simpler services mechanic at some point, and will remove this.

return savedObjectsTagging.hasApi // TODO: clean up this logic once https://github.com/elastic/kibana/issues/140433 is resolved
? ({
ui: savedObjectsTagging,
} as TableListViewKibanaDependencies['savedObjectsTagging'])
: undefined;
}, [savedObjectsTagging]);

return (
<I18nProvider>
<TableListViewKibanaProvider
Expand All @@ -217,11 +225,7 @@ export const DashboardListing = ({
http,
},
toMountPoint,
savedObjectsTagging: savedObjectsTagging.hasApi // TODO: clean up this logic once https://github.com/elastic/kibana/issues/140433 is resolved
? ({
ui: savedObjectsTagging,
} as TableListViewKibanaDependencies['savedObjectsTagging'])
: undefined,
savedObjectsTagging: savedObjectsTaggingFakePlugin,
FormattedRelative,
}}
>
Expand Down