diff --git a/backend/src/routes/api/groups-config/groupsConfigUtil.ts b/backend/src/routes/api/groups-config/groupsConfigUtil.ts index 488993efb8..44b20e348b 100644 --- a/backend/src/routes/api/groups-config/groupsConfigUtil.ts +++ b/backend/src/routes/api/groups-config/groupsConfigUtil.ts @@ -16,19 +16,12 @@ const SYSTEM_AUTHENTICATED = 'system:authenticated'; export const getGroupsConfig = async (fastify: KubeFastifyInstance): Promise => { const customObjectsApi = fastify.kube.customObjectsApi; - try { - const groupsCluster = await getAllGroups(customObjectsApi); - const groupsData = getGroupsCR(); - const groupsProcessed = processGroupData(groupsData); - const groupsConfigProcessed = processGroupConfig(groupsProcessed, groupsCluster); - await removeDeletedGroups(fastify, groupsData, groupsConfigProcessed.groupsCRData); - - return groupsConfigProcessed.groupsConfig; - } catch (e) { - fastify.log.error(e, 'Error retrieving group configuration.'); - const error = createError(500, 'Error retrieving group configuration'); - throw error; - } + const groupsCluster = await getAllGroups(customObjectsApi); + const groupsData = getGroupsCR(); + const groupsProcessed = processGroupData(groupsData); + const groupsConfigProcessed = processGroupConfig(fastify, groupsProcessed, groupsCluster); + await removeDeletedGroups(fastify, groupsData, groupsConfigProcessed.groupsCRData); + return groupsConfigProcessed.groupsConfig; }; const transformGroupsConfig = (groupStatus: GroupStatus[]): string[] => { @@ -38,7 +31,7 @@ const transformGroupsConfig = (groupStatus: GroupStatus[]): string[] => { export const updateGroupsConfig = async ( fastify: KubeFastifyInstance, request: FastifyRequest<{ Body: GroupsConfig }>, -): Promise<{ success: GroupsConfig | null; error: string | null }> => { +): Promise => { const customObjectsApi = fastify.kube.customObjectsApi; const { namespace } = fastify.kube; @@ -58,26 +51,17 @@ export const updateGroupsConfig = async ( const error = createError(403, 'Error, groups cannot be empty'); throw error; } - try { - const dataUpdated: GroupsConfigBody = { - adminGroups: adminConfig.join(','), - allowedGroups: allowedConfig.join(','), - }; - - const groupsData = await updateGroupsCR(fastify, dataUpdated); - const groupsProcessed = processGroupData(groupsData); - const groupsCluster = await getAllGroups(customObjectsApi); - const updatedConfig = processGroupConfig(groupsProcessed, groupsCluster); - await removeDeletedGroups(fastify, groupsData, updatedConfig.groupsCRData); - return { - success: updatedConfig.groupsConfig, - error: null, - }; - } catch (e) { - fastify.log.error(e, 'Error updating group configuration.'); - const error = createError(500, 'Error updating group configuration'); - throw error; - } + const dataUpdated: GroupsConfigBody = { + adminGroups: adminConfig.join(','), + allowedGroups: allowedConfig.join(','), + }; + + const groupsData = await updateGroupsCR(fastify, dataUpdated); + const groupsProcessed = processGroupData(groupsData); + const groupsCluster = await getAllGroups(customObjectsApi); + const updatedConfig = processGroupConfig(fastify, groupsProcessed, groupsCluster); + await removeDeletedGroups(fastify, groupsData, updatedConfig.groupsCRData); + return updatedConfig.groupsConfig; }; const processGroupData = (groupsData: GroupsConfigBody): GroupsConfigBodyList => { @@ -105,6 +89,7 @@ const mapListToGroupStatus = * @returns Processed object with the groups, removing missing groups that might be selected */ const processGroupConfig = ( + fastify: KubeFastifyInstance, groupsDataList: GroupsConfigBodyList, groups: string[], ): { groupsConfig: GroupsConfig; groupsCRData: GroupsConfigBody } => { @@ -120,9 +105,14 @@ const processGroupConfig = ( const groupsConfig: GroupsConfig = { adminGroups: adminGroupsConfig, allowedGroups: allowedGroupsConfig, - errorAdmin: getError(groupsDataList.adminGroups, (group) => !groups.includes(group)), + errorAdmin: getError( + fastify, + groupsDataList.adminGroups.filter((group) => group), + (group) => !groups.includes(group), + ), errorUser: getError( - groupsDataList.allowedGroups, + fastify, + groupsDataList.allowedGroups.filter((group) => group), (group) => !groups.includes(group) && group !== SYSTEM_AUTHENTICATED, ), }; @@ -137,13 +127,26 @@ const processGroupConfig = ( return { groupsConfig, groupsCRData: updatedBody }; }; -const getError = (array: string[], predicate: (group: string) => boolean): string | undefined => { +const getError = ( + fastify: KubeFastifyInstance, + array: string[], + predicate: (group: string) => boolean, +): string | undefined => { + let error; + if (array.length === 0) { + error = 'No group is set in the group config, please set one or more group.'; + fastify.log.error(error); + return error; + } + const missingItems = array.filter(predicate); if (missingItems.length === 0) return undefined; - return `The group${missingItems.length === 1 ? '' : 's'} ${missingItems.join( + error = `The group${missingItems.length === 1 ? '' : 's'} ${missingItems.join( ', ', )} no longer exists in OpenShift and has been removed from the selected group list.`; + fastify.log.error(error); + return error; }; /** diff --git a/backend/src/routes/api/groups-config/index.ts b/backend/src/routes/api/groups-config/index.ts index e2b07c58ac..9b70b5e59e 100644 --- a/backend/src/routes/api/groups-config/index.ts +++ b/backend/src/routes/api/groups-config/index.ts @@ -6,8 +6,13 @@ import { secureAdminRoute } from '../../../utils/route-security'; export default async (fastify: FastifyInstance): Promise => { fastify.get( '/', - secureAdminRoute(fastify)(async () => { - return getGroupsConfig(fastify); + secureAdminRoute(fastify)(async (request, reply) => { + return getGroupsConfig(fastify).catch((e) => { + fastify.log.error( + `Error retrieving group configuration, ${e.response?.body?.message || e.message}`, + ); + reply.status(500).send({ message: e.response?.body?.message || e.message }); + }); }), ); diff --git a/backend/src/utils/groupsUtils.ts b/backend/src/utils/groupsUtils.ts index cca1ad8077..2cfc266b7a 100644 --- a/backend/src/utils/groupsUtils.ts +++ b/backend/src/utils/groupsUtils.ts @@ -16,7 +16,7 @@ export class MissingGroupError extends Error { } export const getGroupsCR = (): GroupsConfigBody => { - if (typeof getDashboardConfig().spec.groupsConfig !== 'undefined') { + if (getDashboardConfig().spec.groupsConfig) { return getDashboardConfig().spec.groupsConfig; } throw new Error(`Failed to retrieve Dashboard CR groups configuration`); diff --git a/frontend/src/services/groupSettingsService.ts b/frontend/src/services/groupSettingsService.ts index 1704c8518f..2e957a342d 100644 --- a/frontend/src/services/groupSettingsService.ts +++ b/frontend/src/services/groupSettingsService.ts @@ -11,9 +11,7 @@ export const fetchGroupsSettings = (): Promise => { }); }; -export const updateGroupsSettings = ( - settings: GroupsConfig, -): Promise<{ success: GroupsConfig | null; error: string | null }> => { +export const updateGroupsSettings = (settings: GroupsConfig): Promise => { const url = '/api/groups-config'; return axios .put(url, settings) diff --git a/frontend/src/utilities/__tests__/useWatchGroups.spec.ts b/frontend/src/utilities/__tests__/useWatchGroups.spec.ts new file mode 100644 index 0000000000..1474cfb4d8 --- /dev/null +++ b/frontend/src/utilities/__tests__/useWatchGroups.spec.ts @@ -0,0 +1,34 @@ +import { testHook } from '~/__tests__/unit/testUtils/hooks'; +import { fetchGroupsSettings } from '~/services/groupSettingsService'; +import { useWatchGroups } from '~/utilities/useWatchGroups'; + +jest.mock('~/services/groupSettingsService', () => ({ + fetchGroupsSettings: jest.fn(), +})); + +jest.mock('react-redux', () => ({ + useDispatch: jest.fn(), +})); + +const fetchGroupSettingsMock = fetchGroupsSettings as jest.Mock; + +describe('useWatchGroups', () => { + it('should fetch groups successfully', async () => { + const mockEmptyGroupSettings = { + adminGroups: [], + allowedGroups: [], + }; + const mockGroupSettings = { + adminGroups: ['odh-admins'], + allowedGroups: [], + }; + fetchGroupSettingsMock.mockReturnValue(Promise.resolve(mockGroupSettings)); + + const renderResult = testHook(useWatchGroups)(); + expect(fetchGroupSettingsMock).toHaveBeenCalledTimes(1); + expect(renderResult.result.current.groupSettings).toStrictEqual(mockEmptyGroupSettings); + + await renderResult.waitForNextUpdate(); + expect(renderResult.result.current.groupSettings).toStrictEqual(mockGroupSettings); + }); +}); diff --git a/frontend/src/utilities/useWatchGroups.tsx b/frontend/src/utilities/useWatchGroups.tsx index 66311036e4..46bb7aadfc 100644 --- a/frontend/src/utilities/useWatchGroups.tsx +++ b/frontend/src/utilities/useWatchGroups.tsx @@ -48,10 +48,10 @@ export const useWatchGroups = (): { React.useEffect(() => { if (errorAdmin) { - notification.error(`Group no longer exists`, errorAdmin); + notification.error(`Group error`, errorAdmin); } if (errorUser) { - notification.error(`Group no longer exists`, errorUser); + notification.error(`Group error`, errorUser); } }, [errorAdmin, errorUser, notification]); @@ -59,13 +59,11 @@ export const useWatchGroups = (): { setIsLoading(true); updateGroupsSettings(group) .then((response) => { - if (response.success) { - setGroupSettings(response.success); - notification.success( - 'Group settings changes saved', - 'It may take up to 2 minutes for configuration changes to be applied.', - ); - } + setGroupSettings(response); + notification.success( + 'Group settings changes saved', + 'It may take up to 2 minutes for configuration changes to be applied.', + ); }) .catch((error) => { setLoadError(error);