bito-code-review[bot] commented on code in PR #43887:
URL: https://github.com/apache/superset/pull/43887#discussion_r4048238940
##########
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 changes correctly align the UI with the semantic layer permission logic.
By replacing the redundant `canDelete` and `canEdit` checks with the
`canWriteLayer` guard, the code ensures that button visibility is consistent
with the required access control, while maintaining the existing early return
behavior.
**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 suggestion to remove the redundant `{canWriteLayer && ...}` wrapper is
appropriate. Since the early return at line 826 already ensures the component
only renders if `canWriteLayer` is true, the subsequent check is unnecessary
and its removal simplifies the code.
**superset-frontend/src/pages/DatabaseList/index.tsx**
```
<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, the Actions column is now
correctly visible for export-only users, ensuring the Export button is
accessible as intended.
**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 correctly addressed the suggestion by centralizing the
semantic-layer route glob into the shared `API_ENDPOINTS` constant and updating
the test assertion to use the existing related-database constant. This change
ensures consistency and maintainability by synchronizing the mock and assertion
with the shared test configuration.
--
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]
