Incorporate review

5 files changed, 57 insertions, 7 deletions

diff --git a/app/models/ci/group.rb b/app/models/ci/group.rb
index d7a499f3368..9d5c55b86c5 100644
--- a/app/models/ci/group.rb
+++ b/app/models/ci/group.rb
@@ -13,14 +13,26 @@ module Ci
       @statuses = statuses
     end
 
+    def status
+      @status ||= commit_statuses.status
+    end
+
     def detailed_status(current_user)
-      Gitlab::Ci::Status::Group::Factory
-        .new(CommitStatus.where(id: statuses.map(&:id)), current_user)
-        .fabricate!
+      if size == 1
+        statuses[0].detailed_status(current_user)
+      else
+        Gitlab::Ci::Status::Group::Factory.new(self, current_user).fabricate!
+      end
     end
 
     def size
       statuses.size
     end
+
+    private
+
+    def commit_statuses
+      @commit_statuses ||= CommitStatus.where(id: statuses.map(&:id))
+    end
   end
 end
diff --git a/app/serializers/base_serializer.rb b/app/serializers/base_serializer.rb
index f262b7b7eb0..311ee9c96be 100644
--- a/app/serializers/base_serializer.rb
+++ b/app/serializers/base_serializer.rb
@@ -1,4 +1,4 @@
-class BaseSerializer  
+class BaseSerializer
   def initialize(parameters = {})
     @request = EntityRequest.new(parameters)
   end
diff --git a/app/serializers/job_group_entity.rb b/app/serializers/job_group_entity.rb
index fb407cf7edd..6e8102b8b58 100644
--- a/app/serializers/job_group_entity.rb
+++ b/app/serializers/job_group_entity.rb
@@ -4,7 +4,7 @@ class JobGroupEntity < Grape::Entity
   expose :name
   expose :size
   expose :detailed_status, as: :status, with: StatusEntity
-  expose :statuses, as: :jobs, if: -> (group, _) { group.size > 1 }, with: JobEntity
+  expose :statuses, as: :jobs, with: JobEntity
 
   private
 
diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb
index d53c4b16914..94b1a6eb6a9 100644
--- a/spec/controllers/projects/pipelines_controller_spec.rb
+++ b/spec/controllers/projects/pipelines_controller_spec.rb
@@ -41,13 +41,27 @@ describe Projects::PipelinesController do
     let!(:pipeline) { create(:ci_pipeline_with_one_job, project: project) }
 
     it 'returns the pipeline' do
-      get :show, namespace_id: project.namespace, project_id: project, id: pipeline, format: :json
+      get_pipeline_json
 
       expect(response).to have_http_status(:ok)
       expect(json_response).not_to be_an(Array)
       expect(json_response['id']).to be(pipeline.id)
       expect(json_response['details']).to have_key 'stages'
     end
+
+    context 'when the pipeline has multiple jobs' do
+      it 'does not perform N + 1 queries' do
+        control_count = ActiveRecord::QueryRecorder.new { get_pipeline_json }.count
+
+        create(:ci_build, pipeline: pipeline)
+
+        expect { get_pipeline_json }.not_to exceed_query_limit(control_count)
+      end
+    end
+
+    def get_pipeline_json
+      get :show, namespace_id: project.namespace, project_id: project, id: pipeline, format: :json
+    end
   end
 
   describe 'GET stages.json' do
diff --git a/spec/models/ci/group_spec.rb b/spec/models/ci/group_spec.rb
index ffa3e97ec3e..7635995770d 100644
--- a/spec/models/ci/group_spec.rb
+++ b/spec/models/ci/group_spec.rb
@@ -1,7 +1,8 @@
 require 'spec_helper'
 
 describe Ci::Group, models: true do
-  subject { described_class.new('test', name: 'rspec', statuses: []) }
+  subject { described_class.new('test', name: 'rspec', statuses: statuses) }
+  let(:statuses) { [] }
 
   describe 'expectations' do
     it { is_expected.to include_module(StaticModel) }
@@ -9,6 +10,7 @@ describe Ci::Group, models: true do
     it { is_expected.to respond_to(:stage) }
     it { is_expected.to respond_to(:name) }
     it { is_expected.to respond_to(:statuses) }
+    it { is_expected.to respond_to(:status) }
   end
 
   describe '#size' do
@@ -16,4 +18,26 @@ describe Ci::Group, models: true do
       expect(subject.size).to eq(0)
     end
   end
+
+  describe '#detailed_status' do
+    let(:job) { build(:ci_build, :success) }
+    let(:statuses) { [job] }
+
+    context 'when there is only one item in the group' do
+      it 'calls the status from the object itself' do
+        expect(job).to receive(:detailed_status)
+
+        subject.detailed_status(nil)
+      end
+    end
+
+    context 'when there are more than 1 commit statuses' do
+      let(:job1) { build(:ci_build) }
+      let(:statuses) { [job, job1] }
+
+      it 'fabricates a new Ci::Status object' do
+        expect(subject.detailed_status(nil)).to be_a(Gitlab::Ci::Status::Created)
+      end
+    end
+  end
 end