Batch load pipelines instead of statuses

Instead of preloading the status, we now preload the
latest pipelines instead for each commit. This gives us more
flexibility on how to display the status.

This also helps us avoid passing the current_user deep through
the model methods. Best to call the detailed_status in the
view/presentation layer.

We also extracted commit's pipeline behavior into its own class.
This is to better isolate and organize the pipeline related behavior
of the Commit object.

Moving the pipeline behavior into its own class resulted
to a lot of breaking changes though. So instead of always having to
create your own instance of a CommitWithPipeline, we're just gonna
use the same commit object but just delegate the pipeline related
calls to the CommitWithPipeline instance.

app/controllers/projects/commits_controller.rb

@@ -72,7 +72,7 @@ class Projects::CommitsController < Projects::ApplicationController
         @repository.commits(@ref, path: @path, limit: @limit, offset: @offset)
       end
 
-    @commits = @commits.with_pipeline_status
+    @commits = @commits.with_latest_pipeline(@ref)
     @commits = set_commits_for_rendering(@commits)
   end
 

app/controllers/projects/merge_requests_controller.rb

@@ -79,7 +79,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     # Get commits from repository
     # or from cache if already merged
     @commits =
-      set_commits_for_rendering(@merge_request.commits.with_pipeline_status)
+      set_commits_for_rendering(@merge_request.commits.with_latest_pipeline)
 
     render json: { html: view_to_html_string('projects/merge_requests/_commits') }
   end

app/helpers/ci_status_helper.rb

@@ -64,7 +64,7 @@ module CiStatusHelper
 
   def ci_icon_for_status(status, size: 16)
     if detailed_status?(status)
-      return sprite_icon(status.icon)
+      return sprite_icon(status.icon, size: size)
     end
 
     icon_name =
@@ -96,23 +96,29 @@ module CiStatusHelper
     sprite_icon(icon_name, size: size)
   end
 
+  def ci_icon_class_for_status(status)
+    group = detailed_status?(status) ? status.group : status.dasherize
+
+    "ci-status-icon-#{group}"
+  end
+
   def pipeline_status_cache_key(pipeline_status)
     "pipeline-status/#{pipeline_status.sha}-#{pipeline_status.status}"
   end
 
-  def render_commit_status(commit, ref: nil, tooltip_placement: 'left')
+  def render_commit_status(commit, status, ref: nil, tooltip_placement: 'left')
     project = commit.project
     path = pipelines_project_commit_path(project, commit, ref: ref)
 
     render_status_with_link(
-      commit.status(ref),
+      status,
       path,
       tooltip_placement: tooltip_placement,
       icon_size: 24)
   end
 
   def render_status_with_link(status, path = nil, type: _('pipeline'), tooltip_placement: 'left', cssclass: '', container: 'body', icon_size: 16)
-    klass = "ci-status-link ci-status-icon-#{status.dasherize} d-inline-flex #{cssclass}"
+    klass = "ci-status-link #{ci_icon_class_for_status(status)} d-inline-flex #{cssclass}"
     title = "#{type.titleize}: #{ci_label_for_status(status)}"
     data = { toggle: 'tooltip', placement: tooltip_placement, container: container }
 
@@ -127,6 +133,7 @@ module CiStatusHelper
 
   def detailed_status?(status)
     status.respond_to?(:text) &&
+      status.respond_to?(:group) &&
       status.respond_to?(:label) &&
       status.respond_to?(:icon)
   end

app/models/ci/pipeline.rb

@@ -279,16 +279,16 @@ module Ci
       end
     end
 
-    # Returns a Hash containing the latest pipeline status for every given
+    # Returns a Hash containing the latest pipeline for every given
     # commit.
     #
-    # The keys of this Hash are the commit SHAs, the values the statuses.
+    # The keys of this Hash are the commit SHAs, the values the pipelines.
     #
-    # commits - The list of commit SHAs to get the status for.
+    # commits - The list of commit SHAs to get the pipelines for.
     # ref - The ref to scope the data to (e.g. "master"). If the ref is not
