Skip to content

Commit

Permalink
MR selector doesn't update without page reload (#3160)
Browse files Browse the repository at this point in the history 
* working version- no tests

* fixed tests

* added more tests

* removed Promiose.resolve

* fixed test

* fixed test1

* fixed test2
  • Loading branch information
@YuliaKrimerman
YuliaKrimerman authored Sep 6, 2024
1 parent 85f2b31 commit f08cb4e
Show file tree
Hide file tree
Showing 5 changed files with 201 additions and 101 deletions.
11 changes: 8 additions & 3 deletions frontend/src/api/useRulesReview.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,12 @@ const checkAccess = (ns: string): Promise<SelfSubjectRulesReviewKind> => {

export const useRulesReview = (
namespace: string,
): [SelfSubjectRulesReviewKind['status'], boolean] => {
): [SelfSubjectRulesReviewKind['status'], boolean, () => void] => {
const [loaded, setLoaded] = React.useState(false);
const [status, setStatus] = React.useState<SelfSubjectRulesReviewKind['status']>(undefined);

React.useEffect(() => {
const refreshRulesReview = React.useCallback(() => {
setLoaded(false);
checkAccess(namespace)
.then((result) => {
if (!result.status?.incomplete && !result.status?.evaluationError) {
Expand All @@ -38,5 +39,9 @@ export const useRulesReview = (
});
}, [namespace]);

return [status, loaded];
React.useEffect(() => {
refreshRulesReview();
}, [refreshRulesReview]);

return [status, loaded, refreshRulesReview];
};
203 changes: 135 additions & 68 deletions frontend/src/concepts/modelRegistry/apiHooks/__tests__/useModelRegistryServices.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -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', () => ({
Expand All @@ -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<ServiceKind>);
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<typeof useRulesReview>);
(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 () => {
Expand All @@ -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);
});
});
30 changes: 20 additions & 10 deletions frontend/src/concepts/modelRegistry/apiHooks/useModelRegistryServices.ts
View file
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -54,21 +50,28 @@ const listServicesOrFetchThemByNames = async (
return services;
};

export const useModelRegistryServices = (): FetchState<ServiceKind[]> => {
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<FetchStateCallbackPromise<ServiceKind[]>>(
() =>
Expand All @@ -81,7 +84,14 @@ export const useModelRegistryServices = (): FetchState<ServiceKind[]> => {
[allowList, accessReviewLoaded, rulesReviewLoaded, serviceNames],
);

return useFetchState(callback, [], {
const [modelRegistryServices, isLoaded, error] = useFetchState(callback, [], {
initialPromisePurity: true,
});

return {
modelRegistryServices,
isLoaded,
error,
refreshRulesReview,
};
};
Loading

0 comments on commit f08cb4e

Please sign in to comment.