Merge branch 'fix/keep-artifacts-button-visibility' into 'master'

Do not show artifacts keep button if not allowed

Closes #26357

See merge request !8501

5 files changed, 49 insertions, 9 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 27042798741..48ffe40abc6 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -507,6 +507,10 @@ module Ci
         end
     end
 
+    def has_expiring_artifacts?
+      artifacts_expire_at.present?
+    end
+
     def keep_artifacts!
       self.update(artifacts_expire_at: nil)
     end
diff --git a/app/views/projects/builds/_sidebar.html.haml b/app/views/projects/builds/_sidebar.html.haml
index 0b3adcbe121..37bf085130a 100644
--- a/app/views/projects/builds/_sidebar.html.haml
+++ b/app/views/projects/builds/_sidebar.html.haml
@@ -22,14 +22,14 @@
           %p.build-detail-row
             The artifacts were removed
             #{time_ago_with_tooltip(@build.artifacts_expire_at)}
-        - elsif @build.artifacts_expire_at
+        - elsif @build.has_expiring_artifacts?
           %p.build-detail-row
             The artifacts will be removed in
             %span.js-artifacts-remove= @build.artifacts_expire_at
 
         - if @build.artifacts?
           .btn-group.btn-group-justified{ role: :group }
-            - if @build.artifacts_expire_at
+            - if @build.has_expiring_artifacts? && can?(current_user, :update_build, @build)
               = link_to keep_namespace_project_build_artifacts_path(@project.namespace, @project, @build), class: 'btn btn-sm btn-default', method: :post do
                 Keep
 
diff --git a/changelogs/unreleased/fix-keep-artifacts-button-visibility.yml b/changelogs/unreleased/fix-keep-artifacts-button-visibility.yml
new file mode 100644
index 00000000000..3d8cf1c74a2
--- /dev/null
+++ b/changelogs/unreleased/fix-keep-artifacts-button-visibility.yml
@@ -0,0 +1,4 @@
+---
+title: Hide build artifacts keep button if operation is not allowed
+merge_request: 8501
+author:
diff --git a/spec/features/projects/builds_spec.rb b/spec/features/projects/builds_spec.rb
index 8c4d4320dc5..11d27feab0b 100644
--- a/spec/features/projects/builds_spec.rb
+++ b/spec/features/projects/builds_spec.rb
@@ -3,6 +3,7 @@ require 'tempfile'
 
 feature 'Builds', :feature do
   let(:user) { create(:user) }
+  let(:user_access_level) { :developer }
   let(:project) { create(:project) }
   let(:pipeline) { create(:ci_pipeline, project: project) }
 
@@ -14,7 +15,7 @@ feature 'Builds', :feature do
   end
 
   before do
-    project.team << [user, :developer]
+    project.team << [user, user_access_level]
     login_as(user)
   end
 
@@ -131,7 +132,9 @@ feature 'Builds', :feature do
 
     context 'Artifacts expire date' do
       before do
-        build.update_attributes(artifacts_file: artifacts_file, artifacts_expire_at: expire_at)
+        build.update_attributes(artifacts_file: artifacts_file,
+                                artifacts_expire_at: expire_at)
+
         visit namespace_project_build_path(project.namespace, project, build)
       end
 
@@ -146,12 +149,23 @@ feature 'Builds', :feature do
       context 'when expire date is defined' do
         let(:expire_at) { Time.now + 7.days }
 
-        it 'keeps artifacts when Keep button is clicked' do
-          expect(page).to have_content 'The artifacts will be removed'
-          click_link 'Keep'
+        context 'when user has ability to update build' do
+          it 'keeps artifacts when keep button is clicked' do
+            expect(page).to have_content 'The artifacts will be removed'
 
-          expect(page).not_to have_link 'Keep'
-          expect(page).not_to have_content 'The artifacts will be removed'
+            click_link 'Keep'
+
+            expect(page).to have_no_link 'Keep'
+            expect(page).to have_no_content 'The artifacts will be removed'
+          end
+        end
+
+        context 'when user does not have ability to update build' do
+          let(:user_access_level) { :guest }
+
+          it 'does not have keep button' do
+            expect(page).to have_no_link 'Keep'
+          end
         end
       end
 
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index af0f6a31eda..3309a7fff9f 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1013,6 +1013,24 @@ describe Ci::Build, :models do
     end
   end
 
+  describe '#has_expiring_artifacts?' do
+    context 'when artifacts have expiration date set' do
+      before { build.update(artifacts_expire_at: 1.day.from_now) }
+
+      it 'has expiring artifacts' do
+        expect(build).to have_expiring_artifacts
+      end
+    end
+
+    context 'when artifacts do not have expiration date set' do
+      before { build.update(artifacts_expire_at: nil) }
+
+      it 'does not have expiring artifacts' do
+        expect(build).not_to have_expiring_artifacts
+      end
+    end
+  end
+
   describe '#has_trace_file?' do
     context 'when there is no trace' do
       it { expect(build.has_trace_file?).to be_falsey }