diff options
author | Luke Duncalfe <lduncalfe@eml.cc> | 2019-04-08 08:07:53 +0300 |
---|---|---|
committer | Luke Duncalfe <lduncalfe@eml.cc> | 2019-04-09 01:57:01 +0300 |
commit | e73f537cb5097e85849110bafe184cb89e3bbc22 (patch) | |
tree | c711e4148007708c5dcb9fb45eab68092b8a9503 | |
parent | 68f189ad23d7a384f40caa152d263fdf1465b30a (diff) |
Refactor PushOptionsHandlerService from review
Exceptions are no longer raised, instead all errors encountered are
added to the errors property.
MergeRequests::BuildService is used to generate attributes of a new
merge request.
Code moved from Api::Internal to Api::Helpers::InternalHelpers.
-rw-r--r-- | app/services/merge_requests/push_options_handler_service.rb | 97 | ||||
-rw-r--r-- | lib/api/helpers/internal_helpers.rb | 17 | ||||
-rw-r--r-- | lib/api/internal.rb | 20 | ||||
-rw-r--r-- | spec/requests/api/internal_spec.rb | 26 | ||||
-rw-r--r-- | spec/services/merge_requests/push_options_handler_service_spec.rb | 73 |
5 files changed, 123 insertions, 110 deletions
diff --git a/app/services/merge_requests/push_options_handler_service.rb b/app/services/merge_requests/push_options_handler_service.rb index 73b4869cc5d..610d1db0506 100644 --- a/app/services/merge_requests/push_options_handler_service.rb +++ b/app/services/merge_requests/push_options_handler_service.rb @@ -2,39 +2,28 @@ module MergeRequests class PushOptionsHandlerService - Error = Class.new(StandardError) - LIMIT = 10 attr_reader :branches, :changes_by_branch, :current_user, :errors, - :project, :push_options + :project, :push_options, :target_project def initialize(project, current_user, changes, push_options) @project = project + @target_project = @project.default_merge_request_target @current_user = current_user - @changes_by_branch = parse_changes(changes) + @branches = get_branches(changes) @push_options = push_options @errors = [] - @branches = @changes_by_branch.keys - - raise Error, 'User is required' if @current_user.nil? - - unless @project.merge_requests_enabled? - raise Error, 'Merge requests are not enabled for project' - end - - if @branches.size > LIMIT - raise Error, "Too many branches pushed (#{@branches.size} were pushed, limit is #{LIMIT})" - end - - if @push_options[:target] && !@project.repository.branch_exists?(@push_options[:target]) - raise Error, "Branch #{@push_options[:target]} does not exist" - end end def execute + validate_service + return self if errors.present? + branches.each do |branch| execute_for_branch(branch) + rescue Gitlab::Access::AccessDeniedError + errors << 'User access was denied' end self @@ -42,32 +31,47 @@ module MergeRequests private - # Parses changes in the push. - # Returns a hash of branch => changes_list - def parse_changes(raw_changes) - Gitlab::ChangesList.new(raw_changes).each_with_object({}) do |change, changes| - next unless Gitlab::Git.branch_ref?(change[:ref]) + def get_branches(raw_changes) + Gitlab::ChangesList.new(raw_changes).map do |changes| + next unless Gitlab::Git.branch_ref?(changes[:ref]) # Deleted branch - next if Gitlab::Git.blank_ref?(change[:newrev]) + next if Gitlab::Git.blank_ref?(changes[:newrev]) # Default branch - branch_name = Gitlab::Git.branch_name(change[:ref]) - next if branch_name == project.default_branch + branch_name = Gitlab::Git.branch_name(changes[:ref]) + next if branch_name == target_project.default_branch + + branch_name + end.compact.uniq + end - changes[branch_name] = change + def validate_service + errors << 'User is required' if current_user.nil? + + unless target_project.merge_requests_enabled? + errors << "Merge requests are not enabled for project #{target_project.full_path}" + end + + if branches.size > LIMIT + errors << "Too many branches pushed (#{branches.size} were pushed, limit is #{LIMIT})" + end + + if push_options[:target] && !target_project.repository.branch_exists?(push_options[:target]) + errors << "Branch #{push_options[:target]} does not exist" end end + # Returns a Hash of branch => MergeRequest def merge_requests - @merge_requests ||= MergeRequest.from_project(@project) + @merge_requests ||= MergeRequest.from_project(target_project) .opened - .from_source_branches(@branches) - .to_a # fetch now + .from_source_branches(branches) + .index_by(&:source_branch) end def execute_for_branch(branch) - merge_request = merge_requests.find { |mr| mr.source_branch == branch } + merge_request = merge_requests[branch] if merge_request update!(merge_request) @@ -82,18 +86,27 @@ module MergeRequests return end - merge_request = ::MergeRequests::CreateService.new( + # Use BuildService to assign the standard attributes of a merge request + merge_request = ::MergeRequests::BuildService.new( project, current_user, create_params(branch) ).execute + unless merge_request.errors.present? + merge_request = ::MergeRequests::CreateService.new( + project, + current_user, + merge_request.attributes + ).execute + end + collect_errors_from_merge_request(merge_request) unless merge_request.persisted? end def update!(merge_request) merge_request = ::MergeRequests::UpdateService.new( - project, + target_project, current_user, update_params ).execute(merge_request) @@ -102,18 +115,12 @@ module MergeRequests end def create_params(branch) - change = changes_by_branch.fetch(branch) - - commits = project.repository.commits_between(project.default_branch, change[:newrev]) - commits = CommitCollection.new(project, commits) - commit = commits.without_merge_commits.first - params = { assignee: current_user, source_branch: branch, - target_branch: push_options[:target] || project.default_branch, - title: commit&.title&.strip || 'New Merge Request', - description: commit&.description&.strip + source_project: project, + target_branch: push_options[:target] || target_project.default_branch, + target_project: target_project } if push_options.key?(:merge_when_pipeline_succeeds) @@ -144,7 +151,9 @@ module MergeRequests end def collect_errors_from_merge_request(merge_request) - errors << merge_request.errors.full_messages.to_sentence + merge_request.errors.full_messages.each do |error| + errors << error + end end end end diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index 5014ba51b94..71c30ec99a5 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -43,6 +43,23 @@ module API ::MergeRequests::GetUrlsService.new(project).execute(params[:changes]) end + def process_mr_push_options(push_options, project, user, changes) + output = {} + + service = ::MergeRequests::PushOptionsHandlerService.new( + project, + user, + changes, + push_options + ).execute + + if service.errors.present? + output[:warnings] = push_options_warning(service.errors.join("\n\n")) + end + + output + end + def push_options_warning(warning) options = Array.wrap(params[:push_options]).map { |p| "'#{p}'" }.join(' ') "Error encountered with push options #{options}: #{warning}" diff --git a/lib/api/internal.rb b/lib/api/internal.rb index ee1fb465608..224aaaaf006 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -264,24 +264,8 @@ module API PostReceive.perform_async(params[:gl_repository], params[:identifier], params[:changes], push_options.as_json) - if (mr_options = push_options.get(:merge_request)) - begin - service = ::MergeRequests::PushOptionsHandlerService.new( - project, - user, - params[:changes], - mr_options - ).execute - - if service.errors.present? - output[:warnings] = push_options_warning(service.errors.join("\n\n")) - end - rescue ::MergeRequests::PushOptionsHandlerService::Error => e - output[:warnings] = push_options_warning(e.message) - rescue Gitlab::Access::AccessDeniedError - output[:warnings] = push_options_warning('User access was denied') - end - end + mr_options = push_options.get(:merge_request) + output.merge!(process_mr_push_options(mr_options, project, user, params[:changes])) if mr_options.present? broadcast_message = BroadcastMessage.current&.last&.message reference_counter_decreased = Gitlab::ReferenceCounter.new(params[:gl_repository]).decrease diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index a5a6e08e73b..26645ff0a44 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -888,8 +888,10 @@ describe API::Internal do } end + let(:branch_name) { 'feature' } + let(:changes) do - "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch" + "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{branch_name}" end let(:push_options) do @@ -924,8 +926,8 @@ describe API::Internal do post api('/internal/post_receive'), params: valid_params expect(json_response['merge_request_urls']).to match [{ - "branch_name" => "new_branch", - "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch", + "branch_name" => branch_name, + "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name}", "new_merge_request" => true }] end @@ -955,26 +957,22 @@ describe API::Internal do post api('/internal/post_receive'), params: valid_params end + it 'creates a new merge request' do + expect do + post api('/internal/post_receive'), params: valid_params + end.to change { MergeRequest.count }.by(1) + end + it 'links to the newly created merge request' do post api('/internal/post_receive'), params: valid_params expect(json_response['merge_request_urls']).to match [{ - 'branch_name' => 'new_branch', + 'branch_name' => branch_name, 'url' => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/1", 'new_merge_request' => false }] end - it 'adds errors raised from MergeRequests::PushOptionsHandlerService to warnings' do - expect(MergeRequests::PushOptionsHandlerService).to receive(:new).and_raise( - MergeRequests::PushOptionsHandlerService::Error, 'my warning' - ) - - post api('/internal/post_receive'), params: valid_params - - expect(json_response['warnings']).to eq('Error encountered with push options \'merge_request.create\': my warning') - end - it 'adds errors on the service instance to warnings' do expect_any_instance_of( MergeRequests::PushOptionsHandlerService diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index eafe7a34617..af52feef24d 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -3,10 +3,13 @@ require 'spec_helper' describe MergeRequests::PushOptionsHandlerService do + include ProjectForksHelper + let(:user) { create(:user) } let(:project) { create(:project, :public, :repository) } + let(:forked_project) { fork_project(project, user, repository: true) } let(:service) { described_class.new(project, user, changes, push_options) } - let(:source_branch) { 'test' } + let(:source_branch) { 'fix' } let(:target_branch) { 'feature' } let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } @@ -38,21 +41,25 @@ describe MergeRequests::PushOptionsHandlerService do expect(last_mr.assignee).to eq(user) end - it 'sets the title and description from the first non-merge commit' do - commits = project.repository.commits('master', limit: 5) + context 'when project has been forked' do + let(:forked_project) { fork_project(project, user, repository: true) } + let(:service) { described_class.new(forked_project, user, changes, push_options) } - expect(Gitlab::Git::Commit).to receive(:between).at_least(:once).and_return(commits) + before do + allow(forked_project).to receive(:empty_repo?).and_return(false) + end - service.execute + it 'sets the correct source project' do + service.execute - merge_commit = commits.first - non_merge_commit = commits.second + expect(last_mr.source_project).to eq(forked_project) + end - expect(merge_commit.merge_commit?).to eq(true) - expect(non_merge_commit.merge_commit?).to eq(false) + it 'sets the correct target project' do + service.execute - expect(last_mr.title).to eq(non_merge_commit.title) - expect(last_mr.description).to eq(non_merge_commit.description) + expect(last_mr.target_project).to eq(project) + end end end @@ -271,7 +278,7 @@ describe MergeRequests::PushOptionsHandlerService do let(:changes) do [ new_branch_changes, - "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/second-branch" + "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/feature_conflict" ] end @@ -287,11 +294,10 @@ describe MergeRequests::PushOptionsHandlerService do end end - it 'throws an error' do - expect { service.execute }.to raise_error( - MergeRequests::PushOptionsHandlerService::Error, - "Too many branches pushed (#{limit + 1} were pushed, limit is #{limit})" - ) + it 'records an error' do + service.execute + + expect(service.errors).to eq(["Too many branches pushed (#{limit + 1} were pushed, limit is #{limit})"]) end end end @@ -308,11 +314,10 @@ describe MergeRequests::PushOptionsHandlerService do let(:push_options) { { create: true } } let(:changes) { new_branch_changes } - it 'throws an error' do - expect { service.execute }.to raise_error( - MergeRequests::PushOptionsHandlerService::Error, - 'User is required' - ) + it 'records an error' do + service.execute + + expect(service.errors).to eq(['User is required']) end end @@ -320,10 +325,12 @@ describe MergeRequests::PushOptionsHandlerService do let(:push_options) { { create: true } } let(:changes) { new_branch_changes } - it 'throws an error' do + it 'records an error' do Members::DestroyService.new(user).execute(ProjectMember.find_by!(user_id: user.id)) - expect { service.execute }.to raise_error(Gitlab::Access::AccessDeniedError) + service.execute + + expect(service.errors).to eq(['User access was denied']) end end @@ -331,11 +338,10 @@ describe MergeRequests::PushOptionsHandlerService do let(:push_options) { { create: true, target: 'my-branch' } } let(:changes) { new_branch_changes } - it 'throws an error' do - expect { service.execute }.to raise_error( - MergeRequests::PushOptionsHandlerService::Error, - 'Branch my-branch does not exist' - ) + it 'records an error' do + service.execute + + expect(service.errors).to eq(['Branch my-branch does not exist']) end end @@ -343,13 +349,12 @@ describe MergeRequests::PushOptionsHandlerService do let(:push_options) { { create: true } } let(:changes) { new_branch_changes } - it 'throws an error' do + it 'records an error' do expect(project).to receive(:merge_requests_enabled?).and_return(false) - expect { service.execute }.to raise_error( - MergeRequests::PushOptionsHandlerService::Error, - 'Merge requests are not enabled for project' - ) + service.execute + + expect(service.errors).to eq(["Merge requests are not enabled for project #{project.full_path}"]) end end |