This is an automated email from the ASF dual-hosted git repository. jli pushed a commit to branch fix-dynamic-search-dataset in repository https://gitbox.apache.org/repos/asf/superset.git
commit fc0c54d8dcdde8389919ce72f4ec686c202a1b66 Author: Joe Li <[email protected]> AuthorDate: Tue Mar 3 21:12:07 2026 -0800 fix(roles): convert permissions/groups dropdowns to AsyncSelect with server-side search The DAR permissions and groups dropdowns used static Select components that broke for workspaces with >100 items. This converts them to AsyncSelect with server-side search, matching the existing UsersField pattern. Key changes: - Add fetchPermissionOptions/fetchGroupOptions in roles/utils.ts with cache-and-merge dual-column search (view_menu.name + permission.name) - Switch PermissionsField/GroupsField from Select to AsyncSelect - Add edit-form hydration: fetch labels by ID on modal open using fetchPaginatedData with fixed pageSize:100 for safe multi-page fetch - Guard hydration against partial data: missing IDs preserved as numeric-label fallbacks with warning toast - Remove upfront fetchPaginatedData for permissions/groups from RolesList - Update list view filters to use fetchSelects - Update RoleForm type: rolePermissions/roleGroups → SelectOption[] - Map SelectOption[] → number[] in submit handlers Co-Authored-By: Claude Opus 4.6 <[email protected]> --- .../src/features/roles/RoleFormItems.tsx | 51 ++-- .../src/features/roles/RoleListAddModal.test.tsx | 17 +- .../src/features/roles/RoleListAddModal.tsx | 21 +- .../src/features/roles/RoleListEditModal.test.tsx | 181 ++++++++++---- .../src/features/roles/RoleListEditModal.tsx | 162 ++++++++++-- superset-frontend/src/features/roles/types.ts | 12 +- superset-frontend/src/features/roles/utils.test.ts | 271 +++++++++++++++++++++ superset-frontend/src/features/roles/utils.ts | 127 ++++++++++ .../src/pages/RolesList/RolesList.test.tsx | 27 +- superset-frontend/src/pages/RolesList/index.tsx | 73 +----- 10 files changed, 752 insertions(+), 190 deletions(-) diff --git a/superset-frontend/src/features/roles/RoleFormItems.tsx b/superset-frontend/src/features/roles/RoleFormItems.tsx index 2be776bcbd7..939cd785741 100644 --- a/superset-frontend/src/features/roles/RoleFormItems.tsx +++ b/superset-frontend/src/features/roles/RoleFormItems.tsx @@ -16,24 +16,14 @@ * specific language governing permissions and limitations * under the License. */ -import { - FormItem, - Input, - Select, - AsyncSelect, -} from '@superset-ui/core/components'; +import { FormItem, Input, AsyncSelect } from '@superset-ui/core/components'; import { t } from '@apache-superset/core'; -import { FC } from 'react'; -import { GroupObject } from 'src/pages/GroupsList'; -import { FormattedPermission } from './types'; import { fetchUserOptions } from '../groups/utils'; +import { fetchGroupOptions, fetchPermissionOptions } from './utils'; -interface PermissionsFieldProps { - permissions: FormattedPermission[]; -} - -interface GroupsFieldProps { - groups: GroupObject[]; +interface AsyncOptionsFieldProps { + addDangerToast: (msg: string) => void; + loading?: boolean; } interface UsersFieldProps { @@ -51,17 +41,19 @@ export const RoleNameField = () => ( </FormItem> ); -export const PermissionsField: FC<PermissionsFieldProps> = ({ - permissions, -}) => ( +export const PermissionsField = ({ + addDangerToast, + loading = false, +}: AsyncOptionsFieldProps) => ( <FormItem name="rolePermissions" label={t('Permissions')}> - <Select + <AsyncSelect mode="multiple" name="rolePermissions" - options={permissions.map(permission => ({ - label: permission.label, - value: permission.id, - }))} + placeholder={t('Select permissions')} + options={(filterValue, page, pageSize) => + fetchPermissionOptions(filterValue, page, pageSize, addDangerToast) + } + loading={loading} getPopupContainer={trigger => trigger.closest('.ant-modal-content')} data-test="permissions-select" /> @@ -83,12 +75,19 @@ export const UsersField = ({ addDangerToast, loading }: UsersFieldProps) => ( </FormItem> ); -export const GroupsField: FC<GroupsFieldProps> = ({ groups }) => ( +export const GroupsField = ({ + addDangerToast, + loading = false, +}: AsyncOptionsFieldProps) => ( <FormItem name="roleGroups" label={t('Groups')}> - <Select + <AsyncSelect mode="multiple" name="roleGroups" - options={groups.map(group => ({ label: group.name, value: group.id }))} + placeholder={t('Select groups')} + options={(filterValue, page, pageSize) => + fetchGroupOptions(filterValue, page, pageSize, addDangerToast) + } + loading={loading} data-test="groups-select" /> </FormItem> diff --git a/superset-frontend/src/features/roles/RoleListAddModal.test.tsx b/superset-frontend/src/features/roles/RoleListAddModal.test.tsx index b9ab46b387a..24d0f9266f9 100644 --- a/superset-frontend/src/features/roles/RoleListAddModal.test.tsx +++ b/superset-frontend/src/features/roles/RoleListAddModal.test.tsx @@ -24,7 +24,7 @@ import { waitFor, } from 'spec/helpers/testing-library'; import RoleListAddModal from './RoleListAddModal'; -import { createRole } from './utils'; +import { createRole, updateRolePermissions } from './utils'; const mockToasts = { addDangerToast: jest.fn(), @@ -33,6 +33,7 @@ const mockToasts = { jest.mock('./utils'); const mockCreateRole = jest.mocked(createRole); +const mockUpdateRolePermissions = jest.mocked(updateRolePermissions); jest.mock('src/components/MessageToasts/withToasts', () => ({ __esModule: true, @@ -46,12 +47,15 @@ describe('RoleListAddModal', () => { show: true, onHide: jest.fn(), onSave: jest.fn(), - permissions: [ - { id: 1, label: 'Permission 1', value: 'Permission_1' }, - { id: 2, label: 'Permission 2', value: 'Permission_2' }, - ], }; + beforeEach(() => { + mockCreateRole.mockResolvedValue({ + json: { id: 1 }, + response: {} as Response, + } as Awaited<ReturnType<typeof createRole>>); + }); + test('renders modal with form fields', () => { render(<RoleListAddModal {...mockProps} />); expect(screen.getByText('Add Role')).toBeInTheDocument(); @@ -91,5 +95,8 @@ describe('RoleListAddModal', () => { await waitFor(() => { expect(mockCreateRole).toHaveBeenCalledWith('New Role'); }); + + // No permissions selected → updateRolePermissions should not be called + expect(mockUpdateRolePermissions).not.toHaveBeenCalled(); }); }); diff --git a/superset-frontend/src/features/roles/RoleListAddModal.tsx b/superset-frontend/src/features/roles/RoleListAddModal.tsx index 4a2e5f724e9..1a9c5d55583 100644 --- a/superset-frontend/src/features/roles/RoleListAddModal.tsx +++ b/superset-frontend/src/features/roles/RoleListAddModal.tsx @@ -22,25 +22,20 @@ import { useToasts } from 'src/components/MessageToasts/withToasts'; import { FormModal, Icons } from '@superset-ui/core/components'; import { createRole, updateRolePermissions } from './utils'; import { PermissionsField, RoleNameField } from './RoleFormItems'; -import { BaseModalProps, FormattedPermission, RoleForm } from './types'; +import { BaseModalProps, RoleForm } from './types'; -export interface RoleListAddModalProps extends BaseModalProps { - permissions: FormattedPermission[]; -} +export type RoleListAddModalProps = BaseModalProps; -function RoleListAddModal({ - show, - onHide, - onSave, - permissions, -}: RoleListAddModalProps) { +function RoleListAddModal({ show, onHide, onSave }: RoleListAddModalProps) { const { addDangerToast, addSuccessToast } = useToasts(); const handleFormSubmit = async (values: RoleForm) => { try { const { json: roleResponse } = await createRole(values.roleName); + const permissionIds = + values.rolePermissions?.map(({ value }) => value) || []; - if (values.rolePermissions?.length > 0) { - await updateRolePermissions(roleResponse.id, values.rolePermissions); + if (permissionIds.length > 0) { + await updateRolePermissions(roleResponse.id, permissionIds); } addSuccessToast(t('The role has been created successfully.')); @@ -70,7 +65,7 @@ function RoleListAddModal({ > <> <RoleNameField /> - <PermissionsField permissions={permissions} /> + <PermissionsField addDangerToast={addDangerToast} /> </> </FormModal> ); diff --git a/superset-frontend/src/features/roles/RoleListEditModal.test.tsx b/superset-frontend/src/features/roles/RoleListEditModal.test.tsx index 4a91495b0db..79aefc72a1b 100644 --- a/superset-frontend/src/features/roles/RoleListEditModal.test.tsx +++ b/superset-frontend/src/features/roles/RoleListEditModal.test.tsx @@ -28,6 +28,7 @@ import rison from 'rison'; import RoleListEditModal from './RoleListEditModal'; import { updateRoleName, + updateRoleGroups, updateRolePermissions, updateRoleUsers, } from './utils'; @@ -39,6 +40,7 @@ const mockToasts = { jest.mock('./utils'); const mockUpdateRoleName = jest.mocked(updateRoleName); +const mockUpdateRoleGroups = jest.mocked(updateRoleGroups); const mockUpdateRolePermissions = jest.mocked(updateRolePermissions); const mockUpdateRoleUsers = jest.mocked(updateRoleUsers); @@ -69,47 +71,11 @@ describe('RoleListEditModal', () => { group_ids: [1, 2], }; - const mockPermissions = [ - { id: 10, label: 'Permission A', value: 'perm_a' }, - { id: 20, label: 'Permission B', value: 'perm_b' }, - ]; - - const mockUsers = [ - { - id: 5, - firstName: 'John', - lastName: 'Doe', - username: 'johndoe', - email: '[email protected]', - isActive: true, - roles: [ - { - id: 1, - name: 'Admin', - }, - ], - }, - ]; - - const mockGroups = [ - { - id: 1, - name: 'Group A', - label: 'Group A', - description: 'Description A', - roles: [], - users: [], - }, - ]; - const mockProps = { role: mockRole, show: true, onHide: jest.fn(), onSave: jest.fn(), - permissions: mockPermissions, - users: mockUsers, - groups: mockGroups, }; test('renders modal with correct title and fields', () => { @@ -142,7 +108,11 @@ describe('RoleListEditModal', () => { expect(screen.getByTestId('form-modal-save-button')).toBeEnabled(); }); - test('calls update functions when save button is clicked', async () => { + test('submit maps {value,label} form values to numeric ID arrays', async () => { + // initialValues sets permissions/groups as {value, label} objects + // (e.g. [{value: 10, label: "10"}, {value: 20, label: "20"}]). + // The submit handler must convert these to plain number arrays + // before calling the update APIs. (SupersetClient.get as jest.Mock).mockImplementation(({ endpoint }) => { if (endpoint?.includes('/api/v1/security/users/')) { return Promise.resolve({ @@ -186,14 +156,24 @@ describe('RoleListEditModal', () => { mockRole.id, 'Updated Role', ); - expect(mockUpdateRolePermissions).toHaveBeenCalledWith( - mockRole.id, - mockRole.permission_ids, + + // Verify APIs receive plain number[], not {value, label}[] + const permissionArg = mockUpdateRolePermissions.mock.calls[0][1]; + expect(permissionArg).toEqual([10, 20]); + expect(permissionArg.every((id: unknown) => typeof id === 'number')).toBe( + true, ); - expect(mockUpdateRoleUsers).toHaveBeenCalledWith( - mockRole.id, - mockRole.user_ids, + + const userArg = mockUpdateRoleUsers.mock.calls[0][1]; + expect(userArg).toEqual([5, 7]); + expect(userArg.every((id: unknown) => typeof id === 'number')).toBe(true); + + const groupArg = mockUpdateRoleGroups.mock.calls[0][1]; + expect(groupArg).toEqual([1, 2]); + expect(groupArg.every((id: unknown) => typeof id === 'number')).toBe( + true, ); + expect(mockProps.onSave).toHaveBeenCalled(); }); }); @@ -225,11 +205,15 @@ describe('RoleListEditModal', () => { expect(mockGet).toHaveBeenCalled(); }); - // verify the endpoint and query parameters - const callArgs = mockGet.mock.calls[0][0]; - expect(callArgs.endpoint).toContain('/api/v1/security/users/'); + const usersCall = mockGet.mock.calls.find(([call]) => + call.endpoint.includes('/api/v1/security/users/'), + )?.[0]; + expect(usersCall).toBeTruthy(); + if (!usersCall) { + throw new Error('Expected users call to be defined'); + } - const urlMatch = callArgs.endpoint.match(/\?q=(.+)/); + const urlMatch = usersCall.endpoint.match(/\?q=(.+)/); expect(urlMatch).toBeTruthy(); const decodedQuery = rison.decode(urlMatch[1]); @@ -245,4 +229,107 @@ describe('RoleListEditModal', () => { ], }); }); + + test('preserves missing IDs as numeric fallbacks on partial hydration', async () => { + const mockGet = SupersetClient.get as jest.Mock; + mockGet.mockImplementation(({ endpoint }) => { + if (endpoint?.includes('/api/v1/security/permissions-resources/')) { + // Only return permission id=10, not id=20 + return Promise.resolve({ + json: { + count: 1, + result: [ + { + id: 10, + permission: { name: 'can_read' }, + view_menu: { name: 'Dashboard' }, + }, + ], + }, + }); + } + if (endpoint?.includes('/api/v1/security/groups/')) { + // Only return group id=1, not id=2 + return Promise.resolve({ + json: { + count: 1, + result: [{ id: 1, name: 'Engineering' }], + }, + }); + } + return Promise.resolve({ json: { count: 0, result: [] } }); + }); + + render(<RoleListEditModal {...mockProps} />); + + await waitFor(() => { + expect(mockToasts.addDangerToast).toHaveBeenCalledWith( + 'Some permissions could not be resolved and are shown as IDs.', + ); + expect(mockToasts.addDangerToast).toHaveBeenCalledWith( + 'Some groups could not be resolved and are shown as IDs.', + ); + }); + }); + + test('fetches permissions and groups by id for hydration', async () => { + const mockGet = SupersetClient.get as jest.Mock; + mockGet.mockResolvedValue({ + json: { + count: 0, + result: [], + }, + }); + + render(<RoleListEditModal {...mockProps} />); + + await waitFor(() => { + expect(mockGet).toHaveBeenCalled(); + }); + + const permissionCall = mockGet.mock.calls.find(([call]) => + call.endpoint.includes('/api/v1/security/permissions-resources/'), + )?.[0]; + const groupsCall = mockGet.mock.calls.find(([call]) => + call.endpoint.includes('/api/v1/security/groups/'), + )?.[0]; + + expect(permissionCall).toBeTruthy(); + expect(groupsCall).toBeTruthy(); + if (!permissionCall || !groupsCall) { + throw new Error('Expected hydration calls to be defined'); + } + + const permissionQuery = permissionCall.endpoint.match(/\?q=(.+)/); + const groupsQuery = groupsCall.endpoint.match(/\?q=(.+)/); + expect(permissionQuery).toBeTruthy(); + expect(groupsQuery).toBeTruthy(); + if (!permissionQuery || !groupsQuery) { + throw new Error('Expected query params to be present'); + } + + expect(rison.decode(permissionQuery[1])).toEqual({ + page_size: 100, + page: 0, + filters: [ + { + col: 'id', + opr: 'in', + value: mockRole.permission_ids, + }, + ], + }); + + expect(rison.decode(groupsQuery[1])).toEqual({ + page_size: 100, + page: 0, + filters: [ + { + col: 'id', + opr: 'in', + value: mockRole.group_ids, + }, + ], + }); + }); }); diff --git a/superset-frontend/src/features/roles/RoleListEditModal.tsx b/superset-frontend/src/features/roles/RoleListEditModal.tsx index 7159c83f98e..ce7a42a7d85 100644 --- a/superset-frontend/src/features/roles/RoleListEditModal.tsx +++ b/superset-frontend/src/features/roles/RoleListEditModal.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { useEffect, useRef, useState } from 'react'; +import { useEffect, useMemo, useRef, useState } from 'react'; import { t } from '@apache-superset/core'; import Tabs from '@superset-ui/core/components/Tabs'; import { RoleObject } from 'src/pages/RolesList'; @@ -29,11 +29,10 @@ import { } from '@superset-ui/core/components'; import { BaseModalProps, - FormattedPermission, RoleForm, + SelectOption, } from 'src/features/roles/types'; import { useToasts } from 'src/components/MessageToasts/withToasts'; -import { GroupObject } from 'src/pages/GroupsList'; import { fetchPaginatedData } from 'src/utils/fetchOptions'; import { type UserObject } from 'src/pages/UsersList/types'; import { ModalTitleWithIcon } from 'src/components/ModalTitleWithIcon'; @@ -48,12 +47,11 @@ import { updateRoleName, updateRolePermissions, updateRoleUsers, + formatPermissionLabel, } from './utils'; export interface RoleListEditModalProps extends BaseModalProps { role: RoleObject; - permissions: FormattedPermission[]; - groups: GroupObject[]; } const roleTabs = { @@ -101,14 +99,26 @@ function RoleListEditModal({ onHide, role, onSave, - permissions, - groups, }: RoleListEditModalProps) { - const { id, name, permission_ids, group_ids } = role; + const { id, name, permission_ids = [], group_ids = [] } = role; + const stablePermissionIds = useMemo( + () => permission_ids, + // eslint-disable-next-line react-hooks/exhaustive-deps + [JSON.stringify(permission_ids)], + ); + const stableGroupIds = useMemo( + () => group_ids, + // eslint-disable-next-line react-hooks/exhaustive-deps + [JSON.stringify(group_ids)], + ); const [activeTabKey, setActiveTabKey] = useState(roleTabs.edit.key); const { addDangerToast, addSuccessToast } = useToasts(); const [roleUsers, setRoleUsers] = useState<UserObject[]>([]); + const [rolePermissions, setRolePermissions] = useState<SelectOption[]>([]); + const [roleGroups, setRoleGroups] = useState<SelectOption[]>([]); const [loadingRoleUsers, setLoadingRoleUsers] = useState(true); + const [loadingRolePermissions, setLoadingRolePermissions] = useState(true); + const [loadingRoleGroups, setLoadingRoleGroups] = useState(true); const formRef = useRef<FormInstance | null>(null); useEffect(() => { @@ -131,10 +141,69 @@ function RoleListEditModal({ email: user.email, }), }); - }, [id]); + }, [addDangerToast, id]); + + useEffect(() => { + if (!stablePermissionIds.length) { + setRolePermissions([]); + setLoadingRolePermissions(false); + return; + } + + setLoadingRolePermissions(true); + const filters = [{ col: 'id', opr: 'in', value: stablePermissionIds }]; + + fetchPaginatedData({ + endpoint: `/api/v1/security/permissions-resources/`, + pageSize: 100, + setData: setRolePermissions, + filters, + setLoadingState: (loading: boolean) => setLoadingRolePermissions(loading), + loadingKey: 'rolePermissions', + addDangerToast, + errorMessage: t('There was an error loading permissions.'), + mapResult: (permission: { + id: number; + permission: { name: string }; + view_menu: { name: string }; + }) => ({ + value: permission.id, + label: formatPermissionLabel( + permission.permission.name, + permission.view_menu.name, + ), + }), + }); + }, [addDangerToast, id, stablePermissionIds]); useEffect(() => { - if (!loadingRoleUsers && formRef.current && roleUsers.length >= 0) { + if (!stableGroupIds.length) { + setRoleGroups([]); + setLoadingRoleGroups(false); + return; + } + + setLoadingRoleGroups(true); + const filters = [{ col: 'id', opr: 'in', value: stableGroupIds }]; + + fetchPaginatedData({ + endpoint: `/api/v1/security/groups/`, + pageSize: 100, + setData: setRoleGroups, + filters, + setLoadingState: (loading: boolean) => setLoadingRoleGroups(loading), + loadingKey: 'roleGroups', + addDangerToast, + errorMessage: t('There was an error loading groups.'), + mapResult: (group: { id: number; name: string }) => ({ + value: group.id, + label: group.name, + }), + }); + }, [addDangerToast, stableGroupIds, id]); + + useEffect(() => { + if (!loadingRoleUsers && formRef.current) { const userOptions = roleUsers.map(user => ({ value: user.id, label: user.username, @@ -146,14 +215,63 @@ function RoleListEditModal({ } }, [loadingRoleUsers, roleUsers]); + useEffect(() => { + if ( + !loadingRolePermissions && + formRef.current && + rolePermissions.length > 0 + ) { + const fetchedIds = new Set(rolePermissions.map(p => p.value)); + const missingIds = stablePermissionIds.filter(id => !fetchedIds.has(id)); + const allPermissions = [ + ...rolePermissions, + ...missingIds.map(id => ({ value: id, label: String(id) })), + ]; + if (missingIds.length > 0) { + addDangerToast( + t('Some permissions could not be resolved and are shown as IDs.'), + ); + } + formRef.current.setFieldsValue({ + rolePermissions: allPermissions, + }); + } + }, [loadingRolePermissions, rolePermissions, stablePermissionIds, addDangerToast]); + + useEffect(() => { + if (!loadingRoleGroups && formRef.current && roleGroups.length > 0) { + const fetchedIds = new Set(roleGroups.map(g => g.value)); + const missingIds = stableGroupIds.filter(id => !fetchedIds.has(id)); + const allGroups = [ + ...roleGroups, + ...missingIds.map(id => ({ value: id, label: String(id) })), + ]; + if (missingIds.length > 0) { + addDangerToast( + t('Some groups could not be resolved and are shown as IDs.'), + ); + } + formRef.current.setFieldsValue({ + roleGroups: allGroups, + }); + } + }, [loadingRoleGroups, roleGroups, stableGroupIds, addDangerToast]); + + const mapSelectedIds = (options?: Array<SelectOption | number>) => + options?.map(option => + typeof option === 'number' ? option : option.value, + ) || []; + const handleFormSubmit = async (values: RoleForm) => { try { const userIds = values.roleUsers?.map(user => user.value) || []; + const permissionIds = mapSelectedIds(values.rolePermissions); + const groupIds = mapSelectedIds(values.roleGroups); await Promise.all([ updateRoleName(id, values.roleName), - updateRolePermissions(id, values.rolePermissions), + updateRolePermissions(id, permissionIds), updateRoleUsers(id, userIds), - updateRoleGroups(id, values.roleGroups), + updateRoleGroups(id, groupIds), ]); addSuccessToast(t('The role has been updated successfully.')); } catch (err) { @@ -166,13 +284,19 @@ function RoleListEditModal({ const initialValues = { roleName: name, - rolePermissions: permission_ids, + rolePermissions: permission_ids.map(permissionId => ({ + value: permissionId, + label: String(permissionId), + })), roleUsers: roleUsers?.map(user => ({ value: user.id, label: user.username, })) || [], - roleGroups: group_ids, + roleGroups: group_ids.map(groupId => ({ + value: groupId, + label: String(groupId), + })), }; return ( @@ -206,12 +330,18 @@ function RoleListEditModal({ > <> <RoleNameField /> - <PermissionsField permissions={permissions} /> + <PermissionsField + addDangerToast={addDangerToast} + loading={loadingRolePermissions} + /> <UsersField addDangerToast={addDangerToast} loading={loadingRoleUsers} /> - <GroupsField groups={groups} /> + <GroupsField + addDangerToast={addDangerToast} + loading={loadingRoleGroups} + /> </> </Tabs.TabPane> <Tabs.TabPane tab={roleTabs.users.name} key={roleTabs.users.key}> diff --git a/superset-frontend/src/features/roles/types.ts b/superset-frontend/src/features/roles/types.ts index 2e9325d4ef1..c21b17d9ab8 100644 --- a/superset-frontend/src/features/roles/types.ts +++ b/superset-frontend/src/features/roles/types.ts @@ -26,12 +26,6 @@ export type PermissionResource = { view_menu: PermissionView; }; -export type FormattedPermission = { - label: string; - value: string; - id: number; -}; - export type RolePermissions = { id: number; permission_name: string; @@ -60,9 +54,9 @@ export type RoleInfo = { export type RoleForm = { roleName: string; - rolePermissions: number[]; - roleUsers: SelectOption[]; - roleGroups: number[]; + rolePermissions?: SelectOption[]; + roleUsers?: SelectOption[]; + roleGroups?: SelectOption[]; }; export interface BaseModalProps { diff --git a/superset-frontend/src/features/roles/utils.test.ts b/superset-frontend/src/features/roles/utils.test.ts new file mode 100644 index 00000000000..fa70c3549bb --- /dev/null +++ b/superset-frontend/src/features/roles/utils.test.ts @@ -0,0 +1,271 @@ +/** + * 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 { SupersetClient } from '@superset-ui/core'; +import rison from 'rison'; +import { + clearPermissionSearchCache, + fetchGroupOptions, + fetchPermissionOptions, +} from './utils'; + +const getMock = jest.spyOn(SupersetClient, 'get'); + +afterEach(() => { + getMock.mockReset(); + clearPermissionSearchCache(); +}); + +test('fetchPermissionOptions fetches all results on page 0 with large page_size', async () => { + getMock.mockResolvedValue({ + json: { + count: 1, + result: [ + { + id: 10, + permission: { name: 'can_access' }, + view_menu: { name: 'dataset_one' }, + }, + ], + }, + } as any); + const addDangerToast = jest.fn(); + + const result = await fetchPermissionOptions('dataset', 0, 50, addDangerToast); + + // Two parallel requests with large page_size for full fetch + expect(getMock).toHaveBeenCalledTimes(2); + + const calls = getMock.mock.calls.map( + ([call]) => (call as { endpoint: string }).endpoint, + ); + const queries = calls.map(ep => rison.decode(ep.split('?q=')[1])); + + expect(queries).toContainEqual({ + page: 0, + page_size: 5000, + filters: [{ col: 'view_menu.name', opr: 'ct', value: 'dataset' }], + }); + expect(queries).toContainEqual({ + page: 0, + page_size: 5000, + filters: [{ col: 'permission.name', opr: 'ct', value: 'dataset' }], + }); + + // Duplicates are removed; both calls return id=10 so result has one entry + expect(result).toEqual({ + data: [{ value: 10, label: 'can access dataset one' }], + totalCount: 1, + }); + expect(addDangerToast).not.toHaveBeenCalled(); +}); + +test('fetchPermissionOptions serves cached slices on subsequent pages', async () => { + // Seed cache with page 0 + let callCount = 0; + getMock.mockImplementation(() => { + callCount += 1; + if (callCount === 1) { + return Promise.resolve({ + json: { + count: 3, + result: [ + { id: 1, permission: { name: 'a' }, view_menu: { name: 'X' } }, + { id: 2, permission: { name: 'b' }, view_menu: { name: 'Y' } }, + { id: 3, permission: { name: 'c' }, view_menu: { name: 'Z' } }, + ], + }, + } as any); + } + return Promise.resolve({ json: { count: 0, result: [] } } as any); + }); + const addDangerToast = jest.fn(); + + // Page 0 populates the cache + await fetchPermissionOptions('test', 0, 2, addDangerToast); + expect(getMock).toHaveBeenCalledTimes(2); + + getMock.mockReset(); + + // Page 1 should serve from cache with zero API calls + const page1 = await fetchPermissionOptions('test', 1, 2, addDangerToast); + expect(getMock).not.toHaveBeenCalled(); + expect(page1).toEqual({ + data: [{ value: 3, label: 'c Z' }], + totalCount: 3, + }); +}); + +test('fetchPermissionOptions makes single request when search term is empty', async () => { + getMock.mockResolvedValue({ + json: { count: 0, result: [] }, + } as any); + const addDangerToast = jest.fn(); + + await fetchPermissionOptions('', 0, 100, addDangerToast); + + expect(getMock).toHaveBeenCalledTimes(1); + const { endpoint } = getMock.mock.calls[0][0] as { endpoint: string }; + const queryString = endpoint.split('?q=')[1]; + expect(rison.decode(queryString)).toEqual({ + page: 0, + page_size: 100, + }); +}); + +test('fetchPermissionOptions fires single toast when both requests fail', async () => { + getMock.mockRejectedValue(new Error('request failed')); + const addDangerToast = jest.fn(); + + const result = await fetchPermissionOptions( + 'dataset', + 0, + 100, + addDangerToast, + ); + + expect(addDangerToast).toHaveBeenCalledTimes(1); + expect(addDangerToast).toHaveBeenCalledWith( + 'There was an error while fetching permissions', + ); + expect(result).toEqual({ data: [], totalCount: 0 }); +}); + +test('fetchPermissionOptions deduplicates results from both columns', async () => { + const sharedResult = { + id: 5, + permission: { name: 'can_read' }, + view_menu: { name: 'ChartView' }, + }; + const viewMenuOnly = { + id: 6, + permission: { name: 'can_write' }, + view_menu: { name: 'ChartView' }, + }; + const permissionOnly = { + id: 7, + permission: { name: 'can_read' }, + view_menu: { name: 'DashboardView' }, + }; + + let callCount = 0; + getMock.mockImplementation(() => { + callCount += 1; + if (callCount === 1) { + // view_menu.name search returns shared + viewMenuOnly + return Promise.resolve({ + json: { count: 2, result: [sharedResult, viewMenuOnly] }, + } as any); + } + // permission.name search returns shared + permissionOnly + return Promise.resolve({ + json: { count: 2, result: [sharedResult, permissionOnly] }, + } as any); + }); + + const addDangerToast = jest.fn(); + const result = await fetchPermissionOptions('chart', 0, 100, addDangerToast); + + // id=5 appears in both results but should be deduplicated + expect(result.data).toEqual([ + { value: 5, label: 'can read ChartView' }, + { value: 6, label: 'can write ChartView' }, + { value: 7, label: 'can read DashboardView' }, + ]); + // totalCount reflects deduplicated cache length + expect(result.totalCount).toBe(3); +}); + +test('fetchPermissionOptions clears cache when search is empty', async () => { + // First, populate cache with a search + getMock.mockResolvedValue({ + json: { + count: 1, + result: [{ id: 1, permission: { name: 'a' }, view_menu: { name: 'X' } }], + }, + } as any); + const addDangerToast = jest.fn(); + await fetchPermissionOptions('test', 0, 50, addDangerToast); + expect(getMock).toHaveBeenCalledTimes(2); + getMock.mockReset(); + + // Empty search should clear cache and make a fresh request + getMock.mockResolvedValue({ json: { count: 0, result: [] } } as any); + await fetchPermissionOptions('', 0, 50, addDangerToast); + expect(getMock).toHaveBeenCalledTimes(1); +}); + +test('fetchGroupOptions sends filters array with search term', async () => { + getMock.mockResolvedValue({ + json: { + count: 2, + result: [ + { id: 1, name: 'Engineering' }, + { id: 2, name: 'Analytics' }, + ], + }, + } as any); + const addDangerToast = jest.fn(); + + const result = await fetchGroupOptions('eng', 1, 25, addDangerToast); + + expect(getMock).toHaveBeenCalledTimes(1); + const { endpoint } = getMock.mock.calls[0][0] as { endpoint: string }; + const queryString = endpoint.split('?q=')[1]; + expect(rison.decode(queryString)).toEqual({ + page: 1, + page_size: 25, + filters: [{ col: 'name', opr: 'ct', value: 'eng' }], + }); + expect(result).toEqual({ + data: [ + { value: 1, label: 'Engineering' }, + { value: 2, label: 'Analytics' }, + ], + totalCount: 2, + }); + expect(addDangerToast).not.toHaveBeenCalled(); +}); + +test('fetchGroupOptions omits filters when search term is empty', async () => { + getMock.mockResolvedValue({ + json: { count: 0, result: [] }, + } as any); + const addDangerToast = jest.fn(); + + await fetchGroupOptions('', 0, 100, addDangerToast); + + const { endpoint } = getMock.mock.calls[0][0] as { endpoint: string }; + const queryString = endpoint.split('?q=')[1]; + expect(rison.decode(queryString)).toEqual({ + page: 0, + page_size: 100, + }); +}); + +test('fetchGroupOptions returns empty options on error', async () => { + getMock.mockRejectedValue(new Error('request failed')); + const addDangerToast = jest.fn(); + + const result = await fetchGroupOptions('eng', 0, 100, addDangerToast); + + expect(addDangerToast).toHaveBeenCalledWith( + 'There was an error while fetching groups', + ); + expect(result).toEqual({ data: [], totalCount: 0 }); +}); diff --git a/superset-frontend/src/features/roles/utils.ts b/superset-frontend/src/features/roles/utils.ts index 6e96768c47e..e654e0bfc62 100644 --- a/superset-frontend/src/features/roles/utils.ts +++ b/superset-frontend/src/features/roles/utils.ts @@ -18,6 +18,9 @@ */ import { SupersetClient } from '@superset-ui/core'; +import { t } from '@apache-superset/core'; +import rison from 'rison'; +import { SelectOption } from './types'; export const createRole = async (name: string) => SupersetClient.post({ @@ -51,3 +54,127 @@ export const updateRoleName = async (roleId: number, name: string) => endpoint: `/api/v1/security/roles/${roleId}`, jsonPayload: { name }, }); + +export const formatPermissionLabel = ( + permissionName: string, + viewMenuName: string, +) => `${permissionName.replace(/_/g, ' ')} ${viewMenuName.replace(/_/g, ' ')}`; + +type PermissionResult = { + id: number; + permission: { name: string }; + view_menu: { name: string }; +}; + +const mapPermissionResults = (results: PermissionResult[]) => + results.map(item => ({ + value: item.id, + label: formatPermissionLabel(item.permission.name, item.view_menu.name), + })); + +const MAX_PERMISSION_SEARCH_SIZE = 5000; +const permissionSearchCache = new Map<string, SelectOption[]>(); + +export const clearPermissionSearchCache = () => { + permissionSearchCache.clear(); +}; + +const fetchPermissionPageRaw = async ( + queryParams: Record<string, unknown>, +) => { + const response = await SupersetClient.get({ + endpoint: `/api/v1/security/permissions-resources/?q=${rison.encode(queryParams)}`, + }); + return { + data: mapPermissionResults(response.json?.result || []), + totalCount: response.json?.count ?? 0, + }; +}; + +export const fetchPermissionOptions = async ( + filterValue: string, + page: number, + pageSize: number, + addDangerToast: (msg: string) => void, +) => { + if (!filterValue) { + permissionSearchCache.clear(); + try { + return await fetchPermissionPageRaw({ page, page_size: pageSize }); + } catch { + addDangerToast(t('There was an error while fetching permissions')); + return { data: [], totalCount: 0 }; + } + } + + try { + let cached = permissionSearchCache.get(filterValue); + if (!cached) { + const searchQuery = { page: 0, page_size: MAX_PERMISSION_SEARCH_SIZE }; + const [byViewMenu, byPermission] = await Promise.all([ + fetchPermissionPageRaw({ + ...searchQuery, + filters: [ + { col: 'view_menu.name', opr: 'ct', value: filterValue }, + ], + }), + fetchPermissionPageRaw({ + ...searchQuery, + filters: [ + { col: 'permission.name', opr: 'ct', value: filterValue }, + ], + }), + ]); + + const seen = new Set<number>(); + cached = [...byViewMenu.data, ...byPermission.data].filter(item => { + if (seen.has(item.value)) return false; + seen.add(item.value); + return true; + }); + permissionSearchCache.set(filterValue, cached); + } + + const start = page * pageSize; + return { + data: cached.slice(start, start + pageSize), + totalCount: cached.length, + }; + } catch { + addDangerToast(t('There was an error while fetching permissions')); + return { data: [], totalCount: 0 }; + } +}; + +export const fetchGroupOptions = async ( + filterValue: string, + page: number, + pageSize: number, + addDangerToast: (msg: string) => void, +) => { + const query = rison.encode({ + page, + page_size: pageSize, + ...(filterValue + ? { filters: [{ col: 'name', opr: 'ct', value: filterValue }] } + : {}), + }); + + try { + const response = await SupersetClient.get({ + endpoint: `/api/v1/security/groups/?q=${query}`, + }); + + const results = response.json?.result || []; + return { + data: results.map((group: { id: number; name: string }) => ({ + value: group.id, + label: group.name, + })), + totalCount: response.json?.count ?? 0, + }; + } catch { + addDangerToast(t('There was an error while fetching groups')); + return { data: [], totalCount: 0 }; + } +}; diff --git a/superset-frontend/src/pages/RolesList/RolesList.test.tsx b/superset-frontend/src/pages/RolesList/RolesList.test.tsx index 23a953dbde5..39d8069429c 100644 --- a/superset-frontend/src/pages/RolesList/RolesList.test.tsx +++ b/superset-frontend/src/pages/RolesList/RolesList.test.tsx @@ -38,6 +38,7 @@ const store = mockStore({}); const rolesEndpoint = 'glob:*/security/roles/search/?*'; const roleEndpoint = 'glob:*/api/v1/security/roles/*'; const permissionsEndpoint = 'glob:*/api/v1/security/permissions-resources/?*'; +const groupsEndpoint = 'glob:*/api/v1/security/groups/?*'; const usersEndpoint = 'glob:*/api/v1/security/users/?*'; const mockRoles = Array.from({ length: 3 }, (_, i) => ({ @@ -45,12 +46,7 @@ const mockRoles = Array.from({ length: 3 }, (_, i) => ({ name: `role ${i}`, user_ids: [i, i + 1], permission_ids: [i, i + 1, i + 2], -})); - -const mockPermissions = Array.from({ length: 10 }, (_, i) => ({ - id: i, - permission: { name: `permission_${i}` }, - view_menu: { name: `view_menu_${i}` }, + group_ids: [i, i + 10], })); const mockUsers = Array.from({ length: 5 }, (_, i) => ({ @@ -88,15 +84,18 @@ fetchMock.get(rolesEndpoint, { count: 3, }); -fetchMock.get(permissionsEndpoint, { - count: mockPermissions.length, - result: mockPermissions, -}); - fetchMock.get(usersEndpoint, { count: mockUsers.length, result: mockUsers, }); +fetchMock.get(permissionsEndpoint, { + count: 0, + result: [], +}); +fetchMock.get(groupsEndpoint, { + count: 0, + result: [], +}); fetchMock.delete(roleEndpoint, {}); fetchMock.put(roleEndpoint, {}); @@ -139,11 +138,13 @@ describe('RolesList', () => { }); }); - test('fetches permissions on load', async () => { + test('does not fetch permissions or groups on load', async () => { await renderAndWait(); await waitFor(() => { const permissionCalls = fetchMock.callHistory.calls(permissionsEndpoint); - expect(permissionCalls.length).toBeGreaterThan(0); + const groupCalls = fetchMock.callHistory.calls(groupsEndpoint); + expect(permissionCalls.length).toBe(0); + expect(groupCalls.length).toBe(0); }); }); diff --git a/superset-frontend/src/pages/RolesList/index.tsx b/superset-frontend/src/pages/RolesList/index.tsx index af309fdbfb9..68978457a9d 100644 --- a/superset-frontend/src/pages/RolesList/index.tsx +++ b/superset-frontend/src/pages/RolesList/index.tsx @@ -17,7 +17,7 @@ * under the License. */ -import { useCallback, useEffect, useMemo, useState } from 'react'; +import { useMemo, useState } from 'react'; import { t } from '@apache-superset/core'; import { SupersetClient } from '@superset-ui/core'; import { useListViewResource } from 'src/views/CRUD/hooks'; @@ -35,13 +35,15 @@ import { type ListViewActionProps, type ListViewFilters, } from 'src/components'; -import { FormattedPermission, UserObject } from 'src/features/roles/types'; +import { UserObject } from 'src/features/roles/types'; import { isUserAdmin } from 'src/dashboard/util/permissionUtils'; import { Icons } from '@superset-ui/core/components/Icons'; -import { fetchPaginatedData } from 'src/utils/fetchOptions'; import { fetchUserOptions } from 'src/features/groups/utils'; +import { + fetchGroupOptions, + fetchPermissionOptions, +} from 'src/features/roles/utils'; import { WIDER_DROPDOWN_WIDTH } from 'src/components/ListView/utils'; -import { GroupObject } from '../GroupsList'; const PAGE_SIZE = 25; @@ -101,50 +103,9 @@ function RolesList({ addDangerToast, addSuccessToast, user }: RolesListProps) { const [currentRole, setCurrentRole] = useState<RoleObject | null>(null); const [roleCurrentlyDeleting, setRoleCurrentlyDeleting] = useState<RoleObject | null>(null); - const [permissions, setPermissions] = useState<FormattedPermission[]>([]); - const [groups, setGroups] = useState<GroupObject[]>([]); - const [loadingState, setLoadingState] = useState({ - permissions: true, - groups: true, - }); const isAdmin = useMemo(() => isUserAdmin(user), [user]); - const fetchPermissions = useCallback(() => { - fetchPaginatedData({ - endpoint: '/api/v1/security/permissions-resources/', - setData: setPermissions, - setLoadingState, - loadingKey: 'permissions', - addDangerToast, - errorMessage: 'Error while fetching permissions', - mapResult: ({ permission, view_menu, id }) => ({ - label: `${permission.name.replace(/_/g, ' ')} ${view_menu.name.replace(/_/g, ' ')}`, - value: `${permission.name}__${view_menu.name}`, - id, - }), - }); - }, [addDangerToast]); - - const fetchGroups = useCallback(() => { - fetchPaginatedData({ - endpoint: '/api/v1/security/groups/', - setData: setGroups, - setLoadingState, - loadingKey: 'groups', - addDangerToast, - errorMessage: t('Error while fetching groups'), - }); - }, [addDangerToast]); - - useEffect(() => { - fetchPermissions(); - }, [fetchPermissions]); - - useEffect(() => { - fetchGroups(); - }, [fetchGroups]); - const handleRoleDelete = async ({ id, name }: RoleObject) => { try { await SupersetClient.delete({ @@ -209,7 +170,7 @@ function RolesList({ addDangerToast, addSuccessToast, user }: RolesListProps) { id: 'group_ids', Header: t('Groups'), hidden: true, - Cell: ({ row: { original } }: any) => original.groups_ids.join(', '), + Cell: ({ row: { original } }: any) => original.group_ids.join(', '), }, { accessor: 'permission_ids', @@ -287,7 +248,6 @@ function RolesList({ addDangerToast, addSuccessToast, user }: RolesListProps) { onClick: () => { openModal(ModalType.ADD); }, - loading: loadingState.permissions, 'data-test': 'add-role-button', }, ); @@ -321,11 +281,8 @@ function RolesList({ addDangerToast, addSuccessToast, user }: RolesListProps) { input: 'select', operator: FilterOperator.RelationOneMany, unfilteredLabel: t('All'), - selects: permissions?.map(permission => ({ - label: permission.label, - value: permission.id, - })), - loading: loadingState.permissions, + fetchSelects: async (filterValue, page, pageSize) => + fetchPermissionOptions(filterValue, page, pageSize, addDangerToast), dropdownStyle: { minWidth: WIDER_DROPDOWN_WIDTH }, }, { @@ -335,15 +292,12 @@ function RolesList({ addDangerToast, addSuccessToast, user }: RolesListProps) { input: 'select', operator: FilterOperator.RelationOneMany, unfilteredLabel: t('All'), - selects: groups?.map(group => ({ - label: group.name, - value: group.id, - })), - loading: loadingState.groups, + fetchSelects: async (filterValue, page, pageSize) => + fetchGroupOptions(filterValue, page, pageSize, addDangerToast), dropdownStyle: { minWidth: WIDER_DROPDOWN_WIDTH }, }, ], - [permissions, groups, loadingState.groups, loadingState.permissions], + [addDangerToast], ); const emptyState = { @@ -372,7 +326,6 @@ function RolesList({ addDangerToast, addSuccessToast, user }: RolesListProps) { refreshData(); closeModal(ModalType.ADD); }} - permissions={permissions} /> {modalState.edit && currentRole && ( <RoleListEditModal @@ -383,8 +336,6 @@ function RolesList({ addDangerToast, addSuccessToast, user }: RolesListProps) { refreshData(); closeModal(ModalType.EDIT); }} - permissions={permissions} - groups={groups} /> )} {modalState.duplicate && currentRole && (
