Merge branch 'mc/feature/artifacts-page-backend' into 'master'

Artifacts Page Backend

See merge request gitlab-org/gitlab!16630

app/controllers/projects/artifacts_controller.rb

@@ -8,10 +8,37 @@ class Projects::ArtifactsController < Projects::ApplicationController
   layout 'project'
   before_action :authorize_read_build!
   before_action :authorize_update_build!, only: [:keep]
+  before_action :authorize_destroy_artifacts!, only: [:destroy]
   before_action :extract_ref_name_and_path
-  before_action :validate_artifacts!, except: [:download]
+  before_action :validate_artifacts!, except: [:index, :download, :destroy]
   before_action :entry, only: [:file]
 
+  MAX_PER_PAGE = 20
+
+  def index
+    # Loading artifacts is very expensive in projects with a lot of artifacts.
+    # This feature flag prevents a DOS attack vector.
+    # It should be removed only after resolving the underlying performance
+    # issues: https://gitlab.com/gitlab-org/gitlab/issues/32281
+    return head :no_content unless Feature.enabled?(:artifacts_management_page, @project)
+
+    finder = ArtifactsFinder.new(@project, artifacts_params)
+    all_artifacts = finder.execute
+
+    @artifacts = all_artifacts.page(params[:page]).per(MAX_PER_PAGE)
+    @total_size = all_artifacts.total_size
+  end
+
+  def destroy
+    notice = if artifact.destroy
+               _('Artifact was successfully deleted.')
+             else
+               _('Artifact could not be deleted.')
+             end
+
+    redirect_to project_artifacts_path(@project), status: :see_other, notice: notice
+  end
+
   def download
     return render_404 unless artifacts_file
 
@@ -74,6 +101,10 @@ class Projects::ArtifactsController < Projects::ApplicationController
     @ref_name, @path = extract_ref(params[:ref_name_and_path])
   end
 
+  def artifacts_params
+    params.permit(:sort)
+  end
+
   def validate_artifacts!
     render_404 unless build&.artifacts?
   end
@@ -85,6 +116,11 @@ class Projects::ArtifactsController < Projects::ApplicationController
     end
   end
 
+  def artifact
+    @artifact ||=
+      project.job_artifacts.find(params[:id])
+  end
+
   def build_from_id
     project.builds.find_by_id(params[:job_id]) if params[:job_id]
   end

app/finders/artifacts_finder.rb

+# frozen_string_literal: true
+
+class ArtifactsFinder
+  def initialize(project, params = {})
+    @project = project
+    @params = params
+  end
+
+  def execute
+    artifacts = @project.job_artifacts
+
+    sort(artifacts)
+  end
+
+  private
+
+  def sort_key
+    @params[:sort] || 'created_desc'
+  end
+
+  def sort(artifacts)
+    artifacts.order_by(sort_key)
+  end
+end

app/helpers/sorting_helper.rb

@@ -28,7 +28,9 @@ module SortingHelper
       sort_value_priority          => sort_title_priority,
       sort_value_upvotes           => sort_title_upvotes,
       sort_value_contacted_date    => sort_title_contacted_date,
-      sort_value_relative_position => sort_title_relative_position
+      sort_value_relative_position => sort_title_relative_position,
+      sort_value_size              => sort_title_size,
+      sort_value_expire_date       => sort_title_expire_date
     }
   end
 
@@ -406,6 +408,14 @@ module SortingHelper
     s_('SortOptions|Manual')
   end
 
+  def sort_title_size
+    s_('SortOptions|Size')
+  end
+
+  def sort_title_expire_date
+    s_('SortOptions|Expired date')
+  end
+
   # Values.
   def sort_value_access_level_asc
     'access_level_asc'
@@ -558,4 +568,12 @@ module SortingHelper
   def sort_value_relative_position
     'relative_position'
   end
+
+  def sort_value_size
+    'size_desc'
+  end
+
+  def sort_value_expire_date
+    'expired_asc'
+  end
 end

app/models/ci/job_artifact.rb

@@ -5,6 +5,7 @@ module Ci
     include AfterCommitQueue
     include ObjectStorage::BackgroundMove
     include UpdateProjectStatistics
+    include Sortable
     extend Gitlab::Ci::Model
 
     NotSupportedAdapterError = Class.new(StandardError)
@@ -143,6 +144,10 @@ 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

app/models/project.rb

@@ -273,6 +273,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'
   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'

config/routes/project.rb

@@ -37,6 +37,8 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       scope '-' do
         get 'archive/*id', constraints: { format: Gitlab::PathRegex.archive_formats_regex, id: /.+?/ }, to: 'repositories#archive', as: 'archive'
 
+        resources :artifacts, only: [:index, :destroy]
+
         resources :jobs, only: [:index, :show], constraints: { id: /\d+/ } do
           collection do
             resources :artifacts, only: [] do

