From 8a9d08af07a892d2699abcc914a81c9e46eff7be Mon Sep 17 00:00:00 2001
From: Shinya Maeda <shinya@gitlab.com>
Date: Fri, 30 Jun 2017 02:22:05 +0900
Subject: [PATCH] Fix policy by new guild line

---
 app/policies/ci/pipeline_schedule_policy.rb   |  18 +-
 .../pipeline_schedules_controller_spec.rb     | 189 ++++++++----------
 .../projects/pipeline_schedules_spec.rb       |  41 ++--
 3 files changed, 112 insertions(+), 136 deletions(-)

diff --git a/app/policies/ci/pipeline_schedule_policy.rb b/app/policies/ci/pipeline_schedule_policy.rb
index 0e26b6e688a..db561dafbd3 100644
--- a/app/policies/ci/pipeline_schedule_policy.rb
+++ b/app/policies/ci/pipeline_schedule_policy.rb
@@ -2,22 +2,24 @@ module Ci
   class PipelineSchedulePolicy < PipelinePolicy
     alias_method :pipeline_schedule, :subject
 
-    def rules
-      super
-
-      if owned_by_developer? && owned_by_another?
-        cannot! :update_pipeline_schedule
-      end
+    condition(:protected_action) do
+      owned_by_developer? && owned_by_another?
     end
 
+    rule { protected_action }.prevent :update_pipeline_schedule
+
     private
 
     def owned_by_developer?
-      pipeline_schedule.project.team.developer?(user)
+      return false unless @user
+
+      pipeline_schedule.project.team.developer?(@user)
     end
 
     def owned_by_another?
-      !pipeline_schedule.owned_by?(user)
+      return false unless @user
+
+      !pipeline_schedule.owned_by?(@user)
     end
   end
 end
diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
index 7ac106c0626..82219db45cb 100644
--- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb
+++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
@@ -19,6 +19,14 @@ describe Projects::PipelineSchedulesController do
       expect(response).to render_template(:index)
     end
 
+    it 'avoids N + 1 queries' do
+      control_count = ActiveRecord::QueryRecorder.new { visit_pipelines_schedules }.count
+
+      create_list(:ci_pipeline_schedule, 2, project: project)
+
+      expect { visit_pipelines_schedules }.not_to exceed_query_limit(control_count)
+    end
+
     context 'when the scope is set to active' do
       let(:scope) { 'active' }
 
@@ -158,13 +166,11 @@ describe Projects::PipelineSchedulesController do
       #     expect(assigns(:schedule).errors['variables.key']).not_to be_empty
       #   end
       # end
-
-      def go
-        post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule
-      end
     end
 
     describe 'security' do
+      let(:schedule) { { description: 'aaaaaaaa', cron: '0 4 * * *', cron_timezone: 'UTC', ref: 'master', active: '1' } }
+
       it { expect { go }.to be_allowed_for(:admin) }
       it { expect { go }.to be_allowed_for(:owner).of(project) }
       it { expect { go }.to be_allowed_for(:master).of(project) }
@@ -174,14 +180,10 @@ describe Projects::PipelineSchedulesController do
       it { expect { go }.to be_denied_for(:user) }
       it { expect { go }.to be_denied_for(:external) }
       it { expect { go }.to be_denied_for(:visitor) }
+    end
 
-      def go
-        post :create, namespace_id: project.namespace.to_param,
-                      project_id: project,
-                      schedule: { description: 'aaaaaaaa', cron: '0 4 * * *',
-                                  cron_timezone: 'UTC', ref: 'master',
-                                  active: '1' }
-      end
+    def go
+      post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule
     end
   end
 
@@ -280,8 +282,8 @@ describe Projects::PipelineSchedulesController do
         end
 
         let!(:pipeline_schedule_variable) do
-          create(:ci_pipeline_schedule_variable, key: 'CCC',
-            pipeline_schedule: pipeline_schedule)
+          create(:ci_pipeline_schedule_variable,
+            key: 'CCC', pipeline_schedule: pipeline_schedule)
         end
 
         context 'when params do not include variables' do
@@ -307,7 +309,7 @@ describe Projects::PipelineSchedulesController do
           context 'when adds a new variable' do
             let(:schedule) do
               basic_param.merge({
-                variables_attributes: [ { key: 'AAA', value: 'AAA123' }]
+                variables_attributes: [{ key: 'AAA', value: 'AAA123' }]
               })
             end
 
@@ -321,7 +323,7 @@ describe Projects::PipelineSchedulesController do
           context 'when updates a variable' do
             let(:schedule) do
               basic_param.merge({
-                variables_attributes: [ { id: pipeline_schedule_variable.id, value: 'new_value' } ]
+                variables_attributes: [{ id: pipeline_schedule_variable.id, value: 'new_value' }]
               })
             end
 
@@ -337,7 +339,7 @@ describe Projects::PipelineSchedulesController do
           context 'when deletes a variable' do
             let(:schedule) do
               basic_param.merge({
-                variables_attributes: [ { id: pipeline_schedule_variable.id, _destroy: true } ]
+                variables_attributes: [{ id: pipeline_schedule_variable.id, _destroy: true }]
               })
             end
 
@@ -347,15 +349,21 @@ describe Projects::PipelineSchedulesController do
           end
         end
       end
-
-      def go
-        put :update, namespace_id: project.namespace.to_param,
-                     project_id: project, id: pipeline_schedule,
-                     schedule: schedule
-      end
     end
 
     describe 'security' do
+      let(:schedule) { { description: 'updated_desc' } }
+
+      it { expect { go }.to be_allowed_for(:admin) }
+      it { expect { go }.to be_allowed_for(:owner).of(project) }
+      it { expect { go }.to be_allowed_for(:master).of(project) }
+      # it { expect { go }.to be_allowed_for(:developer).of(project) }
+      it { expect { go }.to be_denied_for(:reporter).of(project) }
+      it { expect { go }.to be_denied_for(:guest).of(project) }
+      it { expect { go }.to be_denied_for(:user) }
+      it { expect { go }.to be_denied_for(:external) }
+      it { expect { go }.to be_denied_for(:visitor) }
+
       context 'when a developer created a pipeline schedule' do
         let(:developer_1) { create(:user) }
         let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project, owner: developer_1) }
@@ -364,17 +372,9 @@ describe Projects::PipelineSchedulesController do
           project.add_developer(developer_1)
         end
 
-        context 'when the developer updates' do
-          it { expect { go }.to be_allowed_for(developer_1) }
-        end
-
-        context 'when another developer updates' do
-          it { expect { go }.to be_denied_for(:developer).of(project) }
-        end
-
-        context 'when a master updates' do
-          it { expect { go }.to be_allowed_for(:master).of(project) }
-        end
+        it { expect { go }.to be_allowed_for(developer_1) }
+        it { expect { go }.to be_denied_for(:developer).of(project) }
+        it { expect { go }.to be_allowed_for(:master).of(project) }
       end
 
       context 'when a master created a pipeline schedule' do
@@ -385,41 +385,69 @@ describe Projects::PipelineSchedulesController do
           project.add_master(master_1)
         end
 
-        context 'when the master updates' do
-          it { expect { go }.to be_allowed_for(master_1) }
-        end
-
-        context 'when other masters updates' do
-          it { expect { go }.to be_allowed_for(:master).of(project) }
-        end
-
-        context 'when a developer updates' do
-          it { expect { go }.to be_denied_for(:developer).of(project) }
-        end
+        it { expect { go }.to be_allowed_for(master_1) }
+        it { expect { go }.to be_allowed_for(:master).of(project) }
+        it { expect { go }.to be_denied_for(:developer).of(project) }
       end
     end
 
     def go
       put :update, namespace_id: project.namespace.to_param,
                    project_id: project, id: pipeline_schedule,
-                   schedule: { description: 'updated_desc' }
+                   schedule: schedule
     end
   end
 
-  describe 'GET edit' do
-    let(:user) { create(:user) }
+  describe 'GET #edit' do
+    describe 'functionality' do
+      let(:user) { create(:user) }
 
-    before do
-      project.add_master(user)
+      before do
+        project.add_master(user)
 
-      sign_in(user)
+        sign_in(user)
+      end
+
+      it 'loads the pipeline schedule' do
+        get :edit, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
+
+        expect(response).to have_http_status(:ok)
+        expect(assigns(:schedule)).to eq(pipeline_schedule)
+      end
     end
 
-    it 'loads the pipeline schedule' do
-      get :edit, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
+    describe 'security' do
+      it { expect { go }.to be_allowed_for(:admin) }
+      it { expect { go }.to be_allowed_for(:owner).of(project) }
+      it { expect { go }.to be_allowed_for(:master).of(project) }
+      # it { expect { go }.to be_allowed_for(:developer).of(project) }
+      it { expect { go }.to be_denied_for(:reporter).of(project) }
+      it { expect { go }.to be_denied_for(:guest).of(project) }
+      it { expect { go }.to be_denied_for(:user) }
+      it { expect { go }.to be_denied_for(:external) }
+      it { expect { go }.to be_denied_for(:visitor) }
+    end
 
