diff --git a/frontend/src/api/useRulesReview.ts b/frontend/src/api/useRulesReview.ts index 8b738d0e28..7e6119b29d 100644 --- a/frontend/src/api/useRulesReview.ts +++ b/frontend/src/api/useRulesReview.ts @@ -19,11 +19,12 @@ const checkAccess = (ns: string): Promise => { export const useRulesReview = ( namespace: string, -): [SelfSubjectRulesReviewKind['status'], boolean] => { +): [SelfSubjectRulesReviewKind['status'], boolean, () => void] => { const [loaded, setLoaded] = React.useState(false); const [status, setStatus] = React.useState(undefined); - React.useEffect(() => { + const refreshRulesReview = React.useCallback(() => { + setLoaded(false); checkAccess(namespace) .then((result) => { if (!result.status?.incomplete && !result.status?.evaluationError) { @@ -38,5 +39,9 @@ export const useRulesReview = ( }); }, [namespace]); - return [status, loaded]; + React.useEffect(() => { + refreshRulesReview(); + }, [refreshRulesReview]); + + return [status, loaded, refreshRulesReview]; }; diff --git a/frontend/src/concepts/modelRegistry/apiHooks/__tests__/useModelRegistryServices.spec.ts b/frontend/src/concepts/modelRegistry/apiHooks/__tests__/useModelRegistryServices.spec.ts index e07936c3a5..6f4b918c77 100644 --- a/frontend/src/concepts/modelRegistry/apiHooks/__tests__/useModelRegistryServices.spec.ts +++ b/frontend/src/concepts/modelRegistry/apiHooks/__tests__/useModelRegistryServices.spec.ts @@ -1,9 +1,12 @@ import { k8sGetResource } from '@openshift/dynamic-plugin-sdk-utils'; -import { act } from 'react-dom/test-utils'; +import { act, waitFor } from '@testing-library/react'; import { useAccessReview, useRulesReview, listServices } from '~/api'; import { ServiceKind } from '~/k8sTypes'; -import { useModelRegistryServices } from '~/concepts/modelRegistry/apiHooks/useModelRegistryServices'; -import { standardUseFetchState, testHook } from '~/__tests__/unit/testUtils/hooks'; +import { + useModelRegistryServices, + ModelRegistryServicesResult, +} from '~/concepts/modelRegistry/apiHooks/useModelRegistryServices'; +import { testHook } from '~/__tests__/unit/testUtils/hooks'; import { mockModelRegistryService } from '~/__mocks__/mockModelRegistryService'; jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({ @@ -22,102 +25,162 @@ jest.mock('~/concepts/modelRegistry/apiHooks/useModelRegistryServices', () => ({ fetchServices: jest.fn(), })); +jest.mock('~/api/useRulesReview', () => ({ + useRulesReview: jest.fn(), +})); + +jest.mock('~/api/useAccessReview'); +jest.mock('~/api/useRulesReview'); +const objectToStandardUseFetchState = (obj: ModelRegistryServicesResult) => [ + obj.modelRegistryServices, + obj.isLoaded, + obj.error, + obj.refreshRulesReview, +]; + const mockGetResource = jest.mocked(k8sGetResource); const mockListServices = jest.mocked(listServices); describe('useModelRegistryServices', () => { beforeEach(() => { jest.clearAllMocks(); + (useAccessReview as jest.Mock).mockReturnValue([false, true]); + (useRulesReview as jest.Mock).mockReturnValue([{ resourceRules: [] }, true, jest.fn()]); + }); + it('should return loading state initially', () => { + const { result } = testHook(() => useModelRegistryServices('namespace'))(); + + const [modelRegistryServices, isLoaded, error] = objectToStandardUseFetchState(result.current); + + expect(modelRegistryServices).toEqual([]); + expect(isLoaded).toBe(false); + expect(error).toBeUndefined(); + }); + it('returns services fetched by names when allowList is false', async () => { + (useAccessReview as jest.Mock).mockReturnValue([false, true]); + (useRulesReview as jest.Mock).mockReturnValue([ + { + resourceRules: [ + { + resources: ['services'], + verbs: ['get'], + resourceNames: ['service-1', 'service-2'], + }, + ], + }, + true, + jest.fn(), + ]); + mockGetResource.mockResolvedValueOnce( + mockModelRegistryService({ name: 'service-1', namespace: 'test-namespace' }), + ); + mockGetResource.mockResolvedValueOnce( + mockModelRegistryService({ name: 'service-2', namespace: 'test-namespace' }), + ); + + const { result } = testHook(() => useModelRegistryServices('test-namespace'))(); + + await waitFor(() => { + const [, isLoaded] = objectToStandardUseFetchState(result.current); + expect(isLoaded).toBe(true); + }); + const [services, isLoaded] = objectToStandardUseFetchState(result.current); + expect(services).toEqual([ + mockModelRegistryService({ name: 'service-1', namespace: 'test-namespace' }), + mockModelRegistryService({ name: 'service-2', namespace: 'test-namespace' }), + ]); + expect(isLoaded).toBe(true); + expect(mockGetResource).toHaveBeenCalledTimes(2); + expect(mockGetResource).toHaveBeenCalledWith({ + queryOptions: { name: 'service-1', ns: 'odh-model-registries' }, + }); + expect(mockGetResource).toHaveBeenCalledWith({ + queryOptions: { name: 'service-2', ns: 'odh-model-registries' }, + }); }); - test('returns loading state initially', () => { - (useAccessReview as jest.Mock).mockReturnValue([false, false]); - (useRulesReview as jest.Mock).mockReturnValue([{}, false]); - const renderResult = testHook(useModelRegistryServices)(); + it('should initialize with empty array and loading state', () => { + const { result } = testHook(() => useModelRegistryServices('namespace'))(); + const [modelRegistryServices, isLoaded, error, refreshRulesReview] = + objectToStandardUseFetchState(result.current); - expect(renderResult.result.current[1]).toBe(false); - expect(renderResult.result.current[0]).toEqual([]); + expect(modelRegistryServices).toEqual([]); + expect(isLoaded).toBe(false); + expect(error).toBeUndefined(); + expect(refreshRulesReview).toEqual(expect.any(Function)); }); it('returns services when allowList is true', async () => { (useAccessReview as jest.Mock).mockReturnValue([true, true]); - (useRulesReview as jest.Mock).mockReturnValue([{}, false]); - (useRulesReview as jest.Mock).mockReturnValue([ - { incomplete: false, nonResourceRules: [], resourceRules: [] }, - true, - ] satisfies ReturnType); + (useRulesReview as jest.Mock).mockReturnValue([{ resourceRules: [] }, true, jest.fn()]); mockListServices.mockResolvedValue([ mockModelRegistryService({ name: 'service-1', namespace: 'test-namespace' }), ]); - const renderResult = testHook(useModelRegistryServices)(); - expect(mockListServices).toHaveBeenCalledTimes(1); - expect(renderResult).hookToStrictEqual(standardUseFetchState([])); - expect(renderResult).hookToHaveUpdateCount(1); + const { result } = testHook(() => useModelRegistryServices('namespace'))(); - // wait for update - await renderResult.waitForNextUpdate(); - expect(mockListServices).toHaveBeenCalledTimes(1); - expect(renderResult).hookToStrictEqual( - standardUseFetchState( - [mockModelRegistryService({ name: 'service-1', namespace: 'test-namespace' })], - true, - ), - ); - expect(renderResult).hookToHaveUpdateCount(2); - expect(renderResult).hookToBeStable([false, false, true, true]); + await act(async () => { + await new Promise((resolve) => { + setTimeout(resolve, 0); + }); + }); - // refresh - mockListServices.mockResolvedValue([ + const [services, isLoaded] = objectToStandardUseFetchState(result.current); + expect(services).toEqual([ mockModelRegistryService({ name: 'service-1', namespace: 'test-namespace' }), ]); - await act(() => renderResult.result.current[3]()); - expect(mockListServices).toHaveBeenCalledTimes(2); - expect(renderResult).hookToHaveUpdateCount(3); - expect(renderResult).hookToBeStable([false, true, true, true]); + expect(isLoaded).toBe(true); + expect(mockListServices).toHaveBeenCalledTimes(1); }); - it('returns services fetched by names when allowList is false', async () => { + it('returns empty array if no service names are provided', async () => { (useAccessReview as jest.Mock).mockReturnValue([false, true]); (useRulesReview as jest.Mock).mockReturnValue([ { resourceRules: [ { resources: ['services'], - verbs: ['get'], - resourceNames: ['service-1'], + verbs: ['use'], + resourceNames: [], }, ], }, true, + jest.fn(), ]); - mockGetResource.mockResolvedValue( - mockModelRegistryService({ name: 'service-1', namespace: 'test-namespace' }), - ); - const renderResult = testHook(useModelRegistryServices)(); - expect(mockGetResource).toHaveBeenCalledTimes(1); - expect(renderResult).hookToStrictEqual(standardUseFetchState([])); - expect(renderResult).hookToHaveUpdateCount(1); - - // wait for update - await renderResult.waitForNextUpdate(); - expect(mockGetResource).toHaveBeenCalledTimes(1); - expect(renderResult).hookToStrictEqual( - standardUseFetchState( - [mockModelRegistryService({ name: 'service-1', namespace: 'test-namespace' })], - true, - ), - ); - expect(renderResult).hookToHaveUpdateCount(2); - expect(renderResult).hookToBeStable([false, false, true, true]); + const { result } = testHook(() => useModelRegistryServices('namespace'))(); - // refresh - mockGetResource.mockResolvedValue(mockModelRegistryService({})); - await act(() => renderResult.result.current[3]()); - expect(mockGetResource).toHaveBeenCalledTimes(2); - expect(renderResult).hookToHaveUpdateCount(3); - expect(renderResult).hookToBeStable([false, true, true, true]); + await act(async () => { + await Promise.resolve(); + }); + + const [services, isLoaded] = objectToStandardUseFetchState(result.current); + expect(services).toEqual([]); + expect(isLoaded).toBe(true); + expect(mockGetResource).not.toHaveBeenCalled(); + }); + + it('should handle errors', async () => { + jest.useFakeTimers(); + const mockError = new Error('Test error'); + mockListServices.mockRejectedValue(mockError); + (useAccessReview as jest.Mock).mockReturnValue([true, true]); + (useRulesReview as jest.Mock).mockReturnValue([{ resourceRules: [] }, true, jest.fn()]); + + const { result } = testHook(() => useModelRegistryServices('namespace'))(); + + await act(async () => { + jest.runAllTimers(); + }); + + const [services, isLoaded, error] = objectToStandardUseFetchState(result.current); + + expect(services).toEqual([]); + expect(isLoaded).toBe(false); + expect(error).toBe(mockError); + + jest.useRealTimers(); }); test('returns empty array if no service names are provided', async () => { @@ -133,15 +196,19 @@ describe('useModelRegistryServices', () => { ], }, true, + jest.fn(), ]); mockGetResource.mockResolvedValue( mockModelRegistryService({ name: 'service-1', namespace: 'test-namespace' }), ); - const renderResult = testHook(useModelRegistryServices)(); - await renderResult.waitForNextUpdate(); - - expect(renderResult.result.current[0]).toEqual([]); - expect(renderResult.result.current[1]).toBe(true); + const { result } = testHook(() => useModelRegistryServices('namespace'))(); + await act(async () => { + await Promise.resolve(); + }); + + const [services, isLoaded] = objectToStandardUseFetchState(result.current); + expect(services).toEqual([]); + expect(isLoaded).toBe(true); expect(mockGetResource).toHaveBeenCalledTimes(0); }); }); diff --git a/frontend/src/concepts/modelRegistry/apiHooks/useModelRegistryServices.ts b/frontend/src/concepts/modelRegistry/apiHooks/useModelRegistryServices.ts index 592a9ffb51..4d3761aba9 100644 --- a/frontend/src/concepts/modelRegistry/apiHooks/useModelRegistryServices.ts +++ b/frontend/src/concepts/modelRegistry/apiHooks/useModelRegistryServices.ts @@ -1,10 +1,6 @@ import React from 'react'; import { k8sGetResource } from '@openshift/dynamic-plugin-sdk-utils'; -import useFetchState, { - FetchState, - FetchStateCallbackPromise, - NotReadyError, -} from '~/utilities/useFetchState'; +import useFetchState, { FetchStateCallbackPromise, NotReadyError } from '~/utilities/useFetchState'; import { AccessReviewResourceAttributes, ServiceKind } from '~/k8sTypes'; import { ServiceModel, useAccessReview, useRulesReview, listServices } from '~/api'; import { MODEL_REGISTRY_DEFAULT_NAMESPACE } from '~/concepts/modelRegistry/const'; @@ -54,21 +50,28 @@ const listServicesOrFetchThemByNames = async ( return services; }; -export const useModelRegistryServices = (): FetchState => { +export type ModelRegistryServicesResult = { + modelRegistryServices: ServiceKind[]; + isLoaded: boolean; + error?: Error; + refreshRulesReview: () => void; +}; + +export const useModelRegistryServices = (namespace: string): ModelRegistryServicesResult => { const [allowList, accessReviewLoaded] = useAccessReview(accessReviewResource); - const [statuses, rulesReviewLoaded] = useRulesReview(MODEL_REGISTRY_DEFAULT_NAMESPACE); + const [rulesReviewStatus, rulesReviewLoaded, refreshRulesReview] = useRulesReview(namespace); const serviceNames = React.useMemo(() => { if (!rulesReviewLoaded) { return []; } - return statuses?.resourceRules + return rulesReviewStatus?.resourceRules .filter( ({ resources, verbs }) => resources?.includes('services') && verbs.some((verb) => verb === 'get'), ) .flatMap((rule) => rule.resourceNames || []); - }, [rulesReviewLoaded, statuses]); + }, [rulesReviewLoaded, rulesReviewStatus]); const callback = React.useCallback>( () => @@ -81,7 +84,14 @@ export const useModelRegistryServices = (): FetchState => { [allowList, accessReviewLoaded, rulesReviewLoaded, serviceNames], ); - return useFetchState(callback, [], { + const [modelRegistryServices, isLoaded, error] = useFetchState(callback, [], { initialPromisePurity: true, }); + + return { + modelRegistryServices, + isLoaded, + error, + refreshRulesReview, + }; }; diff --git a/frontend/src/concepts/modelRegistry/context/ModelRegistrySelectorContext.tsx b/frontend/src/concepts/modelRegistry/context/ModelRegistrySelectorContext.tsx index 1ac9f2b7f5..7e75eb56ac 100644 --- a/frontend/src/concepts/modelRegistry/context/ModelRegistrySelectorContext.tsx +++ b/frontend/src/concepts/modelRegistry/context/ModelRegistrySelectorContext.tsx @@ -2,6 +2,7 @@ import * as React from 'react'; import { ServiceKind } from '~/k8sTypes'; import useModelRegistryEnabled from '~/concepts/modelRegistry/useModelRegistryEnabled'; import { useModelRegistryServices } from '~/concepts/modelRegistry/apiHooks/useModelRegistryServices'; +import { MODEL_REGISTRY_DEFAULT_NAMESPACE } from '~/concepts/modelRegistry/const'; export type ModelRegistrySelectorContextType = { modelRegistryServicesLoaded: boolean; @@ -9,6 +10,7 @@ export type ModelRegistrySelectorContextType = { modelRegistryServices: ServiceKind[]; preferredModelRegistry: ServiceKind | undefined; updatePreferredModelRegistry: (modelRegistry: ServiceKind | undefined) => void; + refreshRulesReview: () => void; }; type ModelRegistrySelectorContextProviderProps = { @@ -21,6 +23,7 @@ export const ModelRegistrySelectorContext = React.createContext undefined, + refreshRulesReview: () => undefined, }); export const ModelRegistrySelectorContextProvider: React.FC< @@ -39,25 +42,30 @@ export const ModelRegistrySelectorContextProvider: React.FC< const EnabledModelRegistrySelectorContextProvider: React.FC< ModelRegistrySelectorContextProviderProps > = ({ children }) => { - const [modelRegistryServices, isLoaded, error] = useModelRegistryServices(); - const [preferredModelRegistry, setPreferredModelRegistry] = - React.useState(undefined); + const { + modelRegistryServices = [], + isLoaded, + error, + refreshRulesReview, + } = useModelRegistryServices(MODEL_REGISTRY_DEFAULT_NAMESPACE); + const [preferredModelRegistry, setPreferredModelRegistry] = React.useState< + ServiceKind | undefined + >(undefined); - const firstModelRegistry = modelRegistryServices.length > 0 ? modelRegistryServices[0] : null; + const contextValue = React.useMemo( + () => ({ + modelRegistryServicesLoaded: isLoaded, + modelRegistryServicesLoadError: error, + modelRegistryServices, + preferredModelRegistry: preferredModelRegistry ?? modelRegistryServices[0], + updatePreferredModelRegistry: setPreferredModelRegistry, + refreshRulesReview, + }), + [isLoaded, error, modelRegistryServices, preferredModelRegistry, refreshRulesReview], + ); return ( - ({ - modelRegistryServicesLoaded: isLoaded, - modelRegistryServicesLoadError: error, - modelRegistryServices, - preferredModelRegistry: preferredModelRegistry ?? firstModelRegistry ?? undefined, - updatePreferredModelRegistry: setPreferredModelRegistry, - }), - [isLoaded, error, modelRegistryServices, preferredModelRegistry, firstModelRegistry], - )} - > + {children} ); diff --git a/frontend/src/pages/modelRegistrySettings/ModelRegistrySettings.tsx b/frontend/src/pages/modelRegistrySettings/ModelRegistrySettings.tsx index aa954ffd7c..2417210a08 100644 --- a/frontend/src/pages/modelRegistrySettings/ModelRegistrySettings.tsx +++ b/frontend/src/pages/modelRegistrySettings/ModelRegistrySettings.tsx @@ -14,12 +14,20 @@ import ApplicationsPage from '~/pages/ApplicationsPage'; import useModelRegistriesBackend from '~/concepts/modelRegistrySettings/useModelRegistriesBackend'; import TitleWithIcon from '~/concepts/design/TitleWithIcon'; import { ProjectObjectType } from '~/concepts/design/utils'; +import { ModelRegistrySelectorContext } from '~/concepts/modelRegistry/context/ModelRegistrySelectorContext'; import ModelRegistriesTable from './ModelRegistriesTable'; import CreateModal from './CreateModal'; const ModelRegistrySettings: React.FC = () => { const [createModalOpen, setCreateModalOpen] = React.useState(false); - const [modelRegistries, loaded, loadError, refresh] = useModelRegistriesBackend(); + const [modelRegistries, loaded, loadError, refreshModelRegistries] = useModelRegistriesBackend(); + const { refreshRulesReview } = React.useContext(ModelRegistrySelectorContext); + + const refreshAll = React.useCallback( + () => Promise.all([refreshModelRegistries(), refreshRulesReview()]), + [refreshModelRegistries, refreshRulesReview], + ); + return ( <> { > setCreateModalOpen(true)} + refresh={refreshAll} + onCreateModelRegistryClick={() => { + setCreateModalOpen(true); + }} /> setCreateModalOpen(false)} - refresh={refresh} + refresh={refreshAll} /> );