refactor: configure service form (#11734)

* refactor: configure service form * improve unit test * address comments
2025-08-30 03:46:10 +00:00 · 2023-05-25 13:33:29 +05:30 · 2023-05-25 13:33:29 +05:30 · c68d85ebd5
commit c68d85ebd5
parent ef2869cd9a
8 changed files with 132 additions and 232 deletions
--- a/openmetadata-ui/src/main/resources/ui/cypress/common/common.js
+++ b/openmetadata-ui/src/main/resources/ui/cypress/common/common.js
@ -17,6 +17,7 @@ import {
  CUSTOM_PROPERTY_INVALID_NAMES,
  CUSTOM_PROPERTY_NAME_VALIDATION_ERROR,
  DELETE_TERM,
+  NAME_VALIDATION_ERROR,
  SEARCH_INDEX,
 } from '../constants/constants';

@ -213,7 +214,20 @@ export const testServiceCreationAndIngestion = ({
  cy.get('[data-testid="next-button"]').should('exist').click();

  // Enter service name in step 2
-  cy.get('[data-testid="service-name"]').should('exist').type(serviceName);
+
+  // validation should work
+  cy.get('[data-testid="next-button"]').should('exist').click();
+
+  cy.get('#name_help').should('be.visible').contains('name is required');
+
+  // invalid name validation should work
+  cy.get('[data-testid="service-name"]').should('exist').type('!@#$%^&*()');
+  cy.get('#name_help').should('be.visible').contains(NAME_VALIDATION_ERROR);
+
+  cy.get('[data-testid="service-name"]')
+    .should('exist')
+    .clear()
+    .type(serviceName);
  interceptURL('GET', '/api/v1/services/ingestionPipelines/ip', 'ipApi');
  interceptURL(
    'GET',
--- a/openmetadata-ui/src/main/resources/ui/src/components/AddService/AddService.component.tsx
+++ b/openmetadata-ui/src/main/resources/ui/src/components/AddService/AddService.component.tsx
@ -28,16 +28,11 @@ import { useHistory } from 'react-router-dom';
 import { showErrorToast } from 'utils/ToastUtils';
 import { getServiceDetailsPath } from '../../constants/constants';
 import { GlobalSettingsMenuCategory } from '../../constants/GlobalSettings.constants';
-import { delimiterRegex, nameWithSpace } from '../../constants/regex.constants';
 import { FormSubmitType } from '../../enums/form.enum';
 import { ServiceCategory } from '../../enums/service.enum';
 import { PipelineType } from '../../generated/entity/services/ingestionPipelines/ingestionPipeline';
 import { ConfigData } from '../../interface/service.interface';
-import {
-  getCurrentUserId,
-  getServiceLogo,
-  isUrlFriendlyName,
-} from '../../utils/CommonUtils';
+import { getCurrentUserId, getServiceLogo } from '../../utils/CommonUtils';
 import { getAddServicePath, getSettingPath } from '../../utils/RouterUtils';
 import {
  getServiceCreatedLabel,
@ -50,7 +45,7 @@ import SuccessScreen from '../common/success-screen/SuccessScreen';
 import TitleBreadcrumb from '../common/title-breadcrumb/title-breadcrumb.component';
 import IngestionStepper from '../IngestionStepper/IngestionStepper.component';
 import ConnectionConfigForm from '../ServiceConfig/ConnectionConfigForm';
-import { AddServiceProps } from './AddService.interface';
+import { AddServiceProps, ServiceConfig } from './AddService.interface';
 import ConfigureService from './Steps/ConfigureService';
 import SelectServiceType from './Steps/SelectServiceType';

@ -78,16 +73,21 @@ const AddService = ({
  const [activeServiceStep, setActiveServiceStep] = useState(1);
  const [activeIngestionStep, setActiveIngestionStep] = useState(1);
  const [selectServiceType, setSelectServiceType] = useState('');
-  const [serviceName, setServiceName] = useState('');
-  const [description, setDescription] = useState('');
+  const [serviceConfig, setServiceConfig] = useState<ServiceConfig>({
+    serviceName: '',
+    description: '',
+  });
+
  const [saveServiceState, setSaveServiceState] =
    useState<LoadingState>('initial');
  const [activeField, setActiveField] = useState<string>('');

  const handleServiceTypeClick = (type: string) => {
    setShowErrorMessage({ ...showErrorMessage, serviceType: false });
-    setServiceName('');
-    setDescription('');
+    setServiceConfig({
+      serviceName: '',
+      description: '',
+    });
    setSelectServiceType(type);
  };

@ -117,47 +117,18 @@ const AddService = ({

  // Configure service name
  const handleConfigureServiceBackClick = () => setActiveServiceStep(1);
-  const handleConfigureServiceNextClick = (descriptionValue: string) => {
-    setDescription(descriptionValue.trim());
-
-    if (!serviceName.trim()) {
-      setShowErrorMessage({ ...showErrorMessage, name: true, isError: true });
-    } else if (nameWithSpace.test(serviceName)) {
-      setShowErrorMessage({
-        ...showErrorMessage,
-        nameWithSpace: true,
-        isError: true,
-      });
-    } else if (delimiterRegex.test(serviceName)) {
-      setShowErrorMessage({
-        ...showErrorMessage,
-        delimit: true,
-        isError: true,
-      });
-    } else if (!isUrlFriendlyName(serviceName.trim())) {
-      setShowErrorMessage({
-        ...showErrorMessage,
-        specialChar: true,
-        isError: true,
-      });
-    } else if (serviceName.length < 1 || serviceName.length > 128) {
-      setShowErrorMessage({
-        ...showErrorMessage,
-        nameLength: true,
-        isError: true,
-      });
-    } else if (!showErrorMessage.isError) {
-      setActiveServiceStep(3);
-    }
+  const handleConfigureServiceNextClick = (value: ServiceConfig) => {
+    setServiceConfig(value);
+    setActiveServiceStep(3);
  };

  // Service connection
  const handleConnectionDetailsBackClick = () => setActiveServiceStep(2);
  const handleConfigUpdate = async (newConfigData: ConfigData) => {
    const data = {
-      name: serviceName,
+      name: serviceConfig.serviceName,
      serviceType: selectServiceType,
-      description: description,
+      description: serviceConfig.description,
      owner: {
        id: getCurrentUserId(),
        type: 'user',
@ -183,7 +154,7 @@ const AddService = ({
        showErrorToast(
          t('server.entity-already-exist', {
            entity: t('label.service'),
-            name: serviceName,
+            name: serviceConfig.serviceName,
          })
        );

@ -203,24 +174,6 @@ const AddService = ({
    }
  };

-  // Service name validation
-  const handleServiceNameValidation = (
-    event: React.ChangeEvent<HTMLInputElement | HTMLSelectElement>
-  ) => {
-    const value = event.target.value;
-    setServiceName(value);
-    if (value) {
-      setShowErrorMessage({
-        ...showErrorMessage,
-        name: false,
-        delimit: false,
-        specialChar: false,
-        nameLength: false,
-        isError: false,
-      });
-    }
-  };
-
  // Service focused field
  const handleFieldFocus = (fieldName: string) => {
    if (isEmpty(fieldName)) {
@ -267,18 +220,7 @@ const AddService = ({

        {activeServiceStep === 2 && (
          <ConfigureService
-            description={description}
-            handleValidation={handleServiceNameValidation}
-            serviceName={serviceName}
-            showError={{
-              name: showErrorMessage.name,
-              duplicateName: showErrorMessage.duplicateName,
-              nameWithSpace: showErrorMessage.nameWithSpace,
-              delimit: showErrorMessage.delimit,
-              specialChar: showErrorMessage.specialChar,
-              nameLength: showErrorMessage.nameLength,
-              allowChar: showErrorMessage.allowChar,
-            }}
+            serviceName={serviceConfig.serviceName}
            onBack={handleConfigureServiceBackClick}
            onNext={handleConfigureServiceNextClick}
          />
@ -303,7 +245,7 @@ const AddService = ({
            showIngestionButton
            handleIngestionClick={() => handleAddIngestion(true)}
            handleViewServiceClick={handleViewServiceClick}
-            name={serviceName}
+            name={serviceConfig.serviceName}
            state={FormSubmitType.ADD}
            suffix={getServiceCreatedLabel(serviceCategory)}
          />
--- a/openmetadata-ui/src/main/resources/ui/src/components/AddService/AddService.interface.ts
+++ b/openmetadata-ui/src/main/resources/ui/src/components/AddService/AddService.interface.ts
@ -31,3 +31,8 @@ export interface AddServiceProps {
  slashedBreadcrumb: TitleBreadcrumbProps['titleLinks'];
  onIngestionDeploy?: () => Promise<void>;
 }
+
+export interface ServiceConfig {
+  serviceName: string;
+  description: string;
+}
--- a/openmetadata-ui/src/main/resources/ui/src/components/AddService/Steps/ConfigureService.test.tsx
+++ b/openmetadata-ui/src/main/resources/ui/src/components/AddService/Steps/ConfigureService.test.tsx
@ -11,83 +11,63 @@
 *  limitations under the License.
 */

-import {
-  findByTestId,
-  findByText,
-  fireEvent,
-  render,
-} from '@testing-library/react';
-import React, { forwardRef } from 'react';
+import { act, render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import React from 'react';
 import ConfigureService from './ConfigureService';
 import { ConfigureServiceProps } from './Steps.interface';

 const mockConfigureServiceProps: ConfigureServiceProps = {
  serviceName: 'testService',
-  description: '',
-  showError: {
-    name: false,
-    duplicateName: false,
-    nameWithSpace: false,
-    delimit: false,
-    specialChar: false,
-    nameLength: false,
-    allowChar: false,
-  },
-  handleValidation: jest.fn(),
  onBack: jest.fn(),
  onNext: jest.fn(),
 };

-jest.mock('../../common/rich-text-editor/RichTextEditor', () => {
-  return forwardRef(
-    jest.fn().mockImplementation(({ initialValue }) => {
-      return (
-        <div
-          ref={(input) => {
-            return {
-              getEditorContent: input,
-            };
-          }}>
-          {initialValue}RichTextEditor.component
-        </div>
-      );
-    })
-  );
-});
-
 describe('Test ConfigureService component', () => {
  it('ConfigureService component should render', async () => {
-    const { container } = render(
-      <ConfigureService {...mockConfigureServiceProps} />
-    );
+    render(<ConfigureService {...mockConfigureServiceProps} />);

-    const configureServiceContainer = await findByTestId(
-      container,
+    const configureServiceContainer = screen.getByTestId(
      'configure-service-container'
    );
-    const serviceName = await findByTestId(container, 'service-name');
-    const backButton = await findByTestId(container, 'back-button');
-    const nextButton = await findByTestId(container, 'next-button');
-    const richTextEditor = await findByText(
-      container,
-      'RichTextEditor.component'
-    );
-
-    fireEvent.change(serviceName, {
-      target: {
-        value: 'newName',
-      },
-    });
-    fireEvent.click(backButton);
-    fireEvent.click(nextButton);
+    const serviceName = screen.getByTestId('service-name');
+    const backButton = screen.getByTestId('back-button');
+    const nextButton = screen.getByTestId('next-button');
+    const richTextEditor = screen.getByTestId('editor');

    expect(configureServiceContainer).toBeInTheDocument();
    expect(richTextEditor).toBeInTheDocument();
    expect(serviceName).toBeInTheDocument();
    expect(backButton).toBeInTheDocument();
    expect(nextButton).toBeInTheDocument();
-    expect(mockConfigureServiceProps.handleValidation).toHaveBeenCalled();
+  });
+
+  it('Back button should work', () => {
+    render(<ConfigureService {...mockConfigureServiceProps} />);
+    const backButton = screen.getByTestId('back-button');
+
+    userEvent.click(backButton);
+
    expect(mockConfigureServiceProps.onBack).toHaveBeenCalled();
+  });
+
+  it('Next button should work', async () => {
+    render(<ConfigureService {...mockConfigureServiceProps} />);
+    const serviceName = screen.getByTestId('service-name');
+    const nextButton = screen.getByTestId('next-button');
+
+    userEvent.type(serviceName, 'newName');
+
+    await act(async () => {
+      userEvent.click(nextButton);
+    });
+
+    expect(serviceName).toHaveValue('newName');
+
    expect(mockConfigureServiceProps.onNext).toHaveBeenCalled();
+    expect(mockConfigureServiceProps.onNext).toHaveBeenCalledWith({
+      description: '',
+      serviceName: 'newName',
+    });
  });
 });
--- a/openmetadata-ui/src/main/resources/ui/src/components/AddService/Steps/ConfigureService.tsx
+++ b/openmetadata-ui/src/main/resources/ui/src/components/AddService/Steps/ConfigureService.tsx
@ -11,96 +11,79 @@
 *  limitations under the License.
 */

-import { Button } from 'antd';
+import { Button, Form, FormProps, Space } from 'antd';
+import { ENTITY_NAME_REGEX } from 'constants/regex.constants';
 import { t } from 'i18next';
-import React, { useRef } from 'react';
-import { errorMsg, requiredField } from '../../../utils/CommonUtils';
-import RichTextEditor from '../../common/rich-text-editor/RichTextEditor';
-import { EditorContentRef } from '../../common/rich-text-editor/RichTextEditor.interface';
-import { Field } from '../../Field/Field';
+import React from 'react';
+import { FieldProp, FieldTypes, generateFormFields } from 'utils/formUtils';
 import { ConfigureServiceProps } from './Steps.interface';

 const ConfigureService = ({
  serviceName,
-  description,
-  showError,
-  handleValidation,
  onBack,
  onNext,
 }: ConfigureServiceProps) => {
-  const markdownRef = useRef<EditorContentRef>();
+  const [form] = Form.useForm();

-  const validationErrorMsg = (): string => {
-    if (showError.name) {
-      return t('message.field-text-is-required', {
-        fieldText: t('label.service-name'),
-      });
-    }
-    if (showError.duplicateName) {
-      return t('message.entity-already-exists', {
-        entity: t('label.service-name'),
-      });
-    }
-    if (showError.delimit) {
-      return t('message.service-with-delimiters-not-allowed');
-    }
-    if (showError.nameWithSpace) {
-      return t('message.service-with-space-not-allowed');
-    }
-    if (showError.nameLength) {
-      return t('message.service-name-length');
-    }
-    if (showError.specialChar) {
-      return t('message.special-character-not-allowed');
-    }
+  const formFields: FieldProp[] = [
+    {
+      name: 'name',
+      id: 'root/name',
+      required: true,
+      label: t('label.service-name'),
+      type: FieldTypes.TEXT,
+      rules: [
+        {
+          pattern: ENTITY_NAME_REGEX,
+          message: t('message.entity-name-validation'),
+        },
+      ],
+      props: {
+        'data-testid': 'service-name',
+      },
+      placeholder: t('label.service-name'),
+      formItemProps: {
+        initialValue: serviceName,
+      },
+    },
+    {
+      name: 'description',
+      required: false,
+      label: t('label.description'),
+      id: 'root/description',
+      type: FieldTypes.DESCRIPTION,
+      props: {
+        'data-testid': 'description',
+        initialValue: '',
+      },
+      formItemProps: {
+        trigger: 'onTextChange',
+        valuePropName: 'initialValue',
+      },
+    },
+  ];

-    return '';
+  const handleSubmit: FormProps['onFinish'] = (data) => {
+    onNext({ serviceName: data.name, description: data.description ?? '' });
  };

  return (
-    <div data-testid="configure-service-container">
-      <Field>
-        <label className="tw-block tw-form-label" htmlFor="serviceName">
-          {requiredField(`${t('label.service-name')}:`)}
-        </label>
-
-        <input
-          className="tw-form-inputs tw-form-inputs-padding"
-          data-testid="service-name"
-          id="serviceName"
-          name="serviceName"
-          placeholder={t('label.service-name')}
-          type="text"
-          value={serviceName}
-          onChange={handleValidation}
-        />
-        {errorMsg(validationErrorMsg())}
-      </Field>
-      <Field>
-        <label className="tw-block tw-form-label" htmlFor="description">
-          {`${t('label.description')}:`}
-        </label>
-        <RichTextEditor initialValue={description} ref={markdownRef} />
-      </Field>
-
-      <Field className="d-flex tw-justify-end tw-mt-10">
-        <Button
-          className="m-r-xs"
-          data-testid="back-button"
-          type="link"
-          onClick={onBack}>
+    <Form
+      data-testid="configure-service-container"
+      form={form}
+      layout="vertical"
+      onFinish={handleSubmit}>
+      {generateFormFields(formFields)}
+      <Space className="w-full justify-end">
+        <Button data-testid="back-button" type="link" onClick={onBack}>
          {t('label.back')}
        </Button>

-        <Button
-          className="font-medium p-x-md p-y-xxs h-auto rounded-6"
-          data-testid="next-button"
-          type="primary"
-          onClick={() => onNext(markdownRef.current?.getEditorContent() || '')}>
+        <Button data-testid="next-button" htmlType="submit" type="primary">
          {t('label.next')}
        </Button>
-      </Field>
-    </div>
+      </Space>
+    </Form>
  );
 };

--- a/openmetadata-ui/src/main/resources/ui/src/components/AddService/Steps/Steps.interface.ts
+++ b/openmetadata-ui/src/main/resources/ui/src/components/AddService/Steps/Steps.interface.ts
@ -13,6 +13,7 @@

 import { DynamicFormFieldType } from 'Models';
 import { ServiceCategory } from '../../../enums/service.enum';
+import { ServiceConfig } from '../AddService.interface';

 export type SelectServiceTypeProps = {
  showError: boolean;
@ -26,21 +27,8 @@ export type SelectServiceTypeProps = {

 export type ConfigureServiceProps = {
  serviceName: string;
-  description: string;
-  showError: {
-    name: boolean;
-    duplicateName: boolean;
-    nameWithSpace: boolean;
-    delimit: boolean;
-    specialChar: boolean;
-    nameLength: boolean;
-    allowChar: boolean;
-  };
-  handleValidation: (
-    event: React.ChangeEvent<HTMLInputElement | HTMLSelectElement>
-  ) => void;
  onBack: () => void;
-  onNext: (description: string) => void;
+  onNext: (data: ServiceConfig) => void;
 };

 export type ConnectionDetailsProps = {
--- a/openmetadata-ui/src/main/resources/ui/src/constants/Services.constant.ts
+++ b/openmetadata-ui/src/main/resources/ui/src/constants/Services.constant.ts
@ -307,14 +307,6 @@ export const STEPS_FOR_ADD_SERVICE: Array<StepperStepType> = [

 export const SERVICE_DEFAULT_ERROR_MAP = {
  serviceType: false,
-  name: false,
-  duplicateName: false,
-  nameWithSpace: false,
-  delimit: false,
-  specialChar: false,
-  nameLength: false,
-  allowChar: false,
-  isError: false,
 };
 // 2 minutes
 export const FETCHING_EXPIRY_TIME = 2 * 60 * 1000;
--- a/openmetadata-ui/src/main/resources/ui/src/styles/tailwind.css
+++ b/openmetadata-ui/src/main/resources/ui/src/styles/tailwind.css
@ -137,10 +137,6 @@
    @apply tw-font-normal tw-px-2;
  }

-  .tw-form-label {
-    @apply tw-block tw-text-body tw-font-normal tw-text-grey-body tw-mb-2;
-  }
-
  .activeCategory .label-category {
    @apply tw-text-primary;
  }