From ba5d71fbf0cc9427aaa4c559dd44d052d7d734a0 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Fri, 12 Apr 2019 17:08:04 +0100 Subject: [PATCH] Initial support for merge request blocks This commit adds a database migration and ActiveRecord models to represent the idea of a merge request that blocks another merge request from being blocked. There is no way to add a block through the UI yet, but you can create them in the Rails console and observe that a merge request becomes unmergeable as a result. --- ...20190412155659_add_merge_request_blocks.rb | 25 +++++++ db/schema.rb | 11 +++ ee/app/models/ee/merge_request.rb | 25 +++++++ ee/app/models/license.rb | 1 + ee/app/models/merge_request_block.rb | 27 +++++++ .../ee/merge_requests/merge_base_service.rb | 7 ++ .../unreleased/9688-mr-merge-order.yml | 5 ++ ee/spec/factories/merge_request_blocks.rb | 8 +++ .../lib/gitlab/import_export/all_models.yml | 4 ++ ee/spec/models/merge_request/blocking_spec.rb | 71 +++++++++++++++++++ ee/spec/models/merge_request_block_spec.rb | 41 +++++++++++ ee/spec/models/merge_request_spec.rb | 5 ++ locale/gitlab.pot | 12 ++++ 13 files changed, 242 insertions(+) create mode 100644 db/migrate/20190412155659_add_merge_request_blocks.rb create mode 100644 ee/app/models/merge_request_block.rb create mode 100644 ee/changelogs/unreleased/9688-mr-merge-order.yml create mode 100644 ee/spec/factories/merge_request_blocks.rb create mode 100644 ee/spec/models/merge_request/blocking_spec.rb create mode 100644 ee/spec/models/merge_request_block_spec.rb 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 00000000000000..9e7f370d1cfe22 --- /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 4fbed50e75b4d5..e576ebf9ec3c32 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 d953f735bb163f..fb26e85e2634c6 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 e2ca473493923b..64095cf5301ca5 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 00000000000000..8996d8a6d1b7fc --- /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 e7f0c914c85242..529ba3582ec7a4 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 00000000000000..28f490f9fbc7c7 --- /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 00000000000000..73102b48c73264 --- /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 997a31a008c04e..8c73e25ca833d0 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 00000000000000..ce9432d9e2e385 --- /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 00000000000000..aa767b4e3e0efe --- /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 f0977bcc80e1b3..e10febcd416681 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 866831c8cc455e..80cd04451b9dd5 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 "" -- GitLab