Merge branch '1979-2-1' into 'master'

Feature switch layer around Approvable and VisibleApprovable

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

ee/app/controllers/concerns/ee/issuable_collections.rb

@@ -9,7 +9,7 @@ module EE
     def preload_for_collection
       @preload_for_collection ||= case collection_type
                                   when 'MergeRequest'
-                                    super.push(:approvals)
+                                    super.push(:approvals, :approval_rules)
                                   else
                                     super
                                   end

ee/app/models/approval_merge_request_rule.rb

@@ -39,4 +39,9 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
     self.approved_approver_ids = merge_request.approvals.map(&:user_id) & approvers.map(&:id)
   end
+
+  def regular
+    !code_owner?
+  end
+  alias_method :regular?, :regular
 end

ee/app/models/approval_project_rule.rb

@@ -9,6 +9,11 @@ class ApprovalProjectRule < ApplicationRecord
   scope :regular, -> { all }
   scope :code_owner, -> { none }
 
+  def regular
+    true
+  end
+  alias_method :regular?, :regular
+
   def code_owner
     false
   end

ee/app/models/approval_state.rb

@@ -36,8 +36,9 @@ class ApprovalState
   end
 
   def approval_rules_overwritten?
-    merge_request.approval_rules.regular.exists?
+    merge_request.approval_rules.any?(&:regular?)
   end
+  alias_method :approvers_overwritten?, :approval_rules_overwritten?
 
   def approval_needed?
     return false unless project.feature_available?(:merge_request_approvers)
@@ -134,10 +135,10 @@ class ApprovalState
   def regular_rules
     strong_memoize(:regular_rules) do
       rule_source = approval_rules_overwritten? ? merge_request : project
-      rules = rule_source.approval_rules.regular
+      rules = rule_source.approval_rules.select(&:regular?)
 
       unless project.feature_available?(:multiple_approval_rules)
-        rules = rules.order(id: :asc).limit(1)
+        rules = rules[0, 1]
       end
 
       wrap_rules(rules)
@@ -146,7 +147,7 @@ class ApprovalState
 
   def code_owner_rules
     strong_memoize(:code_owner_rules) do
-      wrap_rules(merge_request.approval_rules.code_owner)
+      wrap_rules(merge_request.approval_rules.select(&:code_owner?))
     end
   end
 

ee/app/models/concerns/approvable_for_rule.rb

+# frozen_string_literal: true
+
+module ApprovableForRule
+  include VisibleApprovableForRule
+
+  FORWARDABLE_METHODS = %w{
+    approval_needed?
+    approved?
+    approvals_left
+    can_approve?
+    has_approved?
+    any_approver_allowed?
+    authors_can_approve?
+    approvers_overwritten?
+  }.freeze
+
+  FORWARDABLE_METHODS.each do |method|
+    define_method(method) do |*args|
+      return super(*args) if approval_rules_disabled?
+
+      approval_state.public_send(method, *args) # rubocop:disable GitlabSecurity/PublicSend
+    end
+  end
+end

ee/app/models/concerns/visible_approvable_for_rule.rb

+# frozen_string_literal: true
+
+# This module may only be used by presenters or modules
+# that include the Approvable concern
+module VisibleApprovableForRule
+  def approvers_left
+    return super if approval_rules_disabled?
+
+    approval_state.unactioned_approvers
+  end
+
+  def overall_approvers(exclude_code_owners: false)
+    return super if approval_rules_disabled?
+
+    options = { target: :users }
+    options[:code_owner] = false if exclude_code_owners
+
+    approvers = approval_state.filtered_approvers(options)
+    approvers.uniq!
+    approvers
+  end
+
+  def all_approvers_including_groups
+    return super if approval_rules_disabled?
+
+    approval_state.approvers
+  end
+
+  def approvers_from_groups
+    return super if approval_rules_disabled?
+
+    groups = approval_state.wrapped_approval_rules.flat_map(&:groups)
+    User.joins(:group_members).where(members: { source_id: groups })
+  end
+
+  def approval_rules_disabled?
+    ::Feature.disabled?(:approval_rules, project)
+  end
+end

ee/app/models/ee/merge_request.rb

@@ -7,6 +7,7 @@ module EE
 
     include ::Approvable
     include ::Gitlab::Utils::StrongMemoize
+    prepend ApprovableForRule
 
     prepended do
       include Elastic::MergeRequestsSearch
