Przeglądaj źródła

[Fixes #1213] Topic Creation: Inconsistency in Deleted parameters (#1395)

* CustomParams: Fixed condition error. Now existingFields updates correctly when one of CustomParamsFields is deleted

* Resolves bug when you select option and after select another - existingFields do not update correctly

* CustomParams fields update correctly

* Increases coverage

Co-authored-by: Damir Abdulganiev <dabdulganiev@provectus.com>
Damir Abdulganiev 3 lat temu
rodzic
commit
7789523613

+ 1 - 1
kafka-ui-react-app/src/components/Topics/New/New.tsx

@@ -37,7 +37,7 @@ const New: React.FC = () => {
 
       history.push(clusterTopicPath(clusterName, data.name));
     } catch (error) {
-      const response = await getResponse(error);
+      const response = await getResponse(error as Response);
       const alert: FailurePayload = {
         subject: ['schema', data.name].join('-'),
         title: `Schema ${data.name}`,

+ 25 - 16
kafka-ui-react-app/src/components/Topics/shared/Form/CustomParams/CustomParamField.tsx

@@ -1,8 +1,7 @@
-import React from 'react';
+import React, { useRef } from 'react';
 import { ErrorMessage } from '@hookform/error-message';
 import { TOPIC_CUSTOM_PARAMS } from 'lib/constants';
 import { FieldArrayWithId, useFormContext } from 'react-hook-form';
-import { remove as _remove } from 'lodash';
 import { TopicFormData } from 'redux/interfaces';
 import { InputLabel } from 'components/common/Input/InputLabel.styled';
 import { FormError } from 'components/common/Input/Input.styled';
@@ -14,7 +13,7 @@ import * as C from 'components/Topics/shared/Form/TopicForm.styled';
 
 import * as S from './CustomParams.styled';
 
-interface Props {
+export interface Props {
   isDisabled: boolean;
   index: number;
   existingFields: string[];
@@ -37,19 +36,17 @@ const CustomParamField: React.FC<Props> = ({
     watch,
   } = useFormContext<TopicFormData>();
   const nameValue = watch(`customParams.${index}.name`);
-  let prevName = '';
+  const prevName = useRef(nameValue);
 
   React.useEffect(() => {
-    prevName = nameValue;
-  }, []);
-
-  React.useEffect(() => {
-    if (nameValue !== prevName) {
+    if (nameValue !== prevName.current) {
       let newExistingFields = [...existingFields];
-      if (prevName) {
-        newExistingFields = _remove(newExistingFields, (el) => el === prevName);
+      if (prevName.current) {
+        newExistingFields = newExistingFields.filter(
+          (name) => name !== prevName.current
+        );
       }
-      prevName = nameValue;
+      prevName.current = nameValue;
       newExistingFields.push(nameValue);
       setExistingFields(newExistingFields);
       setValue(`customParams.${index}.value`, TOPIC_CUSTOM_PARAMS[nameValue]);
@@ -83,7 +80,10 @@ const CustomParamField: React.FC<Props> = ({
               ))}
           </Select>
           <FormError>
-            <ErrorMessage errors={errors} name={`customParams.${index}.name`} />
+            <ErrorMessage
+              errors={errors}
+              name={`customParams.${index}.name` as const}
+            />
           </FormError>
         </div>
       </>
@@ -101,13 +101,22 @@ const CustomParamField: React.FC<Props> = ({
           disabled={isDisabled}
         />
         <FormError>
-          <ErrorMessage errors={errors} name={`customParams.${index}.value`} />
+          <ErrorMessage
+            errors={errors}
+            name={`customParams.${index}.value` as const}
+          />
         </FormError>
       </div>
 
       <S.DeleteButtonWrapper>
-        <IconButtonWrapper onClick={() => remove(index)} aria-hidden>
-          <CloseIcon />
+        <IconButtonWrapper
+          onClick={() => remove(index)}
+          onKeyDown={(e: React.KeyboardEvent) =>
+            e.code === 'Space' && remove(index)
+          }
+          title={`Delete customParam field ${index}`}
+        >
+          <CloseIcon aria-hidden />
         </IconButtonWrapper>
       </S.DeleteButtonWrapper>
     </C.Column>

+ 3 - 4
kafka-ui-react-app/src/components/Topics/shared/Form/CustomParams/CustomParams.styled.ts

@@ -6,11 +6,10 @@ export const ParamsWrapper = styled.div`
 `;
 
 export const DeleteButtonWrapper = styled.div`
-  height: 32px;
+  min-height: 32px;
   display: flex;
   flex-direction: column;
-  justify-content: center;
   align-items: center;
-  align-self: flex-end;
-  flex-grow: 0.25 !important;
+  justify-self: flex-start;
+  margin-top: 32px;
 `;

+ 18 - 5
kafka-ui-react-app/src/components/Topics/shared/Form/CustomParams/CustomParams.tsx

@@ -1,6 +1,6 @@
 import React from 'react';
 import { TopicConfigByName, TopicFormData } from 'redux/interfaces';
-import { useFieldArray, useFormContext } from 'react-hook-form';
+import { useFieldArray, useFormContext, useWatch } from 'react-hook-form';
 import { Button } from 'components/common/Button/Button';
 
 import CustomParamField from './CustomParamField';
@@ -8,28 +8,41 @@ import * as S from './CustomParams.styled';
 
 export const INDEX_PREFIX = 'customParams';
 
-interface Props {
+export interface CustomParamsProps {
   isSubmitting: boolean;
   config?: TopicConfigByName;
 }
 
-const CustomParams: React.FC<Props> = ({ isSubmitting }) => {
+const CustomParams: React.FC<CustomParamsProps> = ({ isSubmitting }) => {
   const { control } = useFormContext<TopicFormData>();
   const { fields, append, remove } = useFieldArray({
     control,
     name: INDEX_PREFIX,
   });
+  const watchFieldArray = useWatch({
+    control,
+    name: INDEX_PREFIX,
+    defaultValue: fields,
+  });
+  const controlledFields = fields.map((field, index) => {
+    return {
+      ...field,
+      ...watchFieldArray[index],
+    };
+  });
+
   const [existingFields, setExistingFields] = React.useState<string[]>([]);
+
   const removeField = (index: number): void => {
     setExistingFields(
-      existingFields.filter((field) => field === fields[index].name)
+      existingFields.filter((field) => field !== controlledFields[index].name)
     );
     remove(index);
   };
 
   return (
     <S.ParamsWrapper>
-      {fields.map((field, idx) => (
+      {controlledFields.map((field, idx) => (
         <CustomParamField
           key={field.id}
           field={field}

+ 123 - 0
kafka-ui-react-app/src/components/Topics/shared/Form/CustomParams/__tests__/CustomParamField.spec.tsx

@@ -0,0 +1,123 @@
+import React from 'react';
+import { screen, within } from '@testing-library/react';
+import { render } from 'lib/testHelpers';
+import CustomParamsField, {
+  Props,
+} from 'components/Topics/shared/Form/CustomParams/CustomParamField';
+import { FormProvider, useForm } from 'react-hook-form';
+import userEvent from '@testing-library/user-event';
+import { TOPIC_CUSTOM_PARAMS } from 'lib/constants';
+
+const isDisabled = false;
+const index = 0;
+const existingFields: string[] = [];
+const field = { name: 'name', value: 'value', id: 'id' };
+const remove = jest.fn();
+const setExistingFields = jest.fn();
+
+const SPACE_KEY = ' ';
+
+describe('CustomParamsField', () => {
+  const setupComponent = (props: Props) => {
+    const Wrapper: React.FC = ({ children }) => {
+      const methods = useForm();
+      return <FormProvider {...methods}>{children}</FormProvider>;
+    };
+
+    return render(
+      <Wrapper>
+        <CustomParamsField {...props} />
+      </Wrapper>
+    );
+  };
+
+  it('renders with props', () => {
+    setupComponent({
+      field,
+      isDisabled,
+      index,
+      remove,
+      existingFields,
+      setExistingFields,
+    });
+    expect(screen.getByRole('listbox')).toBeInTheDocument();
+    expect(screen.getByRole('textbox')).toBeInTheDocument();
+    expect(screen.getByRole('button')).toBeInTheDocument();
+  });
+
+  describe('core functionality works', () => {
+    it('click on button triggers remove', () => {
+      setupComponent({
+        field,
+        isDisabled,
+        index,
+        remove,
+        existingFields,
+        setExistingFields,
+      });
+      userEvent.click(screen.getByRole('button'));
+      expect(remove.mock.calls.length).toBe(1);
+    });
+
+    it('pressing space on button triggers remove', () => {
+      setupComponent({
+        field,
+        isDisabled,
+        index,
+        remove,
+        existingFields,
+        setExistingFields,
+      });
+      userEvent.type(screen.getByRole('button'), SPACE_KEY);
+      // userEvent.type triggers remove two times as at first it clicks on element and then presses space
+      expect(remove.mock.calls.length).toBe(2);
+    });
+
+    it('can select option', () => {
+      setupComponent({
+        field,
+        isDisabled,
+        index,
+        remove,
+        existingFields,
+        setExistingFields,
+      });
+      const listbox = screen.getByRole('listbox');
+      userEvent.selectOptions(listbox, ['compression.type']);
+
+      const option = within(listbox).getByRole('option', { selected: true });
+      expect(option).toHaveValue('compression.type');
+    });
+
+    it('selecting option updates textbox value', () => {
+      setupComponent({
+        field,
+        isDisabled,
+        index,
+        remove,
+        existingFields,
+        setExistingFields,
+      });
+      const listbox = screen.getByRole('listbox');
+      userEvent.selectOptions(listbox, ['compression.type']);
+
+      const textbox = screen.getByRole('textbox');
+      expect(textbox).toHaveValue(TOPIC_CUSTOM_PARAMS['compression.type']);
+    });
+
+    it('selecting option updates triggers setExistingFields', () => {
+      setupComponent({
+        field,
+        isDisabled,
+        index,
+        remove,
+        existingFields,
+        setExistingFields,
+      });
+      const listbox = screen.getByRole('listbox');
+      userEvent.selectOptions(listbox, ['compression.type']);
+
+      expect(setExistingFields.mock.calls.length).toBe(1);
+    });
+  });
+});

+ 171 - 0
kafka-ui-react-app/src/components/Topics/shared/Form/CustomParams/__tests__/CustomParams.spec.tsx

@@ -0,0 +1,171 @@
+import React from 'react';
+import { screen, within } from '@testing-library/react';
+import { render } from 'lib/testHelpers';
+import CustomParams, {
+  CustomParamsProps,
+} from 'components/Topics/shared/Form/CustomParams/CustomParams';
+import { FormProvider, useForm } from 'react-hook-form';
+import userEvent from '@testing-library/user-event';
+import { TOPIC_CUSTOM_PARAMS } from 'lib/constants';
+
+describe('CustomParams', () => {
+  const setupComponent = (props: CustomParamsProps) => {
+    const Wrapper: React.FC = ({ children }) => {
+      const methods = useForm();
+      return <FormProvider {...methods}>{children}</FormProvider>;
+    };
+
+    return render(
+      <Wrapper>
+        <CustomParams {...props} />
+      </Wrapper>
+    );
+  };
+
+  beforeEach(() => {
+    setupComponent({ isSubmitting: false });
+  });
+
+  it('renders with props', () => {
+    const addParamButton = screen.getByRole('button');
+    expect(addParamButton).toBeInTheDocument();
+    expect(addParamButton).toHaveTextContent('Add Custom Parameter');
+  });
+
+  describe('works with user inputs correctly', () => {
+    it('button click creates custom param fieldset', () => {
+      const addParamButton = screen.getByRole('button');
+      userEvent.click(addParamButton);
+
+      const listbox = screen.getByRole('listbox');
+      expect(listbox).toBeInTheDocument();
+
+      const textbox = screen.getByRole('textbox');
+      expect(textbox).toBeInTheDocument();
+    });
+
+    it('can select option', () => {
+      const addParamButton = screen.getByRole('button');
+      userEvent.click(addParamButton);
+
+      const listbox = screen.getByRole('listbox');
+
+      userEvent.selectOptions(listbox, ['compression.type']);
+
+      const option = screen.getByRole('option', {
+        selected: true,
+      });
+      expect(option).toHaveValue('compression.type');
+      expect(option).toBeDisabled();
+
+      const textbox = screen.getByRole('textbox');
+      expect(textbox).toHaveValue(TOPIC_CUSTOM_PARAMS['compression.type']);
+    });
+
+    it('when selected option changes disabled options update correctly', () => {
+      const addParamButton = screen.getByRole('button');
+      userEvent.click(addParamButton);
+
+      const listbox = screen.getByRole('listbox');
+
+      userEvent.selectOptions(listbox, ['compression.type']);
+
+      const option = screen.getByRole('option', {
+        name: 'compression.type',
+      });
+      expect(option).toBeDisabled();
+
+      userEvent.selectOptions(listbox, ['delete.retention.ms']);
+      const newOption = screen.getByRole('option', {
+        name: 'delete.retention.ms',
+      });
+      expect(newOption).toBeDisabled();
+
+      expect(option).toBeEnabled();
+    });
+
+    it('multiple button clicks create multiple fieldsets', () => {
+      const addParamButton = screen.getByRole('button');
+      userEvent.click(addParamButton);
+      userEvent.click(addParamButton);
+      userEvent.click(addParamButton);
+
+      const listboxes = screen.getAllByRole('listbox');
+      expect(listboxes.length).toBe(3);
+
+      const textboxes = screen.getAllByRole('textbox');
+      expect(textboxes.length).toBe(3);
+    });
+
+    it("can't select already selected option", () => {
+      const addParamButton = screen.getByRole('button');
+      userEvent.click(addParamButton);
+      userEvent.click(addParamButton);
+
+      const listboxes = screen.getAllByRole('listbox');
+
+      const firstListbox = listboxes[0];
+      userEvent.selectOptions(firstListbox, ['compression.type']);
+
+      const firstListboxOption = within(firstListbox).getByRole('option', {
+        selected: true,
+      });
+      expect(firstListboxOption).toBeDisabled();
+
+      const secondListbox = listboxes[1];
+      const secondListboxOption = within(secondListbox).getByRole('option', {
+        name: 'compression.type',
+      });
+      expect(secondListboxOption).toBeDisabled();
+    });
+
+    it('when fieldset with selected custom property type is deleted disabled options update correctly', async () => {
+      const addParamButton = screen.getByRole('button');
+      userEvent.click(addParamButton);
+      userEvent.click(addParamButton);
+      userEvent.click(addParamButton);
+
+      const listboxes = screen.getAllByRole('listbox');
+
+      const firstListbox = listboxes[0];
+      userEvent.selectOptions(firstListbox, ['compression.type']);
+
+      const firstListboxOption = within(firstListbox).getByRole('option', {
+        selected: true,
+      });
+      expect(firstListboxOption).toBeDisabled();
+
+      const secondListbox = listboxes[1];
+      userEvent.selectOptions(secondListbox, ['delete.retention.ms']);
+      const secondListboxOption = within(secondListbox).getByRole('option', {
+        selected: true,
+      });
+      expect(secondListboxOption).toBeDisabled();
+
+      const thirdListbox = listboxes[2];
+      userEvent.selectOptions(thirdListbox, ['file.delete.delay.ms']);
+      const thirdListboxOption = within(thirdListbox).getByRole('option', {
+        selected: true,
+      });
+      expect(thirdListboxOption).toBeDisabled();
+
+      const deleteSecondFieldsetButton = screen.getByTitle(
+        'Delete customParam field 1'
+      );
+      userEvent.click(deleteSecondFieldsetButton);
+      expect(secondListbox).not.toBeInTheDocument();
+
+      expect(
+        within(firstListbox).getByRole('option', {
+          name: 'delete.retention.ms',
+        })
+      ).toBeEnabled();
+
+      expect(
+        within(thirdListbox).getByRole('option', {
+          name: 'delete.retention.ms',
+        })
+      ).toBeEnabled();
+    });
+  });
+});

+ 4 - 1
kafka-ui-react-app/src/components/common/Icons/IconButtonWrapper.ts

@@ -1,6 +1,9 @@
 import styled from 'styled-components';
 
-const IconButtonWrapper = styled.span`
+const IconButtonWrapper = styled.span.attrs(() => ({
+  role: 'button',
+  tabIndex: '0',
+}))`
   height: 16px !important;
   display: inline-block;
   &:hover {