From c68d85ebd559e81f744bce4dcc90076e581a5d43 Mon Sep 17 00:00:00 2001 From: Sachin Chaurasiya Date: Thu, 25 May 2023 13:33:29 +0530 Subject: [PATCH] refactor: configure service form (#11734) * refactor: configure service form * improve unit test * address comments --- .../resources/ui/cypress/common/common.js | 16 ++- .../AddService/AddService.component.tsx | 96 +++---------- .../AddService/AddService.interface.ts | 5 + .../Steps/ConfigureService.test.tsx | 92 +++++-------- .../AddService/Steps/ConfigureService.tsx | 127 ++++++++---------- .../AddService/Steps/Steps.interface.ts | 16 +-- .../ui/src/constants/Services.constant.ts | 8 -- .../main/resources/ui/src/styles/tailwind.css | 4 - 8 files changed, 132 insertions(+), 232 deletions(-) diff --git a/openmetadata-ui/src/main/resources/ui/cypress/common/common.js b/openmetadata-ui/src/main/resources/ui/cypress/common/common.js index 95df0993e6f..12ebbeed049 100644 --- 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', diff --git 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 index e16f9704995..56a3917cfd1 100644 --- 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({ + serviceName: '', + description: '', + }); + const [saveServiceState, setSaveServiceState] = useState('initial'); const [activeField, setActiveField] = useState(''); 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 - ) => { - 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 && ( @@ -303,7 +245,7 @@ const AddService = ({ showIngestionButton handleIngestionClick={() => handleAddIngestion(true)} handleViewServiceClick={handleViewServiceClick} - name={serviceName} + name={serviceConfig.serviceName} state={FormSubmitType.ADD} suffix={getServiceCreatedLabel(serviceCategory)} /> diff --git 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 index b7a32835491..8deae89a720 100644 --- 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; } + +export interface ServiceConfig { + serviceName: string; + description: string; +} diff --git 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 index e974fd7a018..79cc46c7f4b 100644 --- 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 ( -
{ - return { - getEditorContent: input, - }; - }}> - {initialValue}RichTextEditor.component -
- ); - }) - ); -}); - describe('Test ConfigureService component', () => { it('ConfigureService component should render', async () => { - const { container } = render( - - ); + render(); - 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(); + const backButton = screen.getByTestId('back-button'); + + userEvent.click(backButton); + expect(mockConfigureServiceProps.onBack).toHaveBeenCalled(); + }); + + it('Next button should work', async () => { + render(); + 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', + }); }); }); diff --git 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 index 31a1f611ad8..5175680bc78 100644 --- 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(); + 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 ( -
- - - - - {errorMsg(validationErrorMsg())} - - - - - - - - - - -
+ + ); }; diff --git 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 index 18a3becf4a6..e88b55dfa10 100644 --- 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 - ) => void; onBack: () => void; - onNext: (description: string) => void; + onNext: (data: ServiceConfig) => void; }; export type ConnectionDetailsProps = { diff --git a/openmetadata-ui/src/main/resources/ui/src/constants/Services.constant.ts b/openmetadata-ui/src/main/resources/ui/src/constants/Services.constant.ts index afa6a1d220d..c6b007f730a 100644 --- 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 = [ 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; diff --git a/openmetadata-ui/src/main/resources/ui/src/styles/tailwind.css b/openmetadata-ui/src/main/resources/ui/src/styles/tailwind.css index 5e7aa2af2c2..e3c5672cb5c 100644 --- 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; }