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

Refactor workspace datasource association #8545

Conversation

ruanyl
Copy link
Member

@ruanyl ruanyl commented Oct 10, 2024

Description

This PR refactored the data source management page to include:

  1. Added a button to page header to associate data source to current workspace
  2. Added a table action set default data source when inside a workspace
  3. Added a table action to dissociate data source from the current workspace
Screen.Recording.2024-10-10.at.18.43.16.mov

Issues Resolved

Screenshot

Testing the changes

Changelog

  • feat: refactor data source list page to include data source association features for workspace

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

Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 49.50980% with 103 lines in your changes missing coverage. Please review.

Project coverage is 60.93%. Comparing base (91fd6d3) to head (3d1f537).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
...ion/manage_direct_query_data_connections_table.tsx 38.33% 25 Missing and 12 partials ⚠️
...components/data_source_table/data_source_table.tsx 54.90% 18 Missing and 5 partials ⚠️
src/plugins/workspace/server/workspace_client.ts 0.00% 16 Missing ⚠️
...ata_source_association/data_source_association.tsx 70.73% 5 Missing and 7 partials ⚠️
src/plugins/workspace/public/workspace_client.ts 0.00% 9 Missing ⚠️
src/plugins/workspace/server/routes/index.ts 25.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8545      +/-   ##
==========================================
- Coverage   60.93%   60.93%   -0.01%     
==========================================
  Files        3771     3777       +6     
  Lines       89541    89845     +304     
  Branches    14017    14082      +65     
==========================================
+ Hits        54563    54745     +182     
- Misses      31567    31668     +101     
- Partials     3411     3432      +21     
Flag Coverage Δ
Linux_1 29.15% <53.76%> (+0.13%) ⬆️
Linux_2 56.28% <ø> (+<0.01%) ⬆️
Linux_3 37.80% <45.94%> (+0.05%) ⬆️
Linux_4 29.94% <ø> (+0.02%) ⬆️
Windows_1 29.16% <53.76%> (+0.13%) ⬆️
Windows_2 56.23% <ø> (+<0.01%) ⬆️
Windows_3 37.81% <45.94%> (+0.05%) ⬆️
Windows_4 29.94% <ø> (+0.02%) ⬆️

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.

error?: string;
}

export interface IWorkspaceClient {
Copy link
Member

Choose a reason for hiding this comment

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

I guess we may need a TODO here to add other methods of client.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, those should be refactored as well

}

export interface WorkspaceUI {
DataSourceAssociation: (props: DataSourceAssociationProps) => JSX.Element;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: How about use typeof DataSourceAssociation here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is interface definition in core, DataSourceAssociation is the implementation in workspace plugin, so it should be the other way around.


const isDashboardAdmin = !!application?.capabilities?.dashboards?.isDashboardAdmin;
const canAssociateDataSource =
!!currentWorkspace && !currentWorkspace.readonly && isDashboardAdmin;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Since we already check isDashboardAdmin at the end, do we need to check if workspace readonly here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

SuZhou-Joe
SuZhou-Joe previously approved these changes Oct 12, 2024
Signed-off-by: Yulong Ruan <[email protected]>
@ruanyl ruanyl merged commit fc03639 into opensearch-project:main Oct 12, 2024
67 of 68 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 12, 2024
* refactor: add data source association button to data source management page

Signed-off-by: Yulong Ruan <[email protected]>

* feat: add action button to dissociate data source from data source table

Signed-off-by: Yulong Ruan <[email protected]>

* Changeset file for PR #8545 created/updated

* fix: automatically set a default data source if default data source been dissociated

Signed-off-by: Yulong Ruan <[email protected]>

* feat: implement bulk dissociate in data source management page

Signed-off-by: Yulong Ruan <[email protected]>

* fix lint

Signed-off-by: Yulong Ruan <[email protected]>

---------

Signed-off-by: Yulong Ruan <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit fc03639)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ruanyl pushed a commit that referenced this pull request Oct 12, 2024
* refactor: add data source association button to data source management page



* feat: add action button to dissociate data source from data source table



* Changeset file for PR #8545 created/updated

* fix: automatically set a default data source if default data source been dissociated



* feat: implement bulk dissociate in data source management page



* fix lint



---------



(cherry picked from commit fc03639)

Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
sejli pushed a commit to sejli/OpenSearch-Dashboards that referenced this pull request Oct 21, 2024
* refactor: add data source association button to data source management page

Signed-off-by: Yulong Ruan <[email protected]>

* feat: add action button to dissociate data source from data source table

Signed-off-by: Yulong Ruan <[email protected]>

* Changeset file for PR opensearch-project#8545 created/updated

* fix: automatically set a default data source if default data source been dissociated

Signed-off-by: Yulong Ruan <[email protected]>

* feat: implement bulk dissociate in data source management page

Signed-off-by: Yulong Ruan <[email protected]>

* fix lint

Signed-off-by: Yulong Ruan <[email protected]>

---------

Signed-off-by: Yulong Ruan <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Qxisylolo pushed a commit to Qxisylolo/OpenSearch-Dashboards that referenced this pull request Oct 30, 2024
* refactor: add data source association button to data source management page

Signed-off-by: Yulong Ruan <[email protected]>

* feat: add action button to dissociate data source from data source table

Signed-off-by: Yulong Ruan <[email protected]>

* Changeset file for PR opensearch-project#8545 created/updated

* fix: automatically set a default data source if default data source been dissociated

Signed-off-by: Yulong Ruan <[email protected]>

* feat: implement bulk dissociate in data source management page

Signed-off-by: Yulong Ruan <[email protected]>

* fix lint

Signed-off-by: Yulong Ruan <[email protected]>

---------

Signed-off-by: Yulong Ruan <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[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
Development

Successfully merging this pull request may close these issues.

4 participants