Refactor MR build process

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

7 files changed, 169 insertions, 116 deletions

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index bdd614973b7..7f9c238507e 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -60,53 +60,27 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def new
-    params[:merge_request] ||= ActionController::Parameters.new(
-      source_project: @project
-    )
-
-    @merge_request = MergeRequest.new(merge_request_params)
-    @merge_request.source_project = @project unless @merge_request.source_project
-    @merge_request.target_project ||= (@project.forked_from_project || @project)
-    @target_branches = @merge_request.target_project.nil? ? [] : @merge_request.target_project.repository.branch_names
-    @merge_request.target_branch ||= @merge_request.target_project.default_branch
-    @source_project = @merge_request.source_project
-
-    if @merge_request.target_branch && @merge_request.source_branch
-      compare_action = Gitlab::Satellite::CompareAction.new(
-        current_user,
-        @merge_request.target_project,
-        @merge_request.target_branch,
-        @merge_request.source_project,
-        @merge_request.source_branch
-      )
-
-      @compare_failed = false
-      @commits = compare_action.commits
-
-      if @commits
-        @commits.map! { |commit| Commit.new(commit) }
-        @commit = @commits.first
-      else
-        # false value because failed to get commits from satellite
-        @commits = []
-        @compare_failed = true
-      end
-
-      @note_counts = Note.where(commit_id: @commits.map(&:id)).
-          group(:commit_id).count
-
-      begin
-        @diffs = compare_action.diffs
-        @merge_request.title = @merge_request.source_branch.titleize.humanize
-        @target_project = @merge_request.target_project
-        @target_repo = @target_project.repository
-
-        diff_line_count = Commit::diff_line_count(@diffs)
-        @suppress_diff = Commit::diff_suppress?(@diffs, diff_line_count)
-        @force_suppress_diff = @suppress_diff
-      rescue Gitlab::Satellite::BranchesWithoutParent
-        @error = "Selected branches have no common commit so they cannot be merged."
-      end
+    params[:merge_request] ||= ActionController::Parameters.new(source_project: @project)
+    @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute
+
+    @target_branches = if @merge_request.target_project
+                         @merge_request.target_project.repository.branch_names
+                       else
+                         []
+                       end
+
+    @target_project = merge_request.target_project
+    @source_project = merge_request.source_project
+    @commits = @merge_request.compare_commits
+    @commit = @merge_request.compare_base_commit
+    @diffs = @merge_request.compare_diffs
+    @note_counts = Note.where(commit_id: @commits.map(&:id)).
+      group(:commit_id).count
+
+    if @diffs.any?
+      @diff_line_count = Commit::diff_line_count(@diffs)
+      @suppress_diff = Commit::diff_suppress?(@diffs, @diff_line_count)
+      @force_suppress_diff = @suppress_diff
     end
   end
 
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index ba4c7068e90..bc2ec84302d 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -221,4 +221,10 @@ module ProjectsHelper
       "Never"
     end
   end
+
+  def contribution_guide_url(project)
+    if project && project.repository.contribution_guide
+      project_blob_path(project, tree_join(project.default_branch, project.repository.contribution_guide.name))
+    end
+  end
 end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 597e02d498b..530e1add8e7 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -42,6 +42,10 @@ class MergeRequest < ActiveRecord::Base
   # It allows us to close or modify broken merge requests
   attr_accessor :allow_broken
 
+  # Temporary fields to store compare vars
+  # when creating new merge request
+  attr_accessor :can_be_created, :compare_failed, :compare_base_commit, :compare_commits, :compare_diffs
+
   ActsAsTaggableOn.strict_case_match = true
   acts_as_taggable_on :labels
 
diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
new file mode 100644
index 00000000000..9c28d3d82d8
--- /dev/null
+++ b/app/services/merge_requests/build_service.rb
@@ -0,0 +1,69 @@
+module MergeRequests
+  class BuildService < MergeRequests::BaseService
+    def execute
+      merge_request = MergeRequest.new(params)
+
+      # Set MR attributes
+      merge_request.can_be_created = false
+      merge_request.compare_failed = false
+      merge_request.compare_commits = []
+      merge_request.compare_diffs = []
+      merge_request.source_project = project unless merge_request.source_project
+      merge_request.target_project ||= (project.forked_from_project || project)
+      merge_request.target_branch ||= merge_request.target_project.default_branch
+
+      unless merge_request.target_branch && merge_request.source_branch
+        return build_failed(merge_request, "You must select source and target branches")
+      end
+
+      # Generate suggested MR title based on source branch name
+      merge_request.title = merge_request.source_branch.titleize.humanize
+
+      # Try to compare branches to get commits list and diffs
+      compare_action = Gitlab::Satellite::CompareAction.new(
+        current_user,
+        merge_request.target_project,
+        merge_request.target_branch,
+        merge_request.source_project,
+        merge_request.source_branch
+      )
+
+      commits = compare_action.commits
+
+      # At this point we decide if merge request can be created
+      # If we have at least one commit to merge -> creation allowed
+      if commits.present?
+        merge_request.compare_commits = Commit.decorate(commits)
+        merge_request.compare_base_commit = Commit.new(commits.first)
+        merge_request.can_be_created = true
+        merge_request.compare_failed = false
+
+        # Try to collect diff for merge request.
+        # Note: even if diff is huge and we can't show it - we still should allow
+        # people to create MR.
+        diffs = compare_action.diffs
+
+        if diffs.present?
+          merge_request.compare_diffs = diffs
+        end
+      else
+        merge_request.can_be_created = false
+        merge_request.compare_failed = true
+      end
+
+      merge_request
+
+    rescue Gitlab::Satellite::BranchesWithoutParent
+      return build_failed(merge_request, "Selected branches have no common commit so they cannot be merged.")
+    #rescue
+      #return build_failed(merge_request, "We cannot create merge request because of huge diff.")
+    end
+
+    def build_failed(merge_request, message)
+      merge_request.errors.add(:base, message)
+      merge_request.compare_commits = []
+      merge_request.can_be_created = false
+      merge_request
+    end
+  end
+end
diff --git a/app/views/projects/merge_requests/_new_compare.html.haml b/app/views/projects/merge_requests/_new_compare.html.haml
index 18e3f419c72..76b5db419f7 100644
--- a/app/views/projects/merge_requests/_new_compare.html.haml
+++ b/app/views/projects/merge_requests/_new_compare.html.haml
@@ -27,12 +27,12 @@
         .panel-footer
           .mr_target_commit
 
-  -if @merge_request.errors.any?
+  - if @merge_request.errors.any?
     .alert.alert-danger
       - @merge_request.errors.full_messages.each do |msg|
         %div= msg
 
-  - if @merge_request.source_branch.present? && @merge_request.target_branch.present?
+  - elsif @merge_request.source_branch.present? && @merge_request.target_branch.present?
     - if @compare_failed
       .alert.alert-danger
         %h4 Compare failed
diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml
index 6050f20d763..73d364b4f93 100644
--- a/app/views/projects/merge_requests/_new_submit.html.haml
+++ b/app/views/projects/merge_requests/_new_submit.html.haml
@@ -10,75 +10,75 @@
 
   %span.pull-right
     = link_to 'Change branches', new_project_merge_request_path(@project)
-- if @error.present?
-  .centered-light-block
-    %h4 #{@error}
-- else
-  = form_for [@project, @merge_request], html: { class: "merge-request-form" } do |f|
-    .panel.panel-default
 
-      .panel-body
-        .form-group
-          .light
-            = f.label :title do
-              = "Title *"
-          = f.text_field :title, class: "form-control input-lg js-gfm-input", maxlength: 255, rows: 5, required: true
-        .form-group
-          .light
-            = f.label :description, "Description"
-          = f.text_area :description, class: "form-control js-gfm-input markdown-area", rows: 10
-          .clearfix.hint
-            .pull-left Description is parsed with #{link_to "GitLab Flavored Markdown", help_page_path("markdown", "markdown"), target: '_blank'}.
-            .pull-right Attach images (JPG, PNG, GIF) by dragging & dropping or #{link_to "selecting them", '#', class: 'markdown-selector' }.
-          .error-alert
-        .form-group
-          .issue-assignee
-            = f.label :assignee_id do
-              %i.icon-user
-              Assign to
-            %div
-              = project_users_select_tag('merge_request[assignee_id]', placeholder: 'Select a user', class: 'custom-form-control', selected: @merge_request.assignee_id, project_id: @merge_request.target_project_id)
-              &nbsp;
-              = link_to 'Assign to me', '#', class: 'btn assign-to-me-link'
-        .form-group
-          .issue-milestone
-            = f.label :milestone_id do
-              %i.icon-time
-              Milestone
-            %div= f.select(:milestone_id, milestone_options(@merge_request), { include_blank: "Select milestone" }, {class: 'select2'})
-      .panel-footer
-        - if @target_repo.contribution_guide
-          - contribution_guide_url = project_blob_path(@target_project, tree_join(@target_repo.root_ref, @target_repo.contribution_guide.name))
-          %p
-            Please review the
-            %strong #{link_to "guidelines for contribution", contribution_guide_url}
-            to this repository.
-        = f.hidden_field :source_project_id
-        = f.hidden_field :target_project_id
-        = f.hidden_field :target_branch
-        = f.hidden_field :source_branch
-        = f.submit 'Submit merge request', class: "btn btn-create"
+= form_for [@project, @merge_request], html: { class: "merge-request-form" } do |f|
+  .panel.panel-default
 
