Skip to content

Commit

Permalink
surface conflicting env var names as warnings and errors
Browse files Browse the repository at this point in the history
  • Loading branch information
christianvogt committed Aug 26, 2024
1 parent 5e854fe commit a668a36
Show file tree
Hide file tree
Showing 9 changed files with 169 additions and 30 deletions.
4 changes: 4 additions & 0 deletions frontend/src/concepts/connectionTypes/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ export type ConnectionTypeField =

export type ConnectionTypeDataField = Exclude<ConnectionTypeField, SectionField>;

export const isConnectionTypeDataField = (
field: ConnectionTypeField,
): field is ConnectionTypeDataField => field.type !== ConnectionTypeFieldType.Section;

export type ConnectionTypeConfigMap = K8sResourceCommon & {
metadata: {
name: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,19 @@ 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 { isEnumMember } from '~/utilities/utils';
import DashboardPopupIconButton from '~/concepts/dashboard/DashboardPopupIconButton';
Expand All @@ -40,6 +45,7 @@ type Props = {
onClose: () => void;
onSubmit: (field: ConnectionTypeDataField) => void;
isEdit?: boolean;
fields?: ConnectionTypeField[];
};

const fieldTypeLabels: { [key: string]: string } = {
Expand All @@ -59,6 +65,7 @@ export const ConnectionTypeDataFieldModal: React.FC<Props> = ({
onClose,
onSubmit,
isEdit,
fields,
}) => {
const [name, setName] = React.useState<string>(field?.name || '');
const [description, setDescription] = React.useState<string | undefined>(field?.description);
Expand All @@ -75,14 +82,13 @@ export const ConnectionTypeDataFieldModal: React.FC<Props> = ({
const [properties, setProperties] = React.useState<unknown>(field?.properties || {});
const [isPropertiesValid, setPropertiesValid] = React.useState(true);

const envVarValidation =
!envVar || ENV_VAR_NAME_REGEX.test(envVar) ? ValidatedOptions.default : ValidatedOptions.error;
const isValid =
!!fieldType &&
isPropertiesValid &&
!!name &&
!!envVar &&
envVarValidation === ValidatedOptions.default;
const isEnvVarConflict = !!fields?.find(
(f) => f !== field && isConnectionTypeDataField(f) && f.envVar === 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
Expand Down Expand Up @@ -177,17 +183,24 @@ export const ConnectionTypeDataFieldModal: React.FC<Props> = ({
value={envVar}
onChange={(_ev, value) => setEnvVar(value)}
data-testid="field-env-var-input"
validated={envVarValidation}
validated={!isEnvVarValid ? 'error' : isEnvVarConflict ? 'warning' : 'default'}
/>
{envVarValidation === ValidatedOptions.error ? (
<FormHelperText>
<FormHelperText>
{!isEnvVarValid ? (
<HelperText>
<HelperTextItem icon={<ExclamationCircleIcon />} variant="error">
{`Invalid variable name. The name must consist of alphabetic characters, digits, '_', '-', or '.', and must not start with a digit.`}
</HelperTextItem>
</HelperText>
</FormHelperText>
) : null}
) : undefined}
{isEnvVarConflict ? (
<HelperText data-testid="envvar-conflict-warning">
<HelperTextItem icon={<WarningTriangleIcon />} variant="warning">
This environment variable name is already being used for an existing field.
</HelperTextItem>
</HelperText>
) : undefined}
</FormHelperText>
</FormGroup>
<FormGroup
fieldId="fieldType"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ type Props = {
onClose: () => void;
onSubmit: (field: ConnectionTypeField) => void;
isEdit?: boolean;
fields?: ConnectionTypeField[];
};

const ConnectionTypeFieldModal: React.FC<Props> = (props) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { PlusCircleIcon } from '@patternfly/react-icons';
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';

Expand Down Expand Up @@ -56,14 +57,6 @@ const ManageConnectionTypeFieldsTable: React.FC<Props> = ({ fields, onFieldsChan
{ field?: ConnectionTypeField; index?: number; isEdit?: boolean } | undefined
>();

const columns = [
'Section heading/field name',
'Type',
'Default value',
'Environment variable',
'Required',
];

const { tableProps, rowsToRender } = useDraggableTableControlled<ConnectionTypeField>(
fields,
onFieldsChange,
Expand All @@ -85,9 +78,9 @@ const ManageConnectionTypeFieldsTable: React.FC<Props> = ({ fields, onFieldsChan
<Tbody {...tableProps.tbodyProps}>
{rowsToRender.map(({ data: row, rowProps }, index) => (
<ManageConnectionTypeFieldsTableRow
fields={fields}
key={index}
row={row}
columns={columns}
onEdit={() => {
setModalField({
field: row,
Expand Down Expand Up @@ -151,6 +144,7 @@ const ManageConnectionTypeFieldsTable: React.FC<Props> = ({ fields, onFieldsChan
)}
{modalField ? (
<ConnectionTypeFieldModal
fields={fields}
field={modalField.field}
isEdit={modalField.isEdit}
onClose={() => setModalField(undefined)}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
import * as React from 'react';
import { ExclamationCircleIcon } from '@patternfly/react-icons';
import { ActionsColumn, Td, Tr } from '@patternfly/react-table';
import { Button, Label, Switch, Truncate } from '@patternfly/react-core';
import { Button, Icon, Label, Switch, Truncate } 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 { columns } from '~/pages/connectionTypes/manage/fieldTableColumns';

type Props = {
fields: ConnectionTypeField[];
row: ConnectionTypeField;
columns: string[];
onEdit: () => void;
onDelete: () => void;
onDuplicate: (field: ConnectionTypeField) => void;
Expand All @@ -20,8 +23,8 @@ type Props = {
} & RowProps;

const ManageConnectionTypeFieldsTableRow: React.FC<Props> = ({
fields,
row,
columns,
onEdit,
onDelete,
onDuplicate,
Expand Down Expand Up @@ -71,6 +74,10 @@ const ManageConnectionTypeFieldsTableRow: React.FC<Props> = ({
);
}

const isEnvVarConflict = !!fields.find(
(f) => f !== row && isConnectionTypeDataField(f) && f.envVar === row.envVar,
);

return (
<Tr draggable data-testid="row" {...props}>
<Td
Expand All @@ -92,6 +99,14 @@ const ManageConnectionTypeFieldsTableRow: React.FC<Props> = ({
</Td>
<Td dataLabel={columns[3]} data-testid="field-env">
{row.envVar || '-'}
{isEnvVarConflict ? (
<>
<Icon status="danger" size="sm" className="pf-v5-u-ml-xs">
<ExclamationCircleIcon />
</Icon>
<span className="pf-v5-u-screen-reader">This environment variable is in conflict.</span>
</>
) : undefined}
</Td>
<Td dataLabel={columns[4]}>
<Switch
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,14 @@ import {
} from '@patternfly/react-core';
import { useNavigate } from 'react-router';
import { OpenDrawerRightIcon } from '@patternfly/react-icons';
import { uniq } from 'lodash-es';
import { useUser } from '~/redux/selectors';
import NameDescriptionField from '~/concepts/k8s/NameDescriptionField';
import { ConnectionTypeConfigMapObj, ConnectionTypeField } from '~/concepts/connectionTypes/types';
import {
ConnectionTypeConfigMapObj,
ConnectionTypeField,
isConnectionTypeDataField,
} from '~/concepts/connectionTypes/types';
import ConnectionTypePreviewDrawer from '~/concepts/connectionTypes/ConnectionTypePreviewDrawer';
import {
createConnectionTypeObj,
Expand Down Expand Up @@ -70,10 +75,15 @@ const ManageConnectionTypePage: React.FC<Props> = ({ prefill, isEdit, onSave })
[connectionNameDesc, connectionEnabled, connectionFields, username],
);

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');
Expand Down Expand Up @@ -141,6 +151,12 @@ const ManageConnectionTypePage: React.FC<Props> = ({ prefill, isEdit, onSave })
Add fields to prompt users to input information, and optionally assign default values
to those fields.
<FormGroup>
{isEnvVarConflict ? (
<Alert isInline variant="danger" title="Environment variables conflict">
Environment variables for one or more fields are conflicting. Change them to
resolve and proceed.
</Alert>
) : null}
<ManageConnectionTypeFieldsTable
fields={connectionFields}
onFieldsChange={(fields) => setConnectionFields(fields)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import '@testing-library/jest-dom';
import { fireEvent, render, screen, waitFor, within } from '@testing-library/react';
import { act } from 'react-dom/test-utils';
import { ConnectionTypeDataFieldModal } from '~/pages/connectionTypes/manage/ConnectionTypeDataFieldModal';
import { ShortTextField, TextField } from '~/concepts/connectionTypes/types';

describe('ConnectionTypeDataFieldModal', () => {
it('should render the modal', () => {
Expand Down Expand Up @@ -64,4 +65,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(
<ConnectionTypeDataFieldModal
onClose={onClose}
onSubmit={onSubmit}
field={field}
fields={[field, field2]}
/>,
);
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();
});
});
Original file line number Diff line number Diff line change
@@ -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<React.ComponentProps<typeof ManageConnectionTypeFieldsTableRow>, 'row'> &
Partial<React.ComponentProps<typeof ManageConnectionTypeFieldsTableRow>>,
) => {
const fn = jest.fn();
return (
<table>
<tbody>
<ManageConnectionTypeFieldsTableRow
fields={[]}
onAddField={fn}
onChange={fn}
onDelete={fn}
onDragEnd={fn}
onDragStart={fn}
onDrop={fn}
onDuplicate={fn}
onEdit={fn}
id="test"
{...props}
/>
</tbody>
</table>
);
};

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');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export const columns = [
'Section heading/field name',
'Type',
'Default value',
'Environment variable',
'Required',
];

0 comments on commit a668a36

Please sign in to comment.