Merge branch 'preserve-merge-train-rows-after-merge' into 'master'

Preserve merge train history

See merge request gitlab-org/gitlab!19864

app/models/merge_request.rb

@@ -27,8 +27,6 @@ class MergeRequest < ApplicationRecord
 
   SORTING_PREFERENCE_FIELD = :merge_requests_sort
 
-  prepend_if_ee('::EE::MergeRequest') # rubocop: disable Cop/InjectEnterpriseEditionModule
-
   belongs_to :target_project, class_name: "Project"
   belongs_to :source_project, class_name: "Project"
   belongs_to :merge_user, class_name: "User"
@@ -1531,3 +1529,5 @@ class MergeRequest < ApplicationRecord
     Gitlab::EtagCaching::Store.new.touch(key)
   end
 end
+
+MergeRequest.prepend_if_ee('::EE::MergeRequest')

changelogs/unreleased/preserve-merge-train-rows-after-merge.yml

+---
+title: Preserve merge train history
+merge_request: 19864
+author:
+type: changed

db/migrate/20191106144901_add_state_to_merge_trains.rb

+# frozen_string_literal: true
+
+class AddStateToMergeTrains < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  MERGE_TRAIN_STATUS_CREATED = 0 # Equivalent to MergeTrain.statuses[:created]
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default :merge_trains, :status, :integer, limit: 2, default: MERGE_TRAIN_STATUS_CREATED
+  end
+
+  def down
+    remove_column :merge_trains, :status
+  end
+end

db/migrate/20191118155702_add_index_on_status_to_merge_trains.rb

+# frozen_string_literal: true
+
+class AddIndexOnStatusToMergeTrains < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_for_status_per_branch_per_project'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :merge_trains, [:target_project_id, :target_branch, :status], name: INDEX_NAME
+    remove_concurrent_index :merge_trains, :target_project_id
+  end
+
+  def down
+    add_concurrent_index :merge_trains, :target_project_id
+    remove_concurrent_index :merge_trains, [:target_project_id, :target_branch, :status], name: INDEX_NAME
+  end
+end

db/schema.rb

@@ -2525,9 +2525,10 @@ ActiveRecord::Schema.define(version: 2019_12_02_031812) do
     t.datetime_with_timezone "updated_at", null: false
     t.integer "target_project_id", null: false
     t.text "target_branch", null: false
+    t.integer "status", limit: 2, default: 0, null: false
     t.index ["merge_request_id"], name: "index_merge_trains_on_merge_request_id", unique: true
     t.index ["pipeline_id"], name: "index_merge_trains_on_pipeline_id"
-    t.index ["target_project_id"], name: "index_merge_trains_on_target_project_id"
+    t.index ["target_project_id", "target_branch", "status"], name: "index_for_status_per_branch_per_project"
     t.index ["user_id"], name: "index_merge_trains_on_user_id"
   end
 

ee/app/models/ee/merge_request.rb

@@ -23,7 +23,7 @@ module EE
       has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approval_rules, class_name: 'ApprovalMergeRequestRule', inverse_of: :merge_request
       has_many :draft_notes
-      has_one :merge_train, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+      has_one :merge_train, inverse_of: :merge_request, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
       has_many :blocks_as_blocker,
                class_name: 'MergeRequestBlock',
@@ -44,6 +44,14 @@ module EE
       participant :participant_approvers
 
       accepts_nested_attributes_for :approval_rules, allow_destroy: true
+
+      state_machine :state_id do
+        after_transition any => :merged do |merge_request|
+          merge_request.merge_train&.merged!
+
+          true
+        end
+      end
     end
 
     class_methods do

ee/app/models/merge_train.rb

@@ -4,21 +4,32 @@ class MergeTrain < ApplicationRecord
   include AfterCommitQueue
 
   belongs_to :target_project, class_name: "Project"
-  belongs_to :merge_request
+  belongs_to :merge_request, inverse_of: :merge_train
   belongs_to :user
   belongs_to :pipeline, class_name: 'Ci::Pipeline'
 
+  after_commit :cleanup_ref, if: -> { saved_change_to_status? && merged? }
+
   after_destroy do |merge_train|
