Clean up implementation

Cleans up the implementation with the suggested changes from the review.

7 files changed, 22 insertions, 19 deletions

diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb
index b1b99145302..7424b521cba 100644
--- a/app/controllers/projects/artifacts_controller.rb
+++ b/app/controllers/projects/artifacts_controller.rb
@@ -13,10 +13,12 @@ class Projects::ArtifactsController < Projects::ApplicationController
   before_action :validate_artifacts!, except: [:index, :download, :destroy]
   before_action :entry, only: [:file]
 
+  MAX_PER_PAGE = 20
+
   def index
     finder = ArtifactsFinder.new(@project, artifacts_params)
-    @artifacts = finder.execute.page(params[:page])
-    @total_size = finder.total_size
+    @artifacts = finder.execute.page(params[:page]).per(MAX_PER_PAGE)
+    @total_size = @artifacts.total_size
   end
 
   def destroy
@@ -26,7 +28,7 @@ class Projects::ArtifactsController < Projects::ApplicationController
                _('Artifact could not be deleted.')
              end
 
-    redirect_to project_artifacts_path(@project), status: :found, notice: notice
+    redirect_to project_artifacts_path(@project), status: :see_other, notice: notice
   end
 
   def download
diff --git a/app/finders/artifacts_finder.rb b/app/finders/artifacts_finder.rb
index 67f7c25e105..2f3ef4e10ca 100644
--- a/app/finders/artifacts_finder.rb
+++ b/app/finders/artifacts_finder.rb
@@ -19,14 +19,10 @@ class ArtifactsFinder
     artifacts.search_by_job_name(@params[:search])
   end
 
-  def total_size
-    Ci::JobArtifact.artifacts_size_for(@project)
-  end
-
   private
 
   def sort_key
-    @params[:sort] || 'created_asc'
+    @params[:sort] || 'created_desc'
   end
 
   def sort(artifacts)
diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb
index 1873e6e143e..ec5288d711c 100644
--- a/app/models/ci/job_artifact.rb
+++ b/app/models/ci/job_artifact.rb
@@ -144,12 +144,16 @@ module Ci
       self.update_column(:file_store, file.object_store)
     end
 
+    def self.total_size
+      self.sum(:size)
+    end
+
     def self.artifacts_size_for(project)
       self.where(project: project).sum(:size)
     end
 
     def self.search_by_job_name(job_name)
-      includes(:job).where(ci_builds: { name: job_name })
+      joins(:job).where(ci_builds: { name: job_name })
     end
 
     def local_store?
diff --git a/app/models/project.rb b/app/models/project.rb
index 4783f9008c3..360584d865d 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -272,7 +272,7 @@ class Project < ApplicationRecord
   has_many :builds, class_name: 'Ci::Build', inverse_of: :project, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :build_trace_section_names, class_name: 'Ci::BuildTraceSectionName'
   has_many :build_trace_chunks, class_name: 'Ci::BuildTraceChunk', through: :builds, source: :trace_chunks
-  has_many :job_artifacts, class_name: 'Ci::JobArtifact', through: :builds
+  has_many :job_artifacts, class_name: 'Ci::JobArtifact'
   has_many :runner_projects, class_name: 'Ci::RunnerProject', inverse_of: :project
   has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
   has_many :variables, class_name: 'Ci::Variable'
diff --git a/spec/controllers/projects/artifacts_controller_spec.rb b/spec/controllers/projects/artifacts_controller_spec.rb
index 9ddd31c1823..f1886232380 100644
--- a/spec/controllers/projects/artifacts_controller_spec.rb
+++ b/spec/controllers/projects/artifacts_controller_spec.rb
@@ -6,7 +6,7 @@ describe Projects::ArtifactsController do
   let(:user) { project.owner }
   set(:project) { create(:project, :repository, :public) }
 
-  let(:pipeline) do
+  set(:pipeline) do
     create(:ci_pipeline,
             project: project,
             sha: project.commit.sha,
@@ -26,18 +26,18 @@ describe Projects::ArtifactsController do
     it 'sets the artifacts variable' do
       subject
 
-      expect(assigns(:artifacts)).to eq(project.job_artifacts)
+      expect(assigns(:artifacts)).to contain_exactly(*project.job_artifacts)
     end
 
     describe 'pagination' do
       before do
-        allow(Kaminari.config).to receive(:default_per_page).and_return(1)
+        stub_const("#{described_class}::MAX_PER_PAGE", 1)
       end
 
       it 'paginates artifacts' do
         subject
 
-        expect(assigns(:artifacts)).to contain_exactly(project.job_artifacts.first)
+        expect(assigns(:artifacts)).to contain_exactly(project.job_artifacts.last)
       end
     end
   end
diff --git a/spec/finders/artifacts_finder_spec.rb b/spec/finders/artifacts_finder_spec.rb
index fb583e639e7..205c007076b 100644
--- a/spec/finders/artifacts_finder_spec.rb
+++ b/spec/finders/artifacts_finder_spec.rb
@@ -30,10 +30,11 @@ describe ArtifactsFinder do
 
     context 'with job_name param' do
       let(:params) { { search: 'unique_name' } }
-      let!(:build) { create(:ci_build, :artifacts, project: project, name: 'unique_name') }
 
       it 'filters the artifacts by job name' do
-        expect(subject).to eq(build.job_artifacts)
+        build = create(:ci_build, :artifacts, project: project, name: 'unique_name')
+
+        expect(subject).to contain_exactly(*build.job_artifacts)
       end
     end
   end
diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb
index 7b2b9bf5089..a175c9490eb 100644
--- a/spec/models/ci/job_artifact_spec.rb
+++ b/spec/models/ci/job_artifact_spec.rb
@@ -96,12 +96,12 @@ describe Ci::JobArtifact do
   end
 
   describe '.search_by_job_name' do
-    let!(:matching_build) { create(:ci_build, :artifacts, name: 'unique_name') }
-    let!(:non_matching_build) { create(:ci_build, :artifacts) }
-
     subject { described_class.search_by_job_name('unique_name') }
 
     it 'returns only artifacts for specified job name' do
+      create(:ci_build, :artifacts)
+      matching_build = create(:ci_build, :artifacts, name: 'unique_name')
+
       expect(subject).to eq(matching_build.job_artifacts)
     end
   end