-    #       given we simply get the latest status for the commits, regardless
-    #       of what refs their pipelines belong to.
-    def self.latest_status_per_commit(commits, ref = nil)
+    #       given we simply get the latest pipelines for the commits, regardless
+    #       of what refs the pipelines belong to.
+    def self.latest_pipeline_per_commit(commits, ref = nil)
       p1 = arel_table
       p2 = arel_table.alias
 
@@ -302,15 +302,14 @@ module Ci
       cond = cond.and(p1[:ref].eq(p2[:ref])) if ref
       join = p1.join(p2, Arel::Nodes::OuterJoin).on(cond)
 
-      relation = select(:sha, :status)
-        .where(sha: commits)
+      relation = where(sha: commits)
         .where(p2[:id].eq(nil))
         .joins(join.join_sources)
 
       relation = relation.where(ref: ref) if ref
 
-      relation.each_with_object({}) do |row, hash|
-        hash[row[:sha]] = row[:status]
+      relation.each_with_object({}) do |pipeline, hash|
+        hash[pipeline.sha] = pipeline
       end
     end
 

app/models/commit.rb

@@ -119,10 +119,22 @@ class Commit
 
     @raw = raw_commit
     @project = project
-    @statuses = {}
     @gpg_commit = Gitlab::Gpg::Commit.new(self) if project
   end
 
+  delegate \
+    :pipelines,
+    :last_pipeline,
+    :latest_pipeline,
+    :latest_pipeline_for_project,
+    :set_latest_pipeline_for_ref,
+    :status,
+    to: :with_pipeline
+
+  def with_pipeline
+    @with_pipeline ||= CommitWithPipeline.new(self)
+  end
+
   def id
     raw.id
   end
@@ -301,30 +313,6 @@ class Commit
     )
   end
 
-  def pipelines
-    project.ci_pipelines.where(sha: sha)
-  end
-
-  def last_pipeline
-    strong_memoize(:last_pipeline) do
-      pipelines.last
-    end
-  end
-
-  def status(ref = nil)
-    return @statuses[ref] if @statuses.key?(ref)
-
-    @statuses[ref] = status_for_project(ref, project)
-  end
-
-  def status_for_project(ref, pipeline_project)
-    pipeline_project.ci_pipelines.latest_status_per_commit(id, ref)[id]
-  end
-
-  def set_status_for_ref(ref, status)
-    @statuses[ref] = status
-  end
-
   def signature
     return @signature if defined?(@signature)
 

app/models/commit_collection.rb

@@ -34,6 +34,20 @@ class CommitCollection
     end
   end
 
+  # Returns the collection with the latest pipeline for every commit pre-set.
+  #
+  # Setting the pipeline for each commit ahead of time removes the need for running
+  # a query for every commit we're displaying.
+  def with_latest_pipeline(ref = nil)
+    pipelines = project.ci_pipelines.latest_pipeline_per_commit(map(&:id), ref)
+
+    each do |commit|
+      commit.set_latest_pipeline_for_ref(ref, pipelines[commit.id])
+    end
+
+    self
+  end
+
   def unenriched
     commits.reject(&:gitaly_commit?)
   end
@@ -65,20 +79,6 @@ class CommitCollection
     self
   end
 
-  # Sets the pipeline status for every commit.
-  #
-  # Setting this status ahead of time removes the need for running a query for
-  # every commit we're displaying.
-  def with_pipeline_status
-    statuses = project.ci_pipelines.latest_status_per_commit(map(&:id), ref)
-
-    each do |commit|
-      commit.set_status_for_ref(ref, statuses[commit.id])
-    end
-
-    self
-  end
-
   def respond_to_missing?(message, inc_private = false)
     commits.respond_to?(message, inc_private)
   end

app/models/commit_with_pipeline.rb

