Merge branch 'mc/feature/show-keep-button-for-locked-artifacts' into 'master'

Show keep button for locked artifacts

See merge request gitlab-org/gitlab!40962

app/models/ci/build.rb

@@ -647,6 +647,10 @@ module Ci
       !artifacts_expired? && artifacts_file&.exists?
     end
 
+    def locked_artifacts?
+      pipeline.artifacts_locked? && 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.

app/serializers/build_details_entity.rb

@@ -27,15 +27,15 @@ class BuildDetailsEntity < JobEntity
   end
 
   expose :artifact, if: -> (*) { can?(current_user, :read_build, build) } do
-    expose :download_path, if: -> (*) { build.pipeline.artifacts_locked? || build.artifacts? } do |build|
+    expose :download_path, if: -> (*) { build.locked_artifacts? || build.artifacts? } do |build|
       download_project_job_artifacts_path(project, build)
     end
 
-    expose :browse_path, if: -> (*) { build.pipeline.artifacts_locked? || build.browsable_artifacts? } do |build|
+    expose :browse_path, if: -> (*) { build.locked_artifacts? || build.browsable_artifacts? } do |build|
       browse_project_job_artifacts_path(project, build)
     end
 
-    expose :keep_path, if: -> (*) { build.has_expiring_archive_artifacts? && can?(current_user, :update_build, build) } do |build|
+    expose :keep_path, if: -> (*) { (build.locked_artifacts? || build.has_expiring_archive_artifacts?) && can?(current_user, :update_build, build) } do |build|
       keep_project_job_artifacts_path(project, build)
     end
 

changelogs/unreleased/mc-feature-show-keep-button-for-locked-artifacts.yml

+---
+title: Show keep button for locked artifacts.
+merge_request: 40962
+author:
+type: changed

spec/controllers/projects/jobs_controller_spec.rb

@@ -201,33 +201,61 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state do
       end
 
       context 'when job has artifacts' do
-        before do
-          get_show_json
-        end
-
         context 'with not expiry date' do
           let(:job) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
 
           it 'exposes needed information' do
+            get_show_json
+
             expect(response).to have_gitlab_http_status(:ok)
             expect(response).to match_response_schema('job/job_details')
             expect(json_response['artifact']['download_path']).to match(%r{artifacts/download})
             expect(json_response['artifact']['browse_path']).to match(%r{artifacts/browse})
+            expect(json_response['artifact']).not_to have_key('keep_path')
             expect(json_response['artifact']).not_to have_key('expired')
             expect(json_response['artifact']).not_to have_key('expired_at')
           end
         end
 
-        context 'with expiry date' do
+        context 'with expired artifacts' do
           let(:job) { create(:ci_build, :success, :artifacts, :expired, pipeline: pipeline) }
 
-          it 'exposes needed information' do
-            expect(response).to have_gitlab_http_status(:ok)
-            expect(response).to match_response_schema('job/job_details')
-            expect(json_response['artifact']).not_to have_key('download_path')
-            expect(json_response['artifact']).not_to have_key('browse_path')
-            expect(json_response['artifact']['expired']).to eq(true)
-            expect(json_response['artifact']['expire_at']).not_to be_empty
+          context 'when artifacts are unlocked' do
+            before do
+              job.pipeline.unlocked!
+            end
+
+            it 'exposes needed information' do
+              get_show_json
+
+              expect(response).to have_gitlab_http_status(:ok)
+              expect(response).to match_response_schema('job/job_details')
+              expect(json_response['artifact']).not_to have_key('download_path')
+              expect(json_response['artifact']).not_to have_key('browse_path')
+              expect(json_response['artifact']).not_to have_key('keep_path')
+              expect(json_response['artifact']['expired']).to eq(true)
+              expect(json_response['artifact']['expire_at']).not_to be_empty
+              expect(json_response['artifact']['locked']).to eq(false)
+            end
+          end
+
+          context 'when artifacts are locked' do
+            before do
+              job.pipeline.artifacts_locked!
+            end
+
+            it 'exposes needed information' do
+              get_show_json
+
+              expect(response).to have_gitlab_http_status(:ok)
+              expect(response).to match_response_schema('job/job_details')
+              expect(json_response['artifact']).to have_key('download_path')
+              expect(json_response['artifact']).to have_key('browse_path')
+              expect(json_response['artifact']).to have_key('keep_path')
+              expect(json_response['artifact']['expired']).to eq(true)
+              expect(json_response['artifact']['expire_at']).not_to be_empty
+              expect(json_response['artifact']['locked']).to eq(true)
+            end
           end
         end
       end

spec/features/projects/jobs_spec.rb

@@ -373,13 +373,29 @@ RSpec.describe 'Jobs', :clean_gitlab_redis_shared_state do
         let(:expire_at) { Time.now + 7.days }
 
         context 'when user has ability to update job' do
-          it 'keeps artifacts when keep button is clicked' do
-            expect(page).to have_content 'The artifacts will be removed in'
+          context 'when artifacts are unlocked' do
+            before do
+              job.pipeline.unlocked!
+            end
 
-            click_link 'Keep'
+            it 'keeps artifacts when keep button is clicked' do
+              expect(page).to have_content 'The artifacts will be removed in'
 
-            expect(page).to have_no_link 'Keep'
-            expect(page).to have_no_content 'The artifacts will be removed in'
+              click_link 'Keep'
+
+              expect(page).to have_no_link 'Keep'
+              expect(page).to have_no_content 'The artifacts will be removed in'
+            end
+          end
+
+          context 'when artifacts are locked' do
+            before do
+              job.pipeline.artifacts_locked!
+            end
+
+            it 'shows the keep button' do
+              expect(page).to have_link 'Keep'
+            end
           end
         end
 
