diff options
-rw-r--r-- | app/policies/ci/pipeline_schedule_policy.rb | 18 | ||||
-rw-r--r-- | spec/controllers/projects/pipeline_schedules_controller_spec.rb | 187 | ||||
-rw-r--r-- | spec/features/projects/pipeline_schedules_spec.rb | 41 |
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 |