Remove n+1 query for pipeline status when rendering merge requests listissue_27168

9 files changed, 81 insertions, 5 deletions

diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb
index 85ae4985e58..31ba372e413 100644
--- a/app/controllers/concerns/issuable_collections.rb
+++ b/app/controllers/concerns/issuable_collections.rb
@@ -39,6 +39,14 @@ module IssuableCollections
     end
   end
 
+  def pipelines_for_collection(merge_requests)
+    pipelines = MergeRequest.pipelines_status_for_collection(merge_requests)
+
+    pipelines.each_with_object({}) do |pipeline, pipelines_per_branch|
+      pipelines_per_branch[pipeline.ref] = pipeline
+    end
+  end
+
   def issues_collection
     issues_finder.execute.preload(:project, :author, :assignee, :labels, :milestone, project: :namespace)
   end
diff --git a/app/controllers/concerns/merge_requests_action.rb b/app/controllers/concerns/merge_requests_action.rb
index d3c8e4888bc..9f7941ff0b7 100644
--- a/app/controllers/concerns/merge_requests_action.rb
+++ b/app/controllers/concerns/merge_requests_action.rb
@@ -10,6 +10,7 @@ module MergeRequestsAction
 
     @collection_type    = "MergeRequest"
     @issuable_meta_data = issuable_meta_data(@merge_requests, @collection_type)
+    @pipeline_statuses  = pipelines_for_collection(@merge_requests)
   end
 
   private
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 82f9b6e06db..83d24f4e6fe 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -40,6 +40,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @merge_requests     = merge_requests_collection
     @merge_requests     = @merge_requests.page(params[:page])
     @issuable_meta_data = issuable_meta_data(@merge_requests, @collection_type)
+    @pipeline_statuses  = pipelines_for_collection(@merge_requests)
 
     if @merge_requests.out_of_range? && @merge_requests.total_pages != 0
       return redirect_to url_for(params.merge(page: @merge_requests.total_pages))
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 0f7b8311588..d653b59db56 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -175,6 +175,15 @@ class MergeRequest < ActiveRecord::Base
     work_in_progress?(title) ? title : "WIP: #{title}"
   end
 
+  def self.pipelines_status_for_collection(merge_requests)
+    refs = merge_requests.pluck(:source_branch)
+
+    Ci::Pipeline.
+      where(
+        id: Ci::Pipeline.select("MAX(id)").where(ref: refs).group(:ref)
+      )
+  end
+
   # `from` argument can be a Namespace or Project.
   def to_reference(from = nil, full: false)
     reference = "#{self.class.reference_prefix}#{iid}"
diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml
index 11b7aaec704..ec8515b093b 100644
--- a/app/views/projects/merge_requests/_merge_request.html.haml
+++ b/app/views/projects/merge_requests/_merge_request.html.haml
@@ -16,9 +16,11 @@
             = icon('ban')
             CLOSED
 
-        - if merge_request.head_pipeline
+        - pipeline = @pipeline_statuses[merge_request.source_branch]
+
+        - if pipeline
           %li
-            = render_pipeline_status(merge_request.head_pipeline)
+            = render_pipeline_status(pipeline)
 
         - if merge_request.open? && merge_request.broken?
           %li
diff --git a/changelogs/unreleased/issue_27168.yml b/changelogs/unreleased/issue_27168.yml
new file mode 100644
index 00000000000..2f83d37eb86
--- /dev/null
+++ b/changelogs/unreleased/issue_27168.yml
@@ -0,0 +1,4 @@
+---
+title: Remove n+1 query for pipeline status when rendering merge requests list
+merge_request:
+author:
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 250d64f7055..912774ad84b 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -160,6 +160,25 @@ describe Projects::MergeRequestsController do
 
     it_behaves_like "issuables list meta-data", :merge_request
 
