[Multiple Datasource] Enhanced data source selector with default datasource shows as first choice #6293

zhyuanqi · 2024-03-29T08:14:09Z

Description

add the default icon in the selector
Add a showDefault function that plugins team can use and get the Default datasource

Issues Resolved

Screenshot

Screen.Recording.2024-04-01.at.9.53.47.AM.mov

Testing the changes

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-03-29T08:29:38Z

Codecov Report

Attention: Patch coverage is 87.87879% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 67.50%. Comparing base (cb24346) to head (bace7c3).

Files	Patch %	Lines
...ents/data_source_selector/data_source_selector.tsx	81.81%	2 Missing ⚠️
.../data_source_management/public/components/utils.ts	88.88%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6293   +/-   ##
=======================================
  Coverage   67.50%   67.50%           
=======================================
  Files        3376     3376           
  Lines       65800    65830   +30     
  Branches    10639    10648    +9     
=======================================
+ Hits        44417    44441   +24     
- Misses      18800    18803    +3     
- Partials     2583     2586    +3

Flag	Coverage Δ
Linux_1	`32.60% <ø> (ø)`
Linux_2	`55.60% <ø> (ø)`
Linux_3	`44.87% <87.87%> (+0.01%)`	⬆️
Linux_4	`34.99% <0.00%> (-0.03%)`	⬇️
Windows_1	`32.62% <ø> (ø)`
Windows_2	`55.57% <ø> (ø)`
Windows_3	`44.89% <87.87%> (+0.03%)`	⬆️
Windows_4	`34.99% <0.00%> (-0.03%)`	⬇️

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.

ZilongX · 2024-04-01T17:03:37Z

nit : add a prefix [Multiple Datasource] in the commit msg so all MD related changes would be easily grouped together, like [Multiple Datasource] - editdatasource selector to have default behavior

ZilongX · 2024-04-01T17:04:34Z

CHANGELOG.md

