feat: show proposed changes with clickable entity links in approval tasks - #29627
Conversation
…asks Re-implements PR #27201 for the new Task entity system (#25894 Task Redesign). The proposed-changes preview was originally written into thread.message; that storage and the legacy CreateApprovalTaskImpl no longer exist. ## What this adds Governance approval task panels now show a "Proposed Changes" card listing exactly which fields changed on the entity (tags, description, synonyms, owners, etc.) before the approver acts. Each field renders as a red chip for the removed value and a green chip for the added value. Tags, glossary terms, related terms, and domains chips are clickable links to their detail page. ## Storage The change-preview map is written into the new Task entity's existing `payload` field under a namespaced `proposedChanges` key, so it coexists with any future workflow payload data (e.g. RecognizerFeedback's `feedback` key, DAR's expiration metadata): ```json { "proposedChanges": { "tags": {"added": ["PII.Sensitive"], "removed": ["PII.None"]}, "description": {"added": ["<p>new</p>"], "removed": ["<p>old</p>"]} } } ``` No new schema fields, no migrations. ## Merge across re-edits (set-cancellation) Each entity edit spawns a fresh workflow instance, which creates a new Task and asynchronously closes the prior one via supersedePriorApprovalTask. To carry forward the accumulated diff, CreateTask now looks up the prior open approval task for the same (entity FQN, workflow definition) BEFORE create and uses its payload as the merge base. The set-cancellation merge produces the net delta from the original entity state: ``` mergedAdded = (oldAdded - newRemoved) ∪ (newAdded - oldRemoved) mergedRemoved = (oldRemoved - newAdded) ∪ (newRemoved - oldAdded) ``` Fields whose added and removed both cancel out are dropped from the preview. ## Per-field accumulator in buildChangeMap For a tag swap (remove BankNumber, add BirthDate) the ChangeDescription emits a fieldsAdded entry AND a fieldsDeleted entry both named `tags`. The previous put-style implementation silently overwrote one side. The new accumulate() helper merges multiple FieldChange entries for the same field name using FieldDiff.merge so both the addition and removal survive. ## Backend changes - `ChangePreviewUtils.java` - `PROPOSED_CHANGES_KEY` constant. - `extractProposedChanges(payload)`, `buildProposedChangesPayload(entity, existingPayload)`. - `accumulate(...)` helper called from `buildChangeMap` for fieldsAdded / fieldsDeleted / fieldsUpdated. - `[ChangePreview]` diagnostic log dumping ChangeDescription, prior map, new map, and merged result for production debugging. - `CreateTask.java` - `findPriorOpenApprovalPayload(...)` looks up the existing open approval task on the same entity bound to a different workflow instance of the same workflow definition. - `applyProposedChangesIfApproval(taskType, entity, payload)` invoked on both the update and create paths for GlossaryApproval and RequestApproval task types. - Create-path payload chain: requestedPayload → workflow payload → prior approval payload → null. ## UI changes - `TaskTabNew.component.tsx` - `extractProposedChanges(task.payload)` parses payload.proposedChanges into a normalized `{added, removed}` map. - Renders the "Proposed Changes" card directly after taskHeader when isTaskApprovalRequest is true and the parsed map is non-empty. - `FIELD_ROUTE_MAP` routes tags / tier / glossaryTerms / relatedTerms / domains chips through their canonical RouterUtils paths. ## Tests - `ChangePreviewUtilsTest` adds coverage for `extractProposedChanges`, `buildProposedChangesPayload` (fresh, merge, cancel-to-empty), and the same-field-in-added-and-deleted accumulator case (39 total tests pass).
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
yan-3005
added
the
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Downgrade [ChangePreview] diagnostic logs from INFO to DEBUG and guard with isDebugEnabled(). Avoids logging raw governance content on every approval task create/update at INFO level. - Strip HTML tags from non-route proposed-change chip values (e.g. description) so chips show readable plain text instead of literal "<p>old</p>" / "<p>new</p>" markup. Route-linked fields (tags, glossary terms, domains) keep their raw FQN.
Jest test CoverageUI tests summary
|
…ound-trip Prior task payload read back from the DB carries `proposedChanges` as a raw `Map<String, Map<String, List<String>>>` — not a `Map<String, FieldDiff>`. `extractProposedChanges` was calling `JsonUtils.convertValue` against `TypeReference<Map<String, FieldDiff>>`; when that throws on Jackson record materialisation it silently returns an empty map, so each new approval task starts with no prior context and the cross-edit merge is lost (e.g. an Article tag added in one edit and removed in the next stays as "added"). Replace `convertValue` with a manual two-shape coercer that accepts either the in-memory `FieldDiff` record (freshly built) or a raw map (post round-trip), and add unit tests for both shapes.
…-changes merge When a glossary term is edited multiple times within an approval session, each edit creates a new approval task. The prior task's payload already folds in every in-session change up to that point. If a later edit causes a version bump (e.g. v0.3 -> v0.4), the entity's cumulative changeDescription reports the net diff between the two committed versions - which may double-count fields the prior payload has already cancelled. Concrete repro from the field: - v0.3 committed with tag PII.None. - User added KnowledgeCenter.Article in-session (prior payload: tags added=[Article, PII.None]). - User removed PII.None in-session (prior payload: tags added=[Article]; PII.None cancelled). - User removed Article -> v0.3 to v0.4 version bump, entity.changeDescription reports tags fieldsDeleted=[PII.None]. - Old behaviour: merge ([Article] - []) U ([] - []) added and ([] - []) U ([PII.None] - [Article]) removed -> tags added=[Article], removed=[PII.None]. Both chips appear, neither matches the user's net intent. - Correct behaviour: tag is empty - the term ended this session with no tags, same as it started. Fix: read entity.getIncrementalChangeDescription() (the just-this-hop diff) when present, falling back to entity.getChangeDescription() only when the incremental form is empty/null. The incremental form is the right unit of work to fold into a running merge: it always reflects the single user action that triggered this CreateTask, never re-asserts already-cancelled state from earlier hops. Adds a regression test that fakes the v0.3 -> v0.4 scenario and asserts the tags entry is dropped from the merged payload.
| {Object.entries(proposedChanges).map( | ||
| ([field, { added, removed }]) => { | ||
| const getUrl = FIELD_ROUTE_MAP[field]; | ||
|
|
||
| return ( | ||
| <div | ||
| className="task-proposed-changes-field-row" | ||
| key={field}> | ||
| <Typography.Text className="task-proposed-changes-field-name"> | ||
| {startCase(field)} | ||
| </Typography.Text> |
|
|
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
Summary
Re-implements PR #27201 for the new Task entity system (#25894 Task Redesign).
The proposed-changes preview was originally written into
thread.message;that storage and the legacy
CreateApprovalTaskImplwere both removed bythe Task Redesign. This PR ports the feature to the new
Taskentity'sgeneric
payloadfield, fixes the merge-across-re-edits behaviour againstthe new supersession flow, and fixes a per-field overwrite bug in
buildChangeMap.Closes the gap left when #27201 was rolled out of
mainto avoidconflicts with the Task Redesign.
Fixes #27440
Fixes #29633
Storage
The change-preview map lives in the new
Task.payloadfield under anamespaced
proposedChangeskey, so it coexists with other workflowpayload data (RecognizerFeedback's
feedback, DAR's expiration metadata,etc.):
{ \"proposedChanges\": { \"tags\": {\"added\": [\"PII.Sensitive\"], \"removed\": [\"PII.None\"]}, \"description\": {\"added\": [\"<p>new</p>\"], \"removed\": [\"<p>old</p>\"]} } }No new schema fields, no migrations.
Identifier extraction priority for each field value:
Merge across re-edits (set-cancellation)
Each entity edit spawns a fresh workflow instance which creates a new
Task and asynchronously closes the prior one via
`supersedePriorApprovalTask`. To carry forward the accumulated diff,
`CreateTask` now looks up the prior open approval task for the same
(entity FQN, workflow definition, different workflow instance) before
creating the new Task and uses its payload as the merge base. The
set-cancellation merge produces the net delta from the original entity
state:
Fields whose `added` and `removed` both cancel out are dropped from the
preview.
Example across 3 edits on a glossary term:
Per-field accumulator in `buildChangeMap`
For a tag swap (remove `BankNumber`, add `BirthDate`) the
`ChangeDescription` emits a `fieldsAdded` entry and a
`fieldsDeleted` entry both named `tags`. The previous `put`-style
implementation silently overwrote one side, leaving the proposed-changes
card showing only half the diff. The new `accumulate(...)` helper merges
multiple `FieldChange` entries for the same field name through
`FieldDiff.merge`, so both the addition and the removal survive.
Backend changes
`openmetadata-service/.../governance/workflows/util/ChangePreviewUtils.java`
fieldsAdded / fieldsDeleted / fieldsUpdated.
prior map, new map, and merged result — kept on this PR to make
production debugging tractable; can be downgraded post-merge.
`openmetadata-service/.../governance/workflows/elements/nodes/userTask/CreateTask.java`
Task on the same entity bound to a different workflow instance of the
same workflow definition.
both the update and create paths for `GlossaryApproval` and
`RequestApproval` task types.
approval payload → null`.
UI changes
`openmetadata-ui/.../components/Entity/Task/TaskTab/TaskTabNew.component.tsx`
`payload.proposedChanges` into a normalized `Record<field, {added,
removed}>` map.
`isTaskApprovalRequest` is true and the parsed map is non-empty.
`relatedTerms` / `domains` chips through their canonical
`RouterUtils` paths.
`task.payload` directly.
The chip styles (`.task-proposed-changes-*`) and the
`label.proposed-change-plural` i18n key were already present on `main`
from PR #27201's earlier landing, so no `.less` or locale changes were
required.
Why not use `feedInfo.entitySpecificInfo` (rich card approach)?
The activity-feed formatter pipeline already produces per-field rich card
data via `feedInfo.entitySpecificInfo` — but `entitySpecificInfo` is a
single polymorphic object, not an array. When multiple fields change in
one approval (e.g. tags + description), the formatter returns a
`List` with one entry per field, each carrying its own
`entitySpecificInfo`. There is only one `feedInfo` slot on the task
thread, so only one field's data can be stored — the rest are lost.
Fixing this properly would require turning `feedInfo.entitySpecificInfo`
into an array in the spec, plus a data migration over every existing
Task row. The Task Redesign has just landed (#25894) and we don't want to
follow it immediately with another schema-shaped change. `task.payload`
is already generic `Map<String, Object>` and is the natural slot for
this.
Why not store inside `thread.message` like the original PR?
`CreateApprovalTaskImpl` (Thread-based) was deleted by #25894. Approval
tasks no longer create `Thread` rows — they create new `Task` entities
via `CreateTask`. The original storage location no longer exists.
Test plan
→ open approval task → "Proposed Changes" shows the tag with a green
chip for added, clickable link to the tag page.
as separate rows.
merged changes across all edits (exercises set-cancellation across
supersession).
list (net zero, disappears from preview).
(regression for the `buildChangeMap` overwrite bug).
→ "Proposed Changes" section is absent.
`RequestTag`, RecognizerFeedback, DAR).
Greptile Summary
This PR ports the "Proposed Changes" preview feature for approval tasks from the removed Thread-based storage to the new
Task.payloadgeneric field, adapting it to the Task Redesign (#25894). It fixes a per-field overwrite bug inbuildChangeMap(tag swap showing only half the diff) and implements set-cancellation merge logic to carry accumulated diffs correctly across re-edits that spawn fresh workflow runs.ChangePreviewUtilsgainsbuildProposedChangesPayload,extractProposedChanges, andpreserveProposedChanges;CreateTaskusesfindPriorOpenApprovalPayload+preserveProposedChangeson both create and update paths to prevent accumulatedproposedChangesfrom being silently discarded whenrequestedPayloadreplaces the task payload.TaskTabNewrenders a Proposed Changes card with clickable entity links (tags, glossary terms, domains) after the task header, scoped toisTaskApprovalRequesttasks viauseMemo.ChangePreviewUtilsTestcover accumulation, JSON round-trips, cancellation-to-empty, incremental change description preference, andpreserveProposedChangesedge cases.Confidence Score: 5/5
Safe to merge — the payload-overwrite issues flagged in prior review rounds are addressed by the new preserveProposedChanges helper, and the per-field overwrite in buildChangeMap is fixed by the accumulate helper.
The two previously flagged payload-accumulation issues (update path and create path) are both resolved cleanly. The buildChangeMap fix is regression-tested by the new accumulates-both-sides test. The incremental-vs-cumulative changeDescription preference is tested and prevents double-counting across re-edits. No schema changes or migrations are needed.
No files require special attention.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant E as Entity Edit (PATCH) participant CT as CreateTask.java participant CPR as ChangePreviewUtils participant TR as TaskRepository participant UI as TaskTabNew (UI) E->>CT: delegate task triggered (new workflow run) CT->>TR: findExistingTask(requestedTaskId) TR-->>CT: "existingTask = null (new task)" CT->>TR: listNonTerminalTasksByEntityAndCategory TR-->>CT: priorApprovalPayload (accumulated proposedChanges) CT->>CPR: preserveProposedChanges(base, priorApprovalPayload) CPR-->>CT: payload with prior proposedChanges seeded CT->>CPR: applyProposedChangesIfApproval(taskType, entity, payload) CPR->>CPR: mergeChangeMaps(priorMap, newMap) CPR-->>CT: payload with updated proposedChanges CT->>TR: taskRepository.create(task) UI->>UI: extractProposedChanges(task.payload) UI->>UI: Render Proposed Changes card with clickable entity links%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant E as Entity Edit (PATCH) participant CT as CreateTask.java participant CPR as ChangePreviewUtils participant TR as TaskRepository participant UI as TaskTabNew (UI) E->>CT: delegate task triggered (new workflow run) CT->>TR: findExistingTask(requestedTaskId) TR-->>CT: "existingTask = null (new task)" CT->>TR: listNonTerminalTasksByEntityAndCategory TR-->>CT: priorApprovalPayload (accumulated proposedChanges) CT->>CPR: preserveProposedChanges(base, priorApprovalPayload) CPR-->>CT: payload with prior proposedChanges seeded CT->>CPR: applyProposedChangesIfApproval(taskType, entity, payload) CPR->>CPR: mergeChangeMaps(priorMap, newMap) CPR-->>CT: payload with updated proposedChanges CT->>TR: taskRepository.create(task) UI->>UI: extractProposedChanges(task.payload) UI->>UI: Render Proposed Changes card with clickable entity linksComments Outside Diff (1)
openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/util/ChangePreviewUtils.java, line 161-168 (link)parseChangeMapis now dead production code — its only caller,applyChangePreview, was removed in this PR. Only the (pre-existing) tests still call it. Consider removing both the method and the corresponding test section to keep the public API minimal, or leave it with an explicit@Deprecatednote if backward compatibility is needed.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/userTask/CreateTask.java, line 500-530 (link)proposedChangessilently overwritten on the update pathOn the update path (
existingTask != null), line 500-501 replaces the full payload withrequestedPayloadwhen it is non-null — discarding the accumulatedproposedChangesthe task already holds.applyProposedChangesIfApprovalat line 529-530 then callsextractProposedChanges(requestedPayload), which returns an empty map becauserequestedPayloadcarries noproposedChangeskey. The merge therefore starts from scratch and only the current edit's delta survives; every prior accumulation on this task instance is lost.This is a distinct issue from the create-path problem noted in the previous review: there the concern is that
priorApprovalPayload(from a superseded task) is ignored; here the concern is that the current task's own accumulatedproposedChangesare discarded wheneverrequestedPayloadis set on an update call. Any workflow definition that injectsrequestedPayloadduring a task-update step will silently reset the proposed-changes preview to only the most recent edit's delta.Reviews (10): Last reviewed commit: "Merge branch 'main' into ram/approval-ta..." | Re-trigger Greptile