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

Show merge errors in merge request widget

See merge request !1241

app/services/merge_requests/merge_service.rb

@@ -16,41 +16,45 @@ module MergeRequests
 
       @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
 
       if @merge_request.target_project.above_size_limit?
         message = Gitlab::RepositorySizeError.new(@merge_request.target_project).merge_error
-        @merge_request.update(merge_error: message)
-        return error(message)
+
+        return log_merge_error(message, save_message_on_model: true)
       end
 
       @source = find_merge_source
 
-      return log_merge_error('No source for merge', true) unless @source
+      unless @source
+        return log_merge_error('No source for merge', 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
 
     def hooks_validation_pass?(merge_request)
+      @merge_request = merge_request
+
       return true if project.merge_requests_ff_only_enabled
 
       push_rule = merge_request.project.push_rule
       return true unless push_rule
 
       unless push_rule.commit_message_allowed?(params[:commit_message])
-        merge_request.update(merge_error: "Commit message does not follow the pattern '#{push_rule.commit_message_regex}'")
+        log_merge_error("Commit message does not follow the pattern '#{push_rule.commit_message_regex}'", save_message_on_model: true)
         return false
       end
 
       unless push_rule.author_email_allowed?(current_user.email)
-        merge_request.update(merge_error: "Commit author's email '#{current_user.email}' does not follow the pattern '#{push_rule.author_email_regex}'")
+        log_merge_error("Commit author's email '#{current_user.email}' does not follow the pattern '#{push_rule.author_email_regex}'", save_message_on_model: true)
         return false
       end
 
@@ -73,11 +77,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}")
@@ -100,10 +104,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

changelogs/unreleased/28124-mrs-don-t-show-all-merge-errors.yml

+---
+title: Show merge errors in merge request widget
+merge_request: 9229
+author:

spec/features/merge_requests/widget_spec.rb

@@ -53,20 +53,17 @@ describe 'Merge request', :feature, :js do
     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) }
-
+  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)
-    end
-
-    it 'shows environments link' do
+      click_button 'Accept Merge Request'
       wait_for_ajax
+    end
 
-      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)
+    it 'updates the MR widget' do
+      page.within('.mr-widget-body') do
+        expect(page).to have_content('Conflicts detected during merge')
       end
     end
   end

spec/services/merge_requests/merge_service_spec.rb

@@ -165,35 +165,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