Create PipelineDetailsEntity

Now we have a PipelineEntity which is a bit smaller, mostly in bytes
needing to send to the frontend. PipelineDetailsEntity is the default
for the PipelineSerializer, limiting the changes needed.

This commit also incorporates the review.

app/models/ci/build.rb

@@ -204,6 +204,8 @@ module Ci
     end
 
     def merge_request
+      return @merge_request if defined?(@merge_request)
+      
       @merge_request ||=
         begin
           merge_requests = MergeRequest.includes(:merge_request_diff)

app/serializers/build_details_entity.rb

@@ -6,29 +6,25 @@ class BuildDetailsEntity < BuildEntity
   expose :runner, using: RunnerEntity
   expose :pipeline, using: PipelineEntity
 
-  expose :merge_request_path do |build|
-    merge_request = build.merge_request
-    project = build.project
-
-    if merge_request.nil? || !can?(request.current_user, :read_merge_request, project)
-      nil
-    else
-      namespace_project_merge_request_path(project.namespace, project, merge_request)
-    end
+  expose :merge_request_path, if: -> (*) { can?(current_user, :read_merge_request, project) } do |build|
+    namespace_project_merge_request_path(project.namespace, project, build.merge_request)
   end
 
-  expose :new_issue_path do |build|
-    project = build.project
-
-    unless build.failed? && can?(request.current_user, :create_issue, project)
-      nil
-    else
-      new_namespace_project_issue_path(project.namespace, project)
-    end
+  expose :new_issue_path, if: -> (*) { can?(request.current_user, :create_issue, project) } do |build|
+    new_namespace_project_issue_path(project.namespace, project)
   end
 
   expose :raw_path do |build|
-    project = build.project
     raw_namespace_project_build_path(project.namespace, project, build)
   end
+
+  private
+
+  def current_user
+    request.current_user
+  end
+
+  def project
+    build.project
+  end
 end

app/serializers/build_serializer.rb

@@ -4,7 +4,5 @@ class BuildSerializer < BaseSerializer
   def represent_status(resource, opts = {}, entity_class = nil)
     data = represent(resource, { only: [:status] })
     data.fetch(:status, {})
-
-    represent(resource, opts, entity_class)
   end
 end

app/serializers/pipeline_details_entity.rb

+class PipelineDetailsEntity < PipelineEntity
+  expose :yaml_errors, if: -> (pipeline, _) { pipeline.has_yaml_errors? }
+  
+  expose :details do
+    expose :detailed_status, as: :status, with: StatusEntity
+    expose :duration
+    expose :finished_at
+    expose :stages, using: StageEntity
+    expose :artifacts, using: BuildArtifactEntity
+    expose :manual_actions, using: BuildActionEntity
+  end
+
+  expose :flags do
+    expose :latest?, as: :latest
+    expose :triggered?, as: :triggered
+    expose :stuck?, as: :stuck
+    expose :has_yaml_errors?, as: :yaml_errors
+    expose :can_retry?, as: :retryable
+    expose :can_cancel?, as: :cancelable
+  end
+end

app/serializers/pipeline_entity.rb

@@ -6,6 +6,8 @@ class PipelineEntity < Grape::Entity
   expose :active?, as: :active
   expose :coverage
 
+  expose :created_at, :updated_at
+
   expose :path do |pipeline|
     namespace_project_pipeline_path(
       pipeline.project.namespace,
@@ -13,24 +15,6 @@ class PipelineEntity < Grape::Entity
       pipeline)
   end
 
-  expose :details do
-    expose :detailed_status, as: :status, with: StatusEntity
-    expose :duration
-    expose :finished_at
-    expose :stages, using: StageEntity
-    expose :artifacts, using: BuildArtifactEntity
-    expose :manual_actions, using: BuildActionEntity
-  end
-
-  expose :flags do
-    expose :latest?, as: :latest
-    expose :triggered?, as: :triggered
-    expose :stuck?, as: :stuck
-    expose :has_yaml_errors?, as: :yaml_errors
-    expose :can_retry?, as: :retryable
-    expose :can_cancel?, as: :cancelable
-  end
-
   expose :ref do
     expose :name do |pipeline|
       pipeline.ref
@@ -47,7 +31,6 @@ class PipelineEntity < Grape::Entity
   end
 
   expose :commit, using: CommitEntity
-  expose :yaml_errors, if: -> (pipeline, _) { pipeline.has_yaml_errors? }
 
   expose :retry_path, if: -> (*) { can_retry? }  do |pipeline|
     retry_namespace_project_pipeline_path(pipeline.project.namespace,
@@ -61,8 +44,6 @@ class PipelineEntity < Grape::Entity
                                            pipeline.id)
   end
 
-  expose :created_at, :updated_at
-
   private
 
   alias_method :pipeline, :object

