-
Notifications
You must be signed in to change notification settings - Fork 885
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] Expose filterfn in datasource menu component to allow filter data sources before rendering in navigation bar #6113
Conversation
Signed-off-by: Lu Yu <nluyu@amazon.com>
Signed-off-by: Lu Yu <nluyu@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6113 +/- ##
==========================================
+ Coverage 67.14% 67.15% +0.01%
==========================================
Files 3326 3326
Lines 64394 64400 +6
Branches 10358 10361 +3
==========================================
+ Hits 43237 43249 +12
+ Misses 18627 18620 -7
- Partials 2530 2531 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -69,18 +70,24 @@ export class DataSourceSelectable extends React.Component< | |||
|
|||
async componentDidMount() { | |||
this._isMounted = true; | |||
getDataSources(this.props.savedObjectsClient) | |||
getDataSourcesWithFields(this.props.savedObjectsClient, ['id', 'title', 'auth.type']) |
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.
how we extend this, which not only can filter auth.type?
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 can extend by adding more fields, the fields entered here are to specify which fields should be returned from the API for data source object, and before, it restricts to id, title, description, auth type is required for security plugin to remove no auth data sources since they would indicate non-FGAC domains
perPage: 10000, | ||
type: 'data-source', | ||
}); | ||
expect(toasts.addWarning).toBeCalledTimes(0); | ||
}); | ||
|
||
it('should filter options if configured', async () => { |
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.
Where we configure this filterfn, is configurable in the yml file or somewhere or must change the code to configure the filter? thanks
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.
This filterFn is passed by plugins when they consume the data source menu component, it is used then in the component when it is mounted to remove unwanted data sources before rendering
Signed-off-by: Lu Yu <nluyu@amazon.com>
a.label.toLowerCase().localeCompare(b.label.toLowerCase()) | ||
); | ||
if (filteredDataSources.length === 0) { | ||
filteredDataSources = fetchedDataSources; |
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.
If no filteredDataSources exist after apply filter, we will show all dataSource instead. And those datasource won't meet the filter condition.
Will it introduce in some confusions? What if we just show nothing.
Thanks
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.
This filter is for plugins to decide how they want to render the options, there is possibility the filter would remove all options then there will be no options displayed. If plugins decide not to add filter options, then it should show all data sources, why it is confusing?
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.
This comment is not a blocker of this PR~
Suppose we have following dataSources:
[{datasource1, NoAuth}, {datasource2, NoAuth}, {datasource3, NoAuth}]
And filter out all "NoAuth" dataSources.
Looks like we will return [{datasource1, NoAuth}, {datasource2, NoAuth}, {datasource3, NoAuth}]
here,
instead of an empty array. Is that expected?
Thanks~
… allow filter data sources before rendering in navigation bar (#6113) * expose filterfn in datasource menu component Signed-off-by: Lu Yu <nluyu@amazon.com> * add changelog Signed-off-by: Lu Yu <nluyu@amazon.com> --------- Signed-off-by: Lu Yu <nluyu@amazon.com> (cherry picked from commit 45096bb) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
… allow filter data sources before rendering in navigation bar (#6113) (#6261) * expose filterfn in datasource menu component Signed-off-by: Lu Yu <nluyu@amazon.com> * add changelog Signed-off-by: Lu Yu <nluyu@amazon.com> --------- Signed-off-by: Lu Yu <nluyu@amazon.com> (cherry picked from commit 45096bb) 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
In #6057, we exposed a few properties in data source selector component for plugins to consume, and in #6082, we created a new data source menu component to be mounted into navigation bar. Based on requirements, we should also expose this filter function to allow plugins to exclude data sources that don't make sense to render in options
Issues Resolved
Screenshot
exportfilterinselectable.mp4
Testing the changes
The following steps were performed in the recording:
Check List
yarn test:jest
yarn test:jest_integration