Merge branch '6127-move-merge-requests-services-ee' into 'master'

Resolve "Extract EE specific files/lines for some merge requests files"

Closes #6127

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

app/presenters/merge_request_presenter.rb

 class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
+  prepend EE::MergeRequestPresenter
+
   include ActionView::Helpers::UrlHelper
   include GitlabRoutingHelper
   include MarkupHelper
@@ -113,12 +115,6 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
     end
   end
 
-  def approvals_path
-    if requires_approve?
-      approvals_project_merge_request_path(project, merge_request)
-    end
-  end
-
   def source_branch_with_namespace_link
     namespace = source_project_namespace
     branch = source_branch

app/services/merge_requests/merge_service.rb

@@ -36,31 +36,6 @@ module MergeRequests
       handle_merge_error(log_message: e.message, save_message_on_model: true)
     end
 
-    def hooks_validation_pass?(merge_request)
-      @merge_request = merge_request
-
-      return true if project.merge_requests_ff_only_enabled
-      return true unless project.feature_available?(:push_rules)
-
-      push_rule = merge_request.project.push_rule
-      return true unless push_rule
-
-      unless push_rule.commit_message_allowed?(params[:commit_message])
-        handle_merge_error(log_message: "Commit message does not follow the pattern '#{push_rule.commit_message_regex}'", save_message_on_model: true)
-        return false
-      end
-
-      unless push_rule.author_email_allowed?(current_user.email)
-        handle_merge_error(log_message: "Commit author's email '#{current_user.email}' does not follow the pattern '#{push_rule.author_email_regex}'", save_message_on_model: true)
-        return false
-      end
-
-      true
-    rescue PushRule::MatchError => e
-      handle_merge_error(log_message: e.message, save_message_on_model: true)
-      false
-    end
-
     private
 
     def error_check!

app/services/merge_requests/refresh_service.rb

 module MergeRequests
   class RefreshService < MergeRequests::BaseService
+    prepend EE::MergeRequests::RefreshService
+
     def execute(oldrev, newrev, ref)
       return true unless Gitlab::Git.branch_ref?(ref)
 
+      do_execute(oldrev, newrev, ref)
+    end
+
+    private
+
+    def do_execute(oldrev, newrev, ref)
       @oldrev, @newrev = oldrev, newrev
       @branch_name = Gitlab::Git.ref_name(ref)
 
@@ -24,13 +32,10 @@ module MergeRequests
       notify_about_push
       mark_mr_as_wip_from_commits
       execute_mr_web_hooks
-      reset_approvals_for_merge_requests
 
       true
     end
 
-    private
-
     def close_upon_missing_source_branch_ref
       # MergeRequest#reload_diff ignores not opened MRs. This means it won't
       # create an `empty` diff for `closed` MRs without a source branch, keeping
@@ -97,22 +102,6 @@ module MergeRequests
       merge_requests_for_source_branch(reload: true)
     end
 
-    # Note: Closed merge requests also need approvals reset.
-    def reset_approvals_for_merge_requests
-      merge_requests = merge_requests_for(@branch_name, mr_states: [:opened, :closed])
-
-      merge_requests.each do |merge_request|
-        target_project = merge_request.target_project
-
-        if target_project.approvals_before_merge.nonzero? &&
-            target_project.reset_approvals_on_push &&
-            merge_request.rebase_commit_sha != @newrev
-
-          merge_request.approvals.delete_all
-        end
-      end
-    end
-
     def reset_merge_when_pipeline_succeeds
       merge_requests_for_source_branch.each(&:reset_merge_when_pipeline_succeeds)
     end

app/services/merge_requests/update_service.rb

@@ -19,19 +19,8 @@ module MergeRequests
         merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)
       end
 
-      old_approvers = merge_request.overall_approvers.to_a
-
       handle_wip_event(merge_request)
       update(merge_request)
-
-      new_approvers = merge_request.overall_approvers.to_a - old_approvers
-
-      if new_approvers.any?
-        todo_service.add_merge_request_approvers(merge_request, new_approvers)
-        notification_service.add_merge_request_approvers(merge_request, new_approvers, current_user)
-      end
-
-      merge_request
     end
 
     # rubocop:disable Metrics/AbcSize
@@ -53,8 +42,6 @@ module MergeRequests
         create_branch_change_note(merge_request, 'target',
                                   merge_request.previous_changes['target_branch'].first,
                                   merge_request.target_branch)
-
-        reset_approvals(merge_request)
       end
 
       if merge_request.previous_changes.include?('assignee_id')
@@ -118,14 +105,6 @@ module MergeRequests
 
     private
 
