Merge branch 'move-domain-logic-to-merge-train' into 'master'

Move domain logic from RefreshMergeRequestService to MergeTrain class

See merge request gitlab-org/gitlab!49520

ee/app/models/merge_train.rb

 # frozen_string_literal: true
 
 class MergeTrain < ApplicationRecord
+  include Gitlab::Utils::StrongMemoize
   include AfterCommitQueue
 
   ACTIVE_STATUSES = %w[idle stale fresh].freeze
@@ -11,6 +12,8 @@ class MergeTrain < ApplicationRecord
   belongs_to :user
   belongs_to :pipeline, class_name: 'Ci::Pipeline'
 
+  alias_attribute :project, :target_project
+
   after_destroy do |merge_train|
     run_after_commit do
       merge_train.pipeline&.cancel_running(retries: 1)
@@ -148,12 +151,32 @@ class MergeTrain < ApplicationRecord
     all_prev.count
   end
 
-  def first_in_train?
-    !follower_in_train?
+  def previous_ref
+    prev&.train_ref_path || merge_request.target_branch_ref
+  end
+
+  def previous_ref_sha
+    project.repository.commit(previous_ref)&.sha
+  end
+
+  def requires_new_pipeline?
+    !has_pipeline? || stale?
+  end
+
+  def pipeline_not_succeeded?
+    has_pipeline? && pipeline.complete? && !pipeline.success?
   end
 
-  def follower_in_train?
-    all_prev.exists?
+  def cancel_pipeline!(new_pipeline)
+    pipeline&.auto_cancel_running(new_pipeline, retries: 1)
+  rescue ActiveRecord::StaleObjectError
+    # Often the pipeline has already been canceled by the default cancellation
+    # mechanizm `Ci::CreatePipelineService#cancel_pending_pipelines`. In this
+    # case, we can ignore the exception as it's already canceled.
+  end
+
+  def mergeable?
+    has_pipeline? && pipeline&.success? && first_in_train?
   end
 
   def cleanup_ref
@@ -167,4 +190,14 @@ class MergeTrain < ApplicationRecord
   def refresh_async
     AutoMergeProcessWorker.perform_async(merge_request_id)
   end
+
+  private
+
+  def has_pipeline?
+    pipeline_id.present? && pipeline
+  end
+
+  def first_in_train?
+    all_prev.none?
+  end
 end

ee/app/services/merge_trains/refresh_merge_request_service.rb

@@ -14,8 +14,8 @@ module MergeTrains
       @merge_request = merge_request
 
       validate!
-      pipeline_created = create_pipeline! if should_create_pipeline?
-      merge! if should_merge?
+      pipeline_created = create_pipeline! if merge_train.requires_new_pipeline? || require_recreate?
+      merge! if merge_train.mergeable?
 
       success(pipeline_created: pipeline_created.present?)
     rescue ProcessError => e
@@ -37,33 +37,26 @@ module MergeTrains
         raise ProcessError, 'merge request is not mergeable'
       end
 
-      unless previous_ref_exist?
+      unless merge_train.previous_ref_sha.present?
         raise ProcessError, 'previous ref does not exist'
       end
 
-      if pipeline_for_merge_train
-        if pipeline_for_merge_train.complete? && !pipeline_for_merge_train.success?
-          raise ProcessError, 'pipeline did not succeed'
-        end
+      if merge_train.pipeline_not_succeeded?
+        raise ProcessError, 'pipeline did not succeed'
       end
     end
 
-    def should_create_pipeline?
-      pipeline_absent? || require_recreate? || stale_pipeline?
-    end
-
     def create_pipeline!
-      result = MergeTrains::CreatePipelineService.new(merge_request.project, merge_user)
-        .execute(merge_request, previous_ref)
+      result = MergeTrains::CreatePipelineService.new(merge_train.project, merge_train.user)
+        .execute(merge_train.merge_request, merge_train.previous_ref)
 
       raise ProcessError, result[:message] unless result[:status] == :success
 
