Merge branch 'mc/bug/keep-latest-artifact-frontend-fix' into 'master'

Consider artifact locking in build_details_entity

See merge request gitlab-org/gitlab!38918

app/controllers/projects/artifacts_controller.rb

@@ -108,7 +108,7 @@ class Projects::ArtifactsController < Projects::ApplicationController
   end
 
   def validate_artifacts!
-    render_404 unless build&.artifacts?
+    render_404 unless build&.available_artifacts?
   end
 
   def build

app/models/ci/build.rb

@@ -647,6 +647,13 @@ module Ci
       !artifacts_expired? && artifacts_file&.exists?
     end
 
+    # This method is similar to #artifacts? but it includes the artifacts
+    # locking mechanics. A new method was created to prevent breaking existing
+    # behavior and avoid introducing N+1s.
+    def available_artifacts?
+      (!artifacts_expired? || pipeline.artifacts_locked?) && job_artifacts_archive&.exists?
+    end
+
     def artifacts_metadata?
       artifacts? && artifacts_metadata&.exists?
     end

app/serializers/build_details_entity.rb

@@ -27,11 +27,11 @@ class BuildDetailsEntity < JobEntity
   end
 
   expose :artifact, if: -> (*) { can?(current_user, :read_build, build) } do
-    expose :download_path, if: -> (*) { build.artifacts? } do |build|
+    expose :download_path, if: -> (*) { build.pipeline.artifacts_locked? || build.artifacts? } do |build|
       download_project_job_artifacts_path(project, build)
     end
 
-    expose :browse_path, if: -> (*) { build.browsable_artifacts? } do |build|
+    expose :browse_path, if: -> (*) { build.pipeline.artifacts_locked? || build.browsable_artifacts? } do |build|
       browse_project_job_artifacts_path(project, build)
     end
 
@@ -46,6 +46,10 @@ class BuildDetailsEntity < JobEntity
     expose :expired, if: -> (*) { build.artifacts_expire_at.present? } do |build|
       build.artifacts_expired?
     end
+
+    expose :locked do |build|
+      build.pipeline.artifacts_locked?
+    end
   end
 
   expose :report_artifacts,

spec/fixtures/api/schemas/job/artifact.json

@@ -5,7 +5,8 @@
     "browse_path": { "type": "string"},
     "keep_path": { "type": "string"},
     "expired": { "type": "boolean" },
-    "expire_at": { "type": "string", "format": "date-time" }
+    "expire_at": { "type": "string", "format": "date-time" },
+    "locked": { "type": "boolean" }
   },
   "additionalProperties": false
 }

spec/models/ci/build_spec.rb

@@ -612,6 +612,62 @@ RSpec.describe Ci::Build do
     end
   end
 
+  describe '#available_artifacts?' do
+    let(:build) { create(:ci_build) }
+
+    subject { build.available_artifacts? }
+
+    context 'when artifacts are not expired' do
+      before do
+        build.artifacts_expire_at = Date.tomorrow
+      end
+
+      context 'when artifacts exist' do
+        before do
+          create(:ci_job_artifact, :archive, job: build)
+        end
+
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when artifacts do not exist' do
+        it { is_expected.to be_falsey }
+      end
+    end
+
+    context 'when artifacts are expired' do
+      before do
+        build.artifacts_expire_at = Date.yesterday
+      end
+
+      context 'when artifacts are not locked' do
+        before do
+          build.pipeline.locked = :unlocked
+        end
+
+        it { is_expected.to be_falsey }
+      end
+
+      context 'when artifacts are locked' do
+        before do
+          build.pipeline.locked = :artifacts_locked
+        end
+
+        context 'when artifacts exist' do
+          before do
+            create(:ci_job_artifact, :archive, job: build)
+          end
+
+          it { is_expected.to be_truthy }
+        end
+
+        context 'when artifacts do not exist' do
+          it { is_expected.to be_falsey }
+        end
+      end
+    end
+  end
+
   describe '#browsable_artifacts?' do
     subject { build.browsable_artifacts? }
 

spec/serializers/build_details_entity_spec.rb

@@ -185,12 +185,38 @@ RSpec.describe BuildDetailsEntity do
       end
     end
 
+    context 'when the build has expired artifacts' do
+      let!(:build) { create(:ci_build, :artifacts, artifacts_expire_at: 7.days.ago) }
+
+      it 'does not expose any artifact actions path' do
+        expect(subject[:artifact].keys).not_to include(:download_path, :browse_path, :keep_path)
+      end
+
+      it 'artifact locked is false' do
+        expect(subject.dig(:artifact, :locked)).to eq(false)
+      end
+
+      context 'when the pipeline is artifacts_locked' do
+        before do
+          build.pipeline.update!(locked: :artifacts_locked)
+        end
+
+        it 'artifact locked is true' do
+          expect(subject.dig(:artifact, :locked)).to eq(true)
+        end
+
+        it 'exposes download and browse artifact actions path' do
+          expect(subject[:artifact].keys).to include(:download_path, :browse_path)
+        end
+      end
+    end
+
     context 'when the build has archive type artifacts' do
       let!(:build) { create(:ci_build, :artifacts, artifacts_expire_at: 7.days.from_now) }
       let!(:report) { create(:ci_job_artifact, :codequality, job: build) }
 
       it 'exposes artifact details' do
-        expect(subject[:artifact].keys).to include(:download_path, :browse_path, :keep_path, :expire_at, :expired)
+        expect(subject[:artifact].keys).to include(:download_path, :browse_path, :keep_path, :expire_at, :expired, :locked)
       end
     end
   end