From ac3de494bde32e4ce13bd665d1de3132b84c002d Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Wed, 19 Jun 2019 16:56:17 +0800 Subject: Support branch creation from confidential issue Accept a `confidential_issue_project_id` param which will be used for the system note target. This also includes some refactoring on the spec to use shared examples. --- app/controllers/projects/branches_controller.rb | 13 +- .../projects/branches_controller_spec.rb | 69 ++++++++ .../controllers/projects/issues_controller_spec.rb | 44 ++++- .../create_from_issue_service_spec.rb | 177 ++++++--------------- 4 files changed, 170 insertions(+), 133 deletions(-) diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb index fc708400657..df4002d075b 100644 --- a/app/controllers/projects/branches_controller.rb +++ b/app/controllers/projects/branches_controller.rb @@ -64,7 +64,7 @@ class Projects::BranchesController < Projects::ApplicationController success = (result[:status] == :success) if params[:issue_iid] && success - issue = IssuesFinder.new(current_user, project_id: @project.id).find_by(iid: params[:issue_iid]) + issue = IssuesFinder.new(current_user, project_id: (confidential_issue_project || @project).id).find_by(iid: params[:issue_iid]) SystemNoteService.new_issue_branch(issue, @project, current_user, branch_name) if issue end @@ -162,4 +162,15 @@ class Projects::BranchesController < Projects::ApplicationController @branches = Kaminari.paginate_array(@branches).page(params[:page]) end end + + def confidential_issue_project + return unless Feature.enabled?(:create_confidential_merge_request, @project) + return if params[:confidential_issue_project_id].blank? + + confidential_issue_project = Project.find(params[:confidential_issue_project_id]) + + return unless can?(current_user, :push_code, confidential_issue_project) + + confidential_issue_project + end end diff --git a/spec/controllers/projects/branches_controller_spec.rb b/spec/controllers/projects/branches_controller_spec.rb index cf201c9f735..8c349b1e988 100644 --- a/spec/controllers/projects/branches_controller_spec.rb +++ b/spec/controllers/projects/branches_controller_spec.rb @@ -103,6 +103,75 @@ describe Projects::BranchesController do } end + context 'confidential_issue_project_id is present' do + let(:confidential_issue_project) { create(:project) } + + def create_branch_with_confidential_issue_project + post( + :create, + params: { + namespace_id: project.namespace, + project_id: project, + branch_name: branch, + confidential_issue_project_id: confidential_issue_project.id, + issue_iid: issue.iid + } + ) + end + + context 'create_confidential_merge_request feature is enabled' do + before do + stub_feature_flags(create_confidential_merge_request: true) + end + + context 'user cannot push code to issue project' do + let(:issue) { create(:issue, project: confidential_issue_project) } + + it 'does not post a system note' do + expect(SystemNoteService).not_to receive(:new_issue_branch) + + create_branch_with_confidential_issue_project + end + end + + context 'user can push code to issue project' do + before do + confidential_issue_project.add_developer(user) + end + + context 'issue is under the specified project' do + let(:issue) { create(:issue, project: confidential_issue_project) } + + it 'posts a system note' do + expect(SystemNoteService).to receive(:new_issue_branch).with(issue, confidential_issue_project, user, "1-feature-branch", branch_project: project) + + create_branch_with_confidential_issue_project + end + end + + context 'issue is not under the specified project' do + it 'does not post a system note' do + expect(SystemNoteService).not_to receive(:new_issue_branch) + + create_branch_with_confidential_issue_project + end + end + end + end + + context 'create_confidential_merge_request feature is disabled' do + before do + stub_feature_flags(create_confidential_merge_request: false) + end + + it 'posts a system note on project' do + expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, "1-feature-branch", branch_project: project) + + create_branch_with_confidential_issue_project + end + end + end + context 'repository-less project' do let(:project) { create :project } diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index f82e3c8c7dc..bc5e0b4671e 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe Projects::IssuesController do + include ProjectForksHelper + let(:project) { create(:project) } let(:user) { create(:user) } let(:issue) { create(:issue, project: project) } @@ -1130,6 +1132,7 @@ describe Projects::IssuesController do end describe 'POST create_merge_request' do + let(:target_project_id) { nil } let(:project) { create(:project, :repository, :public) } before do @@ -1163,13 +1166,42 @@ describe Projects::IssuesController do expect(response).to have_gitlab_http_status(404) end + context 'target_project_id is set' do + let(:target_project) { fork_project(project, user, repository: true) } + let(:target_project_id) { target_project.id } + + context 'create_confidential_merge_request feature is enabled' do + before do + stub_feature_flags(create_confidential_merge_request: true) + end + + it 'creates a new merge request' do + expect { create_merge_request }.to change(target_project.merge_requests, :count).by(1) + end + end + + context 'create_confidential_merge_request feature is disabled' do + before do + stub_feature_flags(create_confidential_merge_request: false) + end + + it 'creates a new merge request' do + expect { create_merge_request }.to change(project.merge_requests, :count).by(1) + end + end + end + def create_merge_request - post :create_merge_request, params: { - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: issue.to_param - }, - format: :json + post( + :create_merge_request, + params: { + namespace_id: project.namespace.to_param, + project_id: project.to_param, + id: issue.to_param, + target_project_id: target_project_id + }, + format: :json + ) end end diff --git a/spec/services/merge_requests/create_from_issue_service_spec.rb b/spec/services/merge_requests/create_from_issue_service_spec.rb index 6b17b14d4b4..2c83b2851a1 100644 --- a/spec/services/merge_requests/create_from_issue_service_spec.rb +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -13,15 +13,20 @@ describe MergeRequests::CreateFromIssueService do let(:custom_source_branch) { 'custom-source-branch' } subject(:service) { described_class.new(project, user, service_params) } - subject(:service_with_custom_source_branch) { described_class.new(project, user, service_params.merge(branch_name: custom_source_branch)) } + subject(:service_with_custom_source_branch) { described_class.new(project, user, branch_name: custom_source_branch, **service_params) } before do project.add_developer(user) end describe '#execute' do - context 'no target_project_id specified' do - let(:service_params) { { issue_iid: issue.iid } } + shared_examples_for 'a service that creates a merge request from an issue' do + it 'returns an error when user can not create merge request on target project' do + result = described_class.new(project, create(:user), service_params).execute + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('Not allowed to create merge request') + end it 'returns an error with invalid issue iid' do result = described_class.new(project, user, issue_iid: -1).execute @@ -30,40 +35,20 @@ describe MergeRequests::CreateFromIssueService do expect(result[:message]).to eq('Invalid issue iid') end - it "inherits labels" do - issue.assign_attributes(label_ids: label_ids) - - result = service.execute - - expect(result[:merge_request].label_ids).to eq(label_ids) - end - - it "inherits milestones" do - result = service.execute - - expect(result[:merge_request].milestone_id).to eq(milestone_id) - end - - it 'delegates the branch creation to CreateBranchService' do - expect_any_instance_of(CreateBranchService).to receive(:execute).once.and_call_original - - service.execute - end - it 'creates a branch based on issue title' do service.execute - expect(project.repository.branch_exists?(issue.to_branch_name)).to be_truthy + expect(target_project.repository.branch_exists?(issue.to_branch_name)).to be_truthy end it 'creates a branch using passed name' do service_with_custom_source_branch.execute - expect(project.repository.branch_exists?(custom_source_branch)).to be_truthy + expect(target_project.repository.branch_exists?(custom_source_branch)).to be_truthy end it 'creates the new_merge_request system note' do - expect(SystemNoteService).to receive(:new_merge_request).with(issue, project, user, instance_of(MergeRequest)) + expect(SystemNoteService).to receive(:new_merge_request).with(issue, target_project, user, instance_of(MergeRequest)) service.execute end @@ -71,19 +56,13 @@ describe MergeRequests::CreateFromIssueService do it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do expect_any_instance_of(MergeRequest).to receive(:valid?).at_least(:once).and_return(false) - expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, issue.to_branch_name) + expect(SystemNoteService).to receive(:new_issue_branch).with(issue, target_project, user, issue.to_branch_name) service.execute end it 'creates a merge request' do - expect { service.execute }.to change(project.merge_requests, :count).by(1) - end - - it 'sets the merge request title to: "WIP: Resolves "$issue-title"' do - result = service.execute - - expect(result[:merge_request].title).to eq("WIP: Resolve \"#{issue.title}\"") + expect { service.execute }.to change(target_project.merge_requests, :count).by(1) end it 'sets the merge request author to current user' do @@ -107,7 +86,7 @@ describe MergeRequests::CreateFromIssueService do it 'sets the merge request target branch to the project default branch' do result = service.execute - expect(result[:merge_request].target_branch).to eq(project.default_branch) + expect(result[:merge_request].target_branch).to eq(target_project.default_branch) end it 'executes quick actions if the build service sets them in the description' do @@ -123,7 +102,7 @@ describe MergeRequests::CreateFromIssueService do end context 'when ref branch is set' do - subject { described_class.new(project, user, service_params.merge(ref: 'feature')).execute } + subject { described_class.new(project, user, ref: 'feature', **service_params).execute } it 'sets the merge request source branch to the new issue branch' do expect(subject[:merge_request].source_branch).to eq(issue.to_branch_name) @@ -134,127 +113,73 @@ describe MergeRequests::CreateFromIssueService do end context 'when ref branch does not exist' do - subject { described_class.new(project, user, service_params.merge(ref: 'no-such-branch')).execute } + subject { described_class.new(project, user, ref: 'no-such-branch', **service_params).execute } it 'creates a merge request' do - expect { subject }.to change(project.merge_requests, :count).by(1) + expect { subject }.to change(target_project.merge_requests, :count).by(1) end it 'sets the merge request target branch to the project default branch' do - expect(subject[:merge_request].target_branch).to eq(project.default_branch) + expect(subject[:merge_request].target_branch).to eq(target_project.default_branch) end end end end - context 'target_project_id is specified' do - let(:service_params) { { issue_iid: issue.iid, target_project_id: target_project.id } } - - context 'target project is not a fork of the project' do - let(:target_project) { create(:project, :repository) } - - it 'does not create merge request' do - expect { service.execute }.to change(target_project.merge_requests, :count).by(0) - end - end - - context 'target project is a fork of project project' do - let(:target_project) { fork_project(project, user, repository: true) } - - it 'creates a branch based on issue title' do - service.execute - - expect(target_project.repository.branch_exists?(issue.to_branch_name)).to be_truthy - end - - it 'creates a branch using passed name' do - service_with_custom_source_branch.execute - - expect(target_project.repository.branch_exists?(custom_source_branch)).to be_truthy - end + context 'no target_project_id specified' do + let(:service_params) { { issue_iid: issue.iid } } + let(:target_project) { project } - it 'creates the new_merge_request system note' do - expect(SystemNoteService).to receive(:new_merge_request).with(issue, target_project, user, instance_of(MergeRequest)) + it_behaves_like 'a service that creates a merge request from an issue' - service.execute - end + it "inherits labels" do + issue.assign_attributes(label_ids: label_ids) - it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do - expect_any_instance_of(MergeRequest).to receive(:valid?).at_least(:once).and_return(false) + result = service.execute - expect(SystemNoteService).to receive(:new_issue_branch).with(issue, target_project, user, issue.to_branch_name) + expect(result[:merge_request].label_ids).to eq(label_ids) + end - service.execute - end + it "inherits milestones" do + result = service.execute - it 'creates a merge request' do - expect { service.execute }.to change(target_project.merge_requests, :count).by(1) - end + expect(result[:merge_request].milestone_id).to eq(milestone_id) + end - it 'sets the merge request title to: "WIP: $issue-branch-name' do - result = service.execute + it 'sets the merge request title to: "WIP: Resolves "$issue-title"' do + result = service.execute - expect(result[:merge_request].title).to eq("WIP: #{issue.to_branch_name.titleize.humanize}") - end + expect(result[:merge_request].title).to eq("WIP: Resolve \"#{issue.title}\"") + end + end - it 'sets the merge request author to current user' do - result = service.execute + context 'target_project_id is specified' do + let(:service_params) { { issue_iid: issue.iid, target_project_id: target_project.id } } - expect(result[:merge_request].author).to eq(user) - end + context 'target project is not a fork of the project' do + let(:target_project) { create(:project, :repository) } - it 'sets the merge request source branch to the new issue branch' do + it 'returns an error about not finding the project' do result = service.execute - expect(result[:merge_request].source_branch).to eq(issue.to_branch_name) + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('Project not found') end - it 'sets the merge request source branch to the passed branch name' do - result = service_with_custom_source_branch.execute - - expect(result[:merge_request].source_branch).to eq(custom_source_branch) + it 'does not create merge request' do + expect { service.execute }.to change(target_project.merge_requests, :count).by(0) end + end - it 'sets the merge request target branch to the project default branch' do - result = service.execute - - expect(result[:merge_request].target_branch).to eq(project.default_branch) - end + context 'target project is a fork of project project' do + let(:target_project) { fork_project(project, user, repository: true) } - it 'executes quick actions if the build service sets them in the description' do - allow(service).to receive(:merge_request).and_wrap_original do |m, *args| - m.call(*args).tap do |merge_request| - merge_request.description = "/assign #{user.to_reference}" - end - end + it_behaves_like 'a service that creates a merge request from an issue' + it 'sets the merge request title to: "WIP: $issue-branch-name' do result = service.execute - expect(result[:merge_request].assignees).to eq([user]) - end - - context 'when ref branch is set' do - subject { described_class.new(project, user, service_params.merge(ref: 'feature')).execute } - - it 'sets the merge request source branch to the new issue branch' do - expect(subject[:merge_request].source_branch).to eq(issue.to_branch_name) - end - - it 'sets the merge request target branch to the ref branch' do - expect(subject[:merge_request].target_branch).to eq('feature') - end - - context 'when ref branch does not exist' do - subject { described_class.new(project, user, service_params.merge(ref: 'no-such-branch')).execute } - - it 'creates a merge request' do - expect { subject }.to change(target_project.merge_requests, :count).by(1) - end - - it 'sets the merge request target branch to the project default branch' do - expect(subject[:merge_request].target_branch).to eq(target_project.default_branch) - end - end + expect(result[:merge_request].title).to eq("WIP: #{issue.to_branch_name.titleize.humanize}") end end end -- cgit v1.2.1