Merge branch '28010-mr-merge-button-default-to-danger' into 'master'

Default to dangerous MR merge button

Closes #28010

See merge request !9245

app/assets/javascripts/merge_request_widget.js.es6

@@ -129,8 +129,9 @@ require('./smart_interval');
     };
 
     MergeRequestWidget.prototype.getMergeStatus = function() {
-      return $.get(this.opts.merge_check_url, function(data) {
+      return $.get(this.opts.merge_check_url, (data) => {
         var $html = $(data);
+        this.updateMergeButton(this.status, this.hasCi, $html);
         $('.mr-widget-body').replaceWith($html.find('.mr-widget-body'));
         $('.mr-widget-footer').replaceWith($html.find('.mr-widget-footer'));
       });
@@ -154,9 +155,9 @@ require('./smart_interval');
       return $.getJSON(this.opts.ci_status_url, (function(_this) {
         return function(data) {
           var message, status, title;
-          if (!data.status) {
-            return;
-          }
+          _this.status = data.status;
+          _this.hasCi = data.has_ci;
+          _this.updateMergeButton(_this.status, _this.hasCi);
           if (data.environments && data.environments.length) _this.renderEnvironments(data.environments);
           if (data.status !== _this.opts.ci_status ||
               data.sha !== _this.opts.ci_sha ||
@@ -227,42 +228,51 @@ require('./smart_interval');
     };
 
     MergeRequestWidget.prototype.showCIStatus = function(state) {
+      // TODO: Can this variable be removed? - Felipe Artur
       var allowed_states;
       if (state == null) {
         return;
       }
       $('.ci_widget').hide();
-      allowed_states = ["failed", "canceled", "running", "pending", "success", "success_with_warnings", "skipped", "not_found"];
-      if (indexOf.call(allowed_states, state) >= 0) {
       $('.ci_widget.ci-' + state).show();
+
+      this.initMiniPipelineGraph();
+    };
+
+    MergeRequestWidget.prototype.showCICoverage = function(coverage) {
+      var text = `Coverage ${coverage}%`;
+      return $('.ci_widget:visible .ci-coverage').text(text);
+    };
+
+    MergeRequestWidget.prototype.updateMergeButton = function(state, hasCi, $html) {
+      const allowed_states = ["failed", "canceled", "running", "pending", "success", "success_with_warnings", "skipped", "not_found"];
+      let stateClass = 'btn-danger';
+      if (!hasCi) {
+        stateClass = 'btn-create';
+      } else if (indexOf.call(allowed_states, state) !== -1) {
         switch (state) {
           case "failed":
           case "canceled":
           case "not_found":
-            this.setMergeButtonClass('btn-danger');
+            stateClass = 'btn-danger';
             break;
           case "running":
-            this.setMergeButtonClass('btn-info');
+            stateClass = 'btn-info';
             break;
           case "success":
           case "success_with_warnings":
-            this.setMergeButtonClass('btn-create');
+            stateClass = 'btn-create';
         }
       } else {
         $('.ci_widget.ci-error').show();
-        this.setMergeButtonClass('btn-danger');
+        stateClass = 'btn-danger';
       }
-      this.initMiniPipelineGraph();
-    };
 
-    MergeRequestWidget.prototype.showCICoverage = function(coverage) {
-      var text;
-      text = 'Coverage ' + coverage + '%';
-      return $('.ci_widget:visible .ci-coverage').text(text);
+      this.setMergeButtonClass(stateClass, $html);
     };
 
-    MergeRequestWidget.prototype.setMergeButtonClass = function(css_class) {
-      return $('.js-merge-button,.accept-action .dropdown-toggle').removeClass('btn-danger btn-info btn-create').addClass(css_class);
+    MergeRequestWidget.prototype.setMergeButtonClass = function(css_class, $html = $('.mr-state-widget')) {
+      return $html.find('.js-merge-button').removeClass('btn-danger btn-info btn-create').addClass(css_class);
     };
 
     MergeRequestWidget.prototype.updatePipelineUrls = function(id) {

app/assets/javascripts/merge_request_widget/ci_bundle.js.es6

@@ -15,15 +15,15 @@
     });
 
     $(document)
-    .off('click', '.accept_merge_request')
-    .on('click', '.accept_merge_request', () => {
-      $('.js-merge-button').html('<i class="fa fa-spinner fa-spin"></i> Merge in progress');
+    .off('click', '.accept-merge-request')
+    .on('click', '.accept-merge-request', () => {
+      $('.js-merge-button, .js-merge-when-pipeline-succeeds-button').html('<i class="fa fa-spinner fa-spin"></i> Merge in progress');
     });
 
     $(document)
-    .off('click', '.merge_when_build_succeeds')
-    .on('click', '.merge_when_build_succeeds', () => {
-      $('#merge_when_build_succeeds').val('1');
+    .off('click', '.merge-when-pipeline-succeeds')
+    .on('click', '.merge-when-pipeline-succeeds', () => {
+      $('#merge_when_pipeline_succeeds').val('1');
     });
 
     $(document)

app/assets/stylesheets/pages/merge_requests.scss

@@ -29,7 +29,7 @@
         background-color: $gl-success;
       }
 
-      .accept_merge_request {
+      .accept-merge-request {
         &.ci-pending,
         &.ci-running {
           @include btn-blue;
@@ -42,6 +42,12 @@
           @include btn-red;
         }
       }
+
+      .dropdown-toggle {
+        .fa {
+          color: inherit;
+        }
+      }
     }
 
     .accept-control {

app/controllers/projects/merge_requests_controller.rb

@@ -308,6 +308,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def merge_check
     @merge_request.check_if_can_be_merged
+    @pipelines = @merge_request.all_pipelines
 
     render partial: "projects/merge_requests/widget/show.html.haml", layout: false
   end
@@ -426,6 +427,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def ci_status
     pipeline = @merge_request.head_pipeline
+    @pipelines = @merge_request.all_pipelines
 
     if pipeline
       status = pipeline.status
@@ -444,7 +446,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       sha: (merge_request.diff_head_commit.short_id if merge_request.diff_head_sha),
       status: status,
       coverage: coverage,
-      pipeline: pipeline.try(:id)
+      pipeline: pipeline.try(:id),
+      has_ci: @merge_request.has_ci?
     }
 
     render json: response

app/models/merge_request.rb

@@ -692,7 +692,10 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def has_ci?
-    source_project.try(:ci_service) && commits.any?
+    has_ci_integration = source_project.try(:ci_service)
+    uses_gitlab_ci = all_pipelines.any?
+
+    (has_ci_integration || uses_gitlab_ci) && commits.any?
   end
 
   def branch_missing?

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

 - content_for :page_specific_javascripts do
   = page_specific_javascript_bundle_tag('merge_request_widget')
 
-- status_class = @pipeline ? " ci-#{@pipeline.status}" : nil
-
 = form_for [:merge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-quick-submit js-requires-input' } do |f|
   = hidden_field_tag :authenticity_token, form_authenticity_token
   = hidden_field_tag :sha, @merge_request.diff_head_sha
@@ -11,10 +9,10 @@
       .accept-action
         - if @pipeline && @pipeline.active?
           %span.btn-group
-            = button_tag class: "btn btn-create js-merge-button merge_when_build_succeeds" do
+            = button_tag class: "btn btn-info js-merge-when-pipeline-succeeds-button merge-when-pipeline-succeeds" do
               Merge When Pipeline Succeeds
-            - unless @project.only_allow_merge_if_build_succeeds?
-              = button_tag class: "btn btn-success dropdown-toggle", 'data-toggle' => 'dropdown' do
+            - unless @project.only_allow_merge_if_pipeline_succeeds?
+              = button_tag class: "btn btn-info dropdown-toggle", 'data-toggle' => 'dropdown' do
                 = icon('caret-down')
                 %span.sr-only
                   Select Merge Moment
@@ -24,11 +22,11 @@
                     = icon('check fw')
                     Merge When Pipeline Succeeds
                 %li
-                  = link_to "#", class: "accept_merge_request" do
+                  = 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
+          = f.button class: "btn btn-grouped js-merge-button accept-merge-request" do
             Accept Merge Request
       - if @merge_request.force_remove_source_branch?
         .accept-control

changelogs/unreleased/28010-mr-merge-button-default-to-danger.yml

+---
+title: Default to subtle MR mege button until CI status is available
+merge_request:
+author:

spec/features/merge_requests/merge_immediately_with_pipeline_spec.rb

@@ -34,7 +34,7 @@ feature 'Merge immediately', :feature, :js do
 
         click_link 'Merge Immediately'
 
-        expect(find('.js-merge-button')).to have_content('Merge in progress')
+        expect(find('.js-merge-when-pipeline-succeeds-button')).to have_content('Merge in progress')
       end
     end
   end

spec/features/merge_requests/widget_spec.rb

@@ -3,8 +3,8 @@ require 'rails_helper'
 describe 'Merge request', :feature, :js do
   include WaitForAjax
 
-  let(:project) { create(:project) }
   let(:user) { create(:user) }
+  let(:project) { create(:project) }
   let(:merge_request) { create(:merge_request, source_project: project) }
 
   before do
@@ -31,7 +31,7 @@ describe 'Merge request', :feature, :js do
 
       wait_for_ajax
 
-      expect(page).to have_selector('.accept_merge_request')
+      expect(page).to have_selector('.accept-merge-request')
     end
   end
 
@@ -51,6 +51,69 @@ describe 'Merge request', :feature, :js do
         expect(find('.js-environment-link')[:href]).to include(environment.formatted_external_url)
       end
     end
+
+    it 'shows green accept merge request button' do
+      # Wait for the `ci_status` and `merge_check` requests
+      wait_for_ajax
+      expect(page).to have_selector('.accept-merge-request.btn-create')
+    end
+  end
+
+  context 'view merge request with external CI service' do
+    before do
+      create(:service, project: project,
+                       active: true,
+                       type: 'CiService',
+                       category: 'ci')
+
+      visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+    end
+
+    it 'has danger button while waiting for external CI status' do
+      # Wait for the `ci_status` and `merge_check` requests
+      wait_for_ajax
+      expect(page).to have_selector('.accept-merge-request.btn-danger')
+    end
+  end
+
+  context 'view merge request with failed GitLab CI pipelines' do
+    before do
+      commit_status = create(:commit_status, project: project, status: 'failed')
+      pipeline = create(:ci_pipeline, project: project,
+                                      sha: merge_request.diff_head_sha,
+                                      ref: merge_request.source_branch,
+                                      status: 'failed',
+                                      statuses: [commit_status])
+      create(:ci_build, :pending, pipeline: pipeline)
+
+      visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+    end
+
+    it 'has danger button when not succeeded' do
+      # Wait for the `ci_status` and `merge_check` requests
+      wait_for_ajax
+      expect(page).to have_selector('.accept-merge-request.btn-danger')
+    end
+  end
+
+  context 'view merge request with MWBS button' do
+    before do
+      commit_status = create(:commit_status, project: project, status: 'pending')
+      pipeline = create(:ci_pipeline, project: project,
+                                      sha: merge_request.diff_head_sha,
+                                      ref: merge_request.source_branch,
+                                      status: 'pending',
+                                      statuses: [commit_status])
+      create(:ci_build, :pending, pipeline: pipeline)
+
+      visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+    end
+
+    it 'has info button when MWBS button' do
+      # Wait for the `ci_status` and `merge_check` requests
+      wait_for_ajax
+      expect(page).to have_selector('.merge-when-pipeline-succeeds.btn-info')
+    end
   end
 
   context 'merge error' do