summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorShinya Maeda <shinya@gitlab.com>2017-06-30 02:22:05 +0900
committerShinya Maeda <shinya@gitlab.com>2017-07-05 18:38:34 +0900
commit8a9d08af07a892d2699abcc914a81c9e46eff7be (patch)
tree2f1fb8a2022599876acf00a9a6c3c7e92b0675f8
parentc98c8f9a883776c4c6ffc3cf9c2214541bb8d75b (diff)
downloadgitlab-ce-8a9d08af07a892d2699abcc914a81c9e46eff7be.tar.gz
Fix policy by new guild line
-rw-r--r--app/policies/ci/pipeline_schedule_policy.rb18
-rw-r--r--spec/controllers/projects/pipeline_schedules_controller_spec.rb187
-rw-r--r--spec/features/projects/pipeline_schedules_spec.rb41
3 files changed, 111 insertions, 135 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)
+ 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
- sign_in(user)
+ 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
- it 'loads the pipeline schedule' do
+ def go
get :edit, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
+ end
+ end
- expect(response).to have_http_status(:ok)
- expect(assigns(:schedule)).to eq(pipeline_schedule)
+ 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