Commit ba5d71fb authored by Nick Thomas's avatar Nick Thomas

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.
parent 3119656a
# 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
...@@ -1812,6 +1812,15 @@ ActiveRecord::Schema.define(version: 20190426180107) do ...@@ -1812,6 +1812,15 @@ ActiveRecord::Schema.define(version: 20190426180107) do
t.index ["user_id"], name: "index_merge_request_assignees_on_user_id", using: :btree t.index ["user_id"], name: "index_merge_request_assignees_on_user_id", using: :btree
end 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| create_table "merge_request_diff_commits", id: false, force: :cascade do |t|
t.datetime "authored_date" t.datetime "authored_date"
t.datetime "committed_date" t.datetime "committed_date"
...@@ -3633,6 +3642,8 @@ ActiveRecord::Schema.define(version: 20190426180107) do ...@@ -3633,6 +3642,8 @@ ActiveRecord::Schema.define(version: 20190426180107) do
add_foreign_key "members", "users", name: "fk_2e88fb7ce9", on_delete: :cascade 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", "merge_requests", on_delete: :cascade
add_foreign_key "merge_request_assignees", "users", 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_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_diff_files", "merge_request_diffs", on_delete: :cascade
add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade
......
...@@ -23,6 +23,18 @@ module EE ...@@ -23,6 +23,18 @@ module EE
has_many :draft_notes has_many :draft_notes
has_one :merge_train 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 validate :validate_approval_rule_source
delegate :sha, to: :head_pipeline, prefix: :head_pipeline, allow_nil: true delegate :sha, to: :head_pipeline, prefix: :head_pipeline, allow_nil: true
...@@ -38,15 +50,28 @@ module EE ...@@ -38,15 +50,28 @@ module EE
def select_from_union(relations) def select_from_union(relations)
where(id: from_union(relations)) where(id: from_union(relations))
end end
# This is an ActiveRecord scope in CE
def with_api_entity_associations
super.preload(:blocking_merge_requests)
end
end end
override :mergeable? override :mergeable?
def mergeable?(skip_ci_check: false) def mergeable?(skip_ci_check: false)
return false unless approved? return false unless approved?
return false if merge_blocked_by_other_mrs?
super super
end 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? override :mergeable_ci_state?
def mergeable_ci_state? def mergeable_ci_state?
return false unless validate_merge_request_pipelines return false unless validate_merge_request_pipelines
......
...@@ -39,6 +39,7 @@ class License < ApplicationRecord ...@@ -39,6 +39,7 @@ class License < ApplicationRecord
EEP_FEATURES = EES_FEATURES + %i[ EEP_FEATURES = EES_FEATURES + %i[
admin_audit_log admin_audit_log
auditor_user auditor_user
blocking_merge_requests
board_assignee_lists board_assignee_lists
board_milestone_lists board_milestone_lists
cross_project_pipelines cross_project_pipelines
......
# 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
...@@ -8,6 +8,7 @@ module EE ...@@ -8,6 +8,7 @@ module EE
override :error_check! override :error_check!
def error_check! def error_check!
check_size_limit check_size_limit
check_blocking_mrs
end end
override :hooks_validation_pass? override :hooks_validation_pass?
...@@ -54,6 +55,12 @@ module EE ...@@ -54,6 +55,12 @@ module EE
raise ::MergeRequests::MergeService::MergeError, message raise ::MergeRequests::MergeService::MergeError, message
end end
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 end
end end
---
title: Allow merge requests to block other MRs from being merged
merge_request: 11600
author:
type: added
# 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
...@@ -15,6 +15,10 @@ merge_requests: ...@@ -15,6 +15,10 @@ merge_requests:
- approved_by_users - approved_by_users
- draft_notes - draft_notes
- merge_train - merge_train
- blocks_as_blocker
- blocks_as_blockee
- blocking_merge_requests
- blocked_merge_requests
ci_pipelines: ci_pipelines:
- source_pipeline - source_pipeline
- source_bridge - source_bridge
......
# 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
# 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
require 'spec_helper' 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 describe MergeRequest do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
include ReactiveCachingHelpers include ReactiveCachingHelpers
......
...@@ -8563,6 +8563,9 @@ msgstr "" ...@@ -8563,6 +8563,9 @@ msgstr ""
msgid "Other information" msgid "Other information"
msgstr "" msgstr ""
msgid "Other merge requests block this MR"
msgstr ""
msgid "Outbound requests" msgid "Outbound requests"
msgstr "" msgstr ""
...@@ -12361,6 +12364,9 @@ msgstr "" ...@@ -12361,6 +12364,9 @@ msgstr ""
msgid "This application will be able to:" msgid "This application will be able to:"
msgstr "" msgstr ""
msgid "This block is self-referential"
msgstr ""
msgid "This board's scope is reduced" msgid "This board's scope is reduced"
msgstr "" msgstr ""
...@@ -14311,9 +14317,15 @@ msgstr "" ...@@ -14311,9 +14317,15 @@ msgstr ""
msgid "cannot be enabled unless all domains have TLS certificates" msgid "cannot be enabled unless all domains have TLS certificates"
msgstr "" msgstr ""
msgid "cannot block others"
msgstr ""
msgid "cannot include leading slash or directory traversal." msgid "cannot include leading slash or directory traversal."
msgstr "" msgstr ""
msgid "cannot itself be blocked"
msgstr ""
msgid "ciReport|%{linkStartTag}Learn more about Container Scanning %{linkEndTag}" msgid "ciReport|%{linkStartTag}Learn more about Container Scanning %{linkEndTag}"
msgstr "" msgstr ""
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment