From 09454b702a960923ca4f7f21f41e64404542d90a Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Wed, 19 Jun 2019 12:41:21 -0800 Subject: Support creating an MR on a fork from an issue --- app/controllers/projects/issues_controller.rb | 1 + .../merge_requests/create_from_issue_service.rb | 38 ++- .../create_from_issue_service_spec.rb | 276 +++++++++++++++------ 3 files changed, 222 insertions(+), 93 deletions(-) diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index f221f0363d3..e275b417784 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -172,6 +172,7 @@ class Projects::IssuesController < Projects::ApplicationController def create_merge_request create_params = params.slice(:branch_name, :ref).merge(issue_iid: issue.iid) + create_params[:target_project_id] = params[:target_project_id] if Feature.enabled?(:create_confidential_merge_request, @project) result = ::MergeRequests::CreateFromIssueService.new(project, current_user, create_params).execute if result[:status] == :success diff --git a/app/services/merge_requests/create_from_issue_service.rb b/app/services/merge_requests/create_from_issue_service.rb index e69791872cc..074a45e328b 100644 --- a/app/services/merge_requests/create_from_issue_service.rb +++ b/app/services/merge_requests/create_from_issue_service.rb @@ -6,27 +6,30 @@ module MergeRequests # branch - the name of new branch # ref - the source of new branch. - @branch_name = params[:branch_name] - @issue_iid = params[:issue_iid] - @ref = params[:ref] + @branch_name = params[:branch_name] + @issue_iid = params[:issue_iid] + @ref = params[:ref] + @target_project_id = params[:target_project_id] super(project, user) end def execute + return error('Project not found') if target_project.blank? + return error('Not allowed to create merge request') unless can_create_merge_request? return error('Invalid issue iid') unless @issue_iid.present? && issue.present? - result = CreateBranchService.new(project, current_user).execute(branch_name, ref) + result = CreateBranchService.new(target_project, current_user).execute(branch_name, ref) return result if result[:status] == :error new_merge_request = create(merge_request) if new_merge_request.valid? - SystemNoteService.new_merge_request(issue, project, current_user, new_merge_request) + SystemNoteService.new_merge_request(issue, target_project, current_user, new_merge_request) success(new_merge_request) else - SystemNoteService.new_issue_branch(issue, project, current_user, branch_name) + SystemNoteService.new_issue_branch(issue, target_project, current_user, branch_name) error(new_merge_request.errors) end @@ -34,6 +37,10 @@ module MergeRequests private + def can_create_merge_request? + can?(current_user, :create_merge_request_from, target_project) + end + # rubocop: disable CodeReuse/ActiveRecord def issue @issue ||= IssuesFinder.new(current_user, project_id: project.id).find_by(iid: @issue_iid) @@ -45,21 +52,21 @@ module MergeRequests end def ref - return @ref if project.repository.branch_exists?(@ref) + return @ref if target_project.repository.branch_exists?(@ref) - project.default_branch || 'master' + target_project.default_branch || 'master' end def merge_request - MergeRequests::BuildService.new(project, current_user, merge_request_params).execute + MergeRequests::BuildService.new(target_project, current_user, merge_request_params).execute end def merge_request_params { issue_iid: @issue_iid, - source_project_id: project.id, + source_project_id: target_project.id, source_branch: branch_name, - target_project_id: project.id, + target_project_id: target_project.id, target_branch: ref } end @@ -67,5 +74,14 @@ module MergeRequests def success(merge_request) super().merge(merge_request: merge_request) end + + def target_project + @target_project ||= + if @target_project_id.present? + project.forks.find_by_id(@target_project_id) + else + project + end + end 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 a0ac7dba89d..6b17b14d4b4 100644 --- a/spec/services/merge_requests/create_from_issue_service_spec.rb +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe MergeRequests::CreateFromIssueService do + include ProjectForksHelper + let(:project) { create(:project, :repository) } let(:user) { create(:user) } let(:label_ids) { create_pair(:label, project: project).map(&:id) } @@ -10,139 +12,249 @@ describe MergeRequests::CreateFromIssueService do let(:issue) { create(:issue, project: project, milestone_id: milestone_id) } let(:custom_source_branch) { 'custom-source-branch' } - subject(:service) { described_class.new(project, user, issue_iid: issue.iid) } - subject(:service_with_custom_source_branch) { described_class.new(project, user, issue_iid: issue.iid, branch_name: 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)) } before do project.add_developer(user) end describe '#execute' do - it 'returns an error with invalid issue iid' do - result = described_class.new(project, user, issue_iid: -1).execute - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Invalid issue iid') - end + context 'no target_project_id specified' do + let(:service_params) { { issue_iid: issue.iid } } - it 'delegates issue search to IssuesFinder' do - expect_any_instance_of(IssuesFinder).to receive(:find_by).once.and_call_original + it 'returns an error with invalid issue iid' do + result = described_class.new(project, user, issue_iid: -1).execute - described_class.new(project, user, issue_iid: -1).execute - end + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('Invalid issue iid') + end - it "inherits labels" do - issue.assign_attributes(label_ids: label_ids) + it "inherits labels" do + issue.assign_attributes(label_ids: label_ids) - result = service.execute + result = service.execute - expect(result[:merge_request].label_ids).to eq(label_ids) - end + expect(result[:merge_request].label_ids).to eq(label_ids) + end - it "inherits milestones" do - result = service.execute + it "inherits milestones" do + result = service.execute - expect(result[:merge_request].milestone_id).to eq(milestone_id) - end + 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 + it 'delegates the branch creation to CreateBranchService' do + expect_any_instance_of(CreateBranchService).to receive(:execute).once.and_call_original - service.execute - end + service.execute + end - it 'creates a branch based on issue title' do - service.execute + it 'creates a branch based on issue title' do + service.execute - expect(project.repository.branch_exists?(issue.to_branch_name)).to be_truthy - end + expect(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 + 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 - end + expect(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)) + it 'creates the new_merge_request system note' do + expect(SystemNoteService).to receive(:new_merge_request).with(issue, project, user, instance_of(MergeRequest)) - service.execute - end + service.execute + end - it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do - project.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED) + 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, project, user, issue.to_branch_name) - service.execute - end + service.execute + end - it 'creates a merge request' do - expect { service.execute }.to change(project.merge_requests, :count).by(1) - 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 + 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}\"") - end + expect(result[:merge_request].title).to eq("WIP: Resolve \"#{issue.title}\"") + end - it 'sets the merge request author to current user' do - result = service.execute + it 'sets the merge request author to current user' do + result = service.execute - expect(result[:merge_request].author).to eq(user) - end + expect(result[:merge_request].author).to eq(user) + end - it 'sets the merge request source branch to the new issue branch' do - result = service.execute + it 'sets the merge request source branch to the new issue branch' do + result = service.execute - expect(result[:merge_request].source_branch).to eq(issue.to_branch_name) - end + expect(result[:merge_request].source_branch).to eq(issue.to_branch_name) + end - it 'sets the merge request source branch to the passed branch name' do - result = service_with_custom_source_branch.execute + 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) - end + expect(result[:merge_request].source_branch).to eq(custom_source_branch) + end - it 'sets the merge request target branch to the project default branch' do - result = service.execute + 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 + expect(result[:merge_request].target_branch).to eq(project.default_branch) + end - 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}" + 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 + + result = service.execute + + expect(result[:merge_request].assignees).to eq([user]) end - result = service.execute + context 'when ref branch is set' do + subject { described_class.new(project, user, service_params.merge(ref: 'feature')).execute } - expect(result[:merge_request].assignees).to eq([user]) - end + 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 - context 'when ref branch is set' do - subject { described_class.new(project, user, issue_iid: issue.iid, ref: 'feature').execute } + it 'sets the merge request target branch to the ref branch' do + expect(subject[:merge_request].target_branch).to eq('feature') + end - 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) + 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(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) + 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 'sets the merge request target branch to the ref branch' do - expect(subject[:merge_request].target_branch).to eq('feature') + it 'does not create merge request' do + expect { service.execute }.to change(target_project.merge_requests, :count).by(0) + end end - context 'when ref branch does not exist' do - subject { described_class.new(project, user, issue_iid: issue.iid, ref: 'no-such-branch').execute } + 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 + + it 'creates the new_merge_request system note' do + expect(SystemNoteService).to receive(:new_merge_request).with(issue, target_project, user, instance_of(MergeRequest)) + + service.execute + end + + 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, target_project, user, issue.to_branch_name) + + service.execute + end it 'creates a merge request' do - expect { subject }.to change(project.merge_requests, :count).by(1) + expect { service.execute }.to change(target_project.merge_requests, :count).by(1) + end + + it 'sets the merge request title to: "WIP: $issue-branch-name' do + result = service.execute + + expect(result[:merge_request].title).to eq("WIP: #{issue.to_branch_name.titleize.humanize}") + end + + it 'sets the merge request author to current user' do + result = service.execute + + expect(result[:merge_request].author).to eq(user) + end + + it 'sets the merge request source branch to the new issue branch' do + result = service.execute + + expect(result[:merge_request].source_branch).to eq(issue.to_branch_name) + 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) 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) + result = service.execute + + expect(result[:merge_request].target_branch).to eq(project.default_branch) + end + + 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 + + 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 end end end -- cgit v1.2.1