-      cancel_pipeline_for_merge_train(result[:pipeline])
-      update_pipeline_for_merge_train(result[:pipeline])
-    end
+      pipeline = result[:pipeline]
+      merge_train.cancel_pipeline!(pipeline)
+      merge_train.refresh_pipeline!(pipeline.id)
 
-    def should_merge?
-      pipeline_for_merge_train&.success? && first_in_train?
+      pipeline
     end
 
     def merge!
@@ -77,64 +70,14 @@ module MergeTrains
       merge_train.finish_merge!
     end
 
-    def stale_pipeline?
-      merge_train.stale?
-    end
-
-    def pipeline_absent?
-      !pipeline_for_merge_train.present?
-    end
-
     def merge_train
       merge_request.merge_train
     end
 
-    def pipeline_for_merge_train
-      merge_train.pipeline
-    end
-
-    def cancel_pipeline_for_merge_train(new_pipeline)
-      pipeline_for_merge_train&.auto_cancel_running(new_pipeline, retries: 1)
-    rescue ActiveRecord::StaleObjectError
-      # Often the pipeline has already been canceled by the default cancelaltion
-      # mechanizm `Ci::CreatePipelineService#cancel_pending_pipelines`. In this
-      # case, we can ignore the exception as it's already canceled.
-    end
-
-    def update_pipeline_for_merge_train(pipeline)
-      merge_train.refresh_pipeline!(pipeline.id)
-    end
-
     def merge_user
       merge_request.merge_user
     end
 
-    def first_in_train?
-      strong_memoize(:is_first_in_train) do
-        merge_train.first_in_train?
-      end
-    end
-
-    def previous_ref_sha
-      strong_memoize(:previous_ref_sha) do
-        merge_request.project.repository.commit(previous_ref)&.sha
-      end
-    end
-
-    def previous_ref
-      previous_merge_request&.train_ref_path || merge_request.target_branch_ref
-    end
-
-    def previous_ref_exist?
-      previous_ref_sha.present?
-    end
-
-    def previous_merge_request
-      strong_memoize(:previous_merge_request) do
-        merge_request.merge_train.prev
-      end
-    end
-
     def require_recreate?
       params[:require_recreate]
     end

ee/spec/models/merge_train_spec.rb

@@ -434,38 +434,151 @@ RSpec.describe MergeTrain do
     end
   end
 
-  describe '#first_in_train?' do
-    subject { merge_train.first_in_train? }
+  describe '#previous_ref' do
+    subject { merge_train.previous_ref }
 
     let(:merge_train) { merge_request.merge_train }
     let!(:merge_request) { create_merge_request_on_train }
 
-    it { is_expected.to be_truthy }
+    context 'when merge request is first on train' do
+      it 'returns the target branch' do
+        is_expected.to eq(merge_request.target_branch_ref)
+      end
+    end
 
-    context 'when the other merge request is on the merge train' do
+    context 'when merge request is not first on train' do
       let(:merge_train) { merge_request_2.merge_train }
-      let!(:merge_request_2) { create_merge_request_on_train(source_branch: 'improve/awesome') }
+      let!(:merge_request_2) { create_merge_request_on_train(source_branch: 'feature-2') }
 
-      it { is_expected.to be_falsy }
+      it 'returns the ref of the previous merge request' do
+        is_expected.to eq(merge_request.train_ref_path)
+      end
     end
   end
 
-  describe '#follower_in_train?' do
-    subject { merge_train.follower_in_train? }
+  describe '#requires_new_pipeline?' do
+    subject { merge_train.requires_new_pipeline? }
 
     let(:merge_train) { merge_request.merge_train }
     let!(:merge_request) { create_merge_request_on_train }
 
-    it { is_expected.to be_falsy }
+    context 'when merge train has a pipeline associated' do
+      before do
+        merge_train.update!(pipeline: create(:ci_pipeline, project: merge_train.project))
+      end
 
