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]DataSourceView add switch to default data source button #6610

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

yujin-emma
Copy link
Contributor

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

Description

Add switch to default data source when pass in invalid data source id for DataSourceView
We expect to see
When pass in invalid data source id

default data source is available no default data source
hide local cluster show switch button and provide the switch to default data source functionality on both popover and toast no switch button display either on popover or toast
not hide local cluster show switch button and provide the switch to default data source functionality on both popover and toast no switch button display either on popover or toast

Issues Resolved

Screenshot

Testing the changes

Happy case:
when pass in valid id without label, the DataSourceView can display the correct label
happy-case

Happy case but the result got filter out, we treat it as invalid id error:

 activeOption: [{ id: '829c0ee0-065f-11ef-ad27-4377edb9ad8f' }],
          dataSourceFilter: (ds) => {
            return ds.id !== '829c0ee0-065f-11ef-ad27-4377edb9ad8f'
          },

happy-case-but-filterout

Overall Test Result:

When pass in invalid data source id

default data source is available no default data source
hide local cluster hide-lc-default-show-button Screenshot 2024-04-23 at 14 07 48Screenshot 2024-04-23 at 14 07 41
not hide local cluster both-show-button Screenshot 2024-04-23 at 14 07 41Screenshot 2024-04-23 at 14 07 48

test input:

<DataSourceMenu
        setMenuMountPoint={setActionMenu}
        componentType={'DataSourceView'}
        componentConfig={{
          notifications,
          savedObjects: savedObjects.client,
          fullWidth: false,
          activeOption: [{ id: 'invalid-id' }],
          dataSourceFilter: (ds) => {
            return true;
          },
          onSelectedDataSources: (ds) => {
            setSelectedDataSources(ds);
          },
        }}
      />
    );

1. When hide local cluster, and there is no default data source

Screenshot 2024-04-23 at 14 07 48 Screenshot 2024-04-23 at 14 07 41

2. When hide local cluster, and there is default data source

hide-lc-default-show-button

3. Test mobile view

Screenshot 2024-04-23 at 11 43 21

4. Test iphone SE View

Screenshot 2024-04-23 at 12 00 46

5. When not hide local cluster:

5.1. When no default data source - both the popover and toast should not display the button to switch the default data source
Screenshot 2024-04-23 at 14 07 41 Screenshot 2024-04-23 at 14 07 48
5.2. When there is default data source - both the popover and toast should display the switch button to switch to the default data source

both-show-button

6. If only pass in valid data source id, the DataSourceView will render the correct data source title, if click the header button, the popover will show up and only checked with the activeOption, there is no error toast, no switch to default button

Test input from examples:

<DataSourceMenu
        setMenuMountPoint={setActionMenu}
        componentType={'DataSourceView'}
        componentConfig={{
          notifications,
          savedObjects: savedObjects.client,
          fullWidth: false,
          activeOption: [{ id: '98652370-01b8-11ef-9df2-d1bcc041cdef' }], // this is basically the test2 data source id
          dataSourceFilter: (ds) => {
            return true;
          },
          onSelectedDataSources: (ds) => {
            setSelectedDataSources(ds);
          },
        }}
      />

Screenshot 2024-04-23 at 14 30 36

Changelog

  • skip

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

@github-actions github-actions bot added valued-contributor Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry labels Apr 23, 2024
Copy link

codecov bot commented Apr 23, 2024

Codecov Report

Attention: Patch coverage is 38.37209% with 53 lines in your changes missing coverage. Please review.

Project coverage is 67.37%. Comparing base (32fbe18) to head (060370f).
Report is 10 commits behind head on main.

