-
Notifications
You must be signed in to change notification settings - Fork 73
OSAC-2932: [UI] Catalog management list page with resource type tabs #100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
4ee9480
ed41374
1c34988
192cb3f
2218b09
3d28c4c
c71dc95
9402235
9fb3e1e
390f0cc
11357d0
ba422a1
f3fcf0b
5c3d592
dfacec5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,201 @@ | ||
| import { createRouterTransport } from '@connectrpc/connect'; | ||
| import { act, waitFor } from '@testing-library/react'; | ||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| import type { BareMetalInstanceCatalogItem } from '@osac/types'; | ||
| import { BareMetalInstanceCatalogItems } from '@osac/types'; | ||
| import type { BareMetalInstanceCatalogItem as PrivateBareMetalInstanceCatalogItem } from '@osac/types/private'; | ||
| import { BareMetalInstanceCatalogItems as PrivateBareMetalInstanceCatalogItems } from '@osac/types/private'; | ||
|
|
||
| import { | ||
| useAdminBareMetalInstanceCatalogItems, | ||
| useAdminSetBareMetalInstanceCatalogItemPublished, | ||
| } from './baremetal-instance'; | ||
| import { renderHookWithProviders } from '../../test-utils/TestProviders'; | ||
|
|
||
| const publicItem: BareMetalInstanceCatalogItem = { | ||
| $typeName: 'osac.public.v1.BareMetalInstanceCatalogItem', | ||
| id: 'public-1', | ||
| title: 'Public bare metal item', | ||
| description: '', | ||
| template: '', | ||
| published: true, | ||
| fieldDefinitions: [], | ||
| }; | ||
|
|
||
| const privateItem: PrivateBareMetalInstanceCatalogItem = { | ||
| $typeName: 'osac.private.v1.BareMetalInstanceCatalogItem', | ||
| id: 'private-1', | ||
| title: 'Private bare metal item', | ||
| description: '', | ||
| template: '', | ||
| published: true, | ||
| tenant: '', | ||
| fieldDefinitions: [], | ||
| }; | ||
|
|
||
| const createTestTransport = (options: { | ||
| onPublicList?: () => void; | ||
| onPrivateList?: () => void; | ||
| onPublicUpdate?: (req: unknown) => void; | ||
| onPrivateUpdate?: (req: unknown) => void; | ||
| }) => | ||
| createRouterTransport((router) => { | ||
| router.service(BareMetalInstanceCatalogItems, { | ||
| list: () => { | ||
| options.onPublicList?.(); | ||
| return { items: [publicItem] }; | ||
| }, | ||
| update: (req) => { | ||
| options.onPublicUpdate?.(req); | ||
| return { object: publicItem }; | ||
| }, | ||
| }); | ||
|
|
||
| router.service(PrivateBareMetalInstanceCatalogItems, { | ||
| list: () => { | ||
| options.onPrivateList?.(); | ||
| return { items: [privateItem] }; | ||
| }, | ||
| update: (req) => { | ||
| options.onPrivateUpdate?.(req); | ||
| return { object: privateItem }; | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| const renderWithSession = <TResult>( | ||
| hook: () => TResult, | ||
| role: 'providerAdmin' | 'tenantAdmin', | ||
| transport: ReturnType<typeof createRouterTransport>, | ||
| ) => renderHookWithProviders(hook, { role, transport }); | ||
|
|
||
| describe('useAdminBareMetalInstanceCatalogItems', () => { | ||
| it('calls the private List endpoint for providerAdmin', async () => { | ||
| let privateListCalled = false; | ||
| let publicListCalled = false; | ||
| const transport = createTestTransport({ | ||
| onPrivateList: () => { | ||
| privateListCalled = true; | ||
| }, | ||
| onPublicList: () => { | ||
| publicListCalled = true; | ||
| }, | ||
| }); | ||
|
|
||
| const { result } = renderWithSession( | ||
| () => useAdminBareMetalInstanceCatalogItems(), | ||
| 'providerAdmin', | ||
| transport, | ||
| ); | ||
|
|
||
| await waitFor(() => expect(result.current.isSuccess).toBe(true)); | ||
| expect(result.current.data).toEqual([privateItem]); | ||
| expect(privateListCalled).toBe(true); | ||
| expect(publicListCalled).toBe(false); | ||
| }); | ||
|
|
||
| it('calls the public List endpoint for tenantAdmin', async () => { | ||
| let privateListCalled = false; | ||
| let publicListCalled = false; | ||
| const transport = createTestTransport({ | ||
| onPrivateList: () => { | ||
| privateListCalled = true; | ||
| }, | ||
| onPublicList: () => { | ||
| publicListCalled = true; | ||
| }, | ||
| }); | ||
|
|
||
| const { result } = renderWithSession( | ||
| () => useAdminBareMetalInstanceCatalogItems(), | ||
| 'tenantAdmin', | ||
| transport, | ||
| ); | ||
|
|
||
| await waitFor(() => expect(result.current.isSuccess).toBe(true)); | ||
| expect(result.current.data).toEqual([publicItem]); | ||
| expect(publicListCalled).toBe(true); | ||
| expect(privateListCalled).toBe(false); | ||
| }); | ||
|
|
||
| it('does not call either endpoint when disabled', async () => { | ||
| let privateListCalled = false; | ||
| let publicListCalled = false; | ||
| const transport = createTestTransport({ | ||
| onPrivateList: () => { | ||
| privateListCalled = true; | ||
| }, | ||
| onPublicList: () => { | ||
| publicListCalled = true; | ||
| }, | ||
| }); | ||
|
|
||
| renderWithSession( | ||
| () => useAdminBareMetalInstanceCatalogItems({}, false), | ||
| 'providerAdmin', | ||
| transport, | ||
| ); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 10)); | ||
| expect(privateListCalled).toBe(false); | ||
| expect(publicListCalled).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('useAdminSetBareMetalInstanceCatalogItemPublished', () => { | ||
| it('sends the update with a published field mask to the private client for providerAdmin', async () => { | ||
| let lastReq: unknown; | ||
| const transport = createTestTransport({ | ||
| onPrivateUpdate: (req) => { | ||
| lastReq = req; | ||
| }, | ||
| }); | ||
|
|
||
| const { result } = renderWithSession( | ||
| () => useAdminSetBareMetalInstanceCatalogItemPublished(), | ||
| 'providerAdmin', | ||
| transport, | ||
| ); | ||
|
|
||
| act(() => { | ||
| result.current.mutate({ id: 'private-1', published: false }); | ||
| }); | ||
|
|
||
| await waitFor(() => expect(result.current.isSuccess).toBe(true)); | ||
| expect(lastReq).toMatchObject({ | ||
| object: { id: 'private-1', published: false }, | ||
| updateMask: { paths: ['published'] }, | ||
| }); | ||
| }); | ||
|
|
||
| it('sends the update to the public client for tenantAdmin', async () => { | ||
| let lastReq: unknown; | ||
| let privateCalled = false; | ||
| const transport = createTestTransport({ | ||
| onPublicUpdate: (req) => { | ||
| lastReq = req; | ||
| }, | ||
| onPrivateUpdate: () => { | ||
| privateCalled = true; | ||
| }, | ||
| }); | ||
|
|
||
| const { result } = renderWithSession( | ||
| () => useAdminSetBareMetalInstanceCatalogItemPublished(), | ||
| 'tenantAdmin', | ||
| transport, | ||
| ); | ||
|
|
||
| act(() => { | ||
| result.current.mutate({ id: 'public-1', published: true }); | ||
| }); | ||
|
|
||
| await waitFor(() => expect(result.current.isSuccess).toBe(true)); | ||
| expect(lastReq).toMatchObject({ | ||
| object: { id: 'public-1', published: true }, | ||
| updateMask: { paths: ['published'] }, | ||
| }); | ||
| expect(privateCalled).toBe(false); | ||
| }); | ||
| }); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,9 +7,11 @@ import { | |
| BareMetalInstanceSchema, | ||
| BareMetalInstances, | ||
| } from '@osac/types'; | ||
| import { BareMetalInstanceCatalogItems as PrivateBareMetalInstanceCatalogItems } from '@osac/types/private'; | ||
|
|
||
| import { useSession } from '../../hooks/use-session'; | ||
| import { useApiFetch } from '../api-context'; | ||
| import { apiQueryKey } from '../types'; | ||
| import { type ListParams, apiQueryKey } from '../types'; | ||
| import { type ApiQueryClient, useApiQuery, useApiQueryClient } from '../use-api-query'; | ||
|
|
||
| export const useBareMetalInstances = () => { | ||
|
|
@@ -31,16 +33,60 @@ export const useBareMetalInstance = (id: string) => { | |
| }); | ||
| }; | ||
|
|
||
| export const useBareMetalInstanceCatalogItems = (enabled = true) => { | ||
| export const useBareMetalInstanceCatalogItems = (params: ListParams = {}, enabled = true) => { | ||
| const client = useApiFetch(BareMetalInstanceCatalogItems); | ||
| return useApiQuery({ | ||
| queryKey: apiQueryKey('v1/baremetal_instance_catalog_items'), | ||
| queryFn: () => client.list({}), | ||
| queryKey: apiQueryKey('v1/baremetal_instance_catalog_items', undefined, params), | ||
| queryFn: () => client.list(params), | ||
| select: (data) => data.items, | ||
| enabled, | ||
| }); | ||
| }; | ||
|
|
||
| /** | ||
| * Admin list hook for the catalog management pages. CSP Admin (`providerAdmin`) sees all items via | ||
| * the private API (including unpublished). Tenant Admin sees their tenant's items via the public API — | ||
| * this currently returns only published items regardless of caller role; unpublished items scoped to | ||
| * the Tenant Admin's own tenant are not visible through this hook (tracked as a backend limitation in | ||
| * OSAC-3121). | ||
| */ | ||
| export const useAdminBareMetalInstanceCatalogItems = (params: ListParams = {}, enabled = true) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets not have private and public hooks in one file. Lets update the structure to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed and done — split into a plain public hook in |
||
| const { role } = useSession(); | ||
| const isProviderAdmin = role === 'providerAdmin'; | ||
| const publicResult = useBareMetalInstanceCatalogItems(params, enabled && !isProviderAdmin); | ||
| const privateClient = useApiFetch(PrivateBareMetalInstanceCatalogItems); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this hooks should always work only with lets not check
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed — deleted |
||
| const privateResult = useApiQuery({ | ||
| queryKey: apiQueryKey('v1/baremetal_instance_catalog_items_private', undefined, params), | ||
| queryFn: () => privateClient.list(params), | ||
| select: (data) => data.items, | ||
| enabled: enabled && isProviderAdmin, | ||
| }); | ||
| return isProviderAdmin ? privateResult : publicResult; | ||
| }; | ||
|
|
||
| export const useAdminSetBareMetalInstanceCatalogItemPublished = () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should have a single hook for updating BM. Not for every operation - ie
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This hook is gone now — see the publish-toggle removal below, there's no longer a BM catalog-item mutation to consolidate. |
||
| const { role } = useSession(); | ||
| const isProviderAdmin = role === 'providerAdmin'; | ||
| const publicClient = useApiFetch(BareMetalInstanceCatalogItems); | ||
| const privateClient = useApiFetch(PrivateBareMetalInstanceCatalogItems); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here - lets not mix role + private/public
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. applies for all other catalog item hooks
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same fix applied here — see the reply on the sibling comment (390f0cc). |
||
| const qc = useApiQueryClient(); | ||
| return useMutation({ | ||
| mutationFn: ({ id, published }: { id: string; published: boolean }): Promise<void> => | ||
| (isProviderAdmin | ||
| ? privateClient.update({ object: { id, published }, updateMask: { paths: ['published'] } }) | ||
| : publicClient.update({ object: { id, published }, updateMask: { paths: ['published'] } }) | ||
| ).then(() => undefined), | ||
| onSuccess: () => | ||
| qc.invalidateQueries({ | ||
| queryKey: apiQueryKey( | ||
| isProviderAdmin | ||
| ? 'v1/baremetal_instance_catalog_items_private' | ||
| : 'v1/baremetal_instance_catalog_items', | ||
| ), | ||
| }), | ||
| }); | ||
| }; | ||
|
|
||
| export const invalidateBareMetalInstancesQueries = async (qc: ApiQueryClient) => { | ||
| await qc.invalidateQueries({ queryKey: apiQueryKey('v1/baremetal_instances') }); | ||
| }; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe
to mirror our proposed api/v1 hooks structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed and applied (fixed the missing closing quote from the suggested diff) — 390f0cc.