Fixes #31692: remove reviewer→approval-task assignee sync from repositories by karanh37 · Pull Request #32912 · open-metadata/OpenMetadata

karanh37 · 2026-09-08T05:31:00Z

Describe your changes

Supersedes the original direction of this PR. Instead of wiring updateTaskWithNewReviewers onto GlossaryTermRepository, this removes the reviewer→approval-task assignee sync entirely, per discussion with @manerow (issue author) and the maintainer review.

Why removal, not wiring

updateTaskWithNewReviewers → TaskRepository.updateApprovalTaskAssignees overwrote an open approval task's assignees with the entity's reviewers whenever the reviewer list changed. That is incorrect: a userApprovalTask chooses its assignees by rule — owners, reviewers, individual users, or teams (SetApprovalAssignees: addReviewers / addOwners / users / teams). Forcing the reviewer list onto the task corrupts any task whose rule is not "reviewers" (e.g. an owners-only approval). Task assignees are a point-in-time snapshot owned by the workflow, not by the repository. Old tasks keep their assignees; a new run produces a new task with the new reviewers.

What is removed

Repository	Removed
`TagRepository`, `TestCaseRepository`, `DataProductRepository`, `DataContractRepository`, `MetricRepository`	`updateReviewers()` override + `updateTaskWithNewReviewers` helper (were live)
`GlossaryTermRepository`	dead `updateTaskWithNewReviewers` + now-unused `resolveEffectiveReviewers` (and the wiring added earlier on this branch)
`KnowledgePageRepository`	dead `updateTaskWithNewReviewers`
`TaskRepository`	`updateApprovalTaskAssignees` (no remaining callers)

Kept: closeApprovalTaskForEntity / findOpenTaskByEntityAndCategory — task closure on Draft/Approve is correct and unaffected.

