Merge branch '35285-user-interface-bugs-for-schedule-pipelines' into 'master'

Hide pipeline schedule 'take ownership' for current owner

Closes #35285

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

5 files changed, 76 insertions, 1 deletions

diff --git a/app/policies/ci/pipeline_schedule_policy.rb b/app/policies/ci/pipeline_schedule_policy.rb
index abcf536b2f7..dc7a4aed577 100644
--- a/app/policies/ci/pipeline_schedule_policy.rb
+++ b/app/policies/ci/pipeline_schedule_policy.rb
@@ -10,6 +10,10 @@ module Ci
       can?(:developer_access) && pipeline_schedule.owned_by?(@user)
     end
 
+    condition(:non_owner_of_schedule) do
+      !pipeline_schedule.owned_by?(@user)
+    end
+
     rule { can?(:developer_access) }.policy do
       enable :play_pipeline_schedule
     end
@@ -19,6 +23,10 @@ module Ci
       enable :admin_pipeline_schedule
     end
 
+    rule { can?(:master_access) & non_owner_of_schedule }.policy do
+      enable :take_ownership_pipeline_schedule
+    end
+
     rule { protected_ref }.prevent :play_pipeline_schedule
   end
 end
diff --git a/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml b/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml
index 800e234275c..a8692b83b07 100644
--- a/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml
+++ b/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml
@@ -29,9 +29,10 @@
         - if can?(current_user, :play_pipeline_schedule, pipeline_schedule)
           = link_to play_pipeline_schedule_path(pipeline_schedule), method: :post, title: s_('Play'), class: 'btn' do
             = icon('play')
-        - if can?(current_user, :update_pipeline_schedule, pipeline_schedule)
+        - if can?(current_user, :take_ownership_pipeline_schedule, pipeline_schedule)
           = link_to take_ownership_pipeline_schedule_path(pipeline_schedule), method: :post, title: s_('PipelineSchedules|Take ownership'), class: 'btn' do
             = s_('PipelineSchedules|Take ownership')
+        - if can?(current_user, :update_pipeline_schedule, pipeline_schedule)
           = link_to edit_pipeline_schedule_path(pipeline_schedule), title: _('Edit'), class: 'btn' do
             = icon('pencil')
         - if can?(current_user, :admin_pipeline_schedule, pipeline_schedule)
diff --git a/changelogs/unreleased/35285-user-interface-bugs-for-schedule-pipelines.yml b/changelogs/unreleased/35285-user-interface-bugs-for-schedule-pipelines.yml
new file mode 100644
index 00000000000..f3a04469884
--- /dev/null
+++ b/changelogs/unreleased/35285-user-interface-bugs-for-schedule-pipelines.yml
@@ -0,0 +1,5 @@
+---
+title: Hide pipeline schedule take ownership for current owner
+merge_request: 12986
+author:
+type: fixed
diff --git a/spec/policies/ci/pipeline_schedule_policy_spec.rb b/spec/policies/ci/pipeline_schedule_policy_spec.rb
index 1b0e9fac355..c0c3eda4911 100644
--- a/spec/policies/ci/pipeline_schedule_policy_spec.rb
+++ b/spec/policies/ci/pipeline_schedule_policy_spec.rb
@@ -88,5 +88,19 @@ describe Ci::PipelineSchedulePolicy, :models do
         expect(policy).to be_allowed :admin_pipeline_schedule
       end
     end
+
+    describe 'rules for non-owner of schedule' do
+      let(:owner) { create(:user) }
+
+      before do
+        project.add_master(owner)
+        project.add_master(user)
+        pipeline_schedule.update(owner: owner)
+      end
+
+      it 'includes abilities to take ownership' do
+        expect(policy).to be_allowed :take_ownership_pipeline_schedule
+      end
+    end
   end
 end
diff --git a/spec/views/projects/pipeline_schedules/_pipeline_schedule.html.haml_spec.rb b/spec/views/projects/pipeline_schedules/_pipeline_schedule.html.haml_spec.rb
new file mode 100644
index 00000000000..6e7d8db99c4
--- /dev/null
+++ b/spec/views/projects/pipeline_schedules/_pipeline_schedule.html.haml_spec.rb
@@ -0,0 +1,47 @@
+require 'spec_helper'
+
+describe 'projects/pipeline_schedules/_pipeline_schedule' do
+  let(:owner) { create(:user) }
+  let(:master) { create(:user) }
+  let(:project) { create(:project) }
+  let(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly, project: project) }
+
+  before do
+    assign(:project, project)
+
+    allow(view).to receive(:current_user).and_return(user)
+    allow(view).to receive(:pipeline_schedule).and_return(pipeline_schedule)
+
+    allow(view).to receive(:can?).and_return(true)
+  end
+
+  context 'taking ownership of schedule' do
+    context 'when non-owner is signed in' do
+      let(:user) { master }
+
+      before do
+        allow(view).to receive(:can?).with(master, :take_ownership_pipeline_schedule, pipeline_schedule).and_return(true)
+      end
+
+      it 'non-owner can take ownership of pipeline' do
+        render
+
+        expect(rendered).to have_link('Take ownership')
+      end
+    end
+
+    context 'when owner is signed in' do
+      let(:user) { owner }
+
+      before do
+        allow(view).to receive(:can?).with(owner, :take_ownership_pipeline_schedule, pipeline_schedule).and_return(false)
+      end
+
+      it 'owner cannot take ownership of pipeline' do
+        render
+
+        expect(rendered).not_to have_link('Take ownership')
+      end
+    end
+  end
+end