-    run_after_commit { merge_train.merge_request.cleanup_refs(only: :train) }
+    run_after_commit { merge_train.cleanup_ref }
   end
 
+  enum status: %i[created merged]
+
+  scope :active, -> { where(status: active_statuses) }
+  scope :merged, -> { where(status: merged_statuses) }
+  scope :for_target, -> (project_id, branch) { where(target_project_id: project_id, target_branch: branch) }
+  scope :by_id, -> { order('merge_trains.id ASC') }
+
   class << self
-    def all_in_train(merge_request)
-      joined_merge_requests(merge_request).order('merge_trains.id ASC')
+    def all_active_mrs_in_train(merge_request)
+      MergeRequest.joins(:merge_train).merge(
+        MergeTrain.active.for_target(merge_request.target_project_id, merge_request.target_branch).by_id
+      )
     end
 
     def first_in_train(merge_request)
-      all_in_train(merge_request).first
+      all_active_mrs_in_train(merge_request).first
     end
 
     def first_in_trains(project)
@@ -27,34 +38,46 @@ class MergeTrain < ApplicationRecord
 
     def first_in_train_from(merge_request_ids)
       merge_request = MergeRequest.find(merge_request_ids.first)
-      all_in_train(merge_request).where(id: merge_request_ids).first
+      all_active_mrs_in_train(merge_request).where(id: merge_request_ids).first
+    end
+
+    def last_merged_mr_in_train(target_project_id, target_branch)
+      MergeRequest.where(id: last_merged_merge_train(target_project_id, target_branch)).take
     end
 
     def total_count_in_train(merge_request)
-      all_in_train(merge_request).count
+      all_active_mrs_in_train(merge_request).count
     end
 
-    def joined_merge_requests(merge_request)
-      MergeRequest.joins(:merge_train)
-        .where('merge_requests.target_project_id = ?', merge_request.target_project_id)
-        .where('merge_requests.target_branch = ?', merge_request.target_branch)
+    def active_statuses
+      statuses.values_at(:created)
+    end
+
+    def merged_statuses
+      statuses.values_at(:merged)
     end
 
     private
 
     def first_merge_request_ids(project)
       MergeTrain.where(target_project: project)
+        .active
         .select('DISTINCT ON (target_branch) merge_request_id')
         .order(:target_branch, :id)
     end
+
+    def last_merged_merge_train(target_project_id, target_branch)
+      MergeTrain.for_target(target_project_id, target_branch)
+        .merged.order(id: :desc).select(:merge_request_id).limit(1)
+    end
   end
 
   def all_next
-    self.class.all_in_train(merge_request).where('merge_trains.id > ?', id)
+    self.class.all_active_mrs_in_train(merge_request).where('merge_trains.id > ?', id)
   end
 
   def all_prev
-    self.class.all_in_train(merge_request).where('merge_trains.id < ?', id)
+    self.class.all_active_mrs_in_train(merge_request).where('merge_trains.id < ?', id)
   end
 
   def next
@@ -76,4 +99,8 @@ class MergeTrain < ApplicationRecord
   def follower_in_train?
     all_prev.exists?
   end
+
+  def cleanup_ref
+    merge_request.cleanup_refs(only: :train)
+  end
 end

ee/app/services/merge_trains/refresh_merge_request_service.rb

@@ -77,9 +77,7 @@ module MergeTrains
       MergeRequests::MergeService.new(project, merge_user, merge_request.merge_params)
                                  .execute(merge_request)
 
-      raise ProcessError, 'failed to merge' unless merge_request.merged?
-
-      merge_train.destroy
+      raise ProcessError, "failed to merge. #{merge_request.merge_error}" unless merge_request.merged?
     end
 
     # NOTE: This method works for both no-ff-merge and ff-merge, however,

ee/spec/factories/merge_trains.rb

@@ -7,5 +7,9 @@ FactoryBot.define do
     merge_request
     user
     pipeline factory: :ci_pipeline
+
+    trait :merged do
+      status { :merged }
+    end
   end
 end

ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb

@@ -92,7 +92,7 @@ describe 'Two merge requests on a merge train' do
       it 'merges merge request 2', :sidekiq_might_not_need_inline do
         expect(merge_request_2).to be_merged
         expect(merge_request_2.metrics.merged_by).to eq(maintainer_2)
