From d17396b0e3ec617b2bae80fbfde70db586576f69 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Tue, 19 Jan 2021 15:04:51 +0000 Subject: [PATCH] Allow comparisons of branches across projects This is already permitted in the merge request view, for creating fork merge requests, so it should be permitted in the repository view too. --- .../projects/compare_controller.rb | 52 +++- .../merge_request_target_project_finder.rb | 11 +- app/helpers/compare_helper.rb | 21 +- app/views/events/event/_push.html.haml | 7 +- app/views/projects/branches/_branch.html.haml | 4 +- app/views/projects/commits/show.html.haml | 4 +- .../development/compare_repo_dropdown.yml | 8 + .../projects/compare_controller_spec.rb | 243 ++++++++++++------ .../projects/merge_request_button_spec.rb | 60 ++++- ...erge_request_target_project_finder_spec.rb | 11 +- 10 files changed, 297 insertions(+), 124 deletions(-) create mode 100644 config/feature_flags/development/compare_repo_dropdown.yml diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index 6be0b465402035..133055cc317257 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -6,6 +6,7 @@ class Projects::CompareController < Projects::ApplicationController include DiffForPath include DiffHelper include RendersCommits + include CompareHelper # Authorize before_action :require_non_empty_project @@ -37,16 +38,18 @@ def diff_for_path end def create - if params[:from].blank? || params[:to].blank? + from_to_vars = { + from: params[:from].presence, + to: params[:to].presence, + from_project_id: params[:from_project_id].presence + } + + if from_to_vars[:from].blank? || from_to_vars[:to].blank? flash[:alert] = "You must select a Source and a Target revision" - from_to_vars = { - from: params[:from].presence, - to: params[:to].presence - } - redirect_to project_compare_index_path(@project, from_to_vars) + + redirect_to project_compare_index_path(source_project, from_to_vars) else - redirect_to project_compare_path(@project, - params[:from], params[:to]) + redirect_to project_compare_path(source_project, from_to_vars) end end @@ -73,13 +76,34 @@ def validate_refs! return if valid.all? flash[:alert] = "Invalid branch name" - redirect_to project_compare_index_path(@project) + redirect_to project_compare_index_path(source_project) + end + + # target == start_ref == from + def target_project + strong_memoize(:target_project) do + next source_project unless params.key?(:from_project_id) + next source_project unless Feature.enabled?(:compare_repo_dropdown, source_project, default_enabled: :yaml) + next source_project if params[:from_project_id].to_i == source_project.id + + target_project = target_projects(source_project).find_by_id(params[:from_project_id]) + + # Just ignore the field if it points at a non-existent or hidden project + next source_project unless target_project && can?(current_user, :download_code, target_project) + + target_project + end + end + + # source == head_ref == to + def source_project + project end def compare return @compare if defined?(@compare) - @compare = CompareService.new(@project, head_ref).execute(@project, start_ref) + @compare = CompareService.new(source_project, head_ref).execute(target_project, start_ref) end def start_ref @@ -102,9 +126,9 @@ def define_diffs def define_environment if compare - environment_params = @repository.branch_exists?(head_ref) ? { ref: head_ref } : { commit: compare.commit } + environment_params = source_project.repository.branch_exists?(head_ref) ? { ref: head_ref } : { commit: compare.commit } environment_params[:find_latest] = true - @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last + @environment = EnvironmentsFinder.new(source_project, current_user, environment_params).execute.last end end @@ -114,8 +138,8 @@ def define_diff_notes_disabled # rubocop: disable CodeReuse/ActiveRecord def merge_request - @merge_request ||= MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened - .find_by(source_project: @project, source_branch: head_ref, target_branch: start_ref) + @merge_request ||= MergeRequestsFinder.new(current_user, project_id: target_project.id).execute.opened + .find_by(source_project: source_project, source_branch: head_ref, target_branch: start_ref) end # rubocop: enable CodeReuse/ActiveRecord end diff --git a/app/finders/merge_request_target_project_finder.rb b/app/finders/merge_request_target_project_finder.rb index 85a73e0c6ffc08..dc9b28ab0a04ca 100644 --- a/app/finders/merge_request_target_project_finder.rb +++ b/app/finders/merge_request_target_project_finder.rb @@ -5,29 +5,30 @@ class MergeRequestTargetProjectFinder attr_reader :current_user, :source_project - def initialize(current_user: nil, source_project:) + def initialize(current_user: nil, source_project:, project_feature: :merge_requests) @current_user = current_user @source_project = source_project + @project_feature = project_feature end - # rubocop: disable CodeReuse/ActiveRecord def execute(include_routes: false) if source_project.fork_network include_routes ? projects.inc_routes : projects else - Project.where(id: source_project) + Project.id_in(source_project.id) end end - # rubocop: enable CodeReuse/ActiveRecord private + attr_reader :project_feature + def projects source_project .fork_network .projects .public_or_visible_to_user(current_user) .non_archived - .with_feature_available_for_user(:merge_requests, current_user) + .with_feature_available_for_user(project_feature, current_user) end end diff --git a/app/helpers/compare_helper.rb b/app/helpers/compare_helper.rb index 9ece8b0bc5bb81..c14aa44ced166a 100644 --- a/app/helpers/compare_helper.rb +++ b/app/helpers/compare_helper.rb @@ -1,22 +1,31 @@ # frozen_string_literal: true module CompareHelper - def create_mr_button?(from = params[:from], to = params[:to], project = @project) + def create_mr_button?(from: params[:from], to: params[:to], source_project: @project, target_project: @target_project) from.present? && to.present? && from != to && - can?(current_user, :create_merge_request_from, project) && - project.repository.branch_exists?(from) && - project.repository.branch_exists?(to) + can?(current_user, :create_merge_request_from, source_project) && + can?(current_user, :create_merge_request_in, target_project) && + target_project.repository.branch_exists?(from) && + source_project.repository.branch_exists?(to) end - def create_mr_path(from = params[:from], to = params[:to], project = @project) + def create_mr_path(from: params[:from], to: params[:to], source_project: @project, target_project: @target_project) project_new_merge_request_path( - project, + target_project, merge_request: { + source_project_id: source_project.id, source_branch: to, + target_project_id: target_project.id, target_branch: from } ) end + + def target_projects(source_project) + MergeRequestTargetProjectFinder + .new(current_user: current_user, source_project: source_project, project_feature: :repository) + .execute(include_routes: true) + end end diff --git a/app/views/events/event/_push.html.haml b/app/views/events/event/_push.html.haml index d6662e1fc31813..97dd606855b446 100644 --- a/app/views/events/event/_push.html.haml +++ b/app/views/events/event/_push.html.haml @@ -21,7 +21,8 @@ %ul.content-list.event_commits = render "events/commit", project: project, event: event - - create_mr = event.new_ref? && create_mr_button?(project.default_branch, event.ref_name, project) && event.authored_by?(current_user) + - create_mr = event.new_ref? && create_mr_button?(from: project.default_branch, to: event.ref_name, source_project: project, target_project: project) && event.authored_by?(current_user) + - create_mr_path = create_mr_path(from: project.default_branch, to: event.ref_name, source_project: project, target_project: project) if create_mr - if event.commits_count > 1 %li.commits-stat %span ... and #{pluralize(event.commits_count - 1, 'more commit')}. @@ -40,9 +41,9 @@ - if create_mr %span or - = link_to create_mr_path(project.default_branch, event.ref_name, project) do + = link_to create_mr_path do create a merge request - elsif create_mr %li.commits-stat - = link_to create_mr_path(project.default_branch, event.ref_name, project) do + = link_to create_mr_path do Create Merge Request diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml index fcf073e1e094cb..6f86ccd7824f1f 100644 --- a/app/views/projects/branches/_branch.html.haml +++ b/app/views/projects/branches/_branch.html.haml @@ -35,8 +35,8 @@ .gl-display-inline-flex.gl-vertical-align-middle.gl-mr-5 %svg.s24 - - if merge_project && create_mr_button?(@repository.root_ref, branch.name) - = link_to create_mr_path(@repository.root_ref, branch.name), class: 'gl-button btn btn-default' do + - if merge_project && create_mr_button?(from: @repository.root_ref, to: branch.name, source_project: @project, target_project: @project) + = link_to create_mr_path(from: @repository.root_ref, to: branch.name, source_project: @project, target_project: @project), class: 'gl-button btn btn-default' do = _('Merge request') - if branch.name != @repository.root_ref diff --git a/app/views/projects/commits/show.html.haml b/app/views/projects/commits/show.html.haml index 802df664241532..463984a13a2a65 100644 --- a/app/views/projects/commits/show.html.haml +++ b/app/views/projects/commits/show.html.haml @@ -18,9 +18,9 @@ - if @merge_request.present? .control.d-none.d-md-block = link_to _("View open merge request"), project_merge_request_path(@project, @merge_request), class: 'btn gl-button' - - elsif create_mr_button?(@repository.root_ref, @ref) + - elsif create_mr_button?(from: @repository.root_ref, to: @ref, source_project: @project, target_project: @project) .control.d-none.d-md-block - = link_to _("Create merge request"), create_mr_path(@repository.root_ref, @ref), class: 'btn gl-button btn-success' + = link_to _("Create merge request"), create_mr_path(from: @repository.root_ref, to: @ref, source_project: @project, target_project: @project), class: 'btn gl-button btn-success' .control = form_tag(project_commits_path(@project, @id), method: :get, class: 'commits-search-form js-signature-container', data: { 'signatures-path' => namespace_project_signatures_path }) do diff --git a/config/feature_flags/development/compare_repo_dropdown.yml b/config/feature_flags/development/compare_repo_dropdown.yml new file mode 100644 index 00000000000000..b47bce8c521ea1 --- /dev/null +++ b/config/feature_flags/development/compare_repo_dropdown.yml @@ -0,0 +1,8 @@ +--- +name: compare_repo_dropdown +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/issues/14615 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/322141 +milestone: '13.9' +type: development +group: group::source code +default_enabled: false diff --git a/spec/controllers/projects/compare_controller_spec.rb b/spec/controllers/projects/compare_controller_spec.rb index 6aa4bfe235bd15..80a6d3960cd69e 100644 --- a/spec/controllers/projects/compare_controller_spec.rb +++ b/spec/controllers/projects/compare_controller_spec.rb @@ -3,8 +3,21 @@ require 'spec_helper' RSpec.describe Projects::CompareController do - let(:project) { create(:project, :repository) } - let(:user) { create(:user) } + include ProjectForksHelper + + using RSpec::Parameterized::TableSyntax + + let_it_be(:project) { create(:project, :repository, :public) } + let_it_be(:user) { create(:user) } + + let(:private_fork) { fork_project(project, nil, repository: true).tap { |fork| fork.update!(visibility: 'private') } } + let(:public_fork) do + fork_project(project, nil, repository: true).tap do |fork| + fork.update!(visibility: 'public') + # Create a reference that only exists in this project + fork.repository.create_ref('refs/heads/improve/awesome', 'refs/heads/improve/more-awesome') + end + end before do sign_in(user) @@ -32,18 +45,20 @@ { namespace_id: project.namespace, project_id: project, - from: source_ref, - to: target_ref, + from_project_id: from_project_id, + from: from_ref, + to: to_ref, w: whitespace } end let(:whitespace) { nil } - context 'when the refs exist' do + context 'when the refs exist in the same project' do context 'when we set the white space param' do - let(:source_ref) { "08f22f25" } - let(:target_ref) { "66eceea0" } + let(:from_project_id) { nil } + let(:from_ref) { '08f22f25' } + let(:to_ref) { '66eceea0' } let(:whitespace) { 1 } it 'shows some diffs with ignore whitespace change option' do @@ -60,8 +75,9 @@ end context 'when we do not set the white space param' do - let(:source_ref) { "improve%2Fawesome" } - let(:target_ref) { "feature" } + let(:from_project_id) { nil } + let(:from_ref) { 'improve%2Fawesome' } + let(:to_ref) { 'feature' } let(:whitespace) { nil } it 'sets the diffs and commits ivars' do @@ -74,9 +90,40 @@ end end + context 'when the refs exist in different projects that the user can see' do + let(:from_project_id) { public_fork.id } + let(:from_ref) { 'improve%2Fmore-awesome' } + let(:to_ref) { 'feature' } + let(:whitespace) { nil } + + it 'shows the diff' do + show_request + + expect(response).to be_successful + expect(assigns(:diffs).diff_files.first).not_to be_nil + expect(assigns(:commits).length).to be >= 1 + end + end + + context 'when the refs exist in different projects but the user cannot see' do + let(:from_project_id) { private_fork.id } + let(:from_ref) { 'improve%2Fmore-awesome' } + let(:to_ref) { 'feature' } + let(:whitespace) { nil } + + it 'does not show the diff' do + show_request + + expect(response).to be_successful + expect(assigns(:diffs)).to be_empty + expect(assigns(:commits)).to be_empty + end + end + context 'when the source ref does not exist' do - let(:source_ref) { 'non-existent-source-ref' } - let(:target_ref) { "feature" } + let(:from_project_id) { nil } + let(:from_ref) { 'non-existent-source-ref' } + let(:to_ref) { 'feature' } it 'sets empty diff and commit ivars' do show_request @@ -88,8 +135,9 @@ end context 'when the target ref does not exist' do - let(:target_ref) { 'non-existent-target-ref' } - let(:source_ref) { "improve%2Fawesome" } + let(:from_project_id) { nil } + let(:from_ref) { 'improve%2Fawesome' } + let(:to_ref) { 'non-existent-target-ref' } it 'sets empty diff and commit ivars' do show_request @@ -101,8 +149,9 @@ end context 'when the target ref is invalid' do - let(:target_ref) { "master%' AND 2554=4423 AND '%'='" } - let(:source_ref) { "improve%2Fawesome" } + let(:from_project_id) { nil } + let(:from_ref) { 'improve%2Fawesome' } + let(:to_ref) { "master%' AND 2554=4423 AND '%'='" } it 'shows a flash message and redirects' do show_request @@ -113,8 +162,9 @@ end context 'when the source ref is invalid' do - let(:source_ref) { "master%' AND 2554=4423 AND '%'='" } - let(:target_ref) { "improve%2Fawesome" } + let(:from_project_id) { nil } + let(:from_ref) { "master%' AND 2554=4423 AND '%'='" } + let(:to_ref) { 'improve%2Fawesome' } it 'shows a flash message and redirects' do show_request @@ -126,24 +176,33 @@ end describe 'GET diff_for_path' do - def diff_for_path(extra_params = {}) - params = { + subject(:diff_for_path_request) { get :diff_for_path, params: request_params } + + let(:request_params) do + { + from_project_id: from_project_id, + from: from_ref, + to: to_ref, namespace_id: project.namespace, - project_id: project + project_id: project, + old_path: old_path, + new_path: new_path } - - get :diff_for_path, params: params.merge(extra_params) end let(:existing_path) { 'files/ruby/feature.rb' } - let(:source_ref) { "improve%2Fawesome" } - let(:target_ref) { "feature" } - context 'when the source and target refs exist' do + let(:from_project_id) { nil } + let(:from_ref) { 'improve%2Fawesome' } + let(:to_ref) { 'feature' } + let(:old_path) { existing_path } + let(:new_path) { existing_path } + + context 'when the source and target refs exist in the same project' do context 'when the user has access target the project' do context 'when the path exists in the diff' do it 'disables diff notes' do - diff_for_path(from: source_ref, to: target_ref, old_path: existing_path, new_path: existing_path) + diff_for_path_request expect(assigns(:diff_notes_disabled)).to be_truthy end @@ -154,16 +213,17 @@ def diff_for_path(extra_params = {}) meth.call(diffs) end - diff_for_path(from: source_ref, to: target_ref, old_path: existing_path, new_path: existing_path) + diff_for_path_request end end context 'when the path does not exist in the diff' do - before do - diff_for_path(from: source_ref, to: target_ref, old_path: existing_path.succ, new_path: existing_path.succ) - end + let(:old_path) { existing_path.succ } + let(:new_path) { existing_path.succ } it 'returns a 404' do + diff_for_path_request + expect(response).to have_gitlab_http_status(:not_found) end end @@ -172,31 +232,56 @@ def diff_for_path(extra_params = {}) context 'when the user does not have access target the project' do before do project.team.truncate - diff_for_path(from: source_ref, to: target_ref, old_path: existing_path, new_path: existing_path) end it 'returns a 404' do + diff_for_path_request + expect(response).to have_gitlab_http_status(:not_found) end end end - context 'when the source ref does not exist' do - before do - diff_for_path(from: source_ref.succ, to: target_ref, old_path: existing_path, new_path: existing_path) + context 'when the source and target refs exist in different projects and the user can see' do + let(:from_project_id) { public_fork.id } + let(:from_ref) { 'improve%2Fmore-awesome' } + + it 'shows the diff for that path' do + expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs| + expect(diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path) + meth.call(diffs) + end + + diff_for_path_request + end + end + + context 'when the source and target refs exist in different projects and the user cannot see' do + let(:from_project_id) { private_fork.id } + + it 'does not show the diff for that path' do + diff_for_path_request + + expect(response).to have_gitlab_http_status(:not_found) end + end + + context 'when the source ref does not exist' do + let(:from_ref) { 'this-ref-does-not-exist' } it 'returns a 404' do + diff_for_path_request + expect(response).to have_gitlab_http_status(:not_found) end end context 'when the target ref does not exist' do - before do - diff_for_path(from: source_ref, to: target_ref.succ, old_path: existing_path, new_path: existing_path) - end + let(:to_ref) { 'this-ref-does-not-exist' } it 'returns a 404' do + diff_for_path_request + expect(response).to have_gitlab_http_status(:not_found) end end @@ -209,53 +294,54 @@ def diff_for_path(extra_params = {}) { namespace_id: project.namespace, project_id: project, - from: source_ref, - to: target_ref + from_project_id: from_project_id, + from: from_ref, + to: to_ref } end context 'when sending valid params' do - let(:source_ref) { "improve%2Fawesome" } - let(:target_ref) { "feature" } + let(:from_ref) { 'awesome%2Ffeature' } + let(:to_ref) { 'feature' } - it 'redirects back to show' do - create_request - - expect(response).to redirect_to(project_compare_path(project, to: target_ref, from: source_ref)) - end - end + context 'without a from_project_id' do + let(:from_project_id) { nil } - context 'when sending invalid params' do - context 'when the source ref is empty and target ref is set' do - let(:source_ref) { '' } - let(:target_ref) { 'master' } - - it 'redirects back to index and preserves the target ref' do + it 'redirects to the show page' do create_request - expect(response).to redirect_to(project_compare_index_path(project, to: target_ref)) + expect(response).to redirect_to(project_compare_path(project, from: from_ref, to: to_ref)) end end - context 'when the target ref is empty and source ref is set' do - let(:source_ref) { 'master' } - let(:target_ref) { '' } + context 'with a from_project_id' do + let(:from_project_id) { 'something or another' } - it 'redirects back to index and preserves source ref' do + it 'redirects to the show page without interpreting from_project_id' do create_request - expect(response).to redirect_to(project_compare_index_path(project, from: source_ref)) + expect(response).to redirect_to(project_compare_path(project, from: from_ref, to: to_ref, from_project_id: from_project_id)) end end + end + + context 'when sending invalid params' do + where(:from_ref, :to_ref, :from_project_id, :expected_redirect_params) do + '' | '' | '' | {} + 'main' | '' | '' | { from: 'main' } + '' | 'main' | '' | { to: 'main' } + '' | '' | '1' | { from_project_id: 1 } + 'main' | '' | '1' | { from: 'main', from_project_id: 1 } + '' | 'main' | '1' | { to: 'main', from_project_id: 1 } + end - context 'when the target and source ref are empty' do - let(:source_ref) { '' } - let(:target_ref) { '' } + with_them do + let(:expected_redirect) { project_compare_index_path(project, expected_redirect_params) } - it 'redirects back to index' do + it 'redirects back to the index' do create_request - expect(response).to redirect_to(namespace_project_compare_index_path) + expect(response).to redirect_to(expected_redirect) end end end @@ -268,15 +354,15 @@ def diff_for_path(extra_params = {}) { namespace_id: project.namespace, project_id: project, - from: source_ref, - to: target_ref, + from: from_ref, + to: to_ref, format: :json } end context 'when the source and target refs exist' do - let(:source_ref) { "improve%2Fawesome" } - let(:target_ref) { "feature" } + let(:from_ref) { 'improve%2Fawesome' } + let(:to_ref) { 'feature' } context 'when the user has access to the project' do render_views @@ -285,14 +371,14 @@ def diff_for_path(extra_params = {}) let(:non_signature_commit) { build(:commit, project: project, safe_message: "message", sha: 'non_signature_commit') } before do - escaped_source_ref = Addressable::URI.unescape(source_ref) - escaped_target_ref = Addressable::URI.unescape(target_ref) + escaped_from_ref = Addressable::URI.unescape(from_ref) + escaped_to_ref = Addressable::URI.unescape(to_ref) - compare_service = CompareService.new(project, escaped_target_ref) - compare = compare_service.execute(project, escaped_source_ref) + compare_service = CompareService.new(project, escaped_to_ref) + compare = compare_service.execute(project, escaped_from_ref) - expect(CompareService).to receive(:new).with(project, escaped_target_ref).and_return(compare_service) - expect(compare_service).to receive(:execute).with(project, escaped_source_ref).and_return(compare) + expect(CompareService).to receive(:new).with(project, escaped_to_ref).and_return(compare_service) + expect(compare_service).to receive(:execute).with(project, escaped_from_ref).and_return(compare) expect(compare).to receive(:commits).and_return([signature_commit, non_signature_commit]) expect(non_signature_commit).to receive(:has_signature?).and_return(false) @@ -313,6 +399,7 @@ def diff_for_path(extra_params = {}) context 'when the user does not have access to the project' do before do project.team.truncate + project.update!(visibility: 'private') end it 'returns a 404' do @@ -324,8 +411,8 @@ def diff_for_path(extra_params = {}) end context 'when the source ref does not exist' do - let(:source_ref) { 'non-existent-ref-source' } - let(:target_ref) { "feature" } + let(:from_ref) { 'non-existent-ref-source' } + let(:to_ref) { 'feature' } it 'returns no signatures' do signatures_request @@ -336,8 +423,8 @@ def diff_for_path(extra_params = {}) end context 'when the target ref does not exist' do - let(:target_ref) { 'non-existent-ref-target' } - let(:source_ref) { "improve%2Fawesome" } + let(:from_ref) { 'improve%2Fawesome' } + let(:to_ref) { 'non-existent-ref-target' } it 'returns no signatures' do signatures_request diff --git a/spec/features/projects/merge_request_button_spec.rb b/spec/features/projects/merge_request_button_spec.rb index e3d8534ace96f2..2a486e6e5e1436 100644 --- a/spec/features/projects/merge_request_button_spec.rb +++ b/spec/features/projects/merge_request_button_spec.rb @@ -3,11 +3,13 @@ require 'spec_helper' RSpec.describe 'Merge Request button' do - shared_examples 'Merge request button only shown when allowed' do - let(:user) { create(:user) } - let(:project) { create(:project, :public, :repository) } - let(:forked_project) { create(:project, :public, :repository, forked_from_project: project) } + include ProjectForksHelper + + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :public, :repository) } + let(:forked_project) { fork_project(project, user, repository: true) } + shared_examples 'Merge request button only shown when allowed' do context 'not logged in' do it 'does not show Create merge request button' do visit url @@ -23,9 +25,15 @@ end it 'shows Create merge request button' do - href = project_new_merge_request_path(project, - merge_request: { source_branch: 'feature', - target_branch: 'master' }) + href = project_new_merge_request_path( + project, + merge_request: { + source_project_id: project.id, + source_branch: 'feature', + target_project_id: project.id, + target_branch: 'master' + } + ) visit url @@ -75,12 +83,16 @@ end context 'on own fork of project' do - let(:user) { forked_project.owner } - it 'shows Create merge request button' do - href = project_new_merge_request_path(forked_project, - merge_request: { source_branch: 'feature', - target_branch: 'master' }) + href = project_new_merge_request_path( + forked_project, + merge_request: { + source_project_id: forked_project.id, + source_branch: 'feature', + target_project_id: forked_project.id, + target_branch: 'master' + } + ) visit fork_url @@ -101,11 +113,33 @@ end context 'on compare page' do + let(:label) { 'Create merge request' } + it_behaves_like 'Merge request button only shown when allowed' do - let(:label) { 'Create merge request' } let(:url) { project_compare_path(project, from: 'master', to: 'feature') } let(:fork_url) { project_compare_path(forked_project, from: 'master', to: 'feature') } end + + it 'shows the correct merge request button when viewing across forks' do + sign_in(user) + project.add_developer(user) + + href = project_new_merge_request_path( + project, + merge_request: { + source_project_id: forked_project.id, + source_branch: 'feature', + target_project_id: project.id, + target_branch: 'master' + } + ) + + visit project_compare_path(forked_project, from: 'master', to: 'feature', from_project_id: project.id) + + within("#content-body") do + expect(page).to have_link(label, href: href) + end + end end context 'on commits page' do diff --git a/spec/finders/merge_request_target_project_finder_spec.rb b/spec/finders/merge_request_target_project_finder_spec.rb index dfb4d86fbb60ab..08fbfd7229aec3 100644 --- a/spec/finders/merge_request_target_project_finder_spec.rb +++ b/spec/finders/merge_request_target_project_finder_spec.rb @@ -16,13 +16,22 @@ expect(finder.execute).to contain_exactly(base_project, other_fork, forked_project) end - it 'does not include projects that have merge requests turned off' do + it 'does not include projects that have merge requests turned off by default' do other_fork.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED) base_project.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED) expect(finder.execute).to contain_exactly(forked_project) end + it 'includes projects that have merge requests turned off by default with a more-permissive project feature' do + finder = described_class.new(current_user: user, source_project: forked_project, project_feature: :repository) + + other_fork.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED) + base_project.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED) + + expect(finder.execute).to contain_exactly(base_project, other_fork, forked_project) + end + it 'does not contain archived projects' do base_project.update!(archived: true) -- GitLab