From 2a01b33e6ac27e84d842f3040d7adf799f7cf258 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 4 Jun 2019 14:11:03 +0700 Subject: [PATCH] Create BaseService for Auto Merge architecture It abstracts some codes for common methods in AutoMerge::*Services. --- app/models/merge_request.rb | 15 -- app/services/auto_merge/base_service.rb | 52 +++++++ .../merge_when_pipeline_succeeds_service.rb | 34 +---- spec/models/merge_request_spec.rb | 16 -- spec/services/auto_merge/base_service_spec.rb | 144 ++++++++++++++++++ 5 files changed, 201 insertions(+), 60 deletions(-) create mode 100644 app/services/auto_merge/base_service.rb create mode 100644 spec/services/auto_merge/base_service_spec.rb diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 59416fb4b51..4fcaac75655 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -984,21 +984,6 @@ class MergeRequest < ApplicationRecord end end - def reset_auto_merge - return unless auto_merge_enabled? - - self.auto_merge_enabled = false - self.merge_user = nil - if merge_params - merge_params.delete('should_remove_source_branch') - merge_params.delete('commit_message') - merge_params.delete('squash_commit_message') - merge_params.delete('auto_merge_strategy') - end - - self.save - end - # Return array of possible target branches # depends on target project of MR def target_branches diff --git a/app/services/auto_merge/base_service.rb b/app/services/auto_merge/base_service.rb new file mode 100644 index 00000000000..058105db3a4 --- /dev/null +++ b/app/services/auto_merge/base_service.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module AutoMerge + class BaseService < ::BaseService + include Gitlab::Utils::StrongMemoize + + def execute(merge_request) + merge_request.merge_params.merge!(params) + merge_request.auto_merge_enabled = true + merge_request.merge_user = current_user + merge_request.auto_merge_strategy = strategy + + return :failed unless merge_request.save + + yield if block_given? + + strategy.to_sym + end + + def cancel(merge_request) + if cancel_auto_merge(merge_request) + yield if block_given? + + success + else + error("Can't cancel the automatic merge", 406) + end + end + + private + + def strategy + strong_memoize(:strategy) do + self.class.name.demodulize.remove('Service').underscore + end + end + + def cancel_auto_merge(merge_request) + merge_request.auto_merge_enabled = false + merge_request.merge_user = nil + + merge_request.merge_params&.except!( + 'should_remove_source_branch', + 'commit_message', + 'squash_commit_message', + 'auto_merge_strategy' + ) + + merge_request.save + end + end +end 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 d0586468859..c41073a73e9 100644 --- a/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb +++ b/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb @@ -1,32 +1,12 @@ # frozen_string_literal: true module AutoMerge - class MergeWhenPipelineSucceedsService < BaseService + class MergeWhenPipelineSucceedsService < AutoMerge::BaseService def execute(merge_request) - return :failed unless merge_request.actual_head_pipeline - - if merge_request.actual_head_pipeline.active? - merge_request.merge_params.merge!(params) - - unless merge_request.auto_merge_enabled? - merge_request.auto_merge_enabled = true - merge_request.merge_user = @current_user - merge_request.auto_merge_strategy = AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS - - SystemNoteService.merge_when_pipeline_succeeds(merge_request, @project, @current_user, merge_request.diff_head_commit) + super do + if merge_request.saved_change_to_auto_merge_enabled? + SystemNoteService.merge_when_pipeline_succeeds(merge_request, project, current_user, merge_request.diff_head_commit) end - - return :failed unless merge_request.save - - :merge_when_pipeline_succeeds - elsif merge_request.actual_head_pipeline.success? - # This can be triggered when a user clicks the auto merge button while - # the tests finish at about the same time - merge_request.merge_async(current_user.id, merge_params) - - :success - else - :failed end end @@ -38,12 +18,8 @@ module AutoMerge end def cancel(merge_request) - if merge_request.reset_auto_merge + super do SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, @project, @current_user) - - success - else - error("Can't cancel the automatic merge", 406) end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 956c5675f38..fc28c216b21 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1088,22 +1088,6 @@ describe MergeRequest do end end - describe "#reset_auto_merge" do - let(:merge_if_green) do - create :merge_request, merge_when_pipeline_succeeds: true, merge_user: create(:user), - merge_params: { "should_remove_source_branch" => "1", "commit_message" => "msg" } - end - - it "sets the item to false" do - merge_if_green.reset_auto_merge - merge_if_green.reload - - expect(merge_if_green.merge_when_pipeline_succeeds).to be_falsey - expect(merge_if_green.merge_params["should_remove_source_branch"]).to be_nil - expect(merge_if_green.merge_params["commit_message"]).to be_nil - end - end - describe '#committers' do it 'returns all the committers of every commit in the merge request' do users = subject.commits.without_merge_commits.map(&:committer_email).uniq.map do |email| diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb new file mode 100644 index 00000000000..197fa16961d --- /dev/null +++ b/spec/services/auto_merge/base_service_spec.rb @@ -0,0 +1,144 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AutoMerge::BaseService do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:service) { described_class.new(project, user, params) } + let(:merge_request) { create(:merge_request) } + let(:params) { {} } + + describe '#execute' do + subject { service.execute(merge_request) } + + it 'sets properies to the merge request' do + subject + + merge_request.reload + expect(merge_request).to be_auto_merge_enabled + expect(merge_request.merge_user).to eq(user) + expect(merge_request.auto_merge_strategy).to eq('base') + end + + it 'yields block' do + expect { |b| service.execute(merge_request, &b) }.to yield_control.once + end + + it 'returns activated strategy name' do + is_expected.to eq(:base) + end + + context 'when merge parameters are given' do + let(:params) do + { + 'commit_message' => "Merge branch 'patch-12' into 'master'", + 'sha' => "200fcc9c260f7219eaf0daba87d818f0922c5b18", + 'should_remove_source_branch' => false, + 'squash' => false, + 'squash_commit_message' => "Update README.md" + } + end + + it 'sets merge parameters' do + subject + + merge_request.reload + expect(merge_request.merge_params['commit_message']).to eq("Merge branch 'patch-12' into 'master'") + expect(merge_request.merge_params['sha']).to eq('200fcc9c260f7219eaf0daba87d818f0922c5b18') + expect(merge_request.merge_params['should_remove_source_branch']).to eq(false) + expect(merge_request.merge_params['squash']).to eq(false) + expect(merge_request.merge_params['squash_commit_message']).to eq('Update README.md') + end + end + + context 'when strategy is merge when pipeline succeeds' do + let(:service) { AutoMerge::MergeWhenPipelineSucceedsService.new(project, user) } + + it 'sets the auto merge strategy' do + subject + + merge_request.reload + expect(merge_request.auto_merge_strategy).to eq(AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) + end + + it 'returns activated strategy name' do + is_expected.to eq(AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS.to_sym) + end + end + + context 'when failed to save' do + before do + allow(merge_request).to receive(:save) { false } + end + + it 'does not yield block' do + expect { |b| service.execute(merge_request, &b) }.not_to yield_control + end + + it 'returns failed' do + is_expected.to eq(:failed) + end + end + end + + describe '#cancel' do + subject { service.cancel(merge_request) } + + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + + it 'removes properies from the merge request' do + subject + + merge_request.reload + expect(merge_request).not_to be_auto_merge_enabled + expect(merge_request.merge_user).to be_nil + expect(merge_request.auto_merge_strategy).to be_nil + end + + it 'yields block' do + expect { |b| service.cancel(merge_request, &b) }.to yield_control.once + end + + it 'returns success status' do + expect(subject[:status]).to eq(:success) + end + + context 'when merge params are set' do + before do + merge_request.update!(merge_params: + { + 'should_remove_source_branch' => false, + 'commit_message' => "Merge branch 'patch-12' into 'master'", + 'squash_commit_message' => "Update README.md", + 'auto_merge_strategy' => 'merge_when_pipeline_succeeds' + }) + end + + it 'removes merge parameters' do + subject + + merge_request.reload + expect(merge_request.merge_params['should_remove_source_branch']).to be_nil + expect(merge_request.merge_params['commit_message']).to be_nil + expect(merge_request.merge_params['squash_commit_message']).to be_nil + expect(merge_request.merge_params['auto_merge_strategy']).to be_nil + end + end + + context 'when failed to save' do + before do + allow(merge_request).to receive(:save) { false } + end + + it 'does not yield block' do + expect { |b| service.execute(merge_request, &b) }.not_to yield_control + end + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq("Can't cancel the automatic merge") + end + end + end +end