Merge branch '38476-improve-merge-jid-cleanup-on-merge-process-ee' into 'master'

[CE backport] Clean merge_jid whenever necessary on the merge process

See merge request gitlab-org/gitlab-ee!3027

app/services/merge_requests/merge_service.rb

@@ -19,7 +19,7 @@ module MergeRequests
       @merge_request = merge_request
 
       unless @merge_request.mergeable?
-        return log_merge_error('Merge request is not mergeable', save_message_on_model: true)
+        return handle_merge_error(log_message: 'Merge request is not mergeable', save_message_on_model: true)
       end
 
       check_size_limit
@@ -27,7 +27,7 @@ module MergeRequests
       @source = find_merge_source
 
       unless @source
-        return log_merge_error('No source for merge', save_message_on_model: true)
+        return handle_merge_error(log_message: 'No source for merge', save_message_on_model: true)
       end
 
       merge_request.in_locked_state do
@@ -38,8 +38,7 @@ module MergeRequests
         end
       end
     rescue MergeError => e
-      clean_merge_jid
-      log_merge_error(e.message, save_message_on_model: true)
+      handle_merge_error(log_message: e.message, save_message_on_model: true)
     end
 
     def hooks_validation_pass?(merge_request)
@@ -52,12 +51,12 @@ module MergeRequests
       return true unless push_rule
 
       unless push_rule.commit_message_allowed?(params[:commit_message])
-        log_merge_error("Commit message does not follow the pattern '#{push_rule.commit_message_regex}'", save_message_on_model: true)
+        handle_merge_error(log_message: "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)
-        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)
+        handle_merge_error(log_message: "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
 
@@ -103,10 +102,16 @@ module MergeRequests
       @merge_request.force_remove_source_branch? ? @merge_request.author : current_user
     end
 
-    def log_merge_error(message, save_message_on_model: false)
-      Rails.logger.error("MergeService ERROR: #{merge_request_info} - #{message}")
+    # Logs merge error message and cleans `MergeRequest#merge_jid`.
+    #
+    def handle_merge_error(log_message:, save_message_on_model: false)
+      Rails.logger.error("MergeService ERROR: #{merge_request_info} - #{log_message}")
 
-      @merge_request.update(merge_error: message) if save_message_on_model
+      if save_message_on_model
+        @merge_request.update(merge_error: log_message, merge_jid: nil)
+      else
+        clean_merge_jid
+      end
     end
 
     def merge_request_info

spec/services/merge_requests/merge_service_spec.rb

@@ -13,20 +13,21 @@ describe MergeRequests::MergeService do
 
   describe '#execute' do
     context 'MergeRequest#merge_jid' do
+      let(:service) do
+        described_class.new(project, user, commit_message: 'Awesome message')
+      end
+
       before do
         merge_request.update_column(:merge_jid, 'hash-123')
       end
 
       it 'is cleaned when no error is raised' do
-        service = described_class.new(project, user, commit_message: 'Awesome message')
-
         service.execute(merge_request)
 
         expect(merge_request.reload.merge_jid).to be_nil
       end
 
       it 'is cleaned when expected error is raised' do
-        service = described_class.new(project, user, commit_message: 'Awesome message')
         allow(service).to receive(:commit).and_raise(described_class::MergeError)
 
         service.execute(merge_request)
@@ -34,6 +35,22 @@ describe MergeRequests::MergeService do
         expect(merge_request.reload.merge_jid).to be_nil
       end
 
+      it 'is cleaned when merge request is not mergeable' do
+        allow(merge_request).to receive(:mergeable?).and_return(false)
+
+        service.execute(merge_request)
+
+        expect(merge_request.reload.merge_jid).to be_nil
+      end
+
+      it 'is cleaned when no source is found' do
+        allow(merge_request).to receive(:diff_head_sha).and_return(nil)
+
+        service.execute(merge_request)
+
+        expect(merge_request.reload.merge_jid).to be_nil
+      end
+
       it 'is not cleaned when unexpected error is raised' do
         service = described_class.new(project, user, commit_message: 'Awesome message')
         allow(service).to receive(:commit).and_raise(StandardError)