From fdf4935e423252b8ea55628a09f9cd16a43b1359 Mon Sep 17 00:00:00 2001 From: Yunwen Zheng Date: Tue, 1 Jul 2025 12:19:48 -0400 Subject: [PATCH] Git sync create folder flow refactor to use new hook (#107422) * Git sync create folder flow refactor to use new hook --------- Co-authored-by: Alex Khomenko --- .betterer.results | 6 - .../components/CreateNewButton.tsx | 6 +- .../DeleteProvisionedFolderForm.test.tsx | 4 +- .../DeleteProvisionedFolderForm.tsx | 4 +- .../NewProvisionedFolderForm.test.tsx | 157 +++++----- .../components/NewProvisionedFolderForm.tsx | 270 ++++++++---------- ... => ResourceEditFormSharedFields.test.tsx} | 8 +- ...s.tsx => ResourceEditFormSharedFields.tsx} | 27 +- .../SaveProvisionedDashboardForm.tsx | 4 +- .../DeleteProvisionedDashboardForm.test.tsx | 4 +- .../DeleteProvisionedDashboardForm.tsx | 4 +- public/locales/en-US/grafana.json | 6 - 12 files changed, 225 insertions(+), 275 deletions(-) rename public/app/features/dashboard-scene/components/Provisioned/{DashboardEditFormSharedFields.test.tsx => ResourceEditFormSharedFields.test.tsx} (96%) rename public/app/features/dashboard-scene/components/Provisioned/{DashboardEditFormSharedFields.tsx => ResourceEditFormSharedFields.tsx} (82%) diff --git a/.betterer.results b/.betterer.results index cc04defd879..f5c2127333f 100644 --- a/.betterer.results +++ b/.betterer.results @@ -1493,12 +1493,6 @@ exports[`better eslint`] = { "public/app/features/browse-dashboards/components/NewFolderForm.tsx:5381": [ [0, 0, 0, "Add noMargin prop to Field components to remove built-in margins. Use layout components like Stack or Grid with the gap prop instead for consistent spacing.", "0"] ], - "public/app/features/browse-dashboards/components/NewProvisionedFolderForm.tsx:5381": [ - [0, 0, 0, "Add noMargin prop to Field components to remove built-in margins. Use layout components like Stack or Grid with the gap prop instead for consistent spacing.", "0"], - [0, 0, 0, "Add noMargin prop to Field components to remove built-in margins. Use layout components like Stack or Grid with the gap prop instead for consistent spacing.", "1"], - [0, 0, 0, "Add noMargin prop to Field components to remove built-in margins. Use layout components like Stack or Grid with the gap prop instead for consistent spacing.", "2"], - [0, 0, 0, "Add noMargin prop to Field components to remove built-in margins. Use layout components like Stack or Grid with the gap prop instead for consistent spacing.", "3"] - ], "public/app/features/browse-dashboards/state/index.ts:5381": [ [0, 0, 0, "Do not use export all (\`export * from ...\`)", "0"], [0, 0, 0, "Do not use export all (\`export * from ...\`)", "1"], diff --git a/public/app/features/browse-dashboards/components/CreateNewButton.tsx b/public/app/features/browse-dashboards/components/CreateNewButton.tsx index 600d0038224..bb00a254d31 100644 --- a/public/app/features/browse-dashboards/components/CreateNewButton.tsx +++ b/public/app/features/browse-dashboards/components/CreateNewButton.tsx @@ -107,11 +107,7 @@ export default function CreateNewButton({ parentFolder, canCreateDashboard, canC size="sm" > {parentFolder?.managedBy === ManagerKind.Repo || isProvisionedInstance ? ( - setShowNewFolderDrawer(false)} - onCancel={() => setShowNewFolderDrawer(false)} - parentFolder={parentFolder} - /> + setShowNewFolderDrawer(false)} parentFolder={parentFolder} /> ) : ( setShowNewFolderDrawer(false)} /> )} diff --git a/public/app/features/browse-dashboards/components/DeleteProvisionedFolderForm.test.tsx b/public/app/features/browse-dashboards/components/DeleteProvisionedFolderForm.test.tsx index d6f62f10e0f..f56e50bf023 100644 --- a/public/app/features/browse-dashboards/components/DeleteProvisionedFolderForm.test.tsx +++ b/public/app/features/browse-dashboards/components/DeleteProvisionedFolderForm.test.tsx @@ -33,8 +33,8 @@ jest.mock('./BrowseActions/DescendantCount', () => ({ DescendantCount: () =>
2 folders, 5 dashboards
, })); -jest.mock('app/features/dashboard-scene/components/Provisioned/DashboardEditFormSharedFields', () => ({ - DashboardEditFormSharedFields: () =>
, +jest.mock('app/features/dashboard-scene/components/Provisioned/ResourceEditFormSharedFields', () => ({ + ResourceEditFormSharedFields: () =>
, })); const mockUseDeleteRepositoryFilesMutation = useDeleteRepositoryFilesWithPathMutation as jest.MockedFunction< diff --git a/public/app/features/browse-dashboards/components/DeleteProvisionedFolderForm.tsx b/public/app/features/browse-dashboards/components/DeleteProvisionedFolderForm.tsx index fac9bbc6864..995f68bbd23 100644 --- a/public/app/features/browse-dashboards/components/DeleteProvisionedFolderForm.tsx +++ b/public/app/features/browse-dashboards/components/DeleteProvisionedFolderForm.tsx @@ -8,7 +8,7 @@ import { Box, Button, Stack } from '@grafana/ui'; import { Folder } from 'app/api/clients/folder/v1beta1'; import { RepositoryView, useDeleteRepositoryFilesWithPathMutation } from 'app/api/clients/provisioning/v0alpha1'; import { AnnoKeySourcePath } from 'app/features/apiserver/types'; -import { DashboardEditFormSharedFields } from 'app/features/dashboard-scene/components/Provisioned/DashboardEditFormSharedFields'; +import { ResourceEditFormSharedFields } from 'app/features/dashboard-scene/components/Provisioned/ResourceEditFormSharedFields'; import { BaseProvisionedFormData } from 'app/features/dashboard-scene/saving/shared'; import { FolderDTO } from 'app/types'; @@ -121,7 +121,7 @@ function FormContent({ /> - { }; }); -jest.mock('app/api/clients/folder/v1beta1', () => { +jest.mock('../hooks/useProvisionedFolderFormData', () => { return { - useGetFolderQuery: jest.fn(), + useProvisionedFolderFormData: jest.fn(), }; }); @@ -53,12 +52,6 @@ jest.mock('app/features/provisioning/hooks/usePullRequestParam', () => { }; }); -jest.mock('app/features/provisioning/hooks/useGetResourceRepositoryView', () => { - return { - useGetResourceRepositoryView: jest.fn(), - }; -}); - jest.mock('react-router-dom-v5-compat', () => { const actual = jest.requireActual('react-router-dom-v5-compat'); return { @@ -79,17 +72,15 @@ jest.mock('../../dashboard-scene/saving/provisioned/defaults', () => { }); interface Props { - onSubmit: () => void; - onCancel: () => void; - parentFolder: FolderDTO; + onDismiss?: () => void; + parentFolder?: FolderDTO; } -function setup(props: Partial = {}) { +function setup(props: Partial = {}, hookData = mockHookData) { const user = userEvent.setup(); const defaultProps: Props = { - onSubmit: jest.fn(), - onCancel: jest.fn(), + onDismiss: jest.fn(), parentFolder: { id: 1, uid: 'folder-uid', @@ -108,6 +99,8 @@ function setup(props: Partial = {}) { ...props, }; + (useProvisionedFolderFormData as jest.Mock).mockReturnValue(hookData); + return { user, ...render(), @@ -123,6 +116,40 @@ const mockRequest = { data: { resource: { upsert: { metadata: { name: 'new-folder' } } } }, }; +const mockHookData: ProvisionedFolderFormDataResult = { + repository: { + name: 'test-repo', + title: 'Test Repository', + type: 'github', + workflows: ['write', 'branch'], + target: 'folder', + }, + folder: { + metadata: { + annotations: { + 'grafana.app/sourcePath': '/dashboards', + }, + }, + spec: { + title: '', + }, + status: {}, + }, + workflowOptions: [ + { label: 'Commit directly', value: 'write' }, + { label: 'Create a branch', value: 'branch' }, + ], + isGitHub: true, + initialValues: { + title: '', + comment: '', + ref: 'folder/test-timestamp', + repo: 'test-repo', + path: '/dashboards', + workflow: 'write', + }, +}; + describe('NewProvisionedFolderForm', () => { beforeEach(() => { jest.clearAllMocks(); @@ -133,39 +160,11 @@ describe('NewProvisionedFolderForm', () => { }; (getAppEvents as jest.Mock).mockReturnValue(mockAppEvents); - (useGetResourceRepositoryView as jest.Mock).mockReturnValue({ - isLoading: false, - repository: { - name: 'test-repo', - title: 'Test Repository', - type: 'github', - github: { - url: 'https://github.com/grafana/grafana', - branch: 'main', - }, - workflows: [{ name: 'default', path: 'workflows/default.json' }], - }, - }); - - // Mock useGetFolderQuery - (useGetFolderQuery as jest.Mock).mockReturnValue({ - data: { - metadata: { - annotations: { - 'source.path': '/dashboards', - }, - }, - }, - isLoading: false, - isError: false, - }); - // Mock usePullRequestParam (usePullRequestParam as jest.Mock).mockReturnValue(null); // Mock useCreateRepositoryFilesWithPathMutation const mockCreate = jest.fn(); - (useCreateRepositoryFilesWithPathMutation as jest.Mock).mockReturnValue([mockCreate, mockRequest]); (validationSrv.validateNewFolderName as jest.Mock).mockResolvedValue(true); @@ -182,25 +181,27 @@ describe('NewProvisionedFolderForm', () => { expect(screen.getByRole('button', { name: /cancel/i })).toBeInTheDocument(); }); - it('should show loading state when repository data is loading', () => { - (useGetResourceRepositoryView as jest.Mock).mockReturnValue({ - isLoading: true, - }); - - setup(); - - expect(screen.getByTestId('Spinner')).toBeInTheDocument(); + it('should return null when initialValues is not available', () => { + const { container } = setup( + {}, + { + ...mockHookData, + initialValues: undefined, + } + ); + expect(container.firstChild).toBeNull(); }); it('should show error when repository is not found', () => { - (useGetResourceRepositoryView as jest.Mock).mockReturnValue({ - isLoading: false, - repository: undefined, - }); - - setup(); - - expect(screen.getByText('Repository not found')).toBeInTheDocument(); + const { container } = setup( + {}, + { + ...mockHookData, + repository: undefined, + initialValues: undefined, + } + ); + expect(container.firstChild).toBeNull(); }); it('should show branch field when branch workflow is selected', async () => { @@ -289,6 +290,7 @@ describe('NewProvisionedFolderForm', () => { expect.objectContaining({ ref: undefined, // write workflow uses undefined ref name: 'test-repo', + path: '/dashboards/new-test-folder/', message: 'Creating a new test folder', body: { title: 'New Test Folder', @@ -298,8 +300,8 @@ describe('NewProvisionedFolderForm', () => { ); }); - // Check if onSubmit was called - expect(props.onSubmit).toHaveBeenCalled(); + // Check if onDismiss was called + expect(props.onDismiss).toHaveBeenCalled(); }); it('should create folder with branch workflow', async () => { @@ -341,6 +343,7 @@ describe('NewProvisionedFolderForm', () => { expect.objectContaining({ ref: 'feature/new-folder', name: 'test-repo', + path: '/dashboards/branch-folder/', message: 'Create folder: Branch Folder', body: { title: 'Branch Folder', @@ -415,33 +418,31 @@ describe('NewProvisionedFolderForm', () => { expect(screen.getByRole('link')).toHaveTextContent('https://github.com/grafana/grafana/pull/1234'); }); - it('should call onCancel when cancel button is clicked', async () => { + it('should call onDismiss when cancel button is clicked', async () => { const { user, props } = setup(); // Click cancel button const cancelButton = screen.getByRole('button', { name: /cancel/i }); await user.click(cancelButton); - // Check if onCancel was called - expect(props.onCancel).toHaveBeenCalled(); + expect(props.onDismiss).toHaveBeenCalled(); }); it('should show read-only alert when repository has no workflows', () => { // Mock repository with empty workflows array - (useGetResourceRepositoryView as jest.Mock).mockReturnValue({ - repository: { - name: 'test-repo', - title: 'Test Repository', - type: 'github', - github: { - url: 'https://github.com/grafana/grafana', - branch: 'main', + setup( + {}, + { + ...mockHookData, + repository: { + name: 'test-repo', + title: 'Test Repository', + type: 'github', + workflows: [], + target: 'folder', }, - workflows: [], - }, - }); - - setup(); + } + ); // Read-only alert should be visible expect(screen.getByText('This repository is read only')).toBeInTheDocument(); diff --git a/public/app/features/browse-dashboards/components/NewProvisionedFolderForm.tsx b/public/app/features/browse-dashboards/components/NewProvisionedFolderForm.tsx index 617d96d5087..ed03a23cb7a 100644 --- a/public/app/features/browse-dashboards/components/NewProvisionedFolderForm.tsx +++ b/public/app/features/browse-dashboards/components/NewProvisionedFolderForm.tsx @@ -1,73 +1,52 @@ import { useEffect } from 'react'; -import { Controller, useForm } from 'react-hook-form'; +import { FormProvider, useForm } from 'react-hook-form'; import { useNavigate } from 'react-router-dom-v5-compat'; import { AppEvents } from '@grafana/data'; import { Trans, t } from '@grafana/i18n'; import { getAppEvents } from '@grafana/runtime'; -import { Alert, Button, Field, Input, RadioButtonGroup, Spinner, Stack, TextArea } from '@grafana/ui'; -import { useCreateRepositoryFilesWithPathMutation } from 'app/api/clients/provisioning/v0alpha1'; +import { Alert, Button, Field, Input, Stack } from '@grafana/ui'; +import { Folder } from 'app/api/clients/folder/v1beta1'; +import { RepositoryView, useCreateRepositoryFilesWithPathMutation } from 'app/api/clients/provisioning/v0alpha1'; import { AnnoKeySourcePath, Resource } from 'app/features/apiserver/types'; -import { getDefaultWorkflow, getWorkflowOptions } from 'app/features/dashboard-scene/saving/provisioned/defaults'; +import { ResourceEditFormSharedFields } from 'app/features/dashboard-scene/components/Provisioned/ResourceEditFormSharedFields'; +import { BaseProvisionedFormData } from 'app/features/dashboard-scene/saving/shared'; import { validationSrv } from 'app/features/manage-dashboards/services/ValidationSrv'; -import { BranchValidationError } from 'app/features/provisioning/Shared/BranchValidationError'; import { PROVISIONING_URL } from 'app/features/provisioning/constants'; -import { useGetResourceRepositoryView } from 'app/features/provisioning/hooks/useGetResourceRepositoryView'; import { usePullRequestParam } from 'app/features/provisioning/hooks/usePullRequestParam'; -import { WorkflowOption } from 'app/features/provisioning/types'; -import { validateBranchName } from 'app/features/provisioning/utils/git'; import { FolderDTO } from 'app/types'; -type FormData = { - ref?: string; - path: string; - comment?: string; - repo: string; - workflow?: WorkflowOption; - title: string; -}; - +import { useProvisionedFolderFormData } from '../hooks/useProvisionedFolderFormData'; +interface FormProps extends Props { + initialValues: BaseProvisionedFormData; + repository?: RepositoryView; + workflowOptions: Array<{ label: string; value: string }>; + folder?: Folder; + isGitHub: boolean; +} interface Props { - onSubmit: () => void; - onCancel: () => void; parentFolder?: FolderDTO; + onDismiss?: () => void; } -const initialFormValues: Partial = { - title: '', - comment: '', - ref: `folder/${Date.now()}`, -}; - -// TODO: use useProvisionedFolderFormData hook to manage form data and repository state -export function NewProvisionedFolderForm({ onSubmit, onCancel, parentFolder }: Props) { - const { repository, folder, isLoading } = useGetResourceRepositoryView({ folderName: parentFolder?.uid }); +function FormContent({ initialValues, repository, workflowOptions, folder, isGitHub, onDismiss }: FormProps) { const prURL = usePullRequestParam(); const navigate = useNavigate(); const [create, request] = useCreateRepositoryFilesWithPathMutation(); - const isGitHub = Boolean(repository?.type === 'github'); - - const { - register, - handleSubmit, - watch, - formState: { errors }, - control, - setValue, - } = useForm({ defaultValues: { ...initialFormValues, workflow: getDefaultWorkflow(repository) } }); + const methods = useForm({ + defaultValues: initialValues, + mode: 'onBlur', // Validates when user leaves the field + }); + const { handleSubmit, watch, register, formState } = methods; const [workflow, ref] = watch(['workflow', 'ref']); - useEffect(() => { - setValue('workflow', getDefaultWorkflow(repository)); - }, [repository, setValue]); - // TODO: replace with useProvisionedRequestHandler hook useEffect(() => { const appEvents = getAppEvents(); if (request.isSuccess && repository) { - onSubmit(); + onDismiss?.(); appEvents.publish({ type: AppEvents.alertSuccess.name, @@ -101,20 +80,7 @@ export function NewProvisionedFolderForm({ onSubmit, onCancel, parentFolder }: P ], }); } - }, [request.isSuccess, request.isError, request.error, onSubmit, ref, request.data, workflow, navigate, repository]); - - if (isLoading) { - return ; - } - - if (!repository) { - return ( - - ); - } + }, [request.isSuccess, request.isError, request.error, ref, request.data, workflow, navigate, repository, onDismiss]); const validateFolderName = async (folderName: string) => { try { @@ -128,7 +94,7 @@ export function NewProvisionedFolderForm({ onSubmit, onCancel, parentFolder }: P } }; - const doSave = async ({ ref, title, workflow, comment }: FormData) => { + const doSave = async ({ ref, title, workflow, comment }: BaseProvisionedFormData) => { const repoName = repository?.name; if (!title || !repoName) { return; @@ -163,107 +129,103 @@ export function NewProvisionedFolderForm({ onSubmit, onCancel, parentFolder }: P }; return ( -
- - {!repository?.workflows?.length && ( - + + + {!repository?.workflows?.length && ( + + + If you have direct access to the target, copy the JSON and paste it there. + + + )} + + - - If you have direct access to the target, copy the JSON and paste it there. - - - )} + + - - - - {/* TODO: use DashboardEditFormSharedFields to replace comment and workflow input*/} - -