Merge branch '220916-new-comment-not-drop-merge-train' into 'master'

Prevent MRs to be dropped from Merge Trains for open discussions

See merge request gitlab-org/gitlab!39957

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

@@ -2473,6 +2473,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
@@ -2576,6 +2627,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