@@ -56,13 +57,18 @@ module EE
       strong_memoize(:participant_approvers) do
         next [] unless approval_needed?
 
-        approvers = []
-        approvers.concat(overall_approvers(exclude_code_owners: true))
-        approvers.concat(approvers_from_groups)
+        if ::Feature.enabled?(:approval_rules, project)
+          approval_state.filtered_approvers(code_owner: false, unactioned: true)
+        else
+          approvers = [
+            *overall_approvers(exclude_code_owners: true),
+            *approvers_from_groups
+          ]
 
           ::User.where(id: approvers.map(&:id)).where.not(id: approved_by_users.select(:id))
         end
       end
+    end
 
     def code_owners
       strong_memoize(:code_owners) do

ee/app/presenters/approval_rule_presenter.rb

+# frozen_string_literal: true
+
+class ApprovalRulePresenter < Gitlab::View::Presenter::Delegated
+  def groups
+    super.public_or_visible_to_user(current_user)
+  end
+end

ee/app/presenters/ee/merge_request_presenter.rb

@@ -3,6 +3,7 @@
 module EE
   module MergeRequestPresenter
     include ::VisibleApprovable
+    prepend VisibleApprovableForRule
 
     def approvals_path
       if requires_approve?

ee/app/services/ee/merge_requests/update_service.rb

@@ -47,6 +47,7 @@ module EE
 
       # TODO remove after #1979 is closed
       def sync_approval_rules(merge_request)
+        return if ::Feature.enabled?(:approval_rules, merge_request.target_project)
         return if merge_request.merged?
         return unless merge_request.previous_changes.include?(:approvals_before_merge)
 

ee/app/services/ee/projects/update_service.rb

@@ -77,6 +77,7 @@ module EE
 
       # TODO remove after #1979 is closed
       def sync_approval_rules
+        return if ::Feature.enabled?(:approval_rules, project)
         return unless project.previous_changes.include?(:approvals_before_merge)
 
         project.approval_rules.update_all(approvals_required: project.approvals_before_merge)

ee/spec/controllers/projects/merge_requests_controller_spec.rb

@@ -100,6 +100,7 @@ describe Projects::MergeRequestsController do
   let(:viewer)        { user }
 
   before do
+    stub_feature_flags(approval_rules: false)
     sign_in(viewer)
   end
 

ee/spec/features/merge_request/user_approves_spec.rb

@@ -61,6 +61,7 @@ describe 'Merge request > User approves', :js do
 
     context 'when CI is running but no approval given' do
       before do
+        stub_feature_flags(approval_rules: false) # TODO check in !9001 when feature enabled
         create :approver_group, group: group, target: merge_request
         pipeline = create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch)
         merge_request.update(head_pipeline: pipeline)

ee/spec/features/merge_request/user_sets_approvers_spec.rb

@@ -53,6 +53,7 @@ describe 'Merge request > User sets approvers', :js do
       let(:other_user) { create(:user) }
 
       before do
+        stub_feature_flags(approval_rules: false) # TODO https://gitlab.com/gitlab-org/gitlab-ee/issues/9430
         project.add_developer(user)
         project.add_developer(other_user)
 
@@ -108,6 +109,7 @@ describe 'Merge request > User sets approvers', :js do
       let(:merge_request) { create(:merge_request, source_project: project) }
 
       before do
+        stub_feature_flags(approval_rules: false) # TODO https://gitlab.com/gitlab-org/gitlab-ee/issues/9430
         project.add_developer(user)
 
         sign_in(user)

ee/spec/features/projects/merge_requests/user_approves_merge_request_spec.rb

@@ -14,6 +14,7 @@ describe 'User approves a merge request', :js do
 
   context 'when user can approve' do
     before do
+      stub_feature_flags(approval_rules: false) # TODO check in !9001 when feature enabled
       visit(merge_request_path(merge_request))
     end
 
@@ -30,6 +31,7 @@ describe 'User approves a merge request', :js do
 
   context 'when a merge request is approved additionally' do
     before do
+      stub_feature_flags(approval_rules: false) # TODO check in !9001 when feature enabled
       project.add_developer(user2)
       project.add_developer(user3)
     end
@@ -73,6 +75,7 @@ describe 'User approves a merge request', :js do
 
   context 'when user cannot approve' do
     before do
+      stub_feature_flags(approval_rules: false) # TODO check in !9001 when feature enabled
       merge_request.approvers.create(user_id: user2.id)
 
       visit(merge_request_path(merge_request))

