Merge branch 'downloading-expired-artifacts' into 'master'

Don't render artifact download links for builds with expired artifacts

See merge request gitlab-org/gitlab-ce!26753

4 files changed, 51 insertions, 12 deletions

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index b81a3cf8362..0b1f119686a 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -686,7 +686,7 @@ module Ci
       # We purposely cast the builds to an Array here. Because we always use the
       # rows if there are more than 0 this prevents us from having to run two
       # queries: one to get the count and one to get the rows.
-      @latest_builds_with_artifacts ||= builds.latest.with_artifacts_archive.to_a
+      @latest_builds_with_artifacts ||= builds.latest.with_artifacts_not_expired.to_a
     end
 
     def has_test_reports?
diff --git a/changelogs/unreleased/downloading-expired-artifacts.yml b/changelogs/unreleased/downloading-expired-artifacts.yml
new file mode 100644
index 00000000000..2f4b79ca106
--- /dev/null
+++ b/changelogs/unreleased/downloading-expired-artifacts.yml
@@ -0,0 +1,5 @@
+---
+title: stop rendering download links for expired artifacts on the project tags page
+merge_request: 26753
+author: Drew Cimino
+type: fixed
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index f3e78630c1b..2b6b12650af 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -2705,18 +2705,19 @@ describe Ci::Pipeline, :mailer do
   end
 
   describe '#latest_builds_with_artifacts' do
-    let!(:pipeline) { create(:ci_pipeline, :success) }
-
-    let!(:build) do
-      create(:ci_build, :success, :artifacts, pipeline: pipeline)
-    end
+    let!(:fresh_build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
+    let!(:stale_build) { create(:ci_build, :success, :expired, :artifacts, pipeline: pipeline) }
 
     it 'returns an Array' do
       expect(pipeline.latest_builds_with_artifacts).to be_an_instance_of(Array)
     end
 
-    it 'returns the latest builds' do
-      expect(pipeline.latest_builds_with_artifacts).to eq([build])
+    it 'returns the latest builds with non-expired artifacts' do
+      expect(pipeline.latest_builds_with_artifacts).to contain_exactly(fresh_build)
+    end
+
+    it 'does not return builds with expired artifacts' do
+      expect(pipeline.latest_builds_with_artifacts).not_to include(stale_build)
     end
 
     it 'memoizes the returned relation' do
diff --git a/spec/views/projects/tags/index.html.haml_spec.rb b/spec/views/projects/tags/index.html.haml_spec.rb
index cb97d17988c..34355e27544 100644
--- a/spec/views/projects/tags/index.html.haml_spec.rb
+++ b/spec/views/projects/tags/index.html.haml_spec.rb
@@ -1,20 +1,53 @@
 require 'spec_helper'
 
 describe 'projects/tags/index' do
-  let(:project) { create(:project, :repository) }
+  let(:project)  { create(:project, :repository) }
+  let(:tags)     { TagsFinder.new(project.repository, {}).execute }
+  let(:git_tag)  { project.repository.tags.last }
+  let(:release)  { create(:release, project: project, sha: git_tag.target_commit.sha) }
+  let(:pipeline) { create(:ci_pipeline, :success, project: project, ref: git_tag.name, sha: release.sha) }
 
   before do
     assign(:project, project)
     assign(:repository, project.repository)
-    assign(:tags, [])
+    assign(:releases, project.releases)
+    assign(:tags, Kaminari.paginate_array(tags).page(0))
+    assign(:tags_pipelines, { git_tag.name => pipeline })
 
     allow(view).to receive(:current_ref).and_return('master')
-    allow(view).to receive(:can?).and_return(false)
+    allow(view).to receive(:current_user).and_return(project.namespace.owner)
   end
 
   it 'defaults sort dropdown toggle to last updated' do
     render
-
     expect(rendered).to have_button('Last updated')
   end
+
+  context 'when the most recent build for a tag has artifacts' do
+    let!(:build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
+
+    it 'renders the Artifacts section in the download list' do
+      render
+      expect(rendered).to have_selector('li', text: 'Artifacts')
+    end
+
+    it 'renders artifact download links' do
+      render
+      expect(rendered).to have_link(href: latest_succeeded_project_artifacts_path(project, "#{pipeline.ref}/download", job: 'test'))
+    end
+  end
+
+  context 'when the most recent build for a tag has expired artifacts' do
+    let!(:build) { create(:ci_build, :success, :expired, :artifacts, pipeline: pipeline) }
+
+    it 'does not render the Artifacts section in the download list' do
+      render
+      expect(rendered).not_to have_selector('li', text: 'Artifacts')
+    end
+
+    it 'does not render artifact download links' do
+      render
+      expect(rendered).not_to have_link(href: latest_succeeded_project_artifacts_path(project, "#{pipeline.ref}/download", job: 'test'))
+    end
+  end
 end