bito-code-review[bot] commented on code in PR #32995:
URL: https://github.com/apache/superset/pull/32995#discussion_r3551138484
##########
superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx:
##########
@@ -43,6 +43,24 @@ jest.mock('src/utils/export', () => ({
default: jest.fn(),
}));
+// Mock bootstrap data so isUserEditorOrAdmin's subject-based check resolves
+// the mock user (userId 1) as a subject editor of the fixtures below.
+jest.mock('src/utils/getBootstrapData', () => {
+ const actual = jest.requireActual('src/utils/getBootstrapData');
+ const { DEFAULT_BOOTSTRAP_DATA } = jest.requireActual('src/constants');
+ return {
+ __esModule: true,
+ ...actual,
+ default: jest.fn(() => ({
+ ...DEFAULT_BOOTSTRAP_DATA,
+ common: {
+ ...DEFAULT_BOOTSTRAP_DATA.common,
+ user_subjects: [1],
+ },
+ })),
+ };
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing mock variable and reset</b></div>
<div id="fix">
The `getBootstrapData` mock creates an unnamed `jest.fn()` that's never
captured or reset between tests. This causes test isolation issues: any test
that modifies the mock's return value will leak state to subsequent tests. The
pattern should match `mockIsFeatureEnabled` on line 64-66 which uses a properly
typed and reset mock variable.
</div>
</div>
<small><i>Code Review Run #a4425c</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/dashboard/util/permissionUtils.ts:
##########
@@ -40,6 +41,17 @@ export const isUserAdmin = (
role => role.toLowerCase() === ADMIN_ROLE_NAME,
);
+export const isUserEditorOrAdmin = (
+ user?: UserWithPermissionsAndRoles | UndefinedUser,
+ editors: Subject[] = [],
+): boolean => {
+ const userSubjects = getUserSubjects();
+ return (
+ editors.some(editor => userSubjects.includes(editor.id)) ||
+ isUserAdmin(user)
+ );
+};
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Semantic duplication with existing function</b></div>
<div id="fix">
The `isUserEditorOrAdmin` function (lines 48-50) duplicates the same control
flow as `isUserDashboardEditor` (lines 56-58): both call `getUserSubjects()`
and use `editors.some(editor => userSubjects.includes(editor.id))`. This
pattern should be extracted to a shared helper to prevent divergent bugs when
the logic needs to change.
</div>
</div>
<small><i>Code Review Run #a4425c</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]
