diff --git a/apps/app-frontend/src/shell/StorageRoutes.test.tsx b/apps/app-frontend/src/shell/StorageRoutes.test.tsx index 8aa2d014..aafa8710 100644 --- a/apps/app-frontend/src/shell/StorageRoutes.test.tsx +++ b/apps/app-frontend/src/shell/StorageRoutes.test.tsx @@ -1,7 +1,10 @@ import { Route, Routes } from 'react-router-dom'; +import { create } from '@bufbuild/protobuf'; import { screen, waitFor } from '@testing-library/react'; import { describe, expect, it, vi } from 'vitest'; +import { StorageBackendSchema } from '@osac/types/private'; +import type { MockApiFixtures } from '@osac/ui-components/test-utils/createMockConnectTransport'; import { renderWithProviders } from '@osac/ui-components/test-utils/TestProviders'; import { StorageRoutes } from './StorageRoutes'; @@ -17,12 +20,12 @@ vi.mock('react-router-dom', async (importOriginal) => { }; }); -const renderAt = (path: string) => +const renderAt = (path: string, apiFixtures?: MockApiFixtures) => renderWithProviders( } /> , - { routerEntries: [path] }, + { routerEntries: [path], apiFixtures }, ); describe('StorageRoutes', () => { @@ -39,10 +42,22 @@ describe('StorageRoutes', () => { expect(screen.getByRole('textbox', { name: 'Name' })).toBeInTheDocument(); }); - it('renders a placeholder for backends/:id/edit', () => { - renderAt('/admin/infrastructure/storage/backends/abc-123/edit'); + it('renders the real edit form for backends/:id/edit', async () => { + renderAt('/admin/infrastructure/storage/backends/abc-123/edit', { + storageBackends: [ + create(StorageBackendSchema, { + id: 'abc-123', + metadata: { name: 'vast-prod-1' }, + spec: { provider: 'vast', endpoint: 'vast.example.com:443', description: '' }, + }), + ], + }); - expect(screen.getByText('Edit storage backend')).toBeInTheDocument(); + await waitFor(() => { + expect(screen.getByRole('heading', { name: 'Edit storage backend' })).toBeInTheDocument(); + }); + expect(screen.getByRole('textbox', { name: 'Endpoint' })).toHaveValue('vast.example.com:443'); + expect(screen.queryByText('This feature is coming soon.')).not.toBeInTheDocument(); }); it('renders the Tiers tab at /admin/infrastructure/storage/tiers', async () => { diff --git a/apps/app-frontend/src/shell/StorageRoutes.tsx b/apps/app-frontend/src/shell/StorageRoutes.tsx index 72825216..d28d84f6 100644 --- a/apps/app-frontend/src/shell/StorageRoutes.tsx +++ b/apps/app-frontend/src/shell/StorageRoutes.tsx @@ -14,10 +14,7 @@ export const StorageRoutes = () => { } /> } /> } /> - } - /> + } /> } /> } /> { it('sends a single spec.endpoint mask entry and never masks metadata.name or spec.provider', async () => { const captured = await mutateAndCaptureUpdate({ id: 'b-1', + version: 3, spec: { endpoint: 'new.example.com' }, }); @@ -183,13 +184,14 @@ describe('useUpdateStorageBackend', () => { expect(paths).toEqual(['spec.endpoint']); expect(paths).not.toContain('metadata.name'); expect(paths).not.toContain('spec.provider'); - const object = captured?.object as { metadata?: unknown }; - expect(object.metadata).toBeUndefined(); + const object = captured?.object as { metadata?: { name?: string } }; + expect(object.metadata?.name).toBe(''); }); it('sends spec.endpoint and spec.description as separate mask entries when both change', async () => { const captured = await mutateAndCaptureUpdate({ id: 'b-1', + version: 3, spec: { endpoint: 'new.example.com', description: 'updated description' }, }); @@ -202,6 +204,7 @@ describe('useUpdateStorageBackend', () => { it('sends a single spec.credentials mask entry, never split into username/password leaves', async () => { const captured = await mutateAndCaptureUpdate({ id: 'b-1', + version: 3, spec: { credentials: { username: 'test-updated-admin', password: 'test-updated-secret' } }, }); @@ -214,6 +217,27 @@ describe('useUpdateStorageBackend', () => { password: 'test-updated-secret', }); }); + + it('sends lock: true for optimistic concurrency', async () => { + const captured = await mutateAndCaptureUpdate({ + id: 'b-1', + version: 3, + spec: { endpoint: 'new.example.com' }, + }); + + expect(captured?.lock).toBe(true); + }); + + it('sends the current version in object.metadata so the server can enforce the lock', async () => { + const captured = await mutateAndCaptureUpdate({ + id: 'b-1', + version: 7, + spec: { endpoint: 'new.example.com' }, + }); + + const object = captured?.object as { metadata?: { version?: number } }; + expect(object.metadata?.version).toBe(7); + }); }); describe('useDeleteStorageBackend', () => { diff --git a/libs/ui-components/src/api/v1/private/storage-backends.ts b/libs/ui-components/src/api/v1/private/storage-backends.ts index 0b95eb0d..9dc5744e 100644 --- a/libs/ui-components/src/api/v1/private/storage-backends.ts +++ b/libs/ui-components/src/api/v1/private/storage-backends.ts @@ -67,6 +67,8 @@ export const useCreateStorageBackend = () => { export type UpdateStorageBackendInput = { id: string; + /** The `metadata.version` of the record the caller last fetched — required so the server can enforce the lock below; a request with no metadata is exempt from the optimistic-lock check regardless of `lock: true`. */ + version: number; spec: MessageInitShape; }; @@ -81,8 +83,9 @@ export const useUpdateStorageBackend = () => { } const resp = await client.update({ - object: { id: input.id, spec }, + object: { id: input.id, metadata: { version: input.version }, spec }, updateMask: { paths: buildUpdateMaskPaths({ spec } as Record) }, + lock: true, }); if (!resp.object) { throw new Error('Update response missing object'); diff --git a/libs/ui-components/src/pages/admin/StorageBackendCreatePage.test.tsx b/libs/ui-components/src/pages/admin/StorageBackendCreatePage.test.tsx index 9541358c..498b0abe 100644 --- a/libs/ui-components/src/pages/admin/StorageBackendCreatePage.test.tsx +++ b/libs/ui-components/src/pages/admin/StorageBackendCreatePage.test.tsx @@ -1,16 +1,23 @@ +import { Route, Routes } from 'react-router-dom'; import { create } from '@bufbuild/protobuf'; import { Code, ConnectError } from '@connectrpc/connect'; import { screen, waitFor } from '@testing-library/react'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { + StorageBackendSchema, type StorageBackendsCreateRequest, type StorageBackendsCreateResponse, StorageBackendsCreateResponseSchema, + type StorageBackendsUpdateRequest, + StorageBackendsUpdateResponseSchema, } from '@osac/types/private'; import { StorageBackendCreatePage } from './StorageBackendCreatePage'; -import type { MockTransportOverrides } from '../../test-utils/createMockConnectTransport'; +import type { + MockApiFixtures, + MockTransportOverrides, +} from '../../test-utils/createMockConnectTransport'; import { renderWithProviders } from '../../test-utils/TestProviders'; const mockNavigate = vi.fn(); @@ -29,195 +36,404 @@ vi.mock('react-router-dom', async (importOriginal) => { const testBackendPassword = 'test-password'; -const renderPage = (overrides?: MockTransportOverrides) => +const renderCreatePage = (overrides?: MockTransportOverrides) => renderWithProviders(, { transportOverrides: overrides, }); +const EDIT_ROUTE_PATH = '/admin/infrastructure/storage/backends/:id/edit'; +const EDIT_PATH = '/admin/infrastructure/storage/backends/b-1/edit'; + +const existingBackend = create(StorageBackendSchema, { + id: 'b-1', + metadata: { name: 'vast-prod-1', version: 7 }, + spec: { + provider: 'vast', + endpoint: 'vast.example.com:443', + description: 'primary array', + credentials: { username: 'test-existing-admin', password: 'test-existing-secret' }, + }, +}); + +const renderEditPage = (overrides?: MockTransportOverrides, apiFixtures?: MockApiFixtures) => + renderWithProviders( + + } /> + , + { + routerEntries: [EDIT_PATH], + apiFixtures: { storageBackends: [existingBackend], ...apiFixtures }, + transportOverrides: overrides, + }, + ); + describe('StorageBackendCreatePage', () => { beforeEach(() => { mockNavigate.mockReset(); }); - const fillValidForm = async (user: ReturnType['user']) => { - await user.type(screen.getByRole('textbox', { name: 'Name' }), 'vast-prod-1'); - await user.click(screen.getByLabelText(/^Provider/)); - await user.click(screen.getByRole('option', { name: 'VAST' })); - await user.type(screen.getByRole('textbox', { name: 'Endpoint' }), 'vast.example.com:443'); - await user.type(screen.getByLabelText(/^Username/), 'admin'); - await user.type(screen.getByLabelText(/^Password/), testBackendPassword); - }; + describe('create mode', () => { + const fillValidForm = async (user: ReturnType['user']) => { + await user.type(screen.getByRole('textbox', { name: 'Name' }), 'vast-prod-1'); + await user.click(screen.getByLabelText(/^Provider/)); + await user.click(screen.getByRole('option', { name: 'VAST' })); + await user.type(screen.getByRole('textbox', { name: 'Endpoint' }), 'vast.example.com:443'); + await user.type(screen.getByLabelText(/^Username/), 'test-admin'); + await user.type(screen.getByLabelText(/^Password/), testBackendPassword); + }; + + it('renders the page title, breadcrumb, and all fields', () => { + renderCreatePage(); + + expect(screen.getByRole('heading', { name: 'Create storage backend' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Storage backends' })).toBeInTheDocument(); + expect(screen.getByRole('textbox', { name: 'Name' })).toBeInTheDocument(); + expect(screen.getByLabelText(/^Provider/)).toBeInTheDocument(); + expect(screen.getByRole('textbox', { name: 'Endpoint' })).toBeInTheDocument(); + expect(screen.getByRole('textbox', { name: 'Description' })).toBeInTheDocument(); + expect(screen.getByLabelText(/^Username/)).toBeInTheDocument(); + expect(screen.getByLabelText(/^Password/)).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Create' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Cancel' })).toBeInTheDocument(); + }); - it('renders the page title, breadcrumb, and all fields', () => { - renderPage(); - - expect(screen.getByRole('heading', { name: 'Create storage backend' })).toBeInTheDocument(); - expect(screen.getByRole('button', { name: 'Storage backends' })).toBeInTheDocument(); - expect(screen.getByRole('textbox', { name: 'Name' })).toBeInTheDocument(); - expect(screen.getByLabelText(/^Provider/)).toBeInTheDocument(); - expect(screen.getByRole('textbox', { name: 'Endpoint' })).toBeInTheDocument(); - expect(screen.getByRole('textbox', { name: 'Description' })).toBeInTheDocument(); - expect(screen.getByLabelText(/^Username/)).toBeInTheDocument(); - expect(screen.getByLabelText(/^Password/)).toBeInTheDocument(); - expect(screen.getByRole('button', { name: 'Create' })).toBeInTheDocument(); - expect(screen.getByRole('button', { name: 'Cancel' })).toBeInTheDocument(); - }); + it('renders name and provider as enabled', () => { + renderCreatePage(); - it('renders the provider select with exactly vast, ceph, and pure options', async () => { - const { user } = renderPage(); + expect(screen.getByRole('textbox', { name: 'Name' })).toBeEnabled(); + expect(screen.getByLabelText(/^Provider/)).toBeEnabled(); + }); - await user.click(screen.getByLabelText(/^Provider/)); + it('renders the provider select with exactly vast, ceph, and pure options', async () => { + const { user } = renderCreatePage(); - const options = screen.getAllByRole('option'); - expect(options).toHaveLength(3); - expect(options.map((option) => option.textContent)).toEqual(['VAST', 'Ceph', 'Pure']); - }); + await user.click(screen.getByLabelText(/^Provider/)); - it('renders the password field as a masked input', () => { - renderPage(); + const options = screen.getAllByRole('option'); + expect(options).toHaveLength(3); + expect(options.map((option) => option.textContent)).toEqual(['VAST', 'Ceph', 'Pure']); + }); - const passwordField = screen.getByLabelText(/^Password/); - expect(passwordField).toHaveAttribute('type', 'password'); - }); + it('renders the password field as a masked input', () => { + renderCreatePage(); - it('shows a DNS-label validation error for an invalid name and does not submit', async () => { - const onStorageBackendCreate = vi.fn(); - const { user } = renderPage({ onStorageBackendCreate }); + const passwordField = screen.getByLabelText(/^Password/); + expect(passwordField).toHaveAttribute('type', 'password'); + }); - await user.type(screen.getByRole('textbox', { name: 'Name' }), 'Invalid_Name'); - await user.click(screen.getByRole('button', { name: 'Create' })); + it('shows a DNS-label validation error for an invalid name and does not submit', async () => { + const onStorageBackendCreate = vi.fn(); + const { user } = renderCreatePage({ onStorageBackendCreate }); - await waitFor(() => { - expect( - screen.getByText( - 'Name must only contain lowercase letters (a-z), digits (0-9), and hyphens (-)', - ), - ).toBeInTheDocument(); + await user.type(screen.getByRole('textbox', { name: 'Name' }), 'Invalid_Name'); + await user.click(screen.getByRole('button', { name: 'Create' })); + + await waitFor(() => { + expect( + screen.getByText( + 'Name must only contain lowercase letters (a-z), digits (0-9), and hyphens (-)', + ), + ).toBeInTheDocument(); + }); + expect(onStorageBackendCreate).not.toHaveBeenCalled(); + expect(mockNavigate).not.toHaveBeenCalled(); }); - expect(onStorageBackendCreate).not.toHaveBeenCalled(); - expect(mockNavigate).not.toHaveBeenCalled(); - }); - it('shows required-field validation errors for endpoint, username, and password', async () => { - const onStorageBackendCreate = vi.fn(); - const { user } = renderPage({ onStorageBackendCreate }); + it('shows required-field validation errors for endpoint, username, and password', async () => { + const onStorageBackendCreate = vi.fn(); + const { user } = renderCreatePage({ onStorageBackendCreate }); - await user.type(screen.getByRole('textbox', { name: 'Name' }), 'vast-prod-1'); - await user.click(screen.getByLabelText(/^Provider/)); - await user.click(screen.getByRole('option', { name: 'VAST' })); - await user.click(screen.getByRole('button', { name: 'Create' })); + await user.type(screen.getByRole('textbox', { name: 'Name' }), 'vast-prod-1'); + await user.click(screen.getByLabelText(/^Provider/)); + await user.click(screen.getByRole('option', { name: 'VAST' })); + await user.click(screen.getByRole('button', { name: 'Create' })); - await waitFor(() => { - expect(screen.getByText('Endpoint is required')).toBeInTheDocument(); + await waitFor(() => { + expect(screen.getByText('Endpoint is required')).toBeInTheDocument(); + }); + expect(screen.getByText('Username is required')).toBeInTheDocument(); + expect(screen.getByText('Password is required')).toBeInTheDocument(); + expect(onStorageBackendCreate).not.toHaveBeenCalled(); }); - expect(screen.getByText('Username is required')).toBeInTheDocument(); - expect(screen.getByText('Password is required')).toBeInTheDocument(); - expect(onStorageBackendCreate).not.toHaveBeenCalled(); - }); - it('shows a required error for provider when nothing is selected, not the removed oneOf message', async () => { - const onStorageBackendCreate = vi.fn(); - const { user } = renderPage({ onStorageBackendCreate }); + it('shows a required error for provider when nothing is selected, not the removed oneOf message', async () => { + const onStorageBackendCreate = vi.fn(); + const { user } = renderCreatePage({ onStorageBackendCreate }); - await user.type(screen.getByRole('textbox', { name: 'Name' }), 'vast-prod-1'); - await user.type(screen.getByRole('textbox', { name: 'Endpoint' }), 'vast.example.com:443'); - await user.type(screen.getByLabelText(/^Username/), 'admin'); - await user.type(screen.getByLabelText(/^Password/), testBackendPassword); - await user.click(screen.getByRole('button', { name: 'Create' })); + await user.type(screen.getByRole('textbox', { name: 'Name' }), 'vast-prod-1'); + await user.type(screen.getByRole('textbox', { name: 'Endpoint' }), 'vast.example.com:443'); + await user.type(screen.getByLabelText(/^Username/), 'test-admin'); + await user.type(screen.getByLabelText(/^Password/), testBackendPassword); + await user.click(screen.getByRole('button', { name: 'Create' })); - await waitFor(() => { - expect(screen.getByText('Provider is required')).toBeInTheDocument(); + await waitFor(() => { + expect(screen.getByText('Provider is required')).toBeInTheDocument(); + }); + expect( + screen.queryByText('Provider must be one of vast, ceph, or pure'), + ).not.toBeInTheDocument(); + expect(onStorageBackendCreate).not.toHaveBeenCalled(); }); - expect( - screen.queryByText('Provider must be one of vast, ceph, or pure'), - ).not.toBeInTheDocument(); - expect(onStorageBackendCreate).not.toHaveBeenCalled(); - }); - it('disables Create while the submission is pending, to prevent duplicate submissions', async () => { - let resolveCreate: (() => void) | undefined; - const onStorageBackendCreate = () => - new Promise((resolve) => { - resolveCreate = () => - resolve(create(StorageBackendsCreateResponseSchema, { object: { id: 'new-backend-1' } })); + it('disables Create while the submission is pending, to prevent duplicate submissions', async () => { + let resolveCreate: (() => void) | undefined; + const onStorageBackendCreate = () => + new Promise((resolve) => { + resolveCreate = () => + resolve( + create(StorageBackendsCreateResponseSchema, { object: { id: 'new-backend-1' } }), + ); + }); + + const { user } = renderCreatePage({ onStorageBackendCreate }); + + await fillValidForm(user); + await user.click(screen.getByRole('button', { name: 'Create' })); + + // Once isLoading is true, PatternFly's Spinner contributes its own + // "Contents" accessible name to the button, so an exact "Create" match + // no longer resolves — match by substring instead (same pattern already + // used elsewhere in this file for accessible-name additions). + await waitFor(() => { + expect(screen.getByRole('button', { name: /Create/ })).toBeDisabled(); }); + expect(screen.getByRole('button', { name: 'Cancel' })).toBeDisabled(); - const { user } = renderPage({ onStorageBackendCreate }); + resolveCreate?.(); - await fillValidForm(user); - await user.click(screen.getByRole('button', { name: 'Create' })); + await waitFor(() => { + expect(mockNavigate).toHaveBeenCalledWith('/admin/infrastructure/storage/backends'); + }); + }, 15000); + + it('submits the expected payload and navigates to the backends list on success', async () => { + let capturedRequest: StorageBackendsCreateRequest | undefined; + const { user } = renderCreatePage({ + onStorageBackendCreate: (req) => { + capturedRequest = req; + return create(StorageBackendsCreateResponseSchema, { + object: { + id: 'new-backend-1', + metadata: req.object?.metadata, + spec: req.object?.spec, + }, + }); + }, + }); + + await fillValidForm(user); + await user.click(screen.getByRole('button', { name: 'Create' })); + + await waitFor(() => { + expect(mockNavigate).toHaveBeenCalledWith('/admin/infrastructure/storage/backends'); + }); + + expect(capturedRequest?.object?.metadata?.name).toBe('vast-prod-1'); + expect(capturedRequest?.object?.spec?.provider).toBe('vast'); + expect(capturedRequest?.object?.spec?.endpoint).toBe('vast.example.com:443'); + expect(capturedRequest?.object?.spec?.credentials?.username).toBe('test-admin'); + expect(capturedRequest?.object?.spec?.credentials?.password).toBe(testBackendPassword); + }, 15000); + + it('shows a form-level error and does not navigate when the name already exists', async () => { + const { user } = renderCreatePage({ + onStorageBackendCreate: () => { + throw new ConnectError('Storage backend name already exists', Code.AlreadyExists); + }, + }); - // Once isLoading is true, PatternFly's Spinner contributes its own - // "Contents" accessible name to the button, so an exact "Create" match - // no longer resolves — match by substring instead (same pattern already - // used elsewhere in this file for accessible-name additions). - await waitFor(() => { - expect(screen.getByRole('button', { name: /Create/ })).toBeDisabled(); + await fillValidForm(user); + await user.click(screen.getByRole('button', { name: 'Create' })); + + await waitFor(() => { + expect(screen.getByText('Failed to create storage backend')).toBeInTheDocument(); + }); + expect(screen.getByText('Storage backend name already exists')).toBeInTheDocument(); + expect(mockNavigate).not.toHaveBeenCalled(); + }, 15000); + + it('navigates back to the backends list on cancel', async () => { + const { user } = renderCreatePage(); + + await user.click(screen.getByRole('button', { name: 'Cancel' })); + + expect(mockNavigate).toHaveBeenCalledWith('/admin/infrastructure/storage/backends'); }); - expect(screen.getByRole('button', { name: 'Cancel' })).toBeDisabled(); - resolveCreate?.(); + it('navigates back to the backends list via breadcrumb', async () => { + const { user } = renderCreatePage(); + + await user.click(screen.getByRole('button', { name: 'Storage backends' })); - await waitFor(() => { expect(mockNavigate).toHaveBeenCalledWith('/admin/infrastructure/storage/backends'); }); - }, 15000); + }); - it('submits the expected payload and navigates to the backends list on success', async () => { - let capturedRequest: StorageBackendsCreateRequest | undefined; - const { user } = renderPage({ - onStorageBackendCreate: (req) => { - capturedRequest = req; - return create(StorageBackendsCreateResponseSchema, { - object: { id: 'new-backend-1', metadata: req.object?.metadata, spec: req.object?.spec }, - }); - }, + describe('edit mode', () => { + it('shows a loading spinner before the backend has been fetched', () => { + renderEditPage(); + + expect(screen.getByRole('progressbar')).toBeInTheDocument(); }); - await fillValidForm(user); - await user.click(screen.getByRole('button', { name: 'Create' })); + it('renders the page prefilled with the backend endpoint and description', async () => { + renderEditPage(); - await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith('/admin/infrastructure/storage/backends'); + await waitFor(() => { + expect(screen.getByRole('heading', { name: 'Edit storage backend' })).toBeInTheDocument(); + }); + expect(screen.getByRole('textbox', { name: 'Endpoint' })).toHaveValue('vast.example.com:443'); + expect(screen.getByRole('textbox', { name: 'Description' })).toHaveValue('primary array'); + expect(screen.getByRole('textbox', { name: 'Name' })).toHaveValue('vast-prod-1'); + expect(screen.getByLabelText(/^Provider/)).toHaveTextContent('VAST'); }); - expect(capturedRequest?.object?.metadata?.name).toBe('vast-prod-1'); - expect(capturedRequest?.object?.spec?.provider).toBe('vast'); - expect(capturedRequest?.object?.spec?.endpoint).toBe('vast.example.com:443'); - expect(capturedRequest?.object?.spec?.credentials?.username).toBe('admin'); - expect(capturedRequest?.object?.spec?.credentials?.password).toBe(testBackendPassword); - }, 15000); + it('renders name and provider as disabled', async () => { + renderEditPage(); - it('shows a form-level error and does not navigate when the name already exists', async () => { - const { user } = renderPage({ - onStorageBackendCreate: () => { - throw new ConnectError('Storage backend name already exists', Code.AlreadyExists); - }, + await waitFor(() => { + expect(screen.getByRole('textbox', { name: 'Name' })).toBeInTheDocument(); + }); + expect(screen.getByRole('textbox', { name: 'Name' })).toBeDisabled(); + expect(screen.getByLabelText(/^Provider/)).toBeDisabled(); }); - await fillValidForm(user); - await user.click(screen.getByRole('button', { name: 'Create' })); + it('renders credential fields blank regardless of the fetched record', async () => { + renderEditPage(); - await waitFor(() => { - expect(screen.getByText('Failed to create storage backend')).toBeInTheDocument(); + await waitFor(() => { + expect(screen.getByLabelText(/^Username/)).toBeInTheDocument(); + }); + expect(screen.getByLabelText(/^Username/)).toHaveValue(''); + expect(screen.getByLabelText(/^Password/)).toHaveValue(''); }); - expect(screen.getByText('Storage backend name already exists')).toBeInTheDocument(); - expect(mockNavigate).not.toHaveBeenCalled(); - }, 15000); - it('navigates back to the backends list on cancel', async () => { - const { user } = renderPage(); + it('shows a validation error and does not submit when only username is filled', async () => { + const onStorageBackendUpdate = vi.fn(); + const { user } = renderEditPage({ onStorageBackendUpdate }); - await user.click(screen.getByRole('button', { name: 'Cancel' })); + await waitFor(() => { + expect(screen.getByLabelText(/^Username/)).toBeInTheDocument(); + }); + await user.type(screen.getByLabelText(/^Username/), 'test-new-admin'); + await user.click(screen.getByRole('button', { name: 'Save' })); - expect(mockNavigate).toHaveBeenCalledWith('/admin/infrastructure/storage/backends'); - }); + await waitFor(() => { + expect( + screen.getAllByText('Enter both username and password, or leave both blank').length, + ).toBeGreaterThan(0); + }); + expect(onStorageBackendUpdate).not.toHaveBeenCalled(); + expect(mockNavigate).not.toHaveBeenCalled(); + }); - it('navigates back to the backends list via breadcrumb', async () => { - const { user } = renderPage(); + it('shows a validation error and does not submit when only password is filled', async () => { + const onStorageBackendUpdate = vi.fn(); + const { user } = renderEditPage({ onStorageBackendUpdate }); - await user.click(screen.getByRole('button', { name: 'Storage backends' })); + await waitFor(() => { + expect(screen.getByLabelText(/^Password/)).toBeInTheDocument(); + }); + await user.type(screen.getByLabelText(/^Password/), 'test-new-secret'); + await user.click(screen.getByRole('button', { name: 'Save' })); + + await waitFor(() => { + expect( + screen.getAllByText('Enter both username and password, or leave both blank').length, + ).toBeGreaterThan(0); + }); + expect(onStorageBackendUpdate).not.toHaveBeenCalled(); + expect(mockNavigate).not.toHaveBeenCalled(); + }); + + it('omits credentials and never submits name/provider when both credential fields are left blank', async () => { + let capturedRequest: StorageBackendsUpdateRequest | undefined; + const { user } = renderEditPage({ + onStorageBackendUpdate: (req) => { + capturedRequest = req; + return create(StorageBackendsUpdateResponseSchema, { object: existingBackend }); + }, + }); + + await waitFor(() => { + expect(screen.getByRole('textbox', { name: 'Endpoint' })).toHaveValue( + 'vast.example.com:443', + ); + }); + await user.clear(screen.getByRole('textbox', { name: 'Endpoint' })); + await user.type(screen.getByRole('textbox', { name: 'Endpoint' }), 'new.example.com:443'); + await user.click(screen.getByRole('button', { name: 'Save' })); + + await waitFor(() => { + expect(mockNavigate).toHaveBeenCalledWith('/admin/infrastructure/storage/backends'); + }); + expect(capturedRequest?.object?.spec?.endpoint).toBe('new.example.com:443'); + expect(capturedRequest?.object?.spec?.credentials).toBeUndefined(); + expect(capturedRequest?.object?.metadata?.name).toBe(''); + expect(capturedRequest?.updateMask?.paths).not.toContain('spec.provider'); + expect(capturedRequest?.updateMask?.paths).not.toContain('metadata.name'); + expect(capturedRequest?.updateMask?.paths).not.toContain('spec.credentials'); + expect(capturedRequest?.lock).toBe(true); + expect(capturedRequest?.object?.metadata?.version).toBe(7); + }); + + it('submits a complete credentials object when both fields are filled', async () => { + let capturedRequest: StorageBackendsUpdateRequest | undefined; + const { user } = renderEditPage({ + onStorageBackendUpdate: (req) => { + capturedRequest = req; + return create(StorageBackendsUpdateResponseSchema, { object: existingBackend }); + }, + }); - expect(mockNavigate).toHaveBeenCalledWith('/admin/infrastructure/storage/backends'); + await waitFor(() => { + expect(screen.getByLabelText(/^Username/)).toBeInTheDocument(); + }); + await user.type(screen.getByLabelText(/^Username/), 'test-new-admin'); + await user.type(screen.getByLabelText(/^Password/), 'test-new-secret'); + await user.click(screen.getByRole('button', { name: 'Save' })); + + await waitFor(() => { + expect(mockNavigate).toHaveBeenCalledWith('/admin/infrastructure/storage/backends'); + }); + expect(capturedRequest?.object?.spec?.credentials).toMatchObject({ + username: 'test-new-admin', + password: 'test-new-secret', + }); + }); + + it('shows a submission error and does not navigate on a stale-version conflict', async () => { + const { user } = renderEditPage({ + onStorageBackendUpdate: () => { + throw new ConnectError('Storage backend was modified by another request', Code.Aborted); + }, + }); + + await waitFor(() => { + expect(screen.getByRole('textbox', { name: 'Endpoint' })).toHaveValue( + 'vast.example.com:443', + ); + }); + await user.click(screen.getByRole('button', { name: 'Save' })); + + await waitFor(() => { + expect(screen.getByText('Failed to update storage backend')).toBeInTheDocument(); + }); + expect( + screen.getByText('Storage backend was modified by another request'), + ).toBeInTheDocument(); + expect(mockNavigate).not.toHaveBeenCalled(); + }); + + it('navigates back to the backends list on cancel', async () => { + const { user } = renderEditPage(); + + await waitFor(() => { + expect(screen.getByRole('button', { name: 'Cancel' })).toBeInTheDocument(); + }); + await user.click(screen.getByRole('button', { name: 'Cancel' })); + + expect(mockNavigate).toHaveBeenCalledWith('/admin/infrastructure/storage/backends'); + }); }); }); diff --git a/libs/ui-components/src/pages/admin/StorageBackendCreatePage.tsx b/libs/ui-components/src/pages/admin/StorageBackendCreatePage.tsx index 27f1aaf1..52a90dc6 100644 --- a/libs/ui-components/src/pages/admin/StorageBackendCreatePage.tsx +++ b/libs/ui-components/src/pages/admin/StorageBackendCreatePage.tsx @@ -1,4 +1,4 @@ -import { useNavigate } from 'react-router-dom'; +import { useNavigate, useParams } from 'react-router-dom'; import { ActionList, ActionListGroup, @@ -6,8 +6,10 @@ import { Alert, Breadcrumb, BreadcrumbItem, + Bullseye, Button, PageSection, + Spinner, Stack, StackItem, Title, @@ -16,7 +18,12 @@ import { Formik } from 'formik'; import type { TFunction } from 'i18next'; import * as Yup from 'yup'; -import { useCreateStorageBackend } from '@osac/ui-components/api/v1/private/storage-backends'; +import type { StorageBackend } from '@osac/types/private'; +import { + useCreateStorageBackend, + usePrivateStorageBackend, + useUpdateStorageBackend, +} from '@osac/ui-components/api/v1/private/storage-backends'; import NameField from '@osac/ui-components/components/catalogProvision/wizard/fields/NameField'; import { InputField } from '@osac/ui-components/components/Form/InputField'; import LeaveFormConfirmation from '@osac/ui-components/components/Form/LeaveFormConfirmation'; @@ -39,30 +46,49 @@ interface StorageBackendFormValues { credentials: { username: string; password: string }; } -const initialValues: StorageBackendFormValues = { - metadata: { name: '' }, - provider: '', - endpoint: '', - description: '', +const getInitialValues = (backend?: StorageBackend): StorageBackendFormValues => ({ + metadata: { name: backend?.metadata?.name ?? '' }, + provider: backend?.spec?.provider ?? '', + endpoint: backend?.spec?.endpoint ?? '', + description: backend?.spec?.description ?? '', credentials: { username: '', password: '' }, -}; +}); -const getStorageBackendSchema = (t: TFunction) => - Yup.object({ +const getStorageBackendSchema = (t: TFunction, isEdit: boolean) => { + const pairError = t('Enter both username and password, or leave both blank'); + return Yup.object({ metadata: Yup.object({ name: resourceNameSchema(t) }), provider: Yup.string().required(t('Provider is required')), endpoint: Yup.string().required(t('Endpoint is required')), description: Yup.string(), - credentials: Yup.object({ - username: Yup.string().required(t('Username is required')), - password: Yup.string().required(t('Password is required')), - }), + // On edit, credentials start blank and are all-or-nothing: both blank keeps them + // unchanged, both filled replaces them, exactly one filled is invalid (there's no + // server-side way to update just one). On create they're always required. + credentials: isEdit + ? Yup.object({ + username: Yup.string().test('credentials-pair', pairError, function (value) { + const parent = this.parent as { username?: string; password?: string } | undefined; + return !!value === !!parent?.password; + }), + password: Yup.string().test('credentials-pair', pairError, function (value) { + const parent = this.parent as { username?: string; password?: string } | undefined; + return !!value === !!parent?.username; + }), + }) + : Yup.object({ + username: Yup.string().required(t('Username is required')), + password: Yup.string().required(t('Password is required')), + }), }); +}; -export const StorageBackendCreatePage = () => { +const StorageBackendForm = ({ backend }: { backend?: StorageBackend }) => { const { t } = useTranslation(); const navigate = useNavigate(); - const { mutateAsync, error } = useCreateStorageBackend(); + const isEdit = !!backend; + const { mutateAsync: create, error: createError } = useCreateStorageBackend(); + const { mutateAsync: update, error: updateError } = useUpdateStorageBackend(); + const error = isEdit ? updateError : createError; const providerOptions: SelectFieldOption[] = [ { value: 'vast', label: t('VAST') }, @@ -70,6 +96,45 @@ export const StorageBackendCreatePage = () => { { value: 'pure', label: t('Pure') }, ]; + const onSubmit = async (values: StorageBackendFormValues) => { + try { + if (backend) { + await update({ + id: backend.id, + version: backend.metadata?.version ?? 0, + spec: { + endpoint: values.endpoint, + description: values.description, + ...(values.credentials.username && values.credentials.password + ? { + credentials: { + username: values.credentials.username, + password: values.credentials.password, + }, + } + : {}), + }, + }); + } else { + await create({ + metadata: values.metadata, + spec: { + provider: values.provider, + endpoint: values.endpoint, + description: values.description, + credentials: { + username: values.credentials.username, + password: values.credentials.password, + }, + }, + }); + } + navigate(BACKENDS_LIST_PATH); + } catch { + // Surfaced via the mutation's own `error` state below; nothing further to do here. + } + }; + return ( <> @@ -80,48 +145,31 @@ export const StorageBackendCreatePage = () => { {t('Storage backends')} - {t('Create')} + {isEdit ? t('Edit') : t('Create')} - {t('Create storage backend')} + {isEdit ? t('Edit storage backend') : t('Create storage backend')} { - try { - await mutateAsync({ - metadata: values.metadata, - spec: { - provider: values.provider, - endpoint: values.endpoint, - description: values.description, - credentials: { - username: values.credentials.username, - password: values.credentials.password, - }, - }, - }); - navigate(BACKENDS_LIST_PATH); - } catch { - // Surfaced via the mutation's own `error` state below; nothing further to do here. - } - }} + initialValues={getInitialValues(backend)} + validationSchema={getStorageBackendSchema(t, isEdit)} + onSubmit={onSubmit} > {({ submitForm, isSubmitting }) => ( - + { name="credentials.username" label={t('Username')} fieldId="storage-backend-username" - isRequired + isRequired={!isEdit} + helperText={ + isEdit ? t('Leave blank to keep the current credentials.') : undefined + } /> {!!error && ( - + {getErrorMessage(error)} @@ -168,7 +227,7 @@ export const StorageBackendCreatePage = () => { isDisabled={isSubmitting} isLoading={isSubmitting} > - {t('Create')} + {isEdit ? t('Save') : t('Create')} @@ -190,3 +249,29 @@ export const StorageBackendCreatePage = () => { ); }; + +export const StorageBackendCreatePage = () => { + const { t } = useTranslation(); + const { id } = useParams<{ id: string }>(); + const { data, isLoading, error } = usePrivateStorageBackend(id ?? ''); + + if (id) { + if (isLoading) { + return ( + + + + ); + } + + if (error) { + return ( + + {getErrorMessage(error)} + + ); + } + } + + return ; +};