Refactor ff merge and get it to work with Merge when build succeeds

app/assets/javascripts/merge_request_widget.js.coffee

@@ -23,7 +23,7 @@ class @MergeRequestWidget
           $('.mr-widget-body').html("<h4>" + data.merge_error + "</h4>")
         else
           callback = -> merge_request_widget.mergeInProgress(deleteSourceBranch)
-          setTimeout(callback, 2000)
+          setTimeout(callback, 1000)
       dataType: 'json'
 
   rebaseInProgress: ->
@@ -31,9 +31,8 @@ class @MergeRequestWidget
       type: 'GET'
       url: $('.merge-request').data('url')
       success: (data) =>
-        debugger
         if data["rebase_in_progress?"]
-          setTimeout(merge_request_widget.rebaseInProgress, 2000)
+          setTimeout(merge_request_widget.rebaseInProgress, 1000)
         else
           location.reload()
       dataType: 'json'

app/controllers/projects/merge_requests_controller.rb

@@ -2,7 +2,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :module_enabled
   before_action :merge_request, only: [
     :edit, :update, :show, :diffs, :commits, :builds, :merge, :merge_check,
-    :ci_status, :toggle_subscription, :approve, :ff_merge, :rebase, :cancel_merge_when_build_succeeds
+    :ci_status, :toggle_subscription, :approve, :rebase, :cancel_merge_when_build_succeeds
   ]
   before_action :closes_issues, only: [:edit, :update, :show, :diffs, :commits, :builds]
   before_action :validates_merge_request, only: [:show, :diffs, :commits, :builds]
@@ -179,6 +179,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       return
     end
 
+    if params[:ff].present? && !@merge_request.ff_merge_possible?
+      @status = :failed
+      return
+    end
+
     @merge_request.update(merge_error: nil)
 
     if params[:merge_when_build_succeeds].present? && @merge_request.ci_commit && @merge_request.ci_commit.active?
@@ -191,6 +196,13 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     end
   end
 
+  def rebase
+    return access_denied! unless @merge_request.can_be_merged_by?(current_user)
+    return render_404 unless @merge_request.approved?
+
+    RebaseWorker.perform_async(@merge_request.id, current_user.id)
+  end
+
   def branch_from
     #This is always source
     @source_project = @merge_request.nil? ? @project : @merge_request.source_project
@@ -248,26 +260,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     redirect_to merge_request_path(@merge_request)
   end
 
-  def ff_merge
-    return access_denied! unless @merge_request.can_be_merged_by?(current_user)
-    return render_404 unless @merge_request.approved?
-
-    if @merge_request.ff_merge_possible?
-      MergeRequests::FfMergeService.new(merge_request.target_project, current_user).
-        execute(merge_request)
-      @status = true
-    else
-      @status = false
-    end
-  end
-
-  def rebase
-    return access_denied! unless @merge_request.can_be_merged_by?(current_user)
-    return render_404 unless @merge_request.approved?
-
-    RebaseWorker.perform_async(@merge_request.id, current_user.id)
-  end
-
   protected
 
   def selected_target_project
@@ -363,7 +355,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def merge_params
-    params.permit(:should_remove_source_branch, :commit_message)
+    params.permit(:should_remove_source_branch, :commit_message, :ff)
   end
 
   # Make sure merge requests created before 8.0

app/helpers/branches_helper.rb

@@ -14,14 +14,4 @@ module BranchesHelper
 
     ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(branch_name)
   end
-
-  def can_rebase?(project, branch_name)
-    if project.protected_branch? branch_name
-      can?(current_user, :push_code_to_protected_branches, project)
-    elsif can?(current_user, :push_code, project)
-      true
-    else
-      false
-    end
-  end
 end

app/services/merge_requests/ff_merge_service.rb

 module MergeRequests
   # MergeService class
   #
-  # Do git merge and in case of success
+  # Do git fast-forward merge and in case of success
   # mark merge request as merged and execute all hooks and notifications
-  # Executed when you do merge via GitLab UI
+  # Executed when you do fast-forward merge via GitLab UI
   #
-  class FfMergeService < MergeRequests::BaseService
-    attr_reader :merge_request
-
-    def execute(merge_request)
-      @merge_request = merge_request
-
-      unless @merge_request.ff_merge_possible?
-        return error('Merge request is not mergeable')
-      end
-
-      merge_request.in_locked_state do
-        if update_head
-          after_merge
-          success
-        else
-          error('Can not merge changes')
-        end
-      end
-    end
-
+  class FfMergeService < MergeRequests::MergeService
     private
 