-    def reset_approvals(merge_request)
-      target_project = merge_request.target_project
-
-      if target_project.approvals_before_merge.nonzero? && target_project.reset_approvals_on_push
-        merge_request.approvals.delete_all
-      end
-    end
-
     def create_branch_change_note(issuable, branch_type, old_branch, new_branch)
       SystemNoteService.change_branch(
         issuable, issuable.project, current_user, branch_type,

ee/app/presenters/ee/merge_request_presenter.rb

+module EE
+  module MergeRequestPresenter
+    def approvals_path
+      if requires_approve?
+        approvals_project_merge_request_path(project, merge_request)
+      end
+    end
+  end
+end

ee/app/services/ee/merge_requests/merge_service.rb

@@ -23,6 +23,33 @@ module EE
         end
       end
 
+      def hooks_validation_pass?(merge_request)
+        # handle_merge_error needs this. We should move that to a separate
+        # object instead of relying on the order of method calls.
+        @merge_request = merge_request # rubocop:disable Gitlab/ModuleWithInstanceVariables
+
+        return true if project.merge_requests_ff_only_enabled
+        return true unless project.feature_available?(:push_rules)
+
+        push_rule = merge_request.project.push_rule
+        return true unless push_rule
+
+        unless push_rule.commit_message_allowed?(params[:commit_message])
+          handle_merge_error(log_message: "Commit message does not follow the pattern '#{push_rule.commit_message_regex}'", save_message_on_model: true)
+          return false
+        end
+
+        unless push_rule.author_email_allowed?(current_user.email)
+          handle_merge_error(log_message: "Commit author's email '#{current_user.email}' does not follow the pattern '#{push_rule.author_email_regex}'", save_message_on_model: true)
+          return false
+        end
+
+        true
+      rescue PushRule::MatchError => e
+        handle_merge_error(log_message: e.message, save_message_on_model: true)
+        false
+      end
+
       private
 
       def check_size_limit

ee/app/services/ee/merge_requests/refresh_service.rb

+module EE
+  module MergeRequests
+    module RefreshService
+      extend ::Gitlab::Utils::Override
+
+      private
+
+      override :do_execute
+      def do_execute(oldrev, newrev, ref)
+        super && reset_approvals_for_merge_requests(ref, newrev)
+      end
+
+      # Note: Closed merge requests also need approvals reset.
+      def reset_approvals_for_merge_requests(ref, newrev)
+        branch_name = ::Gitlab::Git.ref_name(ref)
+        merge_requests = merge_requests_for(branch_name, mr_states: [:opened, :closed])
+
+        merge_requests.each do |merge_request|
+          target_project = merge_request.target_project
+
+          if target_project.approvals_before_merge.nonzero? &&
+              target_project.reset_approvals_on_push &&
+              merge_request.rebase_commit_sha != newrev
+
+            merge_request.approvals.delete_all
+          end
+        end
+      end
+    end
+  end
+end

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

@@ -8,13 +8,40 @@ module EE
       override :execute
       def execute(merge_request)
         should_remove_old_approvers = params.delete(:remove_old_approvers)
+        old_approvers = merge_request.overall_approvers.to_a
 
         merge_request = super(merge_request)
 
-        cleanup_approvers(merge_request, reload: true) if should_remove_old_approvers && merge_request.valid?
+        new_approvers = merge_request.overall_approvers.to_a - old_approvers
+
+        if new_approvers.any?
+          todo_service.add_merge_request_approvers(merge_request, new_approvers)
+          notification_service.add_merge_request_approvers(merge_request, new_approvers, current_user)
+        end
+
+        if should_remove_old_approvers && merge_request.valid?
+          cleanup_approvers(merge_request, reload: true)
+        end
 
         merge_request
       end
+
+      override :create_branch_change_note
+      def create_branch_change_note(merge_request, branch_type, old_branch, new_branch)
+        super
+
+        reset_approvals(merge_request)
+      end
+
+      private
+
+      def reset_approvals(merge_request)
+        target_project = merge_request.target_project
+
+        if target_project.approvals_before_merge.nonzero? && target_project.reset_approvals_on_push
+          merge_request.approvals.delete_all
+        end
+      end
     end
   end
 end

spec/workers/update_merge_requests_worker_spec.rb

@@ -18,8 +18,13 @@ describe UpdateMergeRequestsWorker do
     end
 
     it 'executes MergeRequests::RefreshService with expected values' do
-      expect(MergeRequests::RefreshService).to receive(:new).with(project, user).and_call_original
-      expect_any_instance_of(MergeRequests::RefreshService).to receive(:execute).with(oldrev, newrev, ref)
+      expect(MergeRequests::RefreshService).to receive(:new)
+        .with(project, user).and_wrap_original do |method, *args|
+          method.call(*args).tap do |refresh_service|
+            expect(refresh_service)
+              .to receive(:execute).with(oldrev, newrev, ref)
+          end
+        end
 
       perform
     end