ee/spec/models/approvable_spec.rb

@@ -5,6 +5,10 @@ describe Approvable do
   let(:project) { merge_request.project }
   let(:author) { merge_request.author }
 
+  before do
+    stub_feature_flags(approval_rules: false)
+  end
+
   describe '#approvers_overwritten?' do
     subject { merge_request.approvers_overwritten? }
 

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -14,6 +14,22 @@ describe ApprovalMergeRequestRule do
     end
   end
 
+  describe '#regular' do
+    it 'returns true for regular records' do
+      subject = create(:approval_merge_request_rule, merge_request: merge_request)
+
+      expect(subject.regular).to eq(true)
+      expect(subject.regular?).to eq(true)
+    end
+
+    it 'returns false for code owner records' do
+      subject = create(:approval_merge_request_rule, merge_request: merge_request, code_owner: true)
+
+      expect(subject.regular).to eq(false)
+      expect(subject.regular?).to eq(false)
+    end
+  end
+
   describe '#approvers' do
     before do
       create(:group) do |group|

ee/spec/models/approval_project_rule_spec.rb

@@ -21,6 +21,13 @@ describe ApprovalProjectRule do
     end
   end
 
+  describe '#regular' do
+    it 'returns true' do
+      expect(subject.regular).to eq(true)
+      expect(subject.regular?).to eq(true)
+    end
+  end
+
   describe '#code_owner' do
     it 'returns false' do
       expect(subject.code_owner).to eq(false)

ee/spec/models/visible_approvable_spec.rb

@@ -5,6 +5,10 @@ describe VisibleApprovable do
   let!(:project) { create(:project, :repository) }
   let!(:user) { project.creator }
 
+  before do
+    stub_feature_flags(approval_rules: false)
+  end
+
   describe '#requires_approve' do
     subject { resource.requires_approve? }
 

ee/spec/presenters/approval_rule_presenter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ApprovalRulePresenter do
+  describe '#groups' do
+    set(:user) { create(:user) }
+    set(:public_group) { create(:group) }
+    set(:private_group) { create(:group, :private) }
+    let(:groups) { [public_group, private_group] }
+
+    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)
+        end
+      end
+
+      context 'when user has access to private group' do
+        it 'includes private group' do
+          private_group.add_owner(user)
+
+          expect(subject.groups).to contain_exactly(*groups)
+        end
+      end
+    end
+
+    context 'project rule' do
+      let(:rule) { create(:approval_project_rule, groups: groups) }
+
+      it_behaves_like 'filtering private group'
+    end
+
+    context 'wrapped approval rule' do
+      let(:rule) do
+        mr_rule = create(:approval_merge_request_rule, groups: groups)
+        ApprovalWrappedRule.new(mr_rule.merge_request, mr_rule)
+      end
+
+      it_behaves_like 'filtering private group'
+    end
+  end
+end

ee/spec/presenters/merge_request_presenter_spec.rb

@@ -21,6 +21,7 @@ describe MergeRequestPresenter do
     let!(:approver) { create(:approver, target: resource) }
 
     before do
+      stub_feature_flags(approval_rules: false)
       resource.approvals.create!(user: approver.user)
     end
 
@@ -69,6 +70,10 @@ describe MergeRequestPresenter do
 
     subject { described_class.new(resource, current_user: user).all_approvers_including_groups }
 
+    before do
+      stub_feature_flags(approval_rules: false)
+    end
+
     it { is_expected.to match_array(public_approver_group.users + [approver.user]) }
 
     context 'when user has access to private group' do

ee/spec/requests/api/merge_request_approvals_spec.rb

@@ -9,6 +9,8 @@ describe API::MergeRequestApprovals do
 
   before do
     project.update_attribute(:approvals_before_merge, 2)
+
+    stub_feature_flags(approval_rules: false)
   end
 
   describe 'GET :id/merge_requests/:merge_request_iid/approvals' do

ee/spec/requests/api/project_approvals_spec.rb

@@ -10,6 +10,10 @@ describe API::ProjectApprovals do
 
   let(:url) { "/projects/#{project.id}/approvals" }
 
+  before do
+    stub_feature_flags(approval_rules: false)
+  end
+
   describe 'GET /projects/:id/approvers' do
     context 'when the request is correct' do
       before do

ee/spec/services/ee/merge_requests/refresh_service_spec.rb

