Fixes 30711: Data Contracts created via the UI are always set to Approved, bypassing Draft/review workflow - #31843
Draft
TeddyCr wants to merge 5 commits into
Draft
Fixes 30711: Data Contracts created via the UI are always set to Approved, bypassing Draft/review workflow#31843TeddyCr wants to merge 5 commits into
TeddyCr wants to merge 5 commits into
Conversation
…ck the status Contracts created through the UI wizard were always persisted as Approved: AddDataContract spread `entityStatus: EntityStatus.Approved` after `...formValues`, so nothing could override it, and the wizard exposed no status control at all. New contracts therefore skipped the Draft/review workflow entirely. Removing the literal alone is not enough. The schema's `"default": "Draft"` is inert -- jsonschema2pojo ignores a `default` sibling to `$ref` and resolves the referenced type/status.json default of `Unprocessed` instead -- and DataContractRepository never sets the status itself. The client has to send it. Add an `entityStatus` select to the contract detail tab and send the author's choice, falling back to Draft. Only Draft / In Review / Approved are offered: Rejected is an outcome a reviewer produces by closing the approval task, Archived and Deprecated are end-of-life states, and Unprocessed is an internal sentinel -- none is a state somebody authors a contract into. Keeping Approved on the list means teams that relied on the old auto-approve are not regressed, it is simply no longer forced. The edit path is untouched: it still patches via compare() from filteredContract, so an untouched status emits no patch op and existing statuses are preserved. No new locale keys -- label.status/draft/in-review/approved already exist. Fixes open-metadata#30711 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was no Playwright coverage of entityStatus for contracts at all -- the existing contract-status-card-item-* locators are validation status, not the entity status. Assert the persisted value on the POST /api/v1/dataContracts response body plus the rendered header badge, for both the untouched default (Draft) and an explicitly picked status (Approved). Asserting the response rather than the select proves persistence rather than echoing back the control just set. The status label shares data-testid="contract-status-label" with the owners label in ContractDetail, hence the text filter before walking up to the badge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ys success ContractDetail hardcoded color="success" on the status pill. That was invisible while every UI-created contract was Approved, but defaulting new contracts to Draft makes the header render a green success pill reading "Draft" -- verified in a browser, the Draft badge carried tw:bg-utility-success-50. So this is a regression introduced by the Draft default, not pre-existing. Add EntityStatusBadgeColor / getEntityStatusBadgeColor next to the existing EntityStatusClass / getEntityStatusClass. The existing helper could not be reused as-is: it returns the legacy StatusType for the Antd StatusBadge, while this pill is the UntitledUI BadgeWithIcon which takes BadgeColors. The new map mirrors the legacy palette from status-badge.less rather than inventing a second one, so a status looks the same in both stacks -- approved green, draft/unprocessed yellow, in-review purple, rejected red, deprecated/archived grey. A missing status now gets neutral grey rather than a confident green. It takes the enum's string values rather than the enum itself because quicktype re-emits EntityStatus per entity with no shared module and TS string enums are nominal, so the dataContract copy could not otherwise be passed. Also rename the Owners label's data-testid, which was a duplicate of contract-status-label, and give the status row its own contract-status-card testid mirroring contract-owner-card. Nothing referenced the duplicate, and it lets the E2E assertions drop an xpath walk coupled to Typography's internal wrapper that would have silently widened to the whole header row. Fixes open-metadata#30711 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s patches The edit path never emitted an /entityStatus op, but nothing asserted it: the existing "should call updateContract for existing contract with JSON patch" checks only expect.any(Array), so it could not have caught a regression there. Assert the contents of the real patch array instead, in both directions -- no /entityStatus op when only an unrelated field changed (mockContract is Approved, so a spurious op would silently rewrite a real status), and a correct replace -> In Review when the author does change it. Proven non-vacuous by injecting the default into the edit branch's patch target, which produces replace /entityStatus -> Draft and fails the guard. Also compare the rendered status options against the authoring list itself rather than literal i18n keys, so renaming a key cannot fail that test for a non-behavioural reason; which statuses the list may contain is still asserted against EntityStatus values in ContractDetailFormTab.test.tsx. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… a noise useMemo
getEntityStatusBadgeColor('') returned '' rather than 'gray': an empty string
short-circuits && and is not nullish, so ?? never fired, and BadgeWithIcon would
have thrown on it. Unreachable through the type or the Java enum today, but the
helper is exported and shared, so make it total for any falsy input.
Drop the useMemo around entityStatusOptions. It maps a 3-element constant into
`fields`, which is itself rebuilt unmemoized on every render, so the memo bought
nothing and only added allocations -- frontend-performance.md calls that noise.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Comment on lines
+88
to
+101
| { | ||
| label: t('label.status'), | ||
| id: 'entityStatus', | ||
| name: 'entityStatus', | ||
| type: FieldTypes.SELECT, | ||
| required: false, | ||
| props: { | ||
| 'data-testid': 'contract-entity-status', | ||
| options: entityStatusOptions, | ||
| placeholder: t('label.please-select-entity', { | ||
| entity: t('label.status'), | ||
| }), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
💡 Edge Case: Edit form can't represent statuses outside the 3 authoring options
The shared status select in ContractDetailFormTab is seeded via form.setFieldsValue with initialValues?.entityStatus (any of the 7 EntityStatus values), but its dropdown only offers Draft/InReview/Approved. When editing a contract whose current status is Rejected, Deprecated, Archived, or Unprocessed, the Ant Select shows the raw value with no matching option, and once the author opens the dropdown they cannot re-select the original state — there is no option representing it. Saving without touching the field is safe (compare emits no /entityStatus op), but any interaction forces the status into one of the three authoring values. Consider disabling/hiding the status control when the contract's current status is not in DATA_CONTRACT_AUTHORING_STATUS_OPTIONS, or appending the current status as a read-only option so it remains representable.
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsWell-structured data contract creation and status workflow update with comprehensive unit and E2E test coverage. Consider handling statuses outside the three authoring options when rendering the edit form. 💡 Edge Case: Edit form can't represent statuses outside the 3 authoring options📄 openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractDetailFormTab/ContractDetailFormTab.tsx:88-101 📄 openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractDetailFormTab/ContractDetailFormTab.tsx:115-120 📄 openmetadata-ui/src/main/resources/ui/src/constants/DataContract.constants.ts:35-42 The shared status select in ContractDetailFormTab is seeded via form.setFieldsValue with initialValues?.entityStatus (any of the 7 EntityStatus values), but its dropdown only offers Draft/InReview/Approved. When editing a contract whose current status is Rejected, Deprecated, Archived, or Unprocessed, the Ant Select shows the raw value with no matching option, and once the author opens the dropdown they cannot re-select the original state — there is no option representing it. Saving without touching the field is safe (compare emits no /entityStatus op), but any interaction forces the status into one of the three authoring values. Consider disabling/hiding the status control when the contract's current status is not in DATA_CONTRACT_AUTHORING_STATUS_OPTIONS, or appending the current status as a read-only option so it remains representable. 🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Contributor
|
| Count | Rule |
|---|---|
| 9 | react-hooks/exhaustive-deps |
| 9 | @typescript-eslint/no-explicit-any |
| 2 | sonarjs/cyclomatic-complexity |
| 2 | @typescript-eslint/no-non-null-assertion |
| 2 | sonarjs/no-duplicate-string |
| 1 | jsx-a11y/label-has-for |
| 1 | jsx-a11y/control-has-associated-label |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/DataContract/AddDataContract/AddDataContract.tsx:90:39 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 13 which is greater than 10 authorized.","cost":3,"secondaryLocations":[{"line":90,"column":38,"endLine":90,"endColumn" |
| 🟡 | src/components/DataContract/AddDataContract/AddDataContract.tsx:278:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'onSave' and 't'. Either include them or remove the dependency array. If 'onSave' changes too often, find the p |
| 🟡 | src/components/DataContract/AddDataContract/AddDataContract.tsx:493:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 't'. Either include it or remove the dependency array. |
| 🟡 | src/components/DataContract/AddDataContract/AddDataContract.tsx:536:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 't'. Either include it or remove the dependency array. |
| 🟡 | src/components/DataContract/ContractDetailFormTab/ContractDetailFormTab.test.tsx:25:24 |
@typescript-eslint/no-explicit-any |
Unexpected any. Specify a different type. |
| 🟡 | src/components/DataContract/ContractDetailFormTab/ContractDetailFormTab.test.tsx:27:9 |
jsx-a11y/label-has-for |
Form label must have ALL of the following types of associated control: nesting, id |
| 🟡 | src/components/DataContract/ContractDetailFormTab/ContractDetailFormTab.test.tsx:28:9 |
jsx-a11y/control-has-associated-label |
A control must be associated with a text label. |
| 🟡 | src/components/DataContract/ContractDetailFormTab/ContractDetailFormTab.tsx:129:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'form'. Either include it or remove the dependency array. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.test.tsx:267:45 |
@typescript-eslint/no-explicit-any |
Unexpected any. Specify a different type. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.test.tsx:273:60 |
@typescript-eslint/no-explicit-any |
Unexpected any. Specify a different type. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.test.tsx:283:60 |
@typescript-eslint/no-explicit-any |
Unexpected any. Specify a different type. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.test.tsx:297:58 |
@typescript-eslint/no-explicit-any |
Unexpected any. Specify a different type. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.test.tsx:307:67 |
@typescript-eslint/no-explicit-any |
Unexpected any. Specify a different type. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.test.tsx:356:50 |
@typescript-eslint/no-explicit-any |
Unexpected any. Specify a different type. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.test.tsx:388:54 |
@typescript-eslint/no-explicit-any |
Unexpected any. Specify a different type. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.test.tsx:393:33 |
@typescript-eslint/no-explicit-any |
Unexpected any. Specify a different type. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.test.tsx:1411:18 |
@typescript-eslint/no-non-null-assertion |
Forbidden non-null assertion. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.test.tsx:1441:18 |
@typescript-eslint/no-non-null-assertion |
Forbidden non-null assertion. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.tsx:121:4 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 16 which is greater than 10 authorized.","cost":6,"secondaryLocations":[{"line":121,"column":3,"endLine":121,"endColumn |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.tsx:175:21 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 4 times. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.tsx:181:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 't'. Either include it or remove the dependency array. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.tsx:199:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 't'. Either include it or remove the dependency array. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.tsx:215:9 |
react-hooks/exhaustive-deps |
The 'handleRunNow' function makes the dependencies of useCallback Hook (at line 282) change on every render. To fix this, wrap the definition of 'handleRunNow' |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.tsx:331:28 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 3 times. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.tsx:540:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has unnecessary dependencies: 'handleRunNow' and 'validateLoading'. Either exclude them or remove the dependency array. |
| 🟡 | src/components/DataContract/ContractDetailTab/ContractDetail.tsx:556:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'fetchLatestContractResults'. Either include it or remove the dependency array. |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes:
Fixes #30711
Every data contract created through the UI wizard was persisted as
Approved, so contracts skipped the Draft/review workflow entirely.AddDataContract.handleSavehardcoded the status in the create payload and spread it after...formValues, so nothing could override it:Compounding it, there was no status control anywhere in the UI —
ContractDetailFormTabexposed onlyname,owners, anddescription. This PR adds a status select to the contract detail tab and sends the author's choice explicitly, defaulting toDraft.Type of change:
High-level design:
Why the status is sent explicitly instead of just deleting the literal. This is the non-obvious part. The JSON Schema looks like it already defaults to Draft —
dataContract.jsonandcreateDataContract.jsonboth declare"default": "Draft"— but that default is inert. It sits beside a$ref, and jsonschema2pojo ignores adefaultsibling to$ref, resolving the referencedtype/status.jsondefault ofUnprocessedinstead. The generatedCreateDataContract.javabears this out (EntityStatus.fromValue("Unprocessed")), and the repo's ownDataContractResourceIT.testDataContractDefaultEntityStatusassertsUNPROCESSED.DataContractRepository.prepare()never sets the field and does not overridesetDefaultStatus.So simply removing the hardcoded literal would have shipped every new contract as
Unprocessed— arguably worse than the bug. The client has to send the status. I deliberately did not "fix" the schema default: correcting it means changingtype/status.json, which would shift the default for every status-bearing entity in the product.Which statuses are offered.
Draft(default),In Review,Approved. Excluded:Rejected— an outcome a reviewer produces by closing the approval task (DataContractRepository.postUpdate), not something you author into.Archived/Deprecated— end-of-life states, meaningless for a contract being created.Unprocessed— an internal sentinel that only leaks from thetype/status.jsondefault; it has no product meaning.Keeping
Approvedon the list is deliberate: teams that rely on today's auto-approve behaviour can still get it in one click, it is simply no longer forced.A regression this PR introduced, and fixed.
ContractDetail.tsxhardcodedcolor="success"on the status pill. That was always correct while every UI-created contract wasApproved— but defaulting toDraftmakes the header render a green "success" pill reading "Draft" (verified in a browser: the Draft badge carriedtw:bg-utility-success-50). Fixed by mapping status to colour.The new
getEntityStatusBadgeColorsits beside the existinggetEntityStatusClassrather than replacing it. They cannot be unified cheaply: the existing helper returns the legacyStatusTypeconsumed by the Ant DesignStatusBadge, whereas this pill is the UntitledUIBadgeWithIcon, which takesBadgeColors. Unifying would mean either changingStatusBadge's contract — blast radiusGlossaryTermTab,EntityStatusBadge,ChangeParentHierarchy, and lineage — or adapting through a 15-member enum of which 8 are irrelevant here. The new map mirrors the legacy palette instatus-badge.lessso a status looks the same in both stacks (approved green, draft/unprocessed yellow, in-review purple, rejected red, deprecated/archived grey). Both maps areRecord<EntityStatus, …>, so TypeScript enforces exhaustiveness on each — the drift guard that actually matters.It takes the enum's string values (
`${EntityStatus}`) rather than the enum itself, because quicktype re-emitsEntityStatusper entity with no shared module and TS string enums are nominal, so a caller holding thedataContractcopy could not otherwise pass it.Files changed
constants/DataContract.constants.tsDEFAULT_DATA_CONTRACT_STATUS+DATA_CONTRACT_AUTHORING_STATUS_OPTIONS— one source of truth for both the form and the payloadContractDetailFormTab.tsxentityStatusselect, seededinitialValues?.entityStatus ?? DraftAddDataContract.tsxentityStatus: formValues.entityStatus ?? DEFAULT_DATA_CONTRACT_STATUSutils/EntityStatusUtils.tsEntityStatusBadgeColor/getEntityStatusBadgeColorContractDetailTab/ContractDetail.tsxdata-testidScope notes. The edit path is untouched — it still patches via
compare()fromfilteredContract, so an untouched status emits no/entityStatusop. The field is shared by create and edit, so edit gains a status control seeded from the contract's real status; that is intentional, since otherwise a Draft contract would have no UI path to Approved. Backend authorization is unchanged:EntityRepository.updateEntityStatusandcheckUpdatedByReviewerstill gate IN_REVIEW→APPROVED/REJECTED to reviewers. No schema change, no migration, nomake generate.Tests:
Use cases covered
Draft, notApprovedIn RevieworApprovedat creation and that choice is what persistsDraft/In Review/Approvedare offered;Rejected/Archived/Deprecated/Unprocessedare notUnit tests
src/components/DataContract/AddDataContract/AddDataContract.test.tsx(updated + 4 new)src/components/DataContract/ContractDetailFormTab/ContractDetailFormTab.test.tsx(2 new)src/components/DataContract/ContractDetailFormTab/ContractDetailFormTab.entityStatus.test.tsx(new file, 4 tests)src/components/DataContract/ContractDetailTab/ContractDetail.test.tsx(7 new)jest --coverage, % lines):ContractDetailFormTab.tsx— 100%ContractDetail.tsx— 97.1%EntityStatusUtils.ts— 90.9% (the only uncovered line is the pre-existingisDeleted; the code added here is fully covered)AddDataContract.tsx— 89.91%Notes on what the tests actually assert, since it matters here:
AddDataContracttests assert the object handed tocreateContract(the HTTP boundary), which is exactly what the bug corrupted.ContractDetailFormTab.entityStatus.test.tsxis a separate new file specifically so it can render the real Ant Design control; the pre-existing sibling stubsgenerateFormFieldsmodule-wide and cannot.should not patch entityStatus when editing without touching the statusinspects the real JSON-patch array, notexpect.any(Array). Verified non-vacuous by injecting the regression, which produces{"op":"replace","path":"/entityStatus","value":"Draft"}and fails the guard.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataContracts.spec.ts(2 new tests)There was previously zero Playwright coverage of
entityStatusfor contracts — the existingcontract-status-card-item-*locators are validation status, not entity status. The new tests assert thePOST /api/v1/dataContractsresponse body, not the form control that was just set, so they prove persistence rather than echoing input. They also assert the rendered badge's palette class.Manual testing performed
Draft.Draft,In Review,Approved— and thatRejected,Archived,Deprecated,Unprocessedare absent.POST /api/v1/dataContractsresponse body contains"entityStatus": "Draft".Status : Draftand the pill is yellow, not green (tw:bg-utility-warning-50)."entityStatus": "Approved"and the pill is green.entityStatusunchanged (no/entityStatusop in the PATCH body).UI screen recording / screenshots:
What the recording should capture, in one continuous take:
POST /api/v1/dataContractsresponse body visible showing"entityStatus": "Draft";Status : Draftwith the yellow pill visible;Status : Approvedheader and green pill.The Network panel is the load-bearing frame — the badge alone does not prove what was persisted. The two pills side by side are what demonstrate the colour fix.
Known gaps (disclosed, not fixed here):
ODCSImportModal.component.tsxalso callscreateContract, forwarding the parsed document verbatim with noentityStatus. So "UI-created contracts default to Draft" is true of the wizard only: importing an ODCS document that omits the field still lands asUnprocessed, while the wizard givesDraft. This is pre-existing and outside this issue's "creation wizard" framing, so it is deliberately left alone — a natural follow-up, and the fix would be the same one-line default. The badge-colour fix in this PR does already make that state render correctly (yellow) instead of green.dataContract, andDataContractRepositoryonly ever closes approval tasks (they are created byCreateApprovalTaskImpl, which no dataContract workflow invokes). This is a pre-existing product gap rather than a regression, and it is not a hard trap —checkUpdatedByRevieweronly restricts the transition whenreviewersis non-empty — but the new dropdown does set an expectation the product does not yet fully meet.Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.🤖 Generated with Claude Code