surface conflicting env var names as warnings and errors (opendatahub…

…-io#3124)
DaoDaoNoCode · Aug 28, 2024 · bccd89c · bccd89c
1 parent 4a0626e
commit bccd89c
Show file tree

Hide file tree

Showing 9 changed files with 182 additions and 36 deletions.
diff --git a/frontend/src/concepts/connectionTypes/types.ts b/frontend/src/concepts/connectionTypes/types.ts
@@ -101,6 +101,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;

diff --git a/frontend/src/pages/connectionTypes/manage/ConnectionTypeDataFieldModal.tsx b/frontend/src/pages/connectionTypes/manage/ConnectionTypeDataFieldModal.tsx
@@ -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 { fieldNameToEnvVar, fieldTypeToString } from '~/concepts/connectionTypes/utils';
 import { isEnumMember } from '~/utilities/utils';
@@ -41,6 +46,7 @@ type Props = {
   onClose: () => void;
   onSubmit: (field: ConnectionTypeDataField) => void;
   isEdit?: boolean;
+  fields?: ConnectionTypeField[];
 };
 
 export const ConnectionTypeDataFieldModal: React.FC<Props> = ({
@@ -49,6 +55,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);
@@ -64,16 +71,16 @@ export const ConnectionTypeDataFieldModal: React.FC<Props> = ({
   const [isTypeSelectOpen, setIsTypeSelectOpen] = React.useState<boolean>(false);
   const [properties, setProperties] = React.useState<unknown>(field?.properties || {});
   const [isPropertiesValid, setPropertiesValid] = React.useState(true);
-
   const [autoGenerateEnvVar, setAutoGenerateEnvVar] = React.useState<boolean>(!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
@@ -178,17 +185,24 @@ export const ConnectionTypeDataFieldModal: React.FC<Props> = ({
               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"
@@ -203,6 +217,7 @@ export const ConnectionTypeDataFieldModal: React.FC<Props> = ({
             selected={fieldType}
             onSelect={(_e, selection) => {
               if (isConnectionTypeFieldType(selection)) {
+                setPropertiesValid(true);
                 setProperties({});
                 setFieldType(selection);
                 setIsTypeSelectOpen(false);

diff --git a/frontend/src/pages/connectionTypes/manage/ConnectionTypeFieldModal.tsx b/frontend/src/pages/connectionTypes/manage/ConnectionTypeFieldModal.tsx
@@ -9,6 +9,7 @@ type Props = {
   onClose: () => void;
   onSubmit: (field: ConnectionTypeField) => void;
   isEdit?: boolean;
+  fields?: ConnectionTypeField[];
 };
 
 const ConnectionTypeFieldModal: React.FC<Props> = (props) => {

diff --git 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';
 import { ConnectionTypeMoveFieldToSectionModal } from './ConnectionTypeFieldMoveModal';
@@ -52,14 +53,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<Props> = ({ fields, onFieldsChange }) => {
   const [modalField, setModalField] = React.useState<
     { field?: ConnectionTypeField; index?: number; isEdit?: boolean } | undefined
@@ -96,7 +89,6 @@ const ManageConnectionTypeFieldsTable: React.FC<Props> = ({ fields, onFieldsChan
                   key={index}
                   row={row}
                   rowIndex={index}
-                  columns={columns}
                   fields={fields}
                   onEdit={() => {
                     setModalField({
@@ -164,6 +156,7 @@ const ManageConnectionTypeFieldsTable: React.FC<Props> = ({ fields, onFieldsChan
       )}
       {modalField ? (
         <ConnectionTypeFieldModal
+          fields={fields}
           field={modalField.field}
           isEdit={modalField.isEdit}
           onClose={() => setModalField(undefined)}

diff --git a/frontend/src/pages/connectionTypes/manage/ManageConnectionTypeFieldsTableRow.tsx b/frontend/src/pages/connectionTypes/manage/ManageConnectionTypeFieldsTableRow.tsx
@@ -1,19 +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 = {
   row: ConnectionTypeField;
   rowIndex: number;
-  columns: ThProps[];
   fields: ConnectionTypeField[];
   onEdit: () => void;
   onRemove: () => void;
@@ -26,7 +28,6 @@ type Props = {
 const ManageConnectionTypeFieldsTableRow: React.FC<Props> = ({
   row,
   rowIndex,
-  columns,
   fields,
   onEdit,
   onRemove,
@@ -45,6 +46,16 @@ const ManageConnectionTypeFieldsTableRow: React.FC<Props> = ({
     return potentialSectionsToMoveTo > 0;
   }, [fields, rowIndex]);
 
+  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 (
       <Tr draggable isStriped data-testid="row" {...props}>
@@ -113,6 +124,14 @@ const ManageConnectionTypeFieldsTableRow: React.FC<Props> = ({
       </Td>
       <Td dataLabel={columns[3].label} 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].label}>
         <Switch

diff --git a/frontend/src/pages/connectionTypes/manage/ManageConnectionTypePage.tsx b/frontend/src/pages/connectionTypes/manage/ManageConnectionTypePage.tsx
@@ -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,
@@ -73,10 +78,15 @@ const ManageConnectionTypePage: React.FC<Props> = ({ 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<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={setConnectionFields}

diff --git a/frontend/src/pages/connectionTypes/manage/__tests__/ConnectionTypeDataFieldModal.spec.tsx b/frontend/src/pages/connectionTypes/manage/__tests__/ConnectionTypeDataFieldModal.spec.tsx
@@ -3,11 +3,12 @@ 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';
-
-let onClose: jest.Mock;
-let onSubmit: jest.Mock;
+import { ShortTextField, TextField } from '~/concepts/connectionTypes/types';
 
 describe('ConnectionTypeDataFieldModal', () => {
+  let onClose: jest.Mock;
+  let onSubmit: jest.Mock;
+
   beforeEach(() => {
     onClose = jest.fn();
     onSubmit = jest.fn();
@@ -230,4 +231,36 @@ describe('ConnectionTypeDataFieldModal', () => {
 
     expect(screen.getByTestId('modal-submit-button')).toBeDisabled();
   });
+
+  it('should display env var conflict warning', () => {
+    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();
+  });
 });
diff --git a/...nd/src/pages/connectionTypes/manage/__tests__/ManageConnectionTypeFieldsTableRow.spec.tsx b/...nd/src/pages/connectionTypes/manage/__tests__/ManageConnectionTypeFieldsTableRow.spec.tsx
@@ -0,0 +1,56 @@
+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}
+          onRemove={fn}
+          onDragEnd={fn}
+          onDragStart={fn}
+          onDrop={fn}
+          onDuplicate={fn}
+          onEdit={fn}
+          onMoveToSection={fn}
+          id="test"
+          rowIndex={0}
+          {...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');
+  });
+});
diff --git a/frontend/src/pages/connectionTypes/manage/fieldTableColumns.ts 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 },
+];