bito-code-review[bot] commented on code in PR #43887:
URL: https://github.com/apache/superset/pull/43887#discussion_r4042036291
##########
superset-frontend/src/pages/DatabaseList/index.tsx:
##########
@@ -824,7 +849,7 @@ function DatabaseList({
onClick={() => openSemanticLayerDeleteModal(original)}
/>
)}
- {canEdit && (
+ {canWriteLayer && (
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Always-true guard on Edit</b></div>
<div id="fix">
Same redundancy as the Delete guard at line 832: the early return `if
(!canWriteLayer) return null;` at line 826 makes this `{canWriteLayer && ...}`
check always true. The pre-change code tested `canEdit` here, which could
differ from the row gate; it no longer can. Dropping the wrapper keeps the
permission model readable.
</div>
</div>
<small><i>Code Review Run #55e39b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Always-true guard on Delete</b></div>
<div id="fix">
The early return `if (!canWriteLayer) return null;` at line 826 guarantees
`canWriteLayer` is truthy here, so this guard is always true — same for the
Edit guard at line 852. Unlike the previous `canDelete`/`canEdit` checks, the
condition can no longer vary independently. Consider dropping the redundant
wrappers so the buttons don't imply independent permission logic.
</div>
</div>
<small><i>Code Review Run #55e39b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Actions column ignores canExport</b></div>
<div id="fix">
The cell renderer returns action buttons when `canExport` alone is set (line
870: `if (!canEdit && !canDelete && !canExport) return null;`), but this
visibility check omits `canExport`, so an export-only user gets a hidden
Actions column with unreachable Export buttons. Adding `!canExport` aligns
column visibility with cell content.
</div>
</div>
<small><i>Code Review Run #55e39b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated endpoint glob literal</b></div>
<div id="fix">
The semantic_layer route glob is hardcoded twice: line 31 registers the mock
and line 66 asserts against the same literal. Sibling tests
(`DatasetList.behavior.test.tsx`) route-match via the shared `API_ENDPOINTS`
constants in `DatasetList.testHelpers.tsx`. A shared constant keeps mock and
assertion in sync if the endpoint changes.
</div>
</div>
<small><i>Code Review Run #55e39b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