-      expect(response).to have_http_status(:ok)
-      expect(assigns(:schedule)).to eq(pipeline_schedule)
+    def go
+      get :edit, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
+    end
+  end
+
+  describe 'GET #take_ownership' do
+    describe 'security' do
+      it { expect { go }.to be_allowed_for(:admin) }
+      it { expect { go }.to be_allowed_for(:owner).of(project) }
+      it { expect { go }.to be_allowed_for(:master).of(project) }
+      # it { expect { go }.to be_allowed_for(:developer).of(project) }
+      it { expect { go }.to be_denied_for(:reporter).of(project) }
+      it { expect { go }.to be_denied_for(:guest).of(project) }
+      it { expect { go }.to be_denied_for(:user) }
+      it { expect { go }.to be_denied_for(:external) }
+      it { expect { go }.to be_denied_for(:visitor) }
+    end
+
+    def go
+      post :take_ownership, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
     end
   end
 
@@ -454,57 +482,4 @@ describe Projects::PipelineSchedulesController do
       end
     end
   end
-
-  describe 'security' do
-    include AccessMatchersForController
-
-    describe 'GET edit' do
-      it { expect { go }.to be_allowed_for(:admin) }
-      it { expect { go }.to be_allowed_for(:owner).of(project) }
-      it { expect { go }.to be_allowed_for(:master).of(project) }
-      it { expect { go }.to be_allowed_for(:developer).of(project) }
-      it { expect { go }.to be_denied_for(:reporter).of(project) }
-      it { expect { go }.to be_denied_for(:guest).of(project) }
-      it { expect { go }.to be_denied_for(:user) }
-      it { expect { go }.to be_denied_for(:external) }
-      it { expect { go }.to be_denied_for(:visitor) }
-
-      def go
-        get :edit, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
-      end
-    end
-
-    describe 'GET take_ownership' do
-      it { expect { go }.to be_allowed_for(:admin) }
-      it { expect { go }.to be_allowed_for(:owner).of(project) }
-      it { expect { go }.to be_allowed_for(:master).of(project) }
-      it { expect { go }.to be_allowed_for(:developer).of(project) }
-      it { expect { go }.to be_denied_for(:reporter).of(project) }
-      it { expect { go }.to be_denied_for(:guest).of(project) }
-      it { expect { go }.to be_denied_for(:user) }
-      it { expect { go }.to be_denied_for(:external) }
-      it { expect { go }.to be_denied_for(:visitor) }
-
-      def go
-        post :take_ownership, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
-      end
-    end
-
-    describe 'PUT update' do
-      it { expect { go }.to be_allowed_for(:admin) }
-      it { expect { go }.to be_allowed_for(:owner).of(project) }
-      it { expect { go }.to be_allowed_for(:master).of(project) }
-      it { expect { go }.to be_allowed_for(:developer).of(project) }
-      it { expect { go }.to be_denied_for(:reporter).of(project) }
-      it { expect { go }.to be_denied_for(:guest).of(project) }
-      it { expect { go }.to be_denied_for(:user) }
-      it { expect { go }.to be_denied_for(:external) }
-      it { expect { go }.to be_denied_for(:visitor) }
-
-      def go
-        put :update, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id,
-                     schedule: { description: 'a' }
-      end
-    end
-  end
 end
diff --git a/spec/features/projects/pipeline_schedules_spec.rb b/spec/features/projects/pipeline_schedules_spec.rb
index 5b88552cacd..f6ad4c26c00 100644
--- a/spec/features/projects/pipeline_schedules_spec.rb
+++ b/spec/features/projects/pipeline_schedules_spec.rb
@@ -19,19 +19,12 @@ feature 'Pipeline Schedules', :feature, js: true do
       visit_pipelines_schedules
     end
 
-    it 'avoids N + 1 queries' do
-      control_count = ActiveRecord::QueryRecorder.new { visit_pipelines_schedules }.count
-
-      create_list(:ci_pipeline_schedule, 2, project: project)
-
-      expect { visit_pipelines_schedules }.not_to exceed_query_limit(control_count)
-    end
-
     describe 'The view' do
       it 'displays the required information description' do
         page.within('.pipeline-schedule-table-row') do
           expect(page).to have_content('pipeline schedule')
-          expect(page).to have_content(pipeline_schedule.real_next_run.strftime('%b %d, %Y'))
+          expect(find(".next-run-cell time")['data-original-title'])
+            .to include(pipeline_schedule.real_next_run.strftime('%b %d, %Y'))
           expect(page).to have_link('master')
           expect(page).to have_link("##{pipeline.id}")
         end