+    it 'loads last pipeline for each merge request source branch' do
+      status = %w(running failed success)
+
+      2.times do |i|
+        branch = "branch_#{i}"
+        create(:ci_empty_pipeline, project: project, ref: branch, status: status.sample, sha: project.commit.id)
+        create(:ci_empty_pipeline, project: project, ref: branch, status: status.sample, sha: project.commit.id)
+        create(:merge_request_with_diffs, target_project: project, source_project: project, source_branch: branch)
+      end
+      create(:merge_request_with_diffs, target_project: project, source_project: project, source_branch: "branch_1", target_branch: "not_master")
+
+      get_merge_requests
+
+      pipelines = assigns(:pipeline_statuses)
+      expect(pipelines.count).to eq(2)
+      expect(pipelines["branch_0"]).to eq(Ci::Pipeline.where(ref: "branch_0").last)
+      expect(pipelines["branch_1"]).to eq(Ci::Pipeline.where(ref: "branch_1").last)
+    end
+
     context 'when page param' do
       let(:last_page) { project.merge_requests.page().total_pages }
       let!(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
diff --git a/spec/features/issuables/issuable_list_spec.rb b/spec/features/issuables/issuable_list_spec.rb
index b90bf6268fd..22e9de48784 100644
--- a/spec/features/issuables/issuable_list_spec.rb
+++ b/spec/features/issuables/issuable_list_spec.rb
@@ -1,7 +1,7 @@
 require 'rails_helper'
 
 describe 'issuable list', feature: true do
-  let(:project) { create(:empty_project) }
+  let(:project) { create(:project) }
   let(:user)    { create(:user) }
 
   issuable_types = [:issue, :merge_request]
@@ -17,7 +17,6 @@ describe 'issuable list', feature: true do
       control_count = ActiveRecord::QueryRecorder.new { visit_issuable_list(issuable_type) }.count
 
       create_issuables(issuable_type)
-
       expect { visit_issuable_list(issuable_type) }.not_to exceed_query_limit(control_count)
     end
 
@@ -51,7 +50,9 @@ describe 'issuable list', feature: true do
         if issuable_type == :issue
           create(:issue, project: project, author: user)
         else
-          create(:merge_request, title: FFaker::Lorem.sentence, source_project: project, source_branch: FFaker::Name.name)
+          source_branch = FFaker::Name.name
+          create(:ci_empty_pipeline, project: project, ref: source_branch, status: %w(running failed success).sample, sha: project.commit.id)
+          create(:merge_request, title: FFaker::Lorem.sentence, source_project: project, source_branch: source_branch)
         end
 
       2.times do
diff --git a/spec/features/merge_requests/index_pipeline_statuses_spec.rb b/spec/features/merge_requests/index_pipeline_statuses_spec.rb
new file mode 100644
index 00000000000..010bca70b4d
--- /dev/null
+++ b/spec/features/merge_requests/index_pipeline_statuses_spec.rb
@@ -0,0 +1,31 @@
+require 'rails_helper'
+
+feature 'Pipeline status icon on merge requests index', feature: true do
+  let(:project)           { create(:project, :public) }
+  let!(:user)             { create(:user) }
+  let(:status_per_branch) { { branch_1: 'running', branch_2: 'failed', branch_3: 'success' } }
+
+  before do
+    status_per_branch.each do |branch, status|
+      create(:ci_empty_pipeline, project: project, ref: branch, status: status, sha: project.commit.id)
+      create(:merge_request, title: FFaker::Lorem.sentence, source_project: project, source_branch: branch)
+    end
+
+    create(:merge_request, title: FFaker::Lorem.sentence, source_project: project, source_branch: 'branch_1', target_branch: 'not_master')
+
+    project.add_master(user)
+    login_as(user)
+
+    visit namespace_project_merge_requests_path(project.namespace, project)
+  end
+
+  it 'shows pipeline status for each merge request' do
+    merge_requests = project.merge_requests
+
+    merge_requests.each do |mr|
+      page.find("#merge_request_#{mr.id}") do |item|
+        expect(item).to have_selector("ci-status-icon-#{status_per_branch[mr.source_branch]}")
+      end
+    end
+  end
+end