Add SearchOnLoad Preference for dataset #8513

LDrago27 · 2024-10-07T20:43:08Z

Description

This introduces a searchOnPageLoad preference for each dataset type.

When Advanced setting of searchOnPageLoad is OFF we respect the Advanced setting
When Advanced setting of searchOnPageLoad is ON we respect the selected dataset's searchOnLoad preference

Issues Resolved

Screenshot

Testing the changes

For this demonstration below I have explicitly updated the searchOnLoad preference for indexes to be false.

Meeting+Recording+-+Sahoo.+Suchit+Instant+Meeting.1.mp4

Changelog

skip

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

codecov · 2024-10-07T20:59:42Z

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 60.93%. Comparing base (58e1645) to head (aae4ca9).
Report is 35 commits behind head on main.

Files with missing lines	Patch %	Lines
...ic/application/view_components/utils/use_search.ts	0.00%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8513      +/-   ##
==========================================
- Coverage   60.93%   60.93%   -0.01%     
==========================================
  Files        3767     3771       +4     
  Lines       89387    89555     +168     
  Branches    13985    14020      +35     
==========================================
+ Hits        54471    54566      +95     
- Misses      31519    31579      +60     
- Partials     3397     3410      +13

Flag	Coverage Δ
Linux_1	`29.00% <0.00%> (+0.04%)`	⬆️
Linux_2	`56.27% <ø> (+0.02%)`	⬆️
Linux_3	`37.74% <0.00%> (-0.01%)`	⬇️
Linux_4	`29.92% <ø> (+<0.01%)`	⬆️
Windows_1	`29.02% <0.00%> (+0.04%)`	⬆️
Windows_2	`56.23% <ø> (+0.02%)`	⬆️
Windows_3	`37.74% <0.00%> (-0.01%)`	⬇️
Windows_4	`29.92% <ø> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Signed-off-by: Suchit Sahoo <[email protected]>

amsiglan · 2024-10-10T20:34:38Z

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

        queryStatus: { startTime },
      }),
-    [shouldSearchOnPageLoad, startTime]
+    [shouldSearchOnPageLoad, startTime, skipInitialFetch]


should we check for skipInitialFetch.current instead?

ananzh · 2024-10-10T21:10:37Z

src/plugins/data/public/query/query_string/dataset_service/types.ts

@@ -30,6 +30,8 @@ export interface DatasetTypeConfig {
    icon: EuiIconProps;
    /** Optional tooltip text */
    tooltip?: string;
+    /** Optional preference for search on page load else defaulted to true */
+    searchOnLoad?: boolean;


nit: So you set this one as an optional property. Seems searchOnLoad property is added to some dataset types but not all? Seems we have searchOnLoad on index pattern type, index type and s3 type. what else types need this searchOnLoad that allows this property to be missing? To make it consistent, maybe remove ? because this return !datasetType || (datasetService?.getType(datasetType)?.meta?.searchOnLoad ?? true already set the default value to true right? so ? is not necessary.

nice callout. I think we should have different types for the config we provide and the config we receive from the service. Not a blocker for the PR though

ananzh · 2024-10-10T21:13:42Z

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

@@ -111,6 +112,15 @@ export const useSearch = (services: DiscoverViewServices) => {
    requests: new RequestAdapter(),
  };

+  const getDatasetAutoSearchOnPageLoadPreference = () => {


I know it is a bit too much to ask. maybe we should add a test here given we have more and more logic in useSearch.

+1 we definitely need to add a test for this

ananzh

I am okay with merging this fix if it is urgent. A few nit points and one question: When a search is not performed automatically due to this setting, consider adding a visual indicator to inform the user that they need to manually trigger the search given that most users are familiar with old auto search and might not know they need to click.

ashwin-pc

Can be a fast follow but lets add tests for the fix.

ashwin-pc · 2024-10-16T09:43:46Z

src/plugins/data/public/query/query_string/dataset_service/types.ts

@@ -30,6 +30,8 @@ export interface DatasetTypeConfig {
    icon: EuiIconProps;
    /** Optional tooltip text */
    tooltip?: string;
+    /** Optional preference for search on page load else defaulted to true */
+    searchOnLoad?: boolean;


nice callout. I think we should have different types for the config we provide and the config we receive from the service. Not a blocker for the PR though

ashwin-pc · 2024-10-16T09:44:37Z

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

@@ -111,6 +112,15 @@ export const useSearch = (services: DiscoverViewServices) => {
    requests: new RequestAdapter(),
  };

+  const getDatasetAutoSearchOnPageLoadPreference = () => {


+1 we definitely need to add a test for this

ashwin-pc · 2024-10-16T09:45:29Z

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

        queryStatus: { startTime },
      }),
-    [shouldSearchOnPageLoad, startTime]
+    [shouldSearchOnPageLoad, startTime, skipInitialFetch]


Signed-off-by: Suchit Sahoo <[email protected]> (cherry picked from commit 5c8b2e3) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

(cherry picked from commit 5c8b2e3) Signed-off-by: Suchit Sahoo <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Signed-off-by: Suchit Sahoo <[email protected]>

github-actions bot added all-star-contributor Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry labels Oct 7, 2024

LDrago27 force-pushed the stopSearchOnPageLoad branch 2 times, most recently from 72b5b0e to edbfd00 Compare October 8, 2024 19:12

LDrago27 added discover-next v2.18.0 discover for discover reinvent labels Oct 8, 2024

LDrago27 marked this pull request as ready for review October 8, 2024 20:31

LDrago27 requested review from ananzh, kavilla, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, curq, bandinib-amzn, SuZhou-Joe, ruanyl, BionIT, xinruiba, zhyuanqi, mengweieric, virajsanghvi and sejli as code owners October 8, 2024 20:31

LDrago27 requested review from joshuali925 and huyaboo as code owners October 8, 2024 20:31

LDrago27 changed the title ~~[WIP] Add SearchOnPageLoad Preference for dataset~~ Add SearchOnLoad Preference for dataset Oct 8, 2024

amsiglan reviewed Oct 9, 2024

View reviewed changes

src/plugins/discover/public/application/view_components/utils/use_search.ts Outdated Show resolved Hide resolved

Add searchOnLoad property to datasets

aae4ca9

Signed-off-by: Suchit Sahoo <[email protected]>

LDrago27 force-pushed the stopSearchOnPageLoad branch from edbfd00 to aae4ca9 Compare October 10, 2024 18:04

amsiglan reviewed Oct 10, 2024

View reviewed changes

ananzh reviewed Oct 10, 2024

View reviewed changes

ananzh approved these changes Oct 10, 2024

View reviewed changes

LDrago27 linked an issue Oct 10, 2024 that may be closed by this pull request

[BUG] Discover runs query again when adding or removing selected fields #8552

Closed

ashwin-pc approved these changes Oct 16, 2024

View reviewed changes

LDrago27 merged commit 5c8b2e3 into opensearch-project:main Oct 16, 2024
68 of 69 checks passed

LDrago27 added the backport 2.x label Oct 16, 2024

opensearch-trigger-bot bot mentioned this pull request Oct 16, 2024

[Backport 2.x] Add SearchOnLoad Preference for dataset #8610

Merged

sejli pushed a commit to sejli/OpenSearch-Dashboards that referenced this pull request Oct 21, 2024

Add searchOnLoad property to datasets (opensearch-project#8513)

db15ef8

Signed-off-by: Suchit Sahoo <[email protected]>

Qxisylolo pushed a commit to Qxisylolo/OpenSearch-Dashboards that referenced this pull request Oct 30, 2024

Add searchOnLoad property to datasets (opensearch-project#8513)

0e0b6dc

Signed-off-by: Suchit Sahoo <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SearchOnLoad Preference for dataset #8513

Add SearchOnLoad Preference for dataset #8513

LDrago27 commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

amsiglan Oct 10, 2024

ashwin-pc Oct 16, 2024

ananzh Oct 10, 2024

ashwin-pc Oct 16, 2024

ananzh Oct 10, 2024

ashwin-pc Oct 16, 2024

ananzh left a comment

ashwin-pc left a comment

ashwin-pc Oct 16, 2024

ashwin-pc Oct 16, 2024

ashwin-pc Oct 16, 2024

Add SearchOnLoad Preference for dataset #8513

Add SearchOnLoad Preference for dataset #8513

Conversation

LDrago27 commented Oct 7, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ananzh left a comment

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LDrago27 commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading