Merge branch 'bvl-wrap-fallback-approval-rule' into 'master'

Wrap the approvals required in a fallback rule

See merge request gitlab-org/gitlab-ee!9491

ee/app/controllers/ee/projects/merge_requests/application_controller.rb

@@ -24,8 +24,9 @@ module EE
         end
 
         # If the number of approvals is not greater than the project default, set to
-        # nil, so that we fall back to the project default. If it's not set, we can
-        # let the normal update logic handle this.
+        # the project default, so that we fall back to the project default. And
+        # still allow overriding rules defined at the project level but not allow
+        # a number of approvals lower than what the project defined.
         def clamp_approvals_before_merge(mr_params)
           return mr_params unless mr_params[:approvals_before_merge]
 
@@ -39,8 +40,8 @@ module EE
                              project
                            end
 
-          if mr_params[:approvals_before_merge].to_i <= target_project.approvals_before_merge
-            mr_params[:approvals_before_merge] = nil
+          if mr_params[:approvals_before_merge].to_i < target_project.min_fallback_approvals
+            mr_params[:approvals_before_merge] = target_project.min_fallback_approvals
           end
 
           mr_params

ee/app/models/approval_merge_request_fallback.rb

+# frozen_string_literal: true
+
+class ApprovalMergeRequestFallback
+  attr_reader :merge_request
+  delegate :approved_by_users, :project, to: :merge_request
+
+  def initialize(merge_request)
+    @merge_request = merge_request
+  end
+
+  # Implements all `WrappedApprovalRule` required methods
+  def id
+    'fallback-rule'
+  end
+
+  def name
+    ''
+  end
+
+  def users
+    User.none
+  end
+
+  def groups
+    Group.none
+  end
+
+  def approvals_required
+    @approvals_required ||= [merge_request.approvals_before_merge.to_i,
+                             project.min_fallback_approvals].max
+  end
+
+  def approvals_left
+    @approvals_left ||= [approvals_required - approved_by_users.size, 0].max
+  end
+
+  def approvers
+    []
+  end
+
+  def approved_approvers
+    approved_by_users
+  end
+
+  def approved?
+    approved_approvers.size >= approvals_required
+  end
+
+  def code_owner
+    false
+  end
+
+  def source_rule
+    nil
+  end
+
+  def regular
+    false
+  end
+
+  def rule_type
+    :fallback
+  end
+
+  def project
+    merge_request.target_project
+  end
+end

ee/app/models/approval_state.rb

@@ -31,40 +31,41 @@ class ApprovalState
 
   def wrapped_approval_rules
     strong_memoize(:wrapped_approval_rules) do
-      regular_rules + code_owner_rules
+      result = use_fallback? ? [fallback_rule] : regular_rules
+      result += code_owner_rules
+      result
     end
   end
 
-  def has_approval_rules?
-    !wrapped_approval_rules.empty?
+  def has_non_fallback_rules?
+    regular_rules.present? || code_owner_rules.present?
   end
 
+  # Use the fallback rule if regular rules are empty
   def use_fallback?
     regular_rules.empty?
   end
 
+  def fallback_rule
+    @fallback_rule ||= ApprovalMergeRequestFallback.new(merge_request)
+  end
+
+  # Determines which set of rules to use (MR or project)
   def approval_rules_overwritten?
-    merge_request.approval_rules.any?(&:regular?)
+    regular_merge_request_rules.any? ||
+      (project.can_override_approvers? && merge_request.approvals_before_merge.present?)
   end
   alias_method :approvers_overwritten?, :approval_rules_overwritten?
 
   def approval_needed?
     return false unless project.feature_available?(:merge_request_approvers)
 
-    result = wrapped_approval_rules.any? { |rule| rule.approvals_required > 0 }
-    result ||= fallback_approvals_required > 0 if use_fallback?
-    result
-  end
-
-  def fallback_approvals_required
-    @fallback_approvals_required ||= [project.approvals_before_merge, merge_request.approvals_before_merge || 0].max
+    wrapped_approval_rules.any? { |rule| rule.approvals_required > 0 }
   end
 
   def approved?
     strong_memoize(:approved) do
-      result = wrapped_approval_rules.all?(&:approved?)
-      result &&= approvals.size >= fallback_approvals_required if use_fallback?
-      result
+      wrapped_approval_rules.all?(&:approved?)
     end
   end
 
@@ -74,9 +75,7 @@ class ApprovalState
 
   def approvals_required
     strong_memoize(:approvals_required) do
-      result = wrapped_approval_rules.sum(&:approvals_required)
-      result = [result, fallback_approvals_required].max if use_fallback?
-      result
+      wrapped_approval_rules.sum(&:approvals_required)
     end
   end
 
@@ -84,9 +83,7 @@ class ApprovalState
   # considered approved.
   def approvals_left
     strong_memoize(:approvals_left) do
-      result = wrapped_approval_rules.sum(&:approvals_left)
-      result = [result, fallback_approvals_required - approved_approvers.size].max if use_fallback?
-      result
+      wrapped_approval_rules.sum(&:approvals_left)
     end
   end
 
@@ -156,10 +153,9 @@ class ApprovalState
 
   def regular_rules
     strong_memoize(:regular_rules) do
-      rule_source = approval_rules_overwritten? ? merge_request : project
-      rules = rule_source.approval_rules.select(&:regular?).sort_by(&:id)
+      rules = approval_rules_overwritten? ? regular_merge_request_rules : regular_project_rules
 
-      unless project.feature_available?(:multiple_approval_rules)
+      unless project.multiple_approval_rules_available?
         rules = rules[0, 1]
       end
 
@@ -167,6 +163,14 @@ class ApprovalState
     end
   end
 
+  def regular_merge_request_rules
+    @regular_merge_request_rules ||= merge_request.approval_rules.select(&:regular?).sort_by(&:id)
+  end
+
+  def regular_project_rules
+    @regular_project_rules ||= project.visible_regular_approval_rules.to_a
+  end
+
   def code_owner_rules
     strong_memoize(:code_owner_rules) do
       wrap_rules(merge_request.approval_rules.select(&:code_owner?))

ee/app/models/approval_wrapped_rule.rb

@@ -8,7 +8,7 @@ class ApprovalWrappedRule
   attr_reader :merge_request
   attr_reader :approval_rule
 
-  def_delegators :@approval_rule, :id, :name, :users, :groups, :approvals_required, :code_owner, :source_rule
+  def_delegators :@approval_rule, :id, :name, :users, :groups, :approvals_required, :code_owner, :source_rule, :rule_type
 
   def initialize(merge_request, approval_rule)
     @merge_request = merge_request

ee/app/models/concerns/approval_rule_like.rb

@@ -38,4 +38,8 @@ module ApprovalRuleLike
       groups.delete(member)
     end
   end
+
+  def rule_type
+    @rule_type ||= code_owner? ? :code_owner : :regular
+  end
 end

ee/app/models/ee/project.rb

@@ -213,6 +213,10 @@ module EE
       feature_available?(:multiple_project_issue_boards)
     end
 
+    def multiple_approval_rules_available?
+      feature_available?(:multiple_approval_rules)
+    end
+
     def service_desk_enabled
       ::EE::Gitlab::ServiceDesk.enabled?(project: self) && super
     end
@@ -286,6 +290,25 @@ module EE
       super
     end
 
+    def visible_regular_approval_rules
+      return approval_rules.none unless ::Feature.enabled?(:approval_rules, self)
+
+      strong_memoize(:visible_regular_approval_rules) do
+        regular_rules = approval_rules.regular.order(:id)
+
+        next regular_rules.take(1) unless multiple_approval_rules_available?
+
+        regular_rules
+      end
+    end
+
+    def min_fallback_approvals
+      strong_memoize(:min_fallback_approvals) do
+        visible_regular_approval_rules.map(&:approvals_required).max ||
+          approvals_before_merge.to_i
+      end
+    end
+
     def reset_approvals_on_push
       super && feature_available?(:merge_request_approvers)
     end

