-
Notifications
You must be signed in to change notification settings - Fork 917
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]Add datasource query for DataSourceView component to add label when only pass id #6315
Conversation
…nly pass id Signed-off-by: yujin-emma <[email protected]>
Signed-off-by: yujin-emma <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6315 +/- ##
==========================================
- Coverage 67.51% 67.49% -0.03%
==========================================
Files 3376 3376
Lines 65783 65798 +15
Branches 10637 10639 +2
==========================================
- Hits 44415 44408 -7
- Misses 18785 18804 +19
- Partials 2583 2586 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Hi @yujin-emma
|
src/plugins/data_source_management/public/components/data_source_view/data_source_view.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data_source_management/public/components/data_source_view/data_source_view.tsx
Outdated
Show resolved
Hide resolved
componentWillUnmount() { | ||
this._isMounted = false; | ||
} | ||
async componentDidMount() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious: why do we need componentDidMount and componentWillUnmount for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to check the props.selectedOption.label exist, if not, need to call getDataSourceById to get the title back and set this value, since we only have one option, we only need to call at most once before rendering, this is the reason why put a componentDidMount here. As for this._isMounted = false;
in componentWillUnmount
, this refer to code here: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data_source_management/public/components/data_source_selector/data_source_selector.tsx#L65
if (this.state.selectedOption) { | ||
items = this.state.selectedOption.map((option) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain me this - why are we using this.state for selectedOption now instead of this.props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we pass the selectedOption from props, there can be some empty field, e.g. label
is optional, we will use state to reconstruct this and render with the state one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, props should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not only copy the value from props, but also add missing label in the state, therefore, only state.selectedOption contains sufficient information to render, props.selectedOption can miss the label for some plugins
Signed-off-by: yujin-emma <[email protected]>
test coverage check failed, can you add more tests? |
CHANGELOG.md
Outdated
@@ -83,6 +83,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) | |||
- [BUG][MD]Fix schema for test connection to separate validation based on auth type ([#5997](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5997)) | |||
- [Discover] Enable 'Back to Top' Feature in Discover for scrolling to top ([#6008](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6008)) | |||
- [Discover] Fix lazy loading of the legacy table from getting stuck ([#6041](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6041)) | |||
- [MDS] Add data source query to fetch missing lable for DataSourceView ([#6315](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6315)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a bug or a new feature/enhancement?
return ( | ||
<DataSourceView | ||
selectedOption={activeOption && activeOption.length > 0 ? activeOption : undefined} | ||
savedObjectsClient={savedObjects!} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
saved object client is not required
return ( | ||
<DataSourceView | ||
selectedOption={activeOption && activeOption.length > 0 ? activeOption : undefined} | ||
savedObjectsClient={savedObjects!} | ||
notifications={notifications!.toasts} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notification service is not required
src/plugins/data_source_management/public/components/data_source_view/data_source_view.test.tsx
Show resolved
Hide resolved
|
||
interface DataSourceViewProps { | ||
savedObjectsClient: SavedObjectsClientContract; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
savedobjects and notifications should be optional config
src/plugins/data_source_management/public/components/data_source_view/data_source_view.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data_source_management/public/components/data_source_view/data_source_view.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: yujin-emma <[email protected]>
Signed-off-by: yujin-emma <[email protected]>
Signed-off-by: yujin-emma <[email protected]>
Signed-off-by: yujin-emma <[email protected]>
Signed-off-by: yujin-emma <[email protected]>
Signed-off-by: yujin-emma <[email protected]>
Signed-off-by: yujin-emma <[email protected]>
src/plugins/data_source_management/public/components/data_source_menu/data_source_menu.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: yujin-emma <[email protected]>
src/plugins/data_source_management/public/components/data_source_menu/data_source_menu.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yu Jin <[email protected]>
…ce_menu/data_source_menu.tsx Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yu Jin <[email protected]>
Signed-off-by: Yu Jin <[email protected]>
LGTM, thanks |
…t to add label when only pass id (#6315) * Add datasource query for DataSourceView component to add label when only pass id Signed-off-by: yujin-emma <[email protected]> * update changelog Signed-off-by: yujin-emma <[email protected]> * fix the failed test Signed-off-by: yujin-emma <[email protected]> * fix comment Signed-off-by: yujin-emma <[email protected]> * extract interface Signed-off-by: yujin-emma <[email protected]> * revert yml Signed-off-by: yujin-emma <[email protected]> * add more test Signed-off-by: yujin-emma <[email protected]> * update snapshot Signed-off-by: yujin-emma <[email protected]> * update snpshot Signed-off-by: yujin-emma <[email protected]> * update change log Signed-off-by: yujin-emma <[email protected]> * update change log and address other comments Signed-off-by: yujin-emma <[email protected]> * Update CHANGELOG.md Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yu Jin <[email protected]> * Update src/plugins/data_source_management/public/components/data_source_menu/data_source_menu.tsx Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yu Jin <[email protected]> --------- Signed-off-by: yujin-emma <[email protected]> Signed-off-by: Yu Jin <[email protected]> Co-authored-by: Lu Yu <[email protected]> (cherry picked from commit cf43af3) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
…t to add label when only pass id (#6315) (#6332) * Add datasource query for DataSourceView component to add label when only pass id Signed-off-by: yujin-emma <[email protected]> * update changelog Signed-off-by: yujin-emma <[email protected]> * fix the failed test Signed-off-by: yujin-emma <[email protected]> * fix comment Signed-off-by: yujin-emma <[email protected]> * extract interface Signed-off-by: yujin-emma <[email protected]> * revert yml Signed-off-by: yujin-emma <[email protected]> * add more test Signed-off-by: yujin-emma <[email protected]> * update snapshot Signed-off-by: yujin-emma <[email protected]> * update snpshot Signed-off-by: yujin-emma <[email protected]> * update change log Signed-off-by: yujin-emma <[email protected]> * update change log and address other comments Signed-off-by: yujin-emma <[email protected]> * Update CHANGELOG.md Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yu Jin <[email protected]> * Update src/plugins/data_source_management/public/components/data_source_menu/data_source_menu.tsx Co-authored-by: Lu Yu <[email protected]> Signed-off-by: Yu Jin <[email protected]> --------- Signed-off-by: yujin-emma <[email protected]> Signed-off-by: Yu Jin <[email protected]> Co-authored-by: Lu Yu <[email protected]> (cherry picked from commit cf43af3) 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>
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 fix the issue in
DataSourceView
component. Adding the getDataSourceById to get the complete data source saved object when plugin only pass idIssues Resolved
#6274
Screenshot
Testing the changes
When only pass id in the
DataSourceMenu
when pass invalid dataSourceId
When activeOption is empty, getDataSourceById throw out exception
Check List
yarn test:jest
yarn test:jest_integration