@@ -73,12 +73,14 @@ describe MergeRequests::RefreshService do
         end
       end
 
-      context 'when code owners enabled' do
+      context 'when code owners enabled, with approval_rule disabled' do
         let(:old_owners) { [] }
         let(:new_owners) { [] }
         let(:relevant_merge_requests) { [merge_request, another_merge_request] }
 
         before do
+          stub_feature_flags(approval_rules: false)
+
           relevant_merge_requests.each do |merge_request|
             expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request).and_return(new_owners)
             expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request, merge_request_diff: anything).and_wrap_original do |m, *args|

ee/spec/services/ee/notification_service_spec.rb

@@ -509,4 +509,120 @@ describe EE::NotificationService, :mailer do
     # Make the watcher a subscriber to detect dupes
     issuable.subscriptions.create(user: @watcher_and_subscriber, subscribed: true)
   end
+
+  context 'Merge Requests' do
+    let(:notification) { NotificationService.new }
+    let(:assignee) { create(:user) }
+    let(:group) { create(:group) }
+    let(:project) { create(:project, :public, :repository, namespace: group) }
+    let(:another_project) { create(:project, :public, namespace: group) }
+    let(:merge_request) { create :merge_request, source_project: project, assignee: create(:user), description: 'cc @participant' }
+
+    around do |example|
+      perform_enqueued_jobs do
+        example.run
+      end
+    end
+
+    before do
+      stub_feature_flags(approval_rules: false)
+
+      project.add_maintainer(merge_request.author)
+      project.add_maintainer(merge_request.assignee)
+      build_team(merge_request.target_project)
+      add_users_with_subscription(merge_request.target_project, merge_request)
+      update_custom_notification(:new_merge_request, @u_guest_custom, resource: project)
+      update_custom_notification(:new_merge_request, @u_custom_global)
+      reset_delivered_emails!
+    end
+
+    describe '#new_merge_request' do
+      context 'when the target project has approvers set' do
+        let(:project_approvers) { create_list(:user, 3) }
+
+        before do
+          merge_request.target_project.update(approvals_before_merge: 1)
+          project_approvers.each { |approver| create(:approver, user: approver, target: merge_request.target_project) }
+          reset_delivered_emails!
+        end
+
+        it 'emails the approvers' do
+          notification.new_merge_request(merge_request, @u_disabled)
+
+          project_approvers.each { |approver| should_email(approver) }
+        end
+
+        it 'does not email the approvers when approval is not necessary' do
+          merge_request.target_project.update(approvals_before_merge: 0)
+          notification.new_merge_request(merge_request, @u_disabled)
+
+          project_approvers.each { |approver| should_not_email(approver) }
+        end
+
+        context 'when the merge request has approvers set' do
+          let(:mr_approvers) { create_list(:user, 3) }
+
+          before do
+            mr_approvers.each { |approver| create(:approver, user: approver, target: merge_request) }
+            reset_delivered_emails!
+          end
+
+          it 'emails the MR approvers' do
+            notification.new_merge_request(merge_request, @u_disabled)
+
+            mr_approvers.each { |approver| should_email(approver) }
+          end
+
+          it 'does not email approvers set on the project who are not approvers of this MR' do
+            notification.new_merge_request(merge_request, @u_disabled)
+
+            project_approvers.each { |approver| should_not_email(approver) }
+          end
+        end
+      end
+    end
+
+    def build_team(project)
+      @u_watcher               = create_global_setting_for(create(:user), :watch)
+      @u_participating         = create_global_setting_for(create(:user), :participating)
+      @u_participant_mentioned = create_global_setting_for(create(:user, username: 'participant'), :participating)
+      @u_disabled              = create_global_setting_for(create(:user), :disabled)
+      @u_mentioned             = create_global_setting_for(create(:user, username: 'mention'), :mention)
+      @u_committer             = create(:user, username: 'committer')
+      @u_not_mentioned         = create_global_setting_for(create(:user, username: 'regular'), :participating)
+      @u_outsider_mentioned    = create(:user, username: 'outsider')
+      @u_custom_global         = create_global_setting_for(create(:user, username: 'custom_global'), :custom)
+
+      # User to be participant by default
+      # This user does not contain any record in notification settings table
+      # It should be treated with a :participating notification_level
+      @u_lazy_participant      = create(:user, username: 'lazy-participant')
+
+      @u_guest_watcher = create_user_with_notification(:watch, 'guest_watching')
+      @u_guest_custom = create_user_with_notification(:custom, 'guest_custom')
+
+      [@u_watcher, @u_participating, @u_participant_mentioned, @u_disabled, @u_mentioned, @u_committer, @u_not_mentioned, @u_lazy_participant, @u_custom_global].each do |user|
+        project.add_maintainer(user)
+      end
+    end
+
+    def add_users_with_subscription(project, issuable)
+      @subscriber = create :user
+      @unsubscriber = create :user
+      @unsubscribed_mentioned = create :user, username: 'unsubscribed_mentioned'
+      @subscribed_participant = create_global_setting_for(create(:user, username: 'subscribed_participant'), :participating)
+      @watcher_and_subscriber = create_global_setting_for(create(:user), :watch)
+
+      [@subscribed_participant, @subscriber, @unsubscriber, @watcher_and_subscriber, @unsubscribed_mentioned].each do |user|
+        project.add_maintainer(user)
+      end
+
+      issuable.subscriptions.create(user: @unsubscribed_mentioned, project: project, subscribed: false)
+      issuable.subscriptions.create(user: @subscriber, project: project, subscribed: true)
+      issuable.subscriptions.create(user: @subscribed_participant, project: project, subscribed: true)
+      issuable.subscriptions.create(user: @unsubscriber, project: project, subscribed: false)
+      # Make the watcher a subscriber to detect dupes
+      issuable.subscriptions.create(user: @watcher_and_subscriber, project: project, subscribed: true)
+    end
+  end
 end