@@ -395,9 +411,26 @@ RSpec.describe 'Jobs', :clean_gitlab_redis_shared_state do
       context 'when artifacts expired' do
         let(:expire_at) { Time.now - 7.days }
 
-        it 'does not have the Keep button' do
-          expect(page).to have_content 'The artifacts were removed'
-          expect(page).not_to have_link 'Keep'
+        context 'when artifacts are unlocked' do
+          before do
+            job.pipeline.unlocked!
+          end
+
+          it 'does not have the Keep button' do
+            expect(page).to have_content 'The artifacts were removed'
+            expect(page).not_to have_link 'Keep'
+          end
+        end
+
+        context 'when artifacts are locked' do
+          before do
+            job.pipeline.artifacts_locked!
+          end
+
+          it 'has the Keep button' do
+            expect(page).not_to have_content 'The artifacts were removed'
+            expect(page).to have_link 'Keep'
+          end
         end
       end
     end

spec/models/ci/build_spec.rb

@@ -612,6 +612,46 @@ RSpec.describe Ci::Build do
     end
   end
 
+  describe '#locked_artifacts?' do
+    subject(:locked_artifacts) { build.locked_artifacts? }
+
+    context 'when pipeline is artifacts_locked' do
+      before do
+        build.pipeline.artifacts_locked!
+      end
+
+      context 'artifacts archive does not exist' do
+        let(:build) { create(:ci_build) }
+
+        it { is_expected.to be_falsy }
+      end
+
+      context 'artifacts archive exists' do
+        let(:build) { create(:ci_build, :artifacts) }
+
+        it { is_expected.to be_truthy }
+      end
+    end
+
+    context 'when pipeline is unlocked' do
+      before do
+        build.pipeline.unlocked!
+      end
+
+      context 'artifacts archive does not exist' do
+        let(:build) { create(:ci_build) }
+
+        it { is_expected.to be_falsy }
+      end
+
+      context 'artifacts archive exists' do
+        let(:build) { create(:ci_build, :artifacts) }
+
+        it { is_expected.to be_falsy }
+      end
+    end
+  end
+
   describe '#available_artifacts?' do
     let(:build) { create(:ci_build) }
 

spec/serializers/build_details_entity_spec.rb

@@ -188,25 +188,31 @@ RSpec.describe BuildDetailsEntity do
     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
+      context 'when pipeline is unlocked' do
+        before do
+          build.pipeline.unlocked!
+        end
+
+        it 'artifact locked is false' do
+          expect(subject.dig(:artifact, :locked)).to eq(false)
+        end
 
-      it 'artifact locked is false' do
-        expect(subject.dig(:artifact, :locked)).to eq(false)
+        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 pipeline is artifacts_locked' do
         before do
-          build.pipeline.update!(locked: :artifacts_locked)
+          build.pipeline.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)
+        it 'exposes download, browse and keep artifact actions path' do
+          expect(subject[:artifact].keys).to include(:download_path, :browse_path, :keep_path)
         end
       end
     end

spec/services/ci/destroy_expired_job_artifacts_service_spec.rb

@@ -11,6 +11,10 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
     let(:service) { described_class.new }
     let!(:artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
 
+    before do
+      artifact.job.pipeline.unlocked!
+    end
+
     context 'when artifact is expired' do
       context 'when artifact is not locked' do
         before do
@@ -88,6 +92,8 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
       before do
         stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 1)
         stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1)
+
+        second_artifact.job.pipeline.unlocked!
       end
 
       let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
@@ -102,7 +108,9 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
     end
 
     context 'when there are no artifacts' do
-      let!(:artifact) { }
+      before do
+        artifact.destroy!
+      end
 
       it 'does not raise error' do
         expect { subject }.not_to raise_error
@@ -112,6 +120,8 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
     context 'when there are artifacts more than batch sizes' do
       before do
         stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1)
+
+        second_artifact.job.pipeline.unlocked!
       end
 
       let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
@@ -126,6 +136,10 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
         let!(:pipeline_artifact_1) { create(:ci_pipeline_artifact, expire_at: 1.week.ago) }
         let!(:pipeline_artifact_2) { create(:ci_pipeline_artifact, expire_at: 1.week.ago) }
 
+        before do
+          [pipeline_artifact_1, pipeline_artifact_2].each { |pipeline_artifact| pipeline_artifact.pipeline.unlocked! }
+        end
+
         it 'destroys pipeline artifacts' do
           expect { subject }.to change { Ci::PipelineArtifact.count }.by(-2)
         end
@@ -135,10 +149,26 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
         let!(:pipeline_artifact_1) { create(:ci_pipeline_artifact, expire_at: 2.days) }
         let!(:pipeline_artifact_2) { create(:ci_pipeline_artifact, expire_at: 2.days) }
 
-        it 'do not destroy pipeline artifacts' do
+        before do
+          [pipeline_artifact_1, pipeline_artifact_2].each { |pipeline_artifact| pipeline_artifact.pipeline.unlocked! }
+        end
+
+        it 'does not destroy pipeline artifacts' do
           expect { subject }.not_to change { Ci::PipelineArtifact.count }
         end
       end
     end
+
+    context 'when some artifacts are locked' do
+      before do
+        pipeline = create(:ci_pipeline, locked: :artifacts_locked)
+        job = create(:ci_build, pipeline: pipeline)
+        create(:ci_job_artifact, expire_at: 1.day.ago, job: job)
+      end
+
+      it 'destroys only unlocked artifacts' do
+        expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+      end
+    end
   end
 end