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

[Multiple DataSource] DataSourceSelectable support to render label by getting it from dataSourceOptions #6358

Merged
merged 18 commits into from
Apr 9, 2024

Conversation

yujin-emma
Copy link
Contributor

@yujin-emma yujin-emma commented Apr 5, 2024

Description

Today the datasource menu does not populate the label of the datasource based on the datasourceId. This can be confusing to users, who may not know what human-readable datasource they are looking at.

This PR is going to resolve: when only given the dataSourceId to DataSourceSelectable, the component can get the label from existing dataSourceIdToLabel map

Issues Resolved

Screenshot

Testing the changes

test cases:

  1. this is invalid case, when there is no activeOption pass in and hide local cluster, there should be a toast error

activeOption={[{id: ''}]}
hideLocalCluster={true} // need to set in the opensearc_dashboards.yml

empty-activeOption-hide-lc-warning
  1. there are ds options, even not pass in activeOptions, display the default options

     hideLocalCluster={true}
    
dsOpts-hidelc-display-default

please also refer to case 9

  1. do not need to take care empty array, since we do not have placeholder for DataSourceSelectable

activeOption={[]}
hideLocalCluster={true} //does not matter
// not filter out default

we do not need this case for DataSourceSelectable

  1. show toast, when pass invalid dataSourceId in activeOption

selectedOption={[{id: 'invalid'}]}
hideLocalCluster={false}

invalid-activeOption-toast
  1. show local cluster when pass empty string for activeOption and not hide local cluster

selectedOption={[{id: ''}]}
hideLocalCluster={false}

not-hide-lc-activeOption-empty-string-show-lc

  1. filter out default, display local cluster
 dataSourceFilter={(ds) => ds.attributes.title !== 'test1'}
        hideLocalCluster={false}

filter-default-show-lc

  1. check when all data sources are deleted, uisetting throw error and no any datasource

hideLocalCluster={true}

Uploading nodsOpts-hide-lc-toast.gif…

  1. when there is no activeOption pass in, and there is default dataSourceOption, show default dataSourceOption and show checked status on the memu

no-active-option-show-default

  1. when there is activeOption pass in, this is the valid dataSourceOption, display it with the label it fetch from dataSourceOptions, and show checked status in the menu list
    valid-active-option-show-itself

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

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
@yujin-emma yujin-emma changed the title get label from dataSourceOptions [Multiple DataSource] DataSourceSelectable support to render label by getting it from dataSourceOptions Apr 5, 2024
@yujin-emma yujin-emma changed the title [Multiple DataSource] DataSourceSelectable support to render label by getting it from dataSourceOptions [No need to review now Multiple DataSource] DataSourceSelectable support to render label by getting it from dataSourceOptions Apr 5, 2024
@yujin-emma yujin-emma changed the title [No need to review now Multiple DataSource] DataSourceSelectable support to render label by getting it from dataSourceOptions [No need to review now - Multiple DataSource] DataSourceSelectable support to render label by getting it from dataSourceOptions Apr 5, 2024
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

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

Project coverage is 67.56%. Comparing base (a0eaf84) to head (0c51068).
Report is 1 commits behind head on main.

❗ Current head 0c51068 differs from pull request most recent head 12f4202. Consider uploading reports for the commit 12f4202 to get more accurate results

Files Patch % Lines
.../data_source_selectable/data_source_selectable.tsx 83.33% 3 Missing and 1 partial ⚠️
.../data_source_management/public/components/utils.ts 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6358   +/-   ##
=======================================
  Coverage   67.56%   67.56%           
=======================================
  Files        3379     3379           
  Lines       65894    65897    +3     
  Branches    10660    10655    -5     
=======================================
+ Hits        44522    44526    +4     
- Misses      18774    18776    +2     
+ Partials     2598     2595    -3     
Flag Coverage Δ
Linux_1 32.63% <ø> (ø)
Linux_2 55.62% <ø> (ø)
Linux_3 45.00% <84.84%> (-0.01%) ⬇️
Linux_4 34.98% <0.00%> (-0.01%) ⬇️
Windows_1 32.65% <ø> (ø)
Windows_2 55.58% <ø> (ø)
Windows_3 45.01% <84.84%> (+<0.01%) ⬆️
Windows_4 34.98% <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.

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
@yujin-emma yujin-emma changed the title [No need to review now - Multiple DataSource] DataSourceSelectable support to render label by getting it from dataSourceOptions [Multiple DataSource] DataSourceSelectable support to render label by getting it from dataSourceOptions Apr 7, 2024
@yujin-emma yujin-emma marked this pull request as ready for review April 7, 2024 18:54
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
yujin-emma and others added 7 commits April 8, 2024 10:05
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
@BionIT
Copy link
Collaborator

BionIT commented Apr 8, 2024

  1. show default when pass in empty array for activeOption
    activeOption={[]}
    hideLocalCluster={true} //does not matter
    // not filter out default

Can we double check the test case 3? Based on the implementation, it should not be the case

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
@yujin-emma
Copy link
Contributor Author

yujin-emma commented Apr 8, 2024

synced with @BionIT offline, we do not need to take care this case in DataSourceSelectable, since we do not have a placeholder

if (!hideLocalCluster) {
// console.log("hideLocalCluster", hideLocalCluster)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, is this for debugging? can we remove?

},
];
}
// console.log("defaultDataSourceAfterCheck", defaultDataSourceAfterCheck)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, is this for debugging? can we remove?

@@ -43,7 +43,7 @@ interface DataSourceSelectableState {
dataSourceOptions: SelectedDataSourceOption[];
isPopoverOpen: boolean;
selectedOption?: SelectedDataSourceOption[];
defaultDataSource: string | null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we need this union type since we set it like const defaultDataSource = this.props.uiSettings?.get('defaultDataSource', null) ?? null;

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
yujin-emma and others added 4 commits April 9, 2024 00:01
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
@ZilongX
Copy link
Collaborator

ZilongX commented Apr 9, 2024

LGTM, rerunning skipped ciGroup5 before merging into main

@ZilongX ZilongX merged commit 85df662 into opensearch-project:main Apr 9, 2024
67 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 9, 2024
… getting it from dataSourceOptions (#6358)

* get label from dataSourceOptions

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* update dataSourceOptions

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* update changelog

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* fix failed test

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* address comments and fix test

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* update selected option checked status and udpate snapshot

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* update selectable test

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* revert example code

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* revern config file

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* push the utils

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* udpate snapshot

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* remove console log

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* udate default data source

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* remove unnessary check for empty input

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* fix failed test

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* fix failed test

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

---------

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
(cherry picked from commit 85df662)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
BionIT pushed a commit that referenced this pull request Apr 9, 2024
… getting it from dataSourceOptions (#6358) (#6382)

* get label from dataSourceOptions

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* update dataSourceOptions

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* update changelog

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* fix failed test

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* address comments and fix test

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* update selected option checked status and udpate snapshot

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* update selectable test

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* revert example code

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* revern config file

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* push the utils

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* udpate snapshot

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* remove console log

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* udate default data source

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* remove unnessary check for empty input

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* fix failed test

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

* fix failed test

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

---------

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
(cherry picked from commit 85df662)
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants