Merge branch...

Merge branch '205422-changing-approval-rule-target-branch-should-not-affect-existing-merge-requests' into 'master'

Fixes a bug with merge request approval rules being changed after creation

See merge request gitlab-org/gitlab!43209

ee/app/models/approval_state.rb

@@ -205,7 +205,7 @@ class ApprovalState
 
   def wrapped_rules
     strong_memoize(:wrapped_rules) do
-      merge_request_rules = merge_request.approval_rules.applicable_to_branch(target_branch)
+      merge_request_rules = merge_request.approval_rules.preload_users
 
       merge_request_rules.map! do |rule|
         ApprovalWrappedRule.wrap(merge_request, rule)

ee/app/models/ee/merge_request.rb

@@ -20,17 +20,16 @@ module EE
       has_many :approver_users, through: :approvers, source: :user
       has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approval_rules, class_name: 'ApprovalMergeRequestRule', inverse_of: :merge_request do
-        def applicable_to_branch(branch)
+        def preload_users
           ActiveRecord::Associations::Preloader.new.preload(
             self,
             [:users, :groups, approval_project_rule: [:users, :groups, :protected_branches]]
           )
 
           self.select do |rule|
-            next true unless rule.approval_project_rule.present?
             next true if rule.overridden?
 
-            rule.approval_project_rule.applies_to_branch?(branch)
+            rule
           end
         end
       end

ee/changelogs/unreleased/205422-changing-approval-rule-target-branch-should-not-affect-existing.yml

+---
+title: Fixes a bug with merge request approval rules being changed after creation
+merge_request: 43209
+author:
+type: fixed

ee/spec/models/approval_state_spec.rb

@@ -1482,9 +1482,9 @@ RSpec.describe ApprovalState do
             )
           end
 
-          it 'returns the rules that are applicable to the merge request target branch' do
+          it 'returns the rules that are applicable to the merge request' do
             expect(subject.user_defined_rules.map(&:approval_rule)).to eq([
-              another_mr_rule
+              mr_rule, another_mr_rule
             ])
           end
 
@@ -1493,7 +1493,7 @@ RSpec.describe ApprovalState do
 
             it 'returns the rules that are applicable to the specified target_branch' do
               expect(subject.user_defined_rules.map(&:approval_rule)).to eq([
-                mr_rule
+                mr_rule, another_mr_rule
               ])
             end
           end

ee/spec/models/merge_request_spec.rb

@@ -27,72 +27,6 @@ RSpec.describe MergeRequest do
     it { is_expected.to have_many(:approval_rules) }
     it { is_expected.to have_many(:approval_merge_request_rule_sources).through(:approval_rules) }
     it { is_expected.to have_many(:approval_project_rules).through(:approval_merge_request_rule_sources) }
-
-    describe 'approval_rules association' do
-      describe '#applicable_to_branch' do
-        let!(:rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
-        let(:branch) { 'stable' }
-
-        subject { merge_request.approval_rules.applicable_to_branch(branch) }
-
-        shared_examples_for 'with applicable rules to specified branch' do
-          it { is_expected.to eq([rule]) }
-        end
-
-        context 'when there are no associated source rules' do
-          it_behaves_like 'with applicable rules to specified branch'
-        end
-
-        context 'when there are associated source rules' do
-          let(:source_rule) { create(:approval_project_rule, project: merge_request.target_project) }
-
-          before do
-            rule.update!(approval_project_rule: source_rule)
-          end
-
-          context 'and rule is not overridden' do
-            before do
-              rule.update!(
-                name: source_rule.name,
-                approvals_required: source_rule.approvals_required,
-                users: source_rule.users,
-                groups: source_rule.groups
-              )
-            end
-
-            context 'and there are no associated protected branches to source rule' do
-              it_behaves_like 'with applicable rules to specified branch'
-            end
-
-            context 'and there are associated protected branches to source rule' do
-              before do
-                source_rule.update!(protected_branches: protected_branches)
-              end
-
-              context 'and branch matches' do
-                let(:protected_branches) { [create(:protected_branch, name: branch)] }
-
-                it_behaves_like 'with applicable rules to specified branch'
-              end
-
-              context 'and branch does not match anything' do
-                let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
-
-                it { is_expected.to be_empty }
-              end
-            end
-          end
-
-          context 'and rule is overridden' do
-            before do
-              rule.update!(name: 'Overridden Rule')
-            end
-
-            it_behaves_like 'with applicable rules to specified branch'
-          end
-        end
-      end
-    end
   end
 
   it_behaves_like 'an editable mentionable with EE-specific mentions' do