Skip to content

Migrated ContractDetails and ODCSModal#29013

Open
Rohit0301 wants to merge 8 commits into
mainfrom
data-contract-mui-migration
Open

Migrated ContractDetails and ODCSModal#29013
Rohit0301 wants to merge 8 commits into
mainfrom
data-contract-mui-migration

Conversation

@Rohit0301

@Rohit0301 Rohit0301 commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Describe your changes:

Fixes #

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

High-level design:

N/A — small change.

Tests:

Use cases covered

Unit tests

Backend integration tests

Ingestion integration tests

Playwright (UI) tests

Manual testing performed

UI screen recording / screenshots:

Not applicable.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.
  • For UI changes: I attached a screen recording and/or screenshots above.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.

Summary by Gitar

  • UI Migration:
    • Migrated ContractDetail and ODCSImportModal components from @mui/material and antd to @openmetadata/ui-core-components.
    • Refactored styles to use Tailwind utility classes and updated component props to align with the new library standards.
  • Component logic updates:
    • Updated ODCSImportModal file upload handling by replacing custom drag-and-drop logic with FileUploadDropZone.
    • Optimized state management and event handlers for better integration with the core component library.
  • Testing updates:
    • Added extensive mocks for the new UI components in ContractDetail.test.tsx and ODCSImportModal.test.tsx to accommodate the library migration.
    • Cleaned up redundant MUI-specific test mocks and updated existing test cases to support the new component structure.

This will update automatically on new commits.

@Rohit0301 Rohit0301 self-assigned this Jun 12, 2026
@Rohit0301 Rohit0301 requested a review from a team as a code owner June 12, 2026 15:01
@Rohit0301 Rohit0301 added safe to test Add this label to run secure Github workflows on PRs skip-pr-checks Bypass PR metadata validation check labels Jun 12, 2026
@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 62%
62.42% (67011/107351) 43.91% (37089/84456) 45.78% (11459/25027)

@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

🔴 Playwright Results — 1 failure(s), 12 flaky

✅ 4291 passed · ❌ 1 failed · 🟡 12 flaky · ⏭️ 88 skipped

Shard Passed Failed Flaky Skipped
✅ Shard 1 301 0 0 4
🟡 Shard 2 811 0 1 9
🟡 Shard 3 807 0 5 8
🔴 Shard 4 846 1 3 12
🟡 Shard 5 733 0 1 47
🟡 Shard 6 793 0 2 8

Genuine Failures (failed on all attempts)

