Add simple PoC of using build view decoratorsfeature/use-decorator-in-build-view

5 files changed, 98 insertions, 27 deletions

diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb
index fbe391fc58c..77b2489fe87 100644
--- a/app/controllers/projects/builds_controller.rb
+++ b/app/controllers/projects/builds_controller.rb
@@ -28,8 +28,8 @@ class Projects::BuildsController < Projects::ApplicationController
   end
 
   def show
-    @builds = @project.pipelines.find_by_sha(@build.sha).builds.order('id DESC')
-    @builds = @builds.where("id not in (?)", @build.id)
+    @builds_pipeline = PipelineDecorator
+      .new(@project.pipelines.find_by_sha(@build.sha))
     @pipeline = @build.pipeline
 
     respond_to do |format|
@@ -94,7 +94,7 @@ class Projects::BuildsController < Projects::ApplicationController
   private
 
   def build
-    @build ||= project.builds.find_by!(id: params[:id])
+    @build ||= BuildDecorator.new(project.builds.find_by!(id: params[:id]), current_user)
   end
 
   def build_path(build)
diff --git a/app/decorators/build_decorator.rb b/app/decorators/build_decorator.rb
new file mode 100644
index 00000000000..1135bbaffe3
--- /dev/null
+++ b/app/decorators/build_decorator.rb
@@ -0,0 +1,50 @@
+class BuildDecorator < SimpleDelegator
+  def initialize(build, user)
+    super(build)
+
+    @build, @user = build, user
+  end
+
+  def erased_by_user?
+    # Build can be erased through API, therefore it does not have
+    # `erase_by` user assigned in that case.
+    erased? && erased_by
+  end
+
+  def erased_by_name
+    erased_by.name if erased_by
+  end
+
+  def coverage_visible?
+    !!coverage
+  end
+
+  def artifacts_visible?
+    Ability.allowed?(@user, :read_build, project) &&
+      (artifacts? || artifacts_expired?)
+  end
+
+  def retry_visible?
+    Ability.allowed?(@user, :update_build, @build) && retryable?
+  end
+
+  def erase_visible?
+    Ability.allowed?(@user, :update_build, project) && erasable?
+  end
+
+  def details_block_first?
+    !(coverage_visible? || artifacts_visible?)
+  end
+
+  def link_to_runner_administration_visible?
+    runner && @user && @user.admin
+  end
+
+  def trigger_request_short_token
+    trigger_request.trigger.short_token
+  end
+
+  def self.ancestors
+    super + [Ci::Build]
+  end
+end
diff --git a/app/decorators/pipeline_decorator.rb b/app/decorators/pipeline_decorator.rb
new file mode 100644
index 00000000000..17ed2eb5866
--- /dev/null
+++ b/app/decorators/pipeline_decorator.rb
@@ -0,0 +1,20 @@
+class PipelineDecorator < SimpleDelegator
+  # It makes clear that this implementation belongs more to
+  # Ci::Pipeline than to decorator, but we only do use ordered
+  # build in view. It makes sense to refine current implementation
+  # to accomadate to this discrepancy and DRY implementation.
+  #
+  def each_ordered_build
+    HasStatus::ORDERED_STATUSES.each do |build_status|
+      builds_for_status(build_status).each do |build|
+        yield build
+      end
+    end
+  end
+
+  def builds_for_status(build_status)
+    builds.order('id DESC').to_a.select do |build|
+      build.status == build_status
+    end
+  end
+end
diff --git a/app/views/projects/builds/_sidebar.html.haml b/app/views/projects/builds/_sidebar.html.haml
index b1053028279..cbe47203398 100644
--- a/app/views/projects/builds/_sidebar.html.haml
+++ b/app/views/projects/builds/_sidebar.html.haml
@@ -1,12 +1,10 @@
-- builds = @build.pipeline.builds.to_a
-
 %aside.right-sidebar.right-sidebar-expanded.build-sidebar.js-build-sidebar
   .block.build-sidebar-header.visible-xs-block.visible-sm-block.append-bottom-default
     Build
     %strong ##{@build.id}
     %a.gutter-toggle.pull-right.js-sidebar-build-toggle{ href: "#" }
       = icon('angle-double-right')
-  - if @build.coverage
+  - if @build.coverage_visible?
     .block.coverage
       .title
         Test coverage
@@ -14,7 +12,7 @@
         #{@build.coverage}%
 
   .blocks-container
-    - if can?(current_user, :read_build, @project) && (@build.artifacts? || @build.artifacts_expired?)
+    - if @build.artifacts_visible?
       .block{ class: ("block-first" if !@build.coverage) }
         .title
           Build artifacts
