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

5 files changed, 48 insertions, 23 deletions

diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 629c131aee6..e46d1995498 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/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
diff --git a/spec/fixtures/api/schemas/entities/merge_request.json b/spec/fixtures/api/schemas/entities/merge_request.json
index 0796d9b8af9..30b4e56bc98 100644
--- a/spec/fixtures/api/schemas/entities/merge_request.json
+++ b/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" },
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index d80d5657c42..188a0a98ec3 100644
--- a/spec/models/merge_request_spec.rb
+++ b/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
diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb
index 2187be0190d..5e114434a67 100644
--- a/spec/presenters/merge_request_presenter_spec.rb
+++ b/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 }
diff --git a/spec/serializers/merge_request_entity_spec.rb b/spec/serializers/merge_request_entity_spec.rb
index 4288955ddbc..4aeb593da44 100644
--- a/spec/serializers/merge_request_entity_spec.rb
+++ b/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)