Merge branch 'dz-add-jobs-finder' into 'master'

Refactor jobs controller index

See merge request gitlab-org/gitlab!20058

app/controllers/admin/jobs_controller.rb

 # frozen_string_literal: true
 
 class Admin::JobsController < Admin::ApplicationController
-  # rubocop: disable CodeReuse/ActiveRecord
   def index
+    # We need all builds for tabs counters
+    @all_builds = JobsFinder.new(current_user: current_user).execute
+
     @scope = params[:scope]
-    @all_builds = Ci::Build
-    @builds = @all_builds.order('id DESC')
-    @builds =
-      case @scope
-      when 'pending'
-        @builds.pending.reverse_order
-      when 'running'
-        @builds.running.reverse_order
-      when 'finished'
-        @builds.finished
-      else
-        @builds
-      end
+    @builds = JobsFinder.new(current_user: current_user, params: params).execute
+    @builds = @builds.eager_load_everything
     @builds = @builds.page(params[:page]).per(30)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def cancel_all
     Ci::Build.running_or_pending.each(&:cancel)

app/controllers/projects/jobs_controller.rb

@@ -17,34 +17,15 @@ class Projects::JobsController < Projects::ApplicationController
 
   layout 'project'
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def index
+    # We need all builds for tabs counters
+    @all_builds = JobsFinder.new(current_user: current_user, project: @project).execute
+
     @scope = params[:scope]
-    @all_builds = project.builds.relevant
-    @builds = @all_builds.order('ci_builds.id DESC')
-    @builds =
-      case @scope
-      when 'pending'
-        @builds.pending.reverse_order
-      when 'running'
-        @builds.running.reverse_order
-      when 'finished'
-        @builds.finished
-      else
-        @builds
-      end
-    @builds = @builds.includes([
-      { pipeline: [:project, :user] },
-      :job_artifacts_archive,
-      :metadata,
-      :trigger_request,
-      :project,
-      :user,
-      :tags
-    ])
+    @builds = JobsFinder.new(current_user: current_user, project: @project, params: params).execute
+    @builds = @builds.eager_load_everything
     @builds = @builds.page(params[:page]).per(30).without_count
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   # rubocop: disable CodeReuse/ActiveRecord
   def show

app/finders/jobs_finder.rb

+# frozen_string_literal: true
+
+class JobsFinder
+  include Gitlab::Allowable
+
+  def initialize(current_user:, project: nil, params: {})
+    @current_user = current_user
+    @project = project
+    @params = params
+  end
+
+  def execute
+    builds = init_collection.order_id_desc
+    filter_by_scope(builds)
+  rescue Gitlab::Access::AccessDeniedError
+    Ci::Build.none
+  end
+
+  private
+
+  attr_reader :current_user, :project, :params
+
+  def init_collection
+    project ? project_builds : all_builds
+  end
+
+  def all_builds
+    raise Gitlab::Access::AccessDeniedError unless current_user&.admin?
+
+    Ci::Build.all
+  end
+
+  def project_builds
+    raise Gitlab::Access::AccessDeniedError unless can?(current_user, :read_build, project)
+
+    project.builds.relevant
+  end
+
+  def filter_by_scope(builds)
+    case params[:scope]
+    when 'pending'
+      builds.pending.reverse_order
+    when 'running'
+      builds.running.reverse_order
+    when 'finished'
+      builds.finished
+    else
+      builds
+    end
+  end
+end

app/models/ci/build.rb

@@ -120,6 +120,20 @@ module Ci
 
     scope :eager_load_job_artifacts, -> { includes(:job_artifacts) }
 
+    scope :eager_load_everything, -> do
+      includes(
+        [
+          { pipeline: [:project, :user] },
+          :job_artifacts_archive,
+          :metadata,
+          :trigger_request,
+          :project,
+          :user,
+          :tags
+        ]
+      )
+    end
+
     scope :with_exposed_artifacts, -> do
       joins(:metadata).merge(Ci::BuildMetadata.with_exposed_artifacts)
         .includes(:metadata, :job_artifacts_metadata)
@@ -161,6 +175,7 @@ module Ci
     end
 
     scope :queued_before, ->(time) { where(arel_table[:queued_at].lt(time)) }
+    scope :order_id_desc, -> { order('ci_builds.id DESC') }
 
     acts_as_taggable
 

spec/finders/jobs_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe JobsFinder, '#execute' do
+  set(:user) { create(:user) }
+  set(:admin) { create(:user, :admin) }
+  set(:project) { create(:project, :private, public_builds: false) }
+  set(:pipeline) { create(:ci_pipeline, project: project) }
+  set(:job_1) { create(:ci_build) }
+  set(:job_2) { create(:ci_build, :running) }
+  set(:job_3) { create(:ci_build, :success, pipeline: pipeline) }
+
+  let(:params) { {} }
+
+  context 'no project' do
+    subject { described_class.new(current_user: admin, params: params).execute }
+
+    it 'returns all jobs' do
+      expect(subject).to match_array([job_1, job_2, job_3])
+    end
+
+    context 'non admin user' do
+      let(:admin) { user }
+
+      it 'returns no jobs' do
+        expect(subject).to be_empty
+      end
+    end
+
+    context 'without user' do
+      let(:admin) { nil }
+
+      it 'returns no jobs' do
+        expect(subject).to be_empty
+      end
+    end
+
+    context 'scope is present' do
+      let(:jobs) { [job_1, job_2, job_3] }
+
+      where(:scope, :index) do
+        [
+          ['pending',  0],
+          ['running',  1],
+          ['finished', 2]
+        ]
+      end
+
+      with_them do
+        let(:params) { { scope: scope } }
+
+        it { expect(subject).to match_array([jobs[index]]) }
+      end
+    end
+  end
+
+  context 'a project is present' do
+    subject { described_class.new(current_user: user, project: project, params: params).execute }
+
+    context 'user has access to the project' do
+      before do
+        project.add_maintainer(user)
+      end
+
+      it 'returns jobs for the specified project' do
+        expect(subject).to match_array([job_3])
+      end
+    end
+
+    context 'user has no access to project builds' do
+      before do
+        project.add_guest(user)
+      end
+
+      it 'returns no jobs' do
+        expect(subject).to be_empty
+      end
+    end
+
+    context 'without user' do
+      let(:user) { nil }
+
+      it 'returns no jobs' do
+        expect(subject).to be_empty
+      end
+    end
+  end
+end