Merge branch '36876-mr-show-json-controller-perf-improvements' into 'master'

Reduce method calls while evaluating Projects::MergeRequestsController#show.json

See merge request gitlab-org/gitlab-ce!14285

app/models/merge_request.rb

@@ -742,10 +742,9 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def has_ci?
-    has_ci_integration = source_project.try(:ci_service)
-    uses_gitlab_ci = all_pipelines.any?
+    return false if has_no_commits?
 
-    (has_ci_integration || uses_gitlab_ci) && commits.any?
+    !!(head_pipeline_id || all_pipelines.any? || source_project&.ci_service)
   end
 
   def branch_missing?

app/presenters/merge_request_presenter.rb

@@ -31,7 +31,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def remove_wip_path
-    if can?(current_user, :update_merge_request, merge_request.project)
+    if work_in_progress? && can?(current_user, :update_merge_request, merge_request.project)
       remove_wip_project_merge_request_path(project, merge_request)
     end
   end

app/serializers/merge_request_entity.rb

@@ -23,7 +23,6 @@ class MergeRequestEntity < IssuableEntity
   expose :closed_event, using: EventEntity
 
   # User entities
-  expose :author, using: UserEntity
   expose :merge_user, using: UserEntity
 
   # Diff sha's
@@ -31,7 +30,6 @@ class MergeRequestEntity < IssuableEntity
     merge_request.diff_head_sha if merge_request.diff_head_commit
   end
 
-  expose :merge_commit_sha
   expose :merge_commit_message
   expose :head_pipeline, with: PipelineDetailsEntity, as: :pipeline
 

app/views/projects/issues/_merge_requests.html.haml

@@ -2,13 +2,13 @@
   %h2.merge-requests-title
     = pluralize(@merge_requests.count, 'Related Merge Request')
   %ul.unstyled-list.related-merge-requests
-    - has_any_ci = @merge_requests.any?(&:head_pipeline)
+    - has_any_head_pipeline = @merge_requests.any?(&:head_pipeline_id)
     - @merge_requests.each do |merge_request|
       %li
         %span.merge-request-ci-status
           - if merge_request.head_pipeline
             = render_pipeline_status(merge_request.head_pipeline)
-          - elsif has_any_ci
+          - elsif has_any_head_pipeline
             = icon('blank fw')
         %span.merge-request-id
           = merge_request.to_reference

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -96,18 +96,6 @@ describe Projects::MergeRequestsController do
           expect(response).to match_response_schema('entities/merge_request')
         end
       end
-
-      context 'number of queries', :request_store do
-        it 'verifies number of queries' do
-          # pre-create objects
-          merge_request
-
-          recorded = ActiveRecord::QueryRecorder.new { go(format: :json) }
-
-          expect(recorded.count).to be_within(5).of(30)
-          expect(recorded.cached_count).to eq(0)
-        end
-      end
     end
 
     describe "as diff" do

spec/fixtures/api/schemas/entities/merge_request.json

@@ -93,7 +93,7 @@
     "merge_commit_message_with_description": { "type": "string" },
     "diverged_commits_count": { "type": "integer" },
     "commit_change_content_path": { "type": "string" },
-    "remove_wip_path": { "type": "string" },
+    "remove_wip_path": { "type": ["string", "null"] },
     "commits_count": { "type": "integer" },
     "remove_source_branch": { "type": ["boolean", "null"] },
     "merge_ongoing": { "type": "boolean" },

spec/models/merge_request_spec.rb

@@ -791,6 +791,49 @@ describe MergeRequest do
     end
   end
 
+  describe '#has_ci?' do
+    let(:merge_request) { build_stubbed(:merge_request) }
+
+    context 'has ci' do
+      it 'returns true if MR has head_pipeline_id and commits' do
+        allow(merge_request).to receive_message_chain(:source_project, :ci_service) { nil }
+        allow(merge_request).to receive(:head_pipeline_id) { double }
+        allow(merge_request).to receive(:has_no_commits?) { false }
+
+        expect(merge_request.has_ci?).to be(true)
+      end
+
+      it 'returns true if MR has any pipeline and commits' do
+        allow(merge_request).to receive_message_chain(:source_project, :ci_service) { nil }
+        allow(merge_request).to receive(:head_pipeline_id) { nil }
+        allow(merge_request).to receive(:has_no_commits?) { false }
+        allow(merge_request).to receive(:all_pipelines) { [double] }
+
+        expect(merge_request.has_ci?).to be(true)
+      end
+
+      it 'returns true if MR has CI service and commits' do
+        allow(merge_request).to receive_message_chain(:source_project, :ci_service) { double }
+        allow(merge_request).to receive(:head_pipeline_id) { nil }
+        allow(merge_request).to receive(:has_no_commits?) { false }
+        allow(merge_request).to receive(:all_pipelines) { [] }
+
+        expect(merge_request.has_ci?).to be(true)
+      end
+    end
+
+    context 'has no ci' do
+      it 'returns false if MR has no CI service nor pipeline, and no commits' do
+        allow(merge_request).to receive_message_chain(:source_project, :ci_service) { nil }
+        allow(merge_request).to receive(:head_pipeline_id) { nil }
+        allow(merge_request).to receive(:all_pipelines) { [] }
+        allow(merge_request).to receive(:has_no_commits?) { true }
+
+        expect(merge_request.has_ci?).to be(false)
+      end
+    end
+  end
+
   describe '#all_pipelines' do
     shared_examples 'returning pipelines with proper ordering' do
       let!(:all_pipelines) do

spec/presenters/merge_request_presenter_spec.rb

@@ -300,6 +300,10 @@ describe MergeRequestPresenter do
       described_class.new(resource, current_user: user).remove_wip_path
     end
 
+    before do
+      allow(resource).to receive(:work_in_progress?).and_return(true)
+    end
+
     context 'when merge request enabled and has permission' do
       it 'has remove_wip_path' do
         allow(project).to receive(:merge_requests_enabled?) { true }

spec/serializers/merge_request_entity_spec.rb

@@ -11,16 +11,6 @@ describe MergeRequestEntity do
     described_class.new(resource, request: request).as_json
   end
 
-  it 'includes author' do
-    req = double('request')
-
-    author_payload = UserEntity
-      .represent(resource.author, request: req)
-      .as_json
-
-    expect(subject[:author]).to eq(author_payload)
-  end
-
   it 'includes pipeline' do
     req = double('request', current_user: user)
     pipeline = build_stubbed(:ci_pipeline)