From 7ce39486b54a375eb13a11deb1835d6fcf218a4a Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Thu, 19 Jan 2017 15:42:18 +0100 Subject: [PATCH 1/5] Only show Merge Request button when user can create a MR The Create Merge Request button only should be shown when the user is allowed to create a Merge request. --- app/helpers/compare_helper.rb | 1 + .../tc-only-mr-button-if-allowed.yml | 4 ++ .../branches/merge_request_buttons_spec.rb | 62 +++++++++++++++++++ .../commits/merge_request_button_spec.rb | 62 +++++++++++++++++++ .../compare/merge_request_button_spec.rb | 62 +++++++++++++++++++ 5 files changed, 191 insertions(+) create mode 100644 changelogs/unreleased/tc-only-mr-button-if-allowed.yml create mode 100644 spec/features/projects/branches/merge_request_buttons_spec.rb create mode 100644 spec/features/projects/commits/merge_request_button_spec.rb create mode 100644 spec/features/projects/compare/merge_request_button_spec.rb diff --git a/app/helpers/compare_helper.rb b/app/helpers/compare_helper.rb index aa54ee07bdc..8fcb82f17d5 100644 --- a/app/helpers/compare_helper.rb +++ b/app/helpers/compare_helper.rb @@ -4,6 +4,7 @@ module CompareHelper to.present? && from != to && project.feature_available?(:merge_requests, current_user) && + can?(current_user, :create_merge_request, project) && project.repository.branch_names.include?(from) && project.repository.branch_names.include?(to) end diff --git a/changelogs/unreleased/tc-only-mr-button-if-allowed.yml b/changelogs/unreleased/tc-only-mr-button-if-allowed.yml new file mode 100644 index 00000000000..a7f5dcb560c --- /dev/null +++ b/changelogs/unreleased/tc-only-mr-button-if-allowed.yml @@ -0,0 +1,4 @@ +--- +title: Only show Merge Request button when user can create a MR +merge_request: 8639 +author: diff --git a/spec/features/projects/branches/merge_request_buttons_spec.rb b/spec/features/projects/branches/merge_request_buttons_spec.rb new file mode 100644 index 00000000000..6864113fcb4 --- /dev/null +++ b/spec/features/projects/branches/merge_request_buttons_spec.rb @@ -0,0 +1,62 @@ +require 'spec_helper' + +feature 'Merge Request buttons on branches page', feature: true do + let(:user) { create(:user) } + let(:project) { create(:project) } + + context 'not logged in' do + it 'does not show merge request buttons' do + visit namespace_project_branches_path(project.namespace, project) + + expect(page).to have_no_link('Merge Request') + end + end + + context 'logged in a developer' do + before do + login_as(user) + project.team << [user, :developer] + end + + it 'shows merge request buttons' do + href = new_namespace_project_merge_request_path(project.namespace, + project, + merge_request: { source_branch: 'feature', + target_branch: 'master' }) + + visit namespace_project_branches_path(project.namespace, project) + + expect(page).to have_link('Merge Request', href: href) + end + end + + context 'logged in as non-member' do + before do + login_as(user) + end + + it 'does not show merge request buttons' do + visit namespace_project_branches_path(project.namespace, project) + + expect(page).to have_no_link('Merge Request') + end + + context 'on own fork of project' do + let(:forked_project) do + create(:project, forked_from_project: project) + end + let(:user) { forked_project.owner } + + it 'shows merge request buttons' do + href = new_namespace_project_merge_request_path(forked_project.namespace, + forked_project, + merge_request: { source_branch: 'feature', + target_branch: 'master' }) + + visit namespace_project_branches_path(forked_project.namespace, forked_project) + + expect(page).to have_link('Merge Request', href: href) + end + end + end +end diff --git a/spec/features/projects/commits/merge_request_button_spec.rb b/spec/features/projects/commits/merge_request_button_spec.rb new file mode 100644 index 00000000000..c986d087eba --- /dev/null +++ b/spec/features/projects/commits/merge_request_button_spec.rb @@ -0,0 +1,62 @@ +require 'spec_helper' + +feature 'Merge Request button on commits page', feature: true do + let(:user) { create(:user) } + let(:project) { create(:project) } + + context 'not logged in' do + it 'does not show Create Merge Request button' do + visit namespace_project_commits_path(project.namespace, project, 'feature') + + expect(page).to have_no_link('Create Merge Request') + end + end + + context 'logged in a developer' do + before do + login_as(user) + project.team << [user, :developer] + end + + it 'shows Create Merge Request button' do + href = new_namespace_project_merge_request_path(project.namespace, + project, + merge_request: { source_branch: 'feature', + target_branch: 'master' }) + + visit namespace_project_commits_path(project.namespace, project, 'feature') + + expect(page).to have_link('Create Merge Request', href: href) + end + end + + context 'logged in as non-member' do + before do + login_as(user) + end + + it 'does not show Create Merge Request button' do + visit namespace_project_commits_path(project.namespace, project, 'feature') + + expect(page).to have_no_link('Create Merge Request') + end + + context 'on own fork of project' do + let(:forked_project) do + create(:project, forked_from_project: project) + end + let(:user) { forked_project.owner } + + it 'shows Create Merge Request button' do + href = new_namespace_project_merge_request_path(forked_project.namespace, + forked_project, + merge_request: { source_branch: 'feature', + target_branch: 'master' }) + + visit namespace_project_commits_path(forked_project.namespace, forked_project, 'feature') + + expect(page).to have_link('Create Merge Request', href: href) + end + end + end +end diff --git a/spec/features/projects/compare/merge_request_button_spec.rb b/spec/features/projects/compare/merge_request_button_spec.rb new file mode 100644 index 00000000000..2b49c394759 --- /dev/null +++ b/spec/features/projects/compare/merge_request_button_spec.rb @@ -0,0 +1,62 @@ +require 'spec_helper' + +feature 'Merge Request button on commits page', feature: true do + let(:user) { create(:user) } + let(:project) { create(:project) } + + context 'not logged in' do + it 'does not show Create Merge Request button' do + visit namespace_project_compare_path(project.namespace, project, from: 'master', to: 'feature') + + expect(page).to have_no_link('Create Merge Request') + end + end + + context 'logged in a developer' do + before do + login_as(user) + project.team << [user, :developer] + end + + it 'shows Create Merge Request button' do + href = new_namespace_project_merge_request_path(project.namespace, + project, + merge_request: { source_branch: 'feature', + target_branch: 'master' }) + + visit namespace_project_compare_path(project.namespace, project, from: 'master', to: 'feature') + + expect(page).to have_link('Create Merge Request', href: href) + end + end + + context 'logged in as non-member' do + before do + login_as(user) + end + + it 'does not show Create Merge Request button' do + visit namespace_project_compare_path(project.namespace, project, from: 'master', to: 'feature') + + expect(page).to have_no_link('Create Merge Request') + end + + context 'on own fork of project' do + let(:forked_project) do + create(:project, forked_from_project: project) + end + let(:user) { forked_project.owner } + + it 'shows Create Merge Request button' do + href = new_namespace_project_merge_request_path(forked_project.namespace, + forked_project, + merge_request: { source_branch: 'feature', + target_branch: 'master' }) + + visit namespace_project_compare_path(forked_project.namespace, forked_project, from: 'master', to: 'feature') + + expect(page).to have_link('Create Merge Request', href: href) + end + end + end +end From 0d2c68d546cb58760a9d30a41b1454b02c462ad8 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Fri, 20 Jan 2017 22:41:03 +0100 Subject: [PATCH 2/5] Deduplicate tests for Merge Request buttons Use shared examples to test the presence of the Merge Request button on the various pages. --- .../branches/merge_request_buttons_spec.rb | 62 ------------- .../commits/merge_request_button_spec.rb | 62 ------------- .../compare/merge_request_button_spec.rb | 62 ------------- .../projects/merge_request_button_spec.rb | 86 +++++++++++++++++++ 4 files changed, 86 insertions(+), 186 deletions(-) delete mode 100644 spec/features/projects/branches/merge_request_buttons_spec.rb delete mode 100644 spec/features/projects/commits/merge_request_button_spec.rb delete mode 100644 spec/features/projects/compare/merge_request_button_spec.rb create mode 100644 spec/features/projects/merge_request_button_spec.rb diff --git a/spec/features/projects/branches/merge_request_buttons_spec.rb b/spec/features/projects/branches/merge_request_buttons_spec.rb deleted file mode 100644 index 6864113fcb4..00000000000 --- a/spec/features/projects/branches/merge_request_buttons_spec.rb +++ /dev/null @@ -1,62 +0,0 @@ -require 'spec_helper' - -feature 'Merge Request buttons on branches page', feature: true do - let(:user) { create(:user) } - let(:project) { create(:project) } - - context 'not logged in' do - it 'does not show merge request buttons' do - visit namespace_project_branches_path(project.namespace, project) - - expect(page).to have_no_link('Merge Request') - end - end - - context 'logged in a developer' do - before do - login_as(user) - project.team << [user, :developer] - end - - it 'shows merge request buttons' do - href = new_namespace_project_merge_request_path(project.namespace, - project, - merge_request: { source_branch: 'feature', - target_branch: 'master' }) - - visit namespace_project_branches_path(project.namespace, project) - - expect(page).to have_link('Merge Request', href: href) - end - end - - context 'logged in as non-member' do - before do - login_as(user) - end - - it 'does not show merge request buttons' do - visit namespace_project_branches_path(project.namespace, project) - - expect(page).to have_no_link('Merge Request') - end - - context 'on own fork of project' do - let(:forked_project) do - create(:project, forked_from_project: project) - end - let(:user) { forked_project.owner } - - it 'shows merge request buttons' do - href = new_namespace_project_merge_request_path(forked_project.namespace, - forked_project, - merge_request: { source_branch: 'feature', - target_branch: 'master' }) - - visit namespace_project_branches_path(forked_project.namespace, forked_project) - - expect(page).to have_link('Merge Request', href: href) - end - end - end -end diff --git a/spec/features/projects/commits/merge_request_button_spec.rb b/spec/features/projects/commits/merge_request_button_spec.rb deleted file mode 100644 index c986d087eba..00000000000 --- a/spec/features/projects/commits/merge_request_button_spec.rb +++ /dev/null @@ -1,62 +0,0 @@ -require 'spec_helper' - -feature 'Merge Request button on commits page', feature: true do - let(:user) { create(:user) } - let(:project) { create(:project) } - - context 'not logged in' do - it 'does not show Create Merge Request button' do - visit namespace_project_commits_path(project.namespace, project, 'feature') - - expect(page).to have_no_link('Create Merge Request') - end - end - - context 'logged in a developer' do - before do - login_as(user) - project.team << [user, :developer] - end - - it 'shows Create Merge Request button' do - href = new_namespace_project_merge_request_path(project.namespace, - project, - merge_request: { source_branch: 'feature', - target_branch: 'master' }) - - visit namespace_project_commits_path(project.namespace, project, 'feature') - - expect(page).to have_link('Create Merge Request', href: href) - end - end - - context 'logged in as non-member' do - before do - login_as(user) - end - - it 'does not show Create Merge Request button' do - visit namespace_project_commits_path(project.namespace, project, 'feature') - - expect(page).to have_no_link('Create Merge Request') - end - - context 'on own fork of project' do - let(:forked_project) do - create(:project, forked_from_project: project) - end - let(:user) { forked_project.owner } - - it 'shows Create Merge Request button' do - href = new_namespace_project_merge_request_path(forked_project.namespace, - forked_project, - merge_request: { source_branch: 'feature', - target_branch: 'master' }) - - visit namespace_project_commits_path(forked_project.namespace, forked_project, 'feature') - - expect(page).to have_link('Create Merge Request', href: href) - end - end - end -end diff --git a/spec/features/projects/compare/merge_request_button_spec.rb b/spec/features/projects/compare/merge_request_button_spec.rb deleted file mode 100644 index 2b49c394759..00000000000 --- a/spec/features/projects/compare/merge_request_button_spec.rb +++ /dev/null @@ -1,62 +0,0 @@ -require 'spec_helper' - -feature 'Merge Request button on commits page', feature: true do - let(:user) { create(:user) } - let(:project) { create(:project) } - - context 'not logged in' do - it 'does not show Create Merge Request button' do - visit namespace_project_compare_path(project.namespace, project, from: 'master', to: 'feature') - - expect(page).to have_no_link('Create Merge Request') - end - end - - context 'logged in a developer' do - before do - login_as(user) - project.team << [user, :developer] - end - - it 'shows Create Merge Request button' do - href = new_namespace_project_merge_request_path(project.namespace, - project, - merge_request: { source_branch: 'feature', - target_branch: 'master' }) - - visit namespace_project_compare_path(project.namespace, project, from: 'master', to: 'feature') - - expect(page).to have_link('Create Merge Request', href: href) - end - end - - context 'logged in as non-member' do - before do - login_as(user) - end - - it 'does not show Create Merge Request button' do - visit namespace_project_compare_path(project.namespace, project, from: 'master', to: 'feature') - - expect(page).to have_no_link('Create Merge Request') - end - - context 'on own fork of project' do - let(:forked_project) do - create(:project, forked_from_project: project) - end - let(:user) { forked_project.owner } - - it 'shows Create Merge Request button' do - href = new_namespace_project_merge_request_path(forked_project.namespace, - forked_project, - merge_request: { source_branch: 'feature', - target_branch: 'master' }) - - visit namespace_project_compare_path(forked_project.namespace, forked_project, from: 'master', to: 'feature') - - expect(page).to have_link('Create Merge Request', href: href) - end - end - end -end diff --git a/spec/features/projects/merge_request_button_spec.rb b/spec/features/projects/merge_request_button_spec.rb new file mode 100644 index 00000000000..177038f9a4c --- /dev/null +++ b/spec/features/projects/merge_request_button_spec.rb @@ -0,0 +1,86 @@ +require 'spec_helper' + +feature 'Merge Request button', feature: true do + shared_examples 'Merge Request button only shown when allowed' do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:forked_project) { create(:project, forked_from_project: project) } + + context 'not logged in' do + it 'does not show Create Merge Request button' do + visit url + + expect(page).not_to have_link(label) + end + end + + context 'logged in as developer' do + before do + login_as(user) + project.team << [user, :developer] + end + + it 'shows Create Merge Request button' do + href = new_namespace_project_merge_request_path(project.namespace, + project, + merge_request: { source_branch: 'feature', + target_branch: 'master' }) + + visit url + + expect(page).to have_link(label, href: href) + end + end + + context 'logged in as non-member' do + before do + login_as(user) + end + + it 'does not show Create Merge Request button' do + visit url + + expect(page).not_to have_link(label) + end + + context 'on own fork of project' do + let(:user) { forked_project.owner } + + it 'shows Create Merge Request button' do + href = new_namespace_project_merge_request_path(forked_project.namespace, + forked_project, + merge_request: { source_branch: 'feature', + target_branch: 'master' }) + + visit fork_url + + expect(page).to have_link(label, href: href) + end + end + end + end + + context 'on branches page' do + it_behaves_like 'Merge Request button only shown when allowed' do + let(:label) { 'Merge Request' } + let(:url) { namespace_project_branches_path(project.namespace, project) } + let(:fork_url) { namespace_project_branches_path(forked_project.namespace, forked_project) } + end + end + + context 'on compare page' do + it_behaves_like 'Merge Request button only shown when allowed' do + let(:label) { 'Create Merge Request' } + let(:url) { namespace_project_compare_path(project.namespace, project, from: 'master', to: 'feature') } + let(:fork_url) { namespace_project_compare_path(forked_project.namespace, forked_project, from: 'master', to: 'feature') } + end + end + + context 'on commits page' do + it_behaves_like 'Merge Request button only shown when allowed' do + let(:label) { 'Create Merge Request' } + let(:url) { namespace_project_commits_path(project.namespace, project, 'feature') } + let(:fork_url) { namespace_project_commits_path(forked_project.namespace, forked_project, 'feature') } + end + end +end From 2669fdbe74a50f80d86119469deb1f48575816f5 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Wed, 25 Jan 2017 11:07:02 +0100 Subject: [PATCH 3/5] Ensure the correct Merge Request button is found The project was not public and this caused a 404. So the tests were giving deceiving results. By searching for the Merge Request button in `#content-body` this is overcome, and also other Merge Request buttons (e.g. in the sidebar) are ignored. --- .../projects/merge_request_button_spec.rb | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/spec/features/projects/merge_request_button_spec.rb b/spec/features/projects/merge_request_button_spec.rb index 177038f9a4c..e3c6347e81f 100644 --- a/spec/features/projects/merge_request_button_spec.rb +++ b/spec/features/projects/merge_request_button_spec.rb @@ -3,14 +3,16 @@ require 'spec_helper' feature 'Merge Request button', feature: true do shared_examples 'Merge Request button only shown when allowed' do let(:user) { create(:user) } - let(:project) { create(:project) } - let(:forked_project) { create(:project, forked_from_project: project) } + let(:project) { create(:project, :public) } + let(:forked_project) { create(:project, :public, forked_from_project: project) } context 'not logged in' do it 'does not show Create Merge Request button' do visit url - expect(page).not_to have_link(label) + within("#content-body") do + expect(page).not_to have_link(label) + end end end @@ -28,7 +30,9 @@ feature 'Merge Request button', feature: true do visit url - expect(page).to have_link(label, href: href) + within("#content-body") do + expect(page).to have_link(label, href: href) + end end end @@ -40,7 +44,9 @@ feature 'Merge Request button', feature: true do it 'does not show Create Merge Request button' do visit url - expect(page).not_to have_link(label) + within("#content-body") do + expect(page).not_to have_link(label) + end end context 'on own fork of project' do @@ -54,7 +60,9 @@ feature 'Merge Request button', feature: true do visit fork_url - expect(page).to have_link(label, href: href) + within("#content-body") do + expect(page).to have_link(label, href: href) + end end end end From eaffe8cd7202dfb0e459db484ebd4fe4c9d2e155 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Wed, 25 Jan 2017 11:11:12 +0100 Subject: [PATCH 4/5] Test there is no Merge Request button when MRs are disabled In case Merge Requests are disabled on the project, no one should see the Merge Request button. --- .../features/projects/merge_request_button_spec.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/spec/features/projects/merge_request_button_spec.rb b/spec/features/projects/merge_request_button_spec.rb index e3c6347e81f..b6728960fb8 100644 --- a/spec/features/projects/merge_request_button_spec.rb +++ b/spec/features/projects/merge_request_button_spec.rb @@ -34,6 +34,20 @@ feature 'Merge Request button', feature: true do expect(page).to have_link(label, href: href) end end + + context 'merge requests are disabled' do + before do + project.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED) + end + + it 'does not show Create Merge Request button' do + visit url + + within("#content-body") do + expect(page).not_to have_link(label) + end + end + end end context 'logged in as non-member' do From aa0bc52a87e98eed2590ab3a5f95f55eb3968bc0 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Wed, 25 Jan 2017 11:12:09 +0100 Subject: [PATCH 5/5] `can?` already includes the `feature_available?` check `can?` already includes the `feature_available?` check, so no need to check this again. --- app/helpers/compare_helper.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/helpers/compare_helper.rb b/app/helpers/compare_helper.rb index 8fcb82f17d5..2aa0449c46e 100644 --- a/app/helpers/compare_helper.rb +++ b/app/helpers/compare_helper.rb @@ -3,7 +3,6 @@ module CompareHelper from.present? && to.present? && from != to && - project.feature_available?(:merge_requests, current_user) && can?(current_user, :create_merge_request, project) && project.repository.branch_names.include?(from) && project.repository.branch_names.include?(to)