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

[Discover] Dataset search on page load issues #8871

Merged
merged 5 commits into from
Nov 15, 2024
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
2 changes: 2 additions & 0 deletions changelogs/fragments/8871.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- Search on page load out of sync state when clicking submit. ([#8871](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8871))
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,48 @@ describe('useSearch', () => {
});
});

it('should initialize with uninitialized state when dataset type config search on page load is disabled', async () => {
const services = createMockServices();
(services.uiSettings.get as jest.Mock).mockReturnValueOnce(true);
(services.data.query.queryString.getDatasetService as jest.Mock).mockReturnValue({
meta: { searchOnLoad: false },
});
(services.data.query.timefilter.timefilter.getRefreshInterval as jest.Mock).mockReturnValue({
pause: true,
value: 10,
});

const { result, waitForNextUpdate } = renderHook(() => useSearch(services), { wrapper });
expect(result.current.data$.getValue()).toEqual(
expect.objectContaining({ status: ResultStatus.UNINITIALIZED })
);

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

it('should initialize with uninitialized state when dataset type config search on page load is enabled but the UI setting is disabled', async () => {
const services = createMockServices();
(services.uiSettings.get as jest.Mock).mockReturnValueOnce(false);
(services.data.query.queryString.getDatasetService as jest.Mock).mockReturnValue({
meta: { searchOnLoad: true },
});
(services.data.query.timefilter.timefilter.getRefreshInterval as jest.Mock).mockReturnValue({
pause: true,
value: 10,
});

const { result, waitForNextUpdate } = renderHook(() => useSearch(services), { wrapper });
expect(result.current.data$.getValue()).toEqual(
expect.objectContaining({ status: ResultStatus.UNINITIALIZED })
);

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

it('should update startTime when hook rerenders', async () => {
const services = createMockServices();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,24 +115,23 @@
requests: new RequestAdapter(),
};

const getDatasetAutoSearchOnPageLoadPreference = () => {
// Checks the searchOnpageLoadPreference for the current dataset if not specifed defaults to true
const datasetType = data.query.queryString.getQuery().dataset?.type;

const datasetService = data.query.queryString.getDatasetService();

return !datasetType || (datasetService?.getType(datasetType)?.meta?.searchOnLoad ?? true);
};

const shouldSearchOnPageLoad = useCallback(() => {
// Checks the searchOnpageLoadPreference for the current dataset if not specifed defaults to UI Settings
const { queryString } = data.query;
const { dataset } = queryString.getQuery();

Check warning on line 121 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#L120-L121

Added lines #L120 - L121 were not covered by tests
const typeConfig = dataset ? queryString.getDatasetService().getType(dataset.type) : undefined;
const datasetPreference =
typeConfig?.meta?.searchOnLoad ?? uiSettings.get(SEARCH_ON_PAGE_LOAD_SETTING);

// A saved search is created on every page load, so we check the ID to see if we're loading a
// previously saved search or if it is just transient
return (
services.uiSettings.get(SEARCH_ON_PAGE_LOAD_SETTING) ||
datasetPreference ||
uiSettings.get(SEARCH_ON_PAGE_LOAD_SETTING) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

why repeat this when part of datasetPreference assignment above?

Copy link
Member

Choose a reason for hiding this comment

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

+1

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 believe i was just over thinking. in terms of code it is redundant but i was little bit worried that return true if undefined isn't the most accurate depiction of what should happen.

but with fresh eyes that doesn't really make sense. in terms of the ci taking an hour, could this be a fast follow?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure, approving

savedSearch?.id !== undefined ||
timefilter.getRefreshInterval().pause === false
);
}, [savedSearch, services.uiSettings, timefilter]);
}, [data.query, savedSearch, uiSettings, timefilter]);

const startTime = Date.now();
const data$ = useMemo(
Expand Down Expand Up @@ -346,9 +345,6 @@
]);

useEffect(() => {
if (!getDatasetAutoSearchOnPageLoadPreference()) {
skipInitialFetch.current = true;
}
const fetch$ = merge(
refetch$,
filterManager.getFetches$(),
Expand Down Expand Up @@ -379,8 +375,6 @@
return () => {
subscription.unsubscribe();
};
// disabling the eslint since we are not adding getDatasetAutoSearchOnPageLoadPreference since this changes when dataset changes and these chnages are already part of data.query.queryString
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [
data$,
data.query.queryString,
Expand Down
Loading