+# frozen_string_literal: true
+
+class CommitWithPipeline < SimpleDelegator
+  include Presentable
+
+  def initialize(commit)
+    @latest_pipelines = {}
+    super(commit)
+  end
+
+  def pipelines
+    project.ci_pipelines.where(sha: sha)
+  end
+
+  def last_pipeline
+    strong_memoize(:last_pipeline) do
+      pipelines.last
+    end
+  end
+
+  def latest_pipeline(ref = nil)
+    @latest_pipelines.fetch(ref) do |ref|
+      @latest_pipelines[ref] = latest_pipeline_for_project(ref, project)
+    end
+  end
+
+  def latest_pipeline_for_project(ref, pipeline_project)
+    pipeline_project.ci_pipelines.latest_pipeline_per_commit(id, ref)[id]
+  end
+
+  def set_latest_pipeline_for_ref(ref, pipeline)
+    @latest_pipelines[ref] = pipeline
+  end
+
+  def status(ref = nil)
+    latest_pipeline(ref)&.status
+  end
+end

app/presenters/commit_presenter.rb

@@ -6,11 +6,15 @@ class CommitPresenter < Gitlab::View::Presenter::Delegated
   presents :commit
 
   def status_for(ref)
-    can?(current_user, :read_commit_status, commit.project) && commit.status(ref)
+    return unless can?(current_user, :read_commit_status, commit.project)
+
+    commit.latest_pipeline(ref)&.detailed_status(current_user)
   end
 
   def any_pipelines?
-    can?(current_user, :read_pipeline, commit.project) && commit.pipelines.any?
+    return false unless can?(current_user, :read_pipeline, commit.project)
+
+    commit.pipelines.any?
   end
 
   def web_url

app/serializers/commit_entity.rb

@@ -35,8 +35,8 @@ class CommitEntity < API::Entities::Commit
     pipeline_project = options[:pipeline_project] || commit.project
     next unless pipeline_ref && pipeline_project
 
-    status = commit.status_for_project(pipeline_ref, pipeline_project)
-    next unless status
+    pipeline = commit.latest_pipeline_for_project(pipeline_ref, pipeline_project)
+    next unless pipeline&.status
 
     pipelines_project_commit_path(pipeline_project, commit.id, ref: pipeline_ref)
   end

app/views/projects/commits/_commit.html.haml

@@ -6,7 +6,8 @@
 - merge_request = local_assigns.fetch(:merge_request, nil)
 - project       = local_assigns.fetch(:project) { merge_request&.project }
 - ref           = local_assigns.fetch(:ref) { merge_request&.source_branch }
-- commit_status = commit.present(current_user: current_user).status_for(ref)
+- commit        = commit.present(current_user: current_user)
+- commit_status = commit.status_for(ref)
 
 - link = commit_path(project, commit, merge_request: merge_request)
 
@@ -48,7 +49,7 @@
         = render partial: 'projects/commit/ajax_signature', locals: { commit: commit }
 
       - if commit_status
-        = render_commit_status(commit, ref: ref)
+        = render_commit_status(commit, commit_status, ref: ref)
 
       .js-commit-pipeline-status{ data: { endpoint: pipelines_project_commit_path(project, commit.id, ref: ref) } }
 

app/views/shared/projects/_project.html.haml

@@ -63,7 +63,7 @@
           - if pipeline_status && can?(current_user, :read_cross_project) && project.pipeline_status.has_status? && can?(current_user, :read_build, project)
             - pipeline_path = pipelines_project_commit_path(project.pipeline_status.project, project.pipeline_status.sha, ref: project.pipeline_status.ref)
             %span.icon-wrapper.pipeline-status
-              = render 'ci/status/icon', status: project.commit.last_pipeline.detailed_status(current_user), tooltip_placement: 'top', path: pipeline_path
+              = render 'ci/status/icon', status: project.last_pipeline.detailed_status(current_user), tooltip_placement: 'top', path: pipeline_path
 
           - if project.archived
             %span.d-flex.icon-wrapper.badge.badge-warning archived

