From ba57158fed8594ddb726add8af571ee0b400c530 Mon Sep 17 00:00:00 2001 From: Christian Vogt Date: Mon, 26 Aug 2024 10:47:02 -0400 Subject: [PATCH] surface conflicting env var names as warnings and errors --- .../src/concepts/connectionTypes/types.ts | 4 ++ .../manage/ConnectionTypeDataFieldModal.tsx | 49 ++++++++++------- .../manage/ConnectionTypeFieldModal.tsx | 1 + .../ManageConnectionTypeFieldsTable.tsx | 14 ++--- .../ManageConnectionTypeFieldsTableRow.tsx | 29 ++++++++-- .../manage/ManageConnectionTypePage.tsx | 22 ++++++-- .../ConnectionTypeDataFieldModal.spec.tsx | 35 ++++++++++++ ...anageConnectionTypeFieldsTableRow.spec.tsx | 54 +++++++++++++++++++ .../manage/fieldTableColumns.ts | 9 ++++ 9 files changed, 182 insertions(+), 35 deletions(-) create mode 100644 frontend/src/pages/connectionTypes/manage/__tests__/ManageConnectionTypeFieldsTableRow.spec.tsx create mode 100644 frontend/src/pages/connectionTypes/manage/fieldTableColumns.ts diff --git a/frontend/src/concepts/connectionTypes/types.ts b/frontend/src/concepts/connectionTypes/types.ts index 79d7311b17..180f7685f0 100644 --- a/frontend/src/concepts/connectionTypes/types.ts +++ b/frontend/src/concepts/connectionTypes/types.ts @@ -93,6 +93,10 @@ export type ConnectionTypeField = export type ConnectionTypeDataField = Exclude; +export const isConnectionTypeDataField = ( + field: ConnectionTypeField, +): field is ConnectionTypeDataField => field.type !== ConnectionTypeFieldType.Section; + export type ConnectionTypeConfigMap = K8sResourceCommon & { metadata: { name: string; diff --git a/frontend/src/pages/connectionTypes/manage/ConnectionTypeDataFieldModal.tsx b/frontend/src/pages/connectionTypes/manage/ConnectionTypeDataFieldModal.tsx index 37ec563409..dd0e13f85c 100644 --- a/frontend/src/pages/connectionTypes/manage/ConnectionTypeDataFieldModal.tsx +++ b/frontend/src/pages/connectionTypes/manage/ConnectionTypeDataFieldModal.tsx @@ -14,20 +14,24 @@ import { SelectOption, TextArea, TextInput, - ValidatedOptions, } from '@patternfly/react-core'; -import { ExclamationCircleIcon, OutlinedQuestionCircleIcon } from '@patternfly/react-icons'; +import { + ExclamationCircleIcon, + OutlinedQuestionCircleIcon, + WarningTriangleIcon, +} from '@patternfly/react-icons'; import DashboardModalFooter from '~/concepts/dashboard/DashboardModalFooter'; import { ConnectionTypeDataField, connectionTypeDataFields, + ConnectionTypeField, ConnectionTypeFieldType, + isConnectionTypeDataField, } from '~/concepts/connectionTypes/types'; -import { fieldNameToEnvVar } from '~/concepts/connectionTypes/utils'; +import { fieldNameToEnvVar, fieldTypeToString } from '~/concepts/connectionTypes/utils'; import { isEnumMember } from '~/utilities/utils'; import DashboardPopupIconButton from '~/concepts/dashboard/DashboardPopupIconButton'; import DataFieldPropertiesForm from '~/pages/connectionTypes/manage/DataFieldPropertiesForm'; -import { fieldTypeToString } from '~/concepts/connectionTypes/utils'; const ENV_VAR_NAME_REGEX = new RegExp('^[-._a-zA-Z][-._a-zA-Z0-9]*$'); @@ -42,6 +46,7 @@ type Props = { onClose: () => void; onSubmit: (field: ConnectionTypeDataField) => void; isEdit?: boolean; + fields?: ConnectionTypeField[]; }; export const ConnectionTypeDataFieldModal: React.FC = ({ @@ -50,6 +55,7 @@ export const ConnectionTypeDataFieldModal: React.FC = ({ onClose, onSubmit, isEdit, + fields, }) => { const [name, setName] = React.useState(field?.name || ''); const [description, setDescription] = React.useState(field?.description); @@ -65,16 +71,16 @@ export const ConnectionTypeDataFieldModal: React.FC = ({ const [isTypeSelectOpen, setIsTypeSelectOpen] = React.useState(false); const [properties, setProperties] = React.useState(field?.properties || {}); const [isPropertiesValid, setPropertiesValid] = React.useState(true); - const [autoGenerateEnvVar, setAutoGenerateEnvVar] = React.useState(!envVar); - const envVarValidation = - !envVar || ENV_VAR_NAME_REGEX.test(envVar) ? ValidatedOptions.default : ValidatedOptions.error; - const isValid = - !!fieldType && - isPropertiesValid && - !!name && - !!envVar && - envVarValidation === ValidatedOptions.default; + + const isEnvVarConflict = React.useMemo( + () => !!fields?.find((f) => f !== field && isConnectionTypeDataField(f) && f.envVar === envVar), + [fields, field, envVar], + ); + + const isEnvVarValid = !envVar || ENV_VAR_NAME_REGEX.test(envVar); + + const isValid = !!fieldType && isPropertiesValid && !!name && !!envVar && isEnvVarValid; const newField = fieldType ? // Cast from specific type to generic type @@ -179,17 +185,24 @@ export const ConnectionTypeDataFieldModal: React.FC = ({ setEnvVar(value); }} data-testid="field-env-var-input" - validated={envVarValidation} + validated={!isEnvVarValid ? 'error' : isEnvVarConflict ? 'warning' : 'default'} /> - {envVarValidation === ValidatedOptions.error ? ( - + + {!isEnvVarValid ? ( } variant="error"> {`Invalid variable name. The name must consist of alphabetic characters, digits, '_', '-', or '.', and must not start with a digit.`} - - ) : null} + ) : undefined} + {isEnvVarConflict ? ( + + } variant="warning"> + This environment variable name is already being used for an existing field. + + + ) : undefined} + void; onSubmit: (field: ConnectionTypeField) => void; isEdit?: boolean; + fields?: ConnectionTypeField[]; }; const ConnectionTypeFieldModal: React.FC = (props) => { diff --git a/frontend/src/pages/connectionTypes/manage/ManageConnectionTypeFieldsTable.tsx b/frontend/src/pages/connectionTypes/manage/ManageConnectionTypeFieldsTable.tsx index 87b5480e05..35ee9467ec 100644 --- a/frontend/src/pages/connectionTypes/manage/ManageConnectionTypeFieldsTable.tsx +++ b/frontend/src/pages/connectionTypes/manage/ManageConnectionTypeFieldsTable.tsx @@ -13,9 +13,10 @@ import { EmptyStateVariant, } from '@patternfly/react-core'; import { PlusCircleIcon } from '@patternfly/react-icons'; -import { Table, Thead, Tbody, Tr, Th, ThProps } from '@patternfly/react-table'; +import { Table, Thead, Tbody, Tr, Th } from '@patternfly/react-table'; import { ConnectionTypeField, ConnectionTypeFieldType } from '~/concepts/connectionTypes/types'; import useDraggableTableControlled from '~/utilities/useDraggableTableControlled'; +import { columns } from '~/pages/connectionTypes/manage/fieldTableColumns'; import ConnectionTypeFieldModal from './ConnectionTypeFieldModal'; import ManageConnectionTypeFieldsTableRow from './ManageConnectionTypeFieldsTableRow'; @@ -51,14 +52,6 @@ type Props = { onFieldsChange: (fields: ConnectionTypeField[]) => void; }; -const columns: ThProps[] = [ - { label: 'Section heading/field name', width: 30 }, - { label: 'Type', width: 10 }, - { label: 'Default value', width: 30 }, - { label: 'Environment variable', width: 20 }, - { label: 'Required', width: 10 }, -]; - const ManageConnectionTypeFieldsTable: React.FC = ({ fields, onFieldsChange }) => { const [modalField, setModalField] = React.useState< { field?: ConnectionTypeField; index?: number; isEdit?: boolean } | undefined @@ -88,9 +81,9 @@ const ManageConnectionTypeFieldsTable: React.FC = ({ fields, onFieldsChan {rowsToRender.map(({ data: row, rowProps }, index) => ( { setModalField({ field: row, @@ -154,6 +147,7 @@ const ManageConnectionTypeFieldsTable: React.FC = ({ fields, onFieldsChan )} {modalField ? ( setModalField(undefined)} diff --git a/frontend/src/pages/connectionTypes/manage/ManageConnectionTypeFieldsTableRow.tsx b/frontend/src/pages/connectionTypes/manage/ManageConnectionTypeFieldsTableRow.tsx index 30277dde71..c23d29d2d6 100644 --- a/frontend/src/pages/connectionTypes/manage/ManageConnectionTypeFieldsTableRow.tsx +++ b/frontend/src/pages/connectionTypes/manage/ManageConnectionTypeFieldsTableRow.tsx @@ -1,18 +1,21 @@ import * as React from 'react'; -import { ActionsColumn, Td, ThProps, Tr } from '@patternfly/react-table'; -import { Button, Label, Switch } from '@patternfly/react-core'; +import { ExclamationCircleIcon } from '@patternfly/react-icons'; +import { ActionsColumn, Td, Tr } from '@patternfly/react-table'; +import { Button, Icon, Label, Switch } from '@patternfly/react-core'; import { ConnectionTypeField, ConnectionTypeFieldType, SectionField, + isConnectionTypeDataField, } from '~/concepts/connectionTypes/types'; import { defaultValueToString, fieldTypeToString } from '~/concepts/connectionTypes/utils'; import type { RowProps } from '~/utilities/useDraggableTableControlled'; import TruncatedText from '~/components/TruncatedText'; +import { columns } from '~/pages/connectionTypes/manage/fieldTableColumns'; type Props = { + fields: ConnectionTypeField[]; row: ConnectionTypeField; - columns: ThProps[]; onEdit: () => void; onDelete: () => void; onDuplicate: (field: ConnectionTypeField) => void; @@ -21,8 +24,8 @@ type Props = { } & RowProps; const ManageConnectionTypeFieldsTableRow: React.FC = ({ + fields, row, - columns, onEdit, onDelete, onDuplicate, @@ -30,6 +33,16 @@ const ManageConnectionTypeFieldsTableRow: React.FC = ({ onChange, ...props }) => { + const isEnvVarConflict = React.useMemo( + () => + row.type === ConnectionTypeFieldType.Section + ? false + : !!fields.find( + (f) => f !== row && isConnectionTypeDataField(f) && f.envVar === row.envVar, + ), + [row, fields], + ); + if (row.type === ConnectionTypeFieldType.Section) { return ( @@ -98,6 +111,14 @@ const ManageConnectionTypeFieldsTableRow: React.FC = ({ {row.envVar || '-'} + {isEnvVarConflict ? ( + <> + + + + This environment variable is in conflict. + + ) : undefined} = ({ prefill, isEdit, onSave }) [connectionNameDesc, connectionEnabled, connectionFields, username, category], ); + const isEnvVarConflict = React.useMemo(() => { + const envVars = connectionFields.filter(isConnectionTypeDataField).map((f) => f.envVar); + return uniq(envVars).length !== envVars.length; + }, [connectionFields]); + const isValid = React.useMemo(() => { const trimmedName = connectionNameDesc.name.trim(); - return Boolean(trimmedName); - }, [connectionNameDesc.name]); + return Boolean(trimmedName) && !isEnvVarConflict; + }, [connectionNameDesc.name, isEnvVarConflict]); const onCancel = () => { navigate('/connectionTypes'); @@ -144,6 +154,12 @@ const ManageConnectionTypePage: React.FC = ({ prefill, isEdit, onSave }) Add fields to prompt users to input information, and optionally assign default values to those fields. + {isEnvVarConflict ? ( + + Environment variables for one or more fields are conflicting. Change them to + resolve and proceed. + + ) : null} { it('should render the modal', () => { @@ -127,4 +128,38 @@ describe('ConnectionTypeDataFieldModal', () => { type: 'short-text', }); }); + + it('should display env var conflict warning', () => { + const onClose = jest.fn(); + const onSubmit = jest.fn(); + const field: ShortTextField = { + type: 'short-text', + name: 'test', + envVar: 'test-envvar', + properties: {}, + }; + const field2: TextField = { + type: 'text', + name: 'test-2', + envVar: 'test-envvar', + properties: {}, + }; + render( + , + ); + const fieldEnvVarInput = screen.getByTestId('field-env-var-input'); + screen.getByTestId('envvar-conflict-warning'); + expect(screen.getByTestId('modal-submit-button')).not.toBeDisabled(); + + act(() => { + fireEvent.change(fieldEnvVarInput, { target: { value: 'new-env-value' } }); + }); + + expect(screen.queryByTestId('envvar-conflict-warning')).not.toBeInTheDocument(); + }); }); diff --git a/frontend/src/pages/connectionTypes/manage/__tests__/ManageConnectionTypeFieldsTableRow.spec.tsx b/frontend/src/pages/connectionTypes/manage/__tests__/ManageConnectionTypeFieldsTableRow.spec.tsx new file mode 100644 index 0000000000..5c4b48610f --- /dev/null +++ b/frontend/src/pages/connectionTypes/manage/__tests__/ManageConnectionTypeFieldsTableRow.spec.tsx @@ -0,0 +1,54 @@ +import * as React from 'react'; +import '@testing-library/jest-dom'; +import { render, screen } from '@testing-library/react'; +import ManageConnectionTypeFieldsTableRow from '~/pages/connectionTypes/manage/ManageConnectionTypeFieldsTableRow'; +import { ShortTextField, TextField } from '~/concepts/connectionTypes/types'; + +const renderRow = ( + props: Pick, 'row'> & + Partial>, +) => { + const fn = jest.fn(); + return ( + + + + +
+ ); +}; + +describe('ManageConnectionTypeFieldsTableRow', () => { + it('should display env variable conflict icon', () => { + const field: ShortTextField = { + type: 'short-text', + name: 'test', + envVar: 'test-envvar', + properties: {}, + }; + const field2: TextField = { + type: 'text', + name: 'test-2', + envVar: 'test-envvar', + properties: {}, + }; + + const result = render(renderRow({ row: field, fields: [field] })); + expect(screen.getByTestId('field-env')).not.toHaveTextContent('conflict'); + + result.rerender(renderRow({ row: field, fields: [field, field2] })); + expect(screen.getByTestId('field-env')).toHaveTextContent('conflict'); + }); +}); diff --git a/frontend/src/pages/connectionTypes/manage/fieldTableColumns.ts b/frontend/src/pages/connectionTypes/manage/fieldTableColumns.ts new file mode 100644 index 0000000000..baa7264dc3 --- /dev/null +++ b/frontend/src/pages/connectionTypes/manage/fieldTableColumns.ts @@ -0,0 +1,9 @@ +import { ThProps } from '@patternfly/react-table'; + +export const columns: ThProps[] = [ + { label: 'Section heading/field name', width: 30 }, + { label: 'Type', width: 10 }, + { label: 'Default value', width: 30 }, + { label: 'Environment variable', width: 20 }, + { label: 'Required', width: 10 }, +];