[Workspace] Refactor 'Associate data sources' in create page to support direct query connections #7961
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| feat: | ||
| - Support DQCs in create page ([#7961](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7961)) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -6,7 +6,6 @@ | |||||
| import './data_source_connection_table.scss'; | ||||||
| import React, { useCallback, useEffect, useMemo, useState } from 'react'; | ||||||
| import { | ||||||
| EuiInMemoryTable, | ||||||
| EuiBasicTableColumn, | ||||||
| EuiTableSelectionType, | ||||||
|
|
@@ -27,26 +26,40 @@ | |||||
| import { DataSourceConnection, DataSourceConnectionType } from '../../../common/types'; | ||||||
| import { AssociationDataSourceModalMode } from '../../../common/constants'; | ||||||
| import { DirectQueryConnectionIcon } from '../workspace_form'; | ||||||
| import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react/public'; | ||||||
| import { CoreStart } from '../../../../../core/public'; | ||||||
|
|
||||||
| interface DataSourceConnectionTableProps { | ||||||
| isDashboardAdmin: boolean; | ||||||
| connectionType: string; | ||||||
| dataSourceConnections: DataSourceConnection[]; | ||||||
| handleUnassignDataSources: (dataSources: DataSourceConnection[]) => void; | ||||||
| onSelectedItems?: (dataSources: DataSourceConnection[]) => void; | ||||||
| inCreatePage?: boolean; | ||||||
| } | ||||||
|
|
||||||
| export const DataSourceConnectionTable = ({ | ||||||
| isDashboardAdmin, | ||||||
| connectionType, | ||||||
| dataSourceConnections, | ||||||
| handleUnassignDataSources, | ||||||
| onSelectedItems, | ||||||
|
There was a problem hiding this comment.
Suggested change
Nit: |
||||||
| inCreatePage = false, | ||||||
|
There was a problem hiding this comment. Could you leave a comment to describe the UI results of setting |
||||||
| }: DataSourceConnectionTableProps) => { | ||||||
| const [selectedItems, setSelectedItems] = useState<DataSourceConnection[]>([]); | ||||||
| const [modalVisible, setModalVisible] = useState(false); | ||||||
| const [popoversState, setPopoversState] = useState<Record<string, boolean>>({}); | ||||||
| const [itemIdToExpandedRowMap, setItemIdToExpandedRowMap] = useState< | ||||||
| Record<string, React.ReactNode> | ||||||
| >({}); | ||||||
| const { | ||||||
| services: { http }, | ||||||
| } = useOpenSearchDashboards<CoreStart>(); | ||||||
| useEffect(() => { | ||||||
| if (onSelectedItems) { | ||||||
| onSelectedItems(selectedItems); | ||||||
| } | ||||||
| }, [selectedItems, onSelectedItems]); | ||||||
|
|
||||||
| useEffect(() => { | ||||||
| // Reset selected items when connectionType changes | ||||||
|
|
@@ -152,19 +165,20 @@ | |||||
| ] | ||||||
| : []), | ||||||
| { | ||||||
| width: '20%', | ||||||
| field: 'name', | ||||||
| name: i18n.translate('workspace.detail.dataSources.table.title', { | ||||||
| defaultMessage: 'Title', | ||||||
| }), | ||||||
| truncateText: true, | ||||||
| render: (name: string, record) => { | ||||||
| const origin = window.location.origin; | ||||||
| const basePath = http.basePath.serverBasePath; | ||||||
| let url: string; | ||||||
| if (record.connectionType === DataSourceConnectionType.OpenSearchConnection) { | ||||||
| url = `${origin}${basePath}/app/dataSources/${record.id}`; | ||||||
|
There was a problem hiding this comment. Can we use There was a problem hiding this comment.
There was a problem hiding this comment. I think Suzhou refers to There was a problem hiding this comment. right, please use http.basePath to get the final path. We should avoid string concatenation when generating the path. |
||||||
| } else { | ||||||
| url = `${origin}${basePath}/app/dataSources/manage/${name}?dataSourceMDSId=${record.parentId}`; | ||||||
| } | ||||||
| return ( | ||||||
| <EuiLink href={url} className="eui-textTruncate"> | ||||||
|
|
@@ -174,27 +188,26 @@ | |||||
| }, | ||||||
| }, | ||||||
| { | ||||||
| width: '20%', | ||||||
| field: 'type', | ||||||
| name: i18n.translate('workspace.detail.dataSources.table.type', { | ||||||
| defaultMessage: 'Type', | ||||||
| }), | ||||||
| truncateText: true, | ||||||
| }, | ||||||
| { | ||||||
| field: 'description', | ||||||
| name: i18n.translate('workspace.detail.dataSources.table.description', { | ||||||
| defaultMessage: 'Description', | ||||||
| }), | ||||||
| truncateText: true, | ||||||
| }, | ||||||
| { | ||||||
| width: '140px', | ||||||
| field: 'relatedConnections', | ||||||
| name: i18n.translate('workspace.detail.dataSources.table.relatedConnections', { | ||||||
| defaultMessage: 'Related connections', | ||||||
| }), | ||||||
| truncateText: true, | ||||||
| render: (relatedConnections: DataSourceConnection[], record) => | ||||||
| relatedConnections?.length > 0 ? ( | ||||||
|
|
@@ -267,12 +280,17 @@ | |||||
| icon: 'unlink', | ||||||
| type: 'icon', | ||||||
| onClick: (item: DataSourceConnection) => { | ||||||
| if (inCreatePage) { | ||||||
|
There was a problem hiding this comment. When in create page, click un-assign button will call |
||||||
| handleUnassignDataSources([item]); | ||||||
|
Check warning on line 284 in src/plugins/workspace/public/components/workspace_detail/data_source_connection_table.tsx
|
||||||
| } else { | ||||||
| setSelectedItems([item]); | ||||||
| setModalVisible(true); | ||||||
| } | ||||||
| }, | ||||||
| 'data-test-subj': 'workspace-detail-dataSources-table-actions-remove', | ||||||
| }, | ||||||
| ], | ||||||
| width: '10%', | ||||||
| } as EuiTableActionsColumnType<DataSourceConnection>, | ||||||
| ] | ||||||
| : []), | ||||||
|
|
@@ -285,22 +303,34 @@ | |||||
|
|
||||||
| return ( | ||||||
| <> | ||||||
| {inCreatePage ? ( | ||||||
|
There was a problem hiding this comment. looks like the only difference is we show pagination when not in create page? interface DataSourceConnectionTableProps extends Pick<EuiInMemoryTableProps<DataSourceConnection>, 'pagination'> {
...
}Then you can pass There was a problem hiding this comment. Another difference is that we show search bar when not in create page, then we have to pass two props. The search props looks like this, we may not need to pass it. There was a problem hiding this comment. This component needs a refactor. If we are trying to make this component a reusable component, we should let the parent of the component to pass in different props to control the behavior of the component. For this component, I think we should split this component into 2 components:
There was a problem hiding this comment. Agree with @SuZhou-Joe 's comments. Current implementation of |
||||||
| <EuiInMemoryTable | ||||||
| items={openSearchConnections} | ||||||
| itemId="id" | ||||||
| columns={columns} | ||||||
| selection={selection} | ||||||
| key={connectionType} | ||||||
| isSelectable={true} | ||||||
| itemIdToExpandedRowMap={itemIdToExpandedRowMap} | ||||||
| isExpandable={true} | ||||||
| /> | ||||||
| ) : ( | ||||||
| <EuiInMemoryTable | ||||||
| items={openSearchConnections} | ||||||
| itemId="id" | ||||||
| columns={columns} | ||||||
| selection={selection} | ||||||
| search={search} | ||||||
| key={connectionType} | ||||||
| isSelectable={true} | ||||||
| itemIdToExpandedRowMap={itemIdToExpandedRowMap} | ||||||
| isExpandable={true} | ||||||
| pagination={{ | ||||||
| initialPageSize: 10, | ||||||
| pageSizeOptions: [10, 20, 30], | ||||||
| }} | ||||||
| /> | ||||||
| )} | ||||||
| {modalVisible && ( | ||||||
| <EuiConfirmModal | ||||||
| data-test-subj="workspaceForm-cancelModal" | ||||||
|
|
||||||
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.
Is it by intension?
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.
Yes, it's always true here, otherwise the component wouldn't be mounted. Above, there's
isDashboardAdmin && isDataSourceEnabled && (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.
I suggest we rename
isDashboardAdmintoshowDataSourceManagementor something like that, because it's a UI behavior,isDashboardAdminhas been widely used as a term that related to permissionThere 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.
But in this SelectDataSourcePanel component,
isDashboardAdminwill continue to be passed to components like DataSourceConnectionTable, where there are some behaviors like 'select' and 'delete' do indeed need permissions verification. UsingshowDataSourceManagementduring the pass may cause confusion.In SelectDataSourcePanel:
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.
I see, so that's an existing props. I think you can change the
props.isDashboardAdminofSelectDataSourcePanelbecause this props is newly aded, let's fix this first if fixingDataSourceConnectionTableis out of the scope of this PR.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.
ok, so in SelectDataSourcePanel it would become:
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.
@wanglam @Kapian1234 maybe we can have a follow up task to align this prop name among these components.
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.
I agree that we should have a follow-up task update these components. The
isDashboardAdmininDataSourceConnectionTablecontrols two behaviors. The first one is to control whether table row can be selectable. The second one is to control whether data source connection can be unassociated. Shall we need break it into two properties?