Merge branch '24795_refactor_merge_request_build_service' into 'master'

Refactor MergeRequest::BuildService

Closes #24795

See merge request !8462

app/services/merge_requests/build_service.rb

 module MergeRequests
   class BuildService < MergeRequests::BaseService
     def execute
-      merge_request = MergeRequest.new(params)
-
-      # Set MR attributes
+      self.merge_request = MergeRequest.new(params)
       merge_request.can_be_created  = true
       merge_request.compare_commits = []
-      merge_request.source_project = project unless merge_request.source_project
+      merge_request.source_project  = find_source_project
+      merge_request.target_project  = find_target_project
+      merge_request.target_branch   = find_target_branch
 
-      merge_request.target_project = nil unless can?(current_user, :read_project, merge_request.target_project)
+      if branches_specified? && branches_valid?
+        compare_branches
+        assign_title_and_description
+      else
+        merge_request.can_be_created = false
+      end
 
-      merge_request.target_project ||= (project.forked_from_project || project)
-      merge_request.target_branch ||= merge_request.target_project.default_branch
+      merge_request
+    end
 
-      messages = validate_branches(merge_request)
-      return build_failed(merge_request, messages) unless messages.empty?
+    private
 
-      compare = CompareService.new.execute(
-        merge_request.source_project,
-        merge_request.source_branch,
-        merge_request.target_project,
-        merge_request.target_branch,
-      )
+    attr_accessor :merge_request
 
-      merge_request.compare_commits = compare.commits
-      merge_request.compare = compare
+    delegate :target_branch, :source_branch, :source_project, :target_project, :compare_commits, :wip_title, :description, :errors, to: :merge_request
 
-      set_title_and_description(merge_request)
+    def find_source_project
+      source_project || project
     end
 
-    private
+    def find_target_project
+      return target_project if target_project.present? && can?(current_user, :read_project, target_project)
+      project.forked_from_project || project
+    end
 
-    def validate_branches(merge_request)
-      messages = []
+    def find_target_branch
+      target_branch || target_project.default_branch
+    end
 
-      if merge_request.target_branch.blank? || merge_request.source_branch.blank?
-        messages <<
-          if params[:source_branch] || params[:target_branch]
-            "You must select source and target branch"
+    def branches_specified?
+      params[:source_branch] && params[:target_branch]
     end
+
+    def branches_valid?
+      validate_branches
+      errors.blank?
     end
 
-      if merge_request.source_project == merge_request.target_project &&
-          merge_request.target_branch == merge_request.source_branch
+    def compare_branches
+      compare = CompareService.new.execute(
+        source_project,
+        source_branch,
+        target_project,
+        target_branch
+      )
 
-        messages << 'You must select different branches'
+      merge_request.compare_commits = compare.commits
+      merge_request.compare = compare
+    end
+
+    def validate_branches
+      add_error('You must select source and target branch') unless branches_present?
+      add_error('You must select different branches') if same_source_and_target?
+      add_error("Source branch \"#{source_branch}\" does not exist") unless source_branch_exists?
+      add_error("Target branch \"#{target_branch}\" does not exist") unless target_branch_exists?
+    end
+
+    def add_error(message)
+      errors.add(:base, message)
+    end
+
+    def branches_present?
+      target_branch.present? && source_branch.present?
     end
 
-      # See if source and target branches exist
-      if merge_request.source_branch.present? && !merge_request.source_project.commit(merge_request.source_branch)
-        messages << "Source branch \"#{merge_request.source_branch}\" does not exist"
+    def same_source_and_target?
+      source_project == target_project && target_branch == source_branch
     end
 
-      if merge_request.target_branch.present? && !merge_request.target_project.commit(merge_request.target_branch)
-        messages << "Target branch \"#{merge_request.target_branch}\" does not exist"
+    def source_branch_exists?
+      source_branch.blank? || source_project.commit(source_branch)
     end
 
-      messages
+    def target_branch_exists?
+      target_branch.blank? || target_project.commit(target_branch)
     end
 
     # When your branch name starts with an iid followed by a dash this pattern will be
@@ -71,17 +97,17 @@ module MergeRequests
     # - Setting the title as 'Resolves "Emoji don't show up in commit title"' if there is
     #   more than one commit in the MR
     #
-    def set_title_and_description(merge_request)
-      if match = merge_request.source_branch.match(/\A(\d+)-/)
+    def assign_title_and_description
+      if match = source_branch.match(/\A(\d+)-/)
         iid = match[1]
       end
 
-      commits = merge_request.compare_commits
+      commits = compare_commits
       if commits && commits.count == 1
         commit = commits.first
         merge_request.title = commit.title
         merge_request.description ||= commit.description.try(:strip)
-      elsif iid && issue = merge_request.target_project.get_issue(iid, current_user)
+      elsif iid && issue = target_project.get_issue(iid, current_user)
         case issue
         when Issue
           merge_request.title = "Resolve \"#{issue.title}\""
@@ -89,31 +115,20 @@ module MergeRequests
           merge_request.title = "Resolve #{issue.title}"
         end
       else
-        merge_request.title = merge_request.source_branch.titleize.humanize
+        merge_request.title = source_branch.titleize.humanize
       end
 
       if iid
         closes_issue = "Closes ##{iid}"
 
-        if merge_request.description.present?
+        if description.present?
           merge_request.description += closes_issue.prepend("\n\n")
         else
           merge_request.description = closes_issue
         end
       end
 
-      merge_request.title = merge_request.wip_title if commits.empty?
-
-      merge_request
-    end
-
-    def build_failed(merge_request, messages)
-      messages.compact.each do |message|
-        merge_request.errors.add(:base, message)
-      end
-      merge_request.compare_commits = []
-      merge_request.can_be_created = false
-      merge_request
+      merge_request.title = wip_title if commits.empty?
     end
   end
 end

changelogs/unreleased/24795_refactor_merge_request_build_service.yml

+---
+title: Refactor MergeRequests::BuildService
+merge_request: 8462
+author: Rydkin Maxim

spec/features/merge_requests/user_uses_slash_commands_spec.rb

@@ -11,7 +11,7 @@ feature 'Merge Requests > User uses slash commands', feature: true, js: true do
 
   it_behaves_like 'issuable record that supports slash commands in its description and notes', :merge_request do
     let(:issuable) { create(:merge_request, source_project: project) }
-    let(:new_url_opts) { { merge_request: { source_branch: 'feature' } } }
+    let(:new_url_opts) { { merge_request: { source_branch: 'feature', target_branch: 'master' } } }
   end
 
   describe 'merge-request-only commands' do