bito-code-review[bot] commented on code in PR #43887:
URL: https://github.com/apache/superset/pull/43887#discussion_r4043722104
##########
superset-frontend/src/pages/DatabaseList/index.tsx:
##########
@@ -798,13 +823,13 @@ function DatabaseList({
const isSemanticLayer = original.source_type === 'semantic_layer';
if (isSemanticLayer) {
- if (!canEdit && !canDelete) return null;
+ if (!canWriteLayer) return null;
const isLoadingDependents =
slDeletePreview?.status === 'loading' &&
slDeletePreview.item.uuid === original.uuid;
return (
<div className="actions">
- {canDelete && (
+ {canWriteLayer && (
Review Comment:
<!-- Bito Reply -->
The change is appropriate. By replacing the redundant `canDelete` and
`canEdit` checks with the single `canWriteLayer` guard, the code correctly
aligns the button visibility with the semantic layer permission logic, ensuring
the UI state is consistent with the underlying access control.
**superset-frontend/src/pages/DatabaseList/index.tsx**
```
if (isSemanticLayer) {
if (!canWriteLayer) return null;
const isLoadingDependents =
slDeletePreview?.status === 'loading' &&
slDeletePreview.item.uuid === original.uuid;
return (
<div className="actions">
{canWriteLayer && (
```
##########
superset-frontend/src/pages/DatabaseList/index.tsx:
##########
@@ -824,7 +849,7 @@ function DatabaseList({
onClick={() => openSemanticLayerDeleteModal(original)}
/>
)}
- {canEdit && (
+ {canWriteLayer && (
Review Comment:
<!-- Bito Reply -->
The change is appropriate. By removing the redundant `{canWriteLayer &&
...}` wrapper, the code is simplified and aligns with the early return logic
established at line 826, which already ensures the component only renders if
`canWriteLayer` is true.
**superset-frontend/src/pages/DatabaseList/index.tsx**
```
{canWriteLayer && (
<IconButton
icon={<Icons.EditOutlined />}
onClick={() => openSemanticLayerDeleteModal(original)}
/>
)}
```
##########
superset-frontend/src/pages/DatabaseList/index.tsx:
##########
@@ -894,7 +919,7 @@ function DatabaseList({
},
Header: t('Actions'),
id: 'actions',
- hidden: !canEdit && !canDelete,
+ hidden: !canEdit && !canDelete && !canWriteLayer,
Review Comment:
<!-- Bito Reply -->
The suggestion is appropriate and correctly addresses the visibility issue
by including `canExport` in the conditional check. This ensures the Actions
column is visible when an export-only user has access, preventing the column
from being hidden while the Export button remains unreachable.
**superset-frontend/src/pages/DatabaseList/index.tsx**
```
Header: t('Actions'),
id: 'actions',
hidden: !canEdit && !canDelete && !canWriteLayer && !canExport,
```
##########
superset-frontend/src/pages/DatasetList/DatasetList.connectionPermissions.test.tsx:
##########
@@ -0,0 +1,69 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import fetchMock from 'fetch-mock';
+import { screen, waitFor } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import {
+ setupMocks,
+ renderDatasetList,
+ mockAdminUser,
+} from './DatasetList.testHelpers';
+
+beforeEach(() => {
+ setupMocks();
+ window.featureFlags = { SEMANTIC_LAYERS: true } as never;
+ fetchMock.get('glob:*/api/v1/semantic_layer/?*', { result: [], count: 0 });
+});
+
+afterEach(() => {
+ window.featureFlags = {} as never;
+ fetchMock.clearHistory().removeRoutes();
+ jest.restoreAllMocks();
+});
+
+test.each([false, true])(
+ 'dataset connection options respect independent layer read (%s)',
+ async canReadLayer => {
+ const user = {
+ ...mockAdminUser,
+ roles: {
+ Admin: [
+ ...mockAdminUser.roles.Admin,
+ ...(canReadLayer ? [['can_read', 'SemanticLayer']] : []),
+ ],
+ },
+ };
+ renderDatasetList(user);
+ await screen.findByTestId('search-filter-container');
+ const filter = screen
+ .getAllByTestId('compact-filter-pill')
+ .find(item => item.textContent?.includes('Data connection'));
+ expect(filter).toBeDefined();
+ await userEvent.click(filter!);
+ await waitFor(() =>
+ expect(
+ fetchMock.callHistory.calls('glob:*/api/v1/dataset/related/database*')
+ .length,
+ ).toBeGreaterThan(0),
+ );
+ expect(
+ fetchMock.callHistory.calls('glob:*/api/v1/semantic_layer/?*').length >
0,
Review Comment:
<!-- Bito Reply -->
The user has addressed the suggestion by moving the semantic-layer route
glob into the shared `API_ENDPOINTS` constant and updating the assertion to use
the existing related-database constant. This change correctly synchronizes the
mock and assertion, ensuring consistency if the endpoint path is updated in the
future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
