Defect #37635
closed
Respect Role#consider_workflow? when checking for allowed status transitions
0%
Description
When defining workflow rules for a role and tracker, we generally check if a role can have any workflows at all. Right now, only roles which have at least one of the edit_issues or add_issue permissions can have any workflows. We check this when displaying editable workflows for admins.
Now, it is possible that the database holds workflow rules for roles which currently do not have one of those permissions (but might have had in the past). Currently, when checking for the allowed issue statuses in Issue#new_statuses_allowed_to (which is used to validate possible new issue statuses for an issue), we do not take into account if the defined workflow rules shall be considered at all.
This might result in workflow rules take into account which can not be (directly) edited by admins, resulting in a rather surprising behavior.
The attached patch changes this by ensuring that we only use workflow transitions which are defined for roles having one of the permissions. Workflow transitions for other roles are ignored.
Files
Related issues
Updated by Go MAEDA over 1 year ago
- Tracker changed from Patch to Defect
- Status changed from New to Confirmed
- Target version set to 5.1.0
Setting the target version to 5.1.0.
Updated by Mischa The Evil over 1 year ago
- Related to Defect #37229: Ignore workflow transitions that already defined and now is not appliable added
Updated by Mischa The Evil over 1 year ago
Just a thought:
The following code in Issue#workflow_rule_by_attribute (source:/trunk/app/models/issue.rb@21799#L680) exists:
user_real = user || User.current
roles = user_real.admin ? Role.all.to_a : user_real.roles_for_project(project)
roles = roles.select(&:consider_workflow?)
The following code addition is proposed in this patch:
roles = user.admin ? Role.all.to_a : user.roles_for_project(project)
roles = roles.select(&:consider_workflow?)
Wouldn't it be better to extract this code into a new (helper) method given the similarity?
Updated by Go MAEDA over 1 year ago
Mischa The Evil wrote:
Wouldn't it be better to extract this code into a new (helper) method given the similarity?
What do you think about extracting the code as follows?
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 84907a475..a5bd372d5 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -2053,4 +2053,9 @@ class Issue < ActiveRecord::Base
Project
end
end
+
+ def roles_for_workflow(user = User.current)
+ roles = user.admin ? Role.all.to_a : user.roles_for_project(project)
+ roles.select(&:consider_workflow?)
+ end
end
Updated by Holger Just over 1 year ago
- File 0001-Consider-only-roles-with-either-add_issues-or-edit_i.patch 0001-Consider-only-roles-with-either-add_issues-or-edit_i.patch added
I'd require the user argument (rather than allowing a default). Then, this looks good for me.
I have attached an updated patch which uses this new extracted method.
Updated by Go MAEDA over 1 year ago
- Status changed from Confirmed to Closed
- Assignee set to Go MAEDA
- Resolution set to Fixed
Committed the fix. Thank you.
Updated by Go MAEDA over 1 year ago
- Has duplicate Defect #28078: Workflows inconsistencies when removing "add/edit issue" permission to a role which already has a workflow defined added
Updated by Mischa The Evil 6 months ago
- Related to Defect #39493: Role with only :edit_own_issues no longer considered for workflow added