ee/spec/services/merge_requests/remove_approval_service_spec.rb

@@ -8,6 +8,10 @@ describe MergeRequests::RemoveApprovalService do
 
     subject(:service) { described_class.new(project, user) }
 
+    before do
+      stub_feature_flags(approval_rules: false)
+    end
+
     def execute!
       service.execute(merge_request)
     end

ee/spec/services/merge_requests/update_service_spec.rb

@@ -60,6 +60,11 @@ describe MergeRequests::UpdateService, :mailer do
     context 'when approvals_before_merge changes' do
       using RSpec::Parameterized::TableSyntax
 
+      context 'when approval_rules is disabled' do
+        before do
+          stub_feature_flags(approval_rules: false)
+        end
+
         where(:project_value, :mr_before_value, :mr_after_value, :result) do
           3 | 4   | 5   | 5
           3 | 4   | nil | 3
@@ -79,5 +84,133 @@ describe MergeRequests::UpdateService, :mailer do
           end
         end
       end
+
+      context 'when approval_rules is enabled' do
+        where(:project_value, :mr_before_value, :mr_after_value, :result) do
+          3 | 4   | 5   | 5
+          3 | 4   | nil | 3
+          3 | nil | 5   | 5
+        end
+
+        with_them do
+          let(:project) { create(:project, :repository, approvals_before_merge: project_value) }
+
+          it "does not update" do
+            merge_request.update(approvals_before_merge: mr_before_value)
+            rule = create(:approval_merge_request_rule, merge_request: merge_request)
+
+            update_merge_request(approvals_before_merge: mr_after_value)
+
+            expect(rule.reload.approvals_required).to eq(0)
+          end
+        end
+      end
+    end
+
+    context 'merge' do
+      before do
+        stub_feature_flags(approval_rules: false)
+      end
+
+      let(:opts) { { merge: merge_request.diff_head_sha } }
+
+      context 'when not approved' do
+        before do
+          merge_request.update(approvals_before_merge: 1)
+
+          perform_enqueued_jobs do
+            update_merge_request(opts)
+            @merge_request = MergeRequest.find(merge_request.id)
+          end
+        end
+
+        it { expect(@merge_request).to be_valid }
+        it { expect(@merge_request.state).to eq('opened') }
+      end
+
+      context 'when approved' do
+        before do
+          merge_request.update(approvals_before_merge: 1)
+          merge_request.approvals.create(user: user)
+
+          perform_enqueued_jobs do
+            update_merge_request(opts)
+            @merge_request = MergeRequest.find(merge_request.id)
+          end
+        end
+
+        it { expect(@merge_request).to be_valid }
+        it { expect(@merge_request.state).to eq('merged') }
+      end
+    end
+
+    context 'when the approvers change' do
+      let(:existing_approver) { create(:user) }
+      let(:removed_approver) { create(:user) }
+      let(:new_approver) { create(:user) }
+
+      before do
+        stub_feature_flags(approval_rules: false)
+
+        project.add_developer(existing_approver)
+        project.add_developer(removed_approver)
+        project.add_developer(new_approver)
+
+        perform_enqueued_jobs do
+          update_merge_request(approver_ids: [existing_approver, removed_approver].map(&:id).join(','))
+        end
+
+        Todo.where(action: Todo::APPROVAL_REQUIRED).destroy_all # rubocop: disable DestroyAll
+        ActionMailer::Base.deliveries.clear
+      end
+
+      context 'when an approver is added and an approver is removed' do
+        before do
+          perform_enqueued_jobs do
+            update_merge_request(approver_ids: [new_approver, existing_approver].map(&:id).join(','))
+          end
+        end
+
+        it 'adds todos for and sends emails to the new approvers' do
+          expect(Todo.where(user: new_approver, action: Todo::APPROVAL_REQUIRED)).not_to be_empty
+          should_email(new_approver)
+        end
+
+        it 'does not add todos for or send emails to the existing approvers' do
+          expect(Todo.where(user: existing_approver, action: Todo::APPROVAL_REQUIRED)).to be_empty
+          should_not_email(existing_approver)
+        end
+
+        it 'does not add todos for or send emails to the removed approvers' do
+          expect(Todo.where(user: removed_approver, action: Todo::APPROVAL_REQUIRED)).to be_empty
+          should_not_email(removed_approver)
+        end
+      end
+
+      context 'when the approvers are set to the same values' do
+        it 'does not create any todos' do
+          expect do
+            update_merge_request(approver_ids: [existing_approver, removed_approver].map(&:id).join(','))
+          end.not_to change { Todo.count }
+        end
+
+        it 'does not send any emails' do
+          expect do
+            update_merge_request(approver_ids: [existing_approver, removed_approver].map(&:id).join(','))
+          end.not_to change { ActionMailer::Base.deliveries.count }
+        end
+      end
+    end
+
+    context 'updating target_branch' do
+      it 'resets approvals when target_branch is changed' do
+        merge_request.target_project.update(reset_approvals_on_push: true)
+        merge_request.approvals.create(user_id: user2.id)
+
+        update_merge_request(target_branch: 'video')
+
+        expect(merge_request.reload.approvals).to be_empty
+      end
+    end
   end
 end

