Commit 610c8e42 authored by Marc Shaw's avatar Marc Shaw

Show an error when failling to save a merge request dependency

Merge Request: gitlab.com/gitlab-org/gitlab/-/merge_requests/48237
Issue: gitlab.com/gitlab-org/gitlab/-/issues/207395
parent ca2f256e
......@@ -33,17 +33,24 @@ module MergeRequests
return unless update?
return unless merge_request.target_project.feature_available?(:blocking_merge_requests)
merge_request
.blocks_as_blockee
.with_blocking_mr_ids(ids_to_del)
.delete_all
valid_references, invalid_references = extract_references
# If the block is invalid, silently fail to add it
ids_to_add.each do |blocking_id|
::MergeRequestBlock.create(
blocking_merge_request_id: blocking_id,
blocked_merge_request_id: merge_request.id
)
delete_old_blockers!(valid_references)
errors = create_blockers(valid_references)
if invalid_references.present?
merge_request.errors.add(:dependencies, 'failed to save: ' + invalid_references.join(', '))
end
if errors.present?
merge_request.errors.add(:dependencies, 'failed to save: ' + errors.join(', '))
end
if invalid_references.present? || errors.present?
# When there are invalid references, we need to reset the associations
# so that the latest blocking merge requests are shown in the UI
merge_request.blocking_merge_requests.reset
end
true
......@@ -65,36 +72,68 @@ module MergeRequests
params.fetch(:references, [])
end
def requested_ids
strong_memoize(:requested_ids) do
next [] unless references.present?
# Returns two lists of references separating valid from invalid ones
#
# @return [Array<Array>] an array of valid and an array of invalid references
def extract_references
invalid_references = []
valid_references = []
return [], [] unless references.present?
# The analyzer will only return references the current user can see
references.each do |reference|
analyzer = ::Gitlab::ReferenceExtractor.new(merge_request.target_project, current_user)
analyzer.analyze(references.join(" "))
analyzer.analyze(reference)
analyzer.merge_requests.map(&:id)
if analyzer.merge_requests.any?
valid_references << analyzer.merge_requests
else
invalid_references << reference
end
end
def visible_ids
strong_memoize(:visible_ids) { visible_blocks.map(&:blocking_merge_request_id) }
[valid_references.flatten.map(&:id), invalid_references]
end
def hidden_ids
strong_memoize(:hidden_ids) { hidden_blocks.map(&:blocking_merge_request_id) }
def delete_old_blockers!(valid_references)
merge_request
.blocks_as_blockee
.with_blocking_mr_ids(ids_to_delete(valid_references))
.delete_all
end
def ids_to_add
strong_memoize(:ids_to_add) { requested_ids - visible_ids }
def create_blockers(valid_references)
new_ids = ids_to_add(valid_references)
new_ids.each_with_object([]) do |blocking_id, errors|
blocked = ::MergeRequestBlock.create(
blocking_merge_request_id: blocking_id,
blocked_merge_request_id: merge_request.id
)
unless blocked.persisted?
errors << blocked.errors.full_messages
end
end
end
def ids_to_del
strong_memoize(:ids_to_del) do
(visible_ids - requested_ids).tap do |ary|
def ids_to_add(valid_references)
valid_references - visible_ids
end
def ids_to_delete(valid_references)
(visible_ids - valid_references).tap do |ary|
ary.push(*hidden_ids) if remove_hidden?
end
end
def visible_ids
strong_memoize(:visible_ids) { visible_blocks.map(&:blocking_merge_request_id) }
end
def hidden_ids
hidden_blocks.map(&:blocking_merge_request_id)
end
end
end
---
title: Show an error when failing to save a merge request dependency
merge_request: 48237
author:
type: changed
......@@ -88,10 +88,20 @@ RSpec.describe MergeRequests::UpdateBlocksService do
context 'with a self-referential block' do
let(:mr_to_add) { merge_request }
it 'ignores the addition' do
it 'has an error on the merge request' do
service.execute
expect(merge_request.blocking_merge_requests).not_to include(mr_to_add)
expect(merge_request.reload.blocking_merge_requests).not_to include(mr_to_add)
expect(merge_request.errors[:dependencies]).to include(/cannot itself be blocked/)
end
end
context 'when an invalid reference' do
it 'has an error on the merge request' do
refs << 'notavalid'
service.execute
expect(merge_request.errors[:dependencies]).to include(/notavalid/)
end
end
end
......
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