diff options
author | Grzegorz Bizon <grzegorz@gitlab.com> | 2019-08-29 07:02:22 +0000 |
---|---|---|
committer | Grzegorz Bizon <grzegorz@gitlab.com> | 2019-08-29 07:02:22 +0000 |
commit | 42484f55c1f3a09f4b01b2b432afd7b314456480 (patch) | |
tree | 0de66aa0f5fac06e454e1c7fadfd063f0de401b0 | |
parent | dc3adc3a2a2b987f7fe012d814c10bc1e292d274 (diff) | |
parent | 50342028aa9eecaa0b9bbf7e4161b2a5eb90dcfa (diff) | |
download | gitlab-ce-42484f55c1f3a09f4b01b2b432afd7b314456480.tar.gz |
Merge branch 'mk/simplify-internal-post-receive-messages' into 'master'
Simplify internal post receive messages
Closes #59808
See merge request gitlab-org/gitlab-ce!31640
-rw-r--r-- | lib/api/entities.rb | 12 | ||||
-rw-r--r-- | lib/api/helpers/internal_helpers.rb | 8 | ||||
-rw-r--r-- | lib/api/internal.rb | 23 | ||||
-rw-r--r-- | lib/gitlab/internal_post_receive/response.rb | 51 | ||||
-rw-r--r-- | spec/lib/gitlab/internal_post_receive/response_spec.rb | 121 | ||||
-rw-r--r-- | spec/requests/api/internal_spec.rb | 64 |
6 files changed, 238 insertions, 41 deletions
diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 6f86adf6a5c..88be76d3c78 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1705,6 +1705,18 @@ module API class ClusterGroup < Cluster expose :group, using: Entities::BasicGroupDetails end + + module InternalPostReceive + class Message < Grape::Entity + expose :message + expose :type + end + + class Response < Grape::Entity + expose :messages, using: Message + expose :reference_counter_decreased + end + end end end diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index 9afe6c5b027..6b438235258 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -44,8 +44,6 @@ module API end def process_mr_push_options(push_options, project, user, changes) - output = {} - Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/61359') service = ::MergeRequests::PushOptionsHandlerService.new( @@ -56,15 +54,13 @@ module API ).execute if service.errors.present? - output[:warnings] = push_options_warning(service.errors.join("\n\n")) + 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}" + "WARNINGS:\nError encountered with push options #{options}: #{warning}" end def redis_ping diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 224aaaaf006..088ea5bd79a 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -256,25 +256,26 @@ module API post '/post_receive' do status 200 - output = {} # Messages to gitlab-shell + response = Gitlab::InternalPostReceive::Response.new user = identify(params[:identifier]) project = Gitlab::GlRepository.parse(params[:gl_repository]).first push_options = Gitlab::PushOptions.new(params[:push_options]) + response.reference_counter_decreased = Gitlab::ReferenceCounter.new(params[:gl_repository]).decrease + PostReceive.perform_async(params[:gl_repository], params[:identifier], params[:changes], push_options.as_json) mr_options = push_options.get(:merge_request) - output.merge!(process_mr_push_options(mr_options, project, user, params[:changes])) if mr_options.present? + if mr_options.present? + message = process_mr_push_options(mr_options, project, user, params[:changes]) + response.add_alert_message(message) + end broadcast_message = BroadcastMessage.current&.last&.message - reference_counter_decreased = Gitlab::ReferenceCounter.new(params[:gl_repository]).decrease + response.add_alert_message(broadcast_message) - output.merge!( - broadcast_message: broadcast_message, - reference_counter_decreased: reference_counter_decreased, - merge_request_urls: merge_request_urls - ) + response.add_merge_request_urls(merge_request_urls) # A user is not guaranteed to be returned; an orphaned write deploy # key could be used @@ -282,11 +283,11 @@ module API redirect_message = Gitlab::Checks::ProjectMoved.fetch_message(user.id, project.id) project_created_message = Gitlab::Checks::ProjectCreated.fetch_message(user.id, project.id) - output[:redirected_message] = redirect_message if redirect_message - output[:project_created_message] = project_created_message if project_created_message + response.add_basic_message(redirect_message) + response.add_basic_message(project_created_message) end - output + present response, with: Entities::InternalPostReceive::Response end end end diff --git a/lib/gitlab/internal_post_receive/response.rb b/lib/gitlab/internal_post_receive/response.rb new file mode 100644 index 00000000000..7e7ec2aa45c --- /dev/null +++ b/lib/gitlab/internal_post_receive/response.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module Gitlab + module InternalPostReceive + class Response + attr_accessor :reference_counter_decreased + attr_reader :messages + + Message = Struct.new(:message, :type) do + def self.basic(text) + new(text, :basic) + end + + def self.alert(text) + new(text, :alert) + end + end + + def initialize + @messages = [] + @reference_counter_decreased = false + end + + def add_merge_request_urls(urls_data) + urls_data.each do |url_data| + add_merge_request_url(url_data) + end + end + + def add_merge_request_url(url_data) + message = if url_data[:new_merge_request] + "To create a merge request for #{url_data[:branch_name]}, visit:" + else + "View merge request for #{url_data[:branch_name]}:" + end + + message += "\n #{url_data[:url]}" + + add_basic_message(message) + end + + def add_basic_message(text) + @messages << Message.basic(text) if text.present? + end + + def add_alert_message(text) + @messages.unshift(Message.alert(text)) if text.present? + end + end + end +end diff --git a/spec/lib/gitlab/internal_post_receive/response_spec.rb b/spec/lib/gitlab/internal_post_receive/response_spec.rb new file mode 100644 index 00000000000..f43762c9248 --- /dev/null +++ b/spec/lib/gitlab/internal_post_receive/response_spec.rb @@ -0,0 +1,121 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::InternalPostReceive::Response do + subject { described_class.new } + + describe '#add_merge_request_urls' do + context 'when there are urls_data' do + it 'adds a message for each merge request URL' do + urls_data = [ + { new_merge_request: false, branch_name: 'foo', url: 'http://example.com/foo/bar/merge_requests/1' }, + { new_merge_request: true, branch_name: 'bar', url: 'http://example.com/foo/bar/merge_requests/new?merge_request%5Bsource_branch%5D=bar' } + ] + + subject.add_merge_request_urls(urls_data) + + expected = [a_kind_of(described_class::Message), a_kind_of(described_class::Message)] + expect(subject.messages).to match(expected) + end + end + end + + describe '#add_merge_request_url' do + context 'when :new_merge_request is false' do + it 'adds a basic message to view the existing merge request' do + url_data = { new_merge_request: false, branch_name: 'foo', url: 'http://example.com/foo/bar/merge_requests/1' } + + subject.add_merge_request_url(url_data) + + message = <<~MESSAGE.strip + View merge request for foo: + http://example.com/foo/bar/merge_requests/1 + MESSAGE + + expect(subject.messages.first.message).to eq(message) + expect(subject.messages.first.type).to eq(:basic) + end + end + + context 'when :new_merge_request is true' do + it 'adds a basic message to create a new merge request' do + url_data = { new_merge_request: true, branch_name: 'bar', url: 'http://example.com/foo/bar/merge_requests/new?merge_request%5Bsource_branch%5D=bar' } + + subject.add_merge_request_url(url_data) + + message = <<~MESSAGE.strip + To create a merge request for bar, visit: + http://example.com/foo/bar/merge_requests/new?merge_request%5Bsource_branch%5D=bar + MESSAGE + + expect(subject.messages.first.message).to eq(message) + expect(subject.messages.first.type).to eq(:basic) + end + end + end + + describe '#add_basic_message' do + context 'when text is present' do + it 'adds a basic message' do + subject.add_basic_message('hello') + + expect(subject.messages.first.message).to eq('hello') + expect(subject.messages.first.type).to eq(:basic) + end + end + + context 'when text is blank' do + it 'does not add a message' do + subject.add_basic_message(' ') + + expect(subject.messages).to be_blank + end + end + end + + describe '#add_alert_message' do + context 'when text is present' do + it 'adds a alert message' do + subject.add_alert_message('hello') + + expect(subject.messages.first.message).to eq('hello') + expect(subject.messages.first.type).to eq(:alert) + end + end + + context 'when text is blank' do + it 'does not add a message' do + subject.add_alert_message(' ') + + expect(subject.messages).to be_blank + end + end + end + + describe '#reference_counter_decreased' do + context 'initially' do + it 'reference_counter_decreased is set to false' do + expect(subject.reference_counter_decreased).to eq(false) + end + end + end + + describe '#reference_counter_decreased=' do + context 'when the argument is truthy' do + it 'reference_counter_decreased is truthy' do + subject.reference_counter_decreased = true + + expect(subject.reference_counter_decreased).to be_truthy + end + end + + context 'when the argument is falsey' do + it 'reference_counter_decreased is falsey' do + subject.reference_counter_decreased = false + + expect(subject.reference_counter_decreased).to be_falsey + end + end + end +end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 3ab1818bebb..c94f6d22e74 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -925,19 +925,20 @@ describe API::Internal do it 'returns link to create new merge request' do post api('/internal/post_receive'), params: valid_params - expect(json_response['merge_request_urls']).to match [{ - "branch_name" => branch_name, - "url" => "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name}", - "new_merge_request" => true - }] + message = <<~MESSAGE.strip + To create a merge request for #{branch_name}, visit: + http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name} + MESSAGE + + expect(json_response['messages']).to include(build_basic_message(message)) end - it 'returns empty array if printing_merge_request_link_enabled is false' do + it 'returns no merge request messages if printing_merge_request_link_enabled is false' do project.update!(printing_merge_request_link_enabled: false) post api('/internal/post_receive'), params: valid_params - expect(json_response['merge_request_urls']).to eq([]) + expect(json_response['messages']).to be_blank end it 'does not invoke MergeRequests::PushOptionsHandlerService' do @@ -968,11 +969,12 @@ describe API::Internal do 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' => branch_name, - 'url' => "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/1", - 'new_merge_request' => false - }] + message = <<~MESSAGE.strip + View merge request for #{branch_name}: + http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/1 + MESSAGE + + expect(json_response['messages']).to include(build_basic_message(message)) end it 'adds errors on the service instance to warnings' do @@ -982,7 +984,8 @@ describe API::Internal do post api('/internal/post_receive'), params: valid_params - expect(json_response['warnings']).to eq('Error encountered with push options \'merge_request.create\': my error') + message = "WARNINGS:\nError encountered with push options 'merge_request.create': my error" + expect(json_response['messages']).to include(build_alert_message(message)) end it 'adds ActiveRecord errors on invalid MergeRequest records to warnings' do @@ -995,38 +998,39 @@ describe API::Internal do post api('/internal/post_receive'), params: valid_params - expect(json_response['warnings']).to eq('Error encountered with push options \'merge_request.create\': my error') + message = "WARNINGS:\nError encountered with push options 'merge_request.create': my error" + expect(json_response['messages']).to include(build_alert_message(message)) end end context 'broadcast message exists' do let!(:broadcast_message) { create(:broadcast_message, starts_at: 1.day.ago, ends_at: 1.day.from_now ) } - it 'returns one broadcast message' do + it 'outputs a broadcast message' do post api('/internal/post_receive'), params: valid_params expect(response).to have_gitlab_http_status(200) - expect(json_response['broadcast_message']).to eq(broadcast_message.message) + expect(json_response['messages']).to include(build_alert_message(broadcast_message.message)) end end context 'broadcast message does not exist' do - it 'returns empty string' do + it 'does not output a broadcast message' do post api('/internal/post_receive'), params: valid_params expect(response).to have_gitlab_http_status(200) - expect(json_response['broadcast_message']).to eq(nil) + expect(has_alert_messages?(json_response['messages'])).to be_falsey end end context 'nil broadcast message' do - it 'returns empty string' do + it 'does not output a broadcast message' do allow(BroadcastMessage).to receive(:current).and_return(nil) post api('/internal/post_receive'), params: valid_params expect(response).to have_gitlab_http_status(200) - expect(json_response['broadcast_message']).to eq(nil) + expect(has_alert_messages?(json_response['messages'])).to be_falsey end end @@ -1038,8 +1042,7 @@ describe API::Internal do post api('/internal/post_receive'), params: valid_params expect(response).to have_gitlab_http_status(200) - expect(json_response["redirected_message"]).to be_present - expect(json_response["redirected_message"]).to eq(project_moved.message) + expect(json_response['messages']).to include(build_basic_message(project_moved.message)) end end @@ -1051,8 +1054,7 @@ describe API::Internal do post api('/internal/post_receive'), params: valid_params expect(response).to have_gitlab_http_status(200) - expect(json_response["project_created_message"]).to be_present - expect(json_response["project_created_message"]).to eq(project_created.message) + expect(json_response['messages']).to include(build_basic_message(project_created.message)) end end @@ -1172,4 +1174,18 @@ describe API::Internal do } ) end + + def build_alert_message(message) + { 'type' => 'alert', 'message' => message } + end + + def build_basic_message(message) + { 'type' => 'basic', 'message' => message } + end + + def has_alert_messages?(messages) + messages.any? do |message| + message['type'] == 'alert' + end + end end |