changelogs/unreleased/eb-fix-ci-status-indicator-for-warnings.yml

+---
+title: Show correct CI indicator when build succeeded with warnings.
+merge_request: 17034
+author:
+type: fixed

spec/models/ci/pipeline_spec.rb

@@ -1932,40 +1932,57 @@ describe Ci::Pipeline, :mailer do
     end
   end
 
-  describe '.latest_status_per_commit' do
+  describe '.latest_pipeline_per_commit' do
     let(:project) { create(:project) }
 
-    before do
-      pairs = [
-        %w[success ref1 123],
-        %w[manual master 123],
-        %w[failed ref 456]
-      ]
-
-      pairs.each do |(status, ref, sha)|
-        create(
-          :ci_empty_pipeline,
-          status: status,
-          ref: ref,
-          sha: sha,
-          project: project
-        )
-      end
+    let!(:commit_123_ref_master) do
+      create(
+        :ci_empty_pipeline,
+        status: 'success',
+        ref: 'master',
+        sha: '123',
+        project: project
+      )
+    end
+    let!(:commit_123_ref_develop) do
+      create(
+        :ci_empty_pipeline,
+        status: 'success',
+        ref: 'develop',
+        sha: '123',
+        project: project
+      )
+    end
+    let!(:commit_456_ref_test) do
+      create(
+        :ci_empty_pipeline,
+        status: 'success',
+        ref: 'test',
+        sha: '456',
+        project: project
+      )
     end
 
     context 'without a ref' do
-      it 'returns a Hash containing the latest status per commit for all refs' do
-        expect(described_class.latest_status_per_commit(%w[123 456]))
-          .to eq({ '123' => 'manual', '456' => 'failed' })
+      it 'returns a Hash containing the latest pipeline per commit for all refs' do
+        result = described_class.latest_pipeline_per_commit(%w[123 456])
+
+        expect(result).to match(
+          '123' => commit_123_ref_develop,
+          '456' => commit_456_ref_test
+        )
       end
 
-      it 'only includes the status of the given commit SHAs' do
-        expect(described_class.latest_status_per_commit(%w[123]))
-          .to eq({ '123' => 'manual' })
+      it 'only includes the latest pipeline of the given commit SHAs' do
+        result = described_class.latest_pipeline_per_commit(%w[123])
+
+        expect(result).to match(
+          '123' => commit_123_ref_develop
+        )
       end
 
       context 'when there are two pipelines for a ref and SHA' do
-        it 'returns the status of the latest pipeline' do
+        let!(:commit_123_ref_master_latest) do
           create(
             :ci_empty_pipeline,
             status: 'failed',
@@ -1973,17 +1990,25 @@ describe Ci::Pipeline, :mailer do
             sha: '123',
             project: project
           )
+        end
+
+        it 'returns the latest pipeline' do
+          result = described_class.latest_pipeline_per_commit(%w[123])
 
-          expect(described_class.latest_status_per_commit(%w[123]))
-            .to eq({ '123' => 'failed' })
+          expect(result).to match(
+            '123' => commit_123_ref_master_latest
+          )
         end
       end
     end
 
     context 'with a ref' do
       it 'only includes the pipelines for the given ref' do
-        expect(described_class.latest_status_per_commit(%w[123 456], 'master'))
-          .to eq({ '123' => 'manual' })
+        result = described_class.latest_pipeline_per_commit(%w[123 456], 'master')
+
+        expect(result).to match(
+          '123' => commit_123_ref_master
+        )
       end
     end
   end

spec/models/commit_collection_spec.rb

@@ -51,6 +51,30 @@ describe CommitCollection do
     end
   end
 
+  describe '#with_latest_pipeline' do
+    let!(:pipeline) do
+      create(
+        :ci_empty_pipeline,
+        ref: 'master',
+        sha: commit.id,
+        status: 'success',
+        project: project
+      )
+    end
+    let(:collection) { described_class.new(project, [commit]) }
+
+    it 'sets the latest pipeline for every commit so no additional queries are necessary' do
+      commits = collection.with_latest_pipeline('master')
+
+      recorder = ActiveRecord::QueryRecorder.new do
+        expect(commits.map { |c| c.latest_pipeline('master') })
+          .to eq([pipeline])
+      end
+
+      expect(recorder.count).to be_zero
+    end
+  end
+
   describe 'enrichment methods' do
     let(:gitaly_commit) { commit }
     let(:hash_commit) { Commit.from_hash(gitaly_commit.to_hash, project) }
@@ -128,27 +152,6 @@ describe CommitCollection do
     end
   end
 
-  describe '#with_pipeline_status' do
-    it 'sets the pipeline status for every commit so no additional queries are necessary' do
-      create(
-        :ci_empty_pipeline,
-        ref: 'master',
-        sha: commit.id,
-        status: 'success',
-        project: project
-      )
-
-      collection = described_class.new(project, [commit])
-      collection.with_pipeline_status
-
-      recorder = ActiveRecord::QueryRecorder.new do
-        expect(commit.status).to eq('success')
-      end
-
-      expect(recorder.count).to be_zero
-    end
-  end
-
   describe '#respond_to_missing?' do
     it 'returns true when the underlying Array responds to the message' do
       collection = described_class.new(project, [])

spec/models/commit_spec.rb

@@ -462,78 +462,6 @@ eos
     end
   end
 
-  describe '#last_pipeline' do
-    let!(:first_pipeline) do
-      create(:ci_empty_pipeline,
-        project: project,
-        sha: commit.sha,
-        status: 'success')
-    end
-    let!(:second_pipeline) do
-      create(:ci_empty_pipeline,
-        project: project,
-        sha: commit.sha,
-        status: 'success')
-    end
-
-    it 'returns last pipeline' do
-      expect(commit.last_pipeline).to eq second_pipeline
-    end
-  end
-
-  describe '#status' do
-    context 'without ref argument' do
-      before do
-        %w[success failed created pending].each do |status|
-          create(:ci_empty_pipeline,
-                 project: project,
-                 sha: commit.sha,
-                 status: status)
-        end
-      end
-
-      it 'gives compound status from latest pipelines' do
-        expect(commit.status).to eq(Ci::Pipeline.latest_status)
-        expect(commit.status).to eq('pending')
-      end
-    end
-
-    context 'when a particular ref is specified' do
-      let!(:pipeline_from_master) do
-        create(:ci_empty_pipeline,
-               project: project,
-               sha: commit.sha,
-               ref: 'master',
-               status: 'failed')
-      end
-
-      let!(:pipeline_from_fix) do
-        create(:ci_empty_pipeline,
-               project: project,
-               sha: commit.sha,
-               ref: 'fix',
-               status: 'success')
-      end
-
-      it 'gives pipelines from a particular branch' do
-        expect(commit.status('master')).to eq(pipeline_from_master.status)
-        expect(commit.status('fix')).to eq(pipeline_from_fix.status)
-      end
-
-      it 'gives compound status from latest pipelines if ref is nil' do
-        expect(commit.status(nil)).to eq(pipeline_from_fix.status)
-      end
-    end
-  end
-
-  describe '#set_status_for_ref' do
-    it 'sets the status for a given reference' do
-      commit.set_status_for_ref('master', 'failed')
-
-      expect(commit.status('master')).to eq('failed')
-    end
-  end
-
   describe '#participants' do
     let(:user1) { build(:user) }
     let(:user2) { build(:user) }

spec/models/commit_with_pipeline_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe CommitWithPipeline do
+  let(:project) { create(:project, :public, :repository) }
+  let(:commit)  { described_class.new(project.commit) }
+
+  describe '#last_pipeline' do
+    let!(:first_pipeline) do
+      create(:ci_empty_pipeline,
+        project: project,
+        sha: commit.sha,
+        status: 'success')
+    end
+    let!(:second_pipeline) do
+      create(:ci_empty_pipeline,
+        project: project,
+        sha: commit.sha,
+        status: 'success')
+    end
+
+    it 'returns last pipeline' do
+      expect(commit.last_pipeline).to eq second_pipeline
+    end
+  end
+
+  describe '#latest_pipeline' do
+    let(:pipeline) { double }
+
+    shared_examples_for 'fetching latest pipeline' do |ref|
+      it 'returns the latest pipeline for the project' do
+        expect(commit)
+          .to receive(:latest_pipeline_for_project)
+          .with(ref, project)
+          .and_return(pipeline)
+
+        expect(result).to eq(pipeline)
+      end
+
+      it "returns the memoized pipeline for the key of #{ref}" do
+        commit.set_latest_pipeline_for_ref(ref, pipeline)
+
+        expect(commit)
+          .not_to receive(:latest_pipeline_for_project)
+
+        expect(result).to eq(pipeline)
+      end
+    end
+
+    context 'without ref argument' do
+      let(:result) { commit.latest_pipeline }
+
+      it_behaves_like 'fetching latest pipeline', nil
+    end
+
+    context 'when a particular ref is specified' do
+      let(:result) { commit.latest_pipeline('master') }
+
+      it_behaves_like 'fetching latest pipeline', 'master'
+    end
+  end
+
+  describe '#latest_pipeline_for_project' do
+    let(:project_pipelines) { double }
+    let(:pipeline_project) { double }
+    let(:pipeline) { double }
+    let(:ref) { 'master' }
+    let(:result) { commit.latest_pipeline_for_project(ref, pipeline_project) }
+
+    before do
+      allow(pipeline_project).to receive(:ci_pipelines).and_return(project_pipelines)
+    end
+
+    it 'returns the latest pipeline of the commit for the given ref and project' do
+      expect(project_pipelines)
+        .to receive(:latest_pipeline_per_commit)
+        .with(commit.id, ref)
+        .and_return(commit.id => pipeline)
+
+      expect(result).to eq(pipeline)
+    end
+  end
+
+  describe '#set_latest_pipeline_for_ref' do
+    let(:pipeline) { double }
+
+    it 'sets the latest pipeline for a given reference' do
+      commit.set_latest_pipeline_for_ref('master', pipeline)
+
+      expect(commit.latest_pipeline('master')).to eq(pipeline)
+    end
+  end
+
+  describe "#status" do
+    it 'returns the status of the latest pipeline for the given ref' do
+      expect(commit)
+        .to receive(:latest_pipeline)
+        .with('master')
+        .and_return(double(status: 'success'))
+
+      expect(commit.status('master')).to eq('success')
+    end
+
+    it 'returns nil when latest pipeline is not present for the given ref' do
+      expect(commit)
+        .to receive(:latest_pipeline)
+        .with('master')
+        .and_return(nil)
+
+      expect(commit.status('master')).to eq(nil)
+    end
+
+    it 'returns the status of the latest pipeline when no ref is given' do
+      expect(commit)
+        .to receive(:latest_pipeline)
+        .with(nil)
+        .and_return(double(status: 'success'))
+
+      expect(commit.status).to eq('success')
+    end
+  end
+end

spec/presenters/commit_presenter_spec.rb

@@ -17,15 +17,19 @@ describe CommitPresenter do
       end
 
       it 'returns commit status for ref' do
-        expect(commit).to receive(:status).with('ref').and_return('test')
+        pipeline = double
+        status = double
 
-        expect(subject).to eq('test')
+        expect(commit).to receive(:latest_pipeline).with('ref').and_return(pipeline)
+        expect(pipeline).to receive(:detailed_status).with(user).and_return(status)
+
+        expect(subject).to eq(status)
       end
     end
 
     context 'when user can not read_commit_status' do
-      it 'is false' do
-        is_expected.to eq(false)
+      it 'is nil' do
+        is_expected.to eq(nil)
       end
     end
   end