Merge branch '28124-mrs-don-t-show-all-merge-errors' into 'master'

Show merge errors in merge request widget

Closes #28124 and gitlab-ee#1652

See merge request !9229

Signed-off-by: Rémy Coutable <remy@rymai.me>

5 files changed, 102 insertions, 17 deletions

diff --git a/app/assets/javascripts/merge_request_widget.js.es6 b/app/assets/javascripts/merge_request_widget.js.es6
index e47047c4cca..ff0abcd8f81 100644
--- a/app/assets/javascripts/merge_request_widget.js.es6
+++ b/app/assets/javascripts/merge_request_widget.js.es6
@@ -106,7 +106,7 @@
               urlSuffix = deleteSourceBranch ? '?deleted_source_branch=true' : '';
               return window.location.href = window.location.pathname + urlSuffix;
             } else if (data.merge_error) {
-              return _this.$widgetBody.html("<h4>" + data.merge_error + "</h4>");
+              return $('.mr-widget-body').html("<h4>" + data.merge_error + "</h4>");
             } else {
               callback = function() {
                 return merge_request_widget.mergeInProgress(deleteSourceBranch);
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index ab9056a3250..395bfd17ce0 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -11,14 +11,14 @@ module MergeRequests
     def execute(merge_request)
       @merge_request = merge_request
 
-      return log_merge_error('Merge request is not mergeable', true) unless @merge_request.mergeable?
+      unless @merge_request.mergeable?
+        return log_merge_error('Merge request is not mergeable', save_message_on_model: true)
+      end
 
       merge_request.in_locked_state do
         if commit
           after_merge
           success
-        else
-          log_merge_error('Can not merge changes', true)
         end
       end
     end
@@ -39,11 +39,11 @@ module MergeRequests
       if commit_id
         merge_request.update(merge_commit_sha: commit_id)
       else
-        merge_request.update(merge_error: 'Conflicts detected during merge')
+        log_merge_error('Conflicts detected during merge', save_message_on_model: true)
         false
       end
     rescue GitHooksService::PreReceiveError => e
-      merge_request.update(merge_error: e.message)
+      log_merge_error(e.message, save_message_on_model: true)
       false
     rescue StandardError => e
       merge_request.update(merge_error: "Something went wrong during merge: #{e.message}")
@@ -66,10 +66,10 @@ module MergeRequests
       @merge_request.force_remove_source_branch? ? @merge_request.author : current_user
     end
 
-    def log_merge_error(message, http_error = false)
+    def log_merge_error(message, save_message_on_model: false)
       Rails.logger.error("MergeService ERROR: #{merge_request_info} - #{message}")
 
-      error(message) if http_error
+      @merge_request.update(merge_error: message) if save_message_on_model
     end
 
     def merge_request_info
diff --git a/changelogs/unreleased/28124-mrs-don-t-show-all-merge-errors.yml b/changelogs/unreleased/28124-mrs-don-t-show-all-merge-errors.yml
new file mode 100644
index 00000000000..cd61c38e1bc
--- /dev/null
+++ b/changelogs/unreleased/28124-mrs-don-t-show-all-merge-errors.yml
@@ -0,0 +1,4 @@
+---
+title: Show merge errors in merge request widget
+merge_request: 9229
+author:
diff --git a/spec/features/merge_requests/widget_spec.rb b/spec/features/merge_requests/widget_spec.rb
new file mode 100644
index 00000000000..4ad944366c8
--- /dev/null
+++ b/spec/features/merge_requests/widget_spec.rb
@@ -0,0 +1,70 @@
+require 'rails_helper'
+
+describe 'Merge request', :feature, :js do
+  include WaitForAjax
+
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+  let(:merge_request) { create(:merge_request, source_project: project) }
+
+  before do
+    project.team << [user, :master]
+    login_as(user)
+  end
+
+  context 'new merge request' do
+    before do
+      visit new_namespace_project_merge_request_path(
+        project.namespace,
+        project,
+        merge_request: {
+          source_project_id: project.id,
+          target_project_id: project.id,
+          source_branch: 'feature',
+          target_branch: 'master'
+        }
+      )
+    end
+
+    it 'shows widget status after creating new merge request' do
+      click_button 'Submit merge request'
+
+      wait_for_ajax
+
+      expect(page).to have_selector('.accept_merge_request')
+    end
+  end
+
+  context 'view merge request' do
+    let!(:environment) { create(:environment, project: project) }
+    let!(:deployment) { create(:deployment, environment: environment, ref: 'feature', sha: merge_request.diff_head_sha) }
+
+    before do
+      visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+    end
+
+    it 'shows environments link' do
+      wait_for_ajax
+
+      page.within('.mr-widget-heading') do
+        expect(page).to have_content("Deployed to #{environment.name}")
+        expect(find('.js-environment-link')[:href]).to include(environment.formatted_external_url)
+      end
+    end
+  end
+
+  context 'merge error' do
+    before do
+      allow_any_instance_of(Repository).to receive(:merge).and_return(false)
+      visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+      click_button 'Accept Merge Request'
+      wait_for_ajax
+    end
+
+    it 'updates the MR widget' do
+      page.within('.mr-widget-body') do
+        expect(page).to have_content('Conflicts detected during merge')
+      end
+    end
+  end
+end
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index 5a89acc96a4..d96f819e66a 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -149,35 +149,46 @@ describe MergeRequests::MergeService, services: true do
     context "error handling" do
       let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') }
 
-      it 'saves error if there is an exception' do
-        allow(service).to receive(:repository).and_raise("error message")
+      before do
+        allow(Rails.logger).to receive(:error)
+      end
 
+      it 'logs and saves error if there is an exception' do
+        error_message = 'error message'
+
+        allow(service).to receive(:repository).and_raise("error message")
         allow(service).to receive(:execute_hooks)
 
         service.execute(merge_request)
 
-        expect(merge_request.merge_error).to eq("Something went wrong during merge: error message")
+        expect(merge_request.merge_error).to include(error_message)
+        expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
       end
 
-      it 'saves error if there is an PreReceiveError exception' do
-        allow(service).to receive(:repository).and_raise(GitHooksService::PreReceiveError, "error")
+      it 'logs and saves error if there is an PreReceiveError exception' do
+        error_message = 'error message'
 
+        allow(service).to receive(:repository).and_raise(GitHooksService::PreReceiveError, error_message)
         allow(service).to receive(:execute_hooks)
 
         service.execute(merge_request)
 
-        expect(merge_request.merge_error).to eq("error")
+        expect(merge_request.merge_error).to include(error_message)
+        expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
       end
 
-      it 'aborts if there is a merge conflict' do
+      it 'logs and saves error if there is a merge conflict' do
+        error_message = 'Conflicts detected during merge'
+
         allow_any_instance_of(Repository).to receive(:merge).and_return(false)
         allow(service).to receive(:execute_hooks)
 
         service.execute(merge_request)
 
-        expect(merge_request.open?).to be_truthy
+        expect(merge_request).to be_open
         expect(merge_request.merge_commit_sha).to be_nil
-        expect(merge_request.merge_error).to eq("Conflicts detected during merge")
+        expect(merge_request.merge_error).to include(error_message)
+        expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
       end
     end
   end