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.

Sign up for GitHub

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

[Discover] Fix Initialization if No Saved Query #8930

Merged
merged 3 commits into from
Dec 3, 2024
Merged

[Discover] Fix Initialization if No Saved Query #8930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a30fbe
replace default query with current query
sejli Nov 26, 2024
e3b8ae8
Changeset file for PR #8930 created/updated
opensearch-changeset-bot[bot] Nov 26, 2024
646cca5
adding unit tests
sejli Nov 27, 2024
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
2 changes: 2 additions & 0 deletions changelogs/fragments/8930.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- Update saved search initialization logic to use current query instead of default query ([#8930](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8930))
59 changes: 58 additions & 1 deletion src/plugins/discover/public/application/view_components/utils/use_search.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,37 @@ jest.mock('./use_index_pattern', () => ({
useIndexPattern: jest.fn(),
}));

const mockQuery = {
query: 'test query',
language: 'test language',
};

const mockDefaultQuery = {
query: 'default query',
language: 'default language',
};

const mockSavedSearch = {
id: 'test-saved-search',
title: 'Test Saved Search',
searchSource: {
setField: jest.fn(),
getField: jest.fn(),
getField: jest.fn().mockReturnValue(mockQuery),
fetch: jest.fn(),
getSearchRequestBody: jest.fn().mockResolvedValue({}),
getOwnField: jest.fn(),
getDataFrame: jest.fn(() => ({ name: 'test-pattern' })),
},
getFullPath: jest.fn(),
getOpenSearchType: jest.fn(),
};

const mockSavedSearchEmptyQuery = {
id: 'test-saved-search',
title: 'Test Saved Search',
searchSource: {
setField: jest.fn(),
getField: jest.fn().mockReturnValue(undefined),
fetch: jest.fn(),
getSearchRequestBody: jest.fn().mockResolvedValue({}),
getOwnField: jest.fn(),
Expand Down Expand Up @@ -215,4 +240,36 @@ describe('useSearch', () => {
expect.objectContaining({ status: ResultStatus.LOADING, rows: [] })
);
});

it('should load saved search', async () => {
const services = createMockServices();
services.data.query.queryString.setQuery = jest.fn();

const { waitForNextUpdate } = renderHook(() => useSearch(services), {
wrapper,
});

await act(async () => {
await waitForNextUpdate();
});

expect(services.data.query.queryString.setQuery).toBeCalledWith(mockQuery);
});

it('if no saved search, use get query', async () => {
const services = createMockServices();
services.getSavedSearchById = jest.fn().mockResolvedValue(mockSavedSearchEmptyQuery);
services.data.query.queryString.getQuery = jest.fn().mockReturnValue(mockDefaultQuery);
services.data.query.queryString.setQuery = jest.fn();

const { waitForNextUpdate } = renderHook(() => useSearch(services), {
wrapper,
});

await act(async () => {
await waitForNextUpdate();
});

expect(services.data.query.queryString.setQuery).toBeCalledWith(mockDefaultQuery);
});
});
5 changes: 2 additions & 3 deletions src/plugins/discover/public/application/view_components/utils/use_search.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,8 +392,7 @@
const savedSearchInstance = await getSavedSearchById(savedSearchId);

const query =
savedSearchInstance.searchSource.getField('query') ||
data.query.queryString.getDefaultQuery();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since .getDefaultQuery() isn't being used here, is it still used in other locations? I would've assumed I would see the removal of this function as well after reading the description

Copy link
Member

Choose a reason for hiding this comment

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

i dont know if we want removal of the function in general. but i understand what you mean it should probably be a private function within the query string manager.

savedSearchInstance.searchSource.getField('query') || data.query.queryString.getQuery();

const isEnhancementsEnabled = await uiSettings.get('query:enhancements:enabled');
if (isEnhancementsEnabled && query.dataset) {
Expand Down Expand Up @@ -432,7 +431,7 @@
}

filterManager.setAppFilters(actualFilters);
data.query.queryString.setQuery(savedQuery ? data.query.queryString.getQuery() : query);
data.query.queryString.setQuery(query);

Check warning on line 434 in src/plugins/discover/public/application/view_components/utils/use_search.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/discover/public/application/view_components/utils/use_search.ts#L434 

Added line #L434 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

does this have any impact to saved queries? like logically to me when reading this this makes sense why we would remove it but save query expected functionality is a little weird to me. so if a saved query is applied and we update the query. but dont fire it's the correct bheavior here right

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this here because originally, query was either the query from the savedSearch or the default query. Since I changed query to data.query.queryString.getQuery(), it shouldn't have any impact since we will instead only be using data.query.queryString.getQuery() if there is no saved search.

setSavedSearch(savedSearchInstance);

if (savedSearchInstance?.id) {
Expand Down
Loading