diff options
author | Shinya Maeda <shinya@gitlab.com> | 2019-06-26 19:24:09 +0700 |
---|---|---|
committer | Shinya Maeda <shinya@gitlab.com> | 2019-07-05 13:19:30 +0700 |
commit | 587ffd11480db3ed492459ec2b6fac32a459ebaa (patch) | |
tree | f695dba44247ea39cf3cbd53b33943880cbb6659 | |
parent | 9414a41f8390511005702ab4fec99239b6c3c6dd (diff) | |
download | gitlab-ce-make-explicit-endpoint-abort-in-auto-merge-ce.tar.gz |
Split AutoMergeService interfaces into two `cancel` and `abort`make-explicit-endpoint-abort-in-auto-merge-ce
Create explicit endpoint - abort.
-rw-r--r-- | app/services/auto_merge/base_service.rb | 14 | ||||
-rw-r--r-- | app/services/auto_merge/merge_when_pipeline_succeeds_service.rb | 8 | ||||
-rw-r--r-- | app/services/auto_merge_service.rb | 6 | ||||
-rw-r--r-- | app/services/merge_requests/base_service.rb | 4 | ||||
-rw-r--r-- | app/services/merge_requests/close_service.rb | 2 | ||||
-rw-r--r-- | app/services/merge_requests/refresh_service.rb | 6 | ||||
-rw-r--r-- | app/services/merge_requests/update_service.rb | 2 | ||||
-rw-r--r-- | app/services/system_note_service.rb | 10 | ||||
-rw-r--r-- | spec/services/auto_merge/base_service_spec.rb | 40 | ||||
-rw-r--r-- | spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb | 11 | ||||
-rw-r--r-- | spec/services/auto_merge_service_spec.rb | 25 | ||||
-rw-r--r-- | spec/services/system_note_service_spec.rb | 16 |
12 files changed, 129 insertions, 15 deletions
diff --git a/app/services/auto_merge/base_service.rb b/app/services/auto_merge/base_service.rb index d726085b89a..e06659a39cd 100644 --- a/app/services/auto_merge/base_service.rb +++ b/app/services/auto_merge/base_service.rb @@ -29,7 +29,7 @@ module AutoMerge end def cancel(merge_request) - if cancel_auto_merge(merge_request) + if clear_auto_merge_parameters(merge_request) yield if block_given? success @@ -38,6 +38,16 @@ module AutoMerge end end + def abort(merge_request, reason) + if clear_auto_merge_parameters(merge_request) + yield if block_given? + + success + else + error("Can't abort the automatic merge", 406) + end + end + private def strategy @@ -46,7 +56,7 @@ module AutoMerge end end - def cancel_auto_merge(merge_request) + def clear_auto_merge_parameters(merge_request) merge_request.auto_merge_enabled = false merge_request.merge_user = nil diff --git a/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb b/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb index cde8c19e8fc..6a33ec071db 100644 --- a/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb +++ b/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb @@ -19,7 +19,13 @@ module AutoMerge def cancel(merge_request) super do - SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, @project, @current_user) + SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, project, current_user) + end + end + + def abort(merge_request, reason) + super do + SystemNoteService.abort_merge_when_pipeline_succeeds(merge_request, project, current_user, reason) end end diff --git a/app/services/auto_merge_service.rb b/app/services/auto_merge_service.rb index 926d2f5fc66..95bf2db2018 100644 --- a/app/services/auto_merge_service.rb +++ b/app/services/auto_merge_service.rb @@ -42,6 +42,12 @@ class AutoMergeService < BaseService get_service_instance(merge_request.auto_merge_strategy).cancel(merge_request) end + def abort(merge_request, reason) + return error("Can't abort the automatic merge", 406) unless merge_request.auto_merge_enabled? + + get_service_instance(merge_request.auto_merge_strategy).abort(merge_request, reason) + end + def available_strategies(merge_request) self.class.all_strategies.select do |strategy| get_service_instance(strategy).available_for?(merge_request) diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index c34fbeb2adb..067510a8a0a 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -68,8 +68,8 @@ module MergeRequests !merge_request.for_fork? end - def cancel_auto_merge(merge_request) - AutoMergeService.new(project, current_user).cancel(merge_request) + def abort_auto_merge(merge_request, reason) + AutoMergeService.new(project, current_user).abort(merge_request, reason) end # Returns all origin and fork merge requests from `@project` satisfying passed arguments. diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index b81a4dd81d2..c2174d2a130 100644 --- a/app/services/merge_requests/close_service.rb +++ b/app/services/merge_requests/close_service.rb @@ -18,7 +18,7 @@ module MergeRequests invalidate_cache_counts(merge_request, users: merge_request.assignees) merge_request.update_project_counter_caches cleanup_environments(merge_request) - cancel_auto_merge(merge_request) + abort_auto_merge(merge_request, 'merge request was closed') end merge_request diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 4b199bd8fa8..8961d2e1023 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -24,7 +24,7 @@ module MergeRequests reload_merge_requests outdate_suggestions refresh_pipelines_on_merge_requests - cancel_auto_merges + abort_auto_merges mark_pending_todos_done cache_merge_requests_closing_issues @@ -142,9 +142,9 @@ module MergeRequests end end - def cancel_auto_merges + def abort_auto_merges merge_requests_for_source_branch.each do |merge_request| - cancel_auto_merge(merge_request) + abort_auto_merge(merge_request, 'source branch was updated') end end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 0066cd0491f..d361e96babf 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -44,7 +44,7 @@ module MergeRequests merge_request.previous_changes['target_branch'].first, merge_request.target_branch) - cancel_auto_merge(merge_request) + abort_auto_merge(merge_request, 'target branch was changed') end if merge_request.assignees != old_assignees diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 4783417ad6d..005050ad08b 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -234,6 +234,16 @@ module SystemNoteService create_note(NoteSummary.new(noteable, project, author, body, action: 'merge')) end + # Called when 'merge when pipeline succeeds' is aborted + def abort_merge_when_pipeline_succeeds(noteable, project, author, reason) + body = "aborted the automatic merge because #{reason}" + + ## + # TODO: Abort message should be sent by the system, not a particular user. + # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63187. + create_note(NoteSummary.new(noteable, project, author, body, action: 'merge')) + end + def handle_merge_request_wip(noteable, project, author) prefix = noteable.work_in_progress? ? "marked" : "unmarked" diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb index 24cb63a0d61..a409f21a7e4 100644 --- a/spec/services/auto_merge/base_service_spec.rb +++ b/spec/services/auto_merge/base_service_spec.rb @@ -121,11 +121,7 @@ describe AutoMerge::BaseService do end end - describe '#cancel' do - subject { service.cancel(merge_request) } - - let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } - + shared_examples_for 'Canceled or Dropped' do it 'removes properies from the merge request' do subject @@ -173,6 +169,20 @@ describe AutoMerge::BaseService do it 'does not yield block' do expect { |b| service.execute(merge_request, &b) }.not_to yield_control end + end + end + + describe '#cancel' do + subject { service.cancel(merge_request) } + + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + + it_behaves_like 'Canceled or Dropped' + + context 'when failed to save' do + before do + allow(merge_request).to receive(:save) { false } + end it 'returns error status' do expect(subject[:status]).to eq(:error) @@ -180,4 +190,24 @@ describe AutoMerge::BaseService do end end end + + describe '#abort' do + subject { service.abort(merge_request, reason) } + + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + let(:reason) { 'an error'} + + it_behaves_like 'Canceled or Dropped' + + context 'when failed to save' do + before do + allow(merge_request).to receive(:save) { false } + end + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq("Can't abort the automatic merge") + end + end + end end diff --git a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb index 5e84ef052ce..931b52470c4 100644 --- a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb @@ -177,6 +177,17 @@ describe AutoMerge::MergeWhenPipelineSucceedsService do end end + describe "#abort" do + before do + service.abort(mr_merge_if_green_enabled, 'an error') + end + + it 'posts a system note' do + note = mr_merge_if_green_enabled.notes.last + expect(note.note).to include 'aborted the automatic merge' + end + end + describe 'pipeline integration' do context 'when there are multiple stages in the pipeline' do let(:ref) { mr_merge_if_green_enabled.source_branch } diff --git a/spec/services/auto_merge_service_spec.rb b/spec/services/auto_merge_service_spec.rb index 93a22e60498..50dfc49a59c 100644 --- a/spec/services/auto_merge_service_spec.rb +++ b/spec/services/auto_merge_service_spec.rb @@ -161,4 +161,29 @@ describe AutoMergeService do end end end + + describe '#abort' do + subject { service.abort(merge_request, error) } + + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + let(:error) { 'an error' } + + it 'delegates to a relevant service instance' do + expect_next_instance_of(AutoMerge::MergeWhenPipelineSucceedsService) do |service| + expect(service).to receive(:abort).with(merge_request, error) + end + + subject + end + + context 'when auto merge is not enabled' do + let(:merge_request) { create(:merge_request) } + + it 'returns error' do + expect(subject[:message]).to eq("Can't abort the automatic merge") + expect(subject[:status]).to eq(:error) + expect(subject[:http_status]).to eq(406) + end + end + end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 9f60e49290e..2d33e1c37eb 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -359,6 +359,22 @@ describe SystemNoteService do end end + describe '.abort_merge_when_pipeline_succeeds' do + let(:noteable) do + create(:merge_request, source_project: project, target_project: project) + end + + subject { described_class.abort_merge_when_pipeline_succeeds(noteable, project, author, 'merge request was closed') } + + it_behaves_like 'a system note' do + let(:action) { 'merge' } + end + + it "posts the 'merge when pipeline succeeds' system note" do + expect(subject.note).to eq "aborted the automatic merge because merge request was closed" + end + end + describe '.change_title' do let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum') } |