Merge branch '199788-append-inapplicable-rules-on-mr-create' into 'master'

Append inapplicable rules when creating MR

See merge request gitlab-org/gitlab!27886

ee/app/models/approval_project_rule.rb

@@ -23,6 +23,10 @@ class ApprovalProjectRule < ApplicationRecord
     includes(:protected_branches).select { |rule| rule.applies_to_branch?(branch) }
   end
 
+  def self.inapplicable_to_branch(branch)
+    includes(:protected_branches).reject { |rule| rule.applies_to_branch?(branch) }
+  end
+
   def applies_to_branch?(branch)
     return true if protected_branches.empty?
 

ee/app/models/concerns/approvable.rb

@@ -31,12 +31,15 @@ module Approvable
     end
   end
 
-  # rubocop:disable Gitlab/ModuleWithInstanceVariables
   def approval_state(target_branch: nil)
-    @approval_state ||= {}
-    @approval_state[target_branch] ||= ApprovalState.new(self, target_branch: target_branch)
+    approval_state = strong_memoize(:approval_state) do
+      Hash.new do |h, key|
+        h[key] = ApprovalState.new(self, target_branch: key)
+      end
+    end
+
+    approval_state[target_branch]
   end
-  # rubocop:enable Gitlab/ModuleWithInstanceVariables
 
   def approvals_given
     approvals.size

ee/app/models/ee/project.rb

@@ -404,9 +404,13 @@ module EE
     end
 
     def visible_approval_rules(target_branch: nil)
-      strong_memoize(:"visible_approval_rules_#{target_branch}") do
-        visible_user_defined_rules(branch: target_branch) + approval_rules.report_approver
+      rules = strong_memoize(:visible_approval_rules) do
+        Hash.new do |h, key|
+          h[key] = visible_user_defined_rules(branch: key) + approval_rules.report_approver
+        end
       end
+
+      rules[target_branch]
     end
 
     def visible_user_defined_rules(branch: nil)
@@ -416,6 +420,12 @@ module EE
       user_defined_rules.applicable_to_branch(branch)
     end
 
+    def visible_user_defined_inapplicable_rules(branch)
+      return [] unless multiple_approval_rules_available?
+
+      user_defined_rules.inapplicable_to_branch(branch)
+    end
+
     # TODO: Clean up this method in the https://gitlab.com/gitlab-org/gitlab/issues/33329
     def min_fallback_approvals
       strong_memoize(:min_fallback_approvals) do

ee/app/services/approval_rules/params_filtering_service.rb

@@ -25,10 +25,15 @@ module ApprovalRules
 
       return params unless params.key?(:approval_rules_attributes)
 
+      source_rule_ids = []
+
       params[:approval_rules_attributes].each do |rule_attributes|
+        source_rule_ids << rule_attributes[:approval_project_rule_id].presence
         handle_rule(rule_attributes)
       end
 
+      append_user_defined_inapplicable_rules(source_rule_ids.compact)
+
       params
     end
 
@@ -113,5 +118,37 @@ module ApprovalRules
         source.approval_rules.includes(:groups).index_by(&:id) # rubocop: disable CodeReuse/ActiveRecord
       end
     end
+
+    # Append inapplicable rules on create so they're still associated to the MR
+    # and will be available when the MR's target branch changes.
+    #
+    # Inapplicable rules are approval rules scoped to protected branches that
+    # does not match the specified `target_branch`.
+    #
+    # For example, there are 2 rules, one scoped to `master`, one scoped to `dev`.
+    # The MR's `target_branch` is set to `dev`, so the rule for `master` is
+    # inapplicable. But in case the MR's `target_branch` changes to `master`, the
+    # `master` rule should be available.
+    def append_user_defined_inapplicable_rules(source_rule_ids)
+      return if updating?
+      return unless project.scoped_approval_rules_enabled? && project.multiple_approval_rules_available?
+
+      project
+        .visible_user_defined_inapplicable_rules(params[:target_branch])
+        .each do |rule|
+          # Check if rule is already set as a source rule in one of the rules
+          # from params to prevent duplicates
+          next if source_rule_ids.include?(rule.id)
+
+          params[:approval_rules_attributes] << {
+            name: rule.name,
+            approval_project_rule_id: rule.id,
+            user_ids: rule.user_ids,
+            group_ids: rule.group_ids,
+            approvals_required: rule.approvals_required,
+            rule_type: rule.rule_type
+          }
+        end
+    end
   end
 end

