Refactor PushOptionsHandlerService from review

Exceptions are no longer raised, instead all errors encountered are
added to the errors property.

MergeRequests::BuildService is used to generate attributes of a new
merge request.

Code moved from Api::Internal to Api::Helpers::InternalHelpers.

app/services/merge_requests/push_options_handler_service.rb

@@ -2,39 +2,28 @@
 
 module MergeRequests
   class PushOptionsHandlerService
-    Error = Class.new(StandardError)
-
     LIMIT = 10
 
     attr_reader :branches, :changes_by_branch, :current_user, :errors,
-                :project, :push_options
+                :project, :push_options, :target_project
 
     def initialize(project, current_user, changes, push_options)
       @project = project
+      @target_project = @project.default_merge_request_target
       @current_user = current_user
-      @changes_by_branch = parse_changes(changes)
+      @branches = get_branches(changes)
       @push_options = push_options
       @errors = []
-      @branches = @changes_by_branch.keys
-
-      raise Error, 'User is required' if @current_user.nil?
-
-      unless @project.merge_requests_enabled?
-        raise Error, 'Merge requests are not enabled for project'
-      end
-
-      if @branches.size > LIMIT
-        raise Error, "Too many branches pushed (#{@branches.size} were pushed, limit is #{LIMIT})"
-      end
-
-      if @push_options[:target] && !@project.repository.branch_exists?(@push_options[:target])
-        raise Error, "Branch #{@push_options[:target]} does not exist"
-      end
     end
 
     def execute
+      validate_service
+      return self if errors.present?
+
       branches.each do |branch|
         execute_for_branch(branch)
+      rescue Gitlab::Access::AccessDeniedError
+        errors << 'User access was denied'
       end
 
       self
@@ -42,32 +31,47 @@ module MergeRequests
 
     private
 
-    # Parses changes in the push.
-    # Returns a hash of branch => changes_list
-    def parse_changes(raw_changes)
-      Gitlab::ChangesList.new(raw_changes).each_with_object({}) do |change, changes|
-        next unless Gitlab::Git.branch_ref?(change[:ref])
+    def get_branches(raw_changes)
+      Gitlab::ChangesList.new(raw_changes).map do |changes|
+        next unless Gitlab::Git.branch_ref?(changes[:ref])
 
         # Deleted branch
-        next if Gitlab::Git.blank_ref?(change[:newrev])
+        next if Gitlab::Git.blank_ref?(changes[:newrev])
 
         # Default branch
-        branch_name = Gitlab::Git.branch_name(change[:ref])
-        next if branch_name == project.default_branch
+        branch_name = Gitlab::Git.branch_name(changes[:ref])
+        next if branch_name == target_project.default_branch
+
+        branch_name
+      end.compact.uniq
+    end
 
-        changes[branch_name] = change
+    def validate_service
+      errors << 'User is required' if current_user.nil?
+
+      unless target_project.merge_requests_enabled?
+        errors << "Merge requests are not enabled for project #{target_project.full_path}"
+      end
+
+      if branches.size > LIMIT
+        errors << "Too many branches pushed (#{branches.size} were pushed, limit is #{LIMIT})"
+      end
+
+      if push_options[:target] && !target_project.repository.branch_exists?(push_options[:target])
+        errors << "Branch #{push_options[:target]} does not exist"
       end
     end
 
+    # Returns a Hash of branch => MergeRequest
     def merge_requests
-      @merge_requests ||= MergeRequest.from_project(@project)
+      @merge_requests ||= MergeRequest.from_project(target_project)
                                       .opened
-                                      .from_source_branches(@branches)
-                                      .to_a # fetch now
+                                      .from_source_branches(branches)
+                                      .index_by(&:source_branch)
     end
 
     def execute_for_branch(branch)
-      merge_request = merge_requests.find { |mr| mr.source_branch == branch }
+      merge_request = merge_requests[branch]
 
       if merge_request
         update!(merge_request)
@@ -82,18 +86,27 @@ module MergeRequests
         return
       end
 
