[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, | ||||||
|
|
@@ -32,21 +31,30 @@ 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> | ||||||
| >({}); | ||||||
| useEffect(() => { | ||||||
| if (onSelectedItems) { | ||||||
| onSelectedItems(selectedItems); | ||||||
| } | ||||||
| }, [selectedItems, onSelectedItems]); | ||||||
|
|
||||||
| useEffect(() => { | ||||||
| // Reset selected items when connectionType changes | ||||||
|
|
@@ -174,27 +182,26 @@ export const DataSourceConnectionTable = ({ | |||||
| }, | ||||||
| }, | ||||||
| { | ||||||
| width: '15%', | ||||||
|
There was a problem hiding this comment. @Kapian1234 Seems we are re-use the same component with workspace detail side. Could you help check with @yubonluo if this width updates can be applied in workspace detail? I'm prefer to add There was a problem hiding this comment. The width updates will indeed be applied in workspace detail, but I've checked with @yubonluo and the new width still presents well there. Perhaps the flag isn't necessary? |
||||||
| 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 +274,17 @@ export const DataSourceConnectionTable = ({ | |||||
| 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]); | ||||||
| } else { | ||||||
| setSelectedItems([item]); | ||||||
| setModalVisible(true); | ||||||
| } | ||||||
| }, | ||||||
| 'data-test-subj': 'workspace-detail-dataSources-table-actions-remove', | ||||||
| }, | ||||||
| ], | ||||||
| width: '10%', | ||||||
| } as EuiTableActionsColumnType<DataSourceConnection>, | ||||||
| ] | ||||||
| : []), | ||||||
|
|
@@ -285,22 +297,34 @@ export const DataSourceConnectionTable = ({ | |||||
|
|
||||||
| 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?