ee/spec/services/projects/update_service_spec.rb

@@ -230,7 +230,10 @@ describe Projects::UpdateService, '#execute' do
   end
 
   context 'with approval_rules' do
+    context 'when approval_rules is disabled' do
       it "updates approval_rules' approvals_required" do
+        stub_feature_flags(approval_rules: false)
+
         rule = create(:approval_project_rule, project: project)
 
         update_project(project, user, approvals_before_merge: 42)
@@ -239,6 +242,17 @@ describe Projects::UpdateService, '#execute' do
       end
     end
 
+    context 'when approval_rules is enabled' do
+      it 'does not update' do
+        rule = create(:approval_project_rule, project: project)
+
+        update_project(project, user, approvals_before_merge: 42)
+
+        expect(rule.reload.approvals_required).to eq(0)
+      end
+    end
+  end
+
   def update_project(project, user, opts)
     Projects::UpdateService.new(project, user, opts).execute
   end

spec/services/merge_requests/update_service_spec.rb

@@ -258,35 +258,6 @@ describe MergeRequests::UpdateService, :mailer do
 
         it { expect(@merge_request.state).to eq('opened') }
       end
-
-      context 'when not approved' do
-        before do
-          merge_request.update(approvals_before_merge: 1)
-
-          perform_enqueued_jobs do
-            service.execute(merge_request)
-            @merge_request = MergeRequest.find(merge_request.id)
-          end
-        end
-
-        it { expect(@merge_request).to be_valid }
-        it { expect(@merge_request.state).to eq('opened') }
-      end
-
-      context 'when approved' do
-        before do
-          merge_request.update(approvals_before_merge: 1)
-          merge_request.approvals.create(user: user)
-
-          perform_enqueued_jobs do
-            service.execute(merge_request)
-            @merge_request = MergeRequest.find(merge_request.id)
-          end
-        end
-
-        it { expect(@merge_request).to be_valid }
-        it { expect(@merge_request.state).to eq('merged') }
-      end
     end
 
     context 'todos' do
@@ -483,58 +454,6 @@ describe MergeRequests::UpdateService, :mailer do
       end
     end
 