app/serializers/pipeline_serializer.rb

 class PipelineSerializer < BaseSerializer
   InvalidResourceError = Class.new(StandardError)
 
-  entity PipelineEntity
+  entity PipelineDetailsEntity
 
   def with_pagination(request, response)
     tap { @paginator = Gitlab::Serializer::Pagination.new(request, response) }

app/serializers/runner_entity.rb

 class RunnerEntity < Grape::Entity
-  expose :id, :name, :description
+  include RequestAwareEntity
+
+  expose :id, :description
+
+  expose :edit_runner_path,
+    if: -> (*) { can?(request.current_user, :admin_build, project)  } do |runner|
+    edit_namespace_project_runner_path(project.namespace, project, runner)
+  end
+
+  private
+
+  def project
+    request.project
+  end
 end

spec/serializers/build_details_entity_spec.rb

@@ -7,7 +7,6 @@ describe BuildDetailsEntity do
 
   describe '#as_json' do
     let(:project) { create(:project, :repository) }
-    let(:user) { create(:user) }
     let!(:build) { create(:ci_build, :failed, project: project) }
     let(:request) { double('request') }
     let(:entity) { described_class.new(build, request: request, current_user: user, project: project) }
@@ -15,12 +14,17 @@ describe BuildDetailsEntity do
 
     before do
       allow(request).to receive(:current_user).and_return(user)
-
-      project.add_master(user)
     end
 
     context 'when the user has access to issues and merge requests' do
-      let!(:merge_request) { create(:merge_request, source_project: project) }
+      let(:user) { create(:admin) }
+      let!(:merge_request) do
+        create(:merge_request, source_project: project, source_branch: build.ref)
+      end
+
+      before do
+        allow(build).to receive(:merge_request).and_return(merge_request)
+      end
 
       it 'contains the needed key value pairs' do
         expect(subject).to include(:coverage, :erased_at, :duration)
@@ -30,6 +34,8 @@ describe BuildDetailsEntity do
     end
 
     context 'when the user can only read the build' do
+      let(:user) { create(:user) }
+
       it "won't display the paths to issues and merge requests" do
         expect(subject['new_issue_path']).to be_nil
         expect(subject['merge_request_path']).to be_nil

spec/serializers/pipeline_details_entity_spec.rb

+require 'spec_helper'
+
+describe PipelineDetailsEntity do
+  set(:user) { create(:user) }
+  let(:request) { double('request') }
+
+  it 'inherrits from PipelineEntity' do
+    expect(described_class).to be < PipelineEntity
+  end
+
+  before do
+    allow(request).to receive(:current_user).and_return(user)
+  end
+
+  let(:entity) do
+    described_class.represent(pipeline, request: request)
+  end
+
+  describe '#as_json' do
+    subject { entity.as_json }
+
+    context 'when pipeline is empty' do
+      let(:pipeline) { create(:ci_empty_pipeline) }
+
+      it 'contains details' do
+        expect(subject).to include :details
+        expect(subject[:details])
+          .to include :duration, :finished_at
+        expect(subject[:details])
+          .to include :stages, :artifacts, :manual_actions
+        expect(subject[:details][:status]).to include :icon, :favicon, :text, :label
+      end
+
+      it 'contains flags' do
+        expect(subject).to include :flags
+        expect(subject[:flags])
+          .to include :latest, :triggered, :stuck,
+                      :yaml_errors, :retryable, :cancelable
+      end
+    end
+
+    context 'when pipeline is retryable' do
+      let(:project) { create(:empty_project) }
+
+      let(:pipeline) do
+        create(:ci_pipeline, status: :success, project: project)
+      end
+
+      before do
+        create(:ci_build, :failed, pipeline: pipeline)
+      end
+
+      context 'user has ability to retry pipeline' do
+        before { project.team << [user, :developer] }
+
+        it 'retryable flag is true' do
+          expect(subject[:flags][:retryable]).to eq true
+        end
+      end
+
+      context 'user does not have ability to retry pipeline' do
+        it 'retryable flag is false' do
+          expect(subject[:flags][:retryable]).to eq false
+        end
+      end
+    end
+
+    context 'when pipeline is cancelable' do
+      let(:project) { create(:empty_project) }
+
+      let(:pipeline) do
+        create(:ci_pipeline, status: :running, project: project)
+      end
+
+      before do
+        create(:ci_build, :pending, pipeline: pipeline)
+      end
+
+      context 'user has ability to cancel pipeline' do
+        before { project.add_developer(user) }
+
+        it 'cancelable flag is true' do
+          expect(subject[:flags][:cancelable]).to eq true
+        end
+      end
+
+      context 'user does not have ability to cancel pipeline' do
+        it 'cancelable flag is false' do
+          expect(subject[:flags][:cancelable]).to eq false
+        end
+      end
+    end
+
+    context 'when pipeline has YAML errors' do
+      let(:pipeline) do
+        create(:ci_pipeline, config: { rspec: { invalid: :value } })
+      end
+
+      it 'contains information about error' do
+        expect(subject[:yaml_errors]).to be_present
+      end
+
+      it 'contains flag that indicates there are errors' do
+        expect(subject[:flags][:yaml_errors]).to be true
+      end
+    end
+
+    context 'when pipeline does not have YAML errors' do
+      let(:pipeline) { create(:ci_empty_pipeline) }
+
+      it 'does not contain field that normally holds an error' do
+        expect(subject).not_to have_key(:yaml_errors)
+      end
+
+      it 'contains flag that indicates there are no errors' do
+        expect(subject[:flags][:yaml_errors]).to be false
+      end
+    end
+  end
+end