ee/changelogs/unreleased/199788-append-inapplicable-rules-on-mr-create.yml

+---
+title: Append inapplicable rules when creating MR
+merge_request: 27886
+author:
+type: fixed

ee/spec/models/approval_project_rule_spec.rb

@@ -180,4 +180,33 @@ describe ApprovalProjectRule do
       end
     end
   end
+
+  describe '.inapplicable_to_branch' do
+    let!(:rule) { create(:approval_project_rule) }
+    let(:branch) { 'stable' }
+
+    subject { described_class.inapplicable_to_branch(branch) }
+
+    context 'when there are no associated protected branches' do
+      it { is_expected.to be_empty }
+    end
+
+    context 'when there are associated protected branches' do
+      before do
+        rule.update!(protected_branches: protected_branches)
+      end
+
+      context 'and branch does not match anything' do
+        let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
+
+        it { is_expected.to eq([rule]) }
+      end
+
+      context 'but branch matches' do
+        let(:protected_branches) { [create(:protected_branch, name: branch)] }
+
+        it { is_expected.to be_empty }
+      end
+    end
+  end
 end

ee/spec/models/project_spec.rb

@@ -1150,6 +1150,49 @@ describe Project do
     end
   end
 
+  describe '#visible_user_defined_inapplicable_rules' do
+    let_it_be(:project) { create(:project) }
+    let!(:rule) { create(:approval_project_rule, project: project) }
+    let!(:another_rule) { create(:approval_project_rule, project: project) }
+
+    context 'when multiple approval rules is available' do
+      before do
+        stub_licensed_features(multiple_approval_rules: true)
+      end
+
+      let(:protected_branch) { create(:protected_branch, project: project, name: 'stable-*') }
+      let(:another_protected_branch) { create(:protected_branch, project: project, name: 'test-*') }
+
+      context 'when rules are scoped' do
+        before do
+          rule.update!(protected_branches: [protected_branch])
+          another_rule.update!(protected_branches: [another_protected_branch])
+        end
+
+        it 'returns rules that are not applicable to target_branch' do
+          expect(project.visible_user_defined_inapplicable_rules('stable-1'))
+            .to match_array([another_rule])
+        end
+      end
+
+      context 'when rules are not scoped' do
+        it 'returns empty array' do
+          expect(project.visible_user_defined_inapplicable_rules('stable-1')).to be_empty
+        end
+      end
+    end
+
+    context 'when multiple approval rules is not available' do
+      before do
+        stub_licensed_features(multiple_approval_rules: false)
+      end
+
+      it 'returns empty array' do
+        expect(project.visible_user_defined_inapplicable_rules('stable-1')).to be_empty
+      end
+    end
+  end
+
   describe '#min_fallback_approvals' do
     let(:project) { create(:project) }
 

ee/spec/services/approval_rules/params_filtering_service_spec.rb

@@ -17,16 +17,18 @@ describe ApprovalRules::ParamsFilteringService do
       project.add_reporter(project_member)
 
       accessible_group.add_developer(user)
-
-      allow(Ability).to receive(:allowed?).and_call_original
-
-      allow(Ability)
-        .to receive(:allowed?)
-        .with(user, :update_approvers, merge_request)
-        .and_return(can_update_approvers?)
     end
 
     shared_examples_for(:assigning_users_and_groups) do
+      before do
+        allow(Ability).to receive(:allowed?).and_call_original
+
+        allow(Ability)
+          .to receive(:allowed?)
+          .with(user, :update_approvers, merge_request)
+          .and_return(can_update_approvers?)
+      end
+
       context 'user can update approvers' do
         let(:can_update_approvers?) { true }
 
@@ -77,6 +79,89 @@ describe ApprovalRules::ParamsFilteringService do
         let(:expected_groups) { [accessible_group] }
       end
 