-      merge_request = ::MergeRequests::CreateService.new(
+      # Use BuildService to assign the standard attributes of a merge request
+      merge_request = ::MergeRequests::BuildService.new(
         project,
         current_user,
         create_params(branch)
       ).execute
 
+      unless merge_request.errors.present?
+        merge_request = ::MergeRequests::CreateService.new(
+          project,
+          current_user,
+          merge_request.attributes
+        ).execute
+      end
+
       collect_errors_from_merge_request(merge_request) unless merge_request.persisted?
     end
 
     def update!(merge_request)
       merge_request = ::MergeRequests::UpdateService.new(
-        project,
+        target_project,
         current_user,
         update_params
       ).execute(merge_request)
@@ -102,18 +115,12 @@ module MergeRequests
     end
 
     def create_params(branch)
-      change = changes_by_branch.fetch(branch)
-
-      commits = project.repository.commits_between(project.default_branch, change[:newrev])
-      commits = CommitCollection.new(project, commits)
-      commit = commits.without_merge_commits.first
-
       params = {
         assignee: current_user,
         source_branch: branch,
-        target_branch: push_options[:target] || project.default_branch,
-        title: commit&.title&.strip || 'New Merge Request',
-        description: commit&.description&.strip
+        source_project: project,
+        target_branch: push_options[:target] || target_project.default_branch,
+        target_project: target_project
       }
 
       if push_options.key?(:merge_when_pipeline_succeeds)
@@ -144,7 +151,9 @@ module MergeRequests
     end
 
     def collect_errors_from_merge_request(merge_request)
-      errors << merge_request.errors.full_messages.to_sentence
+      merge_request.errors.full_messages.each do |error|
+        errors << error
+      end
     end
   end
 end

lib/api/helpers/internal_helpers.rb

@@ -43,6 +43,23 @@ module API
         ::MergeRequests::GetUrlsService.new(project).execute(params[:changes])
       end
 
+      def process_mr_push_options(push_options, project, user, changes)
+        output = {}
+
+        service = ::MergeRequests::PushOptionsHandlerService.new(
+          project,
+          user,
+          changes,
+          push_options
+        ).execute
+
+        if service.errors.present?
+          output[:warnings] = push_options_warning(service.errors.join("\n\n"))
+        end
+
+        output
+      end
+
       def push_options_warning(warning)
         options = Array.wrap(params[:push_options]).map { |p| "'#{p}'" }.join(' ')
         "Error encountered with push options #{options}: #{warning}"

lib/api/internal.rb

@@ -264,24 +264,8 @@ module API
         PostReceive.perform_async(params[:gl_repository], params[:identifier],
           params[:changes], push_options.as_json)
 
-        if (mr_options = push_options.get(:merge_request))
-          begin
-            service = ::MergeRequests::PushOptionsHandlerService.new(
-              project,
-              user,
-              params[:changes],
-              mr_options
-            ).execute
-
-            if service.errors.present?
-              output[:warnings] = push_options_warning(service.errors.join("\n\n"))
-            end
-          rescue ::MergeRequests::PushOptionsHandlerService::Error => e
-            output[:warnings] = push_options_warning(e.message)
-          rescue Gitlab::Access::AccessDeniedError
-            output[:warnings] = push_options_warning('User access was denied')
-          end
-        end
+        mr_options = push_options.get(:merge_request)
+        output.merge!(process_mr_push_options(mr_options, project, user, params[:changes])) if mr_options.present?
 
         broadcast_message = BroadcastMessage.current&.last&.message
         reference_counter_decreased = Gitlab::ReferenceCounter.new(params[:gl_repository]).decrease

spec/requests/api/internal_spec.rb

@@ -888,8 +888,10 @@ describe API::Internal do
       }
     end
 
+    let(:branch_name) { 'feature' }
+
     let(:changes) do
-      "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch"
+      "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{branch_name}"
     end
 
     let(:push_options) do
@@ -924,8 +926,8 @@ describe API::Internal do
       post api('/internal/post_receive'), params: valid_params
 
       expect(json_response['merge_request_urls']).to match [{
-        "branch_name" => "new_branch",
-        "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
+        "branch_name" => branch_name,
+        "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name}",
         "new_merge_request" => true
       }]
     end
@@ -955,26 +957,22 @@ describe API::Internal do
         post api('/internal/post_receive'), params: valid_params
       end
 
+      it 'creates a new merge request' do
+        expect do
+          post api('/internal/post_receive'), params: valid_params
+        end.to change { MergeRequest.count }.by(1)
+      end
+
       it 'links to the newly created merge request' do
         post api('/internal/post_receive'), params: valid_params
 
         expect(json_response['merge_request_urls']).to match [{
-          'branch_name' => 'new_branch',
+          'branch_name' => branch_name,
           'url' => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/1",
           'new_merge_request' => false
         }]
       end
 