spec/serializers/pipeline_entity_spec.rb

 require 'spec_helper'
 
 describe PipelineEntity do
-  let(:user) { create(:user) }
+  set(:user) { create(:user) }
   let(:request) { double('request') }
 
   before do
@@ -23,22 +23,6 @@ describe PipelineEntity do
         expect(subject).to include :ref, :commit
         expect(subject).to include :updated_at, :created_at
       end
-
-      it 'contains details' do
-        expect(subject).to include :details
-        expect(subject[:details])
-          .to include :duration, :finished_at
-        expect(subject[:details])
-          .to include :stages, :artifacts, :manual_actions
-        expect(subject[:details][:status]).to include :icon, :favicon, :text, :label
-      end
-
-      it 'contains flags' do
-        expect(subject).to include :flags
-        expect(subject[:flags])
-          .to include :latest, :triggered, :stuck,
-                      :yaml_errors, :retryable, :cancelable
-      end
     end
 
     context 'when pipeline is retryable' do
@@ -55,20 +39,12 @@ describe PipelineEntity do
       context 'user has ability to retry pipeline' do
         before { project.team << [user, :developer] }
 
-        it 'retryable flag is true' do
-          expect(subject[:flags][:retryable]).to eq true
-        end
-
         it 'contains retry path' do
           expect(subject[:retry_path]).to be_present
         end
       end
 
       context 'user does not have ability to retry pipeline' do
-        it 'retryable flag is false' do
-          expect(subject[:flags][:retryable]).to eq false
-        end
-
         it 'does not contain retry path' do
           expect(subject).not_to have_key(:retry_path)
         end
@@ -87,11 +63,7 @@ describe PipelineEntity do
       end
 
       context 'user has ability to cancel pipeline' do
-        before { project.team << [user, :developer] }
-
-        it 'cancelable flag is true' do
-          expect(subject[:flags][:cancelable]).to eq true
-        end
+        before { project.add_developer(user) }
 
         it 'contains cancel path' do
           expect(subject[:cancel_path]).to be_present
@@ -99,42 +71,12 @@ describe PipelineEntity do
       end
 
       context 'user does not have ability to cancel pipeline' do
-        it 'cancelable flag is false' do
-          expect(subject[:flags][:cancelable]).to eq false
-        end
-
         it 'does not contain cancel path' do
           expect(subject).not_to have_key(:cancel_path)
         end
       end
     end
 
-    context 'when pipeline has YAML errors' do
-      let(:pipeline) do
-        create(:ci_pipeline, config: { rspec: { invalid: :value } })
-      end
-
-      it 'contains flag that indicates there are errors' do
-        expect(subject[:flags][:yaml_errors]).to be true
-      end
-
-      it 'contains information about error' do
-        expect(subject[:yaml_errors]).to be_present
-      end
-    end
-
-    context 'when pipeline does not have YAML errors' do
-      let(:pipeline) { create(:ci_empty_pipeline) }
-
-      it 'contains flag that indicates there are no errors' do
-        expect(subject[:flags][:yaml_errors]).to be false
-      end
-
-      it 'does not contain field that normally holds an error' do
-        expect(subject).not_to have_key(:yaml_errors)
-      end
-    end
-
     context 'when pipeline ref is empty' do
       let(:pipeline) { create(:ci_empty_pipeline) }
 

spec/serializers/runner_entity_spec.rb

 require 'spec_helper'
 
 describe RunnerEntity do
-  let(:runner) { build(:ci_runner) }
-  let(:entity) { described_class.represent(runner) }
+  let(:runner) { create(:ci_runner) }
+  let(:entity) { described_class.new(runner, request: request, current_user: user) }
+  let(:request) { double('request') }
+  let(:project) { create(:empty_project) }
+  let(:user) { create(:admin) }
+
+  before do
+    allow(request).to receive(:current_user).and_return(user)
+    allow(request).to receive(:project).and_return(project)
+  end
 
   describe '#as_json' do
     subject { entity.as_json }
 
     it 'contains required fields' do
-      expect(subject).to include(:id, :name, :description)
+      expect(subject).to include(:id, :description)
+      expect(subject).to include(:edit_runner_path)
     end
   end
 end