Pages/DataContractInheritance.spec.ts › Delete Asset Contract - Falls back to showing inherited contract from Data Product (shard 4)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator: locator('.ant-modal-title')
Expected: visible
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for locator('.ant-modal-title')�[22m

🟡 12 flaky test(s) (passed on retry)
  • Features/Glossary/GlossaryAdvancedOperations.spec.ts › should create term with related terms (shard 2, 1 retry)
  • Features/KnowledgeCenterList.spec.ts › Knowledge Center List - Verify Recently Viewed widget (shard 3, 1 retry)
  • Features/RTL.spec.ts › Verify Following widget functionality (shard 3, 2 retries)
  • Features/Table.spec.ts › Table pagination with sorting should works (shard 3, 1 retry)
  • Features/Table.spec.ts › should show dbt tab if only path is present (shard 3, 1 retry)
  • Features/Tasks/TaskNavigation.spec.ts › navigating to /table/TASK-XXXXX should show 404 (invalid URL pattern) (shard 3, 1 retry)
  • Flow/PersonaFlow.spec.ts › Set default persona for team should work properly (shard 4, 1 retry)
  • Pages/CustomProperties.spec.ts › Email (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Store Procedure (shard 4, 1 retry)
  • Pages/ExplorePageRightPanel_KnowledgeCenter.spec.ts › Should remove user owner for knowledgeCenter (shard 5, 1 retry)
  • Pages/Lineage/DataAssetLineage.spec.ts › Column lineage for searchIndex -> table (shard 6, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@sonarqubecloud

Copy link
Copy Markdown

Comment on lines +201 to 209
<DeleteModal
entityTitle={contract?.name ?? ''}
message={t('message.are-you-sure-you-want-to-delete-this-entity', {
entity: t('label.contract'),
})}
open={isDeleteModalVisible}
onCancel={() => setIsDeleteModalVisible(false)}
onDelete={handleContractDeleteConfirm}
/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Edge Case: DeleteModal not passed isDeleting; confirm allows double-click

ContractTab now uses the generic DeleteModal, which exposes an optional isDeleting prop to disable/show loading on the confirm button. handleContractDeleteConfirm is async (it awaits deleteContractById) but no loading state is tracked and isDeleting is not passed to the modal. During the in-flight delete the confirm button stays enabled, so a user can click it multiple times and fire several deleteContractById calls. The previous DeleteWidgetModal managed its own in-flight state, so this is a behavioral regression introduced by the migration. Consider adding an isDeleting state, setting it around the await, and passing isDeleting={isDeleting} to DeleteModal.

Was this helpful? React with 👍 / 👎

@gitar-bot

gitar-bot Bot commented Jun 14, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 1 resolved / 2 findings

Migrates ContractDetails and ODCSModal components to use OpenMetadata core components and Tailwind styling. Consider passing the isDeleting prop to DeleteModal in ContractTab to prevent duplicate form submissions.

💡 Edge Case: DeleteModal not passed isDeleting; confirm allows double-click

📄 openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractTab/ContractTab.tsx:121-135 📄 openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractTab/ContractTab.tsx:201-209

ContractTab now uses the generic DeleteModal, which exposes an optional isDeleting prop to disable/show loading on the confirm button. handleContractDeleteConfirm is async (it awaits deleteContractById) but no loading state is tracked and isDeleting is not passed to the modal. During the in-flight delete the confirm button stays enabled, so a user can click it multiple times and fire several deleteContractById calls. The previous DeleteWidgetModal managed its own in-flight state, so this is a behavioral regression introduced by the migration. Consider adding an isDeleting state, setting it around the await, and passing isDeleting={isDeleting} to DeleteModal.

✅ 1 resolved
Edge Case: JSON contract uploads no longer accepted after migration

📄 openmetadata-ui/src/main/resources/ui/src/components/DataContract/ODCSImportModal/ODCSImportModal.component.tsx:1159-1167
The old implementation accepted .yaml, .yml and .json files for drag-and-drop import (validExtensions = ['.yaml', '.yml', '.json']). The new FileUploadDropZone is configured with accept=".yaml,.yml" and the library filters files against accept before calling onDropFiles, so JSON files are now silently rejected. If JSON contract import was a supported workflow, this is a functional regression. If dropping JSON support is intentional (the hint text now says "supports-yaml-format"), this can be ignored — otherwise add .json back to accept.

🤖 Prompt for agents
Code Review: Migrates ContractDetails and ODCSModal components to use OpenMetadata core components and Tailwind styling. Consider passing the `isDeleting` prop to `DeleteModal` in `ContractTab` to prevent duplicate form submissions.

1. 💡 Edge Case: DeleteModal not passed isDeleting; confirm allows double-click
   Files: openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractTab/ContractTab.tsx:121-135, openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractTab/ContractTab.tsx:201-209

   `ContractTab` now uses the generic `DeleteModal`, which exposes an optional `isDeleting` prop to disable/show loading on the confirm button. `handleContractDeleteConfirm` is async (it awaits `deleteContractById`) but no loading state is tracked and `isDeleting` is not passed to the modal. During the in-flight delete the confirm button stays enabled, so a user can click it multiple times and fire several `deleteContractById` calls. The previous `DeleteWidgetModal` managed its own in-flight state, so this is a behavioral regression introduced by the migration. Consider adding an `isDeleting` state, setting it around the await, and passing `isDeleting={isDeleting}` to `DeleteModal`.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@github-actions

Copy link
Copy Markdown
Contributor

❌ UI Checkstyle Failed

❌ Playwright — ESLint + Prettier + Organise Imports

One or more Playwright test files have linting or formatting issues.

Affected files
  • openmetadata-ui/src/main/resources/ui/playwright/utils/dataContracts.ts

Fix locally (fast — only checks files changed in this branch):

make ui-checkstyle-changed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs skip-pr-checks Bypass PR metadata validation check

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant