From 554685069e5bbb197296d5da21d9b4ff79146eb8 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sun, 22 Sep 2019 14:31:58 -0700
Subject: [PATCH] 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 https://gitlab.com/gitlab-org/gitlab/issues/32477
---
 .../sh-fix-any-approver-handling.yml          |  5 ++++
 .../approval_rules/finalize_service.rb        | 16 ++++++++++++
 .../approval_rules/finalize_service_spec.rb   | 25 +++++++++++++++++++
 3 files changed, 46 insertions(+)
 create mode 100644 changelogs/unreleased/sh-fix-any-approver-handling.yml

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 00000000000000..4230dce3b74e68
--- /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 048f61f1174cbe..fd80a8c1f271d3 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 01194d6942fc7f..7cb0ff972884da 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
 
-- 
GitLab