@@ -162,6 +162,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - [Discover] Enhanced the data source selector with added sorting functionality ([#5719](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5719))
 - [Multiple Datasource] Add datasource picker component and use it in devtools and tutorial page when multiple datasource is enabled ([#5756](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5756))
 - [Multiple Datasource] Add datasource picker to import saved object flyout when multiple data source is enabled ([#5781](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5781))
+- [Multiple Datasource] Add show default datasource for selector picker ([#6293](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6293/files))


nit : add the changelog entry in the middle (say line 163) instead of append it in the last line would save some conflict resolve efforts in case any

Got it! Good advise! It is ver helpful. Thanks

correct the PR link to remove the "file" at the end?

Suggested change

- [Multiple Datasource] Add show default datasource for selector picker ([#6293](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6293/files))

- [Multiple Datasource] Add show default datasource for selector picker ([#6293](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6293))

Can we fix the position of the changelog, it is now under 2.12 section

Flyingliuhub · 2024-04-01T17:10:03Z

src/plugins/data_source_management/public/components/utils.test.ts

+    });
+
+    it('should return local cluster if it exists and no default options in the data sources', () => {
+      mockUiSettingsCalls(uiSettings, 'get', null);


null, Is it correct argument which passed here?

Yes. So if the uiSettings could not find the defaultDataSource in the config, it will return null.

Flyingliuhub · 2024-04-01T17:15:26Z

src/plugins/data_source_management/public/components/utils.ts

+  } else if (
+    uiSettings &&
+    uiSettings?.get('defaultDataSource', null) !== null &&
+    dataSources.find((dataSource) => dataSource.id === uiSettings?.get('defaultDataSource'))


There are two locations were used uiSettings?.get('defaultDataSource'), is it possible to use const defaultDataSourceId = uiSettings?.get('defaultDataSource'); to simplify the code

Sure. I can work on this

Flyingliuhub · 2024-04-01T17:19:02Z

src/plugins/data_source_management/public/components/utils.ts

+) {
+  let defaultShowingDataSource: DataSourceOption;
+
+  if (defaultOption && dataSources.find((dataSource) => dataSource.id === defaultOption?.[0].id)) {


May be we can define the those value at the beginning, make the logic more clear

if (defaultDataSource) { ... } else if (uiSettingsDataSource) { .... } else if (!hideLocalCluster) { .... } else { ... }

Make sense, let me check on this

@zhyuanqi @Flyingliuhub

think about adopting early return pattern

https://gomakethings.com/the-early-return-pattern-in-javascript/

Sure. Will check on this.

.../data_source_management/public/components/data_source_selector/data_source_selector.test.tsx

xinruiba · 2024-04-01T19:15:50Z

...ugins/data_source_management/public/components/data_source_selector/data_source_selector.tsx

+        if (this.props.showDefault) {
+          const dataSources = getFilterDataSources(
+            this.state.allDataSources,
+            this.props.dataSourceFilter
+          );
+          const defaultDataSource = getDefaultDataSource(
+            dataSources,
+            LocalCluster,
+            this.props.uiSettings,
+            this.props.hideLocalCluster,
+            this.props.defaultOption
+          );
+          this.props.showDefault(defaultDataSource);
+        }


Thanks for this change.
I remember @BionIT have a PR to Use data source filter function before rendering

And in this line: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6175/files#diff-ec5b6ef64504177e50cad326dcc07ada2135faafe924d25b0f4df088bc7b4823L77 she move filter function call from componentDidMount() to render().

Are there any concerns to follow the same format by also moving this logic into render() function?

Thanks

Hi This showDefault should be use as a callback function that plugins team can use to get from the defaultShowing datasource. Below is how plugin team use it.

const [defaultDataSource, setDefaultDataSource] = React.useState<DataSourceOption>(); const showDefault = (source: DataSourceOption) => { setDefaultDataSource(source); }; <DataSourceSelector ....... showDefault={showDefault} />

As componentDidMount should perform data fetching and initialization where render focus on rendering the UI based on component's state.Let me look into this more

zhongnansu · 2024-04-01T18:25:44Z

src/plugins/data_source_management/public/components/utils.ts

@@ -78,6 +79,41 @@ export async function setFirstDataSourceAsDefault(
  }
 }

+export function getFilterDataSources(


would getFilteredDataSources be a better naming?

Sure will rename it

@zhongnansu @zhyuanqi , would you use suggested fix feature for quick apply the suggestion, like below

Suggested change

export function getFilterDataSources(

export function getFilteredDataSources(

good suggestion, will do!

zhongnansu · 2024-04-01T19:39:08Z

src/plugins/data_source_management/public/components/utils.test.ts

 import { AuthenticationMethod, AuthenticationMethodRegistry } from '../auth_registry';
 import { deepEqual } from 'assert';
+import { DataSourceAttributes } from 'src/plugins/data_source/common/data_sources'; // Import the missing 'SigV4Content' here


do we need to import SigV4Content? can we remove the comment?

Let me remove this line here.

zhongnansu · 2024-04-01T19:52:25Z

...ugins/data_source_management/public/components/data_source_selector/data_source_selector.tsx

@@ -29,6 +30,8 @@ export interface DataSourceSelectorProps {
  removePrepend?: boolean;
  dataSourceFilter?: (dataSource: SavedObject<DataSourceAttributes>) => boolean;
  compressed?: boolean;
+  uiSettings?: IUiSettingsClient;
+  showDefault?: (source: DataSourceOption) => void;


might be a dumb question, but why do we want those 2 param to be optional? shouldn't we always show default?

The uiSettings, for plugins, we want to pass it during plugin setup, so i set it as optional. The showDefault is a callback function, depends if plugin wants to call this the see what is the default or not. So it is optional

got it, thx Ella

BionIT · 2024-04-02T03:51:30Z

CHANGELOG.md

@@ -162,6 +162,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - [Discover] Enhanced the data source selector with added sorting functionality ([#5719](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5719))
 - [Multiple Datasource] Add datasource picker component and use it in devtools and tutorial page when multiple datasource is enabled ([#5756](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5756))
 - [Multiple Datasource] Add datasource picker to import saved object flyout when multiple data source is enabled ([#5781](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5781))
+- [Multiple Datasource] Add show default datasource for selector picker ([#6293](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6293/files))


Can we fix the position of the changelog, it is now under 2.12 section

...ource_management/public/components/data_source_selector/create_data_source_selector.test.tsx

BionIT · 2024-04-02T03:57:13Z

...ugins/data_source_management/public/components/data_source_selector/data_source_selector.tsx

@@ -76,6 +79,20 @@ export class DataSourceSelector extends React.Component<
            allDataSources: fetchedDataSources,
          });
        }
+        if (this.props.showDefault) {
+          const dataSources = getFilterDataSources(
+            this.state.allDataSources,


we can just pass fetchedDataSources

BionIT · 2024-04-02T03:57:40Z

...ugins/data_source_management/public/components/data_source_selector/data_source_selector.tsx

@@ -76,6 +79,20 @@ export class DataSourceSelector extends React.Component<
            allDataSources: fetchedDataSources,
          });
        }
+        if (this.props.showDefault) {
+          const dataSources = getFilterDataSources(


this should be filteredDataSources

BionIT · 2024-04-02T04:00:08Z

...ugins/data_source_management/public/components/data_source_selector/data_source_selector.tsx

+            this.props.hideLocalCluster,
+            this.props.defaultOption
+          );
+          this.props.showDefault(defaultDataSource);


can we reuse the existing callback function? onSelectedDataSource

I can make the change. Just worry about if it would break any existing behavior. Let me change on this

BionIT · 2024-04-02T04:04:42Z

...ugins/data_source_management/public/components/data_source_selector/data_source_selector.tsx

-    const dataSources = this.props.dataSourceFilter
-      ? this.state.allDataSources.filter((ds) => this.props.dataSourceFilter!(ds))
-      : this.state.allDataSources;
+    const dataSources = getFilterDataSources(


instead of having 2 separate functions and have one of them called twice, would it be better to have one function which takes the filter function and does the filtering logic and store the default data source in the state here, so that we just need to call the function once here?

BionIT · 2024-04-02T04:05:41Z

...ugins/data_source_management/public/components/data_source_selector/data_source_selector.tsx

@@ -140,6 +159,16 @@ export class DataSourceSelector extends React.Component<
        isDisabled={this.props.disabled}
        fullWidth={this.props.fullWidth || false}
        data-test-subj={'dataSourceSelectorComboBox'}
+        renderOption={(option) => (
+          <EuiFlexGroup alignItems="center">
+            <EuiFlexItem grow={1}>{option.label}</EuiFlexItem>


can we add a test?

BionIT · 2024-04-02T04:11:23Z

src/plugins/data_source_management/public/components/utils.ts

+  } else if (
+    uiSettings &&
+    uiSettings?.get('defaultDataSource', null) !== null &&
+    dataSources.find((dataSource) => dataSource.id === uiSettings?.get('defaultDataSource'))


...ugins/data_source_management/public/components/data_source_selector/data_source_selector.tsx

src/plugins/data_source_management/public/components/utils.ts

BionIT · 2024-04-03T06:31:23Z

src/plugins/dev_tools/public/application.tsx

 }: DevToolsWrapperProps) {
  const mountedTool = useRef<MountedDevToolDescriptor | null>(null);
+  const [isLoading, setIsLoading] = React.useState(false);


should isLoading be true initially, then set to false once it is loaded?

…t to customer Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

…ce_selector/data_source_selector.tsx Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

…source shows as first choice (#6293) * edit datasource selector to show dafault datasource and return default to customer Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> * Update src/plugins/data_source_management/public/components/data_source_selector/data_source_selector.tsx Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> * Apply suggestions from code review Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> * fix syntax Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> --------- Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> Co-authored-by: Lu Yu <[email protected]> (cherry picked from commit 557fbcf) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

…source shows as first choice (#6293) (#6339) * edit datasource selector to show dafault datasource and return default to customer Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> * Update src/plugins/data_source_management/public/components/data_source_selector/data_source_selector.tsx Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> * Apply suggestions from code review Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> * fix syntax Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> --------- Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> Co-authored-by: Lu Yu <[email protected]> (cherry picked from commit 557fbcf) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

zhyuanqi requested review from ananzh, kavilla, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, BSFishy, curq, bandinib-amzn, SuZhou-Joe, ruanyl, BionIT and xinruiba as code owners March 29, 2024 08:14

github-actions bot added the repeat-contributor label Mar 29, 2024

zhyuanqi force-pushed the selector branch 3 times, most recently from 9a8f88b to 01073e8 Compare April 1, 2024 04:01

ZilongX reviewed Apr 1, 2024

View reviewed changes

Flyingliuhub reviewed Apr 1, 2024

View reviewed changes

xinruiba reviewed Apr 1, 2024

View reviewed changes

.../data_source_management/public/components/data_source_selector/data_source_selector.test.tsx Show resolved Hide resolved

xinruiba reviewed Apr 1, 2024

View reviewed changes

zhongnansu reviewed Apr 1, 2024

View reviewed changes

BionIT reviewed Apr 2, 2024

View reviewed changes

BionIT mentioned this pull request Apr 3, 2024

[MDS] TSVB Support #6298

Merged

7 tasks

zhyuanqi force-pushed the selector branch 2 times, most recently from 9073369 to 3bc74ae Compare April 3, 2024 04:32

BionIT reviewed Apr 3, 2024

View reviewed changes

zhyuanqi and others added 3 commits April 3, 2024 17:34

edit datasource selector to show dafault datasource and return defaul…

fa73989

…t to customer Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

Update src/plugins/data_source_management/public/components/data_sour…

7ea17fd

…ce_selector/data_source_selector.tsx Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

Apply suggestions from code review

2746d0f

Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

zhyuanqi force-pushed the selector branch 2 times, most recently from d26ef5f to 3bf9adf Compare April 3, 2024 18:05

BionIT previously approved these changes Apr 3, 2024

View reviewed changes

BionIT mentioned this pull request Apr 3, 2024

[Multiple Datasource] With default data source rendered in the data source component, we need to let consumers know which default data source id is used #6276

Closed

2 tasks

BionIT changed the title ~~edit datasource selector to have default behavior~~ [Multiple Datasource] Enhanced data source selector with default datasource shows as first choice Apr 3, 2024

BionIT added multiple datasource multiple datasource project v2.14.0 backport 2.x labels Apr 3, 2024

fix syntax

b5be1f0

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

zhyuanqi dismissed BionIT’s stale review via b5be1f0 April 3, 2024 22:12

zhyuanqi force-pushed the selector branch from 8ab83fd to b5be1f0 Compare April 3, 2024 22:12

Merge branch 'main' into selector

bace7c3

zhongnansu approved these changes Apr 3, 2024

View reviewed changes

ZilongX self-requested a review April 3, 2024 23:36

ZilongX approved these changes Apr 3, 2024

View reviewed changes

ZilongX merged commit 557fbcf into opensearch-project:main Apr 3, 2024
69 checks passed

opensearch-trigger-bot bot mentioned this pull request Apr 3, 2024

[Backport 2.x] [Multiple Datasource] Enhanced data source selector with default datasource shows as first choice #6339

Merged

BionIT mentioned this pull request Apr 22, 2024

[Meta][UX][Multiple Datasource] Improvement to data source components in the navigation bar #6216

Closed

36 tasks

	- [Multiple Datasource] Add show default datasource for selector picker ([#6293](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6293/files))
	- [Multiple Datasource] Add show default datasource for selector picker ([#6293](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6293))

	export function getFilterDataSources(
	export function getFilteredDataSources(

[Multiple Datasource] Enhanced data source selector with default datasource shows as first choice #6293

[Multiple Datasource] Enhanced data source selector with default datasource shows as first choice #6293

Conversation

zhyuanqi commented Mar 29, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Check List

codecov bot commented Mar 29, 2024 • edited Loading

Codecov Report

ZilongX commented Apr 1, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flyingliuhub Apr 1, 2024 • edited Loading

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

Choose a reason for hiding this comment

xinruiba Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

zhyuanqi Apr 2, 2024 • edited Loading

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

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

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

Choose a reason for hiding this comment

zhyuanqi commented Mar 29, 2024 •

edited

Loading

codecov bot commented Mar 29, 2024 •

edited

Loading

Flyingliuhub Apr 1, 2024 •

edited

Loading

xinruiba Apr 1, 2024 •

edited

Loading

zhyuanqi Apr 2, 2024 •

edited

Loading