Commit 3efe5342 authored by Sean McGivern's avatar Sean McGivern

Merge branch '26488-target-disabled-mr' into 'master'

Fix 404 when upstream disabled merge requests

Closes #26488

See merge request !10427
parents 59175a57 ccac05dd
module MergeRequestsHelper module MergeRequestsHelper
def new_mr_path_from_push_event(event) def new_mr_path_from_push_event(event)
target_project = event.project.forked_from_project || event.project target_project = event.project.default_merge_request_target
new_namespace_project_merge_request_path( new_namespace_project_merge_request_path(
event.project.namespace, event.project.namespace,
event.project, event.project,
...@@ -127,6 +127,10 @@ module MergeRequestsHelper ...@@ -127,6 +127,10 @@ module MergeRequestsHelper
end end
end end
def target_projects(project)
[project, project.default_merge_request_target].uniq
end
def merge_request_button_visibility(merge_request, closed) def merge_request_button_visibility(merge_request, closed)
return 'hidden' if merge_request.closed? == closed || (merge_request.merged? == closed && !merge_request.closed?) || merge_request.closed_without_fork? return 'hidden' if merge_request.closed? == closed || (merge_request.merged? == closed && !merge_request.closed?) || merge_request.closed_without_fork?
end end
......
...@@ -100,6 +100,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -100,6 +100,7 @@ class MergeRequest < ActiveRecord::Base
validates :merge_user, presence: true, if: :merge_when_pipeline_succeeds?, unless: :importing? validates :merge_user, presence: true, if: :merge_when_pipeline_succeeds?, unless: :importing?
validate :validate_branches, unless: [:allow_broken, :importing?, :closed_without_fork?] validate :validate_branches, unless: [:allow_broken, :importing?, :closed_without_fork?]
validate :validate_fork, unless: :closed_without_fork? validate :validate_fork, unless: :closed_without_fork?
validate :validate_target_project, on: :create
scope :by_source_or_target_branch, ->(branch_name) do scope :by_source_or_target_branch, ->(branch_name) do
where("source_branch = :branch OR target_branch = :branch", branch: branch_name) where("source_branch = :branch OR target_branch = :branch", branch: branch_name)
...@@ -330,6 +331,12 @@ class MergeRequest < ActiveRecord::Base ...@@ -330,6 +331,12 @@ class MergeRequest < ActiveRecord::Base
end end
end end
def validate_target_project
return true if target_project.merge_requests_enabled?
errors.add :base, 'Target project has disabled merge requests'
end
def validate_fork def validate_fork
return true unless target_project && source_project return true unless target_project && source_project
return true if target_project == source_project return true if target_project == source_project
......
...@@ -1314,6 +1314,14 @@ class Project < ActiveRecord::Base ...@@ -1314,6 +1314,14 @@ class Project < ActiveRecord::Base
namespace_id_changed? namespace_id_changed?
end end
def default_merge_request_target
if forked_from_project&.merge_requests_enabled?
forked_from_project
else
self
end
end
alias_method :name_with_namespace, :full_name alias_method :name_with_namespace, :full_name
alias_method :human_name, :full_name alias_method :human_name, :full_name
alias_method :path_with_namespace, :full_path alias_method :path_with_namespace, :full_path
......
...@@ -28,7 +28,7 @@ module MergeRequests ...@@ -28,7 +28,7 @@ module MergeRequests
def find_target_project def find_target_project
return target_project if target_project.present? && can?(current_user, :read_project, target_project) return target_project if target_project.present? && can?(current_user, :read_project, target_project)
project.forked_from_project || project project.default_merge_request_target
end end
def find_target_branch def find_target_branch
......
...@@ -38,7 +38,7 @@ ...@@ -38,7 +38,7 @@
.panel-heading .panel-heading
Target branch Target branch
.panel-body.clearfix .panel-body.clearfix
- projects = @project.forked_from_project.nil? ? [@project] : [@project, @project.forked_from_project] - projects = target_projects(@project)
.merge-request-select.dropdown .merge-request-select.dropdown
= f.hidden_field :target_project_id = f.hidden_field :target_project_id
= dropdown_toggle f.object.target_project.path_with_namespace, { toggle: "dropdown", field_name: "#{f.object_name}[target_project_id]", disabled: @merge_request.persisted? }, { toggle_class: "js-compare-dropdown js-target-project" } = dropdown_toggle f.object.target_project.path_with_namespace, { toggle: "dropdown", field_name: "#{f.object_name}[target_project_id]", disabled: @merge_request.persisted? }, { toggle_class: "js-compare-dropdown js-target-project" }
......
---
title: Disallow merge requests from fork when source project have disabled merge requests
merge_request:
author: mhasbini
...@@ -20,6 +20,8 @@ module API ...@@ -20,6 +20,8 @@ module API
error!(errors[:validate_fork], 422) error!(errors[:validate_fork], 422)
elsif errors[:validate_branches].any? elsif errors[:validate_branches].any?
conflict!(errors[:validate_branches]) conflict!(errors[:validate_branches])
elsif errors[:base].any?
error!(errors[:base], 422)
end end
render_api_error!(errors, 400) render_api_error!(errors, 400)
......
...@@ -23,6 +23,8 @@ module API ...@@ -23,6 +23,8 @@ module API
error!(errors[:validate_fork], 422) error!(errors[:validate_fork], 422)
elsif errors[:validate_branches].any? elsif errors[:validate_branches].any?
conflict!(errors[:validate_branches]) conflict!(errors[:validate_branches])
elsif errors[:base].any?
error!(errors[:base], 422)
end end
render_api_error!(errors, 400) render_api_error!(errors, 400)
......
...@@ -64,7 +64,7 @@ describe MergeRequestsHelper do ...@@ -64,7 +64,7 @@ describe MergeRequestsHelper do
it do it do
@project = project @project = project
is_expected.to eq("#1, #2, and #{other_project.namespace.path}/#{other_project.path}#3") is_expected.to eq("#1, #2, and #{other_project.namespace.path}/#{other_project.path}#3")
end end
end end
...@@ -149,6 +149,50 @@ describe MergeRequestsHelper do ...@@ -149,6 +149,50 @@ describe MergeRequestsHelper do
end end
end end
describe '#target_projects' do
let(:project) { create(:empty_project) }
let(:fork_project) { create(:empty_project, forked_from_project: project) }
context 'when target project has enabled merge requests' do
it 'returns the forked_from project' do
expect(target_projects(fork_project)).to contain_exactly(project, fork_project)
end
end
context 'when target project has disabled merge requests' do
it 'returns the forked project' do
project.project_feature.update(merge_requests_access_level: 0)
expect(target_projects(fork_project)).to contain_exactly(fork_project)
end
end
end
describe '#new_mr_path_from_push_event' do
subject(:url_params) { URI.decode_www_form(new_mr_path_from_push_event(event)).to_h }
let(:user) { create(:user) }
let(:project) { create(:empty_project, creator: user) }
let(:fork_project) { create(:project, forked_from_project: project, creator: user) }
let(:event) do
push_data = Gitlab::DataBuilder::Push.build_sample(fork_project, user)
create(:event, :pushed, project: fork_project, target: fork_project, author: user, data: push_data)
end
context 'when target project has enabled merge requests' do
it 'returns link to create merge request on source project' do
expect(url_params['merge_request[target_project_id]'].to_i).to eq(project.id)
end
end
context 'when target project has disabled merge requests' do
it 'returns link to create merge request on forked project' do
project.project_feature.update(merge_requests_access_level: 0)
expect(url_params['merge_request[target_project_id]'].to_i).to eq(fork_project.id)
end
end
end
describe '#mr_issues_mentioned_but_not_closing' do describe '#mr_issues_mentioned_but_not_closing' do
let(:user_1) { create(:user) } let(:user_1) { create(:user) }
let(:user_2) { create(:user) } let(:user_2) { create(:user) }
......
...@@ -434,6 +434,19 @@ describe API::MergeRequests do ...@@ -434,6 +434,19 @@ describe API::MergeRequests do
expect(json_response['title']).to eq('Test merge_request') expect(json_response['title']).to eq('Test merge_request')
end end
it 'returns 422 when target project has disabled merge requests' do
project.project_feature.update(merge_requests_access_level: 0)
post api("/projects/#{fork_project.id}/merge_requests", user2),
title: 'Test',
target_branch: 'master',
source_branch: 'markdown',
author: user2,
target_project_id: project.id
expect(response).to have_http_status(422)
end
it "returns 400 when source_branch is missing" do it "returns 400 when source_branch is missing" do
post api("/projects/#{fork_project.id}/merge_requests", user2), post api("/projects/#{fork_project.id}/merge_requests", user2),
title: 'Test merge_request', target_branch: "master", author: user2, target_project_id: project.id title: 'Test merge_request', target_branch: "master", author: user2, target_project_id: project.id
......
...@@ -338,6 +338,19 @@ describe API::MergeRequests do ...@@ -338,6 +338,19 @@ describe API::MergeRequests do
expect(json_response['title']).to eq('Test merge_request') expect(json_response['title']).to eq('Test merge_request')
end end
it "returns 422 when target project has disabled merge requests" do
project.project_feature.update(merge_requests_access_level: 0)
post v3_api("/projects/#{fork_project.id}/merge_requests", user2),
title: 'Test',
target_branch: "master",
source_branch: 'markdown',
author: user2,
target_project_id: project.id
expect(response).to have_http_status(422)
end
it "returns 400 when source_branch is missing" do it "returns 400 when source_branch is missing" do
post v3_api("/projects/#{fork_project.id}/merge_requests", user2), post v3_api("/projects/#{fork_project.id}/merge_requests", user2),
title: 'Test merge_request', target_branch: "master", author: user2, target_project_id: project.id title: 'Test merge_request', target_branch: "master", author: user2, target_project_id: project.id
......
...@@ -261,6 +261,16 @@ describe MergeRequests::BuildService, services: true do ...@@ -261,6 +261,16 @@ describe MergeRequests::BuildService, services: true do
end end
end end
context 'upstream project has disabled merge requests' do
let(:upstream_project) { create(:empty_project, :merge_requests_disabled) }
let(:project) { create(:empty_project, forked_from_project: upstream_project) }
let(:commits) { Commit.decorate([commit_1], project) }
it 'sets target project correctly' do
expect(merge_request.target_project).to eq(project)
end
end
context 'target_project is set and accessible by current_user' do context 'target_project is set and accessible by current_user' do
let(:target_project) { create(:project, :public, :repository)} let(:target_project) { create(:project, :public, :repository)}
let(:commits) { Commit.decorate([commit_1], project) } let(:commits) { Commit.decorate([commit_1], project) }
......
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