Simplify internal post receive messages

Instead of sending varied data to Gitaly, and making Gitaly construct
various messages, build the messages first and have Gitaly print
either basic messages or alert messages, in the order they come.

Depends on https://gitlab.com/gitlab-org/gitaly/merge_requests/1410

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

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

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
-        output.merge!(
-          broadcast_message: broadcast_message,
-          reference_counter_decreased: reference_counter_decreased,
-          merge_request_urls: merge_request_urls
-        )
+        response.add_alert_message(broadcast_message)
+        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

lib/gitlab/internal_post_receive/response.rb

+# 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

spec/lib/gitlab/internal_post_receive/response_spec.rb

+# 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

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