feat: show proposed changes with clickable entity links in approval tasks#27201
feat: show proposed changes with clickable entity links in approval tasks#27201
Conversation
…asks Governance approval tasks now display a "Proposed Changes" preview so approvers can see exactly what changed before approving. Backend runs the entity's changeDescription through the activity-feed formatter pipeline and stores a markdown bullet list in thread.message. UI renders the preview using Showdown + dangerouslySetInnerHTML and injects clickable hyperlinks for tags, tier, glossary terms, related terms, domains, owners, reviewers, and experts directly in the frontend before conversion.
yan-3005
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds a “Proposed Changes” preview for governance approval tasks by formatting an entity’s changeDescription into activity-feed-style messages, persisting them in the task thread, and rendering them in the Task UI with injected clickable links for certain changed fields.
Changes:
- Backend: build per-field formatted change messages from
changeDescriptionand write a markdown bullet list into the approval task thread’smessage. - UI: render the proposed-changes block for approval tasks and inject clickable links into diff spans for selected fields.
- i18n/UI styling: add a new label key and style the proposed-changes section.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/userTask/impl/CreateApprovalTaskImpl.java | Builds formatted diffs from changeDescription and stores them into the task thread message for approvals. |
| openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/TaskTabNew.component.tsx | Renders “Proposed Changes” section and injects hyperlinks into diff spans in the rendered markdown/HTML. |
| openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/task-tab-new.less | Adds styling for the proposed-changes container/title. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/en-us.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/ar-sa.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/de-de.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/es-es.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/fr-fr.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/gl-es.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/he-he.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/ja-jp.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/ko-kr.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/mr-in.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/nl-nl.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/pr-pr.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/pt-br.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/pt-pt.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/ru-ru.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/th-th.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/tr-tr.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/zh-cn.json | Adds label.proposed-change-plural translation key. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/zh-tw.json | Adds label.proposed-change-plural translation key. |
| const DIFF_SPAN_RE = | ||
| /(<span[^>]*class="diff-(?:added|removed)"[^>]*>)([^<]+)(<\/span>)/g; |
There was a problem hiding this comment.
DIFF_SPAN_RE only matches spans whose class attribute is exactly "diff-added"/"diff-removed". Other diff renderers in the UI use multiple classes on the element (e.g. class="ant-typography diff-added"), which would prevent link injection from working. Consider widening the regex to match diff-added/diff-removed anywhere within the class attribute value (word-boundary match), rather than requiring an exact class value.
| { | ||
| pattern: /\*\*(owners|reviewers|experts)\*\*/, | ||
| getUrl: (fqn) => getUserPath(fqn), | ||
| }, |
There was a problem hiding this comment.
The owners|reviewers|experts link mapping uses getUserPath(fqn) for every diff value. In formatted change messages, owner values are derived from EntityReference.displayName (not necessarily the username), and owners can also be teams; both cases will generate incorrect URLs (e.g. team owners should use the team details path). Consider either (1) emitting explicit entity links in the backend formatter output for these fields so the UI can render correct links, or (2) only linking when you can reliably disambiguate user vs team and have a stable identifier (name/FQN), otherwise skip link injection for these fields.
| void applyChangePreview( | ||
| Thread taskThread, EntityInterface entity, MessageParser.EntityLink about) { | ||
| final List<Thread> perFieldThreads = buildFormattedDiffs(entity, about); | ||
| taskThread.withCardStyle(null).withFieldOperation(null).withFeedInfo(null); | ||
| if (perFieldThreads.isEmpty()) { | ||
| taskThread.withMessage(null); | ||
| return; | ||
| } | ||
| final StringBuilder builder = new StringBuilder(); | ||
| for (final Thread perField : perFieldThreads) { | ||
| builder.append("- ").append(perField.getMessage()).append('\n'); | ||
| } | ||
| taskThread.withMessage(builder.toString()); |
There was a problem hiding this comment.
applyChangePreview overwrites thread.message with a markdown bullet list containing raw HTML diff spans (e.g. <span class="diff-added">…</span>). Other UI surfaces render task.message as plain text (e.g., notifications for glossary approval tasks), which will now show escaped HTML / list markers and read poorly. Consider keeping thread.message as a plain-text summary and storing the preview elsewhere (e.g. an existing task field), or updating all consumers of thread.message for approval tasks to render it as markdown/HTML consistently.
|
Re: Copilot comment on
Copilot's concern is valid as a future risk — if task assignment email notifications are wired up later, they'd inherit the raw HTML diff spans from |
Jest test CoverageUI tests summary
|
🟡 Playwright Results — all passed (36 flaky)✅ 3645 passed · ❌ 0 failed · 🟡 36 flaky · ⏭️ 89 skipped
🟡 36 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
| const addEntityLinks = (message: string): string => | ||
| message | ||
| .split('\n') | ||
| .map((line) => { | ||
| const matcher = FIELD_LINK_MAP.find((m) => m.pattern.test(line)); | ||
| if (!matcher) { | ||
| return line; | ||
| } | ||
|
|
||
| return line.replace( | ||
| DIFF_SPAN_RE, | ||
| (_, openTag: string, fqn: string, closeTag: string) => | ||
| `${openTag}<a href="${matcher.getUrl(fqn.trim())}">${escapeHtml( | ||
| fqn | ||
| )}</a>${closeTag}` | ||
| ); | ||
| }) | ||
| .join('\n'); |
There was a problem hiding this comment.
addEntityLinks wraps the entire text inside each diff-added/diff-removed span with a single link. The backend formatter serializes array values as a comma-separated string (e.g., multiple tags / glossary terms become "tagA, tagB"), so the generated href will point to an encoded combined string that won’t resolve to a real entity page. Consider splitting the span text on commas (preserving separators/whitespace) and linkifying each individual value so multi-value changes produce valid links.
…om/open-metadata/OpenMetadata into ram/approval-task-proposed-changes
…lback to task.message for non-JSON
| @Test | ||
| void extractIdentifiers_arrayOfStrings_returnsAll() { | ||
| String json = "[\"one\",\"two\"]"; | ||
| assertEquals(List.of("one", "two"), ChangePreviewUtils.extractIdentifiers(json)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Unit tests for extractIdentifiers() only cover JSON-in-a-string cases, but production FieldChange values are commonly List/Map objects after deserialization. Add test cases for list/map inputs (e.g. List<Map<String,Object>> for tags/owners and Map<String,Object> for single refs) to prevent regressions once the extractor is updated.
| @Test | |
| void extractIdentifiers_arrayOfStrings_returnsAll() { | |
| String json = "[\"one\",\"two\"]"; | |
| assertEquals(List.of("one", "two"), ChangePreviewUtils.extractIdentifiers(json)); | |
| } | |
| @Test | |
| void extractIdentifiers_arrayOfTagMaps_returnsAllTagFqns() { | |
| List<Map<String, Object>> tags = | |
| List.of( | |
| Map.of("tagFQN", "PII.Sensitive", "name", "Sensitive"), | |
| Map.of("tagFQN", "PersonalData.Personal", "name", "Personal")); | |
| assertEquals( | |
| List.of("PII.Sensitive", "PersonalData.Personal"), | |
| ChangePreviewUtils.extractIdentifiers(tags)); | |
| } | |
| @Test | |
| void extractIdentifiers_arrayOfOwnerMaps_returnsDisplayNames() { | |
| List<Map<String, Object>> owners = | |
| List.of( | |
| Map.of("displayName", "Aaron Johnson", "name", "aaron.johnson"), | |
| Map.of("displayName", "Jane Doe", "name", "jane.doe")); | |
| assertEquals(List.of("Aaron Johnson", "Jane Doe"), ChangePreviewUtils.extractIdentifiers(owners)); | |
| } | |
| @Test | |
| void extractIdentifiers_singleReferenceMap_returnsFullyQualifiedName() { | |
| Map<String, Object> reference = | |
| Map.of("fullyQualifiedName", "Marketing.Glossary1", "displayName", "Glossary 1"); | |
| assertEquals(List.of("Marketing.Glossary1"), ChangePreviewUtils.extractIdentifiers(reference)); | |
| } | |
| @Test | |
| void extractIdentifiers_singleNameOnlyMap_returnsName() { | |
| Map<String, Object> reference = Map.of("name", "myEntity"); | |
| assertEquals(List.of("myEntity"), ChangePreviewUtils.extractIdentifiers(reference)); | |
| } | |
| @Test | |
| void extractIdentifiers_arrayOfStrings_returnsAll() { | |
| String json = "[\"one\",\"two\"]"; | |
| assertEquals(List.of("one", "two"), ChangePreviewUtils.extractIdentifiers(json)); | |
| } | |
| @Test | |
| void extractIdentifiers_listOfStrings_returnsAll() { | |
| assertEquals(List.of("one", "two"), ChangePreviewUtils.extractIdentifiers(List.of("one", "two"))); | |
| } |
| "remove-lineage-edge": "删除血缘连线", | ||
| "rename-entity": "修改{{entity}}的名称和显示名", | ||
| "request-approval-message": "批准请求", | ||
| "request-approval-notification": "Approval required for", |
There was a problem hiding this comment.
request-approval-notification is added with an English value ("Approval required for") in this non-English locale file, which will cause mixed-language UI. Please provide a localized translation here (and in the other locale files updated in this PR) or fall back to an existing translated key if you want a shared message.
| "request-approval-notification": "Approval required for", | |
| "request-approval-notification": "需要批准", |
| const normalized: ProposedChanges = {}; | ||
| for (const [field, value] of Object.entries(parsed)) { | ||
| if (typeof value !== 'object' || value === null || Array.isArray(value)) { | ||
| continue; | ||
| } | ||
| const entry = value as Record<string, unknown>; | ||
| normalized[field] = { | ||
| added: Array.isArray(entry.added) | ||
| ? (entry.added as unknown[]).filter( | ||
| (v): v is string => typeof v === 'string' | ||
| ) | ||
| : [], | ||
| removed: Array.isArray(entry.removed) | ||
| ? (entry.removed as unknown[]).filter( | ||
| (v): v is string => typeof v === 'string' | ||
| ) | ||
| : [], | ||
| }; | ||
| } |
There was a problem hiding this comment.
parseProposedChanges builds normalized as a plain object and writes arbitrary keys from parsed JSON into it. If taskThread.message ever contains a JSON object with keys like __proto__/constructor/prototype, this can trigger prototype-pollution style issues. Use a null-prototype object (e.g. Object.create(null)) and/or explicitly skip those reserved keys when copying entries.
| @@ -1135,6 +1191,66 @@ export const TaskTabNew = ({ | |||
| </Col> | |||
| <Divider className="m-0" type="horizontal" /> | |||
| <Col span={24}>{taskHeader}</Col> | |||
| {proposedChanges !== null && ( | |||
| <Col span={24}> | |||
| <div className="task-proposed-changes"> | |||
| <Typography.Text className="task-proposed-changes-title"> | |||
| {t('label.proposed-change-plural')} | |||
| </Typography.Text> | |||
There was a problem hiding this comment.
The Proposed Changes card is rendered solely based on taskThread.message looking like JSON, regardless of task type. This can cause non-approval tasks (or any task message that happens to be JSON) to show a Proposed Changes section unexpectedly. Gate both parsing and rendering behind taskDetails?.type === TaskType.RequestApproval (or isTaskGlossaryApproval).
| try { | ||
| JsonValue json = JsonUtils.readJson(fieldValue.toString()); | ||
| if (json.getValueType() == JsonValue.ValueType.ARRAY) { | ||
| return extractFromArray(json.asJsonArray()); | ||
| } | ||
| if (json.getValueType() == JsonValue.ValueType.OBJECT) { | ||
| return extractFromObject(json.asJsonObject()); | ||
| } | ||
| } catch (Exception e) { | ||
| // not JSON — treat as a plain string | ||
| } | ||
| return List.of(fieldValue.toString().strip()); | ||
| } | ||
|
|
||
| private static List<String> extractFromArray(JsonArray array) { | ||
| List<String> result = new ArrayList<>(); | ||
| for (JsonValue item : array) { | ||
| if (item.getValueType() == JsonValue.ValueType.OBJECT) { | ||
| result.addAll(extractFromObject(item.asJsonObject())); | ||
| } else if (item.getValueType() == JsonValue.ValueType.STRING) { | ||
| result.add(((JsonString) item).getString().strip()); | ||
| } | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| private static List<String> extractFromObject(JsonObject obj) { | ||
| Set<String> keys = obj.keySet(); | ||
| if (keys.contains("tagFQN")) return List.of(obj.getString("tagFQN")); | ||
| if (keys.contains("fullyQualifiedName")) return List.of(obj.getString("fullyQualifiedName")); | ||
| if (keys.contains("displayName")) return List.of(obj.getString("displayName")); | ||
| if (keys.contains("name")) return List.of(obj.getString("name")); |
There was a problem hiding this comment.
extractIdentifiers() currently assumes fieldValue.toString() is JSON and falls back to treating the whole value as a single string. In practice, FieldChange values are often deserialized as List/Map (see existing FieldChangeValueExtractor), so this will produce incorrect identifiers like [{tagFQN=...}] or Java toString() output instead of extracting tagFQN/fullyQualifiedName/etc. Update the extractor to handle List/Map inputs directly (and/or recursively parse JSON when the value is a string) so change previews consistently contain usable identifiers.
| try { | |
| JsonValue json = JsonUtils.readJson(fieldValue.toString()); | |
| if (json.getValueType() == JsonValue.ValueType.ARRAY) { | |
| return extractFromArray(json.asJsonArray()); | |
| } | |
| if (json.getValueType() == JsonValue.ValueType.OBJECT) { | |
| return extractFromObject(json.asJsonObject()); | |
| } | |
| } catch (Exception e) { | |
| // not JSON — treat as a plain string | |
| } | |
| return List.of(fieldValue.toString().strip()); | |
| } | |
| private static List<String> extractFromArray(JsonArray array) { | |
| List<String> result = new ArrayList<>(); | |
| for (JsonValue item : array) { | |
| if (item.getValueType() == JsonValue.ValueType.OBJECT) { | |
| result.addAll(extractFromObject(item.asJsonObject())); | |
| } else if (item.getValueType() == JsonValue.ValueType.STRING) { | |
| result.add(((JsonString) item).getString().strip()); | |
| } | |
| } | |
| return result; | |
| } | |
| private static List<String> extractFromObject(JsonObject obj) { | |
| Set<String> keys = obj.keySet(); | |
| if (keys.contains("tagFQN")) return List.of(obj.getString("tagFQN")); | |
| if (keys.contains("fullyQualifiedName")) return List.of(obj.getString("fullyQualifiedName")); | |
| if (keys.contains("displayName")) return List.of(obj.getString("displayName")); | |
| if (keys.contains("name")) return List.of(obj.getString("name")); | |
| return extractIdentifiersInternal(fieldValue); | |
| } | |
| private static List<String> extractIdentifiersInternal(Object fieldValue) { | |
| if (fieldValue == null) { | |
| return List.of(); | |
| } | |
| if (fieldValue instanceof List<?> listValue) { | |
| List<String> result = new ArrayList<>(); | |
| for (Object item : listValue) { | |
| result.addAll(extractIdentifiersInternal(item)); | |
| } | |
| return result; | |
| } | |
| if (fieldValue instanceof Map<?, ?> mapValue) { | |
| return extractFromMap(mapValue); | |
| } | |
| if (fieldValue instanceof String stringValue) { | |
| String strippedValue = stringValue.strip(); | |
| if (strippedValue.isEmpty()) { | |
| return List.of(); | |
| } | |
| try { | |
| JsonValue json = JsonUtils.readJson(strippedValue); | |
| return extractFromJsonValue(json); | |
| } catch (Exception e) { | |
| return List.of(strippedValue); | |
| } | |
| } | |
| String stringValue = fieldValue.toString().strip(); | |
| return stringValue.isEmpty() ? List.of() : List.of(stringValue); | |
| } | |
| private static List<String> extractFromJsonValue(JsonValue jsonValue) { | |
| if (jsonValue == null) { | |
| return List.of(); | |
| } | |
| return switch (jsonValue.getValueType()) { | |
| case ARRAY -> extractFromArray(jsonValue.asJsonArray()); | |
| case OBJECT -> extractFromObject(jsonValue.asJsonObject()); | |
| case STRING -> extractIdentifiersInternal(((JsonString) jsonValue).getString()); | |
| default -> List.of(); | |
| }; | |
| } | |
| private static List<String> extractFromArray(JsonArray array) { | |
| List<String> result = new ArrayList<>(); | |
| for (JsonValue item : array) { | |
| result.addAll(extractFromJsonValue(item)); | |
| } | |
| return result; | |
| } | |
| private static List<String> extractFromMap(Map<?, ?> map) { | |
| List<String> preferredIdentifier = extractPreferredIdentifier(map); | |
| if (!preferredIdentifier.isEmpty()) { | |
| return preferredIdentifier; | |
| } | |
| List<String> result = new ArrayList<>(); | |
| for (Object value : map.values()) { | |
| result.addAll(extractIdentifiersInternal(value)); | |
| } | |
| return result; | |
| } | |
| private static List<String> extractFromObject(JsonObject obj) { | |
| List<String> preferredIdentifier = extractPreferredIdentifier(obj); | |
| if (!preferredIdentifier.isEmpty()) { | |
| return preferredIdentifier; | |
| } | |
| List<String> result = new ArrayList<>(); | |
| for (JsonValue value : obj.values()) { | |
| result.addAll(extractFromJsonValue(value)); | |
| } | |
| return result; | |
| } | |
| private static List<String> extractPreferredIdentifier(Map<?, ?> map) { | |
| for (String key : List.of("tagFQN", "fullyQualifiedName", "displayName", "name")) { | |
| Object value = map.get(key); | |
| if (value != null) { | |
| String identifier = value.toString().strip(); | |
| if (!identifier.isEmpty()) { | |
| return List.of(identifier); | |
| } | |
| } | |
| } | |
| return List.of(); | |
| } | |
| private static List<String> extractPreferredIdentifier(JsonObject obj) { | |
| Set<String> keys = obj.keySet(); | |
| if (keys.contains("tagFQN")) return List.of(obj.getString("tagFQN").strip()); | |
| if (keys.contains("fullyQualifiedName")) return List.of(obj.getString("fullyQualifiedName").strip()); | |
| if (keys.contains("displayName")) return List.of(obj.getString("displayName").strip()); | |
| if (keys.contains("name")) return List.of(obj.getString("name").strip()); |
…ed changes on task type, fix prototype pollution
| ? t('message.request-approval-notification') | ||
| : task.message} | ||
| </span> |
There was a problem hiding this comment.
In the RequestApproval notification text, relying on a trailing space inside the i18n string causes locales without that trailing space to render the entity link immediately adjacent (e.g., "Approval required for"). Add the separator explicitly in JSX (or include the entity name as an interpolation param) and keep the translation value free of trailing whitespace.
| ? t('message.request-approval-notification') | |
| : task.message} | |
| </span> | |
| ? t('message.request-approval-notification').trimEnd() | |
| : task.message} | |
| </span>{' '} |
| public static List<String> extractIdentifiers(Object fieldValue) { | ||
| if (nullOrEmpty(fieldValue)) return List.of(); | ||
| if (fieldValue instanceof List<?> list) { | ||
| List<String> result = new ArrayList<>(); | ||
| for (Object item : list) { | ||
| result.addAll(extractIdentifiers(item)); | ||
| } | ||
| return result; | ||
| } | ||
| if (fieldValue instanceof Map<?, ?> map) { | ||
| return extractFromMap(map); | ||
| } |
There was a problem hiding this comment.
ChangePreviewUtils.extractIdentifiers starts with nullOrEmpty(fieldValue), but because fieldValue is typed as Object, it binds to CommonUtil.nullOrEmpty(Object) (which checks object.toString().isEmpty()), not the Map/Collection overloads. As a result, an empty map (e.g., {}) is treated as non-empty and extractFromMap returns a serialized {} identifier, which would surface in the UI. Consider handling Map/Collection emptiness explicitly before calling nullOrEmpty, or change the first check to an instanceof chain (String/List/Map) that uses the correct emptiness semantics.
| className="task-proposed-changes-field-row" | ||
| key={field}> | ||
| <Typography.Text className="task-proposed-changes-field-name"> | ||
| {field} |
There was a problem hiding this comment.
The proposed-changes field label currently renders the raw field key (e.g., glossaryTerms) and CSS text-transform: capitalize will produce unreadable labels like "Glossaryterms". Consider formatting with startCase(field) or mapping known keys to existing i18n labels so the UI shows human-friendly names.
| {field} | |
| {startCase(field)} |
| {removed.map((val) => | ||
| getUrl ? ( | ||
| <Link | ||
| className="task-proposed-changes-chip task-proposed-changes-chip--removed" | ||
| key={`removed-${val}`} | ||
| to={getUrl(val)}> | ||
| {val} | ||
| </Link> | ||
| ) : ( | ||
| <span | ||
| className="task-proposed-changes-chip task-proposed-changes-chip--removed" | ||
| key={`removed-${val}`}> | ||
| {val} | ||
| </span> | ||
| ) | ||
| )} | ||
| {added.map((val) => | ||
| getUrl ? ( | ||
| <Link | ||
| className="task-proposed-changes-chip task-proposed-changes-chip--added" | ||
| key={`added-${val}`} | ||
| to={getUrl(val)}> | ||
| {val} | ||
| </Link> | ||
| ) : ( | ||
| <span | ||
| className="task-proposed-changes-chip task-proposed-changes-chip--added" | ||
| key={`added-${val}`}> |
There was a problem hiding this comment.
The chip keys use only the value string (e.g. key={\added-${val}`}`), which can collide if duplicates exist in the backend payload, leading to React key warnings and unstable rendering. Use a stable unique key (e.g., include the index or ensure de-duplication before rendering).
| {removed.map((val) => | |
| getUrl ? ( | |
| <Link | |
| className="task-proposed-changes-chip task-proposed-changes-chip--removed" | |
| key={`removed-${val}`} | |
| to={getUrl(val)}> | |
| {val} | |
| </Link> | |
| ) : ( | |
| <span | |
| className="task-proposed-changes-chip task-proposed-changes-chip--removed" | |
| key={`removed-${val}`}> | |
| {val} | |
| </span> | |
| ) | |
| )} | |
| {added.map((val) => | |
| getUrl ? ( | |
| <Link | |
| className="task-proposed-changes-chip task-proposed-changes-chip--added" | |
| key={`added-${val}`} | |
| to={getUrl(val)}> | |
| {val} | |
| </Link> | |
| ) : ( | |
| <span | |
| className="task-proposed-changes-chip task-proposed-changes-chip--added" | |
| key={`added-${val}`}> | |
| {removed.map((val, index) => | |
| getUrl ? ( | |
| <Link | |
| className="task-proposed-changes-chip task-proposed-changes-chip--removed" | |
| key={`${field}-removed-${val}-${index}`} | |
| to={getUrl(val)}> | |
| {val} | |
| </Link> | |
| ) : ( | |
| <span | |
| className="task-proposed-changes-chip task-proposed-changes-chip--removed" | |
| key={`${field}-removed-${val}-${index}`}> | |
| {val} | |
| </span> | |
| ) | |
| )} | |
| {added.map((val, index) => | |
| getUrl ? ( | |
| <Link | |
| className="task-proposed-changes-chip task-proposed-changes-chip--added" | |
| key={`${field}-added-${val}-${index}`} | |
| to={getUrl(val)}> | |
| {val} | |
| </Link> | |
| ) : ( | |
| <span | |
| className="task-proposed-changes-chip task-proposed-changes-chip--added" | |
| key={`${field}-added-${val}-${index}`}> |
| font-size: 12px; | ||
| font-weight: 500; | ||
| padding-top: 2px; | ||
| text-transform: capitalize; |
There was a problem hiding this comment.
text-transform: capitalize only uppercases the first character and won't properly format camelCase field names (e.g., relatedTerms becomes "Relatedterms"). Since the UI is showing field identifiers, consider removing this and formatting the label in React (e.g., startCase) so the rendered text is correct.
| text-transform: capitalize; |
| "remove-lineage-edge": "Remove lineage edge", | ||
| "rename-entity": "Rename the Name and Display Name for the {{entity}}.", | ||
| "request-approval-message": "Approval request for", | ||
| "request-approval-notification": "Approval required for ", |
There was a problem hiding this comment.
The en-US translation value for message.request-approval-notification contains a trailing space. Trailing whitespace in i18n strings is easy to miss and makes localization inconsistent; it’s safer to remove it and add spacing explicitly in JSX where the text is composed with links.
| "request-approval-notification": "Approval required for ", | |
| "request-approval-notification": "Approval required for", |
…ith index, remove CSS capitalize, explicit JSX space
…FieldDiff record, descriptive variable names
| .task-proposed-changes { | ||
| border-radius: 12px; | ||
| border: 0.5px solid #eaecf5; | ||
| background: @grey-9; | ||
| padding: 16px; |
There was a problem hiding this comment.
The new proposed-changes container uses a hardcoded border color #eaecf5. There is already a corresponding Less variable (@grey-15) in src/styles/variables.less; using it avoids duplicating palette values and keeps styling consistent across components.
| "remove-lineage-edge": "删除血缘连线", | ||
| "rename-entity": "修改{{entity}}的名称和显示名", | ||
| "request-approval-message": "批准请求", | ||
| "request-approval-notification": "Approval required for", |
There was a problem hiding this comment.
request-approval-notification is added to the zh-CN locale but the value is still English ("Approval required for"), which makes the notification partially untranslated for this locale. Either provide the proper localized translation here (and in other non-en locales) or omit the key from non-en locale files so i18n can fall back to en-us consistently.
| "request-approval-notification": "Approval required for", | |
| "request-approval-notification": "需要批准：", |
| {proposedChanges !== null && ( | ||
| <Col span={24}> | ||
| <div className="task-proposed-changes"> | ||
| <Typography.Text className="task-proposed-changes-title"> | ||
| {t('label.proposed-change-plural')} | ||
| </Typography.Text> | ||
| <div className="task-proposed-changes-fields"> |
There was a problem hiding this comment.
The new Proposed Changes branch (JSON parsing from taskThread.message and rendering the chip list) isn’t covered by the existing TaskTabNew unit tests. Add test cases that pass a JSON taskThread.message and assert the Proposed Changes title renders, the added/removed chips render, and mapped fields render as links with the expected to URLs.
| try { | ||
| Map<String, FieldDiff> merged = | ||
| mergeChangeMaps(parseChangeMap(oldMessage), buildChangeMap(changeDescription)); | ||
| taskThread.withMessage(merged.isEmpty() ? "{}" : JsonUtils.pojoToJson(merged)); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
applyChangePreview() serializes the change preview into thread.message (JSON like { "tags": ... }). Thread.message is consumed as human-readable text in other channels (e.g., task assignment email templates populate taskName from thread.getMessage()), so this will leak raw JSON into email/Slack/Teams notifications and other renderers. Consider keeping message human-readable (e.g., "Approval required for") and storing the structured preview elsewhere (task fields), or prefixing with a readable string and updating UI parsing to extract only the JSON portion; alternatively update downstream notification/template renderers to detect/format the JSON payload for RequestApproval tasks.
| background-color: #ecfdf3; | ||
| color: #027a48; | ||
| border: 1px solid #a9efc5; |
There was a problem hiding this comment.
New proposed-changes UI styles introduce additional hardcoded hex colors (e.g., #ecfdf3, #027a48, #a9efc5). The codebase already defines equivalent Less variables (e.g., @green-9, @green-10, @green-16) in src/styles/variables.less; using those keeps theming consistent and simplifies future palette changes.
| background-color: #ecfdf3; | |
| color: #027a48; | |
| border: 1px solid #a9efc5; | |
| background-color: @green-9; | |
| color: @green-10; | |
| border: 1px solid @green-16; |
… for approval notification
Code Review ✅ Approved 5 resolved / 5 findingsImplements clickable entity links in approval tasks while resolving team routing, HTML escaping, schema violations, redundant calls, and deserialization nullability issues. No remaining findings identified. ✅ 5 resolved✅ Bug: Owner/reviewer links always route to /users, broken for teams
✅ Security: FQN text inserted into link innerHTML without HTML escaping
✅ Bug: thread.message set to null violates schema required constraint
✅ Performance: parseProposedChanges called twice with same argument
✅ Edge Case: FieldDiff record fields can be null after Jackson deserialization
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
| <Typography.Text className="task-proposed-changes-field-name"> | ||
| {startCase(field)} | ||
| </Typography.Text> |
There was a problem hiding this comment.
Field labels in the Proposed Changes section are derived via startCase(field), which produces user-facing text that is not localized. Please map known field keys to i18n labels (with a sensible fallback) so this UI remains fully translatable.
| setHasAddedComment(false); | ||
| }, [taskThread.id]); | ||
|
|
||
| const proposedChanges = isTaskGlossaryApproval |
There was a problem hiding this comment.
isTaskGlossaryApproval is used to gate the Proposed Changes rendering, but it is simply TaskType.RequestApproval (not glossary-specific). Consider renaming this boolean (and associated constants like GLOSSARY_TASK_ACTION_LIST usage if applicable) to reflect its actual meaning (e.g., isRequestApprovalTask) to avoid confusion as this feature expands beyond glossary entities.
| const proposedChanges = isTaskGlossaryApproval | |
| const isRequestApprovalTask = isTaskGlossaryApproval; | |
| const proposedChanges = isRequestApprovalTask |
|
|
|
Cherry picked to 1.12.7 4ace688 |
Summary
Approval Task Image:
Fixes #27440
Governance approval task threads now include a Proposed Changes section showing exactly what changed on the entity before the approver acts.
Why not use
feedInfo.entitySpecificInfo(rich card approach)?The activity-feed formatter pipeline already produces per-field rich card data via
feedInfo.entitySpecificInfo— butentitySpecificInfois a single polymorphic object, not an array. When multiple fields change in one approval (e.g. tags + description), the formatter returns aList<Thread>with one entry per field, each carrying its ownentitySpecificInfo. There is only onefeedInfoslot on the task thread, so only one field's data can be stored — the rest are lost.Fixing this properly would require turning
feedInfo.entitySpecificInfointo an array in the spec, plus a data migration over every existingThreadrow in the feed table (potentially millions of records in production). A Task Redesign is landing in ~2 months, so we are not introducing schema changes or data migrations for this.Where the data lives in the Thread JSON
The
messagefield is a top-level field on theThreadobject returned byGET /api/v1/feed?entityLink=...&type=Task. The backend writes a structured JSON object into this field.cardStyle,fieldOperation, andfeedInfoare explicitly set tonullfor approval tasks (preventing stale rich-card data).{ "id": "5a00a510-...", "type": "Task", "about": "<#E::glossaryTerm::1.2>", "message": "{\"tags\":{\"added\":[\"PII.Sensitive\"],\"removed\":[\"PII.None\"]},\"description\":{\"added\":[\"<p>new text</p>\"],\"removed\":[\"<p>old text</p>\"]}}", "cardStyle": null, "fieldOperation": null, "feedInfo": null, "task": { "id": 22, "type": "RequestApproval", "assignees": [ { "name": "aaron.singh2" } ], "status": "Open" } }The JSON structure inside
message:{ "tags": { "added": ["PII.Sensitive", "PersonalData.Personal"], "removed": ["PII.None"] }, "description": { "added": ["<p>new description</p>"], "removed": ["<p>old description</p>"] }, "owners": { "added": ["Aaron Johnson"], "removed": ["Jane Smith"] } }Identifier extraction priority for each field value:
tagFQN— for tag labelsfullyQualifiedName— for domains, glossary terms, entitiesdisplayName— for users, teams, entity referencesname— fallback for named objectsdescription,entityStatusNo new JSON schema fields were added to
Thread,TaskDetails, orFeedInfo. The existingthread.messagefield carries this payload.Merge across re-edits (set-cancellation)
When an entity is edited while its approval task is still open, the changes are merged — not overwritten. The algorithm uses set-cancellation so the approver always sees the net difference from the original state:
Example across 3 edits on a glossary term:
PII.None→PII.Sensitivetags: {added: [PII.Sensitive], removed: [PII.None]}PersonalData.Personaltags: {added: [PII.Sensitive, PersonalData.Personal], removed: [PII.None]}PII.Sensitivetags: {added: [PersonalData.Personal], removed: [PII.None]}Fields where both
addedandremovedbecome empty after cancellation are dropped entirely.Backend changes —
CreateApprovalTaskImpl.javaAll change-preview logic extracted into
org.openmetadata.service.governance.workflows.util.ChangePreviewUtils:buildChangeMap(ChangeDescription)— convertsfieldsAdded/Updated/Deletedto the JSON mapmergeChangeMaps(oldMap, newMap)— set-cancellation merge across re-editsparseChangeMap(String message)— deserializes existing JSON message for mergingextractIdentifiers(Object fieldValue)— handles JSON arrays, objects, and plain stringsapplyChangePreview(Thread, EntityInterface, String oldMessage)— orchestrates the full preview update; clearscardStyle/fieldOperation/feedInfoto nullCreateApprovalTaskImplis now pure Flowable orchestration — callsChangePreviewUtils.applyChangePreview(...)on both the create and update paths.UI changes —
TaskTabNew.component.tsxThe Proposed Changes card is rendered immediately after
taskHeader. It is shown whentaskThread.message?.trimStart().startsWith('{')(JSON detection).Each field renders as a row: field name label + red chips for removed values + green chips for added values. Fields in
FIELD_ROUTE_MAP(tags,relatedTerms,domains) render as clickable links to the relevant entity page.Removed
dangerouslySetInnerHTML,FIELD_LINK_MAP,DIFF_SPAN_RE,escapeHtml,addEntityLinks, and the markdown/HTML rendering pipeline entirely.Notification panel (
NotificationFeedCard.component.tsx): replaced{task.message}(which leaked raw HTML/JSON into the bell notification) with the static i18n string"Approval request for".Test plan
changeDescription) → "Proposed Changes" section is absentRequestDescription,RequestTag, etc.)