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] Use data source filter function before rendering #6175

Merged
merged 6 commits into from
Mar 19, 2024

Conversation

BionIT
Copy link
Collaborator

@BionIT BionIT commented Mar 18, 2024

Description

This change will store all data sources fetched during initial load, then apply the datasource filter function before rendering so that it can filter based on changes in the filter function.

Issues Resolved

Fixes #6174

Screenshot

filterbeforerender.mp4

Testing the changes

The following steps were performed in the recording:

  1. go to add sample data, and data source selector should render correctly with currently available data sources
  2. add sample data from a data source is successful, and dashboard renders successfully
  3. go to dev tool, and data source selector should render correctly with currently available data sources
  4. make a query and is successfully returning response
  5. go to search relevance and the data source selector should render correctly, with local cluster as default option and no other options available to choose
  6. select a data source from the data source menu in the nav bar which acts as filter, then check the data source selector, which should have the selected data source as another option in addition to local cluster

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: Lu Yu <nluyu@amazon.com>
@BionIT BionIT changed the title use filter function before rendering [Multiple Datasource] Use data source filter function before rendering Mar 18, 2024
Signed-off-by: Lu Yu <nluyu@amazon.com>
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

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

Project coverage is 67.25%. Comparing base (d2347ca) to head (f835a57).

Files Patch % Lines
...ents/data_source_selector/data_source_selector.tsx 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6175      +/-   ##
==========================================
- Coverage   67.25%   67.25%   -0.01%     
==========================================
  Files        3345     3345              
  Lines       64840    64835       -5     
  Branches    10435    10432       -3     
==========================================
- Hits        43606    43602       -4     
  Misses      18690    18690              
+ Partials     2544     2543       -1     
Flag Coverage Δ
Linux_1 31.64% <ø> (ø)
Linux_2 55.45% <ø> (ø)
Linux_3 44.74% <80.00%> (?)
Linux_4 35.06% <0.00%> (+<0.01%) ⬆️
Windows_1 31.67% <ø> (ø)
Windows_2 55.41% <ø> (ø)
Windows_3 44.74% <80.00%> (-0.01%) ⬇️
Windows_4 35.06% <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
ruanyl previously approved these changes Mar 18, 2024
Signed-off-by: Lu Yu <nluyu@amazon.com>
@ZilongX
Copy link
Collaborator

ZilongX commented Mar 19, 2024

LGTM, picked up latest main branch and now rerun all checks

@@ -119,6 +99,16 @@ export class DataSourceSelector extends React.Component<
this.props.placeholderText === undefined
? 'Select a data source'
: this.props.placeholderText;

const dataSources = this.props.dataSourceFilter
Copy link
Member

Choose a reason for hiding this comment

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

Do we have UT to cover this changes? Thanks

Copy link
Member

Choose a reason for hiding this comment

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

+1. Approved but I'm wondering if we need UT for this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@BionIT BionIT merged commit 8975381 into opensearch-project:main Mar 19, 2024
68 checks passed
@BionIT BionIT added the v2.14.0 label Mar 19, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 25, 2024
#6175)

* use filter function before rendering

Signed-off-by: Lu Yu <nluyu@amazon.com>

* add change log

Signed-off-by: Lu Yu <nluyu@amazon.com>

---------

Signed-off-by: Lu Yu <nluyu@amazon.com>
Co-authored-by: ZilongX <99905560+ZilongX@users.noreply.github.com>
(cherry picked from commit 8975381)
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 Mar 25, 2024
#6175) (#6265)

* use filter function before rendering

Signed-off-by: Lu Yu <nluyu@amazon.com>

* add change log

Signed-off-by: Lu Yu <nluyu@amazon.com>

---------

Signed-off-by: Lu Yu <nluyu@amazon.com>
Co-authored-by: ZilongX <99905560+ZilongX@users.noreply.github.com>
(cherry picked from commit 8975381)
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
None yet
6 participants