Merge branch 'squash-errors' into 'master'

Improve error messages when squashing fails

Closes gitlab-com/support-forum#1568

See merge request !1318

app/services/merge_requests/merge_service.rb

@@ -6,6 +6,8 @@ module MergeRequests
   # Executed when you do merge via GitLab UI
   #
   class MergeService < MergeRequests::BaseService
+    MergeError = Class.new(StandardError)
+
     attr_reader :merge_request, :source
 
     def execute(merge_request)
@@ -38,6 +40,8 @@ module MergeRequests
           success
         end
       end
+    rescue MergeError => e
+      log_merge_error(e.message, save_message_on_model: true)
     end
 
     def hooks_validation_pass?(merge_request)
@@ -119,12 +123,11 @@ module MergeRequests
 
       squash_result = SquashService.new(project, current_user, params).execute(merge_request)
 
-      if squash_result[:status] == :success
+      case squash_result[:status]
+      when :success
         squash_result[:squash_sha]
-      else
-        log_merge_error("Squashing #{merge_request_info} failed")
-
-        nil
+      when :error
+        raise MergeError, squash_result[:message]
       end
     end
   end

app/services/merge_requests/squash_service.rb

@@ -18,8 +18,7 @@ module MergeRequests
       end
 
       if merge_request.squash_in_progress?
-        log_error('Squash task canceled: Another squash is already in progress')
-        return false
+        return error('Squash task canceled: another squash is already in progress')
       end
 
       run_git_command(

changelogs/unreleased-ee/squash-errors.yml

+---
+title: Improve error messages when squashing fails
+merge_request:
+author:

spec/services/merge_requests/merge_service_spec.rb

@@ -206,6 +206,36 @@ describe MergeRequests::MergeService, services: true do
         expect(merge_request.merge_error).to include(error_message)
         expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
       end
+
+      context 'when squashing' do
+        it 'logs and saves error if there is an error when squashing' do
+          error_message = 'Failed to squash. Should be done manually'
+
+          allow_any_instance_of(MergeRequests::SquashService).to receive(:squash).and_return(nil)
+          merge_request.update(squash: true)
+
+          service.execute(merge_request)
+
+          expect(merge_request).to be_open
+          expect(merge_request.merge_commit_sha).to be_nil
+          expect(merge_request.merge_error).to include(error_message)
+          expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
+        end
+
+        it 'logs and saves error if there is a squash in progress' do
+          error_message = 'another squash is already in progress'
+
+          allow_any_instance_of(MergeRequest).to receive(:squash_in_progress?).and_return(true)
+          merge_request.update(squash: true)
+
+          service.execute(merge_request)
+
+          expect(merge_request).to be_open
+          expect(merge_request.merge_commit_sha).to be_nil
+          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
   end