-    def update_head
-      repository.ff_merge(current_user, merge_request.source_sha, merge_request.target_branch)
-    end
+    def commit
+      return false unless @merge_request.ff_merge_possible?
 
-    def after_merge
-      MergeRequests::PostMergeService.new(project, current_user).execute(merge_request)
+      repository.ff_merge(current_user, merge_request.source_sha, merge_request.target_branch)
     end
   end
 end

app/services/merge_requests/merge_service.rb

@@ -9,6 +9,13 @@ module MergeRequests
     attr_reader :merge_request
 
     def execute(merge_request)
+      # Delete the ff param so we don't get into an infinite loop if it's present,
+      # since FfMergeService inherits from MergeService.
+      if params.delete(:ff).present?
+        FfMergeService.new(project, current_user, params).execute(merge_request)
+        return
+      end
+
       @merge_request = merge_request
 
       return error('Merge request is not mergeable') unless @merge_request.mergeable?

app/views/projects/merge_requests/ff_merge.js.haml

-- if @status
-  :plain
-    location.reload()
-- else
-  :plain
-    $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/reload'))}");

app/views/projects/merge_requests/rebase.js.haml

 :plain
-  $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/ff_accept'))}")
+  $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/rebase', rebase_in_progress: true))}")
   merge_request_widget.rebaseInProgress();

app/views/projects/merge_requests/widget/_open.html.haml

@@ -20,7 +20,10 @@
     - elsif !@merge_request.can_be_merged_by?(current_user)
       = render 'projects/merge_requests/widget/open/not_allowed'
     - elsif @project.merge_requests_ff_only_enabled
-      = render 'projects/merge_requests/widget/open/ff_accept'
+      - if @merge_request.ff_merge_possible?
+        = render 'projects/merge_requests/widget/open/ff_accept'
+      - else
+        = render 'projects/merge_requests/widget/open/rebase'
     - elsif @merge_request.can_be_merged?
       = render 'projects/merge_requests/widget/open/accept'
 

app/views/projects/merge_requests/widget/open/_ff_accept.html.haml