-  .mr-compare
-    %div.panel.panel-default
-      .panel-heading
-        Commits (#{@commits.count})
-      - if @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
-        %ul.well-list
-          - Commit.decorate(@commits.first(MergeRequestDiff::COMMITS_SAFE_SIZE)).each do |commit|
-            = render "projects/commits/inline_commit", commit: commit, project: @project
-          %li.warning-row.unstyled
-            other #{@commits.size - MergeRequestDiff::COMMITS_SAFE_SIZE} commits hidden to prevent performance issues.
-      - else
-        %ul.well-list= render Commit.decorate(@commits), project: @project
+    .panel-body
+      .form-group
+        .light
+          = f.label :title do
+            = "Title *"
+        = f.text_field :title, class: "form-control input-lg js-gfm-input", maxlength: 255, rows: 5, required: true
+      .form-group
+        .light
+          = f.label :description, "Description"
+        = f.text_area :description, class: "form-control js-gfm-input markdown-area", rows: 10
+        .clearfix.hint
+          .pull-left Description is parsed with #{link_to "GitLab Flavored Markdown", help_page_path("markdown", "markdown"), target: '_blank'}.
+          .pull-right Attach images (JPG, PNG, GIF) by dragging & dropping or #{link_to "selecting them", '#', class: 'markdown-selector' }.
+        .error-alert
+      .form-group
+        .issue-assignee
+          = f.label :assignee_id do
+            %i.icon-user
+            Assign to
+          %div
+            = project_users_select_tag('merge_request[assignee_id]', placeholder: 'Select a user', class: 'custom-form-control', selected: @merge_request.assignee_id, project_id: @merge_request.target_project_id)
+            &nbsp;
+            = link_to 'Assign to me', '#', class: 'btn assign-to-me-link'
+      .form-group
+        .issue-milestone
+          = f.label :milestone_id do
+            %i.icon-time
+            Milestone
+          %div= f.select(:milestone_id, milestone_options(@merge_request), { include_blank: "Select milestone" }, {class: 'select2'})
+    .panel-footer
+      - if contribution_guide_url(@target_project)
+        %p
+          Please review the
+          %strong #{link_to "guidelines for contribution", contribution_guide_url(@target_project)}
+          to this repository.
+      = f.hidden_field :source_project_id
+      = f.hidden_field :target_project_id
+      = f.hidden_field :target_branch
+      = f.hidden_field :source_branch
+      = f.submit 'Submit merge request', class: "btn btn-create"
 
-    %h4 Changes
-    - if @diffs.present?
-      = render "projects/commits/diffs", diffs: @diffs, project: @project
-    - elsif @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
-      .bs-callout.bs-callout-danger
-        %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits.
-        %p To preserve performance the line changes are not shown.
+.mr-compare
+  %div.panel.panel-default
+    .panel-heading
+      Commits (#{@commits.count})
+    - if @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
+      %ul.well-list
+        - Commit.decorate(@commits.first(MergeRequestDiff::COMMITS_SAFE_SIZE)).each do |commit|
+          = render "projects/commits/inline_commit", commit: commit, project: @project
+        %li.warning-row.unstyled
+          other #{@commits.size - MergeRequestDiff::COMMITS_SAFE_SIZE} commits hidden to prevent performance issues.
+    - else
+      %ul.well-list= render Commit.decorate(@commits), project: @project
+
+  %h4 Changes
+  - if @diffs.present?
+    = render "projects/commits/diffs", diffs: @diffs, project: @project
+  - elsif @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
+    .bs-callout.bs-callout-danger
+      %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits.
+      %p To preserve performance the line changes are not shown.
+  - else
+    .bs-callout.bs-callout-danger
+      %h4 This comparison includes huge diff.
+      %p To preserve performance the line changes are not shown.
 
 
 :javascript
diff --git a/app/views/projects/merge_requests/new.html.haml b/app/views/projects/merge_requests/new.html.haml
index c24e5916721..4756903d0e0 100644
--- a/app/views/projects/merge_requests/new.html.haml
+++ b/app/views/projects/merge_requests/new.html.haml
@@ -1,4 +1,4 @@
-- if @commits.present?
+- if @merge_request.can_be_created
   = render 'new_submit'
 - else
   = render 'new_compare'