diff --git a/changelogs/unreleased/sh-fix-any-approver-handling.yml b/changelogs/unreleased/sh-fix-any-approver-handling.yml new file mode 100644 index 0000000000000000000000000000000000000000..4230dce3b74e687302e2631952b54d6dc9ca1129 --- /dev/null +++ b/changelogs/unreleased/sh-fix-any-approver-handling.yml @@ -0,0 +1,5 @@ +--- +title: Fix bug that caused a merge to show an error message +merge_request: 17466 +author: +type: fixed diff --git a/ee/app/services/approval_rules/finalize_service.rb b/ee/app/services/approval_rules/finalize_service.rb index 048f61f1174cbe463caeb05255d70d08aecf54d9..fd80a8c1f271d34e51cc1c88feb0e42afaa5922a 100644 --- a/ee/app/services/approval_rules/finalize_service.rb +++ b/ee/app/services/approval_rules/finalize_service.rb @@ -30,10 +30,26 @@ def merge_group_members_into_users 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) diff --git a/ee/spec/services/approval_rules/finalize_service_spec.rb b/ee/spec/services/approval_rules/finalize_service_spec.rb index 01194d6942fc7fc3c111942e0ad6a508360366f9..7cb0ff972884dade26fd6cbf2f8963457539a2be 100644 --- a/ee/spec/services/approval_rules/finalize_service_spec.rb +++ b/ee/spec/services/approval_rules/finalize_service_spec.rb @@ -62,6 +62,31 @@ 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