-    context 'when the approvers change' do
-      let(:existing_approver) { create(:user) }
-      let(:removed_approver) { create(:user) }
-      let(:new_approver) { create(:user) }
-
-      before do
-        perform_enqueued_jobs do
-          update_merge_request(approver_ids: [existing_approver, removed_approver].map(&:id).join(','))
-        end
-
-        Todo.where(action: Todo::APPROVAL_REQUIRED).destroy_all # rubocop: disable DestroyAll
-        ActionMailer::Base.deliveries.clear
-      end
-
-      context 'when an approver is added and an approver is removed' do
-        before do
-          perform_enqueued_jobs do
-            update_merge_request(approver_ids: [new_approver, existing_approver].map(&:id).join(','))
-          end
-        end
-
-        it 'adds todos for and sends emails to the new approvers' do
-          expect(Todo.where(user: new_approver, action: Todo::APPROVAL_REQUIRED)).not_to be_empty
-          should_email(new_approver)
-        end
-
-        it 'does not add todos for or send emails to the existing approvers' do
-          expect(Todo.where(user: existing_approver, action: Todo::APPROVAL_REQUIRED)).to be_empty
-          should_not_email(existing_approver)
-        end
-
-        it 'does not add todos for or send emails to the removed approvers' do
-          expect(Todo.where(user: removed_approver, action: Todo::APPROVAL_REQUIRED)).to be_empty
-          should_not_email(removed_approver)
-        end
-      end
-
-      context 'when the approvers are set to the same values' do
-        it 'does not create any todos' do
-          expect do
-            update_merge_request(approver_ids: [existing_approver, removed_approver].map(&:id).join(','))
-          end.not_to change { Todo.count }
-        end
-
-        it 'does not send any emails' do
-          expect do
-            update_merge_request(approver_ids: [existing_approver, removed_approver].map(&:id).join(','))
-          end.not_to change { ActionMailer::Base.deliveries.count }
-        end
-      end
-    end
-
     context 'updating mentions' do
       let(:mentionable) { merge_request }
       include_examples 'updating mentions', described_class
@@ -620,17 +539,6 @@ describe MergeRequests::UpdateService, :mailer do
       end
     end
 
-    context 'updating target_branch' do
-      it 'resets approvals when target_branch is changed' do
-        merge_request.target_project.update(reset_approvals_on_push: true)
-        merge_request.approvals.create(user_id: user2.id)
-
-        update_merge_request(target_branch: 'video')
-
-        expect(merge_request.reload.approvals).to be_empty
-      end
-    end
-
     context 'updating asssignee_id' do
       it 'does not update assignee when assignee_id is invalid' do
         merge_request.update(assignee_id: user.id)

spec/services/notification_service_spec.rb

@@ -1304,48 +1304,6 @@ describe NotificationService, :mailer do
         should_email(user_4)
       end
 
-      context 'when the target project has approvers set' do
-        let(:project_approvers) { create_list(:user, 3) }
-
-        before do
-          merge_request.target_project.update(approvals_before_merge: 1)
-          project_approvers.each { |approver| create(:approver, user: approver, target: merge_request.target_project) }
-        end
-
-        it 'emails the approvers' do
-          notification.new_merge_request(merge_request, @u_disabled)
-
-          project_approvers.each { |approver| should_email(approver) }
-        end
-
-        it 'does not email the approvers when approval is not necessary' do
-          merge_request.target_project.update(approvals_before_merge: 0)
-          notification.new_merge_request(merge_request, @u_disabled)
-
-          project_approvers.each { |approver| should_not_email(approver) }
-        end
-
-        context 'when the merge request has approvers set' do
-          let(:mr_approvers) { create_list(:user, 3) }
-
-          before do
-            mr_approvers.each { |approver| create(:approver, user: approver, target: merge_request) }
-          end
-
-          it 'emails the MR approvers' do
-            notification.new_merge_request(merge_request, @u_disabled)
-
-            mr_approvers.each { |approver| should_email(approver) }
-          end
-
-          it 'does not email approvers set on the project who are not approvers of this MR' do
-            notification.new_merge_request(merge_request, @u_disabled)
-
-            project_approvers.each { |approver| should_not_email(approver) }
-          end
-        end
-      end
-
       context 'participating' do
         it_should_behave_like 'participating by assignee notification' do
           let(:participant) { create(:user, username: 'user-participant')}