ee/lib/ee/api/entities.rb

@@ -243,13 +243,16 @@ module EE
         expose :title
       end
 
-      class ApprovalRule < Grape::Entity
+      class ApprovalRuleShort < Grape::Entity
+        expose :id, :name, :rule_type
+      end
+
+      class ApprovalRule < ApprovalRuleShort
         def initialize(object, options = {})
           presenter = ::ApprovalRulePresenter.new(object, current_user: options[:current_user])
           super(presenter, options)
         end
 
-        expose :id, :name
         expose :approvers, using: ::API::Entities::UserBasic
         expose :approvals_required
         expose :users, using: ::API::Entities::UserBasic
@@ -273,16 +276,12 @@ module EE
         end
 
         expose :wrapped_approval_rules, as: :rules, using: MergeRequestApprovalRule
-        expose :fallback_approvals_required
-        expose :use_fallback do |approval_state|
-          approval_state.use_fallback?
-        end
       end
 
       # Decorates Project
       class ProjectApprovalRules < Grape::Entity
-        expose :approval_rules, as: :rules, using: ApprovalRule
-        expose :approvals_before_merge, as: :fallback_approvals_required
+        expose :visible_regular_approval_rules, as: :rules, using: ApprovalRule
+        expose :min_fallback_approvals, as: :fallback_approvals_required
       end
 
       # @deprecated
@@ -383,12 +382,14 @@ module EE
           approval_state.can_approve?(options[:current_user])
         end
 
-        expose :approval_rules_left do |approval_state, options|
-          approval_state.approval_rules_left.map(&:name)
-        end
+        expose :approval_rules_left, using: ApprovalRuleShort
 
         expose :has_approval_rules do |approval_state|
-          approval_state.has_approval_rules?
+          approval_state.has_non_fallback_rules?
+        end
+
+        expose :multiple_approval_rules_available do |approval_state|
+          approval_state.project.multiple_approval_rules_available?
         end
       end
 

ee/spec/controllers/projects/merge_requests/creations_controller_spec.rb

@@ -38,8 +38,8 @@ describe Projects::MergeRequests::CreationsController do
           create_merge_request(approvals_before_merge: 1)
         end
 
-        it 'sets the param to nil' do
-          expect(created_merge_request.approvals_before_merge).to eq(nil)
+        it 'sets the param to the project value' do
+          expect(created_merge_request.reload.approvals_before_merge).to eq(2)
         end
 
         it 'creates the merge request' do
@@ -53,8 +53,8 @@ describe Projects::MergeRequests::CreationsController do
           create_merge_request(approvals_before_merge: 2)
         end
 
-        it 'sets the param to nil' do
-          expect(created_merge_request.approvals_before_merge).to eq(nil)
+        it 'sets the param to the correct value' do
+          expect(created_merge_request.reload.approvals_before_merge).to eq(2)
         end
 
         it 'creates the merge request' do
@@ -88,7 +88,7 @@ describe Projects::MergeRequests::CreationsController do
         end
 
         it 'uses the default from the target project' do
-          expect(created_merge_request.approvals_before_merge).to eq(nil)
+          expect(created_merge_request.reload.approvals_before_merge).to eq(4)
         end
 
         it 'creates the merge request' do

ee/spec/controllers/projects/merge_requests_controller_spec.rb

@@ -164,6 +164,12 @@ describe Projects::MergeRequestsController do
           expect(merge_request.reload.approvals_before_merge).to eq(2)
         end
 
+        it 'does not allow approvels before merge lower than the project setting' do
+          update_merge_request(approvals_before_merge: 0)
+
+          expect(merge_request.reload.approvals_before_merge).to eq(1)
+        end
+
         it 'creates rules' do
           users = create_list(:user, 3)
           users.each { |user| project.add_developer(user) }
