diff --git a/db/migrate/20190412155659_add_merge_request_blocks.rb b/db/migrate/20190412155659_add_merge_request_blocks.rb new file mode 100644 index 0000000000000000000000000000000000000000..9e7f370d1cfe2204446f7278b3fbdad93e99a039 --- /dev/null +++ b/db/migrate/20190412155659_add_merge_request_blocks.rb @@ -0,0 +1,25 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 4fbed50e75b4d500dfe873166101bb17763a97c5..e576ebf9ec3c328ad2f00ca86da8630486253017 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1812,6 +1812,15 @@ 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" @@ -3633,6 +3642,8 @@ 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 diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index d953f735bb163f5cdb66574f0dab4f468c8a7146..fb26e85e2634c62b4bacf854aa07c9f37b2e1262 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -23,6 +23,18 @@ module MergeRequest 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 MergeRequest 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 diff --git a/ee/app/models/license.rb b/ee/app/models/license.rb index e2ca473493923b1895c6da6ada40ca04d5de99cf..64095cf5301ca589fcfc15c7dac86464eed8ff56 100644 --- a/ee/app/models/license.rb +++ b/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 diff --git a/ee/app/models/merge_request_block.rb b/ee/app/models/merge_request_block.rb new file mode 100644 index 0000000000000000000000000000000000000000..8996d8a6d1b7fcf39860f9d703b382a002072748 --- /dev/null +++ b/ee/app/models/merge_request_block.rb @@ -0,0 +1,27 @@ +# 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 diff --git a/ee/app/services/ee/merge_requests/merge_base_service.rb b/ee/app/services/ee/merge_requests/merge_base_service.rb index e7f0c914c85242dff0d1fb36f11eecf2c97c8657..529ba3582ec7a479b821a4453f8068558cfacfd9 100644 --- a/ee/app/services/ee/merge_requests/merge_base_service.rb +++ b/ee/app/services/ee/merge_requests/merge_base_service.rb @@ -8,6 +8,7 @@ module MergeBaseService override :error_check! def error_check! check_size_limit + check_blocking_mrs end override :hooks_validation_pass? @@ -54,6 +55,12 @@ def check_size_limit 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 diff --git a/ee/changelogs/unreleased/9688-mr-merge-order.yml b/ee/changelogs/unreleased/9688-mr-merge-order.yml new file mode 100644 index 0000000000000000000000000000000000000000..28f490f9fbc7c74c301f4f581a4efcbf90708b75 --- /dev/null +++ b/ee/changelogs/unreleased/9688-mr-merge-order.yml @@ -0,0 +1,5 @@ +--- +title: Allow merge requests to block other MRs from being merged +merge_request: 11600 +author: +type: added diff --git a/ee/spec/factories/merge_request_blocks.rb b/ee/spec/factories/merge_request_blocks.rb new file mode 100644 index 0000000000000000000000000000000000000000..73102b48c7326471fa5118693310a55dbc8f1a7a --- /dev/null +++ b/ee/spec/factories/merge_request_blocks.rb @@ -0,0 +1,8 @@ +# 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 diff --git a/ee/spec/lib/gitlab/import_export/all_models.yml b/ee/spec/lib/gitlab/import_export/all_models.yml index 997a31a008c04e65d3da2684eaa76ecfd7e1f47e..8c73e25ca833d00e883d9fc343c230738a2ac781 100644 --- a/ee/spec/lib/gitlab/import_export/all_models.yml +++ b/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 diff --git a/ee/spec/models/merge_request/blocking_spec.rb b/ee/spec/models/merge_request/blocking_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..ce9432d9e2e385df0d1cefe7b146c7a68c0c8c97 --- /dev/null +++ b/ee/spec/models/merge_request/blocking_spec.rb @@ -0,0 +1,71 @@ +# 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 diff --git a/ee/spec/models/merge_request_block_spec.rb b/ee/spec/models/merge_request_block_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..aa767b4e3e0efe13be52c3611de5e25808aef09a --- /dev/null +++ b/ee/spec/models/merge_request_block_spec.rb @@ -0,0 +1,41 @@ +# 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 diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index f0977bcc80e1b37f0ad0312da894811f941907b5..e10febcd4166817d3a936225856cf225b82177ee 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -1,5 +1,10 @@ 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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 866831c8cc455e91b20ca06e92cc21454e84588f..80cd04451b9dd5a752a4f1163f4d92292b452de8 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -8563,6 +8563,9 @@ msgstr "" msgid "Other information" msgstr "" +msgid "Other merge requests block this MR" +msgstr "" + msgid "Outbound requests" msgstr "" @@ -12361,6 +12364,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 "" @@ -14311,9 +14317,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 ""