[Fixes #1428] Improve Sonar Cloud security hotspots metrics (#1436)

* Removes regex from compareVersions

* Replaces regexes in reducers

* Rewrite Version and compareVersions to expect undefined

* Adds safeguard against strange cases

* Fixes regression from another PR

Co-authored-by: Damir Abdulganiev <[email protected]>

Author: Motherships

kafka-ui-react-app/src/components/App.tsx

@@ -61,7 +61,7 @@ const App: React.FC = () => {
             <S.Hyperlink href="/ui">UI for Apache Kafka</S.Hyperlink>
 
             <S.NavbarItem>
-              <Version tag={GIT_TAG} commit={GIT_COMMIT} />
+              {GIT_TAG && <Version tag={GIT_TAG} commit={GIT_COMMIT} />}
             </S.NavbarItem>
           </S.NavbarBrand>
         </S.Navbar>

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

@@ -1,5 +1,5 @@
 import React from 'react';
-import {screen, waitFor, within} from '@testing-library/react';
+import { screen, waitFor, within } from '@testing-library/react';
 import { render } from 'lib/testHelpers';
 import CustomParamsField, {
   Props,

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

@@ -9,8 +9,10 @@ import userEvent from '@testing-library/user-event';
 import { TOPIC_CUSTOM_PARAMS } from 'lib/constants';
 
 const selectOption = async (listbox: HTMLElement, option: string) => {
-  await waitFor(() => userEvent.click(listbox));
-  await waitFor(() => userEvent.click(screen.getByText(option)));
+  await waitFor(() => {
+    userEvent.click(listbox);
+    userEvent.click(screen.getByText(option));
+  });
 };
 
 const expectOptionIsSelected = (listbox: HTMLElement, option: string) => {
@@ -19,17 +21,27 @@ const expectOptionIsSelected = (listbox: HTMLElement, option: string) => {
   expect(selectedOption[0]).toHaveTextContent(option);
 };
 
-const expectOptionIsDisabled = async (
+const expectOptionAvailability = async (
   listbox: HTMLElement,
   option: string,
   disabled: boolean
 ) => {
   await waitFor(() => userEvent.click(listbox));
-  const selectedOption = within(listbox).getAllByText(option);
-  expect(selectedOption[1]).toHaveStyleRule(
+  const selectedOptions = within(listbox).getAllByText(option).reverse();
+  // its either two or one nodes, we only need last one
+  const selectedOption = selectedOptions[0];
+
+  if (disabled) {
+    expect(selectedOption).toHaveAttribute('disabled');
+  } else {
+    expect(selectedOption).toBeEnabled();
+  }
+
+  expect(selectedOption).toHaveStyleRule(
     'cursor',
     disabled ? 'not-allowed' : 'pointer'
   );
+  await waitFor(() => userEvent.click(listbox));
 };
 
 describe('CustomParams', () => {
@@ -51,15 +63,15 @@ describe('CustomParams', () => {
   });
 
   it('renders with props', () => {
-    const addParamButton = screen.getByRole('button');
-    expect(addParamButton).toBeInTheDocument();
-    expect(addParamButton).toHaveTextContent('Add Custom Parameter');
+    const button = screen.getByRole('button');
+    expect(button).toBeInTheDocument();
+    expect(button).toHaveTextContent('Add Custom Parameter');
   });
 
   describe('works with user inputs correctly', () => {
     it('button click creates custom param fieldset', async () => {
-      const addParamButton = screen.getByRole('button');
-      await waitFor(() => userEvent.click(addParamButton));
+      const button = screen.getByRole('button');
+      await waitFor(() => userEvent.click(button));
 
       const listbox = screen.getByRole('listbox');
       expect(listbox).toBeInTheDocument();
@@ -69,38 +81,38 @@ describe('CustomParams', () => {
     });
 
     it('can select option', async () => {
-      const addParamButton = screen.getByRole('button');
-      await waitFor(() => userEvent.click(addParamButton));
-
+      const button = screen.getByRole('button');
+      await waitFor(() => userEvent.click(button));
       const listbox = screen.getByRole('listbox');
 
       await selectOption(listbox, 'compression.type');
       expectOptionIsSelected(listbox, 'compression.type');
-      expectOptionIsDisabled(listbox, 'compression.type', true);
+      await expectOptionAvailability(listbox, 'compression.type', true);
 
       const textbox = screen.getByRole('textbox');
       expect(textbox).toHaveValue(TOPIC_CUSTOM_PARAMS['compression.type']);
     });
 
     it('when selected option changes disabled options update correctly', async () => {
-      const addParamButton = screen.getByRole('button');
-      await waitFor(() => userEvent.click(addParamButton));
+      const button = screen.getByRole('button');
+      await waitFor(() => userEvent.click(button));
 
       const listbox = screen.getByRole('listbox');
 
       await selectOption(listbox, 'compression.type');
-      expectOptionIsDisabled(listbox, 'compression.type', true);
+      expectOptionIsSelected(listbox, 'compression.type');
+      await expectOptionAvailability(listbox, 'compression.type', true);
 
       await selectOption(listbox, 'delete.retention.ms');
-      expectOptionIsDisabled(listbox, 'delete.retention.ms', true);
-      expectOptionIsDisabled(listbox, 'compression.type', false);
+      await expectOptionAvailability(listbox, 'delete.retention.ms', true);
+      await expectOptionAvailability(listbox, 'compression.type', false);
     });
 
     it('multiple button clicks create multiple fieldsets', async () => {
-      const addParamButton = screen.getByRole('button');
-      await waitFor(() => userEvent.click(addParamButton));
-      await waitFor(() => userEvent.click(addParamButton));
-      await waitFor(() => userEvent.click(addParamButton));
+      const button = screen.getByRole('button');
+      await waitFor(() => userEvent.click(button));
+      await waitFor(() => userEvent.click(button));
+      await waitFor(() => userEvent.click(button));
 
       const listboxes = screen.getAllByRole('listbox');
       expect(listboxes.length).toBe(3);
@@ -110,48 +122,64 @@ describe('CustomParams', () => {
     });
 
     it("can't select already selected option", async () => {
-      const addParamButton = screen.getByRole('button');
-      userEvent.click(addParamButton);
-      userEvent.click(addParamButton);
+      const button = screen.getByRole('button');
+      await waitFor(() => userEvent.click(button));
+      await waitFor(() => userEvent.click(button));
 
       const listboxes = screen.getAllByRole('listbox');
 
       const firstListbox = listboxes[0];
       await selectOption(firstListbox, 'compression.type');
-      expectOptionIsDisabled(firstListbox, 'compression.type', true);
+      await expectOptionAvailability(firstListbox, 'compression.type', true);
 
       const secondListbox = listboxes[1];
-      expectOptionIsDisabled(secondListbox, 'compression.type', true);
+      await expectOptionAvailability(secondListbox, 'compression.type', true);
     });
 
     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 button = screen.getByRole('button');
+      await waitFor(() => userEvent.click(button));
+      await waitFor(() => userEvent.click(button));
+      await waitFor(() => userEvent.click(button));
 
       const listboxes = screen.getAllByRole('listbox');
 
       const firstListbox = listboxes[0];
       await selectOption(firstListbox, 'compression.type');
-      expectOptionIsDisabled(firstListbox, 'compression.type', true);
+      await expectOptionAvailability(firstListbox, 'compression.type', true);
 
       const secondListbox = listboxes[1];
       await selectOption(secondListbox, 'delete.retention.ms');
-      expectOptionIsDisabled(secondListbox, 'delete.retention.ms', true);
+      await expectOptionAvailability(
+        secondListbox,
+        'delete.retention.ms',
+        true
+      );
 
       const thirdListbox = listboxes[2];
       await selectOption(thirdListbox, 'file.delete.delay.ms');
-      expectOptionIsDisabled(thirdListbox, 'file.delete.delay.ms', true);
+      await expectOptionAvailability(
+        thirdListbox,
+        'file.delete.delay.ms',
+        true
+      );
 
       const deleteSecondFieldsetButton = screen.getByTitle(
         'Delete customParam field 1'
       );
-      userEvent.click(deleteSecondFieldsetButton);
+      await waitFor(() => userEvent.click(deleteSecondFieldsetButton));
       expect(secondListbox).not.toBeInTheDocument();
 
-      expectOptionIsDisabled(firstListbox, 'delete.retention.ms', false);
-      expectOptionIsDisabled(thirdListbox, 'delete.retention.ms', false);
+      await expectOptionAvailability(
+        firstListbox,
+        'delete.retention.ms',
+        false
+      );
+      await expectOptionAvailability(
+        thirdListbox,
+        'delete.retention.ms',
+        false
+      );
     });
   });
 });

kafka-ui-react-app/src/components/Version/Version.tsx

@@ -5,7 +5,7 @@ import { GIT_REPO_LATEST_RELEASE_LINK } from 'lib/constants';
 import compareVersions from './compareVersions';
 
 export interface VesionProps {
-  tag?: string;
+  tag: string;
   commit?: string;
 }
 
@@ -15,20 +15,15 @@ const Version: React.FC<VesionProps> = ({ tag, commit }) => {
     latestTag: '',
   });
   useEffect(() => {
-    if (tag) {
-      fetch(GIT_REPO_LATEST_RELEASE_LINK)
-        .then((response) => response.json())
-        .then((data) => {
-          setLatestVersionInfo({
-            outdated: compareVersions(tag, data.tag_name) === -1,
-            latestTag: data.tag_name,
-          });
+    fetch(GIT_REPO_LATEST_RELEASE_LINK)
+      .then((response) => response.json())
+      .then((data) => {
+        setLatestVersionInfo({
+          outdated: compareVersions(tag, data.tag_name) === -1,
+          latestTag: data.tag_name,
         });
-    }
+      });
   }, [tag]);
-  if (!tag) {
-    return null;
-  }
 
   const { outdated, latestTag } = latestVersionInfo;
 

kafka-ui-react-app/src/components/Version/__tests__/Version.spec.tsx

@@ -1,29 +1,22 @@
 import React from 'react';
-import { mount } from 'enzyme';
-import Version from 'components/Version/Version';
+import Version, { VesionProps } from 'components/Version/Version';
+import { screen } from '@testing-library/react';
+import { render } from 'lib/testHelpers';
 
 const tag = 'v1.0.1-SHAPSHOT';
 const commit = '123sdf34';
 
 describe('Version', () => {
-  it('shows nothing if tag is not defined', () => {
-    const component = mount(<Version />);
-    expect(component.html()).toEqual(null);
-  });
+  const setupComponent = (props: VesionProps) => render(<Version {...props} />);
 
-  it('shows current tag when only tag is defined', () => {
-    const component = mount(<Version tag={tag} />);
-    expect(component.text()).toContain(tag);
+  it('renders', () => {
+    setupComponent({ tag });
+    expect(screen.getByText('Version:')).toBeInTheDocument();
   });
 
   it('shows current tag and commit', () => {
-    const component = mount(<Version tag={tag} commit={commit} />);
-    expect(component.text()).toContain(tag);
-    expect(component.text()).toContain(commit);
-  });
-
-  it('matches snapshot', () => {
-    const component = mount(<Version tag={tag} commit={commit} />);
-    expect(component).toMatchSnapshot();
+    setupComponent({ tag, commit });
+    expect(screen.getByText(tag)).toBeInTheDocument();
+    expect(screen.getByText(commit)).toBeInTheDocument();
   });
 });

kafka-ui-react-app/src/components/Version/__tests__/__snapshots__/Version.spec.tsx.snap

@@ -1,6 +1,3 @@
-// eslint-disable-next-line @typescript-eslint/ban-ts-comment
-// @ts-nocheck
-
 import compareVersions from 'components/Version/compareVersions';
 
 const runTests = (dataSet: [string, string, number][]) => {
@@ -63,10 +60,11 @@ describe('compareVersions function', () => {
   });
 
   it('returns valid result (negative test cases)', () => {
+    expect(compareVersions()).toEqual(0);
+    expect(compareVersions('v0.0.0')).toEqual(0);
+    // @ts-expect-error first arg is number
     expect(compareVersions(123, 'v0.0.0')).toEqual(0);
-    expect(compareVersions(undefined, 'v0.0.0')).toEqual(0);
+    // @ts-expect-error second arg is number
     expect(compareVersions('v0.0.0', 123)).toEqual(0);
-    expect(compareVersions('v0.0.0', undefined)).toEqual(0);
-    expect(compareVersions(undefined, undefined)).toEqual(0);
   });
 });

kafka-ui-react-app/src/components/Version/__tests__/compareVersions.spec.ts

@@ -1,9 +1,12 @@
 const split = (v: string): string[] => {
-  const c = v.replace(/^v/, '').replace(/\+.*$/, '');
-  return c.split('-')[0].split('.');
+  const c = v.replace('v', '').split('-')[0];
+  return c.split('.');
 };
 
-const compareVersions = (v1: string, v2: string): number => {
+const compareVersions = (v1?: string, v2?: string): number => {
+  if (!v1 || !v2) return 0;
+
+  // try..catch - is our safeguard for strange git tags (or usecases without network)
   try {
     const s1 = split(v1);
     const s2 = split(v2);

kafka-ui-react-app/src/components/Version/compareVersions.ts

@@ -9,8 +9,7 @@ export const initialState: AlertsState = {};
 const reducer = (state = initialState, action: Action): AlertsState => {
   const { type } = action;
 
-  const matches = /(.*)__(FAILURE)$/.exec(type);
-  if (matches && matches[2]) return addError(state, action);
+  if (type.endsWith('__FAILURE')) return addError(state, action);
 
   if (type === getType(dismissAlert)) {
     return removeAlert(state, action);

kafka-ui-react-app/src/redux/reducers/alerts/reducer.ts

@@ -4,12 +4,17 @@ export const initialState: LoaderState = {};
 
 const reducer = (state = initialState, action: Action): LoaderState => {
   const { type } = action;
-  const matches = /(.*)__(REQUEST|SUCCESS|FAILURE|CANCEL)$/.exec(type);
+  const splitType = type.split('__');
+  const requestState = splitType.pop();
+  const requestName = splitType.join('__');
 
   // not a *__REQUEST / *__SUCCESS /  *__FAILURE /  *__CANCEL actions, so we ignore them
-  if (!matches) return state;
-
-  const [, requestName, requestState] = matches;
+  if (
+    requestState &&
+    !['REQUEST', 'SUCCESS', 'FAILURE', 'CANCEL'].includes(requestState)
+  ) {
+    return state;
+  }
 
   switch (requestState) {
     case 'REQUEST':