Skip to content

Draft: Prefer unapproved rules over approved rules

Hunter Stewart requested to merge hustewart-mulit-approval-bug-fix into master

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.

Edited by Hunter Stewart

Merge request reports