Commit 93f1236f authored by Erick Bajao's avatar Erick Bajao Committed by Stan Hu

Don't show keep path for non archive artifacts

Non archive artifacts are not keepable so don't return
the keep_path for them.

Use has_expiring_archive_artifacts in BuildArtifactEntity.

Move has_expiring_artifacts to be private.

Preload job_artifacts_archive when loading bunch of pipelines
to avoid N+1 query.
parent 3f6a0167
...@@ -763,8 +763,8 @@ module Ci ...@@ -763,8 +763,8 @@ module Ci
end end
end end
def has_expiring_artifacts? def has_expiring_archive_artifacts?
artifacts_expire_at.present? && artifacts_expire_at > Time.now has_expiring_artifacts? && job_artifacts_archive.present?
end end
def keep_artifacts! def keep_artifacts!
...@@ -979,6 +979,10 @@ module Ci ...@@ -979,6 +979,10 @@ module Ci
value.with_indifferent_access value.with_indifferent_access
end end
end end
def has_expiring_artifacts?
artifacts_expire_at.present? && artifacts_expire_at > Time.now
end
end end
end end
......
...@@ -15,7 +15,7 @@ class BuildArtifactEntity < Grape::Entity ...@@ -15,7 +15,7 @@ class BuildArtifactEntity < Grape::Entity
fast_download_project_job_artifacts_path(project, job) fast_download_project_job_artifacts_path(project, job)
end end
expose :keep_path, if: -> (*) { job.has_expiring_artifacts? } do |job| expose :keep_path, if: -> (*) { job.has_expiring_archive_artifacts? } do |job|
fast_keep_project_job_artifacts_path(project, job) fast_keep_project_job_artifacts_path(project, job)
end end
......
...@@ -31,7 +31,7 @@ class BuildDetailsEntity < JobEntity ...@@ -31,7 +31,7 @@ class BuildDetailsEntity < JobEntity
browse_project_job_artifacts_path(project, build) browse_project_job_artifacts_path(project, build)
end end
expose :keep_path, if: -> (*) { build.has_expiring_artifacts? && can?(current_user, :update_build, build) } do |build| expose :keep_path, if: -> (*) { build.has_expiring_archive_artifacts? && can?(current_user, :update_build, build) } do |build|
keep_project_job_artifacts_path(project, build) keep_project_job_artifacts_path(project, build)
end end
......
...@@ -58,7 +58,8 @@ class PipelineSerializer < BaseSerializer ...@@ -58,7 +58,8 @@ class PipelineSerializer < BaseSerializer
pending_builds: :project, pending_builds: :project,
project: [:route, { namespace: :route }], project: [:route, { namespace: :route }],
artifacts: { artifacts: {
project: [:route, { namespace: :route }] project: [:route, { namespace: :route }],
job_artifacts_archive: []
} }
}, },
{ triggered_by_pipeline: [:project, :user] }, { triggered_by_pipeline: [:project, :user] },
......
---
title: Remove keep button for non archive artifacts
merge_request: 23762
author:
type: fixed
...@@ -2338,14 +2338,24 @@ describe Ci::Build do ...@@ -2338,14 +2338,24 @@ describe Ci::Build do
end end
end end
describe '#has_expiring_artifacts?' do describe '#has_expiring_archive_artifacts?' do
context 'when artifacts have expiration date set' do context 'when artifacts have expiration date set' do
before do before do
build.update(artifacts_expire_at: 1.day.from_now) build.update(artifacts_expire_at: 1.day.from_now)
end end
it 'has expiring artifacts' do context 'and job artifacts archive record exists' do
expect(build).to have_expiring_artifacts let!(:archive) { create(:ci_job_artifact, :archive, job: build) }
it 'has expiring artifacts' do
expect(build).to have_expiring_archive_artifacts
end
end
context 'and job artifacts archive record does not exist' do
it 'does not have expiring artifacts' do
expect(build).not_to have_expiring_archive_artifacts
end
end end
end end
...@@ -2355,7 +2365,7 @@ describe Ci::Build do ...@@ -2355,7 +2365,7 @@ describe Ci::Build do
end end
it 'does not have expiring artifacts' do it 'does not have expiring artifacts' do
expect(build).not_to have_expiring_artifacts expect(build).not_to have_expiring_archive_artifacts
end end
end end
end end
......
...@@ -4,6 +4,8 @@ require 'spec_helper' ...@@ -4,6 +4,8 @@ require 'spec_helper'
describe BuildArtifactEntity do describe BuildArtifactEntity do
let(:job) { create(:ci_build, name: 'test:job', artifacts_expire_at: 1.hour.from_now) } let(:job) { create(:ci_build, name: 'test:job', artifacts_expire_at: 1.hour.from_now) }
let!(:archive) { create(:ci_job_artifact, :archive, job: job) }
let!(:metadata) { create(:ci_job_artifact, :metadata, job: job) }
let(:entity) do let(:entity) do
described_class.new(job, request: double) described_class.new(job, request: double)
......
...@@ -176,5 +176,27 @@ describe BuildDetailsEntity do ...@@ -176,5 +176,27 @@ describe BuildDetailsEntity do
expect(subject[:reports].first[:file_type]).to eq('codequality') expect(subject[:reports].first[:file_type]).to eq('codequality')
end end
end end
context 'when the build has no archive type artifacts' do
let!(:report) { create(:ci_job_artifact, :codequality, job: build) }
it 'does not expose any artifact actions path' do
expect(subject[:artifact].keys).not_to include(:download_path, :browse_path, :keep_path)
end
end
context 'when the build has archive type artifacts' do
let!(:report) { create(:ci_job_artifact, :codequality, job: build) }
let!(:archive) { create(:ci_job_artifact, :archive, job: build) }
let!(:metadata) { create(:ci_job_artifact, :metadata, job: build) }
before do
build.update(artifacts_expire_at: 7.days.from_now)
end
it 'exposes artifact details' do
expect(subject[:artifact].keys).to include(:download_path, :browse_path, :keep_path, :expire_at, :expired)
end
end
end end
end end
...@@ -159,7 +159,7 @@ describe PipelineSerializer do ...@@ -159,7 +159,7 @@ describe PipelineSerializer do
it 'verifies number of queries', :request_store do it 'verifies number of queries', :request_store do
recorded = ActiveRecord::QueryRecorder.new { subject } recorded = ActiveRecord::QueryRecorder.new { subject }
expected_queries = Gitlab.ee? ? 42 : 39 expected_queries = Gitlab.ee? ? 43 : 40
expect(recorded.count).to be_within(2).of(expected_queries) expect(recorded.count).to be_within(2).of(expected_queries)
expect(recorded.cached_count).to eq(0) expect(recorded.cached_count).to eq(0)
...@@ -180,7 +180,7 @@ describe PipelineSerializer do ...@@ -180,7 +180,7 @@ describe PipelineSerializer do
# pipeline. With the same ref this check is cached but if refs are # pipeline. With the same ref this check is cached but if refs are
# different then there is an extra query per ref # different then there is an extra query per ref
# https://gitlab.com/gitlab-org/gitlab-foss/issues/46368 # https://gitlab.com/gitlab-org/gitlab-foss/issues/46368
expected_queries = Gitlab.ee? ? 45 : 42 expected_queries = Gitlab.ee? ? 46 : 43
expect(recorded.count).to be_within(2).of(expected_queries) expect(recorded.count).to be_within(2).of(expected_queries)
expect(recorded.cached_count).to eq(0) expect(recorded.cached_count).to eq(0)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment