Refactor read-only component to cover more edge cases #6416

zhyuanqi · 2024-04-11T22:56:20Z

Description

Refactor read-only component to cover more edge cases

Issues Resolved

#6393

Screenshot

Testing the changes

activeOption={[{id: ''}]} with hideLocalCluster is set to true in yaml
Toast: Data source is not available

activeOption={[{id: 'invalid'}]}
Toast: Failed to fetch data source

activeOption={[{id: ''}]} with hideLocalCluster is set to false in yaml
Show local cluster

activeOption={[{id: 'valid id'}]}
show selected cluster

activeOption={[{id: 'valid id', iabel:"show label"}]}
show selected cluster with label name

When valid id pass in, but it has been filtered out
activeOption: [{ id: '6304d1d0-f826-11ee-ab60-439c91fde80b', label:'show label' }],
dataSourceFilter: (ds) => {return ds.id != '6304d1d0-f826-11ee-ab60-439c91fde80b'},
toast: datasource is not available

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

src/plugins/data_source_management/public/components/data_source_menu/types.ts

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

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

BionIT · 2024-04-11T23:51:32Z

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

@@ -130,6 +142,9 @@ export async function getDataSourceById(
  savedObjectsClient: SavedObjectsClientContract
 ) {
  return savedObjectsClient.get('data-source', id).then((response) => {
+    if (!response || response.error) {
+      throw new Error('Unable to find data source');


does the get data source API throw exception when failed to get data source or do we need to handle it this way?

It does not throw exception if we don't handle it. i have done test on this. So if we put an invalid id such as invalid id, it would return back something as below:

Object { client: {…}, attributes: {}, _version: undefined, id: "invalidID", type: "data-source", migrationVersion: undefined, error: {…}, references: [], updated_at: undefined }

If that's the case, then we don't need the try catch block, right?

Sure . would do

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

BionIT · 2024-04-11T23:58:20Z

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

+    }
+
+    try {
+      const selectedDataSource = await getDataSourceById(optionId, this.props.savedObjectsClient!);


we only do this when label is not passed in

I need to use this DatasourceById to find if it is an invalid ID. If the ID is invalid, getDataSourceByID would fail and throw exception. This would be caught and return as a warning message

We don't need to check each time, when label is passed in, we will accept it, if it is not passed it, we will look for it. This component is used internally by developers, no need for over defensive programming

src/plugins/data_source_management/public/components/data_source_view/data_source_view.test.tsx

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

zhongnansu · 2024-04-12T05:01:13Z

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

+    });
+
+    test('should add warning with error message when error is provided', () => {
+      const error = 'Failed to fetch data';


Suggested change

const error = 'Failed to fetch data';

const errorMessage= 'Failed to fetch data';

or const error = new Error("Filed to fetch data")?

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

BionIT · 2024-04-12T20:19:42Z

Can we add the screenshots? Also test failed, could we address https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/8667700544/job/23771246546?pr=6416

BionIT · 2024-04-13T00:58:29Z

src/plugins/data_source_management/public/components/data_source_menu/types.ts

@@ -44,9 +44,11 @@ export const DataSourceComponentType = {
 export type DataSourceComponentType = typeof DataSourceComponentType[keyof typeof DataSourceComponentType];

 export interface DataSourceViewConfig extends DataSourceBaseConfig {
+  onSelectedDataSources?: (dataSources: DataSourceOption[]) => void;


Let's move optional fields below required

BionIT · 2024-04-13T00:59:41Z

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


 interface DataSourceViewProps {
  fullWidth: boolean;
  selectedOption: DataSourceOption[];
  savedObjectsClient?: SavedObjectsClientContract;
  notifications?: ToastsStart;
+  uiSettings?: IUiSettingsClient;
+  hideLocalCluster: boolean;


Let's move optional fields below required

BionIT · 2024-04-13T01:03:26Z

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

+    }
+
+    try {
+      const selectedDataSource = await getDataSourceById(optionId, this.props.savedObjectsClient!);


We don't need to check each time, when label is passed in, we will accept it, if it is not passed it, we will look for it. This component is used internally by developers, no need for over defensive programming

BionIT · 2024-04-13T01:06:53Z

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

@@ -130,6 +142,9 @@ export async function getDataSourceById(
  savedObjectsClient: SavedObjectsClientContract
 ) {
  return savedObjectsClient.get('data-source', id).then((response) => {
+    if (!response || response.error) {
+      throw new Error('Unable to find data source');


If that's the case, then we don't need the try catch block, right?

BionIT · 2024-04-13T01:07:39Z

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

+
+export function handleNoAvailableDataSourceError(notifications: ToastsStart) {
+  notifications.addWarning(
+    i18n.translate('dataSource.fetchDataSourceError', {


Suggested change

i18n.translate('dataSource.fetchDataSourceError', {

i18n.translate('dataSource.noAvailableDataSourceError', {

codecov · 2024-04-15T18:03:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 32.72%. Comparing base (e785fd3) to head (018c6d5).
Report is 1 commits behind head on main.

❗ Current head 018c6d5 differs from pull request most recent head f08ebe4. Consider uploading reports for the commit f08ebe4 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #6416       +/-   ##
===========================================
- Coverage   49.55%   32.72%   -16.84%     
===========================================
  Files        2670     2252      -418     
  Lines       54290    45574     -8716     
  Branches     8878     7150     -1728     
===========================================
- Hits        26906    14913    -11993     
- Misses      25720    29962     +4242     
+ Partials     1664      699      -965

Flag	Coverage Δ
Linux_1	`32.72% <ø> (?)`
Windows_1	`?`
Windows_3	`?`

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: Yuanqi(Ella) Zhu <zhyuanqi@amazon.com>

* Refactor read-only component to cover more edge case Signed-off-by: Yuanqi(Ella) Zhu <zhyuanqi@amazon.com> * fix test Signed-off-by: Yuanqi(Ella) Zhu <zhyuanqi@amazon.com> * fix snapshot Signed-off-by: Yuanqi(Ella) Zhu <zhyuanqi@amazon.com> --------- Signed-off-by: Yuanqi(Ella) Zhu <zhyuanqi@amazon.com> (cherry picked from commit b9acf57) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

* Refactor read-only component to cover more edge case Signed-off-by: Yuanqi(Ella) Zhu <zhyuanqi@amazon.com> * fix test Signed-off-by: Yuanqi(Ella) Zhu <zhyuanqi@amazon.com> * fix snapshot Signed-off-by: Yuanqi(Ella) Zhu <zhyuanqi@amazon.com> --------- Signed-off-by: Yuanqi(Ella) Zhu <zhyuanqi@amazon.com> (cherry picked from commit b9acf57) 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, curq, bandinib-amzn, SuZhou-Joe, ruanyl, BionIT and xinruiba as code owners April 11, 2024 22:56

github-actions bot added the valued-contributor label Apr 11, 2024

zhyuanqi force-pushed the readonly-logic branch from a84b784 to 5aa4e7d Compare April 11, 2024 23:02

BionIT reviewed Apr 11, 2024

View reviewed changes

zhongnansu added backport 2.x multiple datasource multiple datasource project v2.14.0 labels Apr 12, 2024

zhongnansu reviewed Apr 12, 2024

View reviewed changes

zhyuanqi force-pushed the readonly-logic branch 4 times, most recently from 83368ff to 57979c1 Compare April 12, 2024 22:07

BionIT reviewed Apr 13, 2024

View reviewed changes

zhyuanqi force-pushed the readonly-logic branch 2 times, most recently from fcf50dd to 688df3c Compare April 15, 2024 17:42

Refactor read-only component to cover more edge case

018c6d5

Signed-off-by: Yuanqi(Ella) Zhu <zhyuanqi@amazon.com>

zhyuanqi force-pushed the readonly-logic branch 2 times, most recently from 5a698ce to ae48c41 Compare April 15, 2024 19:14

fix test

b33d06f

Signed-off-by: Yuanqi(Ella) Zhu <zhyuanqi@amazon.com>

zhyuanqi force-pushed the readonly-logic branch from ae48c41 to b33d06f Compare April 15, 2024 19:16

zhongnansu previously approved these changes Apr 15, 2024

View reviewed changes

fix snapshot

3a3257c

Signed-off-by: Yuanqi(Ella) Zhu <zhyuanqi@amazon.com>

zhyuanqi dismissed zhongnansu’s stale review via 3a3257c April 15, 2024 22:23

zhyuanqi force-pushed the readonly-logic branch from 540c353 to 3a3257c Compare April 15, 2024 22:23

zhyuanqi added 2 commits April 15, 2024 15:23

Merge branch 'main' into readonly-logic

de2771d

Merge branch 'main' into readonly-logic

f08ebe4

BionIT approved these changes Apr 16, 2024

View reviewed changes

zhongnansu approved these changes Apr 16, 2024

View reviewed changes

zhongnansu merged commit b9acf57 into opensearch-project:main Apr 16, 2024
67 checks passed

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

[Backport 2.x] Refactor read-only component to cover more edge cases #6466

Merged

This was referenced Apr 22, 2024

[Meta] [2.14] Security, bug fix and enhancemanet for dashboards anywhere #5872

Closed

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor read-only component to cover more edge cases #6416

Refactor read-only component to cover more edge cases #6416

zhyuanqi commented Apr 11, 2024 •

edited

BionIT Apr 11, 2024

zhyuanqi Apr 12, 2024

BionIT Apr 13, 2024

zhyuanqi Apr 15, 2024 •

edited

BionIT Apr 11, 2024

zhyuanqi Apr 12, 2024 •

edited

BionIT Apr 13, 2024

zhyuanqi Apr 15, 2024

zhongnansu Apr 12, 2024

zhyuanqi Apr 12, 2024

BionIT commented Apr 12, 2024 •

edited

BionIT Apr 13, 2024

zhyuanqi Apr 15, 2024

BionIT Apr 13, 2024

zhyuanqi Apr 15, 2024

BionIT Apr 13, 2024

BionIT Apr 13, 2024

BionIT Apr 13, 2024

codecov bot commented Apr 15, 2024 •

edited

	const error = 'Failed to fetch data';
	const errorMessage= 'Failed to fetch data';

	i18n.translate('dataSource.fetchDataSourceError', {
	i18n.translate('dataSource.noAvailableDataSourceError', {

Refactor read-only component to cover more edge cases #6416

Refactor read-only component to cover more edge cases #6416

Conversation

zhyuanqi commented Apr 11, 2024 • edited

Description

Issues Resolved

Screenshot

Testing the changes

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhyuanqi Apr 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhyuanqi Apr 12, 2024 • edited

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 12, 2024 • edited

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

codecov bot commented Apr 15, 2024 • edited

Codecov Report

zhyuanqi commented Apr 11, 2024 •

edited

zhyuanqi Apr 15, 2024 •

edited

zhyuanqi Apr 12, 2024 •

edited

BionIT commented Apr 12, 2024 •

edited

codecov bot commented Apr 15, 2024 •

edited