Commit 55468506 authored by Stan Hu's avatar Stan Hu 🔴

Fix bug that caused a merge to show an error message

When a project approval rule is created that requires N approvers in the
project (also known as an `any_approver` rule), `MergeService` would
always fail with a "Name has already been taken" validation error.

This causes `MergeWorker` to run again and show different error messages
to the user. If squashing were enabled, users would see, "Merge failed:
Failed to squash. Should be done manually.. Please try again."
Otherwise, users would see, "Merge failed: Merge request is not
mergeable". In both cases, the merge was actually successful.

This error occurred because:

1. When a merge request is created, `MergeRequests::CreateService` syncs
some project approval rules (e.g. report types) to the list of merge
request approvals.

2. If only the `any_approver` rule is present,
`ApprovalRules::FinalizeService` will attempt to create a new merge
request approval rule since
`merge_request.approval_rules.regular.exists?` is `false`..

To fix this, we don't copy over the merge request rule if one by the
same name already exists.

Closes gitlab-org/gitlab#32477
parent b776dc97
---
title: Fix bug that caused a merge to show an error message
merge_request: 17466
author:
type: fixed
......@@ -30,10 +30,26 @@ module ApprovalRules
end
end
# This freezes the approval state at the time of merge. By copying
# project-level rules as merge request-level rules, the approval
# state will be unaffected if project rules get changed or removed.
def copy_project_approval_rules
rules_by_name = merge_request.approval_rules.index_by(&:name)
merge_request.target_project.approval_rules.each do |project_rule|
users = project_rule.approvers
groups = project_rule.groups.public_or_visible_to_user(merge_request.author)
name = project_rule.name
next unless name.present?
rule = rules_by_name[name]
# If the rule already exists, we just skip this one without
# updating the current state. If the approval rules were changed
# after merging a merge request, syncing the data might make it
# appear as though this merge request hadn't been approved.
next if rule
merge_request.approval_rules.create!(
project_rule.attributes.slice('approvals_required', 'name').merge(users: users, groups: groups)
......
......@@ -62,6 +62,31 @@ describe ApprovalRules::FinalizeService do
expect(rule.approved_approvers).to contain_exactly(user1, group1_user)
end
shared_examples 'idempotent approval tests' do |rule_type|
before do
project_rule.destroy
rule = create(:approval_project_rule, project: project, name: 'another rule', approvals_required: 2, rule_type: rule_type)
rule.users = [user1]
rule.groups << group1
# Emulate merge requests approval rules synced with project rule
mr_rule = create(:approval_merge_request_rule, merge_request: merge_request, name: rule.name, approvals_required: 2, rule_type: rule_type)
mr_rule.users = rule.users
mr_rule.groups = rule.groups
end
it 'does not create a new rule if one exists' do
expect do
2.times { subject.execute }
end.not_to change { ApprovalMergeRequestRule.count }
end
end
ApprovalProjectRule.rule_types.except(:code_owner, :report_approver).each do |rule_type, _value|
it_behaves_like 'idempotent approval tests', rule_type
end
end
end
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment