Merge branch '9688-mr-merge-order' into 'master'

Backend for blocking merge requests MVC

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

db/migrate/20190412155659_add_merge_request_blocks.rb

+# frozen_string_literal: true
+
+class AddMergeRequestBlocks < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table :merge_request_blocks, id: :bigserial do |t|
+      t.references :blocking_merge_request,
+                   index: false, null: false,
+                   foreign_key: { to_table: :merge_requests, on_delete: :cascade }
+
+      t.references :blocked_merge_request,
+                   index: true, null: false,
+                   foreign_key: { to_table: :merge_requests, on_delete: :cascade }
+
+      t.index [:blocking_merge_request_id, :blocked_merge_request_id],
+              unique: true,
+              name: 'index_mr_blocks_on_blocking_and_blocked_mr_ids'
+
+      t.timestamps_with_timezone
+    end
+  end
+end

db/schema.rb

@@ -1814,6 +1814,15 @@ ActiveRecord::Schema.define(version: 20190506135400) do
     t.index ["user_id"], name: "index_merge_request_assignees_on_user_id", using: :btree
   end
 
+  create_table "merge_request_blocks", force: :cascade do |t|
+    t.integer "blocking_merge_request_id", null: false
+    t.integer "blocked_merge_request_id", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
+    t.index ["blocked_merge_request_id"], name: "index_merge_request_blocks_on_blocked_merge_request_id", using: :btree
+    t.index ["blocking_merge_request_id", "blocked_merge_request_id"], name: "index_mr_blocks_on_blocking_and_blocked_mr_ids", unique: true, using: :btree
+  end
+
   create_table "merge_request_diff_commits", id: false, force: :cascade do |t|
     t.datetime "authored_date"
     t.datetime "committed_date"
@@ -3639,6 +3648,8 @@ ActiveRecord::Schema.define(version: 20190506135400) do
   add_foreign_key "members", "users", name: "fk_2e88fb7ce9", on_delete: :cascade
   add_foreign_key "merge_request_assignees", "merge_requests", on_delete: :cascade
   add_foreign_key "merge_request_assignees", "users", on_delete: :cascade
+  add_foreign_key "merge_request_blocks", "merge_requests", column: "blocked_merge_request_id", on_delete: :cascade
+  add_foreign_key "merge_request_blocks", "merge_requests", column: "blocking_merge_request_id", on_delete: :cascade
   add_foreign_key "merge_request_diff_commits", "merge_request_diffs", on_delete: :cascade
   add_foreign_key "merge_request_diff_files", "merge_request_diffs", on_delete: :cascade
   add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade

ee/app/models/ee/merge_request.rb

@@ -23,6 +23,18 @@ module EE
       has_many :draft_notes
       has_one :merge_train
 
+      has_many :blocks_as_blocker,
+               class_name: 'MergeRequestBlock',
+               foreign_key: :blocking_merge_request_id
+
+      has_many :blocks_as_blockee,
+               class_name: 'MergeRequestBlock',
+               foreign_key: :blocked_merge_request_id
+
+      has_many :blocking_merge_requests, through: :blocks_as_blockee
+
+      has_many :blocked_merge_requests, through: :blocks_as_blocker
+
       validate :validate_approval_rule_source
 
       delegate :sha, to: :head_pipeline, prefix: :head_pipeline, allow_nil: true
@@ -38,15 +50,28 @@ module EE
       def select_from_union(relations)
         where(id: from_union(relations))
       end
+
+      # This is an ActiveRecord scope in CE
+      def with_api_entity_associations
+        super.preload(:blocking_merge_requests)
+      end
     end
 
     override :mergeable?
     def mergeable?(skip_ci_check: false)
       return false unless approved?
+      return false if merge_blocked_by_other_mrs?
 
       super
     end
 
+    def merge_blocked_by_other_mrs?
+      strong_memoize(:merge_blocked_by_other_mrs) do
+        project.feature_available?(:blocking_merge_requests) &&
+          blocking_merge_requests.any? { |mr| !mr.merged? }
+      end
+    end
+
     override :mergeable_ci_state?
     def mergeable_ci_state?
       return false unless validate_merge_request_pipelines

ee/app/models/license.rb

@@ -39,6 +39,7 @@ class License < ApplicationRecord
   EEP_FEATURES = EES_FEATURES + %i[
     admin_audit_log
     auditor_user
+    blocking_merge_requests
     board_assignee_lists
     board_milestone_lists
     cross_project_pipelines

ee/app/models/merge_request_block.rb

+# frozen_string_literal: true
+
+class MergeRequestBlock < ApplicationRecord
+  belongs_to :blocking_merge_request, class_name: 'MergeRequest'
+  belongs_to :blocked_merge_request, class_name: 'MergeRequest'
+
+  validates_presence_of :blocking_merge_request
+  validates_presence_of :blocked_merge_request
+  validates_uniqueness_of [:blocking_merge_request, :blocked_merge_request]
+
+  validate :check_block_constraints
+
+  private
+
+  def check_block_constraints
+    return unless blocking_merge_request && blocked_merge_request
+
+    errors.add(:base, _('This block is self-referential')) if
+      blocking_merge_request == blocked_merge_request
+
+    errors.add(:blocking_merge_request, _('cannot itself be blocked')) if
+      blocking_merge_request.blocks_as_blockee.any?
+
+    errors.add(:blocked_merge_request, _('cannot block others')) if
+      blocked_merge_request.blocks_as_blocker.any?
+  end
+end

ee/app/services/ee/merge_requests/merge_base_service.rb

@@ -8,6 +8,7 @@ module EE
       override :error_check!
       def error_check!
         check_size_limit
+        check_blocking_mrs
       end
 
       override :hooks_validation_pass?
@@ -54,6 +55,12 @@ module EE
           raise ::MergeRequests::MergeService::MergeError, message
         end
       end
+
+      def check_blocking_mrs
+        return unless merge_request.merge_blocked_by_other_mrs?
+
+        raise ::MergeRequests::MergeService::MergeError, _('Other merge requests block this MR')
+      end
     end
   end
 end

ee/changelogs/unreleased/9688-mr-merge-order.yml

+---
+title: Allow merge requests to block other MRs from being merged
+merge_request: 11600
+author:
+type: added

ee/spec/factories/merge_request_blocks.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :merge_request_block do
+    blocking_merge_request { create(:merge_request) }
+    blocked_merge_request { create(:merge_request) }
+  end
+end

ee/spec/lib/gitlab/import_export/all_models.yml

@@ -15,6 +15,10 @@ merge_requests:
 - approved_by_users
 - draft_notes
 - merge_train
+- blocks_as_blocker
+- blocks_as_blockee
+- blocking_merge_requests
+- blocked_merge_requests
 ci_pipelines:
 - source_pipeline
 - source_bridge

ee/spec/models/merge_request/blocking_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequest do
+  let(:block) { create(:merge_request_block) }
+
+  let(:blocking_mr) { block.blocking_merge_request }
+  let(:blocked_mr) { block.blocked_merge_request }
+
+  describe 'associations' do
+    it { expect(blocking_mr.blocks_as_blocker).to contain_exactly(block) }
+    it { expect(blocking_mr.blocks_as_blockee).to be_empty }
+
+    it { expect(blocked_mr.blocks_as_blocker).to be_empty }
+    it { expect(blocked_mr.blocks_as_blockee).to contain_exactly(block) }
+
+    it { expect(blocking_mr.blocking_merge_requests).to be_empty }
+    it { expect(blocking_mr.blocked_merge_requests).to contain_exactly(blocked_mr) }
+
+    it { expect(blocked_mr.blocking_merge_requests).to contain_exactly(blocking_mr) }
+    it { expect(blocked_mr.blocked_merge_requests).to be_empty }
+  end
+
+  describe '#mergeable? (blocking MRs)' do
+    it 'checks MergeRequest#merge_blocked_by_other_mrs?' do
+      expect(blocked_mr).to receive(:merge_blocked_by_other_mrs?) { true }
+
+      expect(blocked_mr.mergeable?).to be(false)
+    end
+  end
+
+  describe '#merge_blocked_by_other_mrs?' do
+    context 'licensed' do
+      before do
+        stub_licensed_features(blocking_merge_requests: true)
+      end
+
+      it 'is false for the blocking MR' do
+        expect(blocking_mr.merge_blocked_by_other_mrs?).to be(false)
+      end
+
+      it 'is true for the blocked MR when the blocking MR is open' do
+        expect(blocked_mr.merge_blocked_by_other_mrs?).to be(true)
+      end
+
+      it 'is true for the blocked MR when the blocking MR is closed' do
+        blocking_mr.close!
+
+        expect(blocked_mr.merge_blocked_by_other_mrs?).to be(true)
+      end
+
+      it 'is false for the blocked MR when the blocking MR is merged' do
+        blocking_mr.state = 'merged'
+        blocking_mr.save!(validate: false)
+
+        expect(blocked_mr.merge_blocked_by_other_mrs?).to be(false)
+      end
+    end
+
+    context 'unlicensed' do
+      before do
+        stub_licensed_features(blocking_merge_requests: false)
+      end
+
+      it 'is false for the blocked MR' do
+        expect(blocked_mr.merge_blocked_by_other_mrs?).to be(false)
+      end
+    end
+  end
+end

ee/spec/models/merge_request_block_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequestBlock do
+  describe 'associations' do
+    it { is_expected.to belong_to(:blocking_merge_request).class_name('MergeRequest') }
+    it { is_expected.to belong_to(:blocked_merge_request).class_name('MergeRequest') }
+  end
+
+  describe 'validations' do
+    subject(:block) { create(:merge_request_block) }
+
+    it { is_expected.to validate_presence_of(:blocking_merge_request) }
+    it { is_expected.to validate_presence_of(:blocked_merge_request) }
+
+    it 'forbids the blocking MR from being the blocked MR' do
+      block.blocking_merge_request = block.blocked_merge_request
+
+      expect(block).not_to be_valid
+    end
+
+    it 'forbids duplicate blocks' do
+      new_block = described_class.new(block.attributes)
+
+      expect(new_block).not_to be_valid
+    end
+
+    it 'forbids blocking MR from becoming blocked' do
+      new_block = build(:merge_request_block, blocked_merge_request: block.blocking_merge_request)
+
+      expect(new_block).not_to be_valid
+    end
+
+    it 'forbids blocked MR from becoming a blocker' do
+      new_block = build(:merge_request_block, blocking_merge_request: block.blocked_merge_request)
+
+      expect(new_block).not_to be_valid
+    end
+  end
+end

ee/spec/models/merge_request_spec.rb

 require 'spec_helper'
 
+# Store feature-specific specs in `ee/spec/models/merge_request instead of
+# making this file longer.
+#
+# For instance, `ee/spec/models/merge_request/blocking_spec.rb` tests the
+# "blocking MRs" feature.
 describe MergeRequest do
   using RSpec::Parameterized::TableSyntax
   include ReactiveCachingHelpers

locale/gitlab.pot

@@ -8689,6 +8689,9 @@ msgstr ""
 msgid "Other information"
 msgstr ""
 
+msgid "Other merge requests block this MR"
+msgstr ""
+
 msgid "Outbound requests"
 msgstr ""
 
@@ -12550,6 +12553,9 @@ msgstr ""
 msgid "This application will be able to:"
 msgstr ""
 
+msgid "This block is self-referential"
+msgstr ""
+
 msgid "This board's scope is reduced"
 msgstr ""
 
@@ -14524,9 +14530,15 @@ msgstr ""
 msgid "cannot be enabled unless all domains have TLS certificates"
 msgstr ""
 
+msgid "cannot block others"
+msgstr ""
+
 msgid "cannot include leading slash or directory traversal."
 msgstr ""
 
+msgid "cannot itself be blocked"
+msgstr ""
+
 msgid "ciReport|%{linkStartTag}Learn more about Container Scanning %{linkEndTag}"
 msgstr ""