Draft: Prefer unapproved rules over approved rules
What does this MR do and why?
Prefer unapproved rules over approved rules
This change makes it so the method responsible for finding rules (which
is used the in ::Deployments::ApprovalService
) prefers unnaproved
rules over approved rules.
While this makes it easier to avoid the described bug, it still doesn't give the user the choice of which group they want to approve for. If there are multiple groups that they are a part of that they have not yet approved, this selection will feel random, and may not be the group the really wanted to approve for.
Technical explanation
An approval request from the UI will result in a graphql mutation handled in:
ee/app/graphql/mutations/deployments/deployment_approve.rb
There, it calls the ::Deployments::ApprovalService
which lives in:
ee/app/services/deployments/approval_service.rb
it eventually runs this code:
def approval_rule
strong_memoize(:approval_rule) do
environment.find_approval_rule_for(current_user, represented_as: params[:represented_as])
end
end
which calls Environment#find_approval_rule_for
from:
ee/app/models/ee/environment.rb
here it is with the related methods it calls from the same class:
def find_approval_rule_for(user, represented_as: nil)
associated_approval_rules.find do |rule|
next if represented_as && rule.humanize.exclude?(represented_as)
rule.check_access(user)
end
end
def associated_approval_rules
strong_memoize(:associated_approval_rules) do
# Return Array instead of AR relationship, otherwise `associated_approval_rules.any?`
# and `associated_approval_rules.sum(:required_approvals)` executes a new query
::ProtectedEnvironments::ApprovalRule
.where(protected_environment: associated_protected_environments).to_a
end
end
def associated_protected_environments
strong_memoize(:associated_protected_environments) do
::ProtectedEnvironment.for_environment(self)
end
end
At this point we have the rules, but we need to return unsatisfied rules first.
How do we check if a rule is approved or not?
This took a little digging: to use the approved?
method, you need to manually set the rule.approvals_for_summary
or you'll always get false
We can see an example in:
ee/app/models/deployments/approval_summary.rb
shows an example
def rules
strong_memoize(:rules) do
approvals_by_rule_id = approvals.group_by(&:approval_rule_id)
associated_approval_rules.each do |rule|
rule.approvals_for_summary = approvals_by_rule_id[rule.id]
end
end
end
particularly rule.approvals_for_summary = approvals_by_rule_id[rule.id]
this line shows setting the approvals for summary. You have to do this first before checking approved?
The changeset is based on adding this to the find_approval_rule_for method
rule.approvals_for_summary = rule.deployment_approvals
And refactoring it a little to avoid looping twice.
MR acceptance checklist
Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
Before | After |
---|---|
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.