locale/gitlab.pot

@@ -1860,6 +1860,12 @@ msgstr ""
 msgid "Artifact ID"
 msgstr ""
 
+msgid "Artifact could not be deleted."
+msgstr ""
+
+msgid "Artifact was successfully deleted."
+msgstr ""
+
 msgid "Artifacts"
 msgstr ""
 
@@ -14483,6 +14489,9 @@ msgstr ""
 msgid "SortOptions|Due soon"
 msgstr ""
 
+msgid "SortOptions|Expired date"
+msgstr ""
+
 msgid "SortOptions|Label priority"
 msgstr ""
 
@@ -14573,6 +14582,9 @@ msgstr ""
 msgid "SortOptions|Recently starred"
 msgstr ""
 
+msgid "SortOptions|Size"
+msgstr ""
+
 msgid "SortOptions|Sort direction"
 msgstr ""
 

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,
@@ -14,12 +14,119 @@ describe Projects::ArtifactsController do
             status: 'success')
   end
 
-  let(:job) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
+  let!(:job) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
 
   before do
     sign_in(user)
   end
 
+  describe 'GET index' do
+    subject { get :index, params: { namespace_id: project.namespace, project_id: project } }
+
+    context 'when feature flag is on' do
+      before do
+        stub_feature_flags(artifacts_management_page: true)
+      end
+
+      it 'sets the artifacts variable' do
+        subject
+
+        expect(assigns(:artifacts)).to contain_exactly(*project.job_artifacts)
+      end
+
+      it 'sets the total size variable' do
+        subject
+
+        expect(assigns(:total_size)).to eq(project.job_artifacts.total_size)
+      end
+
+      describe 'pagination' do
+        before do
+          stub_const("#{described_class}::MAX_PER_PAGE", 1)
+        end
+
+        it 'paginates artifacts' do
+          subject
+
+          expect(assigns(:artifacts)).to contain_exactly(project.job_artifacts.last)
+        end
+      end
+    end
+
+    context 'when feature flag is off' do
+      before do
+        stub_feature_flags(artifacts_management_page: false)
+      end
+
+      it 'renders no content' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:no_content)
+      end
+
+      it 'does not set the artifacts variable' do
+        subject
+
+        expect(assigns(:artifacts)).to eq(nil)
+      end
+
+      it 'does not set the total size variable' do
+        subject
+
+        expect(assigns(:total_size)).to eq(nil)
+      end
+    end
+  end
+
+  describe 'DELETE destroy' do
+    let!(:artifact) { job.job_artifacts.erasable.first }
+
+    subject { delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: artifact } }
+
+    it 'deletes the artifact' do
+      expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+      expect(artifact).not_to exist
+    end
+
+    it 'redirects to artifacts index page' do
+      subject
+
+      expect(response).to redirect_to(project_artifacts_path(project))
+    end
+
+    it 'sets the notice' do
+      subject
+
+      expect(flash[:notice]).to eq('Artifact was successfully deleted.')
+    end
+
+    context 'when artifact deletion fails' do
+      before do
+        allow_any_instance_of(Ci::JobArtifact).to receive(:destroy).and_return(false)
+      end
+
+      it 'redirects to artifacts index page' do
+        subject
+
+        expect(response).to redirect_to(project_artifacts_path(project))
+      end
+
+      it 'sets the notice' do
+        subject
+
+        expect(flash[:notice]).to eq('Artifact could not be deleted.')
+      end
+    end
+
+    context 'when user is not authorized' do
+      let(:user) { create(:user) }
+
+      it 'does not delete the artifact' do
+        expect { subject }.not_to change { Ci::JobArtifact.count }
+      end
+    end
+  end
+
   describe 'GET download' do
     def download_artifact(extra_params = {})
       params = { namespace_id: project.namespace, project_id: project, job_id: job }.merge(extra_params)

spec/finders/artifacts_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ArtifactsFinder do
+  let(:project) { create(:project) }
+
+  describe '#execute' do
+    before do
+      create(:ci_build, :artifacts, project: project)
+    end
+
+    subject { described_class.new(project, params).execute }
+
+    context 'with empty params' do
+      let(:params) { {} }
+
+      it 'returns all artifacts belonging to the project' do
+        expect(subject).to contain_exactly(*project.job_artifacts)
+      end
+    end
+
+    context 'with sort param' do
+      let(:params) { { sort: 'size_desc' } }
+
+      it 'sorts the artifacts' do
+        expect(subject).to eq(project.job_artifacts.order_by('size_desc'))
+      end
+    end
+  end
+end

spec/lib/gitlab/import_export/all_models.yml

@@ -349,6 +349,7 @@ project:
 - members_and_requesters
 - build_trace_section_names
 - build_trace_chunks
+- job_artifacts
 - root_of_fork_network
 - fork_network_member
 - fork_network