Commit 46c6231b authored by Sean McGivern's avatar Sean McGivern

Merge branch '22448-fix-500-nonexisting-branch' into 'master'

Add validation errors to Merge Request form

Closes #22448

See merge request !7163
parents 16d98f42 3c2f40cd
Pipeline #4830211 passed with stages
in 62 minutes and 16 seconds
......@@ -9,6 +9,7 @@ Please view this file on the master branch, on stable branches it's out of date.
- Adds an optional path parameter to the Commits API to filter commits by path (Luis HGO)
- Fix Markdown styling inside reference links (Jan Zdráhal)
- Fix extra space on Build sidebar on Firefox !7060
- Fail gracefully when creating merge request with non-existing branch (alexsanford)
- Fix mobile layout issues in admin user overview page !7087
- Fix HipChat notifications rendering (airatshigapov, eisnerd)
- Refactor Jira service to use jira-ruby gem
......
......@@ -13,20 +13,8 @@ module MergeRequests
merge_request.target_project ||= (project.forked_from_project || project)
merge_request.target_branch ||= merge_request.target_project.default_branch
if merge_request.target_branch.blank? || merge_request.source_branch.blank?
message =
if params[:source_branch] || params[:target_branch]
"You must select source and target branch"
end
return build_failed(merge_request, message)
end
if merge_request.source_project == merge_request.target_project &&
merge_request.target_branch == merge_request.source_branch
return build_failed(merge_request, 'You must select different branches')
end
messages = validate_branches(merge_request)
return build_failed(merge_request, messages) unless messages.empty?
compare = CompareService.new.execute(
merge_request.source_project,
......@@ -43,6 +31,34 @@ module MergeRequests
private
def validate_branches(merge_request)
messages = []
if merge_request.target_branch.blank? || merge_request.source_branch.blank?
messages <<
if params[:source_branch] || params[:target_branch]
"You must select source and target branch"
end
end
if merge_request.source_project == merge_request.target_project &&
merge_request.target_branch == merge_request.source_branch
messages << 'You must select different branches'
end
# See if source and target branches exist
unless merge_request.source_project.commit(merge_request.source_branch)
messages << "Source branch \"#{merge_request.source_branch}\" does not exist"
end
unless merge_request.target_project.commit(merge_request.target_branch)
messages << "Target branch \"#{merge_request.target_branch}\" does not exist"
end
messages
end
# When your branch name starts with an iid followed by a dash this pattern will be
# interpreted as the user wants to close that issue on this project.
#
......@@ -91,8 +107,10 @@ module MergeRequests
merge_request
end
def build_failed(merge_request, message)
merge_request.errors.add(:base, message) unless message.nil?
def build_failed(merge_request, messages)
messages.compact.each do |message|
merge_request.errors.add(:base, message)
end
merge_request.compare_commits = []
merge_request.can_be_created = false
merge_request
......
......@@ -59,4 +59,12 @@ feature 'Create New Merge Request', feature: true, js: true do
expect(page).to have_css('a.btn.active', text: 'Side-by-side')
end
end
it 'does not allow non-existing branches' do
visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { target_branch: 'non-exist-target', source_branch: 'non-exist-source' })
expect(page).to have_content('The form contains the following errors')
expect(page).to have_content('Source branch "non-exist-source" does not exist')
expect(page).to have_content('Target branch "non-exist-target" does not exist')
end
end
......@@ -25,6 +25,8 @@ describe MergeRequests::BuildService, services: true do
before do
allow(CompareService).to receive_message_chain(:new, :execute).and_return(compare)
allow(project).to receive(:commit).and_return(commit_1)
allow(project).to receive(:commit).and_return(commit_2)
end
describe 'execute' do
......@@ -193,5 +195,52 @@ describe MergeRequests::BuildService, services: true do
end
end
end
context 'source branch does not exist' do
before do
allow(project).to receive(:commit).with(source_branch).and_return(nil)
allow(project).to receive(:commit).with(target_branch).and_return(commit_1)
end
it 'forbids the merge request from being created' do
expect(merge_request.can_be_created).to eq(false)
end
it 'adds an error message to the merge request' do
expect(merge_request.errors).to contain_exactly('Source branch "feature-branch" does not exist')
end
end
context 'target branch does not exist' do
before do
allow(project).to receive(:commit).with(source_branch).and_return(commit_1)
allow(project).to receive(:commit).with(target_branch).and_return(nil)
end
it 'forbids the merge request from being created' do
expect(merge_request.can_be_created).to eq(false)
end
it 'adds an error message to the merge request' do
expect(merge_request.errors).to contain_exactly('Target branch "master" does not exist')
end
end
context 'both source and target branches do not exist' do
before do
allow(project).to receive(:commit).and_return(nil)
end
it 'forbids the merge request from being created' do
expect(merge_request.can_be_created).to eq(false)
end
it 'adds both error messages to the merge request' do
expect(merge_request.errors).to contain_exactly(
'Source branch "feature-branch" does not exist',
'Target branch "master" does not exist'
)
end
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