-    context 'when the other merge request is on the merge train' do
-      let(:merge_train) { merge_request_2.merge_train }
-      let!(:merge_request_2) { create_merge_request_on_train(source_branch: 'improve/awesome') }
+      it { is_expected.to be_falsey }
+
+      context 'when merge train is stale' do
+        before do
+          merge_train.update!(status: MergeTrain.state_machines[:status].states[:stale].value)
+        end
+
+        it { is_expected.to be_truthy }
+      end
+    end
+
+    context 'when merge train does not have a pipeline' do
+      before do
+        merge_train.update!(pipeline: nil)
+      end
 
       it { is_expected.to be_truthy }
     end
   end
 
+  describe '#pipeline_not_succeeded?' do
+    subject { merge_train.pipeline_not_succeeded? }
+
+    let(:merge_train) { merge_request.merge_train }
+    let!(:merge_request) { create_merge_request_on_train }
+
+    context 'when merge train does not have a pipeline' do
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when merge train has a pipeline' do
+      let(:pipeline) { create(:ci_pipeline, project: merge_train.project, status: status) }
+
+      before do
+        merge_train.update!(pipeline: pipeline)
+      end
+
+      context 'when pipeline failed' do
+        let(:status) { :failed }
+
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when pipeline succeeded' do
+        let(:status) { :success }
+
+        it { is_expected.to be_falsey }
+      end
+
+      context 'when pipeline is running' do
+        let(:status) { :running }
+
+        it { is_expected.to be_falsey }
+      end
+    end
+  end
+
+  describe '#cancel_pipeline!' do
+    subject { merge_train.cancel_pipeline!(new_pipeline) }
+
+    let(:merge_train) { merge_request.merge_train }
+    let!(:merge_request) { create_merge_request_on_train }
+    let!(:pipeline) { create(:ci_pipeline, project: merge_train.project) }
+    let!(:new_pipeline) { create(:ci_pipeline, project: merge_train.project) }
+
+    before do
+      merge_train.update!(pipeline: pipeline)
+    end
+
+    it 'cancels the existing pipeline' do
+      expect(pipeline).to receive(:cancel_running).and_call_original
+
+      subject
+
+      expect(pipeline.reload.auto_canceled_by).to eq(new_pipeline)
+    end
+  end
+
+  describe '#mergeable?' do
+    subject { merge_train.mergeable? }
+
+    let(:merge_train) { merge_request.merge_train }
+    let!(:merge_request) { create_merge_request_on_train }
+
+    context 'when merge train has successful pipeline' do
+      before do
+        merge_train.update!(pipeline: create(:ci_pipeline, :success, project: merge_request.project))
+      end
+
+      context 'when merge request is first on train' do
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when the other merge request is on the merge train' do
+        let(:merge_train) { merge_request_2.merge_train }
+        let!(:merge_request_2) { create_merge_request_on_train(source_branch: 'improve/awesome') }
+
+        it { is_expected.to be_falsy }
+      end
+    end
+
+    context 'when merge train has non successful pipeline' do
+      before do
+        merge_train.update!(pipeline: create(:ci_pipeline, :failed, project: merge_request.project))
+      end
+
+      context 'when merge request is first on train' do
+        it { is_expected.to be_falsey }
+      end
+    end
+  end
+
   describe '#index' do
     subject { merge_train.index }
 

ee/spec/services/merge_trains/refresh_merge_request_service_spec.rb

@@ -138,7 +138,7 @@ RSpec.describe MergeTrains::RefreshMergeRequestService do
       let(:previous_ref) { 'refs/tmp/test' }
 
       before do
-        allow(service).to receive(:previous_ref) { previous_ref }
+        allow(merge_request.merge_train).to receive(:previous_ref) { previous_ref }
       end
 
       it_behaves_like 'drops the merge request from the merge train' do