-      it 'adds errors raised from MergeRequests::PushOptionsHandlerService to warnings' do
-        expect(MergeRequests::PushOptionsHandlerService).to receive(:new).and_raise(
-          MergeRequests::PushOptionsHandlerService::Error, 'my warning'
-        )
-
-        post api('/internal/post_receive'), params: valid_params
-
-        expect(json_response['warnings']).to eq('Error encountered with push options \'merge_request.create\': my warning')
-      end
-
       it 'adds errors on the service instance to warnings' do
         expect_any_instance_of(
           MergeRequests::PushOptionsHandlerService

spec/services/merge_requests/push_options_handler_service_spec.rb

@@ -3,10 +3,13 @@
 require 'spec_helper'
 
 describe MergeRequests::PushOptionsHandlerService do
+  include ProjectForksHelper
+
   let(:user) { create(:user) }
   let(:project) { create(:project, :public, :repository) }
+  let(:forked_project) { fork_project(project, user, repository: true) }
   let(:service) { described_class.new(project, user, changes, push_options) }
-  let(:source_branch) { 'test' }
+  let(:source_branch) { 'fix' }
   let(:target_branch) { 'feature' }
   let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" }
   let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" }
@@ -38,21 +41,25 @@ describe MergeRequests::PushOptionsHandlerService do
       expect(last_mr.assignee).to eq(user)
     end
 
-    it 'sets the title and description from the first non-merge commit' do
-      commits = project.repository.commits('master', limit: 5)
+    context 'when project has been forked' do
+      let(:forked_project) { fork_project(project, user, repository: true) }
+      let(:service) { described_class.new(forked_project, user, changes, push_options) }
 
-      expect(Gitlab::Git::Commit).to receive(:between).at_least(:once).and_return(commits)
+      before do
+        allow(forked_project).to receive(:empty_repo?).and_return(false)
+      end
 
-      service.execute
+      it 'sets the correct source project' do
+        service.execute
 
-      merge_commit = commits.first
-      non_merge_commit = commits.second
+        expect(last_mr.source_project).to eq(forked_project)
+      end
 
-      expect(merge_commit.merge_commit?).to eq(true)
-      expect(non_merge_commit.merge_commit?).to eq(false)
+      it 'sets the correct target project' do
+        service.execute
 
-      expect(last_mr.title).to eq(non_merge_commit.title)
-      expect(last_mr.description).to eq(non_merge_commit.description)
+        expect(last_mr.target_project).to eq(project)
+      end
     end
   end
 
@@ -271,7 +278,7 @@ describe MergeRequests::PushOptionsHandlerService do
     let(:changes) do
       [
         new_branch_changes,
-        "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/second-branch"
+        "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/feature_conflict"
       ]
     end
 
@@ -287,11 +294,10 @@ describe MergeRequests::PushOptionsHandlerService do
         end
       end
 
-      it 'throws an error' do
-        expect { service.execute }.to raise_error(
-          MergeRequests::PushOptionsHandlerService::Error,
-          "Too many branches pushed (#{limit + 1} were pushed, limit is #{limit})"
-        )
+      it 'records an error' do
+        service.execute
+
+        expect(service.errors).to eq(["Too many branches pushed (#{limit + 1} were pushed, limit is #{limit})"])
       end
     end
   end
@@ -308,11 +314,10 @@ describe MergeRequests::PushOptionsHandlerService do
     let(:push_options) { { create: true } }
     let(:changes) { new_branch_changes }
 
-    it 'throws an error' do
-      expect { service.execute }.to raise_error(
-        MergeRequests::PushOptionsHandlerService::Error,
-        'User is required'
-      )
+    it 'records an error' do
+      service.execute
+
+      expect(service.errors).to eq(['User is required'])
     end
   end
 
@@ -320,10 +325,12 @@ describe MergeRequests::PushOptionsHandlerService do
     let(:push_options) { { create: true } }
     let(:changes) { new_branch_changes }
 
-    it 'throws an error' do
+    it 'records an error' do
       Members::DestroyService.new(user).execute(ProjectMember.find_by!(user_id: user.id))
 
-      expect { service.execute }.to raise_error(Gitlab::Access::AccessDeniedError)
+      service.execute
+
+      expect(service.errors).to eq(['User access was denied'])
     end
   end
 
@@ -331,11 +338,10 @@ describe MergeRequests::PushOptionsHandlerService do
     let(:push_options) { { create: true, target: 'my-branch' } }
     let(:changes) { new_branch_changes }
 
-    it 'throws an error' do
-      expect { service.execute }.to raise_error(
-        MergeRequests::PushOptionsHandlerService::Error,
-        'Branch my-branch does not exist'
-      )
+    it 'records an error' do
+      service.execute
+
+      expect(service.errors).to eq(['Branch my-branch does not exist'])
     end
   end
 
@@ -343,13 +349,12 @@ describe MergeRequests::PushOptionsHandlerService do
     let(:push_options) { { create: true } }
     let(:changes) { new_branch_changes }
 
-    it 'throws an error' do
+    it 'records an error' do
       expect(project).to receive(:merge_requests_enabled?).and_return(false)
 
-      expect { service.execute }.to raise_error(
-        MergeRequests::PushOptionsHandlerService::Error,
-        'Merge requests are not enabled for project'
-      )
+      service.execute
+
+      expect(service.errors).to eq(["Merge requests are not enabled for project #{project.full_path}"])
     end
   end