@@ -221,8 +227,8 @@ describe Projects::MergeRequestsController do
             update_merge_request(approvals_before_merge: 1)
           end
 
-          it 'sets the param to nil' do
-            expect(merge_request.approvals_before_merge).to eq(nil)
+          it 'sets the param to the sames as the project' do
+            expect(merge_request.reload.approvals_before_merge).to eq(2)
           end
 
           it 'updates the merge request' do
@@ -236,8 +242,8 @@ describe Projects::MergeRequestsController do
             update_merge_request(approvals_before_merge: 2)
           end
 
-          it 'sets the param to nil' do
-            expect(merge_request.reload.approvals_before_merge).to eq(nil)
+          it 'sets the param to the same as the project' do
+            expect(merge_request.reload.approvals_before_merge).to eq(2)
           end
 
           it 'updates the merge request' do
@@ -287,8 +293,8 @@ describe Projects::MergeRequestsController do
             update_merge_request(approvals_before_merge: 1)
           end
 
-          it 'sets the param to nil' do
-            expect(merge_request.reload.approvals_before_merge).to eq(nil)
+          it 'sets the param to the same as the target project' do
+            expect(merge_request.reload.approvals_before_merge).to eq(2)
           end
 
           it 'updates the merge request' do
@@ -302,8 +308,8 @@ describe Projects::MergeRequestsController do
             update_merge_request(approvals_before_merge: 2)
           end
 
-          it 'sets the param to nil' do
-            expect(merge_request.reload.approvals_before_merge).to eq(nil)
+          it 'sets the param to the same as the target project' do
+            expect(merge_request.reload.approvals_before_merge).to eq(2)
           end
 
           it 'updates the merge request' do

ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json

@@ -4,6 +4,7 @@
     "id": { "type": "integer" },
     "name": { "type": "string" },
     "approvals_required": { "type": "integer" },
+    "rule_type": { "type": "tring" },
     "approvers": {
       "type": "array",
       "items": {

ee/spec/javascripts/fixtures/merge_requests.rb

@@ -31,6 +31,8 @@ describe Projects::MergeRequestsController, '(JavaScript fixtures)', type: :cont
     approver_group = create(:approver_group, target: project)
     approver_group.group.add_owner(create(:owner))
 
+    stub_feature_flags(approval_rules: false)
+
     sign_in(admin)
   end
 

ee/spec/models/approval_merge_request_fallback_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe ApprovalMergeRequestFallback do
+  using RSpec::Parameterized::TableSyntax
+
+  let(:merge_request) { create(:merge_request, approvals_before_merge: 2) }
+  let(:project) { merge_request.project }
+  subject(:rule) { described_class.new(merge_request) }
+
+  describe '#approvals_required' do
+    where(:merge_request_requirement, :project_requirement, :project_rule_requirement, :expected) do
+      nil | nil | nil | 0
+      10  | 5   | nil | 10
+      2   | 9   | nil | 9
+      2   | 9   | 7   | 7
+      10  | 9   | 7   | 10
+    end
+
+    with_them do
+      before do
+        merge_request.approvals_before_merge = merge_request_requirement
+        project.approvals_before_merge = project_requirement
+        if project_rule_requirement
+          create(:approval_project_rule,
+                 project: project,
+                 approvals_required: project_rule_requirement)
+        end
+      end
+
+      it 'returns the expected value' do
+        expect(rule.approvals_required).to eq(expected)
+      end
+    end
+  end
+
+  describe '#approvals_left' do
+    it 'returns the correct number of approvals left' do
+      create(:approval, merge_request: merge_request)
+
+      expect(rule.approvals_left).to eq(1)
+    end
+  end
+
+  describe '#approved?' do
+    it 'is falsy' do
+      expect(rule.approved?).to be(false)
+    end
+
+    it 'is true if there where enough approvals' do
+      create_list(:approval, 2, merge_request: merge_request)
+
+      expect(rule.approved?).to be(true)
+    end
+  end
+end

ee/spec/models/approval_state_spec.rb

@@ -10,6 +10,13 @@ describe ApprovalState do
     create(factory, params)
   end
 
+  def approve_rules(rules)
+    rules_to_approve = rules.select { |rule| rule.approvals_required > 0 }
+    rules_to_approve.each do |rule|
+      create(:approval, merge_request: merge_request, user: rule.users.first)
+    end
+  end
+
   let(:merge_request) { create(:merge_request) }
   let(:project) { merge_request.target_project }
   let(:approver1) { create(:user) }
@@ -50,39 +57,58 @@ describe ApprovalState do
     end
   end
 
-  context 'when multiple rules are allowed' do
-    before do
-      stub_licensed_features(multiple_approval_rules: true)
+  context '#approval_rules_overwritten?' do
+    context 'when approval rule on the merge request does not exist' do
+      it 'returns false' do
+        expect(subject.approval_rules_overwritten?).to eq(false)
+      end
     end
 
-    describe '#wrapped_approval_rules' do
+    context 'when approval rule on the merge request exists' do
       before do
-        2.times { create_rule }
+        create(:approval_merge_request_rule, merge_request: merge_request)
       end
 
-      it 'returns all rules in wrapper' do
-        expect(subject.wrapped_approval_rules).to all(be_an(ApprovalWrappedRule))
-        expect(subject.wrapped_approval_rules.size).to eq(2)
+      it 'returns true' do
+        expect(subject.approval_rules_overwritten?).to eq(true)
       end
     end
 
-    describe '#approval_rules_overwritten?' do
-      context 'when approval rule on the merge request does not exist' do
-        it 'returns false' do
-          expect(subject.approval_rules_overwritten?).to eq(false)
-        end
+    context 'when `approvals_before_merge` is set on a merge request' do
+      before do
+        merge_request.update!(approvals_before_merge: 7)
+      end
+
+      it 'returns true' do
+        expect(subject.approval_rules_overwritten?).to eq(true)
       end
 
-      context 'when approval rule on the merge request exists' do
+      context 'when overriding approvals is not allowed' do
         before do
-          create(:approval_merge_request_rule, merge_request: merge_request)
+          project.update!(disable_overriding_approvers_per_merge_request: true)
         end
-
         it 'returns true' do
-          expect(subject.approval_rules_overwritten?).to eq(true)
+          expect(subject.approval_rules_overwritten?).to eq(false)
         end
       end
     end
+  end
+
+  context 'when multiple rules are allowed' do
+    before do
+      stub_licensed_features(multiple_approval_rules: true)
+    end
+
+    describe '#wrapped_approval_rules' do
+      before do
+        2.times { create_rule }
+      end
+
+      it 'returns all rules in wrapper' do
+        expect(subject.wrapped_approval_rules).to all(be_an(ApprovalWrappedRule))
+        expect(subject.wrapped_approval_rules.size).to eq(2)
+      end
+    end
 
     describe '#approval_needed?' do
       context 'when feature not available' do
@@ -130,9 +156,7 @@ describe ApprovalState do
       shared_examples_for 'when rules are present' do
         context 'when all rules are approved' do
           before do
-            subject.wrapped_approval_rules.each do |rule|
-              create(:approval, merge_request: merge_request, user: rule.users.first)
-            end
+            approve_rules(subject.wrapped_approval_rules)
           end
 
           it 'returns true' do
@@ -154,10 +178,6 @@ describe ApprovalState do
       shared_examples_for 'checking fallback_approvals_required' do
         before do
           project.update(approvals_before_merge: 1)
-
-          subject.wrapped_approval_rules.each do |rule|
-            allow(rule).to receive(:approved?).and_return(true)
-          end
         end
 
         context 'when it is not met' do
@@ -622,21 +642,21 @@ describe ApprovalState do
       end
     end
 
-    describe '#approval_rules_overwritten?' do
-      context 'when approval rule does not exist' do
-        it 'returns false' do
-          expect(subject.approval_rules_overwritten?).to eq(false)
-        end
+    describe '#has_non_fallback_rules?' do
+      it 'returns true when there are rules' do
+        create_rules
+
+        expect(subject.has_non_fallback_rules?).to be(true)
       end
 
-      context 'when approval rule exists' do
-        before do
-          create(:approval_merge_request_rule, merge_request: merge_request)
-        end
+      it 'returns false if there are no rules' do
+        expect(subject.has_non_fallback_rules?).to be(false)
+      end
 
-        it 'returns true' do
-          expect(subject.approval_rules_overwritten?).to eq(true)
-        end
+      it 'returns false if there are only fallback rules' do
+        project.update!(approvals_before_merge: 1)
+
+        expect(subject.has_non_fallback_rules?).to be(false)
       end
     end
 
@@ -686,9 +706,7 @@ describe ApprovalState do
       shared_examples_for 'when rules are present' do
         context 'when all rules are approved' do
           before do
-            subject.wrapped_approval_rules.each do |rule|
-              create(:approval, merge_request: merge_request, user: rule.users.first)
-            end
+            approve_rules(subject.wrapped_approval_rules)
           end
 
           it 'returns true' do
@@ -710,10 +728,6 @@ describe ApprovalState do
       shared_examples_for 'checking fallback_approvals_required' do
         before do
           project.update(approvals_before_merge: 1)
-
-          subject.wrapped_approval_rules.each do |rule|
-            allow(rule).to receive(:approved?).and_return(true)
-          end
         end
 
         context 'when it is not met' do
@@ -737,7 +751,8 @@ describe ApprovalState do
 
       context 'when only code owner rules present' do
         before do
-          2.times { create_rule(users: [create(:user)], code_owner: true) }
+          # setting approvals required to 0 since we don't want to block on them now
+          2.times { create_rule(users: [create(:user)], code_owner: true, approvals_required: 0) }
         end
 
         it_behaves_like 'when rules are present'
@@ -752,6 +767,40 @@ describe ApprovalState do
 
         it_behaves_like 'when rules are present'
       end
+
+      context 'when a single project rule is present' do
+        before do
+          create(:approval_project_rule, users: [create(:user)], project: project)
+        end
+
+        it_behaves_like 'when rules are present'
+
+        context 'when the project rule is overridden by a fallback but the project does not allow overriding' do
+          before do
+            merge_request.update!(approvals_before_merge: 1)
+            project.update!(disable_overriding_approvers_per_merge_request: true)
+          end
+
+          it_behaves_like 'when rules are present'
+        end
+
+        context 'when the project rule is overridden by a fallback' do
+          before do
+            merge_request.update!(approvals_before_merge: 1)
+          end
+
+          it_behaves_like 'checking fallback_approvals_required'
+        end
+      end
+
+      context 'when a single project rule is present that is overridden in the merge request' do
+        before do
+          create(:approval_project_rule, users: [create(:user)], project: project)
+          merge_request.update!(approvals_before_merge: 1)
+        end
+
+        it_behaves_like 'checking fallback_approvals_required'
+      end
     end
 
     describe '#any_approver_allowed?' do

ee/spec/models/concerns/approval_rule_like_spec.rb

@@ -118,4 +118,12 @@ describe ApprovalRuleLike do
       expect(subject.group_users).to eq([user1])
     end
   end
+
+  context '#rule_type' do
+    it 'returns the correct type' do
+      expect(build(:code_owner_rule).rule_type).to eq(:code_owner)
+      expect(build(:approval_merge_request_rule).rule_type).to eq(:regular)
+      expect(build(:approval_project_rule).rule_type).to eq(:regular)
+    end
+  end
 end

ee/spec/models/project_spec.rb

@@ -944,6 +944,72 @@ describe Project do
     end
   end
 
+  describe '#visible_regular_approval_rules' do
+    let(:project) { create(:project) }
+    let!(:approval_rules) { create_list(:approval_project_rule, 2, project: project) }
+
+    before do
+      stub_licensed_features(multiple_approval_rules: true)
+    end
+
+    it 'returns all approval rules' do
+      expect(project.visible_regular_approval_rules).to contain_exactly(*approval_rules)
+    end
+
+    context 'when multiple approval rules is not available' do
+      before do
+        stub_licensed_features(multiple_approval_rules: false)
+      end
+
+      it 'returns the first approval rule' do
+        expect(project.visible_regular_approval_rules).to contain_exactly(approval_rules.first)
+      end
+    end
+
+    context 'when approval rules are disabled' do
+      before do
+        stub_feature_flags(approval_rules: false)
+      end
+
+      it 'does not return any approval rules' do
+        expect(project.visible_regular_approval_rules).to be_empty
+      end
+    end
+  end
+
+  describe '#min_fallback_approvals' do
+    let(:project) { create(:project, approvals_before_merge: 1) }
+
+    it 'returns approvals before merge if there are no rules' do
+      expect(project.min_fallback_approvals).to eq(1)
+    end
+
+    context 'when approval rules are present' do
+      before do
+        create(:approval_project_rule, project: project, approvals_required: 2)
+        create(:approval_project_rule, project: project, approvals_required: 3)
+
+        stub_licensed_features(multiple_approval_rules: true)
+      end
+
+      it 'returns the maximum requirement' do
+        expect(project.min_fallback_approvals).to eq(3)
+      end
+
+      it 'returns the first rule requirement if there is a rule' do
+        stub_licensed_features(multiple_approval_rules: false)
+
+        expect(project.min_fallback_approvals).to eq(2)
+      end
+
+      it 'returns approvals before merge when code owner rules is disabled' do
+        stub_feature_flags(approval_rules: false)
+
+        expect(project.min_fallback_approvals).to eq(1)
+      end
+    end
+  end
+
   shared_examples 'project with disabled services' do
     it 'has some disabled services' do
       stub_const('License::ANY_PLAN_FEATURES', [])

ee/spec/presenters/approval_rule_presenter_spec.rb

@@ -8,10 +8,9 @@ describe ApprovalRulePresenter do
     set(:public_group) { create(:group) }
     set(:private_group) { create(:group, :private) }
     let(:groups) { [public_group, private_group] }
+    subject { described_class.new(rule, current_user: user) }
 
     shared_examples 'filtering private group' do
-      subject { described_class.new(rule, current_user: user) }
-
       context 'when user has no access to private group' do
         it 'excludes private group' do
           expect(subject.groups).to contain_exactly(public_group)
@@ -41,5 +40,13 @@ describe ApprovalRulePresenter do
 
       it_behaves_like 'filtering private group'
     end
+
+    context 'fallback rule' do
+      let(:rule) { ApprovalMergeRequestFallback.new(create(:merge_request)) }
+
+      it 'contains no groups without raising an error' do
+        expect(subject.groups).to be_empty
+      end
+    end
   end
 end

ee/spec/requests/api/merge_request_approvals_spec.rb

@@ -434,7 +434,10 @@ describe "API::MergeRequestApprovals with approval_rule enabled" do
       expect(response).to have_gitlab_http_status(200)
       expect(json_response['approvals_required']).to eq 2
       expect(json_response['approvals_left']).to eq 2
-      expect(json_response['approval_rules_left']).to eq(['foo'])
+
+      short_approval = { "id" => rule.id, "name" => rule.name, "rule_type" => rule.rule_type.to_s }
+      expect(json_response['approval_rules_left']).to eq([short_approval])
+
       expect(json_response['approved_by']).to be_empty
       expect(json_response['user_can_approve']).to be true
       expect(json_response['user_has_approved']).to be false