Prevent MR to be dropped from Merge Trains for open discussions

After adding an MR to Merge Train, new discussions and comments will not drop the MR from the train.

app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js

@@ -17,7 +17,7 @@ export default function deviseState() {
     return stateKey.pipelineFailed;
   } else if (this.workInProgress) {
     return stateKey.workInProgress;
-  } else if (this.hasMergeableDiscussionsState) {
+  } else if (this.hasMergeableDiscussionsState && !this.autoMergeEnabled) {
     return stateKey.unresolvedDiscussions;
   } else if (this.isPipelineBlocked) {
     return stateKey.pipelineBlocked;

app/models/merge_request.rb

@@ -955,8 +955,9 @@ class MergeRequest < ApplicationRecord
     self.class.wip_title(self.title)
   end
 
-  def mergeable?(skip_ci_check: false)
-    return false unless mergeable_state?(skip_ci_check: skip_ci_check)
+  def mergeable?(skip_ci_check: false, skip_discussions_check: false)
+    return false unless mergeable_state?(skip_ci_check: skip_ci_check,
+                                         skip_discussions_check: skip_discussions_check)
 
     check_mergeability
 

app/services/merge_requests/merge_service.rb

@@ -10,13 +10,14 @@ module MergeRequests
   class MergeService < MergeRequests::MergeBaseService
     delegate :merge_jid, :state, to: :@merge_request
 
-    def execute(merge_request)
+    def execute(merge_request, options = {})
       if project.merge_requests_ff_only_enabled && !self.is_a?(FfMergeService)
         FfMergeService.new(project, current_user, params).execute(merge_request)
         return
       end
 
       @merge_request = merge_request
+      @options = options
 
       validate!
 
@@ -55,7 +56,7 @@ module MergeRequests
       error =
         if @merge_request.should_be_rebased?
           'Only fast-forward merge is allowed for your project. Please update your source branch'
-        elsif !@merge_request.mergeable?
+        elsif !@merge_request.mergeable?(skip_discussions_check: @options[:skip_discussions_check])
           'Merge request is not mergeable'
         elsif !@merge_request.squash && project.squash_always?
           'This project requires squashing commits when merge requests are accepted.'

changelogs/unreleased/220916-new-comment-not-drop-merge-train.yml

+---
+title: Prevent MRs to be dropped from Merge Trains for open discussions
+merge_request: 39957
+author:
+type: changed

ee/app/models/ee/merge_request.rb

@@ -103,7 +103,7 @@ module EE
     end
 
     override :mergeable?
-    def mergeable?(skip_ci_check: false)
+    def mergeable?(skip_ci_check: false, skip_discussions_check: false)
       return false unless approved?
       return false if has_denied_policies?
       return false if merge_blocked_by_other_mrs?

ee/app/services/merge_trains/refresh_merge_request_service.rb

@@ -33,7 +33,7 @@ module MergeTrains
         raise ProcessError, 'merge request is not on a merge train'
       end
 
-      unless merge_request.mergeable_state?(skip_ci_check: true)
+      unless merge_request.mergeable_state?(skip_ci_check: true, skip_discussions_check: true)
         raise ProcessError, 'merge request is not mergeable'
       end
 
@@ -70,7 +70,7 @@ module MergeTrains
       merge_train.start_merge!
 
       MergeRequests::MergeService.new(project, merge_user, merge_request.merge_params)
-                                 .execute(merge_request)
+                                 .execute(merge_request, skip_discussions_check: true)
 
       raise ProcessError, "failed to merge. #{merge_request.merge_error}" unless merge_request.merged?
 

ee/spec/services/merge_trains/refresh_merge_request_service_spec.rb

@@ -203,7 +203,7 @@ RSpec.describe MergeTrains::RefreshMergeRequestService do
           expect(merge_request.merge_train).to receive(:start_merge!).and_call_original
           expect(merge_request.merge_train).to receive(:finish_merge!).and_call_original
           expect_next_instance_of(MergeRequests::MergeService, project, maintainer, anything) do |service|
-            expect(service).to receive(:execute).with(merge_request).and_call_original
+            expect(service).to receive(:execute).with(merge_request, skip_discussions_check: true).and_call_original
           end
 
           expect { subject }

spec/frontend/vue_mr_widget/stores/get_state_key_spec.js

@@ -30,6 +30,7 @@ describe('getStateKey', () => {
     expect(bound()).toEqual('notAllowedToMerge');
 
     context.autoMergeEnabled = true;
+    context.hasMergeableDiscussionsState = true;
 
     expect(bound()).toEqual('autoMergeEnabled');
 
@@ -44,6 +45,7 @@ describe('getStateKey', () => {
     expect(bound()).toEqual('pipelineBlocked');
 
     context.hasMergeableDiscussionsState = true;
+    context.autoMergeEnabled = false;
 
     expect(bound()).toEqual('unresolvedDiscussions');
 

spec/models/merge_request_spec.rb

@@ -2467,6 +2467,57 @@ RSpec.describe MergeRequest, factory_default: :keep do
 
       expect(subject.mergeable?).to be_truthy
     end
+
+    context 'with skip_ci_check option' do
+      using RSpec::Parameterized::TableSyntax
+
+      before do
+        allow(subject).to receive_messages(check_mergeability: nil,
+                                           can_be_merged?: true,
+                                           broken?: false)
+      end
+
+      where(:mergeable_ci_state, :skip_ci_check, :expected_mergeable) do
+        false | false | false
+        false | true  | true
+        true  | false | true
+        true  | true  | true
+      end
+
+      with_them do
+        it 'overrides mergeable_ci_state?' do
+          allow(subject).to receive(:mergeable_ci_state?) { mergeable_ci_state }
+
+          expect(subject.mergeable?(skip_ci_check: skip_ci_check)).to eq(expected_mergeable)
+        end
+      end
+    end
+
+    context 'with skip_discussions_check option' do
+      using RSpec::Parameterized::TableSyntax
+
+      before do
+        allow(subject).to receive_messages(mergeable_ci_state?: true,
+                                           check_mergeability: nil,
+                                           can_be_merged?: true,
+                                           broken?: false)
+      end
+
+      where(:mergeable_discussions_state, :skip_discussions_check, :expected_mergeable) do
+        false | false | false
+        false | true  | true
+        true  | false | true
+        true  | true  | true
+      end
+
+      with_them do
+        it 'overrides mergeable_discussions_state?' do
+          allow(subject).to receive(:mergeable_discussions_state?) { mergeable_discussions_state }
+
+          expect(subject.mergeable?(skip_discussions_check: skip_discussions_check)).to eq(expected_mergeable)
+        end
+      end
+    end
   end
 
   describe '#check_mergeability' do
@@ -2570,6 +2621,10 @@ RSpec.describe MergeRequest, factory_default: :keep do
         it 'returns false' do
           expect(subject.mergeable_state?).to be_falsey
         end
+
+        it 'returns true when skipping ci check' do
+          expect(subject.mergeable_state?(skip_ci_check: true)).to be(true)
+        end
       end
 
       context 'when #mergeable_discussions_state? is false' do

spec/services/merge_requests/merge_service_spec.rb

@@ -435,6 +435,43 @@ RSpec.describe MergeRequests::MergeService do
           end
         end
       end
+
+      context 'when not mergeable' do
+        let!(:error_message) { 'Merge request is not mergeable' }
+
+        context 'with failing CI' do
+          before do
+            allow(merge_request).to receive(:mergeable_ci_state?) { false }
+          end
+
+          it 'logs and saves error' do
+            service.execute(merge_request)
+
+            expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message))
+          end
+        end
+
+        context 'with unresolved discussions' do
+          before do
+            allow(merge_request).to receive(:mergeable_discussions_state?) { false }
+          end
+
+          it 'logs and saves error' do
+            service.execute(merge_request)
+
+            expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message))
+          end
+
+          context 'when passing `skip_discussions_check: true` as `options` parameter' do
+            it 'merges the merge request' do
+              service.execute(merge_request, skip_discussions_check: true)
+
+              expect(merge_request).to be_valid
+              expect(merge_request).to be_merged
+            end
+          end
+        end
+      end
     end
   end
 end