-        expect(merge_request_2.merge_train).to be_nil
+        expect(merge_request_2.merge_train).to be_merged
       end
     end
   end
@@ -114,7 +114,7 @@ describe 'Two merge requests on a merge train' do
       it 'merges merge request 2', :sidekiq_might_not_need_inline do
         expect(merge_request_2).to be_merged
         expect(merge_request_2.metrics.merged_by).to eq(maintainer_2)
-        expect(merge_request_2.merge_train).to be_nil
+        expect(merge_request_2.merge_train).to be_merged
       end
     end
   end
@@ -130,7 +130,7 @@ describe 'Two merge requests on a merge train' do
     it 'merges merge request 1', :sidekiq_might_not_need_inline do
       expect(merge_request_1).to be_merged
       expect(merge_request_1.metrics.merged_by).to eq(maintainer_1)
-      expect(merge_request_1.merge_train).to be_nil
+      expect(merge_request_1.merge_train).to be_merged
     end
 
     it_behaves_like 'has an intact pipeline for merge request 2'

ee/spec/models/merge_request_spec.rb

@@ -719,4 +719,32 @@ describe MergeRequest do
       it { is_expected.to be_falsy }
     end
   end
+
+  describe 'state machine' do
+    context 'when the merge request is on a merge train' do
+      let(:merge_request) do
+        create(:merge_request, :on_train, source_project: project, target_project: project)
+      end
+
+      context 'when the merge request is merged' do
+        it 'ensures to finish merge train' do
+          expect(merge_request.merge_train).to receive(:merged!)
+
+          merge_request.mark_as_merged!
+        end
+      end
+    end
+
+    context 'when the merge request is not on a merge train' do
+      let(:merge_request) do
+        create(:merge_request, source_project: project, target_project: project)
+      end
+
+      context 'when the merge request is merged' do
+        it 'does not raise error' do
+          expect { merge_request.mark_as_merged! }.not_to raise_error
+        end
+      end
+    end
+  end
 end

ee/spec/models/merge_train_spec.rb

@@ -15,8 +15,66 @@ describe MergeTrain do
     allow(AutoMergeProcessWorker).to receive(:perform_async)
   end
 
-  describe '.all_in_train' do
-    subject { described_class.all_in_train(merge_request) }
+  describe '.active' do
+    subject { described_class.active }
+
+    let!(:merge_train_created) { create(:merge_train) }
+    let!(:merge_train_merged) { create(:merge_train, :merged) }
+
+    it 'returns only active merge trains' do
+      is_expected.to contain_exactly(merge_train_created)
+    end
+  end
+
+  describe '.merged' do
+    subject { described_class.merged }
+
+    let!(:merge_train_created) { create(:merge_train) }
+    let!(:merge_train_merged) { create(:merge_train, :merged) }
+
+    it 'returns only merged merge trains' do
+      is_expected.to contain_exactly(merge_train_merged)
+    end
+  end
+
+  describe '.for_target' do
+    subject { described_class.for_target(project_id, branch) }
+
+    let!(:merge_train_1) { create(:merge_train) }
+    let!(:merge_train_2) { create(:merge_train) }
+
+    context "when target merge train 1's project" do
+      let(:project_id) { merge_train_1.target_project_id }
+      let(:branch) { merge_train_1.target_branch }
+
+      it 'returns merge train 1 only' do
+        is_expected.to eq([merge_train_1])
+      end
+    end
+
+    context "when target merge train 2's project" do
+      let(:project_id) { merge_train_2.target_project_id }
+      let(:branch) { merge_train_2.target_branch }
+
+      it 'returns merge train 2 only' do
+        is_expected.to eq([merge_train_2])
+      end
+    end
+  end
+
+  describe '.by_id' do
+    subject { described_class.by_id }
+
+    let!(:merge_train_1) { create(:merge_train, target_project: project, target_branch: 'master') }
+    let!(:merge_train_2) { create(:merge_train, target_project: project, target_branch: 'master') }
+
+    it 'returns merge trains by id ASC' do
+      is_expected.to eq([merge_train_1, merge_train_2])
+    end
+  end
+
+  describe '.all_active_mrs_in_train' do
+    subject { described_class.all_active_mrs_in_train(merge_request) }
 
     let!(:merge_request) { create_merge_request_on_train }
 