@@ -62,7 +55,7 @@ feature 'Pipeline Schedules', :feature, js: true do
       it 'deletes the pipeline' do
         click_link 'Delete'
 
-        expect(page).not_to have_content('pipeline schedule')
+        expect(page).not_to have_css(".pipeline-schedule-table-row")
       end
     end
 
@@ -150,16 +143,18 @@ feature 'Pipeline Schedules', :feature, js: true do
 
     scenario 'user sees the new variable in edit window' do
       find(".content-list .pipeline-schedule-table-row:nth-child(1) .btn-group a[title='Edit']").click
-      expect(find(".pipeline-variable-list .pipeline-variable-row:nth-child(1) .pipeline-variable-key-input").value).to eq('AAA')
-      expect(find(".pipeline-variable-list .pipeline-variable-row:nth-child(1) .pipeline-variable-value-input").value).to eq('AAA123')
-      expect(find(".pipeline-variable-list .pipeline-variable-row:nth-child(2) .pipeline-variable-key-input").value).to eq('BBB')
-      expect(find(".pipeline-variable-list .pipeline-variable-row:nth-child(2) .pipeline-variable-value-input").value).to eq('BBB123')
+      page.within('.pipeline-variable-list') do
+        expect(find(".pipeline-variable-row:nth-child(1) .pipeline-variable-key-input").value).to eq('AAA')
+        expect(find(".pipeline-variable-row:nth-child(1) .pipeline-variable-value-input").value).to eq('AAA123')
+        expect(find(".pipeline-variable-row:nth-child(2) .pipeline-variable-key-input").value).to eq('BBB')
+        expect(find(".pipeline-variable-row:nth-child(2) .pipeline-variable-value-input").value).to eq('BBB123')
+      end
     end
   end
 
   context 'when user edits a variable of a pipeline schedule' do
     background do
-      create(:ci_pipeline_schedule, owner: user).tap do |pipeline_schedule|
+      create(:ci_pipeline_schedule, project: project, owner: user).tap do |pipeline_schedule|
         create(:ci_pipeline_schedule_variable, key: 'AAA', value: 'AAA123', pipeline_schedule: pipeline_schedule)
       end
       visit_pipelines_schedules
@@ -171,26 +166,30 @@ feature 'Pipeline Schedules', :feature, js: true do
 
     scenario 'user sees the updated variable in edit window' do
       find(".content-list .pipeline-schedule-table-row:nth-child(1) .btn-group a[title='Edit']").click
-      expect(find(".pipeline-variable-list .pipeline-variable-row:nth-child(1) .pipeline-variable-key-input").value).to eq('foo')
-      expect(find(".pipeline-variable-list .pipeline-variable-row:nth-child(1) .pipeline-variable-value-input").value).to eq('bar')
+      page.within('.pipeline-variable-list') do
+        expect(find(".pipeline-variable-row:nth-child(1) .pipeline-variable-key-input").value).to eq('foo')
+        expect(find(".pipeline-variable-row:nth-child(1) .pipeline-variable-value-input").value).to eq('bar')
+      end
     end
   end
 
   context 'when user removes a variable of a pipeline schedule' do
     background do
-      create(:ci_pipeline_schedule, owner: user).tap do |pipeline_schedule|
+      create(:ci_pipeline_schedule, project: project, owner: user).tap do |pipeline_schedule|
         create(:ci_pipeline_schedule_variable, key: 'AAA', value: 'AAA123', pipeline_schedule: pipeline_schedule)
       end
       visit_pipelines_schedules
       find(".content-list .pipeline-schedule-table-row:nth-child(1) .btn-group a[title='Edit']").click
-      first('.pipeline-variable-list .pipeline-variable-row-remove-button').click
+      find('.pipeline-variable-list .pipeline-variable-row-remove-button').click
       click_button 'Save pipeline schedule'
     end
 
     scenario 'user does not see the removed variable in edit window' do
       find(".content-list .pipeline-schedule-table-row:nth-child(1) .btn-group a[title='Edit']").click
-      expect(find(".pipeline-variable-list .pipeline-variable-row:nth-child(1) .pipeline-variable-key-input").value).to eq('')
-      expect(find(".pipeline-variable-list .pipeline-variable-row:nth-child(1) .pipeline-variable-value-input").value).to eq('')
+      page.within('.pipeline-variable-list') do
+        expect(find(".pipeline-variable-row:nth-child(1) .pipeline-variable-key-input").value).to eq('')
+        expect(find(".pipeline-variable-row:nth-child(1) .pipeline-variable-value-input").value).to eq('')
+      end
     end
   end