Add default icon for selectable component and make sure the default datasource shows automatically #6327

zhyuanqi · 2024-04-03T07:16:45Z

Description

Modify seletable picker in datasource management to show the default datasource

Issues Resolved

Screenshot

Screen.Recording.2024-04-03.at.6.36.10.PM.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-04-03T07:32:12Z

Codecov Report

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

Project coverage is 67.51%. Comparing base (b92387a) to head (36307a2).

Files	Patch %	Lines
.../data_source_selectable/data_source_selectable.tsx	80.76%	3 Missing and 2 partials ⚠️
...rc/plugins/data_source_management/public/plugin.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6327   +/-   ##
=======================================
  Coverage   67.50%   67.51%           
=======================================
  Files        3376     3376           
  Lines       65830    65837    +7     
  Branches    10648    10649    +1     
=======================================
+ Hits        44441    44447    +6     
- Misses      18803    18805    +2     
+ Partials     2586     2585    -1

Flag	Coverage Δ
Linux_1	`32.60% <ø> (ø)`
Linux_2	`55.60% <ø> (ø)`
Linux_3	`44.89% <78.57%> (+0.02%)`	⬆️
Linux_4	`34.99% <0.00%> (-0.01%)`	⬇️
Windows_1	`32.62% <ø> (ø)`
Windows_2	`55.57% <ø> (ø)`
Windows_3	`44.90% <78.57%> (+<0.01%)`	⬆️
Windows_4	`34.99% <0.00%> (-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.

ruanyl · 2024-04-03T08:06:50Z

config/opensearch_dashboards.yml

@@ -247,7 +247,7 @@
 # vis_builder.enabled: false

 # Set the value of this setting to true to enable multiple data source feature.
-#data_source.enabled: false
+data_source.enabled: true


Is this change by intention?

ruanyl · 2024-04-03T08:36:48Z

...s/data_source_management/public/components/data_source_selectable/data_source_selectable.tsx

+          if (selectedDataSource.length === 0) {
+            this.props.notifications.addWarning('No connected data source available.');
+          } else {
+            this.setState({


I think this.setState here will override dataSourceOptions set on line 113 because state update is async, this.state here is still the old one, thus dataSourceOptions been override.

Thanks for pointing this out! Let me chek on this.

Found this article on it as "React may batch multiple setState() calls into a single update for performance " in https://legacy.reactjs.org/docs/state-and-lifecycle.html. There is possibility that the datasourceOptions can be override. I am looking into this see how to fix this issue

ruanyl · 2024-04-03T08:38:14Z

...s/data_source_management/public/components/data_source_selectable/data_source_selectable.tsx

+              ...this.state,
+              selectedOption: selectedDataSource,
+            });
+            this.props.onSelectedDataSource(selectedDataSource);


Should it be onSelectedDataSources?

Suggested change

this.props.onSelectedDataSource(selectedDataSource);

this.props.onSelectedDataSources(selectedDataSource);

ruanyl · 2024-04-03T08:46:59Z

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

@@ -54,6 +55,44 @@ export async function getDataSourcesWithFields(
  return response?.savedObjects;
 }

+export function getDefaultDataSource(


Would be nice to add some test cases for this function :)

Maybe leave a comment to describe the default data source selection logic?

Sure. It is a draft CR. Would add a comment and test case

BionIT · 2024-04-03T16:11:57Z

src/plugins/data_source_management/public/components/data_source_menu/data_source_menu.tsx

-  function renderDataSourceSelectable(config: DataSourceSelectableConfig): ReactElement | null {
+  function renderDataSourceSelectable(
+    config: DataSourceSelectableConfig,
+    ui: IUiSettingsClient


looks like we already extract the uiSettings props above, and we don't need to add it here

Suggested change

ui: IUiSettingsClient

BionIT · 2024-04-03T16:12:13Z

src/plugins/data_source_management/public/components/data_source_menu/data_source_menu.tsx

@@ -73,6 +77,7 @@ export function DataSourceMenu<T>(props: DataSourceMenuProps<T>): ReactElement |
        dataSourceFilter={dataSourceFilter}
        hideLocalCluster={hideLocalCluster || false}
        fullWidth={fullWidth}
+        uiSettings={ui}


Suggested change

uiSettings={ui}

uiSettings={uiSettings}

BionIT · 2024-04-03T16:12:24Z

src/plugins/data_source_management/public/components/data_source_menu/data_source_menu.tsx

-        return renderDataSourceSelectable(componentConfig as DataSourceSelectableConfig);
+        return renderDataSourceSelectable(
+          componentConfig as DataSourceSelectableConfig,
+          uiSettings


Suggested change

uiSettings

ashwin-pc · 2024-04-03T17:47:31Z

@zhyuanqi can you add details to the PR Description for the various sections?

zhyuanqi · 2024-04-03T22:26:32Z

@zhyuanqi can you add details to the PR Description for the various sections?

Hi This suppose to be a draft PR, forgot to mark it as draft. Will do it next time. THanks

zhyuanqi · 2024-04-04T01:42:09Z

...s/data_source_management/public/components/data_source_selectable/data_source_selectable.tsx

 }

 interface SelectedDataSourceOption extends DataSourceOption {
-  checked?: boolean;
+  checked?: string;


Checked should be a string as "on" or "off"

zhyuanqi · 2024-04-04T01:44:32Z

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

@@ -16,7 +16,7 @@ import {
  noAuthCredentialAuthMethod,
 } from '../types';
 import { AuthenticationMethodRegistry } from '../auth_registry';
-import { DataSourceOption } from './data_source_selector/data_source_selector';
+import { DataSourceOption } from './data_source_menu/types';


Change to use the DataSourceOption from /data_source_menu/types as this one only contains label and id. Previous one from selector contains an extra check which i don't use it here.

seraphjiang · 2024-04-04T03:10:49Z

CHANGELOG.md

@@ -76,6 +76,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - [Multiple Datasource] Add multi data source support to sample vega visualizations ([#6218](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6218))
 - [Multiple Datasource] Fetch data source title for DataSourceView when only id is provided ([#6315](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6315)
 - [Workspace] Add permission control logic ([#6052](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6052))
+- [Multiple Datasource] Add default icon for selectable component and make sure the default datasource shows automatically ([#6327](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6327))


nit: The description here appears to be more descriptive than the PR title.

Will modify the PR title and commit message

seraphjiang · 2024-04-04T03:27:28Z

...s/data_source_management/public/components/data_source_selectable/data_source_selectable.tsx

@@ -77,35 +83,58 @@ export class DataSourceSelectable extends React.Component<

  async componentDidMount() {
    this._isMounted = true;
+    let filteredDataSources: Array<SavedObject<DataSourceAttributes>> = [];
+    let dataSourceOptions: DataSourceOption[] = [];
    getDataSourcesWithFields(this.props.savedObjectsClient, ['id', 'title', 'auth.type'])


could we covert Promise.then to await syntax for better readability and maintainability

Suggested change

getDataSourcesWithFields(this.props.savedObjectsClient, ['id', 'title', 'auth.type'])

const ds = await getDataSourcesWithFields(this.props.savedObjectsClient, ['id', 'title', 'auth.type']);

BionIT · 2024-04-04T05:51:55Z

There is one snapshot in the test that needs to be updated

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

…atasource shows automatically (#6327) Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> (cherry picked from commit 726fb0e) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

…atasource shows automatically (#6327) (#6346) Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> (cherry picked from commit 726fb0e) 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 April 3, 2024 07:16

github-actions bot added the repeat-contributor label Apr 3, 2024

ruanyl reviewed Apr 3, 2024

View reviewed changes

BionIT reviewed Apr 3, 2024

View reviewed changes

zhyuanqi force-pushed the selectable branch from f585520 to 2c2d8d0 Compare April 3, 2024 18:35

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

zhyuanqi marked this pull request as draft April 3, 2024 21:50

zhyuanqi force-pushed the selectable branch from 2c2d8d0 to b504d71 Compare April 4, 2024 01:27

zhyuanqi marked this pull request as ready for review April 4, 2024 01:28

zhyuanqi changed the title ~~Add selectable~~ Modify seletable picker in datasource management to show the default datasource Apr 4, 2024

zhyuanqi force-pushed the selectable branch 3 times, most recently from 6dce230 to 36a24b8 Compare April 4, 2024 01:41

zhyuanqi commented Apr 4, 2024

View reviewed changes

BionIT previously approved these changes Apr 4, 2024

View reviewed changes

zhyuanqi dismissed BionIT’s stale review via 1d7e948 April 4, 2024 01:57

zhyuanqi force-pushed the selectable branch from 36a24b8 to 1d7e948 Compare April 4, 2024 01:57

seraphjiang reviewed Apr 4, 2024

View reviewed changes

zhyuanqi changed the title ~~Modify seletable picker in datasource management to show the default datasource~~ Add default icon for selectable component and make sure the default datasource shows automatically Apr 4, 2024

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

Add default icon for selectable component and make sure the default d…

d334720

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

zhyuanqi force-pushed the selectable branch from 1d7e948 to d334720 Compare April 4, 2024 06:32

BionIT approved these changes Apr 4, 2024

View reviewed changes

seraphjiang approved these changes Apr 4, 2024

View reviewed changes

Merge branch 'main' into selectable

36307a2

zhongnansu approved these changes Apr 4, 2024

View reviewed changes

zhongnansu merged commit 726fb0e into opensearch-project:main Apr 4, 2024
69 checks passed

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

[Backport 2.x] Add default icon for selectable component and make sure the default datasource shows automatically #6346

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default icon for selectable component and make sure the default datasource shows automatically #6327

Add default icon for selectable component and make sure the default datasource shows automatically #6327

zhyuanqi commented Apr 3, 2024 •

edited

Loading

codecov bot commented Apr 3, 2024 •

edited

Loading

ruanyl Apr 3, 2024

ruanyl Apr 3, 2024

zhyuanqi Apr 3, 2024

zhyuanqi Apr 3, 2024 •

edited

Loading

ruanyl Apr 3, 2024

ruanyl Apr 3, 2024

ruanyl Apr 3, 2024

zhyuanqi Apr 3, 2024

BionIT Apr 3, 2024

BionIT Apr 3, 2024

BionIT Apr 3, 2024

ashwin-pc commented Apr 3, 2024

zhyuanqi commented Apr 3, 2024

zhyuanqi Apr 4, 2024

zhyuanqi Apr 4, 2024

seraphjiang Apr 4, 2024

zhyuanqi Apr 4, 2024

seraphjiang Apr 4, 2024

seraphjiang Apr 4, 2024

BionIT commented Apr 4, 2024

	this.props.onSelectedDataSource(selectedDataSource);
	this.props.onSelectedDataSources(selectedDataSource);

	getDataSourcesWithFields(this.props.savedObjectsClient, ['id', 'title', 'auth.type'])
	const ds = await getDataSourcesWithFields(this.props.savedObjectsClient, ['id', 'title', 'auth.type']);

Add default icon for selectable component and make sure the default datasource shows automatically #6327

Add default icon for selectable component and make sure the default datasource shows automatically #6327

Conversation

zhyuanqi commented Apr 3, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Check List

codecov bot commented Apr 3, 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

zhyuanqi Apr 3, 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

ashwin-pc commented Apr 3, 2024

zhyuanqi commented Apr 3, 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

BionIT commented Apr 4, 2024

zhyuanqi commented Apr 3, 2024 •

edited

Loading

codecov bot commented Apr 3, 2024 •

edited

Loading

zhyuanqi Apr 3, 2024 •

edited

Loading