@@ -27,11 +85,19 @@ describe MergeTrain do
     context 'when the other merge request is on the merge train' do
       let!(:merge_request_2) { create_merge_request_on_train(source_branch: 'improve/awesome') }
 
-      it 'returns the merge request' do
+      it 'returns the merge requests' do
         is_expected.to eq([merge_request, merge_request_2])
       end
     end
 
+    context 'when the merge request has already been merged' do
+      before do
+        merge_request.merge_train.merged!
+      end
+
+      it { is_expected.to be_empty }
+    end
+
     context 'when the merge request is not on merge train' do
       let(:merge_request) { create(:merge_request) }
 
@@ -58,6 +124,14 @@ describe MergeTrain do
       end
     end
 
+    context 'when the merge request has already been merged' do
+      before do
+        merge_request.merge_train.merged!
+      end
+
+      it { is_expected.to be_nil }
+    end
+
     context 'when the merge request is not on merge train' do
       let(:merge_request) { create(:merge_request) }
 
@@ -79,6 +153,16 @@ describe MergeTrain do
     it 'returns only first merge requests per merge train' do
       is_expected.to contain_exactly(first_on_master, first_on_stable)
     end
+
+    context 'when first_on_master has already been merged' do
+      before do
+        first_on_master.merge_train.merged!
+      end
+
+      it 'returns second on master as active MR' do
+        is_expected.to contain_exactly(second_on_master, first_on_stable)
+      end
+    end
   end
 
   describe '.first_in_train_from' do
@@ -101,6 +185,16 @@ describe MergeTrain do
         is_expected.to eq(merge_request_1)
       end
 
+      context 'when the first merge request has already been merged' do
+        before do
+          merge_request_1.merge_train.merged!
+        end
+
+        it 'returns the first active merge request on the merge train from the given ids' do
+          is_expected.to eq(merge_request_2)
+        end
+      end
+
       context "when specifies merge request 2's id only" do
         let(:merge_request_ids) { [merge_request_2.id] }
 
@@ -111,6 +205,51 @@ describe MergeTrain do
     end
   end
 
+  describe '.last_merged_mr_in_train' do
+    subject { described_class.last_merged_mr_in_train(target_project_id, target_branch) }
+
+    let(:target_project_id) { project.id }
+    let(:target_branch) { 'master' }
+
+    context 'when there is a merge request on train' do
+      let!(:merge_request_1) { create_merge_request_on_train }
+
+      context 'when the merge request has already been merged' do
+        before do
+          merge_request_1.merge_train.merged!
+        end
+
+        it 'returns the merge request' do
+          is_expected.to eq(merge_request_1)
+        end
+
+        context 'when there is another merge request on train and it has been merged' do
+          let!(:merge_request_2) { create_merge_request_on_train(source_branch: 'improve/awesome') }
+
+          before do
+            merge_request_2.merge_train.merged!
+          end
+
+          it 'returns the last merge request' do
+            is_expected.to eq(merge_request_2)
+          end
+        end
+      end
+
+      context 'when the merge request has not been merged yet' do
+        it 'returns nothing' do
+          is_expected.to be_nil
+        end
+      end
+    end
+
+    context 'when there are no merge requests on train' do
+      it 'returns nothing' do
+        is_expected.to be_nil
+      end
+    end
+  end
+
   describe '.total_count_in_train' do
     subject { described_class.total_count_in_train(merge_request) }
 
@@ -128,6 +267,16 @@ describe MergeTrain do
       end
     end
 
+    context 'when the merge request has already been merged' do
+      before do
+        merge_request.merge_train.merged!
+      end
+
+      it 'returns zero' do
+        is_expected.to be(0)
+      end
+    end
+
     context 'when the merge request is not on merge train' do
       let(:merge_request) { create(:merge_request) }
 
@@ -175,6 +324,16 @@ describe MergeTrain do
       it 'returns the previous merge requests' do
         is_expected.to eq([merge_request])
       end
+
+      context 'when the previous merge request has already been merged' do
+        before do
+          merge_request.merge_train.merged!
+        end
+
+        it 'returns empty array' do
+          is_expected.to be_empty
+        end
+      end
     end
   end
 
@@ -269,6 +428,44 @@ describe MergeTrain do
     end
   end
 
+  describe 'status transition' do
+    context 'when status is created' do
+      let(:merge_train) { create(:merge_train) }
+
+      context 'and transits to merged' do
+        it 'cleanup ref' do
+          expect(merge_train).to receive(:cleanup_ref).once
+
+          merge_train.merged!
+        end
+      end
+    end
+
+    context 'when status is merged' do
+      let(:merge_train) { create(:merge_train, :merged) }
+
+      context 'and transits to merged' do
+        it 'does not cleanup ref' do
+          expect(merge_train).not_to receive(:cleanup_ref)
+
+          merge_train.merged!
+        end
+      end
+    end
+  end
+
+  describe '#cleanup_ref' do
+    subject { merge_train.cleanup_ref }
+
+    let(:merge_train) { create(:merge_train) }
+
+    it 'executes cleanup_refs for merge request' do
+      expect(merge_train.merge_request).to receive(:cleanup_refs).with(only: :train)
+
+      subject
+    end
+  end
+
   def create_merge_request_on_train(target_project: project, target_branch: 'master', source_project: project, source_branch: 'feature')
     create(:merge_request,
       :on_train,

ee/spec/services/merge_trains/refresh_merge_request_service_spec.rb

@@ -28,7 +28,7 @@ describe MergeTrains::RefreshMergeRequestService do
 
       it do
         expect_next_instance_of(AutoMerge::MergeTrainService) do |service|
-          expect(service).to receive(:abort).with(merge_request, kind_of(String), hash_including(process_next: false))
+          expect(service).to receive(:abort).with(merge_request, expected_reason, hash_including(process_next: false))
         end
 
         subject
@@ -80,9 +80,9 @@ describe MergeTrains::RefreshMergeRequestService do
       end
     end
 
-    context 'when merge train project configuration is disabled' do
+    context 'when merge pipelines project configuration is disabled' do
       before do
-        project.update!(merge_trains_enabled: false)
+        project.update!(merge_pipelines_enabled: false)
       end
 
       it_behaves_like 'drops the merge request from the merge train' do
@@ -90,7 +90,7 @@ describe MergeTrains::RefreshMergeRequestService do
       end
 
       after do
-        project.update!(merge_trains_enabled: true)
+        project.update!(merge_pipelines_enabled: true)
       end
     end
 
@@ -186,26 +186,31 @@ describe MergeTrains::RefreshMergeRequestService do
       let(:previous_ref_sha) { project.repository.commit('refs/heads/master').sha }
 
       before do
-        merge_request.merge_train.update!(pipeline: pipeline)
+        merge_request.merge_train.pipeline = pipeline
+        merge_request.merge_params[:sha] = merge_request.diff_head_sha
+        merge_request.save
       end
 
       context 'when the merge request is the first queue' do
         it 'merges the merge request' do
           expect(merge_request).to receive(:cleanup_refs).with(only: :train)
           expect_next_instance_of(MergeRequests::MergeService, project, maintainer, anything) do |service|
-            expect(service).to receive(:execute).with(merge_request)
+            expect(service).to receive(:execute).with(merge_request).and_call_original
           end
 
-          expect { subject }.to change { MergeTrain.count }.by(-1)
+          expect { subject }
+            .to change { merge_request.merge_train.status }.from('created').to('merged')
         end
 
         context 'when it failed to merge the merge request' do
           before do
+            allow(merge_request).to receive(:mergeable_state?) { true }
+            merge_request.update(merge_error: 'Branch has been updated since the merge was requested.')
             allow_any_instance_of(MergeRequests::MergeService).to receive(:execute) { { result: :error } }
           end
 
           it_behaves_like 'drops the merge request from the merge train' do
-            let(:expected_reason) { 'failed to merge' }
+            let(:expected_reason) { 'failed to merge. Branch has been updated since the merge was requested.' }
           end
         end
       end