Behavior impact (stated explicitly, per Adrià's review)

Group 1 — GlossaryTerm, KnowledgePage: the helper was dead (no caller). Removing it changes zero behavior. Glossary is doubly safe: the seeded GlossaryTermApprovalWorkflow re-triggers on a reviewer change and supersedes the open task with a fresh one assigned to the current reviewers.
Group 2 — Tag, TestCase, DataProduct, DataContract, Metric: the helper was live, but these entity types have no seeded approval workflow. For a user-defined approval workflow this sync was the only thing that moved an open task's assignees on a reviewer change. After this PR, changing reviewers leaves an existing open approval task's assignees untouched — the intended semantics (assignees ≠ reviewers). This is a deliberate, agreed behavior change, not a pure cleanup.

No Collate change is needed. The Knowledge Center page feature now lives upstream in OSS as KnowledgePageRepository (the page entity) — whose dead updateTaskWithNewReviewers this PR removes. Collate's old io.collate.repository.KnowledgeCenterRepository no longer exists on openmetadata-collate main, and no collate-service code (checked across all branches) references the removed methods, so the downstream build is unaffected. The submodule pointer bump is handled separately after this PR merges.

Closes #31692

Type of change

Bug fix (non-breaking change which fixes an issue)

How was this patch tested?

Two layers, both targeting the invariant directly (assignees follow the userApprovalTask rule, reviewers do not leak in):

Unit — SetApprovalAssigneesImplTest (+2): testOwnersOnlyRule_ignoresReviewers and testExplicitUsersRule_ignoresReviewers assert that an owners-only / explicit-users approval rule does not pull the entity's reviewers into the task's assignees. Fast, deterministic. Ran green: 16 tests, 0 failures.
Runtime IT — WorkflowDefinitionResourceIT#test_reviewerChangeDoesNotOverwriteOwnerAssignedApprovalTask: creates a custom approval workflow whose task is assigned to owners on a tag (a Group-2 entity), triggered on Created only so a reviewer change cannot re-trigger it. Creates a tag owned by USER1 with USER2 as reviewer, waits for the owner-assigned approval task, changes the reviewers, and asserts the task's assignees are unchanged (still the owner, the added reviewer never injected). This fails without the removal — the repository overwrote the open task's assignees with the reviewer list inside the PATCH transaction. Ran green: Tests run: 1, Failures: 0, Errors: 0 (Testcontainers, MySQL/Postgres + ES).

The prior in-place-patch IT was removed. openmetadata-service and openmetadata-integration-tests compile and pass spotless:apply.

Checklist:

I have performed a self-review of my code
My changes generate no new warnings
I have added a test for the acceptance criterion
I have made corresponding changes to the documentation (n/a)

GlossaryTermRepository.updateTaskWithNewReviewers() existed but had no caller, unlike the five sibling repositories (Tag, TestCase, DataProduct, DataContract, Metric) that invoke it from an updateReviewers() override. Reviewer/assignee consistency relied solely on the seeded Glossary Approval Workflow re-triggering, which supersedes the open approval task with a new one (new task id) rather than patching it in place, orphaning comment history and coupling the guarantee to workflow configuration. Add an updateReviewers() override in GlossaryTermUpdater following the TagRepository pattern: when the reviewer list changes it patches the existing open approval task's assignees in place. updateApprovalTaskAssignees returns early when no open task exists, so terms without an approval workflow are unaffected. Add GlossaryTermReviewerTaskSyncIT: creates a term with one reviewer, waits for its open approval task, adds a second reviewer, and asserts the original task (fetched by id) is patched to include the new reviewer. Because the repository patch is synchronous within the PATCH transaction and the workflow safety net can only open a new task, this isolates the repository mechanism from the workflow without disabling the seeded workflow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

github-actions · 2026-09-08T05:31:12Z

✅ PR checks passed

The linked issue has a description and all required Shipping project fields set. Thanks!

github-actions · 2026-09-08T06:24:02Z

✅ Playwright Results — workflow succeeded

Validated commit 884e311fc7cd8ae3e78fba8e7718f059866aa8cc in Playwright run 34207740306, attempt 1.

✅ 4475 passed · ❌ 0 failed · 🟡 8 flaky · ⏭️ 1 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 46m 32s

⏱️ Max setup 4m 7s · max shard execution 21m 55s · max shard-job elapsed before upload 26m 6s · reporting 16s

🌐 217.11 requests/attempt · 2.31 app boots/UI scenario · 37.72% common-shard skew

Optimization targets still in progress:

Common shard skew was 37.72% (convergence target: at most 15%).
Browser traffic was 217.11 requests per attempt (convergence target: fewer than 200).
Application boot ratio was 2.31 per UI scenario (10932 boots / 4740 scenarios; convergence target: at most 1).

Shard	Passed	Flaky	Skipped
✅ Shard advanced-search-01	130	0	0
✅ Shard chromium-01	174	0	0
✅ Shard chromium-02	167	0	0
✅ Shard chromium-03	157	0	0
✅ Shard chromium-04	186	0	0
🟡 Shard chromium-05	191	1	0
🟡 Shard chromium-06	151	1	0
🟡 Shard chromium-07	174	1	0
✅ Shard chromium-08	167	0	0
✅ Shard chromium-09	140	0	0
✅ Shard chromium-10	226	0	0
✅ Shard chromium-11	201	0	0
🟡 Shard chromium-12	210	1	0
🟡 Shard chromium-13	183	1	1
🟡 Shard chromium-14	166	1	0
✅ Shard chromium-15	215	0	0
✅ Shard chromium-16	164	0	0
✅ Shard chromium-17	160	0	0
✅ Shard chromium-18	216	0	0
🟡 Shard chromium-19	143	2	0
✅ Shard chromium-20	180	0	0
✅ Shard chromium-21	187	0	0
✅ Shard chromium-22	167	0	0
✅ Shard data-asset-rules-01	65	0	0
✅ Shard domain-isolation-01	16	0	0
✅ Shard global-state-01	34	0	0
✅ Shard import-export-01	80	0	0
✅ Shard import-export-02	70	0	0
✅ Shard ingestion-01	39	0	0
✅ Shard ingestion-02	47	0	0
✅ Shard reindex-01	28	0	0
✅ Shard search-01	12	0	0
✅ Shard search-rbac-01	29	0	0

🟡 8 flaky test(s) (passed on retry)

Pages/DataProductAndSubdomains.spec.ts › Add assets to data product and verify count (shard chromium-05, 1 retry)
Pages/Domains.spec.ts › Follow/unfollow subdomain and create nested sub domain (shard chromium-06, 1 retry)
Pages/TasksUIFlow.spec.ts › Create and reject tag task for Dashboard via UI (shard chromium-07, 1 retry)
Features/Workflows/WorkflowOssRestrictions.spec.ts › schedule-type-select is disabled in OSS (shard chromium-12, 1 retry)
Pages/DataContractsSemanticRules.spec.ts › Validate Description Rule Is_Not_Set (shard chromium-13, 1 retry)
Pages/CustomProperties.spec.ts › Should search custom properties for topic in right panel (shard chromium-14, 1 retry)
Features/ActivityFeed.spec.ts › emoji reactions can be added and toggled off on a feed card (shard chromium-19, 1 retry)
Flow/CustomizeWidgets.spec.ts › KPI Widget (shard chromium-19, 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

…tories updateTaskWithNewReviewers / TaskRepository.updateApprovalTaskAssignees overwrote an open approval task's assignees with the entity's reviewers whenever the reviewer list changed. That is wrong: a userApprovalTask chooses its assignees by rule (owners, reviewers, individual users or teams), so forcing the reviewer list onto the task corrupts any task not configured to assign reviewers. Task assignees are a point-in-time snapshot owned by the workflow, not by the repository. Remove the mechanism everywhere: - updateReviewers() overrides in Tag, TestCase, DataProduct, DataContract, Metric (were live) - dead updateTaskWithNewReviewers in GlossaryTerm and KnowledgePage (never called) plus GlossaryTerm's now-unused resolveEffectiveReviewers - TaskRepository.updateApprovalTaskAssignees (no remaining callers) This also reverts the wiring added earlier on this branch. Behavior note: Tag/TestCase/DataProduct/DataContract/Metric have no seeded approval workflow, so for a custom approval workflow this sync was the only thing that moved an open task's assignees on a reviewer change. After this change, changing reviewers leaves an existing open approval task's assignees untouched — the intended semantics. GlossaryTerm and AI assets have seeded workflows that already supersede the open task with a fresh one assigned to the current reviewers, so their user-facing behavior is unchanged. Replace the prior in-place-patch IT with GlossaryTermReviewerChangeApprovalIT, which asserts the acceptance criterion: adding a reviewer to an in-review term leaves exactly one open approval task assigned to the new reviewers.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Require the settled open approval task to be a NEW task (id != original) assigned to the added reviewer, and fail fast with diagnostics if the approval workflow settles terminally without producing it, instead of a bare Awaitility timeout.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Replace the glossary supersession IT (which passed both before and after the removal) with tests that target the invariant directly: - SetApprovalAssigneesImplTest: an owners-only and an explicit-users userApprovalTask rule must not pull the entity's reviewers into the task's assignees — the principle the removed repository sync violated. - WorkflowDefinitionResourceIT#test_reviewerChangeDoesNotOverwriteOwnerAssignedApprovalTask: a custom approval workflow assigns its task to owners on a tag (Group-2 entity), triggered on Created only so a reviewer change cannot re-trigger it. Changing the tag's reviewers must leave the owner-assigned task's assignees untouched. This fails without the removal: the repository overwrote the open task's assignees with the reviewer list in the PATCH.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

gitar-bot · 2026-09-08T09:02:21Z

Code Review ✅ Approved

Removes the reviewer→approval-task assignee sync from repositories, fixing incorrect overwrites of task assignees with entity reviewers. The sync corrupted approval tasks whose assignee rule was not "reviewers" (e.g., owners-only); task assignees are workflow-owned snapshots, not repository-driven. Behavior is unchanged for GlossaryTerm and KnowledgePage (dead code removal), and is an intentional improvement for Tag, TestCase, DataProduct, DataContract, and Metric (open tasks now keep their assignees on reviewer changes). Test coverage includes the acceptance criterion with robust polling for task supersession. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

`Compact`
`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source}

Copilot AI lite review requested due to automatic review settings September 8, 2026 05:31

karanh37 requested a review from a team as a code owner September 8, 2026 05:31

Copilot AI reviewed Sep 8, 2026

github-actions Bot added safe to test Add this label to run secure Github workflows on PRs UI UI specific issues labels Sep 8, 2026

karanh37 temporarily deployed to test September 8, 2026 05:59 — with GitHub Actions Inactive

yan-3005 added 2 commits September 8, 2026 13:45

Merge remote-tracking branch 'origin/main' into fix-issue-31692

42c15e8

Copilot AI review requested due to automatic review settings September 8, 2026 08:28

Copilot AI reviewed Sep 8, 2026

yan-3005 changed the title ~~Fixes #31692: wire GlossaryTerm approval-task reviewer sync~~ Fixes #31692: remove reviewer→approval-task assignee sync from repositories Sep 8, 2026

Copilot AI review requested due to automatic review settings September 8, 2026 08:31

Copilot AI reviewed Sep 8, 2026

yan-3005 had a problem deploying to test September 8, 2026 08:58 — with GitHub Actions Error

Copilot AI review requested due to automatic review settings September 8, 2026 08:58

Copilot AI reviewed Sep 8, 2026

Merge branch 'main' into fix-issue-31692

884e311

Copilot AI review requested due to automatic review settings September 8, 2026 09:01

Copilot AI reviewed Sep 8, 2026

yan-3005 temporarily deployed to test September 8, 2026 09:13 — with GitHub Actions Inactive

manerow approved these changes Sep 8, 2026

View reviewed changes

Navigation Menu

Uh oh!

Fixes #31692: remove reviewer→approval-task assignee sync from repositories - #32912

Fixes #31692: remove reviewer→approval-task assignee sync from repositories#32912
karanh37 wants to merge 6 commits into
mainfrom
fix-issue-31692

karanh37 commented Sep 8, 2026 •

edited by yan-3005

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented Sep 8, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 8, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

gitar-bot Bot commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

karanh37 commented Sep 8, 2026 • edited by yan-3005 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Why removal, not wiring

What is removed

Behavior impact (stated explicitly, per Adrià's review)

Type of change

How was this patch tested?

Checklist:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ PR checks passed

Uh oh!

github-actions Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Playwright Results — workflow succeeded

Performance

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

gitar-bot Bot commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

karanh37 commented Sep 8, 2026 •

edited by yan-3005

Loading

github-actions Bot commented Sep 8, 2026 •

edited

Loading

github-actions Bot commented Sep 8, 2026 •

edited

Loading