@@ -40,10 +38,10 @@
               = link_to browse_namespace_project_build_artifacts_path(@project.namespace, @project, @build), class: 'btn btn-sm btn-default' do
                 Browse
 
-    .block{ class: ("block-first" if !@build.coverage && !(can?(current_user, :read_build, @project) && (@build.artifacts? || @build.artifacts_expired?))) }
+    .block{ class: ("block-first" if @build.details_block_first?) }
       .title
         Build details
-        - if can?(current_user, :update_build, @build) && @build.retryable?
+        - if @build.retry_visible?
           = link_to "Retry", retry_namespace_project_build_path(@project.namespace, @project, @build), class: 'pull-right retry-link', method: :post
       - if @build.merge_request
         %p.build-detail-row
@@ -63,7 +61,7 @@
           #{time_ago_with_tooltip(@build.erased_at)}
       %p.build-detail-row
         %span.build-light-text Runner:
-        - if @build.runner && current_user && current_user.admin
+        - if @build.link_to_runner_administration_visible?
           = link_to "##{@build.runner.id}", admin_runner_path(@build.runner.id)
         - elsif @build.runner
           \##{@build.runner.id}
@@ -72,7 +70,7 @@
           = link_to 'Raw', raw_namespace_project_build_path(@project.namespace, @project, @build), class: 'btn btn-sm btn-default'
         - if @build.active?
           = link_to "Cancel", cancel_namespace_project_build_path(@project.namespace, @project, @build), class: 'btn btn-sm btn-default', method: :post
-        - if can?(current_user, :update_build, @project) && @build.erasable?
+        - if @build.erase_visible?
           = link_to erase_namespace_project_build_path(@project.namespace, @project, @build),
                     class: "btn btn-sm btn-default", method: :post,
                     data: { confirm: "Are you sure you want to erase this build?" } do
@@ -85,7 +83,7 @@
 
         %p
           %span.build-light-text Token:
-          #{@build.trigger_request.trigger.short_token}
+          #{@build.trigger_request_short_token}
 
         - if @build.trigger_request.variables
           %p
@@ -123,16 +121,17 @@
               %a.stage-item= stage
 
   .builds-container
-    - HasStatus::ORDERED_STATUSES.each do |build_status|
-      - builds.select{|build| build.status == build_status}.each do |build|
-        .build-job{class: sidebar_build_class(build, @build), data: {stage: build.stage}}
-          = link_to namespace_project_build_path(@project.namespace, @project, build) do
-            = icon('arrow-right')
-            = ci_icon_for_status(build.status)
-            %span
-              - if build.name
-                = build.name
-              - else
-                = build.id
-            - if build.retried?
-              %i.fa.fa-refresh.has-tooltip{data: { container: 'body', placement: 'bottom' }, title: 'Build was retried'}
+    - @builds_pipeline.each_ordered_build do |build|
+      - next if build == @build
+
+      .build-job{class: sidebar_build_class(build, @build), data: {stage: build.stage}}
+        = link_to namespace_project_build_path(@project.namespace, @project, build) do
+          = icon('arrow-right')
+          = ci_icon_for_status(build.status)
+          %span
+            - if build.name
+              = build.name
+            - else
+              = build.id
+          - if build.retried?
+            %i.fa.fa-refresh.has-tooltip{data: { container: 'body', placement: 'bottom' }, title: 'Build was retried'}
diff --git a/app/views/projects/builds/show.html.haml b/app/views/projects/builds/show.html.haml
index b5e8b0bf6eb..a135b54fc71 100644
--- a/app/views/projects/builds/show.html.haml
+++ b/app/views/projects/builds/show.html.haml
@@ -33,8 +33,10 @@
           %button.btn.btn-success.btn-sm#autoscroll-button{:type => "button", :data => {:state => 'disabled'}} enable autoscroll
       - if @build.erased?
         .erased.alert.alert-warning
-          - erased_by = "by #{link_to @build.erased_by.name, user_path(@build.erased_by)}" if @build.erased_by
-          Build has been erased #{erased_by.html_safe} #{time_ago_with_tooltip(@build.erased_at)}
+          - if @build.erased_by_user?
+            Build has been erased by #{link_to(@build.erased_by_name, user_path(@build.erased_by))} #{time_ago_with_tooltip(@build.erased_at)}
+          - else
+            Build has been erased #{time_ago_with_tooltip(@build.erased_at)}
       - else
         #js-build-scroll.scroll-controls
           = link_to '#build-trace', class: 'btn' do