+      context 'inapplicable user defined rules' do
+        let!(:source_rule) { create(:approval_project_rule, project: project) }
+        let!(:another_source_rule) { create(:approval_project_rule, project: project) }
+        let(:protected_branch) { create(:protected_branch, project: project, name: 'stable-*') }
+
+        let(:approval_rules_attributes) do
+          [
+            { name: another_source_rule.name, approval_project_rule_id: another_source_rule.id, user_ids: [project_member.id, outsider.id] }
+          ]
+        end
+
+        before do
+          source_rule.update!(protected_branches: [protected_branch])
+        end
+
+        context 'when multiple_approval_rules feature is available' do
+          before do
+            stub_licensed_features(multiple_approval_rules: true)
+          end
+
+          it 'adds inapplicable user defined rules' do
+            params = service.execute
+            approval_rules_attrs = params[:approval_rules_attributes]
+
+            aggregate_failures do
+              expect(approval_rules_attrs.size).to eq(2)
+
+              expect(approval_rules_attrs.first).to include(
+                name: another_source_rule.name,
+                approval_project_rule_id: another_source_rule.id
+              )
+
+              expect(approval_rules_attrs.last).to include(
+                name: source_rule.name,
+                approval_project_rule_id: source_rule.id,
+                user_ids: source_rule.user_ids,
+                group_ids: source_rule.group_ids,
+                approvals_required: source_rule.approvals_required,
+                rule_type: source_rule.rule_type
+              )
+            end
+          end
+
+          context 'when scoped_approval_rules feature is disabled' do
+            before do
+              stub_feature_flags(scoped_approval_rules: false)
+            end
+
+            it 'does not add inapplicable user defined rules' do
+              params = service.execute
+              approval_rules_attrs = params[:approval_rules_attributes]
+
+              aggregate_failures do
+                expect(approval_rules_attrs.size).to eq(1)
+                expect(approval_rules_attrs.first).to include(
+                  name: another_source_rule.name,
+                  approval_project_rule_id: another_source_rule.id
+                )
+              end
+            end
+          end
+        end
+
+        context 'when multiple_approval_rules feature is not available' do
+          before do
+            stub_licensed_features(multiple_approval_rules: false)
+          end
+
+          it 'does not add inapplicable user defined rules' do
+            params = service.execute
+            approval_rules_attrs = params[:approval_rules_attributes]
+
+            aggregate_failures do
+              expect(approval_rules_attrs.size).to eq(1)
+              expect(approval_rules_attrs.first).to include(
+                name: another_source_rule.name,
+                approval_project_rule_id: another_source_rule.id
+              )
+            end
+          end
+        end
+      end
+
       context 'any approver rule' do
         let(:can_update_approvers?) { true }
         let(:approval_rules_attributes) do
@@ -96,7 +181,7 @@ describe ApprovalRules::ParamsFilteringService do
     end
 
     context 'update' do
-      let(:merge_request) { create(:merge_request, target_project: project, source_project: project)}
+      let(:merge_request) { create(:merge_request, target_project: project, source_project: project) }
       let(:existing_private_group) { create(:group, :private) }
       let!(:rule1) { create(:approval_merge_request_rule, merge_request: merge_request, users: [create(:user)]) }
       let!(:rule2) { create(:approval_merge_request_rule, merge_request: merge_request, groups: [existing_private_group]) }
@@ -113,6 +198,30 @@ describe ApprovalRules::ParamsFilteringService do
         let(:expected_groups) { [accessible_group, existing_private_group] }
       end
 
+      context 'inapplicable user defined rules' do
+        let!(:source_rule) { create(:approval_project_rule, project: project) }
+        let(:protected_branch) { create(:protected_branch, project: project, name: 'stable-*') }
+
+        let(:params) do
+          {
+            approval_rules_attributes: [
+              { id: rule1.id, name: 'foo', user_ids: [project_member.id, outsider.id] }
+            ]
+          }
+        end
+
+        before do
+          source_rule.update!(protected_branches: [protected_branch])
+        end
+
+        it 'does not add inapplicable user defined rules' do
+          params = service.execute
+
+          expect(params[:approval_rules_attributes].size).to eq(1)
+          expect(params[:approval_rules_attributes].first[:name]).to eq('foo')
+        end
+      end
+
       context 'with remove_hidden_groups being true' do
         it_behaves_like :assigning_users_and_groups do
           let(:params) do