Merge remote-tracking branch 'upstream/master' into fix-git-hooks-when-creating-filefix-git-hooks-when-creating-file

* upstream/master: (190 commits)
  Remove unnecessary returns / unset variables from the CoffeeScript -> JS conversion.
  update spec
  Change the reply shortcut to focus the field even without a selection.
  use destroy_all
  Remove settings cog from within admin scroll tabs; keep links centered
  add changelog
  remove old project members from project
  add spec replicating validation error
  Fix small typo on new branch button spec
  Improve styling of the new issue message
  Don't capitalize environment name in show page
  Abillity to promote project labels to group labels
  Edited the column header for the environments list from created to updated and added created to environments detail page colum header titles
  Update and pin the `jwt` gem to ~> 1.5.6
  refactor merge request build service
  Update index.md
  Clarify that Auto Deploy requires a public project.
  19164 Add settings dropdown to mobile screens
  cop for gem fetched from a git source
  Add CHANGELOG entry
  ...

1 files changed, 72 insertions, 57 deletions

diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index 548c7b9baf4..f4d52e3ebbd 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -1,63 +1,89 @@
 module MergeRequests
   class BuildService < MergeRequests::BaseService
     def execute
-      merge_request = MergeRequest.new(params)
-
-      # Set MR attributes
-      merge_request.can_be_created = true
+      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
+
+      if branches_specified? && branches_valid?
+        compare_branches
+        assign_title_and_description
+      else
+        merge_request.can_be_created = false
+      end
+
+      merge_request
+    end
 
-      merge_request.target_project = nil unless can?(current_user, :read_project, merge_request.target_project)
+    private
 
-      merge_request.target_project ||= (project.forked_from_project || project)
-      merge_request.target_branch ||= merge_request.target_project.default_branch
+    attr_accessor :merge_request
 
-      messages = validate_branches(merge_request)
-      return build_failed(merge_request, messages) unless messages.empty?
+    delegate :target_branch, :source_branch, :source_project, :target_project, :compare_commits, :wip_title, :description, :errors, to: :merge_request
+
+    def find_source_project
+      source_project || project
+    end
 
+    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 find_target_branch
+      target_branch || target_project.default_branch
+    end
+
+    def branches_specified?
+      params[:source_branch] && params[:target_branch]
+    end
+
+    def branches_valid?
+      validate_branches
+      errors.blank?
+    end
+
+    def compare_branches
       compare = CompareService.new(
-        merge_request.source_project,
-        merge_request.source_branch
+        source_project,
+        source_branch
       ).execute(
-        merge_request.target_project,
-        merge_request.target_branch,
+        target_project,
+        target_branch
       )
 
       merge_request.compare_commits = compare.commits
       merge_request.compare = compare
-
-      set_title_and_description(merge_request)
     end
 
-    private
-
-    def validate_branches(merge_request)
-      messages = []
-
-      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"
-          end
-      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
 
-      if merge_request.source_project == merge_request.target_project &&
-          merge_request.target_branch == merge_request.source_branch
+    def add_error(message)
+      errors.add(:base, message)
+    end
 
-        messages << 'You must select different branches'
-      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"
-      end
+    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"
-      end
+    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
@@ -72,17 +98,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}\""
@@ -90,31 +116,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