-- if @merge_request.ff_merge_possible?
-  = form_for [:ff_merge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-requires-input' } do |f|
-    = hidden_field_tag :authenticity_token, form_authenticity_token
-    .accept-merge-holder.clearfix.js-toggle-container
-      .accept-action
-        = f.button class: "btn btn-create accept-mr" do
+- status_class = @ci_commit ? " ci-#{@ci_commit.status}" : nil
+
+= form_for [:merge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-requires-input' } do |f|
+  = hidden_field_tag :authenticity_token, form_authenticity_token
+  .accept-merge-holder.clearfix.js-toggle-container
+    .accept-action
+      - if @ci_commit && @ci_commit.active?
+        %span.btn-group
+          = button_tag class: "btn btn-create js-merge-button merge_when_build_succeeds" do
+            Merge When Build Succeeds
+          = button_tag class: "btn btn-success dropdown-toggle", 'data-toggle' => 'dropdown' do
+            %span.caret
+            %span.sr-only
+              Select Merge Moment
+          %ul.js-merge-dropdown.dropdown-menu.dropdown-menu-right{ role: 'menu' }
+            %li
+              = link_to "#", class: "merge_when_build_succeeds" do
+                = icon('check fw')
+                Merge When Build Succeeds
+            %li
+              = link_to "#", class: "accept_merge_request" do
+                = icon('warning fw')
+                Merge Immediately
+      - else
+        = f.button class: "btn btn-create btn-grouped js-merge-button accept_merge_request #{status_class}" do
           Accept Merge Request
+    - if @merge_request.can_remove_source_branch?(current_user)
+      .accept-control.checkbox
+        = label_tag :should_remove_source_branch, class: "remove_source_checkbox" do
+          = check_box_tag :should_remove_source_branch
+          Remove source branch
+
+    .accept-control.right
+      Fast-forward merge without creating merge commit
+
+    = hidden_field_tag :merge_when_build_succeeds, "", autocomplete: "off"
+    = hidden_field_tag :ff, "1"
 
-      .accept-control
-        Fast-forward merge without creating merge commit
-- else
-  = form_for [:rebase, @project.namespace.becomes(Namespace), @project, @merge_request],
-    remote: true, method: :post, html: { class: 'accept-mr-form js-requires-input' } do |f|
-    = hidden_field_tag :authenticity_token, form_authenticity_token
-    .accept-merge-holder.clearfix.js-toggle-container
-      - if @merge_request.target_project.merge_requests_rebase_enabled && can_rebase?(@merge_request.source_project, @merge_request.source_branch)
-        - if @merge_request.rebase_in_progress?
-          %h4 Rebase in progress... It can take a while. Reload at will.
-        - else
-          .accept-action
-            = f.button class: "btn btn-reopen rebase-mr" do
-              Rebase onto #{@merge_request.target_branch}
-      .accept-control
-        Fast-forward merge is not possible. Branch must be rebased first
+:javascript
+  $('.accept-mr-form').on('ajax:send', function() {
+    $(".accept-mr-form :input").disable();
+  });
 
-:coffeescript
-  $('.accept-mr-form').on 'ajax:before', ->
-    btn = $('.accept-mr')
-    btn.disable()
-    btn.html("<i class='fa fa-spinner fa-spin'></i> Merge in progress")
+  $('.accept_merge_request').on('click', function() {
+    $('.js-merge-button').html("<i class='fa fa-spinner fa-spin'></i> Merge in progress");
+  });
 
-  $('.rebase-mr-form').on 'ajax:before', ->
-    btn = $('.rebase-mr')
-    btn.disable()
-    btn.html("<i class='fa fa-spinner fa-spin'></i> Rebase in progress. It could take some time")
+  $('.merge_when_build_succeeds').on('click', function() {
+    $("#merge_when_build_succeeds").val("1");
+  });
 
-  - if #{@merge_request.rebase_in_progress?}
-    $ ->
-      merge_request_widget.rebaseInProgress()
+  $('.js-merge-dropdown a').on('click', function(e) {
+    e.preventDefault();
+    $(this).closest("form").submit();
+  });

app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml

@@ -5,7 +5,10 @@
   - should_remove_source_branch = @merge_request.merge_params["should_remove_source_branch"].present?
   %p
     = succeed '.' do
-      The changes will be merged into
+      - if @merge_request.merge_params["ff"].present?
+        The changes will be fast-forward merged into
+      - else
+        The changes will be merged into
       %span.label-branch= @merge_request.target_branch
     - if should_remove_source_branch
       The source branch will be removed.

app/views/projects/merge_requests/widget/open/_rebase.html.haml

+- if @merge_request.rebase_in_progress? || (defined?(rebase_in_progress) && rebase_in_progress)
+  %h4
+    = icon("spinner spin")
+    Rebase in progress…
+  %p
+    This merge request is in the process of being rebased, during which time it is locked and cannot be closed.
+
+  :javascript
+    $(function() {
+      merge_request_widget.rebaseInProgress()
+    });
+- elsif !@merge_request.target_project.merge_requests_rebase_enabled || !can_push_branch?(@merge_request.source_project, @merge_request.source_branch)
+  %h4
+    = icon("exclamation-triangle")
+    Fast-forward merge is not possible
+  %p
+    Please rebase this branch on
+    %span.label-branch= @merge_request.target_branch
+    and try again.
+- else
+  = form_for [:rebase, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'rebase-mr-form js-requires-input' } do |f|
+    = hidden_field_tag :authenticity_token, form_authenticity_token
+    .accept-merge-holder.clearfix.js-toggle-container
+      .accept-action
+        = f.button class: "btn btn-reopen js-rebase-button" do
+          Rebase onto #{@merge_request.target_branch}
+      .accept-control
+        Fast-forward merge is not possible. Branch must be rebased first
+
+  :javascript
+    $('.rebase-mr-form').on('ajax:send', function() {
+      $('.rebase-mr-form :input').disable();
+    });
+
+    $('.js-rebase-button').on('click', function() {
+      $('.js-rebase-button').html("<i class='fa fa-spinner fa-spin'></i> Rebase in progress");
+    });

config/routes.rb

@@ -606,7 +606,6 @@ Rails.application.routes.draw do
             post :toggle_subscription
             post :approve
             post :rebase
-            post :ff_merge
           end
 
           collection do