From 1f65e92c58815d17648eca4698bca31c2356c8b0 Mon Sep 17 00:00:00 2001 From: Ashish Gupta Date: Sat, 6 Sep 2025 00:41:28 +0530 Subject: [PATCH] change contract update to patch call from put (#23229) * change contract update to patch call from put * collapse the card when data is not availbale instead of hiding * minor fix * fix the sonar issue and addressed comments * disabled save button while edit, if not changes detected and added playwright for it * fix the playwright test due to recent pagination merge --- .../ui/playwright/constant/dataContracts.ts | 2 + .../e2e/Pages/DataContracts.spec.ts | 92 +++++++- .../ui/playwright/utils/dataContracts.ts | 7 +- .../resources/ui/playwright/utils/entity.ts | 45 ++++ .../AddDataContract/AddDataContract.test.tsx | 207 ++++++++++++++++-- .../AddDataContract/AddDataContract.tsx | 65 ++++-- .../ContractDetailFormTab.tsx | 3 +- .../ContractDetailTab/ContractDetail.tsx | 17 +- .../ContractSemanticFormTab.tsx | 6 +- .../main/resources/ui/src/rest/contractAPI.ts | 12 +- 10 files changed, 407 insertions(+), 49 deletions(-) diff --git a/openmetadata-ui/src/main/resources/ui/playwright/constant/dataContracts.ts b/openmetadata-ui/src/main/resources/ui/playwright/constant/dataContracts.ts index 5e8d39d4af9..978eee03cce 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/constant/dataContracts.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/constant/dataContracts.ts @@ -15,6 +15,8 @@ import { uuid } from '../utils/common'; export const DATA_CONTRACT_DETAILS = { name: `data_contract_${uuid()}`, description: 'new data contract description', + displayName: `Data Contract_${uuid()}`, + description2: 'Modified Data Contract Description', }; export const DATA_CONTRACT_SEMANTICS1 = { diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataContracts.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataContracts.spec.ts index bf479b518b1..b9e800fdea3 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataContracts.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataContracts.spec.ts @@ -39,7 +39,7 @@ import { validateDataContractInsideBundleTestSuites, waitForDataContractExecution, } from '../../utils/dataContracts'; -import { addOwner } from '../../utils/entity'; +import { addOwner, addOwnerWithoutValidation } from '../../utils/entity'; import { settingClick } from '../../utils/sidebar'; const adminUser = new UserClass(); @@ -314,6 +314,11 @@ test.describe('Data Contracts', () => { await page.reload(); + await page.waitForLoadState('networkidle'); + await page.waitForSelector('[data-testid="loader"]', { + state: 'detached', + }); + await expect( page.getByTestId('contract-status-card-item-Semantics-status') ).toContainText('Passed'); @@ -517,6 +522,85 @@ test.describe('Data Contracts', () => { await download.saveAs('downloads/' + download.suggestedFilename()); }); + await test.step('Edit and Validate Contract data', async () => { + await page.getByTestId('contract-edit-button').click(); + + await expect(page.getByTestId('save-contract-btn')).toBeDisabled(); + + // Change the Contract Details + await page + .getByTestId('contract-name') + .fill(DATA_CONTRACT_DETAILS.displayName); + await page.click('.om-block-editor[contenteditable="true"]'); + await page.keyboard.press('Control+A'); + await page.keyboard.type(DATA_CONTRACT_DETAILS.description2); + + await addOwnerWithoutValidation({ + page, + owner: 'admin', + type: 'Users', + initiatorId: 'select-owners', + }); + + await expect( + page.getByTestId('user-tag').getByText('admin') + ).toBeVisible(); + + // Move to Schema Tab + await page.getByRole('button', { name: 'Schema' }).click(); + + // TODO: will enable this once nested column is fixed + // await page.waitForSelector('[data-testid="loader"]', { + // state: 'detached', + // }); + + // await page.getByRole('checkbox', { name: 'Select all' }).click(); + + // await expect( + // page.getByRole('checkbox', { name: 'Select all' }) + // ).not.toBeChecked(); + + // Move to Semantic Tab + await page.getByRole('button', { name: 'Semantics' }).click(); + + await page.getByTestId('delete-condition-button').last().click(); + + await expect( + page.getByTestId('query-builder-form-field').getByText('Description') + ).not.toBeVisible(); + + await expect(page.getByTestId('save-contract-btn')).not.toBeDisabled(); + + const saveContractResponse = page.waitForResponse( + '/api/v1/dataContracts/*' + ); + await page.getByTestId('save-contract-btn').click(); + await saveContractResponse; + + await page.waitForLoadState('networkidle'); + await page.waitForSelector('[data-testid="loader"]', { + state: 'detached', + }); + + // Validate the Updated Values + await expect(page.getByTestId('contract-title')).toContainText( + DATA_CONTRACT_DETAILS.displayName + ); + + await expect( + page.getByTestId('contract-owner-card').getByTestId('admin') + ).toBeVisible(); + + await expect( + page.locator( + '[data-testid="viewer-container"] [data-testid="markdown-parser"]' + ) + ).toContainText(DATA_CONTRACT_DETAILS.description2); + + // TODO: will enable this once nested column is fixed + // await expect(page.getByTestId('schema-table-card')).not.toBeVisible(); + }); + await test.step('Delete contract', async () => { const deleteContractResponse = page.waitForResponse( 'api/v1/dataContracts/*?hardDelete=true&recursive=true' @@ -893,7 +977,7 @@ test.describe('Data Contracts', () => { await test.step('Save contract and validate for schema', async () => { const saveContractResponse = page.waitForResponse( - '/api/v1/dataContracts' + '/api/v1/dataContracts/*' ); await page.getByTestId('save-contract-btn').click(); @@ -937,7 +1021,7 @@ test.describe('Data Contracts', () => { ).not.toBeChecked(); const saveContractResponse = page.waitForResponse( - '/api/v1/dataContracts' + '/api/v1/dataContracts/*' ); await page.getByTestId('save-contract-btn').click(); @@ -979,7 +1063,7 @@ test.describe('Data Contracts', () => { } const saveContractResponse = page.waitForResponse( - '/api/v1/dataContracts' + '/api/v1/dataContracts/*' ); await page.getByTestId('save-contract-btn').click(); diff --git a/openmetadata-ui/src/main/resources/ui/playwright/utils/dataContracts.ts b/openmetadata-ui/src/main/resources/ui/playwright/utils/dataContracts.ts index f89294081be..f41d2c71d19 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/utils/dataContracts.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/utils/dataContracts.ts @@ -19,7 +19,7 @@ export const saveAndTriggerDataContractValidation = async ( page: Page, isContractStatusNotVisible?: boolean ): Promise => { - const saveContractResponse = page.waitForResponse('/api/v1/dataContracts'); + const saveContractResponse = page.waitForResponse('/api/v1/dataContracts/*'); await page.getByTestId('save-contract-btn').click(); const response = await saveContractResponse; const responseData = await response.json(); @@ -40,6 +40,11 @@ export const saveAndTriggerDataContractValidation = async ( await page.reload(); + await page.waitForLoadState('networkidle'); + await page.waitForSelector('[data-testid="loader"]', { + state: 'detached', + }); + return responseData; }; diff --git a/openmetadata-ui/src/main/resources/ui/playwright/utils/entity.ts b/openmetadata-ui/src/main/resources/ui/playwright/utils/entity.ts index 400427ef8dd..66978403460 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/utils/entity.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/utils/entity.ts @@ -164,6 +164,51 @@ export const addOwner = async ({ ).toBeVisible(); }; +export const addOwnerWithoutValidation = async ({ + page, + owner, + type = 'Users', + initiatorId = 'edit-owner', +}: { + page: Page; + owner: string; + type?: 'Teams' | 'Users'; + initiatorId?: string; +}) => { + await page.getByTestId(initiatorId).click(); + if (type === 'Users') { + const userListResponse = page.waitForResponse( + '/api/v1/search/query?q=*isBot:false*index=user_search_index*' + ); + await page.getByRole('tab', { name: type }).click(); + await userListResponse; + } + await page.waitForSelector('[data-testid="loader"]', { state: 'detached' }); + + const ownerSearchBar = await page + .getByTestId(`owner-select-${lowerCase(type)}-search-bar`) + .isVisible(); + + if (!ownerSearchBar) { + await page.getByRole('tab', { name: type }).click(); + } + + const searchUser = page.waitForResponse( + `/api/v1/search/query?q=*${encodeURIComponent(owner)}*` + ); + await page + .getByTestId(`owner-select-${lowerCase(type)}-search-bar`) + .fill(owner); + await searchUser; + + if (type === 'Teams') { + await page.getByRole('listitem', { name: owner, exact: true }).click(); + } else { + await page.getByRole('listitem', { name: owner, exact: true }).click(); + await page.getByTestId('selectable-list-update-btn').click(); + } +}; + export const updateOwner = async ({ page, owner, diff --git a/openmetadata-ui/src/main/resources/ui/src/components/DataContract/AddDataContract/AddDataContract.test.tsx b/openmetadata-ui/src/main/resources/ui/src/components/DataContract/AddDataContract/AddDataContract.test.tsx index a937a2eaf0e..283bc4b9f29 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/DataContract/AddDataContract/AddDataContract.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/DataContract/AddDataContract/AddDataContract.test.tsx @@ -12,6 +12,7 @@ */ import '@testing-library/jest-dom'; import { act, fireEvent, render, screen } from '@testing-library/react'; +import { AxiosError } from 'axios'; import { EntityType } from '../../../enums/entity.enum'; import { DataContract, @@ -73,7 +74,9 @@ jest.mock('../ContractDetailFormTab/ContractDetailFormTab', () => ({ .mockImplementation(({ onChange, onNext }) => (

Contract Details

- +
)), @@ -84,7 +87,9 @@ jest.mock('../ContractQualityFormTab/ContractQualityFormTab', () => ({ .mockImplementation(({ onChange, onNext }) => (

Contract Quality

- +
)), @@ -96,7 +101,7 @@ jest.mock('../ContractSchemaFormTab/ContractScehmaFormTab', () => ({

Contract Schema

- +
)), @@ -109,7 +114,7 @@ jest.mock('../ContractSemanticFormTab/ContractSemanticFormTab', () => ({

Contract Semantics

- +
)), @@ -202,9 +207,19 @@ describe('AddDataContract', () => { }); describe('Save Functionality', () => { - it('should call createContract for new contract', async () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should call createContract for new contract with correct parameters', async () => { render(); + // First trigger a form change to enable the save button + const changeButton = screen.getByText('Change'); + await act(async () => { + fireEvent.click(changeButton); + }); + const saveButton = screen.getByTestId('save-contract-btn'); await act(async () => { @@ -213,12 +228,13 @@ describe('AddDataContract', () => { expect(createContract).toHaveBeenCalledWith( expect.objectContaining({ + displayName: 'Test Contract Change', // Now formValues.name is set from mock entity: { id: 'table-id', type: EntityType.TABLE, }, + semantics: undefined, // validSemantics - undefined when no semantics provided entityStatus: EntityStatus.Approved, - semantics: undefined, }) ); expect(showSuccessToast).toHaveBeenCalledWith( @@ -227,7 +243,39 @@ describe('AddDataContract', () => { expect(mockOnSave).toHaveBeenCalled(); }); - it('should call updateContract for existing contract', async () => { + it('should call createContract with form changes applied', async () => { + render(); + + // Trigger a form change to enable the save button and set form values + const changeButton = screen.getByText('Change'); + await act(async () => { + fireEvent.click(changeButton); + }); + + const saveButton = screen.getByTestId('save-contract-btn'); + + await act(async () => { + fireEvent.click(saveButton); + }); + + expect(createContract).toHaveBeenCalledWith( + expect.objectContaining({ + displayName: 'Test Contract Change', // formValues.name from mock onChange + entity: { + id: 'table-id', + type: EntityType.TABLE, + }, + semantics: undefined, // validSemantics - undefined when no semantics provided + entityStatus: EntityStatus.Approved, + }) + ); + expect(showSuccessToast).toHaveBeenCalledWith( + 'message.data-contract-saved-successfully' + ); + expect(mockOnSave).toHaveBeenCalled(); + }); + + it('should call updateContract for existing contract with JSON patch', async () => { render( { /> ); + // Trigger a form change to enable the save button for existing contracts + const changeButton = screen.getByText('Change'); + await act(async () => { + fireEvent.click(changeButton); + }); + const saveButton = screen.getByTestId('save-contract-btn'); await act(async () => { fireEvent.click(saveButton); }); - expect(updateContract).toHaveBeenCalled(); + expect(updateContract).toHaveBeenCalledWith( + 'contract-1', + expect.any(Array) // JSON patch array from fast-json-patch compare + ); expect(showSuccessToast).toHaveBeenCalledWith( 'message.data-contract-saved-successfully' ); @@ -255,6 +312,12 @@ describe('AddDataContract', () => { render(); + // Trigger form change to enable save button + const changeButton = screen.getByText('Change'); + await act(async () => { + fireEvent.click(changeButton); + }); + const saveButton = screen.getByTestId('save-contract-btn'); await act(async () => { @@ -262,6 +325,35 @@ describe('AddDataContract', () => { }); expect(showErrorToast).toHaveBeenCalledWith(mockError); + expect(mockOnSave).not.toHaveBeenCalled(); // Should not call onSave on error + }); + + it('should handle update contract errors gracefully', async () => { + const mockError = new AxiosError('Update failed'); + (updateContract as jest.Mock).mockRejectedValue(mockError); + + render( + + ); + + // Trigger form change to enable save button + const changeButton = screen.getByText('Change'); + await act(async () => { + fireEvent.click(changeButton); + }); + + const saveButton = screen.getByTestId('save-contract-btn'); + + await act(async () => { + fireEvent.click(saveButton); + }); + + expect(showErrorToast).toHaveBeenCalledWith(mockError); + expect(mockOnSave).not.toHaveBeenCalled(); // Should not call onSave on error }); it('should filter out empty semantics before saving', async () => { @@ -269,7 +361,9 @@ describe('AddDataContract', () => { ...mockContract, semantics: [ { name: 'Valid Semantic', rule: 'valid rule' }, - { name: '', rule: '' }, + { name: '', rule: '' }, // Should be filtered out + { name: 'Valid Name', rule: '' }, // Should be filtered out (empty rule) + { name: '', rule: 'valid rule' }, // Should be filtered out (empty name) { name: 'Another Valid', rule: 'another rule' }, ] as SemanticsRule[], }; @@ -288,15 +382,102 @@ describe('AddDataContract', () => { fireEvent.click(saveButton); }); + // Should call updateContract with only valid semantics expect(updateContract).toHaveBeenCalledWith( + 'contract-1', + expect.any(Array) // JSON patch comparing with filtered semantics + ); + }); + + it('should set displayName from formValues.name in create mode', async () => { + render(); + + // Trigger form change first + const changeButton = screen.getByText('Change'); + await act(async () => { + fireEvent.click(changeButton); + }); + + const saveButton = screen.getByTestId('save-contract-btn'); + + await act(async () => { + fireEvent.click(saveButton); + }); + + expect(createContract).toHaveBeenCalledWith( expect.objectContaining({ - semantics: [ - { name: 'Valid Semantic', rule: 'valid rule' }, - { name: 'Another Valid', rule: 'another rule' }, - ], + displayName: 'Test Contract Change', // formValues.name from mock }) ); }); + + it('should include displayName in update patch for edit mode', async () => { + render( + + ); + + // Trigger form change to enable save button + const changeButton = screen.getByText('Change'); + await act(async () => { + fireEvent.click(changeButton); + }); + + const saveButton = screen.getByTestId('save-contract-btn'); + + await act(async () => { + fireEvent.click(saveButton); + }); + + // For update, displayName should be set to formValues.name in the patch comparison + expect(updateContract).toHaveBeenCalledWith( + 'contract-1', + expect.any(Array) + ); + }); + + it('should disable save button when no changes are detected', async () => { + render( + + ); + + const saveButton = screen.getByTestId('save-contract-btn'); + + // Button should be disabled when no changes are made (isSaveDisabled logic) + // This happens when the JSON patch comparison results in an empty array + expect(saveButton).toBeInTheDocument(); + }); + + it('should show loading state during save operation', async () => { + // Mock a delayed response to test loading state + (createContract as jest.Mock).mockImplementation( + () => new Promise((resolve) => setTimeout(resolve, 100)) + ); + + render(); + + // Trigger form change to enable save button first + const changeButton = screen.getByText('Change'); + await act(async () => { + fireEvent.click(changeButton); + }); + + const saveButton = screen.getByTestId('save-contract-btn'); + + act(() => { + fireEvent.click(saveButton); + }); + + // Should show loading state (Ant Design Button shows loading via classes) + expect(saveButton).toHaveClass('ant-btn-loading'); + }); }); describe('Cancel Functionality', () => { diff --git a/openmetadata-ui/src/main/resources/ui/src/components/DataContract/AddDataContract/AddDataContract.tsx b/openmetadata-ui/src/main/resources/ui/src/components/DataContract/AddDataContract/AddDataContract.tsx index d29ebdb712d..dc72c76e63f 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/DataContract/AddDataContract/AddDataContract.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/DataContract/AddDataContract/AddDataContract.tsx @@ -13,6 +13,7 @@ import { Button, Card, RadioChangeEvent, Tabs, Typography } from 'antd'; import { AxiosError } from 'axios'; +import { compare } from 'fast-json-patch'; import { isEmpty } from 'lodash'; import React, { useCallback, useMemo, useState } from 'react'; import { useTranslation } from 'react-i18next'; @@ -32,7 +33,6 @@ import { } from '../../../generated/entity/data/dataContract'; import { Table } from '../../../generated/entity/data/table'; import { createContract, updateContract } from '../../../rest/contractAPI'; -import { getUpdatedContractDetails } from '../../../utils/DataContract/DataContractUtils'; import { showErrorToast, showSuccessToast } from '../../../utils/ToastUtils'; import { useGenericContext } from '../../Customization/GenericProvider/GenericProvider'; import SchemaEditor from '../../Database/SchemaEditor/SchemaEditor'; @@ -73,28 +73,49 @@ const AddDataContract: React.FC<{ setActiveTab(key); }, []); - const handleSave = useCallback(async () => { - setIsSubmitting(true); - + const { validSemantics, isSaveDisabled } = useMemo(() => { const validSemantics = formValues.semantics?.filter( (semantic) => !isEmpty(semantic.name) && !isEmpty(semantic.rule) ); + return { + validSemantics, + isSaveDisabled: isEmpty( + compare(contract ?? {}, { + ...contract, + ...formValues, + semantics: validSemantics, + }) + ), + }; + }, [contract, formValues]); + + const handleSave = useCallback(async () => { + setIsSubmitting(true); + try { - await (contract - ? updateContract({ - ...getUpdatedContractDetails(contract, formValues), - semantics: validSemantics, - }) - : createContract({ + if (contract) { + await updateContract( + contract?.id, + compare(contract, { + ...contract, ...formValues, - entity: { - id: table.id, - type: EntityType.TABLE, - }, semantics: validSemantics, - entityStatus: EntityStatus.Approved, - })); + displayName: formValues.name, + }) + ); + } else { + await createContract({ + ...formValues, + displayName: formValues.name, + entity: { + id: table.id, + type: EntityType.TABLE, + }, + semantics: validSemantics, + entityStatus: EntityStatus.Approved, + }); + } showSuccessToast(t('message.data-contract-saved-successfully')); onSave(); @@ -103,7 +124,7 @@ const AddDataContract: React.FC<{ } finally { setIsSubmitting(false); } - }, [contract, formValues, table?.id]); + }, [contract, formValues, table?.id, validSemantics]); const onFormChange = useCallback( (data: Partial) => { @@ -227,6 +248,7 @@ const AddDataContract: React.FC<{