diff --git a/workspaces/bulk-import/.changeset/nervous-waves-lie.md b/workspaces/bulk-import/.changeset/nervous-waves-lie.md new file mode 100644 index 0000000..6905f7b --- /dev/null +++ b/workspaces/bulk-import/.changeset/nervous-waves-lie.md @@ -0,0 +1,5 @@ +--- +'@red-hat-developer-hub/backstage-plugin-bulk-import': minor +--- + +update preview form to use separate formik context diff --git a/workspaces/bulk-import/plugins/bulk-import/package.json b/workspaces/bulk-import/plugins/bulk-import/package.json index 728e5f6..0219446 100644 --- a/workspaces/bulk-import/plugins/bulk-import/package.json +++ b/workspaces/bulk-import/plugins/bulk-import/package.json @@ -54,7 +54,8 @@ "js-yaml": "^4.1.0", "lodash": "^4.17.21", "react-use": "^17.2.4", - "yaml": "^2.0.0" + "yaml": "^2.0.0", + "yup": "^1.4.0" }, "peerDependencies": { "react": "16.13.1 || ^17.0.0 || ^18.0.0", diff --git a/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/KeyValueTextField.tsx b/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/KeyValueTextField.tsx index 0220f01..535e992 100644 --- a/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/KeyValueTextField.tsx +++ b/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/KeyValueTextField.tsx @@ -12,83 +12,28 @@ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. - */ import React, { useState } from 'react'; + */ +import React from 'react'; import { FormHelperText, TextField } from '@material-ui/core'; -import { PullRequestPreview, PullRequestPreviewData } from '../../types'; - interface KeyValueTextFieldProps { - repoId: string; label: string; name: string; value: string; onChange: ( event: React.FocusEvent, ) => void; - formErrors: PullRequestPreviewData; - setFormErrors: (pullRequest: PullRequestPreviewData) => void; + fieldError: string; } -const validateKeyValuePairs = (value: string): string | null => { - const keyValuePairs = value.split(';').map(pair => pair.trim()); - for (const pair of keyValuePairs) { - if (pair) { - const [key, val] = pair.split(':').map(part => part.trim()); - if (!key || !val) { - return 'Each entry must have a key and a value separated by a colon.'; - } - } - } - return null; -}; - -const KeyValueTextField: React.FC = ({ - repoId, +const KeyValueTextField = ({ label, name, value, onChange, - setFormErrors, - formErrors, -}) => { - const [error, setError] = useState(null); - const fieldName = name.split('.').pop() ?? ''; - - const removeError = () => { - const err = { ...formErrors }; - if (err[repoId]) { - delete err[repoId][fieldName as keyof PullRequestPreview]; - } - setFormErrors(err); - }; - - const getUpdatedFormError = ( - validationError: string, - ): PullRequestPreviewData => { - return { - ...formErrors, - [repoId]: { - ...(formErrors?.[repoId] || {}), - [fieldName]: validationError, - }, - }; - }; - - const handleChange = ( - event: React.FocusEvent, - ) => { - const validationError = validateKeyValuePairs(event.target.value); - if (validationError) { - setError(validationError); - setFormErrors(getUpdatedFormError(validationError)); - } else { - setError(null); - removeError(); - } - onChange(event); - }; - + fieldError, +}: KeyValueTextFieldProps) => { return (
= ({ fullWidth name={name} value={value} - onChange={handleChange} - error={!!error} - helperText={error} + onChange={onChange} + error={!!fieldError} + helperText={fieldError} /> Use semicolon to separate {label.toLocaleLowerCase('en-US')} diff --git a/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewFileSidebar.tsx b/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewFileSidebar.tsx index 78c9678..28b58e9 100644 --- a/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewFileSidebar.tsx +++ b/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewFileSidebar.tsx @@ -130,7 +130,7 @@ export const PreviewFileSidebar = ({ [id]: { error: { message: [ - `The entity YAML in your pull request is invalid (empty file or missing apiVersion, kind, or metadata.name). A new YAML has been generated below.`, + 'The entity YAML in your pull request is invalid (empty file or missing apiVersion, kind, or metadata.name). A new YAML has been generated below.', ], }, }, diff --git a/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewFileSidebarDrawerContent.tsx b/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewFileSidebarDrawerContent.tsx index 54c131a..98bf70f 100644 --- a/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewFileSidebarDrawerContent.tsx +++ b/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewFileSidebarDrawerContent.tsx @@ -199,7 +199,7 @@ export const PreviewFileSidebarDrawerContent = ({ isSubmitting || (!!formErrors && Object.values(formErrors).length > 0 && - Object.values(formErrors).every( + Object.values(formErrors).some( fe => !!fe && Object.values(fe).length > 0, )) } diff --git a/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewPullRequest.tsx b/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewPullRequest.tsx index 5ef927f..ae00a45 100644 --- a/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewPullRequest.tsx +++ b/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewPullRequest.tsx @@ -51,8 +51,6 @@ export const PreviewPullRequest = ({ }) => { const { status } = useFormikContext(); - const [entityOwner, setEntityOwner] = React.useState(''); - const error = status?.errors?.[repoId]; const info = status?.infos?.[repoId]; if ( @@ -114,8 +112,6 @@ export const PreviewPullRequest = ({ )} ({ - ...jest.requireActual('react'), - useState: jest.fn(), -})); - jest.mock('@material-ui/core', () => ({ ...jest.requireActual('@material-ui/core'), makeStyles: () => () => { @@ -64,12 +60,6 @@ class MockBulkImportApi { } } -const setState = jest.fn(); - -beforeEach(() => { - (useState as jest.Mock).mockImplementation(initial => [initial, setState]); -}); - const mockBulkImportApi = new MockBulkImportApi(); const mockCatalogApi: Partial = { @@ -110,8 +100,6 @@ describe('Preview Pull Request Form', () => { { { { /Add Backstage catalog entity descriptor files/, ); fireEvent.change(prTitle, { target: { value: '' } }); - expect(setFormErrors).toHaveBeenCalledWith({ - 'org/dessert/cupcake': { - prTitle: 'Pull request title is missing', - }, + await waitFor(() => { + expect( + screen.queryByText('Pull request title is required'), + ).toBeInTheDocument(); }); const componentName = getByPlaceholderText(/Component Name/); fireEvent.change(componentName, { target: { value: '' } }); - expect(setFormErrors).toHaveBeenCalledWith({ - 'org/dessert/cupcake': { - componentName: 'Component name is missing', - }, + await waitFor(() => { + expect( + screen.queryByText('Component name is required'), + ).toBeInTheDocument(); }); }); }); diff --git a/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewPullRequestForm.tsx b/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewPullRequestForm.tsx index ae88d21..3bdb698 100644 --- a/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewPullRequestForm.tsx +++ b/workspaces/bulk-import/plugins/bulk-import/src/components/PreviewFile/PreviewPullRequestForm.tsx @@ -12,7 +12,8 @@ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. - */ import * as React from 'react'; + */ +import * as React from 'react'; import { useAsync } from 'react-use'; import { Entity, EntityMeta } from '@backstage/catalog-model'; @@ -33,7 +34,7 @@ import Box from '@mui/material/Box'; import CircularProgress from '@mui/material/CircularProgress'; import FormControlLabel from '@mui/material/FormControlLabel'; import Typography from '@mui/material/Typography'; -import { useFormikContext } from 'formik'; +import { FormikErrors, useFormik, useFormikContext } from 'formik'; import { AddRepositoriesFormValues, @@ -41,8 +42,8 @@ import { PullRequestPreviewData, } from '../../types'; import { - componentNameRegex, convertKeyValuePairsToString, + getValidationSchema, getYamlKeyValuePairs, } from '../../utils/repository-utils'; import KeyValueTextField from './KeyValueTextField'; @@ -63,15 +64,11 @@ export const PreviewPullRequestForm = ({ setPullRequest, formErrors, setFormErrors, - entityOwner, - setEntityOwner, }: { repoId: string; repoUrl: string; pullRequest: PullRequestPreviewData; formErrors: PullRequestPreviewData; - entityOwner: string; - setEntityOwner: (name: string) => void; setPullRequest: (pullRequest: PullRequestPreviewData) => void; setFormErrors: (pullRequest: PullRequestPreviewData) => void; }) => { @@ -82,6 +79,16 @@ export const PreviewPullRequestForm = ({ const approvalTool = values.approvalTool === 'git' ? 'Pull request' : 'ServiceNow ticket'; + const formik = useFormik({ + enableReinitialize: true, + initialValues: pullRequest[repoId], + initialErrors: formErrors?.[ + repoId + ] as any as FormikErrors, + validationSchema: getValidationSchema(approvalTool), + onSubmit: () => {}, + }); + const { loading: entitiesLoading, value: entities } = useAsync(async () => { const allEntities = await catalogApi.getEntities({ filter: { @@ -94,34 +101,15 @@ export const PreviewPullRequestForm = ({ .sort((a, b) => a.localeCompare(b)); }); - React.useEffect(() => { - const newFormErrors = { - ...formErrors, - [repoId]: { - ...formErrors?.[repoId], - entityOwner: 'Entity owner is missing', - }, - }; - - if ( - Object.keys(pullRequest || {}).length > 0 && - !pullRequest[repoId]?.entityOwner && - !pullRequest[repoId]?.useCodeOwnersFile - ) { - if (JSON.stringify(formErrors) !== JSON.stringify(newFormErrors)) { - setFormErrors(newFormErrors); - } - } else if (pullRequest[repoId]?.entityOwner) { - setEntityOwner(pullRequest[repoId].entityOwner || 'user:default/guest'); - } - }, [setFormErrors, setEntityOwner, formErrors, repoId, pullRequest]); - const updatePullRequestKeyValuePairFields = ( field: string, value: string, yamlKey: string, ) => { - const yamlUpdate: Entity = { ...pullRequest[repoId]?.yaml }; + const yamlUpdate: Entity = { ...formik.values?.yaml }; + if (formik.values?.[field as keyof PullRequestPreview] !== value) { + formik.setFieldValue(field, value); + } if (value.length === 0) { if (yamlKey.includes('.')) { @@ -145,119 +133,65 @@ export const PreviewPullRequestForm = ({ setPullRequest({ ...pullRequest, [repoId]: { - ...pullRequest[repoId], + ...formik.values, [field]: value, yaml: yamlUpdate, }, }); }; - const updateFieldAndErrors = ( - field: string, - value: string, - errorMessage: string, - ) => { - setPullRequest({ - ...pullRequest, + const updateField = (field: string, value: string) => { + if (formik.values?.[field as keyof PullRequestPreview] !== value) { + formik.setFieldValue(field, value); + } + + const pr: PullRequestPreviewData = { [repoId]: { - ...pullRequest[repoId], + ...formik.values, [field]: value, ...(field === 'componentName' ? { yaml: { - ...pullRequest[repoId]?.yaml, + ...formik.values?.yaml, metadata: { - ...pullRequest[repoId]?.yaml.metadata, + ...formik.values?.yaml.metadata, name: value, }, }, } : {}), + ...(field === 'entityOwner' + ? { + yaml: { + ...formik.values?.yaml, + spec: { + ...formik.values?.yaml.spec, + owner: value, + }, + }, + } + : {}), }, - }); - - if (!value) { - setFormErrors({ - ...formErrors, - [repoId]: { - ...formErrors?.[repoId], - [field]: errorMessage, - }, - }); - } else if (field === 'componentName' && !componentNameRegex.exec(value)) { - setFormErrors({ - ...formErrors, - [repoId]: { - ...formErrors?.[repoId], - [field]: `"${value}" is not valid; expected a string that is sequences of [a-zA-Z0-9] separated by any of [-_.], at most 63 characters in total. To learn more about catalog file format, visit: https://github.com/backstage/backstage/blob/master/docs/architecture-decisions/adr002-default-catalog-file-format.md`, - }, - }); - } else { - const err = { ...formErrors }; - delete err[repoId]?.[field as keyof PullRequestPreview]; - setFormErrors(err); - } + }; + setPullRequest({ ...pullRequest, ...pr }); }; const handleChange = ( event: React.FocusEvent, ) => { - const targetName = event.target.name - .split('.') - .find(s => - [ - 'prTitle', - 'prDescription', - 'componentName', - 'prAnnotations', - 'prLabels', - 'prSpec', - 'entityOwner', - ].includes(s), - ); + const targetName = event.target.name; if (!targetName) return; - const inputValue = event.target.value; switch (targetName) { case 'prTitle': - updateFieldAndErrors( - 'prTitle', - inputValue, - `${approvalTool} title is missing`, - ); - break; case 'prDescription': - updateFieldAndErrors( - 'prDescription', - inputValue, - `${approvalTool} description is missing`, - ); - break; case 'entityOwner': - setPullRequest({ - ...pullRequest, - [repoId]: { - ...pullRequest[repoId], - entityOwner: inputValue, - yaml: { - ...pullRequest[repoId]?.yaml, - spec: { - ...pullRequest[repoId]?.yaml.spec, - ...(inputValue ? { owner: inputValue } : {}), - }, - }, - }, - }); - break; case 'componentName': - updateFieldAndErrors( - 'componentName', - inputValue, - 'Component name is missing', - ); - + case 'useCodeOwnersFile': + updateField(targetName, inputValue); break; + case 'prAnnotations': updatePullRequestKeyValuePairFields( 'prAnnotations', @@ -285,31 +219,44 @@ export const PreviewPullRequestForm = ({ label: 'Annotations', name: 'prAnnotations', value: - pullRequest[repoId]?.prAnnotations ?? + formik.values?.prAnnotations ?? convertKeyValuePairsToString( - pullRequest[repoId]?.yaml?.metadata?.annotations, + formik.values?.yaml?.metadata?.annotations, ), }, { label: 'Labels', name: 'prLabels', value: - pullRequest[repoId]?.prLabels ?? - convertKeyValuePairsToString( - pullRequest[repoId]?.yaml?.metadata?.labels, - ), + formik.values?.prLabels ?? + convertKeyValuePairsToString(formik.values?.yaml?.metadata?.labels), }, { label: 'Spec', name: 'prSpec', value: - pullRequest[repoId]?.prSpec ?? + formik.values?.prSpec ?? convertKeyValuePairsToString( - pullRequest[repoId]?.yaml?.spec as Record, + formik.values?.yaml?.spec as Record, ), }, ]; + React.useEffect(() => { + const err = { + ...formErrors, + [repoId]: formik.errors as any as PullRequestPreview, + }; + + if (!formik.errors) { + delete err[repoId]; + } + + setFormErrors(err); + + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [formik.errors]); + return ( <> @@ -322,10 +269,11 @@ export const PreviewPullRequestForm = ({ variant="outlined" margin="normal" fullWidth - name={`repositories.${pullRequest[repoId]?.componentName}.prTitle`} - value={pullRequest[repoId]?.prTitle} + name="prTitle" + value={formik.values?.prTitle} onChange={handleChange} - error={!!formErrors?.[repoId]?.prTitle} + error={!!formik.errors?.prTitle} + helperText={formik.errors?.prTitle} required /> @@ -336,9 +284,10 @@ export const PreviewPullRequestForm = ({ variant="outlined" fullWidth onChange={handleChange} - name={`repositories.${pullRequest[repoId]?.componentName}.prDescription`} - value={pullRequest[repoId]?.prDescription} - error={!!formErrors?.[repoId]?.prDescription} + name="prDescription" + value={formik.values?.prDescription} + error={!!formik.errors?.prDescription} + helperText={formik.errors?.prDescription} multiline required /> @@ -353,63 +302,43 @@ export const PreviewPullRequestForm = ({ margin="normal" variant="outlined" onChange={handleChange} - value={pullRequest[repoId]?.componentName} - name={`repositories.${pullRequest[repoId]?.componentName}.componentName`} - error={!!formErrors?.[repoId]?.componentName} - helperText={ - formErrors?.[repoId]?.componentName - ? formErrors?.[repoId]?.componentName - : '' - } + name="componentName" + value={formik.values?.componentName} + error={!!formik.errors?.componentName} + helperText={formik.errors?.componentName} fullWidth required />

- {!pullRequest[repoId]?.useCodeOwnersFile && ( + {!formik.values?.useCodeOwnersFile && ( , value: string | null) => { - setEntityOwner(value || ''); + onChange={(_event, value) => handleChange({ target: { name: 'entityOwner', value }, - } as any); - }} - onInputChange={(_e, newSearch: string) => { - setEntityOwner(newSearch || ''); + } as any) + } + onInputChange={(_event, value) => handleChange({ - target: { name: 'entityOwner', value: newSearch }, - } as any); - if (!newSearch && !pullRequest[repoId]?.useCodeOwnersFile) { - setFormErrors({ - ...formErrors, - [repoId]: { - ...formErrors?.[repoId], - entityOwner: 'Entity Owner is required', - }, - }); - } else { - const err = { ...formErrors }; - delete err[repoId]?.entityOwner; - setFormErrors(err); - } - }} + target: { name: 'entityOwner', value }, + } as any) + } renderInput={params => ( ) => { - const pr = { - ...pullRequest, - [repoId]: { - ...pullRequest[repoId], - useCodeOwnersFile: event.target.checked, + name="useCodeOwnersFile" + checked={formik.values?.useCodeOwnersFile} + onChange={(event: React.ChangeEvent) => + handleChange({ + target: { + name: 'useCodeOwnersFile', + value: event.target.checked, }, - }; - - delete pr[repoId]?.entityOwner; - delete pr[repoId]?.yaml?.spec?.owner; - - setPullRequest(pr); - if (event.target.checked) { - const err = { ...formErrors }; - delete err[repoId]?.entityOwner; - setFormErrors(err); - } - }} + } as any) + } /> } label={ @@ -463,18 +382,20 @@ export const PreviewPullRequestForm = ({ WARNING: This may fail if no CODEOWNERS file is found at the target location. - {keyValueTextFields.map(field => ( - - ))} + {keyValueTextFields.map(field => { + return ( + + ); + })} Preview {`${approvalTool.toLocaleLowerCase('en-US')}`} @@ -482,8 +403,8 @@ export const PreviewPullRequestForm = ({ { repositories: { ['org/dessert/cupcake']: { ...mockGetImportJobs.imports[0], + status: RepositoryStatus.ADDED, catalogInfoYaml: { status: RepositoryStatus.ADDED, }, diff --git a/workspaces/bulk-import/plugins/bulk-import/src/utils/repository-utils.tsx b/workspaces/bulk-import/plugins/bulk-import/src/utils/repository-utils.tsx index 48de992..6e21811 100644 --- a/workspaces/bulk-import/plugins/bulk-import/src/utils/repository-utils.tsx +++ b/workspaces/bulk-import/plugins/bulk-import/src/utils/repository-utils.tsx @@ -12,7 +12,8 @@ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. - */ import * as React from 'react'; + */ +import * as React from 'react'; import { Entity } from '@backstage/catalog-model'; import { StatusOK, StatusPending } from '@backstage/core-components'; @@ -20,6 +21,7 @@ import { StatusOK, StatusPending } from '@backstage/core-components'; import * as jsyaml from 'js-yaml'; import { get } from 'lodash'; import * as yaml from 'yaml'; +import * as yup from 'yup'; import GitAltIcon from '../components/GitAltIcon'; import { @@ -383,7 +385,8 @@ export const convertKeyValuePairsToString = ( ): string => { return keyValuePairs ? Object.entries(keyValuePairs) - .map(([key, value]) => `${key.trim()}: ${value.trim()}`) + .filter(val => val) + .map(([key, value]) => `${key.trim()}: ${value?.trim() || ''}`) .join('; ') : ''; }; @@ -612,3 +615,48 @@ export const prepareDataForAddedRepositories = ( }, {}); return repoData; }; + +const validateKeyValuePair = yup + .string() + .nullable() + .test( + 'is-key-value-pair', + 'Each entry must have a key and a value separated by a colon.', + value => { + if (!value) return true; + const keyValuePairs = value.split(';').map(pair => pair.trim()); + for (const pair of keyValuePairs) { + if (pair) { + const [key, val] = pair.split(':').map(part => part.trim()); + if (!key || !val) { + return false; + } + } + } + return true; + }, + ); + +export const getValidationSchema = (approvalTool: string) => + yup.object().shape({ + prTitle: yup.string().required(`${approvalTool} title is required`), + prDescription: yup + .string() + .required(`${approvalTool} description is required`), + componentName: yup + .string() + .matches( + componentNameRegex, + `"${yup.string()}" is not valid; expected a string that is sequences of [a-zA-Z0-9] separated by any of [-_.], at most 63 characters in total. To learn more about catalog file format, visit: https://github.com/backstage/backstage/blob/master/docs/architecture-decisions/adr002-default-catalog-file-format.md`, + ) + .required('Component name is required'), + useCodeOwnersFile: yup.boolean(), + entityOwner: yup.string().when('useCodeOwnersFile', { + is: false, + then: schema => schema.required('Entity Owner is required'), + otherwise: schema => schema.notRequired(), + }), + prLabels: validateKeyValuePair, + prAnnotations: validateKeyValuePair, + prSpec: validateKeyValuePair, + }); diff --git a/workspaces/bulk-import/yarn.lock b/workspaces/bulk-import/yarn.lock index d6bc4ec..5f1b901 100644 --- a/workspaces/bulk-import/yarn.lock +++ b/workspaces/bulk-import/yarn.lock @@ -11107,6 +11107,7 @@ __metadata: react-use: ^17.2.4 start-server-and-test: 2.0.8 yaml: ^2.0.0 + yup: ^1.4.0 peerDependencies: react: 16.13.1 || ^17.0.0 || ^18.0.0 react-router-dom: ^6.0.0 @@ -37586,7 +37587,7 @@ __metadata: languageName: node linkType: hard -"yup@npm:^1.0.0": +"yup@npm:^1.0.0, yup@npm:^1.4.0": version: 1.4.0 resolution: "yup@npm:1.4.0" dependencies: