Enable Approval Workflows for All Assets with Flexible Assignee Sources#26055
Enable Approval Workflows for All Assets with Flexible Assignee Sources#26055
Conversation
… support Co-authored-by: karanh37 <33024356+karanh37@users.noreply.github.com>
…-reviewer entities fall back to owners Co-authored-by: karanh37 <33024356+karanh37@users.noreply.github.com>
|
|
||
| // Determine if we should add assignees from the entity's reviewers or owners. | ||
| // assigneeSource takes precedence; addReviewers is kept for backward compatibility. | ||
| boolean useReviewers = |
There was a problem hiding this comment.
We only have reviewers and owners as the source, what if the task assignee should be altogether a different user to be selected from drop down? if the fullyQualifiedName (fqn) is sent, we need to pick that up, there can be multiple inputs as well.
For example:
- we can send two user fqns
- We can choose reviewers and owners
- We can send reviewers and users
- owners and users
There was a problem hiding this comment.
Done in commit 1cf482c. The assignees config now accepts assigneeSources — an array where each entry can be:
"reviewers"— entity's reviewers (auto-approves if entity has reviewers field but it's empty)"owners"— entity's owners- A specific entity link string, e.g.
<#E::user::john.doe>— a named user or team
Multiple sources are unioned and deduplicated, so you can combine them freely:
{ "assigneeSources": ["reviewers", "<#E::user::alice>", "<#E::user::bob>"] }
{ "assigneeSources": ["owners", "<#E::team::data-team>"] }
{ "assigneeSources": ["reviewers", "owners"] }
{ "assigneeSources": ["<#E::user::alice>", "<#E::user::bob>"] }All legacy formats (assigneeSource single string, addReviewers: true) still work via fallback precedence.
There was a problem hiding this comment.
Also few Important points to note:
-
We are only changing the assignees of the task in the workflows, we need to gracefully resolve the Feed resolution as well, Today, we have Feedresolution in the FeedRepository per entity, and only the entity with reviewers have the resolution in their corresponding repository.
For example, check static class ApprovalTaskWorkflow in GlossaryTermRepository, DataContractRepository etc etc
We need to generalise this into the EntityRepository only, now that we are going to allow the task resolution of all the entities in OpenMetadata.
We have the resolve this Generically, not depending on the entity types. -
we have this function preDelete in all the repositories right? before deleting we check if the update is done by a reviewer or not, now that we are having approval tasks for people other than reviewers, we should not do this anymore, rather check if there is a task, and check if the assignees and the person who is deleting this entity match, if they match, we can allow the deletion, if not, we shouldnt allow it as there is a pending task and the task assignee should be the one to take deletion in their hands.
-
in performTask in the separate repository tasks, in the static class ApprovalTaskWorkflow we have this function performTask, and there is this statement checkUpdatedByReviewer, this is no longer the case, we should substitute with the above comment, we should check if the task assignee is truly the person who is approving the task, if not dont allow this operation
-
we also have this field
"assignees": {
"addReviewers": true
},
For example, check GlossaryApprovalWorkflow.json, this is a seeded one, there can be user created ones as well
in the workflow config right? what are we doing with this now, now that we have the assignee source?
if you are removing this, then write the migrations as well properly in 1.12.2 as a java migration only
…er entity links Co-authored-by: karanh37 <33024356+karanh37@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
@copilot address |
…2 migration Co-authored-by: karanh37 <33024356+karanh37@users.noreply.github.com>
Addressed in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| WorkflowDefinition workflow = | ||
| JsonUtils.readValue( | ||
| MAPPER.writeValueAsString(updatedNode), WorkflowDefinition.class); | ||
| collectionDAO.workflowDefinitionDAO().update(workflow); |
There was a problem hiding this comment.
@copilot This is not correct, Use WorkflowTransactionManager's createOrUpdateWorkflowDefinition function to update the workflow, check 1.10.5 migrationutil on how this is done as an example
By using the above method, we will deploy the new workflow to flowable in the method chain
There was a problem hiding this comment.
Fixed in commit 525b6f3. The migration now uses WorkflowDefinitionRepository.createOrUpdate(null, updated, ADMIN_USER_NAME) (same pattern as v1105) so the updated workflow is deployed to Flowable as part of the migration chain.
…e for Flowable deployment Co-authored-by: yan-3005 <81347100+yan-3005@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot Fix the seeded workflow jsons as well, Fix GlossaryApprovalWorkflow.json and RecognizerFeedbackReviewWorkflow.json with the default source as reviewers, fix it properly now |
|
TypeScript types have been updated based on the JSON schema changes in the PR |
Jest test CoverageUI tests summary
|
🔍 CI failure analysis for 2075eaa: playwright-ci-postgresql (3, 6) had 6 test failures, primarily due to 500 Server Errors when deleting glossaries - NullPointerException caused by EntityReference.getName() returning null, likely related to the PR's entity repository changes.IssueThe Test Results:
Critical Error Pattern: At least 8 glossaries failed to delete with this same error. Root CauseThis is a NullPointerException in the backend when processing entity references:
Likely Related to PR Changes: This PR modifies entity repositories and introduces new task handling logic:
The PR's commit Other Test Failures:
Details
Relationship to Other FailuresFor commit
Fix RequiredInvestigate why
The code should either ensure EntityReference names are never null, or add null checks before calling methods on the name. Tip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
|
Approval workflows were restricted to entities with a
reviewersfield. This lifts that restriction, introduces correct fallback semantics based on whether the entity schema supports reviewers at all, adds support for multiple assignee sources including specific users/teams, and generalizes approval task resolution and delete guards across all entity repositories.Describe your changes:
I worked on several improvements to the approval workflow system:
1. Fix reviewers fallback logic (
SetApprovalAssigneesImpl)Previously, when
assigneeSource = "reviewers"and the entity had no reviewers, the workflow fell back to owners. The correct behaviour is:table) → fall back to owners; auto-approve if owners are also empty2. Support multiple assignee sources
The
assigneesconfig now acceptsassigneeSources— an array where each entry can be:"reviewers"— entity's reviewers (with the corrected fallback semantics above)"owners"— entity's owners<#E::user::john.doe>— a named user or teamMultiple sources are unioned and deduplicated, enabling combinations such as:
{ "assigneeSources": ["reviewers", "<#E::user::alice>"] } { "assigneeSources": ["owners", "<#E::team::data-team>"] } { "assigneeSources": ["reviewers", "owners"] } { "assigneeSources": ["<#E::user::alice>", "<#E::user::bob>"] }Invalid entity link strings are skipped with a warning rather than failing the whole task.
All legacy formats (
assigneeSourcesingle string,addReviewers: true) continue to work via fallback precedence:assigneeSources→assigneeSource→addReviewers.3. Generalized
ApprovalTaskWorkflowinEntityRepositoryPreviously each entity repository (
GlossaryTermRepository,DataContractRepository,MetricRepository,DataProductRepository,TestCaseRepository,TagRepository) had its own duplicateApprovalTaskWorkflowinner class. These are now removed and replaced by a single generic implementation inEntityRepository. The generic implementation:checkUpdatedByTaskAssignee(checks task assignees, not entity reviewers)WorkflowHandler.resolveTaskentityStatuspatch if the Flowable workflow record is missing4.
preDelete— task-assignee guard instead of reviewer guardAll per-entity
preDeleteoverrides that previously blocked deletion by checkingcheckUpdatedByReviewernow callcheckDeleteAllowedByTaskAssigneeinstead. This method:RequestApprovaltask for the entity5.
performTask— task-assignee check instead of reviewer checkcheckUpdatedByRevieweris no longer called in anyperformTaskimplementation. The newcheckUpdatedByTaskAssigneeutility verifies that the acting user appears in the task'sassigneeslist (including team membership expansion).RecognizerFeedbackTaskWorkflowinTagRepositoryis also updated.6. v1.12.2 Java migration for
addReviewersfieldCreated a Java-only migration (
v1122) for MySQL and PostgreSQL that converts existing workflow definitions using the deprecated"assignees": {"addReviewers": true}format to the new"assignees": {"assigneeSources": ["reviewers"]}format. The migration usesWorkflowDefinitionRepository.createOrUpdate()(matching the v1105 pattern) so that Flowable also receives the updated workflow definition as part of the migration chain. The migration is idempotent and skips workflows that are already migrated or do not useaddReviewers.7. Updated seeded workflow JSON files
The bundled workflow definitions now use the new
assigneeSourcesformat directly:GlossaryApprovalWorkflow.json— bothApproveGlossaryTermandApprovalForUpdatesnodes updatedRecognizerFeedbackReviewWorkflow.json—ReviewFeedbacknode updatedAll occurrences of
"assignees": {"addReviewers": true}replaced with"assignees": {"assigneeSources": ["reviewers"]}.Schema & Types
assigneeSourcesarray field touserApprovalTask.jsonassignees configassigneeSource(single enum) andaddReviewers(boolean) retained as deprecated for backward compatibilitybootstrap/sql/migrations/native/1.12.2/Type of change:
Checklist:
Fixes <issue-number>: <short explanation>🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
Summary by Gitar
reviewersfield); owners used as fallback for non-reviewer entitiesassigneeSourcesarray supports reviewers, owners, and specific user/team entity linksassigneeSourcestring,addReviewersboolean) via three-tier fallback precedenceApprovalTaskWorkflowclasses; single implementation inEntityRepositoryaddReviewers: true→assigneeSources: ["reviewers"]formatWorkflowDefinitionRepository.createOrUpdate()to sync with FlowableIN_REVIEWentities deletable only by open task assignees (not entity reviewers)This will update automatically on new commits.