Files Patch % Lines
...c/components/data_source_view/data_source_view.tsx 36.20% 36 Missing and 1 partial ⚠️
.../data_source_management/public/components/utils.ts 7.69% 12 Missing ⚠️
...onents/data_source_view/data_source_view_error.tsx 66.66% 3 Missing ⚠️
.../components/toast_button/switch_default_button.tsx 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6610      +/-   ##
==========================================
- Coverage   67.43%   67.37%   -0.07%     
==========================================
  Files        3444     3446       +2     
  Lines       67847    67906      +59     
  Branches    11035    11047      +12     
==========================================
- Hits        45755    45750       -5     
- Misses      19426    19491      +65     
+ Partials     2666     2665       -1     
Flag Coverage Δ
Linux_1 33.08% <ø> (ø)
Linux_2 55.12% <ø> (ø)
Linux_3 ?
Linux_4 34.78% <5.95%> (-0.04%) ⬇️
Windows_1 33.11% <ø> (-0.03%) ⬇️
Windows_2 55.09% <ø> (ø)
Windows_3 45.21% <38.37%> (-0.06%) ⬇️
Windows_4 34.78% <5.95%> (-0.04%) ⬇️

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
Copy link
Collaborator

ZilongX commented Apr 23, 2024

5 ci group failed somehow, re-running now

@BionIT
Copy link
Collaborator

BionIT commented Apr 23, 2024

When hide local cluster, and there is no default data source

What's the input for the data source component? Also can we show the popover?

@BionIT
Copy link
Collaborator

BionIT commented Apr 23, 2024

When hide local cluster, and there is default data source

The toast message should change too and can you double check the requirement?

@BionIT
Copy link
Collaborator

BionIT commented Apr 23, 2024

Can we add test for when invalid datasource id gets passed in and there is default data source, we should the right popover with toast and be able to switch to use default data source?

@yujin-emma
Copy link
Contributor Author

Can we add test for when invalid datasource id gets passed in and there is default data source, we should the right popover with toast and be able to switch to use default data source?

Test 5.2 cover this case

@yujin-emma
Copy link
Contributor Author

yujin-emma commented Apr 23, 2024

When hide local cluster, and there is default data source

The toast message should change too and can you double check the requirement?

Test 2 cover this

@yujin-emma yujin-emma force-pushed the invalid-id-pr branch 3 times, most recently from 5dea762 to 0ef40a5 Compare April 29, 2024 16:56
zhongnansu
zhongnansu previously approved these changes Apr 29, 2024
@yujin-emma yujin-emma force-pushed the invalid-id-pr branch 2 times, most recently from 1a3d93c to db706f0 Compare May 3, 2024 00:36
if (optionId === '' && !this.props.hideLocalCluster) {
this.setState({
selectedOption: [LocalCluster],
defaultDataSource,
defaultDataSource: filteredDefaultDataSourceOption,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why changing default data source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might miss this comment before, but for optionId === ' ' && !this.props.hideLocalCluster, we only update the defaultDataSource ( type of string ) as previous logic

defaultDataSource: string | null
) {
const selectedDataSource = await this.getSelectedDataSource(dataSourceId, defaultDataSource);
if (!this._isMounted) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this check?

}

await this.getAndSetDefaultDataSourceOption(dataSourceId, defaultDataSource);
this.handleInvalidDataSourceError(dataSourceId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

are we calling this.handleInvalidDataSourceError twice if there is exception in this.getAndSetDefaultDataSourceOption

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops, it did call twice then, let me update the getAndSetDefaultDataSourceOption to only return error not call handleInvalidDataSourceError

}
this.clearSelectedOptionState();
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be same as handle invalid data source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what clearSelectedOptionState does:

  1. clear the state for selectedOption
  2. pass empty array to onSelectedDataSources if it exist
    what handle invalid data source does:
  3. show error state
  4. set defaultDataSourceOption
  5. show notification
  6. pass empty array to onSelectedDataSources if it exist
  7. show switch to default option if the defaultOption exist
    the only overlap is pass empty array to onSelectedDataSources if it exist, therefore, I think they are different

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multiple datasource multiple datasource project Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry v2.16.0 valued-contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants